[PATCH v6 0/5] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[PATCH v6 0/5] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Théo Lebrun]

Currently, system-wide suspend is broken on J7200 because of a
controller reset. The TI wrapper does not get re-initialised at resume
and the first register access from cdns core fails.

We address that in two ways:

 - In the cdns3-ti wrapper, if a reset has occured at resume,
   we reconfigure the hardware.

 - We add a xhci->lost_power flag. Identical to the XHCI_RESET_ON_RESUME
   quirk, expect that it can be set at runtime.

   At resume, to summarise, we do:
      xhci->lost_power = cdns_power_is_lost(cdns);

The previous revision merged both XHCI_RESET_ON_RESUME quirk and
xhci->lost_power concepts into a single one (the quirk was the default
value of the flag). Now, we separate those. It simplifies things
because no additional compatible is required; we can detect everything
at runtime.

Have a nice day,
Théo

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Changes in v6:
- Drop two upstreamed patches:
  8e3dc6a51cca ("dt-bindings: usb: ti,j721e-usb: fix compatible list")
  d7fad3c5c53e ("arm64: dts: ti: k3-am64: add USB fallback compatible to J721E")
- dt-bindings: fix dt-schema syntax in compatible property.
- Change the approach about xhci->lost_power and the
  XHCI_RESET_ON_RESUME quirk. They are now separate and are checked
  independently at resume. The quirk stays the same, the flag can be
  detected at resume.
- Drop many patches, now that we don't add a new compatible for J7200:
  dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
  usb: cdns3: add quirk to platform data for reset-on-resume
  usb: cdns3-ti: grab auxdata from match data
  usb: cdns3-ti: add J7200 support with reset-on-resume behavior
  arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
- Link to v5: https://lore.kernel.org/r/20240726-s2r-cdns-v5-0-8664bfb032ac@bootlin.com

Changes in v5:
- dt-bindings: take Reviewed-by Rob and Conor for the first
  patch: "dt-bindings: usb: ti,j721e-usb: fix compatible list".
- cdns3-ti:
  - We now do have HW init code inside cdns_ti_reset_and_init_hw().
  - It gets called at probe unconditionally and from ->runtime_resume()
    if a reset is detected (using the W1 register).
  - Auxdata patches have been reworked now that there is default auxdata
    since commit b50a2da03bd9 ("usb: cdns3-ti: Add workaround for
    Errata i2409"). We now have a patch that moves auxdata to match
    data: "usb: cdns3-ti: grab auxdata from match data".
- cdns3/xhci: those are three new patches.
  - First, we rename "hibernated" to "lost_power" in arguments to
    the role ->resume() callbacks.
  - Then we add the xhci->lost_power flag, and only have it always copy
    the value from XHCI_RESET_ON_RESUME.
  - Finally, we set the flag from the host role driver.
- Link to v4: https://lore.kernel.org/lkml/20240307-j7200-usb-suspend-v4-0-5ec7615431f3@bootlin.com/

Changes in v4:
- dt-bindings: usb: ti,j721e-usb:
  - Remove ti,am64-usb single compatible entry.
  - Reverse ordering of compatible pair j721e + am64
    (becoming am64 + j721e).
  - Add j7200 + j721e compatible pair (versus only j7200). It is the
    same thing as am64: only the integration differs with base j721e
    compatible.
  - NOT taking trailers from Conor as patches changed substantially.
- arm64: dts: ti: j3-j7200:
  - Use j7200 + j721e compatible pair (versus only j7200 previously).
- arm64: dts: ti: j3-am64:
  - Fix to use am64 + j721e compatible pair (versus only am64).
    This is a new patch.
- Link to v3: https://lore.kernel.org/r/20240223-j7200-usb-suspend-v3-0-b41c9893a130@bootlin.com

Changes in v3:
- dt-bindings: use an enum to list compatibles instead of the previous
  odd construct. This is done in a separate patch from the one adding
  J7200 compatible.
- dt-bindings: dropped Acked-by Conor as the changes were modified a lot.
- Add runtime PM back. Put the init sequence in ->runtime_resume(). It
  gets called at probe for all compatibles and at resume for J7200.
- Introduce a cdns_ti_match_data struct rather than rely on compatible
  from code.
- Reorder code changes. Add infrastructure based on match data THEN add
  compatible and its match data.
- DTSI: use only J7200 compatible rather than both J7200 then J721E.
- Link to v2: https://lore.kernel.org/r/20231120-j7200-usb-suspend-v2-0-038c7e4a3df4@bootlin.com

Changes in v2:
- Remove runtime PM from cdns3-ti; it brings nothing. That means our
  cdns3-ti suspend/resume patch is simpler; there is no need to handle
  runtime PM at suspend/resume.
- Do not add cdns3 host role suspend/resume callbacks; they are not
  needed as core detects reset on resume & calls cdns_drd_host_on when
  needed.
- cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value
  computation.
- cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it
  is unneeded on our platform.
- Link to v1: https://lore.kernel.org/r/20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com

---
Théo Lebrun (5):
      usb: cdns3-ti: move reg writes to separate function
      usb: cdns3-ti: run HW init at resume() if HW was reset
      usb: cdns3: rename hibernated argument of role->resume() to lost_power
      xhci: introduce xhci->lost_power flag
      usb: cdns3: host: transmit lost_power signal from wrapper to XHCI

 drivers/usb/cdns3/cdns3-gadget.c |   4 +-
 drivers/usb/cdns3/cdns3-ti.c     | 109 +++++++++++++++++++++++++--------------
 drivers/usb/cdns3/cdnsp-gadget.c |   2 +-
 drivers/usb/cdns3/core.h         |   2 +-
 drivers/usb/cdns3/host.c         |  10 ++++
 drivers/usb/host/xhci.c          |   3 +-
 drivers/usb/host/xhci.h          |   6 +++
 7 files changed, 93 insertions(+), 43 deletions(-)
---
base-commit: d1869e31ecb0bb7397c6a04c29f281864e9257e3
change-id: 20240726-s2r-cdns-4b180cd960ff

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH v6 1/5] usb: cdns3-ti: move reg writes to separate function

[Théo Lebrun]

The device probe function mixes management code and hardware
initialisation code. Extract the latter into an explicitly named
cdns_ti_reset_and_init_hw() function to clarify intent. It also will
allow easier transition to using runtime PM for triggering HW init.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 84 ++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 040bb91e9c017d8298a7e251fd0e192a336e8a52..d704eb39820ad08a8774be7f00aa473c3ff267c0 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,7 @@ struct cdns_ti {
 	unsigned vbus_divider:1;
 	struct clk *usb2_refclk;
 	struct clk *lpm_clk;
+	int usb2_refclk_rate_code;
 };
 
 static const int cdns_ti_rate_table[] = {	/* in KHZ */
@@ -98,15 +99,52 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
 	{},
 };
 
+static void cdns_ti_reset_and_init_hw(struct cdns_ti *data)
+{
+	u32 reg;
+
+	/* assert RESET */
+	reg = cdns_ti_readl(data, USBSS_W1);
+	reg &= ~USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* set static config */
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
+	reg |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
+
+	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
+
+	if (data->vbus_divider)
+		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
+
+	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+
+	/* set USB2_ONLY mode if requested */
+	reg = cdns_ti_readl(data, USBSS_W1);
+
+	if (data->usb2_only)
+		reg |= USBSS_W1_USB2_ONLY;
+
+	/* set default modestrap */
+	reg |= USBSS_W1_MODESTRAP_SEL;
+	reg &= ~USBSS_W1_MODESTRAP_MASK;
+	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* de-assert RESET */
+	reg |= USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+}
+
 static int cdns_ti_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
 	struct cdns_ti *data;
-	int error;
-	u32 reg;
-	int rate_code, i;
 	unsigned long rate;
+	int error, i;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -146,7 +184,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	rate_code = i;
+	data->usb2_refclk_rate_code = i;
+	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
+	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+
+	cdns_ti_reset_and_init_hw(data);
 
 	pm_runtime_enable(dev);
 	error = pm_runtime_get_sync(dev);
@@ -155,40 +197,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	/* assert RESET */
-	reg = cdns_ti_readl(data, USBSS_W1);
-	reg &= ~USBSS_W1_PWRUP_RST;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
-	/* set static config */
-	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
-	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
-
-	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
-	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
-	if (data->vbus_divider)
-		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
-
-	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
-	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-
-	/* set USB2_ONLY mode if requested */
-	reg = cdns_ti_readl(data, USBSS_W1);
-	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
-	if (data->usb2_only)
-		reg |= USBSS_W1_USB2_ONLY;
-
-	/* set default modestrap */
-	reg |= USBSS_W1_MODESTRAP_SEL;
-	reg &= ~USBSS_W1_MODESTRAP_MASK;
-	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
-	/* de-assert RESET */
-	reg |= USBSS_W1_PWRUP_RST;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
 	error = of_platform_populate(node, NULL, cdns_ti_auxdata, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);

-- 
2.47.1

Re: [PATCH v6 1/5] usb: cdns3-ti: move reg writes to separate function

[Roger Quadros]

