diff mbox series

[v4,4/7] dt-bindings: usb: ci-hdrc-usb2: add restrictions for reg, interrupts, clock and clock-names properties

Message ID 20240119071936.3481439-4-xu.yang_2@nxp.com
State New
Headers show
Series [v4,1/7] dt-bindings: usb: usbmisc-imx: add fsl,imx8ulp-usbmisc compatible | expand

Commit Message

Xu Yang Jan. 19, 2024, 7:19 a.m. UTC
Change reg, interrupts, clock and clock-names as common properties and add
restrictions on them for different compatibles.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v4:
 - new patch since v3's discussion
 - split the reg, interrupts, clock and clock-names properties into
   common part and device-specific
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
 1 file changed, 102 insertions(+), 16 deletions(-)

Comments

Xu Yang Jan. 31, 2024, 8:24 a.m. UTC | #1
Hi Krzysztof,

> 
> On 19/01/2024 08:19, Xu Yang wrote:
> > Change reg, interrupts, clock and clock-names as common properties and add
> > restrictions on them for different compatibles.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >
> > ---
> > Changes in v4:
> >  - new patch since v3's discussion
> >  - split the reg, interrupts, clock and clock-names properties into
> >    common part and device-specific
> > ---
> >  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
> >  1 file changed, 102 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
> hdrc-usb2.yaml
> > index b7e664f7395b..78e30ca0a8ca 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > @@ -73,22 +73,10 @@ properties:
> >                - nuvoton,npcm845-udc
> >            - const: nuvoton,npcm750-udc
> >
> > -  reg:
> > -    minItems: 1
> > -    maxItems: 2
> > -
> > -  interrupts:
> > -    minItems: 1
> > -    maxItems: 2
> > -
> > -  clocks:
> > -    minItems: 1
> > -    maxItems: 3
> > -
> > -  clock-names:
> > -    minItems: 1
> > -    maxItems: 3
> 
> Why all these are gone? They are supposed to be here. Your if:then: only
> customizes them.

I have also concerns of whether to make this part common.
I will revert this later.

> 
> > -
> > +  reg: true
> > +  interrupts: true
> > +  clocks: true
> > +  clock-names: true
> 
> No. These are not booleans on other variants.

Okay.

> 
> >    dr_mode: true
> >
> >    power-domains:
> > @@ -412,6 +400,104 @@ allOf:
> >          samsung,picophy-pre-emp-curr-control: false
> >          samsung,picophy-dc-vol-level-adjust: false
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          oneOf:
> > +            - items:
> > +                - const: fsl,imx27-usb
> 
> No, the syntax you need is contains:.
> 
> Look at existing code - there is no single binding with oneOf: in if: block.

I wonder why 'make dt_binding_check' does not report this issue if the syntax
is not correct?

So I need to add contains as below, right?

  - if:
      properties:
        compatible:
          contains:
            oneOf:
              - items:
                  - const: fsl,imx27-usb
              - items:
                  - enum:
                      - fsl,imx25-usb
                      - fsl,imx35-usb
                  - const: fsl,imx27-usb

The purpose of this code is to match:

  - compatible = "fsl,imx27-usb";
  - compatible = "fsl,imx25-usb", "fsl,imx27-usb";
  - compatible = "fsl,imx35-usb", "fsl,imx27-usb";

but should not match:

  - compatible = "fsl,imx7d-usb", "fsl,imx27-usb";

Is this feasible?

Thanks,
Xu Yang
Krzysztof Kozlowski Jan. 31, 2024, 8:41 a.m. UTC | #2
On 31/01/2024 09:24, Xu Yang wrote:
> Hi Krzysztof,
> 
>>
>> On 19/01/2024 08:19, Xu Yang wrote:
>>> Change reg, interrupts, clock and clock-names as common properties and add
>>> restrictions on them for different compatibles.
>>>
>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>>
>>> ---
>>> Changes in v4:
>>>  - new patch since v3's discussion
>>>  - split the reg, interrupts, clock and clock-names properties into
>>>    common part and device-specific
>>> ---
>>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
>>>  1 file changed, 102 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
>> hdrc-usb2.yaml
>>> index b7e664f7395b..78e30ca0a8ca 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> @@ -73,22 +73,10 @@ properties:
>>>                - nuvoton,npcm845-udc
>>>            - const: nuvoton,npcm750-udc
>>>
>>> -  reg:
>>> -    minItems: 1
>>> -    maxItems: 2
>>> -
>>> -  interrupts:
>>> -    minItems: 1
>>> -    maxItems: 2
>>> -
>>> -  clocks:
>>> -    minItems: 1
>>> -    maxItems: 3
>>> -
>>> -  clock-names:
>>> -    minItems: 1
>>> -    maxItems: 3
>>
>> Why all these are gone? They are supposed to be here. Your if:then: only
>> customizes them.
> 
> I have also concerns of whether to make this part common.
> I will revert this later.

Revert? No. This patch must be correct.
> 
>>
>>> -
>>> +  reg: true
>>> +  interrupts: true
>>> +  clocks: true
>>> +  clock-names: true
>>
>> No. These are not booleans on other variants.
> 
> Okay.
> 
>>
>>>    dr_mode: true
>>>
>>>    power-domains:
>>> @@ -412,6 +400,104 @@ allOf:
>>>          samsung,picophy-pre-emp-curr-control: false
>>>          samsung,picophy-dc-vol-level-adjust: false
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          oneOf:
>>> +            - items:
>>> +                - const: fsl,imx27-usb
>>
>> No, the syntax you need is contains:.
>>
>> Look at existing code - there is no single binding with oneOf: in if: block.
> 
> I wonder why 'make dt_binding_check' does not report this issue if the syntax
> is not correct?

I did not say syntax is incorrect.


> 
> So I need to add contains as below, right?
> 
>   - if:
>       properties:
>         compatible:
>           contains:
>             oneOf:
>               - items:
>                   - const: fsl,imx27-usb
>               - items:
>                   - enum:
>                       - fsl,imx25-usb
>                       - fsl,imx35-usb
>                   - const: fsl,imx27-usb
> 
> The purpose of this code is to match:
> 
>   - compatible = "fsl,imx27-usb";
>   - compatible = "fsl,imx25-usb", "fsl,imx27-usb";
>   - compatible = "fsl,imx35-usb", "fsl,imx27-usb";
> 
> but should not match:
> 
>   - compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> 
> Is this feasible?

So maybe they are not compatible? Your patch creates some unusual
constraints for all the variants, which is probably result of huge one
binding for all implementations of re-used IP block. I don't think that
this huge if: you add here and further in the patch helps. Just like for
other re-used IP blocks, this should have common part and
per-device/per-family/per-implementation binding.

Best regards,
Krzysztof
Xu Yang Jan. 31, 2024, 9:02 a.m. UTC | #3
Hi Krzysztof,

> 
> On 31/01/2024 09:24, Xu Yang wrote:
> > Hi Krzysztof,
> >
> >>
> >> On 19/01/2024 08:19, Xu Yang wrote:
> >>> Change reg, interrupts, clock and clock-names as common properties and add
> >>> restrictions on them for different compatibles.
> >>>
> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>>
> >>> ---
> >>> Changes in v4:
> >>>  - new patch since v3's discussion
> >>>  - split the reg, interrupts, clock and clock-names properties into
> >>>    common part and device-specific
> >>> ---
> >>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
> >>>  1 file changed, 102 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
> >> hdrc-usb2.yaml
> >>> index b7e664f7395b..78e30ca0a8ca 100644
> >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> @@ -73,22 +73,10 @@ properties:
> >>>                - nuvoton,npcm845-udc
> >>>            - const: nuvoton,npcm750-udc
> >>>
> >>> -  reg:
> >>> -    minItems: 1
> >>> -    maxItems: 2
> >>> -
> >>> -  interrupts:
> >>> -    minItems: 1
> >>> -    maxItems: 2
> >>> -
> >>> -  clocks:
> >>> -    minItems: 1
> >>> -    maxItems: 3
> >>> -
> >>> -  clock-names:
> >>> -    minItems: 1
> >>> -    maxItems: 3
> >>
> >> Why all these are gone? They are supposed to be here. Your if:then: only
> >> customizes them.
> >
> > I have also concerns of whether to make this part common.
> > I will revert this later.
> 
> Revert? No. This patch must be correct.

I mean only this part keeps unchanged like before.

