On Fri, Apr 14, 2023 at 02:05:44AM +0000, Ping-Ke Shih wrote: > > > -----Original Message----- > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > Sent: Tuesday, April 11, 2023 6:26 PM > > To: Ping-Ke Shih <pkshih@xxxxxxxxxxx> > > Cc: linux-wireless <linux-wireless@xxxxxxxxxxxxxxx>; Hans Ulli Kroll <linux@xxxxxxxxxxxxx>; Larry Finger > > <Larry.Finger@xxxxxxxxxxxx>; Tim K <tpkuester@xxxxxxxxx>; Alex G . <mr.nuke.me@xxxxxxxxx>; Nick Morrow > > <morrownr@xxxxxxxxx>; Viktor Petrenko <g0000ga@xxxxxxxxx>; Andreas Henriksson <andreas@xxxxxxxx>; > > ValdikSS <iam@xxxxxxxxxxxxxxx>; kernel@xxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxx> > > > > Sent: Tuesday, April 4, 2023 3:25 PM > > > > To: linux-wireless <linux-wireless@xxxxxxxxxxxxxxx> > > > > Cc: Hans Ulli Kroll <linux@xxxxxxxxxxxxx>; Larry Finger <Larry.Finger@xxxxxxxxxxxx>; Ping-Ke Shih > > > > <pkshih@xxxxxxxxxxx>; Tim K <tpkuester@xxxxxxxxx>; Alex G . <mr.nuke.me@xxxxxxxxx>; Nick Morrow > > > > <morrownr@xxxxxxxxx>; Viktor Petrenko <g0000ga@xxxxxxxxx>; Andreas Henriksson <andreas@xxxxxxxx>; > > > > ValdikSS <iam@xxxxxxxxxxxxxxx>; kernel@xxxxxxxxxxxxxx; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; > > > > stable@xxxxxxxxxxxxxxx > > > > 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 -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
