diff mbox series

[3/3] usb: dwc3-am62: Fix up wake-up configuration and spurious wake up

Message ID 20230316131226.89540-4-rogerq@kernel.org (mailing list archive)
State New, archived
Headers show
Series usb: dwc3-am62: Add system wake-up support | expand

Commit Message

Roger Quadros March 16, 2023, 1:12 p.m. UTC 
Explicitly set and clear wakeup config so we don't leave anything
to chance.

Clear wakeup status on suspend so we know what caused wake up.

The LINESTATE wake up should not be enabled in device mode
if we are not connected to a USB host else it will cause spurious
wake up.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/dwc3/dwc3-am62.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Comments

Roger Quadros March 20, 2023, 8:24 a.m. UTC | #1 
Hi,

On 16/03/2023 15:12, Roger Quadros wrote:
> Explicitly set and clear wakeup config so we don't leave anything
> to chance.
> 
> Clear wakeup status on suspend so we know what caused wake up.
> 
> The LINESTATE wake up should not be enabled in device mode
> if we are not connected to a USB host else it will cause spurious
> wake up.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-am62.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
> index 859b48279658..af0524e2f1e1 100644
> --- a/drivers/usb/dwc3/dwc3-am62.c
> +++ b/drivers/usb/dwc3/dwc3-am62.c
> @@ -60,6 +60,13 @@
>  #define USBSS_WAKEUP_CFG_SESSVALID_EN	BIT(1)
>  #define USBSS_WAKEUP_CFG_VBUSVALID_EN	BIT(0)
>  
> +#define USBSS_WAKEUP_CFG_ALL	(USBSS_WAKEUP_CFG_VBUSVALID_EN | \
> +				 USBSS_WAKEUP_CFG_SESSVALID_EN | \
> +				 USBSS_WAKEUP_CFG_LINESTATE_EN | \
> +				 USBSS_WAKEUP_CFG_OVERCURRENT_EN)
> +
> +#define USBSS_WAKEUP_CFG_NONE	0
> +
>  /* WAKEUP STAT register bits */
>  #define USBSS_WAKEUP_STAT_OVERCURRENT	BIT(4)
>  #define USBSS_WAKEUP_STAT_LINESTATE	BIT(3)
> @@ -103,6 +110,7 @@ struct dwc3_data {
>  	struct regmap *syscon;
>  	unsigned int offset;
>  	unsigned int vbus_divider;
> +	u32 wakeup_stat;
>  };
>  
>  static const int dwc3_ti_rate_table[] = {	/* in KHZ */
> @@ -294,6 +302,7 @@ static int dwc3_ti_suspend_common(struct device *dev)
>  {
>  	struct dwc3_data *data = dev_get_drvdata(dev);
>  	u32 reg, current_prtcap_dir;
> +	u32 vbus_stat;
>  
>  	if (device_may_wakeup(dev)) {
>  		reg = dwc3_ti_readl(data, USBSS_CORE_STAT);
> @@ -302,12 +311,20 @@ static int dwc3_ti_suspend_common(struct device *dev)
>  		/* Set wakeup config enable bits */
>  		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
>  		if (current_prtcap_dir == DWC3_GCTL_PRTCAP_HOST) {
> -			reg |= USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
> +			reg = USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
>  		} else {
> -			reg |= USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
> -			       USBSS_WAKEUP_CFG_VBUSVALID_EN;
> +			reg = USBSS_WAKEUP_CFG_VBUSVALID_EN | USBSS_WAKEUP_CFG_SESSVALID_EN;
> +			/*
> +			 * Enable LINESTATE wake up only if connected to bus else
> +			 * it causes spurious wake-up.
> +			 */
> +			vbus_stat = dwc3_ti_readl(data, USBSS_VBUS_STAT);
> +			if (vbus_stat & (USBSS_VBUS_STAT_SESSVALID | USBSS_VBUS_STAT_VBUSVALID))
> +				reg |= USBSS_WAKEUP_CFG_LINESTATE_EN;

There is one corner case where a spurious wake-up still happens.
i.e. If we are not in USB_SUSPEND state while entering SoC sleep.

So looks like we need to check if we are in USB SUSPEND before enabling
LINESTATE wakeup enable.

>  		}
>  		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, reg);
> +		/* clear wakeup status so we know what caused the wake up */
> +		dwc3_ti_writel(data, USBSS_WAKEUP_STAT, USBSS_WAKEUP_STAT_CLR);
>  	}
>  
>  	clk_disable_unprepare(data->usb2_refclk);
> @@ -324,16 +341,11 @@ static int dwc3_ti_resume_common(struct device *dev)
>  
>  	if (device_may_wakeup(dev)) {
>  		/* Clear wakeup config enable bits */
> -		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
> -		reg &= ~(USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
> -			 USBSS_WAKEUP_CFG_VBUSVALID_EN);
> -		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, reg);
> +		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, USBSS_WAKEUP_CFG_NONE);
>  	}
>  
>  	reg = dwc3_ti_readl(data, USBSS_WAKEUP_STAT);
> -	/* Clear the wakeup status with wakeup clear bit */
> -	reg |= USBSS_WAKEUP_STAT_CLR;
> -	dwc3_ti_writel(data, USBSS_WAKEUP_STAT, reg);
> +	data->wakeup_stat = reg;
>  
>  	return 0;
>  }

