[PATCH v5 0/7] Add Rockchip NFC drivers for RK3308 and others

[PATCH v5 0/7] Add Rockchip NFC drivers for RK3308 and others

[Yifeng Zhao]

Rockchp's NFC(Nand Flash Controller) has three versions: V6, V8 and V9.
This series patch can support all three versions.


Changes in v5:
- Fix some wrong define
- Add boot-medium define
- Remove some compatible define
- Add boot blocks support  with different ecc for bootrom
- Rename rockchip-nand.c to rockchip-nand-controller.c
- Unification of other variable names
- Remove some compatible define

Changes in v4:
- The compatible define with rkxx_nfc
- Add assigned-clocks
- Fix some wrong define
- Define platform data structure for the register offsets.
- The compatible define with rkxx_nfc.
- Use SET_SYSTEM_SLEEP_PM_OPS to define PM_OPS.
- Use exec_op instead of legacy hooks.

Changes in v3:
- Change the title for the dt-bindings

Changes in v2:
- Fix compile error.
- Include header files sorted by file name

Yifeng Zhao (7):
  dt-bindings: mtd: Describe Rockchip RK3xxx NAND flash controller
  mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others
  MAINTAINERS: add maintainers to rockchip nfc
  arm64: dts: rockchip: Add nfc dts for RK3308 SOC
  arm64: dts: rockchip: Add nfc dts for PX30 SOC
  xarm: dts: rockchip: Add nfc dts for RV1108 SOC
  arm: dts: rockchip: Add nfc dts for RK3066 and RK3188 SOC

 .../mtd/rockchip,nand-controller.yaml         |  124 ++
 MAINTAINERS                                   |    4 +-
 arch/arm/boot/dts/rk3xxx.dtsi                 |    9 +
 arch/arm/boot/dts/rv1108.dtsi                 |   11 +
 arch/arm64/boot/dts/rockchip/px30.dtsi        |   15 +
 arch/arm64/boot/dts/rockchip/rk3308.dtsi      |   14 +
 drivers/mtd/nand/raw/Kconfig                  |    7 +
 drivers/mtd/nand/raw/Makefile                 |    1 +
 .../mtd/nand/raw/rockchip-nand-controller.c   | 1344 +++++++++++++++++
 9 files changed, 1527 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/rockchip,nand-controller.yaml
 create mode 100644 drivers/mtd/nand/raw/rockchip-nand-controller.c

-- 
2.17.1

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

[Yifeng Zhao]

Documentation support for Rockchip RK3xxx NAND flash controllers

Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
+
+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>;
+      };
+    };
+
+...
-- 
2.17.1

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

[Rob Herring]

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> +
> +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
> 
> 
> 

Re: Re: [PATCH v5 1/7] dt-bindings: mtd: Describe Rockchip RK3xxx NAND flash controller【请注意，邮件由robherring2@gmail.com代发】

[赵仪峰]

Hi Rob,

>So 'ahb' is required and 'nfc' is optional? That's what you defined, but
>that seems backwards.

For some old devices, sush as RK3066, RK3188, only have ahb clock(Internal clock and external io clock).
For other devices, sush as RV1108, PX30，RK3288， need both ahb clock(internal clock) and nfc clock(external io clock, can be flexibly configured).
So it is difficult to define it and the document defined only check abh clock.

Thanks，
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
>> 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
>>
>>
>>
>
>
>

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

[Johan Jonker]

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>> 
>> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
>> +
>> +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
>> 
>> 
>>

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

[Miquel Raynal]

Hi Johan,

Johan Jonker <jbx6244-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> ---
> >> 
> >> 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

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

[Johan Jonker]

Hi Miquel,

On 4/29/20 11:13 AM, Miquel Raynal wrote:
> Hi Johan,
> 
> Johan Jonker <jbx6244-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>> ---
>>>>
>>>> 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

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

[赵仪峰]

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
>>>
>>>
>>>
>
>
>
>

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

[Johan Jonker]

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> +
> +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>;

> +      };
> +    };
> +
> +...
> 

[PATCH v5 3/7] MAINTAINERS: add maintainers to rockchip nfc

[Yifeng Zhao]

Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 58725773cec4..187415b1481a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2308,11 +2308,11 @@ L:	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git
 F:	Documentation/devicetree/bindings/i2c/i2c-rk3x.yaml
-F:	Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
-F:	Documentation/devicetree/bindings/spi/spi-rockchip.yaml
+F:	Documentation/devicetree/bindings/*/*rockchip*.yaml
 F:	arch/arm/boot/dts/rk3*
 F:	arch/arm/boot/dts/rv1108*
 F:	arch/arm/mach-rockchip/
+F:	drivers/*/*/*/*rockchip*
 F:	drivers/*/*/*rockchip*
 F:	drivers/*/*rockchip*
 F:	drivers/clk/rockchip/
-- 
2.17.1

Re: [PATCH v5 3/7] MAINTAINERS: add maintainers to rockchip nfc

[Johan Jonker]

Hi Yifeng,

Thank you for version 5.
A test with the command below gives this warning:

WARNING: Missing commit description - Add an appropriate one

WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic
order
#21: FILE: MAINTAINERS:2314:
 F:	Documentation/devicetree/bindings/i2c/i2c-rk3x.yaml
+F:	Documentation/devicetree/bindings/*/*rockchip*.yaml

./scripts/checkpatch.pl --strict
0003-MAINTAINERS-add-maintainers-to-rockchip-nfc.patch

The enties of MAINTAINERS have been sort with this command:

./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS
--order

On 4/26/20 12:02 PM, Yifeng Zhao wrote:

Add a commit description.

> Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  MAINTAINERS | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58725773cec4..187415b1481a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2308,11 +2308,11 @@ L:	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>  S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git

>  F:	Documentation/devicetree/bindings/i2c/i2c-rk3x.yaml
> -F:	Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.yaml
> -F:	Documentation/devicetree/bindings/spi/spi-rockchip.yaml
> +F:	Documentation/devicetree/bindings/*/*rockchip*.yaml

The script likes to see a different order here.

>  F:	arch/arm/boot/dts/rk3*
>  F:	arch/arm/boot/dts/rv1108*
>  F:	arch/arm/mach-rockchip/
> +F:	drivers/*/*/*/*rockchip*
>  F:	drivers/*/*/*rockchip*
>  F:	drivers/*/*rockchip*
>  F:	drivers/clk/rockchip/
> 

[PATCH v5 4/7] arm64: dts: rockchip: Add nfc dts for RK3308 SOC

[Yifeng Zhao]

From: Yifeng Zhao <zyf-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Add nfc(nand flash controller) node for RK3308 Soc.

Signed-off-by: Yifeng Zhao <zyf-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm64/boot/dts/rockchip/rk3308.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3308.dtsi b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
index 1a58df6e242b..d99b0ef77fe2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi
@@ -627,6 +627,20 @@
 		status = "disabled";
 	};
 
+	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 = <&cru 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";
+		status = "disabled";
+	};
+
 	cru: clock-controller@ff500000 {
 		compatible = "rockchip,rk3308-cru";
 		reg = <0x0 0xff500000 0x0 0x1000>;
-- 
2.17.1

[PATCH v5 5/7] arm64: dts: rockchip: Add nfc dts for PX30 SOC

[Yifeng Zhao]

Add nfc(nand flash controller) node for PX30 Soc.

Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm64/boot/dts/rockchip/px30.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
index f809dd6d5dc3..749692c21ebe 100644
--- a/arch/arm64/boot/dts/rockchip/px30.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
@@ -969,6 +969,21 @@
 		status = "disabled";
 	};
 
+	nfc: nand-controller@ff3b0000 {
+		compatible = "rockchip,px30_nfc";
+		reg = <0x0 0xff3b0000 0x0 0x4000>;
+		interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru HCLK_NANDC>, <&cru SCLK_NANDC>;
+		clock-names = "ahb", "nfc";
+		assigned-clocks = <&cru SCLK_NANDC>;
+		assigned-clock-rates = <150000000>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&flash_ale &flash_bus8 &flash_cle &flash_cs0
+			     &flash_rdn &flash_rdy &flash_wrn &flash_dqs>;
+		power-domains = <&power PX30_PD_MMC_NAND>;
+		status = "disabled";
+	};
+
 	gpu: gpu@ff400000 {
 		compatible = "rockchip,px30-mali", "arm,mali-bifrost";
 		reg = <0x0 0xff400000 0x0 0x4000>;
-- 
2.17.1

[PATCH v5 6/7] xarm: dts: rockchip: Add nfc dts for RV1108 SOC

[Yifeng Zhao]

Add nfc(nand flash controller) node for RV1108 Soc.

Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm/boot/dts/rv1108.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/rv1108.dtsi b/arch/arm/boot/dts/rv1108.dtsi
index f9cfe2c80791..970d55db867b 100644
--- a/arch/arm/boot/dts/rv1108.dtsi
+++ b/arch/arm/boot/dts/rv1108.dtsi
@@ -451,6 +451,17 @@
 		#reset-cells = <1>;
 	};
 
+	nfc: nand-controller@30100000 {
+		compatible = "rockchip,rv1108_nfc";
+		reg = <0x30100000  0x1000>;
+		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru HCLK_NANDC>, <&cru SCLK_NANDC>;
+		clock-names = "ahb", "nfc";
+		assigned-clocks = <&cru SCLK_NANDC>;
+		assigned-clock-rates = <150000000>;
+		status = "disabled";
+	};
+
 	emmc: mmc@30110000 {
 		compatible = "rockchip,rv1108-dw-mshc", "rockchip,rk3288-dw-mshc";
 		reg = <0x30110000 0x4000>;
-- 
2.17.1

[PATCH v5 7/7] arm: dts: rockchip: Add nfc dts for RK3066 and RK3188 SOC

[Yifeng Zhao]

Add nfc(nand flash controller) node for RK3066 and RK3188 Soc.

Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 arch/arm/boot/dts/rk3xxx.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index f9fcb7e9657b..5333e8a659ec 100644
--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi
@@ -273,6 +273,15 @@
 		status = "disabled";
 	};
 
