> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> Sent: Wednesday, December 28, 2022 7:30 AM
> To: linux-wireless@xxxxxxxxxxxxxxx
> Cc: Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; Ulf Hansson
> <ulf.hansson@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> linux-mmc@xxxxxxxxxxxxxxx; Chris Morgan <macroalpha82@xxxxxxxxx>; Nitin Gupta <nitin.gupta981@xxxxxxxxx>;
> Neo Jou <neojou@xxxxxxxxx>; Ping-Ke Shih <pkshih@xxxxxxxxxxx>; Jernej Skrabec <jernej.skrabec@xxxxxxxxx>;
> Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> 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 <jernej.skrabec@xxxxxxxxx>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> ---
> 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);
> + }
> +}
> +
[...]