cheers,
-roger
Greg Kroah-Hartman March 23, 2023, 6:18 p.m. UTC | #2 
On Mon, Mar 20, 2023 at 10:24:17AM +0200, Roger Quadros wrote:
> Hi,
> 
> On 16/03/2023 15:12, Roger Quadros wrote:
> > Explicitly set and clear wakeup config so we don't leave anything
> > to chance.
> > 
> > Clear wakeup status on suspend so we know what caused wake up.
> > 
> > The LINESTATE wake up should not be enabled in device mode
> > if we are not connected to a USB host else it will cause spurious
> > wake up.
> > 
> > Signed-off-by: Roger Quadros <rogerq@kernel.org>
> > ---
> >  drivers/usb/dwc3/dwc3-am62.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
> > index 859b48279658..af0524e2f1e1 100644
> > --- a/drivers/usb/dwc3/dwc3-am62.c
> > +++ b/drivers/usb/dwc3/dwc3-am62.c
> > @@ -60,6 +60,13 @@
> >  #define USBSS_WAKEUP_CFG_SESSVALID_EN	BIT(1)
> >  #define USBSS_WAKEUP_CFG_VBUSVALID_EN	BIT(0)
> >  
> > +#define USBSS_WAKEUP_CFG_ALL	(USBSS_WAKEUP_CFG_VBUSVALID_EN | \
> > +				 USBSS_WAKEUP_CFG_SESSVALID_EN | \
> > +				 USBSS_WAKEUP_CFG_LINESTATE_EN | \
> > +				 USBSS_WAKEUP_CFG_OVERCURRENT_EN)
> > +
> > +#define USBSS_WAKEUP_CFG_NONE	0
> > +
> >  /* WAKEUP STAT register bits */
> >  #define USBSS_WAKEUP_STAT_OVERCURRENT	BIT(4)
> >  #define USBSS_WAKEUP_STAT_LINESTATE	BIT(3)
> > @@ -103,6 +110,7 @@ struct dwc3_data {
> >  	struct regmap *syscon;
> >  	unsigned int offset;
> >  	unsigned int vbus_divider;
> > +	u32 wakeup_stat;
> >  };
> >  
> >  static const int dwc3_ti_rate_table[] = {	/* in KHZ */
> > @@ -294,6 +302,7 @@ static int dwc3_ti_suspend_common(struct device *dev)
> >  {
> >  	struct dwc3_data *data = dev_get_drvdata(dev);
> >  	u32 reg, current_prtcap_dir;
> > +	u32 vbus_stat;
> >  
> >  	if (device_may_wakeup(dev)) {
> >  		reg = dwc3_ti_readl(data, USBSS_CORE_STAT);
> > @@ -302,12 +311,20 @@ static int dwc3_ti_suspend_common(struct device *dev)
> >  		/* Set wakeup config enable bits */
> >  		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
> >  		if (current_prtcap_dir == DWC3_GCTL_PRTCAP_HOST) {
> > -			reg |= USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
> > +			reg = USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
> >  		} else {
> > -			reg |= USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
> > -			       USBSS_WAKEUP_CFG_VBUSVALID_EN;
> > +			reg = USBSS_WAKEUP_CFG_VBUSVALID_EN | USBSS_WAKEUP_CFG_SESSVALID_EN;
> > +			/*
> > +			 * Enable LINESTATE wake up only if connected to bus else
> > +			 * it causes spurious wake-up.
> > +			 */
> > +			vbus_stat = dwc3_ti_readl(data, USBSS_VBUS_STAT);
> > +			if (vbus_stat & (USBSS_VBUS_STAT_SESSVALID | USBSS_VBUS_STAT_VBUSVALID))
> > +				reg |= USBSS_WAKEUP_CFG_LINESTATE_EN;
> 
> There is one corner case where a spurious wake-up still happens.
> i.e. If we are not in USB_SUSPEND state while entering SoC sleep.
> 
> So looks like we need to check if we are in USB SUSPEND before enabling
> LINESTATE wakeup enable.

Ok, I'll not apply this one, only the first 2 now.

thanks,

greg k-h
Roger Quadros March 23, 2023, 7:41 p.m. UTC | #3 
Hi Greg,

On 23/03/2023 20:18, Greg KH wrote:
> On Mon, Mar 20, 2023 at 10:24:17AM +0200, Roger Quadros wrote:
>> Hi,
>>
>> On 16/03/2023 15:12, Roger Quadros wrote:
>>> Explicitly set and clear wakeup config so we don't leave anything
>>> to chance.
>>>
>>> Clear wakeup status on suspend so we know what caused wake up.
>>>
>>> The LINESTATE wake up should not be enabled in device mode
>>> if we are not connected to a USB host else it will cause spurious
>>> wake up.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>> ---
>>>  drivers/usb/dwc3/dwc3-am62.c | 32 ++++++++++++++++++++++----------
>>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
>>> index 859b48279658..af0524e2f1e1 100644
>>> --- a/drivers/usb/dwc3/dwc3-am62.c
>>> +++ b/drivers/usb/dwc3/dwc3-am62.c
>>> @@ -60,6 +60,13 @@
>>>  #define USBSS_WAKEUP_CFG_SESSVALID_EN	BIT(1)
>>>  #define USBSS_WAKEUP_CFG_VBUSVALID_EN	BIT(0)
>>>  
>>> +#define USBSS_WAKEUP_CFG_ALL	(USBSS_WAKEUP_CFG_VBUSVALID_EN | \
>>> +				 USBSS_WAKEUP_CFG_SESSVALID_EN | \
>>> +				 USBSS_WAKEUP_CFG_LINESTATE_EN | \
>>> +				 USBSS_WAKEUP_CFG_OVERCURRENT_EN)
>>> +
>>> +#define USBSS_WAKEUP_CFG_NONE	0
>>> +
>>>  /* WAKEUP STAT register bits */
>>>  #define USBSS_WAKEUP_STAT_OVERCURRENT	BIT(4)
>>>  #define USBSS_WAKEUP_STAT_LINESTATE	BIT(3)
>>> @@ -103,6 +110,7 @@ struct dwc3_data {
>>>  	struct regmap *syscon;
>>>  	unsigned int offset;
>>>  	unsigned int vbus_divider;
>>> +	u32 wakeup_stat;
>>>  };
>>>  
>>>  static const int dwc3_ti_rate_table[] = {	/* in KHZ */
>>> @@ -294,6 +302,7 @@ static int dwc3_ti_suspend_common(struct device *dev)
>>>  {
>>>  	struct dwc3_data *data = dev_get_drvdata(dev);
>>>  	u32 reg, current_prtcap_dir;
>>> +	u32 vbus_stat;
>>>  
>>>  	if (device_may_wakeup(dev)) {
>>>  		reg = dwc3_ti_readl(data, USBSS_CORE_STAT);
>>> @@ -302,12 +311,20 @@ static int dwc3_ti_suspend_common(struct device *dev)
>>>  		/* Set wakeup config enable bits */
>>>  		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
>>>  		if (current_prtcap_dir == DWC3_GCTL_PRTCAP_HOST) {
>>> -			reg |= USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
>>> +			reg = USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
>>>  		} else {
>>> -			reg |= USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
>>> -			       USBSS_WAKEUP_CFG_VBUSVALID_EN;
>>> +			reg = USBSS_WAKEUP_CFG_VBUSVALID_EN | USBSS_WAKEUP_CFG_SESSVALID_EN;
>>> +			/*
>>> +			 * Enable LINESTATE wake up only if connected to bus else
>>> +			 * it causes spurious wake-up.
>>> +			 */
>>> +			vbus_stat = dwc3_ti_readl(data, USBSS_VBUS_STAT);
>>> +			if (vbus_stat & (USBSS_VBUS_STAT_SESSVALID | USBSS_VBUS_STAT_VBUSVALID))
>>> +				reg |= USBSS_WAKEUP_CFG_LINESTATE_EN;
>>
>> There is one corner case where a spurious wake-up still happens.
>> i.e. If we are not in USB_SUSPEND state while entering SoC sleep.
>>
>> So looks like we need to check if we are in USB SUSPEND before enabling
>> LINESTATE wakeup enable.
> 
> Ok, I'll not apply this one, only the first 2 now.

Without this patch it is much worse as LINESTATE wake up is unconditionally
enabled. This will cause SoC to always wake up when in device mode.

I'll send a v2 patch for this one without LINESTATE wake up so at least
we are sure there is no spurious wake up. LINESTATE wakeup can be
enabled later when we have addressed all corner cases.

cheers,
-roger
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
index 859b48279658..af0524e2f1e1 100644
--- a/drivers/usb/dwc3/dwc3-am62.c
+++ b/drivers/usb/dwc3/dwc3-am62.c
@@ -60,6 +60,13 @@ 
 #define USBSS_WAKEUP_CFG_SESSVALID_EN	BIT(1)
 #define USBSS_WAKEUP_CFG_VBUSVALID_EN	BIT(0)
 
+#define USBSS_WAKEUP_CFG_ALL	(USBSS_WAKEUP_CFG_VBUSVALID_EN | \
+				 USBSS_WAKEUP_CFG_SESSVALID_EN | \
+				 USBSS_WAKEUP_CFG_LINESTATE_EN | \
+				 USBSS_WAKEUP_CFG_OVERCURRENT_EN)
+
+#define USBSS_WAKEUP_CFG_NONE	0
+
 /* WAKEUP STAT register bits */
 #define USBSS_WAKEUP_STAT_OVERCURRENT	BIT(4)
 #define USBSS_WAKEUP_STAT_LINESTATE	BIT(3)
@@ -103,6 +110,7 @@  struct dwc3_data {
 	struct regmap *syscon;
 	unsigned int offset;
 	unsigned int vbus_divider;
+	u32 wakeup_stat;
 };
 
 static const int dwc3_ti_rate_table[] = {	/* in KHZ */
@@ -294,6 +302,7 @@  static int dwc3_ti_suspend_common(struct device *dev)
 {
 	struct dwc3_data *data = dev_get_drvdata(dev);
 	u32 reg, current_prtcap_dir;
+	u32 vbus_stat;
 
 	if (device_may_wakeup(dev)) {
 		reg = dwc3_ti_readl(data, USBSS_CORE_STAT);
@@ -302,12 +311,20 @@  static int dwc3_ti_suspend_common(struct device *dev)
 		/* Set wakeup config enable bits */
 		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
 		if (current_prtcap_dir == DWC3_GCTL_PRTCAP_HOST) {
-			reg |= USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
+			reg = USBSS_WAKEUP_CFG_LINESTATE_EN | USBSS_WAKEUP_CFG_OVERCURRENT_EN;
 		} else {
-			reg |= USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
-			       USBSS_WAKEUP_CFG_VBUSVALID_EN;
+			reg = USBSS_WAKEUP_CFG_VBUSVALID_EN | USBSS_WAKEUP_CFG_SESSVALID_EN;
+			/*
+			 * Enable LINESTATE wake up only if connected to bus else
+			 * it causes spurious wake-up.
+			 */
+			vbus_stat = dwc3_ti_readl(data, USBSS_VBUS_STAT);
+			if (vbus_stat & (USBSS_VBUS_STAT_SESSVALID | USBSS_VBUS_STAT_VBUSVALID))
+				reg |= USBSS_WAKEUP_CFG_LINESTATE_EN;
 		}
 		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, reg);
+		/* clear wakeup status so we know what caused the wake up */
+		dwc3_ti_writel(data, USBSS_WAKEUP_STAT, USBSS_WAKEUP_STAT_CLR);
 	}
 
 	clk_disable_unprepare(data->usb2_refclk);
@@ -324,16 +341,11 @@  static int dwc3_ti_resume_common(struct device *dev)
 
 	if (device_may_wakeup(dev)) {
 		/* Clear wakeup config enable bits */
-		reg = dwc3_ti_readl(data, USBSS_WAKEUP_CONFIG);
-		reg &= ~(USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
-			 USBSS_WAKEUP_CFG_VBUSVALID_EN);
-		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, reg);
+		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, USBSS_WAKEUP_CFG_NONE);
 	}
 
 	reg = dwc3_ti_readl(data, USBSS_WAKEUP_STAT);
-	/* Clear the wakeup status with wakeup clear bit */
-	reg |= USBSS_WAKEUP_STAT_CLR;
-	dwc3_ti_writel(data, USBSS_WAKEUP_STAT, reg);
+	data->wakeup_stat = reg;
 
 	return 0;
 }