Re: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling

From: Thinh Nguyen
Date: Mon Feb 06 2023 - 19:49:04 EST

Next message: Stephen Rothwell: "linux-next: build failure after merge of the integrity tree"
Previous message: Andrew Lunn: "Re: [PATCH net-next v5 03/23] net: phy: micrel: add ksz9477_get_features()"
In reply to: Elson Roy Serrao: "[PATCH v3 2/5] usb: dwc3: Add remote wakeup handling"
Next in thread: Elson Serrao: "Re: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> ---
> 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

Next message: Stephen Rothwell: "linux-next: build failure after merge of the integrity tree"
Previous message: Andrew Lunn: "Re: [PATCH net-next v5 03/23] net: phy: micrel: add ksz9477_get_features()"
In reply to: Elson Roy Serrao: "[PATCH v3 2/5] usb: dwc3: Add remote wakeup handling"
Next in thread: Elson Serrao: "Re: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]