> -----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.
> > @@ -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?
> > /* 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.
>
> 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.
Another note is that check_hw_ready() is also used by many other places.
> 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.
Ping-Ke
