[PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Ping-Ke Shih]

The register REG_SYS_CFG1 is used to determine chip basic information
as arguments of following flows, such as download firmware and load PHY
parameters, so driver read the value early (before power-on).

However, the direct IO is disallowed before power-on, or it causes wrong
values, which driver recognizes a chip as a wrong type RF_1T1R, but
actually RF_2T2R, causing driver warns:

  rtw88_8822cs mmc1:0001:1: unsupported rf path (1)

Fix it by using indirect IO before power-on.

Reported-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
Closes: https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index cc2d4fef3587..99d7c629eac6 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -144,6 +144,10 @@ static u32 rtw_sdio_to_io_address(struct rtw_dev *rtwdev, u32 addr,
 
 static bool rtw_sdio_use_direct_io(struct rtw_dev *rtwdev, u32 addr)
 {
+	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags) &&
+	    !rtw_sdio_is_bus_addr(addr))
+		return false;
+
 	return !rtw_sdio_is_sdio30_supported(rtwdev) ||
 		rtw_sdio_is_bus_addr(addr);
 }
-- 
2.25.1

RE: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Ping-Ke Shih]

Ping-Ke Shih <pkshih@realtek.com> wrote:
> The register REG_SYS_CFG1 is used to determine chip basic information
> as arguments of following flows, such as download firmware and load PHY
> parameters, so driver read the value early (before power-on).
> 
> However, the direct IO is disallowed before power-on, or it causes wrong
> values, which driver recognizes a chip as a wrong type RF_1T1R, but
> actually RF_2T2R, causing driver warns:
> 
>   rtw88_8822cs mmc1:0001:1: unsupported rf path (1)
> 
> Fix it by using indirect IO before power-on.
> 
> Reported-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
> Closes: https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

Pick from [1] & remove mailto:

Tested-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>

[1] https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#m5af73c4f7f35f16071392f395ebec10e36d39128

> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index cc2d4fef3587..99d7c629eac6 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -144,6 +144,10 @@ static u32 rtw_sdio_to_io_address(struct rtw_dev *rtwdev, u32 addr,
> 
>  static bool rtw_sdio_use_direct_io(struct rtw_dev *rtwdev, u32 addr)
>  {
> +	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags) &&
> +	    !rtw_sdio_is_bus_addr(addr))
> +		return false;
> +
>  	return !rtw_sdio_is_sdio30_supported(rtwdev) ||
>  		rtw_sdio_is_bus_addr(addr);
>  }
> --
> 2.25.1
> 

RE: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Martin Blumenstingl]

> The register REG_SYS_CFG1 is used to determine chip basic information
> as arguments of following flows, such as download firmware and load PHY
> parameters, so driver read the value early (before power-on).
> 
> However, the direct IO is disallowed before power-on, or it causes wrong
> values, which driver recognizes a chip as a wrong type RF_1T1R, but
> actually RF_2T2R, causing driver warns:
> 
>   rtw88_8822cs mmc1:0001:1: unsupported rf path (1)
> 
> Fix it by using indirect IO before power-on.
> 
> Reported-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
> Closes: https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks for finding and fixing this!
The fix looks correct since indirect access (according to the vendor
driver) is needed for both reads and writes (which is what the
approach from this patch achives).

Re: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Ping-Ke Shih]

Ping-Ke Shih <pkshih@realtek.com> wrote:

> The register REG_SYS_CFG1 is used to determine chip basic information
> as arguments of following flows, such as download firmware and load PHY
> parameters, so driver read the value early (before power-on).
> 
> However, the direct IO is disallowed before power-on, or it causes wrong
> values, which driver recognizes a chip as a wrong type RF_1T1R, but
> actually RF_2T2R, causing driver warns:
> 
>   rtw88_8822cs mmc1:0001:1: unsupported rf path (1)
> 
> Fix it by using indirect IO before power-on.
> 
> Reported-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
> Closes: https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> Tested-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

1 patch(es) applied to rtw-next branch of rtw.git, thanks.

58de1f91e033 wifi: rtw88: sdio: use indirect IO for device registers before power-on

---
https://github.com/pkshih/rtw.git

Re: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Andrey Skvortsov]

Hi,

This patch was recently backported to stable kernels (v6.12.58) and it broke
wlan on PinePhone, that uses 8723cs SDIO chip. The same problem
appears of course on latest 6.18-rc6. Reverting this change resolves
the problem.

