[22/43] dt-bindings: mtd: add DT bindings for ts7250 nand

  Add YAML bindings for ts7250 NAND.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 .../bindings/mtd/technologic,nand.yaml        | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml
  

On Mon, Apr 24, 2023 at 03:34:38PM +0300, Nikita Shubin wrote:
> Add YAML bindings for ts7250 NAND.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../bindings/mtd/technologic,nand.yaml        | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> new file mode 100644
> index 000000000000..3234d93a1c21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Technologic Systems NAND controller
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: technologic,ts7200-nand
> +      - const: gen_nand

Not a useful compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells': true
> +  '#size-cells': true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: true

No, 'true' is not allowed here.

> +
> +examples:
> +  - |
> +    nand-parts@0 {
> +      compatible = "technologic,ts7200-nand", "gen_nand";
> +      reg = <0x60000000 0x8000000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      partition@0 {
> +        label = "TS-BOOTROM";
> +        reg = <0x00000000 0x00020000>;
> +        read-only;
> +      };
> +
> +      partition@20000 {
> +        label = "Linux";
> +        reg = <0x00020000 0x07d00000>;
> +      };
> +
> +      partition@7d20000 {
> +        label = "RedBoot";
> +        reg = <0x07d20000 0x002e0000>;
> +        read-only;
> +      };
> +    };
> +
> +...
> -- 
> 2.39.2
>
  

Hi Nikita,

nikita.shubin@maquefel.me wrote on Mon, 24 Apr 2023 15:34:38 +0300:

> Add YAML bindings for ts7250 NAND.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../bindings/mtd/technologic,nand.yaml        | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> new file mode 100644
> index 000000000000..3234d93a1c21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Technologic Systems NAND controller
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: technologic,ts7200-nand

would -nand-controller instead of -nand work as a suffix here?

You mention ts7250 in the title, should we have a more specific
compatible than ts7200 as well?

I see by looking at the mtd patch that you actually try to match both,
so they should both be defined in the bindings.

> +      - const: gen_nand

This is a old hack for very simple controllers (converted to DT probing
12 years ago). The logic used by this driver has been deprecated for
like 10 years and does not really apply to modern APIs. I would really
like to keep this driver contained with platform data coming from arch/
data only.

I suggest you create a real NAND controller driver based on the
generic one (should not be very complex, just duplicate the code so the
migration to the up-to-date API is eased) and you flag it as "must be
updated to ->exec_op() somehow. This way if someone starts the
conversion, it does not need to cope with the 5 other users of the
generic driver which anyway share nothing in common besides the
deprecated ->cmd_ctrl() backbone.

I read the comments on the cover letter, people are kind of pushing on
having this merged quickly. I am fine accepting a legacy controller
driver and migrating it to ->exec_op() later, but the current driver
conversion does not fit the approach taken years ago towards a cleaner
mtd tree.

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells': true
> +  '#size-cells': true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: true
> +
> +examples:
> +  - |
> +    nand-parts@0 {
> +      compatible = "technologic,ts7200-nand", "gen_nand";
> +      reg = <0x60000000 0x8000000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      partition@0 {
> +        label = "TS-BOOTROM";
> +        reg = <0x00000000 0x00020000>;
> +        read-only;
> +      };

Partitions are not useful here, but if you want them, use the
partitions container instead, please.

> +
> +      partition@20000 {
> +        label = "Linux";
> +        reg = <0x00020000 0x07d00000>;
> +      };
> +
> +      partition@7d20000 {
> +        label = "RedBoot";
> +        reg = <0x07d20000 0x002e0000>;
> +        read-only;
> +      };
> +    };
> +
> +...


Thanks,
Miquèl
  

Hello Miquel!

Thank you for looking into it.

On Tue, 2023-05-02 at 11:48 +0200, Miquel Raynal wrote:
> Hi Nikita,
> 
> nikita.shubin@maquefel.me wrote on Mon, 24 Apr 2023 15:34:38 +0300:
> 
> > Add YAML bindings for ts7250 NAND.
> > 
> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > ---
> >  .../bindings/mtd/technologic,nand.yaml        | 56
> > +++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > new file mode 100644
> > index 000000000000..3234d93a1c21
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Technologic Systems NAND controller
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@denx.de>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: technologic,ts7200-nand
> 
> would -nand-controller instead of -nand work as a suffix here?
> 
> You mention ts7250 in the title, should we have a more specific
> compatible than ts7200 as well?
> 
> I see by looking at the mtd patch that you actually try to match
> both,
> so they should both be defined in the bindings.
> 
> > +      - const: gen_nand
> 
> This is a old hack for very simple controllers (converted to DT
> probing
> 12 years ago). The logic used by this driver has been deprecated for
> like 10 years and does not really apply to modern APIs. I would
> really
> like to keep this driver contained with platform data coming from
> arch/
> data only.
> 
> I suggest you create a real NAND controller driver based on the
> generic one (should not be very complex, just duplicate the code so
> the
> migration to the up-to-date API is eased) and you flag it as "must be
> updated to ->exec_op() somehow. This way if someone starts the
> conversion, it does not need to cope with the 5 other users of the
> generic driver which anyway share nothing in common besides the
> deprecated ->cmd_ctrl() backbone.
> 
> I read the comments on the cover letter, people are kind of pushing
> on
> having this merged quickly. I am fine accepting a legacy controller
> driver and migrating it to ->exec_op() later, but the current driver
> conversion does not fit the approach taken years ago towards a
> cleaner
> mtd tree.

