| Message ID | 20240202121050.977223-1-fiona.klute@gmx.de |
|---|---|
| Headers | show |
| Series | rtw88: Add support for RTL8723CS/RTL8703B | expand |
Am 02.02.24 um 14:13 schrieb Dmitry Antipov: > On 2/2/24 15:10, Fiona Klute wrote: > >> This patch set adds a driver for RTL8723CS, which is used in the >> Pinephone and a few other devices. It is a combined wifi/bluetooth >> device, the wifi part is called RTL8703B. There is already a mainline >> driver for the bluetooth part. RTL8703B is similar to the RTL8723D >> chip already supported by rtw88. I've been using the out-of-tree >> rtl8723cs driver as reference. > > On top of wireless-next (17903a283593), I'm seeing: > > drivers/net/wireless/realtek/rtw88/mac.c: In function > '__rtw_download_firmware_legacy': > drivers/net/wireless/realtek/rtw88/mac.c:940:33: error: > 'RTW_CHIP_TYPE_8703B' undeclared > (first use in this function); did you mean 'RTW_CHIP_TYPE_8723D'? > 940 | if (rtwdev->chip->id == RTW_CHIP_TYPE_8703B && > | ^~~~~~~~~~~~~~~~~~~ > | RTW_CHIP_TYPE_8723D > drivers/net/wireless/realtek/rtw88/mac.c:940:33: note: each undeclared > identifier is > reported only once for each function it appears in > > It seems that 'enum rtw_chip_type' has missing an entry. That definition should have been added to main.h by patch 3 ("wifi: rtw88: Add definitions for 8703b chip"). Was there some issue with applying that one?
Am 02.02.24 um 14:54 schrieb Dmitry Antipov: > The only minor issue actually is: > > drivers/net/wireless/realtek/rtw88/rtw8723x.c:314:6: warning: no previous > prototype for function '__rtw8723x_cfg_ldo25' [-Wmissing-prototypes] > 314 | void __rtw8723x_cfg_ldo25(struct rtw_dev *rtwdev, bool enable) > | ^ > drivers/net/wireless/realtek/rtw88/rtw8723x.c:314:1: note: declare 'static' > if the function is not intended to be used outside of this translation unit > 314 | void __rtw8723x_cfg_ldo25(struct rtw_dev *rtwdev, bool enable) > | ^ > | static Looks like I forgot a static there, will fix that in the next version. Thank you! Fiona
Hi! > Ping-Ke Shih kindly offered to add the required s-o-b for the firmware > and help get it into linux-firmware when it's time, for testing now > please see the code I used to extract firmware from the out-of-tree > driver [1]. Can I get you to apply this? So far I got driver to insert, but I did not have firmware installed. That should be ok on next reboot. Best regards, Pavel diff --git a/Makefile b/Makefile index 1f8e8b6..fbc1f14 100644 --- a/Makefile +++ b/Makefile @@ -7,6 +7,10 @@ firmware_files = rtw8703b_ap_fw.bin \ all: $(firmware_files) +install: $(firmware_files) + sudo mkdir /lib/firmware/rtw88/ + sudo cp $(firmware_files) /lib/firmware/rtw88/ + %.o: %.c gcc -fPIC -DCONFIG_RTL8703B -c -o $@ $<
Hi! > This patch set adds a driver for RTL8723CS, which is used in the > Pinephone and a few other devices. It is a combined wifi/bluetooth > device, the wifi part is called RTL8703B. There is already a mainline > driver for the bluetooth part. RTL8703B is similar to the RTL8723D > chip already supported by rtw88. I've been using the out-of-tree > rtl8723cs driver as reference. I've installed in on PinePhone with v6.8-rc1, got firmware, and it simply started working (maybe with slightly reduced performance, but I did not check). In fact, I'm sending this email using the new driver. Tested-by: Pavel Machek <pavel@ucw.cz> Thank you! Best regards, Pavel
Hi! > This patch set adds a driver for RTL8723CS, which is used in the > Pinephone and a few other devices. It is a combined wifi/bluetooth > device, the wifi part is called RTL8703B. There is already a mainline > driver for the bluetooth part. RTL8703B is similar to the RTL8723D > chip already supported by rtw88. I've been using the out-of-tree > rtl8723cs driver as reference. I tried compiling *23cs into kernel (not modular), but that one fails, with "multiple definition of rtw8723x_iqk_backup_path_ctrl" and similar. Sorry, can't cut&paste right now, but it should be easy to reproduce and fix. Best regards, Pavel
Am 02.02.24 um 21:19 schrieb Pavel Machek: > Can I get you to apply this? Done, good idea! > diff --git a/Makefile b/Makefile > index 1f8e8b6..fbc1f14 100644 > --- a/Makefile > +++ b/Makefile > @@ -7,6 +7,10 @@ firmware_files = rtw8703b_ap_fw.bin \ > > all: $(firmware_files) > > +install: $(firmware_files) > + sudo mkdir /lib/firmware/rtw88/ > + sudo cp $(firmware_files) /lib/firmware/rtw88/ > + > %.o: %.c > gcc -fPIC -DCONFIG_RTL8703B -c -o $@ $< > >
Am 02.02.24 um 21:52 schrieb Pavel Machek: > I tried compiling *23cs into kernel (not modular), but that one fails, > with "multiple definition of rtw8723x_iqk_backup_path_ctrl" and > similar. Sorry, can't cut&paste right now, but it should be easy to > reproduce and fix. The IQK helper functions need to marked as static, after that (and adding firmware to the initramfs) the built-in driver works. Queued for v2, thank you! Best regards, Fiona
> -----Original Message----- > From: Fiona Klute <fiona.klute@gmx.de> > Sent: Friday, February 2, 2024 8:11 PM > To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> > 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>; Fiona Klute <fiona.klute@gmx.de> > Subject: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h > > This is the main header for the new rtw88_8703b chip driver. > > Signed-off-by: Fiona Klute <fiona.klute@gmx.de> > --- > drivers/net/wireless/realtek/rtw88/rtw8703b.h | 62 +++++++++++++++++++ > 1 file changed, 62 insertions(+) > create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.h > > diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.h > b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > new file mode 100644 > index 0000000000..f5ff23f2ee > --- /dev/null > +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ > +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ > + > +#ifndef __RTW8703B_H__ > +#define __RTW8703B_H__ > + > +extern const struct rtw_chip_info rtw8703b_hw_spec; > + > +/* phy status parsing */ > +#define GET_PHY_STAT_AGC_GAIN_A(phy_stat) \ > + (le32_get_bits(*((__le32 *)(phy_stat) + 0x00), GENMASK(6, 0))) We are planning to use struct and le32_get_bits() directly, so don't introduce this old style anymore. An example is struct rtw8703b_phy_stat { __le32 w0; __le32 w1; ... }; #define RTW8703B_PHY_STAT_W0_AGC_GAIN_A GENMASK(6, 0) val_s8 = le32_get_bits(stat->w0, RTW8703B_PHY_STAT_W0_AGC_GAIN_A); > + > +#define GET_PHY_STAT_PWDB(phy_stat) \ > + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(7, 0)) > +#define GET_PHY_STAT_VGA(phy_stat) \ > + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(12, 8)) > +#define GET_PHY_STAT_LNA_L(phy_stat) \ > + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(15, 13)) > +/* the high LNA stat bit if 4 bit format is used */ > +#define GET_PHY_STAT_LNA_H(phy_stat) \ > + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), BIT(23)) > +#define BIT_LNA_H_MASK BIT(3) > +#define BIT_LNA_L_MASK GENMASK(2, 0) > + > +#define GET_PHY_STAT_CFO_TAIL_A(phy_stat) \ > + (le32_get_bits(*((__le32 *)(phy_stat) + 0x02), GENMASK(15, 8))) > +#define GET_PHY_STAT_RXEVM_A(phy_stat) \ > + (le32_get_bits(*((__le32 *)(phy_stat) + 0x03), GENMASK(15, 8))) > +#define GET_PHY_STAT_RXSNR_A(phy_stat) \ > + (le32_get_bits(*((__le32 *)(phy_stat) + 0x03), GENMASK(31, 24))) > + > +/* Baseband registers */ > +#define REG_BB_PWR_SAV5_11N 0x0818 > +/* BIT(11) should be 1 for 8703B *and* 8723D, which means LNA uses 4 > + * bit for CCK rates in report, not 3. Vendor driver logs a warning if > + * it's 0, but handles the case. > + * > + * Purpose of other parts of this register is unknown, 8723cs driver > + * code indicates some other chips use certain bits for antenna > + * diversity. > + */ > +#define REG_BB_AMP 0x0950 > +#define BIT_MASK_RX_LNA (BIT(11)) > + > +/* 0xaXX: 40MHz channel settings */ > +#define REG_CCK_TXSF2 0x0a24 /* CCK TX filter 2 */ > +#define REG_CCK_DBG 0x0a28 /* debug port */ > +#define REG_OFDM0_A_TX_AFE 0x0c84 > +#define REG_TXIQK_MATRIXB_LSB2_11N 0x0c9c > +#define REG_OFDM0_TX_PSD_NOISE 0x0ce4 /* TX pseudo noise weighting */ > +/* is != 0 when IQK is done */ Is this comment for 0x0e90? move to rear of the line? > +#define REG_IQK_RDY 0x0e90 > + > +/* RF registers */ > +#define RF_RCK1 0x1E > + > +#define AGG_BURST_NUM 3 > +#define AGG_BURST_SIZE 0 /* 1K */ > +#define BIT_MASK_AGG_BURST_NUM (GENMASK(3, 2)) > +#define BIT_MASK_AGG_BURST_SIZE (GENMASK(5, 4)) > + > +#endif /* __RTW8703B_H__ */ > -- > 2.43.0 >
> -----Original Message----- > From: Fiona Klute <fiona.klute@gmx.de> > Sent: Friday, February 2, 2024 8:11 PM > To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> > 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>; Fiona Klute <fiona.klute@gmx.de> > Subject: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c > > This is the main source for the new rtw88_8703b chip driver. > > Signed-off-by: Fiona Klute <fiona.klute@gmx.de> > --- > > rtw8703b_read_efuse retrieves the MAC address from DT, if > available. The code is not tied to any particular hardware, but > required to get a persistent address on the Pinephone. Do I need to > add a DT binding for this? Also, I think this could be moved into > rtw_chip_efuse_info_setup(), in preference to falling back to a random > MAC if there isn't a valid one in EFUSE. Would that be acceptable? If > yes, should EFUSE or DT take priority? > > All the RTL8723CS EFUSE samples I've seen so far contain almost > entirely invalid data (all 0xff, except rtl_id, afe, and > thermal_meter), so I've added fallbacks in the EFUSE parser. In some > cases they alter specific bits so parsing in rtw_chip_efuse_info_setup > will get the right results. I'm not sure if this is the best approach: > The good part is that it works without changing the core EFUSE code, > the negative is that it's not visible to the core code that a fallback > has been applied. What do you think? I think efuse take priority, but you have said most are invalid data, so you can write a rule to determine efuse is valid before using them. If invalid, just use DT. Sorry, I'm not familiar with DT, could you show me an example of DT node? > > 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. > + > +#define BIT_MASK_TXQ_INIT (BIT(7)) > +#define WLAN_RL_VAL 0x3030 > +/* disable BAR */ > +#define WLAN_BAR_VAL 0x0201ffff > +#define WLAN_PIFS_VAL 0 > +#define WLAN_RX_PKT_LIMIT 0x18 > +#define WLAN_SLOT_TIME 0x09 > +#define WLAN_SPEC_SIFS 0x100a > +#define WLAN_MAX_AGG_NR 0x1f > +#define WLAN_AMPDU_MAX_TIME 0x70 > + > +/* unit is 32us */ > +#define TBTT_PROHIBIT_SETUP_TIME 0x04 > +#define TBTT_PROHIBIT_HOLD_TIME 0x80 > +#define TBTT_PROHIBIT_HOLD_TIME_STOP_BCN 0x64 > + > +/* raw pkt_stat->drv_info_sz is in unit of 8-bytes */ > +#define RX_DRV_INFO_SZ_UNIT_8703B 8 > + > +#define TRANS_SEQ_END \ > + { \ > + 0xFFFF, \ > + RTW_PWR_CUT_ALL_MSK, \ > + RTW_PWR_INTF_ALL_MSK, \ > + 0, \ > + RTW_PWR_CMD_END, 0, 0} nit: This looks a little odd, not like others of trans_pre_enable_8703b[]. How about this: #define TRANS_SEQ_END \ {0xFFFF, \ RTW_PWR_CUT_ALL_MSK, \ RTW_PWR_INTF_ALL_MSK, \ 0, \ RTW_PWR_CMD_END, 0, 0} [...] > +static const u8 rtw8703b_cck_swing_table[][16] = { > + {0x44, 0x42, 0x3C, 0x33, 0x28, 0x1C, 0x13, 0x0B, 0x05, 0x02, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, /*-16dB*/ nit: align "{" ? {0x44, 0x42, 0x3C, 0x33, 0x28, 0x1C, 0x13, 0x0B, 0x05, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, /*-16dB*/ [...] > + > +/* Default power index table for RTL8703B, used if EFUSE does not > + * contain valid data. Replaces EFUSE data from offset 0x10 (start of > + * txpwr_idx_table). > + */ > +static const u8 rtw8703b_txpwr_idx_table[] = { > + 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, > + 0x2D, 0x2D, 0x2D, 0x2D, 0x2D, 0x02 > +}; > + > +#define RTW8703B_TXPWR_IDX_TABLE_LEN ARRAY_SIZE(rtw8703b_txpwr_idx_table) nit: This definition seems not save much. > + > +#define DBG_EFUSE_FIX(name) \ suggest to not hide 'rtwdev' > + rtw_dbg(rtwdev, RTW_DBG_EFUSE, "Fixed invalid EFUSE value: " \ > + # name "=0x%x\n", rtwdev->efuse.name) a blank line? > +static int rtw8703b_read_efuse(struct rtw_dev *rtwdev, u8 *log_map) > +{ > + struct rtw_efuse *efuse = &rtwdev->efuse; > + int ret = rtw8723x_read_efuse(rtwdev, log_map); I prefer int ret; ret = rtw8723x_read_efuse(rtwdev, log_map); if (ret) return ret; > + > + 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? > + > + 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? [...] > +static void rtw8703b_set_channel_bb(struct rtw_dev *rtwdev, u8 channel, u8 bw, > + u8 primary_ch_idx) > +{ > + const struct rtw_backup_info *cck_dfir; > + int i; > + > + cck_dfir = channel <= 13 ? cck_dfir_cfg[0] : cck_dfir_cfg[1]; > + > + for (i = 0; i < CCK_DFIR_NR_8703B; i++, cck_dfir++) > + rtw_write32(rtwdev, cck_dfir->reg, cck_dfir->val); > + > + switch (bw) { > + case RTW_CHANNEL_WIDTH_20: > + rtw_write32_mask(rtwdev, REG_FPGA0_RFMOD, BIT_MASK_RFMOD, 0x0); > + rtw_write32_mask(rtwdev, REG_FPGA1_RFMOD, BIT_MASK_RFMOD, 0x0); > + rtw_write32_mask(rtwdev, REG_OFDM0_TX_PSD_NOISE, > + GENMASK(31, 20), 0x0); > + rtw_write32(rtwdev, REG_BBRX_DFIR, 0x4A880000); > + rtw_write32(rtwdev, REG_OFDM0_A_TX_AFE, 0x19F60000); > + break; > + case RTW_CHANNEL_WIDTH_40: > + rtw_write32_mask(rtwdev, REG_FPGA0_RFMOD, BIT_MASK_RFMOD, 0x1); > + rtw_write32_mask(rtwdev, REG_FPGA1_RFMOD, BIT_MASK_RFMOD, 0x1); > + rtw_write32(rtwdev, REG_BBRX_DFIR, 0x40100000); > + rtw_write32(rtwdev, REG_OFDM0_A_TX_AFE, 0x51F60000); > + rtw_write32_mask(rtwdev, REG_CCK0_SYS, BIT_CCK_SIDE_BAND, > + (primary_ch_idx == RTW_SC_20_UPPER ? 1 : 0)); unnecessary parenthesis around ? : > + rtw_write32_mask(rtwdev, REG_OFDM_FA_RSTD_11N, 0xC00, > + primary_ch_idx == RTW_SC_20_UPPER ? 2 : 1); > + > + rtw_write32_mask(rtwdev, REG_BB_PWR_SAV5_11N, GENMASK(27, 26), > + primary_ch_idx == RTW_SC_20_UPPER ? 1 : 2); > + break; > + default: > + break; > + } > +} > + [...] > +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. [...] > +static > +void rtw8703b_iqk_fill_a_matrix(struct rtw_dev *rtwdev, const s32 result[]) > +{ > + s32 oldval_1; > + s32 x, y; > + s32 tx1_a, tx1_a_ext; > + s32 tx1_c, tx1_c_ext; > + u32 tmp_rx_iqi = 0x40000100 & GENMASK(31, 16); reverse X'mas tree order > + > +static void rtw8703b_phy_calibration(struct rtw_dev *rtwdev) > +{ > + /* For some reason path A is called S1 and B S0 in shared > + * rtw88 calibration data. > + */ > + struct rtw_dm_info *dm_info = &rtwdev->dm_info; > + s32 result[IQK_ROUND_SIZE][IQK_NR]; > + struct rtw8723x_iqk_backup_regs backup; > + u8 i, j; > + u8 final_candidate = IQK_ROUND_INVALID; > + bool good; reverse X'mas tree order > +MODULE_FIRMWARE("rtw88/rtw8703b_wow_fw.bin"); Just curious. Have you tried WOW for this chip?
Am 05.02.24 um 04:01 schrieb Ping-Ke Shih: >> -----Original Message----- >> From: Fiona Klute <fiona.klute@gmx.de> >> Sent: Friday, February 2, 2024 8:11 PM >> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> >> 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>; Fiona Klute <fiona.klute@gmx.de> >> Subject: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c >> >> This is the main source for the new rtw88_8703b chip driver. >> >> Signed-off-by: Fiona Klute <fiona.klute@gmx.de> >> --- >> >> rtw8703b_read_efuse retrieves the MAC address from DT, if >> available. The code is not tied to any particular hardware, but >> required to get a persistent address on the Pinephone. Do I need to >> add a DT binding for this? Also, I think this could be moved into >> rtw_chip_efuse_info_setup(), in preference to falling back to a random >> MAC if there isn't a valid one in EFUSE. Would that be acceptable? If >> yes, should EFUSE or DT take priority? >> >> All the RTL8723CS EFUSE samples I've seen so far contain almost >> entirely invalid data (all 0xff, except rtl_id, afe, and >> thermal_meter), so I've added fallbacks in the EFUSE parser. In some >> cases they alter specific bits so parsing in rtw_chip_efuse_info_setup >> will get the right results. I'm not sure if this is the best approach: >> The good part is that it works without changing the core EFUSE code, >> the negative is that it's not visible to the core code that a fallback >> has been applied. What do you think? > > I think efuse take priority, but you have said most are invalid data, so > you can write a rule to determine efuse is valid before using them. If > invalid, just use DT. That's no problem, I could use is_valid_ether_addr() like rtw_chip_efuse_info_setup() in main.c already does. That's why I'm wondering if it'd make sense to move getting the MAC from DT to there, and use MAC from EFUSE, DT, or random in that order of preference for all rtw88 devices. > Sorry, I'm not familiar with DT, could you show me an example of DT node? 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 { 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). [...] >> + >> + 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. >> + >> + 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. 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. [...] >> +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. 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. [...] >> +MODULE_FIRMWARE("rtw88/rtw8703b_wow_fw.bin"); > > Just curious. Have you tried WOW for this chip? Kind of. It definitely doesn't work with this code, that's why I haven't filled wowlan_stub yet. I already know the check if WOW has been enabled would have to be changed in wow.c (I'd probably add a "legacy" function like for the firmware download), but there must be some other details because I couldn't get it to work yet. But others got it to work with the out-of-tree driver, so it must be possible. I've applied the other minor changes you suggested.
Am 05.02.24 um 03:24 schrieb Ping-Ke Shih: >> -----Original Message----- >> From: Fiona Klute <fiona.klute@gmx.de> >> Sent: Friday, February 2, 2024 8:11 PM >> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> >> 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>; Fiona Klute <fiona.klute@gmx.de> >> Subject: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h >> >> This is the main header for the new rtw88_8703b chip driver. >> >> Signed-off-by: Fiona Klute <fiona.klute@gmx.de> >> --- >> drivers/net/wireless/realtek/rtw88/rtw8703b.h | 62 +++++++++++++++++++ >> 1 file changed, 62 insertions(+) >> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.h >> >> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.h >> b/drivers/net/wireless/realtek/rtw88/rtw8703b.h >> new file mode 100644 >> index 0000000000..f5ff23f2ee >> --- /dev/null >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h >> @@ -0,0 +1,62 @@ >> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ >> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ >> + >> +#ifndef __RTW8703B_H__ >> +#define __RTW8703B_H__ >> + >> +extern const struct rtw_chip_info rtw8703b_hw_spec; >> + >> +/* phy status parsing */ >> +#define GET_PHY_STAT_AGC_GAIN_A(phy_stat) \ >> + (le32_get_bits(*((__le32 *)(phy_stat) + 0x00), GENMASK(6, 0))) > > We are planning to use struct and le32_get_bits() directly, so don't introduce > this old style anymore. An example is > > struct rtw8703b_phy_stat { > __le32 w0; > __le32 w1; > ... > }; > > #define RTW8703B_PHY_STAT_W0_AGC_GAIN_A GENMASK(6, 0) > > val_s8 = le32_get_bits(stat->w0, RTW8703B_PHY_STAT_W0_AGC_GAIN_A); Sorry, of all your mails this one got stuck in the spam filter. This answers my question about what you meant by struct style, I hadn't thought of using __le types. I assume you'd still want to use appropriately sized types/arrays for elements that are multiples of one byte? >> + >> +#define GET_PHY_STAT_PWDB(phy_stat) \ >> + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(7, 0)) >> +#define GET_PHY_STAT_VGA(phy_stat) \ >> + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(12, 8)) >> +#define GET_PHY_STAT_LNA_L(phy_stat) \ >> + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), GENMASK(15, 13)) >> +/* the high LNA stat bit if 4 bit format is used */ >> +#define GET_PHY_STAT_LNA_H(phy_stat) \ >> + le32_get_bits(*((__le32 *)(phy_stat) + 0x01), BIT(23)) >> +#define BIT_LNA_H_MASK BIT(3) >> +#define BIT_LNA_L_MASK GENMASK(2, 0) >> + >> +#define GET_PHY_STAT_CFO_TAIL_A(phy_stat) \ >> + (le32_get_bits(*((__le32 *)(phy_stat) + 0x02), GENMASK(15, 8))) >> +#define GET_PHY_STAT_RXEVM_A(phy_stat) \ >> + (le32_get_bits(*((__le32 *)(phy_stat) + 0x03), GENMASK(15, 8))) >> +#define GET_PHY_STAT_RXSNR_A(phy_stat) \ >> + (le32_get_bits(*((__le32 *)(phy_stat) + 0x03), GENMASK(31, 24))) >> + >> +/* Baseband registers */ >> +#define REG_BB_PWR_SAV5_11N 0x0818 >> +/* BIT(11) should be 1 for 8703B *and* 8723D, which means LNA uses 4 >> + * bit for CCK rates in report, not 3. Vendor driver logs a warning if >> + * it's 0, but handles the case. >> + * >> + * Purpose of other parts of this register is unknown, 8723cs driver >> + * code indicates some other chips use certain bits for antenna >> + * diversity. >> + */ >> +#define REG_BB_AMP 0x0950 >> +#define BIT_MASK_RX_LNA (BIT(11)) >> + >> +/* 0xaXX: 40MHz channel settings */ >> +#define REG_CCK_TXSF2 0x0a24 /* CCK TX filter 2 */ >> +#define REG_CCK_DBG 0x0a28 /* debug port */ >> +#define REG_OFDM0_A_TX_AFE 0x0c84 >> +#define REG_TXIQK_MATRIXB_LSB2_11N 0x0c9c >> +#define REG_OFDM0_TX_PSD_NOISE 0x0ce4 /* TX pseudo noise weighting */ >> +/* is != 0 when IQK is done */ > > Is this comment for 0x0e90? move to rear of the line? Yes, I'll do that. >> +#define REG_IQK_RDY 0x0e90 >> + >> +/* RF registers */ >> +#define RF_RCK1 0x1E >> + >> +#define AGG_BURST_NUM 3 >> +#define AGG_BURST_SIZE 0 /* 1K */ >> +#define BIT_MASK_AGG_BURST_NUM (GENMASK(3, 2)) >> +#define BIT_MASK_AGG_BURST_SIZE (GENMASK(5, 4)) >> + >> +#endif /* __RTW8703B_H__ */ >> -- >> 2.43.0 >> >
> -----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 = <®_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 <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/
> -----Original Message----- > From: Fiona Klute <fiona.klute@gmx.de> > Sent: Tuesday, February 6, 2024 9:09 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 4/9] wifi: rtw88: Add rtw8703b.h > > Am 05.02.24 um 03:24 schrieb Ping-Ke Shih: > >> -----Original Message----- > >> From: Fiona Klute <fiona.klute@gmx.de> > >> Sent: Friday, February 2, 2024 8:11 PM > >> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> > >> 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>; Fiona Klute <fiona.klute@gmx.de> > >> Subject: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h > >> > >> This is the main header for the new rtw88_8703b chip driver. > >> > >> Signed-off-by: Fiona Klute <fiona.klute@gmx.de> > >> --- > >> drivers/net/wireless/realtek/rtw88/rtw8703b.h | 62 +++++++++++++++++++ > >> 1 file changed, 62 insertions(+) > >> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.h > >> > >> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.h > >> b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > >> new file mode 100644 > >> index 0000000000..f5ff23f2ee > >> --- /dev/null > >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > >> @@ -0,0 +1,62 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ > >> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ > >> + > >> +#ifndef __RTW8703B_H__ > >> +#define __RTW8703B_H__ > >> + > >> +extern const struct rtw_chip_info rtw8703b_hw_spec; > >> + > >> +/* phy status parsing */ > >> +#define GET_PHY_STAT_AGC_GAIN_A(phy_stat) \ > >> + (le32_get_bits(*((__le32 *)(phy_stat) + 0x00), GENMASK(6, 0))) > > > > We are planning to use struct and le32_get_bits() directly, so don't introduce > > this old style anymore. An example is > > > > struct rtw8703b_phy_stat { > > __le32 w0; > > __le32 w1; > > ... > > }; > > > > #define RTW8703B_PHY_STAT_W0_AGC_GAIN_A GENMASK(6, 0) > > > > val_s8 = le32_get_bits(stat->w0, RTW8703B_PHY_STAT_W0_AGC_GAIN_A); > > Sorry, of all your mails this one got stuck in the spam filter. This > answers my question about what you meant by struct style, I hadn't > thought of using __le types. I assume you'd still want to use > appropriately sized types/arrays for elements that are multiples of one > byte? Not sure "multiples of one byte" means. I guess you want to use something like u8 or __le16 for the elements that aren't required bit access, right? I'd say it is hard to define single one rule for all cases. Example 1-1 (fake): struct rtw8703b_phy_stat { u8 mac_id; u8 rssi; u8 evm; u8 evm_2; ... } __packed; Example 1-2 (fake): struct rtw8703b_phy_stat { __le32 w0; ... } __packed; #define PHY_STAT_W0_MACID GENMASK(7, 0) #define PHY_STAT_W0_RSSI GENMASK(15, 8) #define PHY_STAT_W0_EVM GENMASK(23, 16) #define PHY_STAT_W0_EVM_2 GENMASK(31,24) It is clear that example 1-1 is better than 1-2. Example 2-1 (fake): struct rtw8703b_phy_stat { u8 mac_id; u8 rssi; __le16 phy_st; // evm: 7, evm_2: 7, rsvd :2 ... } __packed; #define PHY_ST_EVM GENMASK(6, 0) #define PHY_ST_EVM_2 GENMASK(13, 7) Example 2-2 (fake): struct rtw8703b_phy_stat { __le32 w0; ... } __packed; #define PHY_STAT_W0_MACID GENMASK(7, 0) #define PHY_STAT_W0_RSSI GENMASK(15, 8) #define PHY_STAT_W0_EVM GENMASK(22, 16) #define PHY_STAT_W0_EVM_2 GENMASK(29, 23) Compare 2-1 and 2-2, it would be hard to say which one is better, because 2-1 mixes u8 and bit field. This is just a simple example, fields of real struct are much more, so normally I use 1-2 and 2-2 styles.
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
> -----Original Message----- > From: Ping-Ke Shih <pkshih@realtek.com> > Sent: Tuesday, February 6, 2024 9:38 AM > To: Fiona Klute <fiona.klute@gmx.de>; 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 > > > > > -----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 > > > > >> + > > >> + 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