+	nfc: nand-controller@10500000 {
+		compatible = "rockchip,rk3xxx_nfc";
+		reg = <0x10500000 0x4000>;
+		interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru HCLK_NANDC0>;
+		clock-names = "ahb";
+		status = "disabled";
+	};
+
 	pmu: pmu@20004000 {
 		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
 		reg = <0x20004000 0x100>;
-- 
2.17.1

[PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others

[Yifeng Zhao]

This driver supports Rockchip NFC (NAND Flash Controller) found on RK3308,
RK3368, RKPX30, RV1108 and other SOCs. The driver has been tested using
8-bit NAND interface on the ARM based RK3308 platform.

Support Rockchip NFC versions:
- V6: ECC 16, 24, 40 and 60 bits per 1KB data. Found on RK3066, RK3368,
      RK3229, RK3188, RK3288, RK3128, RKPX3SE, RKPx3, RKPX5, RK3036 and
      RK3126.
- V8: ECC 16 bits per 1KB data. Found on RV1108/7, RK3308.
- V9: ECC 16, 40, 60 and 70 bits per 1KB data. Found on RK3326, RKPX30.

Support feature:
- Read full page data by DMA.
- Support HW ECC(one step is 1KB).
- Support 2 - 32K page size.
- Support 4 CS(depend on Soc)

Limitations:
- Unsupport 512B ecc step.
- Raw page read and write without ecc redundancy code. So could not support
  raw data dump and restore.
- Untested on some SOCs.
- Unsupport subpage.
- Unsupport randomizer.
- The original bad block mask is not supported. It is recommended to use
  the BBT(bad block table).

Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
---

Changes in v5:
- Add boot blocks support  with different ecc for bootrom
- Rename rockchip-nand.c to rockchip-nand-controller.c
- Unification of other variable names
- Remove some compatible define

Changes in v4:
- Define platform data structure for the register offsets.
- The compatible define with rkxx_nfc.
- Use SET_SYSTEM_SLEEP_PM_OPS to define PM_OPS.
- Use exec_op instead of legacy hooks.

Changes in v3: None
Changes in v2:
- Fix compile error.
- Include header files sorted by file name

 drivers/mtd/nand/raw/Kconfig                  |    7 +
 drivers/mtd/nand/raw/Makefile                 |    1 +
 .../mtd/nand/raw/rockchip-nand-controller.c   | 1344 +++++++++++++++++
 3 files changed, 1352 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/rockchip-nand-controller.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..8313b12a9d85 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -433,6 +433,13 @@ config MTD_NAND_MESON
 	  Enables support for NAND controller on Amlogic's Meson SoCs.
 	  This controller is found on Meson SoCs.
 
+config MTD_NAND_ROCKCHIP
+	tristate "Rockchip NAND controller"
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Enables support for NAND controller on Rockchip SoCs.
+
 config MTD_NAND_GPIO
 	tristate "GPIO assisted NAND controller"
 	depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 2d136b158fb7..8bafa59b8940 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
 obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
 obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
 obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o
+obj-$(CONFIG_MTD_NAND_ROCKCHIP)		+= rockchip_nand.o
 
 nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_onfi.o
diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
new file mode 100644
index 000000000000..cf473ed01999
--- /dev/null
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -0,0 +1,1344 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Rockchip NAND Flash controller driver.
+ * Copyright (C) 2020 Rockchip Inc.
+ * Authors: Yifeng Zhao <yifeng.zhao@rock-chips.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/mtd.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+/*
+ * NFC Page Data Layout:
+ *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
+ *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
+ *	......
+ * NAND Page Data Layout:
+ *	1024 * n Data + m Bytes oob
+ * Original Bad Block Mask Location:
+ *	first byte of oob(spare)
+ * nand_chip->oob_poi data layout:
+ *	4Bytes sys data + .... + 4Bytes sys data + ecc data
+ */
+
+/* NAND controller register definition */
+#define	NFC_READ		(0)
+#define	NFC_WRITE		(1)
+#define	NFC_FMCTL		(0x00)
+#define		FMCTL_CE_SEL_M		0xFF
+#define		FMCTL_CE_SEL(x)		(1 << (x))
+#define		FMCTL_WP		BIT(8)
+#define		FMCTL_RDY		BIT(9)
+#define	NFC_FMWAIT		(0x04)
+#define		FLCTL_RST		BIT(0)
+#define		FLCTL_WR		(1)	/* 0: read, 1: write */
+#define		FLCTL_XFER_ST		BIT(2)
+#define		FLCTL_XFER_EN		BIT(3)
+#define		FLCTL_ACORRECT		BIT(10) /* auto correct error bits */
+#define		FLCTL_XFER_READY	BIT(20)
+#define		FLCTL_XFER_SECTOR	(22)
+#define		FLCTL_TOG_FIX		BIT(29)
+#define		BCHCTL_BANK_M	(7 << 5)
+#define		BCHCTL_BANK	(5)
+#define		DMA_ST			BIT(0)
+#define		DMA_WR			(1)	/* 0: write, 1: read */
+#define		DMA_EN			BIT(2)
+#define		DMA_AHB_SIZE		(3)	/* 0: 1, 1: 2, 2: 4 */
+#define		DMA_BURST_SIZE		(6)	/* 0: 1, 3: 4, 5: 8, 7: 16 */
+#define		DMA_INC_NUM		(9)	/* 1 - 16 */
+#define		ECC_ERR_CNT(x, e) ((((x) >> (e).low) & (e).low_mask) \
+			| (((x) >> (e).high) & (e).high_mask) << (e).low_bn)
+#define		INT_DMA			BIT(0)
+#define	NFC_BANK		(0x800)
+#define	NFC_BANK_STEP		(0x100)
+#define		BANK_DATA		(0x00)
+#define		BANK_ADDR		(0x04)
+#define		BANK_CMD		(0x08)
+#define	NFC_SRAM0		(0x1000)
+#define	NFC_SRAM1		(0x1400)
+#define	NFC_SRAM_SIZE		(0x400)
+#define	THIS_NAME		"rk-nand"
+#define	NFC_TIMEOUT		(500000)
+#define	NFC_MAX_OOB_PER_STEP	128
+#define	NFC_MIN_OOB_PER_STEP	64
+#define	MAX_DATA_SIZE		0xFFFC
+#define	MAX_ADDRESS_CYC		6
+#define	NFC_ECC_MAX_MODES	4
+#define	NFC_MAX_NSELS		(4) /* Some Soc only has 1 or 2 CSs */
+#define	NFC_SYS_DATA_SIZE	(4) /* 4 bytes sys data in oob pre 1024 data */
+#define	RK_DEFAULT_CLOCK_RATE	(150 * 1000 * 1000) /* 150 Mhz*/
+#define	ACCTIMING(csrw, rwpw, rwcs) ((csrw) << 12 | (rwpw) << 5 | (rwcs))
+
+enum nfc_type {
+	NFC_V6,
+	NFC_V8,
+	NFC_V9,
+};
+
+/**
+ * struct rk_ecc_cnt_status: represent a ecc status data.
+ * @err_flag_bit: error flag bit index at register.
+ * @low: ecc count low bit index at register.
+ * @low_mask: mask bit.
+ * @low_bn: ecc count low bit number.
+ * @high: ecc count high bit index at register.
+ * @high_mask: mask bit
+ */
+struct ecc_cnt_status {
+	u8 err_flag_bit;
+	u8 low;
+	u8 low_mask;
+	u8 low_bn;
+	u8 high;
+	u8 high_mask;
+};
+
+/**
+ * @type: nfc version
+ * @ecc_strengths: ecc strengths
+ * @ecc_cfgs: ecc config values
+ * @flctl_off: FLCTL register offset
+ * @bchctl_off: BCHCTL register offset
+ * @dma_data_buf_off: DMA_DATA_BUF register offset
+ * @dma_oob_buf_off: DMA_OOB_BUF register offset
+ * @dma_cfg_off: DMA_CFG register offset
+ * @dma_st_off: DMA_ST register offset
+ * @bch_st_off: BCG_ST register offset
+ * @randmz_off: RANDMZ register offset
+ * @int_en_off: interrupt enable register offset
+ * @int_clr_off: interrupt clean register offset
+ * @int_st_off: interrupt status register offset
+ * @oob0_off: oob0 register offset
+ * @oob1_off: oob1 register offset
+ * @ecc0: represent ECC0 status data
+ * @ecc1: represent ECC1 status data
+ */
+struct nfc_cfg {
+	enum nfc_type type;
+	u8 ecc_strengths[NFC_ECC_MAX_MODES];
+	u32 ecc_cfgs[NFC_ECC_MAX_MODES];
+	u32 flctl_off;
+	u32 bchctl_off;
+	u32 dma_cfg_off;
+	u32 dma_data_buf_off;
+	u32 dma_oob_buf_off;
+	u32 dma_st_off;
+	u32 bch_st_off;
+	u32 randmz_off;
+	u32 int_en_off;
+	u32 int_clr_off;
+	u32 int_st_off;
+	u32 oob0_off;
+	u32 oob1_off;
+	struct ecc_cnt_status ecc0;
+	struct ecc_cnt_status ecc1;
+};
+
+struct rk_nfc_nand_chip {
+	struct list_head node;
+	struct nand_chip chip;
+
+	u32 spare_per_sector;
+	u32 oob_buf_per_sector;
+	u32 boot_blks;
+	u32 boot_ecc;
+
+	int nsels;
+	u8 sels[0];
+	/* nothing after this field */
+};
+
+struct rk_nfc_clk {
+	int nfc_rate;
+	struct clk *nfc_clk;
+	struct clk *ahb_clk;
+};
+
+struct rk_nfc {
+	struct nand_controller controller;
+	struct rk_nfc_clk clk;
+
+	struct device *dev;
+	const struct nfc_cfg *cfg;
+	void __iomem *regs;
+
+	int selected_bank;
+	int band_offset;
+
+	struct completion done;
+	struct list_head chips;
+
+	u8 *buffer;
+	u8 *page_buf;
+	u32 *oob_buf;
+
+	unsigned long assigned_cs;
+};
+
+static inline struct rk_nfc_nand_chip *to_rk_nand(struct nand_chip *chip)
+{
+	return container_of(chip, struct rk_nfc_nand_chip, chip);
+}
+
+static inline u8 *data_ptr(struct nand_chip *chip, const u8 *p, int i)
+{
+	return (u8 *)p + i * chip->ecc.size;
+}
+
+static inline u8 *oob_ptr(struct nand_chip *chip, int i)
+{
+	u8 *poi;
+
+	poi = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
+
+	return poi;
+}
+
+static inline int rk_data_len(struct nand_chip *chip)
+{
+	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
+
+	return chip->ecc.size + rk_nand->spare_per_sector;
+}
+
+static inline u8 *rk_data_ptr(struct nand_chip *chip,  int i)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+
+	return nfc->buffer + i * rk_data_len(chip);
+}
+
+static inline u8 *rk_oob_ptr(struct nand_chip *chip, int i)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+
+	return nfc->buffer + i * rk_data_len(chip) + chip->ecc.size;
+}
+
+static void rk_nfc_select_chip(struct nand_chip *chip, int cs)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
+	u32 val;
+
+	if (cs < 0) {
+		nfc->selected_bank = -1;
+		/* Deselect the currently selected target */
+		val = readl_relaxed(nfc->regs + NFC_FMCTL);
+		val &= ~FMCTL_CE_SEL_M;
+		writel(val, nfc->regs + NFC_FMCTL);
+		return;
+	}
+
+	nfc->selected_bank = rk_nand->sels[cs];
+	nfc->band_offset = NFC_BANK + nfc->selected_bank * NFC_BANK_STEP;
+
+	val = readl_relaxed(nfc->regs + NFC_FMCTL);
+	val &= ~FMCTL_CE_SEL_M;
+	val |= FMCTL_CE_SEL(nfc->selected_bank);
+
+	writel(val, nfc->regs + NFC_FMCTL);
+}
+
+static inline void rk_nfc_wait_ioready(struct rk_nfc *nfc)
+{
+	int rc;
+	u32 val;
+
+	rc = readl_poll_timeout_atomic(nfc->regs + NFC_FMCTL, val,
+				       val & FMCTL_RDY, 10, NFC_TIMEOUT);
+	if (rc < 0)
+		dev_err(nfc->dev, "data not ready\n");
+}
+
+static inline u8 rk_nfc_read_byte(struct nand_chip *chip)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+
+	return readb_relaxed(nfc->regs + nfc->band_offset + BANK_DATA);
+}
+
+static void rk_nfc_read_buf(struct nand_chip *chip, u8 *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		buf[i] = rk_nfc_read_byte(chip);
+}
+
+static void rk_nfc_write_byte(struct nand_chip *chip, u8 byte)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+
+	writeb(byte, nfc->regs + nfc->band_offset + BANK_DATA);
+}
+
+static void rk_nfc_write_buf(struct nand_chip *chip, const u8 *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		rk_nfc_write_byte(chip, buf[i]);
+}
+
+static int rk_nfc_cmd(struct nand_chip *chip,
+		      const struct nand_subop *subop)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	u32 cnt = 0;
+	unsigned int i, j, remaining, start;
+	int reg_offset = nfc->band_offset;
+	u8 *inbuf = NULL;
+	const u8 *outbuf;
+	void __iomem *data_reg;
+
+	for (i = 0; i < subop->ninstrs; i++) {
+		const struct nand_op_instr *instr = &subop->instrs[i];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			writeb(instr->ctx.cmd.opcode,
+			       nfc->regs + reg_offset + BANK_CMD);
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			remaining = nand_subop_get_num_addr_cyc(subop, i);
+			start = nand_subop_get_addr_start_off(subop, i);
+
+			for (j = 0; j < 8 && j + start < remaining; j++)
+				writeb(instr->ctx.addr.addrs[j + start],
+				       nfc->regs + reg_offset + BANK_ADDR);
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+		case NAND_OP_DATA_OUT_INSTR:
+			start = nand_subop_get_data_start_off(subop, i);
+			cnt = nand_subop_get_data_len(subop, i);
+			data_reg = nfc->regs + nfc->band_offset + BANK_DATA;
+
+			if (instr->type == NAND_OP_DATA_OUT_INSTR) {
+				outbuf = instr->ctx.data.buf.out + start;
+				for (j = 0; j < cnt; j++)
+					writeb(outbuf[j], data_reg);
+			} else {
+				inbuf = instr->ctx.data.buf.in + start;
+				for (j = 0; j < cnt; j++)
+					inbuf[j] = readb_relaxed(data_reg);
+			}
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			rk_nfc_wait_ioready(nfc);
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static const struct nand_op_parser rk_nfc_op_parser = NAND_OP_PARSER(
+	NAND_OP_PARSER_PATTERN(
+		rk_nfc_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
+		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, MAX_DATA_SIZE)),
+	NAND_OP_PARSER_PATTERN(
+		rk_nfc_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
+		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, MAX_DATA_SIZE),
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
+);
+
+static int rk_nfc_exec_op(struct nand_chip *chip,
+			  const struct nand_operation *op,
+			  bool check_only)
+{
+	rk_nfc_select_chip(chip, op->cs);
+	return nand_op_parser_exec_op(chip, &rk_nfc_op_parser, op,
+				      check_only);
+}
+
+static int rk_nfc_setup_data_interface(struct nand_chip *chip, int csline,
+				       const struct nand_data_interface *conf)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	const struct nand_sdr_timings *timings;
+	u32 rate, tc2rw, trwpw, trw2c;
+	u32 temp;
+
+	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+		return 0;
+
+	/* not onfi nand flash */
+	if (!chip->parameters.onfi)
+		return 0;
+
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return -ENOTSUPP;
+
+	if (IS_ERR(nfc->clk.nfc_clk))
+		rate = clk_get_rate(nfc->clk.ahb_clk);
+	else
+		rate = clk_get_rate(nfc->clk.nfc_clk);
+
+	/* turn clock rate into KHZ */
+	rate /= 1000;
+
+	tc2rw = 1;
+	trw2c = 1;
+
+	trwpw = max(timings->tWC_min, timings->tRC_min) / 1000;
+	trwpw = DIV_ROUND_UP(trwpw * rate, 1000000);
+
+	temp = timings->tREA_max / 1000;
+	temp = DIV_ROUND_UP(temp * rate, 1000000);
+
+	if (trwpw < temp)
+		trwpw = temp;
+
+	/*
+	 * ACCON: access timing control register
+	 * -------------------------------------
+	 * 31:18: reserved
+	 * 17:12: csrw, clock cycles from the falling edge of CSn to the
+		 falling edge of RDn or WRn
+	 * 11:11: reserved
+	 * 10:05: rwpw, the width of RDn or WRn in processor clock cycles
+	 * 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the
+		 rising edge of CSn
+	 */
+	temp = ACCTIMING(tc2rw, trwpw, trw2c);
+	writel(temp, nfc->regs + NFC_FMWAIT);
+
+	return 0;
+}
+
+static int rk_nfc_hw_ecc_setup(struct nand_chip *chip,
+			       struct nand_ecc_ctrl *ecc,
+			       uint32_t strength)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	u32 reg, i;
+
+	for (i = 0; i < NFC_ECC_MAX_MODES; i++) {
+		if (ecc->strength == nfc->cfg->ecc_strengths[i]) {
+			reg = nfc->cfg->ecc_cfgs[i];
+			break;
+		}
+	}
+
+	if (i >= NFC_ECC_MAX_MODES)
+		return -EINVAL;
+
+	writel(reg, nfc->regs + nfc->cfg->bchctl_off);
+
+	return 0;
+}
+
+static void rk_nfc_xfer_start(struct rk_nfc *nfc, u8 rw, u8 n_KB,
+			      dma_addr_t dma_data, dma_addr_t dma_oob)
+{
+	u32 dma_reg, fl_reg, bch_reg;
+
+	dma_reg = DMA_ST | ((!rw) << DMA_WR)  | DMA_EN | (2 << DMA_AHB_SIZE) |
+	      (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM);
+
+	fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT |
+		 (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX;
+
+	if (nfc->cfg->type == NFC_V6 || nfc->cfg->type == NFC_V8) {
+		bch_reg = readl_relaxed(nfc->regs + nfc->cfg->bchctl_off);
+		bch_reg = (bch_reg & (~BCHCTL_BANK_M)) |
+			  (nfc->selected_bank << BCHCTL_BANK);
+		writel(bch_reg, nfc->regs + nfc->cfg->bchctl_off);
+	}
+
+	writel(dma_reg, nfc->regs + nfc->cfg->dma_cfg_off);
+	writel((u32)dma_data, nfc->regs + nfc->cfg->dma_data_buf_off);
+	writel((u32)dma_oob, nfc->regs + nfc->cfg->dma_oob_buf_off);
+	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
+	fl_reg |= FLCTL_XFER_ST;
+	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
+}
+
+static int rk_nfc_wait_for_xfer_done(struct rk_nfc *nfc)
+{
+	u32 reg;
+	int ret = 0;
+	void __iomem *ptr;
+
+	ptr = nfc->regs + nfc->cfg->flctl_off;
+
+	ret = readl_poll_timeout_atomic(ptr, reg,
+					reg & FLCTL_XFER_READY,
+					10, NFC_TIMEOUT);
+	if (ret)
+		dev_err(nfc->dev, "timeout reg=%x\n", reg);
+
+	return ret;
+}
+
+static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			     const u8 *buf, int page, int raw)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
+			NFC_MIN_OOB_PER_STEP;
+	int pages_per_blk = mtd->erasesize / mtd->writesize;
+	int ret = 0, i, boot_rom_mode = 0;
+	dma_addr_t dma_data, dma_oob;
+	u32 reg;
+	u8 *oob;
+
+	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
+
+	if (!raw) {
+		memcpy(nfc->page_buf, buf, mtd->writesize);
+		memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
+
+		/*
+		 * The first 8(some device is 4 or 16) blocks in use by
+		 * the bootloader. And the first 32 bits of oob need link
+		 * to next page address in the same block for Bootrom.
+		 * Swap the first oob with the seventh oob, and bad block
+		 * mask save at seventh oob.
+		 */
+		if (page < pages_per_blk * rk_nand->boot_blks &&
+		    chip->options & NAND_IS_BOOT_MEDIUM) {
+			boot_rom_mode = 1;
+			if (rk_nand->boot_ecc != ecc->strength)
+				rk_nfc_hw_ecc_setup(chip, ecc,
+						    rk_nand->boot_ecc);
+		}
+
+		swap(chip->oob_poi[0], chip->oob_poi[7]);
+
+		for (i = 0; i < ecc->steps; i++) {
+			oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
+			reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
+			      oob[3] << 24;
+			if (!i && boot_rom_mode)
+				reg = (page & (pages_per_blk - 1)) * 4;
+
+			if (nfc->cfg->type == NFC_V6 ||
+			    nfc->cfg->type == NFC_V8)
+				nfc->oob_buf[i * oob_step / 4] = reg;
+			else
+				nfc->oob_buf[i] = reg;
+		}
+
+		dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
+					  mtd->writesize, DMA_TO_DEVICE);
+		dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
+					 ecc->steps * oob_step,
+					 DMA_TO_DEVICE);
+
+		init_completion(&nfc->done);
+		writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
+
+		rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
+				  dma_oob);
+		ret = wait_for_completion_timeout(&nfc->done,
+						  msecs_to_jiffies(100));
+		if (!ret)
+			ret = -ETIMEDOUT;
+		ret = rk_nfc_wait_for_xfer_done(nfc);
+
+		dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
+				 DMA_TO_DEVICE);
+		dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
+				 DMA_TO_DEVICE);
+
+		if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
+			rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
+	} else {
+		rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize);
+	}
+
+	if (ret)
+		return ret;
+
+	ret = nand_prog_page_end_op(chip);
+
+	/* Deselect the currently selected target */
+	rk_nfc_select_chip(chip, -1);
+
+	return ret;
+}
+
+static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
+				 int oob_on, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	u32 i;
+
+	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
+	swap(chip->oob_poi[0], chip->oob_poi[7]);
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (buf)
+			memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
+			       chip->ecc.size);
+
+		memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i),
+		       NFC_SYS_DATA_SIZE);
+	}
+
+	return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
+}
+
+static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
+{
+	return rk_nfc_write_page_raw(chip, NULL, 1, page);
+}
+
+static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+			    u32 data_offs, u32 readlen,
+			    u8 *buf, int page, int raw)
+{
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
+			NFC_MIN_OOB_PER_STEP;
+	int pages_per_blk = mtd->erasesize / mtd->writesize;
+	dma_addr_t dma_data, dma_oob;
+	int ret = 0, i, boot_rom_mode = 0;
+	int bitflips = 0, bch_st;
+	u8 *oob;
+	u32 tmp;
+
+	nand_read_page_op(chip, page, 0, NULL, 0);
+	if (!raw) {
+		dma_data = dma_map_single(nfc->dev, nfc->page_buf,
+					  mtd->writesize,
+					  DMA_FROM_DEVICE);
+		dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
+					 ecc->steps * oob_step,
+					 DMA_FROM_DEVICE);
+
+		if (page < pages_per_blk * rk_nand->boot_blks &&
+		    chip->options & NAND_IS_BOOT_MEDIUM) {
+			boot_rom_mode = 1;
+			if (rk_nand->boot_ecc != ecc->strength)
+				rk_nfc_hw_ecc_setup(chip, ecc,
+						    rk_nand->boot_ecc);
+		}
+
+		init_completion(&nfc->done);
+		writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
+		rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
+				  dma_oob);
+		ret = wait_for_completion_timeout(&nfc->done,
+						  msecs_to_jiffies(100));
+		if (!ret)
+			dev_warn(nfc->dev, "read ahb/dma done timeout\n");
+		rk_nfc_wait_for_xfer_done(nfc);
+		dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
+				 DMA_FROM_DEVICE);
+		dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
+				 DMA_FROM_DEVICE);
+
+		for (i = 0; i < ecc->steps; i++) {
+			oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
+			if (nfc->cfg->type == NFC_V6 ||
+			    nfc->cfg->type == NFC_V8)
+				tmp = nfc->oob_buf[i * oob_step / 4];
+			else
+				tmp = nfc->oob_buf[i];
+			*oob++ = (u8)tmp;
+			*oob++ = (u8)(tmp >> 8);
+			*oob++ = (u8)(tmp >> 16);
+			*oob++ = (u8)(tmp >> 24);
+		}
+		swap(chip->oob_poi[0], chip->oob_poi[7]);
+		for (i = 0; i < ecc->steps / 2; i++) {
+			bch_st = readl_relaxed(nfc->regs +
+					       nfc->cfg->bch_st_off + i * 4);
+			if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
+			    bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
+				mtd->ecc_stats.failed++;
+				bitflips = -1;
+			} else {
+				ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
+				mtd->ecc_stats.corrected += ret;
+				bitflips = max_t(u32, bitflips, ret);
+
+				ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
+				mtd->ecc_stats.corrected += ret;
+				bitflips = max_t(u32, bitflips, ret);
+			}
+		}
+		memcpy(buf, nfc->page_buf, mtd->writesize);
+
+		if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
+			rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
+
+		if (bitflips == -1)
+			dev_err(nfc->dev, "read_page %x %x %x %x %x %x\n",
+				page, bitflips, bch_st, ((u32 *)buf)[0],
+				((u32 *)buf)[1], (u32)dma_data);
+	} else {
+		rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
+	}
+
+	/* Deselect the currently selected target */
+	rk_nfc_select_chip(chip, -1);
+
+	return bitflips;
+}
+
+static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
+				   int oob_on, int page)
+{
+	return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
+}
+
+static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
+				  int pg)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
+}
+
+static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,
+				int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	int i, ret;
+
+	ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
+			       page, 1);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < chip->ecc.steps; i++) {
+		memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i),
+		       NFC_SYS_DATA_SIZE);
+
+		if (buf)
+			memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
+			       chip->ecc.size);
+	}
+	swap(chip->oob_poi[0], chip->oob_poi[7]);
+
+	return ret;
+}
+
+static int rk_nfc_read_oob_std(struct nand_chip *chip, int page)
+{
+	return rk_nfc_read_page_raw(chip, NULL, 1, page);
+}
+
+static inline void rk_nfc_hw_init(struct rk_nfc *nfc)
+{
+	/* disable flash wp */
+	writel(FMCTL_WP, nfc->regs + NFC_FMCTL);
+	/* config default timing */
+	writel(0x1081, nfc->regs + NFC_FMWAIT);
+	/* disable randomizer and dma */
+	writel(0, nfc->regs + nfc->cfg->randmz_off);
+	writel(0, nfc->regs + nfc->cfg->dma_cfg_off);
+	writel(FLCTL_RST, nfc->regs + nfc->cfg->flctl_off);
+}
+
+static irqreturn_t rk_nfc_irq(int irq, void *id)
+{
+	struct rk_nfc *nfc = id;
+	u32 sta, ien;
+
+	sta = readl_relaxed(nfc->regs + nfc->cfg->int_st_off);
+	ien = readl_relaxed(nfc->regs + nfc->cfg->int_en_off);
+
+	if (!(sta & ien))
+		return IRQ_NONE;
+
+	writel(sta, nfc->regs + nfc->cfg->int_clr_off);
+	writel(~sta & ien, nfc->regs + nfc->cfg->int_en_off);
+
+	complete(&nfc->done);
+
+	return IRQ_HANDLED;
+}
+
+static int rk_nfc_enable_clk(struct device *dev, struct rk_nfc_clk *clk)
+{
+	int ret;
+
+	if (!IS_ERR(clk->nfc_clk)) {
+		ret = clk_prepare_enable(clk->nfc_clk);
+		if (ret) {
+			dev_err(dev, "failed to enable nfc clk\n");
+			return ret;
+		}
+	}
+
+	ret = clk_prepare_enable(clk->ahb_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable ahb clk\n");
+		if (!IS_ERR(clk->nfc_clk))
+			clk_disable_unprepare(clk->nfc_clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void rk_nfc_disable_clk(struct rk_nfc_clk *clk)
+{
+	if (!IS_ERR(clk->nfc_clk))
+		clk_disable_unprepare(clk->nfc_clk);
+	clk_disable_unprepare(clk->ahb_clk);
+}
+
+static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
+				 struct mtd_oob_region *oob_region)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section >= chip->ecc.steps)
+		return -ERANGE;
+
+	if (!section) {
+		/* The first byte is bad block mask flag */
+		oob_region->length = NFC_SYS_DATA_SIZE - 1;
+		oob_region->offset = 1;
+	} else {
+		oob_region->length = NFC_SYS_DATA_SIZE;
+		oob_region->offset = section * NFC_SYS_DATA_SIZE;
+	}
+
+	return 0;
+}
+
+static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
+				struct mtd_oob_region *oob_region)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
+	oob_region->length = mtd->oobsize - oob_region->offset;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
+	.free = rk_nfc_ooblayout_free,
+	.ecc = rk_nfc_ooblayout_ecc,
+};
+
+static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	const u8 *strengths = nfc->cfg->ecc_strengths;
+	u8 max_strength, nfc_max_strength;
+	int i;
+
+	nfc_max_strength = nfc->cfg->ecc_strengths[0];
+	/* if optional dt settings not present */
+	if (!ecc->size || !ecc->strength ||
+	    ecc->strength > nfc_max_strength) {
+		/* use datasheet requirements */
+		ecc->strength = chip->base.eccreq.strength;
+		ecc->size = chip->base.eccreq.step_size;
+
+		/*
+		 * align eccstrength and eccsize
+		 * this controller only supports 512 and 1024 sizes
+		 */
+		if (chip->ecc.size < 1024) {
+			if (mtd->writesize > 512) {
+				chip->ecc.size = 1024;
+				chip->ecc.strength <<= 1;
+			} else {
+				dev_err(dev, "ecc.size not supported\n");
+				return -EINVAL;
+			}
+		} else {
+			chip->ecc.size = 1024;
+		}
+
+		ecc->steps = mtd->writesize / ecc->size;
+		max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 / 14;
+		if (max_strength > nfc_max_strength)
+			max_strength = nfc_max_strength;
+
+		for (i = 0; i < 4; i++) {
+			if (max_strength >= strengths[i])
+				break;
+		}
+
+		if (i >= 4) {
+			dev_err(nfc->dev, "unsupported strength\n");
+			return -ENOTSUPP;
+		}
+
+		ecc->strength = strengths[i];
+	}
+	ecc->steps = mtd->writesize / ecc->size;
+	ecc->bytes = DIV_ROUND_UP(ecc->strength * 14, 8);
+	/* HW ECC always work with even numbers of ECC bytes */
+	ecc->bytes = ALIGN(ecc->bytes, 2);
+
+	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
+
+	return 0;
+}
+
+static int rk_nfc_attach_chip(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct device *dev = mtd->dev.parent;
+	struct rk_nfc *nfc = nand_get_controller_data(chip);
+	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	int len;
+	int ret;
+
+	if (chip->options & NAND_BUSWIDTH_16) {
+		dev_err(dev, "16bits buswidth not supported");
+		return -EINVAL;
+	}
+
+	if (ecc->mode != NAND_ECC_HW)
+		return 0;
+
+	ret = rk_nfc_ecc_init(dev, mtd);
+	if (ret)
+		return ret;
+	rk_nand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE;
+
+	/* Check buffer first, avoid duplicate alloc buffer */
+	if (nfc->buffer)
+		return 0;
+
+	len = mtd->writesize + mtd->oobsize;
+	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
+	if (!nfc->buffer)
+		return -ENOMEM;
+
+	nfc->page_buf = nfc->buffer;
+	len = ecc->steps * NFC_MAX_OOB_PER_STEP;
+	nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
+	if (!nfc->oob_buf) {
+		devm_kfree(dev, nfc->buffer);
+		nfc->buffer = NULL;
+		nfc->oob_buf = NULL;
+		return -ENOMEM;
+	}
+
+	chip->ecc.write_page_raw = rk_nfc_write_page_raw;
+	chip->ecc.write_page = rk_nfc_write_page_hwecc;
+	chip->ecc.write_oob_raw = rk_nfc_write_oob_std;
+	chip->ecc.write_oob = rk_nfc_write_oob_std;
+
+	chip->ecc.read_page_raw = rk_nfc_read_page_raw;
+	chip->ecc.read_page = rk_nfc_read_page_hwecc;
+	chip->ecc.read_oob_raw = rk_nfc_read_oob_std;
+	chip->ecc.read_oob = rk_nfc_read_oob_std;
+
+	return 0;
+}
+
+static const struct nand_controller_ops rk_nfc_controller_ops = {
+	.attach_chip = rk_nfc_attach_chip,
+	.exec_op = rk_nfc_exec_op,
+	.setup_data_interface = rk_nfc_setup_data_interface,
+};
+
+static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
+				 struct device_node *np)
+{
+	struct rk_nfc_nand_chip *nand;
+	struct nand_chip *chip;
+	struct mtd_info *mtd;
+	int nsels;
+	u32 tmp;
+	int ret;
+	int i;
+
+	if (!of_get_property(np, "reg", &nsels))
+		return -ENODEV;
+	nsels /= sizeof(u32);
+	if (!nsels || nsels > NFC_MAX_NSELS) {
+		dev_err(dev, "invalid reg property size %d\n", nsels);
+		return -EINVAL;
+	}
+
+	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
+			    GFP_KERNEL);
+	if (!nand)
+		return -ENOMEM;
+
+	nand->nsels = nsels;
+	for (i = 0; i < nsels; i++) {
+		ret = of_property_read_u32_index(np, "reg", i, &tmp);
+		if (ret) {
+			dev_err(dev, "reg property failure : %d\n", ret);
+			return ret;
+		}
+
+		if (tmp >= NFC_MAX_NSELS) {
+			dev_err(dev, "invalid CS: %u\n", tmp);
+			return -EINVAL;
+		}
+
+		if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
+			dev_err(dev, "CS %u already assigned\n", tmp);
+			return -EINVAL;
+		}
+
+		nand->sels[i] = tmp;
+	}
+
+	chip = &nand->chip;
+	chip->controller = &nfc->controller;
+
+	nand_set_flash_node(chip, np);
+	nand_set_controller_data(chip, nfc);
+
+	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
+	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
+
+	/* set default mode in case dt entry is missing */
+	chip->ecc.mode = NAND_ECC_HW;
+
+	mtd = nand_to_mtd(chip);
+	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = dev;
+	mtd->name = THIS_NAME;
+	mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops);
+	rk_nfc_hw_init(nfc);
+	ret = nand_scan(chip, nsels);
+	if (ret)
+		return ret;
+
+	if (chip->options & NAND_IS_BOOT_MEDIUM) {
+		ret = of_property_read_u32(np, "rockchip-boot-blks", &tmp);
+		nand->boot_blks = ret ? 0 : tmp;
+
+		ret = of_property_read_u32(np, "rockchip-boot-ecc-strength",
+					   &tmp);
+		nand->boot_ecc = ret ? chip->ecc.strength : tmp;
+	}
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (ret) {
+		dev_err(dev, "mtd parse partition error\n");
+		nand_release(chip);
+		return ret;
+	}
+
+	list_add_tail(&nand->node, &nfc->chips);
+
+	return 0;
+}
+
+static int rk_nfc_nand_chips_init(struct device *dev, struct rk_nfc *nfc)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *nand_np;
+	int ret = -EINVAL;
+	int tmp;
+
+	for_each_child_of_node(np, nand_np) {
+		tmp = rk_nfc_nand_chip_init(dev, nfc, nand_np);
+		if (tmp) {
+			of_node_put(nand_np);
+			return ret;
+		}
+		/* At least one nand chip is initialized */
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static struct nfc_cfg nfc_v6_cfg = {
+		.type			= NFC_V6,
+		.ecc_strengths		= {60, 40, 24, 16},
+		.ecc_cfgs		= {
+			0x00040011, 0x00040001, 0x00000011, 0x00000001,
+		},
+		.flctl_off		= 0x08,
+		.bchctl_off		= 0x0C,
+		.dma_cfg_off		= 0x10,
+		.dma_data_buf_off	= 0x14,
+		.dma_oob_buf_off	= 0x18,
+		.dma_st_off		= 0x1C,
+		.bch_st_off		= 0x20,
+		.randmz_off		= 0x150,
+		.int_en_off		= 0x16C,
+		.int_clr_off		= 0x170,
+		.int_st_off		= 0x174,
+		.oob0_off		= 0x200,
+		.oob1_off		= 0x230,
+		.ecc0			= {
+			.err_flag_bit	= 2,
+			.low		= 3,
+			.low_mask	= 0x1F,
+			.low_bn		= 5,
+			.high		= 27,
+			.high_mask	= 0x1,
+		},
+		.ecc1			= {
+			.err_flag_bit	= 15,
+			.low		= 16,
+			.low_mask	= 0x1F,
+			.low_bn		= 5,
+			.high		= 29,
+			.high_mask	= 0x1,
+		},
+};
+
+static struct nfc_cfg nfc_v8_cfg = {
+		.type			= NFC_V8,
+		.ecc_strengths		= {16, 16, 16, 16},
+		.ecc_cfgs		= {
+			0x00000001, 0x00000001, 0x00000001, 0x00000001,
+		},
+		.flctl_off		= 0x08,
+		.bchctl_off		= 0x0C,
+		.dma_cfg_off		= 0x10,
+		.dma_data_buf_off	= 0x14,
+		.dma_oob_buf_off	= 0x18,
+		.dma_st_off		= 0x1C,
+		.bch_st_off		= 0x20,
+		.bch_st_off		= 0x20,
+		.randmz_off		= 0x150,
+		.int_en_off		= 0x16C,
+		.int_clr_off		= 0x170,
+		.int_st_off		= 0x174,
+		.oob0_off		= 0x200,
+		.oob1_off		= 0x230,
+		.ecc0			= {
+			.err_flag_bit	= 2,
+			.low		= 3,
+			.low_mask	= 0x1F,
+			.low_bn		= 5,
+			.high		= 27,
+			.high_mask	= 0x1,
+		},
+		.ecc1			= {
+			.err_flag_bit	= 15,
+			.low		= 16,
+			.low_mask	= 0x1F,
+			.low_bn		= 5,
+			.high		= 29,
+			.high_mask	= 0x1,
+		},
+};
+
+static struct nfc_cfg nfc_v9_cfg = {
+		.type			= NFC_V9,
+		.ecc_strengths		= {70, 60, 40, 16},
+		.ecc_cfgs		= {
+			0x00000001, 0x06000001, 0x04000001, 0x02000001,
+		},
+		.flctl_off		= 0x10,
+		.bchctl_off		= 0x20,
+		.dma_cfg_off		= 0x30,
+		.dma_data_buf_off	= 0x34,
+		.dma_oob_buf_off	= 0x38,
+		.dma_st_off		= 0x3C,
+		.bch_st_off		= 0x150,
+		.randmz_off		= 0x208,
+		.int_en_off		= 0x120,
+		.int_clr_off		= 0x124,
+		.int_st_off		= 0x128,
+		.oob0_off		= 0x200,
+		.oob1_off		= 0x204,
+		.ecc0			= {
+			.err_flag_bit	= 2,
+			.low		= 3,
+			.low_mask	= 0x7F,
+			.low_bn		= 7,
+			.high		= 0,
+			.high_mask	= 0x0,
+		},
+		.ecc1			= {
+			.err_flag_bit	= 18,
+			.low		= 19,
+			.low_mask	= 0x7F,
+			.low_bn		= 7,
+			.high		= 0,
+			.high_mask	= 0x0,
+		},
+};
+
+static const struct of_device_id rk_nfc_id_table[] = {
+	{.compatible = "rockchip,px30_nfc",
+		.data = &nfc_v9_cfg },
+	{.compatible = "rockchip,rk3xxx_nfc",
+		.data = &nfc_v6_cfg },
+	{.compatible = "rockchip,rk3308_nfc",
+		.data = &nfc_v8_cfg },
+	{.compatible = "rockchip,rv1108_nfc",
+		.data = &nfc_v8_cfg },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rk_nfc_id_table);
+
+static int rk_nfc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rk_nfc *nfc;
+	struct resource *res;
+	int ret, irq;
+
+	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	nand_controller_init(&nfc->controller);
+	INIT_LIST_HEAD(&nfc->chips);
+	nfc->controller.ops = &rk_nfc_controller_ops;
+
+	nfc->cfg = of_device_get_match_data(dev);
+	nfc->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(nfc->regs)) {
+		ret = PTR_ERR(nfc->regs);
+		goto release_nfc;
+	}
+
+	nfc->clk.nfc_clk = devm_clk_get(dev, "nfc");
+	if (IS_ERR(nfc->clk.nfc_clk)) {
+		dev_err(dev, "no nfc clk\n");
+		/* Some old device, sush as rk3036, has no nfc clk. */
+	}
+
+	nfc->clk.ahb_clk = devm_clk_get(dev, "ahb");
+	if (IS_ERR(nfc->clk.ahb_clk)) {
+		dev_err(dev, "no ahb clk\n");
+		ret = PTR_ERR(nfc->clk.ahb_clk);
+		goto release_nfc;
+	}
+
+	ret = rk_nfc_enable_clk(dev, &nfc->clk);
+	if (ret)
+		goto release_nfc;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no nfc irq resource\n");
+		ret = -EINVAL;
+		goto clk_disable;
+	}
+
+	writel(0, nfc->regs + nfc->cfg->int_en_off);
+	ret = devm_request_irq(dev, irq, rk_nfc_irq, 0x0, "rk-nand", nfc);
+	if (ret) {
+		dev_err(dev, "failed to request nfc irq\n");
+		goto clk_disable;
+	}
+
+	platform_set_drvdata(pdev, nfc);
+
+	ret = rk_nfc_nand_chips_init(dev, nfc);
+	if (ret) {
+		dev_err(dev, "failed to init nand chips\n");
+		goto clk_disable;
+	}
+	return 0;
+
+clk_disable:
+	rk_nfc_disable_clk(&nfc->clk);
+release_nfc:
+	return ret;
+}
+
+static int rk_nfc_remove(struct platform_device *pdev)
+{
+	struct rk_nfc *nfc = platform_get_drvdata(pdev);
+	struct rk_nfc_nand_chip *nand;
+
+	while (!list_empty(&nfc->chips)) {
+		nand = list_first_entry(&nfc->chips, struct rk_nfc_nand_chip,
+					node);
+		nand_release(&nand->chip);
+		list_del(&nand->node);
+	}
+
+	rk_nfc_disable_clk(&nfc->clk);
+
+	return 0;
+}
+
+static int __maybe_unused rk_nfc_suspend(struct device *dev)
+{
+	struct rk_nfc *nfc = dev_get_drvdata(dev);
+
+	rk_nfc_disable_clk(&nfc->clk);
+
+	return 0;
+}
+
+static int __maybe_unused rk_nfc_resume(struct device *dev)
+{
+	struct rk_nfc *nfc = dev_get_drvdata(dev);
+	struct rk_nfc_nand_chip *nand;
+	struct nand_chip *chip;
+	int ret;
+	u32 i;
+
+	ret = rk_nfc_enable_clk(dev, &nfc->clk);
+	if (ret)
+		return ret;
+
+	/* reset NAND chip if VCC was powered off */
+	list_for_each_entry(nand, &nfc->chips, node) {
+		chip = &nand->chip;
+		for (i = 0; i < nand->nsels; i++)
+			nand_reset(chip, i);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops rk_nfc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(rk_nfc_suspend, rk_nfc_resume)
+};
+
+static struct platform_driver rk_nfc_driver = {
+	.probe  = rk_nfc_probe,
+	.remove = rk_nfc_remove,
+	.driver = {
+		.name  = THIS_NAME,
+		.of_match_table = rk_nfc_id_table,
+		.pm = &rk_nfc_pm_ops,
+	},
+};
+
+module_platform_driver(rk_nfc_driver);
+
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_AUTHOR("Yifeng Zhao <yifeng.zhao@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver");
+MODULE_ALIAS("platform:rockchip_nand");
-- 
2.17.1




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others

[Johan Jonker]

Hi Yifeng,

Thank you for version 5.

- This patch doesn't compile.

make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- modules

make[4]: *** No rule to make target
'drivers/mtd/nand/raw/rockchip_nand.o', needed by '__build'.  Stop.

The Makefile is in need for a change.

- Add more mail lists.

./scripts/get_maintainer.pl --nogit-fallback [patch]

linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

- A few more comments below for now (part 1).
Code review is for the maintainers.
Key point for acceptance is bad block detection.
Could you give us a status update on that subject?

Kind regards,

Johan

On 4/26/20 12:02 PM, Yifeng Zhao wrote:
> This driver supports Rockchip NFC (NAND Flash Controller) found on RK3308,
> RK3368, RKPX30, RV1108 and other SOCs. The driver has been tested using
> 8-bit NAND interface on the ARM based RK3308 platform.
> 
> Support Rockchip NFC versions:
> - V6: ECC 16, 24, 40 and 60 bits per 1KB data. Found on RK3066, RK3368,
>       RK3229, RK3188, RK3288, RK3128, RKPX3SE, RKPx3, RKPX5, RK3036 and
>       RK3126.
> - V8: ECC 16 bits per 1KB data. Found on RV1108/7, RK3308.
> - V9: ECC 16, 40, 60 and 70 bits per 1KB data. Found on RK3326, RKPX30.
> 
> Support feature:
> - Read full page data by DMA.
> - Support HW ECC(one step is 1KB).
> - Support 2 - 32K page size.
> - Support 4 CS(depend on Soc)
> 
> Limitations:
> - Unsupport 512B ecc step.
> - Raw page read and write without ecc redundancy code. So could not support
>   raw data dump and restore.
> - Untested on some SOCs.
> - Unsupport subpage.
> - Unsupport randomizer.
> - The original bad block mask is not supported. It is recommended to use
>   the BBT(bad block table).
> 
> Signed-off-by: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
> Changes in v5:
> - Add boot blocks support  with different ecc for bootrom
> - Rename rockchip-nand.c to rockchip-nand-controller.c
> - Unification of other variable names
> - Remove some compatible define
> 
> Changes in v4:
> - Define platform data structure for the register offsets.
> - The compatible define with rkxx_nfc.
> - Use SET_SYSTEM_SLEEP_PM_OPS to define PM_OPS.
> - Use exec_op instead of legacy hooks.
> 
> Changes in v3: None
> Changes in v2:
> - Fix compile error.
> - Include header files sorted by file name
> 
>  drivers/mtd/nand/raw/Kconfig                  |    7 +
>  drivers/mtd/nand/raw/Makefile                 |    1 +
>  .../mtd/nand/raw/rockchip-nand-controller.c   | 1344 +++++++++++++++++
>  3 files changed, 1352 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/rockchip-nand-controller.c
> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index a80a46bb5b8b..8313b12a9d85 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -433,6 +433,13 @@ config MTD_NAND_MESON
>  	  Enables support for NAND controller on Amlogic's Meson SoCs.
>  	  This controller is found on Meson SoCs.
>  

> +config MTD_NAND_ROCKCHIP
> +	tristate "Rockchip NAND controller"
> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Enables support for NAND controller on Rockchip SoCs.
> +

New entries are added down the Kconfig list I think.

From Miquel:
> Can you write an exhaustive list if you know it? Or at least the
> families? It is quite challenging when you are not in the Rockchip
> world to know what SoC is similar to another random SoC.



>  config MTD_NAND_GPIO
>  	tristate "GPIO assisted NAND controller"
>  	depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 2d136b158fb7..8bafa59b8940 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
>  obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
>  obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
>  obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o

> +obj-$(CONFIG_MTD_NAND_ROCKCHIP)		+= rockchip_nand.o

-obj-$(CONFIG_MTD_NAND_ROCKCHIP)                += rockchip_nand.o
+obj-$(CONFIG_MTD_NAND_ROCKCHIP)                +=
rockchip-nand-controller.o


>  
>  nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
>  nand-objs += nand_onfi.o
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> new file mode 100644
> index 000000000000..cf473ed01999
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -0,0 +1,1344 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Rockchip NAND Flash controller driver.
> + * Copyright (C) 2020 Rockchip Inc.

> + * Authors: Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Author:

> + */
> +

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>

> +#include <linux/mtd/rawnand.h>
> +#include <linux/mtd/mtd.h>

Sort includes.

> +#include <linux/module.h>

> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>

> +
> +/*
> + * NFC Page Data Layout:
> + *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> + *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> + *	......
> + * NAND Page Data Layout:
> + *	1024 * n Data + m Bytes oob
> + * Original Bad Block Mask Location:
> + *	first byte of oob(spare)
> + * nand_chip->oob_poi data layout:
> + *	4Bytes sys data + .... + 4Bytes sys data + ecc data
> + */
> +
> +/* NAND controller register definition */
> +#define	NFC_READ		(0)
> +#define	NFC_WRITE		(1)

> +#define	NFC_FMCTL		(0x00)
> +#define		FMCTL_CE_SEL_M		0xFF
> +#define		FMCTL_CE_SEL(x)		(1 << (x))
> +#define		FMCTL_WP		BIT(8)
> +#define		FMCTL_RDY		BIT(9)

> +#define	NFC_FMWAIT		(0x04)

Move somewhere more logical.

> +#define		FLCTL_RST		BIT(0)
> +#define		FLCTL_WR		(1)	/* 0: read, 1: write */
> +#define		FLCTL_XFER_ST		BIT(2)
> +#define		FLCTL_XFER_EN		BIT(3)
> +#define		FLCTL_ACORRECT		BIT(10) /* auto correct error bits */
> +#define		FLCTL_XFER_READY	BIT(20)
> +#define		FLCTL_XFER_SECTOR	(22)
> +#define		FLCTL_TOG_FIX		BIT(29)

Sort defines on bit. Remove extra TAB.
Ask Miquel for exact prefix.

> +#define		BCHCTL_BANK_M	(7 << 5)
> +#define		BCHCTL_BANK	(5)
> +#define		DMA_ST			BIT(0)
> +#define		DMA_WR			(1)	/* 0: write, 1: read */
> +#define		DMA_EN			BIT(2)
> +#define		DMA_AHB_SIZE		(3)	/* 0: 1, 1: 2, 2: 4 */
> +#define		DMA_BURST_SIZE		(6)	/* 0: 1, 3: 4, 5: 8, 7: 16 */
> +#define		DMA_INC_NUM		(9)	/* 1 - 16 */
> +#define		ECC_ERR_CNT(x, e) ((((x) >> (e).low) & (e).low_mask) \
> +			| (((x) >> (e).high) & (e).high_mask) << (e).low_bn)
> +#define		INT_DMA			BIT(0)
> +#define	NFC_BANK		(0x800)
> +#define	NFC_BANK_STEP		(0x100)
> +#define		BANK_DATA		(0x00)
> +#define		BANK_ADDR		(0x04)
> +#define		BANK_CMD		(0x08)
> +#define	NFC_SRAM0		(0x1000)
> +#define	NFC_SRAM1		(0x1400)
> +#define	NFC_SRAM_SIZE		(0x400)
> +#define	THIS_NAME		"rk-nand"
> +#define	NFC_TIMEOUT		(500000)
> +#define	NFC_MAX_OOB_PER_STEP	128
> +#define	NFC_MIN_OOB_PER_STEP	64
> +#define	MAX_DATA_SIZE		0xFFFC
> +#define	MAX_ADDRESS_CYC		6
> +#define	NFC_ECC_MAX_MODES	4
> +#define	NFC_MAX_NSELS		(4) /* Some Soc only has 1 or 2 CSs */
> +#define	NFC_SYS_DATA_SIZE	(4) /* 4 bytes sys data in oob pre 1024 data */
> +#define	RK_DEFAULT_CLOCK_RATE	(150 * 1000 * 1000) /* 150 Mhz*/
> +#define	ACCTIMING(csrw, rwpw, rwcs) ((csrw) << 12 | (rwpw) << 5 | (rwcs))
> +
> +enum nfc_type {
> +	NFC_V6,
> +	NFC_V8,
> +	NFC_V9,
> +};
> +
> +/**
> + * struct rk_ecc_cnt_status: represent a ecc status data.
> + * @err_flag_bit: error flag bit index at register.
> + * @low: ecc count low bit index at register.
> + * @low_mask: mask bit.
> + * @low_bn: ecc count low bit number.
> + * @high: ecc count high bit index at register.
> + * @high_mask: mask bit
> + */
> +struct ecc_cnt_status {
> +	u8 err_flag_bit;
> +	u8 low;
> +	u8 low_mask;
> +	u8 low_bn;
> +	u8 high;
> +	u8 high_mask;
> +};
> +
> +/**
> + * @type: nfc version
> + * @ecc_strengths: ecc strengths
> + * @ecc_cfgs: ecc config values
> + * @flctl_off: FLCTL register offset
> + * @bchctl_off: BCHCTL register offset
> + * @dma_data_buf_off: DMA_DATA_BUF register offset
> + * @dma_oob_buf_off: DMA_OOB_BUF register offset
> + * @dma_cfg_off: DMA_CFG register offset
> + * @dma_st_off: DMA_ST register offset
> + * @bch_st_off: BCG_ST register offset
> + * @randmz_off: RANDMZ register offset
> + * @int_en_off: interrupt enable register offset
> + * @int_clr_off: interrupt clean register offset
> + * @int_st_off: interrupt status register offset
> + * @oob0_off: oob0 register offset
> + * @oob1_off: oob1 register offset
> + * @ecc0: represent ECC0 status data
> + * @ecc1: represent ECC1 status data
> + */
> +struct nfc_cfg {
> +	enum nfc_type type;
> +	u8 ecc_strengths[NFC_ECC_MAX_MODES];
> +	u32 ecc_cfgs[NFC_ECC_MAX_MODES];
> +	u32 flctl_off;
> +	u32 bchctl_off;
> +	u32 dma_cfg_off;
> +	u32 dma_data_buf_off;
> +	u32 dma_oob_buf_off;
> +	u32 dma_st_off;
> +	u32 bch_st_off;
> +	u32 randmz_off;
> +	u32 int_en_off;
> +	u32 int_clr_off;
> +	u32 int_st_off;
> +	u32 oob0_off;
> +	u32 oob1_off;
> +	struct ecc_cnt_status ecc0;
> +	struct ecc_cnt_status ecc1;
> +};
> +
> +struct rk_nfc_nand_chip {
> +	struct list_head node;
> +	struct nand_chip chip;
> +
> +	u32 spare_per_sector;
> +	u32 oob_buf_per_sector;
> +	u32 boot_blks;
> +	u32 boot_ecc;
> +
> +	int nsels;
> +	u8 sels[0];
> +	/* nothing after this field */
> +};
> +
> +struct rk_nfc_clk {
> +	int nfc_rate;
> +	struct clk *nfc_clk;
> +	struct clk *ahb_clk;
> +};
> +
> +struct rk_nfc {
> +	struct nand_controller controller;
> +	struct rk_nfc_clk clk;
> +
> +	struct device *dev;
> +	const struct nfc_cfg *cfg;
> +	void __iomem *regs;
> +
> +	int selected_bank;
> +	int band_offset;
> +
> +	struct completion done;
> +	struct list_head chips;
> +
> +	u8 *buffer;
> +	u8 *page_buf;
> +	u32 *oob_buf;
> +
> +	unsigned long assigned_cs;
> +};
> +
> +static inline struct rk_nfc_nand_chip *to_rk_nand(struct nand_chip *chip)
> +{
> +	return container_of(chip, struct rk_nfc_nand_chip, chip);
> +}
> +
> +static inline u8 *data_ptr(struct nand_chip *chip, const u8 *p, int i)
> +{
> +	return (u8 *)p + i * chip->ecc.size;
> +}
> +
> +static inline u8 *oob_ptr(struct nand_chip *chip, int i)
> +{
> +	u8 *poi;
> +
> +	poi = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
> +
> +	return poi;
> +}
> +
> +static inline int rk_data_len(struct nand_chip *chip)
> +{
> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> +
> +	return chip->ecc.size + rk_nand->spare_per_sector;
> +}
> +
> +static inline u8 *rk_data_ptr(struct nand_chip *chip,  int i)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> +	return nfc->buffer + i * rk_data_len(chip);
> +}
> +
> +static inline u8 *rk_oob_ptr(struct nand_chip *chip, int i)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> +	return nfc->buffer + i * rk_data_len(chip) + chip->ecc.size;
> +}
> +
> +static void rk_nfc_select_chip(struct nand_chip *chip, int cs)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> +	u32 val;
> +
> +	if (cs < 0) {
> +		nfc->selected_bank = -1;

> +		/* Deselect the currently selected target */

/* Deselect the currently selected target. */

A phrase starts with a capital and ends with a dot.
Check the rest as well.

> +		val = readl_relaxed(nfc->regs + NFC_FMCTL);
> +		val &= ~FMCTL_CE_SEL_M;
> +		writel(val, nfc->regs + NFC_FMCTL);
> +		return;
> +	}
> +
> +	nfc->selected_bank = rk_nand->sels[cs];
> +	nfc->band_offset = NFC_BANK + nfc->selected_bank * NFC_BANK_STEP;
> +
> +	val = readl_relaxed(nfc->regs + NFC_FMCTL);
> +	val &= ~FMCTL_CE_SEL_M;
> +	val |= FMCTL_CE_SEL(nfc->selected_bank);
> +
> +	writel(val, nfc->regs + NFC_FMCTL);
> +}
> +
> +static inline void rk_nfc_wait_ioready(struct rk_nfc *nfc)
> +{
> +	int rc;
> +	u32 val;
> +
> +	rc = readl_poll_timeout_atomic(nfc->regs + NFC_FMCTL, val,
> +				       val & FMCTL_RDY, 10, NFC_TIMEOUT);

> +	if (rc < 0)
> +		dev_err(nfc->dev, "data not ready\n");

Do we need debug here or move to call function?

> +}
> +
> +static inline u8 rk_nfc_read_byte(struct nand_chip *chip)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> +	return readb_relaxed(nfc->regs + nfc->band_offset + BANK_DATA);
> +}
> +
> +static void rk_nfc_read_buf(struct nand_chip *chip, u8 *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		buf[i] = rk_nfc_read_byte(chip);
> +}
> +
> +static void rk_nfc_write_byte(struct nand_chip *chip, u8 byte)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> +	writeb(byte, nfc->regs + nfc->band_offset + BANK_DATA);
> +}
> +
> +static void rk_nfc_write_buf(struct nand_chip *chip, const u8 *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		rk_nfc_write_byte(chip, buf[i]);
> +}
> +
> +static int rk_nfc_cmd(struct nand_chip *chip,
> +		      const struct nand_subop *subop)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 cnt = 0;
> +	unsigned int i, j, remaining, start;
> +	int reg_offset = nfc->band_offset;
> +	u8 *inbuf = NULL;
> +	const u8 *outbuf;
> +	void __iomem *data_reg;
> +
> +	for (i = 0; i < subop->ninstrs; i++) {
> +		const struct nand_op_instr *instr = &subop->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			writeb(instr->ctx.cmd.opcode,
> +			       nfc->regs + reg_offset + BANK_CMD);
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			remaining = nand_subop_get_num_addr_cyc(subop, i);
> +			start = nand_subop_get_addr_start_off(subop, i);
> +
> +			for (j = 0; j < 8 && j + start < remaining; j++)
> +				writeb(instr->ctx.addr.addrs[j + start],
> +				       nfc->regs + reg_offset + BANK_ADDR);
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +		case NAND_OP_DATA_OUT_INSTR:
> +			start = nand_subop_get_data_start_off(subop, i);
> +			cnt = nand_subop_get_data_len(subop, i);
> +			data_reg = nfc->regs + nfc->band_offset + BANK_DATA;
> +
> +			if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> +				outbuf = instr->ctx.data.buf.out + start;
> +				for (j = 0; j < cnt; j++)
> +					writeb(outbuf[j], data_reg);
> +			} else {
> +				inbuf = instr->ctx.data.buf.in + start;
> +				for (j = 0; j < cnt; j++)
> +					inbuf[j] = readb_relaxed(data_reg);
> +			}
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			rk_nfc_wait_ioready(nfc);
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct nand_op_parser rk_nfc_op_parser = NAND_OP_PARSER(
> +	NAND_OP_PARSER_PATTERN(
> +		rk_nfc_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, MAX_DATA_SIZE)),
> +	NAND_OP_PARSER_PATTERN(
> +		rk_nfc_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
> +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, MAX_DATA_SIZE),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> +);
> +
> +static int rk_nfc_exec_op(struct nand_chip *chip,
> +			  const struct nand_operation *op,
> +			  bool check_only)
> +{
> +	rk_nfc_select_chip(chip, op->cs);
> +	return nand_op_parser_exec_op(chip, &rk_nfc_op_parser, op,
> +				      check_only);
> +}
> +
> +static int rk_nfc_setup_data_interface(struct nand_chip *chip, int csline,
> +				       const struct nand_data_interface *conf)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	const struct nand_sdr_timings *timings;
> +	u32 rate, tc2rw, trwpw, trw2c;
> +	u32 temp;
> +
> +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> +		return 0;
> +
> +	/* not onfi nand flash */
> +	if (!chip->parameters.onfi)
> +		return 0;
> +
> +	timings = nand_get_sdr_timings(conf);
> +	if (IS_ERR(timings))
> +		return -ENOTSUPP;
> +
> +	if (IS_ERR(nfc->clk.nfc_clk))
> +		rate = clk_get_rate(nfc->clk.ahb_clk);
> +	else
> +		rate = clk_get_rate(nfc->clk.nfc_clk);
> +
> +	/* turn clock rate into KHZ */
> +	rate /= 1000;
> +
> +	tc2rw = 1;
> +	trw2c = 1;
> +
> +	trwpw = max(timings->tWC_min, timings->tRC_min) / 1000;
> +	trwpw = DIV_ROUND_UP(trwpw * rate, 1000000);
> +
> +	temp = timings->tREA_max / 1000;
> +	temp = DIV_ROUND_UP(temp * rate, 1000000);
> +
> +	if (trwpw < temp)
> +		trwpw = temp;
> +
> +	/*
> +	 * ACCON: access timing control register
> +	 * -------------------------------------
> +	 * 31:18: reserved
> +	 * 17:12: csrw, clock cycles from the falling edge of CSn to the
> +		 falling edge of RDn or WRn
> +	 * 11:11: reserved
> +	 * 10:05: rwpw, the width of RDn or WRn in processor clock cycles
> +	 * 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the
> +		 rising edge of CSn
> +	 */
> +	temp = ACCTIMING(tc2rw, trwpw, trw2c);
> +	writel(temp, nfc->regs + NFC_FMWAIT);
> +
> +	return 0;
> +}
> +
> +static int rk_nfc_hw_ecc_setup(struct nand_chip *chip,
> +			       struct nand_ecc_ctrl *ecc,
> +			       uint32_t strength)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 reg, i;
> +
> +	for (i = 0; i < NFC_ECC_MAX_MODES; i++) {
> +		if (ecc->strength == nfc->cfg->ecc_strengths[i]) {
> +			reg = nfc->cfg->ecc_cfgs[i];
> +			break;
> +		}
> +	}
> +
> +	if (i >= NFC_ECC_MAX_MODES)
> +		return -EINVAL;
> +
> +	writel(reg, nfc->regs + nfc->cfg->bchctl_off);
> +
> +	return 0;
> +}
> +
> +static void rk_nfc_xfer_start(struct rk_nfc *nfc, u8 rw, u8 n_KB,
> +			      dma_addr_t dma_data, dma_addr_t dma_oob)
> +{
> +	u32 dma_reg, fl_reg, bch_reg;
> +
> +	dma_reg = DMA_ST | ((!rw) << DMA_WR)  | DMA_EN | (2 << DMA_AHB_SIZE) |
> +	      (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM);
> +
> +	fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT |
> +		 (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX;
> +
> +	if (nfc->cfg->type == NFC_V6 || nfc->cfg->type == NFC_V8) {
> +		bch_reg = readl_relaxed(nfc->regs + nfc->cfg->bchctl_off);
> +		bch_reg = (bch_reg & (~BCHCTL_BANK_M)) |
> +			  (nfc->selected_bank << BCHCTL_BANK);
> +		writel(bch_reg, nfc->regs + nfc->cfg->bchctl_off);
> +	}
> +
> +	writel(dma_reg, nfc->regs + nfc->cfg->dma_cfg_off);
> +	writel((u32)dma_data, nfc->regs + nfc->cfg->dma_data_buf_off);
> +	writel((u32)dma_oob, nfc->regs + nfc->cfg->dma_oob_buf_off);
> +	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
> +	fl_reg |= FLCTL_XFER_ST;
> +	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
> +}
> +
> +static int rk_nfc_wait_for_xfer_done(struct rk_nfc *nfc)
> +{
> +	u32 reg;
> +	int ret = 0;
> +	void __iomem *ptr;
> +
> +	ptr = nfc->regs + nfc->cfg->flctl_off;
> +
> +	ret = readl_poll_timeout_atomic(ptr, reg,
> +					reg & FLCTL_XFER_READY,
> +					10, NFC_TIMEOUT);

> +	if (ret)
> +		dev_err(nfc->dev, "timeout reg=%x\n", reg);

Leave debug/action up to the call function.

> +
> +	return ret;
> +}
> +
> +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			     const u8 *buf, int page, int raw)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
> +			NFC_MIN_OOB_PER_STEP;
> +	int pages_per_blk = mtd->erasesize / mtd->writesize;
> +	int ret = 0, i, boot_rom_mode = 0;
> +	dma_addr_t dma_data, dma_oob;
> +	u32 reg;
> +	u8 *oob;
> +
> +	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> +
> +	if (!raw) {
> +		memcpy(nfc->page_buf, buf, mtd->writesize);
> +		memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
> +
> +		/*
> +		 * The first 8(some device is 4 or 16) blocks in use by
                                                             ^
                                                            are
> +		 * the bootloader. And the first 32 bits of oob need link
                                                                    ^
                                                                    to
> +		 * to next page address in the same block for Bootrom.
> +		 * Swap the first oob with the seventh oob, and bad block

                                                          ^
                                                        remove comma
> +		 * mask save at seventh oob.
                       ^       ^
                   is saved   the
> +		 */

Recheck the phrase above.

> +		if (page < pages_per_blk * rk_nand->boot_blks &&
> +		    chip->options & NAND_IS_BOOT_MEDIUM) {
> +			boot_rom_mode = 1;
> +			if (rk_nand->boot_ecc != ecc->strength)
> +				rk_nfc_hw_ecc_setup(chip, ecc,
> +						    rk_nand->boot_ecc);
> +		}

From Miquel:
> This would probably deserve a helper

> +
> +		swap(chip->oob_poi[0], chip->oob_poi[7]);
> +
> +		for (i = 0; i < ecc->steps; i++) {
> +			oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
> +			reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
> +			      oob[3] << 24;
> +			if (!i && boot_rom_mode)
> +				reg = (page & (pages_per_blk - 1)) * 4;
> +
> +			if (nfc->cfg->type == NFC_V6 ||
> +			    nfc->cfg->type == NFC_V8)
> +				nfc->oob_buf[i * oob_step / 4] = reg;
> +			else
> +				nfc->oob_buf[i] = reg;
> +		}
> +
> +		dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
> +					  mtd->writesize, DMA_TO_DEVICE);
> +		dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
> +					 ecc->steps * oob_step,
> +					 DMA_TO_DEVICE);
> +

