diff mbox series

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

Message ID 20231120-j7200-usb-suspend-v2-1-038c7e4a3df4@bootlin.com
State New
Headers show
Series usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand

Commit Message

Théo Lebrun Nov. 20, 2023, 5:06 p.m. UTC 
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(+)

Comments

Théo Lebrun Nov. 21, 2023, 4:53 p.m. UTC | #1 
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
Krzysztof Kozlowski Nov. 21, 2023, 5:11 p.m. UTC | #2 
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
Théo Lebrun Nov. 22, 2023, 10:46 a.m. UTC | #3 
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
Krzysztof Kozlowski Nov. 22, 2023, noon UTC | #4 
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
Théo Lebrun Nov. 22, 2023, 5:14 p.m. UTC | #5 
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
diff mbox series

Patch

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