Message-ID: <ec99fcdc-9404-8cd9-6a30-95e4f5c1edcd@quicinc.com>
Date: Wed, 7 Aug 2024 15:11:13 +0530
From: Prashanth K <quic_prashk@...cinc.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Fix latency of DSTS while receiving wakeup
 event



On 07-08-24 05:21 am, Thinh Nguyen wrote:
> Hi,
> 
> On Tue, Jul 30, 2024, Prashanth K wrote:
>> When operating in High-Speed, it is observed that DSTS[USBLNKST] doesn't
>> update link state immediately after receiving the wakeup interrupt. Since
>> wakeup event handler calls the resume callbacks, there is a chance that
>> function drivers can perform an ep queue. Which in turn tries to perform
>> remote wakeup from send_gadget_ep_cmd(), this happens because DSTS[[21:18]
>> wasn't updated to U0 yet. It is observed that the latency of DSTS can be
>> in order of milli-seconds. Hence update the dwc->link_state from evtinfo,
>> and use this variable to prevent calling remote wakup unnecessarily.
>>
>> Fixes: ecba9bc9946b ("usb: dwc3: gadget: Check for L1/L2/U3 for Start Transfer")
> 
> This commit ID is corrupted. Please check.
> 
Will fix it, was supposed to be 63c4c320ccf7, thanks for pointing out.

> While operating in usb2 speed, if the device is in low power link state
> (L1/L2), CMDACT may not complete and time out. The programming guide
> suggested to initiate remote wakeup to bring the device to ON state,
> allowing the command to go through. However, clearing the

Yea true, we need ensure that the linkstate is not in L1/L2/U3 for 
HS/SS. But since we are relying on DSTS for this, we may issue 
remote-wakeup to host even when not needed. During host initiated wakeup 
scenario, we get a wakeup interrupt which calls function driver resume 
calls. If function driver queues something, then startxfer has to be 
issued, but DSTS was still showing U3 instead of U0. When checked with 
our design team, they mentioned the latency in DSTS is expected since 
and latency would be in msec order from Resume to U0. Can you please 
confirm this once, I simply added a polling mechanism in wakeup handler.

@@ -4175,6 +4177,14 @@ static void dwc3_gadget_wakeup_interrupt(struct 
dwc3 *dwc, unsigned int evtinfo)
          * TODO take core out of low power mode when that's
          * implemented.
          */
+       while (retries++ < 20000) {
+               reg = dwc3_readl(dwc->regs, DWC3_DSTS);
+               /* in HS, means ON */
+               if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
+                       break;
+               udelay(2);
+       }
+       pr_info("DWC3 Wakeup: %d", retries);

And turns out, retries 1500 to 15000 (worst case), which can range from 
3ms to 30ms. By this time, control can reach startXfer, where it tries 
to perform remote-wakeup even if host just resumed the gadget.

For SS case, this retries count was consistently 1, it was passing in 
first try itself. But unfortunately doesn't behave the same way in HS.

> GUSB2PHYCFG.suspendusb2 turns on the signal required to complete a
> command within 50us. This happens within the timeout required for an
> endpoint command. As a result, there's no need to perform remote wakeup.
> 
> For usb3 speed, if it's in U3, the gadget is in suspend anyway. There
> will be no ep_queue to trigger the Start Transfer command.
> 
> You can just remove the whole Start Transfer check for remote wakeup
> completely.
> 
Sorry, i didnt understand your suggestion. The startxfer check is needed 
as per databook, but we also need to handle the latency seen in DSTS 
when operating in HS.

Thanks,
Prashanth K

Powered by blists - more mailing lists