On Tue, Aug 13, 2024, Prashanth K wrote:
On 08-08-24 05:36 am, Thinh Nguyen wrote:
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.
This was done as part of experiment, just to determine the latency in DSTS. And it was around 3-30ms. Saw rhis same behaviour when polling DSTS in __dwc3_gadget_wakeup(sync)
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 TransferWhen we receive the wakeup event, then the device is no longer in L1/L2/U3. The Start Tranfer command should go through. >
Ok will do this, I hope there won't be any corner cases where the link is down when start_xfer happens. I was not really sure about the history, thats why tried to incorporate my fix into the above IF check.
It was initially implemented verbatim base on the Start Transfer command suggestion from the programming guide without considering the dwc3 driver flow. First dwc3 checks for U1/U2/U3 state. Then we fixed to only check for L1/L2/U3 state, but it's still not right. I've had this on my TODO list for awhile and haven't made an update since it's not critical.
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.
Sounds like a good idea, we can move the req to pending list, then issue async wakeup, and queue it back once linksts_change interrupt indicates L0/U0. Special care is needed in dwc3_gadget_func_wakeup() when making it async.
Yes. That would be great.
Thanks, Thinh