On 01/06/2023 01:49, Chris Packham wrote:
> From: Vadym Kochan <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chris Packham <[email protected]>
> ---
>
> 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.
> - 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://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell NAND Flash Controller (NFC)
> +
> +maintainers:
> + - Miquel Raynal <[email protected]>
> +
> +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.
> +
> + 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.
> +
> + 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
> +
> + required:
> + - reg
> + - nand-rb
> +
required: block goes here
> +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
> +
> + clock-names:
> + minItems: 2
> +
> + required:
> + - marvell,system-controller
> +
Best regards,
Krzysztof
On 1/06/23 19:05, Krzysztof Kozlowski wrote:
> On 01/06/2023 01:49, Chris Packham wrote:
>> From: Vadym Kochan <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
>>
>> 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.
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.
>> - 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=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEF-_8xrP2A&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmtd%2fmarvell%2cnand-controller%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEFvq8B-ZjQ&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell NAND Flash Controller (NFC)
>> +
>> +maintainers:
>> + - Miquel Raynal <[email protected]>
>> +
>> +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.
>
>> +
>> + 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.
>
>> +
>> + 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
>
>> +
>> + required:
>> + - reg
>> + - nand-rb
>> +
> required: block goes here
>
>> +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
>> +
>> + clock-names:
>> + minItems: 2
>> +
>> + required:
>> + - marvell,system-controller
>> +
>
> Best regards,
> Krzysztof
>
Hi Krzystof,
On 1/06/23 19:05, Krzysztof Kozlowski wrote:
> On 01/06/2023 01:49, Chris Packham wrote:
>> From: Vadym Kochan <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
>>
>> 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.
>> - 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=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEF-_8xrP2A&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmtd%2fmarvell%2cnand-controller%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEFvq8B-ZjQ&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell NAND Flash Controller (NFC)
>> +
>> +maintainers:
>> + - Miquel Raynal <[email protected]>
>> +
>> +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).
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?
>
>> +
>> + required:
>> + - reg
>> + - nand-rb
>> +
> required: block goes here
Will move
>
>> +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
>> +
>> + clock-names:
>> + minItems: 2
>> +
>> + required:
>> + - marvell,system-controller
>> +
>
> Best regards,
> Krzysztof
>
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Chris Packham <[email protected]>
>>> ---
>>>
>>> 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.
>>> - 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=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEF-_8xrP2A&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmtd%2fmarvell%2cnand-controller%2eyaml%23
>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=18P45NWaR4V6pXt_kuivNCiVAXCm3C7MEFvq8B-ZjQ&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +title: Marvell NAND Flash Controller (NFC)
>>> +
>>> +maintainers:
>>> + - Miquel Raynal <[email protected]>
>>> +
>>> +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
>
> 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?
Best regards,
Krzysztof
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Chris Packham <[email protected]>
>>>> ---
>>>>
>>>> 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 <[email protected]>
>>>> +
>>>> +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.
>
> Best regards,
> Krzysztof
> > Then you are doing something different than all other bindings.
>
> Not intentionally. I should probably check that the existing bindings
> actually work as expected.
This binding supports marvell,pxa3xx-nand. That is really really
old. So it could well be it works in the kernel, but the YAML does not
fully implement what the kernel actually supports. Best practices
could of pushed modern day DT to a subset, and YAML only supports that
subset of reality. And once you get outside of this subset, you run
into trouble.
I don't actually know this is the case, but you should keep it in
mind.
Andrew
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Chris Packham <[email protected]>
>>>>> ---
>>>>>
>>>>> 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 <[email protected]>
>>>>> +
>>>>> +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
Hi Chris,
[email protected] 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 <[email protected]>
> >>>>>
> >>>>> 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 <[email protected]>
> >>>>> Signed-off-by: Chris Packham <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> 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 <[email protected]>
> >>>>> +
> >>>>> +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
On 06/06/2023 09:48, Miquel Raynal wrote:
>>>>>>> + 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.
No, neither nand-controller.yaml nor nand-chip.yaml limit the properties
in this context, so each device schema must have unevaluatedProperties:
false, for which I asked few emails ago.
Best regards,
Krzysztof
Hi Krzysztof,
[email protected] wrote on Tue, 6 Jun 2023 10:44:34 +0200:
> On 06/06/2023 09:48, Miquel Raynal wrote:
> >>>>>>> + 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.
>
> No, neither nand-controller.yaml nor nand-chip.yaml limit the properties
> in this context, so each device schema must have unevaluatedProperties:
> false, for which I asked few emails ago.
The controller description shall be guarded by unevaluatedProperties:
false, we agree. Do you mean the nand chip description in each nand
controller binding should also include it at its own level? Because
that is not what we enforced so far IIRC. I am totally fine doing so
starting from now on if this is a new requirement (which makes sense).
If yes, then it means we would need to list *all* the nand
chip properties in each schema, which clearly involves a lot of
duplication as you would need to define all types of partitions,
partition parsers, generic properties, etc in order for the examples to
pass all the checks. Only the properties like pinctrl-* would not need
to be listed I guess.
As Chris was having issues comparing his work with the ingenic and qcom
yaml files, I gave your input a try and hopefully "fixed" these
bindings. I'll Cc Chris on the submission so that he has an example of
working -but maybe not fully valid, let's see- binding to take as
example.
Thanks,
Miquèl
On 06/06/2023 12:28, Miquel Raynal wrote:
> Hi Krzysztof,
>
> [email protected] wrote on Tue, 6 Jun 2023 10:44:34 +0200:
>
>> On 06/06/2023 09:48, Miquel Raynal wrote:
>>>>>>>>> + 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.
>>
>> No, neither nand-controller.yaml nor nand-chip.yaml limit the properties
>> in this context, so each device schema must have unevaluatedProperties:
>> false, for which I asked few emails ago.
>
> The controller description shall be guarded by unevaluatedProperties:
> false, we agree. Do you mean the nand chip description in each nand
> controller binding should also include it at its own level? Because
> that is not what we enforced so far IIRC. I am totally fine doing so
> starting from now on if this is a new requirement (which makes sense).
>
> If yes, then it means we would need to list *all* the nand
> chip properties in each schema, which clearly involves a lot of
> duplication as you would need to define all types of partitions,
> partition parsers, generic properties, etc in order for the examples to
> pass all the checks. Only the properties like pinctrl-* would not need
> to be listed I guess.
Yes, this is what should be done. Each node should have either
additional or unevaluatedProperties: false. It might come from
referenced schema or from final (device) schema), but it looks here it
is missing. It's exactly the same in all bindings. You will find plenty
of examples, e.g. individual leds children and common.yaml.
>
> As Chris was having issues comparing his work with the ingenic and qcom
> yaml files, I gave your input a try and hopefully "fixed" these
> bindings.
Both are wrong in this matter. You can easily test it. Patch like:
diff --git a/Documentation/devicetree/bindings/mtd/ingenic,nand.yaml
b/Documentation/devicetree/bindings/mtd/ingenic,nand.yaml
index a7bdb5d3675c..968c42f6b5a2 100644
--- a/Documentation/devicetree/bindings/mtd/ingenic,nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/ingenic,nand.yaml
@@ -97,6 +97,8 @@ examples:
nand-ecc-mode = "hw";
nand-on-flash-bbt;
+ fake-stuff;
+
pinctrl-names = "default";
pinctrl-0 = <&pins_nemc_cs1>;
should trigger warning as this is clearly fake-stuff. No warning though...
Best regards,
Krzysztof