Re: [PATCH v8 3/3] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Chris,

Chris.Packham@alliedtelesis.co.nz 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@plvision.eu>
> >>>>>
> >>>>> 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@plvision.eu>
> >>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>>>> ---
> >>>>>
> >>>>> 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@bootlin.com>
> >>>>> +
> >>>>> +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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/