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

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

[Théo Lebrun]

Hi,

Suspend on the TI J7200 platform is broken currently. There are two
components that need to be patched so that they assume reset on
resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
controller.

About (1): the TI wrapper only did its hardware configuration at probe
time. Update so that it is done at resume on J7200 SoC.

About (2): signal from cdns3-ti to cdns3 host to xHCI that the
controller resets on resume. This way we avoid the following warning:

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

Strictly speaking (2) is not required to have working suspend on J7200;
its only goal is to silence this warning.

Those patches have been tested on the TI J7200 EVM GP. No need to
mention that other patches are required for S2R to work, but those will
be sent later down the road. Those USB patches are rather standalone.

Thanks,
Théo

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

Signed-off-by: Théo Lebrun <theo.lebrun@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 (7):
      dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
      usb: cdns3-ti: remove runtime PM
      usb: cdns3-ti: move reg writes from probe into an init_hw helper
      usb: cdns3-ti: add suspend/resume procedures for J7200
      usb: cdns3: add quirk to platform data for reset-on-resume
      usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform
      arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

 .../devicetree/bindings/usb/ti,j721e-usb.yaml      |   4 +
 arch/arm64/boot/dts/ti/k3-j7200-main.dtsi          |   2 +-
 drivers/usb/cdns3/cdns3-ti.c                       | 134 +++++++++++++--------
 drivers/usb/cdns3/core.h                           |   1 +
 drivers/usb/cdns3/host.c                           |   3 +
 5 files changed, 90 insertions(+), 54 deletions(-)
---
base-commit: 1d42d5c8f1ca11106579dcaadef4161fee03419e
change-id: 20231113-j7200-usb-suspend-2a47f2281e04

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

[PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

On this platform, the controller & its wrapper are reset on resume. This
makes it have a different behavior from other platforms.

We allow using the new compatible with a fallback onto the original
ti,j721e-usb compatible. We therefore allow using an older kernel with
a more recent devicetree.

Acked-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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..69a222dfd9ff 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -12,11 +12,15 @@ maintainers:
 properties:
   compatible:
     oneOf:
+      - const: ti,j7200-usb
       - const: ti,j721e-usb
       - const: ti,am64-usb
       - items:
           - const: ti,j721e-usb
           - const: ti,am64-usb
+      - items:
+          - const: ti,j721e-usb
+          - const: ti,j7200-usb
 
   reg:
     maxItems: 1

-- 
2.42.0

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Krzysztof Kozlowski]

On 20/11/2023 18:06, Théo Lebrun wrote:
> On this platform, the controller & its wrapper are reset on resume. This
> makes it have a different behavior from other platforms.
> 
> We allow using the new compatible with a fallback onto the original
> ti,j721e-usb compatible. We therefore allow using an older kernel with

Where is fallback ti,j721e-usb used? Please point me to the code.


> a more recent devicetree.
> 
> Acked-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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 95ff9791baea..69a222dfd9ff 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -12,11 +12,15 @@ maintainers:
>  properties:
>    compatible:
>      oneOf:
> +      - const: ti,j7200-usb
>        - const: ti,j721e-usb
>        - const: ti,am64-usb
>        - items:
>            - const: ti,j721e-usb
>            - const: ti,am64-usb
> +      - items:
> +          - const: ti,j721e-usb

This makes little sense. It's already on the list. Twice! Don't add it
third time.

I am sorry, but this binding makes no sense. I mean, existing binding
makes no sense, but your change is not making it anyhow better.

Best regards,
Krzysztof

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

Hello,

On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
> On 20/11/2023 18:06, Théo Lebrun wrote:
> > On this platform, the controller & its wrapper are reset on resume. This
> > makes it have a different behavior from other platforms.
> > 
> > We allow using the new compatible with a fallback onto the original
> > ti,j721e-usb compatible. We therefore allow using an older kernel with
>
> Where is fallback ti,j721e-usb used? Please point me to the code.

No fallback is implemented in code. Using a kernel that doesn't have
this patch series but a more recent devicetree: DT has both
devicetrees & the kernel will know which driver to use.

That is opposed to having only compatible = "ti,j7200-usb". If using an
old kernel, it would not know what driver to match it to.

[...]

> > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > @@ -12,11 +12,15 @@ maintainers:
> >  properties:
> >    compatible:
> >      oneOf:
> > +      - const: ti,j7200-usb
> >        - const: ti,j721e-usb
> >        - const: ti,am64-usb
> >        - items:
> >            - const: ti,j721e-usb
> >            - const: ti,am64-usb
> > +      - items:
> > +          - const: ti,j721e-usb
>
> This makes little sense. It's already on the list. Twice! Don't add it
> third time.
>
> I am sorry, but this binding makes no sense. I mean, existing binding
> makes no sense, but your change is not making it anyhow better.

The goal of the DT schema pre-patch was to allow all three:

   compatible = "ti,j721e-usb";
   compatible = "ti,am64-usb";
   compatible = "ti,j721e-usb", "ti,am64-usb";

I've followed the same scheme & added both of those:

   compatible = "ti,j7200-usb";
   compatible = "ti,j7200-usb", "ti,j721e-usb";

I messed up the ordering in the added 'items' options, but the logic
seems right to me. And dtbs_check agrees. Am I missing something?

Thanks,

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

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Krzysztof Kozlowski]

On 21/11/2023 17:53, Théo Lebrun wrote:
> Hello,
> 
> On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
>> On 20/11/2023 18:06, Théo Lebrun wrote:
>>> On this platform, the controller & its wrapper are reset on resume. This
>>> makes it have a different behavior from other platforms.
>>>
>>> We allow using the new compatible with a fallback onto the original
>>> ti,j721e-usb compatible. We therefore allow using an older kernel with
>>
>> Where is fallback ti,j721e-usb used? Please point me to the code.
> 
> No fallback is implemented in code. Using a kernel that doesn't have
> this patch series but a more recent devicetree: DT has both
> devicetrees & the kernel will know which driver to use.

I meant your bindings. You said - with fallback to ti,j721e-usb. I do
not see it. To me the commit description is not accurate.

> 
> That is opposed to having only compatible = "ti,j7200-usb". If using an
> old kernel, it would not know what driver to match it to.
> 
> [...]
> 
>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>> @@ -12,11 +12,15 @@ maintainers:
>>>  properties:
>>>    compatible:
>>>      oneOf:
>>> +      - const: ti,j7200-usb
>>>        - const: ti,j721e-usb
>>>        - const: ti,am64-usb
>>>        - items:
>>>            - const: ti,j721e-usb
>>>            - const: ti,am64-usb
>>> +      - items:
>>> +          - const: ti,j721e-usb
>>
>> This makes little sense. It's already on the list. Twice! Don't add it
>> third time.
>>
>> I am sorry, but this binding makes no sense. I mean, existing binding
>> makes no sense, but your change is not making it anyhow better.
> 
> The goal of the DT schema pre-patch was to allow all three:
> 
>    compatible = "ti,j721e-usb";
>    compatible = "ti,am64-usb";
>    compatible = "ti,j721e-usb", "ti,am64-usb";

Which does not make sense.

How ti,j721e-usb can be and cannot be compatible with am64 in the same time?

> 
> I've followed the same scheme & added both of those:
> 
>    compatible = "ti,j7200-usb";
>    compatible = "ti,j7200-usb", "ti,j721e-usb";
> 
> I messed up the ordering in the added 'items' options, but the logic
> seems right to me. And dtbs_check agrees. Am I missing something?
> 

Logic is wrong. Device either is or is not compatible with something. At
least usually. We have some exceptions like SMMU for Adreno. Is this the
case? Why the device is and is not compatible with some other variant?

Best regards,
Krzysztof

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

Hello,

