[PATCH 0/3] usb: dwc3-am62: Add system wake-up support

[PATCH 0/3] usb: dwc3-am62: Add system wake-up support

[Roger Quadros]

Hi,

This series enables System wake up support via USB events on TI's AM62 SoC.

cheers,
-roger

Aswath Govindraju (1):
  usb: dwc3-am62: Add support for system wakeup based on USB events

Roger Quadros (2):
  usb: dwc3-am62: Enable as a wakeup source by default
  usb: dwc3-am62: Fix up wake-up configuration and spurious wake up

 drivers/usb/dwc3/dwc3-am62.c | 54 ++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

-- 
2.34.1

[PATCH 1/3] usb: dwc3-am62: Add support for system wakeup based on USB events

[Roger Quadros]

From: Aswath Govindraju <a-govindraju@ti.com>

The USB2SS IP in TI's AM62 SoC is capable of supporting wakeup from
deep sleep based on the following events,

1) VBUS state change
2) Overcurrent detection
3) Line state change

Wakeup from these events can enabled by setting their corresponding bits
in the WAKEUP_CONFIG register. The events to be enabled are decided based
on the current role of the controller.

When the role of the controller is host, the comparators for detecting
VBUS state change are disabled while entering low power mode. This is done
as VBUS state is not used in host mode and disabling the comparators helps
in reducing the power consumption. So, wakeup from VBUS state change should
be disabled in host mode. While operating in peripheral mode all the wakeup
events can be enabled.

Therefore, add support for the same in the suspend/resume hooks.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/usb/dwc3/dwc3-am62.c | 39 ++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
index 173cf3579c55..867bfa1252b8 100644
--- a/drivers/usb/dwc3/dwc3-am62.c
+++ b/drivers/usb/dwc3/dwc3-am62.c
@@ -17,6 +17,8 @@
 #include <linux/regmap.h>
 #include <linux/pinctrl/consumer.h>
 
+#include "core.h"
+
 /* USB WRAPPER register offsets */
 #define USBSS_PID			0x0
 #define USBSS_OVERCURRENT_CTRL		0x4
@@ -45,6 +47,10 @@
 #define USBSS_PHY_VBUS_SEL_SHIFT	1
 #define USBSS_PHY_LANE_REVERSE		BIT(0)
 
+/* CORE STAT register bits */
+#define USBSS_CORE_OPERATIONAL_MODE_MASK	GENMASK(13, 12)
+#define USBSS_CORE_OPERATIONAL_MODE_SHIFT	12
+
 /* MODE CONTROL register bits */
 #define USBSS_MODE_VALID	BIT(0)
 
@@ -233,6 +239,9 @@ static int dwc3_ti_probe(struct platform_device *pdev)
 	reg |= USBSS_MODE_VALID;
 	dwc3_ti_writel(data, USBSS_MODE_CONTROL, reg);
 
+	/* Device has capability to wakeup system from sleep */
+	device_set_wakeup_capable(dev, true);
+
 	/* Setting up autosuspend */
 	pm_runtime_set_autosuspend_delay(dev, DWC3_AM62_AUTOSUSPEND_DELAY);
 	pm_runtime_use_autosuspend(dev);
@@ -281,6 +290,22 @@ static int dwc3_ti_remove(struct platform_device *pdev)
 static int dwc3_ti_suspend_common(struct device *dev)
 {
 	struct dwc3_data *data = dev_get_drvdata(dev);
+	u32 reg, current_prtcap_dir;
+
+	if (device_may_wakeup(dev)) {
+		reg = dwc3_ti_readl(data, USBSS_CORE_STAT);
+		current_prtcap_dir = (reg & USBSS_CORE_OPERATIONAL_MODE_MASK)
+				     >> USBSS_CORE_OPERATIONAL_MODE_SHIFT;
+		/* 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;
+		} else {
+			reg |= USBSS_WAKEUP_CFG_OVERCURRENT_EN | USBSS_WAKEUP_CFG_LINESTATE_EN |
+			       USBSS_WAKEUP_CFG_VBUSVALID_EN;
+		}
+		dwc3_ti_writel(data, USBSS_WAKEUP_CONFIG, reg);
+	}
 
 	clk_disable_unprepare(data->usb2_refclk);
 
@@ -290,9 +315,23 @@ static int dwc3_ti_suspend_common(struct device *dev)
 static int dwc3_ti_resume_common(struct device *dev)
 {
 	struct dwc3_data *data = dev_get_drvdata(dev);
+	u32 reg;
 
 	clk_prepare_enable(data->usb2_refclk);
 
+	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);
+	}
+
+	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);
+
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/3] usb: dwc3-am62: Enable as a wakeup source by default

[Roger Quadros]

USB module can wakeup system. Enable it as a wakeup source
by default. Finer grain wakeup enable/disable can be done
from the power/wakeup system control file of the respective
USB device.

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

diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
index 867bfa1252b8..859b48279658 100644
--- a/drivers/usb/dwc3/dwc3-am62.c
+++ b/drivers/usb/dwc3/dwc3-am62.c
@@ -241,6 +241,9 @@ static int dwc3_ti_probe(struct platform_device *pdev)
 
 	/* Device has capability to wakeup system from sleep */
 	device_set_wakeup_capable(dev, true);
+	ret = device_wakeup_enable(dev);
+	if (ret)
+		dev_err(dev, "couldn't enable device as a wakeup source: %d\n", ret);
 
 	/* Setting up autosuspend */
 	pm_runtime_set_autosuspend_delay(dev, DWC3_AM62_AUTOSUSPEND_DELAY);
-- 
2.34.1

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

[Roger Quadros]

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;
 		}
 		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;
 }
-- 
2.34.1

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

[Roger Quadros]

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

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

[Greg KH]

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

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

[Roger Quadros]

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

[PATCH v2] usb: dwc3-am62: Fix up wake-up configuration and spurious wake up

[Roger Quadros]

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 and in USB suspend (U2/L3)
else it will cause spurious wake up.

For now, don't enable LINESTATE. This means wake up from
USB resume will not work but at least we won't have any spurious
wake ups.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
Changelog:
v2: don't enable LINESTATE wake-up at all in device mode.

 drivers/usb/dwc3/dwc3-am62.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
index 859b48279658..b22fb78bc8e7 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 */
@@ -302,12 +310,17 @@ 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
+			 * and in U2/L3 state else it causes spurious wake-up.
+			 */
 		}
 		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 +337,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;
 }
-- 
2.34.1

Re: [PATCH v2] usb: dwc3-am62: Fix up wake-up configuration and spurious wake up

[Thinh Nguyen]

On Fri, Mar 24, 2023, 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 and in USB suspend (U2/L3)
> else it will cause spurious wake up.
> 
> For now, don't enable LINESTATE. This means wake up from
> USB resume will not work but at least we won't have any spurious
> wake ups.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
> Changelog:
> v2: don't enable LINESTATE wake-up at all in device mode.
> 
>  drivers/usb/dwc3/dwc3-am62.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-am62.c b/drivers/usb/dwc3/dwc3-am62.c
> index 859b48279658..b22fb78bc8e7 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 */
> @@ -302,12 +310,17 @@ 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
> +			 * and in U2/L3 state else it causes spurious wake-up.
> +			 */
>  		}
>  		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 +337,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;
>  }
> -- 
> 2.34.1
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH 0/3] usb: dwc3-am62: Add system wake-up support

[Thinh Nguyen]

Hi,

On Thu, Mar 16, 2023, Roger Quadros wrote:
> Hi,
> 
> This series enables System wake up support via USB events on TI's AM62 SoC.
> 
> cheers,
> -roger
> 
> Aswath Govindraju (1):
>   usb: dwc3-am62: Add support for system wakeup based on USB events
> 
> Roger Quadros (2):
>   usb: dwc3-am62: Enable as a wakeup source by default
>   usb: dwc3-am62: Fix up wake-up configuration and spurious wake up
> 
>  drivers/usb/dwc3/dwc3-am62.c | 54 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> -- 
> 2.34.1
> 

I can't review the detail config, but the logic looks fine to me.
For this entire series:

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh