Message-ID: <d720cc45-5c0e-cee2-da71-8fc713e63f4b@alliedtelesis.co.nz>
Date:   Tue, 30 May 2023 21:57:09 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC:     "richard@....at" <richard@....at>,
        "vigneshr@...com" <vigneshr@...com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "sebastian.hesselbarth@...il.com" <sebastian.hesselbarth@...il.com>,
        "vadym.kochan@...ision.eu" <vadym.kochan@...ision.eu>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "enachman@...vell.com" <enachman@...vell.com>
Subject: Re: [PATCH v6 1/2] dt-bindings: mtd: marvell-nand: Convert to YAML DT
 scheme


On 31/05/23 01:04, Miquel Raynal wrote:
> 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.
Hmm it doesn't appear t be picking them up. I was only getting away with 
it because I didn't have unevaluatedProperties: false.
>>>            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://scanmail.trustwave.com/?c=20988&d=9fT15MoBhmyC0BWsX8X7RkhpsyYpcEL9vK7BlmjMYw&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmtd%2fmarvell%2cnand-controller%2eyaml%23
>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=9fT15MoBhmyC0BWsX8X7RkhpsyYpcEL9vKqUlW2aNg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +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