Re: [PATCH v6 1/2] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

From: Chris Packham
Date: Tue May 30 2023 - 17:57:18 EST


On 31/05/23 01:04, Miquel Raynal wrote:
> Hi Krzysztof,
>
> krzysztof.kozlowski@xxxxxxxxxx wrote on Tue, 30 May 2023 14:24:22 +0200:
>
>> On 30/05/2023 02:53, Chris Packham wrote:
>>> From: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>>
>>> 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@xxxxxxxxxxx>
>> 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@d0000 {
>>> + compatible = "marvell,armada370-nand-controller";
>>> + reg = <0xd0000 0x54>;
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>
>> Best regards,
>> Krzysztof
>>
> Thanks for doing this!
>
> Miquèl