[v9,3/3] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

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 v9:
    - depend on series from Miquel
      https://lore.kernel.org/linux-mtd/20230606175246.190465-1-miquel.raynal@bootlin.com/
    - enforce minimum/maximum for nand-rb
    - move required: block for controller
    - move unevaluatedProperties: for nand chip
    - remove label, partitions and nand-on-flash-bbt which are covered by
      generic schema
    
    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)
    - 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 | 218 ++++++++++++++++++
 .../devicetree/bindings/mtd/marvell-nand.txt  | 126 ----------
 MAINTAINERS                                   |   1 -
 3 files changed, 218 insertions(+), 127 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
 delete mode 100644 Documentation/devicetree/bindings/mtd/marvell-nand.txt

On Thu, 15 Jun 2023 16:04:47 +1200, 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 v9:
>     - depend on series from Miquel
>       https://lore.kernel.org/linux-mtd/20230606175246.190465-1-miquel.raynal@bootlin.com/
>     - enforce minimum/maximum for nand-rb
>     - move required: block for controller
>     - move unevaluatedProperties: for nand chip
>     - remove label, partitions and nand-on-flash-bbt which are covered by
>       generic schema
> 
>     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)
>     - 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 | 218 ++++++++++++++++++
>  .../devicetree/bindings/mtd/marvell-nand.txt  | 126 ----------
>  MAINTAINERS                                   |   1 -
>  3 files changed, 218 insertions(+), 127 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mtd/marvell-nand.txt
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/mtd/raw-nand-chip.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.example.dtb: nand-controller@d0000: nand@0: False schema does not allow {'reg': [[0]], 'label': ['main-storage'], 'nand-rb': [[0]], 'nand-ecc-mode': ['hw'], 'marvell,nand-keep-config': True, 'nand-on-flash-bbt': True, 'nand-ecc-strength': [[4]], 'nand-ecc-step-size': [[512]], 'partitions': {'compatible': ['fixed-partitions'], '#address-cells': [[1]], '#size-cells': [[1]], 'partition@0': {'label': ['Rootfs'], 'reg': [[0, 1073741824]]}}}
	from schema $id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.example.dtb: nand-controller@d0000: nand@0: Unevaluated properties are not allowed ('label', 'nand-on-flash-bbt', 'partitions' were unexpected)
	from schema $id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.example.dtb: nand-controller@720000: nand@0: False schema does not allow {'reg': [[0]], 'label': ['main-storage'], 'nand-rb': [[0]], 'nand-ecc-mode': ['hw'], 'nand-ecc-strength': [[8]], 'nand-ecc-step-size': [[512]]}
	from schema $id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.example.dtb: nand-controller@720000: nand@0: Unevaluated properties are not allowed ('label' was unexpected)
	from schema $id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mtd/marvell,nand-controller.example.dtb: nand-controller@43100000: nand@0: False schema does not allow {'reg': [[0]], 'nand-rb': [[0]], 'nand-ecc-mode': ['hw'], 'marvell,nand-keep-config': True}
	from schema $id: http://devicetree.org/schemas/mtd/marvell,nand-controller.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230615040447.3484564-4-chris.packham@alliedtelesis.co.nz

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

On 15/06/2023 06:04, 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 v9:
>     - depend on series from Miquel
>       https://lore.kernel.org/linux-mtd/20230606175246.190465-1-miquel.raynal@bootlin.com/
>     - enforce minimum/maximum for nand-rb
>     - move required: block for controller
>     - move unevaluatedProperties: for nand chip
>     - remove label, partitions and nand-on-flash-bbt which are covered by
>       generic schema
>     
>     Changes in v8:
>     - Mark deprecated compatible values as such
>     - Allow "marvell,armada-8k-nand-controller" without
>       "marvell,armada370-nand-controller"

???


> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: marvell,armada-8k-nand-controller
> +          - const: marvell,armada370-nand-controller
> +      - enum:
> +          - marvell,armada-8k-nand-controller

This is wrong. 8k cannot be both: compatible and not compatible with
370. It's not someone's cat to be in both states at the same time...

> +          - 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@[a-f0-9]$":
> +    type: object
> +    $ref: raw-nand-chip.yaml
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 3
> +
> +      nand-rb:
> +        items:
> +          - minimum: 0
> +            maximum: 1
> +
> +      nand-ecc-step-size:
> +        const: 512
> +
> +      nand-ecc-strength:
> +        enum: [1, 4, 8, 12, 16]
> +
> +      nand-ecc-mode:
> +        const: hw
> +
> +      marvell,nand-keep-config:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: |

Do not need '|' unless you need to preserve formatting.

> +          Orders the driver not to take the timings from the core and
> +          leaving them completely untouched. Bootloader timings will then
> +          be used.
> +
> +      marvell,nand-enable-arbiter:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: |

Do not need '|' unless you need to preserve formatting.

> +          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).
> +        deprecated: true
> +
> +    required:
> +      - reg
> +      - nand-rb
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +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

else:
narrow clocks to 1? Or is the second clod also valid for other variants?
Does not look like from your example.


Best regards,
Krzysztof

Hi Krzysztof,

On 15/06/23 19:45, Krzysztof Kozlowski wrote:
> On 15/06/2023 06:04, 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 v9:
>>      - depend on series from Miquel https://lore.kernel.org/linux-mtd/20230606175246.190465-1-miquel.raynal@bootlin.com/
>>      - enforce minimum/maximum for nand-rb
>>      - move required: block for controller
>>      - move unevaluatedProperties: for nand chip
>>      - remove label, partitions and nand-on-flash-bbt which are covered by
>>        generic schema
>>      
>>      Changes in v8:
>>      - Mark deprecated compatible values as such
>>      - Allow "marvell,armada-8k-nand-controller" without
>>        "marvell,armada370-nand-controller"
> ???
>
>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - const: marvell,armada-8k-nand-controller
>> +          - const: marvell,armada370-nand-controller
>> +      - enum:
>> +          - marvell,armada-8k-nand-controller
> This is wrong. 8k cannot be both: compatible and not compatible with
> 370. It's not someone's cat to be in both states at the same time...

The correct state (IMHO, Miquel correct me if I'm wrong) is `compatible 
= "marvell,armada-8k-nand-controller";`  as there are some 8K specific 
requirements that aren't present on the 370 (specifically the 
system-controller and the 2nd clock). The only reason `compatible = 
"marvell,armada-8k-nand-controller", 
"marvell,armada370-nand-controller";` is supported is because that is 
what is in use in the existing dtses.

>> +          - 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@[a-f0-9]$":
>> +    type: object
>> +    $ref: raw-nand-chip.yaml
>> +
>> +    properties:
>> +      reg:
>> +        minimum: 0
>> +        maximum: 3
>> +
>> +      nand-rb:
>> +        items:
>> +          - minimum: 0
>> +            maximum: 1
>> +
>> +      nand-ecc-step-size:
>> +        const: 512
>> +
>> +      nand-ecc-strength:
>> +        enum: [1, 4, 8, 12, 16]
>> +
>> +      nand-ecc-mode:
>> +        const: hw
>> +
>> +      marvell,nand-keep-config:
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +        description: |
> Do not need '|' unless you need to preserve formatting.
Ack
>> +          Orders the driver not to take the timings from the core and
>> +          leaving them completely untouched. Bootloader timings will then
>> +          be used.
>> +
>> +      marvell,nand-enable-arbiter:
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +        description: |
> Do not need '|' unless you need to preserve formatting.
>
>> +          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).
>> +        deprecated: true
>> +
>> +    required:
>> +      - reg
>> +      - nand-rb
>> +
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +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
> else:
> narrow clocks to 1? Or is the second clod also valid for other variants?
> Does not look like from your example.
Correct. Will update.
>
>
> Best regards,
> Krzysztof
>

On 15/06/2023 23:06, Chris Packham wrote:
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - const: marvell,armada-8k-nand-controller
>>> +          - const: marvell,armada370-nand-controller
>>> +      - enum:
>>> +          - marvell,armada-8k-nand-controller
>> This is wrong. 8k cannot be both: compatible and not compatible with
>> 370. It's not someone's cat to be in both states at the same time...
> 
> The correct state (IMHO, Miquel correct me if I'm wrong) is `compatible 
> = "marvell,armada-8k-nand-controller";`  as there are some 8K specific 
> requirements that aren't present on the 370 (specifically the 
> system-controller and the 2nd clock). 

Presence of specific requirements does not invalidate compatibility. Two
devices are compatible if the 8k can bind and work with 370 compatible
string, even if this means some lower performance or less features (e.g.
subset of features).

Now whether they are really compatible or not - I don't know. I expect
the answer from you (as plural you).

> The only reason `compatible = 
> "marvell,armada-8k-nand-controller", 
> "marvell,armada370-nand-controller";` is supported is because that is 
> what is in use in the existing dtses.

Fix the DTS, use here the correct combination and, if it is a deviation
from old binding, mention this in commit msg.

Best regards,
Krzysztof

Hi Krzysztof,

krzysztof.kozlowski@linaro.org wrote on Fri, 16 Jun 2023 10:15:31 +0200:

> On 15/06/2023 23:06, Chris Packham wrote:
> >>  
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - const: marvell,armada-8k-nand-controller
> >>> +          - const: marvell,armada370-nand-controller
> >>> +      - enum:
> >>> +          - marvell,armada-8k-nand-controller  
> >> This is wrong. 8k cannot be both: compatible and not compatible with
> >> 370. It's not someone's cat to be in both states at the same time...  
> > 
> > The correct state (IMHO, Miquel correct me if I'm wrong) is `compatible 
> > = "marvell,armada-8k-nand-controller";`  as there are some 8K specific 
> > requirements that aren't present on the 370 (specifically the 
> > system-controller and the 2nd clock).   
> 
> Presence of specific requirements does not invalidate compatibility. Two
> devices are compatible if the 8k can bind and work with 370 compatible
> string, even if this means some lower performance or less features (e.g.
> subset of features).

Quoting myself from 2019 (comment in the driver):
"Some SoCs like A7k/A8k need to enable manually the NAND controller,
gated clocks and reset bits to avoid being bootloader dependent."

So can the 8k controller work using a 370 compatible : yes, if the
booloader enabled the NAND controller already, no otherwise. But in
practice it is the same controller.

Given this information I don't know whether it makes sense to qualify
the 8k controller compatible with the 370 compatible or not.

> Now whether they are really compatible or not - I don't know. I expect
> the answer from you (as plural you).

:-)

> > The only reason `compatible = 
> > "marvell,armada-8k-nand-controller", 
> > "marvell,armada370-nand-controller";` is supported is because that is 
> > what is in use in the existing dtses.  
> 
> Fix the DTS, use here the correct combination and, if it is a deviation
> from old binding, mention this in commit msg.

Fine by me.
 
> Best regards,
> Krzysztof
> 


Thanks,
Miquèl

On 16/06/2023 10:24, Miquel Raynal wrote:
> Hi Krzysztof,
> 
> krzysztof.kozlowski@linaro.org wrote on Fri, 16 Jun 2023 10:15:31 +0200:
> 
>> On 15/06/2023 23:06, Chris Packham wrote:
>>>>  
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - const: marvell,armada-8k-nand-controller
>>>>> +          - const: marvell,armada370-nand-controller
>>>>> +      - enum:
>>>>> +          - marvell,armada-8k-nand-controller  
>>>> This is wrong. 8k cannot be both: compatible and not compatible with
>>>> 370. It's not someone's cat to be in both states at the same time...  
>>>
>>> The correct state (IMHO, Miquel correct me if I'm wrong) is `compatible 
>>> = "marvell,armada-8k-nand-controller";`  as there are some 8K specific 
>>> requirements that aren't present on the 370 (specifically the 
>>> system-controller and the 2nd clock).   
>>
>> Presence of specific requirements does not invalidate compatibility. Two
>> devices are compatible if the 8k can bind and work with 370 compatible
>> string, even if this means some lower performance or less features (e.g.
>> subset of features).
> 
> Quoting myself from 2019 (comment in the driver):
> "Some SoCs like A7k/A8k need to enable manually the NAND controller,
> gated clocks and reset bits to avoid being bootloader dependent."
> 
> So can the 8k controller work using a 370 compatible : yes, if the
> booloader enabled the NAND controller already, no otherwise. But in
> practice it is the same controller.
> 
> Given this information I don't know whether it makes sense to qualify
> the 8k controller compatible with the 370 compatible or not.

Indeed, a bit tricky decision. For me they could stay compatible,
because apparently programming model is the same or similar.

Best regards,
Krzysztof

Hi Krzysztof,

krzysztof.kozlowski@linaro.org wrote on Fri, 16 Jun 2023 10:38:38 +0200:

> On 16/06/2023 10:24, Miquel Raynal wrote:
> > Hi Krzysztof,
> > 
> > krzysztof.kozlowski@linaro.org wrote on Fri, 16 Jun 2023 10:15:31 +0200:
> >   
> >> On 15/06/2023 23:06, Chris Packham wrote:  
> >>>>    
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    oneOf:
> >>>>> +      - items:
> >>>>> +          - const: marvell,armada-8k-nand-controller
> >>>>> +          - const: marvell,armada370-nand-controller
> >>>>> +      - enum:
> >>>>> +          - marvell,armada-8k-nand-controller    
> >>>> This is wrong. 8k cannot be both: compatible and not compatible with
> >>>> 370. It's not someone's cat to be in both states at the same time...    
> >>>
> >>> The correct state (IMHO, Miquel correct me if I'm wrong) is `compatible 
> >>> = "marvell,armada-8k-nand-controller";`  as there are some 8K specific 
> >>> requirements that aren't present on the 370 (specifically the 
> >>> system-controller and the 2nd clock).     
> >>
> >> Presence of specific requirements does not invalidate compatibility. Two
> >> devices are compatible if the 8k can bind and work with 370 compatible
> >> string, even if this means some lower performance or less features (e.g.
> >> subset of features).  
> > 
> > Quoting myself from 2019 (comment in the driver):
> > "Some SoCs like A7k/A8k need to enable manually the NAND controller,
> > gated clocks and reset bits to avoid being bootloader dependent."
> > 
> > So can the 8k controller work using a 370 compatible : yes, if the
> > booloader enabled the NAND controller already, no otherwise. But in
> > practice it is the same controller.
> > 
> > Given this information I don't know whether it makes sense to qualify
> > the 8k controller compatible with the 370 compatible or not.  
> 
> Indeed, a bit tricky decision. For me they could stay compatible,
> because apparently programming model is the same or similar.

Agreed, the programming model wrt the controller itself is the same. So
we will expect 8k controllers to be described with both 8k and 370
compatible strings then.

Thanks for your feedback.

Cheers,
Miquèl