Third round of the RTW88 USB bug fixes.
After some discussion and thinking I came to the conclusion that the
v1 variant of "wifi: rtw88: rtw8821c: Fix rfe_option field width" is
better than the one posted in v2, so I reverted back to this version,
but added a note to the commit message why this might not be entirely
correct for all chip variants (though for all variants currently
supported in the driver).
The patches are sorted in order of importance. 1/4 hasn't seen any
negative comments and I think it should be applied right now.
As stated above I think 2/4 should be applied as well. 3/4 fixes
something I stumbled upon while reading in the vendor driver, but
I don't what effect it actually has, I didn't notice any change
in behaviour of the driver. 4/4 straightens the logic how
rtw8821c_switch_rf_set() is called for different variants of the
rtw8821c. This is taken from the vendor driver. From the supported
chip variants this should only have an effect on the ones with
rfe_option = 6, but I don't have that one available here for
testing.
I would be glad if at least 1/4 and 2/4 could be applied as these
fix real issues in the driver.
Sascha
Sascha Hauer (4):
wifi: rtw88: usb: fix priority queue to endpoint mapping
wifi: rtw88: rtw8821c: Fix rfe_option field width
wifi: rtw88: set pkg_type correctly for specific rtw8821c variants
wifi: rtw88: call rtw8821c_switch_rf_set() according to chip variant
drivers/net/wireless/realtek/rtw88/main.c | 2 +-
drivers/net/wireless/realtek/rtw88/main.h | 2 +
drivers/net/wireless/realtek/rtw88/rtw8821c.c | 25 +++++--
drivers/net/wireless/realtek/rtw88/usb.c | 70 +++++++++++++------
4 files changed, 69 insertions(+), 30 deletions(-)
--
2.39.2
On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
vendor 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.
Note that this might not be the whole truth. In the vendor driver
there are indeed places where the unmasked rfe_option value is used.
However, the driver has several places where rfe_option is tested
with the pattern if (rfe_option == 2 || rfe_option == 0x22) or
if (rfe_option == 4 || rfe_option == 0x24), so that rfe_option BIT(5)
has no influence on the code path taken. We therefore mask BIT(5)
out from rfe_option entirely until this assumption is proved wrong
by some chip variant we do not know yet.
Signed-off-by: Sascha Hauer <[email protected]>
Tested-by: Alexandru gagniuc <[email protected]>
Tested-by: Larry Finger <[email protected]>
Tested-by: ValdikSS <[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
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
We have to call rtw8821c_switch_rf_set() with SWITCH_TO_WLG or
SWITCH_TO_BTG according to the chip variant as denoted in rfe_option.
The information which argument to use for which variant has been
taken from the vendor driver.
Signed-off-by: Sascha Hauer <[email protected]>
---
drivers/net/wireless/realtek/rtw88/main.h | 1 +
drivers/net/wireless/realtek/rtw88/rtw8821c.c | 19 +++++++++++++++----
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 9946aca7a72ce..462f69547be03 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1892,6 +1892,7 @@ struct rtw_hal {
u8 oem_id;
u8 pkg_type;
struct rtw_phy_cond phy_cond;
+ bool rfe_btg;
u8 ps_mode;
u8 current_channel;
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 94c582a27b9ff..a50753ae235b5 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -67,6 +67,17 @@ static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
hal->pkg_type = map->rfe_option & BIT(5) ? 1 : 0;
+ switch (efuse->rfe_option) {
+ case 0x2:
+ case 0x4:
+ case 0x7:
+ case 0xa:
+ case 0xc:
+ case 0xf:
+ hal->rfe_btg = true;
+ break;
+ }
+
for (i = 0; i < 4; i++)
efuse->txpwr_idx_table[i] = map->txpwr_idx_table[i];
@@ -289,6 +300,7 @@ static void rtw8821c_switch_rf_set(struct rtw_dev *rtwdev, u8 rf_set)
static void rtw8821c_set_channel_rf(struct rtw_dev *rtwdev, u8 channel, u8 bw)
{
+ struct rtw_hal *hal = &rtwdev->hal;
u32 rf_reg18;
rf_reg18 = rtw_read_rf(rtwdev, RF_PATH_A, 0x18, RFREG_MASK);
@@ -320,11 +332,10 @@ 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)
+ if (hal->rfe_btg)
rtw8821c_switch_rf_set(rtwdev, SWITCH_TO_BTG);
+ else
+ 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 {
--
2.39.2
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: Monday, April 17, 2023 10:04 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 v3 1/4] 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.
>
> 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]>
[...]
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: Monday, April 17, 2023 10:04 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 v3 2/4] wifi: rtw88: rtw8821c: Fix rfe_option field width
>
> On my RTW8821CU chipset rfe_option reads as 0x22. Looking at the
> vendor 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.
>
> Note that this might not be the whole truth. In the vendor driver
> there are indeed places where the unmasked rfe_option value is used.
> However, the driver has several places where rfe_option is tested
> with the pattern if (rfe_option == 2 || rfe_option == 0x22) or
> if (rfe_option == 4 || rfe_option == 0x24), so that rfe_option BIT(5)
> has no influence on the code path taken. We therefore mask BIT(5)
> out from rfe_option entirely until this assumption is proved wrong
> by some chip variant we do not know yet.
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Tested-by: Alexandru gagniuc <[email protected]>
> Tested-by: Larry Finger <[email protected]>
> Tested-by: ValdikSS <[email protected]>
> Cc: [email protected]
Reviewed-by: Ping-Ke Shih <[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: Monday, April 17, 2023 10:04 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]>
> Subject: [PATCH v3 4/4] wifi: rtw88: call rtw8821c_switch_rf_set() according to chip variant
>
> We have to call rtw8821c_switch_rf_set() with SWITCH_TO_WLG or
> SWITCH_TO_BTG according to the chip variant as denoted in rfe_option.
> The information which argument to use for which variant has been
> taken from the vendor driver.
>
> Signed-off-by: Sascha Hauer <[email protected]>
Reviewed-by: Ping-Ke Shih <[email protected]>
[...]
Sascha Hauer <[email protected]> wrote:
> 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]
> Reviewed-by: Ping-Ke Shih <[email protected]>
4 patches applied to wireless-next.git, thanks.
a6f187f92bcc wifi: rtw88: usb: fix priority queue to endpoint mapping
14705f969d98 wifi: rtw88: rtw8821c: Fix rfe_option field width
97c75e1adeda wifi: rtw88: set pkg_type correctly for specific rtw8821c variants
172591baa2cc wifi: rtw88: call rtw8821c_switch_rf_set() according to chip variant
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches