[PATCH v4 0/2] MTK Smart Device Gen1 NAND Driver

[PATCH v4 0/2] MTK Smart Device Gen1 NAND Driver

[Jorge Ramirez-Ortiz]

  Fixes/Changes on v4 relative to: 
  --------------------------------

  tree		: https://github.com/bbrezillon/linux-0day
  branch	: nand/ecclayout
  commit	:
 		'commit "mtd: kill the nand_ecclayout struct"'
		 Author: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
		 Date:	Thu Feb 4 10:16:18 2016 +0100
  Patch v4
  --------
  mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND
       - nand-on-flash-bbt, nand-ecc-mode, nand-ecc-step-size and
         nand-ecc-strength are now optional parameters
       - spare-per-sector removed from DT settings.
       - extend documentation.

  mtd: mediatek: driver for MTK Smart Device Gen1 NAND
       - rename functions (semantic issues).
       - add lock to ECC engine.
       - rework ECC interface (single enable/disable function).
       - rework bad block mark (now stored in OOB).
       - fix stat reporting.
       - rework FDM calculation.
       - handle optional DT settings (strength, mode, ecc step size).
       - spare per sector clamped to valid register values (hw spec).
		
  Patch v3:
  --------
  mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND
	- rename nandc (nand controller)

  mtd: mediatek: driver for MTK Smart Device Gen1 NAND
 	- rename source and header files
	- register definitions move to source files
	- ecc interface dropped
	- blank lines between functions (checkpatch.pl doesn't catch them)
	- ecc_check function renamed ecc_get_stats
	- ecc->dev assigned during probe()
	- generic ecc control function replaced by individual functions
	- required ecc parameters passed in ecc specific structures
	- fix memory corruption (size 0 array)
	- clarify nand offset/ptr function helpers (naming corrected)
	- move ecc config call to select_chip()
	- comment on hw issue workaround during read_byte
	- implement write_buf
	- remove switch_oob
	- fix suspend/resume operation
	- make nand-ecc-mode mandatory in device tree 

  RFC v2:
  -------
 
  mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND
	- split nand flash interface and ecc engine

  mtd: mediatek: driver for MTK Smart Device Gen1 NAND
	- split host and ecc in two separate drivers.
	- expose ecc layout through mtd_ooblayout_ops.
	- fix wait loops.
	- use device tree to describe ecc strength and ecc size.
	- use device tree to describe CS.
	- replace cmdfunc with cmd_ctrl.
	- fix read byte interface.

  dont enable controller in mt2701.dtsi
	- requires additional device tree changes. This will be done in
	  the near future.

Overview:
=========

The following patch-set adds support for Mediatek's Smart Device
Gen1 NAND controller.

* Controller expected layout:
-----------------------------

The controller expects the OOB information relevant to each sector
within the page to follow immediately the data field.

For example for a 4KiB page with 1KiB sectors and 224B of OOB information,
the controller would expect the following layout.

-----------------------------------------------------
| 1KiB | 56B | 1KiB | 56B | 1KiB | 56B | 1KiB | 56B |
-----------------------------------------------------

This implies that the following actions are required:

- raw writes:
  to match the controller expected layout, the driver uses the CPU and
  a private bounce buffer to reassemble the page before the actual
  writes.
  
- non raw writes:
  ecc is hardware generated and data DMA'd in a single transfer.

Bad Block mark
==============
The bad block mark is stored in the OOB region (one byte)

Controller limitations:
=======================

 * dma chaning not supported:
	To guarantee that the pages corresponding to the buffers are contiguous
	in memory the driver enables NAND_USE_BOUNCE_BUFFER.
 * spare per sector: 16B to 128B
 * sector sizes: 512B or 1024B
 * programable page sizes: 512B, 2KB, 4KB, 8K, 16K 
 

Tests:
======

* UBIFS support has been validated on 512MiB device (iozone, dd)
* All drivers/mtd/tests/* pass clean.
* Speed tests:

	- eraseblock write speed		: 7487 KiB/s
	- eraseblock read speed 		: 10786 KiB/s
	- page write speed 			: 7406 KiB/s
	- page read speed 			: 10741 KiB/s
	- 2 page write speed 			: 7447 KiB/s
	- 2 page read speed			: 10763 KiB/s
	- erase speed				: 105774 KiB/s
	- 2x multi-block erase speed 		: 361120 KiB/s
	- 4x multi-block erase speed		: 362622 KiB/s
	- 8x multi-block erase speed		: 362622 KiB/s
	- 16x multi-block erase speed		: 363377 KiB/s
	- 32x multi-block erase speed		: 363377 KiB/s
	- 64x multi-block erase speed		: 363883 KiB/s


Jorge Ramirez-Ortiz (2):
  mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND
  mtd: mediatek: driver for MTK Smart Device Gen1 NAND

 Documentation/devicetree/bindings/mtd/mtk-nand.txt |  161 +++
 drivers/mtd/nand/Kconfig                           |    7 +
 drivers/mtd/nand/Makefile                          |    1 +
 drivers/mtd/nand/mtk_ecc.c                         |  527 +++++++
 drivers/mtd/nand/mtk_ecc.h                         |   53 +
 drivers/mtd/nand/mtk_nand.c                        | 1432 ++++++++++++++++++++
 6 files changed, 2181 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt
 create mode 100644 drivers/mtd/nand/mtk_ecc.c
 create mode 100644 drivers/mtd/nand/mtk_ecc.h
 create mode 100644 drivers/mtd/nand/mtk_nand.c

-- 
2.7.4

[PATCH v4 1/2] mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND

[Jorge Ramirez-Ortiz]

This patch adds documentation support for Smart Device Gen1 type of
NAND controllers.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/mtk-nand.txt | 161 +++++++++++++++++++++
 1 file changed, 161 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
new file mode 100644
index 0000000..175767d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
@@ -0,0 +1,161 @@
+MTK SoCs NAND FLASH controller (NFC) DT binding
+
+This file documents the device tree bindings for MTK SoCs NAND controllers.
+The functional split of the controller requires two drivers to operate:
+the nand controller interface driver and the ECC engine driver.
+
+The hardware description for both devices must be captured as device
+tree nodes.
+
+1) NFC NAND Controller Interface (NFI):
+=======================================
+
+The first part of NFC is NAND Controller Interface (NFI) HW.
+Required NFI properties:
+- compatible:			Should be "mediatek,mtxxxx-nfc".
+- reg:				Base physical address and size of NFI.
+- interrupts:			Interrupts of NFI.
+- clocks:			NFI required clocks.
+- clock-names:			NFI clocks internal name.
+- status:			Disabled default. Then set "okay" by platform.
+- ecc-engine:			Required ECC Engine node.
+- #address-cells:		NAND chip index, should be 1.
+- #size-cells:			Should be 0.
+
+Example:
+
+	nandc: nfi@1100d000 {
+		compatible = "mediatek,mt2701-nfc";
+		reg = <0 0x1100d000 0 0x1000>;
+		interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&pericfg CLK_PERI_NFI>,
+			 <&pericfg CLK_PERI_NFI_PAD>;
+		clock-names = "nfi_clk", "pad_clk";
+		status = "disabled";
+		ecc-engine = <&bch>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+        };
+
+Platform related properties, should be set in {platform_name}.dts:
+- children nodes:	NAND chips.
+
+Children nodes properties:
+- reg:			Chip Select Signal, default 0.
+			Set as reg = <0>, <1> when need 2 CS.
+Optional:
+- nand-on-flash-bbt:	Store BBT on NAND Flash.
+- nand-ecc-mode:	the NAND ecc mode (check driver for supported modes)
+- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
+			The controller only supports 512 and 1024.
+			For large page NANDs ther recommended value is 1024.
+- nand-ecc-strength:	Number of bits to correct per ECC step.
+			The valid values that the controller supports are: 4, 6,
+			8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, 40, 44,
+			48, 52, 56, 60.
+			The strength should be calculated as follows:
+			E = (S - F) * 8 / 14
+			S = O / (P / Q)
+				E :nand-ecc-strength;
+				S :spare size per sector;
+				F : FDM size, should be in the range [1,8].
+				    It is used to store free oob data.
+				O : oob size;
+				P : page size;
+				Q :nand-ecc-step-size
+			If the result does not match any one of the listed
+			choices above, please select the smaller valid value from
+			the list.
+			(otherwise the driver will do the clamping at runtime).
+- vmch-supply:		NAND power supply.
+- pinctrl-names:	Default NAND pin GPIO setting name.
+- pinctrl-0:		GPIO setting node.
+
+Example:
+	&pio {
+		nand_pins_default: nanddefault {
+			pins_dat {
+				pinmux = <MT2701_PIN_111_MSDC0_DAT7__FUNC_NLD7>,
+					 <MT2701_PIN_112_MSDC0_DAT6__FUNC_NLD6>,
+					 <MT2701_PIN_114_MSDC0_DAT4__FUNC_NLD4>,
+					 <MT2701_PIN_118_MSDC0_DAT3__FUNC_NLD3>,
+					 <MT2701_PIN_121_MSDC0_DAT0__FUNC_NLD0>,
+					 <MT2701_PIN_120_MSDC0_DAT1__FUNC_NLD1>,
+					 <MT2701_PIN_113_MSDC0_DAT5__FUNC_NLD5>,
+					 <MT2701_PIN_115_MSDC0_RSTB__FUNC_NLD8>,
+					 <MT2701_PIN_119_MSDC0_DAT2__FUNC_NLD2>;
+				input-enable;
+				drive-strength = <MTK_DRIVE_8mA>;
+				bias-pull-up;
+			};
+
+			pins_we {
+				pinmux = <MT2701_PIN_117_MSDC0_CLK__FUNC_NWEB>;
+				drive-strength = <MTK_DRIVE_8mA>;
+				bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
+			};
+
+			pins_ale {
+				pinmux = <MT2701_PIN_116_MSDC0_CMD__FUNC_NALE>;
+				drive-strength = <MTK_DRIVE_8mA>;
+				bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
+			};
+		};
+	};
+
+	&nandc {
+		status = "okay";
+		pinctrl-names = "default";
+		pinctrl-0 = <&nand_pins_default>;
+		nand@0 {
+			reg = <0>;
+			nand-on-flash-bbt;
+			nand-ecc-mode = "hw";
+			nand-ecc-strength = <24>;
+			nand-ecc-step-size = <1024>;
+		};
+	};
+
+NAND chip optional subnodes:
+- Partitions, see Documentation/devicetree/bindings/mtd/partition.txt
+
+Example:
+	nand@0 {
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			preloader@0 {
+				label = "pl";
+				read-only;
+				reg = <0x00000000 0x00400000>;
+			};
+			android@0x00400000 {
+				label = "android";
+				reg = <0x00400000 0x12c00000>;
+			};
+		};
+	};
+
+2) ECC Engine:
+==============
+
+Required BCH properties:
+- compatible:	Should be "mediatek,mtxxxx-ecc".
+- reg:		Base physical address and size of ECC.
+- interrupts:	Interrupts of ECC.
+- clocks:	ECC required clocks.
+- clock-names:	ECC clocks internal name.
+- status:	Disabled default. Then set "okay" by platform.
+
+Example:
+
+	bch: ecc@1100e000 {
+		compatible = "mediatek,mt2701-ecc";
+		reg = <0 0x1100e000 0 0x1000>;
+		interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&pericfg CLK_PERI_NFI_ECC>;
+		clock-names = "nfiecc_clk";
+		status = "disabled";
+	};
-- 
2.7.4

Re: [PATCH v4 1/2] mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND

[Boris Brezillon]

Hi Jorge,

On Fri, 29 Apr 2016 12:17:21 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> This patch adds documentation support for Smart Device Gen1 type of
> NAND controllers.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mtd/mtk-nand.txt | 161 +++++++++++++++++++++
>  1 file changed, 161 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> new file mode 100644
> index 0000000..175767d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt

[...]

> +
> +Children nodes properties:
> +- reg:			Chip Select Signal, default 0.
> +			Set as reg = <0>, <1> when need 2 CS.
> +Optional:
> +- nand-on-flash-bbt:	Store BBT on NAND Flash.
> +- nand-ecc-mode:	the NAND ecc mode (check driver for supported modes)
> +- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
> +			The controller only supports 512 and 1024.
> +			For large page NANDs ther recommended value is 1024.
> +- nand-ecc-strength:	Number of bits to correct per ECC step.
> +			The valid values that the controller supports are: 4, 6,
> +			8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, 40, 44,
> +			48, 52, 56, 60.
> +			The strength should be calculated as follows:
> +			E = (S - F) * 8 / 14
> +			S = O / (P / Q)
> +				E :nand-ecc-strength;
> +				S :spare size per sector;
> +				F : FDM size, should be in the range [1,8].
> +				    It is used to store free oob data.
> +				O : oob size;
> +				P : page size;
> +				Q :nand-ecc-step-size
> +			If the result does not match any one of the listed
> +			choices above, please select the smaller valid value from
> +			the list.
> +			(otherwise the driver will do the clamping at runtime).
> +- vmch-supply:		NAND power supply.

Where is this supply name coming from? I most datasheets I see Vdd or
Vcc, but nothing close to Vmch.



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 1/2] mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/06/2016 09:38 AM, Boris Brezillon wrote:
> Hi Jorge,
>
> On Fri, 29 Apr 2016 12:17:21 -0400
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> This patch adds documentation support for Smart Device Gen1 type of
>> NAND controllers.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/mtd/mtk-nand.txt | 161 +++++++++++++++++++++
>>   1 file changed, 161 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
>> new file mode 100644
>> index 0000000..175767d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> [...]
>
>> +
>> +Children nodes properties:
>> +- reg:			Chip Select Signal, default 0.
>> +			Set as reg = <0>, <1> when need 2 CS.
>> +Optional:
>> +- nand-on-flash-bbt:	Store BBT on NAND Flash.
>> +- nand-ecc-mode:	the NAND ecc mode (check driver for supported modes)
>> +- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
>> +			The controller only supports 512 and 1024.
>> +			For large page NANDs ther recommended value is 1024.
>> +- nand-ecc-strength:	Number of bits to correct per ECC step.
>> +			The valid values that the controller supports are: 4, 6,
>> +			8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, 40, 44,
>> +			48, 52, 56, 60.
>> +			The strength should be calculated as follows:
>> +			E = (S - F) * 8 / 14
>> +			S = O / (P / Q)
>> +				E :nand-ecc-strength;
>> +				S :spare size per sector;
>> +				F : FDM size, should be in the range [1,8].
>> +				    It is used to store free oob data.
>> +				O : oob size;
>> +				P : page size;
>> +				Q :nand-ecc-step-size
>> +			If the result does not match any one of the listed
>> +			choices above, please select the smaller valid value from
>> +			the list.
>> +			(otherwise the driver will do the clamping at runtime).
>> +- vmch-supply:		NAND power supply.
> Where is this supply name coming from? I most datasheets I see Vdd or
> Vcc, but nothing close to Vmch.
>
>
>

um yes we are removing this since we dont use it (it is the NAND vcc on 
some other MTK platform to do power management via regulator).

Re: [PATCH v4 1/2] mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND

[Boris Brezillon]

On Tue, 10 May 2016 07:57:25 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/06/2016 09:38 AM, Boris Brezillon wrote:
> > Hi Jorge,
> >
> > On Fri, 29 Apr 2016 12:17:21 -0400
> > Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >  
> >> This patch adds documentation support for Smart Device Gen1 type of
> >> NAND controllers.
> >>
> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>   Documentation/devicetree/bindings/mtd/mtk-nand.txt | 161 +++++++++++++++++++++
> >>   1 file changed, 161 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/mtd/mtk-nand.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/mtk-nand.txt b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> >> new file mode 100644
> >> index 0000000..175767d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt  
> > [...]
> >  
> >> +
> >> +Children nodes properties:
> >> +- reg:			Chip Select Signal, default 0.
> >> +			Set as reg = <0>, <1> when need 2 CS.
> >> +Optional:
> >> +- nand-on-flash-bbt:	Store BBT on NAND Flash.
> >> +- nand-ecc-mode:	the NAND ecc mode (check driver for supported modes)
> >> +- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
> >> +			The controller only supports 512 and 1024.
> >> +			For large page NANDs ther recommended value is 1024.
> >> +- nand-ecc-strength:	Number of bits to correct per ECC step.
> >> +			The valid values that the controller supports are: 4, 6,
> >> +			8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36, 40, 44,
> >> +			48, 52, 56, 60.
> >> +			The strength should be calculated as follows:
> >> +			E = (S - F) * 8 / 14
> >> +			S = O / (P / Q)
> >> +				E :nand-ecc-strength;
> >> +				S :spare size per sector;
> >> +				F : FDM size, should be in the range [1,8].
> >> +				    It is used to store free oob data.
> >> +				O : oob size;
> >> +				P : page size;
> >> +				Q :nand-ecc-step-size
> >> +			If the result does not match any one of the listed
> >> +			choices above, please select the smaller valid value from
> >> +			the list.
> >> +			(otherwise the driver will do the clamping at runtime).
> >> +- vmch-supply:		NAND power supply.  
> > Where is this supply name coming from? I most datasheets I see Vdd or
> > Vcc, but nothing close to Vmch.
> >
> >
> >  
> 
> um yes we are removing this since we dont use it (it is the NAND vcc on 
> some other MTK platform to do power management via regulator).
> 

Ok, no problem. Actually it's yet another property that should be
described in the generic binding doc.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez-Ortiz]

This patch adds support for mediatek's SDG1 NFC nand controller
embedded in SoC 2701

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mtd/nand/Kconfig    |    7 +
 drivers/mtd/nand/Makefile   |    1 +
 drivers/mtd/nand/mtk_ecc.c  |  527 ++++++++++++++++
 drivers/mtd/nand/mtk_ecc.h  |   53 ++
 drivers/mtd/nand/mtk_nand.c | 1432 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 2020 insertions(+)
 create mode 100644 drivers/mtd/nand/mtk_ecc.c
 create mode 100644 drivers/mtd/nand/mtk_ecc.h
 create mode 100644 drivers/mtd/nand/mtk_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9..3c26e89 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -563,4 +563,11 @@ config MTD_NAND_QCOM
 	  Enables support for NAND flash chips on SoCs containing the EBI2 NAND
 	  controller. This controller is found on IPQ806x SoC.
 
+config MTD_NAND_MTK
+	tristate "Support for NAND controller on MTK SoCs"
+	depends on HAS_DMA
+	help
+	  Enables support for NAND controller on MTK SoCs.
+	  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f553353..cafde6f 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -57,5 +57,6 @@ obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
 obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
+obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_nand.o mtk_ecc.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
new file mode 100644
index 0000000..28769f1
--- /dev/null
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -0,0 +1,527 @@
+/*
+ * MTK ECC controller driver.
+ * Copyright (C) 2016  MediaTek Inc.
+ * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/iopoll.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/semaphore.h>
+
+#include "mtk_ecc.h"
+
+#define ECC_ENCCON		(0x00)
+#define		ENC_EN			(1)
+#define		ENC_DE			(0)
+#define ECC_ENCCNFG		(0x04)
+#define		ECC_CNFG_4BIT		(0)
+#define		ECC_CNFG_6BIT		(1)
+#define		ECC_CNFG_8BIT		(2)
+#define		ECC_CNFG_10BIT		(3)
+#define		ECC_CNFG_12BIT		(4)
+#define		ECC_CNFG_14BIT		(5)
+#define		ECC_CNFG_16BIT		(6)
+#define		ECC_CNFG_18BIT		(7)
+#define		ECC_CNFG_20BIT		(8)
+#define		ECC_CNFG_22BIT		(9)
+#define		ECC_CNFG_24BIT		(0xa)
+#define		ECC_CNFG_28BIT		(0xb)
+#define		ECC_CNFG_32BIT		(0xc)
+#define		ECC_CNFG_36BIT		(0xd)
+#define		ECC_CNFG_40BIT		(0xe)
+#define		ECC_CNFG_44BIT		(0xf)
+#define		ECC_CNFG_48BIT		(0x10)
+#define		ECC_CNFG_52BIT		(0x11)
+#define		ECC_CNFG_56BIT		(0x12)
+#define		ECC_CNFG_60BIT		(0x13)
+#define		ECC_MODE_SHIFT		(5)
+#define		ECC_MS_SHIFT		(16)
+#define ECC_ENCDIADDR		(0x08)
+#define ECC_ENCIDLE		(0x0C)
+#define		ENC_IDLE		BIT(0)
+#define ECC_ENCPAR(x)		(0x10 + (x) * sizeof(u32))
+#define ECC_ENCIRQ_EN		(0x80)
+#define		ENC_IRQEN		BIT(0)
+#define ECC_ENCIRQ_STA		(0x84)
+#define ECC_DECCON		(0x100)
+#define		DEC_EN			(1)
+#define		DEC_DE			(0)
+#define ECC_DECCNFG		(0x104)
+#define		DEC_EMPTY_EN		BIT(31)
+#define		DEC_CNFG_CORRECT	(0x3 << 12)
+#define ECC_DECIDLE		(0x10C)
+#define		DEC_IDLE		BIT(0)
+#define ECC_DECENUM0		(0x114)
+#define		ERR_MASK		(0x3f)
+#define ECC_DECDONE		(0x124)
+#define ECC_DECIRQ_EN		(0x200)
+#define		DEC_IRQEN		BIT(0)
+#define ECC_DECIRQ_STA		(0x204)
+
+#define ECC_TIMEOUT		(500000)
+
+#define ECC_IDLE_REG(x)		((x) == ECC_ENC ? ECC_ENCIDLE : ECC_DECIDLE)
+#define ECC_IDLE_MASK(x)	((x) == ECC_ENC ? ENC_IDLE : DEC_IDLE)
+#define ECC_IRQ_REG(x)		((x) == ECC_ENC ? ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
+#define ECC_IRQ_EN(x)		((x) == ECC_ENC ? ENC_IRQEN : DEC_IRQEN)
+#define ECC_CTL_REG(x)		((x) == ECC_ENC ? ECC_ENCCON : ECC_DECCON)
+#define ECC_CODEC_ENABLE(x)	((x) == ECC_ENC ? ENC_EN : DEC_EN)
+#define ECC_CODEC_DISABLE(x)	((x) == ECC_ENC ? ENC_DE : DEC_DE)
+
+struct mtk_ecc {
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *clk;
+
+	struct completion done;
+	struct semaphore sem;
+	u32 sec_mask;
+};
+
+static inline void mtk_ecc_codec_wait_idle(struct mtk_ecc *ecc,
+					enum mtk_ecc_codec codec)
+{
+	struct device *dev = ecc->dev;
+	u32 val;
+	int ret;
+
+	ret = readl_poll_timeout_atomic(ecc->regs + ECC_IDLE_REG(codec), val,
+					val & ECC_IDLE_MASK(codec),
+					10, ECC_TIMEOUT);
+	if (ret)
+		dev_warn(dev, "%s NOT idle\n",
+			codec == ECC_ENC ? "encoder" : "decoder");
+}
+
+static irqreturn_t mtk_ecc_irq(int irq, void *id)
+{
+	struct mtk_ecc *ecc = id;
+	enum mtk_ecc_codec codec;
+	u32 dec, enc;
+
+	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
+	if (dec) {
+		codec = ECC_DEC;
+		dec = readw(ecc->regs + ECC_DECDONE);
+		if (dec & ecc->sec_mask) {
+			ecc->sec_mask = 0;
+			complete(&ecc->done);
+		} else
+			return IRQ_HANDLED;
+	} else {
+		enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
+		if (enc) {
+			codec = ECC_ENC;
+			complete(&ecc->done);
+		} else
+			return IRQ_NONE;
+	}
+
+	writel(0, ecc->regs + ECC_IRQ_REG(codec));
+
+	return IRQ_HANDLED;
+}
+
+static void mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
+{
+	u32 ecc_bit = ECC_CNFG_4BIT, dec_sz, enc_sz;
+	u32 reg;
+
+	switch (config->strength) {
+	case 4:
+		ecc_bit = ECC_CNFG_4BIT;
+		break;
+	case 6:
+		ecc_bit = ECC_CNFG_6BIT;
+		break;
+	case 8:
+		ecc_bit = ECC_CNFG_8BIT;
+		break;
+	case 10:
+		ecc_bit = ECC_CNFG_10BIT;
+		break;
+	case 12:
+		ecc_bit = ECC_CNFG_12BIT;
+		break;
+	case 14:
+		ecc_bit = ECC_CNFG_14BIT;
+		break;
+	case 16:
+		ecc_bit = ECC_CNFG_16BIT;
+		break;
+	case 18:
+		ecc_bit = ECC_CNFG_18BIT;
+		break;
+	case 20:
+		ecc_bit = ECC_CNFG_20BIT;
+		break;
+	case 22:
+		ecc_bit = ECC_CNFG_22BIT;
+		break;
+	case 24:
+		ecc_bit = ECC_CNFG_24BIT;
+		break;
+	case 28:
+		ecc_bit = ECC_CNFG_28BIT;
+		break;
+	case 32:
+		ecc_bit = ECC_CNFG_32BIT;
+		break;
+	case 36:
+		ecc_bit = ECC_CNFG_36BIT;
+		break;
+	case 40:
+		ecc_bit = ECC_CNFG_40BIT;
+		break;
+	case 44:
+		ecc_bit = ECC_CNFG_44BIT;
+		break;
+	case 48:
+		ecc_bit = ECC_CNFG_48BIT;
+		break;
+	case 52:
+		ecc_bit = ECC_CNFG_52BIT;
+		break;
+	case 56:
+		ecc_bit = ECC_CNFG_56BIT;
+		break;
+	case 60:
+		ecc_bit = ECC_CNFG_60BIT;
+		break;
+	default:
+		dev_err(ecc->dev, "invalid strength %d\n", config->strength);
+	}
+
+	if (config->codec == ECC_ENC) {
+		/* configure ECC encoder (in bits) */
+		enc_sz = config->enc_len << 3;
+
+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
+		reg |= (enc_sz << ECC_MS_SHIFT);
+		writel(reg, ecc->regs + ECC_ENCCNFG);
+
+		if (config->ecc_mode != ECC_NFI_MODE)
+			writel(lower_32_bits(config->addr),
+				ecc->regs + ECC_ENCDIADDR);
+
+	} else {
+		/* configure ECC decoder (in bits) */
+		dec_sz = config->dec_len;
+
+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
+		reg |= (dec_sz << ECC_MS_SHIFT) | DEC_CNFG_CORRECT;
+		reg |= DEC_EMPTY_EN;
+		writel(reg, ecc->regs + ECC_DECCNFG);
+
+		if (config->sec_mask)
+			ecc->sec_mask = 1 << (config->sec_mask - 1);
+	}
+}
+
+void mtk_ecc_get_stats(struct mtk_ecc *ecc, struct mtk_ecc_stats *stats,
+			int sectors)
+{
+	u32 offset, i, err;
+	u32 bitflips = 0;
+
+	stats->corrected = 0;
+	stats->failed = 0;
+
+	for (i = 0; i < sectors; i++) {
+		offset = (i >> 2) << 2;
+		err = readl(ecc->regs + ECC_DECENUM0 + offset);
+		err = err >> ((i % 4) * 8);
+		err &= ERR_MASK;
+		if (err == ERR_MASK) {
+			/* uncorrectable errors */
+			stats->failed++;
+			continue;
+		}
+
+		stats->corrected += err;
+		bitflips = max_t(u32, bitflips, err);
+	}
+
+	stats->bitflips = bitflips;
+}
+EXPORT_SYMBOL(mtk_ecc_get_stats);
+
+void mtk_ecc_release(struct mtk_ecc *ecc)
+{
+	clk_disable_unprepare(ecc->clk);
+	put_device(ecc->dev);
+}
+EXPORT_SYMBOL(mtk_ecc_release);
+
+static struct mtk_ecc *mtk_ecc_get(struct device_node *np)
+{
+	struct platform_device *pdev;
+	struct mtk_ecc *ecc;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev || !platform_get_drvdata(pdev))
+		return ERR_PTR(-EPROBE_DEFER);
+
+	get_device(&pdev->dev);
+	ecc = platform_get_drvdata(pdev);
+	clk_prepare_enable(ecc->clk);
+	mtk_ecc_hw_init(ecc);
+
+	return ecc;
+}
+
+struct mtk_ecc *of_mtk_ecc_get(struct device_node *of_node)
+{
+	struct mtk_ecc *ecc = NULL;
+	struct device_node *np;
+
+	np = of_parse_phandle(of_node, "ecc-engine", 0);
+	if (np) {
+		ecc = mtk_ecc_get(np);
+		of_node_put(np);
+	}
+
+	return ecc;
+}
+EXPORT_SYMBOL(of_mtk_ecc_get);
+
+int mtk_ecc_enable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
+{
+	enum mtk_ecc_codec codec = config->codec;
+	int ret;
+
+	ret = down_interruptible(&ecc->sem);
+	if (ret) {
+		dev_err(ecc->dev, "interrupted when attempting to lock\n");
+		return ret;
+	}
+
+	mtk_ecc_codec_wait_idle(ecc, codec);
+	mtk_ecc_config(ecc, config);
+	writew(ECC_CODEC_ENABLE(codec), ecc->regs + ECC_CTL_REG(codec));
+
+	init_completion(&ecc->done);
+	writew(ECC_IRQ_EN(codec), ecc->regs + ECC_IRQ_REG(codec));
+
+	return 0;
+}
+EXPORT_SYMBOL(mtk_ecc_enable);
+
+void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
+{
+	enum mtk_ecc_codec codec = config->codec;
+
+	mtk_ecc_codec_wait_idle(ecc, codec);
+	writew(0, ecc->regs + ECC_IRQ_REG(codec));
+	writew(ECC_CODEC_DISABLE(codec), ecc->regs + ECC_CTL_REG(codec));
+	up(&ecc->sem);
+}
+EXPORT_SYMBOL(mtk_ecc_disable);
+
+int mtk_ecc_wait_irq_done(struct mtk_ecc *ecc, enum mtk_ecc_codec codec)
+{
+	int ret;
+
+	ret = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
+	if (!ret) {
+		dev_err(ecc->dev, "%s timeout - interrupt did not arrive)\n",
+				(codec == ECC_ENC) ? "encoder" : "decoder");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(mtk_ecc_wait_irq_done);
+
+int mtk_ecc_encode_non_nfi_mode(struct mtk_ecc *ecc,
+			struct mtk_ecc_config *config, u8 *data, u32 bytes)
+{
+	dma_addr_t addr;
+	u32 *p, len, i;
+	int ret = 0;
+
+	addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
+	ret = dma_mapping_error(ecc->dev, addr);
+	if (ret) {
+		dev_err(ecc->dev, "dma mapping error\n");
+		return -EINVAL;
+	}
+
+	config->codec = ECC_ENC;
+	config->addr = addr;
+	ret = mtk_ecc_enable(ecc, config);
+	if (ret) {
+		dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
+		return ret;
+	}
+
+	ret = mtk_ecc_wait_irq_done(ecc, ECC_ENC);
+	if (ret)
+		goto timeout;
+
+	mtk_ecc_codec_wait_idle(ecc, ECC_ENC);
+
+	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
+	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
+	p = (u32 *) (data + bytes);
+
+	/* write the parity bytes generated by the ECC back to the OOB region */
+	for (i = 0; i < len; i++)
+		p[i] = readl(ecc->regs + ECC_ENCPAR(i));
+timeout:
+
+	dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
+	mtk_ecc_disable(ecc, config);
+
+	return ret;
+}
+EXPORT_SYMBOL(mtk_ecc_encode_non_nfi_mode);
+
+void mtk_ecc_hw_init(struct mtk_ecc *ecc)
+{
+	mtk_ecc_codec_wait_idle(ecc, ECC_ENC);
+	writew(ENC_DE, ecc->regs + ECC_ENCCON);
+
+	mtk_ecc_codec_wait_idle(ecc, ECC_DEC);
+	writel(DEC_DE, ecc->regs + ECC_DECCON);
+}
+
+void mtk_ecc_update_strength(u32 *p)
+{
+	u32 ecc[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
+			40, 44, 48, 52, 56, 60};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ecc); i++) {
+		if (*p <= ecc[i]) {
+			if (!i)
+				*p = ecc[i];
+			else if (*p != ecc[i])
+				*p = ecc[i - 1];
+			return;
+		}
+	}
+
+	*p = ecc[ARRAY_SIZE(ecc) - 1];
+}
+EXPORT_SYMBOL(mtk_ecc_update_strength);
+
+static int mtk_ecc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_ecc *ecc;
+	struct resource *res;
+	int irq, ret;
+
+	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
+	if (!ecc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ecc->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ecc->regs)) {
+		dev_err(dev, "failed to map regs: %ld\n", PTR_ERR(ecc->regs));
+		return PTR_ERR(ecc->regs);
+	}
+
+	ecc->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ecc->clk)) {
+		dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(ecc->clk));
+		return PTR_ERR(ecc->clk);
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "failed to get irq\n");
+		return -EINVAL;
+	}
+
+	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(dev, "failed to set DMA mask\n");
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, irq, mtk_ecc_irq, 0x0, "mtk-ecc", ecc);
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		return -EINVAL;
+	}
+
+	ecc->dev = dev;
+	sema_init(&ecc->sem, 1);
+	platform_set_drvdata(pdev, ecc);
+	dev_info(dev, "probed\n");
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_ecc_suspend(struct device *dev)
+{
+	struct mtk_ecc *ecc = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(ecc->clk);
+
+	return 0;
+}
+
+static int mtk_ecc_resume(struct device *dev)
+{
+	struct mtk_ecc *ecc = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(ecc->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clk\n");
+		return ret;
+	}
+
+	mtk_ecc_hw_init(ecc);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(mtk_ecc_pm_ops, mtk_ecc_suspend, mtk_ecc_resume);
+#endif
+
+static const struct of_device_id mtk_ecc_dt_match[] = {
+	{ .compatible = "mediatek,mt2701-ecc" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, mtk_ecc_dt_match);
+
+static struct platform_driver mtk_ecc_driver = {
+	.probe  = mtk_ecc_probe,
+	.driver = {
+		.name  = "mtk-ecc",
+		.of_match_table = of_match_ptr(mtk_ecc_dt_match),
+#ifdef CONFIG_PM_SLEEP
+		.pm = &mtk_ecc_pm_ops,
+#endif
+	},
+};
+
+module_platform_driver(mtk_ecc_driver);
+
+MODULE_AUTHOR("Xiaolei Li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
+MODULE_AUTHOR("Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
+MODULE_DESCRIPTION("MTK Nand ECC Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
new file mode 100644
index 0000000..434826f
--- /dev/null
+++ b/drivers/mtd/nand/mtk_ecc.h
@@ -0,0 +1,53 @@
+/*
+ * MTK SDG1 ECC controller
+ *
+ * Copyright (c) 2016 Mediatek
+ * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#ifndef __DRIVERS_MTD_NAND_MTK_ECC_H__
+#define __DRIVERS_MTD_NAND_MTK_ECC_H__
+
+#include <linux/types.h>
+
+#define ECC_PARITY_BITS		(14)
+
+enum mtk_ecc_mode {ECC_DMA_MODE = 0, ECC_NFI_MODE = 1};
+enum mtk_ecc_codec {ECC_ENC, ECC_DEC};
+
+struct device_node;
+struct mtk_ecc;
+
+struct mtk_ecc_stats {
+	u32 corrected;
+	u32 bitflips;
+	u32 failed;
+};
+
+struct mtk_ecc_config {
+	enum mtk_ecc_mode ecc_mode;
+	enum mtk_ecc_codec codec;
+	dma_addr_t addr;
+	u32 sec_mask;
+	u32 strength;
+	u32 enc_len;
+	u32 dec_len;
+};
+
+int mtk_ecc_enable(struct mtk_ecc *, struct mtk_ecc_config *);
+void mtk_ecc_disable(struct mtk_ecc *, struct mtk_ecc_config *);
+int mtk_ecc_encode_non_nfi_mode(struct mtk_ecc *, struct mtk_ecc_config *,
+				u8 *, u32);
+void mtk_ecc_get_stats(struct mtk_ecc *, struct mtk_ecc_stats *, int);
+int mtk_ecc_wait_irq_done(struct mtk_ecc *, enum mtk_ecc_codec);
+void mtk_ecc_hw_init(struct mtk_ecc *);
+void mtk_ecc_update_strength(u32 *);
+
+struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
+void mtk_ecc_release(struct mtk_ecc *);
+
+#endif
diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
new file mode 100644
index 0000000..907b90c
--- /dev/null
+++ b/drivers/mtd/nand/mtk_nand.c
@@ -0,0 +1,1432 @@
+/*
+ * MTK NAND Flash controller driver.
+ * Copyright (C) 2016 MediaTek Inc.
+ * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/mtd.h>
+#include <linux/module.h>
+#include <linux/iopoll.h>
+#include <linux/of.h>
+#include "mtk_ecc.h"
+
+/* NAND controller register definition */
+#define NFI_CNFG		(0x00)
+#define		CNFG_AHB		BIT(0)
+#define		CNFG_READ_EN		BIT(1)
+#define		CNFG_DMA_BURST_EN	BIT(2)
+#define		CNFG_BYTE_RW		BIT(6)
+#define		CNFG_HW_ECC_EN		BIT(8)
+#define		CNFG_AUTO_FMT_EN	BIT(9)
+#define		CNFG_OP_CUST		(6 << 12)
+#define NFI_PAGEFMT		(0x04)
+#define		PAGEFMT_FDM_ECC_SHIFT	(12)
+#define		PAGEFMT_FDM_SHIFT	(8)
+#define		PAGEFMT_SPARE_16	(0)
+#define		PAGEFMT_SPARE_26	(1)
+#define		PAGEFMT_SPARE_27	(2)
+#define		PAGEFMT_SPARE_28	(3)
+#define		PAGEFMT_SPARE_32	(4)
+#define		PAGEFMT_SPARE_36	(5)
+#define		PAGEFMT_SPARE_40	(6)
+#define		PAGEFMT_SPARE_44	(7)
+#define		PAGEFMT_SPARE_48	(8)
+#define		PAGEFMT_SPARE_49	(9)
+#define		PAGEFMT_SPARE_50	(0xa)
+#define		PAGEFMT_SPARE_51	(0xb)
+#define		PAGEFMT_SPARE_52	(0xc)
+#define		PAGEFMT_SPARE_62	(0xd)
+#define		PAGEFMT_SPARE_63	(0xe)
+#define		PAGEFMT_SPARE_64	(0xf)
+#define		PAGEFMT_SPARE_SHIFT	(4)
+#define		PAGEFMT_SEC_SEL_512	BIT(2)
+#define		PAGEFMT_512_2K		(0)
+#define		PAGEFMT_2K_4K		(1)
+#define		PAGEFMT_4K_8K		(2)
+#define		PAGEFMT_8K_16K		(3)
+/* NFI control */
+#define NFI_CON			(0x08)
+#define		CON_FIFO_FLUSH		BIT(0)
+#define		CON_NFI_RST		BIT(1)
+#define		CON_BRD			BIT(8)  /* burst  read */
+#define		CON_BWR			BIT(9)	/* burst  write */
+#define		CON_SEC_SHIFT		(12)
+/* Timming control register */
+#define NFI_ACCCON		(0x0C)
+#define NFI_INTR_EN		(0x10)
+#define		INTR_AHB_DONE_EN	BIT(6)
+#define NFI_INTR_STA		(0x14)
+#define NFI_CMD			(0x20)
+#define NFI_ADDRNOB		(0x30)
+#define NFI_COLADDR		(0x34)
+#define NFI_ROWADDR		(0x38)
+#define NFI_STRDATA		(0x40)
+#define		STAR_EN			(1)
+#define		STAR_DE			(0)
+#define NFI_CNRNB		(0x44)
+#define NFI_DATAW		(0x50)
+#define NFI_DATAR		(0x54)
+#define NFI_PIO_DIRDY		(0x58)
+#define		PIO_DI_RDY		(0x01)
+#define NFI_STA			(0x60)
+#define		STA_CMD			BIT(0)
+#define		STA_ADDR		BIT(1)
+#define		STA_BUSY		BIT(8)
+#define		STA_EMP_PAGE		BIT(12)
+#define		NFI_FSM_CUSTDATA	(0xe << 16)
+#define		NFI_FSM_MASK		(0xf << 16)
+#define NFI_ADDRCNTR		(0x70)
+#define		CNTR_MASK		GENMASK(16, 12)
+#define NFI_STRADDR		(0x80)
+#define NFI_BYTELEN		(0x84)
+#define NFI_CSEL		(0x90)
+#define NFI_FDML(x)		(0xA0 + (x) * sizeof(u32) * 2)
+#define NFI_FDMM(x)		(0xA4 + (x) * sizeof(u32) * 2)
+#define NFI_FDM_MAX_SIZE	(8)
+#define NFI_MASTER_STA		(0x224)
+#define		MASTER_STA_MASK		(0x0FFF)
+#define NFI_EMPTY_THRESH	(0x23C)
+
+#define MTK_NAME		"mtk-nand"
+#define KB(x)			((x) * 1024UL)
+#define MB(x)			(KB(x) * 1024UL)
+
+#define MTK_TIMEOUT		(500000)
+#define MTK_RESET_TIMEOUT	(1000000)
+#define MTK_MAX_SECTOR		(16)
+#define MTK_NAND_MAX_NSELS	(2)
+
+typedef void (*bad_mark_swap)(struct mtd_info *, uint8_t *buf, int raw);
+struct mtk_nfc_bad_mark_ctl {
+	bad_mark_swap bm_swap;
+	u32 sec;
+	u32 pos;
+};
+
+/*
+ * FDM: region used to store free OOB data
+ */
+struct mtk_nfc_fdm {
+	u32 reg_size;
+	u32 ecc_size;
+};
+
+struct mtk_nfc_nand_chip {
+	struct list_head node;
+	struct nand_chip nand;
+
+	struct mtk_nfc_bad_mark_ctl bad_mark;
+	struct mtk_nfc_fdm fdm;
+	u32 spare_per_sector;
+
+	int nsels;
+	u8 sels[0];
+	/* nothing after this field */
+};
+
+struct mtk_nfc_clk {
+	struct clk *nfi_clk;
+	struct clk *pad_clk;
+};
+
+struct mtk_nfc {
+	struct nand_hw_control controller;
+	struct mtk_ecc_config ecc_cfg;
+	struct mtk_nfc_clk clk;
+	struct mtk_ecc *ecc;
+
+	struct device *dev;
+	void __iomem *regs;
+
+	struct completion done;
+	struct list_head chips;
+
+	u8 *buffer;
+};
+
+static inline struct mtk_nfc_nand_chip *to_mtk_nand(struct nand_chip *nand)
+{
+	return container_of(nand, struct mtk_nfc_nand_chip, nand);
+}
+
+static inline uint8_t *data_ptr(struct nand_chip *chip, const uint8_t *p, int i)
+{
+	return (uint8_t *) p + i * chip->ecc.size;
+}
+
+static inline uint8_t *oob_ptr(struct nand_chip *chip, int i)
+{
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	uint8_t *poi;
+
+	if (i < mtk_nand->bad_mark.sec)
+		poi = chip->oob_poi + (i + 1) * mtk_nand->fdm.reg_size;
+	else if (i == mtk_nand->bad_mark.sec)
+		poi = chip->oob_poi;
+	else
+		poi = chip->oob_poi + i * mtk_nand->fdm.reg_size;
+
+	return poi;
+}
+
+static inline int mtk_data_len(struct nand_chip *chip)
+{
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+
+	return chip->ecc.size + mtk_nand->spare_per_sector;
+}
+
+static inline uint8_t *mtk_data_ptr(struct nand_chip *chip,  int i)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+
+	return nfc->buffer + i * mtk_data_len(chip);
+}
+
+static inline uint8_t *mtk_oob_ptr(struct nand_chip *chip, int i)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+
+	return nfc->buffer + i * mtk_data_len(chip) + chip->ecc.size;
+}
+
+static inline void nfi_writel(struct mtk_nfc *nfc, u32 val, u32 reg)
+{
+	writel(val, nfc->regs + reg);
+}
+
+static inline void nfi_writew(struct mtk_nfc *nfc, u16 val, u32 reg)
+{
+	writew(val, nfc->regs + reg);
+}
+
+static inline void nfi_writeb(struct mtk_nfc *nfc, u8 val, u32 reg)
+{
+	writeb(val, nfc->regs + reg);
+}
+
+static inline u32 nfi_readl(struct mtk_nfc *nfc, u32 reg)
+{
+	return readl_relaxed(nfc->regs + reg);
+}
+
+static inline u16 nfi_readw(struct mtk_nfc *nfc, u32 reg)
+{
+	return readw_relaxed(nfc->regs + reg);
+}
+
+static inline u8 nfi_readb(struct mtk_nfc *nfc, u32 reg)
+{
+	return readb_relaxed(nfc->regs + reg);
+}
+
+static void mtk_nfc_hw_reset(struct mtk_nfc *nfc)
+{
+	struct device *dev = nfc->dev;
+	u32 val;
+	int ret;
+
+	/* reset all registers and force the NFI master to terminate */
+	nfi_writel(nfc, CON_FIFO_FLUSH | CON_NFI_RST, NFI_CON);
+
+	/* wait for the master to finish the last transaction */
+	ret = readl_poll_timeout(nfc->regs + NFI_MASTER_STA, val,
+			!(val & MASTER_STA_MASK), 50, MTK_RESET_TIMEOUT);
+	if (ret)
+		dev_warn(dev, "master active in reset [0x%x] = 0x%x\n",
+			NFI_MASTER_STA, val);
+
+	/* ensure any status register affected by the NFI master is reset */
+	nfi_writel(nfc, CON_FIFO_FLUSH | CON_NFI_RST, NFI_CON);
+	nfi_writew(nfc, STAR_DE, NFI_STRDATA);
+}
+
+static int mtk_nfc_send_command(struct mtk_nfc *nfc, u8 command)
+{
+	struct device *dev = nfc->dev;
+	u32 val;
+	int ret;
+
+	nfi_writel(nfc, command, NFI_CMD);
+
+	ret = readl_poll_timeout_atomic(nfc->regs + NFI_STA, val,
+					!(val & STA_CMD), 10,  MTK_TIMEOUT);
+	if (ret) {
+		dev_warn(dev, "nfi core timed out entering command mode\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int mtk_nfc_send_address(struct mtk_nfc *nfc, int addr)
+{
+	struct device *dev = nfc->dev;
+	u32 val;
+	int ret;
+
+	nfi_writel(nfc, addr, NFI_COLADDR);
+	nfi_writel(nfc, 0, NFI_ROWADDR);
+	nfi_writew(nfc, 1, NFI_ADDRNOB);
+
+	ret = readl_poll_timeout_atomic(nfc->regs + NFI_STA, val,
+					!(val & STA_ADDR), 10, MTK_TIMEOUT);
+	if (ret) {
+		dev_warn(dev, "nfi core timed out entering address mode\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	u32 fmt, spare;
+
+	if (!mtd->writesize)
+		return 0;
+
+	spare = mtk_nand->spare_per_sector;
+
+	switch (mtd->writesize) {
+	case 512:
+		fmt = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
+		break;
+	case KB(2):
+		if (chip->ecc.size == 512)
+			fmt = PAGEFMT_2K_4K | PAGEFMT_SEC_SEL_512;
+		else
+			fmt = PAGEFMT_512_2K;
+		break;
+	case KB(4):
+		if (chip->ecc.size == 512)
+			fmt = PAGEFMT_4K_8K | PAGEFMT_SEC_SEL_512;
+		else
+			fmt = PAGEFMT_2K_4K;
+		break;
+	case KB(8):
+		if (chip->ecc.size == 512)
+			fmt = PAGEFMT_8K_16K | PAGEFMT_SEC_SEL_512;
+		else
+			fmt = PAGEFMT_4K_8K;
+		break;
+	case KB(16):
+		fmt = PAGEFMT_8K_16K;
+		break;
+	default:
+		dev_err(nfc->dev, "invalid page len: %d\n", mtd->writesize);
+		return -EINVAL;
+	}
+
+	/* the hardware doubles the value for this eccsize so let's halve it */
+	if (chip->ecc.size == 1024)
+		spare >>= 1;
+
+	switch (spare) {
+	case 16:
+		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 26:
+		fmt |= (PAGEFMT_SPARE_26 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 27:
+		fmt |= (PAGEFMT_SPARE_27 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 28:
+		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 32:
+		fmt |= (PAGEFMT_SPARE_32 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 36:
+		fmt |= (PAGEFMT_SPARE_36 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 40:
+		fmt |= (PAGEFMT_SPARE_40 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 44:
+		fmt |= (PAGEFMT_SPARE_44 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 48:
+		fmt |= (PAGEFMT_SPARE_48 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 49:
+		fmt |= (PAGEFMT_SPARE_49 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 50:
+		fmt |= (PAGEFMT_SPARE_50 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 51:
+		fmt |= (PAGEFMT_SPARE_51 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 52:
+		fmt |= (PAGEFMT_SPARE_52 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 62:
+		fmt |= (PAGEFMT_SPARE_62 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 63:
+		fmt |= (PAGEFMT_SPARE_63 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 64:
+		fmt |= (PAGEFMT_SPARE_64 << PAGEFMT_SPARE_SHIFT);
+		break;
+	default:
+		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
+		return -EINVAL;
+	}
+
+	fmt |= mtk_nand->fdm.reg_size << PAGEFMT_FDM_SHIFT;
+	fmt |= mtk_nand->fdm.ecc_size << PAGEFMT_FDM_ECC_SHIFT;
+	nfi_writew(nfc, fmt, NFI_PAGEFMT);
+
+	nfc->ecc_cfg.strength = chip->ecc.strength;
+	nfc->ecc_cfg.enc_len = chip->ecc.size + mtk_nand->fdm.ecc_size;
+	nfc->ecc_cfg.dec_len = (nfc->ecc_cfg.enc_len << 3)
+				+ chip->ecc.strength * ECC_PARITY_BITS;
+
+	return 0;
+}
+
+static void mtk_nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(nand);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(nand);
+
+	if (chip < 0)
+		return;
+
+	mtk_nfc_hw_runtime_config(mtd);
+
+	nfi_writel(nfc, mtk_nand->sels[chip], NFI_CSEL);
+}
+
+static int mtk_nfc_dev_ready(struct mtd_info *mtd)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
+
+	if (nfi_readl(nfc, NFI_STA) & STA_BUSY)
+		return 0;
+
+	return 1;
+}
+
+static void mtk_nfc_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
+
+	if (ctrl & NAND_ALE)
+		mtk_nfc_send_address(nfc, dat);
+	else if (ctrl & NAND_CLE) {
+		mtk_nfc_hw_reset(nfc);
+
+		nfi_writew(nfc, CNFG_OP_CUST, NFI_CNFG);
+		mtk_nfc_send_command(nfc, dat);
+	}
+}
+
+static inline void mtk_nfc_wait_ioready(struct mtk_nfc *nfc)
+{
+	int rc;
+	u8 val;
+
+	rc = readb_poll_timeout_atomic(nfc->regs + NFI_PIO_DIRDY, val,
+					val & PIO_DI_RDY, 10, MTK_TIMEOUT);
+	if (rc < 0)
+		dev_err(nfc->dev, "data not ready\n");
+}
+
+static inline uint8_t mtk_nfc_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	u32 reg;
+
+	reg = nfi_readl(nfc, NFI_STA) & NFI_FSM_MASK;
+	if (reg != NFI_FSM_CUSTDATA) {
+		reg = nfi_readw(nfc, NFI_CNFG);
+		reg |= CNFG_BYTE_RW | CNFG_READ_EN;
+		nfi_writew(nfc, reg, NFI_CNFG);
+
+		reg = (MTK_MAX_SECTOR << CON_SEC_SHIFT) | CON_BRD;
+		nfi_writel(nfc, reg, NFI_CON);
+
+		/* trigger to fetch data */
+		nfi_writew(nfc, STAR_EN, NFI_STRDATA);
+	}
+
+	mtk_nfc_wait_ioready(nfc);
+
+	return nfi_readb(nfc, NFI_DATAR);
+}
+
+static void mtk_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		buf[i] = mtk_nfc_read_byte(mtd);
+}
+
+static void mtk_nfc_write_byte(struct mtd_info *mtd, uint8_t byte)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
+	u32 reg;
+
+	reg = nfi_readl(nfc, NFI_STA) & NFI_FSM_MASK;
+
+	if (reg != NFI_FSM_CUSTDATA) {
+		reg = nfi_readw(nfc, NFI_CNFG) | CNFG_BYTE_RW;
+		nfi_writew(nfc, reg, NFI_CNFG);
+
+		reg = MTK_MAX_SECTOR << CON_SEC_SHIFT | CON_BWR;
+		nfi_writel(nfc, reg, NFI_CON);
+
+		nfi_writew(nfc, STAR_EN, NFI_STRDATA);
+	}
+
+	mtk_nfc_wait_ioready(nfc);
+	nfi_writeb(nfc, byte, NFI_DATAW);
+}
+
+static void mtk_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		mtk_nfc_write_byte(mtd, buf[i]);
+}
+
+static int mtk_nfc_sector_encode(struct nand_chip *chip, u8 *data)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	int size = chip->ecc.size + mtk_nand->fdm.reg_size;
+
+	nfc->ecc_cfg.ecc_mode = ECC_DMA_MODE;
+	nfc->ecc_cfg.codec = ECC_ENC;
+	return mtk_ecc_encode_non_nfi_mode(nfc->ecc, &nfc->ecc_cfg, data, size);
+}
+
+static void mtk_nfc_no_bad_mark_swap(struct mtd_info *a, uint8_t *b, int c)
+{
+	/* nope */
+}
+
+static void mtk_nfc_bad_mark_swap(struct mtd_info *mtd, uint8_t *buf, int raw)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *nand = to_mtk_nand(chip);
+	u32 bad_pos = nand->bad_mark.pos;
+
+	if (raw)
+		bad_pos += nand->bad_mark.sec * mtk_data_len(chip);
+	else
+		bad_pos += nand->bad_mark.sec * chip->ecc.size;
+
+	swap(chip->oob_poi[0], buf[bad_pos]);
+}
+
+static int mtk_nfc_format_subpage(struct mtd_info *mtd, uint32_t offset,
+			uint32_t len, const uint8_t *buf)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_fdm *fdm = &mtk_nand->fdm;
+	u32 start, end;
+	int i, ret;
+
+	start = offset / chip->ecc.size;
+	end = DIV_ROUND_UP(offset + len, chip->ecc.size);
+
+	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
+	for (i = 0; i < chip->ecc.steps; i++) {
+
+		memcpy(mtk_data_ptr(chip, i), data_ptr(chip, buf, i),
+			chip->ecc.size);
+
+		if (start > i || i >= end)
+			continue;
+
+		if (i == mtk_nand->bad_mark.sec)
+			mtk_nand->bad_mark.bm_swap(mtd, nfc->buffer, 1);
+
+		memcpy(mtk_oob_ptr(chip, i), oob_ptr(chip, i), fdm->reg_size);
+
+		/* program the CRC back to the OOB */
+		ret = mtk_nfc_sector_encode(chip, mtk_data_ptr(chip, i));
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void mtk_nfc_format_page(struct mtd_info *mtd, const uint8_t *buf)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_fdm *fdm = &mtk_nand->fdm;
+	u32 i;
+
+	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (buf)
+			memcpy(mtk_data_ptr(chip, i), data_ptr(chip, buf, i),
+				chip->ecc.size);
+
+		if (i == mtk_nand->bad_mark.sec)
+			mtk_nand->bad_mark.bm_swap(mtd, nfc->buffer, 1);
+
+		memcpy(mtk_oob_ptr(chip, i), oob_ptr(chip, i), fdm->reg_size);
+	}
+}
+
+static inline void mtk_nfc_read_fdm(struct nand_chip *chip, u32 start,
+					u32 sectors)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	u32 *p;
+	int i;
+
+	for (i = 0; i < sectors; i++) {
+		p = (u32 *) oob_ptr(chip, start + i);
+		p[0] = nfi_readl(nfc, NFI_FDML(i));
+		p[1] = nfi_readl(nfc, NFI_FDMM(i));
+	}
+}
+
+static inline void mtk_nfc_write_fdm(struct nand_chip *chip)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	u32 *p;
+	int i;
+
+	for (i = 0; i < chip->ecc.steps ; i++) {
+		p = (u32 *) oob_ptr(chip, i);
+		nfi_writel(nfc, p[0], NFI_FDML(i));
+		nfi_writel(nfc, p[1], NFI_FDMM(i));
+	}
+}
+
+static int mtk_nfc_do_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+				const uint8_t *buf, int page, int len)
+{
+
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct device *dev = nfc->dev;
+	dma_addr_t addr;
+	u32 reg;
+	int ret;
+
+	addr = dma_map_single(dev, (void *) buf, len, DMA_TO_DEVICE);
+	ret = dma_mapping_error(nfc->dev, addr);
+	if (ret) {
+		dev_err(nfc->dev, "dma mapping error\n");
+		return -EINVAL;
+	}
+
+	reg = nfi_readw(nfc, NFI_CNFG) | CNFG_AHB | CNFG_DMA_BURST_EN;
+	nfi_writew(nfc, reg, NFI_CNFG);
+
+	nfi_writel(nfc, chip->ecc.steps << CON_SEC_SHIFT, NFI_CON);
+	nfi_writel(nfc, lower_32_bits(addr), NFI_STRADDR);
+	nfi_writew(nfc, INTR_AHB_DONE_EN, NFI_INTR_EN);
+
+	init_completion(&nfc->done);
+
+	reg = nfi_readl(nfc, NFI_CON) | CON_BWR;
+	nfi_writel(nfc, reg, NFI_CON);
+	nfi_writew(nfc, STAR_EN, NFI_STRDATA);
+
+	ret = wait_for_completion_timeout(&nfc->done, msecs_to_jiffies(500));
+	if (!ret) {
+		dev_err(dev, "program ahb done timeout\n");
+		nfi_writew(nfc, 0, NFI_INTR_EN);
+		ret = -ETIMEDOUT;
+		goto timeout;
+	}
+
+	ret = readl_poll_timeout_atomic(nfc->regs + NFI_ADDRCNTR, reg,
+			(reg & CNTR_MASK) >= chip->ecc.steps, 10, MTK_TIMEOUT);
+	if (ret)
+		dev_err(dev, "hwecc write timeout\n");
+
+timeout:
+
+	dma_unmap_single(nfc->dev, addr, len, DMA_TO_DEVICE);
+	nfi_writel(nfc, 0, NFI_CON);
+
+	return ret;
+}
+
+static int mtk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, int page, int raw)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	size_t len;
+	const u8 *bufpoi;
+	u32 reg;
+	int ret;
+
+	if (!raw) {
+		/* OOB => FDM: from register,  ECC: from HW */
+		reg = nfi_readw(nfc, NFI_CNFG) | CNFG_AUTO_FMT_EN;
+		nfi_writew(nfc, reg | CNFG_HW_ECC_EN, NFI_CNFG);
+
+		nfc->ecc_cfg.codec = ECC_ENC;
+		nfc->ecc_cfg.ecc_mode = ECC_NFI_MODE;
+		ret = mtk_ecc_enable(nfc->ecc, &nfc->ecc_cfg);
+		if (ret) {
+			/* clear NFI config */
+			reg = nfi_readw(nfc, NFI_CNFG);
+			reg &= ~(CNFG_AUTO_FMT_EN | CNFG_HW_ECC_EN);
+			nfi_writew(nfc, reg, NFI_CNFG);
+
+			return ret;
+		}
+
+		memcpy(nfc->buffer, buf, mtd->writesize);
+		mtk_nand->bad_mark.bm_swap(mtd, nfc->buffer, raw);
+		bufpoi = nfc->buffer;
+
+		/* write OOB into the FDM registers (OOB area in MTK NAND) */
+		mtk_nfc_write_fdm(chip);
+	} else
+		bufpoi = buf;
+
+	len = mtd->writesize + (raw ? mtd->oobsize : 0);
+	ret = mtk_nfc_do_write_page(mtd, chip, bufpoi, page, len);
+
+	if (!raw)
+		mtk_ecc_disable(nfc->ecc, &nfc->ecc_cfg);
+
+	return ret;
+}
+
+static int mtk_nfc_write_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, const uint8_t *buf, int oob_on, int page)
+{
+	return mtk_nfc_write_page(mtd, chip, buf, page, 0);
+}
+
+static int mtk_nfc_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+				const uint8_t *buf, int oob_on, int pg)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+
+	mtk_nfc_format_page(mtd, buf);
+	return mtk_nfc_write_page(mtd, chip, nfc->buffer, pg, 1);
+}
+
+static int mtk_nfc_write_subpage_hwecc(struct mtd_info *mtd,
+		struct nand_chip *chip, uint32_t offset, uint32_t data_len,
+		const uint8_t *buf, int oob_on, int page)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	int ret;
+
+	ret = mtk_nfc_format_subpage(mtd, offset, data_len, buf);
+	if (ret < 0)
+		return ret;
+
+	/* use the data in the private buffer (now with FDM and CRC) */
+	return mtk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
+}
+
+static int mtk_nfc_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
+					int page)
+{
+	int ret;
+
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+
+	ret = mtk_nfc_write_page_raw(mtd, chip, NULL, 1, page);
+	if (ret < 0)
+		return -EIO;
+
+	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+	ret = chip->waitfunc(mtd, chip);
+
+	return ret & NAND_STATUS_FAIL ? -EIO : 0;
+}
+
+static int mtk_nfc_update_ecc_stats(struct mtd_info *mtd, u8 *buf, u32 sectors)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_ecc_stats stats;
+	int rc, i;
+
+	rc = nfi_readl(nfc, NFI_STA) & STA_EMP_PAGE;
+	if (rc) {
+		memset(buf, 0xff, sectors * chip->ecc.size);
+		for (i = 0; i < sectors; i++)
+			memset(oob_ptr(chip, i), 0xff, mtk_nand->fdm.reg_size);
+		return 0;
+	}
+
+	mtk_ecc_get_stats(nfc->ecc, &stats, sectors);
+	mtd->ecc_stats.corrected += stats.corrected;
+	mtd->ecc_stats.failed += stats.failed;
+
+	return stats.bitflips;
+}
+
+static int mtk_nfc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
+		uint32_t data_offs, uint32_t readlen, uint8_t *bufpoi,
+		int page, int raw)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	u32 spare = mtk_nand->spare_per_sector;
+	u32 column, sectors, start, end, reg;
+	dma_addr_t addr;
+	int bitflips;
+	size_t len;
+	u8 *buf;
+	int rc;
+
+	start = data_offs / chip->ecc.size;
+	end = DIV_ROUND_UP(data_offs + readlen, chip->ecc.size);
+
+	sectors = end - start;
+	column = start * (chip->ecc.size + spare);
+
+	len = sectors * chip->ecc.size + (raw ? sectors * spare : 0);
+	buf = bufpoi + start * chip->ecc.size;
+
+	if (column != 0)
+		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, column, -1);
+
+	addr = dma_map_single(nfc->dev, buf, len, DMA_FROM_DEVICE);
+	rc = dma_mapping_error(nfc->dev, addr);
+	if (rc) {
+		dev_err(nfc->dev, "dma mapping error\n");
+
+		return -EINVAL;
+	}
+
+	reg = nfi_readw(nfc, NFI_CNFG);
+	reg |= CNFG_READ_EN | CNFG_DMA_BURST_EN | CNFG_AHB;
+	if (!raw) {
+		reg |= CNFG_AUTO_FMT_EN | CNFG_HW_ECC_EN;
+		nfi_writew(nfc, reg, NFI_CNFG);
+
+		nfc->ecc_cfg.ecc_mode = ECC_NFI_MODE;
+		nfc->ecc_cfg.sec_mask = sectors;
+		nfc->ecc_cfg.codec = ECC_DEC;
+		rc = mtk_ecc_enable(nfc->ecc, &nfc->ecc_cfg);
+		if (rc) {
+			dev_err(nfc->dev, "ecc enable\n");
+			/* clear NFI_CNFG */
+			reg &= ~(CNFG_DMA_BURST_EN | CNFG_AHB | CNFG_READ_EN |
+				CNFG_AUTO_FMT_EN | CNFG_HW_ECC_EN);
+			nfi_writew(nfc, reg, NFI_CNFG);
+			dma_unmap_single(nfc->dev, addr, len, DMA_FROM_DEVICE);
+
+			return rc;
+		}
+	} else
+		nfi_writew(nfc, reg, NFI_CNFG);
+
+	nfi_writel(nfc, sectors << CON_SEC_SHIFT, NFI_CON);
+	nfi_writew(nfc, INTR_AHB_DONE_EN, NFI_INTR_EN);
+	nfi_writel(nfc, lower_32_bits(addr), NFI_STRADDR);
+
+	init_completion(&nfc->done);
+	reg = nfi_readl(nfc, NFI_CON) | CON_BRD;
+	nfi_writel(nfc, reg, NFI_CON);
+	nfi_writew(nfc, STAR_EN, NFI_STRDATA);
+
+	rc = wait_for_completion_timeout(&nfc->done, msecs_to_jiffies(500));
+	if (!rc)
+		dev_warn(nfc->dev, "read ahb/dma done timeout\n");
+
+	rc = readl_poll_timeout_atomic(nfc->regs + NFI_BYTELEN, reg,
+				(reg & CNTR_MASK) >= sectors, 10, MTK_TIMEOUT);
+	if (rc < 0) {
+		dev_err(nfc->dev, "subpage done timeout\n");
+		bitflips = -EIO;
+	} else {
+		bitflips = 0;
+		if (!raw) {
+			rc = mtk_ecc_wait_irq_done(nfc->ecc, ECC_DEC);
+			bitflips = rc < 0 ? -ETIMEDOUT :
+				mtk_nfc_update_ecc_stats(mtd, buf, sectors);
+			mtk_nfc_read_fdm(chip, start, sectors);
+		}
+	}
+
+	dma_unmap_single(nfc->dev, addr, len, DMA_FROM_DEVICE);
+
+	if (raw)
+		goto done;
+
+	mtk_ecc_disable(nfc->ecc, &nfc->ecc_cfg);
+
+	if (clamp(mtk_nand->bad_mark.sec, start, end) == mtk_nand->bad_mark.sec)
+		mtk_nand->bad_mark.bm_swap(mtd, bufpoi, raw);
+done:
+	nfi_writel(nfc, 0, NFI_CON);
+
+	return bitflips;
+}
+
+static int mtk_nfc_read_subpage_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, uint32_t off, uint32_t len, uint8_t *p, int pg)
+{
+	return mtk_nfc_read_subpage(mtd, chip, off, len, p, pg, 0);
+}
+
+static int mtk_nfc_read_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *p, int oob_on, int pg)
+{
+	return mtk_nfc_read_subpage(mtd, chip, 0, mtd->writesize, p, pg, 0);
+}
+
+static int mtk_nfc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+				uint8_t *buf, int oob_on, int page)
+{
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	struct mtk_nfc_fdm *fdm = &mtk_nand->fdm;
+	int i, ret;
+
+	memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
+	ret = mtk_nfc_read_subpage(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), mtk_oob_ptr(chip, i), fdm->reg_size);
+		if (i == mtk_nand->bad_mark.sec)
+			mtk_nand->bad_mark.bm_swap(mtd, nfc->buffer, 1);
+
+		if (buf)
+			memcpy(data_ptr(chip, buf, i), mtk_data_ptr(chip, i),
+				chip->ecc.size);
+	}
+
+	return ret;
+}
+
+static int mtk_nfc_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
+				int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+
+	return mtk_nfc_read_page_raw(mtd, chip, NULL, 1, page);
+}
+
+static inline void mtk_nfc_hw_init(struct mtk_nfc *nfc)
+{
+	nfi_writel(nfc, 0x10804211, NFI_ACCCON);
+	nfi_writew(nfc, 0xf1, NFI_CNRNB);
+	nfi_writew(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
+
+	mtk_nfc_hw_reset(nfc);
+
+	nfi_readl(nfc, NFI_INTR_STA);
+	nfi_writel(nfc, 0, NFI_INTR_EN);
+}
+
+static irqreturn_t mtk_nfc_irq(int irq, void *id)
+{
+	struct mtk_nfc *nfc = id;
+	u16 sta, ien;
+
+	sta = nfi_readw(nfc, NFI_INTR_STA);
+	ien = nfi_readw(nfc, NFI_INTR_EN);
+
+	if (!(sta & ien))
+		return IRQ_NONE;
+
+	nfi_writew(nfc, ~sta & ien, NFI_INTR_EN);
+	complete(&nfc->done);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_nfc_enable_clk(struct device *dev, struct mtk_nfc_clk *clk)
+{
+	int ret;
+
+	ret = clk_prepare_enable(clk->nfi_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable nfi clk\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(clk->pad_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable pad clk\n");
+		clk_disable_unprepare(clk->nfi_clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mtk_nfc_disable_clk(struct mtk_nfc_clk *clk)
+{
+	clk_disable_unprepare(clk->nfi_clk);
+	clk_disable_unprepare(clk->pad_clk);
+}
+
+static int mtk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
+				struct mtd_oob_region *oob_region)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	struct mtk_nfc_fdm *fdm = &mtk_nand->fdm;
+	u32 eccsteps;
+
+	eccsteps = mtd->writesize / chip->ecc.size;
+
+	if (section >= eccsteps)
+		return -ERANGE;
+
+	oob_region->length = fdm->reg_size - fdm->ecc_size;
+	oob_region->offset = section * fdm->reg_size + fdm->ecc_size;
+
+	return 0;
+}
+
+static int mtk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
+				struct mtd_oob_region *oob_region)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
+	u32 eccsteps;
+
+	if (section)
+		return -ERANGE;
+
+	eccsteps = mtd->writesize / chip->ecc.size;
+	oob_region->offset = mtk_nand->fdm.reg_size * eccsteps;
+	oob_region->length = mtd->oobsize - oob_region->offset;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops mtk_nfc_ooblayout_ops = {
+	.free = mtk_nfc_ooblayout_free,
+	.ecc = mtk_nfc_ooblayout_ecc,
+};
+
+static void mtk_nfc_set_fdm(struct mtk_nfc_fdm *fdm, struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct mtk_nfc_nand_chip *chip = to_mtk_nand(nand);
+	u32 ecc_bytes;
+
+	ecc_bytes = DIV_ROUND_UP(nand->ecc.strength * ECC_PARITY_BITS, 8);
+
+	fdm->reg_size = chip->spare_per_sector - ecc_bytes;
+	if (fdm->reg_size > NFI_FDM_MAX_SIZE)
+		fdm->reg_size = NFI_FDM_MAX_SIZE;
+
+	/* bad block mark storage */
+	fdm->ecc_size = 1;
+}
+
+static void mtk_nfc_set_bad_mark_ctl(struct mtk_nfc_bad_mark_ctl *bm_ctl,
+				struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+
+	if (mtd->writesize == 512)
+		bm_ctl->bm_swap = mtk_nfc_no_bad_mark_swap;
+	else {
+		bm_ctl->bm_swap = mtk_nfc_bad_mark_swap;
+		bm_ctl->sec = mtd->writesize / mtk_data_len(nand);
+		bm_ctl->pos = mtd->writesize % mtk_data_len(nand);
+	}
+}
+
+static void mtk_nfc_set_spare_per_sector(u32 *sps, struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	u32 spare[] = {16, 26, 27, 28, 32, 36, 40, 44,
+			48, 49, 50, 51, 52, 62, 63, 64};
+	u32 eccsteps, i;
+
+	eccsteps = mtd->writesize / nand->ecc.size;
+	*sps = mtd->oobsize / eccsteps;
+
+	if (nand->ecc.size == 1024)
+		*sps >>= 1;
+
+	for (i = 0; i < ARRAY_SIZE(spare); i++) {
+		if (*sps <= spare[i]) {
+			if (!i)
+				*sps = spare[i];
+			else if (*sps != spare[i])
+				*sps = spare[i - 1];
+			break;
+		}
+	}
+
+	if (i >= ARRAY_SIZE(spare))
+		*sps = spare[ARRAY_SIZE(spare) - 1];
+
+	if (nand->ecc.size == 1024)
+		*sps <<= 1;
+}
+
+static int mtk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	u32 spare;
+
+	/* support only ecc hw mode */
+	if (nand->ecc.mode != NAND_ECC_HW) {
+		dev_err(dev, "ecc.mode not supported\n");
+		return -EINVAL;
+	}
+
+	/* if optional DT settings are not present */
+	if (!nand->ecc.size || !nand->ecc.strength) {
+
+		/* controller only supports sizes 512 and 1024 */
+		nand->ecc.size = (mtd->writesize > 512) ? 1024 : 512;
+
+		/* get controller valid values */
+		mtk_nfc_set_spare_per_sector(&spare, mtd);
+		spare = spare - NFI_FDM_MAX_SIZE;
+		nand->ecc.strength = (spare << 3) / ECC_PARITY_BITS;
+	}
+
+	mtk_ecc_update_strength(&nand->ecc.strength);
+
+	dev_info(dev, "eccsize %d eccstrength %d\n",
+		nand->ecc.size, nand->ecc.strength);
+
+	return 0;
+}
+
+static int mtk_nfc_nand_chip_init(struct device *dev, struct mtk_nfc *nfc,
+				struct device_node *np)
+{
+	struct mtk_nfc_nand_chip *chip;
+	struct nand_chip *nand;
+	struct mtd_info *mtd;
+	int nsels, len;
+	u32 tmp;
+	int ret;
+	int i;
+
+	if (!of_get_property(np, "reg", &nsels))
+		return -ENODEV;
+
+	nsels /= sizeof(u32);
+	if (!nsels || nsels > MTK_NAND_MAX_NSELS) {
+		dev_err(dev, "invalid reg property size %d\n", nsels);
+		return -EINVAL;
+	}
+
+	chip = devm_kzalloc(dev,
+			sizeof(*chip) + nsels * sizeof(u8), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->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;
+		}
+		chip->sels[i] = tmp;
+	}
+
+	nand = &chip->nand;
+	nand->controller = &nfc->controller;
+
+	nand_set_flash_node(nand, np);
+	nand_set_controller_data(nand, nfc);
+
+	nand->options |= NAND_USE_BOUNCE_BUFFER | NAND_SUBPAGE_READ;
+	nand->dev_ready = mtk_nfc_dev_ready;
+	nand->select_chip = mtk_nfc_select_chip;
+	nand->write_byte = mtk_nfc_write_byte;
+	nand->write_buf = mtk_nfc_write_buf;
+	nand->read_byte = mtk_nfc_read_byte;
+	nand->read_buf = mtk_nfc_read_buf;
+	nand->cmd_ctrl = mtk_nfc_cmd_ctrl;
+
+	/* set default mode in case dt entry is missing */
+	nand->ecc.mode = NAND_ECC_HW;
+
+	nand->ecc.write_subpage = mtk_nfc_write_subpage_hwecc;
+	nand->ecc.write_page_raw = mtk_nfc_write_page_raw;
+	nand->ecc.write_page = mtk_nfc_write_page_hwecc;
+	nand->ecc.write_oob_raw = mtk_nfc_write_oob_std;
+	nand->ecc.write_oob = mtk_nfc_write_oob_std;
+
+	nand->ecc.read_subpage = mtk_nfc_read_subpage_hwecc;
+	nand->ecc.read_page_raw = mtk_nfc_read_page_raw;
+	nand->ecc.read_page = mtk_nfc_read_page_hwecc;
+	nand->ecc.read_oob_raw = mtk_nfc_read_oob_std;
+	nand->ecc.read_oob = mtk_nfc_read_oob_std;
+
+	mtd = nand_to_mtd(nand);
+	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = dev;
+	mtd->name = MTK_NAME;
+	mtd_set_ooblayout(mtd, &mtk_nfc_ooblayout_ops);
+
+	mtk_nfc_hw_init(nfc);
+
+	ret = nand_scan_ident(mtd, nsels, NULL);
+	if (ret)
+		return -ENODEV;
+
+	/* store bbt magic in page, cause OOB is not protected */
+	if (nand->bbt_options & NAND_BBT_USE_FLASH)
+		nand->bbt_options |= NAND_BBT_NO_OOB;
+
+	ret = mtk_nfc_ecc_init(dev, mtd);
+	if (ret)
+		return -EINVAL;
+
+	mtk_nfc_set_spare_per_sector(&chip->spare_per_sector, mtd);
+	mtk_nfc_set_fdm(&chip->fdm, mtd);
+	mtk_nfc_set_bad_mark_ctl(&chip->bad_mark, mtd);
+
+	len = mtd->writesize + mtd->oobsize;
+	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL);
+	if (!nfc->buffer)
+		return  -ENOMEM;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		return -ENODEV;
+
+	ret = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
+	if (ret) {
+		dev_err(dev, "mtd parse partition error\n");
+		nand_release(mtd);
+		return ret;
+	}
+
+	list_add_tail(&chip->node, &nfc->chips);
+
+	return 0;
+}
+
+static int mtk_nfc_nand_chips_init(struct device *dev, struct mtk_nfc *nfc)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *nand_np;
+	int ret;
+
+	for_each_child_of_node(np, nand_np) {
+		ret = mtk_nfc_nand_chip_init(dev, nfc, nand_np);
+		if (ret) {
+			of_node_put(nand_np);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int mtk_nfc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct mtk_nfc *nfc;
+	struct resource *res;
+	int ret, irq;
+
+	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	spin_lock_init(&nfc->controller.lock);
+	init_waitqueue_head(&nfc->controller.wq);
+	INIT_LIST_HEAD(&nfc->chips);
+
+	/* probe defer if not ready */
+	nfc->ecc = of_mtk_ecc_get(np);
+	if (IS_ERR(nfc->ecc))
+		return PTR_ERR(nfc->ecc);
+	else if (!nfc->ecc)
+		return -ENODEV;
+
+	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);
+		dev_err(dev, "no nfi base\n");
+		goto release_ecc;
+	}
+
+	nfc->clk.nfi_clk = devm_clk_get(dev, "nfi_clk");
+	if (IS_ERR(nfc->clk.nfi_clk)) {
+		dev_err(dev, "no clk\n");
+		ret = PTR_ERR(nfc->clk.nfi_clk);
+		goto release_ecc;
+	}
+
+	nfc->clk.pad_clk = devm_clk_get(dev, "pad_clk");
+	if (IS_ERR(nfc->clk.pad_clk)) {
+		dev_err(dev, "no pad clk\n");
+		ret = PTR_ERR(nfc->clk.pad_clk);
+		goto release_ecc;
+	}
+
+	ret = mtk_nfc_enable_clk(dev, &nfc->clk);
+	if (ret)
+		goto release_ecc;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no nfi irq resource\n");
+		ret = -EINVAL;
+		goto clk_disable;
+	}
+
+	ret = devm_request_irq(dev, irq, mtk_nfc_irq, 0x0, "mtk-nand", nfc);
+	if (ret) {
+		dev_err(dev, "failed to request nfi irq\n");
+		goto clk_disable;
+	}
+
+	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(dev, "failed to set dma mask\n");
+		goto clk_disable;
+	}
+
+	platform_set_drvdata(pdev, nfc);
+
+	ret = mtk_nfc_nand_chips_init(dev, nfc);
+	if (ret) {
+		dev_err(dev, "failed to init nand chips\n");
+		goto clk_disable;
+	}
+
+	return 0;
+
+clk_disable:
+	mtk_nfc_disable_clk(&nfc->clk);
+
+release_ecc:
+	mtk_ecc_release(nfc->ecc);
+
+	return ret;
+}
+
+static int mtk_nfc_remove(struct platform_device *pdev)
+{
+	struct mtk_nfc *nfc = platform_get_drvdata(pdev);
+	struct mtk_nfc_nand_chip *chip;
+
+	while (!list_empty(&nfc->chips)) {
+		chip = list_first_entry(&nfc->chips, struct mtk_nfc_nand_chip,
+					node);
+		nand_release(nand_to_mtd(&chip->nand));
+		list_del(&chip->node);
+	}
+
+	mtk_ecc_release(nfc->ecc);
+	mtk_nfc_disable_clk(&nfc->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_nfc_suspend(struct device *dev)
+{
+	struct mtk_nfc *nfc = dev_get_drvdata(dev);
+
+	mtk_nfc_disable_clk(&nfc->clk);
+
+	return 0;
+}
+
+static int mtk_nfc_resume(struct device *dev)
+{
+	struct mtk_nfc *nfc = dev_get_drvdata(dev);
+	struct mtk_nfc_nand_chip *chip;
+	struct nand_chip *nand;
+	struct mtd_info *mtd;
+	int ret;
+	u32 i;
+
+	udelay(200);
+
+	ret = mtk_nfc_enable_clk(dev, &nfc->clk);
+	if (ret)
+		return ret;
+
+	mtk_nfc_hw_init(nfc);
+
+	list_for_each_entry(chip, &nfc->chips, node) {
+		nand = &chip->nand;
+		mtd = nand_to_mtd(nand);
+		for (i = 0; i < chip->nsels; i++) {
+			nand->select_chip(mtd, i);
+			nand->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+		}
+	}
+
+	return 0;
+}
+static SIMPLE_DEV_PM_OPS(mtk_nfc_pm_ops, mtk_nfc_suspend, mtk_nfc_resume);
+#endif
+
+static const struct of_device_id mtk_nfc_id_table[] = {
+	{ .compatible = "mediatek,mt2701-nfc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_nfc_id_table);
+
+static struct platform_driver mtk_nfc_driver = {
+	.probe  = mtk_nfc_probe,
+	.remove = mtk_nfc_remove,
+	.driver = {
+		.name  = MTK_NAME,
+		.of_match_table = mtk_nfc_id_table,
+#ifdef CONFIG_PM_SLEEP
+		.pm = &mtk_nfc_pm_ops,
+#endif
+	},
+};
+
+module_platform_driver(mtk_nfc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Xiaolei Li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
+MODULE_AUTHOR("Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
+MODULE_DESCRIPTION("MTK Nand Flash Controller Driver");
-- 
2.7.4

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[John Crispin]

Hi,

i tried V4 just now. result is as before

if i set the ecc-strength above 16 it will give irq errors during boot
as shown below. values of 16 and below will probe without error but
hexdump on the device node will then throw ecc timeouts.

[    1.964296] nand: device found, Manufacturer ID: 0xef, Chip ID: 0xda
[    1.970603] nand: Unknown W29N02GV
[    1.974038] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048,
OOB size: 64
[    1.981554] mtk-nand 1100d000.nfi: eccsize 1024 eccstrength 24
[    2.481827] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    2.988919] mtk-ecc 1100e000.ecc: decoder NOT idle
[    3.491818] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    3.998907] mtk-ecc 1100e000.ecc: decoder NOT idle
[    4.501819] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    4.733511] mt7623-gsw switch@1b100000: port 3 link up
[    5.008907] mtk-ecc 1100e000.ecc: decoder NOT idle
[    5.511817] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    6.018907] mtk-ecc 1100e000.ecc: decoder NOT idle
[    6.023671] Bad block table not found for chip 0
[    6.521817] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    7.028907] mtk-ecc 1100e000.ecc: decoder NOT idle
[    7.531816] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    8.038907] mtk-ecc 1100e000.ecc: decoder NOT idle
[    8.541817] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[    9.048901] mtk-ecc 1100e000.ecc: decoder NOT idle
[    9.551816] mtk-ecc 1100e000.ecc: decoder timeout - interrupt did not
arrive)
[   10.058906] mtk-ecc 1100e000.ecc: decoder NOT idle
[   10.063671] Bad block table not found for chip 0
[   10.068247] Scanning device for bad blocks
[   10.072769] Bad eraseblock 1 at 0x000000020000
[   10.077384] Bad eraseblock 2 at 0x000000040000
[   10.082017] Bad eraseblock 3 at 0x000000060000
[   10.086631] Bad eraseblock 4 at 0x000000080000