On Tue Nov 21, 2023 at 6:11 PM CET, Krzysztof Kozlowski wrote:
> On 21/11/2023 17:53, Théo Lebrun wrote:
> > On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
> >> On 20/11/2023 18:06, Théo Lebrun wrote:
> >>> On this platform, the controller & its wrapper are reset on resume. This
> >>> makes it have a different behavior from other platforms.
> >>>
> >>> We allow using the new compatible with a fallback onto the original
> >>> ti,j721e-usb compatible. We therefore allow using an older kernel with
> >>
> >> Where is fallback ti,j721e-usb used? Please point me to the code.
> > 
> > No fallback is implemented in code. Using a kernel that doesn't have
> > this patch series but a more recent devicetree: DT has both
> > devicetrees & the kernel will know which driver to use.
>
> I meant your bindings. You said - with fallback to ti,j721e-usb. I do
> not see it. To me the commit description is not accurate.

I see your point, I'll remove that aspect.

> > That is opposed to having only compatible = "ti,j7200-usb". If using an
> > old kernel, it would not know what driver to match it to.
> > 
> > [...]
> > 
> >>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>> @@ -12,11 +12,15 @@ maintainers:
> >>>  properties:
> >>>    compatible:
> >>>      oneOf:
> >>> +      - const: ti,j7200-usb
> >>>        - const: ti,j721e-usb
> >>>        - const: ti,am64-usb
> >>>        - items:
> >>>            - const: ti,j721e-usb
> >>>            - const: ti,am64-usb
> >>> +      - items:
> >>> +          - const: ti,j721e-usb
> >>
> >> This makes little sense. It's already on the list. Twice! Don't add it
> >> third time.
> >>
> >> I am sorry, but this binding makes no sense. I mean, existing binding
> >> makes no sense, but your change is not making it anyhow better.
> > 
> > The goal of the DT schema pre-patch was to allow all three:
> > 
> >    compatible = "ti,j721e-usb";
> >    compatible = "ti,am64-usb";
> >    compatible = "ti,j721e-usb", "ti,am64-usb";
>
> Which does not make sense.
>
> How ti,j721e-usb can be and cannot be compatible with am64 in the same time?

The code tells us that there is no difference between ti,j721e-usb &
ti,am64-usb. And the commit adding the of_device_id entry agrees, see
4f30b9d2315f (usb: cdns3: Add support for TI's AM64 SoC, 2021-01-19).
Here is the entire patch because it is so small:

   diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
   index 90e246601537..eccb1c766bba 100644
   --- a/drivers/usb/cdns3/cdns3-ti.c
   +++ b/drivers/usb/cdns3/cdns3-ti.c
   @@ -214,6 +214,7 @@ static int cdns_ti_remove(struct platform_device *pdev)

    static const struct of_device_id cdns_ti_of_match[] = {
      { .compatible = "ti,j721e-usb", },
   +  { .compatible = "ti,am64-usb", },
      {},
    };
    MODULE_DEVICE_TABLE(of, cdns_ti_of_match);

> > I've followed the same scheme & added both of those:
> > 
> >    compatible = "ti,j7200-usb";
> >    compatible = "ti,j7200-usb", "ti,j721e-usb";
> > 
> > I messed up the ordering in the added 'items' options, but the logic
> > seems right to me. And dtbs_check agrees. Am I missing something?
>
> Logic is wrong. Device either is or is not compatible with something. At
> least usually. We have some exceptions like SMMU for Adreno. Is this the
> case? Why the device is and is not compatible with some other variant?

My understanding is this: j721e & am64 are strictly equivalent. On our
j7200 we know we reset on resume. I see three ways forward:

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

   We keep both ti,j721e-usb & ti,am64-usb separate even though they are
   compatible. It makes for simpler changes & it avoids touching
   devicetrees.

 - properties:
   compatible:
     oneOf:
       - const: ti,j7200-usb
       - const: ti,j721e-usb

   AM64 is a duplicate of J721E. Remove it and update upstream
   devicetrees.

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

   J721E & AM64 are compatible, express that & update devicetrees.

Option one is simpler & doesn't change devicetrees so I'd lean in that
direction. What's your opinion?

Regards,

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

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Krzysztof Kozlowski]

On 22/11/2023 11:46, Théo Lebrun wrote:

>>
>> How ti,j721e-usb can be and cannot be compatible with am64 in the same time?
> 
> The code tells us that there is no difference between ti,j721e-usb &
> ti,am64-usb. And the commit adding the of_device_id entry agrees, see
> 4f30b9d2315f (usb: cdns3: Add support for TI's AM64 SoC, 2021-01-19).
> Here is the entire patch because it is so small:
> 
>    diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
>    index 90e246601537..eccb1c766bba 100644
>    --- a/drivers/usb/cdns3/cdns3-ti.c
>    +++ b/drivers/usb/cdns3/cdns3-ti.c
>    @@ -214,6 +214,7 @@ static int cdns_ti_remove(struct platform_device *pdev)
> 
>     static const struct of_device_id cdns_ti_of_match[] = {
>       { .compatible = "ti,j721e-usb", },
>    +  { .compatible = "ti,am64-usb", },
>       {},
>     };
>     MODULE_DEVICE_TABLE(of, cdns_ti_of_match);
> 
>>> I've followed the same scheme & added both of those:
>>>
>>>    compatible = "ti,j7200-usb";
>>>    compatible = "ti,j7200-usb", "ti,j721e-usb";
>>>
>>> I messed up the ordering in the added 'items' options, but the logic
>>> seems right to me. And dtbs_check agrees. Am I missing something?
>>
>> Logic is wrong. Device either is or is not compatible with something. At
>> least usually. We have some exceptions like SMMU for Adreno. Is this the
>> case? Why the device is and is not compatible with some other variant?
> 
> My understanding is this: j721e & am64 are strictly equivalent. On our

Then this should be expressed in the bindings. Currently and in your
patch, you express that they are not compatible.


...

> 
>  - properties:
>    compatible:
>      oneOf:
>        - const: ti,j7200-usb
>        - items:
>            - const: ti,j721e-usb
>            - const: ti,am64-usb
> 
>    J721E & AM64 are compatible, express that & update devicetrees.
> 
> Option one is simpler & doesn't change devicetrees so I'd lean in that
> direction. What's your opinion?


This one should be for am64.

For your j7200, it depends whether the fallback to j721e makes sense,
IOW, if the Linux can use j721e compatible solely to use j7200 device.

Best regards,
Krzysztof

Re: [PATCH v2 1/7] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

Hello,

On Wed Nov 22, 2023 at 1:00 PM CET, Krzysztof Kozlowski wrote:
> On 22/11/2023 11:46, Théo Lebrun wrote:
> >  - properties:
> >    compatible:
> >      oneOf:
> >        - const: ti,j7200-usb
> >        - items:
> >            - const: ti,j721e-usb
> >            - const: ti,am64-usb
> > 
> >    J721E & AM64 are compatible, express that & update devicetrees.
> > 
> > Option one is simpler & doesn't change devicetrees so I'd lean in that
> > direction. What's your opinion?
>
> This one should be for am64.
>
> For your j7200, it depends whether the fallback to j721e makes sense,
> IOW, if the Linux can use j721e compatible solely to use j7200 device.

All compatibles might be equivalent if the reset-on-resume behavior is
observed on all three platforms. I don't have access to J721E or AM64
to test that.

@Roger Quadros: do you have any news if USB suspend/resume works on
J721E and/or AM64? My testing on the J7200 EVM is (1) boot, (2) put
the "USB2.0_MUX_SEL" GPIO high to have USB devices connected without
plugging anything, then (3) trigger a s2idle. I get a memory bus
exception on resume without my patches.

Regards,

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

[PATCH v2 2/7] usb: cdns3-ti: remove runtime PM

[Théo Lebrun]

The driver does not use RPM. It enables it & gets a reference at probe.
It then undoes that on probe error or at remove.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..dc1594acdcee 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -135,13 +135,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
 
 	rate_code = i;
 
-	pm_runtime_enable(dev);
-	error = pm_runtime_get_sync(dev);
-	if (error < 0) {
-		dev_err(dev, "pm_runtime_get_sync failed: %d\n", error);
-		goto err;
-	}
-
 	/* assert RESET */
 	reg = cdns_ti_readl(data, USBSS_W1);
 	reg &= ~USBSS_W1_PWRUP_RST;
@@ -179,16 +172,10 @@ static int cdns_ti_probe(struct platform_device *pdev)
 	error = of_platform_populate(node, NULL, NULL, dev);
 	if (error) {
 		dev_err(dev, "failed to create children: %d\n", error);
-		goto err;
+		return error;
 	}
 
 	return 0;
-
-err:
-	pm_runtime_put_sync(data->dev);
-	pm_runtime_disable(data->dev);
-
-	return error;
 }
 
 static int cdns_ti_remove_core(struct device *dev, void *c)
@@ -205,8 +192,6 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 
 	device_for_each_child(dev, NULL, cdns_ti_remove_core);
-	pm_runtime_put_sync(dev);
-	pm_runtime_disable(dev);
 
 	platform_set_drvdata(pdev, NULL);
 }

-- 
2.42.0

Re: [PATCH v2 2/7] usb: cdns3-ti: remove runtime PM

[Kevin Hilman]

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

> The driver does not use RPM. It enables it & gets a reference at probe.
> It then undoes that on probe error or at remove.

...which is a fairly standard thing to do for a rudimentary runtime PM
support on platforms that use power domains.

This will likely (almost surely) break other platforms.

Without a runtime PM get call, the power domain that this device is in
could be powered off without this driver ever knowing about it, causing
a crash as soon as the driver is used after the domain is turned off.

Kevin

[PATCH v2 3/7] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Théo Lebrun]

The hardware initialisation register write sequence is only used at
probe. To support suspend/resume with a controller losing power, we
must redo this sequence of writes.

Extract the register write sequence to a new cdns_ti_init_hw function to
reuse it later down the road, at resume.

We keep the devicetree-parsing aspect of the sequence in probe & add a
new field in the private struct to remember the USB2 refclk rate code
computation result.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index dc1594acdcee..d4232b440e4e 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 */
@@ -85,15 +86,50 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
 	writel(value, data->usbss + offset);
 }
 
+static void cdns_ti_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)
@@ -133,41 +169,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	rate_code = i;
-
-	/* 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->usb2_refclk_rate_code = i;
 	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);
+	cdns_ti_init_hw(data);
 
 	error = of_platform_populate(node, NULL, NULL, dev);
 	if (error) {

-- 
2.42.0

[PATCH v2 4/7] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hardware initialisation is only done at probe. The J7200 USB controller
is reset at resume because of power-domains toggling off & on. We
therefore reconfigure the hardware at resume.

Reuse the newly extracted cdns_ti_init_hw() function that contains the
register write sequence.

Only focus J7200 as other SoC are untested. If the controller does not
reset we do not want to redo reg writes.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index d4232b440e4e..84f93c2fcd5c 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,7 @@ struct cdns_ti {
 	struct clk *usb2_refclk;
 	struct clk *lpm_clk;
 	int usb2_refclk_rate_code;
+	bool reset_on_resume;
 };
 
 static const int cdns_ti_rate_table[] = {	/* in KHZ */
@@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
 	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");
+	data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");
 
 	cdns_ti_init_hw(data);
 
@@ -202,7 +204,26 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+#ifdef CONFIG_PM
+
+static int cdns_ti_resume(struct device *dev)
+{
+	struct cdns_ti *data = dev_get_drvdata(dev);
+
+	if (data->reset_on_resume)
+		cdns_ti_init_hw(data);
+
+	return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, cdns_ti_resume)
+};
+
+#endif /* CONFIG_PM */
+
 static const struct of_device_id cdns_ti_of_match[] = {
+	{ .compatible = "ti,j7200-usb", },
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
 	{},
@@ -213,8 +234,9 @@ static struct platform_driver cdns_ti_driver = {
 	.probe		= cdns_ti_probe,
 	.remove_new	= cdns_ti_remove,
 	.driver		= {
-		.name	= "cdns3-ti",
+		.name		= "cdns3-ti",
 		.of_match_table	= cdns_ti_of_match,
+		.pm		= pm_ptr(&cdns_ti_pm_ops),
 	},
 };
 

-- 
2.42.0

Re: [PATCH v2 4/7] usb: cdns3-ti: add suspend/resume procedures for J7200

[Krzysztof Kozlowski]

On 20/11/2023 18:06, Théo Lebrun wrote:
> Hardware initialisation is only done at probe. The J7200 USB controller
> is reset at resume because of power-domains toggling off & on. We
> therefore reconfigure the hardware at resume.
> 
> Reuse the newly extracted cdns_ti_init_hw() function that contains the
> register write sequence.
> 
> Only focus J7200 as other SoC are untested. If the controller does not
> reset we do not want to redo reg writes.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index d4232b440e4e..84f93c2fcd5c 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -58,6 +58,7 @@ struct cdns_ti {
>  	struct clk *usb2_refclk;
>  	struct clk *lpm_clk;
>  	int usb2_refclk_rate_code;
> +	bool reset_on_resume;
>  };
>  
>  static const int cdns_ti_rate_table[] = {	/* in KHZ */
> @@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  	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");
> +	data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");

No, use driver data for this. Don't put compatibles in the code. It
makes it unmanageable soon.

I am repeating this issue from time to time :/

Best regards,
Krzysztof

Re: [PATCH v2 4/7] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hello Krzysztof,

On Mon Nov 20, 2023 at 6:31 PM CET, Krzysztof Kozlowski wrote:
> On 20/11/2023 18:06, Théo Lebrun wrote:

[...]

> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> >  static const int cdns_ti_rate_table[] = {	/* in KHZ */
> > @@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >  	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");
> > +	data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");
>
> No, use driver data for this. Don't put compatibles in the code. It
> makes it unmanageable soon.

Fixed for next revision. I hesitated on this patch but I'll know for
next time.

Thanks,

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

[PATCH v2 5/7] 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.

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

Re: [PATCH v2 5/7] usb: cdns3: add quirk to platform data for reset-on-resume

[Peter Chen]

On 23-11-20 18:06:05, 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.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

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

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

-- 

Thanks,
Peter Chen

[PATCH v2 6/7] usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform

[Théo Lebrun]

Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.

Avoid this warning on resume:

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

When used, remote wakeup is not expected to work.

Only focus J7200 as other SoC are untested.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 84f93c2fcd5c..7d56a1acbc54 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -16,6 +16,7 @@
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include "core.h"
 
 /* USB Wrapper register offsets */
 #define USBSS_PID		0x0
@@ -128,6 +129,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;
@@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
 
 	cdns_ti_init_hw(data);
 
-	error = of_platform_populate(node, NULL, NULL, dev);
+	auxdata = of_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);
 		return error;
@@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
 
 #endif /* CONFIG_PM */
 
+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 of_device_id cdns_ti_of_match[] = {
-	{ .compatible = "ti,j7200-usb", },
+	{ .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata, },
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
 	{},

-- 
2.42.0

Re: [PATCH v2 6/7] usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform

[Roger Quadros]

On 20/11/2023 19:06, Théo Lebrun wrote:
> Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
> in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
> 
> Avoid this warning on resume:
> 
>   [   16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
> 
> When used, remote wakeup is not expected to work.
> 
> Only focus J7200 as other SoC are untested.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 84f93c2fcd5c..7d56a1acbc54 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> +#include "core.h"
>  
>  /* USB Wrapper register offsets */
>  #define USBSS_PID		0x0
> @@ -128,6 +129,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;
> @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
>  
>  	cdns_ti_init_hw(data);
>  
> -	error = of_platform_populate(node, NULL, NULL, dev);
> +	auxdata = of_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);
>  		return error;
> @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
>  
>  #endif /* CONFIG_PM */
>  
> +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> +	.quirks = CDNS3_RESET_ON_RESUME,
> +};

We will need to introduce a new data structure "struct cdns_ti_platform_data"
and add platform specific details like "reset_on_resume" to it.
This is to address what Krzysztof pointed to in patch 4.

> +
> +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", },
> +	{ .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata, },

Here we should pass "struct cdns_ti_platform_data"

>  	{ .compatible = "ti,j721e-usb", },
>  	{ .compatible = "ti,am64-usb", },
>  	{},
> 

-- 
cheers,
-roger

Re: [PATCH v2 6/7] usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform

[Théo Lebrun]

Hello,

On Tue Nov 21, 2023 at 5:53 PM CET, Roger Quadros wrote:
> On 20/11/2023 19:06, Théo Lebrun wrote:
> > Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
> > in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
> > 
> > Avoid this warning on resume:
> > 
> >   [   16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
> > 
> > When used, remote wakeup is not expected to work.
> > 
> > Only focus J7200 as other SoC are untested.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 84f93c2fcd5c..7d56a1acbc54 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/property.h>
> > +#include "core.h"
> >  
> >  /* USB Wrapper register offsets */
> >  #define USBSS_PID		0x0
> > @@ -128,6 +129,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;
> > @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >  
> >  	cdns_ti_init_hw(data);
> >  
> > -	error = of_platform_populate(node, NULL, NULL, dev);
> > +	auxdata = of_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);
> >  		return error;
> > @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
> >  
> >  #endif /* CONFIG_PM */
> >  
> > +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> > +	.quirks = CDNS3_RESET_ON_RESUME,
> > +};
>
> We will need to introduce a new data structure "struct cdns_ti_platform_data"
> and add platform specific details like "reset_on_resume" to it.
> This is to address what Krzysztof pointed to in patch 4.

Yes I've got it locally following Krzysztof's review. Below my signature
is a sneak peak as I'll wait a bit more before a V3. First we implement
resume behavior in the wrapper driver using match data then we add
auxdata passed to the subdevices.

Regards,

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


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


From 69a59e3408668dfa06d3790cb20948961708791d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20Lebrun?= <theo.lebrun@bootlin.com>
Date: Mon, 20 Nov 2023 16:47:29 +0100
Subject: [PATCH 05/13] usb: cdns3-ti: add suspend/resume procedures for J7200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hardware initialisation is only done at probe. The J7200 USB controller
is reset at resume because of power-domains toggling off & on. We
therefore reconfigure the hardware at resume.

Reuse the newly extracted cdns_ti_init_hw() function that contains the
register write sequence.

Only focus J7200 as other SoC are untested. If the controller does not
reset we do not want to redo reg writes.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index d4232b440e4e..7530b6b5159d 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,11 @@ struct cdns_ti {
   struct clk *usb2_refclk;
   struct clk *lpm_clk;
   int usb2_refclk_rate_code;
+  const struct cdns_ti_match_data *match_data;
+};
+
+struct cdns_ti_match_data {
+  bool reset_on_resume;
 };

 static const int cdns_ti_rate_table[] = {   /* in KHZ */
@@ -138,6 +143,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
   platform_set_drvdata(pdev, data);

   data->dev = dev;
+  data->match_data = of_device_get_match_data(dev);

   data->usbss = devm_platform_ioremap_resource(pdev, 0);
   if (IS_ERR(data->usbss)) {
@@ -202,7 +208,30 @@ static void cdns_ti_remove(struct platform_device *pdev)
   platform_set_drvdata(pdev, NULL);
 }

+#ifdef CONFIG_PM
+
+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)
+     cdns_ti_init_hw(data);
+
+  return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+  SET_SYSTEM_SLEEP_PM_OPS(NULL, cdns_ti_resume)
+};
+
+#endif /* CONFIG_PM */
+
+static const struct cdns_ti_match_data cdns_ti_j7200_match_data = {
+  .reset_on_resume = true,
+};
+
 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", },
   {},
