| Message ID | 20240227235507.781615-5-fiona.klute@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtw88: Add support for RTL8723CS/RTL8703B | expand |
> -----Original Message----- > From: Fiona Klute <fiona.klute@gmx.de> > Sent: Wednesday, February 28, 2024 7:55 AM > To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> > Cc: Fiona Klute <fiona.klute@gmx.de>; kvalo@kernel.org; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; > pavel@ucw.cz; megi@xff.cz > Subject: [PATCH v2 4/9] wifi: rtw88: Add rtw8703b.h > > This is the main header for the new rtw88_8703b chip driver. > > Acked-by: Ping-Ke Shih <pkshih@realtek.com> > Tested-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Fiona Klute <fiona.klute@gmx.de> > --- > drivers/net/wireless/realtek/rtw88/rtw8703b.h | 103 ++++++++++++++++++ > 1 file changed, 103 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 00000000000..69dac101d33 > --- /dev/null > +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > @@ -0,0 +1,103 @@ > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ > +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ > + > +#ifndef __RTW8703B_H__ > +#define __RTW8703B_H__ > + > +#include <linux/types.h> > +#include <linux/compiler_attributes.h> Removing these two headers can still compiled pass in my side. Please check why you need them. By the way, rtw8723d.h does #include "rtw8723x.h" Can we use the same stuff? > + > +extern const struct rtw_chip_info rtw8703b_hw_spec; > + > +/* phy status parsing */ > +#define VGA_BITS GENMASK(4, 0) > +#define LNA_L_BITS GENMASK(7, 5) > +#define LNA_H_BIT BIT(7) > +/* masks for assembling LNA index from high and low bits */ > +#define BIT_LNA_H_MASK BIT(3) > +#define BIT_LNA_L_MASK GENMASK(2, 0) > + > +struct phy_rx_agc_info { > +#ifdef __LITTLE_ENDIAN > + u8 gain: 7; > + u8 trsw: 1; > +#else > + u8 trsw: 1; > + u8 gain: 7; > +#endif > +} __packed; This struct is quite simple, or I will suggest to define MASK and access them by u8_get_bits(), like #define RX_AGC_GAIN GENMASK(6, 0) gain = u8_get_bits(RX_AGC_GAIN, raw); Then, struct phy_status_8703b { u8 path_agc[2]; ... Just for reference. You can decide if changing to this style. > + > +/* This struct is called phy_status_rpt_8192cd in the vendor driver, > + * there might be potential to share it with drivers for other chips > + * of the same generation. > + */ > +struct phy_status_8703b { > + struct phy_rx_agc_info path_agc[2]; > + u8 ch_corr[2]; > + u8 cck_sig_qual_ofdm_pwdb_all; > + /* for CCK: bits 0:4: VGA index, bits 5:7: LNA index (low) */ > + u8 cck_agc_rpt_ofdm_cfosho_a; > + /* for CCK: bit 7 is high bit of LNA index if long report type */ > + u8 cck_rpt_b_ofdm_cfosho_b; > + u8 reserved_1; > + u8 noise_power_db_msb; > + s8 path_cfotail[2]; > + u8 pcts_mask[2]; > + s8 stream_rxevm[2]; > + u8 path_rxsnr[2]; > + u8 noise_power_db_lsb; > + u8 reserved_2[3]; > + u8 stream_csi[2]; > + u8 stream_target_csi[2]; > + s8 sig_evm; > + u8 reserved_3; > + > +#ifdef __LITTLE_ENDIAN > + u8 antsel_rx_keep_2: 1; > + u8 sgi_en: 1; > + u8 rxsc: 2; > + u8 idle_long: 1; > + u8 r_ant_train_en: 1; > + u8 ant_sel_b: 1; > + u8 ant_sel: 1; > +#else /* __BIG_ENDIAN */ > + u8 ant_sel: 1; > + u8 ant_sel_b: 1; > + u8 r_ant_train_en: 1; > + u8 idle_long: 1; > + u8 rxsc: 2; > + u8 sgi_en: 1; > + u8 antsel_rx_keep_2: 1; > +#endif > +} __packed; > + > +/* 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 */ > +#define REG_IQK_RDY 0x0e90 /* is != 0 when IQK is done */ > + > +/* 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 >
Am 01.03.24 um 03:09 schrieb Ping-Ke Shih: >> -----Original Message----- >> From: Fiona Klute <fiona.klute@gmx.de> >> Sent: Wednesday, February 28, 2024 7:55 AM >> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com> >> Cc: Fiona Klute <fiona.klute@gmx.de>; kvalo@kernel.org; ulf.hansson@linaro.org; linux-mmc@vger.kernel.org; >> pavel@ucw.cz; megi@xff.cz >> Subject: [PATCH v2 4/9] wifi: rtw88: Add rtw8703b.h >> >> This is the main header for the new rtw88_8703b chip driver. >> >> Acked-by: Ping-Ke Shih <pkshih@realtek.com> >> Tested-by: Pavel Machek <pavel@ucw.cz> >> Signed-off-by: Fiona Klute <fiona.klute@gmx.de> >> --- >> drivers/net/wireless/realtek/rtw88/rtw8703b.h | 103 ++++++++++++++++++ >> 1 file changed, 103 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 00000000000..69dac101d33 >> --- /dev/null >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h >> @@ -0,0 +1,103 @@ >> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ >> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ >> + >> +#ifndef __RTW8703B_H__ >> +#define __RTW8703B_H__ >> + >> +#include <linux/types.h> >> +#include <linux/compiler_attributes.h> > > Removing these two headers can still compiled pass in my side. Please check why > you need them. If I remove them whether the code compiles depends on the order of #includes. If some other header that includes those two is included before rtw8703b.h it works, otherwise it will break. I don't think that's desirable, though other rtw88 headers already behave that way (e.g. main.h must be included before the others). Also, clangd will complain about undefined types (u8, s8), which is less important but still annoying when working on the code. So I'd prefer to keep the includes. > By the way, rtw8723d.h does > #include "rtw8723x.h" > > Can we use the same stuff? I don't think so. The vendor driver has different code paths: With 8723D it takes one for "PHYSTS_2ND_TYPE_IC" [1], with 8703B the one for "ODM_IC_11N_SERIES" a few lines below. For those "2nd type" ICs there are different structs depending on the page number (phy_sts_rpt_jgr2_type[0-2]), while the "11N series" ones always use phy_status_rpt_8192cd (all defined in [2]). However, the mainline rtlwifi driver has an equivalent struct called "phy_status_rpt" in drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.h. I don't know if sharing such definitions between different drivers is desirable. If yes, please let me know where the header should go. [1] https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.c?h=orange-pi-6.7#n3142 [2] https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.h?h=orange-pi-6.7 >> + >> +extern const struct rtw_chip_info rtw8703b_hw_spec; >> + >> +/* phy status parsing */ >> +#define VGA_BITS GENMASK(4, 0) >> +#define LNA_L_BITS GENMASK(7, 5) >> +#define LNA_H_BIT BIT(7) >> +/* masks for assembling LNA index from high and low bits */ >> +#define BIT_LNA_H_MASK BIT(3) >> +#define BIT_LNA_L_MASK GENMASK(2, 0) >> + >> +struct phy_rx_agc_info { >> +#ifdef __LITTLE_ENDIAN >> + u8 gain: 7; >> + u8 trsw: 1; >> +#else >> + u8 trsw: 1; >> + u8 gain: 7; >> +#endif >> +} __packed; > > This struct is quite simple, or I will suggest to define MASK and access them > by u8_get_bits(), like > > #define RX_AGC_GAIN GENMASK(6, 0) > gain = u8_get_bits(RX_AGC_GAIN, raw); > > Then, > struct phy_status_8703b { > u8 path_agc[2]; > ... > > Just for reference. You can decide if changing to this style. > >> + >> +/* This struct is called phy_status_rpt_8192cd in the vendor driver, >> + * there might be potential to share it with drivers for other chips >> + * of the same generation. >> + */ >> +struct phy_status_8703b { >> + struct phy_rx_agc_info path_agc[2]; >> + u8 ch_corr[2]; >> + u8 cck_sig_qual_ofdm_pwdb_all; >> + /* for CCK: bits 0:4: VGA index, bits 5:7: LNA index (low) */ >> + u8 cck_agc_rpt_ofdm_cfosho_a; >> + /* for CCK: bit 7 is high bit of LNA index if long report type */ >> + u8 cck_rpt_b_ofdm_cfosho_b; >> + u8 reserved_1; >> + u8 noise_power_db_msb; >> + s8 path_cfotail[2]; >> + u8 pcts_mask[2]; >> + s8 stream_rxevm[2]; >> + u8 path_rxsnr[2]; >> + u8 noise_power_db_lsb; >> + u8 reserved_2[3]; >> + u8 stream_csi[2]; >> + u8 stream_target_csi[2]; >> + s8 sig_evm; >> + u8 reserved_3; >> + >> +#ifdef __LITTLE_ENDIAN >> + u8 antsel_rx_keep_2: 1; >> + u8 sgi_en: 1; >> + u8 rxsc: 2; >> + u8 idle_long: 1; >> + u8 r_ant_train_en: 1; >> + u8 ant_sel_b: 1; >> + u8 ant_sel: 1; >> +#else /* __BIG_ENDIAN */ >> + u8 ant_sel: 1; >> + u8 ant_sel_b: 1; >> + u8 r_ant_train_en: 1; >> + u8 idle_long: 1; >> + u8 rxsc: 2; >> + u8 sgi_en: 1; >> + u8 antsel_rx_keep_2: 1; >> +#endif >> +} __packed; >> + >> +/* 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 */ >> +#define REG_IQK_RDY 0x0e90 /* is != 0 when IQK is done */ >> + >> +/* 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 >> >
On Fri, 2024-03-01 at 17:35 +0100, Fiona Klute wrote: > > Am 01.03.24 um 03:09 schrieb Ping-Ke Shih: > > > +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h > > > @@ -0,0 +1,103 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ > > > +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ > > > + > > > +#ifndef __RTW8703B_H__ > > > +#define __RTW8703B_H__ > > > + > > > +#include <linux/types.h> > > > +#include <linux/compiler_attributes.h> > > > > Removing these two headers can still compiled pass in my side. Please check why > > you need them. > > If I remove them whether the code compiles depends on the order of > #includes. If some other header that includes those two is included > before rtw8703b.h it works, otherwise it will break. I don't think > that's desirable, though other rtw88 headers already behave that way > (e.g. main.h must be included before the others). Also, clangd will > complain about undefined types (u8, s8), which is less important but > still annoying when working on the code. So I'd prefer to keep the includes. > > > By the way, rtw8723d.h does > > #include "rtw8723x.h" > > > > Can we use the same stuff? > > I don't think so. The vendor driver has different code paths: With 8723D > it takes one for "PHYSTS_2ND_TYPE_IC" [1], with 8703B the one for > "ODM_IC_11N_SERIES" a few lines below. For those "2nd type" ICs there > are different structs depending on the page number > (phy_sts_rpt_jgr2_type[0-2]), while the "11N series" ones always use > phy_status_rpt_8192cd (all defined in [2]). > > However, the mainline rtlwifi driver has an equivalent struct called > "phy_status_rpt" in > drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.h. I don't know if > sharing such definitions between different drivers is desirable. If yes, > please let me know where the header should go. > > [1] > https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.c?h=orange-pi-6.7#n3142 > [2] > https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.h?h=orange-pi-6.7 > Sorry for confusing. I meant to remove #include <linux/types.h> #include <linux/compiler_attributes.h> and to add #include "rtw8723x.h" like rtw8723d.h does after this patchset, not reference to vendor driver. Ping-Ke
Am 02.03.24 um 01:33 schrieb Ping-Ke Shih: > On Fri, 2024-03-01 at 17:35 +0100, Fiona Klute wrote: >> >> Am 01.03.24 um 03:09 schrieb Ping-Ke Shih: >>>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h >>>> @@ -0,0 +1,103 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ >>>> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ >>>> + >>>> +#ifndef __RTW8703B_H__ >>>> +#define __RTW8703B_H__ >>>> + >>>> +#include <linux/types.h> >>>> +#include <linux/compiler_attributes.h> >>> >>> Removing these two headers can still compiled pass in my side. Please check why >>> you need them. >> >> If I remove them whether the code compiles depends on the order of >> #includes. If some other header that includes those two is included >> before rtw8703b.h it works, otherwise it will break. I don't think >> that's desirable, though other rtw88 headers already behave that way >> (e.g. main.h must be included before the others). Also, clangd will >> complain about undefined types (u8, s8), which is less important but >> still annoying when working on the code. So I'd prefer to keep the includes. >> >>> By the way, rtw8723d.h does >>> #include "rtw8723x.h" >>> >>> Can we use the same stuff? >> >> I don't think so. The vendor driver has different code paths: With 8723D >> it takes one for "PHYSTS_2ND_TYPE_IC" [1], with 8703B the one for >> "ODM_IC_11N_SERIES" a few lines below. For those "2nd type" ICs there >> are different structs depending on the page number >> (phy_sts_rpt_jgr2_type[0-2]), while the "11N series" ones always use >> phy_status_rpt_8192cd (all defined in [2]). >> >> However, the mainline rtlwifi driver has an equivalent struct called >> "phy_status_rpt" in >> drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.h. I don't know if >> sharing such definitions between different drivers is desirable. If yes, >> please let me know where the header should go. >> >> [1] >> https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.c?h=orange-pi-6.7#n3142 >> [2] >> https://xff.cz/git/linux/tree/drivers/staging/rtl8723cs/hal/phydm/phydm_phystatus.h?h=orange-pi-6.7 >> > > Sorry for confusing. I meant to remove > #include <linux/types.h> > #include <linux/compiler_attributes.h> > and to add > #include "rtw8723x.h" > like rtw8723d.h does after this patchset, not reference to vendor driver. Oh yes, that makes sense. Thanks for the clarification, I'll do that. Best regards, Fiona
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.h b/drivers/net/wireless/realtek/rtw88/rtw8703b.h new file mode 100644 index 00000000000..69dac101d33 --- /dev/null +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ +/* Copyright Fiona Klute <fiona.klute@gmx.de> */ + +#ifndef __RTW8703B_H__ +#define __RTW8703B_H__ + +#include <linux/types.h> +#include <linux/compiler_attributes.h> + +extern const struct rtw_chip_info rtw8703b_hw_spec; + +/* phy status parsing */ +#define VGA_BITS GENMASK(4, 0) +#define LNA_L_BITS GENMASK(7, 5) +#define LNA_H_BIT BIT(7) +/* masks for assembling LNA index from high and low bits */ +#define BIT_LNA_H_MASK BIT(3) +#define BIT_LNA_L_MASK GENMASK(2, 0) + +struct phy_rx_agc_info { +#ifdef __LITTLE_ENDIAN + u8 gain: 7; + u8 trsw: 1; +#else + u8 trsw: 1; + u8 gain: 7; +#endif +} __packed; + +/* This struct is called phy_status_rpt_8192cd in the vendor driver, + * there might be potential to share it with drivers for other chips + * of the same generation. + */ +struct phy_status_8703b { + struct phy_rx_agc_info path_agc[2]; + u8 ch_corr[2]; + u8 cck_sig_qual_ofdm_pwdb_all; + /* for CCK: bits 0:4: VGA index, bits 5:7: LNA index (low) */ + u8 cck_agc_rpt_ofdm_cfosho_a; + /* for CCK: bit 7 is high bit of LNA index if long report type */ + u8 cck_rpt_b_ofdm_cfosho_b; + u8 reserved_1; + u8 noise_power_db_msb; + s8 path_cfotail[2]; + u8 pcts_mask[2]; + s8 stream_rxevm[2]; + u8 path_rxsnr[2]; + u8 noise_power_db_lsb; + u8 reserved_2[3]; + u8 stream_csi[2]; + u8 stream_target_csi[2]; + s8 sig_evm; + u8 reserved_3; + +#ifdef __LITTLE_ENDIAN + u8 antsel_rx_keep_2: 1; + u8 sgi_en: 1; + u8 rxsc: 2; + u8 idle_long: 1; + u8 r_ant_train_en: 1; + u8 ant_sel_b: 1; + u8 ant_sel: 1; +#else /* __BIG_ENDIAN */ + u8 ant_sel: 1; + u8 ant_sel_b: 1; + u8 r_ant_train_en: 1; + u8 idle_long: 1; + u8 rxsc: 2; + u8 sgi_en: 1; + u8 antsel_rx_keep_2: 1; +#endif +} __packed; + +/* 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 */ +#define REG_IQK_RDY 0x0e90 /* is != 0 when IQK is done */ + +/* 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__ */