[RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Larry Finger]

As discussed in the links below, the SDIO part of RTW8821CS fails to
start correctly if such startup happens while the UART portion of
the chip is initializing. The logged results with such failure is

[   10.230516] rtw_8821cs mmc3:0001:1: Start of rtw_sdio_probe
[   10.306569] Bluetooth: HCI UART driver ver 2.3
[   10.306717] Bluetooth: HCI UART protocol Three-wire (H5) registered
[   10.307167] of_dma_request_slave_channel: dma-names property of node '/serial@fe650000' missing or empty
[   10.307199] dw-apb-uart fe650000.serial: failed to request DMA
[   10.543474] rtw_8821cs mmc3:0001:1: Firmware version 24.8.0, H2C version 12
[   10.730744] rtw_8821cs mmc3:0001:1: sdio read32 failed (0x11080): -110
[   10.730923] rtw_8821cs mmc3:0001:1: sdio write32 failed (0x11080): -110

Due to the above errors, wifi fails to work.

For those instances when wifi works, the following is logged:

[   10.452861] Bluetooth: HCI UART protocol Three-wire (H5) registered
[   10.453580] of_dma_request_slave_channel: dma-names property of node '/serial@fe650000' missing or empty
[   10.453621] dw-apb-uart fe650000.serial: failed to request DMA
[   10.455741] rtw_8821cs mmc3:0001:1: Start of rtw_sdio_probe
[   10.639186] rtw_8821cs mmc3:0001:1: Firmware version 24.8.0, H2C version 12

In this case, SDIO wifi works correctly. The correct case is ensured by
adding an mdelay(500) statement before the call to rtw_core_init(). No
adverse effects are observed.

Link: https://1EHFQ.trk.elasticemail.com/tracking/click?d=1UfsVowwwMAM6kBoyumkHP3o7pYa4kGXhuklYI-QPLuVUi2ohkUG8HssjZcN67C_2TySHAezxTUVFT_8wvKkE9xqzm8H8qwhbclOJ9HB0cExNK65eHoXK4LaCW3PT7iyvMI3d6qqwN6PHhYj2GEblxeP4xr4CJPwZE7lyMCRTuxZ0
Link: https://1EHFQ.trk.elasticemail.com/tracking/click?d=XUEf4t8W9xt0czASPOeeDt8BnPqLK_YeGMMwadyXNu17p5TeSDk6RmEZ25rBt0-C5d-GR5IlKqu5URoKaespUAOAffgoysVTLbvgzQoAO57Ix1ChR-fYsf2VGRa1vIR9iWmNK1bUBvdHTZb0StprYVFMTYNi3fpihu3YoQYbaiZsYjqFovexmfiXfyHM3S00rqaJrXQwoylF4bELv8WaAC-QQM6OuDzE6rb32eW83smj0
Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
Signed-off-by: Larry Finger <Larry.Finger@gmail.com>
Cc: stable@vger.kernel.org # v6.4+
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 0cae5746f540..eec0ad85be72 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1325,6 +1325,34 @@ int rtw_sdio_probe(struct sdio_func *sdio_func,
 	rtwdev->hci.ops = &rtw_sdio_ops;
 	rtwdev->hci.type = RTW_HCI_TYPE_SDIO;
 
+	/* Insert a delay of 500 ms. Without the delay, the wifi part
+	 * and the UART that controls Bluetooth interfere with one
+	 * another resulting in the following being logged:
+	 *
+	 * Start of SDIO probe function.
+	 * Bluetooth: HCI UART driver ver 2.3
+	 * Bluetooth: HCI UART protocol Three-wire (H5) registered
+	 * of_dma_request_slave_channel: dma-names property of node '/serial@fe650000'
+	 *	 missing or empty
+	 * dw-apb-uart fe650000.serial: failed to request DMA
+`	 * rtw_8821cs mmc3:0001:1: Firmware version 24.8.0, H2C version 12
+	 * rtw_8821cs mmc3:0001:1: sdio read32 failed (0x11080): -110
+	 *
+	 * If the UART is finished initializing before the SDIO probe
+	 * function startw, the following is logged:
+	 *
+	 * Bluetooth: HCI UART protocol Three-wire (H5) registered
+	 * of_dma_request_slave_channel: dma-names property of node '/serial@fe650000'
+	 *	missing or empty
+	 * dw-apb-uart fe650000.serial: failed to request DMA
+	 * Start of SDIO probe function.
+	 * rtw_8821cs mmc3:0001:1: Firmware version 24.8.0, H2C version 12
+	 * Bluetooth: hci0: RTL: examining hci_ver=08 hci_rev=000c lmp_ver=08 lmp_subver=8821
+	 * SDIO wifi works correctly.
+	 *
+	 * No adverse effects are observed from the delay.
+	 */
+	mdelay(500);
 	ret = rtw_core_init(rtwdev);
 	if (ret)
 		goto err_release_hw;
-- 
2.44.0


https://1EHFQ.trk.elasticemail.com/tracking/unsubscribe?d=Q8ssbFKKxFyDYflgetP6nBeacBsR9zpdaz0g6MwIeA6CJKd2sQMmPI9ONTwNj6faC9CgSWZc53ZQ96HQFrbDSPngQHeCyqSokhmrQJQALWxI0

RE: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Ping-Ke Shih]

Larry Finger <Larry.Finger@gmail.com> wrote:

> As discussed in the links below, the SDIO part of RTW8821CS fails to
> start correctly if such startup happens while the UART portion of
> the chip is initializing. 

I checked with SDIO team internally, but they didn't meet this case, so we may
take this workaround.

SDIO team wonder if something other than BT cause this failure, and after
system boots everything will be well. Could you boot the system without WiFi/BT
drivers, but insmod drivers manually after booting?


> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index 0cae5746f540..eec0ad85be72 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1325,6 +1325,34 @@ int rtw_sdio_probe(struct sdio_func *sdio_func,

[...]

> +       mdelay(500);

Will it better to use sleep function? 

Re: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Larry Finger]

On 4/10/24 9:13 PM, Ping-Ke Shih wrote:
> 
> Larry Finger <Larry.Finger@gmail.com> wrote:
> 
>> As discussed in the links below, the SDIO part of RTW8821CS fails to
>> start correctly if such startup happens while the UART portion of
>> the chip is initializing.
> 
> I checked with SDIO team internally, but they didn't meet this case, so we may
> take this workaround.
> 
> SDIO team wonder if something other than BT cause this failure, and after
> system boots everything will be well. Could you boot the system without WiFi/BT
> drivers, but insmod drivers manually after booting?

I sent the request to the user with the problem. I do not have any SDIO devices.

> 
> 
>> ---
>>   drivers/net/wireless/realtek/rtw88/sdio.c | 28 +++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
>> index 0cae5746f540..eec0ad85be72 100644
>> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
>> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
>> @@ -1325,6 +1325,34 @@ int rtw_sdio_probe(struct sdio_func *sdio_func,
> 
> [...]
> 
>> +       mdelay(500);
> 
> Will it better to use sleep function?

My thoughts were that a sleep function would tie up a CPU, whereas the delay 
would not. Initially, we tested an msleep(150) statement, but that only gave a 
60% success rate, whereas mdelay(500) worked 20 straight tries.

Thanks for your comments.

Larry

RE: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Ping-Ke Shih]

Larry Finger <larry.finger@gmail.com> wrote:

> 
> On 4/10/24 9:13 PM, Ping-Ke Shih wrote:
> >
> > Larry Finger <Larry.Finger@gmail.com> wrote:
> >
> >> +       mdelay(500);
> >
> > Will it better to use sleep function?
> 
> My thoughts were that a sleep function would tie up a CPU, whereas the delay
> would not. Initially, we tested an msleep(150) statement, but that only gave a
> 60% success rate, whereas mdelay(500) worked 20 straight tries.
> 

Sorry, I didn't consider the experimental results of msleep(150) and mdelay(500).

My point was busy waiting of mdelay(). I just want to say if msleep(500) is 
better than mdelay(500). 

Re: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Kalle Valo]

(dropping stable list from cc)

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 4/10/24 9:13 PM, Ping-Ke Shih wrote:
>> Larry Finger <Larry.Finger@gmail.com> wrote:
>> 
>>> As discussed in the links below, the SDIO part of RTW8821CS fails to
>>> start correctly if such startup happens while the UART portion of
>>> the chip is initializing.
>> I checked with SDIO team internally, but they didn't meet this case,
>> so we may
>> take this workaround.
>> SDIO team wonder if something other than BT cause this failure, and
>> after
>> system boots everything will be well. Could you boot the system without WiFi/BT
>> drivers, but insmod drivers manually after booting?
>
> I sent the request to the user with the problem. I do not have any SDIO devices.
>
>> 
>>> ---
>>>   drivers/net/wireless/realtek/rtw88/sdio.c | 28 +++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
>>> index 0cae5746f540..eec0ad85be72 100644
>>> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
>>> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
>>> @@ -1325,6 +1325,34 @@ int rtw_sdio_probe(struct sdio_func *sdio_func,
>> [...]
>> 
>>> +       mdelay(500);
>> Will it better to use sleep function?
>
> My thoughts were that a sleep function would tie up a CPU, whereas the
> delay would not.

It's actually the opposite, msleep() allows other processes to run.

"In general, use of mdelay is discouraged and code should be refactored
to allow for the use of msleep."

https://docs.kernel.org/timers/timers-howto.html

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

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

Re: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Larry Finger]

On 4/11/24 2:08 AM, Kalle Valo wrote:
> (dropping stable list from cc)
> 
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
>> On 4/10/24 9:13 PM, Ping-Ke Shih wrote:
>>> Larry Finger <Larry.Finger@gmail.com> wrote:
>>>
>>>> As discussed in the links below, the SDIO part of RTW8821CS fails to
>>>> start correctly if such startup happens while the UART portion of
>>>> the chip is initializing.
>>> I checked with SDIO team internally, but they didn't meet this case,
>>> so we may
>>> take this workaround.
>>> SDIO team wonder if something other than BT cause this failure, and
>>> after
>>> system boots everything will be well. Could you boot the system without WiFi/BT
>>> drivers, but insmod drivers manually after booting?
>>
>> I sent the request to the user with the problem. I do not have any SDIO devices.
>>
>>>
>>>> ---
>>>>    drivers/net/wireless/realtek/rtw88/sdio.c | 28 +++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
>>>> index 0cae5746f540..eec0ad85be72 100644
>>>> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
>>>> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
>>>> @@ -1325,6 +1325,34 @@ int rtw_sdio_probe(struct sdio_func *sdio_func,
>>> [...]
>>>
>>>> +       mdelay(500);
>>> Will it better to use sleep function?
>>
>> My thoughts were that a sleep function would tie up a CPU, whereas the
>> delay would not.
> 
> It's actually the opposite, msleep() allows other processes to run.
> 
> "In general, use of mdelay is discouraged and code should be refactored
> to allow for the use of msleep."
> 
> https://docs.kernel.org/timers/timers-howto.html

Kalle and Ping-ke,

Sorry that I got that wrong. I have become just another confused old man. :)

The OP at GitHub has tested kernels where he blacklisted the BT drivers. The 
wifi failed every time (3 tries with a delay and 3 times without). When he built 
a kernel with BT built-in rather than as a module, it worked (again 3 tries). 
There definitely is an interaction between the BT initialization and the wifi 
setup. If the BT part is not started and has not completed the UART 
initialization, SDIO wifi does not initialize correctly.

When it fails, unloading the wifi driver and reloading it restores wifi.

I originally wondered if there was a power problem when both were simultaneously 
started, but I see that is not the case. To reiterate, the BT must start first 
and complete UART initialization before the wifi starts initializing. It seems 
to me that a suitable msleep() at the start of the SDIO probe routine seems to 
be a viable workaround even though it is not aesthetically pleasing. A sleep of 
150 ms is too short, but 500 seems to work reliably.

Perhaps the SDIO experts at Realtek may be able to offer a more satisfying 
explanation. The OP at GitHub and I will try to answer any questions they might 
have. Note that I do not have the hardware, but my contact at GitHub is 
knowledgeable, and seems eager to help.

Larry

RE: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Ping-Ke Shih]

Larry Finger <larry.finger@gmail.com> wrote:
> 
> I originally wondered if there was a power problem when both were simultaneously
> started, but I see that is not the case. To reiterate, the BT must start first
> and complete UART initialization before the wifi starts initializing. It seems
> to me that a suitable msleep() at the start of the SDIO probe routine seems to
> be a viable workaround even though it is not aesthetically pleasing. A sleep of
> 150 ms is too short, but 500 seems to work reliably.

Suggest to apply this workaround but only if UART-BT + SDIO-WiFi, because SDIO
experts didn't remember they have met this problem. They need real hardware to
measure signals to know what it is wrong, but unfortunately they don't have
bandwidth to process this because of limitation of human resources. Sorry
for that.

Ping-Ke

Re: [RFC] rtw88: Fix startup problems for SDIO wifi plus UART Bluetooth

[Larry Finger]

On 4/14/24 7:45 PM, Ping-Ke Shih wrote:
> Larry Finger <larry.finger@gmail.com> wrote:
>>
>> I originally wondered if there was a power problem when both were simultaneously
>> started, but I see that is not the case. To reiterate, the BT must start first
>> and complete UART initialization before the wifi starts initializing. It seems
>> to me that a suitable msleep() at the start of the SDIO probe routine seems to
>> be a viable workaround even though it is not aesthetically pleasing. A sleep of
>> 150 ms is too short, but 500 seems to work reliably.
> 
> Suggest to apply this workaround but only if UART-BT + SDIO-WiFi, because SDIO
> experts didn't remember they have met this problem. They need real hardware to
> measure signals to know what it is wrong, but unfortunately they don't have
> bandwidth to process this because of limitation of human resources. Sorry
> for that.

The OP at GitHub reported today that there was a DTB error for that chip [1]. I 
am going to drop the patch for the stable kernel, but I will still apply it to 
my rtw88 repo at GitHub.com. That way, users with older kernels will get the 
benefit of the msleep() even though their DTB may not be fixed.

Thanks to you and the SDIO team for your efforts toward a problem that turned 
out to be in the kernel. Sorry for some noise.

Larry


[1] 
https://github.com/ROCKNIX/distribution/pull/63/files#diff-e2de6222b1794f89311bdc1597c1597c76f34503c575ea3f7e7ec9c5376218d6