diff mbox series

[v3,4/6] dt-bindings: usb: ci-hdrc-usb2: add compatible and clock-names restriction for imx93

Message ID 20240112111747.1250915-4-xu.yang_2@nxp.com
State New
Headers show
Series None | expand

Commit Message

Xu Yang Jan. 12, 2024, 11:17 a.m. UTC
The i.MX93 needs a wakup clock to work properly. This will add compatible
and restriction for i.MX93 platform.

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

---
Changes in v2:
 - no changes
Changes in v3:
 - add clocks restriction
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Alexander Stein Jan. 15, 2024, 10:29 a.m. UTC | #1
Hi,

Am Freitag, 12. Januar 2024, 12:17:45 CET schrieb Xu Yang:
> The i.MX93 needs a wakup clock to work properly. This will add compatible
> and restriction for i.MX93 platform.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Looks good to me:
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> 
> ---
> Changes in v2:
>  - no changes
> Changes in v3:
>  - add clocks restriction
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml index
> b7e664f7395b..6e75099b6394 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> @@ -57,6 +57,7 @@ properties:
>            - enum:
>                - fsl,imx8mm-usb
>                - fsl,imx8mn-usb
> +              - fsl,imx93-usb
>            - const: fsl,imx7d-usb
>            - const: fsl,imx27-usb
>        - items:
> @@ -412,6 +413,21 @@ allOf:
>          samsung,picophy-pre-emp-curr-control: false
>          samsung,picophy-dc-vol-level-adjust: false
> 
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx93-usb
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: usb_ctrl_root_clk
> +            - const: usb_wakeup_clk
> +        clocks:
> +          minItems: 2
> +          maxItems: 2
> +
>  unevaluatedProperties: false
> 
>  examples:
Krzysztof Kozlowski Jan. 15, 2024, 11:01 a.m. UTC | #2
On 12/01/2024 12:17, Xu Yang wrote:
> The i.MX93 needs a wakup clock to work properly. This will add compatible
> and restriction for i.MX93 platform.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> ---
> Changes in v2:
>  - no changes
> Changes in v3:
>  - add clocks restriction
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> index b7e664f7395b..6e75099b6394 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> @@ -57,6 +57,7 @@ properties:
>            - enum:
>                - fsl,imx8mm-usb
>                - fsl,imx8mn-usb
> +              - fsl,imx93-usb
>            - const: fsl,imx7d-usb
>            - const: fsl,imx27-usb
>        - items:
> @@ -412,6 +413,21 @@ allOf:
>          samsung,picophy-pre-emp-curr-control: false
>          samsung,picophy-dc-vol-level-adjust: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx93-usb
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: usb_ctrl_root_clk
> +            - const: usb_wakeup_clk
> +        clocks:
> +          minItems: 2
> +          maxItems: 2

Nothing improved regarding my comments. Why do you allow
any/unspecific/unconstrain interrupts and reg?

You said:
"However, reset, reg and interrupts property is not special for imx93."
but what does it even mean? How they can be special or not special?

My comments from previous version apply. If you do not want to work on
existing technical debt, BTW added by another NXP employee, then I will
NAK any new submissions.

Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 16, 2024, 7:52 a.m. UTC | #3
On 16/01/2024 08:49, Xu Yang wrote:
> Hi Krzysztof,
> 
>>
>> On 12/01/2024 12:17, Xu Yang wrote:
>>> The i.MX93 needs a wakup clock to work properly. This will add compatible
>>> and restriction for i.MX93 platform.
>>>
>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>>
>>> ---
>>> Changes in v2:
>>>  - no changes
>>> Changes in v3:
>>>  - add clocks restriction
>>> ---
>>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
>> hdrc-usb2.yaml
>>> index b7e664f7395b..6e75099b6394 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> @@ -57,6 +57,7 @@ properties:
>>>            - enum:
>>>                - fsl,imx8mm-usb
>>>                - fsl,imx8mn-usb
>>> +              - fsl,imx93-usb
>>>            - const: fsl,imx7d-usb
>>>            - const: fsl,imx27-usb
>>>        - items:
>>> @@ -412,6 +413,21 @@ allOf:
>>>          samsung,picophy-pre-emp-curr-control: false
>>>          samsung,picophy-dc-vol-level-adjust: false
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: fsl,imx93-usb
>>> +    then:
>>> +      properties:
>>> +        clock-names:
>>> +          items:
>>> +            - const: usb_ctrl_root_clk
>>> +            - const: usb_wakeup_clk
>>> +        clocks:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> Nothing improved regarding my comments. Why do you allow
>> any/unspecific/unconstrain interrupts and reg?
>>
>> You said:
>> "However, reset, reg and interrupts property is not special for imx93."
>> but what does it even mean? How they can be special or not special?
>>
>> My comments from previous version apply. If you do not want to work on
>> existing technical debt, BTW added by another NXP employee, then I will
>> NAK any new submissions.
> 
> You want me to adjust below properties to be more common properties
> and add device specific limitations, right?

Yes

> 
> ---
>   reg:
>     minItems: 1
>     maxItems: 2
> 
>   interrupts:
>     minItems: 1
>     maxItems: 2
> 
>   clocks:
>     minItems: 1
>     maxItems: 3
> 
>   clock-names:
>     minItems: 1
>     maxItems: 3
> ---
> 
> For most of the devices, property reg, interrupts, clocks and clock-names
> has 1 item. So these properties can set maxItems to 1. For special devices,
> I should list them standalone, is this your expectation?

Just like you constrain clocks for new variant, your variant should have
constrained reg, interrupts and whatever else is there variable. I don't
require fixing all the devices in this binding, but at least your new
one and preferably other NXP as well.


Best regards,
Krzysztof
Xu Yang Jan. 16, 2024, 7:59 a.m. UTC | #4
Hi Krzysztof,

> 
> On 16/01/2024 08:49, Xu Yang wrote:
> > Hi Krzysztof,
> >
> >>
> >> On 12/01/2024 12:17, Xu Yang wrote:
> >>> The i.MX93 needs a wakup clock to work properly. This will add compatible
> >>> and restriction for i.MX93 platform.
> >>>
> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>  - no changes
> >>> Changes in v3:
> >>>  - add clocks restriction
> >>> ---
> >>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
> >> hdrc-usb2.yaml
> >>> index b7e664f7395b..6e75099b6394 100644
> >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> @@ -57,6 +57,7 @@ properties:
> >>>            - enum:
> >>>                - fsl,imx8mm-usb
> >>>                - fsl,imx8mn-usb
> >>> +              - fsl,imx93-usb
> >>>            - const: fsl,imx7d-usb
> >>>            - const: fsl,imx27-usb
> >>>        - items:
> >>> @@ -412,6 +413,21 @@ allOf:
> >>>          samsung,picophy-pre-emp-curr-control: false
> >>>          samsung,picophy-dc-vol-level-adjust: false
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: fsl,imx93-usb
> >>> +    then:
> >>> +      properties:
> >>> +        clock-names:
> >>> +          items:
> >>> +            - const: usb_ctrl_root_clk
> >>> +            - const: usb_wakeup_clk
> >>> +        clocks:
> >>> +          minItems: 2
> >>> +          maxItems: 2
> >>
> >> Nothing improved regarding my comments. Why do you allow
> >> any/unspecific/unconstrain interrupts and reg?
> >>
> >> You said:
> >> "However, reset, reg and interrupts property is not special for imx93."
> >> but what does it even mean? How they can be special or not special?
> >>
> >> My comments from previous version apply. If you do not want to work on
> >> existing technical debt, BTW added by another NXP employee, then I will
> >> NAK any new submissions.
> >
> > You want me to adjust below properties to be more common properties
> > and add device specific limitations, right?
> 
> Yes
> 
> >
> > ---
> >   reg:
> >     minItems: 1
> >     maxItems: 2
> >
> >   interrupts:
> >     minItems: 1
> >     maxItems: 2
> >
> >   clocks:
> >     minItems: 1
> >     maxItems: 3
> >
> >   clock-names:
> >     minItems: 1
> >     maxItems: 3
> > ---
> >
> > For most of the devices, property reg, interrupts, clocks and clock-names
> > has 1 item. So these properties can set maxItems to 1. For special devices,
> > I should list them standalone, is this your expectation?
> 
> Just like you constrain clocks for new variant, your variant should have
> constrained reg, interrupts and whatever else is there variable. I don't
> require fixing all the devices in this binding, but at least your new
> one and preferably other NXP as well.

Okay. I will do this and test it.

