> -----Original Message----- > From: Oleksij Rempel [mailto:o.rempel@xxxxxxxxxxxxxx] > Sent: Saturday, July 10, 2021 1:32 AM > To: Pkshih > Cc: kvalo@xxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxx> > > --- > > 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. [...]
