diff mbox series

[v2,2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width

Message ID 20230404072508.578056-3-s.hauer@pengutronix.de
State Superseded
Headers show
Series None | expand

Commit Message

Sascha Hauer April 4, 2023, 7:25 a.m. UTC 
On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
downstream driver suggests that the field width of rfe_option is 5 bit,
so rfe_option should be masked with 0x1f.

Without this the rfe_option comparisons with 2 further down the
driver evaluate as false when they should really evaluate as true.
The effect is that 2G channels do not work.

rfe_option is also used as an array index into rtw8821c_rfe_defs[].
rtw8821c_rfe_defs[34] (0x22) was added as part of adding USB support,
likely because rfe_option reads as 0x22. As this now becomes 0x2,
rtw8821c_rfe_defs[34] is no longer used and can be removed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: ValdikSS <iam@valdikss.org.ru>
Tested-by: Alexandru gagniuc <mr.nuke.me@gmail.com>
Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/realtek/rtw88/rtw8821c.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Ping-Ke Shih April 6, 2023, 1:54 a.m. UTC | #1 
> -----Original Message-----
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, April 4, 2023 3:25 PM
> To: linux-wireless <linux-wireless@vger.kernel.org>
> Cc: Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger <Larry.Finger@lwfinger.net>; Ping-Ke Shih
> <pkshih@realtek.com>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de>;
> stable@vger.kernel.org
> Subject: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> 
> On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
> downstream driver suggests that the field width of rfe_option is 5 bit,
> so rfe_option should be masked with 0x1f.

I don't aware of this. Could you point where you get it?

As I check it internally, 0x22 is expected, so I suggest to have 0x22 entry
as below

-       [34] = RTW_DEF_RFE(8821c, 0, 0),
+       [34] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),  // copy from type 2

> 
> Without this the rfe_option comparisons with 2 further down the
> driver evaluate as false when they should really evaluate as true.
> The effect is that 2G channels do not work.
> 
> rfe_option is also used as an array index into rtw8821c_rfe_defs[].
> rtw8821c_rfe_defs[34] (0x22) was added as part of adding USB support,
> likely because rfe_option reads as 0x22. As this now becomes 0x2,
> rtw8821c_rfe_defs[34] is no longer used and can be removed.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: ValdikSS <iam@valdikss.org.ru>
> Tested-by: Alexandru gagniuc <mr.nuke.me@gmail.com>
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/wireless/realtek/rtw88/rtw8821c.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> index 17f800f6efbd0..67efa58dd78ee 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> @@ -47,7 +47,7 @@ static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
> 
>         map = (struct rtw8821c_efuse *)log_map;
> 
> -       efuse->rfe_option = map->rfe_option;
> +       efuse->rfe_option = map->rfe_option & 0x1f;
>         efuse->rf_board_option = map->rf_board_option;
>         efuse->crystal_cap = map->xtal_k;
>         efuse->pa_type_2g = map->pa_type;
> @@ -1537,7 +1537,6 @@ static const struct rtw_rfe_def rtw8821c_rfe_defs[] = {
>         [2] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
>         [4] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
>         [6] = RTW_DEF_RFE(8821c, 0, 0),
> -       [34] = RTW_DEF_RFE(8821c, 0, 0),
>  };
> 
>  static struct rtw_hw_reg rtw8821c_dig[] = {
> --
> 2.39.2
Sascha Hauer April 11, 2023, 10:26 a.m. UTC | #2 
On Thu, Apr 06, 2023 at 01:54:55AM +0000, Ping-Ke Shih wrote:
> 
> 
> > -----Original Message-----
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, April 4, 2023 3:25 PM
> > To: linux-wireless <linux-wireless@vger.kernel.org>
> > Cc: Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger <Larry.Finger@lwfinger.net>; Ping-Ke Shih
> > <pkshih@realtek.com>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> > <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> > ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de>;
> > stable@vger.kernel.org
> > Subject: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> > 
> > On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
> > downstream driver suggests that the field width of rfe_option is 5 bit,
> > so rfe_option should be masked with 0x1f.
> 
> I don't aware of this. Could you point where you get it?

See
https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c1ant.c#L2480
and
https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c2ant.c#L2519

But I now see that this masked value is only used at the places I
pointed to, there are other places in the driver that use the unmasked
value.

> 
> As I check it internally, 0x22 is expected, so I suggest to have 0x22 entry
> as below
> 
> -       [34] = RTW_DEF_RFE(8821c, 0, 0),
> +       [34] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),  // copy from type 2

That alone is not enough. There are other places in rtw8821c.c that
compare with rfe_option. See below for a patch with annotations where to
find the corresponding code in the downstream driver. Note how BIT(5) is
irrelevant for all decisions. I can't tell of course if that's just by
chance or by intent.

I don't know where to go from here. It looks like we really only want to
make a decision between SWITCH_TO_WLG and SWITCH_TO_BTG at most places,
so it might be better to store a flag somewhere rather than having the
big switch/case in multiple places.

Sascha

-------------------------------8<---------------------------------

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 17f800f6efbd0..5da7787cea129 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -317,11 +317,32 @@ static void rtw8821c_set_channel_rf(struct rtw_dev *rtwdev, u8 channel, u8 bw)
 	}
 
 	if (channel <= 14) {
-		if (rtwdev->efuse.rfe_option == 0)
-			rtw8821c_switch_rf_set(rtwdev, SWITCH_TO_WLG);
-		else if (rtwdev->efuse.rfe_option == 2 ||
-			 rtwdev->efuse.rfe_option == 4)
+		/*
+		 * see:
+		 * https://github.com/morrownr/8821cu-20210916/blob/main/hal/phydm/rtl8821c/phydm_hal_api8821c.c#L338
+		 */
+		switch (rtwdev->efuse.rfe_option) {
+		case 0x02: case 0x22:
+		case 0x04: case 0x24:
+		case 0x07: case 0x27:
+		case 0x2a:
+		case 0x2c:
+		case 0x2f:
 			rtw8821c_switch_rf_set(rtwdev, SWITCH_TO_BTG);
+			break;
+		case 0x00: case 0x20:
+		case 0x01: case 0x21:
+		case 0x03: case 0x23:
+		case 0x05: case 0x25:
+		case 0x06: case 0x26:
+		case 0x28:
+		case 0x29:
+		case 0x2b:
+		case 0x2d:
+		case 0x2e:
+		default:
+			rtw8821c_switch_rf_set(rtwdev, SWITCH_TO_WLG);
+		}
 		rtw_write_rf(rtwdev, RF_PATH_A, RF_LUTDBG, BIT(6), 0x1);
 		rtw_write_rf(rtwdev, RF_PATH_A, 0x64, 0xf, 0xf);
 	} else {
@@ -501,12 +522,35 @@ static s8 get_cck_rx_pwr(struct rtw_dev *rtwdev, u8 lna_idx, u8 vga_idx)
 	s8 rx_pwr_all = 0;
 	s8 lna_gain = 0;
 
-	if (efuse->rfe_option == 0) {
-		lna_gain_table = lna_gain_table_0;
-		lna_gain_table_size = ARRAY_SIZE(lna_gain_table_0);
-	} else {
+	/*
+	 * see:
+	 * https://github.com/morrownr/8821cu-20210916/blob/main/hal/phydm/rtl8821c/phydm_hal_api8821c.c#L52
+	 * https://github.com/morrownr/8821cu-20210916/blob/main/hal/phydm/phydm.c#L178
+	 */
+	switch (rtwdev->efuse.rfe_option) {
+	case 0x02: case 0x22:
+	case 0x04: case 0x24:
+	case 0x07: case 0x27:
+	case 0x2a:
+	case 0x2c:
+	case 0x2f:
 		lna_gain_table = lna_gain_table_1;
 		lna_gain_table_size = ARRAY_SIZE(lna_gain_table_1);
+		break;
+	case 0x00: case 0x20:
+	case 0x01: case 0x21:
+	case 0x03: case 0x23:
+	case 0x05: case 0x25:
+	case 0x06: case 0x26:
+	case 0x28:
+	case 0x29:
+	case 0x2b:
+	case 0x2d:
+	case 0x2e:
+	default:
+		lna_gain_table = lna_gain_table_0;
+		lna_gain_table_size = ARRAY_SIZE(lna_gain_table_0);
+		break;
 	}
 
 	if (lna_idx >= lna_gain_table_size) {
@@ -821,6 +865,9 @@ static void rtw8821c_coex_cfg_ant_switch(struct rtw_dev *rtwdev, u8 ctrl_type,
 				DPDT_CTRL_PIN);
 
 		if (pos_type == COEX_SWITCH_TO_WLG_BT) {
+			/*
+			 * What here? Cannot find refval = 0x3 in downstream driver
+			 */
 			if (coex_rfe->rfe_module_type != 0x4 &&
 			    coex_rfe->rfe_module_type != 0x2)
 				regval = 0x3;
@@ -902,7 +949,12 @@ static void rtw8821c_coex_cfg_rfe_type(struct rtw_dev *rtwdev)
 	coex_rfe->ant_switch_exist = true;
 	coex_rfe->wlg_at_btg = false;
 
-	switch (coex_rfe->rfe_module_type) {
+	/*
+	 * see:
+	 * https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c1ant.c#L2480
+	 * https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c2ant.c#L2519
+	 */
+	switch (coex_rfe->rfe_module_type & 0x1f) {
 	case 0:
 	case 8:
 	case 1:
@@ -1533,11 +1585,30 @@ static const struct rtw_intf_phy_para_table phy_para_table_8821c = {
 };
 
 static const struct rtw_rfe_def rtw8821c_rfe_defs[] = {
-	[0] = RTW_DEF_RFE(8821c, 0, 0),
-	[2] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
-	[4] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
-	[6] = RTW_DEF_RFE(8821c, 0, 0),
-	[34] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x00] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x01] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x02] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x03] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x04] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x05] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x06] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x07] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x20] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x21] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x22] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x23] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x24] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x25] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x26] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x27] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x28] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x29] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x2a] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x2b] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x2c] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
+	[0x2d] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x2e] = RTW_DEF_RFE(8821c, 0, 0),
+	[0x2f] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
 };
 
 static struct rtw_hw_reg rtw8821c_dig[] = {
Ping-Ke Shih April 14, 2023, 2:05 a.m. UTC | #3 
> -----Original Message-----
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Tuesday, April 11, 2023 6:26 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: linux-wireless <linux-wireless@vger.kernel.org>; Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger
> <Larry.Finger@lwfinger.net>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; stable@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> 
> On Thu, Apr 06, 2023 at 01:54:55AM +0000, Ping-Ke Shih wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > Sent: Tuesday, April 4, 2023 3:25 PM
> > > To: linux-wireless <linux-wireless@vger.kernel.org>
> > > Cc: Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger <Larry.Finger@lwfinger.net>; Ping-Ke Shih
> > > <pkshih@realtek.com>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> > > <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> > > ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de>;
> > > stable@vger.kernel.org
> > > Subject: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> > >
> > > On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
> > > downstream driver suggests that the field width of rfe_option is 5 bit,
> > > so rfe_option should be masked with 0x1f.
> >
> > I don't aware of this. Could you point where you get it?
> 
> See
> https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c1ant.c#L2480
> and
> https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c2ant.c#L2519
> 
> But I now see that this masked value is only used at the places I
> pointed to, there are other places in the driver that use the unmasked
> value.

After I read vendor driver, there are three variety of rfe_option for 8821C.
1. raw value from efuse
   hal->rfe_type = map[EEPROM_RFE_OPTION_8821C];

2. BT-coexistence 
   rfe_type->rfe_module_type = board_info->rfe_type & 0x1f;

3. PHY
   dm->rfe_type_expand = hal->rfe_type = raw value
   dm->rfe_type = dm->rfe_type_expand >> 3;


For rtw88, there are only two variety, but they are identical
   coex_rfe->rfe_module_type = efuse->rfe_option;

The flaws are rfe_type->rfe_module_type of item 2 and dm->rfe_type of item 3
above, and most things are addressed by your draft patch. Exception is
check_positive() check dm->rfe_type, but we don't have this conversion in
rtw88 (i.e. cond.rfe = efuse->rfe_option; in rtw_phy_setup_phy_cond()).

Since I don't have a hardware with rfe_option larger than 8, could you
please give below patch a try?

--- a/phy.c
+++ b/phy.c
@@ -1048,6 +1048,9 @@ void rtw_phy_setup_phy_cond(struct rtw_dev *rtwdev, u32 pkg)
        cond.plat = 0x04;
        cond.rfe = efuse->rfe_option;

+       if (rtwdev->chip->id == RTW_CHIP_TYPE_8821C)
+               cond.rfe = efuse->rfe_option >> 3;
+
        switch (rtw_hci_type(rtwdev)) {
        case RTW_HCI_TYPE_USB:
                cond.intf = INTF_USB;


8821C is more complex than others, and I'm not familiar with it, so maybe I
could miss something. Please correct me if any.

> 
> >
> > As I check it internally, 0x22 is expected, so I suggest to have 0x22 entry
> > as below
> >
> > -       [34] = RTW_DEF_RFE(8821c, 0, 0),
> > +       [34] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),  // copy from type 2
> 
> That alone is not enough. There are other places in rtw8821c.c that
> compare with rfe_option. See below for a patch with annotations where to
> find the corresponding code in the downstream driver. Note how BIT(5) is
> irrelevant for all decisions. I can't tell of course if that's just by
> chance or by intent.

You're right. I miss these points.

> 
> I don't know where to go from here. It looks like we really only want to
> make a decision between SWITCH_TO_WLG and SWITCH_TO_BTG at most places,
> so it might be better to store a flag somewhere rather than having the
> big switch/case in multiple places.
> 

Agreed. Add something like:

--- a/main.h
+++ b/main.h
@@ -2076,6 +2076,7 @@ struct rtw_hal {
        u8 mp_chip;
        u8 oem_id;
        struct rtw_phy_cond phy_cond;
+       bool rfe_btg;

        u8 ps_mode;
        u8 current_channel;

--- a/rtw8821c.c
+++ b/rtw8821c.c
@@ -47,6 +47,7 @@ enum rtw8821ce_rf_set {

 static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
 {
+       struct rtw_hal *hal = &rtwdev->hal;
        struct rtw_efuse *efuse = &rtwdev->efuse;
        struct rtw8821c_efuse *map;
        int i;
@@ -91,6 +92,12 @@ static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
                return -ENOTSUPP;
        }

+       switch (efuse->rfe_option) {
+       case 0x02: case 0x22: // ...
+               hal->rfe_btg = true;
+               break;
+       }
+
        return 0;
 }


[...]

>  static const struct rtw_rfe_def rtw8821c_rfe_defs[] = {
> -       [0] = RTW_DEF_RFE(8821c, 0, 0),
> -       [2] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> -       [4] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> -       [6] = RTW_DEF_RFE(8821c, 0, 0),
> -       [34] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x00] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x01] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x02] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x03] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x04] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x05] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x06] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x07] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x20] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x21] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x22] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x23] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x24] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x25] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x26] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x27] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x28] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x29] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x2a] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x2b] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x2c] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
> +       [0x2d] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x2e] = RTW_DEF_RFE(8821c, 0, 0),
> +       [0x2f] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),

