Messages in this thread | | | Date | Tue, 16 Aug 2022 12:57:28 -0700 | Subject | Re: [PATCH 2/5] usb: gadget: Add function wakeup support | From | Elson Serrao <> |
| |
On 8/12/2022 5:46 PM, Thinh Nguyen wrote: > On 8/11/2022, Thinh Nguyen wrote: >> On 8/11/2022, Thinh Nguyen wrote: >>> On 8/11/2022, Elson Serrao wrote: >>>> >>>> >>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote: >>> >>> <snip> >>> >>> >>>>> To summarize the points: >>>>> >>>>> 1) The host only arms function remote wakeup if the device is capable of >>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware >>>>> capability) >>>>> >>>>> 2) If the device is in suspend, the device can do remote wakeup (through >>>>> LFPS handshake) if the function is armed for remote wakeup (through >>>>> SET_FEATURE(FUNC_SUSPEND)). >>>>> >>>>> 3) If the link transitions to U0 after the device triggering a remote >>>>> wakeup, the device will also send device notification function wake for >>>>> all the interfaces armed with remote wakeup. >>>>> >>>>> 4) If the device is not in suspend, the device can send device >>>>> notification function wake if it's in U0. >>>>> >>>>> >>>>> Now, remote wakeup and function wake device notification are 2 separate >>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I >>>>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget, >>>>> intf_id) for the device notification. What you did was combining both >>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for >>>>> point 4) (assuming you fix the U0 check), but not point 3). >>>> >>>> Thank you for your feedback and summary. I will rename func_wakeup to >>>> send_wakeup_notification to better align with the approach. The reason I >>>> have combined remote_wakeup and function wake notification in >>>> usb_gadget_ops->func_wakeup() is because since the implementation is at >>>> function/composite level it has no knowledge on the link state. So I >>>> have delegated that task to controller driver to handle the notification >>>> accordingly. That is do a LFPS handshake first if the device is >>>> suspended and then send notification (explained below). But we can >>>> definitely separate this by adding an additional flag in the composite >>>> layer to set the link state based on the gadget suspend callback called >>>> when U3 SUSPEND interrupt is received. Let me know if you feel >>>> separating the two is a better approach. >>>> >>> >>> The reason I think we need to separate it is because of point 3. As I >>> note earlier, the spec states that "If remote wake event occurs in >>> multiple functions, each function shall send a Function Wake Notification." >>> >>> But if there's no remote wake event, and the host brought the device up >>> instead, then the function suspend state is retained. >>> >>> If we separate these 2 operations, the caller can check whether the >>> operation went through properly. For example, if the wakeup() is >>> initiated properly, but the function wake device notification didn't go >>> through. We would only need to resend the device notification rather >>> than initiate remote wakeup again. >> >> If we don't have to send device notification for other interfaces, we >> can combine the operations here as you did. >> > > I still think it's better to split up the operations. The way you're > handling it now is not clear. > > If the func_awake() returns -EAGAIN, I'd expect that the remote wake did > not go through and expect user to retry again. But here it does initiate > remote wake, but it just does not send device notification yet. This is > confusing. > > Also, instead of all the function wake handling coming from the function > driver, now we depend on the controller driver to call function resume() > on state change to U0, which will trigger device notification. What > happen if it doesn't call resume(). There's too many dependencies and it > seems fragile. > > I think all this can be handled in the function driver. You can fix the > dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which > is what it's supposed to poll.
For transitioning from U3 to U0, the current upstream implementation is to poll for U0 state when dwc3_gadget_wakeup() is called and it is a blocking call. (this is a common API for both HS and SS)
/* poll until Link State changes to ON */ retries = 20000;
while (retries--) { reg = dwc3_readl(dwc->regs, DWC3_DSTS);
/* in HS, means ON */ if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0) break; }
In my experiments I found that certain hosts take longer time to drive HS resume signalling between the remote wakeup and the resume K and this time varies across hosts. Hence the above polling timer is not generic across hosts. So how do we converge on a polling timer value to work across HS/SS and without blocking the system for a long time?
Some data layers like TCP/IP hold a TX lock while sending data (that causes a remote wakeup event) and hence this wakeup() may run in atomic context.
To make this generic across hosts, I had switched to interrupt based approach, enabling link state events and return error value immediately from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But yeah, then we have to rely on the resume callback as an indication that link is transitioned to ON state.
On usb_gadget_wakeup() returns > successful, we'd expect the device is linked up and woken up. then you > can send device notification through a different api such as > usb_gadget_send_wake_notification(). > > Thanks, > Thinh
| |