RE: [PATCH v4 6/7] wifi: rtw88: sdio: set up RX aggregation and interrupts for RTL8723BS

[Ping-Ke Shih <pkshih@realtek.com>]

luka.gejak@linux.dev <luka.gejak@linux.dev> wrote:
> From: Luka Gejak <luka.gejak@linux.dev>
> 
> Enable the existing RX aggregation setup for this chip and select the
> larger DMA burst count it needs. The RTL8723BS does not raise CPWM1, so
> leave that source out of its interrupt mask, and set the SDIO TX control
> bit the vendor driver uses to have transfers always recognised.
> 
> The chip also keeps raising the interrupt after resume if undefined
> status bits are written back when acknowledging, so acknowledge only the
> defined and unmasked bits. That is scoped to this chip; the other SDIO
> parts keep writing the status word back unchanged.
> 
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>

[...]

> @@ -967,12 +974,22 @@ static void rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
> 
>  static int rtw_sdio_start(struct rtw_dev *rtwdev)
>  {
> +       u32 clear;
> +
>         if (rtw_is_8723bs(rtwdev)) {
>                 rtw_sdio_8723bs_check_rqpn(rtwdev);
>                 rtw_sdio_8723bs_init_free_txpg(rtwdev);
>         }
> 
>         rtw_sdio_enable_rx_aggregation(rtwdev);
> +
> +       if (rtw_is_8723bs(rtwdev)) {
> +               clear = rtw_read32(rtwdev, REG_SDIO_HISR) &
> +                       RTW_SDIO_HISR_CLEAR_MASK;

Though it is little over 80 characters, straightening it will be more readable. 

> +               if (clear)
> +                       rtw_write32(rtwdev, REG_SDIO_HISR, clear);
> +       }
> +
>         rtw_sdio_enable_interrupt(rtwdev);
> 
>         return 0;
> @@ -1052,6 +1069,8 @@ static void rtw_sdio_interface_cfg(struct rtw_dev *rtwdev)
> 
>         val = rtw_read32(rtwdev, REG_SDIO_TX_CTRL);
>         val &= 0xfff8;
> +       if (rtw_is_8723bs(rtwdev))
> +               val |= BIT_SDIO_TX_CTRL_ALWAYS_RECOGNIZE;
>         rtw_write32(rtwdev, REG_SDIO_TX_CTRL, val);
>  }
> 
> @@ -1316,6 +1335,14 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
>                 rtw_sdio_rx_isr(rtwdev);
>         }
> 
> +       /*
> +        * RTL8723BS keeps raising the interrupt after resume if undefined
> +        * status bits are written back, so acknowledge only the bits that are
> +        * both defined and unmasked. Other chips keep the existing behaviour.
> +        */
> +       if (rtw_is_8723bs(rtwdev))
> +               hisr &= rtwsdio->irq_mask & RTW_SDIO_HISR_CLEAR_MASK;
> +

I'm not sure if I read something wrong...

rtwsdio->irq_mask = REG_SDIO_HIMR_RX_REQUEST;  // BIT(0)
RTW_SDIO_HISR_CLEAR_MASK // BIT(2-7, 17-24)

So, (rtwsdio->irq_mask & RTW_SDIO_HISR_CLEAR_MASK) == 0
Then, hisr &= 0  --> hisr = 0. 

Which point is wrong to me?

>         rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
> 
>         rtwsdio->irq_thread = NULL;

[...]