when i see errors during probe, the core will have received 2 ecc irqs

 25:       2058          0          0          0  MT_SYSIRQ  56 Level
 mtk-nand
 26:          2          0          0          0  MT_SYSIRQ  55 Level
 mtk-ecc

to me it is ambigous what FDM actually is. it seems that if FDM is known
the ecc-strength can be calculated and the amount of dts properties can
be reduced even further.

what i dont understand just yet is, do these erros i am seeing mean the
ecc core is not brought up properly or do they come from the dts
properties being bad.

	John

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[John Crispin]

Hi,

> Your ecc-strength setting is wrong, because your nand device's OOB size
> is not enough to store 24 bit ecc parity data.
> For this nand, its ecc-strength is max 12 bit per 1024B for our
> controller.
> You can get the setting as mtk-nand DT Documentation description.

i tried 12 aswell and it also gives an error. the problem with the
documentation is that FDM is unknown to me.


> Because the ecc-size and ecc-strength settings are optional, so if you
> are not sure how to set them, you can remove the setting from DT file.

tried that just now, the driver defaults to 12 and problem remains


> In addition, can I have a check whether you have run valid nand driver
> on your platform before? Can you do a full device erase operation at
> first, then use this V4 for your test?

the SDK kernel works fine on the board using nand as a primary boot
source. i am confident that this is not a board level issue.

how would i go about doing a full device reset operation ?

	John

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/02/2016 02:13 AM, John Crispin wrote:
> Hi,
>
>> Your ecc-strength setting is wrong, because your nand device's OOB size
>> is not enough to store 24 bit ecc parity data.
>> For this nand, its ecc-strength is max 12 bit per 1024B for our
>> controller.
>> You can get the setting as mtk-nand DT Documentation description.
> i tried 12 aswell and it also gives an error. the problem with the
> documentation is that FDM is unknown to me.
>
>
>> Because the ecc-size and ecc-strength settings are optional, so if you
>> are not sure how to set them, you can remove the setting from DT file.
> tried that just now, the driver defaults to 12 and problem remains

can you send the boot log letting the driver use its own calculated values?
ie, just don't specify any of the controller optional values in the 
device tree and let the driver calculate them for you.

Also instead of a read operation, can you try the oobtest?
you will have to insmod or modprobe "mtd_oobtest dev="

I don't have the spec for the MT7623 but maybe also worth checking the 
configuration for the NFI_ACCON settings (timing control register).

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[John Crispin]

On 02/05/2016 13:38, Jorge Ramirez wrote:
> 
> 
> On 05/02/2016 02:13 AM, John Crispin wrote:
>> Hi,
>>
>>> Your ecc-strength setting is wrong, because your nand device's OOB size
>>> is not enough to store 24 bit ecc parity data.
>>> For this nand, its ecc-strength is max 12 bit per 1024B for our
>>> controller.
>>> You can get the setting as mtk-nand DT Documentation description.
>> i tried 12 aswell and it also gives an error. the problem with the
>> documentation is that FDM is unknown to me.
>>
>>
>>> Because the ecc-size and ecc-strength settings are optional, so if you
>>> are not sure how to set them, you can remove the setting from DT file.
>> tried that just now, the driver defaults to 12 and problem remains
> 
> can you send the boot log letting the driver use its own calculated values?
> ie, just don't specify any of the controller optional values in the
> device tree and let the driver calculate them for you.
> 
> Also instead of a read operation, can you try the oobtest?
> you will have to insmod or modprobe "mtd_oobtest dev="
> 
> I don't have the spec for the MT7623 but maybe also worth checking the
> configuration for the NFI_ACCON settings (timing control register).

Hi,

i changed the 2 timing registers to those present in the atags for this
specific chip. i also verified them with the ones used by uboot.
unfortunately that did not fix the problem

+       nfi_writel(nfc, 0x30c77fff, NFI_ACCCON);
+       nfi_writel(nfc, 0xC03222, NFI_ACCCON1);

the bootlog without any properties in the devicetree is

[    1.914168] nand: device found, Manufacturer ID: 0xef, Chip ID: 0xda
[    1.920476] nand: Unknown W29N02GV
[    1.923909] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048,
OOB size: 64
[    1.931424] mtk-nand 1100d000.nfi: eccsize 1024 eccstrength 12
[    1.937237] Scanning device for bad blocks
[    1.943547] Bad eraseblock 1 at 0x000000020000
[    1.949066] Bad eraseblock 2 at 0x000000040000
[    1.954614] Bad eraseblock 3 at 0x000000060000
[    1.960128] Bad eraseblock 4 at 0x000000080000
[    1.966766] Bad eraseblock 6 at 0x0000000c0000
[    1.975604] Bad eraseblock 10 at 0x000000140000
[    1.981204] Bad eraseblock 11 at 0x000000160000
[    1.986819] Bad eraseblock 12 at 0x000000180000
[    1.992422] Bad eraseblock 13 at 0x0000001a0000
[    1.998022] Bad eraseblock 14 at 0x0000001c0000
[    2.003638] Bad eraseblock 15 at 0x0000001e0000
[    2.009237] Bad eraseblock 16 at 0x000000200000
[    2.014852] Bad eraseblock 17 at 0x000000220000
[    2.020452] Bad eraseblock 18 at 0x000000240000
[    2.026066] Bad eraseblock 19 at 0x000000260000
[    2.031666] Bad eraseblock 20 at 0x000000280000
[    2.037283] Bad eraseblock 21 at 0x0000002a0000
[    2.042897] Bad eraseblock 22 at 0x0000002c0000
[    2.048497] Bad eraseblock 23 at 0x0000002e0000
[    2.054113] Bad eraseblock 24 at 0x000000300000
[    2.059713] Bad eraseblock 25 at 0x000000320000
[    2.065324] Bad eraseblock 26 at 0x000000340000
[    2.070920] Bad eraseblock 27 at 0x000000360000
[    2.076582] Bad eraseblock 28 at 0x000000380000
[    2.082188] Bad eraseblock 29 at 0x0000003a0000
[    2.087788] Bad eraseblock 30 at 0x0000003c0000
[    2.093406] Bad eraseblock 31 at 0x0000003e0000
[    2.099007] Bad eraseblock 32 at 0x000000400000
[    2.104623] Bad eraseblock 33 at 0x000000420000
[    2.110223] Bad eraseblock 34 at 0x000000440000
[    2.115838] Bad eraseblock 35 at 0x000000460000
[    2.121438] Bad eraseblock 36 at 0x000000480000
[    2.127053] Bad eraseblock 37 at 0x0000004a0000
[    2.132657] Bad eraseblock 38 at 0x0000004c0000
[    2.138257] Bad eraseblock 39 at 0x0000004e0000
[    2.143872] Bad eraseblock 40 at 0x000000500000
[    2.149472] Bad eraseblock 41 at 0x000000520000
[    2.155088] Bad eraseblock 42 at 0x000000540000
[    2.160688] Bad eraseblock 43 at 0x000000560000
[    2.166302] Bad eraseblock 44 at 0x000000580000
[    2.171906] Bad eraseblock 45 at 0x0000005a0000
[    2.177505] Bad eraseblock 46 at 0x0000005c0000
[    2.183127] Bad eraseblock 47 at 0x0000005e0000
[    2.188726] Bad eraseblock 48 at 0x000000600000
[    2.194341] Bad eraseblock 49 at 0x000000620000
[    2.199941] Bad eraseblock 50 at 0x000000640000
[    2.205556] Bad eraseblock 51 at 0x000000660000
[    2.211155] Bad eraseblock 52 at 0x000000680000
[    2.216770] Bad eraseblock 53 at 0x0000006a0000
[    2.222373] Bad eraseblock 54 at 0x0000006c0000
[    2.227972] Bad eraseblock 55 at 0x0000006e0000
[    2.233588] Bad eraseblock 56 at 0x000000700000
[    2.239188] Bad eraseblock 57 at 0x000000720000
[    2.244803] Bad eraseblock 58 at 0x000000740000
[    2.250402] Bad eraseblock 59 at 0x000000760000
[    2.256017] Bad eraseblock 60 at 0x000000780000
[    2.261616] Bad eraseblock 61 at 0x0000007a0000
[    2.267235] Bad eraseblock 62 at 0x0000007c0000
[    2.272840] Bad eraseblock 63 at 0x0000007e0000
[    2.278439] Bad eraseblock 64 at 0x000000800000
[    2.284054] Bad eraseblock 65 at 0x000000820000
[    2.289653] Bad eraseblock 66 at 0x000000840000
[    2.295268] Bad eraseblock 67 at 0x000000860000
[    2.300867] Bad eraseblock 68 at 0x000000880000
[    2.306482] Bad eraseblock 69 at 0x0000008a0000
[    2.312086] Bad eraseblock 70 at 0x0000008c0000
[    2.317685] Bad eraseblock 71 at 0x0000008e0000
[    2.323301] Bad eraseblock 72 at 0x000000900000
[    2.328901] Bad eraseblock 73 at 0x000000920000
[    2.334516] Bad eraseblock 74 at 0x000000940000
[    2.340115] Bad eraseblock 75 at 0x000000960000
[    2.345731] Bad eraseblock 76 at 0x000000980000
[    2.351330] Bad eraseblock 77 at 0x0000009a0000
[    2.356945] Bad eraseblock 78 at 0x0000009c0000
[    2.362548] Bad eraseblock 79 at 0x0000009e0000
[    2.368147] Bad eraseblock 80 at 0x000000a00000
[    2.373763] Bad eraseblock 81 at 0x000000a20000
[    2.379362] Bad eraseblock 82 at 0x000000a40000
[    2.384977] Bad eraseblock 83 at 0x000000a60000
[    2.390576] Bad eraseblock 84 at 0x000000a80000
[    2.396192] Bad eraseblock 85 at 0x000000aa0000
[    2.401807] Bad eraseblock 86 at 0x000000ac0000
[    2.407406] Bad eraseblock 87 at 0x000000ae0000
[    3.859664] mt7623-gsw switch@1b100000: port 3 link up
[    4.452341] Bad eraseblock 1927 at 0x00000f0e0000
[    4.590029] 5 ofpart partitions found on MTD device mtk-nand
[    4.595671] Creating 5 MTD partitions on "mtk-nand":
[    4.600599] 0x0000000c0000-0x000000100000 : "uboot-env"
[    4.606846] 0x000000100000-0x000000140000 : "factory"
[    4.612807] 0x000000140000-0x000002140000 : "kernel"
[    4.618843] 0x000002140000-0x000004140000 : "recovery"
[    4.625035] 0x000004140000-0x000005140000 : "rootfs"