On 10/12/2024 19:13, Théo Lebrun wrote:
> The device probe function mixes management code and hardware
> initialisation code. Extract the latter into an explicitly named
> cdns_ti_reset_and_init_hw() function to clarify intent. It also will
> allow easier transition to using runtime PM for triggering HW init.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 84 ++++++++++++++++++++++++--------------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 040bb91e9c017d8298a7e251fd0e192a336e8a52..d704eb39820ad08a8774be7f00aa473c3ff267c0 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -58,6 +58,7 @@ struct cdns_ti {
>  	unsigned vbus_divider:1;
>  	struct clk *usb2_refclk;
>  	struct clk *lpm_clk;
> +	int usb2_refclk_rate_code;
>  };
>  
>  static const int cdns_ti_rate_table[] = {	/* in KHZ */
> @@ -98,15 +99,52 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
>  	{},
>  };
>  
> +static void cdns_ti_reset_and_init_hw(struct cdns_ti *data)
> +{
> +	u32 reg;
> +
> +	/* assert RESET */
> +	reg = cdns_ti_readl(data, USBSS_W1);
> +	reg &= ~USBSS_W1_PWRUP_RST;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +
> +	/* set static config */
> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> +	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
> +	reg |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
> +
> +	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
> +
new line not required?

> +	if (data->vbus_divider)
> +		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
> +
> +	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
> +	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> +
> +	/* set USB2_ONLY mode if requested */
> +	reg = cdns_ti_readl(data, USBSS_W1);
> +
here too?

> +	if (data->usb2_only)
> +		reg |= USBSS_W1_USB2_ONLY;
> +
> +	/* set default modestrap */
> +	reg |= USBSS_W1_MODESTRAP_SEL;
> +	reg &= ~USBSS_W1_MODESTRAP_MASK;
> +	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +
> +	/* de-assert RESET */
> +	reg |= USBSS_W1_PWRUP_RST;
> +	cdns_ti_writel(data, USBSS_W1, reg);
> +}
> +
>  static int cdns_ti_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = pdev->dev.of_node;
>  	struct cdns_ti *data;
> -	int error;
> -	u32 reg;
> -	int rate_code, i;
>  	unsigned long rate;
> +	int error, i;
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
> @@ -146,7 +184,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	rate_code = i;
> +	data->usb2_refclk_rate_code = i;
> +	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> +	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> +
> +	cdns_ti_reset_and_init_hw(data);
>  
>  	pm_runtime_enable(dev);
>  	error = pm_runtime_get_sync(dev);
> @@ -155,40 +197,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	/* assert RESET */
> -	reg = cdns_ti_readl(data, USBSS_W1);
> -	reg &= ~USBSS_W1_PWRUP_RST;
> -	cdns_ti_writel(data, USBSS_W1, reg);
> -
> -	/* set static config */
> -	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> -	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
> -	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
> -
> -	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
> -	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> -	if (data->vbus_divider)
> -		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
> -
> -	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
> -	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
> -
> -	/* set USB2_ONLY mode if requested */
> -	reg = cdns_ti_readl(data, USBSS_W1);
> -	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> -	if (data->usb2_only)
> -		reg |= USBSS_W1_USB2_ONLY;
> -
> -	/* set default modestrap */
> -	reg |= USBSS_W1_MODESTRAP_SEL;
> -	reg &= ~USBSS_W1_MODESTRAP_MASK;
> -	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
> -	cdns_ti_writel(data, USBSS_W1, reg);
> -
> -	/* de-assert RESET */
> -	reg |= USBSS_W1_PWRUP_RST;
> -	cdns_ti_writel(data, USBSS_W1, reg);
> -
>  	error = of_platform_populate(node, NULL, cdns_ti_auxdata, dev);
>  	if (error) {
>  		dev_err(dev, "failed to create children: %d\n", error);
> 

Reviewed-by: Roger Quadros <rogerq@kernel.org>

-- 
cheers,
-roger

[PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Théo Lebrun]

At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
if an hardware reset occurred. If it did, run the hardware init sequence.

This callback will be called at system-wide resume. Previously, if a
reset occurred during suspend, we would crash. The wrapper config had
not been written, leading to invalid register accesses inside cdns3.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
 	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
 	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
 
+	/*
+	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
+	 * detects it as uninitialised. We want to enforce a reset at probe,
+	 * and so do it manually here. This means the first runtime_resume()
+	 * will be a no-op.
+	 */
 	cdns_ti_reset_and_init_hw(data);
 
 	pm_runtime_enable(dev);
@@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+static int cdns_ti_runtime_resume(struct device *dev)
+{
+	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
+	struct cdns_ti *data = dev_get_drvdata(dev);
+	u32 w1;
+
+	w1 = cdns_ti_readl(data, USBSS_W1);
+	if ((w1 & mask) != mask)
+		cdns_ti_reset_and_init_hw(data);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
@@ -245,6 +269,7 @@ static struct platform_driver cdns_ti_driver = {
 	.driver		= {
 		.name	= "cdns3-ti",
 		.of_match_table	= cdns_ti_of_match,
+		.pm     = pm_ptr(&cdns_ti_pm_ops),
 	},
 };
 

-- 
2.47.1

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Roger Quadros]

On 10/12/2024 19:13, Théo Lebrun wrote:
> At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> if an hardware reset occurred. If it did, run the hardware init sequence.
> 
> This callback will be called at system-wide resume. Previously, if a
> reset occurred during suspend, we would crash. The wrapper config had
> not been written, leading to invalid register accesses inside cdns3.
> 

Did I understand right that the Controller reset can happen only at
system suspend and never at runtime suspend?

If so do you really need the runtime suspend/resume hooks?
you should have different system suspend/resume hooks than runtime suspend/resume
hooks and deal with the re-initialization in system resume hook.


> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
>  	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
>  
> +	/*
> +	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
> +	 * detects it as uninitialised. We want to enforce a reset at probe,
> +	 * and so do it manually here. This means the first runtime_resume()
> +	 * will be a no-op.
> +	 */

Separate system sleep hooks will also prevent this kind of behavior.

>  	cdns_ti_reset_and_init_hw(data);
>  
>  	pm_runtime_enable(dev);
> @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, NULL);
>  }
>  
> +static int cdns_ti_runtime_resume(struct device *dev)
> +{
> +	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	u32 w1;
> +
> +	w1 = cdns_ti_readl(data, USBSS_W1);
> +	if ((w1 & mask) != mask)
> +		cdns_ti_reset_and_init_hw(data);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cdns_ti_pm_ops = {
> +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +
>  static const struct of_device_id cdns_ti_of_match[] = {
>  	{ .compatible = "ti,j721e-usb", },
>  	{ .compatible = "ti,am64-usb", },
> @@ -245,6 +269,7 @@ static struct platform_driver cdns_ti_driver = {
>  	.driver		= {
>  		.name	= "cdns3-ti",
>  		.of_match_table	= cdns_ti_of_match,
> +		.pm     = pm_ptr(&cdns_ti_pm_ops),
>  	},
>  };
>  
> 

-- 
cheers,
-roger

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Théo Lebrun]

On Thu Dec 12, 2024 at 1:18 PM CET, Roger Quadros wrote:
> On 10/12/2024 19:13, Théo Lebrun wrote:
> > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> > if an hardware reset occurred. If it did, run the hardware init sequence.
> > 
> > This callback will be called at system-wide resume. Previously, if a
> > reset occurred during suspend, we would crash. The wrapper config had
> > not been written, leading to invalid register accesses inside cdns3.
> > 
>
> Did I understand right that the Controller reset can happen only at
> system suspend and never at runtime suspend?

J7200 + upstream kernel => if the power domain is cut off (it is
implicitly at runtime PM) then it resets. This is 100% board dependent.
We just never let it go into runtime suspend, for the moment.

> If so do you really need the runtime suspend/resume hooks?
> you should have different system suspend/resume hooks than runtime suspend/resume
> hooks and deal with the re-initialization in system resume hook.

The patch series works in the current setup with the wrapper that is
never shut off. But it would also work if someone decides to use RPM on
the wrapper.

Overall, the current kernel-wide strategy is to minimise
suspend/resume-specific code. Having only the concept of "runtime PM"
and triggering that at system-wide suspend/resume is easier to reason
about. It unifies concepts and reduces the states a device can be in.

We could even imagine a future where ->suspend|resume() callbacks
are pm_runtime_force_suspend|resume() by default.
That'd be the dream, at least.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Roger Quadros]

On 13/12/2024 17:28, Théo Lebrun wrote:
> On Thu Dec 12, 2024 at 1:18 PM CET, Roger Quadros wrote:
>> On 10/12/2024 19:13, Théo Lebrun wrote:
>>> At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
>>> if an hardware reset occurred. If it did, run the hardware init sequence.
>>>
>>> This callback will be called at system-wide resume. Previously, if a
>>> reset occurred during suspend, we would crash. The wrapper config had
>>> not been written, leading to invalid register accesses inside cdns3.
>>>
>>
>> Did I understand right that the Controller reset can happen only at
>> system suspend and never at runtime suspend?
> 
> J7200 + upstream kernel => if the power domain is cut off (it is
> implicitly at runtime PM) then it resets. This is 100% board dependent.
> We just never let it go into runtime suspend, for the moment.
> 
>> If so do you really need the runtime suspend/resume hooks?
>> you should have different system suspend/resume hooks than runtime suspend/resume
>> hooks and deal with the re-initialization in system resume hook.
> 
> The patch series works in the current setup with the wrapper that is
> never shut off. But it would also work if someone decides to use RPM on
> the wrapper.

But will it work? if we let it runtime suspend and controller looses power,
even though we restore the wrapper, who is restoring XHCI controller on
runtime resume?

Also we would be interested in wakeup events at runtime suspend
and by loosing power it doesn't look like it will ever wake up with any USB
event.

> 
> Overall, the current kernel-wide strategy is to minimise
> suspend/resume-specific code. Having only the concept of "runtime PM"
> and triggering that at system-wide suspend/resume is easier to reason
> about. It unifies concepts and reduces the states a device can be in.

or we just focus on system suspend/resume if the driver can't do
meaningful runtime suspend/resume?

> 
> We could even imagine a future where ->suspend|resume() callbacks
> are pm_runtime_force_suspend|resume() by default.
> That'd be the dream, at least.
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
cheers,
-roger

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Peter Chen]

On 24-12-10 18:13:36, Théo Lebrun wrote:
> At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> if an hardware reset occurred. If it did, run the hardware init sequence.
> 
> This callback will be called at system-wide resume. Previously, if a
> reset occurred during suspend, we would crash. The wrapper config had
> not been written, leading to invalid register accesses inside cdns3.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
>  	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
>  
> +	/*
> +	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
> +	 * detects it as uninitialised. We want to enforce a reset at probe,
> +	 * and so do it manually here. This means the first runtime_resume()
> +	 * will be a no-op.
> +	 */
>  	cdns_ti_reset_and_init_hw(data);
>  
>  	pm_runtime_enable(dev);
> @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, NULL);
>  }
>  
> +static int cdns_ti_runtime_resume(struct device *dev)
> +{
> +	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	u32 w1;
> +
> +	w1 = cdns_ti_readl(data, USBSS_W1);
> +	if ((w1 & mask) != mask)
> +		cdns_ti_reset_and_init_hw(data);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cdns_ti_pm_ops = {
> +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)

Why only runtime resume, but without runtime suspend?

Peter

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Théo Lebrun]

On Sat Dec 14, 2024 at 9:49 AM CET, Peter Chen wrote:
> On 24-12-10 18:13:36, Théo Lebrun wrote:
> > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> > if an hardware reset occurred. If it did, run the hardware init sequence.
> > 
> > This callback will be called at system-wide resume. Previously, if a
> > reset occurred during suspend, we would crash. The wrapper config had
> > not been written, leading to invalid register accesses inside cdns3.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >  	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> >  	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> >  
> > +	/*
> > +	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
> > +	 * detects it as uninitialised. We want to enforce a reset at probe,
> > +	 * and so do it manually here. This means the first runtime_resume()
> > +	 * will be a no-op.
> > +	 */
> >  	cdns_ti_reset_and_init_hw(data);
> >  
> >  	pm_runtime_enable(dev);
> > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, NULL);
> >  }
> >  
> > +static int cdns_ti_runtime_resume(struct device *dev)
> > +{
> > +	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
> > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > +	u32 w1;
> > +
> > +	w1 = cdns_ti_readl(data, USBSS_W1);
> > +	if ((w1 & mask) != mask)
> > +		cdns_ti_reset_and_init_hw(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops cdns_ti_pm_ops = {
> > +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
>
> Why only runtime resume, but without runtime suspend?

I don't see any situation where we would want "runtime suspend" be
equivalent to "reset the cdns3 wrapper". It implies losing child
states, triggering rediscovery of all USB devices at resume. Is that a
desired property of runtime suspend on any discoverable bus?

Sidenote: also, in our implementation, we do the standard thing of using
pm_runtime_force_suspend|resume() as suspend callback implementations.
With that, if we did reset the wrapper at suspend, we would:
 - always have to rediscover USB devices at resume and,
 - break wakeup sources.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

[Peter Chen]

On 24-12-16 15:02:43, Théo Lebrun wrote:
> On Sat Dec 14, 2024 at 9:49 AM CET, Peter Chen wrote:
> > On 24-12-10 18:13:36, Théo Lebrun wrote:
> > > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> > > if an hardware reset occurred. If it did, run the hardware init sequence.
> > > 
> > > This callback will be called at system-wide resume. Previously, if a
> > > reset occurred during suspend, we would crash. The wrapper config had
> > > not been written, leading to invalid register accesses inside cdns3.
> > > 
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > > ---
> > >  drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
> > > --- a/drivers/usb/cdns3/cdns3-ti.c
> > > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
> > >  	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> > >  	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> > >  
> > > +	/*
> > > +	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
> > > +	 * detects it as uninitialised. We want to enforce a reset at probe,
> > > +	 * and so do it manually here. This means the first runtime_resume()
> > > +	 * will be a no-op.
> > > +	 */
> > >  	cdns_ti_reset_and_init_hw(data);
> > >  
> > >  	pm_runtime_enable(dev);
> > > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
> > >  	platform_set_drvdata(pdev, NULL);
> > >  }
> > >  
> > > +static int cdns_ti_runtime_resume(struct device *dev)
> > > +{
> > > +	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
> > > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > > +	u32 w1;
> > > +
> > > +	w1 = cdns_ti_readl(data, USBSS_W1);
> > > +	if ((w1 & mask) != mask)
> > > +		cdns_ti_reset_and_init_hw(data);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops cdns_ti_pm_ops = {
> > > +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
> >
> > Why only runtime resume, but without runtime suspend?
> 
> I don't see any situation where we would want "runtime suspend" be
> equivalent to "reset the cdns3 wrapper". It implies losing child
> states, triggering rediscovery of all USB devices at resume. Is that a
> desired property of runtime suspend on any discoverable bus?

Usually, if wrapper needs to put controller to lower power state, 
it will do some jobs like close clock, etc. And if there is a option for
wakeup enable at wrapper, you may also need to set it.

Peter

[PATCH v6 3/5] usb: cdns3: rename hibernated argument of role->resume() to lost_power

[Théo Lebrun]

The cdns_role_driver->resume() callback takes a second boolean argument
named `hibernated` in its implementations. This is mistaken; the only
potential caller is:

int cdns_resume(struct cdns *cdns)
{
	/* ... */

	if (cdns->roles[cdns->role]->resume)
		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));

	return 0;
}

The argument can be true in cases outside of return from hibernation.
Reflect the true meaning by renaming both arguments to `lost_power`.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-gadget.c | 4 ++--
 drivers/usb/cdns3/cdnsp-gadget.c | 2 +-
 drivers/usb/cdns3/core.h         | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index fd1beb10bba726cef258e7438d642f31d6567dfe..694aa14577390b6e9a98ce8c4685ac8f56e43ad4 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -3468,7 +3468,7 @@ __must_hold(&cdns->lock)
 	return 0;
 }
 
-static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdns3_gadget_resume(struct cdns *cdns, bool lost_power)
 {
 	struct cdns3_device *priv_dev = cdns->gadget_dev;
 
@@ -3476,7 +3476,7 @@ static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
 		return 0;
 
 	cdns3_gadget_config(priv_dev);
-	if (hibernated)
+	if (lost_power)
 		writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf);
 
 	return 0;
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index 4a3f0f95825698f0524cace5c06bfcf27f763149..7d05180442fb94c5d1aab3d8ef766e365685f454 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -1973,7 +1973,7 @@ static int cdnsp_gadget_suspend(struct cdns *cdns, bool do_wakeup)
 	return 0;
 }
 
