diff mbox series

[v3,12/14] dt-bindings: mtd: gpmi-nand: Fix matching of clocks on different SoCs

Message ID 20200904152404.20636-13-krzk@kernel.org
State New
Headers show
Series dt-bindings: Cleanup of i.MX 8 | expand

Commit Message

Krzysztof Kozlowski Sept. 4, 2020, 3:24 p.m. UTC
Driver requires different amount of clocks for different SoCs.  Describe
these requirements properly to fix dtbs_check warnings like:

    arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v1:
1. Do not require order of clocks (use pattern).
---
 .../devicetree/bindings/mtd/gpmi-nand.yaml    | 76 +++++++++++++++----
 1 file changed, 61 insertions(+), 15 deletions(-)

Comments

Rob Herring Sept. 4, 2020, 10:36 p.m. UTC | #1
On Fri, Sep 4, 2020 at 9:25 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Driver requires different amount of clocks for different SoCs.  Describe
> these requirements properly to fix dtbs_check warnings like:
>
>     arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Changes since v1:
> 1. Do not require order of clocks (use pattern).

To the extent that you can, you should fix the order in dts files
first. If we just adjust the schemas to match the dts files, then
what's the point?

> ---
>  .../devicetree/bindings/mtd/gpmi-nand.yaml    | 76 +++++++++++++++----
>  1 file changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> index 28ff8c581837..e08e0a50929e 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> @@ -9,9 +9,6 @@ title: Freescale General-Purpose Media Interface (GPMI) binding
>  maintainers:
>    - Han Xu <han.xu@nxp.com>
>
> -allOf:
> -  - $ref: "nand-controller.yaml"
> -
>  description: |
>    The GPMI nand controller provides an interface to control the NAND
>    flash chips. The device tree may optionally contain sub-nodes
> @@ -58,22 +55,10 @@ properties:
>    clocks:
>      minItems: 1
>      maxItems: 5
> -    items:
> -      - description: SoC gpmi io clock
> -      - description: SoC gpmi apb clock
> -      - description: SoC gpmi bch clock
> -      - description: SoC gpmi bch apb clock
> -      - description: SoC per1 bch clock
>
>    clock-names:
>      minItems: 1
>      maxItems: 5
> -    items:
> -      - const: gpmi_io
> -      - const: gpmi_apb
> -      - const: gpmi_bch
> -      - const: gpmi_bch_apb
> -      - const: per1_bch
>
>    fsl,use-minimum-ecc:
>      type: boolean
> @@ -107,6 +92,67 @@ required:
>
>  unevaluatedProperties: false
>
> +allOf:
> +  - $ref: "nand-controller.yaml"
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx23-gpmi-nand
> +              - fsl,imx28-gpmi-nand
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: SoC gpmi io clock
> +        clock-names:
> +          items:
> +            - const: gpmi_io
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx6q-gpmi-nand
> +              - fsl,imx6sx-gpmi-nand
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: SoC gpmi io clock
> +            - description: SoC gpmi apb clock
> +            - description: SoC gpmi bch clock
> +            - description: SoC gpmi bch apb clock
> +            - description: SoC per1 bch clock
> +        clock-names:
> +          items:
> +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"

BTW, you can make 'items' a schema rather than a list to apply a
constraint to all entries:

maxItems: 5
items:
  pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,imx7d-gpmi-nand
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: SoC gpmi io clock
> +            - description: SoC gpmi bch apb clock
> +        clock-names:
> +          minItems: 2
> +          maxItems: 2

You can drop these. It's the default based on the size of 'items'.

> +          items:
> +            - pattern: "^gpmi_(io|bch_apb)$"
> +            - pattern: "^gpmi_(io|bch_apb)$"

Surely here we can define the order.

> +
>  examples:
>    - |
>      nand-controller@8000c000 {
> --
> 2.17.1
>
Krzysztof Kozlowski Sept. 7, 2020, 6:09 a.m. UTC | #2
On Fri, Sep 04, 2020 at 04:36:39PM -0600, Rob Herring wrote:
> On Fri, Sep 4, 2020 at 9:25 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Driver requires different amount of clocks for different SoCs.  Describe
> > these requirements properly to fix dtbs_check warnings like:
> >
> >     arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >
> > ---
> >
> > Changes since v1:
> > 1. Do not require order of clocks (use pattern).
> 
> To the extent that you can, you should fix the order in dts files
> first. If we just adjust the schemas to match the dts files, then
> what's the point?

The DTSes do not have mixed order of clocks between each other, as fair
as I remember. It was fix after Sasha Hauer comment that order is not
necessarily good.

We have the clock-names property, why enforcing the order?

> 
> > ---
> >  .../devicetree/bindings/mtd/gpmi-nand.yaml    | 76 +++++++++++++++----
> >  1 file changed, 61 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> > index 28ff8c581837..e08e0a50929e 100644
> > --- a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
> > @@ -9,9 +9,6 @@ title: Freescale General-Purpose Media Interface (GPMI) binding
> >  maintainers:
> >    - Han Xu <han.xu@nxp.com>
> >
> > -allOf:
> > -  - $ref: "nand-controller.yaml"
> > -
> >  description: |
> >    The GPMI nand controller provides an interface to control the NAND
> >    flash chips. The device tree may optionally contain sub-nodes
> > @@ -58,22 +55,10 @@ properties:
> >    clocks:
> >      minItems: 1
> >      maxItems: 5
> > -    items:
> > -      - description: SoC gpmi io clock
> > -      - description: SoC gpmi apb clock
> > -      - description: SoC gpmi bch clock
> > -      - description: SoC gpmi bch apb clock
> > -      - description: SoC per1 bch clock
> >
> >    clock-names:
> >      minItems: 1
> >      maxItems: 5
> > -    items:
> > -      - const: gpmi_io
> > -      - const: gpmi_apb
> > -      - const: gpmi_bch
> > -      - const: gpmi_bch_apb
> > -      - const: per1_bch
> >
> >    fsl,use-minimum-ecc:
> >      type: boolean
> > @@ -107,6 +92,67 @@ required:
> >
> >  unevaluatedProperties: false
> >
> > +allOf:
> > +  - $ref: "nand-controller.yaml"
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - fsl,imx23-gpmi-nand
> > +              - fsl,imx28-gpmi-nand
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: SoC gpmi io clock
> > +        clock-names:
> > +          items:
> > +            - const: gpmi_io
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - fsl,imx6q-gpmi-nand
> > +              - fsl,imx6sx-gpmi-nand
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: SoC gpmi io clock
> > +            - description: SoC gpmi apb clock
> > +            - description: SoC gpmi bch clock
> > +            - description: SoC gpmi bch apb clock
> > +            - description: SoC per1 bch clock
> > +        clock-names:
> > +          items:
> > +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> > +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> > +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> > +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> > +            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
> 
> BTW, you can make 'items' a schema rather than a list to apply a
> constraint to all entries:
> 
> maxItems: 5
> items:
>   pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"

Right, I forgot about such syntax.

> 
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,imx7d-gpmi-nand
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: SoC gpmi io clock
> > +            - description: SoC gpmi bch apb clock
> > +        clock-names:
> > +          minItems: 2
> > +          maxItems: 2
> 
> You can drop these. It's the default based on the size of 'items'.

Sure.

> 
> > +          items:
> > +            - pattern: "^gpmi_(io|bch_apb)$"
> > +            - pattern: "^gpmi_(io|bch_apb)$"
> 
> Surely here we can define the order.

Yes, but still the same question as before - do we want the order? Why
enforcing it?

Best regards,
Krzysztof
Rob Herring Sept. 8, 2020, 4:50 p.m. UTC | #3
On Mon, Sep 7, 2020 at 12:10 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Sep 04, 2020 at 04:36:39PM -0600, Rob Herring wrote:
> > On Fri, Sep 4, 2020 at 9:25 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > Driver requires different amount of clocks for different SoCs.  Describe
> > > these requirements properly to fix dtbs_check warnings like:
> > >
> > >     arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
> > >
> > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > >
> > > ---
> > >
> > > Changes since v1:
> > > 1. Do not require order of clocks (use pattern).
> >
> > To the extent that you can, you should fix the order in dts files
> > first. If we just adjust the schemas to match the dts files, then
> > what's the point?
>
> The DTSes do not have mixed order of clocks between each other, as fair
> as I remember. It was fix after Sasha Hauer comment that order is not
> necessarily good.
>
> We have the clock-names property, why enforcing the order?

Because DT/OpenFirmware has always had a defined order for property
values. '*-names' is just extra information.

Rob
Krzysztof Kozlowski Sept. 10, 2020, 6:34 p.m. UTC | #4
On Tue, 8 Sep 2020 at 18:51, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Sep 7, 2020 at 12:10 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Fri, Sep 04, 2020 at 04:36:39PM -0600, Rob Herring wrote:
> > > On Fri, Sep 4, 2020 at 9:25 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > Driver requires different amount of clocks for different SoCs.  Describe
> > > > these requirements properly to fix dtbs_check warnings like:
> > > >
> > > >     arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dt.yaml: nand-controller@33002000: clock-names:1: 'gpmi_apb' was expected
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > >
> > > > ---
> > > >
> > > > Changes since v1:
> > > > 1. Do not require order of clocks (use pattern).
> > >
> > > To the extent that you can, you should fix the order in dts files
> > > first. If we just adjust the schemas to match the dts files, then
> > > what's the point?
> >
> > The DTSes do not have mixed order of clocks between each other, as fair
> > as I remember. It was fix after Sasha Hauer comment that order is not
> > necessarily good.
> >
> > We have the clock-names property, why enforcing the order?
>
> Because DT/OpenFirmware has always had a defined order for property
> values. '*-names' is just extra information.

Thanks for the explanation. There are few nonobvious requirements
about writing schema which seems many (including me) miss. It might be
a good topic for some conference. Too bad ELCE CFP ended some time
ago. :)

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
index 28ff8c581837..e08e0a50929e 100644
--- a/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.yaml
@@ -9,9 +9,6 @@  title: Freescale General-Purpose Media Interface (GPMI) binding
 maintainers:
   - Han Xu <han.xu@nxp.com>
 