> +		init_completion(&nfc->done);

From Miquel:
> One init_completion is needed (in the probe, probably) then please use
> reinit_completion().

> +		writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
> +
> +		rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
> +				  dma_oob);
> +		ret = wait_for_completion_timeout(&nfc->done,
> +						  msecs_to_jiffies(100));
> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		ret = rk_nfc_wait_for_xfer_done(nfc);
> +
> +		dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
> +				 DMA_TO_DEVICE);
> +		dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
> +				 DMA_TO_DEVICE);
> +
> +		if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
> +			rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +	} else {
> +		rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize);
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_prog_page_end_op(chip);
> +
> +	/* Deselect the currently selected target */
> +	rk_nfc_select_chip(chip, -1);
> +
> +	return ret;
> +}
> +
> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
> +				 int oob_on, int page)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 i;
> +
> +	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
> +	for (i = 0; i < chip->ecc.steps; i++) {
> +		if (buf)
> +			memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
> +			       chip->ecc.size);
> +
> +		memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i),
> +		       NFC_SYS_DATA_SIZE);
> +	}
> +
> +	return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
> +}
> +
> +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
> +{
> +	return rk_nfc_write_page_raw(chip, NULL, 1, page);
> +}
> +
> +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			    u32 data_offs, u32 readlen,
> +			    u8 *buf, int page, int raw)
> +{
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
> +			NFC_MIN_OOB_PER_STEP;
> +	int pages_per_blk = mtd->erasesize / mtd->writesize;
> +	dma_addr_t dma_data, dma_oob;
> +	int ret = 0, i, boot_rom_mode = 0;
> +	int bitflips = 0, bch_st;
> +	u8 *oob;
> +	u32 tmp;
> +
> +	nand_read_page_op(chip, page, 0, NULL, 0);
> +	if (!raw) {
> +		dma_data = dma_map_single(nfc->dev, nfc->page_buf,
> +					  mtd->writesize,
> +					  DMA_FROM_DEVICE);
> +		dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
> +					 ecc->steps * oob_step,
> +					 DMA_FROM_DEVICE);
> +

> +		if (page < pages_per_blk * rk_nand->boot_blks &&
> +		    chip->options & NAND_IS_BOOT_MEDIUM) {
> +			boot_rom_mode = 1;
> +			if (rk_nand->boot_ecc != ecc->strength)
> +				rk_nfc_hw_ecc_setup(chip, ecc,
> +						    rk_nand->boot_ecc);
> +		}

From Miquel:
> This would probably deserve a helper
> +

> +		init_completion(&nfc->done);

From Miquel:
> One init_completion is needed (in the probe, probably) then please use
> reinit_completion().
> +		writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
> +		rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
> +				  dma_oob);

> +		ret = wait_for_completion_timeout(&nfc->done,
> +						  msecs_to_jiffies(100));
> +		if (!ret)
> +			dev_warn(nfc->dev, "read ahb/dma done timeout\n");

No action?

> +		rk_nfc_wait_for_xfer_done(nfc);

No action?

> +		dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
> +				 DMA_FROM_DEVICE);
> +		dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
> +				 DMA_FROM_DEVICE);
> +
> +		for (i = 0; i < ecc->steps; i++) {
> +			oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
> +			if (nfc->cfg->type == NFC_V6 ||
> +			    nfc->cfg->type == NFC_V8)
> +				tmp = nfc->oob_buf[i * oob_step / 4];
> +			else
> +				tmp = nfc->oob_buf[i];
> +			*oob++ = (u8)tmp;
> +			*oob++ = (u8)(tmp >> 8);
> +			*oob++ = (u8)(tmp >> 16);
> +			*oob++ = (u8)(tmp >> 24);
> +		}
> +		swap(chip->oob_poi[0], chip->oob_poi[7]);
> +		for (i = 0; i < ecc->steps / 2; i++) {
> +			bch_st = readl_relaxed(nfc->regs +
> +					       nfc->cfg->bch_st_off + i * 4);
> +			if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
> +			    bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
> +				mtd->ecc_stats.failed++;
> +				bitflips = -1;
> +			} else {
> +				ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
> +				mtd->ecc_stats.corrected += ret;
> +				bitflips = max_t(u32, bitflips, ret);
> +
> +				ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
> +				mtd->ecc_stats.corrected += ret;
> +				bitflips = max_t(u32, bitflips, ret);
> +			}
> +		}
> +		memcpy(buf, nfc->page_buf, mtd->writesize);
> +
> +		if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
> +			rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +
> +		if (bitflips == -1)

> +			dev_err(nfc->dev, "read_page %x %x %x %x %x %x\n",
> +				page, bitflips, bch_st, ((u32 *)buf)[0],
> +				((u32 *)buf)[1], (u32)dma_data);

From Miquel:
> This is not very informative, please write a real error message. Avoid
> putting too much debug information, people will troubleshoot themselves
> if needed.



> +	} else {
> +		rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
> +	}
> +
> +	/* Deselect the currently selected target */
> +	rk_nfc_select_chip(chip, -1);
> +

> +	return bitflips;

Return value use by MTD bad block detection?

> +}
> +
> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
> +				   int oob_on, int page)
> +{
> +	return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
> +}
> +
> +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
> +				  int pg)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
> +}
> +
> +static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,
> +				int page)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	int i, ret;
> +
> +	ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
> +			       page, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < chip->ecc.steps; i++) {
> +		memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i),
> +		       NFC_SYS_DATA_SIZE);
> +
> +		if (buf)
> +			memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
> +			       chip->ecc.size);
> +	}
> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
> +
> +	return ret;
> +}
> +
> +static int rk_nfc_read_oob_std(struct nand_chip *chip, int page)
> +{
> +	return rk_nfc_read_page_raw(chip, NULL, 1, page);
> +}
> +
> +static inline void rk_nfc_hw_init(struct rk_nfc *nfc)
> +{
> +	/* disable flash wp */
> +	writel(FMCTL_WP, nfc->regs + NFC_FMCTL);
> +	/* config default timing */

> +	writel(0x1081, nfc->regs + NFC_FMWAIT);

Explain 0x1081 with define?
Fixed value for timing ns.

> +	/* disable randomizer and dma */
> +	writel(0, nfc->regs + nfc->cfg->randmz_off);
> +	writel(0, nfc->regs + nfc->cfg->dma_cfg_off);
> +	writel(FLCTL_RST, nfc->regs + nfc->cfg->flctl_off);
> +}
> +
> +static irqreturn_t rk_nfc_irq(int irq, void *id)
> +{
> +	struct rk_nfc *nfc = id;
> +	u32 sta, ien;
> +
> +	sta = readl_relaxed(nfc->regs + nfc->cfg->int_st_off);
> +	ien = readl_relaxed(nfc->regs + nfc->cfg->int_en_off);
> +
> +	if (!(sta & ien))
> +		return IRQ_NONE;
> +
> +	writel(sta, nfc->regs + nfc->cfg->int_clr_off);
> +	writel(~sta & ien, nfc->regs + nfc->cfg->int_en_off);
> +
> +	complete(&nfc->done);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rk_nfc_enable_clk(struct device *dev, struct rk_nfc_clk *clk)
> +{
> +	int ret;
> +
> +	if (!IS_ERR(clk->nfc_clk)) {
> +		ret = clk_prepare_enable(clk->nfc_clk);
> +		if (ret) {
> +			dev_err(dev, "failed to enable nfc clk\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(clk->ahb_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ahb clk\n");
> +		if (!IS_ERR(clk->nfc_clk))
> +			clk_disable_unprepare(clk->nfc_clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void rk_nfc_disable_clk(struct rk_nfc_clk *clk)
> +{
> +	if (!IS_ERR(clk->nfc_clk))
> +		clk_disable_unprepare(clk->nfc_clk);
> +	clk_disable_unprepare(clk->ahb_clk);
> +}
> +
> +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
> +				 struct mtd_oob_region *oob_region)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section >= chip->ecc.steps)
> +		return -ERANGE;
> +
> +	if (!section) {
> +		/* The first byte is bad block mask flag */
> +		oob_region->length = NFC_SYS_DATA_SIZE - 1;
> +		oob_region->offset = 1;
> +	} else {
> +		oob_region->length = NFC_SYS_DATA_SIZE;
> +		oob_region->offset = section * NFC_SYS_DATA_SIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
> +				struct mtd_oob_region *oob_region)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
> +	oob_region->length = mtd->oobsize - oob_region->offset;
> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
> +	.free = rk_nfc_ooblayout_free,
> +	.ecc = rk_nfc_ooblayout_ecc,
> +};
> +
> +static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	const u8 *strengths = nfc->cfg->ecc_strengths;
> +	u8 max_strength, nfc_max_strength;
> +	int i;
> +
> +	nfc_max_strength = nfc->cfg->ecc_strengths[0];
> +	/* if optional dt settings not present */
> +	if (!ecc->size || !ecc->strength ||
> +	    ecc->strength > nfc_max_strength) {
> +		/* use datasheet requirements */
> +		ecc->strength = chip->base.eccreq.strength;
> +		ecc->size = chip->base.eccreq.step_size;
> +
> +		/*
> +		 * align eccstrength and eccsize
> +		 * this controller only supports 512 and 1024 sizes
> +		 */
> +		if (chip->ecc.size < 1024) {
> +			if (mtd->writesize > 512) {
> +				chip->ecc.size = 1024;
> +				chip->ecc.strength <<= 1;
> +			} else {
> +				dev_err(dev, "ecc.size not supported\n");
> +				return -EINVAL;
> +			}
> +		} else {
> +			chip->ecc.size = 1024;
> +		}
> +
> +		ecc->steps = mtd->writesize / ecc->size;
> +		max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 / 14;
> +		if (max_strength > nfc_max_strength)
> +			max_strength = nfc_max_strength;
> +
> +		for (i = 0; i < 4; i++) {
> +			if (max_strength >= strengths[i])
> +				break;
> +		}
> +
> +		if (i >= 4) {
> +			dev_err(nfc->dev, "unsupported strength\n");
> +			return -ENOTSUPP;
> +		}
> +
> +		ecc->strength = strengths[i];
> +	}
> +	ecc->steps = mtd->writesize / ecc->size;

/* HW ECC always request ECC bytes for 1024 bytes blocks */

> +	ecc->bytes = DIV_ROUND_UP(ecc->strength * 14, 8);

14 is replacement for fls(8 * 1024)

Show intention.

	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);

> +	/* HW ECC always work with even numbers of ECC bytes */
> +	ecc->bytes = ALIGN(ecc->bytes, 2);
> +
> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +
> +	return 0;
> +}
> +
> +static int rk_nfc_attach_chip(struct nand_chip *chip)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct device *dev = mtd->dev.parent;
> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	int len;
> +	int ret;
> +
> +	if (chip->options & NAND_BUSWIDTH_16) {

> +		dev_err(dev, "16bits buswidth not supported");

16 bits bus width

> +		return -EINVAL;
> +	}
> +
> +	if (ecc->mode != NAND_ECC_HW)
> +		return 0;
> +
> +	ret = rk_nfc_ecc_init(dev, mtd);
> +	if (ret)
> +		return ret;
> +	rk_nand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE;
> +
> +	/* Check buffer first, avoid duplicate alloc buffer */
> +	if (nfc->buffer)
> +		return 0;
> +
> +	len = mtd->writesize + mtd->oobsize;
> +	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
> +	if (!nfc->buffer)
> +		return -ENOMEM;
> +
> +	nfc->page_buf = nfc->buffer;
> +	len = ecc->steps * NFC_MAX_OOB_PER_STEP;
> +	nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
> +	if (!nfc->oob_buf) {
> +		devm_kfree(dev, nfc->buffer);
> +		nfc->buffer = NULL;
> +		nfc->oob_buf = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	chip->ecc.write_page_raw = rk_nfc_write_page_raw;
> +	chip->ecc.write_page = rk_nfc_write_page_hwecc;
> +	chip->ecc.write_oob_raw = rk_nfc_write_oob_std;
> +	chip->ecc.write_oob = rk_nfc_write_oob_std;
> +
> +	chip->ecc.read_page_raw = rk_nfc_read_page_raw;
> +	chip->ecc.read_page = rk_nfc_read_page_hwecc;
> +	chip->ecc.read_oob_raw = rk_nfc_read_oob_std;
> +	chip->ecc.read_oob = rk_nfc_read_oob_std;
> +
> +	return 0;
> +}
> +
> +static const struct nand_controller_ops rk_nfc_controller_ops = {
> +	.attach_chip = rk_nfc_attach_chip,
> +	.exec_op = rk_nfc_exec_op,
> +	.setup_data_interface = rk_nfc_setup_data_interface,
> +};
> +
> +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> +				 struct device_node *np)
> +{
> +	struct rk_nfc_nand_chip *nand;
> +	struct nand_chip *chip;
> +	struct mtd_info *mtd;
> +	int nsels;
> +	u32 tmp;
> +	int ret;
> +	int i;
> +
> +	if (!of_get_property(np, "reg", &nsels))
> +		return -ENODEV;
> +	nsels /= sizeof(u32);
> +	if (!nsels || nsels > NFC_MAX_NSELS) {
> +		dev_err(dev, "invalid reg property size %d\n", nsels);
> +		return -EINVAL;
> +	}
> +
> +	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
> +			    GFP_KERNEL);
> +	if (!nand)
> +		return -ENOMEM;
> +
> +	nand->nsels = nsels;
> +	for (i = 0; i < nsels; i++) {
> +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> +		if (ret) {
> +			dev_err(dev, "reg property failure : %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (tmp >= NFC_MAX_NSELS) {
> +			dev_err(dev, "invalid CS: %u\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> +			dev_err(dev, "CS %u already assigned\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		nand->sels[i] = tmp;
> +	}
> +
> +	chip = &nand->chip;
> +	chip->controller = &nfc->controller;
> +
> +	nand_set_flash_node(chip, np);
> +	nand_set_controller_data(chip, nfc);
> +
> +	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> +	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +
> +	/* set default mode in case dt entry is missing */
> +	chip->ecc.mode = NAND_ECC_HW;
> +
> +	mtd = nand_to_mtd(chip);
> +	mtd->owner = THIS_MODULE;
> +	mtd->dev.parent = dev;
> +	mtd->name = THIS_NAME;
> +	mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops);
> +	rk_nfc_hw_init(nfc);
> +	ret = nand_scan(chip, nsels);
> +	if (ret)
> +		return ret;
> +
> +	if (chip->options & NAND_IS_BOOT_MEDIUM) {
> +		ret = of_property_read_u32(np, "rockchip-boot-blks", &tmp);
> +		nand->boot_blks = ret ? 0 : tmp;
> +
> +		ret = of_property_read_u32(np, "rockchip-boot-ecc-strength",
> +					   &tmp);
> +		nand->boot_ecc = ret ? chip->ecc.strength : tmp;
> +	}
> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret) {
> +		dev_err(dev, "mtd parse partition error\n");
> +		nand_release(chip);
> +		return ret;
> +	}
> +
> +	list_add_tail(&nand->node, &nfc->chips);
> +
> +	return 0;
> +}
> +
> +static int rk_nfc_nand_chips_init(struct device *dev, struct rk_nfc *nfc)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *nand_np;
> +	int ret = -EINVAL;
> +	int tmp;
> +
> +	for_each_child_of_node(np, nand_np) {
> +		tmp = rk_nfc_nand_chip_init(dev, nfc, nand_np);
> +		if (tmp) {
> +			of_node_put(nand_np);
> +			return ret;
> +		}
> +		/* At least one nand chip is initialized */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct nfc_cfg nfc_v6_cfg = {
> +		.type			= NFC_V6,
> +		.ecc_strengths		= {60, 40, 24, 16},
> +		.ecc_cfgs		= {
> +			0x00040011, 0x00040001, 0x00000011, 0x00000001,
> +		},
> +		.flctl_off		= 0x08,
> +		.bchctl_off		= 0x0C,
> +		.dma_cfg_off		= 0x10,
> +		.dma_data_buf_off	= 0x14,
> +		.dma_oob_buf_off	= 0x18,
> +		.dma_st_off		= 0x1C,
> +		.bch_st_off		= 0x20,
> +		.randmz_off		= 0x150,
> +		.int_en_off		= 0x16C,
> +		.int_clr_off		= 0x170,
> +		.int_st_off		= 0x174,
> +		.oob0_off		= 0x200,
> +		.oob1_off		= 0x230,
> +		.ecc0			= {
> +			.err_flag_bit	= 2,
> +			.low		= 3,
> +			.low_mask	= 0x1F,
> +			.low_bn		= 5,
> +			.high		= 27,
> +			.high_mask	= 0x1,
> +		},
> +		.ecc1			= {
> +			.err_flag_bit	= 15,
> +			.low		= 16,
> +			.low_mask	= 0x1F,
> +			.low_bn		= 5,
> +			.high		= 29,
> +			.high_mask	= 0x1,
> +		},
> +};
> +
> +static struct nfc_cfg nfc_v8_cfg = {
> +		.type			= NFC_V8,
> +		.ecc_strengths		= {16, 16, 16, 16},
> +		.ecc_cfgs		= {
> +			0x00000001, 0x00000001, 0x00000001, 0x00000001,
> +		},
> +		.flctl_off		= 0x08,
> +		.bchctl_off		= 0x0C,
> +		.dma_cfg_off		= 0x10,
> +		.dma_data_buf_off	= 0x14,
> +		.dma_oob_buf_off	= 0x18,
> +		.dma_st_off		= 0x1C,
> +		.bch_st_off		= 0x20,
> +		.bch_st_off		= 0x20,
> +		.randmz_off		= 0x150,
> +		.int_en_off		= 0x16C,
> +		.int_clr_off		= 0x170,
> +		.int_st_off		= 0x174,
> +		.oob0_off		= 0x200,
> +		.oob1_off		= 0x230,
> +		.ecc0			= {
> +			.err_flag_bit	= 2,
> +			.low		= 3,
> +			.low_mask	= 0x1F,
> +			.low_bn		= 5,
> +			.high		= 27,
> +			.high_mask	= 0x1,
> +		},
> +		.ecc1			= {
> +			.err_flag_bit	= 15,
> +			.low		= 16,
> +			.low_mask	= 0x1F,
> +			.low_bn		= 5,
> +			.high		= 29,
> +			.high_mask	= 0x1,
> +		},
> +};
> +
> +static struct nfc_cfg nfc_v9_cfg = {
> +		.type			= NFC_V9,
> +		.ecc_strengths		= {70, 60, 40, 16},
> +		.ecc_cfgs		= {
> +			0x00000001, 0x06000001, 0x04000001, 0x02000001,
> +		},
> +		.flctl_off		= 0x10,
> +		.bchctl_off		= 0x20,
> +		.dma_cfg_off		= 0x30,
> +		.dma_data_buf_off	= 0x34,
> +		.dma_oob_buf_off	= 0x38,
> +		.dma_st_off		= 0x3C,
> +		.bch_st_off		= 0x150,
> +		.randmz_off		= 0x208,
> +		.int_en_off		= 0x120,
> +		.int_clr_off		= 0x124,
> +		.int_st_off		= 0x128,
> +		.oob0_off		= 0x200,
> +		.oob1_off		= 0x204,
> +		.ecc0			= {
> +			.err_flag_bit	= 2,
> +			.low		= 3,
> +			.low_mask	= 0x7F,
> +			.low_bn		= 7,
> +			.high		= 0,
> +			.high_mask	= 0x0,
> +		},
> +		.ecc1			= {
> +			.err_flag_bit	= 18,
> +			.low		= 19,
> +			.low_mask	= 0x7F,
> +			.low_bn		= 7,
> +			.high		= 0,
> +			.high_mask	= 0x0,
> +		},
> +};
> +
> +static const struct of_device_id rk_nfc_id_table[] = {
> +	{.compatible = "rockchip,px30_nfc",
> +		.data = &nfc_v9_cfg },
> +	{.compatible = "rockchip,rk3xxx_nfc",
> +		.data = &nfc_v6_cfg },
> +	{.compatible = "rockchip,rk3308_nfc",
> +		.data = &nfc_v8_cfg },
> +	{.compatible = "rockchip,rv1108_nfc",
> +		.data = &nfc_v8_cfg },

> +	{}

	{ /* sentinel */ },

> +};
> +MODULE_DEVICE_TABLE(of, rk_nfc_id_table);
> +
> +static int rk_nfc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rk_nfc *nfc;
> +	struct resource *res;
> +	int ret, irq;
> +
> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	nand_controller_init(&nfc->controller);
> +	INIT_LIST_HEAD(&nfc->chips);
> +	nfc->controller.ops = &rk_nfc_controller_ops;
> +
> +	nfc->cfg = of_device_get_match_data(dev);
> +	nfc->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nfc->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(nfc->regs)) {
> +		ret = PTR_ERR(nfc->regs);
> +		goto release_nfc;
> +	}
> +
> +	nfc->clk.nfc_clk = devm_clk_get(dev, "nfc");
> +	if (IS_ERR(nfc->clk.nfc_clk)) {

> +		dev_err(dev, "no nfc clk\n");
> +		/* Some old device, sush as rk3036, has no nfc clk. */

/* Some older devices, such as rk3036, have no nfc clock. */

This is not an error, but normal for rk3066.
Maybe change to dev_dbg.

> +	}
> +
> +	nfc->clk.ahb_clk = devm_clk_get(dev, "ahb");
> +	if (IS_ERR(nfc->clk.ahb_clk)) {
> +		dev_err(dev, "no ahb clk\n");
> +		ret = PTR_ERR(nfc->clk.ahb_clk);
> +		goto release_nfc;
> +	}
> +
> +	ret = rk_nfc_enable_clk(dev, &nfc->clk);
> +	if (ret)
> +		goto release_nfc;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "no nfc irq resource\n");
> +		ret = -EINVAL;
> +		goto clk_disable;
> +	}
> +
> +	writel(0, nfc->regs + nfc->cfg->int_en_off);
> +	ret = devm_request_irq(dev, irq, rk_nfc_irq, 0x0, "rk-nand", nfc);
> +	if (ret) {
> +		dev_err(dev, "failed to request nfc irq\n");
> +		goto clk_disable;
> +	}
> +
> +	platform_set_drvdata(pdev, nfc);
> +
> +	ret = rk_nfc_nand_chips_init(dev, nfc);
> +	if (ret) {
> +		dev_err(dev, "failed to init nand chips\n");
> +		goto clk_disable;
> +	}
> +	return 0;
> +
> +clk_disable:
> +	rk_nfc_disable_clk(&nfc->clk);
> +release_nfc:
> +	return ret;
> +}
> +
> +static int rk_nfc_remove(struct platform_device *pdev)
> +{
> +	struct rk_nfc *nfc = platform_get_drvdata(pdev);
> +	struct rk_nfc_nand_chip *nand;
> +
> +	while (!list_empty(&nfc->chips)) {
> +		nand = list_first_entry(&nfc->chips, struct rk_nfc_nand_chip,
> +					node);
> +		nand_release(&nand->chip);
> +		list_del(&nand->node);
> +	}
> +
> +	rk_nfc_disable_clk(&nfc->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rk_nfc_suspend(struct device *dev)
> +{
> +	struct rk_nfc *nfc = dev_get_drvdata(dev);
> +
> +	rk_nfc_disable_clk(&nfc->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rk_nfc_resume(struct device *dev)
> +{
> +	struct rk_nfc *nfc = dev_get_drvdata(dev);
> +	struct rk_nfc_nand_chip *nand;
> +	struct nand_chip *chip;
> +	int ret;
> +	u32 i;
> +
> +	ret = rk_nfc_enable_clk(dev, &nfc->clk);
> +	if (ret)
> +		return ret;
> +
> +	/* reset NAND chip if VCC was powered off */
> +	list_for_each_entry(nand, &nfc->chips, node) {
> +		chip = &nand->chip;
> +		for (i = 0; i < nand->nsels; i++)
> +			nand_reset(chip, i);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops rk_nfc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(rk_nfc_suspend, rk_nfc_resume)
> +};
> +
> +static struct platform_driver rk_nfc_driver = {
> +	.probe  = rk_nfc_probe,
> +	.remove = rk_nfc_remove,
> +	.driver = {
> +		.name  = THIS_NAME,
> +		.of_match_table = rk_nfc_id_table,
> +		.pm = &rk_nfc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(rk_nfc_driver);
> +
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_AUTHOR("Yifeng Zhao <yifeng.zhao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>");
> +MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver");
> +MODULE_ALIAS("platform:rockchip_nand");
> 

Re: Re: [PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others【请注意，邮件由linux-rockchip-bounces+yifeng.zhao=rock-chips.com@lists.infradead.org代发】

[赵仪峰]

Hi Johan

>- A few more comments below for now (part 1).
>Code review is for the maintainers.
>Key point for acceptance is bad block detection.
>Could you give us a status update on that subject?

As I know there are five ways to judge the original bad block (different manufacturers are different):

1. The 1st Byte in the spare area of the 1st page does not contain FFh 

2. The 1st Byte in the data area of the 1st page does not contain FFh 

3. The 1st Byte in the spare area of the last page does not contain FFh 

4. The 1st Byte in the spare area of the last-1 page does not contain FFh 

5. A2h-80h-10h programming command error

Because of the different data layout, the original bad block mark will be overwritten when ECC is enabled, that is, the programmed page cannot read the original bad block mark.

I think the best way is to enable bad block table management. The original bad block scanning is completed in uboot and created the bad block table. 
This serie does so.

>From Miquel:
>> Can you write an exhaustive list if you know it? Or at least the
>> families? It is quite challenging when you are not in the Rockchip
>> world to know what SoC is similar to another random SoC.

V800 : RV1108 and RK3308,  ECC：16bits/1K , clock: ahb and nfc
V900 : PX30 and RK3326, ECC: 16,40,60,70 bits/1K, clock: ahb and nfc
V600：RK3066，RK3188， RK3229, ECC: 16,24,40,60 bits/1K, clock: ahb
V622 ：RK3126C，RK3128，RK3288, RK3036，RK3229, similar with V600,  ECC: 16,24,40,60 bits/1K, clock: ahb and nfc

>Hi Yifeng,
>
>Thank you for version 5.
>
>- This patch doesn't compile.
>
>make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- modules
>Move somewhere more logical.
>make[4]: *** No rule to make target
>'drivers/mtd/nand/raw/rockchip_nand.o', needed by '__build'.  Stop.
>
>The Makefile is in need for a change.
>
>- Add more mail lists.
>
>./scripts/get_maintainer.pl --nogit-fallback [patch]
>
>linux-arm-kernel@lists.infradead.org
>linux-kernel@vger.kernel.org
>
>- A few more comments below for now (part 1).
>Code review is for the maintainers.
>Key point for acceptance is bad block detection.
>Could you give us a status update on that subject?
>
>Kind regards,
>
>Johan
>
>On 4/26/20 12:02 PM, Yifeng Zhao wrote:
>> This driver supports Rockchip NFC (NAND Flash Controller) found on RK3308,
>> RK3368, RKPX30, RV1108 and other SOCs. The driver has been tested using
>> 8-bit NAND interface on the ARM based RK3308 platform.
>>
>> Support Rockchip NFC versions:
>> - V6: ECC 16, 24, 40 and 60 bits per 1KB data. Found on RK3066, RK3368,
>>       RK3229, RK3188, RK3288, RK3128, RKPX3SE, RKPx3, RKPX5, RK3036 and
>>       RK3126.
>> - V8: ECC 16 bits per 1KB data. Found on RV1108/7, RK3308.
>> - V9: ECC 16, 40, 60 and 70 bits per 1KB data. Found on RK3326, RKPX30.
>>
>> Support feature:
>> - Read full page data by DMA.
>> - Support HW ECC(one step is 1KB).
>> - Support 2 - 32K page size.
>> - Support 4 CS(depend on Soc)
>>
>> Limitations:
>> - Unsupport 512B ecc step.
>> - Raw page read and write without ecc redundancy code. So could not support
>>   raw data dump and restore.
>> - Untested on some SOCs.
>> - Unsupport subpage.
>> - Unsupport randomizer.
>> - The original bad block mask is not supported. It is recommended to use
>>   the BBT(bad block table).
>>
>> Signed-off-by: Yifeng Zhao <yifeng.zhao@rock-chips.com>
>> ---
>>
>> Changes in v5:
>> - Add boot blocks support  with different ecc for bootrom
>> - Rename rockchip-nand.c to rockchip-nand-controller.c
>> - Unification of other variable names
>> - Remove some compatible define
>>
>> Changes in v4:
>> - Define platform data structure for the register offsets.
>> - The compatible define with rkxx_nfc.
>> - Use SET_SYSTEM_SLEEP_PM_OPS to define PM_OPS.
>> - Use exec_op instead of legacy hooks.
>>
>> Changes in v3: None
>> Changes in v2:
>> - Fix compile error.
>> - Include header files sorted by file name
>>
>>  drivers/mtd/nand/raw/Kconfig                  |    7 +
>>  drivers/mtd/nand/raw/Makefile                 |    1 +
>>  .../mtd/nand/raw/rockchip-nand-controller.c   | 1344 +++++++++++++++++
>>  3 files changed, 1352 insertions(+)
>>  create mode 100644 drivers/mtd/nand/raw/rockchip-nand-controller.c
>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index a80a46bb5b8b..8313b12a9d85 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -433,6 +433,13 @@ config MTD_NAND_MESON
>>    Enables support for NAND controller on Amlogic's Meson SoCs.
>>    This controller is found on Meson SoCs.
>> 
>
>> +config MTD_NAND_ROCKCHIP
>> +	tristate "Rockchip NAND controller"
>> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
>> +	depends on HAS_IOMEM
>> +	help
>> +	  Enables support for NAND controller on Rockchip SoCs.
>> +
>
>New entries are added down the Kconfig list I think.
>
>From Miquel:
>> Can you write an exhaustive list if you know it? Or at least the
>> families? It is quite challenging when you are not in the Rockchip
>> world to know what SoC is similar to another random SoC.

>>  config MTD_NAND_GPIO
>>  tristate "GPIO assisted NAND controller"
>>  depends on GPIOLIB || COMPILE_TEST
>> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
>> index 2d136b158fb7..8bafa59b8940 100644
>> --- a/drivers/mtd/nand/raw/Makefile
>> +++ b/drivers/mtd/nand/raw/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)	+= tegra_nand.o
>>  obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
>>  obj-$(CONFIG_MTD_NAND_MESON)	+= meson_nand.o
>>  obj-$(CONFIG_MTD_NAND_CADENCE)	+= cadence-nand-controller.o
>
>> +obj-$(CONFIG_MTD_NAND_ROCKCHIP)	+= rockchip_nand.o
>
>-obj-$(CONFIG_MTD_NAND_ROCKCHIP)                += rockchip_nand.o
>+obj-$(CONFIG_MTD_NAND_ROCKCHIP)                +=
>rockchip-nand-controller.o
>
>
>> 
>>  nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
>>  nand-objs += nand_onfi.o
>> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> new file mode 100644
>> index 000000000000..cf473ed01999
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> @@ -0,0 +1,1344 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Rockchip NAND Flash controller driver.
>> + * Copyright (C) 2020 Rockchip Inc.
>
>> + * Authors: Yifeng Zhao <yifeng.zhao@rock-chips.com>
>
>Author:
>
>> + */
>> +
>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>
>> +#include <linux/mtd/rawnand.h>
>> +#include <linux/mtd/mtd.h>
>
>Sort includes.
>
>> +#include <linux/module.h>
>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>
>> +
>> +/*
>> + * NFC Page Data Layout:
>> + *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
>> + *	1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
>> + *	......
>> + * NAND Page Data Layout:
>> + *	1024 * n Data + m Bytes oob
>> + * Original Bad Block Mask Location:
>> + *	first byte of oob(spare)
>> + * nand_chip->oob_poi data layout:
>> + *	4Bytes sys data + .... + 4Bytes sys data + ecc data
>> + */
>> +
>> +/* NAND controller register definition */
>> +#define	NFC_READ	(0)
>> +#define	NFC_WRITE	(1)
>
>> +#define	NFC_FMCTL	(0x00)
>> +#define	FMCTL_CE_SEL_M	0xFF
>> +#define	FMCTL_CE_SEL(x)	(1 << (x))
>> +#define	FMCTL_WP	BIT(8)
>> +#define	FMCTL_RDY	BIT(9)
>
>> +#define	NFC_FMWAIT	(0x04)
>
>Move somewhere more logical.
>
>> +#define	FLCTL_RST	BIT(0)
>> +#define	FLCTL_WR	(1)	/* 0: read, 1: write */
>> +#define	FLCTL_XFER_ST	BIT(2)
>> +#define	FLCTL_XFER_EN	BIT(3)
>> +#define	FLCTL_ACORRECT	BIT(10) /* auto correct error bits */
>> +#define	FLCTL_XFER_READY	BIT(20)
>> +#define	FLCTL_XFER_SECTOR	(22)
>> +#define	FLCTL_TOG_FIX	BIT(29)
>
>Sort defines on bit. Remove extra TAB.
>Ask Miquel for exact prefix.

Referring to the definition of mtk_nand.c，I think this is better to read，so follow it.


>> +#define	BCHCTL_BANK_M	(7 << 5)
>> +#define	BCHCTL_BANK	(5)
>> +#define	DMA_ST	BIT(0)
>> +#define	DMA_WR	(1)	/* 0: write, 1: read */
>> +#define	DMA_EN	BIT(2)
>> +#define	DMA_AHB_SIZE	(3)	/* 0: 1, 1: 2, 2: 4 */
>> +#define	DMA_BURST_SIZE	(6)	/* 0: 1, 3: 4, 5: 8, 7: 16 */
>> +#define	DMA_INC_NUM	(9)	/* 1 - 16 */
>> +#define	ECC_ERR_CNT(x, e) ((((x) >> (e).low) & (e).low_mask) \
>> +	| (((x) >> (e).high) & (e).high_mask) << (e).low_bn)
>> +#define	INT_DMA	BIT(0)
>> +#define	NFC_BANK	(0x800)
>> +#define	NFC_BANK_STEP	(0x100)
>> +#define	BANK_DATA	(0x00)
>> +#define	BANK_ADDR	(0x04)
>> +#define	BANK_CMD	(0x08)
>> +#define	NFC_SRAM0	(0x1000)
>> +#define	NFC_SRAM1	(0x1400)
>> +#define	NFC_SRAM_SIZE	(0x400)
>> +#define	THIS_NAME	"rk-nand"
>> +#define	NFC_TIMEOUT	(500000)
>> +#define	NFC_MAX_OOB_PER_STEP	128
>> +#define	NFC_MIN_OOB_PER_STEP	64
>> +#define	MAX_DATA_SIZE	0xFFFC
>> +#define	MAX_ADDRESS_CYC	6
>> +#define	NFC_ECC_MAX_MODES	4
>> +#define	NFC_MAX_NSELS	(4) /* Some Soc only has 1 or 2 CSs */
>> +#define	NFC_SYS_DATA_SIZE	(4) /* 4 bytes sys data in oob pre 1024 data */
>> +#define	RK_DEFAULT_CLOCK_RATE	(150 * 1000 * 1000) /* 150 Mhz*/
>> +#define	ACCTIMING(csrw, rwpw, rwcs) ((csrw) << 12 | (rwpw) << 5 | (rwcs))
>> +
>> +enum nfc_type {
>> +	NFC_V6,
>> +	NFC_V8,
>> +	NFC_V9,
>> +};
>> +
>> +/**
>> + * struct rk_ecc_cnt_status: represent a ecc status data.
>> + * @err_flag_bit: error flag bit index at register.
>> + * @low: ecc count low bit index at register.
>> + * @low_mask: mask bit.
>> + * @low_bn: ecc count low bit number.
>> + * @high: ecc count high bit index at register.
>> + * @high_mask: mask bit
>> + */
>> +struct ecc_cnt_status {
>> +	u8 err_flag_bit;
>> +	u8 low;
>> +	u8 low_mask;
>> +	u8 low_bn;
>> +	u8 high;
>> +	u8 high_mask;
>> +};
>> +
>> +/**
>> + * @type: nfc version
>> + * @ecc_strengths: ecc strengths
>> + * @ecc_cfgs: ecc config values
>> + * @flctl_off: FLCTL register offset
>> + * @bchctl_off: BCHCTL register offset
>> + * @dma_data_buf_off: DMA_DATA_BUF register offset
>> + * @dma_oob_buf_off: DMA_OOB_BUF register offset
>> + * @dma_cfg_off: DMA_CFG register offset
>> + * @dma_st_off: DMA_ST register offset
>> + * @bch_st_off: BCG_ST register offset
>> + * @randmz_off: RANDMZ register offset
>> + * @int_en_off: interrupt enable register offset
>> + * @int_clr_off: interrupt clean register offset
>> + * @int_st_off: interrupt status register offset
>> + * @oob0_off: oob0 register offset
>> + * @oob1_off: oob1 register offset
>> + * @ecc0: represent ECC0 status data
>> + * @ecc1: represent ECC1 status data
>> + */
>> +struct nfc_cfg {
>> +	enum nfc_type type;
>> +	u8 ecc_strengths[NFC_ECC_MAX_MODES];
>> +	u32 ecc_cfgs[NFC_ECC_MAX_MODES];
>> +	u32 flctl_off;
>> +	u32 bchctl_off;
>> +	u32 dma_cfg_off;
>> +	u32 dma_data_buf_off;
>> +	u32 dma_oob_buf_off;
>> +	u32 dma_st_off;
>> +	u32 bch_st_off;
>> +	u32 randmz_off;
>> +	u32 int_en_off;
>> +	u32 int_clr_off;
>> +	u32 int_st_off;
>> +	u32 oob0_off;
>> +	u32 oob1_off;
>> +	struct ecc_cnt_status ecc0;
>> +	struct ecc_cnt_status ecc1;
>> +};
>> +
>> +struct rk_nfc_nand_chip {
>> +	struct list_head node;
>> +	struct nand_chip chip;
>> +
>> +	u32 spare_per_sector;
>> +	u32 oob_buf_per_sector;
>> +	u32 boot_blks;
>> +	u32 boot_ecc;
>> +
>> +	int nsels;
>> +	u8 sels[0];
>> +	/* nothing after this field */
>> +};
>> +
>> +struct rk_nfc_clk {
>> +	int nfc_rate;
>> +	struct clk *nfc_clk;
>> +	struct clk *ahb_clk;
>> +};
>> +
>> +struct rk_nfc {
>> +	struct nand_controller controller;
>> +	struct rk_nfc_clk clk;
>> +
>> +	struct device *dev;
>> +	const struct nfc_cfg *cfg;
>> +	void __iomem *regs;
>> +
>> +	int selected_bank;
>> +	int band_offset;
>> +
>> +	struct completion done;
>> +	struct list_head chips;
>> +
>> +	u8 *buffer;
>> +	u8 *page_buf;
>> +	u32 *oob_buf;
>> +
>> +	unsigned long assigned_cs;
>> +};
>> +
>> +static inline struct rk_nfc_nand_chip *to_rk_nand(struct nand_chip *chip)
>> +{
>> +	return container_of(chip, struct rk_nfc_nand_chip, chip);
>> +}
>> +
>> +static inline u8 *data_ptr(struct nand_chip *chip, const u8 *p, int i)
>> +{
>> +	return (u8 *)p + i * chip->ecc.size;
>> +}
>> +
>> +static inline u8 *oob_ptr(struct nand_chip *chip, int i)
>> +{
>> +	u8 *poi;
>> +
>> +	poi = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
>> +
>> +	return poi;
>> +}
>> +
>> +static inline int rk_data_len(struct nand_chip *chip)
>> +{
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
>> +
>> +	return chip->ecc.size + rk_nand->spare_per_sector;
>> +}
>> +
>> +static inline u8 *rk_data_ptr(struct nand_chip *chip,  int i)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +
>> +	return nfc->buffer + i * rk_data_len(chip);
>> +}
>> +
>> +static inline u8 *rk_oob_ptr(struct nand_chip *chip, int i)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +
>> +	return nfc->buffer + i * rk_data_len(chip) + chip->ecc.size;
>> +}
>> +
>> +static void rk_nfc_select_chip(struct nand_chip *chip, int cs)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
>> +	u32 val;
>> +
>> +	if (cs < 0) {
>> +	nfc->selected_bank = -1;
>
>> +	/* Deselect the currently selected target */
>
>/* Deselect the currently selected target. */
>
>A phrase starts with a capital and ends with a dot.
>Check the rest as well.
>
>> +	val = readl_relaxed(nfc->regs + NFC_FMCTL);
>> +	val &= ~FMCTL_CE_SEL_M;
>> +	writel(val, nfc->regs + NFC_FMCTL);
>> +	return;
>> +	}
>> +
>> +	nfc->selected_bank = rk_nand->sels[cs];
>> +	nfc->band_offset = NFC_BANK + nfc->selected_bank * NFC_BANK_STEP;
>> +
>> +	val = readl_relaxed(nfc->regs + NFC_FMCTL);
>> +	val &= ~FMCTL_CE_SEL_M;
>> +	val |= FMCTL_CE_SEL(nfc->selected_bank);
>> +
>> +	writel(val, nfc->regs + NFC_FMCTL);
>> +}
>> +
>> +static inline void rk_nfc_wait_ioready(struct rk_nfc *nfc)
>> +{
>> +	int rc;
>> +	u32 val;
>> +
>> +	rc = readl_poll_timeout_atomic(nfc->regs + NFC_FMCTL, val,
>> +	       val & FMCTL_RDY, 10, NFC_TIMEOUT);
>
>> +	if (rc < 0)
>> +	dev_err(nfc->dev, "data not ready\n");
>
>Do we need debug here or move to call function?

Usually there is no problem, but if there is a hardware problem with the RB pin, 
here will report an error, which can help to location troubleshoot the hardware problem.

>> +}
>> +
>> +static inline u8 rk_nfc_read_byte(struct nand_chip *chip)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +
>> +	return readb_relaxed(nfc->regs + nfc->band_offset + BANK_DATA);
>> +}
>> +
>> +static void rk_nfc_read_buf(struct nand_chip *chip, u8 *buf, int len)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +	buf[i] = rk_nfc_read_byte(chip);
>> +}
>> +
>> +static void rk_nfc_write_byte(struct nand_chip *chip, u8 byte)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +
>> +	writeb(byte, nfc->regs + nfc->band_offset + BANK_DATA);
>> +}
>> +
>> +static void rk_nfc_write_buf(struct nand_chip *chip, const u8 *buf, int len)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +	rk_nfc_write_byte(chip, buf[i]);
>> +}
>> +
>> +static int rk_nfc_cmd(struct nand_chip *chip,
>> +	      const struct nand_subop *subop)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	u32 cnt = 0;
>> +	unsigned int i, j, remaining, start;
>> +	int reg_offset = nfc->band_offset;
>> +	u8 *inbuf = NULL;
>> +	const u8 *outbuf;
>> +	void __iomem *data_reg;
>> +
>> +	for (i = 0; i < subop->ninstrs; i++) {
>> +	const struct nand_op_instr *instr = &subop->instrs[i];
>> +
>> +	switch (instr->type) {
>> +	case NAND_OP_CMD_INSTR:
>> +	writeb(instr->ctx.cmd.opcode,
>> +	       nfc->regs + reg_offset + BANK_CMD);
>> +	break;
>> +
>> +	case NAND_OP_ADDR_INSTR:
>> +	remaining = nand_subop_get_num_addr_cyc(subop, i);
>> +	start = nand_subop_get_addr_start_off(subop, i);
>> +
>> +	for (j = 0; j < 8 && j + start < remaining; j++)
>> +	writeb(instr->ctx.addr.addrs[j + start],
>> +	       nfc->regs + reg_offset + BANK_ADDR);
>> +	break;
>> +
>> +	case NAND_OP_DATA_IN_INSTR:
>> +	case NAND_OP_DATA_OUT_INSTR:
>> +	start = nand_subop_get_data_start_off(subop, i);
>> +	cnt = nand_subop_get_data_len(subop, i);
>> +	data_reg = nfc->regs + nfc->band_offset + BANK_DATA;
>> +
>> +	if (instr->type == NAND_OP_DATA_OUT_INSTR) {
>> +	outbuf = instr->ctx.data.buf.out + start;
>> +	for (j = 0; j < cnt; j++)
>> +	writeb(outbuf[j], data_reg);
>> +	} else {
>> +	inbuf = instr->ctx.data.buf.in + start;
>> +	for (j = 0; j < cnt; j++)
>> +	inbuf[j] = readb_relaxed(data_reg);
>> +	}
>> +	break;
>> +
>> +	case NAND_OP_WAITRDY_INSTR:
>> +	rk_nfc_wait_ioready(nfc);
>> +	break;
>> +	}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct nand_op_parser rk_nfc_op_parser = NAND_OP_PARSER(
>> +	NAND_OP_PARSER_PATTERN(
>> +	rk_nfc_cmd,
>> +	NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +	NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
>> +	NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +	NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
>> +	NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, MAX_DATA_SIZE)),
>> +	NAND_OP_PARSER_PATTERN(
>> +	rk_nfc_cmd,
>> +	NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +	NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
>> +	NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, MAX_DATA_SIZE),
>> +	NAND_OP_PARSER_PAT_CMD_ELEM(true),
>> +	NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
>> +);
>> +
>> +static int rk_nfc_exec_op(struct nand_chip *chip,
>> +	  const struct nand_operation *op,
>> +	  bool check_only)
>> +{
>> +	rk_nfc_select_chip(chip, op->cs);
>> +	return nand_op_parser_exec_op(chip, &rk_nfc_op_parser, op,
>> +	      check_only);
>> +}
>> +
>> +static int rk_nfc_setup_data_interface(struct nand_chip *chip, int csline,
>> +	       const struct nand_data_interface *conf)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	const struct nand_sdr_timings *timings;
>> +	u32 rate, tc2rw, trwpw, trw2c;
>> +	u32 temp;
>> +
>> +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
>> +	return 0;
>> +
>> +	/* not onfi nand flash */
>> +	if (!chip->parameters.onfi)
>> +	return 0;
>> +
>> +	timings = nand_get_sdr_timings(conf);
>> +	if (IS_ERR(timings))
>> +	return -ENOTSUPP;
>> +
>> +	if (IS_ERR(nfc->clk.nfc_clk))
>> +	rate = clk_get_rate(nfc->clk.ahb_clk);
>> +	else
>> +	rate = clk_get_rate(nfc->clk.nfc_clk);
>> +
>> +	/* turn clock rate into KHZ */
>> +	rate /= 1000;
>> +
>> +	tc2rw = 1;
>> +	trw2c = 1;
>> +
>> +	trwpw = max(timings->tWC_min, timings->tRC_min) / 1000;
>> +	trwpw = DIV_ROUND_UP(trwpw * rate, 1000000);
>> +
>> +	temp = timings->tREA_max / 1000;
>> +	temp = DIV_ROUND_UP(temp * rate, 1000000);
>> +
>> +	if (trwpw < temp)
>> +	trwpw = temp;
>> +
>> +	/*
>> +	* ACCON: access timing control register
>> +	* -------------------------------------
>> +	* 31:18: reserved
>> +	* 17:12: csrw, clock cycles from the falling edge of CSn to the
>> +	falling edge of RDn or WRn
>> +	* 11:11: reserved
>> +	* 10:05: rwpw, the width of RDn or WRn in processor clock cycles
>> +	* 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the
>> +	rising edge of CSn
>> +	*/
>> +	temp = ACCTIMING(tc2rw, trwpw, trw2c);
>> +	writel(temp, nfc->regs + NFC_FMWAIT);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_nfc_hw_ecc_setup(struct nand_chip *chip,
>> +	       struct nand_ecc_ctrl *ecc,
>> +	       uint32_t strength)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	u32 reg, i;
>> +
>> +	for (i = 0; i < NFC_ECC_MAX_MODES; i++) {
>> +	if (ecc->strength == nfc->cfg->ecc_strengths[i]) {
>> +	reg = nfc->cfg->ecc_cfgs[i];
>> +	break;
>> +	}
>> +	}
>> +
>> +	if (i >= NFC_ECC_MAX_MODES)
>> +	return -EINVAL;
>> +
>> +	writel(reg, nfc->regs + nfc->cfg->bchctl_off);
>> +
>> +	return 0;
>> +}
>> +
>> +static void rk_nfc_xfer_start(struct rk_nfc *nfc, u8 rw, u8 n_KB,
>> +	      dma_addr_t dma_data, dma_addr_t dma_oob)
>> +{
>> +	u32 dma_reg, fl_reg, bch_reg;
>> +
>> +	dma_reg = DMA_ST | ((!rw) << DMA_WR)  | DMA_EN | (2 << DMA_AHB_SIZE) |
>> +	      (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM);
>> +
>> +	fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT |
>> +	(n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX;
>> +
>> +	if (nfc->cfg->type == NFC_V6 || nfc->cfg->type == NFC_V8) {
>> +	bch_reg = readl_relaxed(nfc->regs + nfc->cfg->bchctl_off);
>> +	bch_reg = (bch_reg & (~BCHCTL_BANK_M)) |
>> +	  (nfc->selected_bank << BCHCTL_BANK);
>> +	writel(bch_reg, nfc->regs + nfc->cfg->bchctl_off);
>> +	}
>> +
>> +	writel(dma_reg, nfc->regs + nfc->cfg->dma_cfg_off);
>> +	writel((u32)dma_data, nfc->regs + nfc->cfg->dma_data_buf_off);
>> +	writel((u32)dma_oob, nfc->regs + nfc->cfg->dma_oob_buf_off);
>> +	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
>> +	fl_reg |= FLCTL_XFER_ST;
>> +	writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
>> +}
>> +
>> +static int rk_nfc_wait_for_xfer_done(struct rk_nfc *nfc)
>> +{
>> +	u32 reg;
>> +	int ret = 0;
>> +	void __iomem *ptr;
>> +
>> +	ptr = nfc->regs + nfc->cfg->flctl_off;
>> +
>> +	ret = readl_poll_timeout_atomic(ptr, reg,
>> +	reg & FLCTL_XFER_READY,
>> +	10, NFC_TIMEOUT);
>
>> +	if (ret)
>> +	dev_err(nfc->dev, "timeout reg=%x\n", reg);
>
>Leave debug/action up to the call function.

OKay, I will remove the print.

>> +
>> +	return ret;
>> +}
>> +
>> +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +	     const u8 *buf, int page, int raw)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
>> +	NFC_MIN_OOB_PER_STEP;
>> +	int pages_per_blk = mtd->erasesize / mtd->writesize;
>> +	int ret = 0, i, boot_rom_mode = 0;
>> +	dma_addr_t dma_data, dma_oob;
>> +	u32 reg;
>> +	u8 *oob;
>> +
>> +	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>> +
>> +	if (!raw) {
>> +	memcpy(nfc->page_buf, buf, mtd->writesize);
>> +	memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
>> +
>> +	/*
>> +	* The first 8(some device is 4 or 16) blocks in use by
>                                                             ^
>                                                            are
>> +	* the bootloader. And the first 32 bits of oob need link
>                                                                    ^
>                                                                    to
>> +	* to next page address in the same block for Bootrom.
>> +	* Swap the first oob with the seventh oob, and bad block
>
>                                                          ^
>                                                        remove comma
>> +	* mask save at seventh oob.
>                       ^       ^
>                   is saved   the
>> +	*/
>
>Recheck the phrase above.
>
>> +	if (page < pages_per_blk * rk_nand->boot_blks &&
>> +	    chip->options & NAND_IS_BOOT_MEDIUM) {
>> +	boot_rom_mode = 1;
>> +	if (rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc,
>> +	    rk_nand->boot_ecc);
>> +	}
>
>From Miquel:
>> This would probably deserve a helper

FIX it.

>> +
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +
>> +	for (i = 0; i < ecc->steps; i++) {
>> +	oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
>> +	reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
>> +	      oob[3] << 24;
>> +	if (!i && boot_rom_mode)
>> +	reg = (page & (pages_per_blk - 1)) * 4;
>> +
>> +	if (nfc->cfg->type == NFC_V6 ||
>> +	    nfc->cfg->type == NFC_V8)
>> +	nfc->oob_buf[i * oob_step / 4] = reg;
>> +	else
>> +	nfc->oob_buf[i] = reg;
>> +	}
>> +
>> +	dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
>> +	  mtd->writesize, DMA_TO_DEVICE);
>> +	dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
>> +	ecc->steps * oob_step,
>> +	DMA_TO_DEVICE);
>> +
>
>> +	init_completion(&nfc->done);
>
>From Miquel:
>> One init_completion is needed (in the probe, probably) then please use
>> reinit_completion().

FIX it.

>> +	writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
>> +
>> +	rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
>> +	  dma_oob);
>> +	ret = wait_for_completion_timeout(&nfc->done,
>> +	  msecs_to_jiffies(100));
>> +	if (!ret)
>> +	ret = -ETIMEDOUT;
>> +	ret = rk_nfc_wait_for_xfer_done(nfc);
>> +
>> +	dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
>> +	DMA_TO_DEVICE);
>> +	dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
>> +	DMA_TO_DEVICE);
>> +
>> +	if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>> +	} else {
>> +	rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize);
>> +	}
>> +
>> +	if (ret)
>> +	return ret;
>> +
>> +	ret = nand_prog_page_end_op(chip);
>> +
>> +	/* Deselect the currently selected target */
>> +	rk_nfc_select_chip(chip, -1);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
>> +	int oob_on, int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	u32 i;
>> +
>> +	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +	for (i = 0; i < chip->ecc.steps; i++) {
>> +	if (buf)
>> +	memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
>> +	       chip->ecc.size);
>> +
>> +	memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i),
>> +	       NFC_SYS_DATA_SIZE);
>> +	}
>> +
>> +	return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
>> +}
>> +
>> +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
>> +{
>> +	return rk_nfc_write_page_raw(chip, NULL, 1, page);
>> +}
>> +
>> +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +	    u32 data_offs, u32 readlen,
>> +	    u8 *buf, int page, int raw)
>> +{
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
>> +	NFC_MIN_OOB_PER_STEP;
>> +	int pages_per_blk = mtd->erasesize / mtd->writesize;
>> +	dma_addr_t dma_data, dma_oob;
>> +	int ret = 0, i, boot_rom_mode = 0;
>> +	int bitflips = 0, bch_st;
>> +	u8 *oob;
>> +	u32 tmp;
>> +
>> +	nand_read_page_op(chip, page, 0, NULL, 0);
>> +	if (!raw) {
>> +	dma_data = dma_map_single(nfc->dev, nfc->page_buf,
>> +	  mtd->writesize,
>> +	  DMA_FROM_DEVICE);
>> +	dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
>> +	ecc->steps * oob_step,
>> +	DMA_FROM_DEVICE);
>> +
>
>> +	if (page < pages_per_blk * rk_nand->boot_blks &&
>> +	    chip->options & NAND_IS_BOOT_MEDIUM) {
>> +	boot_rom_mode = 1;
>> +	if (rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc,
>> +	    rk_nand->boot_ecc);
>> +	}
>
>From Miquel:
>> This would probably deserve a helper
>> +
>
>> +	init_completion(&nfc->done);
>
>From Miquel:
>> One init_completion is needed (in the probe, probably) then please use
>> reinit_completion().
>> +	writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
>> +	rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
>> +	  dma_oob);
>
>> +	ret = wait_for_completion_timeout(&nfc->done,
>> +	  msecs_to_jiffies(100));
>> +	if (!ret)
>> +	dev_warn(nfc->dev, "read ahb/dma done timeout\n");
>
>No action?
>
>> +	rk_nfc_wait_for_xfer_done(nfc);
>
>No action?
>
>> +	dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
>> +	DMA_FROM_DEVICE);
>> +	dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
>> +	DMA_FROM_DEVICE);
>> +
>> +	for (i = 0; i < ecc->steps; i++) {
>> +	oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
>> +	if (nfc->cfg->type == NFC_V6 ||
>> +	    nfc->cfg->type == NFC_V8)
>> +	tmp = nfc->oob_buf[i * oob_step / 4];
>> +	else
>> +	tmp = nfc->oob_buf[i];
>> +	*oob++ = (u8)tmp;
>> +	*oob++ = (u8)(tmp >> 8);
>> +	*oob++ = (u8)(tmp >> 16);
>> +	*oob++ = (u8)(tmp >> 24);
>> +	}
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +	for (i = 0; i < ecc->steps / 2; i++) {
>> +	bch_st = readl_relaxed(nfc->regs +
>> +	       nfc->cfg->bch_st_off + i * 4);
>> +	if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
>> +	    bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
>> +	mtd->ecc_stats.failed++;
>> +	bitflips = -1;
>> +	} else {
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>> +	mtd->ecc_stats.corrected += ret;
>> +	bitflips = max_t(u32, bitflips, ret);
>> +
>> +	ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>> +	mtd->ecc_stats.corrected += ret;
>> +	bitflips = max_t(u32, bitflips, ret);
>> +	}
>> +	}
>> +	memcpy(buf, nfc->page_buf, mtd->writesize);
>> +
>> +	if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>> +
>> +	if (bitflips == -1)
>
>> +	dev_err(nfc->dev, "read_page %x %x %x %x %x %x\n",
>> +	page, bitflips, bch_st, ((u32 *)buf)[0],
>> +	((u32 *)buf)[1], (u32)dma_data);
>
>From Miquel:
>> This is not very informative, please write a real error message. Avoid
>> putting too much debug information, people will troubleshoot themselves
>> if needed.

ECC error, I will add some need infomation.

>
>> +	} else {
>> +	rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
>> +	}
>> +
>> +	/* Deselect the currently selected target */
>> +	rk_nfc_select_chip(chip, -1);
>> +
>
>> +	return bitflips;
>
>Return value use by MTD bad block detection?

ECC error not mean bad block, return value is ecc corrected bits.

>> +}
>> +
>> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>> +	   int oob_on, int page)
>> +{
>> +	return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
>> +}
>> +
>> +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
>> +	  int pg)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +
>> +	return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
>> +}
>> +
>> +static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,
>> +	int page)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	int i, ret;
>> +
>> +	ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
>> +	       page, 1);
>> +	if (ret < 0)
>> +	return ret;
>> +
>> +	for (i = 0; i < chip->ecc.steps; i++) {
>> +	memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i),
>> +	       NFC_SYS_DATA_SIZE);
>> +
>> +	if (buf)
>> +	memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
>> +	       chip->ecc.size);
>> +	}
>> +	swap(chip->oob_poi[0], chip->oob_poi[7]);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rk_nfc_read_oob_std(struct nand_chip *chip, int page)
>> +{
>> +	return rk_nfc_read_page_raw(chip, NULL, 1, page);
>> +}
>> +
>> +static inline void rk_nfc_hw_init(struct rk_nfc *nfc)
>> +{
>> +	/* disable flash wp */
>> +	writel(FMCTL_WP, nfc->regs + NFC_FMCTL);
>> +	/* config default timing */
>
>> +	writel(0x1081, nfc->regs + NFC_FMWAIT);
>
>Explain 0x1081 with define?
>Fixed value for timing ns.

This is default config, or I add a helper to calculate timing ns?
	/*
	 * ACCON: access timing control register
	 * -------------------------------------
	 * 31:18: reserved
	 * 17:12: csrw, clock cycles from the falling edge of CSn to the
		 falling edge of RDn or WRn
	 * 11:11: reserved
	 * 10:05: rwpw, the width of RDn or WRn in processor clock cycles
	 * 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the
		 rising edge of CSn
	 */

>> +	/* disable randomizer and dma */
>> +	writel(0, nfc->regs + nfc->cfg->randmz_off);
>> +	writel(0, nfc->regs + nfc->cfg->dma_cfg_off);
>> +	writel(FLCTL_RST, nfc->regs + nfc->cfg->flctl_off);
>> +}
>> +
>> +static irqreturn_t rk_nfc_irq(int irq, void *id)
>> +{
>> +	struct rk_nfc *nfc = id;
>> +	u32 sta, ien;
>> +
>> +	sta = readl_relaxed(nfc->regs + nfc->cfg->int_st_off);
>> +	ien = readl_relaxed(nfc->regs + nfc->cfg->int_en_off);
>> +
>> +	if (!(sta & ien))
>> +	return IRQ_NONE;
>> +
>> +	writel(sta, nfc->regs + nfc->cfg->int_clr_off);
>> +	writel(~sta & ien, nfc->regs + nfc->cfg->int_en_off);
>> +
>> +	complete(&nfc->done);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rk_nfc_enable_clk(struct device *dev, struct rk_nfc_clk *clk)
>> +{
>> +	int ret;
>> +
>> +	if (!IS_ERR(clk->nfc_clk)) {
>> +	ret = clk_prepare_enable(clk->nfc_clk);
>> +	if (ret) {
>> +	dev_err(dev, "failed to enable nfc clk\n");
>> +	return ret;
>> +	}
>> +	}
>> +
>> +	ret = clk_prepare_enable(clk->ahb_clk);
>> +	if (ret) {
>> +	dev_err(dev, "failed to enable ahb clk\n");
>> +	if (!IS_ERR(clk->nfc_clk))
>> +	clk_disable_unprepare(clk->nfc_clk);
>> +	return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rk_nfc_disable_clk(struct rk_nfc_clk *clk)
>> +{
>> +	if (!IS_ERR(clk->nfc_clk))
>> +	clk_disable_unprepare(clk->nfc_clk);
>> +	clk_disable_unprepare(clk->ahb_clk);
>> +}
>> +
>> +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>> +	struct mtd_oob_region *oob_region)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>> +	if (section >= chip->ecc.steps)
>> +	return -ERANGE;
>> +
>> +	if (!section) {
>> +	/* The first byte is bad block mask flag */
>> +	oob_region->length = NFC_SYS_DATA_SIZE - 1;
>> +	oob_region->offset = 1;
>> +	} else {
>> +	oob_region->length = NFC_SYS_DATA_SIZE;
>> +	oob_region->offset = section * NFC_SYS_DATA_SIZE;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +	struct mtd_oob_region *oob_region)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>> +	if (section)
>> +	return -ERANGE;
>> +
>> +	oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
>> +	oob_region->length = mtd->oobsize - oob_region->offset;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
>> +	.free = rk_nfc_ooblayout_free,
>> +	.ecc = rk_nfc_ooblayout_ecc,
>> +};
>> +
>> +static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	const u8 *strengths = nfc->cfg->ecc_strengths;
>> +	u8 max_strength, nfc_max_strength;
>> +	int i;
>> +
>> +	nfc_max_strength = nfc->cfg->ecc_strengths[0];
>> +	/* if optional dt settings not present */
>> +	if (!ecc->size || !ecc->strength ||
>> +	    ecc->strength > nfc_max_strength) {
>> +	/* use datasheet requirements */
>> +	ecc->strength = chip->base.eccreq.strength;
>> +	ecc->size = chip->base.eccreq.step_size;
>> +
>> +	/*
>> +	* align eccstrength and eccsize
>> +	* this controller only supports 512 and 1024 sizes
>> +	*/
>> +	if (chip->ecc.size < 1024) {
>> +	if (mtd->writesize > 512) {
>> +	chip->ecc.size = 1024;
>> +	chip->ecc.strength <<= 1;
>> +	} else {
>> +	dev_err(dev, "ecc.size not supported\n");
>> +	return -EINVAL;
>> +	}
>> +	} else {
>> +	chip->ecc.size = 1024;
>> +	}
>> +
>> +	ecc->steps = mtd->writesize / ecc->size;
>> +	max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 / 14;
>> +	if (max_strength > nfc_max_strength)
>> +	max_strength = nfc_max_strength;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +	if (max_strength >= strengths[i])
>> +	break;
>> +	}
>> +
>> +	if (i >= 4) {
>> +	dev_err(nfc->dev, "unsupported strength\n");
>> +	return -ENOTSUPP;
>> +	}
>> +
>> +	ecc->strength = strengths[i];
>> +	}
>> +	ecc->steps = mtd->writesize / ecc->size;
>
>/* HW ECC always request ECC bytes for 1024 bytes blocks */
>
>> +	ecc->bytes = DIV_ROUND_UP(ecc->strength * 14, 8);
>
>14 is replacement for fls(8 * 1024)
>
>Show intention.

FIX it.

>	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
>
>> +	/* HW ECC always work with even numbers of ECC bytes */
>> +	ecc->bytes = ALIGN(ecc->bytes, 2);
>> +
>> +	rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_nfc_attach_chip(struct nand_chip *chip)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct device *dev = mtd->dev.parent;
>> +	struct rk_nfc *nfc = nand_get_controller_data(chip);
>> +	struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	int len;
>> +	int ret;
>> +
>> +	if (chip->options & NAND_BUSWIDTH_16) {
>
>> +	dev_err(dev, "16bits buswidth not supported");
>
>16 bits bus width

FIX it.

>> +	return -EINVAL;
>> +	}
>> +
>> +	if (ecc->mode != NAND_ECC_HW)
>> +	return 0;
>> +
>> +	ret = rk_nfc_ecc_init(dev, mtd);
>> +	if (ret)
>> +	return ret;
>> +	rk_nand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE;
>> +
>> +	/* Check buffer first, avoid duplicate alloc buffer */
>> +	if (nfc->buffer)
>> +	return 0;
>> +
>> +	len = mtd->writesize + mtd->oobsize;
>> +	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
>> +	if (!nfc->buffer)
>> +	return -ENOMEM;
>> +
>> +	nfc->page_buf = nfc->buffer;
>> +	len = ecc->steps * NFC_MAX_OOB_PER_STEP;
>> +	nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
>> +	if (!nfc->oob_buf) {
>> +	devm_kfree(dev, nfc->buffer);
>> +	nfc->buffer = NULL;
>> +	nfc->oob_buf = NULL;
>> +	return -ENOMEM;
>> +	}
>> +
>> +	chip->ecc.write_page_raw = rk_nfc_write_page_raw;
>> +	chip->ecc.write_page = rk_nfc_write_page_hwecc;
>> +	chip->ecc.write_oob_raw = rk_nfc_write_oob_std;
>> +	chip->ecc.write_oob = rk_nfc_write_oob_std;
>> +
>> +	chip->ecc.read_page_raw = rk_nfc_read_page_raw;
>> +	chip->ecc.read_page = rk_nfc_read_page_hwecc;
>> +	chip->ecc.read_oob_raw = rk_nfc_read_oob_std;
>> +	chip->ecc.read_oob = rk_nfc_read_oob_std;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct nand_controller_ops rk_nfc_controller_ops = {
>> +	.attach_chip = rk_nfc_attach_chip,
>> +	.exec_op = rk_nfc_exec_op,
>> +	.setup_data_interface = rk_nfc_setup_data_interface,
>> +};
>> +
>> +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
>> +	struct device_node *np)
>> +{
>> +	struct rk_nfc_nand_chip *nand;
>> +	struct nand_chip *chip;
>> +	struct mtd_info *mtd;
>> +	int nsels;
>> +	u32 tmp;
>> +	int ret;
>> +	int i;
>> +
>> +	if (!of_get_property(np, "reg", &nsels))
>> +	return -ENODEV;
>> +	nsels /= sizeof(u32);
>> +	if (!nsels || nsels > NFC_MAX_NSELS) {
>> +	dev_err(dev, "invalid reg property size %d\n", nsels);
>> +	return -EINVAL;
>> +	}
>> +
>> +	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
>> +	    GFP_KERNEL);
>> +	if (!nand)
>> +	return -ENOMEM;
>> +
>> +	nand->nsels = nsels;
>> +	for (i = 0; i < nsels; i++) {
>> +	ret = of_property_read_u32_index(np, "reg", i, &tmp);
>> +	if (ret) {
>> +	dev_err(dev, "reg property failure : %d\n", ret);
>> +	return ret;
>> +	}
>> +
>> +	if (tmp >= NFC_MAX_NSELS) {
>> +	dev_err(dev, "invalid CS: %u\n", tmp);
>> +	return -EINVAL;
>> +	}
>> +
>> +	if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
>> +	dev_err(dev, "CS %u already assigned\n", tmp);
>> +	return -EINVAL;
>> +	}
>> +
>> +	nand->sels[i] = tmp;
>> +	}
>> +
>> +	chip = &nand->chip;
>> +	chip->controller = &nfc->controller;
>> +
>> +	nand_set_flash_node(chip, np);
>> +	nand_set_controller_data(chip, nfc);
>> +
>> +	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
>> +	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>> +
>> +	/* set default mode in case dt entry is missing */
>> +	chip->ecc.mode = NAND_ECC_HW;
>> +
>> +	mtd = nand_to_mtd(chip);
>> +	mtd->owner = THIS_MODULE;
>> +	mtd->dev.parent = dev;
>> +	mtd->name = THIS_NAME;
>> +	mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops);
>> +	rk_nfc_hw_init(nfc);
>> +	ret = nand_scan(chip, nsels);
>> +	if (ret)
>> +	return ret;
>> +
>> +	if (chip->options & NAND_IS_BOOT_MEDIUM) {
>> +	ret = of_property_read_u32(np, "rockchip-boot-blks", &tmp);
>> +	nand->boot_blks = ret ? 0 : tmp;
>> +
>> +	ret = of_property_read_u32(np, "rockchip-boot-ecc-strength",
>> +	   &tmp);
>> +	nand->boot_ecc = ret ? chip->ecc.strength : tmp;
>> +	}
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (ret) {
>> +	dev_err(dev, "mtd parse partition error\n");
>> +	nand_release(chip);
>> +	return ret;
>> +	}
>> +
>> +	list_add_tail(&nand->node, &nfc->chips);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_nfc_nand_chips_init(struct device *dev, struct rk_nfc *nfc)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *nand_np;
>> +	int ret = -EINVAL;
>> +	int tmp;
>> +
>> +	for_each_child_of_node(np, nand_np) {
>> +	tmp = rk_nfc_nand_chip_init(dev, nfc, nand_np);
>> +	if (tmp) {
>> +	of_node_put(nand_np);
>> +	return ret;
>> +	}
>> +	/* At least one nand chip is initialized */
>> +	ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static struct nfc_cfg nfc_v6_cfg = {
>> +	.type	= NFC_V6,
>> +	.ecc_strengths	= {60, 40, 24, 16},
>> +	.ecc_cfgs	= {
>> +	0x00040011, 0x00040001, 0x00000011, 0x00000001,
>> +	},
>> +	.flctl_off	= 0x08,
>> +	.bchctl_off	= 0x0C,
>> +	.dma_cfg_off	= 0x10,
>> +	.dma_data_buf_off	= 0x14,
>> +	.dma_oob_buf_off	= 0x18,
>> +	.dma_st_off	= 0x1C,
>> +	.bch_st_off	= 0x20,
>> +	.randmz_off	= 0x150,
>> +	.int_en_off	= 0x16C,
>> +	.int_clr_off	= 0x170,
>> +	.int_st_off	= 0x174,
>> +	.oob0_off	= 0x200,
>> +	.oob1_off	= 0x230,
>> +	.ecc0	= {
>> +	.err_flag_bit	= 2,
>> +	.low	= 3,
>> +	.low_mask	= 0x1F,
>> +	.low_bn	= 5,
>> +	.high	= 27,
>> +	.high_mask	= 0x1,
>> +	},
>> +	.ecc1	= {
>> +	.err_flag_bit	= 15,
>> +	.low	= 16,
>> +	.low_mask	= 0x1F,
>> +	.low_bn	= 5,
>> +	.high	= 29,
>> +	.high_mask	= 0x1,
>> +	},
>> +};
>> +
>> +static struct nfc_cfg nfc_v8_cfg = {
>> +	.type	= NFC_V8,
>> +	.ecc_strengths	= {16, 16, 16, 16},
>> +	.ecc_cfgs	= {
>> +	0x00000001, 0x00000001, 0x00000001, 0x00000001,
>> +	},
>> +	.flctl_off	= 0x08,
>> +	.bchctl_off	= 0x0C,
>> +	.dma_cfg_off	= 0x10,
>> +	.dma_data_buf_off	= 0x14,
>> +	.dma_oob_buf_off	= 0x18,
>> +	.dma_st_off	= 0x1C,
>> +	.bch_st_off	= 0x20,
>> +	.bch_st_off	= 0x20,
>> +	.randmz_off	= 0x150,
>> +	.int_en_off	= 0x16C,
>> +	.int_clr_off	= 0x170,
>> +	.int_st_off	= 0x174,
>> +	.oob0_off	= 0x200,
>> +	.oob1_off	= 0x230,
>> +	.ecc0	= {
>> +	.err_flag_bit	= 2,
>> +	.low	= 3,
>> +	.low_mask	= 0x1F,
>> +	.low_bn	= 5,
>> +	.high	= 27,
>> +	.high_mask	= 0x1,
>> +	},
>> +	.ecc1	= {
>> +	.err_flag_bit	= 15,
>> +	.low	= 16,
>> +	.low_mask	= 0x1F,
>> +	.low_bn	= 5,
>> +	.high	= 29,
>> +	.high_mask	= 0x1,
>> +	},
>> +};
>> +
>> +static struct nfc_cfg nfc_v9_cfg = {
>> +	.type	= NFC_V9,
>> +	.ecc_strengths	= {70, 60, 40, 16},
>> +	.ecc_cfgs	= {
>> +	0x00000001, 0x06000001, 0x04000001, 0x02000001,
>> +	},
>> +	.flctl_off	= 0x10,
>> +	.bchctl_off	= 0x20,
>> +	.dma_cfg_off	= 0x30,
>> +	.dma_data_buf_off	= 0x34,
>> +	.dma_oob_buf_off	= 0x38,
>> +	.dma_st_off	= 0x3C,
>> +	.bch_st_off	= 0x150,
>> +	.randmz_off	= 0x208,
>> +	.int_en_off	= 0x120,
>> +	.int_clr_off	= 0x124,
>> +	.int_st_off	= 0x128,
>> +	.oob0_off	= 0x200,
>> +	.oob1_off	= 0x204,
>> +	.ecc0	= {
>> +	.err_flag_bit	= 2,
>> +	.low	= 3,
>> +	.low_mask	= 0x7F,
>> +	.low_bn	= 7,
>> +	.high	= 0,
>> +	.high_mask	= 0x0,
>> +	},
>> +	.ecc1	= {
>> +	.err_flag_bit	= 18,
>> +	.low	= 19,
>> +	.low_mask	= 0x7F,
>> +	.low_bn	= 7,
>> +	.high	= 0,
>> +	.high_mask	= 0x0,
>> +	},
>> +};
>> +
>> +static const struct of_device_id rk_nfc_id_table[] = {
>> +	{.compatible = "rockchip,px30_nfc",
>> +	.data = &nfc_v9_cfg },
>> +	{.compatible = "rockchip,rk3xxx_nfc",
>> +	.data = &nfc_v6_cfg },
>> +	{.compatible = "rockchip,rk3308_nfc",
>> +	.data = &nfc_v8_cfg },
>> +	{.compatible = "rockchip,rv1108_nfc",
>> +	.data = &nfc_v8_cfg },
>
>> +	{}
>
>	{ /* sentinel */ },
>
>> +};
>> +MODULE_DEVICE_TABLE(of, rk_nfc_id_table);
>> +
>> +static int rk_nfc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rk_nfc *nfc;
>> +	struct resource *res;
>> +	int ret, irq;
>> +
>> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>> +	if (!nfc)
>> +	return -ENOMEM;
>> +
>> +	nand_controller_init(&nfc->controller);
>> +	INIT_LIST_HEAD(&nfc->chips);
>> +	nfc->controller.ops = &rk_nfc_controller_ops;
>> +
>> +	nfc->cfg = of_device_get_match_data(dev);
>> +	nfc->dev = dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	nfc->regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(nfc->regs)) {
>> +	ret = PTR_ERR(nfc->regs);
>> +	goto release_nfc;
>> +	}
>> +
>> +	nfc->clk.nfc_clk = devm_clk_get(dev, "nfc");
>> +	if (IS_ERR(nfc->clk.nfc_clk)) {
>
>> +	dev_err(dev, "no nfc clk\n");
>> +	/* Some old device, sush as rk3036, has no nfc clk. */
>
>/* Some older devices, such as rk3036, have no nfc clock. */
>
>This is not an error, but normal for rk3066.
>Maybe change to dev_dbg.

FIX it.

>> +	}
>> +
>> +	nfc->clk.ahb_clk = devm_clk_get(dev, "ahb");
>> +	if (IS_ERR(nfc->clk.ahb_clk)) {
>> +	dev_err(dev, "no ahb clk\n");
>> +	ret = PTR_ERR(nfc->clk.ahb_clk);
>> +	goto release_nfc;
>> +	}
>> +
>> +	ret = rk_nfc_enable_clk(dev, &nfc->clk);
>> +	if (ret)
>> +	goto release_nfc;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +	dev_err(dev, "no nfc irq resource\n");
>> +	ret = -EINVAL;
>> +	goto clk_disable;
>> +	}
>> +
>> +	writel(0, nfc->regs + nfc->cfg->int_en_off);
>> +	ret = devm_request_irq(dev, irq, rk_nfc_irq, 0x0, "rk-nand", nfc);
>> +	if (ret) {
>> +	dev_err(dev, "failed to request nfc irq\n");
>> +	goto clk_disable;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, nfc);
>> +
>> +	ret = rk_nfc_nand_chips_init(dev, nfc);
>> +	if (ret) {
>> +	dev_err(dev, "failed to init nand chips\n");
>> +	goto clk_disable;
>> +	}
>> +	return 0;
>> +
>> +clk_disable:
>> +	rk_nfc_disable_clk(&nfc->clk);
>> +release_nfc:
>> +	return ret;
>> +}
>> +
>> +static int rk_nfc_remove(struct platform_device *pdev)
>> +{
>> +	struct rk_nfc *nfc = platform_get_drvdata(pdev);
>> +	struct rk_nfc_nand_chip *nand;
>> +
>> +	while (!list_empty(&nfc->chips)) {
>> +	nand = list_first_entry(&nfc->chips, struct rk_nfc_nand_chip,
>> +	node);
>> +	nand_release(&nand->chip);
>> +	list_del(&nand->node);
>> +	}
>> +
>> +	rk_nfc_disable_clk(&nfc->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused rk_nfc_suspend(struct device *dev)
>> +{
>> +	struct rk_nfc *nfc = dev_get_drvdata(dev);
>> +
>> +	rk_nfc_disable_clk(&nfc->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused rk_nfc_resume(struct device *dev)
>> +{
>> +	struct rk_nfc *nfc = dev_get_drvdata(dev);
>> +	struct rk_nfc_nand_chip *nand;
>> +	struct nand_chip *chip;
>> +	int ret;
>> +	u32 i;
>> +
>> +	ret = rk_nfc_enable_clk(dev, &nfc->clk);
>> +	if (ret)
>> +	return ret;
>> +
>> +	/* reset NAND chip if VCC was powered off */
>> +	list_for_each_entry(nand, &nfc->chips, node) {
>> +	chip = &nand->chip;
>> +	for (i = 0; i < nand->nsels; i++)
>> +	nand_reset(chip, i);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops rk_nfc_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(rk_nfc_suspend, rk_nfc_resume)
>> +};
>> +
>> +static struct platform_driver rk_nfc_driver = {
>> +	.probe  = rk_nfc_probe,
>> +	.remove = rk_nfc_remove,
>> +	.driver = {
>> +	.name  = THIS_NAME,
>> +	.of_match_table = rk_nfc_id_table,
>> +	.pm = &rk_nfc_pm_ops,
>> +	},
>> +};
>> +
>> +module_platform_driver(rk_nfc_driver);
>> +
>> +MODULE_LICENSE("Dual MIT/GPL");
>> +MODULE_AUTHOR("Yifeng Zhao <yifeng.zhao@rock-chips.com>");
>> +MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver");
>> +MODULE_ALIAS("platform:rockchip_nand");
>>
>
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

Re: [PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others

[Johan Jonker]

Hi Yifeng,

A few more comments below for now (part 2).

On 4/26/20 12:02 PM, Yifeng Zhao wrote:

[..]

> +#define	THIS_NAME		"rk-nand"

> +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> +				 struct device_node *np)
> +{
> +	struct rk_nfc_nand_chip *nand;
> +	struct nand_chip *chip;
> +	struct mtd_info *mtd;
> +	int nsels;
> +	u32 tmp;
> +	int ret;
> +	int i;
> +
> +	if (!of_get_property(np, "reg", &nsels))
> +		return -ENODEV;
> +	nsels /= sizeof(u32);
> +	if (!nsels || nsels > NFC_MAX_NSELS) {
> +		dev_err(dev, "invalid reg property size %d\n", nsels);
> +		return -EINVAL;
> +	}
> +
> +	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
> +			    GFP_KERNEL);
> +	if (!nand)
> +		return -ENOMEM;
> +
> +	nand->nsels = nsels;
> +	for (i = 0; i < nsels; i++) {
> +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> +		if (ret) {
> +			dev_err(dev, "reg property failure : %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (tmp >= NFC_MAX_NSELS) {
> +			dev_err(dev, "invalid CS: %u\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> +			dev_err(dev, "CS %u already assigned\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		nand->sels[i] = tmp;
> +	}
> +
> +	chip = &nand->chip;
> +	chip->controller = &nfc->controller;
> +
> +	nand_set_flash_node(chip, np);
> +	nand_set_controller_data(chip, nfc);
> +
> +	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> +	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +
> +	/* set default mode in case dt entry is missing */
> +	chip->ecc.mode = NAND_ECC_HW;
> +
> +	mtd = nand_to_mtd(chip);
> +	mtd->owner = THIS_MODULE;
> +	mtd->dev.parent = dev;

> +	mtd->name = THIS_NAME;

The 'mtd->name' shows up somewhere in file tree.
The rk3288 has 2 nfc's. In theory 2 probes and also 2 device names, so I
think that we shouldn't use a fixed define for 'mtd->name'.
Maybe use something like this:

	mtd->name = devm_kasprintf(ctrl->dev, GFP_KERNEL, "%s",
				   dev_name(ctrl->dev));

> +	mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops);
> +	rk_nfc_hw_init(nfc);
> +	ret = nand_scan(chip, nsels);
> +	if (ret)
> +		return ret;
> +
> +	if (chip->options & NAND_IS_BOOT_MEDIUM) {
> +		ret = of_property_read_u32(np, "rockchip-boot-blks", &tmp);
> +		nand->boot_blks = ret ? 0 : tmp;
> +
> +		ret = of_property_read_u32(np, "rockchip-boot-ecc-strength",
> +					   &tmp);
> +		nand->boot_ecc = ret ? chip->ecc.strength : tmp;
> +	}
> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret) {
> +		dev_err(dev, "mtd parse partition error\n");
> +		nand_release(chip);
> +		return ret;
> +	}
> +
> +	list_add_tail(&nand->node, &nfc->chips);
> +
> +	return 0;
> +}

[..]

> +static struct platform_driver rk_nfc_driver = {
> +	.probe  = rk_nfc_probe,
> +	.remove = rk_nfc_remove,
> +	.driver = {

> +		.name  = THIS_NAME,

		.name  = "rockchip-nfc",
		.name  = "rockchip-nand-controller",

The driver name shows up in the kernel log and is used in combination
with 'greb'.
This name should stay in line with all other rockchip drivers.

rockchip-drm
rockchip-rk3066-hdmi
rockchip-pm-domain
rockchip-u3phy
rockchip-thermal

> +		.of_match_table = rk_nfc_id_table,
> +		.pm = &rk_nfc_pm_ops,
> +	},
> +};

Re: [PATCH v5 2/7] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others

[Miquel Raynal]

Hi Yifeng,

Johan Jonker <jbx6244-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on Wed, 29 Apr 2020 17:55:56
+0200:

> Hi Yifeng,
> 
> A few more comments below for now (part 2).
> 
> On 4/26/20 12:02 PM, Yifeng Zhao wrote:
> 
> [..]
> 
> > +#define	THIS_NAME		"rk-nand"  
> 
> > +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> > +				 struct device_node *np)
> > +{
> > +	struct rk_nfc_nand_chip *nand;
> > +	struct nand_chip *chip;
> > +	struct mtd_info *mtd;
> > +	int nsels;
> > +	u32 tmp;
> > +	int ret;
> > +	int i;
> > +
> > +	if (!of_get_property(np, "reg", &nsels))
> > +		return -ENODEV;
> > +	nsels /= sizeof(u32);
> > +	if (!nsels || nsels > NFC_MAX_NSELS) {
> > +		dev_err(dev, "invalid reg property size %d\n", nsels);
> > +		return -EINVAL;
> > +	}
> > +
> > +	nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8),
> > +			    GFP_KERNEL);
> > +	if (!nand)
> > +		return -ENOMEM;
> > +
> > +	nand->nsels = nsels;
> > +	for (i = 0; i < nsels; i++) {
> > +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> > +		if (ret) {
> > +			dev_err(dev, "reg property failure : %d\n", ret);
> > +			return ret;
> > +		}
> > +
> > +		if (tmp >= NFC_MAX_NSELS) {
> > +			dev_err(dev, "invalid CS: %u\n", tmp);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> > +			dev_err(dev, "CS %u already assigned\n", tmp);
> > +			return -EINVAL;
> > +		}
> > +
> > +		nand->sels[i] = tmp;
> > +	}
> > +
> > +	chip = &nand->chip;
> > +	chip->controller = &nfc->controller;
> > +
> > +	nand_set_flash_node(chip, np);
> > +	nand_set_controller_data(chip, nfc);
> > +
> > +	chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> > +	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> > +
> > +	/* set default mode in case dt entry is missing */
> > +	chip->ecc.mode = NAND_ECC_HW;
> > +
> > +	mtd = nand_to_mtd(chip);
> > +	mtd->owner = THIS_MODULE;
> > +	mtd->dev.parent = dev;  
> 
> > +	mtd->name = THIS_NAME;  
> 
> The 'mtd->name' shows up somewhere in file tree.

Good catch.

> The rk3288 has 2 nfc's. In theory 2 probes and also 2 device names, so I
> think that we shouldn't use a fixed define for 'mtd->name'.
> Maybe use something like this:

Yifeng, please use the NAND chip "label" DT property, which is parsed
by the core automatically and will give you meaningful names for every
chip:

nand_set_flash_node(chip, np);
if (!mtd->name) {
	dev_err(nfc->dev, "NAND label property is mandatory\n");
	return -EINVAL;
}


Thanks,
Miquèl