| 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 |
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
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
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 --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:
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(-)