RE: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c - Ping-Ke Shih

From: Ping-Ke Shih <pkshih@realtek.com>
To: Fiona Klute <fiona.klute@gmx.de>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "Kalle Valo" <kvalo@kernel.org>,
	"Ulf Hansson" <ulf.hansson@linaro.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"Pavel Machek" <pavel@ucw.cz>, "Ondřej Jirman" <megi@xff.cz>
Subject: RE: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
Date: Tue, 6 Feb 2024 01:37:39 +0000	[thread overview]
Message-ID: <28c1571cc90b49ce928ddb929e2bc93f@realtek.com> (raw)
In-Reply-To: <09d93cef-5338-4463-b656-dab934029c63@gmx.de>

> -----Original Message-----
> From: Fiona Klute <fiona.klute@gmx.de>
> Sent: Tuesday, February 6, 2024 3:06 AM
> To: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org
> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; Pavel
> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>
> Subject: Re: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
> 
> 
> I'm afraid I'm not familiar with the details either, but this is how the
> SDIO wifi device for the Pinephone is defined (in
> arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi, as of v6.8-rc2):
> 
> &mmc1 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&mmc1_pins>;
>         vmmc-supply = <&reg_vbat_wifi>;
>         vqmmc-supply = <&reg_dldo4>;
>         bus-width = <4>;
>         non-removable;
>         status = "okay";
> 
>         rtl8723cs: wifi@1 {

I think rtl8723cs is module name of vendor driver, so will you add rtw88_8723ds?

>                 reg = <1>;
>         };
> };
> 
> As far as I understand the "reg = <1>;" line implies that the bootloader
> can provide some setup (like the MAC address). I hope someone with more
> knowledge can correct me or add missing details.
> 
> >>   drivers/net/wireless/realtek/rtw88/rtw8703b.c | 2106 +++++++++++++++++
> >>   1 file changed, 2106 insertions(+)
> >>   create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> new file mode 100644
> >> index 0000000000..ac9b1bf6ea
> >> --- /dev/null
> >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> @@ -0,0 +1,2106 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> >> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */
> >> +
> >> +#include <linux/of_net.h>
> >> +#include "main.h"
> >> +#include "coex.h"
> >> +#include "debug.h"
> >> +#include "mac.h"
> >> +#include "phy.h"
> >> +#include "reg.h"
> >> +#include "rx.h"
> >> +#include "rtw8703b.h"
> >> +#include "rtw8703b_tables.h"
> >> +#include "rtw8723x.h"
> >> +
> >> +#define GET_RX_DESC_BW(rxdesc)                                              \
> >> +       (le32_get_bits(*((__le32 *)(rxdesc) + 0x04), GENMASK(31, 24)))
> >
> > use struct and le32_get_bits() directly.
> 
> Do you mean to create a struct to represent the RX descriptor and then
> use le*_get_bits() on the fields to get the values? I could try, but I'd
> have to replace all the other GET_RX_DESC_* macros defined in rx.h (and
> shared by the other chip drivers), too, or it won't really make a
> difference (more below).

Like this:
88b9d8e6cf9c ("wifi: rtw88: use struct instead of macros to set TX desc")

It needs to modify all across chips. 

> 
> [...]
> 
> >> +
> >> +       if (ret != 0)
> >> +               return ret;
> >> +
> >> +#ifdef CONFIG_OF
> >> +       /* Prefer MAC from DT, if available. On some devices like the
> >> +        * Pinephone that might be the only way to get a valid MAC.
> >> +        */
> >> +       struct device_node *node = rtwdev->dev->of_node;
> >
> > Should move this statement to topmost of this function? no compiler warning?
> >
> > Or, make an individual function to read mac addr from DT?
> 
> I can move that to a separate function if you prefer, see below for the
> compiler warning.

Because this is CONFIG_OF chunk, it will look like below if you move declaration upward:

#ifdef CONFIG_OF
	struct device_node *node = rtwdev->dev->of_node;
#endif
	// other declaration ...

	// other code
#ifdef CONFIG_OF
	if (node) {
		...
	}
#endif

It seems like too much #ifdef. With a separate function, it can be single #ifdef. 
That is my point. 

