On 30/07/2024 08:47, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
>>
>> Enable USB RX aggregation when there is at least 1 Mbps RX or TX
>> traffic, otherwise disable it.
>>
>> USB RX aggregation improves the RX speed on certain ARM systems, like
>> the NanoPi NEO Core2. With RTL8811CU, before: 28 Mbps, after: 231 Mbps.
>>
>> The official drivers for these chips use the same logic for SDIO, but
>> for some reason rtw88_sdio always enables RX aggregation, so this patch
>> only toggles aggregation for USB devices.
>>
>> RTL8703B is likely not found in USB devices, and RTL8723DU doesn't like
>> aggregation.
>
> Please explicitly set .rx_aggregation = NULL to these two chips, so
> we know these two chips don't have this feature.
>
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
>> ---
>> drivers/net/wireless/realtek/rtw88/main.c | 18 +++++++++++----
>> drivers/net/wireless/realtek/rtw88/main.h | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8821c.c | 23 +++++++++++++++++++
>> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 23 +++++++++++++++++++
>> drivers/net/wireless/realtek/rtw88/rtw8822c.c | 23 +++++++++++++++++++
>> 5 files changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
>> index 9d9d33a4a503..b3a089b4f707 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.c
>> +++ b/drivers/net/wireless/realtek/rtw88/main.c
>> @@ -210,8 +210,10 @@ static void rtw_watch_dog_work(struct work_struct *work)
>> struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>> watch_dog_work.work);
>> struct rtw_traffic_stats *stats = &rtwdev->stats;
>> + const struct rtw_chip_info *chip = rtwdev->chip;
>> struct rtw_watch_dog_iter_data data = {};
>> bool busy_traffic = test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags);
>> + u32 tx_unicast_shift, rx_unicast_shift;
>> bool ps_active;
>>
>> mutex_lock(&rtwdev->mutex);
>> @@ -236,13 +238,21 @@ static void rtw_watch_dog_work(struct work_struct *work)
>> else
>> ps_active = false;
>>
>> - ewma_tp_add(&stats->tx_ewma_tp,
>> - (u32)(stats->tx_unicast >> RTW_TP_SHIFT));
>> - ewma_tp_add(&stats->rx_ewma_tp,
>> - (u32)(stats->rx_unicast >> RTW_TP_SHIFT));
>> + tx_unicast_shift = stats->tx_unicast >> RTW_TP_SHIFT;
>> + rx_unicast_shift = stats->rx_unicast >> RTW_TP_SHIFT;
>
> {tx,rx}_unicast_mbps because 'shift' is to get Mbps.
>
>> +
>> + ewma_tp_add(&stats->tx_ewma_tp, tx_unicast_shift);
>> + ewma_tp_add(&stats->rx_ewma_tp, rx_unicast_shift);
>> stats->tx_throughput = ewma_tp_read(&stats->tx_ewma_tp);
>> stats->rx_throughput = ewma_tp_read(&stats->rx_ewma_tp);
>>
>> + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB && chip->ops->rx_aggregation) {
>> + if (tx_unicast_shift < 1 && rx_unicast_shift < 1)
>> + chip->ops->rx_aggregation(rtwdev, false);
>> + else
>> + chip->ops->rx_aggregation(rtwdev, true);
>> + }
>
> Move this chunk to a function with arguments {tx,rx}_unicast_mbps.
> The function name might be something like rtw_dynamic_usb_rx_aggregation().
>
>> +
>> /* reset tx/rx statictics */
>> stats->tx_unicast = 0;
>> stats->rx_unicast = 0;
>> diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
>> index 9d21637cf5d5..65bedd1668cc 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.h
>> +++ b/drivers/net/wireless/realtek/rtw88/main.h
>> @@ -888,6 +888,7 @@ struct rtw_chip_ops {
>> void (*fill_txdesc_checksum)(struct rtw_dev *rtwdev,
>> struct rtw_tx_pkt_info *pkt_info,
>> u8 *txdesc);
>> + void (*rx_aggregation)(struct rtw_dev *rtwdev, bool enable);
>>
>> /* for coex */
>> void (*coex_set_init)(struct rtw_dev *rtwdev);
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> index 55b6fe874710..3efdb41f22c5 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
>> @@ -1276,6 +1276,28 @@ static void rtw8821c_fill_txdesc_checksum(struct rtw_dev *rtwdev,
>> fill_txdesc_checksum_common(txdesc, 16);
>> }
>>
>> +static void rtw8821c_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
>> +{
>> + u8 size, timeout;
>> + u16 val16;
>> +
>> + rtw_write32_set(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
>> + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
>> + rtw_write8_clr(rtwdev, REG_RXDMA_AGG_PG_TH + 3, BIT(7));
>> +
>> + if (enable) {
>> + size = 0x5;
>> + timeout = 0x20;
>> + } else {
>> + 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);
>> +}
>> +
>
> All use the same settings. Move this to rtw_usb_rx_aggregation() called by
> rtw_dynamic_usb_rx_aggregation().
>
These three chips use the same settings. RTL8821AU/RTL8812AU
will be a bit different:
static void rtw8821au_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
{
struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
u8 rxagg_usb_size, rxagg_usb_timeout;
u16 val16;
if (rtwusb->udev->speed == USB_SPEED_SUPER) {
rxagg_usb_size = 0x7;
rxagg_usb_timeout = 0x1a;
} else {
rxagg_usb_size = 0x5;
rxagg_usb_timeout = 0x20;
}
if (!enable) {
rxagg_usb_size = 0x0;
rxagg_usb_timeout = 0x1;
}
val16 = (rxagg_usb_timeout << 8) | rxagg_usb_size;
rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
}
And RTL8814AU will be more different.
I suppose they can be moved to the same place, like the burst
stuff. Maybe struct rtw_hci_ops should have a new member called
rx_aggregation, in case someone decides to make the SDIO driver
use dynamic RX aggregation as well?
