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 Transfer
When 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.
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.
Regards,
Prashanth K