and oobtest gives the following output

[  167.892722]
[  167.894212] =================================================
[  167.899909] mtd_oobtest: MTD device: 2
[  167.903675] mtd_oobtest: MTD device size 33554432, eraseblock size
131072, page size 2048, count of eraseblocks 256, pages per eraseblock
64, OOB size 64
[  167.917340] mtd_test: scanning for bad eraseblocks
[  167.922123] mtd_test: block 0 is bad
[  167.925668] mtd_test: block 1 is bad
[  167.929210] mtd_test: block 2 is bad
[  167.932775] mtd_test: block 3 is bad
[  167.936346] mtd_test: block 4 is bad
[  167.939888] mtd_test: block 5 is bad
[  167.943451] mtd_test: block 6 is bad
[  167.946995] mtd_test: block 7 is bad
[  167.950537] mtd_test: block 8 is bad
[  167.954100] mtd_test: block 9 is bad
[  167.957643] mtd_test: block 10 is bad
[  167.961271] mtd_test: block 11 is bad
[  167.964917] mtd_test: block 12 is bad
[  167.968547] mtd_test: block 13 is bad
[  167.972191] mtd_test: block 14 is bad
[  167.975821] mtd_test: block 15 is bad
[  167.979449] mtd_test: block 16 is bad
[  167.983094] mtd_test: block 17 is bad
[  167.986724] mtd_test: block 18 is bad
[  167.990352] mtd_test: block 19 is bad
[  167.993997] mtd_test: block 20 is bad
[  167.997626] mtd_test: block 21 is bad
[  168.001254] mtd_test: block 22 is bad
[  168.004899] mtd_test: block 23 is bad
[  168.008529] mtd_test: block 24 is bad
[  168.012171] mtd_test: block 25 is bad
[  168.015800] mtd_test: block 26 is bad
[  168.019429] mtd_test: block 27 is bad
[  168.023080] mtd_test: block 28 is bad
[  168.026713] mtd_test: block 29 is bad
[  168.030342] mtd_test: block 30 is bad
[  168.033987] mtd_test: block 31 is bad
[  168.037617] mtd_test: block 32 is bad
[  168.041245] mtd_test: block 33 is bad
[  168.044886] mtd_test: block 34 is bad
[  168.048516] mtd_test: block 35 is bad
[  168.052161] mtd_test: block 36 is bad
[  168.055790] mtd_test: block 37 is bad
[  168.059419] mtd_test: block 38 is bad
[  168.063064] mtd_test: block 39 is bad
[  168.066692] mtd_test: block 40 is bad
[  168.070321] mtd_test: block 41 is bad
[  168.074015] mtd_test: block 42 is bad
[  168.077648] mtd_test: block 43 is bad
[  168.081276] mtd_test: block 44 is bad
[  168.084927] mtd_test: block 45 is bad
[  168.088556] mtd_test: block 46 is bad
[  168.092200] mtd_test: block 47 is bad
[  168.095829] mtd_test: block 48 is bad
[  168.099458] mtd_test: block 49 is bad
[  168.103103] mtd_test: block 50 is bad
[  168.106732] mtd_test: block 51 is bad
[  168.110360] mtd_test: block 52 is bad
[  168.114005] mtd_test: block 53 is bad
[  168.117635] mtd_test: block 54 is bad
[  168.121263] mtd_test: block 55 is bad
[  168.124907] mtd_test: block 56 is bad
[  168.128536] mtd_test: block 57 is bad
[  168.132179] mtd_test: block 58 is bad
[  168.135808] mtd_test: block 59 is bad
[  168.139437] mtd_test: block 60 is bad
[  168.143113] mtd_test: block 61 is bad
[  168.146743] mtd_test: block 62 is bad
[  168.150372] mtd_test: block 63 is bad
[  168.154020] mtd_test: block 64 is bad
[  168.157649] mtd_test: block 65 is bad
[  168.161278] mtd_test: block 66 is bad
[  168.164924] mtd_test: block 67 is bad
[  168.168553] mtd_test: block 68 is bad
[  168.172197] mtd_test: block 69 is bad
[  168.175826] mtd_test: block 70 is bad
[  168.179455] mtd_test: block 71 is bad
[  168.183100] mtd_test: block 72 is bad
[  168.186729] mtd_test: block 73 is bad
[  168.190357] mtd_test: block 74 is bad
[  168.194002] mtd_test: block 75 is bad
[  168.197632] mtd_test: block 76 is bad
[  168.201260] mtd_test: block 77 is bad
[  168.205077] mtd_test: scanned 256 eraseblocks, 78 are bad
[  168.210428] mtd_oobtest: test 1 of 5
[  168.714011] mtk-nand 1100d000.nfi: data not ready
[  168.718679] mtd_test: error -5 while erasing EB 78
[  168.723489] mtd_oobtest: error -5 occurred
[  168.727548] =================================================
failed to insert /mtd_oobtest.ko

