diff mbox series

[1/5] usb: dwc3: Add remote wakeup handling

Message ID 1659467920-9095-2-git-send-email-quic_eserrao@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add function suspend/resume and remote wakeup support | expand

Commit Message

Elson Roy Serrao Aug. 2, 2022, 7:18 p.m. UTC 
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
accordingly.

Some hosts may take longer time to initiate the resume
signaling after device triggers a remote wakeup. So improve the
gadget_wakeup op to interrupt based rather than polling based by
enabling link status change events.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.h   |  4 +++
 drivers/usb/dwc3/ep0.c    |  4 +++
 drivers/usb/dwc3/gadget.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 74 insertions(+), 3 deletions(-)

Comments

Thinh Nguyen Aug. 3, 2022, 12:01 a.m. UTC | #1 
On 8/2/2022, 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
> accordingly.
>
> Some hosts may take longer time to initiate the resume
> signaling after device triggers a remote wakeup. So improve the
> gadget_wakeup op to interrupt based rather than polling based by
> enabling link status change events.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>   drivers/usb/dwc3/core.h   |  4 +++
>   drivers/usb/dwc3/ep0.c    |  4 +++
>   drivers/usb/dwc3/gadget.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4fe4287..3306b1c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1113,6 +1113,8 @@ struct dwc3_scratchpad_array {
>    *		     address.
>    * @num_ep_resized: carries the current number endpoints which have had its tx
>    *		    fifo resized.
> + * @is_remote_wakeup_enabled: remote wakeup status from host perspective
> + * @is_gadget_wakeup: remote wakeup requested via gadget op.
>    */
>   struct dwc3 {
>   	struct work_struct	drd_work;
> @@ -1326,6 +1328,8 @@ struct dwc3 {
>   	int			max_cfg_eps;
>   	int			last_fifo_depth;
>   	int			num_ep_resized;
> +	bool			is_remote_wakeup_enabled;
> +	bool			is_gadget_wakeup;
>   };
>   
>   #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 197af63..4cc3d3a 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -353,6 +353,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->is_remote_wakeup_enabled <<
> +						USB_DEVICE_REMOTE_WAKEUP;

You need to create a new macro for function remote enabled. Name it 
something like USB_DEV_STAT_FUNC_RW_ENABLED to match with others in ch9.h

>   		}
>   
>   		break;
> @@ -473,6 +476,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>   
>   	switch (wValue) {
>   	case USB_DEVICE_REMOTE_WAKEUP:
> +		dwc->is_remote_wakeup_enabled = 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 4366c45..d6697da 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2232,6 +2232,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>   
>   /* -------------------------------------------------------------------------- */
>   
> +static void linksts_change_events_set(struct dwc3 *dwc, bool set)

It should be named something that's consistent with the other functions. 
Something like dwc3_gadget_enable_linkstate_change_event()

> +{
> +	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);
> +
> +	/* Required to complete this operation before returning */
> +	mb();

Why do we need memory barrier here? It's just register write, and we 
already have io barrier for that.

> +}
> +
>   static int dwc3_gadget_get_frame(struct usb_gadget *g)
>   {
>   	struct dwc3		*dwc = gadget_to_dwc(g);
> @@ -2270,9 +2286,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   		return -EINVAL;
>   	}
>   
> +	linksts_change_events_set(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");
> +		linksts_change_events_set(dwc, false);
> +		dwc->is_gadget_wakeup = false;
>   		return ret;
>   	}
>   
> @@ -2284,9 +2304,15 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>   	}
>   
> +	/* If remote wakeup is triggered from function driver, bail out.

Follow this type of doc

/*
  * xxxx
  * yyyy
  */

> +	 * Since link status change events are enabled we would receive
> +	 * an U0 event when wakeup is successful.
> +	 */
> +	if (dwc->is_gadget_wakeup)
> +		return -EAGAIN;
> +
>   	/* poll until Link State changes to ON */
>   	retries = 20000;
> -
>   	while (retries--) {
>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>   
> @@ -2295,6 +2321,8 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   			break;
>   	}
>   
> +	linksts_change_events_set(dwc, false);
> +
>   	if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
>   		dev_err(dwc->dev, "failed to send remote wakeup\n");
>   		return -EINVAL;
> @@ -2310,7 +2338,20 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>   	int			ret;
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
> +	if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled) {
> +		dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> +		ret =  -EPERM;
> +		goto out;
> +	}
> +	if (dwc->is_gadget_wakeup) {
> +		dev_err(dwc->dev, "%s: remote wakeup in progress\n", __func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	dwc->is_gadget_wakeup = true;
>   	ret = __dwc3_gadget_wakeup(dwc);
> +
> +out:
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
>   	return ret;
> @@ -2766,6 +2807,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
>   	dwc->gadget_driver	= driver;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_remote_wakeup_enabled = false;
> +	dwc->is_gadget_wakeup = false;
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
>   	return 0;
> @@ -2785,6 +2829,9 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
>   	dwc->gadget_driver	= NULL;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_remote_wakeup_enabled = false;
> +	dwc->is_gadget_wakeup = false;
>   	dwc->max_cfg_eps = 0;
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
> @@ -3768,6 +3815,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>   	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>   
>   	dwc->connected = false;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_gadget_wakeup = false;
>   }
>   
>   static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> @@ -3855,6 +3904,10 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>   	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>   	reg &= ~(DWC3_DCFG_DEVADDR_MASK);
>   	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> +
> +	dwc->is_remote_wakeup_enabled = false;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_gadget_wakeup = false;
>   }
>   
>   static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
> @@ -3998,8 +4051,9 @@ 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)
>   {
> +	enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
>   	/*
>   	 * TODO take core out of low power mode when that's
>   	 * implemented.
> @@ -4010,6 +4064,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>   		dwc->gadget_driver->resume(dwc->gadget);
>   		spin_lock(&dwc->lock);
>   	}
> +
> +	dwc->link_state = next;
>   }
>   
>   static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> @@ -4091,6 +4147,13 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>   	}
>   
>   	switch (next) {
> +	case DWC3_LINK_STATE_U0:
> +		if (dwc->is_gadget_wakeup) {
> +			linksts_change_events_set(dwc, false);
> +			dwc3_resume_gadget(dwc);
> +			dwc->is_gadget_wakeup = false;
> +		}
> +		break;
>   	case DWC3_LINK_STATE_U1:
>   		if (dwc->speed == USB_SPEED_SUPER)
>   			dwc3_suspend_gadget(dwc);
> @@ -4159,7 +4222,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,
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4fe4287..3306b1c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1113,6 +1113,8 @@  struct dwc3_scratchpad_array {
  *		     address.
  * @num_ep_resized: carries the current number endpoints which have had its tx
  *		    fifo resized.
+ * @is_remote_wakeup_enabled: remote wakeup status from host perspective
+ * @is_gadget_wakeup: remote wakeup requested via gadget op.
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1326,6 +1328,8 @@  struct dwc3 {
 	int			max_cfg_eps;
 	int			last_fifo_depth;
 	int			num_ep_resized;
+	bool			is_remote_wakeup_enabled;
+	bool			is_gadget_wakeup;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 197af63..4cc3d3a 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -353,6 +353,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->is_remote_wakeup_enabled <<
+						USB_DEVICE_REMOTE_WAKEUP;
 		}
 
 		break;
@@ -473,6 +476,7 @@  static int dwc3_ep0_handle_device(struct dwc3 *dwc,
 
 	switch (wValue) {
 	case USB_DEVICE_REMOTE_WAKEUP:
+		dwc->is_remote_wakeup_enabled = 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 4366c45..d6697da 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2232,6 +2232,22 @@  static const struct usb_ep_ops dwc3_gadget_ep_ops = {
 
 /* -------------------------------------------------------------------------- */
 
+static void linksts_change_events_set(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);
+
+	/* Required to complete this operation before returning */
+	mb();
+}
+
 static int dwc3_gadget_get_frame(struct usb_gadget *g)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
@@ -2270,9 +2286,13 @@  static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		return -EINVAL;
 	}
 
+	linksts_change_events_set(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");
+		linksts_change_events_set(dwc, false);
+		dwc->is_gadget_wakeup = false;
 		return ret;
 	}
 
@@ -2284,9 +2304,15 @@  static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
 	}
 
+	/* If remote wakeup is triggered from function driver, bail out.
+	 * Since link status change events are enabled we would receive
+	 * an U0 event when wakeup is successful.
+	 */
+	if (dwc->is_gadget_wakeup)
+		return -EAGAIN;
+
 	/* poll until Link State changes to ON */
 	retries = 20000;
-
 	while (retries--) {
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 
@@ -2295,6 +2321,8 @@  static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 			break;
 	}
 
+	linksts_change_events_set(dwc, false);
+
 	if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
 		dev_err(dwc->dev, "failed to send remote wakeup\n");
 		return -EINVAL;
@@ -2310,7 +2338,20 @@  static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	int			ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
+	if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled) {
+		dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
+		ret =  -EPERM;
+		goto out;
+	}
+	if (dwc->is_gadget_wakeup) {
+		dev_err(dwc->dev, "%s: remote wakeup in progress\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+	dwc->is_gadget_wakeup = true;
 	ret = __dwc3_gadget_wakeup(dwc);
+
+out:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
@@ -2766,6 +2807,9 @@  static int dwc3_gadget_start(struct usb_gadget *g,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= driver;
+	linksts_change_events_set(dwc, false);
+	dwc->is_remote_wakeup_enabled = false;
+	dwc->is_gadget_wakeup = false;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
@@ -2785,6 +2829,9 @@  static int dwc3_gadget_stop(struct usb_gadget *g)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
+	linksts_change_events_set(dwc, false);
+	dwc->is_remote_wakeup_enabled = false;
+	dwc->is_gadget_wakeup = false;
 	dwc->max_cfg_eps = 0;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -3768,6 +3815,8 @@  static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
 	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
 
 	dwc->connected = false;
+	linksts_change_events_set(dwc, false);
+	dwc->is_gadget_wakeup = false;
 }
 
 static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
@@ -3855,6 +3904,10 @@  static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
 	reg &= ~(DWC3_DCFG_DEVADDR_MASK);
 	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+
+	dwc->is_remote_wakeup_enabled = false;
+	linksts_change_events_set(dwc, false);
+	dwc->is_gadget_wakeup = false;
 }
 
 static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
@@ -3998,8 +4051,9 @@  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)
 {
+	enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
 	/*
 	 * TODO take core out of low power mode when that's
 	 * implemented.
@@ -4010,6 +4064,8 @@  static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
 		dwc->gadget_driver->resume(dwc->gadget);
 		spin_lock(&dwc->lock);
 	}
+
+	dwc->link_state = next;
 }
 
 static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4091,6 +4147,13 @@  static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 	}
 
 	switch (next) {
+	case DWC3_LINK_STATE_U0:
+		if (dwc->is_gadget_wakeup) {
+			linksts_change_events_set(dwc, false);
+			dwc3_resume_gadget(dwc);
+			dwc->is_gadget_wakeup = false;
+		}
+		break;
 	case DWC3_LINK_STATE_U1:
 		if (dwc->speed == USB_SPEED_SUPER)
 			dwc3_suspend_gadget(dwc);
@@ -4159,7 +4222,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,