Re: [PATCH v8 3/3] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme
From: Miquel Raynal
Date: Tue Jun 06 2023 - 03:53:17 EST
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