```
$ sudo dmesg | grep -i rtw88
[   24.940551] rtw88_8723cs mmc1:0001:1: WOW Firmware version 11.0.0, H2C version 0
[   24.953085] rtw88_8723cs mmc1:0001:1: Firmware version 11.0.0, H2C version 0
[   24.955892] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
[   24.973135] rtw88_8723cs mmc1:0001:1: sdio write8 failed (0x1c): -110
[   24.980673] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
...
[   25.446691] rtw88_8723cs mmc1:0001:1: sdio read8 failed (0x100): -110
[   25.453569] rtw88_8723cs mmc1:0001:1: mac power on failed
[   25.459077] rtw88_8723cs mmc1:0001:1: failed to power on mac
[   25.464841] rtw88_8723cs mmc1:0001:1: failed to setup chip efuse info
[   25.464856] rtw88_8723cs mmc1:0001:1: failed to setup chip information
[   25.478341] rtw88_8723cs mmc1:0001:1: probe with driver rtw88_8723cs failed with error -114
```

On 25-07-24 08:48, Ping-Ke Shih wrote:
> The register REG_SYS_CFG1 is used to determine chip basic information
> as arguments of following flows, such as download firmware and load PHY
> parameters, so driver read the value early (before power-on).
> 
> However, the direct IO is disallowed before power-on, or it causes wrong
> values, which driver recognizes a chip as a wrong type RF_1T1R, but
> actually RF_2T2R, causing driver warns:
> 
>   rtw88_8822cs mmc1:0001:1: unsupported rf path (1)
> 
> Fix it by using indirect IO before power-on.
> 
> Reported-by: Piotr Oniszczuk <piotr.oniszczuk@gmail.com>
> Closes: https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index cc2d4fef3587..99d7c629eac6 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -144,6 +144,10 @@ static u32 rtw_sdio_to_io_address(struct rtw_dev *rtwdev, u32 addr,
>  
>  static bool rtw_sdio_use_direct_io(struct rtw_dev *rtwdev, u32 addr)
>  {
> +	if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags) &&
> +	    !rtw_sdio_is_bus_addr(addr))
> +		return false;
> +
>  	return !rtw_sdio_is_sdio30_supported(rtwdev) ||
>  		rtw_sdio_is_bus_addr(addr);
>  }
> -- 
> 2.25.1
> 

-- 
Best regards,
Andrey Skvortsov

RE: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Ping-Ke Shih]

Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> Hi,
> 
> This patch was recently backported to stable kernels (v6.12.58) and it broke
> wlan on PinePhone, that uses 8723cs SDIO chip. The same problem
> appears of course on latest 6.18-rc6. Reverting this change resolves
> the problem.
> 
> ```
> $ sudo dmesg | grep -i rtw88
> [   24.940551] rtw88_8723cs mmc1:0001:1: WOW Firmware version 11.0.0, H2C version 0
> [   24.953085] rtw88_8723cs mmc1:0001:1: Firmware version 11.0.0, H2C version 0
> [   24.955892] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
> [   24.973135] rtw88_8723cs mmc1:0001:1: sdio write8 failed (0x1c): -110
> [   24.980673] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
> ...
> [   25.446691] rtw88_8723cs mmc1:0001:1: sdio read8 failed (0x100): -110
> [   25.453569] rtw88_8723cs mmc1:0001:1: mac power on failed
> [   25.459077] rtw88_8723cs mmc1:0001:1: failed to power on mac
> [   25.464841] rtw88_8723cs mmc1:0001:1: failed to setup chip efuse info
> [   25.464856] rtw88_8723cs mmc1:0001:1: failed to setup chip information
> [   25.478341] rtw88_8723cs mmc1:0001:1: probe with driver rtw88_8723cs failed with error -114
> ```
> 

Check original link of this patch [1] that 8822cs read incorrect from 0xf0
resulting in "rtw88_8822cs mmc1:0001:1: unsupported rf path (1)".

I wonder if we can add additional checking rule of chip ID, like:

--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -144,8 +144,10 @@ static u32 rtw_sdio_to_io_address(struct rtw_dev *rtwdev, u32 addr,

 static bool rtw_sdio_use_direct_io(struct rtw_dev *rtwdev, u32 addr)
 {
+       bool might_indirect_under_power_off = rtwdev->chip->id != RTW_CHIP_TYPE_8703B;
+
        if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags) &&
-           !rtw_sdio_is_bus_addr(addr))
+           !rtw_sdio_is_bus_addr(addr) && might_indirect_under_power_off)
                return false;

        return !rtw_sdio_is_sdio30_supported(rtwdev) ||

[1] https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t

Re: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Andrey Skvortsov]

