| Message ID | 1675710806-9735-3-git-send-email-quic_eserrao@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add function suspend/resume and remote wakeup support | expand |
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. > + 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
Hi Elson,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.2-rc7 next-20230207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230207-031528
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1675710806-9735-3-git-send-email-quic_eserrao%40quicinc.com
patch subject: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling
config: i386-randconfig-a002-20230206 (https://download.01.org/0day-ci/archive/20230207/202302071850.VZRjtYXx-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/e0d9f3f5f168e36cdb599617634010326a1412af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230207-031528
git checkout e0d9f3f5f168e36cdb599617634010326a1412af
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/dwc3/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/usb/dwc3/gadget.c:2318:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
case DWC3_LINK_STATE_RESET:
^
drivers/usb/dwc3/gadget.c:2318:2: note: insert 'break;' to avoid fall-through
case DWC3_LINK_STATE_RESET:
^
break;
1 warning generated.
vim +2318 drivers/usb/dwc3/gadget.c
72246da40f3719af Felipe Balbi 2011-08-19 2291
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2292 static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
72246da40f3719af Felipe Balbi 2011-08-19 2293 {
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2294 int retries;
72246da40f3719af Felipe Balbi 2011-08-19 2295
218ef7b647e3367c Felipe Balbi 2016-04-04 2296 int ret;
72246da40f3719af Felipe Balbi 2011-08-19 2297 u32 reg;
72246da40f3719af Felipe Balbi 2011-08-19 2298
72246da40f3719af Felipe Balbi 2011-08-19 2299 u8 link_state;
72246da40f3719af Felipe Balbi 2011-08-19 2300
72246da40f3719af Felipe Balbi 2011-08-19 2301 /*
72246da40f3719af Felipe Balbi 2011-08-19 2302 * According to the Databook Remote wakeup request should
72246da40f3719af Felipe Balbi 2011-08-19 2303 * be issued only when the device is in early suspend state.
72246da40f3719af Felipe Balbi 2011-08-19 2304 *
72246da40f3719af Felipe Balbi 2011-08-19 2305 * We can check that via USB Link State bits in DSTS register.
72246da40f3719af Felipe Balbi 2011-08-19 2306 */
72246da40f3719af Felipe Balbi 2011-08-19 2307 reg = dwc3_readl(dwc->regs, DWC3_DSTS);
72246da40f3719af Felipe Balbi 2011-08-19 2308
72246da40f3719af Felipe Balbi 2011-08-19 2309 link_state = DWC3_DSTS_USBLNKST(reg);
72246da40f3719af Felipe Balbi 2011-08-19 2310
72246da40f3719af Felipe Balbi 2011-08-19 2311 switch (link_state) {
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2312 case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2313 if (!dwc->rw_configured) {
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2314 dev_err(dwc->dev,
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2315 "device not configured for remote wakeup\n");
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2316 return -EINVAL;
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2317 }
d0550cd20e52558e Thinh Nguyen 2020-01-31 @2318 case DWC3_LINK_STATE_RESET:
72246da40f3719af Felipe Balbi 2011-08-19 2319 case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */
c560e76319a94a3b Thinh Nguyen 2021-04-19 2320 case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */
c560e76319a94a3b Thinh Nguyen 2021-04-19 2321 case DWC3_LINK_STATE_U1:
d0550cd20e52558e Thinh Nguyen 2020-01-31 2322 case DWC3_LINK_STATE_RESUME:
72246da40f3719af Felipe Balbi 2011-08-19 2323 break;
72246da40f3719af Felipe Balbi 2011-08-19 2324 default:
218ef7b647e3367c Felipe Balbi 2016-04-04 2325 return -EINVAL;
72246da40f3719af Felipe Balbi 2011-08-19 2326 }
72246da40f3719af Felipe Balbi 2011-08-19 2327
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2328 if (async)
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2329 dwc3_gadget_enable_linksts_evts(dwc, true);
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2330
8598bde7fa125e85 Felipe Balbi 2012-01-02 2331 ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
8598bde7fa125e85 Felipe Balbi 2012-01-02 2332 if (ret < 0) {
8598bde7fa125e85 Felipe Balbi 2012-01-02 2333 dev_err(dwc->dev, "failed to put link in Recovery\n");
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2334 dwc3_gadget_enable_linksts_evts(dwc, false);
218ef7b647e3367c Felipe Balbi 2016-04-04 2335 return ret;
8598bde7fa125e85 Felipe Balbi 2012-01-02 2336 }
72246da40f3719af Felipe Balbi 2011-08-19 2337
802fde983e8a3391 Paul Zimmerman 2012-04-27 2338 /* Recent versions do this automatically */
9af21dd6faeba593 Thinh Nguyen 2020-04-11 2339 if (DWC3_VER_IS_PRIOR(DWC3, 194A)) {
72246da40f3719af Felipe Balbi 2011-08-19 2340 /* write zeroes to Link Change Request */
fcc023c726b5879d Felipe Balbi 2012-05-24 2341 reg = dwc3_readl(dwc->regs, DWC3_DCTL);
72246da40f3719af Felipe Balbi 2011-08-19 2342 reg &= ~DWC3_DCTL_ULSTCHNGREQ_MASK;
72246da40f3719af Felipe Balbi 2011-08-19 2343 dwc3_writel(dwc->regs, DWC3_DCTL, reg);
802fde983e8a3391 Paul Zimmerman 2012-04-27 2344 }
72246da40f3719af Felipe Balbi 2011-08-19 2345
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2346 /*
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2347 * Since link status change events are enabled we will receive
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2348 * an U0 event when wakeup is successful. So bail out.
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2349 */
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2350 if (async)
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2351 return 0;
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2352
1d046793958f128d Paul Zimmerman 2012-02-15 2353 /* poll until Link State changes to ON */
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2354 retries = 20000;
72246da40f3719af Felipe Balbi 2011-08-19 2355
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2356 while (retries--) {
72246da40f3719af Felipe Balbi 2011-08-19 2357 reg = dwc3_readl(dwc->regs, DWC3_DSTS);
72246da40f3719af Felipe Balbi 2011-08-19 2358
72246da40f3719af Felipe Balbi 2011-08-19 2359 /* in HS, means ON */
72246da40f3719af Felipe Balbi 2011-08-19 2360 if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
72246da40f3719af Felipe Balbi 2011-08-19 2361 break;
72246da40f3719af Felipe Balbi 2011-08-19 2362 }
72246da40f3719af Felipe Balbi 2011-08-19 2363
72246da40f3719af Felipe Balbi 2011-08-19 2364 if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
72246da40f3719af Felipe Balbi 2011-08-19 2365 dev_err(dwc->dev, "failed to send remote wakeup\n");
218ef7b647e3367c Felipe Balbi 2016-04-04 2366 return -EINVAL;
72246da40f3719af Felipe Balbi 2011-08-19 2367 }
72246da40f3719af Felipe Balbi 2011-08-19 2368
218ef7b647e3367c Felipe Balbi 2016-04-04 2369 return 0;
218ef7b647e3367c Felipe Balbi 2016-04-04 2370 }
218ef7b647e3367c Felipe Balbi 2016-04-04 2371
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 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
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 1.) Enabled l1. 2.) Disabled the remote wakeup software path (i.e avoid calling __gadget_wakeup() if link is in l1 in the gadget_ep_cmd() path). 3.) Sent an IN packet when the link was in l1. From the lecroy logs it looks like the controller initiated a remote wakeup and sent the data. Below are the events and the corresponding lecroy snippet 1.)Packet(55551) ------------> LPM token from Windows Host PC. 2.) Link in l1 for 2.445 secs 3. ) Send a ping data from device to host 4. )Packet(55554) ----------------> Resume 5.) IN data Packet# _______|_______________________________________________________________________ Transaction(26584) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us) _______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time Stamp(27 . 204 671 632) _______|_______________________________________________________________________Ch0 Packet(55550) Dir H(S) EXT(0x0F) ADDR(11) ENDP(0) CRC5(0x04) Pkt Len(8) _______| Duration(133.330 ns) Idle(200.660 ns) Time Stamp(27 . 204 671 632) _______|_______________________________________________________________________Ch0 Packet(55551) Dir H(S) LPM(0xC3) BESL(150 us) Link State(0x1) _______| Rem Wake(0x1) Rsvd(0x0) CRC5(0x04) Pkt Len(8) Duration(133.330 ns) _______| Idle(182.660 ns) Time Stamp(27 . 204 671 966) _______|_______________________________________________________________________Ch0 Packet(55552) Dir H(S) ACK(0x4B) Pkt Len(6) Duration(100.000 ns) _______| Idle( 11.450 us) Time Stamp(27 . 204 672 282) _______|_______________________________________________________________________Ch0 Packet(55553) Dir(?) Full Speed J (Suspend)( 2.445 sec) _______| Time Stamp(27 . 204 683 832) _______|_______________________________________________________________________Ch0 Packet(55554) Dir(?) Full Speed K (Resume?)( 95.168 us) Time(165.134 us) _______| Time Stamp(29 . 649 644 482) _______|_______________________________________________________________________ Transfer(67) H(S) Bulk(IN) ADDR(11) ENDP(1) Bytes Transferred(142) _______| Time(309.366 us) Time Stamp(29 . 649 809 616) _______|_______________________________________________________________________ Transfer(68) H(S) Bulk(OUT) ADDR(11) ENDP(1) Bytes Transferred(142) _______| Time(520.050 us) Time Stamp(29 . 650 118 982) _______|_______________________________________________________________________ Transaction(26655) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us) _______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time( 12.168 us) _______| Time Stamp(29 . 650 639 032) _______|_______________________________________________________________________ If software was solely responsible for waking up from l1, then there should be no reason why the host would exit l1 in this scenario. I tried with different ping intervals and saw that the duration for which the link was in l1 correlates with the ping interval. Thanks 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
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; + + 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 */ + 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; /* * FIXME We might be setting max_speed to <SUPER, however versions
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(-)