-static int cdnsp_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdnsp_gadget_resume(struct cdns *cdns, bool lost_power)
 {
 	struct cdnsp_device *pdev = cdns->gadget_dev;
 	enum usb_device_speed max_speed;
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 57d47348dc193b1060f4543c2ef22905f464293b..921cccf1ca9db27a66b06c7c7873b13537c74b05 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -30,7 +30,7 @@ struct cdns_role_driver {
 	int (*start)(struct cdns *cdns);
 	void (*stop)(struct cdns *cdns);
 	int (*suspend)(struct cdns *cdns, bool do_wakeup);
-	int (*resume)(struct cdns *cdns, bool hibernated);
+	int (*resume)(struct cdns *cdns, bool lost_power);
 	const char *name;
 #define CDNS_ROLE_STATE_INACTIVE	0
 #define CDNS_ROLE_STATE_ACTIVE		1

-- 
2.47.1

Re: [PATCH v6 3/5] usb: cdns3: rename hibernated argument of role->resume() to lost_power

[Peter Chen]

On 24-12-10 18:13:37, Théo Lebrun wrote:
> The cdns_role_driver->resume() callback takes a second boolean argument
> named `hibernated` in its implementations. This is mistaken; the only
> potential caller is:
> 
> int cdns_resume(struct cdns *cdns)
> {
> 	/* ... */
> 
> 	if (cdns->roles[cdns->role]->resume)
> 		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
> 
> 	return 0;
> }
> 
> The argument can be true in cases outside of return from hibernation.
> Reflect the true meaning by renaming both arguments to `lost_power`.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter
> ---
>  drivers/usb/cdns3/cdns3-gadget.c | 4 ++--
>  drivers/usb/cdns3/cdnsp-gadget.c | 2 +-
>  drivers/usb/cdns3/core.h         | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index fd1beb10bba726cef258e7438d642f31d6567dfe..694aa14577390b6e9a98ce8c4685ac8f56e43ad4 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -3468,7 +3468,7 @@ __must_hold(&cdns->lock)
>  	return 0;
>  }
>  
> -static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
> +static int cdns3_gadget_resume(struct cdns *cdns, bool lost_power)
>  {
>  	struct cdns3_device *priv_dev = cdns->gadget_dev;
>  
> @@ -3476,7 +3476,7 @@ static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
>  		return 0;
>  
>  	cdns3_gadget_config(priv_dev);
> -	if (hibernated)
> +	if (lost_power)
>  		writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf);
>  
>  	return 0;
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
> index 4a3f0f95825698f0524cace5c06bfcf27f763149..7d05180442fb94c5d1aab3d8ef766e365685f454 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -1973,7 +1973,7 @@ static int cdnsp_gadget_suspend(struct cdns *cdns, bool do_wakeup)
>  	return 0;
>  }
>  
> -static int cdnsp_gadget_resume(struct cdns *cdns, bool hibernated)
> +static int cdnsp_gadget_resume(struct cdns *cdns, bool lost_power)
>  {
>  	struct cdnsp_device *pdev = cdns->gadget_dev;
>  	enum usb_device_speed max_speed;
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 57d47348dc193b1060f4543c2ef22905f464293b..921cccf1ca9db27a66b06c7c7873b13537c74b05 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -30,7 +30,7 @@ struct cdns_role_driver {
>  	int (*start)(struct cdns *cdns);
>  	void (*stop)(struct cdns *cdns);
>  	int (*suspend)(struct cdns *cdns, bool do_wakeup);
> -	int (*resume)(struct cdns *cdns, bool hibernated);
> +	int (*resume)(struct cdns *cdns, bool lost_power);
>  	const char *name;
>  #define CDNS_ROLE_STATE_INACTIVE	0
>  #define CDNS_ROLE_STATE_ACTIVE		1
> 
> -- 
> 2.47.1
> 

[PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
expect a reset after resume. It is also used by some to enforce a XHCI
reset on resume (see needs-reset-on-resume DT prop).

Some wrappers are unsure beforehands if they will reset. Add a mechanism
to signal *at resume* if power has been lost. Parent devices can set
this flag, that defaults to false.

The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
controller. This is required as we do not know if a suspend will
trigger a reset, so the best guess is to avoid runtime PM.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/host/xhci.c | 3 ++-
 drivers/usb/host/xhci.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 
 	spin_lock_irq(&xhci->lock);
 
-	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
+	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
+	    xhci->broken_suspend || xhci->lost_power)
 		reinit_xhc = true;
 
 	if (!reinit_xhc) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1645,6 +1645,12 @@ struct xhci_hcd {
 	unsigned		broken_suspend:1;
 	/* Indicates that omitting hcd is supported if root hub has no ports */
 	unsigned		allow_single_roothub:1;
+	/*
+	 * Signal from upper stacks that we lost power during system-wide
+	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
+	 * it is safe for wrappers to not modify lost_power at resume.
+	 */
+	unsigned                lost_power:1;
 	/* cached extended protocol port capabilities */
 	struct xhci_port_cap	*port_caps;
 	unsigned int		num_port_caps;

-- 
2.47.1

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Roger Quadros]

On 10/12/2024 19:13, Théo Lebrun wrote:
> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
> expect a reset after resume. It is also used by some to enforce a XHCI
> reset on resume (see needs-reset-on-resume DT prop).
> 
> Some wrappers are unsure beforehands if they will reset. Add a mechanism
> to signal *at resume* if power has been lost. Parent devices can set
> this flag, that defaults to false.
> 
> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
> controller. This is required as we do not know if a suspend will
> trigger a reset, so the best guess is to avoid runtime PM.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/host/xhci.c | 3 ++-
>  drivers/usb/host/xhci.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>  
>  	spin_lock_irq(&xhci->lock);
>  
> -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
> +	    xhci->broken_suspend || xhci->lost_power)
>  		reinit_xhc = true;
>  
>  	if (!reinit_xhc) {
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1645,6 +1645,12 @@ struct xhci_hcd {
>  	unsigned		broken_suspend:1;
>  	/* Indicates that omitting hcd is supported if root hub has no ports */
>  	unsigned		allow_single_roothub:1;
> +	/*
> +	 * Signal from upper stacks that we lost power during system-wide
> +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
> +	 * it is safe for wrappers to not modify lost_power at resume.
> +	 */
> +	unsigned                lost_power:1;

I suppose this is private to XHCI driver and not legitimate to be accessed
by another driver after HCD is instantiated?
Doesn't access to xhci_hcd need to be serialized via xhci->lock?

Just curious, what happens if you don't include patch 4 and 5?
Is USB functionality still broken for you?

Doesn't XHCI driver detect that power was lost and issue a reset in any case
via the below code

        /* re-initialize the HC on Restore Error, or Host Controller Error */
        if ((temp & (STS_SRE | STS_HCE)) &&
            !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
                reinit_xhc = true;
                if (!xhci->broken_suspend)
                        xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
        }

>  	/* cached extended protocol port capabilities */
>  	struct xhci_port_cap	*port_caps;
>  	unsigned int		num_port_caps;
> 

-- 
cheers,
-roger

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote:
> On 10/12/2024 19:13, Théo Lebrun wrote:
> > The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
> > expect a reset after resume. It is also used by some to enforce a XHCI
> > reset on resume (see needs-reset-on-resume DT prop).
> > 
> > Some wrappers are unsure beforehands if they will reset. Add a mechanism
> > to signal *at resume* if power has been lost. Parent devices can set
> > this flag, that defaults to false.
> > 
> > The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
> > controller. This is required as we do not know if a suspend will
> > trigger a reset, so the best guess is to avoid runtime PM.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/host/xhci.c | 3 ++-
> >  drivers/usb/host/xhci.h | 6 ++++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
> >  
> >  	spin_lock_irq(&xhci->lock);
> >  
> > -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> > +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
> > +	    xhci->broken_suspend || xhci->lost_power)
> >  		reinit_xhc = true;
> >  
> >  	if (!reinit_xhc) {
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1645,6 +1645,12 @@ struct xhci_hcd {
> >  	unsigned		broken_suspend:1;
> >  	/* Indicates that omitting hcd is supported if root hub has no ports */
> >  	unsigned		allow_single_roothub:1;
> > +	/*
> > +	 * Signal from upper stacks that we lost power during system-wide
> > +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
> > +	 * it is safe for wrappers to not modify lost_power at resume.
> > +	 */
> > +	unsigned                lost_power:1;
>
> I suppose this is private to XHCI driver and not legitimate to be accessed
> by another driver after HCD is instantiated?

Yes it is private.

> Doesn't access to xhci_hcd need to be serialized via xhci->lock?

Good question. In theory maybe. In practice I don't see how
cdns_host_resume(), called by cdns_resume(), could clash with anything
else. I'll add that to be safe.

> Just curious, what happens if you don't include patch 4 and 5?
> Is USB functionality still broken for you?

No it works fine. Patches 4+5 are only there to avoid the below warning.
Logging "xHC error in resume" is a lie, so I want to avoid it.

> Doesn't XHCI driver detect that power was lost and issue a reset in any case
> via the below code
>
>         /* re-initialize the HC on Restore Error, or Host Controller Error */
>         if ((temp & (STS_SRE | STS_HCE)) &&
>             !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
>                 reinit_xhc = true;
>                 if (!xhci->broken_suspend)
>                         xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
>         }
>
> >  	/* cached extended protocol port capabilities */
> >  	struct xhci_port_cap	*port_caps;
> >  	unsigned int		num_port_caps;
> > 

I'll wait for your opinion on the [PATCH v6 2/5] email thread before
sending a new revision.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Roger Quadros]

On 13/12/2024 18:03, Théo Lebrun wrote:
> On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote:
>> On 10/12/2024 19:13, Théo Lebrun wrote:
>>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
>>> expect a reset after resume. It is also used by some to enforce a XHCI
>>> reset on resume (see needs-reset-on-resume DT prop).
>>>
>>> Some wrappers are unsure beforehands if they will reset. Add a mechanism
>>> to signal *at resume* if power has been lost. Parent devices can set
>>> this flag, that defaults to false.
>>>
>>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
>>> controller. This is required as we do not know if a suspend will
>>> trigger a reset, so the best guess is to avoid runtime PM.
>>>
>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>> ---
>>>  drivers/usb/host/xhci.c | 3 ++-
>>>  drivers/usb/host/xhci.h | 6 ++++++
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>>>  
>>>  	spin_lock_irq(&xhci->lock);
>>>  
>>> -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
>>> +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
>>> +	    xhci->broken_suspend || xhci->lost_power)
>>>  		reinit_xhc = true;
>>>  
>>>  	if (!reinit_xhc) {
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1645,6 +1645,12 @@ struct xhci_hcd {
>>>  	unsigned		broken_suspend:1;
>>>  	/* Indicates that omitting hcd is supported if root hub has no ports */
>>>  	unsigned		allow_single_roothub:1;
>>> +	/*
>>> +	 * Signal from upper stacks that we lost power during system-wide
>>> +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
>>> +	 * it is safe for wrappers to not modify lost_power at resume.
>>> +	 */
>>> +	unsigned                lost_power:1;
>>
>> I suppose this is private to XHCI driver and not legitimate to be accessed
>> by another driver after HCD is instantiated?
> 
> Yes it is private.
> 
>> Doesn't access to xhci_hcd need to be serialized via xhci->lock?
> 
> Good question. In theory maybe. In practice I don't see how
> cdns_host_resume(), called by cdns_resume(), could clash with anything
> else. I'll add that to be safe.
> 
>> Just curious, what happens if you don't include patch 4 and 5?
>> Is USB functionality still broken for you?
> 
> No it works fine. Patches 4+5 are only there to avoid the below warning.
> Logging "xHC error in resume" is a lie, so I want to avoid it.

How is it a lie?
The XHCI controller did loose its save/restore state during a PM operation.
As far as XHCI is concerned this is an error. no?

> 
>> Doesn't XHCI driver detect that power was lost and issue a reset in any case
>> via the below code
>>
>>         /* re-initialize the HC on Restore Error, or Host Controller Error */
>>         if ((temp & (STS_SRE | STS_HCE)) &&
>>             !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
>>                 reinit_xhc = true;
>>                 if (!xhci->broken_suspend)
>>                         xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
>>         }
>>
>>>  	/* cached extended protocol port capabilities */
>>>  	struct xhci_port_cap	*port_caps;
>>>  	unsigned int		num_port_caps;
>>>
> 
> I'll wait for your opinion on the [PATCH v6 2/5] email thread before
> sending a new revision.

Sorry for the delay. I'm not an expert in PM but will give my opinion there anyways.

> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> 

-- 
cheers,
-roger

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

Hello Roger, Peter, Pawel, Greg, Mathias,

On Tue Dec 17, 2024 at 10:00 PM CET, Roger Quadros wrote:
>
>
> On 13/12/2024 18:03, Théo Lebrun wrote:
> > On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote:
> >> On 10/12/2024 19:13, Théo Lebrun wrote:
> >>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
> >>> expect a reset after resume. It is also used by some to enforce a XHCI
> >>> reset on resume (see needs-reset-on-resume DT prop).
> >>>
> >>> Some wrappers are unsure beforehands if they will reset. Add a mechanism
> >>> to signal *at resume* if power has been lost. Parent devices can set
> >>> this flag, that defaults to false.
> >>>
> >>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
> >>> controller. This is required as we do not know if a suspend will
> >>> trigger a reset, so the best guess is to avoid runtime PM.
> >>>
> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >>> ---
> >>>  drivers/usb/host/xhci.c | 3 ++-
> >>>  drivers/usb/host/xhci.h | 6 ++++++
> >>>  2 files changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> >>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
> >>> --- a/drivers/usb/host/xhci.c
> >>> +++ b/drivers/usb/host/xhci.c
> >>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
> >>>  
> >>>  	spin_lock_irq(&xhci->lock);
> >>>  
> >>> -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> >>> +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
> >>> +	    xhci->broken_suspend || xhci->lost_power)
> >>>  		reinit_xhc = true;
> >>>  
> >>>  	if (!reinit_xhc) {
> >>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> >>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
> >>> --- a/drivers/usb/host/xhci.h
> >>> +++ b/drivers/usb/host/xhci.h
> >>> @@ -1645,6 +1645,12 @@ struct xhci_hcd {
> >>>  	unsigned		broken_suspend:1;
> >>>  	/* Indicates that omitting hcd is supported if root hub has no ports */
> >>>  	unsigned		allow_single_roothub:1;
> >>> +	/*
> >>> +	 * Signal from upper stacks that we lost power during system-wide
> >>> +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
> >>> +	 * it is safe for wrappers to not modify lost_power at resume.
> >>> +	 */
> >>> +	unsigned                lost_power:1;
> >>
> >> I suppose this is private to XHCI driver and not legitimate to be accessed
> >> by another driver after HCD is instantiated?
> > 
> > Yes it is private.
> > 
> >> Doesn't access to xhci_hcd need to be serialized via xhci->lock?
> > 
> > Good question. In theory maybe. In practice I don't see how
> > cdns_host_resume(), called by cdns_resume(), could clash with anything
> > else. I'll add that to be safe.
> > 
> >> Just curious, what happens if you don't include patch 4 and 5?
> >> Is USB functionality still broken for you?
> > 
> > No it works fine. Patches 4+5 are only there to avoid the below warning.
> > Logging "xHC error in resume" is a lie, so I want to avoid it.
>
> How is it a lie?
> The XHCI controller did loose its save/restore state during a PM operation.
> As far as XHCI is concerned this is an error. no?

The `xhci->quirks & XHCI_RESET_ON_RESUME` is exactly the same thing;
both the quirk and the flag we add have for purpose:

1. skipping this block

	if (!reinit_xhc) {
		retval = xhci_handshake(&xhci->op_regs->status,
					STS_CNR, 0, 10 * 1000 * 1000);
		// ...
		xhci_restore_registers(xhci);
		xhci_set_cmd_ring_deq(xhci);
		command = readl(&xhci->op_regs->command);
		command |= CMD_CRS;
		writel(command, &xhci->op_regs->command);
		if (xhci_handshake(&xhci->op_regs->status,
			      STS_RESTORE, 0, 100 * 1000)) {
			// ...
		}
	}

2. avoiding this warning:

	xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);

I don't think the block skipped is important in resume duration (to be
confirmed). But the xhci_warn() is not desired: we do not want to log
warnings if we know it is expected.

I'll think some more about it.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

On Wed Dec 18, 2024 at 6:49 PM CET, Théo Lebrun wrote:
> On Tue Dec 17, 2024 at 10:00 PM CET, Roger Quadros wrote:
> > On 13/12/2024 18:03, Théo Lebrun wrote:
> > > On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote:
> > >> On 10/12/2024 19:13, Théo Lebrun wrote:
> > >>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
> > >>> expect a reset after resume. It is also used by some to enforce a XHCI
> > >>> reset on resume (see needs-reset-on-resume DT prop).
> > >>>
> > >>> Some wrappers are unsure beforehands if they will reset. Add a mechanism
> > >>> to signal *at resume* if power has been lost. Parent devices can set
> > >>> this flag, that defaults to false.
> > >>>
> > >>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
> > >>> controller. This is required as we do not know if a suspend will
> > >>> trigger a reset, so the best guess is to avoid runtime PM.
> > >>>
> > >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > >>> ---
> > >>>  drivers/usb/host/xhci.c | 3 ++-
> > >>>  drivers/usb/host/xhci.h | 6 ++++++
> > >>>  2 files changed, 8 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > >>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
> > >>> --- a/drivers/usb/host/xhci.c
> > >>> +++ b/drivers/usb/host/xhci.c
> > >>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
> > >>>  
> > >>>  	spin_lock_irq(&xhci->lock);
> > >>>  
> > >>> -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> > >>> +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
> > >>> +	    xhci->broken_suspend || xhci->lost_power)
> > >>>  		reinit_xhc = true;
> > >>>  
> > >>>  	if (!reinit_xhc) {
> > >>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > >>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
> > >>> --- a/drivers/usb/host/xhci.h
> > >>> +++ b/drivers/usb/host/xhci.h
> > >>> @@ -1645,6 +1645,12 @@ struct xhci_hcd {
> > >>>  	unsigned		broken_suspend:1;
> > >>>  	/* Indicates that omitting hcd is supported if root hub has no ports */
> > >>>  	unsigned		allow_single_roothub:1;
> > >>> +	/*
> > >>> +	 * Signal from upper stacks that we lost power during system-wide
> > >>> +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
> > >>> +	 * it is safe for wrappers to not modify lost_power at resume.
> > >>> +	 */
> > >>> +	unsigned                lost_power:1;
> > >>
> > >> I suppose this is private to XHCI driver and not legitimate to be accessed
> > >> by another driver after HCD is instantiated?
> > > 
> > > Yes it is private.
> > > 
> > >> Doesn't access to xhci_hcd need to be serialized via xhci->lock?
> > > 
> > > Good question. In theory maybe. In practice I don't see how
> > > cdns_host_resume(), called by cdns_resume(), could clash with anything
> > > else. I'll add that to be safe.
> > > 
> > >> Just curious, what happens if you don't include patch 4 and 5?
> > >> Is USB functionality still broken for you?
> > > 
> > > No it works fine. Patches 4+5 are only there to avoid the below warning.
> > > Logging "xHC error in resume" is a lie, so I want to avoid it.
> >
> > How is it a lie?
> > The XHCI controller did loose its save/restore state during a PM operation.
> > As far as XHCI is concerned this is an error. no?
>
> The `xhci->quirks & XHCI_RESET_ON_RESUME` is exactly the same thing;
> both the quirk and the flag we add have for purpose:
>
> 1. skipping this block
>
> 	if (!reinit_xhc) {
> 		retval = xhci_handshake(&xhci->op_regs->status,
> 					STS_CNR, 0, 10 * 1000 * 1000);
> 		// ...
> 		xhci_restore_registers(xhci);
> 		xhci_set_cmd_ring_deq(xhci);
> 		command = readl(&xhci->op_regs->command);
> 		command |= CMD_CRS;
> 		writel(command, &xhci->op_regs->command);
> 		if (xhci_handshake(&xhci->op_regs->status,
> 			      STS_RESTORE, 0, 100 * 1000)) {
> 			// ...
> 		}
> 	}
>
> 2. avoiding this warning:
>
> 	xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
>
> I don't think the block skipped is important in resume duration (to be
> confirmed). But the xhci_warn() is not desired: we do not want to log
> warnings if we know it is expected.
>
> I'll think some more about it.

About this series, there were two discussions:

 - The desire to avoid putting the hardware init sequence of cdns3-ti
   inside  runtime_resume() as we don't do runtime PM.
   *That is fine and will be fixed for the next revision.*
   See [PATCH V6 2/5]: https://lore.kernel.org/lkml/8a1ed4be-c41c-46b6-ae25-33a6035b8c8d@kernel.org/

 - [PATCH V6 4/5] and [PATCH V6 5/5] are dedicated to avoiding a warning
   at XHCI resume on J7200:

      xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);

   https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-4-28a17f9715a2@bootlin.com/
   https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-5-28a17f9715a2@bootlin.com/

   Roger Quadros asked if we should not instead keep it, as there is
   indeed a reinit of the xHC block. I don't think we do: we don't want
   a warning at resume; IMO it would imply the reinit was unexpected.

   Proof is there is already a platform with a ->broken_suspend boolean
   that disables the warning even though there is a reinit. It doesn't
   log a warning as the reinit was expected.

   So we currently have:
    - xhci->broken_suspend: set at probe & implies the resume sequence
      won't work.
    - xhci->quirks & XHCI_RESET_ON_RESUME: set at probe & implies the
      controller reset during suspend.

   IIUC xhci->broken_suspend is NOT equivalent to "the controller reset
   during suspend". Else we wouldn't have both the broken_suspend flag
   and the XHCI_RESET_ON_RESUME quirk.

   In our case we want exactly the same thing as the
   XHCI_RESET_ON_RESUME quirk but updated at resume depending on what
   the wrapper driver detects.

   We could either:
   1. Update xhci->quirks at resume from upper layers.
   2. Introduce a xhci->lost_power flag. It would be strictly equivalent
      to the XHCI_RESET_ON_RESUME quirk BUT updated at resume from
      upper layers.

   @Mathias Nyman: what is your thought on the matter? Option (2) was
   the one taken in this series. Is there another option I am missing?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Mathias Nyman]

On 8.1.2025 12.59, Théo Lebrun wrote:
> On Wed Dec 18, 2024 at 6:49 PM CET, Théo Lebrun wrote:
>> On Tue Dec 17, 2024 at 10:00 PM CET, Roger Quadros wrote:
>>> On 13/12/2024 18:03, Théo Lebrun wrote:
>>>> On Thu Dec 12, 2024 at 1:37 PM CET, Roger Quadros wrote:
>>>>> On 10/12/2024 19:13, Théo Lebrun wrote:
>>>>>> The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they
>>>>>> expect a reset after resume. It is also used by some to enforce a XHCI
>>>>>> reset on resume (see needs-reset-on-resume DT prop).
>>>>>>
>>>>>> Some wrappers are unsure beforehands if they will reset. Add a mechanism
>>>>>> to signal *at resume* if power has been lost. Parent devices can set
>>>>>> this flag, that defaults to false.
>>>>>>
>>>>>> The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the
>>>>>> controller. This is required as we do not know if a suspend will
>>>>>> trigger a reset, so the best guess is to avoid runtime PM.
>>>>>>
>>>>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>>>>> ---
>>>>>>   drivers/usb/host/xhci.c | 3 ++-
>>>>>>   drivers/usb/host/xhci.h | 6 ++++++
>>>>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>>>> index 5ebde8cae4fc44cdb997b0f61314e309bda56c0d..ae2c8daa206a87da24d58a62b0a0485ebf68cdd6 100644
>>>>>> --- a/drivers/usb/host/xhci.c
>>>>>> +++ b/drivers/usb/host/xhci.c
>>>>>> @@ -1017,7 +1017,8 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>>>>>>   
>>>>>>   	spin_lock_irq(&xhci->lock);
>>>>>>   
>>>>>> -	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
>>>>>> +	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME ||
>>>>>> +	    xhci->broken_suspend || xhci->lost_power)
>>>>>>   		reinit_xhc = true;
>>>>>>   
>>>>>>   	if (!reinit_xhc) {
>>>>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>>>>> index 4914f0a10cff42dbc1448dcf7908534d582c848e..32526df75925989d40cbe7d59a187c945f498a30 100644
>>>>>> --- a/drivers/usb/host/xhci.h
>>>>>> +++ b/drivers/usb/host/xhci.h
>>>>>> @@ -1645,6 +1645,12 @@ struct xhci_hcd {
>>>>>>   	unsigned		broken_suspend:1;
>>>>>>   	/* Indicates that omitting hcd is supported if root hub has no ports */
>>>>>>   	unsigned		allow_single_roothub:1;
>>>>>> +	/*
>>>>>> +	 * Signal from upper stacks that we lost power during system-wide
>>>>>> +	 * suspend. Its default value is based on XHCI_RESET_ON_RESUME, meaning
>>>>>> +	 * it is safe for wrappers to not modify lost_power at resume.
>>>>>> +	 */
>>>>>> +	unsigned                lost_power:1;
>>>>>
>>>>> I suppose this is private to XHCI driver and not legitimate to be accessed
>>>>> by another driver after HCD is instantiated?
>>>>
>>>> Yes it is private.
>>>>
>>>>> Doesn't access to xhci_hcd need to be serialized via xhci->lock?
>>>>
>>>> Good question. In theory maybe. In practice I don't see how
>>>> cdns_host_resume(), called by cdns_resume(), could clash with anything
>>>> else. I'll add that to be safe.
>>>>
>>>>> Just curious, what happens if you don't include patch 4 and 5?
>>>>> Is USB functionality still broken for you?
>>>>
>>>> No it works fine. Patches 4+5 are only there to avoid the below warning.
>>>> Logging "xHC error in resume" is a lie, so I want to avoid it.
>>>
>>> How is it a lie?
>>> The XHCI controller did loose its save/restore state during a PM operation.
>>> As far as XHCI is concerned this is an error. no?
>>
>> The `xhci->quirks & XHCI_RESET_ON_RESUME` is exactly the same thing;
>> both the quirk and the flag we add have for purpose:
>>
>> 1. skipping this block
>>
>> 	if (!reinit_xhc) {
>> 		retval = xhci_handshake(&xhci->op_regs->status,
>> 					STS_CNR, 0, 10 * 1000 * 1000);
>> 		// ...
>> 		xhci_restore_registers(xhci);
>> 		xhci_set_cmd_ring_deq(xhci);
>> 		command = readl(&xhci->op_regs->command);
>> 		command |= CMD_CRS;
>> 		writel(command, &xhci->op_regs->command);
>> 		if (xhci_handshake(&xhci->op_regs->status,
>> 			      STS_RESTORE, 0, 100 * 1000)) {
>> 			// ...
>> 		}
>> 	}
>>
>> 2. avoiding this warning:
>>
>> 	xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
>>
>> I don't think the block skipped is important in resume duration (to be
>> confirmed). But the xhci_warn() is not desired: we do not want to log
>> warnings if we know it is expected.
>>
>> I'll think some more about it.
> 
> About this series, there were two discussions:
> 
>   - The desire to avoid putting the hardware init sequence of cdns3-ti
>     inside  runtime_resume() as we don't do runtime PM.
>     *That is fine and will be fixed for the next revision.*
>     See [PATCH V6 2/5]: https://lore.kernel.org/lkml/8a1ed4be-c41c-46b6-ae25-33a6035b8c8d@kernel.org/
> 
>   - [PATCH V6 4/5] and [PATCH V6 5/5] are dedicated to avoiding a warning
>     at XHCI resume on J7200:
> 
>        xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> 

Adding a new quirk or private xhci_cd meme


>     https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-4-28a17f9715a2@bootlin.com/
>     https://lore.kernel.org/lkml/20241210-s2r-cdns-v6-5-28a17f9715a2@bootlin.com/
> 
>     Roger Quadros asked if we should not instead keep it, as there is
>     indeed a reinit of the xHC block. I don't think we do: we don't want
>     a warning at resume; IMO it would imply the reinit was unexpected.
> 
>     Proof is there is already a platform with a ->broken_suspend boolean
>     that disables the warning even though there is a reinit. It doesn't
>     log a warning as the reinit was expected.
> 
>     So we currently have:
>      - xhci->broken_suspend: set at probe & implies the resume sequence
>        won't work.
>      - xhci->quirks & XHCI_RESET_ON_RESUME: set at probe & implies the
>        controller reset during suspend.
> 
>     IIUC xhci->broken_suspend is NOT equivalent to "the controller reset
>     during suspend". Else we wouldn't have both the broken_suspend flag
>     and the XHCI_RESET_ON_RESUME quirk.
> 
>     In our case we want exactly the same thing as the
>     XHCI_RESET_ON_RESUME quirk but updated at resume depending on what
>     the wrapper driver detects.
> 
>     We could either:
>     1. Update xhci->quirks at resume from upper layers.
>     2. Introduce a xhci->lost_power flag. It would be strictly equivalent
>        to the XHCI_RESET_ON_RESUME quirk BUT updated at resume from
>        upper layers.
> 
>     @Mathias Nyman: what is your thought on the matter? Option (2) was
>     the one taken in this series. Is there another option I am missing?

This would be a fourth way the upper layers inform xhci_resume() that the
xHC host should be reset instead of restoring the registers.

option 1 creates the first dynamic xhci->quirk flag,
option 2 adds a xhci->lost_power flag that is touched outside of xhci driver.

Neither seem like a good idea just to get rid of a dev_warn() message.

Maybe its time to split xhci_resume() into xhci_reset_resume()
and xhci_restore_resume(), and let those upper layers decide what they need.

Doesn't cdns driver already have a xhci_plat_priv resume_quirk() function
called  during xhci_plat_resume(), before xhci_resume()?
Can that be used?

Thanks
Mathias

Re: [PATCH v6 4/5] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

Hello Mathias,

On Wed Jan 8, 2025 at 7:43 PM CET, Mathias Nyman wrote:
> This would be a fourth way the upper layers inform xhci_resume() that the
> xHC host should be reset instead of restoring the registers.
>
> option 1 creates the first dynamic xhci->quirk flag,
> option 2 adds a xhci->lost_power flag that is touched outside of xhci driver.
>
> Neither seem like a good idea just to get rid of a dev_warn() message.
>
> Maybe its time to split xhci_resume() into xhci_reset_resume()
> and xhci_restore_resume(), and let those upper layers decide what they need.
>
> Doesn't cdns driver already have a xhci_plat_priv resume_quirk() function
> called  during xhci_plat_resume(), before xhci_resume()?
> Can that be used?

I agree with your feeling: another solution is needed. Discussing the
topic gave some new ideas and I have a new solution that feels much
more appropriate.

### Opinion on splitting xhci_resume() into two implementations

About splitting xhci_resume() into two different implementations that is
picked by above layer: I am not convinced.

What would go into xhci_reset_resume() and xhci_restore_resume()? There
isn't a clear cut inbetween the reset procedure and the normal restore
procedure, as we might do a reset if the normal restore procedure
fails (with some logging that reset was unexpected).

We would probably end up with many small functions called by either,
which would blur the overall step sequence.

### Proposal

Instead, I propose we keep xhci_resume() as a single function but change
its signature from the current:

   int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);

To this:

   int xhci_resume(struct xhci_hcd *xhci, bool power_lost,
                   bool is_auto_resume);

The key insight is that xhci_resume() extracts two information out of
the message:
 - whether we are after hibernation: msg.event == PM_EVENT_RESTORE,
 - whether this is an auto resume: msg.event == PM_EVENT_AUTO_RESUME.

First bulletpoint is somewhat wrong: driver wants to know if the device
did lose power, it doesn't care about hibernation per se. It just
happened to be that hibernation was the only case of power loss.
Knowing that, we can refactor to ask upper layers the right questions:
(1) "did we lose power?" and, (2) "is this an auto resume?".

Then, each caller is responsible for handing those booleans. If they
themselves receive pm_message_t messages (eg xhci-pci), then they do
the simple conversion:

      bool power_lost = msg.event == PM_EVENT_RESTORE;
      bool is_auto_resume = msg.event == PM_EVENT_AUTO_RESUME;

Others can do more more or less computation to pick a power_lost value.

### About xhci-plat power_lost value

In the case of xhci-plat, I think it will be:
 - A new power_lost field in `struct xhci_plat_priv`.
 - That gets set inside the cdns_role_driver::resume() callback of
   drivers/usb/cdns3/host.c.
 - Then xhci_plat_resume_common() computes power_lost being:
      power_lost = is_restore || priv->power_lost;

