Date:   Tue, 30 May 2023 15:04:39 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Chris Packham <chris.packham@...iedtelesis.co.nz>, richard@....at,
        vigneshr@...com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        andrew@...n.ch, gregory.clement@...tlin.com,
        sebastian.hesselbarth@...il.com, vadym.kochan@...ision.eu,
        linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        enachman@...vell.com
Subject: Re: [PATCH v6 1/2] dt-bindings: mtd: marvell-nand: Convert to YAML
 DT scheme

Hi Krzysztof,

krzysztof.kozlowski@...aro.org wrote on Tue, 30 May 2023 14:24:22 +0200:

> On 30/05/2023 02:53, Chris Packham wrote:
> > From: Vadym Kochan <vadym.kochan@...ision.eu>
> > 
> > Switch the DT binding to a YAML schema to enable the DT validation.
> > 
> > Dropped deprecated compatibles and properties described in txt file.
> > 
> > Signed-off-by: Vadym Kochan <vadym.kochan@...ision.eu>
> > Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> > ---
> > 
> > Notes:
> >     Changes in v6:
> >     - remove properties covered by nand-controller.yaml
> >     - add example using armada-8k compatible
> >     
> >     earlier changes:
> >     
> >     v5:
> >        1) Get back "label" and "partitions" properties but without  
> 
> Where are they? Did you drop them in v6?

label and partitions are defined in partitions/partition.yaml,
referenced by partitions.yaml, referenced by mtd.yaml, referenced by
nand-chip.yaml, referenced by nand-controller.yaml, itself referenced
in this file :-)

So I believe there is nothing else to add in the controller's binding
for these two properties? They are very generic, it would not be
optimal if we had to take care about them.

> >           ref to the "partition.yaml" which was wrongly used.  
> 
> 
> >     
> >        2) Add "additionalProperties: false" for nand@ because all possible
> >           properties are described.  
> 
> Where? This cannot be silently dropped!
> 
> >     
> >     v4:
> >        1) Remove "label" and "partitions" properties
> >     
> >        2) Use 2 clocks for A7K/8K platform which is a requirement
> >     
> >     v3:
> >       1) Remove txt version from the MAINTAINERS list
> >     
> >       2) Use enum for some of compatible strings
> >     
> >       3) Drop:
> >             #address-cells
> >             #size-cells:
> >     
> >          as they are inherited from the nand-controller.yaml
> >     
> >       4) Add restriction to use 2 clocks for A8K SoC
> >     
> >       5) Dropped description for clock-names and extend it with
> >          minItems: 1
> >     
> >       6) Drop description for "dmas"
> >     
> >       7) Use "unevalautedProperties: false"
> >     
> >       8) Drop quites from yaml refs.
> >     
> >       9) Use 4-space indentation for the example section
> >     
> >     v2:
> >       1) Fixed warning by yamllint with incorrect indentation for compatible list
> > 
> >  .../bindings/mtd/marvell,nand-controller.yaml | 190 ++++++++++++++++++
> >  .../devicetree/bindings/mtd/marvell-nand.txt  | 126 ------------
> >  MAINTAINERS                                   |   1 -
> >  3 files changed, 190 insertions(+), 127 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml b/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
> > new file mode 100644
> > index 000000000000..c4b003f5fa9f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
> > @@ -0,0 +1,190 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Marvell NAND Flash Controller (NFC)
> > +
> > +maintainers:
> > +  - Miquel Raynal <miquel.raynal@...tlin.com>  
> 
> Is it correct person for Marvell NAND? This should be not a subsystem
> maintainer, but a device maintainer.

I did not bother converting this file yet but I actually rewrote the
corresponding Linux driver (5 years ago) entirely so I don't mind.

> 
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - const: marvell,armada-8k-nand-controller
> > +          - const: marvell,armada370-nand-controller

I don't think we ever wanted having these two compatibles to describe a
single hardware block?

> > +      - enum:
> > +          - marvell,armada370-nand-controller
> > +          - marvell,pxa3xx-nand-controller  
> 
> You miss here deprecated compatibles, which are BTW still used. Don't
> drop properties and compatibles during conversion.
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description:
> > +      Shall reference the NAND controller clocks, the second one is
> > +      is only needed for the Armada 7K/8K SoCs
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:  
> 
> Missing minItems: 1
> 
> > +    items:
> > +      - const: core
> > +      - const: reg
> > +
> > +  dmas:
> > +    maxItems: 1
> > +
> > +  dma-names:
> > +    items:
> > +      - const: rxtx
> > +
> > +  marvell,system-controller:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Syscon node that handles NAND controller related registers
> > +
> > +patternProperties:
> > +  "^nand@[0-3]$":
> > +    type: object  
> 
> Missing unevaluatedProperties: false on this level.
>
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 3

Same as below, it is an array as well IIRC.

> > +
> > +      nand-rb:
> > +        minimum: 0
> > +        maximum: 1  
> 
> It's an array, so this does not sound right. You might want to put it
> under items:.  Then you also miss min/maxItems.

That's true, you can have either one or two members with the value
[0-1], so you need both.

> > +
> > +      nand-ecc-step-size:
> > +        const: 512
> > +
> > +      nand-ecc-strength:
> > +        enum: [1, 4, 8]

The controller (and the driver) actually supports 1, 4, 8, 12, 16.

> > +
> > +      marvell,nand-keep-config:
> > +        description: |
> > +          Orders the driver not to take the timings from the core and
> > +          leaving them completely untouched. Bootloader timings will then
> > +          be used.
> > +        $ref: /schemas/types.yaml#/definitions/flag
> > +
> > +      marvell,nand-enable-arbiter:
> > +        description: |
> > +          To enable the arbiter, all boards blindly used it,
> > +          this bit was set by the bootloader for many boards and even if
> > +          it is marked reserved in several datasheets, it might be needed to set
> > +          it (otherwise it is harmless) so whether or not this property is set,
> > +          the bit is selected by the driver.

Maybe we should slightly rephrase this to avoid driver related
information.

> > +        $ref: /schemas/types.yaml#/definitions/flag
> > +        deprecated: true
> > +
> > +    required:
> > +      - reg
> > +      - nand-rb
> > +
> > +allOf:
> > +  - $ref: nand-controller.yaml
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: marvell,pxa3xx-nand-controller
> > +    then:
> > +      required:
> > +        - dmas
> > +        - dma-names
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: marvell,armada-8k-nand-controller
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 2
> > +          maxItems: 2  
> 
> Drop maxItems. You don't have it in clock-names.
> 
> > +
> > +        clock-names:
> > +          minItems: 2
> > +
> > +      required:
> > +        - marvell,system-controller
> > +    else:
> > +      properties:
> > +        clocks:
> > +          maxItems: 1
> > +
> > +        clock-names:
> > +          maxItems: 1  
> 
> I doubt that you tested it in above variant...
> 
> > +
> > +unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    nand_controller: nand-controller@...00 {
> > +        compatible = "marvell,armada370-nand-controller";
> > +        reg = <0xd0000 0x54>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;  
> 
> 
> Best regards,
> Krzysztof
> 

Thanks for doing this!

Miquèl

Powered by blists - more mailing lists