Re: [PATCH v6 1/2] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

quoted

Hi Krzysztof,

krzysztof.kozlowski@linaro.org wrote on Tue, 30 May 2023 14:24:22 +0200:

On 30/05/2023 02:53, Chris Packham wrote:
From: Vadym Kochan <redacted>

Switch the DT binding to a YAML schema to enable the DT validation.

Dropped deprecated compatibles and properties described in txt file.

Signed-off-by: Vadym Kochan <redacted>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    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  
Where are they? Did you drop them in v6?
label and partitions are defined in partitions/partition.yaml,
referenced by partitions.yaml, referenced by mtd.yaml, referenced by
nand-chip.yaml, referenced by nand-controller.yaml, itself referenced
in this file :-)

So I believe there is nothing else to add in the controller's binding
for these two properties? They are very generic, it would not be
optimal if we had to take care about them.

          ref to the "partition.yaml" which was wrongly used.  

    
       2) Add "additionalProperties: false" for nand@ because all possible
          properties are described.  
Where? This cannot be silently dropped!

    
    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 | 190 ++++++++++++++++++
 .../devicetree/bindings/mtd/marvell-nand.txt  | 126 ------------
 MAINTAINERS                                   |   1 -
 3 files changed, 190 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..c4b003f5fa9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/marvell,nand-controller.yaml
@@ -0,0 +1,190 @@
+# 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 <miquel.raynal@bootlin.com>  
Is it correct person for Marvell NAND? This should be not a subsystem
maintainer, but a device maintainer.
I did not bother converting this file yet but I actually rewrote the
corresponding Linux driver (5 years ago) entirely so I don't mind.


+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: marvell,armada-8k-nand-controller
+          - const: marvell,armada370-nand-controller
I don't think we ever wanted having these two compatibles to describe a
single hardware block?

+      - enum:
+          - marvell,armada370-nand-controller
+          - marvell,pxa3xx-nand-controller  
You miss here deprecated compatibles, which are BTW still used. Don't
drop properties and compatibles during conversion.

+
+  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:  
Missing minItems: 1

+    items:
+      - const: core
+      - const: reg
+
+  dmas:
+    maxItems: 1
+
+  dma-names:
+    items:
+      - const: rxtx
+
+  marvell,system-controller:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Syscon node that handles NAND controller related registers
+
+patternProperties:
+  "^nand@[0-3]$":
+    type: object  
Missing unevaluatedProperties: false on this level.

+    properties:
+      reg:
+        minimum: 0
+        maximum: 3
Same as below, it is an array as well IIRC.

+
+      nand-rb:
+        minimum: 0
+        maximum: 1  
It's an array, so this does not sound right. You might want to put it
under items:.  Then you also miss min/maxItems.
That's true, you can have either one or two members with the value
[0-1], so you need both.

+
+      nand-ecc-step-size:
+        const: 512
+
+      nand-ecc-strength:
+        enum: [1, 4, 8]
The controller (and the driver) actually supports 1, 4, 8, 12, 16.

+
+      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) so whether or not this property is set,
+          the bit is selected by the driver.
Maybe we should slightly rephrase this to avoid driver related
information.

+        $ref: /schemas/types.yaml#/definitions/flag
+        deprecated: true
+
+    required:
+      - reg
+      - nand-rb
+
+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
+          maxItems: 2  
Drop maxItems. You don't have it in clock-names.

+
+        clock-names:
+          minItems: 2
+
+      required:
+        - marvell,system-controller
+    else:
+      properties:
+        clocks:
+          maxItems: 1
+
+        clock-names:
+          maxItems: 1  
I doubt that you tested it in above variant...

+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    nand_controller: nand-controller@d0000 {
+        compatible = "marvell,armada370-nand-controller";
+        reg = <0xd0000 0x54>;
+        #address-cells = <1>;
+        #size-cells = <0>;  

Best regards,
Krzysztof
Thanks for doing this!

Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help