RE: [PATCH 03/24] rtw89: add core and trx files

-----Original Message-----
From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
Sent: Saturday, July 10, 2021 1:32 AM
To: Pkshih
Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
Subject: Re: [PATCH 03/24] rtw89: add core and trx files

On Fri, Jun 18, 2021 at 02:46:04PM +0800, Ping-Ke Shih wrote:

quoted

Implement main flows that contains register/unregister mac80211 hw with
hardware capability, power on/off sequence, STA state actions, and
TX/RX path.

The chip info is read from efuse while probing PCI, and then it can be
used to induce supported channel, band, bitrate, ht/vht/he capability,
and etc. Then, we register hardware with these capabilities.

When network interface is up, driver does power-on sequence to enable MAC,
BB and RF function blocks. Oppositely, do power-off sequence when
interface is going to down.

To maintain STA state, five callbacks are implemented -- add, assoc,
disassoc, disconnect and remove. In which state, driver tells firmware STA
info via H2C.

TX flow:
When a SKB is going to be transmitted, we must know its type first. If
the type is mgmt or fwcmd made by driver, SKB is queued into corresponding
DMA channel and PCI ring. The other type is data frame that is more
complex, because it needs to establish BA session to have better throughput
with AMPDU and AMSDU.
In order to have better PCI DMA efficiency, we don't kick off DMA every
SKB. With wake TX queue, kick off DMA after a bunch of SKBs are written.
To achieve this, we have two HCI ops -- tx_write and tx_kick_off.

BA establishment work:
For data frames, we start to establish BA session if the STA is associated
with APMDU capability and the TID session isn't established, and then the
BA work is used to ask mac80211 to start AMPDU actions. Driver implements
AMPDU action callbacks to know the session is established, so that we can
set AGG_EN bit in TX descriptor to enable AMPDU.

RX flow:
When a RX SKB is delivered from PCI, rtw89_core_rx() process it depneds on
its type -- WIFI, C2H or PPDU. If type is C2H, it's queued into a C2H
queue, and wake a work to handle the C2H packet. If type is WIFI, it's a
normal RX packet. When mgmt or data frame is received, it is queued
into pending RX SKB queue to wait for corresponding PPDU packet (another
RX packet with PPDU type) to fill its rx_status, like RSSI. And, then
indicate this packet to mac80211. When control frame is received, indicate
it to mac80211 immediately.

Track work:
Use track work to monitor PHY status to know the changes of environment,
and then update RA status or do RFK accordingly.

Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/core.c | 2359 +++++++++++++++
 drivers/net/wireless/realtek/rtw89/core.h | 3336 +++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/txrx.h |  393 +++
 drivers/net/wireless/realtek/rtw89/util.c |   37 +
 drivers/net/wireless/realtek/rtw89/util.h |   31 +
 5 files changed, 6156 insertions(+)
 create mode 100644 drivers/net/wireless/realtek/rtw89/core.c
 create mode 100644 drivers/net/wireless/realtek/rtw89/core.h
 create mode 100644 drivers/net/wireless/realtek/rtw89/txrx.h
 create mode 100644 drivers/net/wireless/realtek/rtw89/util.c
 create mode 100644 drivers/net/wireless/realtek/rtw89/util.h

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
new file mode 100644
index 000000000000..3bd31e669aea
--- /dev/null
+++ b/drivers/net/wireless/realtek/rtw89/core.c

@@ -0,0 +1,2359 @@

[...]

quoted

+
+static enum rtw89_ps_mode rtw89_update_ps_mode(struct rtw89_dev *rtwdev)
+{
+	const struct rtw89_chip_info *chip = rtwdev->chip;
+
+	if (rtw89_disable_ps_mode || !chip->ps_mode_supported)
+		return RTW89_PS_MODE_NONE;
+
+	if (chip->ps_mode_supported & BIT(RTW89_PS_MODE_PWR_GATED))
+		return RTW89_PS_MODE_PWR_GATED;

Current driver support only one chip with RTW89_PS_MODE_PWR_GATED,
RTW89_PS_MODE_CLK_GATED and RTW89_PS_MODE_RFOFF.

According to this code, only RTW89_PS_MODE_PWR_GATED will return. Please
remove all other modes to reduce review overhead.

They are not only list of modes, but also the priority. So, I'd like to
preserve the code to change the priority easier. And, I think the logic
is very easy that there's no too much overhead.

[...]

quoted

+
+int rtw89_core_power_on(struct rtw89_dev *rtwdev)
+{
+	int ret;
+
+	ret = rtw89_mac_pwr_on(rtwdev);
+	if (ret) {
+		rtw89_err(rtwdev, "failed to start power sequence\n");
+		goto err;
+	}
+
+	return 0;
+
+err:
+	return ret;
+}

rtw89_core_power_on() is not used. please remote it to avoid review
overhead.

Removed.

quoted

+u8 rtw89_core_acquire_bit_map(unsigned long *addr, unsigned long size)
+{
+	unsigned long bit;
+
+	bit = find_first_zero_bit(addr, size);
+	if (bit < size)
+		set_bit(bit, addr);
+
+	return bit;
+}
+
+void rtw89_core_release_bit_map(unsigned long *addr, u8 bit)
+{
+	clear_bit(bit, addr);
+}
+
+void rtw89_core_release_all_bits_map(unsigned long *addr, unsigned int nbits)
+{
+	bitmap_zero(addr, nbits);
+}
+
+#define RTW89_TYPE_MAPPING(_type)	\
+	case NL80211_IFTYPE_ ## _type:	\
+		rtwvif->wifi_role = RTW89_WIFI_ROLE_ ## _type;	\
+		break
+void rtw89_vif_type_mapping(struct ieee80211_vif *vif, bool assoc)
+{
+	struct rtw89_vif *rtwvif = (struct rtw89_vif *)vif->drv_priv;
+
+	switch (vif->type) {
+	RTW89_TYPE_MAPPING(ADHOC);
+	RTW89_TYPE_MAPPING(STATION);
+	RTW89_TYPE_MAPPING(AP);
+	RTW89_TYPE_MAPPING(AP_VLAN);
+	RTW89_TYPE_MAPPING(MONITOR);
+	RTW89_TYPE_MAPPING(MESH_POINT);
+	RTW89_TYPE_MAPPING(P2P_CLIENT);
+	RTW89_TYPE_MAPPING(P2P_GO);
+	RTW89_TYPE_MAPPING(P2P_DEVICE);
+	RTW89_TYPE_MAPPING(NAN);
+	default:
+		WARN_ON(1);
+		break;
+	}

This switch statement makes no sense. Only NL80211_IFTYPE_AP,
NL80211_IFTYPE_MESH_POINT, NL80211_IFTYPE_ADHOC and
NL80211_IFTYPE_STATION are supported. Please remove this mapping
open code rtwvif->wifi_role in the following switch

Removed.

quoted

+	switch (vif->type) {
+	case NL80211_IFTYPE_AP:
+	case NL80211_IFTYPE_MESH_POINT:
+		rtwvif->net_type = RTW89_NET_TYPE_AP_MODE;
+		rtwvif->self_role = RTW89_SELF_ROLE_AP;
+		break;
+	case NL80211_IFTYPE_ADHOC:
+		rtwvif->net_type = RTW89_NET_TYPE_AD_HOC;

Please set rtwvif->self_role here, to make it easy to read.

Will add it.

[...]