Hi,
On Wed, Aug 07, 2024, Prashanth K wrote:
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.
No need for this polling. When you receive wakeup event, it's already in the state that you can issue Start Transfer command.
@@ -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.
Polling for 20K time is a bit much, and this will vary depending on different setup. This is something that I want to fix in the wakeup() ops and keep everything async.
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.
usb_ep_queue should not trigger remote wakeup; it should be done by wakeup() ops. The programming guide just noted that the Start Transfer command should not be issued while in L1/L2/U3. It suggested to wake up the host to bring it out of L1/L2/U3 state so the command can go through.
My suggestion is to remove the L1/L2/U3 check in dwc3_send_gadget_ep_cmd(), and it will still work fine with reasons noted previously. So, just do this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 0ea2ca0f0d28..6ef6c4ef2a7b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -411,30 +411,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); }
- if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { - int link_state; - - /* - * Initiate remote wakeup if the link state is in U3 when - * operating in SS/SSP or L1/L2 when operating in HS/FS. If the - * link state is in U1/U2, no remote wakeup is needed. The Start - * Transfer command will initiate the link recovery. - */ - link_state = dwc3_gadget_get_link_state(dwc); - switch (link_state) { - case DWC3_LINK_STATE_U2: - if (dwc->gadget->speed >= USB_SPEED_SUPER) - break; - - fallthrough; - case DWC3_LINK_STATE_U3: - ret = __dwc3_gadget_wakeup(dwc, false); - dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n", - ret); - break; - } - } - /* * For some commands such as Update Transfer command, DEPCMDPARn * registers are reserved. Since the driver often sends Update Transfer
When we receive the wakeup event, then the device is no longer in L1/L2/U3. The Start Tranfer command should go through.
We do have an issue where if the function driver issues remote wakeup, the link may not transition before ep_queue() because wakeup() can be async. In that case, you probably want to keep the usb_requests in the pending_list until the link_state transitions out of low power.
The other thing that I noted previously is that I want to fix is the wakeup() ops. Currently it can be async or synchronous. We should keep it consistent and make it async throughout.
BR, Thinh