| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3825dcb6-00b1-4e03-ab1a-258bcd3265ba@kernel.org>
Date: Mon, 2 Jun 2025 09:23:53 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Balamanikandan Gunasundar <balamanikandan.gunasundar@...rochip.com>,
miquel.raynal@...tlin.com, richard@....at, vigneshr@...com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, nicolas.ferre@...rochip.com,
alexandre.belloni@...tlin.com, claudiu.beznea@...on.dev,
krzysztof.kozlowski+dt@...aro.org
Cc: linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/4] dt-bindings: mtd: atmel-nand: add legacy nand
controllers
On 02/06/2025 07:35, Balamanikandan Gunasundar wrote:
> Add support for atmel legacy nand controllers. These bindings should not be
No new support for legacy bindings. Both your commit msg and subject do
not describe what you do here. I see you convert EXISTING bindings
instead of adding support. But if you insist on adding, that would be
NAKed because why would we want to accept new stuff which is already
deprecated?
> used with the new device trees.
>
> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@...rochip.com>
> ---
> .../devicetree/bindings/mtd/atmel-nand.txt | 116 ------------
> .../devicetree/bindings/mtd/atmel-nand.yaml | 167 ++++++++++++++++++
Filename matching compatible. Also look at your 4/4 patch and compare
what is here and there.
> 2 files changed, 167 insertions(+), 116 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/mtd/atmel-nand.txt
> create mode 100644 Documentation/devicetree/bindings/mtd/atmel-nand.yaml
>
...
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.yaml b/Documentation/devicetree/bindings/mtd/atmel-nand.yaml
> new file mode 100644
> index 000000000000..a437d40a523f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.yaml
> @@ -0,0 +1,167 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/atmel-nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel NAND flash controller
> +
> +maintainers:
> + - Balamanikandan Gunasundar <balamanikandan.gunasundar@...rochip.com>
> +
> +description:
> + Atmel nand flash controller. These are legacy bindings and
> + deprecated. Find the latest in microchip,nand-controller.yaml
> +
Missing allOf/ref to nand-controller
> +properties:
> + $nodename:
> + pattern: "^nand(@.*)?"
Drop
> +
> + compatible:
> + enum:
> + - atmel,at91rm9200-nand
> + - atmel,sama5d2-nand
> + - atmel,sama5d4-nand
> +
> + reg:
> + description:
> + The localbus address and size used for the chip, and hardware ECC
> + controller if available. If the hardware ECC is PMECC, it should
> + contain address and size for PMECC and PMECC Error Location
> + controller. The PMECC lookup table address and size in ROM is
> + optional. If not specified, driver will build it in runtime.
> +
> + nand-on-flash-bbt:
> + description:
> + enable on flash bbt option if not present false
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + nand-ecc-mode:
> + description:
> + operation mode of the NAND ecc
> + enum:
> + [none, soft, hw, hw_syndrome, hw_oob_first, soft_bch]
> + default: soft
> + $ref: /schemas/types.yaml#/definitions/string
> +
> + nand-bus-width:
> + description:
> + nand bus width
> + enum:
> + [8, 16]
> + default: 8
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 1
You have several redundant properties. What's more, you are basically
re-definingn them. Drop and keep only constraints. Look at other
bindings and follow how they are doing this.
> +
> + gpios:
> + minItems: 2
> + items:
> + - description: Ready/Busy
> + - description: Chip Enable
> + - description: Optional Card detect GPIO; can be 0 if unused
> +
> + atmel,nand-addr-offset:
> + description:
> + offset for the address latch.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 31
> +
> + atmel,nand-cmd-offset:
> + description:
> + offset for the command latch.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 31
> +
> + atmel,nand-has-dma:
> + description:
> + support dma transfer for nand read/write.
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + atmel,has-pmecc:
> + description:
> + enable Programmable Multibit ECC hardware, capable of BCH encoding
> + and decoding, on devices where it is present.
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + atmel,pmecc-cap:
> + description:
> + error correct capability for Programmable Multibit ECC Controller.
> + enum:
> + [2, 4, 8, 12, 24, 32]
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + atmel,pmecc-sector-size:
> + description:
> + sector size for ECC computation.
> + enum:
> + [512, 1024]
> + default: 512
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +
Just one blank line
> + atmel,pmecc-lookup-table-offset:
> + description:
> + Two offsets of lookup table in ROM for different sector size. First
> + one is for sector size 512, the next is for sector size 1024. If not
> + specified, driver will build the table in runtime.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + default: 512
> +
> +required:
> + - compatible
> + - reg
> + - atmel,nand-addr-offset
> + - atmel,nand-cmd-offset
> + - "#address-cells"
> + - "#size-cells"
Use consistent quotes, either ' or "
> +
> +unevaluatedProperties: false
Without $ref this makes no sense, so it clearly points you to missing ref.
> +
> +examples:
> + - |
> + nand@...00000 {
> + compatible = "atmel,at91rm9200-nand";
> + #address-cells = <1>;
> + #size-cells = <1>;
Follow DTS coding style.
> + reg = <0x40000000 0x10000000
> + 0xffffe800 0x200>;
These are two entries, not one.
> + atmel,nand-addr-offset = <21>; /* ale */
> + atmel,nand-cmd-offset = <22>; /* cle */
> + nand-on-flash-bbt;
> + nand-ecc-mode = "soft";
> + gpios = <&pioC 13 0 /* rdy */
> + &pioC 14 0 /* nce */
> + 0 /* cd */
All this is not following standard style. Two entries. Use proper
defines for GPIO flags.
> + >;
> + };
> + - |
> + /* for PMECC supported chips */
> + nand@...00000 {
> + compatible = "atmel,at91rm9200-nand";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x40000000 0x10000000 /* bus addr & size */
> + 0xffffe000 0x00000600 /* PMECC addr & size */
> + 0xffffe600 0x00000200 /* PMECC ERRLOC addr & size */
> + 0x00100000 0x00100000>; /* ROM addr & size */
Four entries, not one.
> +
> + atmel,nand-addr-offset = <21>; /* ale */
> + atmel,nand-cmd-offset = <22>; /* cle */
> + nand-on-flash-bbt;
> + nand-ecc-mode = "hw";
> + atmel,has-pmecc; /* enable PMECC */
> + atmel,pmecc-cap = <2>;
> + atmel,pmecc-sector-size = <512>;
> + atmel,pmecc-lookup-table-offset = <0x8000 0x10000>;
> + gpios = <&pioD 5 0 /* rdy */
> + &pioD 4 0 /* nce */
> + 0 /* cd */
> + >;
> + };
Best regards,
Krzysztof
Powered by blists - more mailing lists