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

[Chris Packham]

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@xxxxxxxxxxx>
>>
>> Switch the DT binding to a YAML schema to enable the DT validation.
>>
>> The text binding didn't mention it as a requirement but existing usage
>> has
>>
>>     compatible = "marvell,armada-8k-nand-controller",
>>                  "marvell,armada370-nand-controller";
>>
>> so the YAML allows this in addition to the individual compatible values.
>>
>> There was also an incorrect reference to dma-names being "rxtx" where
>> the driver and existing device trees actually use dma-names = "data" so
>> this is corrected in the conversion.
>>
>> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> Notes:
>>      Changes in v9:
>>      - depend on series from Miquel https://lore.kernel.org/linux-mtd/20230606175246.190465-1-miquel.raynal@xxxxxxxxxxx/
>>      - 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
>