[PATCH v4 0/9] usb: cdns: fix suspend on J7200 by assuming reset-on-resume

[PATCH v4 0/9] usb: cdns: fix suspend on J7200 by assuming reset-on-resume

[Théo Lebrun]

Hi,

Here is a new revision of the J7200 USB suspend fix. It is currently
broken on the platform, leading to a kernel panic at resume. Patches
are tested on a J7200 evaluation board, both s2idle and suspend-to-RAM.

This revision only changes dt-bindings and DTS stuff. We fix the current
situation (as discussed previously [0]) and add our J7200 compatible.

Have a nice day,
Théo

[0]: https://lore.kernel.org/lkml/20240223-j7200-usb-suspend-v3-1-b41c9893a130@bootlin.com/

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

Signed-off-by: Théo Lebrun <theo.lebrun@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 (9):
      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 from probe into ->runtime_resume()
      usb: cdns3-ti: support reset-on-resume behavior
      usb: cdns3-ti: pass auxdata from match data to of_platform_populate()
      usb: cdns3: add quirk to platform data for reset-on-resume
      usb: cdns3-ti: add J7200 support with reset-on-resume behavior
      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      |   6 +-
 arch/arm64/boot/dts/ti/k3-am64-main.dtsi           |   2 +-
 arch/arm64/boot/dts/ti/k3-j7200-main.dtsi          |   2 +-
 drivers/usb/cdns3/cdns3-ti.c                       | 125 ++++++++++++++++-----
 drivers/usb/cdns3/core.h                           |   1 +
 drivers/usb/cdns3/host.c                           |   3 +
 6 files changed, 105 insertions(+), 34 deletions(-)
---
base-commit: 1871c27e3539e5b812d50ec6ccad7567ec5414f2
change-id: 20231113-j7200-usb-suspend-2a47f2281e04

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/9] 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.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/9] dt-bindings: usb: ti,j721e-usb: fix compatible list

[Rob Herring]

On Thu, 07 Mar 2024 10:55:02 +0100, 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.
> 
> 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(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/9] dt-bindings: usb: ti,j721e-usb: fix compatible list

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 338 bytes --]

On Thu, Mar 07, 2024 at 10:55:02AM +0100, 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.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for the update.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/9] 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 653a89586f4e..e8f7e7511483 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -16,6 +16,9 @@ properties:
       - items:
           - const: ti,am64-usb
           - const: ti,j721e-usb
+      - items:
+          - const: ti,j7200-usb
+          - const: ti,j721e-usb
 
   reg:
     maxItems: 1

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/9] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Rob Herring]

On Thu, Mar 07, 2024 at 10:55:03AM +0100, 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 653a89586f4e..e8f7e7511483 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -16,6 +16,9 @@ properties:
>        - items:
>            - const: ti,am64-usb
>            - const: ti,j721e-usb
> +      - items:
> +          - const: ti,j7200-usb
> +          - const: ti,j721e-usb

Combine this with the previous entry:

items:
  - enum:
      - ti,am64-usb
      - ti,j7200-usb
  - const: ti,j721e-usb


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/9] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

Hello Rob,

On Thu Mar 7, 2024 at 3:21 PM CET, Rob Herring wrote:
> On Thu, Mar 07, 2024 at 10:55:03AM +0100, 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 | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > index 653a89586f4e..e8f7e7511483 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > @@ -16,6 +16,9 @@ properties:
> >        - items:
> >            - const: ti,am64-usb
> >            - const: ti,j721e-usb
> > +      - items:
> > +          - const: ti,j7200-usb
> > +          - const: ti,j721e-usb
>
> Combine this with the previous entry:
>
> items:
>   - enum:
>       - ti,am64-usb
>       - ti,j7200-usb
>   - const: ti,j721e-usb

Makes sense, will do. Full block will become:

	properties:
	  compatible:
	    oneOf:
	      - const: ti,j721e-usb
	      - items:
	          - enum:
	              - const: ti,am64-usb
	              - const: ti,j7200-usb
	          - const: ti,j721e-usb

Thanks,

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/9] usb: cdns3-ti: move reg writes from probe into ->runtime_resume()

[Théo Lebrun]

The hardware initialisation register write sequence is only used at
probe. Move it from being done at explicitely at probe to being done
implicitely by pm_runtime_get_sync() that calls ->runtime_resume().

Keep devicetree parsing in probe and add a new field in the private
struct to remember the USB2 refclk rate code computation result.

This opens the door to having the init sequence being executed later
down the road, at system-wide resume for example. This is NOT currently
happening because runtime PM is disabled at suspend without the
refcount being affected.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..4c8a557e6a6f 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -57,6 +57,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 */
@@ -90,10 +91,8 @@ 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)
@@ -133,7 +132,9 @@ 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");
 
 	pm_runtime_enable(dev);
 	error = pm_runtime_get_sync(dev);
@@ -142,40 +143,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, NULL, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);
@@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+static int cdns_ti_runtime_resume(struct device *dev)
+{
+	struct cdns_ti *data = dev_get_drvdata(dev);
+	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);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
@@ -224,6 +237,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.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/9] usb: cdns3-ti: move reg writes from probe into ->runtime_resume()

[Roger Quadros]

Hi,

On 07/03/2024 11:55, Théo Lebrun wrote:
> The hardware initialisation register write sequence is only used at
> probe. Move it from being done at explicitely at probe to being done
> implicitely by pm_runtime_get_sync() that calls ->runtime_resume().

explicitly / implicitly

> 
> Keep devicetree parsing in probe and add a new field in the private
> struct to remember the USB2 refclk rate code computation result.
> 
> This opens the door to having the init sequence being executed later
> down the road, at system-wide resume for example. This is NOT currently
> happening because runtime PM is disabled at suspend without the
> refcount being affected.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++-------------------
>  1 file changed, 52 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 5945c4b1e11f..4c8a557e6a6f 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -57,6 +57,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 */
> @@ -90,10 +91,8 @@ 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)
> @@ -133,7 +132,9 @@ 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");
>  
>  	pm_runtime_enable(dev);
>  	error = pm_runtime_get_sync(dev);
> @@ -142,40 +143,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, NULL, dev);
>  	if (error) {
>  		dev_err(dev, "failed to create children: %d\n", error);
> @@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, NULL);
>  }
>  
> +static int cdns_ti_runtime_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	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);

I don't think USB controller requires a reset and re-init between runtime suspend/resume.

What you need is reset/re-init during system Resume on certain platforms.
So you should move this part of code into a helper function and call it
from .probe() and .system_resume()


> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cdns_ti_pm_ops = {
> +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id cdns_ti_of_match[] = {
>  	{ .compatible = "ti,j721e-usb", },
>  	{ .compatible = "ti,am64-usb", },
> @@ -224,6 +237,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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/9] usb: cdns3-ti: move reg writes from probe into ->runtime_resume()

[Théo Lebrun]

Hello Roger,

On Thu Mar 7, 2024 at 1:31 PM CET, Roger Quadros wrote:
> Hi,
>
> On 07/03/2024 11:55, Théo Lebrun wrote:
> > The hardware initialisation register write sequence is only used at
> > probe. Move it from being done at explicitely at probe to being done
> > implicitely by pm_runtime_get_sync() that calls ->runtime_resume().
>
> explicitly / implicitly
>
> > 
> > Keep devicetree parsing in probe and add a new field in the private
> > struct to remember the USB2 refclk rate code computation result.
> > 
> > This opens the door to having the init sequence being executed later
> > down the road, at system-wide resume for example. This is NOT currently
> > happening because runtime PM is disabled at suspend without the
> > refcount being affected.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++-------------------
> >  1 file changed, 52 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 5945c4b1e11f..4c8a557e6a6f 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -57,6 +57,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 */
> > @@ -90,10 +91,8 @@ 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)
> > @@ -133,7 +132,9 @@ 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");
> >  
> >  	pm_runtime_enable(dev);
> >  	error = pm_runtime_get_sync(dev);
> > @@ -142,40 +143,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, NULL, dev);
> >  	if (error) {
> >  		dev_err(dev, "failed to create children: %d\n", error);
> > @@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, NULL);
> >  }
> >  
> > +static int cdns_ti_runtime_resume(struct device *dev)
> > +{
> > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > +	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);
>
> I don't think USB controller requires a reset and re-init between
> runtime suspend/resume.
>
> What you need is reset/re-init during system Resume on certain platforms.
> So you should move this part of code into a helper function and call it
> from .probe() and .system_resume()

Runtime resume is being called at probe() and system-wide resume. See
our runtime_resume() implementation as that helper function you are
describing.

A previous revision did what you are recommending. We leaned towards the
current version. See:
https://lore.kernel.org/lkml/7h34wxfmwn.fsf@baylibre.com/

Also, assuming we enable runtime PM, a reset and re-init after runtime
suspend would be the right thing to do anyways. My reading of
drivers/pmdomain/core.c tells me that if our device goes to runtime
suspend, domains will be shut down. Our controller will be reset and
we'll need to re-init it. The GENPD_FLAG_RPM_ALWAYS_ON flag is of
interest to avoid the PD to be shut down during runtime PM.

Regards,

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/9] usb: cdns3-ti: support reset-on-resume behavior

[Théo Lebrun]

Add match data support, with one boolean to indicate whether the
hardware resets after a system-wide suspend. If hardware resets, we
force execute ->runtime_resume() at system-wide resume to run the
hardware init sequence.

No compatible exploits this functionality, just yet.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 4c8a557e6a6f..f76327566798 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -57,9 +57,14 @@ struct cdns_ti {
 	unsigned vbus_divider:1;
 	struct clk *usb2_refclk;
 	struct clk *lpm_clk;
+	const struct cdns_ti_match_data *match_data;
 	int usb2_refclk_rate_code;
 };
 
+struct cdns_ti_match_data {
+	bool reset_on_resume;
+};
+
 static const int cdns_ti_rate_table[] = {	/* in KHZ */
 	9600,
 	10000,
@@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, data);
 
 	data->dev = dev;
+	data->match_data = device_get_match_data(dev);
 
 	data->usbss = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(data->usbss)) {
@@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int cdns_ti_suspend(struct device *dev)
+{
+	struct cdns_ti *data = dev_get_drvdata(dev);
+
+	if (data->match_data && data->match_data->reset_on_resume)
+		return pm_runtime_force_suspend(dev);
+	else
+		return 0;
+}
+
+static int cdns_ti_resume(struct device *dev)
+{
+	struct cdns_ti *data = dev_get_drvdata(dev);
+
+	if (data->match_data && data->match_data->reset_on_resume)
+		return pm_runtime_force_resume(dev);
+	else
+		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(cdns_ti_suspend, cdns_ti_resume)
 };
 
 static const struct of_device_id cdns_ti_of_match[] = {

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/9] usb: cdns3-ti: support reset-on-resume behavior

[Roger Quadros]

On 07/03/2024 11:55, Théo Lebrun wrote:
> Add match data support, with one boolean to indicate whether the
> hardware resets after a system-wide suspend. If hardware resets, we
> force execute ->runtime_resume() at system-wide resume to run the
> hardware init sequence.
> 
> No compatible exploits this functionality, just yet.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 4c8a557e6a6f..f76327566798 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -57,9 +57,14 @@ struct cdns_ti {
>  	unsigned vbus_divider:1;
>  	struct clk *usb2_refclk;
>  	struct clk *lpm_clk;
> +	const struct cdns_ti_match_data *match_data;
>  	int usb2_refclk_rate_code;
>  };
>  
> +struct cdns_ti_match_data {
> +	bool reset_on_resume;
> +};
> +
>  static const int cdns_ti_rate_table[] = {	/* in KHZ */
>  	9600,
>  	10000,
> @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
>  
>  	data->dev = dev;
> +	data->match_data = device_get_match_data(dev);
>  
>  	data->usbss = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(data->usbss)) {
> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int cdns_ti_suspend(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +
> +	if (data->match_data && data->match_data->reset_on_resume)
> +		return pm_runtime_force_suspend(dev);
> +	else
> +		return 0;
> +}
> +
> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +

Instead of this just do Reset and re-init here on affected
platforms.
This is after you remove reset/re-init code from .runtime_resume().

> +	if (data->match_data && data->match_data->reset_on_resume)
> +		return pm_runtime_force_resume(dev);
> +	else
> +		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(cdns_ti_suspend, cdns_ti_resume)
>  };
>  
>  static const struct of_device_id cdns_ti_of_match[] = {
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/9] usb: cdns3-ti: support reset-on-resume behavior

[Kevin Hilman]

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

> Add match data support, with one boolean to indicate whether the
> hardware resets after a system-wide suspend. If hardware resets, we
> force execute ->runtime_resume() at system-wide resume to run the
> hardware init sequence.

Is "whether the hardware resets after a system-wide suspend" really a
function of the IP itself, or rather whether the IP is in a power domain
that might power down?

> No compatible exploits this functionality, just yet.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 4c8a557e6a6f..f76327566798 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -57,9 +57,14 @@ struct cdns_ti {
>  	unsigned vbus_divider:1;
>  	struct clk *usb2_refclk;
>  	struct clk *lpm_clk;
> +	const struct cdns_ti_match_data *match_data;
>  	int usb2_refclk_rate_code;
>  };
>  
> +struct cdns_ti_match_data {
> +	bool reset_on_resume;
> +};
> +
>  static const int cdns_ti_rate_table[] = {	/* in KHZ */
>  	9600,
>  	10000,
> @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
>  
>  	data->dev = dev;
> +	data->match_data = device_get_match_data(dev);
>  
>  	data->usbss = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(data->usbss)) {
> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int cdns_ti_suspend(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +
> +	if (data->match_data && data->match_data->reset_on_resume)
> +		return pm_runtime_force_suspend(dev);
> +	else
> +		return 0;
> +}
> +
> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +
> +	if (data->match_data && data->match_data->reset_on_resume)
> +		return pm_runtime_force_resume(dev);
> +	else
> +		return 0;
> +}

Conditionally forcing runtime suspend/resume based on a property of the
IP doesn't feel right to me.

IMO, the device should always runtime suspend/resume, and in the
runtime PM hooks is where the conditional logic should be.

And speaking of the conditional logic... let's go back to whether
"resets_on_resume" is a property of the IP or the enclosing power
domain.

Instead of having an IP-specific flag, another way of approaching this
when ->runtime_resume() is called every time is simply for that hook to
check if a reset has happend.  Sometimes you can tell this simply by
reading a register that has been previously programmed by the driver but
has a known reset.  Simply check that regisister and you can tell
whether context has been lost.

Doing it this way makes the driver "smart" and then you don't have to
rely on bool flag based on the IP and dependent on the DT compatible.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/9] usb: cdns3-ti: support reset-on-resume behavior

[Théo Lebrun]

Hello,

On Fri Mar 8, 2024 at 10:58 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > Add match data support, with one boolean to indicate whether the
> > hardware resets after a system-wide suspend. If hardware resets, we
> > force execute ->runtime_resume() at system-wide resume to run the
> > hardware init sequence.
>
> Is "whether the hardware resets after a system-wide suspend" really a
> function of the IP itself, or rather whether the IP is in a power domain
> that might power down?

Is a compatible defining (1) the IP block involved or (2) the IP block
involved and its integration? This is a rethorical question, I've asked
it internally at Bootlin and we had some interesting discussions. :-)

Whether compatible or some other mechanism indicate expected suspend
behavior, we agreed that suspend types where not modeled properly by
the kernel currently.

> > No compatible exploits this functionality, just yet.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 4c8a557e6a6f..f76327566798 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -57,9 +57,14 @@ struct cdns_ti {
> >  	unsigned vbus_divider:1;
> >  	struct clk *usb2_refclk;
> >  	struct clk *lpm_clk;
> > +	const struct cdns_ti_match_data *match_data;
> >  	int usb2_refclk_rate_code;
> >  };
> >  
> > +struct cdns_ti_match_data {
> > +	bool reset_on_resume;
> > +};
> > +
> >  static const int cdns_ti_rate_table[] = {	/* in KHZ */
> >  	9600,
> >  	10000,
> > @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, data);
> >  
> >  	data->dev = dev;
> > +	data->match_data = device_get_match_data(dev);
> >  
> >  	data->usbss = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(data->usbss)) {
> > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
> >  	return 0;
> >  }
> >  
> > +static int cdns_ti_suspend(struct device *dev)
> > +{
> > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > +
> > +	if (data->match_data && data->match_data->reset_on_resume)
> > +		return pm_runtime_force_suspend(dev);
> > +	else
> > +		return 0;
> > +}
> > +
> > +static int cdns_ti_resume(struct device *dev)
> > +{
> > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > +
> > +	if (data->match_data && data->match_data->reset_on_resume)
> > +		return pm_runtime_force_resume(dev);
> > +	else
> > +		return 0;
> > +}
>
> Conditionally forcing runtime suspend/resume based on a property of the
> IP doesn't feel right to me.
>
> IMO, the device should always runtime suspend/resume, and in the
> runtime PM hooks is where the conditional logic should be.
>
> And speaking of the conditional logic... let's go back to whether
> "resets_on_resume" is a property of the IP or the enclosing power
> domain.
>
> Instead of having an IP-specific flag, another way of approaching this
> when ->runtime_resume() is called every time is simply for that hook to
> check if a reset has happend.  Sometimes you can tell this simply by
> reading a register that has been previously programmed by the driver but
> has a known reset.  Simply check that regisister and you can tell
> whether context has been lost.
>
> Doing it this way makes the driver "smart" and then you don't have to
> rely on bool flag based on the IP and dependent on the DT compatible.

I agree! I never digged into this for a reason: the HXCI subsystem takes
a quirk flag that tells it whether it resets on resume
(XHCI_RESET_ON_RESUME). My plans weren't grandiose enough to think
about touching this aspect. That means we need to know this bool value
at probe.

About a smart ->runtime_resume() implementation: it is doable. I've
experimented with that following your message. The condition is rather
simple, looking like:

	static int cdns_ti_runtime_resume(struct device *dev)
	{
		struct cdns_ti *data = dev_get_drvdata(dev);
		u32 w1, mask;

		w1 = cdns_ti_readl(data, USBSS_W1);
		mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;

		if ((w1 & mask) != mask)
			cdns_ti_reset_and_init_hw(dev, data);

		return 0;
	}

Tested on J7200-EVM, works as expected. Both bits reset to zero. First
is software reset; second is described as "this bit has to be always
set to 1".

cdns_ti_reset_and_init_hw() would also be call at probe before enabling
runtime PM to ensure we always reset the IP at probe.
Then ->runtime_resume() would get called once during probe without any
impact as the conditional would return false. It would trigger at
resume. System-wide suspend/resume hooks would become:

	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
			pm_runtime_force_resume)

Do you have any ideas related to the handling of XHCI_RESET_ON_RESUME?
See xhci_resume(). Some notes:

 - XHCI is capable of detect what it calls reinit (reset after resume).
   The flags only shortcut the restore attempt (which is useless when
   the controller reset) and disables warning because of reinit. Logs
   contain this when we do not pass the flag:

   [   18.518138] xhci-hcd xhci-hcd.11.auto: xHC error in resume, USBSTS 0x401, Reinit
   [   18.525522] usb usb1: root hub lost power or was reset
   [   18.530647] usb usb2: root hub lost power or was reset

 - Ways forward I can imagine:

    - A quick and dirty solution would be to grab a reference to the
      xhci_hcd struct pointer from TI wrapper and update quirks on the
      go.

    - Another one would be to keep as-is and let xhci-hcd warn on each
      resume. Mentioning this for exhaustiveness.

    - Introduce a new mechanism to communicate through the stack: from
      the TI wrapper, to cdns3 core, to the xhci-hcd device. Or use an
      existing mechanism if one is present.

    - Remove the assumption xHCI does that a suspend is broken if the
      controller was reset during it. That doesn't sound right to me,
      can you confirm?

Thanks!

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/9] usb: cdns3-ti: pass auxdata from match data to of_platform_populate()

[Théo Lebrun]

Allow compatible to pick auxdata given to child platform devices.