On 25-11-24 08:16, Ping-Ke Shih wrote:
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > Hi,
> > 
> > This patch was recently backported to stable kernels (v6.12.58) and it broke
> > wlan on PinePhone, that uses 8723cs SDIO chip. The same problem
> > appears of course on latest 6.18-rc6. Reverting this change resolves
> > the problem.
> > 
> > ```
> > $ sudo dmesg | grep -i rtw88
> > [   24.940551] rtw88_8723cs mmc1:0001:1: WOW Firmware version 11.0.0, H2C version 0
> > [   24.953085] rtw88_8723cs mmc1:0001:1: Firmware version 11.0.0, H2C version 0
> > [   24.955892] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
I see it fails with timeout on the first operation to read chip version.

> > [   24.973135] rtw88_8723cs mmc1:0001:1: sdio write8 failed (0x1c): -110
> > [   24.980673] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
> > ...
> > [   25.446691] rtw88_8723cs mmc1:0001:1: sdio read8 failed (0x100): -110
> > [   25.453569] rtw88_8723cs mmc1:0001:1: mac power on failed
> > [   25.459077] rtw88_8723cs mmc1:0001:1: failed to power on mac
> > [   25.464841] rtw88_8723cs mmc1:0001:1: failed to setup chip efuse info
> > [   25.464856] rtw88_8723cs mmc1:0001:1: failed to setup chip information
> > [   25.478341] rtw88_8723cs mmc1:0001:1: probe with driver rtw88_8723cs failed with error -114
> > ```
> > 
> 
> Check original link of this patch [1] that 8822cs read incorrect from 0xf0
> resulting in "rtw88_8822cs mmc1:0001:1: unsupported rf path (1)".
Sorry, I haven't meant, that this patch should be reverted. I see,
that it solves real problem. I've meant, that patch caused this
regression was identified and it was confirmed by reverting it.

> I wonder if we can add additional checking rule of chip ID, like:

I've tried suggested change and wlan is recognized again.

> 
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -144,8 +144,10 @@ static u32 rtw_sdio_to_io_address(struct rtw_dev *rtwdev, u32 addr,
> 
>  static bool rtw_sdio_use_direct_io(struct rtw_dev *rtwdev, u32 addr)
>  {
> +       bool might_indirect_under_power_off = rtwdev->chip->id != RTW_CHIP_TYPE_8703B;
> +
>         if (!test_bit(RTW_FLAG_POWERON, rtwdev->flags) &&
> -           !rtw_sdio_is_bus_addr(addr))
> +           !rtw_sdio_is_bus_addr(addr) && might_indirect_under_power_off)
>                 return false;
> 
>         return !rtw_sdio_is_sdio30_supported(rtwdev) ||
> 
> [1] https://lore.kernel.org/linux-wireless/699C22B4-A3E3-4206-97D0-22AB3348EBF6@gmail.com/T/#t
> 

-- 
Best regards,
Andrey Skvortsov

RE: [PATCH rtw-next] wifi: rtw88: sdio: use indirect IO for device registers before power-on

[Ping-Ke Shih]

Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> On 25-11-24 08:16, Ping-Ke Shih wrote:
> > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > > Hi,
> > >
> > > This patch was recently backported to stable kernels (v6.12.58) and it broke
> > > wlan on PinePhone, that uses 8723cs SDIO chip. The same problem
> > > appears of course on latest 6.18-rc6. Reverting this change resolves
> > > the problem.
> > >
> > > ```
> > > $ sudo dmesg | grep -i rtw88
> > > [   24.940551] rtw88_8723cs mmc1:0001:1: WOW Firmware version 11.0.0, H2C version 0
> > > [   24.953085] rtw88_8723cs mmc1:0001:1: Firmware version 11.0.0, H2C version 0
> > > [   24.955892] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
> I see it fails with timeout on the first operation to read chip version.
> 
> > > [   24.973135] rtw88_8723cs mmc1:0001:1: sdio write8 failed (0x1c): -110
> > > [   24.980673] rtw88_8723cs mmc1:0001:1: sdio read32 failed (0xf0): -110
> > > ...
> > > [   25.446691] rtw88_8723cs mmc1:0001:1: sdio read8 failed (0x100): -110
> > > [   25.453569] rtw88_8723cs mmc1:0001:1: mac power on failed
> > > [   25.459077] rtw88_8723cs mmc1:0001:1: failed to power on mac
> > > [   25.464841] rtw88_8723cs mmc1:0001:1: failed to setup chip efuse info
> > > [   25.464856] rtw88_8723cs mmc1:0001:1: failed to setup chip information
> > > [   25.478341] rtw88_8723cs mmc1:0001:1: probe with driver rtw88_8723cs failed with error -114
> > > ```
> > >
> >
> > Check original link of this patch [1] that 8822cs read incorrect from 0xf0
> > resulting in "rtw88_8822cs mmc1:0001:1: unsupported rf path (1)".
> Sorry, I haven't meant, that this patch should be reverted. I see,
> that it solves real problem. I've meant, that patch caused this
> regression was identified and it was confirmed by reverting it.

I just want to point out the original problem we encountered, and the user
weirdly get unexpected value from 0xF0 without any error. 

> 
> > I wonder if we can add additional checking rule of chip ID, like:
> 
> I've tried suggested change and wlan is recognized again.
> 

I will check if vendor driver does similar things, and then I can make
a formal patch for 8723CS.