@@ -213,8 +242,9 @@ static struct platform_driver cdns_ti_driver = {
   .probe      = cdns_ti_probe,
   .remove_new = cdns_ti_remove,
   .driver     = {
-     .name = "cdns3-ti",
+     .name    = "cdns3-ti",
      .of_match_table   = cdns_ti_of_match,
+     .pm      = pm_ptr(&cdns_ti_pm_ops),
   },
 };

--
2.42.0


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


From 4ff91a036da297e9e8585980c6133bee9c45d9a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20Lebrun?= <theo.lebrun@bootlin.com>
Date: Mon, 20 Nov 2023 17:02:44 +0100
Subject: [PATCH 07/13] usb: cdns3-ti: signal reset-on-resume to xHCI for J7200
 platform
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.

Avoid this warning on resume:

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

When used, remote wakeup is not expected to work.

Only focus J7200 as other SoC are untested.

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

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 7530b6b5159d..da2648ebc179 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -16,6 +16,7 @@
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include "core.h"

 /* USB Wrapper register offsets */
 #define USBSS_PID      0x0
@@ -62,7 +63,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 */
@@ -132,6 +134,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;
@@ -181,7 +184,9 @@ static int cdns_ti_probe(struct platform_device *pdev)

   cdns_ti_init_hw(data);

-  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);
      return error;
@@ -226,8 +231,21 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {

 #endif /* CONFIG_PM */

+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[] = {
--
2.42.0

[PATCH v2 7/7] 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 709081cd1e7f..52c4ee0fa334 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
@@ -788,7 +788,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.42.0

Re: [PATCH v2 7/7] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Krzysztof Kozlowski]

On 20/11/2023 18:06, 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>

Fix your addressee list...

> ---
>  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 709081cd1e7f..52c4ee0fa334 100644
> --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> @@ -788,7 +788,7 @@ pcie1_ep: pcie-ep@2910000 {
>  	};
>  
>  	usbss0: cdns-usb@4104000 {
> -		compatible = "ti,j721e-usb";
> +		compatible = "ti,j7200-usb", "ti,j721e-usb";

It does not match your bindings. Bindings say something entirely else.

Best regards,
Krzysztof

Re: [PATCH v2 0/7] usb: cdns: fix suspend on J7200 by assuming reset-on-resume

[Krzysztof Kozlowski]

On 20/11/2023 18:06, Théo Lebrun wrote:
> Hi,
> 
> Suspend on the TI J7200 platform is broken currently. There are two
> components that need to be patched so that they assume reset on
> resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
> controller.
> 
> About (1): the TI wrapper only did its hardware configuration at probe
> time. Update so that it is done at resume on J7200 SoC.
> 
> About (2): signal from cdns3-ti to cdns3 host to xHCI that the
> controller resets on resume. This way we avoid the following warning:
> 
>    xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
> 
> Strictly speaking (2) is not required to have working suspend on J7200;
> its only goal is to silence this warning.
> 
> Those patches have been tested on the TI J7200 EVM GP. No need to

Yeah, but you forgot about dtbs_check testing. It's a step before
testing on the device. It does not make sense to test a code if it
causes build-time warnings.

Best regards,
Krzysztof

Re: [PATCH v2 0/7] usb: cdns: fix suspend on J7200 by assuming reset-on-resume

[Kevin Hilman]

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

> Hi,
>
> Suspend on the TI J7200 platform is broken currently. There are two
> components that need to be patched so that they assume reset on
> resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
> controller.
>
> About (1): the TI wrapper only did its hardware configuration at probe
> time. Update so that it is done at resume on J7200 SoC.
>
> About (2): signal from cdns3-ti to cdns3 host to xHCI that the
> controller resets on resume. This way we avoid the following warning:
>
>    xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
>
> Strictly speaking (2) is not required to have working suspend on J7200;
> its only goal is to silence this warning.
>
> Those patches have been tested on the TI J7200 EVM GP. No need to
> mention that other patches are required for S2R to work, but those will
> be sent later down the road. Those USB patches are rather standalone.
>
> Thanks,
> Théo
>
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
> Signed-off-by: Théo Lebrun <theo.lebrun@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.

Sorry I sent comments on v2 before I noticed v3 was out, but this is not
a good idea IMO.  I sent more detailed comment on that specific patch.

Kevin