[v2,RFC,3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence

Add the code specific to SDIO HCI in the MAC power on sequence. This is
based on the RTL8822BS and RTL8822CS vendor drivers.

Co-developed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
Changes since v1:
- only access REG_SDIO_HIMR for RTW_HCI_TYPE_SDIO
- use proper BIT_HCI_SUS_REQ, BIT_HCI_RESUME_RDY and BIT_SDIO_PAD_E5
  macros as suggested by Ping-Ke


 drivers/net/wireless/realtek/rtw88/mac.c | 46 +++++++++++++++++++++---
 1 file changed, 42 insertions(+), 4 deletions(-)

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Saturday, March 11, 2023 4:29 AM
> To: linux-wireless@vger.kernel.org
> Cc: Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org; netdev@vger.kernel.org;
> linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta <nitin.gupta981@gmail.com>;
> Neo Jou <neojou@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>; Jernej Skrabec <jernej.skrabec@gmail.com>;
> Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Subject: [PATCH v2 RFC 3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence
> 
> Add the code specific to SDIO HCI in the MAC power on sequence. This is
> based on the RTL8822BS and RTL8822CS vendor drivers.
> 
> Co-developed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> Changes since v1:
> - only access REG_SDIO_HIMR for RTW_HCI_TYPE_SDIO
> - use proper BIT_HCI_SUS_REQ, BIT_HCI_RESUME_RDY and BIT_SDIO_PAD_E5
>   macros as suggested by Ping-Ke
> 
> 
>  drivers/net/wireless/realtek/rtw88/mac.c | 46 +++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
> index cfdfc8a2c836..17704394cca3 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac.c
> @@ -7,6 +7,7 @@
>  #include "reg.h"
>  #include "fw.h"
>  #include "debug.h"
> +#include "sdio.h"
> 
>  void rtw_set_channel_mac(struct rtw_dev *rtwdev, u8 channel, u8 bw,
>                          u8 primary_ch_idx)
> @@ -60,6 +61,7 @@ EXPORT_SYMBOL(rtw_set_channel_mac);
> 
>  static int rtw_mac_pre_system_cfg(struct rtw_dev *rtwdev)
>  {
> +       unsigned int retry;
>         u32 value32;
>         u8 value8;
> 
> @@ -77,6 +79,28 @@ static int rtw_mac_pre_system_cfg(struct rtw_dev *rtwdev)
>         case RTW_HCI_TYPE_PCIE:
>                 rtw_write32_set(rtwdev, REG_HCI_OPT_CTRL, BIT_USB_SUS_DIS);
>                 break;
> +       case RTW_HCI_TYPE_SDIO:
> +               rtw_write8_clr(rtwdev, REG_SDIO_HSUS_CTRL, BIT_HCI_SUS_REQ);
> +
> +               for (retry = 0; retry < RTW_PWR_POLLING_CNT; retry++) {
> +                       if (rtw_read8(rtwdev, REG_SDIO_HSUS_CTRL) & BIT_HCI_RESUME_RDY)
> +                               break;
> +
> +                       usleep_range(10, 50);
> +               }
> +
> +               if (retry == RTW_PWR_POLLING_CNT) {
> +                       rtw_err(rtwdev, "failed to poll REG_SDIO_HSUS_CTRL[1]");
> +                       return -ETIMEDOUT;
> +               }
> +
> +               if (rtw_sdio_is_sdio30_supported(rtwdev))
> +                       rtw_write8_set(rtwdev, REG_HCI_OPT_CTRL + 2,
> +                                      BIT_SDIO_PAD_E5 >> 16);
> +               else
> +                       rtw_write8_clr(rtwdev, REG_HCI_OPT_CTRL + 2,
> +                                      BIT_SDIO_PAD_E5 >> 16);
> +               break;
>         case RTW_HCI_TYPE_USB:
>                 break;
>         default:
> @@ -248,6 +272,7 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
>  {
>         const struct rtw_chip_info *chip = rtwdev->chip;
>         const struct rtw_pwr_seq_cmd **pwr_seq;
> +       u32 imr;
>         u8 rpwm;
>         bool cur_pwr;
>         int ret;
> @@ -273,18 +298,24 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
>         if (pwr_on == cur_pwr)
>                 return -EALREADY;
> 
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) {
> +               imr = rtw_read32(rtwdev, REG_SDIO_HIMR);
> +               rtw_write32(rtwdev, REG_SDIO_HIMR, 0);
> +       }
> +
>         if (!pwr_on)
>                 clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
> 
>         pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
>         ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> -       if (ret)
> -               return ret;
> +
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO)
> +               rtw_write32(rtwdev, REG_SDIO_HIMR, imr);
> 
>         if (pwr_on)
>                 set_bit(RTW_FLAG_POWERON, rtwdev->flags);

If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable?
Did you meet real problem here?

Maybe, here can be 

         if (pwr_on && !ret)
                 set_bit(RTW_FLAG_POWERON, rtwdev->flags);

> 
> -       return 0;
> +       return ret;
>  }
> 
>  static int __rtw_mac_init_system_cfg(struct rtw_dev *rtwdev)
> @@ -455,6 +486,9 @@ static void download_firmware_reg_backup(struct rtw_dev *rtwdev,
>         rtw_write16(rtwdev, REG_FIFOPAGE_INFO_1, 0x200);
>         rtw_write32(rtwdev, REG_RQPN_CTRL_2, bckp[bckp_idx - 1].val);
> 
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO)
> +               rtw_read32(rtwdev, REG_SDIO_FREE_TXPG);
> +
>         /* Disable beacon related functions */
>         tmp = rtw_read8(rtwdev, REG_BCN_CTRL);
>         bckp[bckp_idx].len = 1;
> @@ -1067,8 +1101,12 @@ static int txdma_queue_mapping(struct rtw_dev *rtwdev)
>         if (rtw_chip_wcpu_11ac(rtwdev))
>                 rtw_write32(rtwdev, REG_H2CQ_CSR, BIT_H2CQ_FULL);
> 
> -       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB)
> +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO) {
> +               rtw_read32(rtwdev, REG_SDIO_FREE_TXPG);
> +               rtw_write32(rtwdev, REG_SDIO_TX_CTRL, 0);
> +       } else if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB) {
>                 rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_ARBBW_EN);
> +       }
> 
>         return 0;
>  }
> --
> 2.39.2

