> -----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; 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
