Hi Chris, Chris.Packham@xxxxxxxxxxxxxxxxxxx wrote on Tue, 6 Jun 2023 04:38:01 +0000: > On 6/06/23 08:44, Chris Packham wrote: > > > > On 4/06/23 21:26, Krzysztof Kozlowski wrote: > >> On 02/06/2023 01:06, Chris Packham wrote: > >>> Hi Krzystof, > >>> > >>> On 1/06/23 19:05, Krzysztof Kozlowski wrote: > >>>> On 01/06/2023 01:49, Chris Packham wrote: > >>>>> From: Vadym Kochan <vadym.kochan@xxxxxxxxxxx> > >>>>> > >>>>> Switch the DT binding to a YAML schema to enable the DT validation. > >>>>> > >>>>> The text binding didn't mention it as a requirement but existing > >>>>> usage > >>>>> has > >>>>> > >>>>> compatible = "marvell,armada-8k-nand-controller", > >>>>> "marvell,armada370-nand-controller"; > >>>>> > >>>>> so the YAML allows this in addition to the individual compatible > >>>>> values. > >>>>> > >>>>> There was also an incorrect reference to dma-names being "rxtx" where > >>>>> the driver and existing device trees actually use dma-names = > >>>>> "data" so > >>>>> this is corrected in the conversion. > >>>>> > >>>>> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx> > >>>>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> Notes: > >>>>> Changes in v8: > >>>>> - Mark deprecated compatible values as such > >>>>> - Allow "marvell,armada-8k-nand-controller" without > >>>>> "marvell,armada370-nand-controller" > >>>>> - Make dma-names usage reflect reality > >>>>> - Update commit message > >>>>> Changes in v7: > >>>>> - Restore "label" and "partitions" properties (should be > >>>>> picked up via > >>>>> nand-controller.yaml but aren't) > >>>> What do you mean by "aren't"? They are not needed. > >>> (sorry I keep responding to snippets rather than putting all the > >>> replies > >>> in one place. For posterity here's the same response I provided in a > >>> separate message). > >>> > >>> I mean I simply cannot make it work and I'm out of ideas (I'm also > >>> in an > >>> awkward timezone so it takes 24hrs for me to ask a question and get a > >>> response which leads to me making guesses instead of waiting). > >>> > >>> nand-controller.yaml references nand-chip.yaml which references > >>> mtd.yaml > >>> which defines the "label" and "partitions" property. > >>> > >>> I thought marvell,nand-controller.yaml could just say `$ref: > >>> nand-controller.yaml` and it would mean I'd get all the definitions > >>> down > >>> the chain but this doesn't seem to work the way I expect (or more > >>> likely > >>> I'm not doing it right). I thought it might have something to do with > >>> the different patternProperties pattern but even when I make that match > >>> what is used in nand-controller.yaml it doesn't seem to pick up those > >>> properties. > >> Then you are doing something different than all other bindings. > > > > Not intentionally. I should probably check that the existing bindings > > actually work as expected. > > > > One thing that this has that the others don't is including "label" and > > "partitions" in the examples. > > > >>>>> - Add/restore nand-on-flash-bbt and nand-ecc-mode which > >>>>> aren't covered > >>>>> by nand-controller.yaml. > >>>>> - Use "unevalautedProperties: false" > >>>>> - Corrections for clock-names, dma-names, nand-rb and > >>>>> nand-ecc-strength > >>>>> - Add pxa3xx-nand-controller example > >>>>> 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 > >>>>> ref to the "partition.yaml" which was wrongly used. > >>>>> 2) Add "additionalProperties: false" for nand@ > >>>>> because all possible > >>>>> properties are described. > >>>>> 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 | 223 > >>>>> ++++++++++++++++++ > >>>>> .../devicetree/bindings/mtd/marvell-nand.txt | 126 ---------- > >>>>> MAINTAINERS | 1 - > >>>>> 3 files changed, 223 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..433feb430555 > >>>>> --- /dev/null > >>>>> +++ > >>>>> b/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml > >>>>> @@ -0,0 +1,223 @@ > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>> +%YAML 1.2 > >>>>> +--- > >>>>> +$id: > >>>>> http://scanmail.trustwave.com/?c=20988&d=yNj85IkMld8k0XBdA9CH4pQjE5peaXAdz-exk_Hdww&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmtd%2fmarvell%2cnand-controller%2eyaml%23 > >>>>> +$schema: > >>>>> http://scanmail.trustwave.com/?c=20988&d=yNj85IkMld8k0XBdA9CH4pQjE5peaXAdz-PkkPSLlg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23 > >>>>> + > >>>>> +title: Marvell NAND Flash Controller (NFC) > >>>>> + > >>>>> +maintainers: > >>>>> + - Miquel Raynal <miquel.raynal@xxxxxxxxxxx> > >>>>> + > >>>>> +properties: > >>>>> + compatible: > >>>>> + oneOf: > >>>>> + - items: > >>>>> + - const: marvell,armada-8k-nand-controller > >>>>> + - const: marvell,armada370-nand-controller > >>>>> + - enum: > >>>>> + - marvell,armada-8k-nand-controller > >>>>> + - marvell,armada370-nand-controller > >>>>> + - marvell,pxa3xx-nand-controller > >>>>> + - description: legacy bindings > >>>>> + deprecated: true > >>>>> + enum: > >>>>> + - marvell,armada-8k-nand > >>>>> + - marvell,armada370-nand > >>>>> + - marvell,pxa3xx-nand > >>>>> + > >>>>> + 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: > >>>>> + minItems: 1 > >>>>> + items: > >>>>> + - const: core > >>>>> + - const: reg > >>>>> + > >>>>> + dmas: > >>>>> + maxItems: 1 > >>>>> + > >>>>> + dma-names: > >>>>> + items: > >>>>> + - const: data > >>>>> + > >>>>> + marvell,system-controller: > >>>>> + $ref: /schemas/types.yaml#/definitions/phandle > >>>>> + description: Syscon node that handles NAND controller related > >>>>> registers > >>>>> + > >>>>> +patternProperties: > >>>>> + "^nand@[0-3]$": > >>>>> + type: object > >>>>> + unevaluatedProperties: false > >>>>> + properties: > >>>>> + reg: > >>>>> + minimum: 0 > >>>>> + maximum: 3 > >>>>> + > >>>>> + nand-rb: > >>>>> + minItems: 1 > >>>> Drop minItems. > >>>> > >>>>> + maxItems: 1 > >>>> Didn't you have here minimum and maximum? I think I did not ask to > >>>> remove them. > >>> I did but I couldn't figure out how to do minimum and maximum with an > >>> array would the following be correct (note removing both minItems and > >>> maxItems as dtb_check complains if I have maxItems and items). > >> items: > >> minimum: n > >> maximum: n > >> maxItems: n > >> > >> or > >> > >> items: > >> - minimum: n > >> maximum: n > >> > >> See for example Documentation/devicetree/bindings/arm/l2c2x0.yaml > > Thanks, so my suggestion below should be OK then. > >>> nand-rb: > >>> items: > >>> - minimum: 0 > >>> maximum: 1 > >>> > >>>>> + > >>>>> + nand-ecc-step-size: > >>>>> + const: 512 > >>>>> + > >>>>> + nand-ecc-strength: > >>>>> + enum: [1, 4, 8, 12, 16] > >>>>> + > >>>>> + nand-on-flash-bbt: > >>>>> + $ref: /schemas/types.yaml#/definitions/flag > >>>>> + > >>>>> + nand-ecc-mode: > >>>>> + const: hw > >>>>> + > >>>>> + label: > >>>>> + $ref: /schemas/types.yaml#/definitions/string > >>>> Drop label > >>>> > >>>>> + > >>>>> + partitions: > >>>>> + type: object > >>>> Drop partitions. > >>> This is the part I can't get to work. It should pick it up via > >>> nand-controller.yaml but nothing I do seems to work. > >>> > >>>>> + > >>>>> + 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). > >>>>> + $ref: /schemas/types.yaml#/definitions/flag > >>>>> + deprecated: true > >>>>> + > >>>>> + additionalProperties: false > >>>> unevaluatedProperties: false > >>> It was hiding by '"^nand@[0-3]$":'. Should I move it here? > >> You cannot have both additionalProps and unevaluatedProps at the same > >> time, so we do not talk about same thing or this was never working? > > > > Hmm, I'm a little confused then. At various times I've been told to > > put 'additionalProperties: false' or 'unevaluatedProperties: false' > > (although never at the same time). I'm not sure when to use one or the > > other. > > > > From what I've been able to glean 'additionalProperties: true' > > indicates that the node is expected to have child nodes defined in a > > different schema so I would have thought 'additionalProperties: false' > > would be appropriate for a schema covering a leaf node. > > 'unevaluatedProperties: false' seems to enable stricter checking which > > makes sense when all the properties are described in the schema. > > So I think this might be the problem. If I look at qcom,nandc.yaml or > ingenic,nand.yaml which both have a partitions property in their > example. Neither have 'unevaluatedProperties: false' on the nand@... > subnode. If I add it sure enough I start getting complaints about the > 'partitions' node being unexpected. Sorry if that was unclear, I think the whole logic around the yaml files is to progressively constrain the descriptions, schema after schema. IOW, in the marvell binding you should set unevaluatedProperties: false for the NAND controller. What is inside (NAND chips, partition container, partition parsers, "mtd" properties, etc) will be handled by other files. Of course you can constrain a bit what can/cannot be used inside these subnodes, but I think you don't need to set unevaluatedProperties in these subnodes (the NAND chip in this case, or even the partitions) because you already reference nand-controller.yaml which references nand-chip.yaml, mtd.yaml, partitions.yaml, etc. *they* will make the generic checks and hopefully apply stricter checks, when deemed relevant. Thanks, Miquèl
