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 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.
>
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
