On Jul 16, 2023, at 9:05 PM, Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:
>
>
>
>> -----Original Message-----
>> From: Larry Finger <larry.finger@xxxxxxxxx> On Behalf Of Larry Finger
>> Sent: Monday, July 17, 2023 2:02 AM
>> To: Sean Mollet <sean@xxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH] RTW88 firmware download issues - improvement, but not perfect
>>
>>
>> Patches for the rtlwifi, rtw88, and rtw89 drivers should be addressed to Ping-Ke
>> Shih <pkshih@xxxxxxxxxxx>. Any patch for a driver in drivers/net/wireless/...
>> should be sent to Kalle Valo <kvalo@xxxxxxxxxx>. Cc linux-wireless.
>
> I subscribe this mailing list and treat rtlwifi/rtw88/rtw89 with special filter, so
> I think I don't miss this mail. :-)
>
>>
>> The subject line should be "wifi: rtw88: ......"
>
> I think this should be "RFC" instead of "PATCH" as subject.
Done.
>>> @@ -794,15 +794,15 @@ static int __rtw_download_firmware(struct rtw_dev *rtwdev,
>>>
>>> wlan_cpu_enable(rtwdev, true);
>>>
>>> - if (!ltecoex_reg_write(rtwdev, 0x38, ltecoex_bckp)) {
>>> - ret = -EBUSY;
>>> - goto dlfw_fail;
>>> - }
>>> -
>>> ret = download_firmware_validate(rtwdev);
>>> if (ret)
>>> goto dlfw_fail;
>>>
>>> + if (!ltecoex_reg_write(rtwdev, 0x38, ltecoex_bckp)) {
>>> + ret = -EBUSY;
>>> + goto dlfw_fail;
>>> + }
>>> +
>
> This looks reason to restore 0x38 after validating firmware. Do you have a result
> how this change can improve?
>
Using a Pi 4 CM as host, this reduces failures from 1 in 5 to 1 in 20.
I don’t know why, but it makes a measurable difference.
>>> /* reset desc and index */
>>> rtw_hci_setup(rtwdev);
>>>
>>> diff --git a/util.c b/util.c
>>> index ff3c269..fbd6599 100644
>>> --- a/util.c
>>> +++ b/util.c
>>> @@ -10,11 +10,11 @@ bool check_hw_ready(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 target)
>>> {
>>> u32 cnt;
>>>
>>> - for (cnt = 0; cnt < 1000; cnt++) {
>>> + for (cnt = 0; cnt < 5000; cnt++) {
>>> if (rtw_read32_mask(rtwdev, addr, mask) == target)
>>> return true;
>>>
>>> - udelay(10);
>>> + udelay(50);
>
> I look into the latest vendor driver, it shows that cnt becomes 10,000 and delay
> is 50us as your change.
Interesting. Is it possible that the real problem is simply not waiting long enough?
Can you share some details of what the chip is doing and how long it should take?
>
>>
>> You have increased the maximum stall time from 10 msec to 250 msec. Do you
>> really need to lock up a CPU for that long? This is a place where you should
>> document how long it actually takes, if it really is more than 10 msec. On my
>> rtw8821ce card, the longest it took was 6.25 msec. The USB device will likely
>> take longer, but I would be interested in your worst case.
>
> Maybe, we can set cnt/udelay according to 'rtwdev->hci.type', and change udelay()
> to fsleep() if all callers are running on thread context.
I like this. If we fsleep all the time, there’s no real cost to having a large ucnt.
>
> Another note is that check_hw_ready() is also used by many other places.
>
Yes. Nearly all of those calls return right away though (I added prints to check for this.)
>> FYI, I changed
>> check_hw_ready() to read
>>
>> for (cnt = 0; cnt < 5000; cnt++) {
>> if (rtw_read32_mask(rtwdev, addr, mask) == target) {
>> if (cnt > 50)
>> pr_info("hw_ready at count %d\n", cnt);
>> return true;
>> }
>>
>> udelay(50);
>
> This looks weird. If udelay() isn't in loop, PCIE device can run quickly and get
> a result "not ready". But, for slow IO USB/SDIO, this might be fine.
>
My average when firmware load succeeds is less than 10. I’ll try increasing ucnt to 10,000 tomorrow and see if perhaps some complete at more than 5000.
> Ping-Ke
Sean
>