thanks for your help !
	John

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon]

Jorge, Xiaolei,

On Fri, 29 Apr 2016 12:17:22 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> This patch adds support for mediatek's SDG1 NFC nand controller
> embedded in SoC 2701
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/mtd/nand/Kconfig    |    7 +
>  drivers/mtd/nand/Makefile   |    1 +
>  drivers/mtd/nand/mtk_ecc.c  |  527 ++++++++++++++++
>  drivers/mtd/nand/mtk_ecc.h  |   53 ++
>  drivers/mtd/nand/mtk_nand.c | 1432 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 2020 insertions(+)
>  create mode 100644 drivers/mtd/nand/mtk_ecc.c
>  create mode 100644 drivers/mtd/nand/mtk_ecc.h
>  create mode 100644 drivers/mtd/nand/mtk_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f05e0e9..3c26e89 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -563,4 +563,11 @@ config MTD_NAND_QCOM
>  	  Enables support for NAND flash chips on SoCs containing the EBI2 NAND
>  	  controller. This controller is found on IPQ806x SoC.
>  
> +config MTD_NAND_MTK
> +	tristate "Support for NAND controller on MTK SoCs"
> +	depends on HAS_DMA
> +	help
> +	  Enables support for NAND controller on MTK SoCs.
> +	  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index f553353..cafde6f 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -57,5 +57,6 @@ obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
>  obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
>  obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
> +obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_nand.o mtk_ecc.o
>  
>  nand-objs := nand_base.o nand_bbt.o nand_timings.o
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> new file mode 100644
> index 0000000..28769f1
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -0,0 +1,527 @@
> +/*
> + * MTK ECC controller driver.
> + * Copyright (C) 2016  MediaTek Inc.
> + * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/iopoll.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/semaphore.h>
> +
> +#include "mtk_ecc.h"
> +
> +#define ECC_ENCCON		(0x00)
> +#define		ENC_EN			(1)
> +#define		ENC_DE			(0)
> +#define ECC_ENCCNFG		(0x04)
> +#define		ECC_CNFG_4BIT		(0)
> +#define		ECC_CNFG_6BIT		(1)
> +#define		ECC_CNFG_8BIT		(2)
> +#define		ECC_CNFG_10BIT		(3)
> +#define		ECC_CNFG_12BIT		(4)
> +#define		ECC_CNFG_14BIT		(5)
> +#define		ECC_CNFG_16BIT		(6)
> +#define		ECC_CNFG_18BIT		(7)
> +#define		ECC_CNFG_20BIT		(8)
> +#define		ECC_CNFG_22BIT		(9)
> +#define		ECC_CNFG_24BIT		(0xa)
> +#define		ECC_CNFG_28BIT		(0xb)
> +#define		ECC_CNFG_32BIT		(0xc)
> +#define		ECC_CNFG_36BIT		(0xd)
> +#define		ECC_CNFG_40BIT		(0xe)
> +#define		ECC_CNFG_44BIT		(0xf)
> +#define		ECC_CNFG_48BIT		(0x10)
> +#define		ECC_CNFG_52BIT		(0x11)
> +#define		ECC_CNFG_56BIT		(0x12)
> +#define		ECC_CNFG_60BIT		(0x13)
> +#define		ECC_MODE_SHIFT		(5)
> +#define		ECC_MS_SHIFT		(16)
> +#define ECC_ENCDIADDR		(0x08)
> +#define ECC_ENCIDLE		(0x0C)
> +#define		ENC_IDLE		BIT(0)
> +#define ECC_ENCPAR(x)		(0x10 + (x) * sizeof(u32))
> +#define ECC_ENCIRQ_EN		(0x80)
> +#define		ENC_IRQEN		BIT(0)
> +#define ECC_ENCIRQ_STA		(0x84)
> +#define ECC_DECCON		(0x100)
> +#define		DEC_EN			(1)
> +#define		DEC_DE			(0)
> +#define ECC_DECCNFG		(0x104)
> +#define		DEC_EMPTY_EN		BIT(31)
> +#define		DEC_CNFG_CORRECT	(0x3 << 12)
> +#define ECC_DECIDLE		(0x10C)
> +#define		DEC_IDLE		BIT(0)
> +#define ECC_DECENUM0		(0x114)
> +#define		ERR_MASK		(0x3f)
> +#define ECC_DECDONE		(0x124)
> +#define ECC_DECIRQ_EN		(0x200)
> +#define		DEC_IRQEN		BIT(0)
> +#define ECC_DECIRQ_STA		(0x204)
> +
> +#define ECC_TIMEOUT		(500000)
> +
> +#define ECC_IDLE_REG(x)		((x) == ECC_ENC ? ECC_ENCIDLE : ECC_DECIDLE)
> +#define ECC_IDLE_MASK(x)	((x) == ECC_ENC ? ENC_IDLE : DEC_IDLE)

No need for this macro, it's always bit0, so just define an ECC_IDLE
macro and use it for both decoder and encoder.

There seems to be some kind of pattern in your ENC/DEC registers.
ENC registers start at 0 and DEC ones at 0x100.
CNF register is always at 0x4 + mode/dir_offset (ie 0x100 for DEC and
0x0 for ENC), ...
Maybe you should define common macros for those registers, and choose
the base offset depending on the mode you're operating in (encoding or
decoding).

> +#define ECC_IRQ_REG(x)		((x) == ECC_ENC ? ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
> +#define ECC_IRQ_EN(x)		((x) == ECC_ENC ? ENC_IRQEN : DEC_IRQEN)
> +#define ECC_CTL_REG(x)		((x) == ECC_ENC ? ECC_ENCCON : ECC_DECCON)
> +#define ECC_CODEC_ENABLE(x)	((x) == ECC_ENC ? ENC_EN : DEC_EN)
> +#define ECC_CODEC_DISABLE(x)	((x) == ECC_ENC ? ENC_DE : DEC_DE)
> +
> +struct mtk_ecc {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct clk *clk;
> +
> +	struct completion done;
> +	struct semaphore sem;

You tried to explain me why you decided to go for a semaphore instead of
a mutex, but I don't remember. Could you explain it again?
If that's all about being interruptible, then you can use
mutex_lock_interruptible.

> +	u32 sec_mask;
> +};
> +
> +static inline void mtk_ecc_codec_wait_idle(struct mtk_ecc *ecc,
> +					enum mtk_ecc_codec codec)

Just nitpicking on the name again, but enum mtk_ecc_codec does not
describe what this enum really encode: the codec mode or direction.

How about renaming this enum into mtk_ecc_codec_dir or
mtd_ecc_codec_mode?

> +{
> +	struct device *dev = ecc->dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(ecc->regs + ECC_IDLE_REG(codec), val,
> +					val & ECC_IDLE_MASK(codec),
> +					10, ECC_TIMEOUT);
> +	if (ret)
> +		dev_warn(dev, "%s NOT idle\n",
> +			codec == ECC_ENC ? "encoder" : "decoder");
> +}
> +
> +static irqreturn_t mtk_ecc_irq(int irq, void *id)
> +{
> +	struct mtk_ecc *ecc = id;
> +	enum mtk_ecc_codec codec;
> +	u32 dec, enc;
> +
> +	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
> +	if (dec) {
> +		codec = ECC_DEC;
> +		dec = readw(ecc->regs + ECC_DECDONE);
> +		if (dec & ecc->sec_mask) {
> +			ecc->sec_mask = 0;
> +			complete(&ecc->done);
> +		} else
> +			return IRQ_HANDLED;
> +	} else {
> +		enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
> +		if (enc) {
> +			codec = ECC_ENC;
> +			complete(&ecc->done);
> +		} else
> +			return IRQ_NONE;
> +	}
> +
> +	writel(0, ecc->regs + ECC_IRQ_REG(codec));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
> +{
> +	u32 ecc_bit = ECC_CNFG_4BIT, dec_sz, enc_sz;
> +	u32 reg;
> +
> +	switch (config->strength) {
> +	case 4:
> +		ecc_bit = ECC_CNFG_4BIT;
> +		break;
> +	case 6:
> +		ecc_bit = ECC_CNFG_6BIT;
> +		break;
> +	case 8:
> +		ecc_bit = ECC_CNFG_8BIT;
> +		break;
> +	case 10:
> +		ecc_bit = ECC_CNFG_10BIT;
> +		break;
> +	case 12:
> +		ecc_bit = ECC_CNFG_12BIT;
> +		break;
> +	case 14:
> +		ecc_bit = ECC_CNFG_14BIT;
> +		break;
> +	case 16:
> +		ecc_bit = ECC_CNFG_16BIT;
> +		break;
> +	case 18:
> +		ecc_bit = ECC_CNFG_18BIT;
> +		break;
> +	case 20:
> +		ecc_bit = ECC_CNFG_20BIT;
> +		break;
> +	case 22:
> +		ecc_bit = ECC_CNFG_22BIT;
> +		break;
> +	case 24:
> +		ecc_bit = ECC_CNFG_24BIT;
> +		break;
> +	case 28:
> +		ecc_bit = ECC_CNFG_28BIT;
> +		break;
> +	case 32:
> +		ecc_bit = ECC_CNFG_32BIT;
> +		break;
> +	case 36:
> +		ecc_bit = ECC_CNFG_36BIT;
> +		break;
> +	case 40:
> +		ecc_bit = ECC_CNFG_40BIT;
> +		break;
> +	case 44:
> +		ecc_bit = ECC_CNFG_44BIT;
> +		break;
> +	case 48:
> +		ecc_bit = ECC_CNFG_48BIT;
> +		break;
> +	case 52:
> +		ecc_bit = ECC_CNFG_52BIT;
> +		break;
> +	case 56:
> +		ecc_bit = ECC_CNFG_56BIT;
> +		break;
> +	case 60:
> +		ecc_bit = ECC_CNFG_60BIT;
> +		break;
> +	default:
> +		dev_err(ecc->dev, "invalid strength %d\n", config->strength);
> +	}
> +
> +	if (config->codec == ECC_ENC) {
> +		/* configure ECC encoder (in bits) */
> +		enc_sz = config->enc_len << 3;
> +
> +		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> +		reg |= (enc_sz << ECC_MS_SHIFT);
> +		writel(reg, ecc->regs + ECC_ENCCNFG);
> +
> +		if (config->ecc_mode != ECC_NFI_MODE)
> +			writel(lower_32_bits(config->addr),
> +				ecc->regs + ECC_ENCDIADDR);
> +
> +	} else {
> +		/* configure ECC decoder (in bits) */
> +		dec_sz = config->dec_len;
> +
> +		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> +		reg |= (dec_sz << ECC_MS_SHIFT) | DEC_CNFG_CORRECT;
> +		reg |= DEC_EMPTY_EN;
> +		writel(reg, ecc->regs + ECC_DECCNFG);
> +
> +		if (config->sec_mask)
> +			ecc->sec_mask = 1 << (config->sec_mask - 1);
> +	}

I see that some of the logic could be shared between the ENC and DEC
cases.
BTW, why do you multiply enc_len by 8 (bits to byte conversion), but
don't do that for dec_len?

> +}
> +

[...]

> +
> +void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)

If you save the mode somewhere in mtk_ecc (or just write in both ENC
and DEC registers), you won't need this extra config arg here.

> +{
> +	enum mtk_ecc_codec codec = config->codec;
> +
> +	mtk_ecc_codec_wait_idle(ecc, codec);
> +	writew(0, ecc->regs + ECC_IRQ_REG(codec));
> +	writew(ECC_CODEC_DISABLE(codec), ecc->regs + ECC_CTL_REG(codec));
> +	up(&ecc->sem);
> +}
> +EXPORT_SYMBOL(mtk_ecc_disable);

[...]

> +
> +void mtk_ecc_hw_init(struct mtk_ecc *ecc)

No need to expose this function. Make it static and remove the
prototype in your header.

> +{
> +	mtk_ecc_codec_wait_idle(ecc, ECC_ENC);
> +	writew(ENC_DE, ecc->regs + ECC_ENCCON);
> +
> +	mtk_ecc_codec_wait_idle(ecc, ECC_DEC);
> +	writel(DEC_DE, ecc->regs + ECC_DECCON);
> +}
> +
> +void mtk_ecc_update_strength(u32 *p)

Please rename it into mtd_ecc_adjust_strength().

> +{
> +	u32 ecc[] = {4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 28, 32, 36,
> +			40, 44, 48, 52, 56, 60};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ecc); i++) {
> +		if (*p <= ecc[i]) {
> +			if (!i)
> +				*p = ecc[i];
> +			else if (*p != ecc[i])
> +				*p = ecc[i - 1];
> +			return;
> +		}
> +	}
> +
> +	*p = ecc[ARRAY_SIZE(ecc) - 1];
> +}

[...]

> diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
> new file mode 100644
> index 0000000..434826f
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_ecc.h
> @@ -0,0 +1,53 @@
> +/*
> + * MTK SDG1 ECC controller
> + *
> + * Copyright (c) 2016 Mediatek
> + * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef __DRIVERS_MTD_NAND_MTK_ECC_H__
> +#define __DRIVERS_MTD_NAND_MTK_ECC_H__
> +
> +#include <linux/types.h>
> +
> +#define ECC_PARITY_BITS		(14)
> +
> +enum mtk_ecc_mode {ECC_DMA_MODE = 0, ECC_NFI_MODE = 1};
> +enum mtk_ecc_codec {ECC_ENC, ECC_DEC};

As suggested, mtk_ecc_codec_dir or just mtk_ecc_dir would be more
appropriate IMHO.

> +
> +struct device_node;
> +struct mtk_ecc;
> +
> +struct mtk_ecc_stats {
> +	u32 corrected;
> +	u32 bitflips;
> +	u32 failed;
> +};
> +
> +struct mtk_ecc_config {
> +	enum mtk_ecc_mode ecc_mode;

s/ecc_mode/mode/ (the ecc_ prefix is implicit here).

> +	enum mtk_ecc_codec codec;
> +	dma_addr_t addr;
> +	u32 sec_mask;

It seems that sec_mask is actually encoding the number of sectors.
Maybe you should rename it sectors. BTW, why do you need both the
length and the number of sectors. It seems to me that given the length
an the sector size you could deduce the number of sectors, or
alternatively, you could specify the number of sectors and the sector
length and calculate the length from those fields.

BTW, I don't see where you configure the sector size. Don't you need it
to switch between 512bytes and 1k?

> +	u32 strength;
> +	u32 enc_len;
> +	u32 dec_len;

Why do you need 2 different fields for that? The ECC engine is either
working in encoding or decoding mode, so a single "len" field should be
enough.

Apparently you're mixing 2 different concepts: the information required
to configure the ECC engine (the fields in mtk_ecc_config), and how
they should be formatted to be written the the ECC engine registers
(that should probably be stored in the mtk_ecc struct or just
calculated on the fly by the driver).
You're driver should hide the internals of the ECC engine as much as
possible.

> +};
> +
> +int mtk_ecc_enable(struct mtk_ecc *, struct mtk_ecc_config *);
> +void mtk_ecc_disable(struct mtk_ecc *, struct mtk_ecc_config *);
> +int mtk_ecc_encode_non_nfi_mode(struct mtk_ecc *, struct mtk_ecc_config *,
> +				u8 *, u32);

mtk_ecc_encode() should be enough, since you dropped the other _encode()
functions.

> +void mtk_ecc_get_stats(struct mtk_ecc *, struct mtk_ecc_stats *, int);
> +int mtk_ecc_wait_irq_done(struct mtk_ecc *, enum mtk_ecc_codec);
> +void mtk_ecc_hw_init(struct mtk_ecc *);
> +void mtk_ecc_update_strength(u32 *);
> +
> +struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
> +void mtk_ecc_release(struct mtk_ecc *);
> +
> +#endif
> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> new file mode 100644
> index 0000000..907b90c
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_nand.c
> @@ -0,0 +1,1432 @@
> +/*
> + * MTK NAND Flash controller driver.
> + * Copyright (C) 2016 MediaTek Inc.
> + * Authors:	Xiaolei Li		<xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/clk.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/module.h>
> +#include <linux/iopoll.h>
> +#include <linux/of.h>
> +#include "mtk_ecc.h"
> +
> +/* NAND controller register definition */
> +#define NFI_CNFG		(0x00)
> +#define		CNFG_AHB		BIT(0)
> +#define		CNFG_READ_EN		BIT(1)
> +#define		CNFG_DMA_BURST_EN	BIT(2)
> +#define		CNFG_BYTE_RW		BIT(6)
> +#define		CNFG_HW_ECC_EN		BIT(8)
> +#define		CNFG_AUTO_FMT_EN	BIT(9)
> +#define		CNFG_OP_CUST		(6 << 12)
> +#define NFI_PAGEFMT		(0x04)
> +#define		PAGEFMT_FDM_ECC_SHIFT	(12)
> +#define		PAGEFMT_FDM_SHIFT	(8)
> +#define		PAGEFMT_SPARE_16	(0)
> +#define		PAGEFMT_SPARE_26	(1)
> +#define		PAGEFMT_SPARE_27	(2)
> +#define		PAGEFMT_SPARE_28	(3)
> +#define		PAGEFMT_SPARE_32	(4)
> +#define		PAGEFMT_SPARE_36	(5)
> +#define		PAGEFMT_SPARE_40	(6)
> +#define		PAGEFMT_SPARE_44	(7)
> +#define		PAGEFMT_SPARE_48	(8)
> +#define		PAGEFMT_SPARE_49	(9)
> +#define		PAGEFMT_SPARE_50	(0xa)
> +#define		PAGEFMT_SPARE_51	(0xb)
> +#define		PAGEFMT_SPARE_52	(0xc)
> +#define		PAGEFMT_SPARE_62	(0xd)
> +#define		PAGEFMT_SPARE_63	(0xe)
> +#define		PAGEFMT_SPARE_64	(0xf)
> +#define		PAGEFMT_SPARE_SHIFT	(4)
> +#define		PAGEFMT_SEC_SEL_512	BIT(2)
> +#define		PAGEFMT_512_2K		(0)
> +#define		PAGEFMT_2K_4K		(1)
> +#define		PAGEFMT_4K_8K		(2)
> +#define		PAGEFMT_8K_16K		(3)
> +/* NFI control */
> +#define NFI_CON			(0x08)
> +#define		CON_FIFO_FLUSH		BIT(0)
> +#define		CON_NFI_RST		BIT(1)
> +#define		CON_BRD			BIT(8)  /* burst  read */
> +#define		CON_BWR			BIT(9)	/* burst  write */
> +#define		CON_SEC_SHIFT		(12)
> +/* Timming control register */
> +#define NFI_ACCCON		(0x0C)
> +#define NFI_INTR_EN		(0x10)
> +#define		INTR_AHB_DONE_EN	BIT(6)
> +#define NFI_INTR_STA		(0x14)
> +#define NFI_CMD			(0x20)
> +#define NFI_ADDRNOB		(0x30)
> +#define NFI_COLADDR		(0x34)
> +#define NFI_ROWADDR		(0x38)
> +#define NFI_STRDATA		(0x40)
> +#define		STAR_EN			(1)
> +#define		STAR_DE			(0)
> +#define NFI_CNRNB		(0x44)
> +#define NFI_DATAW		(0x50)
> +#define NFI_DATAR		(0x54)
> +#define NFI_PIO_DIRDY		(0x58)
> +#define		PIO_DI_RDY		(0x01)
> +#define NFI_STA			(0x60)
> +#define		STA_CMD			BIT(0)
> +#define		STA_ADDR		BIT(1)
> +#define		STA_BUSY		BIT(8)
> +#define		STA_EMP_PAGE		BIT(12)
> +#define		NFI_FSM_CUSTDATA	(0xe << 16)
> +#define		NFI_FSM_MASK		(0xf << 16)
> +#define NFI_ADDRCNTR		(0x70)
> +#define		CNTR_MASK		GENMASK(16, 12)
> +#define NFI_STRADDR		(0x80)
> +#define NFI_BYTELEN		(0x84)
> +#define NFI_CSEL		(0x90)
> +#define NFI_FDML(x)		(0xA0 + (x) * sizeof(u32) * 2)
> +#define NFI_FDMM(x)		(0xA4 + (x) * sizeof(u32) * 2)
> +#define NFI_FDM_MAX_SIZE	(8)
> +#define NFI_MASTER_STA		(0x224)
> +#define		MASTER_STA_MASK		(0x0FFF)
> +#define NFI_EMPTY_THRESH	(0x23C)
> +
> +#define MTK_NAME		"mtk-nand"
> +#define KB(x)			((x) * 1024UL)
> +#define MB(x)			(KB(x) * 1024UL)
> +
> +#define MTK_TIMEOUT		(500000)
> +#define MTK_RESET_TIMEOUT	(1000000)
> +#define MTK_MAX_SECTOR		(16)
> +#define MTK_NAND_MAX_NSELS	(2)
> +
> +typedef void (*bad_mark_swap)(struct mtd_info *, uint8_t *buf, int raw);
> +struct mtk_nfc_bad_mark_ctl {
> +	bad_mark_swap bm_swap;

Just defined the function prototype directly here:

	void (*bm_swap)(struct mtd_info *, uint8_t *buf, int raw);

> +	u32 sec;
> +	u32 pos;
> +};

[...]

> +static inline uint8_t *oob_ptr(struct nand_chip *chip, int i)
> +{
> +	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
> +	uint8_t *poi;
> +
> +	if (i < mtk_nand->bad_mark.sec)
> +		poi = chip->oob_poi + (i + 1) * mtk_nand->fdm.reg_size;
> +	else if (i == mtk_nand->bad_mark.sec)
> +		poi = chip->oob_poi;
> +	else
> +		poi = chip->oob_poi + i * mtk_nand->fdm.reg_size;

Could you add some comments explaining what you're doing here, because
I don't get it :).

> +
> +	return poi;
> +}
> +

[...]

> +static int mtk_nfc_hw_runtime_config(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
> +	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 fmt, spare;
> +
> +	if (!mtd->writesize)
> +		return 0;
> +
> +	spare = mtk_nand->spare_per_sector;
> +
> +	switch (mtd->writesize) {
> +	case 512:
> +		fmt = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
> +		break;
> +	case KB(2):
> +		if (chip->ecc.size == 512)
> +			fmt = PAGEFMT_2K_4K | PAGEFMT_SEC_SEL_512;
> +		else
> +			fmt = PAGEFMT_512_2K;
> +		break;
> +	case KB(4):
> +		if (chip->ecc.size == 512)
> +			fmt = PAGEFMT_4K_8K | PAGEFMT_SEC_SEL_512;
> +		else
> +			fmt = PAGEFMT_2K_4K;
> +		break;
> +	case KB(8):
> +		if (chip->ecc.size == 512)
> +			fmt = PAGEFMT_8K_16K | PAGEFMT_SEC_SEL_512;
> +		else
> +			fmt = PAGEFMT_4K_8K;
> +		break;
> +	case KB(16):
> +		fmt = PAGEFMT_8K_16K;
> +		break;
> +	default:
> +		dev_err(nfc->dev, "invalid page len: %d\n", mtd->writesize);
> +		return -EINVAL;
> +	}
> +
> +	/* the hardware doubles the value for this eccsize so let's halve it */
> +	if (chip->ecc.size == 1024)
> +		spare >>= 1;
> +
> +	switch (spare) {
> +	case 16:
> +		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 26:
> +		fmt |= (PAGEFMT_SPARE_26 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 27:
> +		fmt |= (PAGEFMT_SPARE_27 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 28:
> +		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 32:
> +		fmt |= (PAGEFMT_SPARE_32 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 36:
> +		fmt |= (PAGEFMT_SPARE_36 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 40:
> +		fmt |= (PAGEFMT_SPARE_40 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 44:
> +		fmt |= (PAGEFMT_SPARE_44 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 48:
> +		fmt |= (PAGEFMT_SPARE_48 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 49:
> +		fmt |= (PAGEFMT_SPARE_49 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 50:
> +		fmt |= (PAGEFMT_SPARE_50 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 51:
> +		fmt |= (PAGEFMT_SPARE_51 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 52:
> +		fmt |= (PAGEFMT_SPARE_52 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 62:
> +		fmt |= (PAGEFMT_SPARE_62 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 63:
> +		fmt |= (PAGEFMT_SPARE_63 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 64:
> +		fmt |= (PAGEFMT_SPARE_64 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	default:
> +		dev_err(nfc->dev, "invalid spare per sector %d\n", spare);
> +		return -EINVAL;
> +	}
> +
> +	fmt |= mtk_nand->fdm.reg_size << PAGEFMT_FDM_SHIFT;
> +	fmt |= mtk_nand->fdm.ecc_size << PAGEFMT_FDM_ECC_SHIFT;
> +	nfi_writew(nfc, fmt, NFI_PAGEFMT);
> +
> +	nfc->ecc_cfg.strength = chip->ecc.strength;
> +	nfc->ecc_cfg.enc_len = chip->ecc.size + mtk_nand->fdm.ecc_size;
> +	nfc->ecc_cfg.dec_len = (nfc->ecc_cfg.enc_len << 3)
> +				+ chip->ecc.strength * ECC_PARITY_BITS;

And here is the explanation to the question I asked in my ECC engine
driver review :).

Please keep the len field consistent for both encoding and decoding
mode, and let the ECC engine driver adapt it to put the correct value
in the ECC engine registers.

> +
> +	return 0;
> +}
> +

[...]

> +
> +static inline uint8_t mtk_nfc_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 reg;
> +
> +	reg = nfi_readl(nfc, NFI_STA) & NFI_FSM_MASK;
> +	if (reg != NFI_FSM_CUSTDATA) {
> +		reg = nfi_readw(nfc, NFI_CNFG);
> +		reg |= CNFG_BYTE_RW | CNFG_READ_EN;
> +		nfi_writew(nfc, reg, NFI_CNFG);
> +
> +		reg = (MTK_MAX_SECTOR << CON_SEC_SHIFT) | CON_BRD;

What is CON_SEC field meaning in this case? The number of bytes to read
(maybe stored in a FIFO)? If that's the case, and the controller is able
to read several bytes at a time, then it would make more sense to
implement ->read_byte() as a wrapper around ->read_buf() and move this
code into ->read_buf().

> +		nfi_writel(nfc, reg, NFI_CON);
> +
> +		/* trigger to fetch data */
> +		nfi_writew(nfc, STAR_EN, NFI_STRDATA);
> +	}
> +
> +	mtk_nfc_wait_ioready(nfc);
> +
> +	return nfi_readb(nfc, NFI_DATAR);
> +}
> +

[...]

> +
> +static void mtk_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		mtk_nfc_write_byte(mtd, buf[i]);
> +}

Ditto.

> +
> +static int mtk_nfc_sector_encode(struct nand_chip *chip, u8 *data)
> +{
> +	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	struct mtk_nfc_nand_chip *mtk_nand = to_mtk_nand(chip);
> +	int size = chip->ecc.size + mtk_nand->fdm.reg_size;
> +
> +	nfc->ecc_cfg.ecc_mode = ECC_DMA_MODE;
> +	nfc->ecc_cfg.codec = ECC_ENC;
> +	return mtk_ecc_encode_non_nfi_mode(nfc->ecc, &nfc->ecc_cfg, data, size);
> +}
> +
> +static void mtk_nfc_no_bad_mark_swap(struct mtd_info *a, uint8_t *b, int c)
> +{
> +	/* nope */

I guess you meant NOP :).

> +}
> +
> +static void mtk_nfc_bad_mark_swap(struct mtd_info *mtd, uint8_t *buf, int raw)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct mtk_nfc_nand_chip *nand = to_mtk_nand(chip);
> +	u32 bad_pos = nand->bad_mark.pos;
> +
> +	if (raw)
> +		bad_pos += nand->bad_mark.sec * mtk_data_len(chip);
> +	else
> +		bad_pos += nand->bad_mark.sec * chip->ecc.size;
> +
> +	swap(chip->oob_poi[0], buf[bad_pos]);

Don't know if you support 16bits bus width, but if that's the case you
should swap 2 bytes (the bad block marker is 2 bytes wide).

> +}
> +

[...]

> +
> +static inline void mtk_nfc_read_fdm(struct nand_chip *chip, u32 start,
> +					u32 sectors)
> +{
> +	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 *p;
> +	int i;
> +
> +	for (i = 0; i < sectors; i++) {
> +		p = (u32 *) oob_ptr(chip, start + i);
> +		p[0] = nfi_readl(nfc, NFI_FDML(i));
> +		p[1] = nfi_readl(nfc, NFI_FDMM(i));

Hm, this should work as long as you're in little-endian, otherwise you
might have to switch bytes.
Maybe you should convert this manually with something like:

		u8 *oobptr = oob_ptr(chip, start + i);
		u32 val = nfi_readl(nfc, NFI_FDML(i));
		oobptr[0] = val;
		oobptr[1] = val >> 8;
		...

> +	}
> +}
> +
> +static inline void mtk_nfc_write_fdm(struct nand_chip *chip)
> +{
> +	struct mtk_nfc *nfc = nand_get_controller_data(chip);
> +	u32 *p;
> +	int i;
> +
> +	for (i = 0; i < chip->ecc.steps ; i++) {
> +		p = (u32 *) oob_ptr(chip, i);
> +		nfi_writel(nfc, p[0], NFI_FDML(i));
> +		nfi_writel(nfc, p[1], NFI_FDMM(i));

Ditto.

> +	}
> +}
> +

[...]

> +
> +static inline void mtk_nfc_hw_init(struct mtk_nfc *nfc)
> +{
> +	nfi_writel(nfc, 0x10804211, NFI_ACCCON);
> +	nfi_writew(nfc, 0xf1, NFI_CNRNB);

Could you explain those magic values?

> +	nfi_writew(nfc, PAGEFMT_8K_16K, NFI_PAGEFMT);
> +
> +	mtk_nfc_hw_reset(nfc);
> +
> +	nfi_readl(nfc, NFI_INTR_STA);
> +	nfi_writel(nfc, 0, NFI_INTR_EN);
> +}
> +

[...]

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_nfc_suspend(struct device *dev)
> +{
> +	struct mtk_nfc *nfc = dev_get_drvdata(dev);
> +
> +	mtk_nfc_disable_clk(&nfc->clk);
> +
> +	return 0;
> +}
> +
> +static int mtk_nfc_resume(struct device *dev)
> +{
> +	struct mtk_nfc *nfc = dev_get_drvdata(dev);
> +	struct mtk_nfc_nand_chip *chip;
> +	struct nand_chip *nand;
> +	struct mtd_info *mtd;
> +	int ret;
> +	u32 i;
> +
> +	udelay(200);
> +
> +	ret = mtk_nfc_enable_clk(dev, &nfc->clk);
> +	if (ret)
> +		return ret;
> +
> +	mtk_nfc_hw_init(nfc);
> +
> +	list_for_each_entry(chip, &nfc->chips, node) {
> +		nand = &chip->nand;
> +		mtd = nand_to_mtd(nand);
> +		for (i = 0; i < chip->nsels; i++) {
> +			nand->select_chip(mtd, i);
> +			nand->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +		}

Interesting, you need to reset the NAND chips when you get out of
suspend. Probably something we should move in the core at some point.

Sorry, but I focused on a few specific aspects in this review (ECC
engine and basic nand_chip functions), which means I might have missed
other issues. I'll try to carefully review the remaining parts later.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>> +#define ECC_IDLE_REG(x)		((x) == ECC_ENC ? ECC_ENCIDLE : ECC_DECIDLE)
>> >+#define ECC_IDLE_MASK(x)	((x) == ECC_ENC ? ENC_IDLE : DEC_IDLE)
> No need for this macro, it's always bit0, so just define an ECC_IDLE
> macro and use it for both decoder and encoder.

this was only done for consistency to help people reading the code (same 
for codec_enable, codec_disable).
I suppose I could remove macros and just write 0 and 1 to the registers 
if you prefer that.

>
> There seems to be some kind of pattern in your ENC/DEC registers.
> ENC registers start at 0 and DEC ones at 0x100.
> CNF register is always at 0x4 + mode/dir_offset (ie 0x100 for DEC and
> 0x0 for ENC), ...
> Maybe you should define common macros for those registers, and choose
> the base offset depending on the mode you're operating in (encoding or
> decoding).

Not sure if you are familiar with George Lakoff and his book "Don't 
Think Of An Elephant! Know Your Values And Frame The Debate" but the key 
message is not to engage in a discussion when you disagree with the 
terms used by your counterpart since you wont be able to frame the 
argument (the book is actually very interesting if politics and and the 
political debate is something that interest you)

I explicitly chose not to talk about modes, instead I chose the engine 
driver to talk about the codecs it controls; for me mode is a higher 
level concept that I didn't have a need for since in this case the mode 
is a 1-1 relationship to the codec. So when you tell me about the mode 
the engine is operating in I'd rather say the codec that the ecc engine 
is accessing. I hope it makes sense.

if you want to talk about modes instead of the encoders and decoders 
that is fine since you are the maintainer.
I can rewrite the relevant parts of the driver but I honestly see no value.

why did I wrote these macros? just for readability since they are simple 
conditionals.
So coming back to your second question, I not sure why I would use a 
base offset when I already have the map. I wouldn't.

>
>> >+#define ECC_IRQ_REG(x)		((x) == ECC_ENC ? ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
>> >+#define ECC_IRQ_EN(x)		((x) == ECC_ENC ? ENC_IRQEN : DEC_IRQEN)
>> >+#define ECC_CTL_REG(x)		((x) == ECC_ENC ? ECC_ENCCON : ECC_DECCON)
>> >+#define ECC_CODEC_ENABLE(x)	((x) == ECC_ENC ? ENC_EN : DEC_EN)
>> >+#define ECC_CODEC_DISABLE(x)	((x) == ECC_ENC ? ENC_DE : DEC_DE)

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon]

On Tue, 10 May 2016 10:37:32 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/10/2016 08:13 AM, Boris Brezillon wrote:
> >> +#define ECC_IDLE_REG(x)		((x) == ECC_ENC ? ECC_ENCIDLE : ECC_DECIDLE)  
> >> >+#define ECC_IDLE_MASK(x)	((x) == ECC_ENC ? ENC_IDLE : DEC_IDLE)  
> > No need for this macro, it's always bit0, so just define an ECC_IDLE
> > macro and use it for both decoder and encoder.  
> 
> this was only done for consistency to help people reading the code (same 
> for codec_enable, codec_disable).
> I suppose I could remove macros and just write 0 and 1 to the registers 
> if you prefer that.

No.

> 
> >
> > There seems to be some kind of pattern in your ENC/DEC registers.
> > ENC registers start at 0 and DEC ones at 0x100.
> > CNF register is always at 0x4 + mode/dir_offset (ie 0x100 for DEC and
> > 0x0 for ENC), ...
> > Maybe you should define common macros for those registers, and choose
> > the base offset depending on the mode you're operating in (encoding or
> > decoding).  
> 
> Not sure if you are familiar with George Lakoff and his book "Don't 
> Think Of An Elephant! Know Your Values And Frame The Debate" but the key 
> message is not to engage in a discussion when you disagree with the 
> terms used by your counterpart since you wont be able to frame the 
> argument (the book is actually very interesting if politics and and the 
> political debate is something that interest you)
> 
> I explicitly chose not to talk about modes, instead I chose the engine 
> driver to talk about the codecs it controls; for me mode is a higher 
> level concept that I didn't have a need for since in this case the mode 
> is a 1-1 relationship to the codec. So when you tell me about the mode 
> the engine is operating in I'd rather say the codec that the ecc engine 
> is accessing. I hope it makes sense.

I'm not arguing about the use of codec, but IMO codec is just
representing a device that is capable of encoding/decoding stuff, it
does not represent in which mode you want to use this device.

> 
> if you want to talk about modes instead of the encoders and decoders 
> that is fine since you are the maintainer.

No, the terms encoders/decoders are fine, I was just suggesting to
share some of the definitions between the encoder and decoder parts.

> I can rewrite the relevant parts of the driver but I honestly see no value.

No that's fine.

> 
> why did I wrote these macros? just for readability since they are simple 
> conditionals.

Again, I'm not arguing against the definition of helper macros, just
suggesting another way to do it to avoid register offset duplication.

> So coming back to your second question, I not sure why I would use a 
> base offset when I already have the map. I wouldn't.

Anyway, let's keep it like this.

> 
> >  
> >> >+#define ECC_IRQ_REG(x)		((x) == ECC_ENC ? ECC_ENCIRQ_EN : ECC_DECIRQ_EN)
> >> >+#define ECC_IRQ_EN(x)		((x) == ECC_ENC ? ENC_IRQEN : DEC_IRQEN)
> >> >+#define ECC_CTL_REG(x)		((x) == ECC_ENC ? ECC_ENCCON : ECC_DECCON)
> >> >+#define ECC_CODEC_ENABLE(x)	((x) == ECC_ENC ? ENC_EN : DEC_EN)
> >> >+#define ECC_CODEC_DISABLE(x)	((x) == ECC_ENC ? ENC_DE : DEC_DE)  
> 
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>> +struct mtk_ecc {
>> >+	struct device *dev;
>> >+	void __iomem *regs;
>> >+	struct clk *clk;
>> >+
>> >+	struct completion done;
>> >+	struct semaphore sem;
> You tried to explain me why you decided to go for a semaphore instead of
> a mutex, but I don't remember. Could you explain it again?
> If that's all about being interruptible, then you can use

Just for flexibility, no other reason really.
Neither the mutex nor the semaphore are actually needed in this driver.
Not knowing how things are going to evolve in the upper layers of MTD I 
didn't feel comfortable taking a lock in a function and unlocking the 
mutex in a different function (which is the way this driver operates). 
with that in mind I opted for a semaphore since it can always be 
unlocked -if needed be- by a different thread.

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon]

On Tue, 10 May 2016 10:45:31 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/10/2016 08:13 AM, Boris Brezillon wrote:
> >> +struct mtk_ecc {  
> >> >+	struct device *dev;
> >> >+	void __iomem *regs;
> >> >+	struct clk *clk;
> >> >+
> >> >+	struct completion done;
> >> >+	struct semaphore sem;  
> > You tried to explain me why you decided to go for a semaphore instead of
> > a mutex, but I don't remember. Could you explain it again?
> > If that's all about being interruptible, then you can use  
> 
> Just for flexibility, no other reason really.
> Neither the mutex nor the semaphore are actually needed in this driver.
> Not knowing how things are going to evolve in the upper layers of MTD I 
> didn't feel comfortable taking a lock in a function and unlocking the 
> mutex in a different function (which is the way this driver operates). 
> with that in mind I opted for a semaphore since it can always be 
> unlocked -if needed be- by a different thread.

But that has nothing to do with possible evolutions in the MTD layer.
The ECC engine resource can only have a single user at a time, hence
the mutex approach. Sorry, but I don't understand the "flexibility"
argument, but maybe I'm misunderstanding the different between a
semaphore and a mutex.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 10:59 AM, Boris Brezillon wrote:
> On Tue, 10 May 2016 10:45:31 -0400
> Jorge Ramirez<jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>  wrote:
>
>> >On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>>>> > >>+struct mtk_ecc {
>>>>> > >> >+	struct device *dev;
>>>>> > >> >+	void __iomem *regs;
>>>>> > >> >+	struct clk *clk;
>>>>> > >> >+
>>>>> > >> >+	struct completion done;
>>>>> > >> >+	struct semaphore sem;
>>> > >You tried to explain me why you decided to go for a semaphore instead of
>>> > >a mutex, but I don't remember. Could you explain it again?
>>> > >If that's all about being interruptible, then you can use
>> >
>> >Just for flexibility, no other reason really.
>> >Neither the mutex nor the semaphore are actually needed in this driver.
>> >Not knowing how things are going to evolve in the upper layers of MTD I
>> >didn't feel comfortable taking a lock in a function and unlocking the
>> >mutex in a different function (which is the way this driver operates).
>> >with that in mind I opted for a semaphore since it can always be
>> >unlocked -if needed be- by a different thread.
> But that has nothing to do with possible evolutions in the MTD layer.
> The ECC engine resource can only have a single user at a time, hence
> the mutex approach. Sorry, but I don't understand the "flexibility"
> argument, but maybe I'm misunderstanding the different between a
> semaphore and a mutex.

mutex can only be unlocked by the same thread that locks it. if for some 
reason (hypotetically speaking) the MTD wishes to unlock ecc engines and 
implement an abort operation before running operations complete this 
could guarantee that the ecc engine is left in an unlocked state. the 
risk of having a mutex in this driver is that the lock/unlock happen in 
_different_ function calls (which is always scary) with DMAs having to 
complete in between so if the running thread was canceled before the 
mutex protecting the resource was unlocked we would never be able to 
unlock it....with a semaphore we could (any thread could unlock the 
resource).

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>> +	if (config->codec == ECC_ENC) {
>> >+		/* configure ECC encoder (in bits) */
>> >+		enc_sz = config->enc_len << 3;
>> >+
>> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
>> >+		reg |= (enc_sz << ECC_MS_SHIFT);
>> >+		writel(reg, ecc->regs + ECC_ENCCNFG);
>> >+
>> >+		if (config->ecc_mode != ECC_NFI_MODE)
>> >+			writel(lower_32_bits(config->addr),
>> >+				ecc->regs + ECC_ENCDIADDR);
>> >+
>> >+	} else {
>> >+		/* configure ECC decoder (in bits) */
>> >+		dec_sz = config->dec_len;
>> >+
>> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
>> >+		reg |= (dec_sz << ECC_MS_SHIFT) | DEC_CNFG_CORRECT;
>> >+		reg |= DEC_EMPTY_EN;
>> >+		writel(reg, ecc->regs + ECC_DECCNFG);
>> >+
>> >+		if (config->sec_mask)
>> >+			ecc->sec_mask = 1 << (config->sec_mask - 1);
>> >+	}
> I see that some of the logic could be shared between the ENC and DEC
> cases.

I guess you are referring to
reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);

ok...

> BTW, why do you multiply enc_len by 8 (bits to byte conversion), but
> don't do that for dec_len?
>

just as needed by the hardware:
the config is in bits, the encoder register requires bytes, the decoder 
register requires bits.

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon]

On Tue, 10 May 2016 10:50:31 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/10/2016 08:13 AM, Boris Brezillon wrote:
> >> +	if (config->codec == ECC_ENC) {  
> >> >+		/* configure ECC encoder (in bits) */
> >> >+		enc_sz = config->enc_len << 3;
> >> >+
> >> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> >> >+		reg |= (enc_sz << ECC_MS_SHIFT);
> >> >+		writel(reg, ecc->regs + ECC_ENCCNFG);
> >> >+
> >> >+		if (config->ecc_mode != ECC_NFI_MODE)
> >> >+			writel(lower_32_bits(config->addr),
> >> >+				ecc->regs + ECC_ENCDIADDR);
> >> >+
> >> >+	} else {
> >> >+		/* configure ECC decoder (in bits) */
> >> >+		dec_sz = config->dec_len;
> >> >+
> >> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> >> >+		reg |= (dec_sz << ECC_MS_SHIFT) | DEC_CNFG_CORRECT;
> >> >+		reg |= DEC_EMPTY_EN;
> >> >+		writel(reg, ecc->regs + ECC_DECCNFG);
> >> >+
> >> >+		if (config->sec_mask)
> >> >+			ecc->sec_mask = 1 << (config->sec_mask - 1);
> >> >+	}  
> > I see that some of the logic could be shared between the ENC and DEC
> > cases.  
> 
> I guess you are referring to
> reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> 
> ok...

and

reg |= (sz << ECC_MS_SHIFT);

Ok, maybe it's not so important.

> 
> > BTW, why do you multiply enc_len by 8 (bits to byte conversion), but
> > don't do that for dec_len?
> >  
> 
> just as needed by the hardware:
> the config is in bits, the encoder register requires bytes, the decoder 
> register requires bits.
> 

Are you sure about that? Cause it seems to me that the NAND controller
drivers put a length in bits in ->dec_len and a length in bytes in
->enc_len, and then you have an extra conversion in the ECC engine
driver code for enc_len to convert it into a value in bits.

I don't care if you decide to store this value in bytes or bits, but it
should be the same unit for both fields (and I even think we should
have a single field for both encoding and decoding mode).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 11:13 AM, Boris Brezillon wrote:
>> >just as needed by the hardware:
>> >the config is in bits, the encoder register requires bytes, the decoder
>> >register requires bits.
>> >
> Are you sure about that? Cause it seems to me that the NAND controller
> drivers put a length in bits in ->dec_len and a length in bytes in
> ->enc_len, and then you have an extra conversion in the ECC engine
> driver code for enc_len to convert it into a value in bits.

yes you are right (was reading at the spec before to be sure but the 
spec is wrong since the driver works...yes I remember now discussing 
this with mediatek).
"ENC_MS: indicates the total bytes size of message block. the message 
block can only be in byte unit." - yes, this line of the spec is wrong.

will fix the code to make it consistent on the config interface (both 
enc_len or dec_len should be the same unit); and in fact as you also 
pointed out we only need one len parameter since the decoder len can be 
deduced from the encoder using the strength and the number of parity bits.

thanks!

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>> +
>> >+void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
> If you save the mode somewhere in mtk_ecc (or just write in both ENC
> and DEC registers), you won't need this extra config arg here.

ok.

>
>> >+{
>> >+	enum mtk_ecc_codec codec = config->codec;
>> >+
>> >+	mtk_ecc_codec_wait_idle(ecc, codec);
>> >+	writew(0, ecc->regs + ECC_IRQ_REG(codec));
>> >+	writew(ECC_CODEC_DISABLE(codec), ecc->regs + ECC_CTL_REG(codec));
>> >+	up(&ecc->sem);
>> >+}
>> >+EXPORT_SYMBOL(mtk_ecc_disable);

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 10:53 AM, Jorge Ramirez wrote:
> On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>>> +
>>> >+void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config 
>>> *config)
>> If you save the mode somewhere in mtk_ecc (or just write in both ENC
>> and DEC registers), you won't need this extra config arg here.
>
> ok. 

actually as I was implementing this change (and it is a simple change to 
do) I am not sure that it is a good idea to store the state of the 
engine in the ecc_driver (ie: codec_x active sort of flag). And writing 
to both set of registers looks ugly to me.

the way the nand driver is written, it is the nand driver the one that 
controls the ecc engine 'states' (init, enable_x,disable_x). Why do we 
want the ecc engine to remember that it was running the encoder or the 
decoder? I think keeping the parameter in this function makes more 
sense, that way the ecc driver itself completely stateless.

however let me see if there is a register I can poke to find out the 
active codec (then I could remove the config parameter that you pointed 
out without having to store state). I should be able to just read the 
ECC control register to determine this.

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon]

On Tue, 10 May 2016 14:14:13 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/10/2016 10:53 AM, Jorge Ramirez wrote:
> > On 05/10/2016 08:13 AM, Boris Brezillon wrote:  
> >>> +  
> >>> >+void mtk_ecc_disable(struct mtk_ecc *ecc, struct mtk_ecc_config   
> >>> *config)  
> >> If you save the mode somewhere in mtk_ecc (or just write in both ENC
> >> and DEC registers), you won't need this extra config arg here.  
> >
> > ok.   
> 
> actually as I was implementing this change (and it is a simple change to 
> do) I am not sure that it is a good idea to store the state of the 
> engine in the ecc_driver (ie: codec_x active sort of flag). And writing 
> to both set of registers looks ugly to me.
> 
> the way the nand driver is written, it is the nand driver the one that 
> controls the ecc engine 'states' (init, enable_x,disable_x). Why do we 
> want the ecc engine to remember that it was running the encoder or the 
> decoder? I think keeping the parameter in this function makes more 
> sense, that way the ecc driver itself completely stateless.
> 
> however let me see if there is a register I can poke to find out the 
> active codec (then I could remove the config parameter that you pointed 
> out without having to store state). I should be able to just read the 
> ECC control register to determine this.
> 
> 

Ok, that's not so important either. It's just that it's more common to
have an enable/config() function that takes some argument to let the
driver know how it should operate and the disable() function that just
disables everything, no matter what config you previously passed. If
it's really inconvenient for you, then let's just keep it as it is
right now. 

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Jorge Ramirez]

On 05/10/2016 08:13 AM, Boris Brezillon wrote:
>> +
>> >+void mtk_ecc_hw_init(struct mtk_ecc *ecc)
> No need to expose this function. Make it static and remove the
> prototype in your header.
>
yes, it was a leftover.