| Message ID | 20240730124742.561408-1-quic_prashk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Fix latency of DSTS while receiving wakeup event | expand |
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. 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 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. Thanks, Thinh
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
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
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
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
On 14-08-24 05:00 am, Thinh Nguyen wrote: >>> 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. > Sure, thanks for the confirmation, will send v2. Regards, Prashanth K
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89fc690fdf34..3b55285118b0 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -328,7 +328,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, } if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { - int link_state; + int link_state; + bool remote_wakeup = false; /* * Initiate remote wakeup if the link state is in U3 when @@ -339,15 +340,26 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, link_state = dwc3_gadget_get_link_state(dwc); switch (link_state) { case DWC3_LINK_STATE_U2: - if (dwc->gadget->speed >= USB_SPEED_SUPER) + if (dwc->gadget->speed < USB_SPEED_SUPER) + remote_wakeup = true; + break; + case DWC3_LINK_STATE_U3: + /* + * In HS, DSTS can take few milliseconds to update linkstate bits, + * so rely on dwc->link_state to identify whether gadget woke up. + * Don't issue remote wakuep again if link is already in U0. + */ + if (dwc->link_state == DWC3_LINK_STATE_U0) break; - fallthrough; - case DWC3_LINK_STATE_U3: + remote_wakeup = true; + break; + } + + if (remote_wakeup) { ret = __dwc3_gadget_wakeup(dwc, false); dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n", ret); - break; } } @@ -4214,6 +4226,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) { dwc->suspended = false; + dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK; /* * TODO take core out of low power mode when that's @@ -4225,8 +4238,6 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) dwc->gadget_driver->resume(dwc->gadget); spin_lock(&dwc->lock); } - - dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK; } static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
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") Cc: <stable@vger.kernel.org> Signed-off-by: Prashanth K <quic_prashk@quicinc.com> --- drivers/usb/dwc3/gadget.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)