This series fixes two bugs in the RTW88 USB driver I was reported from
several people and that I also encountered myself.
The first one resulted in "timed out to flush queue 3" messages from the
driver and sometimes a complete stall of the TX queues.
The second one is specific to the RTW8821CU chipset. Here 2GHz networks
were hardly seen and impossible to connect to. This goes down to
misinterpreting the rfe_option field in the efuse.
Changes since v1:
- Add Cc: [email protected] tag to the patches rather than Cc'ing
stable@ directly
- Add Tested-by: tags
Sascha Hauer (2):
wifi: rtw88: usb: fix priority queue to endpoint mapping
wifi: rtw88: rtw8821c: Fix rfe_option field width
drivers/net/wireless/realtek/rtw88/rtw8821c.c | 3 +-
drivers/net/wireless/realtek/rtw88/usb.c | 70 +++++++++++++------
2 files changed, 48 insertions(+), 25 deletions(-)
--
2.39.2
The RTW88 chipsets have four different priority queues in hardware. For
the USB type chipsets the packets destined for a specific priority queue
must be sent through the endpoint corresponding to the queue. This was
not fully understood when porting from the RTW88 USB out of tree driver
and thus violated.
This patch implements the qsel to endpoint mapping as in
get_usb_bulkout_id_88xx() in the downstream driver.
Without this the driver often issues "timed out to flush queue 3"
warnings and often TX stalls completely.
Signed-off-by: Sascha Hauer <[email protected]>
Tested-by: ValdikSS <[email protected]>
Tested-by: Alexandru gagniuc <[email protected]>
Tested-by: Larry Finger <[email protected]>
Cc: [email protected]
---
drivers/net/wireless/realtek/rtw88/usb.c | 70 ++++++++++++++++--------
1 file changed, 47 insertions(+), 23 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 2a8336b1847a5..a10d6fef4ffaf 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -118,6 +118,22 @@ static void rtw_usb_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
rtw_usb_write(rtwdev, addr, val, 4);
}
+static int dma_mapping_to_ep(enum rtw_dma_mapping dma_mapping)
+{
+ switch (dma_mapping) {
+ case RTW_DMA_MAPPING_HIGH:
+ return 0;
+ case RTW_DMA_MAPPING_NORMAL:
+ return 1;
+ case RTW_DMA_MAPPING_LOW:
+ return 2;
+ case RTW_DMA_MAPPING_EXTRA:
+ return 3;
+ default:
+ return -EINVAL;
+ }
+}
+
static int rtw_usb_parse(struct rtw_dev *rtwdev,
struct usb_interface *interface)
{
@@ -129,6 +145,8 @@ static int rtw_usb_parse(struct rtw_dev *rtwdev,
int num_out_pipes = 0;
int i;
u8 num;
+ const struct rtw_chip_info *chip = rtwdev->chip;
+ const struct rtw_rqpn *rqpn;
for (i = 0; i < interface_desc->bNumEndpoints; i++) {
endpoint = &host_interface->endpoint[i].desc;
@@ -183,31 +201,34 @@ static int rtw_usb_parse(struct rtw_dev *rtwdev,
rtwdev->hci.bulkout_num = num_out_pipes;
- switch (num_out_pipes) {
- case 4:
- case 3:
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = 2;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = 2;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = 2;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = 2;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID4] = 1;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID5] = 1;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID6] = 0;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID7] = 0;
- break;
- case 2:
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = 1;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = 1;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = 1;
- rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = 1;
- break;
- case 1:
- break;
- default:
- rtw_err(rtwdev, "failed to get out_pipes(%d)\n", num_out_pipes);
+ if (num_out_pipes < 1 || num_out_pipes > 4) {
+ rtw_err(rtwdev, "invalid number of endpoints %d\n", num_out_pipes);
return -EINVAL;
}
+ rqpn = &chip->rqpn_table[num_out_pipes];
+
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = dma_mapping_to_ep(rqpn->dma_map_be);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = dma_mapping_to_ep(rqpn->dma_map_bk);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = dma_mapping_to_ep(rqpn->dma_map_bk);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = dma_mapping_to_ep(rqpn->dma_map_be);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID4] = dma_mapping_to_ep(rqpn->dma_map_vi);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID5] = dma_mapping_to_ep(rqpn->dma_map_vi);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID6] = dma_mapping_to_ep(rqpn->dma_map_vo);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID7] = dma_mapping_to_ep(rqpn->dma_map_vo);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID8] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID9] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID10] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID11] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID12] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID13] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID14] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_TID15] = -EINVAL;
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_BEACON] = dma_mapping_to_ep(rqpn->dma_map_hi);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_HIGH] = dma_mapping_to_ep(rqpn->dma_map_hi);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_MGMT] = dma_mapping_to_ep(rqpn->dma_map_mg);
+ rtwusb->qsel_to_ep[TX_DESC_QSEL_H2C] = dma_mapping_to_ep(rqpn->dma_map_hi);
+
return 0;
}
@@ -250,7 +271,7 @@ static void rtw_usb_write_port_tx_complete(struct urb *urb)
static int qsel_to_ep(struct rtw_usb *rtwusb, unsigned int qsel)
{
if (qsel >= ARRAY_SIZE(rtwusb->qsel_to_ep))
- return 0;
+ return -EINVAL;
return rtwusb->qsel_to_ep[qsel];
}
@@ -265,6 +286,9 @@ static int rtw_usb_write_port(struct rtw_dev *rtwdev, u8 qsel, struct sk_buff *s
int ret;
int ep = qsel_to_ep(rtwusb, qsel);
+ if (ep < 0)
+ return ep;
+
pipe = usb_sndbulkpipe(usbd, rtwusb->out_ep[ep]);
urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!urb)
--
2.39.2
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 <[email protected]>
Tested-by: ValdikSS <[email protected]>
Tested-by: Alexandru gagniuc <[email protected]>
Tested-by: Larry Finger <[email protected]>
Cc: [email protected]
---
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
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: Tuesday, April 4, 2023 3:25 PM
> To: linux-wireless <[email protected]>
> Cc: Hans Ulli Kroll <[email protected]>; Larry Finger <[email protected]>; Ping-Ke Shih
> <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> ValdikSS <[email protected]>; [email protected]; Sascha Hauer <[email protected]>;
> [email protected]
> Subject: [PATCH v2 1/2] wifi: rtw88: usb: fix priority queue to endpoint mapping
>
> The RTW88 chipsets have four different priority queues in hardware. For
> the USB type chipsets the packets destined for a specific priority queue
> must be sent through the endpoint corresponding to the queue. This was
> not fully understood when porting from the RTW88 USB out of tree driver
> and thus violated.
>
> This patch implements the qsel to endpoint mapping as in
> get_usb_bulkout_id_88xx() in the downstream driver.
>
> Without this the driver often issues "timed out to flush queue 3"
> warnings and often TX stalls completely.
>
Add a Fixes tag?
Fixes: a82dfd33d123 ("wifi: rtw88: Add common USB chip support")
As well as second patch.
> Signed-off-by: Sascha Hauer <[email protected]>
> Tested-by: ValdikSS <[email protected]>
> Tested-by: Alexandru gagniuc <[email protected]>
> Tested-by: Larry Finger <[email protected]>
> Cc: [email protected]
Reviewed-by: Ping-Ke Shih <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/usb.c | 70 ++++++++++++++++--------
> 1 file changed, 47 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 2a8336b1847a5..a10d6fef4ffaf 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -118,6 +118,22 @@ static void rtw_usb_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
> rtw_usb_write(rtwdev, addr, val, 4);
> }
>
> +static int dma_mapping_to_ep(enum rtw_dma_mapping dma_mapping)
> +{
> + switch (dma_mapping) {
> + case RTW_DMA_MAPPING_HIGH:
> + return 0;
> + case RTW_DMA_MAPPING_NORMAL:
> + return 1;
> + case RTW_DMA_MAPPING_LOW:
> + return 2;
> + case RTW_DMA_MAPPING_EXTRA:
> + return 3;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int rtw_usb_parse(struct rtw_dev *rtwdev,
> struct usb_interface *interface)
> {
> @@ -129,6 +145,8 @@ static int rtw_usb_parse(struct rtw_dev *rtwdev,
> int num_out_pipes = 0;
> int i;
> u8 num;
> + const struct rtw_chip_info *chip = rtwdev->chip;
> + const struct rtw_rqpn *rqpn;
>
> for (i = 0; i < interface_desc->bNumEndpoints; i++) {
> endpoint = &host_interface->endpoint[i].desc;
> @@ -183,31 +201,34 @@ static int rtw_usb_parse(struct rtw_dev *rtwdev,
>
> rtwdev->hci.bulkout_num = num_out_pipes;
>
> - switch (num_out_pipes) {
> - case 4:
> - case 3:
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = 2;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = 2;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = 2;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = 2;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID4] = 1;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID5] = 1;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID6] = 0;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID7] = 0;
> - break;
> - case 2:
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = 1;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = 1;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = 1;
> - rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = 1;
> - break;
> - case 1:
> - break;
> - default:
> - rtw_err(rtwdev, "failed to get out_pipes(%d)\n", num_out_pipes);
> + if (num_out_pipes < 1 || num_out_pipes > 4) {
> + rtw_err(rtwdev, "invalid number of endpoints %d\n", num_out_pipes);
> return -EINVAL;
> }
>
> + rqpn = &chip->rqpn_table[num_out_pipes];
> +
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID0] = dma_mapping_to_ep(rqpn->dma_map_be);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID1] = dma_mapping_to_ep(rqpn->dma_map_bk);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID2] = dma_mapping_to_ep(rqpn->dma_map_bk);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID3] = dma_mapping_to_ep(rqpn->dma_map_be);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID4] = dma_mapping_to_ep(rqpn->dma_map_vi);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID5] = dma_mapping_to_ep(rqpn->dma_map_vi);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID6] = dma_mapping_to_ep(rqpn->dma_map_vo);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID7] = dma_mapping_to_ep(rqpn->dma_map_vo);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID8] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID9] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID10] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID11] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID12] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID13] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID14] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_TID15] = -EINVAL;
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_BEACON] = dma_mapping_to_ep(rqpn->dma_map_hi);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_HIGH] = dma_mapping_to_ep(rqpn->dma_map_hi);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_MGMT] = dma_mapping_to_ep(rqpn->dma_map_mg);
> + rtwusb->qsel_to_ep[TX_DESC_QSEL_H2C] = dma_mapping_to_ep(rqpn->dma_map_hi);
> +
> return 0;
> }
>
> @@ -250,7 +271,7 @@ static void rtw_usb_write_port_tx_complete(struct urb *urb)
> static int qsel_to_ep(struct rtw_usb *rtwusb, unsigned int qsel)
> {
> if (qsel >= ARRAY_SIZE(rtwusb->qsel_to_ep))
> - return 0;
> + return -EINVAL;
>
> return rtwusb->qsel_to_ep[qsel];
> }
> @@ -265,6 +286,9 @@ static int rtw_usb_write_port(struct rtw_dev *rtwdev, u8 qsel, struct sk_buff *s
> int ret;
> int ep = qsel_to_ep(rtwusb, qsel);
>
> + if (ep < 0)
> + return ep;
> +
> pipe = usb_sndbulkpipe(usbd, rtwusb->out_ep[ep]);
> urb = usb_alloc_urb(0, GFP_ATOMIC);
> if (!urb)
> --
> 2.39.2
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: Tuesday, April 4, 2023 3:25 PM
> To: linux-wireless <[email protected]>
> Cc: Hans Ulli Kroll <[email protected]>; Larry Finger <[email protected]>; Ping-Ke Shih
> <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> ValdikSS <[email protected]>; [email protected]; Sascha Hauer <[email protected]>;
> [email protected]
> 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 <[email protected]>
> Tested-by: ValdikSS <[email protected]>
> Tested-by: Alexandru gagniuc <[email protected]>
> Tested-by: Larry Finger <[email protected]>
> Cc: [email protected]
> ---
> 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
On Thu, Apr 06, 2023 at 01:54:55AM +0000, Ping-Ke Shih wrote:
>
>
> > -----Original Message-----
> > From: Sascha Hauer <[email protected]>
> > Sent: Tuesday, April 4, 2023 3:25 PM
> > To: linux-wireless <[email protected]>
> > Cc: Hans Ulli Kroll <[email protected]>; Larry Finger <[email protected]>; Ping-Ke Shih
> > <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> > <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> > ValdikSS <[email protected]>; [email protected]; Sascha Hauer <[email protected]>;
> > [email protected]
> > 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[] = {
--
2.39.2
--
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 |
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: Tuesday, April 11, 2023 6:26 PM
> To: Ping-Ke Shih <[email protected]>
> Cc: linux-wireless <[email protected]>; Hans Ulli Kroll <[email protected]>; Larry Finger
> <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> ValdikSS <[email protected]>; [email protected]; [email protected]
> 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 <[email protected]>
> > > Sent: Tuesday, April 4, 2023 3:25 PM
> > > To: linux-wireless <[email protected]>
> > > Cc: Hans Ulli Kroll <[email protected]>; Larry Finger <[email protected]>; Ping-Ke Shih
> > > <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> > > <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> > > ValdikSS <[email protected]>; [email protected]; Sascha Hauer <[email protected]>;
> > > [email protected]
> > > 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
On Fri, Apr 14, 2023 at 02:05:44AM +0000, Ping-Ke Shih wrote:
>
> > -----Original Message-----
> > From: Sascha Hauer <[email protected]>
> > Sent: Tuesday, April 11, 2023 6:26 PM
> > To: Ping-Ke Shih <[email protected]>
> > Cc: linux-wireless <[email protected]>; Hans Ulli Kroll <[email protected]>; Larry Finger
> > <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> > <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> > ValdikSS <[email protected]>; [email protected]; [email protected]
> > 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 <[email protected]>
> > > > Sent: Tuesday, April 4, 2023 3:25 PM
> > > > To: linux-wireless <[email protected]>
> > > > Cc: Hans Ulli Kroll <[email protected]>; Larry Finger <[email protected]>; Ping-Ke Shih
> > > > <[email protected]>; Tim K <[email protected]>; Alex G . <[email protected]>; Nick Morrow
> > > > <[email protected]>; Viktor Petrenko <[email protected]>; Andreas Henriksson <[email protected]>;
> > > > ValdikSS <[email protected]>; [email protected]; Sascha Hauer <[email protected]>;
> > > > [email protected]
> > > > 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 |