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

[Pkshih <pkshih@realtek.com>]

> -----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:
> > 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 @@

[...]

> > +
> > +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.

[...]

> > +
> > +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.

> > +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.

> > +	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.

[...]