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

[Chris Packham]

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.

>
>>
>> Best regards,
>> Krzysztof