[PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[PATCH v5 00/12] 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 a few ways:
 - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
 - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
   a resume.
 - We add a xhci->lost_power flag.

The previous revision had one big issue: we had to know if
reset-on-resume was true, at probe-time. This is where the main
difference with previous revisions is. We now pass the information from
wrapper devices back up into XHCI. The xhci->lost_power flag gets its
default value from the XHCI_RESET_ON_RESUME quirk. It however allows
wrappers to signal *at resume* if they still expect a reset.

That means wrappers that are unsure if they will reset should:
 - (1) set the quirk at probe and,
 - (2) potentially set xhci->lost_power to false at resume.

We implement that for cdns3, by piggybacking on the host role ->resume()
callback already receives the information from its caller.

Have a nice day,
Théo

Signed-off-by: Théo Lebrun <theo.lebrun@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 (12):
      dt-bindings: usb: ti,j721e-usb: fix compatible list
      dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
      usb: cdns3-ti: move reg writes to separate function
      usb: cdns3-ti: run HW init at resume() if HW was reset
      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
      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
      arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
      arm64: dts: ti: k3-am64: add USB fallback compatible to J721E

 .../devicetree/bindings/usb/ti,j721e-usb.yaml      |   5 +-
 arch/arm64/boot/dts/ti/k3-am64-main.dtsi           |   2 +-
 arch/arm64/boot/dts/ti/k3-j7200-main.dtsi          |   2 +-
 drivers/usb/cdns3/cdns3-gadget.c                   |   4 +-
 drivers/usb/cdns3/cdns3-ti.c                       | 151 ++++++++++++++-------
 drivers/usb/cdns3/cdnsp-gadget.c                   |   2 +-
 drivers/usb/cdns3/core.h                           |   3 +-
 drivers/usb/cdns3/host.c                           |  13 ++
 drivers/usb/host/xhci.c                            |   8 +-
 drivers/usb/host/xhci.h                            |   6 +
 10 files changed, 136 insertions(+), 60 deletions(-)
---
base-commit: c33ffdb70cc6df4105160f991288e7d2567d7ffa
change-id: 20240726-s2r-cdns-4b180cd960ff

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

[PATCH v5 01/12] dt-bindings: usb: ti,j721e-usb: fix compatible list

[Théo Lebrun]

Compatible can be A or B+A, not A or B or A+B. B got added afterwards,
we want B+A not A+B. A=ti,j721e-usb and B=ti,am64-usb.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..653a89586f4e 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -13,10 +13,9 @@ properties:
   compatible:
     oneOf:
       - const: ti,j721e-usb
-      - const: ti,am64-usb
       - items:
-          - const: ti,j721e-usb
           - const: ti,am64-usb
+          - const: ti,j721e-usb
 
   reg:
     maxItems: 1

-- 
2.45.2

[PATCH v5 02/12] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

On J7200, the controller & its wrapper are reset on resume. It has the
same behavior as ti,j721e-usb with a different SoC integration.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 653a89586f4e..038fdf173841 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -14,7 +14,9 @@ properties:
     oneOf:
       - const: ti,j721e-usb
       - items:
-          - const: ti,am64-usb
+          - enum:
+              - const: ti,am64-usb
+              - const: ti,j7200-usb
           - const: ti,j721e-usb
 
   reg:

-- 
2.45.2

[PATCH v5 03/12] 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 cfabc12ee0e3..5253829ea1d8 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.45.2

[PATCH v5 04/12] 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 5253829ea1d8..618fba5fa2bb 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.45.2

[PATCH v5 05/12] usb: cdns3: add quirk to platform data for reset-on-resume

[Théo Lebrun]

The cdns3 host role does not care about reset-on-resume. xHCI however
reconfigures itself in silence rather than printing a warning about a
resume error. Related warning example:

  [   16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit

Allow passing a CDNS3_RESET_ON_RESUME quirk flag from cdns3 pdata down
to xHCI pdata. The goal is to allow signaling about reset-on-resume
behavior from platform wrapper drivers.

When used, remote wakeup is not expected to work.

Acked-by: Peter Chen <peter.chen@kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/core.h | 1 +
 drivers/usb/cdns3/host.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 57d47348dc19..9207bd6365f7 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -45,6 +45,7 @@ struct cdns3_platform_data {
 	unsigned long quirks;
 #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
 #define CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE	BIT(1)
+#define CDNS3_RESET_ON_RESUME			BIT(2)
 };
 
 /**
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index ceca4d839dfd..d2cb529630e4 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -103,6 +103,9 @@ static int __cdns_host_init(struct cdns *cdns)
 	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
 		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
+	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_RESET_ON_RESUME))
+		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME;
+
 	ret = platform_device_add_data(xhci, cdns->xhci_plat_data,
 			sizeof(struct xhci_plat_priv));
 	if (ret)

-- 
2.45.2

[PATCH v5 06/12] usb: cdns3-ti: grab auxdata from match data

[Théo Lebrun]

Current code uses the global `cdns_ti_auxdata` variable as auxiliary
data passed to of_platform_populate(). Use match data to store a
pointer to auxdata.

Current behavior is not changed; it allows future compatibles to provide
different auxiliary data.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 618fba5fa2bb..159814dfc856 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -87,18 +87,6 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
 	writel(value, data->usbss + offset);
 }
 
-static struct cdns3_platform_data cdns_ti_pdata = {
-	.quirks = CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE,   /* Errata i2409 */
-};
-
-static const struct of_dev_auxdata cdns_ti_auxdata[] = {
-	{
-		.compatible = "cdns,usb3",
-		.platform_data = &cdns_ti_pdata,
-	},
-	{},
-};
-
 static void cdns_ti_reset_and_init_hw(struct cdns_ti *data)
 {
 	u32 reg;
@@ -142,6 +130,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
+	const struct of_dev_auxdata *auxdata;
 	struct cdns_ti *data;
 	unsigned long rate;
 	int error, i;
@@ -203,7 +192,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	error = of_platform_populate(node, NULL, cdns_ti_auxdata, dev);
+	auxdata = device_get_match_data(dev);
+	error = of_platform_populate(node, NULL, auxdata, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);
 		goto err;
@@ -256,9 +246,21 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
 	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 };
 
+static struct cdns3_platform_data cdns_ti_pdata = {
+	.quirks = CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE,   /* Errata i2409 */
+};
+
+static const struct of_dev_auxdata cdns_ti_auxdata[] = {
+	{
+		.compatible = "cdns,usb3",
+		.platform_data = &cdns_ti_pdata,
+	},
+	{},
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
-	{ .compatible = "ti,j721e-usb", },
-	{ .compatible = "ti,am64-usb", },
+	{ .compatible = "ti,j721e-usb", .data = cdns_ti_auxdata },
+	{ .compatible = "ti,am64-usb", .data = cdns_ti_auxdata },
 	{},
 };
 MODULE_DEVICE_TABLE(of, cdns_ti_of_match);

-- 
2.45.2

[PATCH v5 07/12] usb: cdns3-ti: add J7200 support with reset-on-resume behavior

[Théo Lebrun]

Add ti,j7200-usb compatible. Match data indicates the controller resets
on resume which tells that to the cdns3 core. This in turn silences a
xHCI warning visible in cases of unexpected resets.

We also inherit the errata quirk CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE from
the default `cdns_ti_auxdata` configuration.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 159814dfc856..65b8b6f4c654 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -258,7 +258,21 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
 	{},
 };
 
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+	.quirks = CDNS3_RESET_ON_RESUME |
+		  CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE,   /* Errata i2409 */
+};
+
+static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = {
+	{
+		.compatible = "cdns,usb3",
+		.platform_data = &cdns_ti_j7200_pdata,
+	},
+	{},
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
+	{ .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata },
 	{ .compatible = "ti,j721e-usb", .data = cdns_ti_auxdata },
 	{ .compatible = "ti,am64-usb", .data = cdns_ti_auxdata },
 	{},

-- 
2.45.2

[PATCH v5 08/12] 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 9207bd6365f7..f385d3980545 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.45.2

[PATCH v5 09/12] 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 the XHCI_RESET_ON_RESUME value.

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.

Reset the xhci->lost_power value each time in xhci_resume(), making it
safe for devices to only set lost_power on some resumes.

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

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0a8cf6c17f82..2c9b32d339f9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend)
 		reinit_xhc = true;
 
+	/* Reset to default value, parent devices might correct it at next resume. */
+	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
+
 	if (!reinit_xhc) {
 		/*
 		 * Some controllers might lose power during suspend, so wait
@@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 	if (get_quirks)
 		get_quirks(dev, xhci);
 
+	/* Default value, that can be corrected at resume. */
+	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
+
 	/* In xhci controllers which follow xhci 1.0 spec gives a spurious
 	 * success event after a short transfer. This quirk will ignore such
 	 * spurious event.
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ebd0afd59a60..ec7c6061363f 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1640,6 +1640,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.45.2

[PATCH v5 10/12] 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. We therefore override its
default value that is based on XHCI_RESET_ON_RESUME.

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 d2cb529630e4..edef39ab8d15 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -139,6 +139,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;
@@ -149,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.45.2

[PATCH v5 11/12] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Théo Lebrun]

On our platform, suspend-to-idle or suspend-to-RAM turn the controller
off. This compatible triggers reset-on-resume behavior to reconfigure
the hardware.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
index 9386bf3ef9f6..6b8e8672b386 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
@@ -766,7 +766,7 @@ pcie1_rc: pcie@2910000 {
 	};
 
 	usbss0: cdns-usb@4104000 {
-		compatible = "ti,j721e-usb";
+		compatible = "ti,j7200-usb", "ti,j721e-usb";
 		reg = <0x00 0x4104000 0x00 0x100>;
 		dma-coherent;
 		power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;

-- 
2.45.2

[PATCH v5 12/12] arm64: dts: ti: k3-am64: add USB fallback compatible to J721E

[Théo Lebrun]

USB on AM64 is the same peripheral as on J721E. It has a specific
compatible for potential integration details. Express this
relationship, matching what the dt-bindings indicate.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
index f8370dd03350..02c8ed93730f 100644
--- a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
@@ -759,7 +759,7 @@ timesync_router: pinctrl@a40000 {
 	};
 
 	usbss0: cdns-usb@f900000 {
-		compatible = "ti,am64-usb";
+		compatible = "ti,am64-usb", "ti,j721e-usb";
 		reg = <0x00 0xf900000 0x00 0x100>;
 		power-domains = <&k3_pds 161 TI_SCI_PD_EXCLUSIVE>;
 		clocks = <&k3_clks 161 9>, <&k3_clks 161 1>;

-- 
2.45.2

Re: [PATCH v5 02/12] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Rob Herring (Arm)]

On Fri, 26 Jul 2024 20:17:50 +0200, Théo Lebrun wrote:
> On J7200, the controller & its wrapper are reset on resume. It has the
> same behavior as ti,j721e-usb with a different SoC integration.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml: properties:compatible:oneOf:1:items: 'oneOf' conditional failed, one must be fixed:
	[{'enum': [{'const': 'ti,am64-usb'}, {'const': 'ti,j7200-usb'}]}, {'const': 'ti,j721e-usb'}] is not of type 'object'
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml: properties:compatible:oneOf:1:items:0:enum: 'oneOf' conditional failed, one must be fixed:
		{'const': 'ti,am64-usb'} is not of type 'integer'
		{'const': 'ti,am64-usb'} is not of type 'string'
		{'const': 'ti,j7200-usb'} is not of type 'integer'
		{'const': 'ti,j7200-usb'} is not of type 'string'
		hint: "enum" must be an array of either integers or strings
		from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml: properties:compatible:oneOf:1:items: 'oneOf' conditional failed, one must be fixed:
	[{'enum': [{'const': 'ti,am64-usb'}, {'const': 'ti,j7200-usb'}]}, {'const': 'ti,j721e-usb'}] is not of type 'object'
	{'const': 'ti,am64-usb'} is not of type 'string'
	{'const': 'ti,j7200-usb'} is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240726-s2r-cdns-v5-2-8664bfb032ac@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Roger Quadros]

Hi,

On 26/07/2024 21:17, 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 a few ways:
>  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
>  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
>    a resume.

OK.
>  - We add a xhci->lost_power flag.

Why?

> 
> The previous revision had one big issue: we had to know if
> reset-on-resume was true, at probe-time. This is where the main

Don't we already know this at probe-time? why not just rely on the existing
XHCI_RESET_ON_RESUME qurik, than add a new mechanism?

> difference with previous revisions is. We now pass the information from
> wrapper devices back up into XHCI. The xhci->lost_power flag gets its
> default value from the XHCI_RESET_ON_RESUME quirk. It however allows
> wrappers to signal *at resume* if they still expect a reset.
> 
> That means wrappers that are unsure if they will reset should:
>  - (1) set the quirk at probe and,
>  - (2) potentially set xhci->lost_power to false at resume.
> 
> We implement that for cdns3, by piggybacking on the host role ->resume()
> callback already receives the information from its caller.
> 
> Have a nice day,
> Théo
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@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 (12):
>       dt-bindings: usb: ti,j721e-usb: fix compatible list
>       dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
>       usb: cdns3-ti: move reg writes to separate function
>       usb: cdns3-ti: run HW init at resume() if HW was reset
>       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
>       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
>       arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
>       arm64: dts: ti: k3-am64: add USB fallback compatible to J721E
> 
>  .../devicetree/bindings/usb/ti,j721e-usb.yaml      |   5 +-
>  arch/arm64/boot/dts/ti/k3-am64-main.dtsi           |   2 +-
>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi          |   2 +-
>  drivers/usb/cdns3/cdns3-gadget.c                   |   4 +-
>  drivers/usb/cdns3/cdns3-ti.c                       | 151 ++++++++++++++-------
>  drivers/usb/cdns3/cdnsp-gadget.c                   |   2 +-
>  drivers/usb/cdns3/core.h                           |   3 +-
>  drivers/usb/cdns3/host.c                           |  13 ++
>  drivers/usb/host/xhci.c                            |   8 +-
>  drivers/usb/host/xhci.h                            |   6 +
>  10 files changed, 136 insertions(+), 60 deletions(-)
> ---
> base-commit: c33ffdb70cc6df4105160f991288e7d2567d7ffa
> change-id: 20240726-s2r-cdns-4b180cd960ff
> 
> Best regards,

-- 
cheers,
-roger

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Théo Lebrun]

Hello Roger,

On Sat Aug 3, 2024 at 5:14 PM CEST, Roger Quadros wrote:
> On 26/07/2024 21:17, 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 a few ways:
> >  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
> >  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
> >    a resume.
>
> OK.
> >  - We add a xhci->lost_power flag.
>
> Why?
>
> > 
> > The previous revision had one big issue: we had to know if
> > reset-on-resume was true, at probe-time. This is where the main
>
> Don't we already know this at probe-time? why not just rely on the existing
> XHCI_RESET_ON_RESUME qurik, than add a new mechanism?

Some TI platforms cannot tell, before going to suspend, if their USB
controller will reset. Suspend behavior is defined by (at least) two
features:

 - Power domains. See arch/arm64/boot/dts/ti/k3-j7200-main.dtsi:

   usbss0: cdns-usb@4104000 {
      compatible = "ti,j7200-usb", "ti,j721e-usb";
      // ...
      power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
      // ...
   };

   This `power-domains` property implies that even s2idle will reset
   the controller.

 - Deep suspend. The type of suspend defines what will happen to various
   controllers. Currently deep suspend is suspend-to-RAM, with the SoC
   being turned off.

   This might evolve over time with more complex rules: the chosen
   suspend behavior could depend on wakeup source and/or wakeup target
   latencies. That information might not be available to Linux, being
   decided upon by firmwares. We need to be able to resume successfully
   without being surprised by a reset.

I am sorry the precise usecase wasn't clear from the get-go.

Thanks,

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

Re: [PATCH v5 01/12] dt-bindings: usb: ti,j721e-usb: fix compatible list

[Roger Quadros]

On 26/07/2024 21:17, Théo Lebrun wrote:
> Compatible can be A or B+A, not A or B or A+B. B got added afterwards,
> we want B+A not A+B. A=ti,j721e-usb and B=ti,am64-usb.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

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

Re: [PATCH v5 09/12] xhci: introduce xhci->lost_power flag

[Roger Quadros]

On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value.
> 
> 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.
> 
> Reset the xhci->lost_power value each time in xhci_resume(), making it
> safe for devices to only set lost_power on some resumes.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/host/xhci.c | 8 +++++++-
>  drivers/usb/host/xhci.h | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0a8cf6c17f82..2c9b32d339f9 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend)

Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently?

XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume.

xhci->lost_power is used by devices that don't have consistent behavior.


>  		reinit_xhc = true;
>  
> +	/* Reset to default value, parent devices might correct it at next resume. */
> +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> +

then you don't need to do this.

>  	if (!reinit_xhc) {
>  		/*
>  		 * Some controllers might lose power during suspend, so wait
> @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  	if (get_quirks)
>  		get_quirks(dev, xhci);
>  
> +	/* Default value, that can be corrected at resume. */
> +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> + 

nor this.

>  	/* In xhci controllers which follow xhci 1.0 spec gives a spurious
>  	 * success event after a short transfer. This quirk will ignore such
>  	 * spurious event.
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index ebd0afd59a60..ec7c6061363f 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1640,6 +1640,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;
> 

-- 
cheers,
-roger

Re: [PATCH v5 05/12] usb: cdns3: add quirk to platform data for reset-on-resume

[Roger Quadros]

On 26/07/2024 21:17, Théo Lebrun wrote:
> The cdns3 host role does not care about reset-on-resume. xHCI however
> reconfigures itself in silence rather than printing a warning about a
> resume error. Related warning example:
> 
>   [   16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
> 
> Allow passing a CDNS3_RESET_ON_RESUME quirk flag from cdns3 pdata down
> to xHCI pdata. The goal is to allow signaling about reset-on-resume
> behavior from platform wrapper drivers.
> 
> When used, remote wakeup is not expected to work.
> 
> Acked-by: Peter Chen <peter.chen@kernel.org>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

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

Re: [PATCH v5 06/12] usb: cdns3-ti: grab auxdata from match data

[Roger Quadros]

On 26/07/2024 21:17, Théo Lebrun wrote:
> Current code uses the global `cdns_ti_auxdata` variable as auxiliary
> data passed to of_platform_populate(). Use match data to store a
> pointer to auxdata.
> 
> Current behavior is not changed; it allows future compatibles to provide
> different auxiliary data.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

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

Re: [PATCH v5 07/12] usb: cdns3-ti: add J7200 support with reset-on-resume behavior

[Roger Quadros]

On 26/07/2024 21:17, Théo Lebrun wrote:
> Add ti,j7200-usb compatible. Match data indicates the controller resets
> on resume which tells that to the cdns3 core. This in turn silences a
> xHCI warning visible in cases of unexpected resets.
> 
> We also inherit the errata quirk CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE from
> the default `cdns_ti_auxdata` configuration.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 159814dfc856..65b8b6f4c654 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -258,7 +258,21 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
>  	{},
>  };
>  
> +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> +	.quirks = CDNS3_RESET_ON_RESUME |

But you mentioned that the behavior can be different based on which
idle state the system went into.
Setting this flag will means Reset is required on every resume.


Instead, you just need to rely on the runtime check and set the xhci->lost_power flag at resume.


> +		  CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE,   /* Errata i2409 */
> +};
> +
> +static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = {
> +	{
> +		.compatible = "cdns,usb3",
> +		.platform_data = &cdns_ti_j7200_pdata,
> +	},
> +	{},
> +};
> +
>  static const struct of_device_id cdns_ti_of_match[] = {
> +	{ .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata },
>  	{ .compatible = "ti,j721e-usb", .data = cdns_ti_auxdata },
>  	{ .compatible = "ti,am64-usb", .data = cdns_ti_auxdata },
>  	{},
> 

-- 
cheers,
-roger

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Roger Quadros]

+Vibhore & Vishal

On 05/08/2024 11:58, Théo Lebrun wrote:
> Hello Roger,
> 
> On Sat Aug 3, 2024 at 5:14 PM CEST, Roger Quadros wrote:
>> On 26/07/2024 21:17, 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 a few ways:
>>>  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
>>>  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
>>>    a resume.
>>
>> OK.
>>>  - We add a xhci->lost_power flag.
>>
>> Why?
>>
>>>
>>> The previous revision had one big issue: we had to know if
>>> reset-on-resume was true, at probe-time. This is where the main
>>
>> Don't we already know this at probe-time? why not just rely on the existing
>> XHCI_RESET_ON_RESUME qurik, than add a new mechanism?
> 
> Some TI platforms cannot tell, before going to suspend, if their USB
> controller will reset. Suspend behavior is defined by (at least) two
> features:
> 
>  - Power domains. See arch/arm64/boot/dts/ti/k3-j7200-main.dtsi:
> 
>    usbss0: cdns-usb@4104000 {
>       compatible = "ti,j7200-usb", "ti,j721e-usb";
>       // ...
>       power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
>       // ...
>    };
> 
>    This `power-domains` property implies that even s2idle will reset
>    the controller.

I'm not so sure. All K3 platforms have the power-domains property for
the USB wrapper nodes.

> 
>  - Deep suspend. The type of suspend defines what will happen to various
>    controllers. Currently deep suspend is suspend-to-RAM, with the SoC
>    being turned off.
> 
>    This might evolve over time with more complex rules: the chosen
>    suspend behavior could depend on wakeup source and/or wakeup target
>    latencies. That information might not be available to Linux, being
>    decided upon by firmwares. We need to be able to resume successfully
>    without being surprised by a reset.
> 

Got it. Might be worth to mention this in the patch description.

> I am sorry the precise usecase wasn't clear from the get-go.
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
cheers,
-roger

Re: [PATCH v5 11/12] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Roger Quadros]

On 26/07/2024 21:17, Théo Lebrun wrote:
> On our platform, suspend-to-idle or suspend-to-RAM turn the controller
> off. This compatible triggers reset-on-resume behavior to reconfigure
> the hardware.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH v5 12/12] arm64: dts: ti: k3-am64: add USB fallback compatible to J721E

[Roger Quadros]

On 26/07/2024 21:18, Théo Lebrun wrote:
> USB on AM64 is the same peripheral as on J721E. It has a specific
> compatible for potential integration details. Express this
> relationship, matching what the dt-bindings indicate.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

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

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

[...]

> Some TI platforms cannot tell, before going to suspend, if their USB
> controller will reset. Suspend behavior is defined by (at least) two
> features:
>
>  - Power domains. See arch/arm64/boot/dts/ti/k3-j7200-main.dtsi:
>
>    usbss0: cdns-usb@4104000 {
>       compatible = "ti,j7200-usb", "ti,j721e-usb";
>       // ...
>       power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
>       // ...
>    };
>
>    This `power-domains` property implies that even s2idle will reset
>    the controller.

minor but important clarification: not *will* reset, but *might* reset the controller.

Kevin

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Peter Chen]

On 24-07-26 20:17:48, 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 a few ways:
>  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
>  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
>    a resume.
>  - We add a xhci->lost_power flag.
> 
> The previous revision had one big issue: we had to know if
> reset-on-resume was true, at probe-time. This is where the main
> difference with previous revisions is. We now pass the information from
> wrapper devices back up into XHCI. The xhci->lost_power flag gets its
> default value from the XHCI_RESET_ON_RESUME quirk. It however allows
> wrappers to signal *at resume* if they still expect a reset.
> 
> That means wrappers that are unsure if they will reset should:
>  - (1) set the quirk at probe and,
>  - (2) potentially set xhci->lost_power to false at resume.

Judge if controller is power lost has implemented at cdns_power_is_lost
Please check if you could use that.

Peter

> 
> We implement that for cdns3, by piggybacking on the host role ->resume()
> callback already receives the information from its caller.
> 
> Have a nice day,
> Théo
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@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 (12):
>       dt-bindings: usb: ti,j721e-usb: fix compatible list
>       dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
>       usb: cdns3-ti: move reg writes to separate function
>       usb: cdns3-ti: run HW init at resume() if HW was reset
>       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
>       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
>       arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
>       arm64: dts: ti: k3-am64: add USB fallback compatible to J721E
> 
>  .../devicetree/bindings/usb/ti,j721e-usb.yaml      |   5 +-
>  arch/arm64/boot/dts/ti/k3-am64-main.dtsi           |   2 +-
>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi          |   2 +-
>  drivers/usb/cdns3/cdns3-gadget.c                   |   4 +-
>  drivers/usb/cdns3/cdns3-ti.c                       | 151 ++++++++++++++-------
>  drivers/usb/cdns3/cdnsp-gadget.c                   |   2 +-
>  drivers/usb/cdns3/core.h                           |   3 +-
>  drivers/usb/cdns3/host.c                           |  13 ++
>  drivers/usb/host/xhci.c                            |   8 +-
>  drivers/usb/host/xhci.h                            |   6 +
>  10 files changed, 136 insertions(+), 60 deletions(-)
> ---
> base-commit: c33ffdb70cc6df4105160f991288e7d2567d7ffa
> change-id: 20240726-s2r-cdns-4b180cd960ff
> 
> Best regards,
> -- 
> Théo Lebrun <theo.lebrun@bootlin.com>
> 

Re: (subset) [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Nishanth Menon]

Hi Théo Lebrun,

On Fri, 26 Jul 2024 20:17:48 +0200, 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 a few ways:
>  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
>  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
>    a resume.
>  - We add a xhci->lost_power flag.
> 
> [...]

Since Greg has picked f7fd939e805672417bbf418f6035dec9400230fd ("dt-bindings:
usb: ti,j721e-usb: fix compatible list"), the corresponding
patch needs to go via the soc dt tree, so picking just that.

I have applied the following to branch ti-k3-dts-next on [1].
Thank you!

[12/12] arm64: dts: ti: k3-am64: add USB fallback compatible to J721E
        commit: 99ced42d6f3ebcae52c2c6d1207d3f96d7cf88ac

Theo: Do let me know if Greg decides to drop the said patch, and I will drop
this off my PR as well. But, no action at the moment for this.

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent up the chain during
the next merge window (or sooner if it is a relevant bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ti/linux.git
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v5 02/12] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

On Fri Jul 26, 2024 at 9:26 PM CEST, Rob Herring (Arm) wrote:
> On Fri, 26 Jul 2024 20:17:50 +0200, Théo Lebrun wrote:
> > On J7200, the controller & its wrapper are reset on resume. It has the
> > same behavior as ti,j721e-usb with a different SoC integration.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
>
> My bot found errors running 'make dt_binding_check' on your patch:

Clearly this patch was wrong.
Past me trusted future me to verify and future me trusted past me.
Sorry!

For reference, new patch content will look like below.
This doesn't trigger a warning on:

    make dt_binding_check DT_SCHEMA_FILES=ti,j721e-usb

------------------------------------------------------------------------

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 653a89586f4e..d14c18b64086 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -14,7 +14,9 @@ properties:
     oneOf:
       - const: ti,j721e-usb
       - items:
-          - const: ti,am64-usb
+          - enum:
+              - ti,am64-usb
+              - ti,j7200-usb
           - const: ti,j721e-usb

   reg:

------------------------------------------------------------------------

Regards,

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

Re: [PATCH v5 09/12] xhci: introduce xhci->lost_power flag

[Théo Lebrun]

On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote:
> On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value.
> > 
> > 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.
> > 
> > Reset the xhci->lost_power value each time in xhci_resume(), making it
> > safe for devices to only set lost_power on some resumes.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/host/xhci.c | 8 +++++++-
> >  drivers/usb/host/xhci.h | 6 ++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 0a8cf6c17f82..2c9b32d339f9 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend)
>
> Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently?
>
> XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume.
>
> xhci->lost_power is used by devices that don't have consistent behavior.