### About xhci_plat_priv::resume_quirk()

It isn't really useful to use. drivers/usb/cdns3/host.c can know whether
power was lost through its USB role resume() callback. From there to
the resume_quirk(), the boolean must be stored somewhere. That
somewhere can only be... inside xhci_plat_priv. So it is simpler if
xhci-plat retrieves the boolean directly.

xhci_plat_priv::resume_quirk() can change the power_lost value if it
wants to, that is fine. But in our case, the info isn't available from
there.

###

I am unsure if the above explanation is clear, so I'll be sending my
current work-in-progress series as an answer to this. The goal being
that you can give me your opinion on the proposal: ACK and we continue
in this direction, NACK and we keep digging.

I'll wait for the merge window to end before sending a proper revision.

Thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 1/9] usb: host: xhci-plat: mvebu: use ->quirks instead of ->init_quirk() func

[Théo Lebrun]

Compatible "marvell,armada3700-xhci" match data uses the
struct xhci_plat_priv::init_quirk() function pointer to add
XHCI_RESET_ON_RESUME as quirk on XHCI.

Instead, use the struct xhci_plat_priv::quirks field.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/host/xhci-mvebu.c | 10 ----------
 drivers/usb/host/xhci-mvebu.h |  6 ------
 drivers/usb/host/xhci-plat.c  |  2 +-
 3 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/usb/host/xhci-mvebu.c b/drivers/usb/host/xhci-mvebu.c
index 87f1597a0e5a..257e4d79971f 100644
--- a/drivers/usb/host/xhci-mvebu.c
+++ b/drivers/usb/host/xhci-mvebu.c
@@ -73,13 +73,3 @@ int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
 
 	return 0;
 }
-
-int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
-{
-	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
-
-	/* Without reset on resume, the HC won't work at all */
-	xhci->quirks |= XHCI_RESET_ON_RESUME;
-
-	return 0;
-}
diff --git a/drivers/usb/host/xhci-mvebu.h b/drivers/usb/host/xhci-mvebu.h
index 3be021793cc8..9d26e22c4842 100644
--- a/drivers/usb/host/xhci-mvebu.h
+++ b/drivers/usb/host/xhci-mvebu.h
@@ -12,16 +12,10 @@ struct usb_hcd;
 
 #if IS_ENABLED(CONFIG_USB_XHCI_MVEBU)
 int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd);
-int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd);
 #else
 static inline int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
 {
 	return 0;
 }
-
-static inline int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
-{
-	return 0;
-}
 #endif
 #endif /* __LINUX_XHCI_MVEBU_H */
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ecaa75718e59..49cc24e3ce23 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -106,7 +106,7 @@ static const struct xhci_plat_priv xhci_plat_marvell_armada = {
 };
 
 static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = {
-	.init_quirk = xhci_mvebu_a3700_init_quirk,
+	.quirks = XHCI_RESET_ON_RESUME,
 };
 
 static const struct xhci_plat_priv xhci_plat_brcm = {
-- 
2.48.1

[PATCH 2/9] usb: xhci: tegra: rename `runtime` boolean to `is_auto_runtime`

[Théo Lebrun]

Unify naming convention: use `is_auto_runtime` in xhci-tegra, to be in
phase with (future) drivers/usb/host/xhci.c.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/host/xhci-tegra.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 76f228e7443c..94dd32f1a0da 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -2161,11 +2161,11 @@ static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra)
 	}
 }
 
-static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime)
+static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd);
 	struct device *dev = tegra->dev;
-	bool wakeup = runtime ? true : device_may_wakeup(dev);
+	bool wakeup = is_auto_resume ? true : device_may_wakeup(dev);
 	unsigned int i;
 	int err;
 	u32 usbcmd;
@@ -2231,11 +2231,11 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime)
 	return err;
 }
 
-static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime)
+static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd);
 	struct device *dev = tegra->dev;
-	bool wakeup = runtime ? true : device_may_wakeup(dev);
+	bool wakeup = is_auto_resume ? true : device_may_wakeup(dev);
 	unsigned int i;
 	u32 usbcmd;
 	int err;
@@ -2286,7 +2286,7 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime)
 	if (wakeup)
 		tegra_xhci_disable_phy_sleepwalk(tegra);
 
-	err = xhci_resume(xhci, runtime ? PMSG_AUTO_RESUME : PMSG_RESUME);
+	err = xhci_resume(xhci, is_auto_resume ? PMSG_AUTO_RESUME : PMSG_RESUME);
 	if (err < 0) {
 		dev_err(tegra->dev, "failed to resume XHCI: %d\n", err);
 		goto disable_phy;
-- 
2.48.1

[PATCH 3/9] usb: cdns3-ti: move reg writes to separate function

[Théo Lebrun]

The device probe function mixes management code and hardware
initialisation code. Extract the latter into an explicitly named
cdns_ti_reset_and_init_hw() function to clarify intent. It also will
allow easier transition to using runtime PM for triggering HW init.

Reviewed-by: Roger Quadros <rogerq@kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 82 +++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index cfabc12ee0e3..2863249665c2 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,7 @@ struct cdns_ti {
 	unsigned vbus_divider:1;
 	struct clk *usb2_refclk;
 	struct clk *lpm_clk;
+	int usb2_refclk_rate_code;
 };
 
 static const int cdns_ti_rate_table[] = {	/* in KHZ */
@@ -98,15 +99,50 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
 	{},
 };
 
+static void cdns_ti_reset_and_init_hw(struct cdns_ti *data)
+{
+	u32 reg;
+
+	/* assert RESET */
+	reg = cdns_ti_readl(data, USBSS_W1);
+	reg &= ~USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* set static config */
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
+	reg |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
+
+	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
+	if (data->vbus_divider)
+		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
+
+	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
+	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+
+	/* set USB2_ONLY mode if requested */
+	reg = cdns_ti_readl(data, USBSS_W1);
+	if (data->usb2_only)
+		reg |= USBSS_W1_USB2_ONLY;
+
+	/* set default modestrap */
+	reg |= USBSS_W1_MODESTRAP_SEL;
+	reg &= ~USBSS_W1_MODESTRAP_MASK;
+	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
+	cdns_ti_writel(data, USBSS_W1, reg);
+
+	/* de-assert RESET */
+	reg |= USBSS_W1_PWRUP_RST;
+	cdns_ti_writel(data, USBSS_W1, reg);
+}
+
 static int cdns_ti_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
 	struct cdns_ti *data;
-	int error;
-	u32 reg;
-	int rate_code, i;
 	unsigned long rate;
+	int error, i;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -146,7 +182,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	rate_code = i;
+	data->usb2_refclk_rate_code = i;
+	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
+	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+
+	cdns_ti_reset_and_init_hw(data);
 
 	pm_runtime_enable(dev);
 	error = pm_runtime_get_sync(dev);
@@ -155,40 +195,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	/* assert RESET */
-	reg = cdns_ti_readl(data, USBSS_W1);
-	reg &= ~USBSS_W1_PWRUP_RST;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
-	/* set static config */
-	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-	reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
-	reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
-
-	reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
-	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
-	if (data->vbus_divider)
-		reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
-
-	cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
-	reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-
-	/* set USB2_ONLY mode if requested */
-	reg = cdns_ti_readl(data, USBSS_W1);
-	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
-	if (data->usb2_only)
-		reg |= USBSS_W1_USB2_ONLY;
-
-	/* set default modestrap */
-	reg |= USBSS_W1_MODESTRAP_SEL;
-	reg &= ~USBSS_W1_MODESTRAP_MASK;
-	reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
-	/* de-assert RESET */
-	reg |= USBSS_W1_PWRUP_RST;
-	cdns_ti_writel(data, USBSS_W1, reg);
-
 	error = of_platform_populate(node, NULL, cdns_ti_auxdata, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);
-- 
2.48.1

[PATCH 4/9] usb: cdns3-ti: run HW init at resume() if HW was reset

[Théo Lebrun]

At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
if an hardware reset occurred. If it did, run the hardware init sequence.

This callback will be called at system-wide resume. Previously, if a
reset occurred during suspend, we would crash. The wrapper config had
not been written, leading to invalid register accesses inside cdns3.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 2863249665c2..225993f7bdb6 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -186,6 +186,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
 	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
 	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
 
+	/*
+	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
+	 * detects it as uninitialised. We want to enforce a reset at probe,
+	 * and so do it manually here. This means the first runtime_resume()
+	 * will be a no-op.
+	 */
 	cdns_ti_reset_and_init_hw(data);
 
 	pm_runtime_enable(dev);
@@ -230,6 +236,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+static int cdns_ti_runtime_resume(struct device *dev)
+{
+	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
+	struct cdns_ti *data = dev_get_drvdata(dev);
+	u32 w1;
+
+	w1 = cdns_ti_readl(data, USBSS_W1);
+	if ((w1 & mask) != mask)
+		cdns_ti_reset_and_init_hw(data);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
@@ -243,6 +267,7 @@ static struct platform_driver cdns_ti_driver = {
 	.driver		= {
 		.name	= "cdns3-ti",
 		.of_match_table	= cdns_ti_of_match,
+		.pm     = pm_ptr(&cdns_ti_pm_ops),
 	},
 };
 
-- 
2.48.1

[PATCH 5/9] usb: cdns3: rename hibernated argument of role->resume() to lost_power

[Théo Lebrun]

The cdns_role_driver->resume() callback takes a second boolean argument
named `hibernated` in its implementations. This is mistaken; the only
potential caller is:

int cdns_resume(struct cdns *cdns)
{
	/* ... */

	if (cdns->roles[cdns->role]->resume)
		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));

	return 0;
}

The argument can be true in cases outside of return from hibernation.
Reflect the true meaning by renaming both arguments to `lost_power`.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-gadget.c | 4 ++--
 drivers/usb/cdns3/cdnsp-gadget.c | 2 +-
 drivers/usb/cdns3/core.h         | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index fd1beb10bba7..694aa1457739 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -3468,7 +3468,7 @@ __must_hold(&cdns->lock)
 	return 0;
 }
 
-static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdns3_gadget_resume(struct cdns *cdns, bool lost_power)
 {
 	struct cdns3_device *priv_dev = cdns->gadget_dev;
 
@@ -3476,7 +3476,7 @@ static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
 		return 0;
 
 	cdns3_gadget_config(priv_dev);
-	if (hibernated)
+	if (lost_power)
 		writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf);
 
 	return 0;
diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
index 4a3f0f958256..7d05180442fb 100644
--- a/drivers/usb/cdns3/cdnsp-gadget.c
+++ b/drivers/usb/cdns3/cdnsp-gadget.c
@@ -1973,7 +1973,7 @@ static int cdnsp_gadget_suspend(struct cdns *cdns, bool do_wakeup)
 	return 0;
 }
 
-static int cdnsp_gadget_resume(struct cdns *cdns, bool hibernated)
+static int cdnsp_gadget_resume(struct cdns *cdns, bool lost_power)
 {
 	struct cdnsp_device *pdev = cdns->gadget_dev;
 	enum usb_device_speed max_speed;
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 57d47348dc19..921cccf1ca9d 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -30,7 +30,7 @@ struct cdns_role_driver {
 	int (*start)(struct cdns *cdns);
 	void (*stop)(struct cdns *cdns);
 	int (*suspend)(struct cdns *cdns, bool do_wakeup);
-	int (*resume)(struct cdns *cdns, bool hibernated);
+	int (*resume)(struct cdns *cdns, bool lost_power);
 	const char *name;
 #define CDNS_ROLE_STATE_INACTIVE	0
 #define CDNS_ROLE_STATE_ACTIVE		1
-- 
2.48.1

[PATCH 6/9] usb: cdns3: call cdns_power_is_lost() only once in cdns_resume()

[Théo Lebrun]

cdns_power_is_lost() does a register read.
Call it only once rather than twice.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 465e9267b49c..799987c88960 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -524,11 +524,12 @@ EXPORT_SYMBOL_GPL(cdns_suspend);
 
 int cdns_resume(struct cdns *cdns)
 {
+	bool power_lost = cdns_power_is_lost(cdns);
 	enum usb_role real_role;
 	bool role_changed = false;
 	int ret = 0;
 
-	if (cdns_power_is_lost(cdns)) {
+	if (power_lost) {
 		if (cdns->role_sw) {
 			cdns->role = cdns_role_get(cdns->role_sw);
 		} else {
@@ -553,7 +554,7 @@ int cdns_resume(struct cdns *cdns)
 	}
 
 	if (cdns->roles[cdns->role]->resume)
-		cdns->roles[cdns->role]->resume(cdns, cdns_power_is_lost(cdns));
+		cdns->roles[cdns->role]->resume(cdns, power_lost);
 
 	return 0;
 }
-- 
2.48.1

[PATCH 7/9] usb: xhci: change xhci_resume() parameters to explicit the desired info

[Théo Lebrun]

Previous signature was:

        int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);

Internally, it extracted two information out of the message:
 - whether we are after hibernation: msg.event == PM_EVENT_RESTORE,
 - whether this is an auto resume: msg.event == PM_EVENT_AUTO_RESUME.

First bulletpoint is somewhat wrong: driver wants to know if the device
did lose power, it doesn't care about hibernation per se. Knowing that,
refactor to ask upper layers the right questions: (1) "did we lose
power?" and, (2) "is this an auto resume?". Change the signature to:

        int xhci_resume(struct xhci_hcd *xhci, bool power_lost,
                        bool is_auto_resume);

The goal is to allow some upper layers (cdns3-plat) to tell us when
power was lost after system-wise suspend.

Note that lost_power is ORed at the start of xhci_resume() to
xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend. It is
simpler to keep those checks inside of xhci_resume() instead of doing
them at each caller of xhci_resume().

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/host/xhci-histb.c |  2 +-
 drivers/usb/host/xhci-pci.c   |  8 +++++---
 drivers/usb/host/xhci-plat.c  | 10 +++++-----
 drivers/usb/host/xhci-tegra.c |  2 +-
 drivers/usb/host/xhci.c       | 19 ++++++++-----------
 drivers/usb/host/xhci.h       |  2 +-
 6 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c
index f9a4a4b0eb57..dde2ae0e9a0f 100644
--- a/drivers/usb/host/xhci-histb.c
+++ b/drivers/usb/host/xhci-histb.c
@@ -355,7 +355,7 @@ static int __maybe_unused xhci_histb_resume(struct device *dev)
 	if (!device_may_wakeup(dev))
 		xhci_histb_host_enable(histb);
 
-	return xhci_resume(xhci, PMSG_RESUME);
+	return xhci_resume(xhci, false, false);
 }
 
 static const struct dev_pm_ops xhci_histb_pm_ops = {
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index cb07cee9ed0c..6362a8d9a74d 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -807,8 +807,10 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 
 static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
 {
-	struct xhci_hcd		*xhci = hcd_to_xhci(hcd);
-	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+	bool power_lost = msg.event == PM_EVENT_RESTORE;
+	bool is_auto_resume = msg.event == PM_EVENT_AUTO_RESUME;
 
 	reset_control_reset(xhci->reset);
 
@@ -839,7 +841,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
 
-	return xhci_resume(xhci, msg);
+	return xhci_resume(xhci, power_lost, is_auto_resume);
 }
 
 static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 49cc24e3ce23..831af518a6ec 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -476,7 +476,7 @@ static int xhci_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
+static int xhci_plat_resume_common(struct device *dev, bool power_lost)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
@@ -498,7 +498,7 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
 	if (ret)
 		goto disable_clks;
 
-	ret = xhci_resume(xhci, pmsg);
+	ret = xhci_resume(xhci, power_lost, false);
 	if (ret)
 		goto disable_clks;
 
@@ -519,12 +519,12 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg)
 
 static int xhci_plat_resume(struct device *dev)
 {
-	return xhci_plat_resume_common(dev, PMSG_RESUME);
+	return xhci_plat_resume_common(dev, false);
 }
 
 static int xhci_plat_restore(struct device *dev)
 {
-	return xhci_plat_resume_common(dev, PMSG_RESTORE);
+	return xhci_plat_resume_common(dev, true);
 }
 
 static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
@@ -545,7 +545,7 @@ static int __maybe_unused xhci_plat_runtime_resume(struct device *dev)
 	struct usb_hcd  *hcd = dev_get_drvdata(dev);
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 
-	return xhci_resume(xhci, PMSG_AUTO_RESUME);
+	return xhci_resume(xhci, false, true);
 }
 
 const struct dev_pm_ops xhci_plat_pm_ops = {
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 94dd32f1a0da..74a4a2719e4f 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -2286,7 +2286,7 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool is_auto_resume)
 	if (wakeup)
 		tegra_xhci_disable_phy_sleepwalk(tegra);
 
-	err = xhci_resume(xhci, is_auto_resume ? PMSG_AUTO_RESUME : PMSG_RESUME);
+	err = xhci_resume(xhci, false, is_auto_resume);
 	if (err < 0) {
 		dev_err(tegra->dev, "failed to resume XHCI: %d\n", err);
 		goto disable_phy;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 899c0effb5d3..ccdc74b24d3f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1001,16 +1001,14 @@ EXPORT_SYMBOL_GPL(xhci_suspend);
  * This is called when the machine transition from S3/S4 mode.
  *
  */
-int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
+int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 {
-	bool			hibernated = (msg.event == PM_EVENT_RESTORE);
 	u32			command, temp = 0;
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	int			retval = 0;
 	bool			comp_timer_running = false;
 	bool			pending_portevent = false;
 	bool			suspended_usb3_devs = false;
-	bool			reinit_xhc = false;
 
 	if (!hcd->state)
 		return 0;
@@ -1029,10 +1027,10 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 
 	spin_lock_irq(&xhci->lock);
 
-	if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
-		reinit_xhc = true;
+	if (xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
+		power_lost = true;
 
-	if (!reinit_xhc) {
+	if (!power_lost) {
 		/*
 		 * Some controllers might lose power during suspend, so wait
 		 * for controller not ready bit to clear, just as in xHC init.
@@ -1072,12 +1070,12 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 	/* re-initialize the HC on Restore Error, or Host Controller Error */
 	if ((temp & (STS_SRE | STS_HCE)) &&
 	    !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
-		reinit_xhc = true;
-		if (!xhci->broken_suspend)
+		if (!power_lost)
 			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
+		power_lost = true;
 	}
 
-	if (reinit_xhc) {
+	if (power_lost) {
 		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
 				!(xhci_all_ports_seen_u0(xhci))) {
 			del_timer_sync(&xhci->comp_mode_recovery_timer);
@@ -1175,8 +1173,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 
 		pending_portevent = xhci_pending_portevent(xhci);
 
-		if (suspended_usb3_devs && !pending_portevent &&
-		    msg.event == PM_EVENT_AUTO_RESUME) {
+		if (suspended_usb3_devs && !pending_portevent && is_auto_resume) {
 			msleep(120);
 			pending_portevent = xhci_pending_portevent(xhci);
 		}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f0fb696d5619..4f045a864ac1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1859,7 +1859,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
 int xhci_ext_cap_init(struct xhci_hcd *xhci);
 
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
-int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg);
+int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume);
 
 irqreturn_t xhci_irq(struct usb_hcd *hcd);
 irqreturn_t xhci_msi_irq(int irq, void *hcd);
-- 
2.48.1

[PATCH 8/9] usb: host: xhci-plat: allow upper layers to signal power loss

[Théo Lebrun]

Now that xhci_resume() exposes a power_lost boolean argument, expose
that to all xhci-plat implementations. They are free to set it from
wherever they want:
 - Their own resume() callback.
 - The xhci_plat_priv::resume_quirk() callback.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/host/xhci-plat.c | 3 ++-
 drivers/usb/host/xhci-plat.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 831af518a6ec..8b18494ccc41 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -479,6 +479,7 @@ static int xhci_plat_suspend(struct device *dev)
 static int xhci_plat_resume_common(struct device *dev, bool power_lost)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	int ret;
 
@@ -498,7 +499,7 @@ static int xhci_plat_resume_common(struct device *dev, bool power_lost)
 	if (ret)
 		goto disable_clks;
 
-	ret = xhci_resume(xhci, power_lost, false);
+	ret = xhci_resume(xhci, power_lost || priv->power_lost, false);
 	if (ret)
 		goto disable_clks;
 
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 6475130eac4b..fe4f95e690fa 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -15,6 +15,7 @@ struct usb_hcd;
 struct xhci_plat_priv {
 	const char *firmware_name;
 	unsigned long long quirks;
+	bool power_lost;
 	void (*plat_start)(struct usb_hcd *);
 	int (*init_quirk)(struct usb_hcd *);
 	int (*suspend_quirk)(struct usb_hcd *);
-- 
2.48.1

[PATCH 9/9] usb: host: cdns3: forward lost power information to xhci

[Théo Lebrun]

cdns3-plat can know if power was lost across system-wide suspend.
Forward that information:

 - Grab the lost_power bool from cdns_role_driver::resume(). Store it
   into the power_lost field in struct xhci_plat_priv.

 - xhci-plat will call xhci_resume() with that value (ORed to whether we
   are in a hibernation restore).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/host.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 7ba760ee62e3..f0df114c2b53 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -138,6 +138,16 @@ static void cdns_host_exit(struct cdns *cdns)
 	cdns_drd_host_off(cdns);
 }
 
+static int cdns_host_resume(struct cdns *cdns, bool power_lost)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+	priv->power_lost = power_lost;
+
+	return 0;
+}
+
 int cdns_host_init(struct cdns *cdns)
 {
 	struct cdns_role_driver *rdrv;
@@ -148,6 +158,7 @@ int cdns_host_init(struct cdns *cdns)
 
 	rdrv->start	= __cdns_host_init;
 	rdrv->stop	= cdns_host_exit;
+	rdrv->resume	= cdns_host_resume;
 	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
 	rdrv->name	= "host";
 
-- 
2.48.1

[PATCH v6 5/5] usb: cdns3: host: transmit lost_power signal from wrapper to XHCI

[Théo Lebrun]

cdns_role_driver->resume() receives the information if power was lost
across suspend (ie if a reset occurred). Transmit that to the XHCI core
using the newly introduced lost_power flag.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/host.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 7ba760ee62e3310e9c678d269d7675c9cb952ec6..01a3d6aad12886096ab2833a7bc276467f08cb1a 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -138,6 +138,15 @@ static void cdns_host_exit(struct cdns *cdns)
 	cdns_drd_host_off(cdns);
 }
 
+static int cdns_host_resume(struct cdns *cdns, bool lost_power)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+	xhci->lost_power = lost_power;
+	return 0;
+}
+
 int cdns_host_init(struct cdns *cdns)
 {
 	struct cdns_role_driver *rdrv;
@@ -148,6 +157,7 @@ int cdns_host_init(struct cdns *cdns)
 
 	rdrv->start	= __cdns_host_init;
 	rdrv->stop	= cdns_host_exit;
+	rdrv->resume	= cdns_host_resume;
 	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
 	rdrv->name	= "host";
 

-- 
2.47.1

Re: [PATCH v6 0/5] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Peter Chen]

On 24-12-10 18:13:34, Théo Lebrun wrote:
> Currently, system-wide suspend is broken on J7200 because of a
> controller reset. The TI wrapper does not get re-initialised at resume
> and the first register access from cdns core fails.
> 
> We address that in two ways:
> 
>  - In the cdns3-ti wrapper, if a reset has occured at resume,
>    we reconfigure the hardware.
> 
>  - We add a xhci->lost_power flag. Identical to the XHCI_RESET_ON_RESUME
>    quirk, expect that it can be set at runtime.
> 
>    At resume, to summarise, we do:
>       xhci->lost_power = cdns_power_is_lost(cdns);

Is it possible you go to change xhci quirks runtime?

Peter
> 
> The previous revision merged both XHCI_RESET_ON_RESUME quirk and
> xhci->lost_power concepts into a single one (the quirk was the default
> value of the flag). Now, we separate those. It simplifies things
> because no additional compatible is required; we can detect everything
> at runtime.
> 
> Have a nice day,
> Théo
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
> Changes in v6:
> - Drop two upstreamed patches:
>   8e3dc6a51cca ("dt-bindings: usb: ti,j721e-usb: fix compatible list")
>   d7fad3c5c53e ("arm64: dts: ti: k3-am64: add USB fallback compatible to J721E")
> - dt-bindings: fix dt-schema syntax in compatible property.
> - Change the approach about xhci->lost_power and the
>   XHCI_RESET_ON_RESUME quirk. They are now separate and are checked
>   independently at resume. The quirk stays the same, the flag can be
>   detected at resume.
> - Drop many patches, now that we don't add a new compatible for J7200:
>   dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
>   usb: cdns3: add quirk to platform data for reset-on-resume
>   usb: cdns3-ti: grab auxdata from match data
>   usb: cdns3-ti: add J7200 support with reset-on-resume behavior
>   arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
> - Link to v5: https://lore.kernel.org/r/20240726-s2r-cdns-v5-0-8664bfb032ac@bootlin.com
> 
> Changes in v5:
> - dt-bindings: take Reviewed-by Rob and Conor for the first
>   patch: "dt-bindings: usb: ti,j721e-usb: fix compatible list".
> - cdns3-ti:
>   - We now do have HW init code inside cdns_ti_reset_and_init_hw().
>   - It gets called at probe unconditionally and from ->runtime_resume()
>     if a reset is detected (using the W1 register).
>   - Auxdata patches have been reworked now that there is default auxdata
>     since commit b50a2da03bd9 ("usb: cdns3-ti: Add workaround for
>     Errata i2409"). We now have a patch that moves auxdata to match
>     data: "usb: cdns3-ti: grab auxdata from match data".
> - cdns3/xhci: those are three new patches.
>   - First, we rename "hibernated" to "lost_power" in arguments to
>     the role ->resume() callbacks.
>   - Then we add the xhci->lost_power flag, and only have it always copy
>     the value from XHCI_RESET_ON_RESUME.
>   - Finally, we set the flag from the host role driver.
> - Link to v4: https://lore.kernel.org/lkml/20240307-j7200-usb-suspend-v4-0-5ec7615431f3@bootlin.com/
> 
> Changes in v4:
> - dt-bindings: usb: ti,j721e-usb:
>   - Remove ti,am64-usb single compatible entry.
>   - Reverse ordering of compatible pair j721e + am64
>     (becoming am64 + j721e).
>   - Add j7200 + j721e compatible pair (versus only j7200). It is the
>     same thing as am64: only the integration differs with base j721e
>     compatible.
>   - NOT taking trailers from Conor as patches changed substantially.
> - arm64: dts: ti: j3-j7200:
>   - Use j7200 + j721e compatible pair (versus only j7200 previously).
> - arm64: dts: ti: j3-am64:
>   - Fix to use am64 + j721e compatible pair (versus only am64).
>     This is a new patch.
> - Link to v3: https://lore.kernel.org/r/20240223-j7200-usb-suspend-v3-0-b41c9893a130@bootlin.com
> 
> Changes in v3:
> - dt-bindings: use an enum to list compatibles instead of the previous
>   odd construct. This is done in a separate patch from the one adding
>   J7200 compatible.
> - dt-bindings: dropped Acked-by Conor as the changes were modified a lot.
> - Add runtime PM back. Put the init sequence in ->runtime_resume(). It
>   gets called at probe for all compatibles and at resume for J7200.
> - Introduce a cdns_ti_match_data struct rather than rely on compatible
>   from code.
> - Reorder code changes. Add infrastructure based on match data THEN add
>   compatible and its match data.
> - DTSI: use only J7200 compatible rather than both J7200 then J721E.
> - Link to v2: https://lore.kernel.org/r/20231120-j7200-usb-suspend-v2-0-038c7e4a3df4@bootlin.com
> 
> Changes in v2:
> - Remove runtime PM from cdns3-ti; it brings nothing. That means our
>   cdns3-ti suspend/resume patch is simpler; there is no need to handle
>   runtime PM at suspend/resume.
> - Do not add cdns3 host role suspend/resume callbacks; they are not
>   needed as core detects reset on resume & calls cdns_drd_host_on when
>   needed.
> - cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value
>   computation.
> - cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it
>   is unneeded on our platform.
> - Link to v1: https://lore.kernel.org/r/20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com
> 
> ---
> Théo Lebrun (5):
>       usb: cdns3-ti: move reg writes to separate function
>       usb: cdns3-ti: run HW init at resume() if HW was reset
>       usb: cdns3: rename hibernated argument of role->resume() to lost_power
>       xhci: introduce xhci->lost_power flag
>       usb: cdns3: host: transmit lost_power signal from wrapper to XHCI
> 
>  drivers/usb/cdns3/cdns3-gadget.c |   4 +-
>  drivers/usb/cdns3/cdns3-ti.c     | 109 +++++++++++++++++++++++++--------------
>  drivers/usb/cdns3/cdnsp-gadget.c |   2 +-
>  drivers/usb/cdns3/core.h         |   2 +-
>  drivers/usb/cdns3/host.c         |  10 ++++
>  drivers/usb/host/xhci.c          |   3 +-
>  drivers/usb/host/xhci.h          |   6 +++
>  7 files changed, 93 insertions(+), 43 deletions(-)
> ---
> base-commit: d1869e31ecb0bb7397c6a04c29f281864e9257e3
> change-id: 20240726-s2r-cdns-4b180cd960ff
> 
> Best regards,
> -- 
> Théo Lebrun <theo.lebrun@bootlin.com>
> 

Re: [PATCH v6 0/5] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Théo Lebrun]

Hello Peter,

On Sat Dec 14, 2024 at 10:06 AM CET, Peter Chen wrote:
> On 24-12-10 18:13:34, Théo Lebrun wrote:
> > Currently, system-wide suspend is broken on J7200 because of a
> > controller reset. The TI wrapper does not get re-initialised at resume
> > and the first register access from cdns core fails.
> > 
> > We address that in two ways:
> > 
> >  - In the cdns3-ti wrapper, if a reset has occured at resume,
> >    we reconfigure the hardware.
> > 
> >  - We add a xhci->lost_power flag. Identical to the XHCI_RESET_ON_RESUME
> >    quirk, expect that it can be set at runtime.
> > 
> >    At resume, to summarise, we do:
> >       xhci->lost_power = cdns_power_is_lost(cdns);
>
> Is it possible you go to change xhci quirks runtime?

I always assumed quirks were read-only once probe was finished.
If I was wrong then we can remove xhci->lost_power and edit
xhci->quirks instead.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v6 0/5] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Peter Chen]

On 24-12-16 15:09:00, Théo Lebrun wrote:
> Hello Peter,
> 
> On Sat Dec 14, 2024 at 10:06 AM CET, Peter Chen wrote:
> > On 24-12-10 18:13:34, Théo Lebrun wrote:
> > > Currently, system-wide suspend is broken on J7200 because of a
> > > controller reset. The TI wrapper does not get re-initialised at resume
> > > and the first register access from cdns core fails.
> > > 
> > > We address that in two ways:
> > > 
> > >  - In the cdns3-ti wrapper, if a reset has occured at resume,
> > >    we reconfigure the hardware.
> > > 
> > >  - We add a xhci->lost_power flag. Identical to the XHCI_RESET_ON_RESUME
> > >    quirk, expect that it can be set at runtime.
> > > 
> > >    At resume, to summarise, we do:
> > >       xhci->lost_power = cdns_power_is_lost(cdns);
> >
> > Is it possible you go to change xhci quirks runtime?
> 
> I always assumed quirks were read-only once probe was finished.
> If I was wrong then we can remove xhci->lost_power and edit
> xhci->quirks instead.
> 

I just want to see if we could avoid change common code, would you
please confirm with xHCI maintainer Mathias Nyman?

Peter