-----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/halbtc8821c1an... and https://github.com/morrownr/8821cu-20210916/blob/main/hal/btc/halbtc8821c2an...
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 2That 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