Thanks,
Xu Yang
Xu Yang Jan. 17, 2024, 5:46 a.m. UTC | #5
Hi Krzysztof,

> 
> On 16/01/2024 08:49, Xu Yang wrote:
> > Hi Krzysztof,
> >
> >>
> >> On 12/01/2024 12:17, Xu Yang wrote:
> >>> The i.MX93 needs a wakup clock to work properly. This will add compatible
> >>> and restriction for i.MX93 platform.
> >>>
> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>  - no changes
> >>> Changes in v3:
> >>>  - add clocks restriction
> >>> ---
> >>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
> >> hdrc-usb2.yaml
> >>> index b7e664f7395b..6e75099b6394 100644
> >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> >>> @@ -57,6 +57,7 @@ properties:
> >>>            - enum:
> >>>                - fsl,imx8mm-usb
> >>>                - fsl,imx8mn-usb
> >>> +              - fsl,imx93-usb
> >>>            - const: fsl,imx7d-usb
> >>>            - const: fsl,imx27-usb
> >>>        - items:
> >>> @@ -412,6 +413,21 @@ allOf:
> >>>          samsung,picophy-pre-emp-curr-control: false
> >>>          samsung,picophy-dc-vol-level-adjust: false
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: fsl,imx93-usb
> >>> +    then:
> >>> +      properties:
> >>> +        clock-names:
> >>> +          items:
> >>> +            - const: usb_ctrl_root_clk
> >>> +            - const: usb_wakeup_clk
> >>> +        clocks:
> >>> +          minItems: 2
> >>> +          maxItems: 2
> >>
> >> Nothing improved regarding my comments. Why do you allow
> >> any/unspecific/unconstrain interrupts and reg?
> >>
> >> You said:
> >> "However, reset, reg and interrupts property is not special for imx93."
> >> but what does it even mean? How they can be special or not special?
> >>
> >> My comments from previous version apply. If you do not want to work on
> >> existing technical debt, BTW added by another NXP employee, then I will
> >> NAK any new submissions.
> >
> > You want me to adjust below properties to be more common properties
> > and add device specific limitations, right?
> 
> Yes
> 
> >
> > ---
> >   reg:
> >     minItems: 1
> >     maxItems: 2
> >
> >   interrupts:
> >     minItems: 1
> >     maxItems: 2
> >
> >   clocks:
> >     minItems: 1
> >     maxItems: 3
> >
> >   clock-names:
> >     minItems: 1
> >     maxItems: 3
> > ---
> >
> > For most of the devices, property reg, interrupts, clocks and clock-names
> > has 1 item. So these properties can set maxItems to 1. For special devices,
> > I should list them standalone, is this your expectation?
> 
> Just like you constrain clocks for new variant, your variant should have
> constrained reg, interrupts and whatever else is there variable. I don't
> require fixing all the devices in this binding, but at least your new
> one and preferably other NXP as well.
> 

I'm tring to set the maxItems of property reg, interrupts, clocks and 
clock-names to 1, then constrain the minItems and maxItems to 3 for
one soc later like below, in such way I needn't to constrain reg and
interrupts for most of the socs.
But dt-validate failed at the first place when validate clocks property.

Is there a way to achieve this?

---
  reg:
    maxItems: 1

  interrupts:
    maxItems: 1

  clocks:
    maxItems: 1

  clock-names:
    maxItems: 1

allOf:
  - if:
      properties:
        compatible:
          oneOf:
            - items:
                - const: fsl,imx27-usb
    then:
      properties:
        clocks:
          minItems: 3
          maxItems: 3
        clock-names:
          minItems: 3
          maxItems: 3
---

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..6e75099b6394 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
@@ -57,6 +57,7 @@  properties:
           - enum:
               - fsl,imx8mm-usb
               - fsl,imx8mn-usb
+              - fsl,imx93-usb
           - const: fsl,imx7d-usb
           - const: fsl,imx27-usb
       - items:
@@ -412,6 +413,21 @@  allOf:
         samsung,picophy-pre-emp-curr-control: false
         samsung,picophy-dc-vol-level-adjust: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx93-usb
+    then:
+      properties:
+        clock-names:
+          items:
+            - const: usb_ctrl_root_clk
+            - const: usb_wakeup_clk
+        clocks:
+          minItems: 2
+          maxItems: 2
+
 unevaluatedProperties: false
 
 examples: