[permalink] [raw]

Subject: RE: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 7:30 AM
> To: [email protected]
> Cc: Yan-Hsuan Chuang <[email protected]>; Kalle Valo <[email protected]>; Ulf Hansson
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Chris Morgan <[email protected]>; Nitin Gupta <[email protected]>;
> Neo Jou <[email protected]>; Ping-Ke Shih <[email protected]>; Jernej Skrabec <[email protected]>;
> Martin Blumenstingl <[email protected]>
> Subject: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets
>
> Add a sub-driver for SDIO based chipsets which implements the following
> functionality:
> - register accessors for 8, 16 and 32 bits for all states of the card
> (including usage of 4x 8 bit access for one 32 bit buffer if the card
> is not fully powered on yet - or if it's fully powered on then 1x 32
> bit access is used)
> - checking whether there's space in the TX FIFO queue to transmit data
> - transfers from the host to the device for actual network traffic,
> reserved pages (for firmware download) and H2C (host-to-card)
> transfers
> - receiving data from the device
> - deep power saving state
>
> The transmit path is optimized so DMA-capable SDIO host controllers can
> directly use the buffers provided because the buffer's physical
> addresses are 8 byte aligned.
>
> The receive path is prepared to support RX aggregation where the
> chipset combines multiple MAC frames into one bigger buffer to reduce
> SDIO transfer overhead.
>
> Co-developed-by: Jernej Skrabec <[email protected]>
> Signed-off-by: Jernej Skrabec <[email protected]>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/Kconfig | 3 +
> drivers/net/wireless/realtek/rtw88/Makefile | 3 +
> drivers/net/wireless/realtek/rtw88/debug.h | 1 +
> drivers/net/wireless/realtek/rtw88/mac.h | 1 -
> drivers/net/wireless/realtek/rtw88/reg.h | 10 +
> drivers/net/wireless/realtek/rtw88/sdio.c | 1242 +++++++++++++++++++
> drivers/net/wireless/realtek/rtw88/sdio.h | 175 +++
> 7 files changed, 1434 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/sdio.h
>

[...]

> +
> +static void rtw_sdio_writel(struct rtw_sdio *rtwsdio, u32 val,
> + u32 addr, int *ret)
> +{
> + u8 buf[4];
> + int i;
> +
> + if (!(addr & 3) && rtwsdio->is_powered_on) {
> + sdio_writel(rtwsdio->sdio_func, val, addr, ret);
> + return;
> + }
> +
> + *(__le32 *)buf = cpu_to_le32(val);
> +
> + for (i = 0; i < 4; i++) {
> + sdio_writeb(rtwsdio->sdio_func, buf[i], addr + i, ret);
> + if (*ret)

Do you need some messages to know something wrong?

> + return;
> + }
> +}
> +
> +static u32 rtw_sdio_readl(struct rtw_sdio *rtwsdio, u32 addr, int *ret)
> +{
> + u8 buf[4];
> + int i;
> +
> + if (!(addr & 3) && rtwsdio->is_powered_on)
> + return sdio_readl(rtwsdio->sdio_func, addr, ret);
> +
> + for (i = 0; i < 4; i++) {
> + buf[i] = sdio_readb(rtwsdio->sdio_func, addr + i, ret);
> + if (*ret)
> + return 0;
> + }
> +
> + return le32_to_cpu(*(__le32 *)buf);
> +}
> +
> +static u8 rtw_sdio_read_indirect8(struct rtw_dev *rtwdev, u32 addr, int *ret)
> +{
> + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> + u32 reg_cfg, reg_data;
> + int retry;
> + u8 tmp;
> +
> + reg_cfg = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_CFG);
> + reg_data = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_DATA);
> +
> + rtw_sdio_writel(rtwsdio, BIT(19) | addr, reg_cfg, ret);
> + if (*ret)
> + return 0;
> +
> + for (retry = 0; retry < RTW_SDIO_INDIRECT_RW_RETRIES; retry++) {
> + tmp = sdio_readb(rtwsdio->sdio_func, reg_cfg + 2, ret);
> + if (!ret && tmp & BIT(4))

'ret' is pointer, do you need '*' ?

if (!*ret && tmp & BIT(4))

As I look into sdio_readb(), it use 'int *err_ret' as arugment.
Would you like to change ' int *ret' to 'int *err_ret'?
It could help to misunderstand.

> + break;
> + }
> +
> + if (*ret)
> + return 0;
> +
> + return sdio_readb(rtwsdio->sdio_func, reg_data, ret);
> +}
> +

[...]

> +
> +static void rtw_sdio_rx_aggregation(struct rtw_dev *rtwdev, bool enable)
> +{
> + u8 size, timeout;
> +
> + if (enable) {
> + if (rtwdev->chip->id == RTW_CHIP_TYPE_8822C) {
> + size = 0xff;
> + timeout = 0x20;
> + } else {
> + size = 0x6;
> + timeout = 0x6;
> + }
> +
> + /* Make the firmware honor the size limit configured below */
> + 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_write16(rtwdev, REG_RXDMA_AGG_PG_TH, size |
> + (timeout << BIT_SHIFT_DMA_AGG_TO_V1));

BIT_RXDMA_AGG_PG_TH GENMASK(7, 0) // for size
BIT_DMA_AGG_TO_V1 GENMASK(15, 8) // for timeout

> +
> + rtw_write8_set(rtwdev, REG_RXDMA_MODE, BIT_DMA_MODE);
> + } else {
> + rtw_write32_clr(rtwdev, REG_RXDMA_AGG_PG_TH, BIT_EN_PRE_CALC);
> + rtw_write8_clr(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
> + rtw_write8_clr(rtwdev, REG_RXDMA_MODE, BIT_DMA_MODE);
> + }
> +}
> +
> +static void rtw_sdio_enable_interrupt(struct rtw_dev *rtwdev)
> +{
> + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> + rtw_write32(rtwdev, REG_SDIO_HIMR, rtwsdio->irq_mask);
> +}
> +
> +static void rtw_sdio_disable_interrupt(struct rtw_dev *rtwdev)
> +{
> + rtw_write32(rtwdev, REG_SDIO_HIMR, 0x0);
> +}
> +
> +static u8 rtw_sdio_get_tx_qsel(struct rtw_dev *rtwdev, struct sk_buff *skb,
> + u8 queue)
> +{
> + switch (queue) {
> + case RTW_TX_QUEUE_BCN:
> + return TX_DESC_QSEL_BEACON;
> + case RTW_TX_QUEUE_H2C:
> + return TX_DESC_QSEL_H2C;
> + case RTW_TX_QUEUE_MGMT:
> + if (rtw_chip_wcpu_11n(rtwdev))
> + return TX_DESC_QSEL_HIGH;
> + else
> + return TX_DESC_QSEL_MGMT;
> + case RTW_TX_QUEUE_HI0:
> + return TX_DESC_QSEL_HIGH;
> + default:
> + return skb->priority;
> + }
> +};

no need ';'

[...]

> +
> +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> +{
> + u32 rx_len;
> +
> + while (true) {

add a limit to prevent infinite loop.

> + if (rtw_chip_wcpu_11n(rtwdev))
> + rx_len = rtw_read16(rtwdev, REG_SDIO_RX0_REQ_LEN);
> + else
> + rx_len = rtw_read32(rtwdev, REG_SDIO_RX0_REQ_LEN);
> +
> + if (!rx_len)
> + break;
> +
> + rtw_sdio_rxfifo_recv(rtwdev, rx_len);
> + }
> +}
> +

[...]

> +
> +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> + enum rtw_tx_queue_type queue)
> +{
> + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> + struct sk_buff *skb;
> + int ret;
> +
> + while (true) {

Can we have a limit?

> + skb = skb_dequeue(&rtwsdio->tx_queue[queue]);
> + if (!skb)
> + break;
> +
> + ret = rtw_sdio_write_port(rtwdev, skb, queue);
> + if (ret) {
> + skb_queue_head(&rtwsdio->tx_queue[queue], skb);
> + break;
> + }
> +
> + if (queue <= RTW_TX_QUEUE_VO)
> + rtw_sdio_indicate_tx_status(rtwdev, skb);
> + else
> + dev_kfree_skb_any(skb);
> + }
> +}
> +

[...]

2022-12-28 12:04:44

by Martin Blumenstingl

[permalink] [raw]

Subject: Re: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets

Hi Ping-Ke,

as always: thank you so much for taking time to go through this!

On Wed, Dec 28, 2022 at 10:39 AM Ping-Ke Shih <[email protected]> wrote:
[...]
>
> > +
> > +static void rtw_sdio_writel(struct rtw_sdio *rtwsdio, u32 val,
> > + u32 addr, int *ret)
> > +{
> > + u8 buf[4];
> > + int i;
> > +
> > + if (!(addr & 3) && rtwsdio->is_powered_on) {
> > + sdio_writel(rtwsdio->sdio_func, val, addr, ret);
> > + return;
> > + }
> > +
> > + *(__le32 *)buf = cpu_to_le32(val);
> > +
> > + for (i = 0; i < 4; i++) {
> > + sdio_writeb(rtwsdio->sdio_func, buf[i], addr + i, ret);
> > + if (*ret)
>
> Do you need some messages to know something wrong?
It's not obvious but we're already logging that something went wrong.
The messages are logged in rtw_sdio_{read,write}{8,16,32}.
We do this because there's multiple ways to access data (direct,
indirect, ...) and some of them require multiple register operations.
So we print one message in the end.

[...]
> > +static u8 rtw_sdio_read_indirect8(struct rtw_dev *rtwdev, u32 addr, int *ret)
> > +{
> > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > + u32 reg_cfg, reg_data;
> > + int retry;
> > + u8 tmp;
> > +
> > + reg_cfg = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_CFG);
> > + reg_data = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_DATA);
> > +
> > + rtw_sdio_writel(rtwsdio, BIT(19) | addr, reg_cfg, ret);
> > + if (*ret)
> > + return 0;
> > +
> > + for (retry = 0; retry < RTW_SDIO_INDIRECT_RW_RETRIES; retry++) {
> > + tmp = sdio_readb(rtwsdio->sdio_func, reg_cfg + 2, ret);
> > + if (!ret && tmp & BIT(4))
>
> 'ret' is pointer, do you need '*' ?
Well spotted - thank you!

[...]
> As I look into sdio_readb(), it use 'int *err_ret' as arugment.
> Would you like to change ' int *ret' to 'int *err_ret'?
> It could help to misunderstand.
Sure, I'll do that

[...]
> > + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, size |
> > + (timeout << BIT_SHIFT_DMA_AGG_TO_V1));
>
> BIT_RXDMA_AGG_PG_TH GENMASK(7, 0) // for size
> BIT_DMA_AGG_TO_V1 GENMASK(15, 8) // for timeout
Thanks, I'll use these

[...]
> > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> > +{
> > + u32 rx_len;
> > +
> > + while (true) {
>
> add a limit to prevent infinite loop.
Do you have any recommendations on how many packets to pull in one go?
My thinking is: pulling to little data at once can hurt performance

[...]
>
> > +
> > +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> > + enum rtw_tx_queue_type queue)
> > +{
> > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > + struct sk_buff *skb;
> > + int ret;
> > +
> > + while (true) {
>
> Can we have a limit?
Similar to the question above: do you have any recommendations on how
many packets (per queue) to send in one go?

Best regards,
Martin

2022-12-29 01:11:07

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets

> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Wednesday, December 28, 2022 8:00 PM
> To: Ping-Ke Shih <[email protected]>
> Cc: [email protected]; Yan-Hsuan Chuang <[email protected]>; Kalle Valo
> <[email protected]>; Ulf Hansson <[email protected]>; [email protected];
> [email protected]; [email protected]; Chris Morgan <[email protected]>; Nitin Gupta
> <[email protected]>; Neo Jou <[email protected]>; Jernej Skrabec <[email protected]>
> Subject: Re: [RFC PATCH v1 12/19] rtw88: sdio: Add HCI implementation for SDIO based chipsets
>
> Hi Ping-Ke,
>
> as always: thank you so much for taking time to go through this!
>
> On Wed, Dec 28, 2022 at 10:39 AM Ping-Ke Shih <[email protected]> wrote:

[...]

> > > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> > > +{
> > > + u32 rx_len;
> > > +
> > > + while (true) {
> >
> > add a limit to prevent infinite loop.
> Do you have any recommendations on how many packets to pull in one go?
> My thinking is: pulling to little data at once can hurt performance

This is to prevent unexpected things happen, and normally we receive/send
all packets at once like while(true) code. So, maybe we can have rough limit,
like 512 that would be enough for most cases. To have accurate number, you
can do experiments with the highest performance to see the loop count in
real usage, and 5 or 10 times count as limit.

However, when I reviewed rtw88 USB patches, I found we can't always break
the loop, because it could only one chance to free skb(s). So, you should
make sure we really can break the loop logically, and then decide which
limit is suitable for us.

>
> [...]
> >
> > > +
> > > +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> > > + enum rtw_tx_queue_type queue)
> > > +{
> > > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > > + struct sk_buff *skb;
> > > + int ret;
> > > +
> > > + while (true) {
> >
> > Can we have a limit?
> Similar to the question above: do you have any recommendations on how
> many packets (per queue) to send in one go?
>

Ping-Ke

2022-12-29 01:42:05

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [RFC PATCH v1 18/19] rtw88: Add support for the SDIO based RTL8822CS chipset

2022-12-29 04:25:38

by Ping-Ke Shih

[permalink] [raw]

Jakub Kicinski <[email protected]> writes:

> On Mon, 16 Jan 2023 18:01:05 +0200 Kalle Valo wrote:
>> > - My understanding is that there's a discussion about the rtw88 Kconfig
>> > symbols. We're adding four new ones within this series. It's not
>> > clear to me what the conclusion is on this topic though.
>>
>> Yeah, there were no conclusions about that. Jakub, do you have any
>> opinions? For example, do we keep per device Kconfig options (eg.
>> CONFIG_RTW88_8822BS, RTW88_8822CS and so on) or should we have only one
>> more bus level option (eg. CONFIG_RTW88_SDIO)? rtw88 now uses the former
>> and IIRC so does mt76. ath10k/ath11k/ath12k again use the latter :)
>
> No strong feelings. Larry (IIRC) provided a fair justification for
> the RTW symbols. If the module binary grows noticeably then having
> the kconfig does indeed make sense.

Thanks, makes sense. So the plan is that rtw88 continues to use per
device Kconfig symbols with SDIO.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches