diff mbox series

[1/3] dt-bindings: mtd: rzn1: Describe Renesas RZ/N1 NAND controller

Message ID 20211118111940.1275351-2-miquel.raynal@bootlin.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Renesas RZ/N1 NAND controller support | expand

Commit Message

Miquel Raynal Nov. 18, 2021, 11:19 a.m. UTC
Add a Yaml description for this Renesas NAND controller bindings.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../renesas,r9a06g032-nand-controller.yaml    | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml

Comments

Geert Uytterhoeven Nov. 19, 2021, 8:41 a.m. UTC | #1
Hi Miquel,

CC Gareth

On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Add a Yaml description for this Renesas NAND controller bindings.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/N1x NAND flash controller device tree bindings
> +
> +maintainers:
> +  - Miquel Raynal <miquel.raynal@bootlin.com>
> +
> +allOf:
> +  - $ref: "nand-controller.yaml"
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a06g032-nand-controller

As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
and RZ/N1L, I think you should add a family-specific compatible value
"renesas,rzn1-nand-controller" as a fallback.

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    nand-controller@40102000 {
> +        compatible = "renesas,r9a06g032-nand-controller";
> +        reg = <0x40102000 0x2000>;
> +        interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&hclk_nand>, <&clk_nand>;

This clocks property is not based on an actual .dtsi, right?

> +        clock-names = "hclk", "eclk";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +    };

The rest looks good to me.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven Nov. 19, 2021, 8:43 a.m. UTC | #2
On Fri, Nov 19, 2021 at 9:41 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Add a Yaml description for this Renesas NAND controller bindings.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/N1x NAND flash controller device tree bindings
> > +
> > +maintainers:
> > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > +
> > +allOf:
> > +  - $ref: "nand-controller.yaml"
> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a06g032-nand-controller
>
> As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
> and RZ/N1L, I think you should add a family-specific compatible value
> "renesas,rzn1-nand-controller" as a fallback.

And please rename the file to renesas,rzn1-nand-controller.yaml.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Miquel Raynal Nov. 19, 2021, 9:19 a.m. UTC | #3
Hi Geert,

geert@linux-m68k.org wrote on Fri, 19 Nov 2021 09:41:35 +0100:

> Hi Miquel,
> 
> CC Gareth
> 
> On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Add a Yaml description for this Renesas NAND controller bindings.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/N1x NAND flash controller device tree bindings
> > +
> > +maintainers:
> > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > +
> > +allOf:
> > +  - $ref: "nand-controller.yaml"
> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a06g032-nand-controller  
> 
> As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
> and RZ/N1L, I think you should add a family-specific compatible value
> "renesas,rzn1-nand-controller" as a fallback.

I see, that's right, I should have added two compatibles.

As there is currently only one 'specific' compatible (r9axxx), should I
describe the two compatibles as being mandatory? Or should I set the
most specific one as optional and the least specific one (rzn1)
mandatory?

I'll then rename the yaml file, the MAINTAINERS entry and the
compatible in the driver of course.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    nand-controller@40102000 {
> > +        compatible = "renesas,r9a06g032-nand-controller";
> > +        reg = <0x40102000 0x2000>;
> > +        interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&hclk_nand>, <&clk_nand>;  
> 
> This clocks property is not based on an actual .dtsi, right?

It's not indeed. As said in the cover letter I am going to work on the
clock tree (nov-dec 2021) because I would like to have this driver fully
working on a mainline base. So far I used a mixed vendor/upstream DT
just to have access to the clocks and focus on the 'real' feature but
now that it is working I am going to switch on the clocks side
(hopefully with your support :) ).

> 
> > +        clock-names = "hclk", "eclk";
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +    };  
> 
> The rest looks good to me.

Thanks for the review!

Thanks,
Miquèl
Geert Uytterhoeven Nov. 19, 2021, 9:36 a.m. UTC | #4
Hi Miquel,

On Fri, Nov 19, 2021 at 10:19 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Fri, 19 Nov 2021 09:41:35 +0100:
> > On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > > Add a Yaml description for this Renesas NAND controller bindings.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> > > @@ -0,0 +1,60 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Renesas RZ/N1x NAND flash controller device tree bindings
> > > +
> > > +maintainers:
> > > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > > +
> > > +allOf:
> > > +  - $ref: "nand-controller.yaml"
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: renesas,r9a06g032-nand-controller
> >
> > As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
> > and RZ/N1L, I think you should add a family-specific compatible value
> > "renesas,rzn1-nand-controller" as a fallback.
>
> I see, that's right, I should have added two compatibles.
>
> As there is currently only one 'specific' compatible (r9axxx), should I
> describe the two compatibles as being mandatory? Or should I set the
> most specific one as optional and the least specific one (rzn1)
> mandatory?

