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