> >
> >>
> >>> -
> >>> +  reg: true
> >>> +  interrupts: true
> >>> +  clocks: true
> >>> +  clock-names: true
> >>
> >> No. These are not booleans on other variants.
> >
> > Okay.
> >
> >>
> >>>    dr_mode: true
> >>>
> >>>    power-domains:
> >>> @@ -412,6 +400,104 @@ allOf:
> >>>          samsung,picophy-pre-emp-curr-control: false
> >>>          samsung,picophy-dc-vol-level-adjust: false
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          oneOf:
> >>> +            - items:
> >>> +                - const: fsl,imx27-usb
> >>
> >> No, the syntax you need is contains:.
> >>
> >> Look at existing code - there is no single binding with oneOf: in if: block.
> >
> > I wonder why 'make dt_binding_check' does not report this issue if the syntax
> > is not correct?
> 
> I did not say syntax is incorrect.
> 
> 
> >
> > So I need to add contains as below, right?
> >
> >   - if:
> >       properties:
> >         compatible:
> >           contains:
> >             oneOf:
> >               - items:
> >                   - const: fsl,imx27-usb
> >               - items:
> >                   - enum:
> >                       - fsl,imx25-usb
> >                       - fsl,imx35-usb
> >                   - const: fsl,imx27-usb
> >
> > The purpose of this code is to match:
> >
> >   - compatible = "fsl,imx27-usb";
> >   - compatible = "fsl,imx25-usb", "fsl,imx27-usb";
> >   - compatible = "fsl,imx35-usb", "fsl,imx27-usb";
> >
> > but should not match:
> >
> >   - compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> >
> > Is this feasible?
> 
> So maybe they are not compatible? Your patch creates some unusual

Yes, they are not fully compatible.

> constraints for all the variants, which is probably result of huge one
> binding for all implementations of re-used IP block. I don't think that
> this huge if: you add here and further in the patch helps. Just like for
> other re-used IP blocks, this should have common part and
> per-device/per-family/per-implementation binding.

Actually I've tested all dts files (not only imx parts) against this dt-binding yaml.

Then I'll rework this patch to focus on imx parts. I'm not sure if someone will add
restrictions for their family/device in the future.

Thanks,
Xu Yang
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
index b7e664f7395b..78e30ca0a8ca 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
@@ -73,22 +73,10 @@  properties:
               - nuvoton,npcm845-udc
           - const: nuvoton,npcm750-udc
 
-  reg:
-    minItems: 1
-    maxItems: 2
-
-  interrupts:
-    minItems: 1
-    maxItems: 2
-
-  clocks:
-    minItems: 1
-    maxItems: 3
-
-  clock-names:
-    minItems: 1
-    maxItems: 3
-
+  reg: true
+  interrupts: true
+  clocks: true
+  clock-names: true
   dr_mode: true
 
   power-domains:
@@ -412,6 +400,104 @@  allOf:
         samsung,picophy-pre-emp-curr-control: false
         samsung,picophy-dc-vol-level-adjust: false
 
+  - if:
+      properties:
+        compatible:
+          oneOf:
+            - items:
+                - const: fsl,imx27-usb
+            - items:
+                - enum:
+                    - fsl,imx25-usb
+                    - fsl,imx35-usb
+                - const: fsl,imx27-usb
+    then:
+      properties:
+        clocks:
+          minItems: 3
+          maxItems: 3
+        clock-names:
+          minItems: 3
+          maxItems: 3
+          items:
+            anyOf:
+              - const: ipg
+              - const: ahb
+              - const: per
+
+  - if:
+      properties:
+        compatible:
+          oneOf:
+            - items:
+                - const: qcom,ci-hdrc
+    then:
+      properties:
+        reg:
+          minItems: 2
+          maxItems: 2
+        interrupts:
+          minItems: 1
+          maxItems: 2
+        clocks:
+          minItems: 2
+          maxItems: 3
+        clock-names:
+          minItems: 2
+          maxItems: 3
+          items:
+            anyOf:
+              - const: core
+              - const: iface
+              - const: fs
+                description: optional
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            oneOf:
+              - const: chipidea,usb2
+              - const: fsl,imx23-usb
+              - const: fsl,imx28-usb
+              - const: fsl,imx7d-usb
+              - const: fsl,vf610-usb
+              - const: lsi,zevio-usb
+              - const: nuvoton,npcm750-udc
+              - pattern: '^fsl,imx5[0-3]+-usb$'
+              - pattern: '^fsl,imx6[a-z]+-usb$'
+              - pattern: '^nvidia,tegra[0-9]+-ehci$'
+              - pattern: '^nvidia,tegra[0-9]+-udc$'
+    then:
+      properties:
+        clocks:
+          minItems: 1
+          maxItems: 1
+        clock-names:
+          minItems: 1
+          maxItems: 1
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            oneOf:
+              - const: chipidea,usb2
+              - const: fsl,imx27-usb
+              - const: fsl,imx6ul-usb
+              - const: lsi,zevio-usb
+              - const: nuvoton,npcm750-udc
+              - pattern: '^nvidia,tegra[0-9]+-ehci$'
+              - pattern: '^nvidia,tegra[0-9]+-udc$'
+    then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 1
+        interrupts:
+          minItems: 1
+          maxItems: 1
+
 unevaluatedProperties: false
 
 examples: