Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Sean Mollet <sean@malmoset.com>
Mime-Version: 1.0 (1.0)
Subject: [RFC] RTW88 firmware download issues - improvement, but not perfect
Date:   Sun, 16 Jul 2023 21:24:25 -0500
Message-Id: <ACAD9039-DD62-477E-BBCF-B8053822E042@malmoset.com>
References: <feadf58594b14c67bcf981dde6a60077@realtek.com>
Cc:     Larry Finger <Larry.Finger@lwfinger.net>,
        linux-wireless@vger.kernel.org
In-Reply-To: <feadf58594b14c67bcf981dde6a60077@realtek.com>
To:     Ping-Ke Shih <pkshih@realtek.com>
Precedence: bulk


On Jul 16, 2023, at 9:05 PM, Ping-Ke Shih <pkshih@realtek.com> wrote:
>=20
> =EF=BB=BF
>=20
>> -----Original Message-----
>> From: Larry Finger <larry.finger@gmail.com> On Behalf Of Larry Finger
>> Sent: Monday, July 17, 2023 2:02 AM
>> To: Sean Mollet <sean@malmoset.com>; linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH] RTW88 firmware download issues - improvement, but no=
t perfect
>>=20
>>=20
>> Patches for the rtlwifi, rtw88, and rtw89 drivers should be addressed to P=
ing-Ke
>> Shih <pkshih@realtek.com>. Any patch for a driver in drivers/net/wireless=
/...
>> should be sent to Kalle Valo <kvalo@kernel.org>. Cc linux-wireless.
>=20
> I subscribe this mailing list and treat rtlwifi/rtw88/rtw89 with special f=
ilter, so
> I think I don't miss this mail. :-)
>=20
>>=20
>> The subject line should be "wifi: rtw88: ......"
>=20
> I think this should be "RFC" instead of "PATCH" as subject.=20

Done.

>>> @@ -794,15 +794,15 @@ static int __rtw_download_firmware(struct rtw_dev *=
rtwdev,
>>>=20
>>>         wlan_cpu_enable(rtwdev, true);
>>>=20
>>> -       if (!ltecoex_reg_write(rtwdev, 0x38, ltecoex_bckp)) {
>>> -               ret =3D -EBUSY;
>>> -               goto dlfw_fail;
>>> -       }
>>> -
>>>         ret =3D download_firmware_validate(rtwdev);
>>>         if (ret)
>>>                 goto dlfw_fail;
>>>=20
>>> +       if (!ltecoex_reg_write(rtwdev, 0x38, ltecoex_bckp)) {
>>> +               ret =3D -EBUSY;
>>> +               goto dlfw_fail;
>>> +       }
>>> +
>=20
> This looks reason to restore 0x38 after validating firmware. Do you have a=
 result=20
> how this change can improve?=20
>=20

Using a Pi 4 CM as host, this reduces failures from 1 in 5 to 1 in 20.

I don=E2=80=99t know why, but it makes a measurable difference.

>>>         /* reset desc and index */
>>>         rtw_hci_setup(rtwdev);
>>>=20
>>> 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;
>>>=20
>>> -       for (cnt =3D 0; cnt < 1000; cnt++) {
>>> +       for (cnt =3D 0; cnt < 5000; cnt++) {
>>>                 if (rtw_read32_mask(rtwdev, addr, mask) =3D=3D target)
>>>                         return true;
>>>=20
>>> -               udelay(10);
>>> +               udelay(50);
>=20
> I look into the latest vendor driver, it shows that cnt becomes 10,000 and=
 delay
> is 50us as your change.=20
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 t=
ake?


>=20
>>=20
>> You have increased the maximum stall time from 10 msec to 250 msec. Do yo=
u
>> really need to lock up a CPU for that long? This is a place where you sho=
uld
>> document how long it actually takes, if it really is more than 10 msec. O=
n my
>> rtw8821ce card, the longest it took was 6.25 msec. The USB device will li=
kely
>> take longer, but I would be interested in your worst case.
>=20
> Maybe, we can set cnt/udelay according to 'rtwdev->hci.type', and change u=
delay()
> to fsleep() if all callers are running on thread context.=20
I like this. If we fsleep all the time, there=E2=80=99s no real cost to havi=
ng a large ucnt.=20

>=20
> Another note is that check_hw_ready() is also used by many other places.
>=20
Yes. Nearly all of those calls return right away though (I added prints to c=
heck for this.)

>> FYI, I changed
>> check_hw_ready() to read
>>=20
>>                 for (cnt =3D 0; cnt < 5000; cnt++) {
>>                         if (rtw_read32_mask(rtwdev, addr, mask) =3D=3D ta=
rget) {
>>                                 if (cnt > 50)
>>                                         pr_info("hw_ready at count %d\n",=
 cnt);
>>                         return true;
>>                 }
>>=20
>>                 udelay(50);
>=20
> This looks weird. If udelay() isn't in loop, PCIE device can run quickly a=
nd get
> a result "not ready". But, for slow IO USB/SDIO, this might be fine.=20
>=20

My average when firmware load succeeds is less than 10. I=E2=80=99ll try inc=
reasing ucnt to 10,000 tomorrow and see if perhaps some complete at more tha=
n 5000.

> Ping-Ke

Sean

>=20