Did you mean that i should at least implement legacy nand controller,
like, for example, Xway (xway_nand.c) ?:

        data->chip.legacy.cmd_ctrl = xway_cmd_ctrl;
        data->chip.legacy.dev_ready = xway_dev_ready;
        data->chip.legacy.select_chip = xway_select_chip;
        data->chip.legacy.write_buf = xway_write_buf;
        data->chip.legacy.read_buf = xway_read_buf;
        data->chip.legacy.read_byte = xway_read_byte;
        data->chip.legacy.chip_delay = 30;

And the best solution would be switching to exec_op completely ?

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#address-cells': true
> > +  '#size-cells': true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: true
> > +
> > +examples:
> > +  - |
> > +    nand-parts@0 {
> > +      compatible = "technologic,ts7200-nand", "gen_nand";
> > +      reg = <0x60000000 0x8000000>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +
> > +      partition@0 {
> > +        label = "TS-BOOTROM";
> > +        reg = <0x00000000 0x00020000>;
> > +        read-only;
> > +      };
> 
> Partitions are not useful here, but if you want them, use the
> partitions container instead, please.
> 
> > +
> > +      partition@20000 {
> > +        label = "Linux";
> > +        reg = <0x00020000 0x07d00000>;
> > +      };
> > +
> > +      partition@7d20000 {
> > +        label = "RedBoot";
> > +        reg = <0x07d20000 0x002e0000>;
> > +        read-only;
> > +      };
> > +    };
> > +
> > +...
> 
> 
> Thanks,
> Miquèl
  

Hi Nikita,

nikita.shubin@maquefel.me wrote on Mon, 15 May 2023 18:48:31 +0300:

> Hello Miquel!
> 
> Thank you for looking into it.
> 
> On Tue, 2023-05-02 at 11:48 +0200, Miquel Raynal wrote:
> > Hi Nikita,
> > 
> > nikita.shubin@maquefel.me wrote on Mon, 24 Apr 2023 15:34:38 +0300:
> >   
> > > Add YAML bindings for ts7250 NAND.
> > > 
> > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > > ---
> > >  .../bindings/mtd/technologic,nand.yaml        | 56
> > > +++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > > b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > > new file mode 100644
> > > index 000000000000..3234d93a1c21
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml
> > > @@ -0,0 +1,56 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Technologic Systems NAND controller
> > > +
> > > +maintainers:
> > > +  - Lukasz Majewski <lukma@denx.de>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: technologic,ts7200-nand  
> > 
> > would -nand-controller instead of -nand work as a suffix here?
> > 
> > You mention ts7250 in the title, should we have a more specific
> > compatible than ts7200 as well?
> > 
> > I see by looking at the mtd patch that you actually try to match
> > both,
> > so they should both be defined in the bindings.
> >   
> > > +      - const: gen_nand  
> > 
> > This is a old hack for very simple controllers (converted to DT
> > probing
> > 12 years ago). The logic used by this driver has been deprecated for
> > like 10 years and does not really apply to modern APIs. I would
> > really
> > like to keep this driver contained with platform data coming from
> > arch/
> > data only.
> > 
> > I suggest you create a real NAND controller driver based on the
> > generic one (should not be very complex, just duplicate the code so
> > the
> > migration to the up-to-date API is eased) and you flag it as "must be
> > updated to ->exec_op() somehow. This way if someone starts the
> > conversion, it does not need to cope with the 5 other users of the
> > generic driver which anyway share nothing in common besides the
> > deprecated ->cmd_ctrl() backbone.
> > 
> > I read the comments on the cover letter, people are kind of pushing
> > on
> > having this merged quickly. I am fine accepting a legacy controller
> > driver and migrating it to ->exec_op() later, but the current driver
> > conversion does not fit the approach taken years ago towards a
> > cleaner
> > mtd tree.  
> 
> Did you mean that i should at least implement legacy nand controller,
> like, for example, Xway (xway_nand.c) ?:
> 
>         data->chip.legacy.cmd_ctrl = xway_cmd_ctrl;
>         data->chip.legacy.dev_ready = xway_dev_ready;
>         data->chip.legacy.select_chip = xway_select_chip;
>         data->chip.legacy.write_buf = xway_write_buf;
>         data->chip.legacy.read_buf = xway_read_buf;
>         data->chip.legacy.read_byte = xway_read_byte;
>         data->chip.legacy.chip_delay = 30;

I don't know how urgent this conversion is, this is really the minimal
step...

> And the best solution would be switching to exec_op completely ?

...and this is what I would really prefer, yes. I don't think it's
huge, the controller being very simple and straightforward.

> 
> >   
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells': true
> > > +  '#size-cells': true
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +unevaluatedProperties: true
> > > +
> > > +examples:
> > > +  - |
> > > +    nand-parts@0 {
> > > +      compatible = "technologic,ts7200-nand", "gen_nand";
> > > +      reg = <0x60000000 0x8000000>;
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> > > +
> > > +      partition@0 {
> > > +        label = "TS-BOOTROM";
> > > +        reg = <0x00000000 0x00020000>;
> > > +        read-only;
> > > +      };  
> > 
> > Partitions are not useful here, but if you want them, use the
> > partitions container instead, please.
> >   
> > > +
> > > +      partition@20000 {
> > > +        label = "Linux";
> > > +        reg = <0x00020000 0x07d00000>;
> > > +      };
> > > +
> > > +      partition@7d20000 {
> > > +        label = "RedBoot";
> > > +        reg = <0x07d20000 0x002e0000>;
> > > +        read-only;
> > > +      };
> > > +    };
> > > +
> > > +...  
> > 
> > 
> > Thanks,
> > Miquèl  
> 


Thanks,
Miquèl