Re: Re: [PATCH v5 1/7] dt-bindings: mtd: Describe Rockchip RK3xxx NAND flash controller

[赵仪峰 <yifeng.zhao@xxxxxxxxxxxxxx>]

Hi Johan,

>Hi Yifeng,
>
>> On Sun, Apr 26, 2020 at 06:02:44PM +0800, Yifeng Zhao wrote:
>>> Documentation support for Rockchip RK3xxx NAND flash controllers
>>>
>>> Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>
>>> ---
>>>
>>> Changes in v5:
>>> - Fix some wrong define
>>> - Add boot-medium define
>>> - Remove some compatible define
>>>
>>> Changes in v4:
>>> - The compatible define with rkxx_nfc
>>> - Add assigned-clocks
>>> - Fix some wrong define
>>>
>>> Changes in v3:
>>> - Change the title for the dt-bindings
>>>
>>> Changes in v2: None
>>>
>>>  .../mtd/rockchip,nand-controller.yaml         | 124 ++++++++++++++++++
>>>  1 file changed, 124 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml b/Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml
>
>The name of this file is based on Miquel's opinion, but the
>compatibility strings, (for which robh has given a 'reviewed by' tag) in
>version 4 don't fit with this format.
>
>>> new file mode 100644
>>> index 000000000000..12354c79d275
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml
>>> @@ -0,0 +1,124 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mtd/rockchip,nand-controller.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Rockchip SoCs NAND FLASH Controller (NFC)
>>> +
>
>>> +allOf:
>>> +  - $ref: "nand-controller.yaml#"
>
>The idea of a common file is that you add additional properties that are
>not already included. This document has a more restricting character.
>Therefore you must the same property names and patterns to be effective.
>See comment about "^nand@[0-3]$".

Rob Herring replied for V3:
Based on reg, should be only '[0-3]'

>>> +
>>> +maintainers:
>>> +  - Heiko Stuebner <heiko@xxxxxxxxx>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - rockchip,px30_nfc
>>> +      - rockchip,rk3xxx_nfc
>
>As told before the binding strings are SoC orientated.
>Use the Soc first in line for V600 and replace
>'rockchip,rk3xxx_nfc' by 'rockchip,rk3066-nfc'

RK3066,RK3188 and RK2928 are included the same dtsi：arch\arm\boot\dts\rk3xxx.dtsi,
So I can define  only one compatible 'rockchip,rk3xxx-nfc' for this three SoCs。
Or give me a appropriate definition.

>>> +      - rockchip,rk3308_nfc
>>> +      - rockchip,rv1108_nfc
>>
>> Use '-', not '_'.
>>
>
>In your driver there are currently 3 data sets:
>V622, V800, V900
>Each additional SoC will then use a fallback string.
>
>properties:
>  compatible:
>    oneOf:
>      - const: rockchip,px30-nfc
>      - const: rockchip,rk3066-nfc
>      - const: rockchip,rk3308-nfc
>      - Item:
>          - enum:
>              - rockchip,rv1108-nfc
>          - const: rockchip,rk3308-nfc
>
>I propose to also include a V600 data set in the driver and an extra dts
>entry for rk3288 to this serie. Add support for CS 8 to your driver.

The V600 and v622 are similar. The main difference is that V600 only has AHB clock, 
while v622 has AHB and NFC clocks. 

>properties:
>  compatible:
>    oneOf:
>      - const: rockchip,px30-nfc
>      - const: rockchip,rk3066-nfc
>      - const: rockchip,rk3288-nfc
>      - const: rockchip,rk3308-nfc
>      - Item:
>          - enum:
>              - rockchip,rk3368-nfc
>          - const: rockchip,rk3288-nfc
>      - Item:
>          - enum:
>              - rockchip,rv1108-nfc
>          - const: rockchip,rk3308-nfc
>
>
>>> +
>>> +  reg:
>
>>> +    minItems: 1
>
>Change this back to version 4:
>
>    maxItems: 1
>
>>> +
>>> +  interrupts:
>
>>> +    minItems: 1
>
>Change this back to version 4:
>
>    maxItems: 1
>
>>> +
>>> +  clocks:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: Bus Clock
>>> +      - description: Module Clock
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>
>> So 'ahb' is required and 'nfc' is optional? That's what you defined, but
>> that seems backwards.
>
>This is needed for rk3066 V600.
>
>>
>>> +    items:
>>> +      - const: ahb
>>> +      - const: nfc
>>> +
>
>Also the use of allOf doesn't check for bogus properties without the use
>of: 'additionalProperties: false'. Check this document by combining it
>into a single file and add additionalProperties.
>
>  assigned-clocks:
>    maxItems: 1
>
>  assigned-clock-rates:
>    maxItems: 1
>
>  pinctrl-0:
>    maxItems: 1
>
>  pinctrl-names:
>    const: default
>
>>> +patternProperties:
>
>>> +  "^nand@[0-3]$":
>
>In combination with $ref: "nand-controller.yaml#" you create 2 reg-exes.
>One with:
>"^nand@[0-3]$" + minimum 0 and maximum 3
>
>A second with:
>"^nand@[a-f0-9]$" + no restrictions
>
>Result all pass, so use the same regex as in the common file.
>Don't try to restrict both in the regex and in the reg properties.
>
>>> +    type: object
>>> +    properties:
>>> +      reg:
>>> +        minimum: 0
>>> +        maximum: 3

All the nfc can support 8 cs，but the cs io limit to 1/2/4/8 for different SoCs.
Maybe use 'maximun: 7' for all the nfc is okay.

      reg:
        minimum: 0
        maximum: 7

>V600 has CS 8.
>Maybe use this if a V600 data set is included:
>
>if:
>  properties:
>    compatible:
>      contains:
>        const: rockchip,rk3066-nfc
>
>then:
>      reg:
>        minimum: 0
>        maximum: 7
>
>else:
>      reg:
>        minimum: 0
>        maximum: 3
>
>>> +
>>> +      nand-ecc-mode:
>>> +        const: hw
>>> +
>>> +      nand-ecc-step-size:
>>> +        const: 1024
>>> +
>>> +      nand-ecc-strength:
>
>>> +        enum: [16,24,40,60,70]
>
>Add space             ^  ^  ^  ^


>        enum: [16, 24, 40, 60, 70]
>
>>> +
>>> +      nand-bus-width:
>>> +        const: 8
>>> +
>
>>> +      nand-is-boot-medium: true
>
>Nothing changed. Already in nand-controller.yaml => remove
>
>>> +
>>> +      rockchip-boot-blks:
>>
>> rockchip,boot-blks
>>
>>> +        minimum: 2
>>> +        default: 16
>>> +        allOf:
>>> +        - $ref: /schemas/types.yaml#/definitions/uint32
>>> +        description:
>>> +          For legacy devices where the bootrom can only handle 16/24 bit
>>> +          BCH/ECC, and for some other devices where the bootrom can support
>>> +          60/70 bit BCH/ECC.
>>> +          In addition, when programming the loader, a linked list needs to
>
>Could you use a better description?                          ^
>Is this a bit, byte, word, pointer or custom and at what position?
>
>>> +          be written in oob for Bootrom to read the correct data sequence.
>>> +          If specified it indicates the number of erase blocks in use by
>>> +          the bootloader that need a different BCH/ECC setting.
>>> +          Only used in combination with 'nand-is-boot-medium'.
>
>Could you disclose the flow/response of the bootrom if we hit a bad
>block? Does it mark that block bad?
>
>Describe why we have a minimum of 2 (1 standard + 1 spare block).
>Does the bootrom for V600, V622 have a maximum from the software point
>of view?

The bootorm does not check the bad block mask, it will try to read next block if the ecc
error occur in current block while load data. 
The maximun blocks that bootrom will scan is bigger than 16, but it is no need
to define a large value, otherwise it will waste nand flash space.

I recommend at least has two copys for loader considering to OTA upgrade requirements.
If we consider the bad block, may be need at least 3 or 4 blocks.

>>> +
>>> +      rockchip-boot-ecc-strength:
>>
>> rockchip,boot-ecc-strength
>>
>>> +        enum: [16,24,40,60,70]
>
>Add space             ^  ^  ^  ^
>
>        enum: [16, 24, 40, 60, 70]
>
>>> +        description:
>>> +          If specified it indicates that use a different BCH/ECC setting for
>>> +          bootrom.
>
>The phrase above is in need for some improvement.
>Could an English speaker help here?
>
>If specified it indicates that a different BCH/ECC setting is used by
>the bootrom.
>
>If specified it describes the BCH/ECC setting used by the bootrom.
>
>>> +          Only used in combination with 'nand-is-boot-medium'.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +  - clock-names
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/rk3308-cru.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    nfc: nand-controller@ff4b0000 {
>>> +      compatible = "rockchip,rk3308_nfc";
>>> +      reg = <0x0 0xff4b0000 0x0 0x4000>;
>>> +      interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
>>> +      clocks = <&cru HCLK_NANDC>, <&cru SCLK_NANDC>;
>>> +      clock-names = "ahb", "nfc";
>>> +      assigned-clocks = <&clks SCLK_NANDC>;
>>> +      assigned-clock-rates = <150000000>;
>>> +
>>> +      pinctrl-0 = <&flash_ale &flash_bus8 &flash_cle &flash_csn0
>>> +                   &flash_rdn &flash_rdy &flash_wrn>;
>>> +      pinctrl-names = "default";
>>> +
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      nand@0 {
>>> +        reg = <0>;
>>> +        nand-bus-width = <8>;
>>> +        nand-ecc-mode = "hw";
>
>>> +        nand-ecc-strength = <16>;
>>> +        nand-ecc-step-size = <1024>;
>
>sort
>
>>> +        nand-is-boot-medium;
>>> +        rockchip-boot-blks = <8>;
>>> +        rockchip-boot-ecc-strength = <16>;
>>> +      };
>>> +    };
>>> +
>>> +...
>>> --
>>> 2.17.1
>>>
>>>
>>>
>
>
>
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/