| Message ID | c7f9a5c0-a90f-4ebe-b7a0-401d300bfa13@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | [1/2] wifi: rtw88: usb: Support USB 3 with RTL8812AU | expand |
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: >> > USB RX aggregation improves the RX speed on certain ARM systems, like > the NanoPi NEO Core2. With RTL8811AU, before: 30 Mbps, after: 224 Mbps. > > The out-of-tree driver uses aggregation size of 7 in USB 3 mode, but > that doesn't work here. rtw88 advertises support for receiving AMSDU > in AMPDU, so the AP sends larger frames, up to ~5100 bytes. With a size > of 7 RTL8812AU frequently tries to aggregate more frames than will fit > in 32768 bytes. Use a size of 6 instead. > > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > drivers/net/wireless/realtek/rtw88/usb.c | 30 ++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c > index 752bca05b9af..9172af63500b 100644 > --- a/drivers/net/wireless/realtek/rtw88/usb.c > +++ b/drivers/net/wireless/realtek/rtw88/usb.c > @@ -790,6 +790,32 @@ static void rtw_usb_dynamic_rx_agg_v1(struct rtw_dev *rtwdev, bool enable) > rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); > } > > +static void rtw_usb_dynamic_rx_agg_v2(struct rtw_dev *rtwdev, bool enable) > +{ > + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > + u8 size, timeout; > + u16 val16; > + How about a shortcut? if (!enable) { size = 0x0; timeout = 0x1; goto rx_agg; } > + if (rtwusb->udev->speed == USB_SPEED_SUPER) { > + size = 0x6; > + timeout = 0x1a; > + } else { > + size = 0x5; > + timeout = 0x20; > + } > + > + if (!enable) { > + size = 0x0; > + timeout = 0x1; > + } > + rx_agg: > + val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) | > + u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1); > + > + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); > + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN); > +} > + > static void rtw_usb_dynamic_rx_agg(struct rtw_dev *rtwdev, bool enable) > { > switch (rtwdev->chip->id) { > @@ -798,6 +824,10 @@ static void rtw_usb_dynamic_rx_agg(struct rtw_dev *rtwdev, bool enable) > case RTW_CHIP_TYPE_8821C: > rtw_usb_dynamic_rx_agg_v1(rtwdev, enable); > break; > + case RTW_CHIP_TYPE_8821A: > + case RTW_CHIP_TYPE_8812A: > + rtw_usb_dynamic_rx_agg_v2(rtwdev, enable); > + break; > case RTW_CHIP_TYPE_8723D: > /* Doesn't like aggregation. */ > break; > -- > 2.47.0
On 08/11/2024 04:40, Ping-Ke Shih wrote: > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: >>> >> USB RX aggregation improves the RX speed on certain ARM systems, like >> the NanoPi NEO Core2. With RTL8811AU, before: 30 Mbps, after: 224 Mbps. >> >> The out-of-tree driver uses aggregation size of 7 in USB 3 mode, but >> that doesn't work here. rtw88 advertises support for receiving AMSDU >> in AMPDU, so the AP sends larger frames, up to ~5100 bytes. With a size >> of 7 RTL8812AU frequently tries to aggregate more frames than will fit >> in 32768 bytes. Use a size of 6 instead. >> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> >> --- >> drivers/net/wireless/realtek/rtw88/usb.c | 30 ++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c >> index 752bca05b9af..9172af63500b 100644 >> --- a/drivers/net/wireless/realtek/rtw88/usb.c >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c >> @@ -790,6 +790,32 @@ static void rtw_usb_dynamic_rx_agg_v1(struct rtw_dev *rtwdev, bool enable) >> rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); >> } >> >> +static void rtw_usb_dynamic_rx_agg_v2(struct rtw_dev *rtwdev, bool enable) >> +{ >> + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); >> + u8 size, timeout; >> + u16 val16; >> + > > How about a shortcut? > > if (!enable) { > size = 0x0; > timeout = 0x1; > > goto rx_agg; > } > >> + if (rtwusb->udev->speed == USB_SPEED_SUPER) { >> + size = 0x6; >> + timeout = 0x1a; >> + } else { >> + size = 0x5; >> + timeout = 0x20; >> + } >> + >> + if (!enable) { >> + size = 0x0; >> + timeout = 0x1; >> + } >> + > > rx_agg: > >> + val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) | >> + u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1); >> + >> + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); >> + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN); >> +} Hmm, I don't like it. What about this? if (!enable) { size = 0x0; timeout = 0x1; } else if (rtwusb->udev->speed == USB_SPEED_SUPER) { size = 0x6; timeout = 0x1a; } else { size = 0x5; timeout = 0x20; }
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > On 08/11/2024 04:40, Ping-Ke Shih wrote: > > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > >>> > >> USB RX aggregation improves the RX speed on certain ARM systems, like > >> the NanoPi NEO Core2. With RTL8811AU, before: 30 Mbps, after: 224 Mbps. > >> > >> The out-of-tree driver uses aggregation size of 7 in USB 3 mode, but > >> that doesn't work here. rtw88 advertises support for receiving AMSDU > >> in AMPDU, so the AP sends larger frames, up to ~5100 bytes. With a size > >> of 7 RTL8812AU frequently tries to aggregate more frames than will fit > >> in 32768 bytes. Use a size of 6 instead. > >> > >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > >> --- > >> drivers/net/wireless/realtek/rtw88/usb.c | 30 ++++++++++++++++++++++++ > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c > >> index 752bca05b9af..9172af63500b 100644 > >> --- a/drivers/net/wireless/realtek/rtw88/usb.c > >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c > >> @@ -790,6 +790,32 @@ static void rtw_usb_dynamic_rx_agg_v1(struct rtw_dev *rtwdev, bool enable) > >> rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); > >> } > >> > >> +static void rtw_usb_dynamic_rx_agg_v2(struct rtw_dev *rtwdev, bool enable) > >> +{ > >> + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > >> + u8 size, timeout; > >> + u16 val16; > >> + > > > > How about a shortcut? > > > > if (!enable) { > > size = 0x0; > > timeout = 0x1; > > > > goto rx_agg; > > } > > > >> + if (rtwusb->udev->speed == USB_SPEED_SUPER) { > >> + size = 0x6; > >> + timeout = 0x1a; > >> + } else { > >> + size = 0x5; > >> + timeout = 0x20; > >> + } > >> + > >> + if (!enable) { > >> + size = 0x0; > >> + timeout = 0x1; > >> + } > >> + > > > > rx_agg: > > > >> + val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) | > >> + u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1); > >> + > >> + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); > >> + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN); > >> +} > > Hmm, I don't like it. Honestly, I don't like it so much. > What about this? > > if (!enable) { > size = 0x0; > timeout = 0x1; > } else if (rtwusb->udev->speed == USB_SPEED_SUPER) { > size = 0x6; > timeout = 0x1a; > } else { > size = 0x5; > timeout = 0x20; > } I thought about this too, but a flaw is first condition is for !enable and later two conditions are for enable. I think the logic below is clear, but more indent though. if (!enable) { size = 0x0; timeout = 0x1; } else { if (rtwusb->udev->speed == USB_SPEED_SUPER) { size = 0x6; timeout = 0x1a; } else { size = 0x5; timeout = 0x20; } } Mine, yours and my last proposal are fine to me. You can decide to take one of them.
diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c index 752bca05b9af..9172af63500b 100644 --- a/drivers/net/wireless/realtek/rtw88/usb.c +++ b/drivers/net/wireless/realtek/rtw88/usb.c @@ -790,6 +790,32 @@ static void rtw_usb_dynamic_rx_agg_v1(struct rtw_dev *rtwdev, bool enable) rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); } +static void rtw_usb_dynamic_rx_agg_v2(struct rtw_dev *rtwdev, bool enable) +{ + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); + u8 size, timeout; + u16 val16; + + if (rtwusb->udev->speed == USB_SPEED_SUPER) { + size = 0x6; + timeout = 0x1a; + } else { + size = 0x5; + timeout = 0x20; + } + + if (!enable) { + size = 0x0; + timeout = 0x1; + } + + val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) | + u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1); + + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16); + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN); +} + static void rtw_usb_dynamic_rx_agg(struct rtw_dev *rtwdev, bool enable) { switch (rtwdev->chip->id) { @@ -798,6 +824,10 @@ static void rtw_usb_dynamic_rx_agg(struct rtw_dev *rtwdev, bool enable) case RTW_CHIP_TYPE_8821C: rtw_usb_dynamic_rx_agg_v1(rtwdev, enable); break; + case RTW_CHIP_TYPE_8821A: + case RTW_CHIP_TYPE_8812A: + rtw_usb_dynamic_rx_agg_v2(rtwdev, enable); + break; case RTW_CHIP_TYPE_8723D: /* Doesn't like aggregation. */ break;
USB RX aggregation improves the RX speed on certain ARM systems, like the NanoPi NEO Core2. With RTL8811AU, before: 30 Mbps, after: 224 Mbps. The out-of-tree driver uses aggregation size of 7 in USB 3 mode, but that doesn't work here. rtw88 advertises support for receiving AMSDU in AMPDU, so the AP sends larger frames, up to ~5100 bytes. With a size of 7 RTL8812AU frequently tries to aggregate more frames than will fit in 32768 bytes. Use a size of 6 instead. Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- drivers/net/wireless/realtek/rtw88/usb.c | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+)