I'm not sure if we add all of them, since some aren't tested, but maybe it would
be better than nothing. 

Ping-Ke
Sascha Hauer April 17, 2023, 12:22 p.m. UTC | #4 
On Fri, Apr 14, 2023 at 02:05:44AM +0000, Ping-Ke Shih wrote:
> 
> > -----Original Message-----
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, April 11, 2023 6:26 PM
> > To: Ping-Ke Shih <pkshih@realtek.com>
> > Cc: linux-wireless <linux-wireless@vger.kernel.org>; Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger
> > <Larry.Finger@lwfinger.net>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> > <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> > ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> > 
> > On Thu, Apr 06, 2023 at 01:54:55AM +0000, Ping-Ke Shih wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Sent: Tuesday, April 4, 2023 3:25 PM
> > > > To: linux-wireless <linux-wireless@vger.kernel.org>
> > > > Cc: Hans Ulli Kroll <linux@ulli-kroll.de>; Larry Finger <Larry.Finger@lwfinger.net>; Ping-Ke Shih
> > > > <pkshih@realtek.com>; Tim K <tpkuester@gmail.com>; Alex G . <mr.nuke.me@gmail.com>; Nick Morrow
> > > > <morrownr@gmail.com>; Viktor Petrenko <g0000ga@gmail.com>; Andreas Henriksson <andreas@fatal.se>;
> > > > ValdikSS <iam@valdikss.org.ru>; kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de>;
> > > > stable@vger.kernel.org
> > > > Subject: [PATCH v2 2/2] wifi: rtw88: rtw8821c: Fix rfe_option field width
> > > >
> > > > On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
> > > > downstream driver suggests that the field width of rfe_option is 5 bit,
> > > > so rfe_option should be masked with 0x1f.
> > >
> > > I don't aware of this. Could you point where you get it?
> > 
> > See
> > https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c1ant.c#L2480
> > and
> > https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c2ant.c#L2519
> > 
> > But I now see that this masked value is only used at the places I
> > pointed to, there are other places in the driver that use the unmasked
> > value.
> 
> After I read vendor driver, there are three variety of rfe_option for 8821C.
> 1. raw value from efuse
>    hal->rfe_type = map[EEPROM_RFE_OPTION_8821C];
> 
> 2. BT-coexistence 
>    rfe_type->rfe_module_type = board_info->rfe_type & 0x1f;
> 
> 3. PHY
>    dm->rfe_type_expand = hal->rfe_type = raw value
>    dm->rfe_type = dm->rfe_type_expand >> 3;
> 
> 
> For rtw88, there are only two variety, but they are identical
>    coex_rfe->rfe_module_type = efuse->rfe_option;
> 
> The flaws are rfe_type->rfe_module_type of item 2 and dm->rfe_type of item 3
> above, and most things are addressed by your draft patch. Exception is
> check_positive() check dm->rfe_type, but we don't have this conversion in
> rtw88 (i.e. cond.rfe = efuse->rfe_option; in rtw_phy_setup_phy_cond()).
> 
> Since I don't have a hardware with rfe_option larger than 8, could you
> please give below patch a try?
> 
> --- a/phy.c
> +++ b/phy.c
> @@ -1048,6 +1048,9 @@ void rtw_phy_setup_phy_cond(struct rtw_dev *rtwdev, u32 pkg)
>         cond.plat = 0x04;
>         cond.rfe = efuse->rfe_option;
> 
> +       if (rtwdev->chip->id == RTW_CHIP_TYPE_8821C)
> +               cond.rfe = efuse->rfe_option >> 3;
> +
>         switch (rtw_hci_type(rtwdev)) {
>         case RTW_HCI_TYPE_USB:
>                 cond.intf = INTF_USB;

This change doesn't make any difference. cond.rfe is only used in
check_positive() which is implemented like this:

static bool check_positive(struct rtw_dev *rtwdev, struct rtw_phy_cond cond)
{
	struct rtw_hal *hal = &rtwdev->hal;
	struct rtw_phy_cond drv_cond = hal->phy_cond;

	if (cond.cut && cond.cut != drv_cond.cut)
		return false;

	if (cond.pkg && cond.pkg != drv_cond.pkg)
		return false;

	if (cond.intf && cond.intf != drv_cond.intf)
		return false;

	if (cond.rfe != drv_cond.rfe)
		return false;

	return true;
}

In my case check_positive() always returns early when comparing cond.pkg which
is always set to '15' in rtw_phy_setup_phy_cond():

void rtw_phy_setup_phy_cond(struct rtw_dev *rtwdev, u32 pkg)
{
	...
	cond.pkg = pkg ? pkg : 15;
	...
}

In the upstream driver rtw_phy_setup_phy_cond() is only ever called with
'0' as pkg argument. Now in the downstream driver I found this snippet:

void phydm_init_hw_info_by_rfe_type_8821c(struct dm_struct *dm)
{
	...

        if (dm->rfe_type_expand == 2 || dm->rfe_type_expand == 4 || dm->rfe_type_expand == 7) {
                dm->default_rf_set_8821c = SWITCH_TO_BTG;
        } else if (dm->rfe_type_expand == 0 || dm->rfe_type_expand == 1 ||
                   dm->rfe_type_expand == 3 || dm->rfe_type_expand == 5 ||
                   dm->rfe_type_expand == 6) {
                dm->default_rf_set_8821c = SWITCH_TO_WLG;
        } else if (dm->rfe_type_expand == 0x22 || dm->rfe_type_expand == 0x24 ||
                   dm->rfe_type_expand == 0x27 || dm->rfe_type_expand == 0x2a ||
                   dm->rfe_type_expand == 0x2c || dm->rfe_type_expand == 0x2f) {
                dm->default_rf_set_8821c = SWITCH_TO_BTG;
                odm_cmn_info_init(dm, ODM_CMNINFO_PACKAGE_TYPE, 1);
        } else if (dm->rfe_type_expand == 0x20 || dm->rfe_type_expand == 0x21 ||
                   dm->rfe_type_expand == 0x23 || dm->rfe_type_expand == 0x25 ||
                   dm->rfe_type_expand == 0x26 || dm->rfe_type_expand == 0x28 ||
                   dm->rfe_type_expand == 0x29 || dm->rfe_type_expand == 0x2b ||
                   dm->rfe_type_expand == 0x2d || dm->rfe_type_expand == 0x2e) {
                dm->default_rf_set_8821c = SWITCH_TO_WLG;
                odm_cmn_info_init(dm, ODM_CMNINFO_PACKAGE_TYPE, 1);
        }

	...
}

odm_cmn_info_init(dm, ODM_CMNINFO_PACKAGE_TYPE, 1); seems to be the
analogue of the pkg type argument. This suggests that for rfe_type >=
0x20 we should call rtw_phy_setup_phy_cond() with '1' as pkg argument.
When doing this here I will end up with check_positive() returning
'true' in some cases.  However, I didn't notice any change in the driver
behaviour then.

Note how the above code snippet looks like the rfe_type is indeed
encoded in the lower 5 bits whereas BIT(5) could be used as the package
type. That could be by chance, but it's striking that rfe_type (x) always
ends up in the same code path as (x + 0x20) also in other parts of this
file.

I'm a bit lost here. I suggest that we stick with the variant I tried in
v1 of this series, but I'll add a note that there might be some
inaccuracies in how some currently unknown chip variants are handled.

Sascha
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 17f800f6efbd0..67efa58dd78ee 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -47,7 +47,7 @@  static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
 
 	map = (struct rtw8821c_efuse *)log_map;
 
-	efuse->rfe_option = map->rfe_option;
+	efuse->rfe_option = map->rfe_option & 0x1f;
 	efuse->rf_board_option = map->rf_board_option;
 	efuse->crystal_cap = map->xtal_k;
 	efuse->pa_type_2g = map->pa_type;
@@ -1537,7 +1537,6 @@  static const struct rtw_rfe_def rtw8821c_rfe_defs[] = {
 	[2] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
 	[4] = RTW_DEF_RFE_EXT(8821c, 0, 0, 2),
 	[6] = RTW_DEF_RFE(8821c, 0, 0),
-	[34] = RTW_DEF_RFE(8821c, 0, 0),
 };
 
 static struct rtw_hw_reg rtw8821c_dig[] = {