-allOf:
-  - $ref: "nand-controller.yaml"
-
 description: |
   The GPMI nand controller provides an interface to control the NAND
   flash chips. The device tree may optionally contain sub-nodes
@@ -58,22 +55,10 @@  properties:
   clocks:
     minItems: 1
     maxItems: 5
-    items:
-      - description: SoC gpmi io clock
-      - description: SoC gpmi apb clock
-      - description: SoC gpmi bch clock
-      - description: SoC gpmi bch apb clock
-      - description: SoC per1 bch clock
 
   clock-names:
     minItems: 1
     maxItems: 5
-    items:
-      - const: gpmi_io
-      - const: gpmi_apb
-      - const: gpmi_bch
-      - const: gpmi_bch_apb
-      - const: per1_bch
 
   fsl,use-minimum-ecc:
     type: boolean
@@ -107,6 +92,67 @@  required:
 
 unevaluatedProperties: false
 
+allOf:
+  - $ref: "nand-controller.yaml"
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx23-gpmi-nand
+              - fsl,imx28-gpmi-nand
+    then:
+      properties:
+        clocks:
+          items:
+            - description: SoC gpmi io clock
+        clock-names:
+          items:
+            - const: gpmi_io
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx6q-gpmi-nand
+              - fsl,imx6sx-gpmi-nand
+    then:
+      properties:
+        clocks:
+          items:
+            - description: SoC gpmi io clock
+            - description: SoC gpmi apb clock
+            - description: SoC gpmi bch clock
+            - description: SoC gpmi bch apb clock
+            - description: SoC per1 bch clock
+        clock-names:
+          items:
+            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
+            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
+            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
+            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
+            - pattern: "^(gpmi_(io|apb|bch|bch_apb)|per1_bch)$"
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx7d-gpmi-nand
+    then:
+      properties:
+        clocks:
+          items:
+            - description: SoC gpmi io clock
+            - description: SoC gpmi bch apb clock
+        clock-names:
+          minItems: 2
+          maxItems: 2
+          items:
+            - pattern: "^gpmi_(io|bch_apb)$"
+            - pattern: "^gpmi_(io|bch_apb)$"
+
 examples:
   - |
     nand-controller@8000c000 {