No compatible exploits this functionality, just yet.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index f76327566798..29fb24c811b3 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -62,7 +62,8 @@ struct cdns_ti {
 };
 
 struct cdns_ti_match_data {
-	bool reset_on_resume;
+	bool				reset_on_resume;
+	const struct of_dev_auxdata	*auxdata;
 };
 
 static const int cdns_ti_rate_table[] = {	/* in KHZ */
@@ -95,6 +96,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 = NULL;
 	struct cdns_ti *data;
 	unsigned long rate;
 	int error, i;
@@ -149,7 +151,9 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	error = of_platform_populate(node, NULL, NULL, dev);
+	if (data->match_data)
+		auxdata = data->match_data->auxdata;
+	error = of_platform_populate(node, NULL, auxdata, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);
 		goto err;

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/9] usb: cdns3-ti: pass auxdata from match data to of_platform_populate()

[Roger Quadros]

On 07/03/2024 11:55, Théo Lebrun wrote:
> Allow compatible to pick auxdata given to child platform devices.
> 
> No compatible exploits this functionality, just yet.
> 

This patch could be merged with Patch 7 so we know exactly how auxdata
is used?

> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index f76327566798..29fb24c811b3 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -62,7 +62,8 @@ struct cdns_ti {
>  };
>  
>  struct cdns_ti_match_data {
> -	bool reset_on_resume;
> +	bool				reset_on_resume;
> +	const struct of_dev_auxdata	*auxdata;
>  };
>  
>  static const int cdns_ti_rate_table[] = {	/* in KHZ */
> @@ -95,6 +96,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 = NULL;
>  	struct cdns_ti *data;
>  	unsigned long rate;
>  	int error, i;
> @@ -149,7 +151,9 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	error = of_platform_populate(node, NULL, NULL, dev);
> +	if (data->match_data)
> +		auxdata = data->match_data->auxdata;
> +	error = of_platform_populate(node, NULL, auxdata, dev);
>  	if (error) {
>  		dev_err(dev, "failed to create children: %d\n", error);
>  		goto err;
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/9] usb: cdns3-ti: pass auxdata from match data to of_platform_populate()

[Théo Lebrun]

Hello,

On Thu Mar 7, 2024 at 1:38 PM CET, Roger Quadros wrote:
>
>
> On 07/03/2024 11:55, Théo Lebrun wrote:
> > Allow compatible to pick auxdata given to child platform devices.
> > 
> > No compatible exploits this functionality, just yet.
> > 
>
> This patch could be merged with Patch 7 so we know exactly how auxdata
> is used?

Indeed. I liked splitting. Previous revision had it as a single patch.
I can revert.

Regards,

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 6/9] 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 81a9c9d6be08..7487067ba23f 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -44,6 +44,7 @@ struct cdns3_platform_data {
 			bool suspend, bool wakeup);
 	unsigned long quirks;
 #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
+#define CDNS3_RESET_ON_RESUME		BIT(1)
 };
 
 /**
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 6164fc4c96a4..28c4d1deb231 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -91,6 +91,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.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 7/9] 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 meaning:
 - The cdns3-ti wrapper init sequence must be ran at resume.
 - Tell the cdns3 core that we reset on resume. This silences a xHCI
   warning visible in cases of unexpected resets.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 29fb24c811b3..648243a27987 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -17,6 +17,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 
+#include "core.h"
+
 /* USB Wrapper register offsets */
 #define USBSS_PID		0x0
 #define	USBSS_W1		0x4
@@ -255,7 +257,25 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
 	SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
 };
 
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+       .quirks = CDNS3_RESET_ON_RESUME,
+};
+
+static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = {
+       {
+               .compatible = "cdns,usb3",
+               .platform_data = &cdns_ti_j7200_pdata,
+       },
+       {},
+};
+
+static const struct cdns_ti_match_data cdns_ti_j7200_match_data = {
+       .reset_on_resume = true,
+       .auxdata = cdns_ti_j7200_auxdata,
+};
+
 static const struct of_device_id cdns_ti_of_match[] = {
+	{ .compatible = "ti,j7200-usb", .data = &cdns_ti_j7200_match_data, },
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
 	{},

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 8/9] 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 45ba9c2d0344..81a799792978 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
@@ -793,7 +793,7 @@ pcie1_ep: pcie-ep@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.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 9/9] 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 e348114f42e0..de04f4350a9e 100644
--- a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
@@ -749,7 +749,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.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel