| Message ID | 20200426100250.14678-2-yifeng.zhao@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Rockchip NFC drivers for RK3308 and others | expand |
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@rock-chips.com> > --- > > 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 > 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#" > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + > +properties: > + compatible: > + enum: > + - rockchip,px30_nfc > + - rockchip,rk3xxx_nfc > + - rockchip,rk3308_nfc > + - rockchip,rv1108_nfc Use '-', not '_'. > + > + reg: > + minItems: 1 > + > + interrupts: > + minItems: 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. > + items: > + - const: ahb > + - const: nfc > + > +patternProperties: > + "^nand@[0-3]$": > + type: object > + properties: > + 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] > + > + nand-bus-width: > + const: 8 > + > + nand-is-boot-medium: true > + > + 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 > + 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'. > + > + rockchip-boot-ecc-strength: rockchip,boot-ecc-strength > + enum: [16,24,40,60,70] > + description: > + If specified it indicates that use a different BCH/ECC setting for > + 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>; > + nand-is-boot-medium; > + rockchip-boot-blks = <8>; > + rockchip-boot-ecc-strength = <16>; > + }; > + }; > + > +... > -- > 2.17.1 > > >
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@rock-chips.com> >> --- >> >> 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]$". >> + >> +maintainers: >> + - Heiko Stuebner <heiko@sntech.de> >> + >> +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' >> + - 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. 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 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? >> + >> + 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 >> >> >>
Hi Johan, Johan Jonker <jbx6244@gmail.com> wrote on Wed, 29 Apr 2020 10:53:30 +0200: > 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@rock-chips.com> > >> --- > >> > >> 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. What do you mean? Is the file name restricted somehow? I just don't want a compatible with just "nand" in it because this word is too vague as it defines: a bus, a spec, a chip, people are also confusing it with the controller and sometimes with the ECC engine too. "nfc" is okay though. Thanks, Miquèl
Hi Miquel, On 4/29/20 11:13 AM, Miquel Raynal wrote: > Hi Johan, > > Johan Jonker <jbx6244@gmail.com> wrote on Wed, 29 Apr 2020 10:53:30 > +0200: > >> 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@rock-chips.com> >>>> --- >>>> >>>> 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. > > What do you mean? Is the file name restricted somehow? I just don't > want a compatible with just "nand" in it because this word is too vague > as it defines: a bus, a spec, a chip, people are also confusing it with > the controller and sometimes with the ECC engine too. "nfc" is okay > though. > > Thanks, > Miquèl > With the review of my binding string rockchip,rk3066-hdmi robh advised to use the binding compatible string and add '.txt' to it for the file name. Is it OK for you to have a file name: rockchip,nand-controller.yaml and a little bit different compatibility string: rockchip,rk3066-nfc Kind regards, Johan
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@rock-chips.com> >>> --- >>> >>> 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@sntech.de> >>> + >>> +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 >>> >>> >>> > > > >
Hi Yifeng, Heiko, A few more comments based on version 5 (part 2). On 4/26/20 12:02 PM, Yifeng Zhao wrote: > Documentation support for Rockchip RK3xxx NAND flash controllers > > Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com> > --- > > 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 > 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#" > + > +maintainers: > + - Heiko Stuebner <heiko@sntech.de> > + > +properties: > + compatible: > + enum: > + - rockchip,px30_nfc > + - rockchip,rk3xxx_nfc > + - rockchip,rk3308_nfc > + - rockchip,rv1108_nfc Based on what's available in mainline and your info. For Heiko? Is this correct? compatible: oneOf: - const: rockchip,px30-nfc - const: rockchip,rk2928-nfc - const: rockchip,rk3308-nfc - items: - const: rockchip,rk3326-nfc - const: rockchip,px30-nfc - items: - enum: - rockchip,rk3036-nfc - rockchip,rk3228-nfc - rockchip,rk3288-nfc - const: rockchip,rk2928-nfc - items: - const: rockchip,rv1108-nfc - const: rockchip,rk3308-nfc static const struct of_device_id rk_nfc_id_table[] = { {.compatible = "rockchip,px30_nfc", .data = &nfc_v9_cfg }, {.compatible = "rockchip,rk2928-nfc", .data = &nfc_v6_cfg }, {.compatible = "rockchip,rk3308_nfc", .data = &nfc_v8_cfg }, { /* sentinel */ }, }; > + > + reg: > + minItems: 1 maxItems: 1 > + > + interrupts: > + minItems: 1 maxItems: 1 > + > + clocks: > + minItems: 1 > + items: > + - description: Bus Clock > + - description: Module Clock > + > + clock-names: > + minItems: 1 > + items: > + - const: ahb > + - const: nfc assigned-clocks: maxItems: 1 assigned-clock-rates: maxItems: 1 pinctrl-0: maxItems: 1 pinctrl-names: const: default power-domains: maxItems: 1 'power-domains' is needed for px30. > + > +patternProperties: > + "^nand@[0-3]$": "^nand@[0-7]$": or? "^nand@[a-f0-9]$": > + type: object > + properties: > + reg: > + minimum: 0 > + maximum: 3 maximum: 7 > + > + nand-ecc-mode: > + const: hw > + > + nand-ecc-step-size: > + const: 1024 > + > + nand-ecc-strength: > + enum: [16,24,40,60,70] Not every SoC has the same array. Add description maybe. > + > + nand-bus-width: > + const: 8 > + > + nand-is-boot-medium: true > + ? With 2 regexes nand-is-boot-medium is maybe needed, but I'm not able to successful test that with a common file? Keep or not? ? dependencies: rockchip,boot-blks: [ nand-is-boot-medium ] rockchip,boot-ecc-strength: [ nand-is-boot-medium ] > + 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 > + 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'. > + > + rockchip-boot-ecc-strength: > + enum: [16,24,40,60,70] Not every SoC has the same array. Add description maybe. > + description: > + If specified it indicates that use a different BCH/ECC setting for > + 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"; 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>; TEST1: Change this in the example: nand@4 { reg = <4>; make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml Result: nothing TEST2: Change "^nand@[0-3]$" to "^nand@[a-f0-9]$" make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml Result: Documentation/devicetree/bindings/mtd/rockchip,nand-controller.example.dt.yaml: nand-controller@ff4b0000: nand@4:reg:0:0: 4 is greater than the maximum of 3 SCHEMA Documentation/devicetree/bindings/processed-schema.yaml Conclusion: ? > + nand-bus-width = <8>; > + nand-ecc-mode = "hw"; > + nand-ecc-strength = <16>; > + nand-ecc-step-size = <1024>; > + nand-is-boot-medium; > + rockchip-boot-blks = <8>; > + rockchip-boot-ecc-strength = <16>; rockchip,boot-blks = <8>; rockchip,boot-ecc-strength = <16>; > + }; > + }; > + > +... >
diff --git a/Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml b/Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml 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#" + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + +properties: + compatible: + enum: + - rockchip,px30_nfc + - rockchip,rk3xxx_nfc + - rockchip,rk3308_nfc + - rockchip,rv1108_nfc + + reg: + minItems: 1 + + interrupts: + minItems: 1 + + clocks: + minItems: 1 + items: + - description: Bus Clock + - description: Module Clock + + clock-names: + minItems: 1 + items: + - const: ahb + - const: nfc + +patternProperties: + "^nand@[0-3]$": + type: object + properties: + 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] + + nand-bus-width: + const: 8 + + nand-is-boot-medium: true + + 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 + 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'. + + rockchip-boot-ecc-strength: + enum: [16,24,40,60,70] + description: + If specified it indicates that use a different BCH/ECC setting for + 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>; + nand-is-boot-medium; + rockchip-boot-blks = <8>; + rockchip-boot-ecc-strength = <16>; + }; + }; + +...
Documentation support for Rockchip RK3xxx NAND flash controllers Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com> --- 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