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/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.
raw value from efuse hal->rfe_type = map[EEPROM_RFE_OPTION_8821C];
BT-coexistence rfe_type->rfe_module_type = board_info->rfe_type & 0x1f;
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