The goal is to avoid almost-duplicate functionality. I feel like:

    XHCI_RESET_ON_RESUME is the default value of xhci->lost_power,
    which might be modified at resume.

Is a more straight forward solution than:

    Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was
    lost at resume. First must be statically known, second can be
    updated during runtime. If second is used, first one must NOT be
    set.

Indeed, the first solution brings two additional lines of code as you
commented below. I'd argue the easier-to-wrap-your-head-around logic is
more important.

Tell me if you are convinced the second approach is better.

>
>
> >  		reinit_xhc = true;
> >  
> > +	/* Reset to default value, parent devices might correct it at next resume. */
> > +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> > +
>
> then you don't need to do this.

To be honest, I added this line out of rigor. We could remove it and say
that any device that modifies xhci->lost_power once at resume must set
it at each later resume.

The above line felt like a small safety net to avoid logic mistakes.

>
> >  	if (!reinit_xhc) {
> >  		/*
> >  		 * Some controllers might lose power during suspend, so wait
> > @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
> >  	if (get_quirks)
> >  		get_quirks(dev, xhci);
> >  
> > +	/* Default value, that can be corrected at resume. */
> > +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
> > + 
>
> nor this.

Regards,

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

Re: [PATCH v5 07/12] usb: cdns3-ti: add J7200 support with reset-on-resume behavior

[Théo Lebrun]

On Mon Aug 5, 2024 at 3:54 PM CEST, Roger Quadros wrote:
> On 26/07/2024 21:17, Théo Lebrun wrote:
> > Add ti,j7200-usb compatible. Match data indicates the controller resets
> > on resume which tells that to the cdns3 core. This in turn silences a
> > xHCI warning visible in cases of unexpected resets.
> > 
> > We also inherit the errata quirk CDNS3_DRD_SUSPEND_RESIDENCY_ENABLE from
> > the default `cdns_ti_auxdata` configuration.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 159814dfc856..65b8b6f4c654 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -258,7 +258,21 @@ static const struct of_dev_auxdata cdns_ti_auxdata[] = {
> >  	{},
> >  };
> >  
> > +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> > +	.quirks = CDNS3_RESET_ON_RESUME |
>
> But you mentioned that the behavior can be different based on which
> idle state the system went into.
> Setting this flag will means Reset is required on every resume.

No, this flag's only behavior is to enable XHCI_RESET_ON_RESUME in the
lower xHCI stack. Code is in __cdns_host_init():

	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_RESET_ON_RESUME))
		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME;

> Instead, you just need to rely on the runtime check and set the xhci->lost_power flag at resume.

I argued about my view of the XHCI_RESET_ON_RESUME quirk under
[PATCH v5 09/12].
https://lore.kernel.org/lkml/D42NIH63EHZG.KKWZR2WZB68L@bootlin.com/

Regards,

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

Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci)

[Théo Lebrun]

Hello Peter,

On Fri Aug 9, 2024 at 3:19 AM CEST, Peter Chen wrote:
> On 24-07-26 20:17:48, 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 a few ways:
> >  - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW.
> >  - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects
> >    a resume.
> >  - We add a xhci->lost_power flag.
> > 
> > The previous revision had one big issue: we had to know if
> > reset-on-resume was true, at probe-time. This is where the main
> > difference with previous revisions is. We now pass the information from
> > wrapper devices back up into XHCI. The xhci->lost_power flag gets its
> > default value from the XHCI_RESET_ON_RESUME quirk. It however allows
> > wrappers to signal *at resume* if they still expect a reset.
> > 
> > That means wrappers that are unsure if they will reset should:
> >  - (1) set the quirk at probe and,
> >  - (2) potentially set xhci->lost_power to false at resume.
>
> Judge if controller is power lost has implemented at cdns_power_is_lost
> Please check if you could use that.

That function is being used! Its return value is passed as second
argument to the resume() callback in struct cdns_role_driver. We set
xhci->lost_power using that exact value.

My cover letter explanation was slightly off, as it is not wrappers that
set xhci->lost_power, but instead role drivers. Wrappers don't have any
reason to touch the xhci struct directly, they are one layer above.

Related: [PATCH v5 08/15] commit message looks like this:

------------------------------------------------------------------------

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`.

------------------------------------------------------------------------

Regards,

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

Re: [PATCH v5 09/12] xhci: introduce xhci->lost_power flag

[Roger Quadros]

Hello Théo,

On 10/09/2024 16:50, Théo Lebrun wrote:
> On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote:
>> On 26/07/2024 21:17, 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 the XHCI_RESET_ON_RESUME value.
>>>
>>> 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.
>>>
>>> Reset the xhci->lost_power value each time in xhci_resume(), making it
>>> safe for devices to only set lost_power on some resumes.
>>>
>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>> ---
>>>  drivers/usb/host/xhci.c | 8 +++++++-
>>>  drivers/usb/host/xhci.h | 6 ++++++
>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 0a8cf6c17f82..2c9b32d339f9 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -1029,9 +1029,12 @@ 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->lost_power || xhci->broken_suspend)
>>
>> Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME independently?
>>
>> XHCI_RESET_ON_RESUME is sued by devices that know they always need to be reset on resume.
>>
>> xhci->lost_power is used by devices that don't have consistent behavior.
> 
> The goal is to avoid almost-duplicate functionality. I feel like:
> 
>     XHCI_RESET_ON_RESUME is the default value of xhci->lost_power,
>     which might be modified at resume.
> 
> Is a more straight forward solution than:
> 
>     Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was
>     lost at resume. First must be statically known, second can be
>     updated during runtime. If second is used, first one must NOT be
>     set.
> 
> Indeed, the first solution brings two additional lines of code as you
> commented below. I'd argue the easier-to-wrap-your-head-around logic is
> more important.
> 
> Tell me if you are convinced the second approach is better.
> 

I would still vote to keep logic tied to separate flags.

so XHCI_RESET_ON_RESUME to always resume on RESET
xhci->lost_power, reset based on runtime checks.

Which implies that platforms using xhci->lost_power should not
set XHCI_RESET_ON_RESUME.

But XHCI maintainers should give their opinion on this.

>>
>>
>>>  		reinit_xhc = true;
>>>  
>>> +	/* Reset to default value, parent devices might correct it at next resume. */
>>> +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
>>> +
>>
>> then you don't need to do this.
> 
> To be honest, I added this line out of rigor. We could remove it and say
> that any device that modifies xhci->lost_power once at resume must set
> it at each later resume.
> 
> The above line felt like a small safety net to avoid logic mistakes.
> 
>>
>>>  	if (!reinit_xhc) {
>>>  		/*
>>>  		 * Some controllers might lose power during suspend, so wait
>>> @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>>>  	if (get_quirks)
>>>  		get_quirks(dev, xhci);
>>>  
>>> +	/* Default value, that can be corrected at resume. */
>>> +	xhci->lost_power = !!(xhci->quirks & XHCI_RESET_ON_RESUME);
>>> + 
>>
>> nor this.
> 
> Regards,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
cheers,
-roger