| 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: <e457c167-59d2-45c9-8da8-e9fbd1f5e17e@microchip.com>
Date: Mon, 2 Jun 2025 08:59:56 +0000
From: <Balamanikandan.Gunasundar@...rochip.com>
To: <krzk@...nel.org>, <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/25 12:53 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> 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?
Yes. I am just converting the exiting bindings. Hope rephrasing the
commit message is sufficient enough.
>
>> 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