Hello Ping-Ke,

On Mon, Mar 13, 2023 at 10:05 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
[...]
> >         pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
> >         ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> > -       if (ret)
> > -               return ret;
> > +
> > +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO)
> > +               rtw_write32(rtwdev, REG_SDIO_HIMR, imr);
> >
> >         if (pwr_on)
> >                 set_bit(RTW_FLAG_POWERON, rtwdev->flags);
>
> If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable?
That sounds very reasonable to me!

> Did you meet real problem here?
>
> Maybe, here can be
>
>          if (pwr_on && !ret)
>                  set_bit(RTW_FLAG_POWERON, rtwdev->flags);
I can't remember any issue that I've seen. I'll verify this at the end
of the week (until then I am pretty busy with my daytime job) and then
go with your suggestion.
Thanks again as always - your feedback is really appreciated!

Also thank you for commenting on the other patches. I'll take a closer
look at your feedback at the end of the week and send another version
of this series.


Best regards,
Martin

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Tuesday, March 14, 2023 4:12 AM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org; Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo
> <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; linux-mmc@vger.kernel.org; Chris Morgan <macroalpha82@gmail.com>; Nitin Gupta
> <nitin.gupta981@gmail.com>; Neo Jou <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>
> Subject: Re: [PATCH v2 RFC 3/9] wifi: rtw88: mac: Support SDIO specific bits in the power on sequence
> 
> Hello Ping-Ke,
> 
> On Mon, Mar 13, 2023 at 10:05 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> [...]
> > >         pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
> > >         ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> > > -       if (ret)
> > > -               return ret;
> > > +
> > > +       if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_SDIO)
> > > +               rtw_write32(rtwdev, REG_SDIO_HIMR, imr);
> > >
> > >         if (pwr_on)
> > >                 set_bit(RTW_FLAG_POWERON, rtwdev->flags);
> >
> > If failed to power on, it still set RTW_FLAG_POWERON. Is it reasonable?
> That sounds very reasonable to me!

Let me clear here more.

Consider a use case:
1. Initially, it enters this function with pwr_on = true, and RTW_FLAG_POWERON is unset.
2. rtw_pwr_seq_parser() return error, so power state is uncertain.
3. Unconditionally, set RTW_FLAG_POWERON.

rtw_mac_power_on() will try to power off/on once again if it fails, so
in step 3 setting RTW_FLAG_POWERON only if rtw_pwr_seq_parser() returns 0
can have the same values as step 1, when it retries to power on.

Honestly, we don't have perfect error handle for error of rtw_pwr_seq_parser(),
but still want to make things easier understand.

> 
> > Did you meet real problem here?
> >
> > Maybe, here can be
> >
> >          if (pwr_on && !ret)
> >                  set_bit(RTW_FLAG_POWERON, rtwdev->flags);
> I can't remember any issue that I've seen. I'll verify this at the end
> of the week (until then I am pretty busy with my daytime job) and then
> go with your suggestion.

Thanks. Wait for you.

> Thanks again as always - your feedback is really appreciated!
> 
> Also thank you for commenting on the other patches. I'll take a closer
> look at your feedback at the end of the week and send another version
> of this series.
> 

I also thank you for cooking these patches voluntarily for people who
can use their own wifi happily. :-)

Ping-Ke