> -----Original Message-----
> From: Fiona Klute <[email protected]>
> Sent: Tuesday, February 6, 2024 3:06 AM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: Kalle Valo <[email protected]>; Ulf Hansson <[email protected]>; [email protected]; Pavel
> Machek <[email protected]>; Ondřej Jirman <[email protected]>
> 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 = <®_vbat_wifi>;
> vqmmc-supply = <®_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 <[email protected]> */
> >> +
> >> +#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/[email protected]/
hi!
> > 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 = <®_vbat_wifi>;
> > vqmmc-supply = <®_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?
dts is supposed to describe hardware, not driver structure. So a) this
label should not really matter b) it really should not really be
changed to match the driver name.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
> -----Original Message-----
> From: Ping-Ke Shih <[email protected]>
> Sent: Tuesday, February 6, 2024 9:38 AM
> To: Fiona Klute <[email protected]>; [email protected]
> Cc: Kalle Valo <[email protected]>; Ulf Hansson <[email protected]>; [email protected]; Pavel
> Machek <[email protected]>; Ondřej Jirman <[email protected]>
> Subject: RE: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
>
>
>
> > -----Original Message-----
> > From: Fiona Klute <[email protected]>
> > Sent: Tuesday, February 6, 2024 3:06 AM
> > To: Ping-Ke Shih <[email protected]>; [email protected]
> > Cc: Kalle Valo <[email protected]>; Ulf Hansson <[email protected]>; [email protected];
> Pavel
> > Machek <[email protected]>; Ondřej Jirman <[email protected]>
> > Subject: Re: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
> >
> > >> +
> > >> + 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 CONFIG_OF isn't defined, compiler will select a static inline function that
just returns -ENODEV [1], so I think you don't need a #ifdef chunk here.
[1] https://elixir.bootlin.com/linux/latest/source/include/linux/of_net.h#L27