> 
> >> +
> >> +       if (node) {
> >> +               ret = of_get_mac_address(node, efuse->addr);
> >> +               if (ret == 0) {
> >> +                       rtw_dbg(rtwdev, RTW_DBG_EFUSE,
> >> +                               "got wifi mac address from DT: %pM\n",
> >> +                               efuse->addr);
> >> +               }
> >> +       }
> >> +#endif /* CONFIG_OF */
> >> +
> >> +       /* If TX power index table in EFUSE is invalid, fall back to
> >> +        * built-in table.
> >> +        */
> >> +       u8 *pwr = (u8 *)efuse->txpwr_idx_table;
> >> +       bool valid = false;
> >
> > I tend to move these declaration to top of this function too, but not sure why
> > compiler also doesn't warn this in my side. Seemingly kernel changes default
> > compiler flags?
> 
> Yes, I learned about that while working on this driver. First the move
> to gnu11, and then removing -Wdeclaration-after-statement with
> b5ec6fd286dfa466f64cb0e56ed768092d0342ae in 6.5. The commit message says
> "It will still be recommeneded [sic!] to place declarations at the start
> of a scope where possible, but it will no longer be enforced", so I'll
> move these up.

Thanks for the info. 

> 
> For the struct device_node pointer I think it makes sense to leave the
> declaration within the #ifdef CONFIG_OF section (unless we restructure
> that into a separate function) because it's unused otherwise.

See my point above. But your point makes sense too. 

> 
> [...]
> 
> >> +static void rtw8703b_query_rx_desc(struct rtw_dev *rtwdev, u8 *rx_desc,
> >> +                                  struct rtw_rx_pkt_stat *pkt_stat,
> >> +                                  struct ieee80211_rx_status *rx_status)
> >> +{
> >> +       struct ieee80211_hdr *hdr;
> >> +       u32 desc_sz = rtwdev->chip->rx_pkt_desc_sz;
> >> +       u8 *phy_status = NULL;
> >> +
> >> +       memset(pkt_stat, 0, sizeof(*pkt_stat));
> >> +
> >> +       pkt_stat->phy_status = GET_RX_DESC_PHYST(rx_desc);
> >> +       pkt_stat->icv_err = GET_RX_DESC_ICV_ERR(rx_desc);
> >> +       pkt_stat->crc_err = GET_RX_DESC_CRC32(rx_desc);
> >> +       pkt_stat->decrypted = !GET_RX_DESC_SWDEC(rx_desc) &&
> >> +                             GET_RX_DESC_ENC_TYPE(rx_desc) != RX_DESC_ENC_NONE;
> >> +       pkt_stat->is_c2h = GET_RX_DESC_C2H(rx_desc);
> >> +       pkt_stat->pkt_len = GET_RX_DESC_PKT_LEN(rx_desc);
> >> +       pkt_stat->drv_info_sz = GET_RX_DESC_DRV_INFO_SIZE(rx_desc);
> >> +       pkt_stat->shift = GET_RX_DESC_SHIFT(rx_desc);
> >> +       pkt_stat->rate = GET_RX_DESC_RX_RATE(rx_desc);
> >> +       pkt_stat->cam_id = GET_RX_DESC_MACID(rx_desc);
> >> +       pkt_stat->ppdu_cnt = 0;
> >> +       pkt_stat->tsf_low = GET_RX_DESC_TSFL(rx_desc);
> >
> > Could you add a separate patch to convert these macros to struct style?
> > It is fine to keep as it was, and do this conversion afterward.
> 
> In principle yes, but as I mentioned above I'd basically have to
> reinvent all the definitions from rx.h to make it work, I'm not sure if
> that really simplifies things. If you want to refactor those I think
> it'd be best to do it for all chip drivers together.

Yes, for all chips. 

> 
> The GET_PHY_STAT_* macros are a different matter. The PHY status
> structure is different between 8703b and the other supported chips, so
> those could be replaced with a struct without duplication. Or at least
> mostly, some elements are bit fields or values with < 8 bits, where I
> think a macro is simpler than a struct with different definitions
> depending on endianess. I am worried about introducing an endianess
> error though, so I'd have to ask for careful review of such a patch.

I think we can keep it as it was for this patchset, and another patches later
to convert this kind of macros. Months ago, Kalle guided the rules how
rtw88/89 can do [1]. For me, I will convert macros to struct once I touch
them. 

[1] https://lore.kernel.org/all/87a5zpb71j.fsf_-_@kernel.org/