Yes please.  Else you need to match on both in the driver, or we cannot
differentiate later if the need ever arises.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Miquel Raynal Nov. 26, 2021, 11:46 a.m. UTC | #5
Hi Geert,

geert@linux-m68k.org wrote on Fri, 19 Nov 2021 10:36:16 +0100:

> Hi Miquel,
> 
> On Fri, Nov 19, 2021 at 10:19 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > geert@linux-m68k.org wrote on Fri, 19 Nov 2021 09:41:35 +0100:  
> > > On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > > Add a Yaml description for this Renesas NAND controller bindings.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> > > > @@ -0,0 +1,60 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Renesas RZ/N1x NAND flash controller device tree bindings
> > > > +
> > > > +maintainers:
> > > > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > > > +
> > > > +allOf:
> > > > +  - $ref: "nand-controller.yaml"
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: renesas,r9a06g032-nand-controller  
> > >
> > > As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
> > > and RZ/N1L, I think you should add a family-specific compatible value
> > > "renesas,rzn1-nand-controller" as a fallback.  
> >
> > I see, that's right, I should have added two compatibles.
> >
> > As there is currently only one 'specific' compatible (r9axxx), should I
> > describe the two compatibles as being mandatory? Or should I set the
> > most specific one as optional and the least specific one (rzn1)
> > mandatory?  
> 
> Yes please.

I am a little bit confused to which answered you said yes.

>  Else you need to match on both in the driver, or we cannot
> differentiate later if the need ever arises.

I believe you meant "yes the two should be described as mandatory in the
bindings" (at least for now) so that when the need arises, the most
specific one can be replaced with a oneOf choice. Am I right?

Thanks,
Miquèl
Geert Uytterhoeven Nov. 26, 2021, 11:53 a.m. UTC | #6
Hi Miquel,

On Fri, Nov 26, 2021 at 12:46 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Fri, 19 Nov 2021 10:36:16 +0100:
> > On Fri, Nov 19, 2021 at 10:19 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > > geert@linux-m68k.org wrote on Fri, 19 Nov 2021 09:41:35 +0100:
> > > > On Thu, Nov 18, 2021 at 12:19 PM Miquel Raynal
> > > > <miquel.raynal@bootlin.com> wrote:
> > > > > Add a Yaml description for this Renesas NAND controller bindings.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
> > > > > @@ -0,0 +1,60 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Renesas RZ/N1x NAND flash controller device tree bindings
> > > > > +
> > > > > +maintainers:
> > > > > +  - Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: "nand-controller.yaml"
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: renesas,r9a06g032-nand-controller
> > > >
> > > > As the NAND Flash Controller is present on all of RZ/N1D, RZ/N1S,
> > > > and RZ/N1L, I think you should add a family-specific compatible value
> > > > "renesas,rzn1-nand-controller" as a fallback.
> > >
> > > I see, that's right, I should have added two compatibles.
> > >
> > > As there is currently only one 'specific' compatible (r9axxx), should I
> > > describe the two compatibles as being mandatory? Or should I set the
> > > most specific one as optional and the least specific one (rzn1)
> > > mandatory?
> >
> > Yes please.
>
> I am a little bit confused to which answered you said yes.

My apologies: yes to making both mandatory.

> >  Else you need to match on both in the driver, or we cannot
> > differentiate later if the need ever arises.
>
> I believe you meant "yes the two should be described as mandatory in the
> bindings" (at least for now) so that when the need arises, the most
> specific one can be replaced with a oneOf choice. Am I right?

Exactly.

You can already use "enum" for the most-specific one, so it's clear where
to add new lines, and to minimize future changes.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
new file mode 100644
index 000000000000..880b6e534573
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/renesas,r9a06g032-nand-controller.yaml
@@ -0,0 +1,60 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/renesas,r9a06g032-nand-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/N1x NAND flash controller device tree bindings
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+allOf:
+  - $ref: "nand-controller.yaml"
+
+properties:
+  compatible:
+    const: renesas,r9a06g032-nand-controller
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Host controller clock
+      - description: External NAND bus clock
+
+  clock-names:
+    items:
+      - const: hclk
+      - const: eclk
+
+  "#address-cells": true
+  "#size-cells": true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    nand-controller@40102000 {
+        compatible = "renesas,r9a06g032-nand-controller";
+        reg = <0x40102000 0x2000>;
+        interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&hclk_nand>, <&clk_nand>;
+        clock-names = "hclk", "eclk";
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };