| Message ID | 1675710806-9735-1-git-send-email-quic_eserrao@quicinc.com |
|---|---|
| Headers | show |
| Series | Add function suspend/resume and remote wakeup support | expand |
Hi Elson, On Mon, Feb 06, 2023, Elson Roy Serrao wrote: > Changes in v3 > - Modified rw_capable flag to reflect the gadgets capability for wakeup > signalling. > - Added a check to configure wakeup bit in bmAttributes only if gadget > is capable of triggering wakeup. > - Implemented a gadget op for composite layer to inform UDC whether device > is configured for remote wakeup. > - Added a check in __usb_gadget_wakeup() API to trigger wakeup only if the > device is configured for it. > - Cosmetic changes in dwc3_gadget_func_wakeup() API. > > Changes in v2 > - Added a flag to indicate whether the device is remote wakeup capable. > - Added an async parameter to _dwc3_gadget_wakeup() API and few cosmetic > changes. > - Added flags to reflect the state of function suspend and function remote > wakeup to usb_function struct rather than function specific struct (f_ecm). > - Changed the dwc3_gadget_func__wakeup() API to run synchronously by first > checking the link state and then sending the device notification. Also > added debug log for DEVICE_NOTIFICATION generic cmd. > - Added changes to arm the device for remotewakeup/function remotewakeup > only if device is capable. > > An usb device can initate a remote wakeup and bring the link out of > suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector. > To achieve this an interface can invoke gadget_wakeup op and wait for the > device to come out of LPM. But the current polling based implementation > fails if the host takes a long time to drive the resume signaling specially > in high speed capable devices. Switching to an interrupt based approach is > more robust and efficient. This can be leveraged by enabling link status > change events and triggering a gadget resume when the link comes to active > state. > > If the device is enhanced super-speed capable, individual interfaces can > also be put into suspend state. An interface can be in function suspend > state even when the device is not in suspend state. Function suspend state > is retained throughout the device suspend entry and exit process. > A function can be put to function suspend through FUNCTION_SUSPEND feature > selector sent by the host. This setup packet also decides whether that > function is capable of initiating a function remote wakeup. When the > function sends a wakeup notification to the host the link must be first > brought to a non-U0 state and then this notification is sent. > > This change adds the infrastructure needed to support the above > functionalities. > > Elson Roy Serrao (5): > usb: gadget: Properly configure the device for remote wakeup > usb: dwc3: Add remote wakeup handling > usb: gadget: Add function wakeup support > usb: dwc3: Add function suspend and function wakeup support > usb: gadget: f_ecm: Add suspend/resume and remote wakeup support > > drivers/usb/dwc3/core.h | 5 ++ > drivers/usb/dwc3/debug.h | 2 + > drivers/usb/dwc3/ep0.c | 16 ++--- > drivers/usb/dwc3/gadget.c | 110 +++++++++++++++++++++++++++++++--- > drivers/usb/gadget/composite.c | 50 +++++++++++++++- > drivers/usb/gadget/function/f_ecm.c | 68 +++++++++++++++++++++ > drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++ > drivers/usb/gadget/function/u_ether.h | 4 ++ > drivers/usb/gadget/udc/core.c | 46 ++++++++++++++ > drivers/usb/gadget/udc/trace.h | 5 ++ > include/linux/usb/composite.h | 6 ++ > include/linux/usb/gadget.h | 12 ++++ > 12 files changed, 371 insertions(+), 16 deletions(-) > > -- > 2.7.4 > Hi Elson, Thanks for your patches! I provided some comments. Hopefully they can be merged soon. Thanks, Thinh
On 2/6/2023 4:48 PM, Thinh Nguyen wrote: > On Mon, Feb 06, 2023, Elson Roy Serrao wrote: >> An usb device can initate a remote wakeup and bring the link out of >> suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector. >> Add support to handle this packet and set the remote wakeup capability. >> >> Some hosts may take longer time to initiate the resume signaling after >> device triggers a remote wakeup. So add async support to the wakeup API >> by enabling link status change events. >> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >> --- >> drivers/usb/dwc3/core.h | 2 ++ >> drivers/usb/dwc3/ep0.c | 4 +++ >> drivers/usb/dwc3/gadget.c | 73 ++++++++++++++++++++++++++++++++++++++++++----- >> 3 files changed, 72 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 8f9959b..ff6e6f6 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array { >> * 3 - Reserved >> * @dis_metastability_quirk: set to disable metastability quirk. >> * @dis_split_quirk: set to disable split boundary. >> + * @rw_configured: set if the device is configured for remote wakeup. >> * @imod_interval: set the interrupt moderation interval in 250ns >> * increments or 0 to disable. >> * @max_cfg_eps: current max number of IN eps used across all USB configs. >> @@ -1326,6 +1327,7 @@ struct dwc3 { >> >> unsigned dis_split_quirk:1; >> unsigned async_callbacks:1; >> + unsigned rw_configured:1; >> >> u16 imod_interval; >> >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >> index 61de693..cd7c0cb 100644 >> --- a/drivers/usb/dwc3/ep0.c >> +++ b/drivers/usb/dwc3/ep0.c >> @@ -356,6 +356,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, >> usb_status |= 1 << USB_DEV_STAT_U1_ENABLED; >> if (reg & DWC3_DCTL_INITU2ENA) >> usb_status |= 1 << USB_DEV_STAT_U2_ENABLED; >> + } else { >> + usb_status |= dwc->gadget->rw_armed << >> + USB_DEVICE_REMOTE_WAKEUP; >> } >> >> break; >> @@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc, >> >> switch (wValue) { >> case USB_DEVICE_REMOTE_WAKEUP: >> + dwc->gadget->rw_armed = set; >> break; >> /* >> * 9.4.1 says only for SS, in AddressState only for >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 89dcfac..d0b9917 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd, >> return ret; >> } >> >> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc); >> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async); >> >> /** >> * dwc3_send_gadget_ep_cmd - issue an endpoint command >> @@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd, >> >> fallthrough; >> case DWC3_LINK_STATE_U3: >> - ret = __dwc3_gadget_wakeup(dwc); >> + ret = __dwc3_gadget_wakeup(dwc, false); >> dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n", >> ret); >> break; >> @@ -2269,6 +2269,19 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = { >> >> /* -------------------------------------------------------------------------- */ >> >> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set) >> +{ >> + u32 reg; > > Add a check here to prevent disabling link state event if the controller > is dwc_usb3 2.50a. Some older controller always enables this event for a > quirk. > >> + >> + reg = dwc3_readl(dwc->regs, DWC3_DEVTEN); >> + if (set) >> + reg |= DWC3_DEVTEN_ULSTCNGEN; >> + else >> + reg &= ~DWC3_DEVTEN_ULSTCNGEN; >> + >> + dwc3_writel(dwc->regs, DWC3_DEVTEN, reg); >> +} >> + >> static int dwc3_gadget_get_frame(struct usb_gadget *g) >> { >> struct dwc3 *dwc = gadget_to_dwc(g); >> @@ -2276,7 +2289,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g) >> return __dwc3_gadget_get_frame(dwc); >> } >> >> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async) >> { >> int retries; >> >> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >> link_state = DWC3_DSTS_USBLNKST(reg); >> >> switch (link_state) { >> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ > > It's also possible to do remote wakeup in L1 for highspeed. > The rw_configured flag here is in context of triggering remote wakeup from bus suspend only. The remote wakeup setting for l1 in HighSpeed is controlled through LPM token and overrides/ignores the config desc bmAttributes wakeup bit. Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to enable or disable the addressed device for remote wake from L1. The value of this flag will temporarily (while in L1) override the current setting of the Remote Wake feature settable by the standard Set/ClearFeature() commands defined in Universal Serial Bus Specification, revision 2.0, Chapter 9." Please let me know if I am missing something. Thanks Elson >> + if (!dwc->rw_configured) { >> + dev_err(dwc->dev, >> + "device not configured for remote wakeup\n"); >> + return -EINVAL; >> + } >> case DWC3_LINK_STATE_RESET: >> case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */ >> - case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ >> case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */ >> case DWC3_LINK_STATE_U1: >> case DWC3_LINK_STATE_RESUME: >> @@ -2307,9 +2325,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >> return -EINVAL; >> } >> >> + if (async) >> + dwc3_gadget_enable_linksts_evts(dwc, true); >> + >> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV); >> if (ret < 0) { >> dev_err(dwc->dev, "failed to put link in Recovery\n"); >> + dwc3_gadget_enable_linksts_evts(dwc, false); >> return ret; >> } >> >> @@ -2321,6 +2343,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >> dwc3_writel(dwc->regs, DWC3_DCTL, reg); >> } >> >> + /* >> + * Since link status change events are enabled we will receive >> + * an U0 event when wakeup is successful. So bail out. >> + */ >> + if (async) >> + return 0; >> + >> /* poll until Link State changes to ON */ >> retries = 20000; >> >> @@ -2347,12 +2376,30 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g) >> int ret; >> >> spin_lock_irqsave(&dwc->lock, flags); >> - ret = __dwc3_gadget_wakeup(dwc); >> + if (!dwc->gadget->rw_armed) { >> + dev_err(dwc->dev, "%s:remote wakeup not enabled\n", __func__); >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + return -EINVAL; >> + } >> + ret = __dwc3_gadget_wakeup(dwc, true); >> + >> spin_unlock_irqrestore(&dwc->lock, flags); >> >> return ret; >> } >> >> +static int dwc3_gadget_set_remotewakeup(struct usb_gadget *g, int set) >> +{ >> + struct dwc3 *dwc = gadget_to_dwc(g); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&dwc->lock, flags); >> + dwc->rw_configured = !!set; >> + spin_unlock_irqrestore(&dwc->lock, flags); >> + >> + return 0; >> +} >> + >> static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, >> int is_selfpowered) >> { >> @@ -2978,6 +3025,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable) >> static const struct usb_gadget_ops dwc3_gadget_ops = { >> .get_frame = dwc3_gadget_get_frame, >> .wakeup = dwc3_gadget_wakeup, >> + .set_remotewakeup = dwc3_gadget_set_remotewakeup, >> .set_selfpowered = dwc3_gadget_set_selfpowered, >> .pullup = dwc3_gadget_pullup, >> .udc_start = dwc3_gadget_start, >> @@ -3821,6 +3869,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc) >> >> dwc->gadget->speed = USB_SPEED_UNKNOWN; >> dwc->setup_packet_pending = false; >> + dwc->gadget->rw_armed = false; >> + dwc3_gadget_enable_linksts_evts(dwc, false); >> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED); >> >> if (dwc->ep0state != EP0_SETUP_PHASE) { >> @@ -3914,6 +3964,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) >> reg &= ~DWC3_DCTL_TSTCTRL_MASK; >> dwc3_gadget_dctl_write_safe(dwc, reg); >> dwc->test_mode = false; >> + dwc->gadget->rw_armed = false; >> + dwc3_gadget_enable_linksts_evts(dwc, false); >> dwc3_clear_stall_all_ep(dwc); >> >> /* Reset device address to zero */ >> @@ -4066,7 +4118,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc) >> */ >> } >> >> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) >> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo) >> { >> /* >> * TODO take core out of low power mode when that's >> @@ -4078,6 +4130,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc) >> 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, >> @@ -4159,6 +4213,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc, >> } >> >> switch (next) { >> + case DWC3_LINK_STATE_U0: >> + dwc3_gadget_enable_linksts_evts(dwc, false); >> + dwc3_resume_gadget(dwc); >> + break; >> case DWC3_LINK_STATE_U1: >> if (dwc->speed == USB_SPEED_SUPER) >> dwc3_suspend_gadget(dwc); >> @@ -4227,7 +4285,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc, >> dwc3_gadget_conndone_interrupt(dwc); >> break; >> case DWC3_DEVICE_EVENT_WAKEUP: >> - dwc3_gadget_wakeup_interrupt(dwc); >> + dwc3_gadget_wakeup_interrupt(dwc, event->event_info); >> break; >> case DWC3_DEVICE_EVENT_HIBER_REQ: >> if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation, >> @@ -4487,6 +4545,7 @@ int dwc3_gadget_init(struct dwc3 *dwc) >> dwc->gadget->sg_supported = true; >> dwc->gadget->name = "dwc3-gadget"; >> dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable; >> + dwc->gadget->rw_capable = dwc->gadget->ops->wakeup ? true : false; > > Just set it to true here. > >> >> /* >> * FIXME We might be setting max_speed to <SUPER, however versions >> -- >> 2.7.4 >> > > Thanks, > Thinh
On Tue, Feb 07, 2023, Elson Serrao wrote: > On 2/6/2023 4:48 PM, Thinh Nguyen wrote: > > > +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async) > > > { > > > int retries; > > > @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) > > > link_state = DWC3_DSTS_USBLNKST(reg); > > > switch (link_state) { > > > + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ > > > > It's also possible to do remote wakeup in L1 for highspeed. > > > > The rw_configured flag here is in context of triggering remote wakeup from > bus suspend only. > > The remote wakeup setting for l1 in HighSpeed is controlled through LPM > token and overrides/ignores the config desc bmAttributes wakeup bit. > > Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to > enable or disable the addressed device > for remote wake from L1. The value of this flag will temporarily (while in > L1) override the current setting of the > Remote Wake feature settable by the standard Set/ClearFeature() commands > defined in Universal Serial Bus Specification, revision 2.0, Chapter 9." > > Please let me know if I am missing something. > It overrides the setting of the SetFeature request, not the device configuration. The rw_configured reflects the user configuration. Whether the host tries to enable the remote wakeup through SetFeature request or LPM token, the device should operate within the user configuration limitation. If the configuration indicates that it doesn't support remote wakeup, we should prevent unexpected behavior from the device. For simplicity, we can just return failure to wakeup for all states. Thanks, Thinh
On 2/7/2023 5:10 PM, Thinh Nguyen wrote: > On Tue, Feb 07, 2023, Elson Serrao wrote: >> On 2/6/2023 4:48 PM, Thinh Nguyen wrote: >>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async) >>>> { >>>> int retries; >>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >>>> link_state = DWC3_DSTS_USBLNKST(reg); >>>> switch (link_state) { >>>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ >>> >>> It's also possible to do remote wakeup in L1 for highspeed. >>> >> >> The rw_configured flag here is in context of triggering remote wakeup from >> bus suspend only. >> >> The remote wakeup setting for l1 in HighSpeed is controlled through LPM >> token and overrides/ignores the config desc bmAttributes wakeup bit. >> >> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to >> enable or disable the addressed device >> for remote wake from L1. The value of this flag will temporarily (while in >> L1) override the current setting of the >> Remote Wake feature settable by the standard Set/ClearFeature() commands >> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9." >> >> Please let me know if I am missing something. >> > > It overrides the setting of the SetFeature request, not the device > configuration. > > The rw_configured reflects the user configuration. Whether the host > tries to enable the remote wakeup through SetFeature request or LPM > token, the device should operate within the user configuration > limitation. > > If the configuration indicates that it doesn't support remote wakeup, we > should prevent unexpected behavior from the device. For simplicity, we > can just return failure to wakeup for all states. > > Thanks, > Thinh L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section 7.1/Table7.2 of dwc3 data book). Even though we block it from SW the l1 exit will still happen from HW point of view. To correlate the user configuration with LPM token, I experimented by disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup bit being set in the LPM token. From the observation it seems like there is no correlation between the wakeup bit in the bmAtrributes and the wakeup bit in the LPM token. Regards Elson
On Thu, Feb 09, 2023, Elson Serrao wrote: > > > On 2/7/2023 6:11 PM, Thinh Nguyen wrote: > > On Tue, Feb 07, 2023, Elson Serrao wrote: > > > > > > > > > On 2/7/2023 5:10 PM, Thinh Nguyen wrote: > > > > On Tue, Feb 07, 2023, Elson Serrao wrote: > > > > > On 2/6/2023 4:48 PM, Thinh Nguyen wrote: > > > > > > > +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async) > > > > > > > { > > > > > > > int retries; > > > > > > > @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) > > > > > > > link_state = DWC3_DSTS_USBLNKST(reg); > > > > > > > switch (link_state) { > > > > > > > + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ > > > > > > > > > > > > It's also possible to do remote wakeup in L1 for highspeed. > > > > > > > > > > > > > > > > The rw_configured flag here is in context of triggering remote wakeup from > > > > > bus suspend only. > > > > > > > > > > The remote wakeup setting for l1 in HighSpeed is controlled through LPM > > > > > token and overrides/ignores the config desc bmAttributes wakeup bit. > > > > > > > > > > Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to > > > > > enable or disable the addressed device > > > > > for remote wake from L1. The value of this flag will temporarily (while in > > > > > L1) override the current setting of the > > > > > Remote Wake feature settable by the standard Set/ClearFeature() commands > > > > > defined in Universal Serial Bus Specification, revision 2.0, Chapter 9." > > > > > > > > > > Please let me know if I am missing something. > > > > > > > > > > > > > It overrides the setting of the SetFeature request, not the device > > > > configuration. > > > > > > > > The rw_configured reflects the user configuration. Whether the host > > > > tries to enable the remote wakeup through SetFeature request or LPM > > > > token, the device should operate within the user configuration > > > > limitation. > > > > > > > > If the configuration indicates that it doesn't support remote wakeup, we > > > > should prevent unexpected behavior from the device. For simplicity, we > > > > can just return failure to wakeup for all states. > > > > > > > > Thanks, > > > > Thinh > > > > > > L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section > > > 7.1/Table7.2 of dwc3 data book). Even though we block it from > > > SW the l1 exit will still happen from HW point of view. > > > > > > To correlate the user configuration with LPM token, I experimented by > > > disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup > > > bit being set in the LPM token. From the observation it seems like there is > > > > That's because the linux xhci driver enables remote wakeup bit in its > > port without regard for the device configuration. > > > > > no correlation between the wakeup bit in the bmAtrributes and the wakeup bit > > > in the LPM token. > > > > > > > The host can bring the device out of L1, that's probably what you saw. > > The controller doesn't initiate remote wakeup by itself. > > > > Thanks, > > Thinh > > Actually it seems the controller is initiating a remote wakeup by itself to > exit from l1 when we send a STARTTRANSFER command. I did below experiment > when the device was in HighSpeed > That's driven by the driver telling the controller to initiate remote wakeup and not the controller itself. When we send the START_TRANSFER command, the driver does remote wakeup so the host would bring the device to ON state so that the command can go through. However you bring up a good point that if we prevent remote wakeup for L1, then we have to delay sending START_TRANSFER command until the host initiate resume. This would require additional enhancement to dwc3 to handle this scenario. For now, can we ignore this specific case when sending START_TRANSFER command and only check for the case when the user trigger remote wakeup via gadget->ops->wakeup/func_wakeup. Thanks, Thinh
On 2/9/2023 6:27 PM, Thinh Nguyen wrote: > On Thu, Feb 09, 2023, Elson Serrao wrote: >> >> >> On 2/7/2023 6:11 PM, Thinh Nguyen wrote: >>> On Tue, Feb 07, 2023, Elson Serrao wrote: >>>> >>>> >>>> On 2/7/2023 5:10 PM, Thinh Nguyen wrote: >>>>> On Tue, Feb 07, 2023, Elson Serrao wrote: >>>>>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote: >>>>>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async) >>>>>>>> { >>>>>>>> int retries; >>>>>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc) >>>>>>>> link_state = DWC3_DSTS_USBLNKST(reg); >>>>>>>> switch (link_state) { >>>>>>>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */ >>>>>>> >>>>>>> It's also possible to do remote wakeup in L1 for highspeed. >>>>>>> >>>>>> >>>>>> The rw_configured flag here is in context of triggering remote wakeup from >>>>>> bus suspend only. >>>>>> >>>>>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM >>>>>> token and overrides/ignores the config desc bmAttributes wakeup bit. >>>>>> >>>>>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to >>>>>> enable or disable the addressed device >>>>>> for remote wake from L1. The value of this flag will temporarily (while in >>>>>> L1) override the current setting of the >>>>>> Remote Wake feature settable by the standard Set/ClearFeature() commands >>>>>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9." >>>>>> >>>>>> Please let me know if I am missing something. >>>>>> >>>>> >>>>> It overrides the setting of the SetFeature request, not the device >>>>> configuration. >>>>> >>>>> The rw_configured reflects the user configuration. Whether the host >>>>> tries to enable the remote wakeup through SetFeature request or LPM >>>>> token, the device should operate within the user configuration >>>>> limitation. >>>>> >>>>> If the configuration indicates that it doesn't support remote wakeup, we >>>>> should prevent unexpected behavior from the device. For simplicity, we >>>>> can just return failure to wakeup for all states. >>>>> >>>>> Thanks, >>>>> Thinh >>>> >>>> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section >>>> 7.1/Table7.2 of dwc3 data book). Even though we block it from >>>> SW the l1 exit will still happen from HW point of view. >>>> >>>> To correlate the user configuration with LPM token, I experimented by >>>> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup >>>> bit being set in the LPM token. From the observation it seems like there is >>> >>> That's because the linux xhci driver enables remote wakeup bit in its >>> port without regard for the device configuration. >>> >>>> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit >>>> in the LPM token. >>>> >>> >>> The host can bring the device out of L1, that's probably what you saw. >>> The controller doesn't initiate remote wakeup by itself. >>> >>> Thanks, >>> Thinh >> >> Actually it seems the controller is initiating a remote wakeup by itself to >> exit from l1 when we send a STARTTRANSFER command. I did below experiment >> when the device was in HighSpeed >> > > That's driven by the driver telling the controller to initiate remote > wakeup and not the controller itself. When we send the START_TRANSFER > command, the driver does remote wakeup so the host would bring the > device to ON state so that the command can go through. > > However you bring up a good point that if we prevent remote wakeup for > L1, then we have to delay sending START_TRANSFER command until the host > initiate resume. This would require additional enhancement to dwc3 to > handle this scenario. For now, can we ignore this specific case when > sending START_TRANSFER command and only check for the case when the user > trigger remote wakeup via gadget->ops->wakeup/func_wakeup. > > Thanks, > Thinh Sure. I will upload v4 with the suggested feedback/comments. Thanks Elson