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

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

[Jorge Ramirez-Ortiz]

Fixes/Changes on v3 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@free-electrons.com>
		 Date:	Thu Feb 4 10:16:18 2016 +0100
		
  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.

The MTK controller generates ECC parity data for both the sector data
and the 8B FDM data associated to that sector.


* 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.
  
  the driver uses the CPU to copy the OOB information for each sector
  to its associated FDM register (4B FDM_L, 4B FDM_H) before
  initiating the DMA.


* Bad block mark position:
--------------------------

The bad block mark position is generated at some offset within the
sector data area.

Similarly, when marking a block as bad, the driver will fill the whole
page with 0x00 (it would be meaningless for this controller to write
the watermark to the OOB area).


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

 * dma chaning not supported:

	To guarantee that the pages corresponding to the buffers are contiguous
	in memory we enabled NAND_USE_BOUNCE_BUFFER.

Tests:
======

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

	- eraseblock write speed		: 7451 KiB/s
	- eraseblock read speed 		: 10817 KiB/s
	- page write speed 			: 7250 KiB/s
	- page read speed 			: 10792 KiB/s
	- 2 page write speed 			: 7350 KiB/s
	- 2 page read speed			: 10802 KiB/s
	- erase speed				: 105215 KiB/s
	- 2x multi-block erase speed 		: 354728 KiB/s
	- 4x multi-block erase speed		: 356658 KiB/s
	- 8x multi-block erase speed		: 357631 KiB/s
	- 16x multi-block erase speed		: 358365 KiB/s
	- 32x multi-block erase speed		: 358365 KiB/s
	- 64x multi-block erase speed		: 358610 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 |  145 +++
 drivers/mtd/nand/Kconfig                           |    7 +
 drivers/mtd/nand/Makefile                          |    1 +
 drivers/mtd/nand/mtk_ecc.c                         |  449 +++++++
 drivers/mtd/nand/mtk_ecc.h                         |   56 +
 drivers/mtd/nand/mtk_nand.c                        | 1266 ++++++++++++++++++++
 6 files changed, 1924 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.5.0

[PATCH v3 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@linaro.org>
---
 Documentation/devicetree/bindings/mtd/mtk-nand.txt | 145 +++++++++++++++++++++
 1 file changed, 145 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..ca3cc95
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
@@ -0,0 +1,145 @@
+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.
+- spare_per_sector:	Spare size of each sector.
+- nand-on-flash-bbt:	Store BBT on NAND Flash.
+- nand-ecc-mode:	NAND ecc mode (check driver for supported modes)
+- nand-ecc-strength:	Number of bits to correct per ECC step.
+- nand-ecc-step-size:	Number of data bytes covered by a single ECC step.
+
+Optional:
+- 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>;
+			spare_per_sector = <56>;
+			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.5.0

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

[Boris Brezillon]

On Mon, 11 Apr 2016 12:56:11 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:

> This patch adds documentation support for Smart Device Gen1 type of
> NAND controllers.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  Documentation/devicetree/bindings/mtd/mtk-nand.txt | 145 +++++++++++++++++++++
>  1 file changed, 145 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..ca3cc95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
> @@ -0,0 +1,145 @@
> +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.
> +- spare_per_sector:	Spare size of each sector.

Do you really need this property? It should be deduced from ECC
strength/step-size and oobsize info (or any other NFC/ECC constraints).


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

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

[Jorge Ramirez-Ortiz]

On 04/11/2016 07:15 PM, Boris Brezillon wrote:
> On Mon, 11 Apr 2016 12:56:11 -0400
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:
>
>> This patch adds documentation support for Smart Device Gen1 type of
>> NAND controllers.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/mtd/mtk-nand.txt | 145 +++++++++++++++++++++
>>  1 file changed, 145 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..ca3cc95
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/mtk-nand.txt
>> @@ -0,0 +1,145 @@
>> +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.
>> +- spare_per_sector:	Spare size of each sector.
> Do you really need this property? It should be deduced from ECC
> strength/step-size and oobsize info (or any other NFC/ECC constraints).
>
>

I think so.

the set of valid values for SPARE_PER_SECTOR is actually defined in the product
data sheet (NFI_PAGEFMT register bits 7:4)
the 16 valid values are (in Bytes) 16, 26, 27, 28, 32, 36, 40, 44, 48, 49, 50,
51, 52,62, 63 and 64.

I suppose we could do some maths (oobsize/step size) and approximate to these
values but we thought it would be simpler just to actually code the right value
in the device tree?



 

[PATCH v3 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@linaro.org>
---
 drivers/mtd/nand/Kconfig    |    7 +
 drivers/mtd/nand/Makefile   |    1 +
 drivers/mtd/nand/mtk_ecc.c  |  449 +++++++++++++++
 drivers/mtd/nand/mtk_ecc.h  |   56 ++
 drivers/mtd/nand/mtk_nand.c | 1266 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 1779 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..627f0a7
--- /dev/null
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -0,0 +1,449 @@
+/*
+ * MTK ECC controller driver.
+ * Copyright (C) 2016  MediaTek Inc.
+ * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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 "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_12BIT		(4)
+#define		ECC_CNFG_24BIT		(10)
+#define		ECC_NFI_MODE		BIT(5)
+#define		ECC_DMA_MODE		(0)
+#define		ECC_ENC_MODE_MASK	(0x3 << 5)
+#define		ECC_MS_SHIFT		(16)
+#define ECC_ENCDIADDR		(0x08)
+#define ECC_ENCIDLE		(0x0C)
+#define		ENC_IDLE		BIT(0)
+#define ECC_ENCPAR0		(0x10)
+#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_PARITY_BITS		(14)
+
+struct mtk_ecc {
+	struct device *dev;
+	void __iomem *regs;
+	struct mutex lock;
+	struct clk *clk;
+
+	struct completion done;
+	u32 sec_mask;
+};
+
+static inline void mtk_ecc_encoder_idle(struct mtk_ecc *ecc)
+{
+	struct device *dev = ecc->dev;
+	u32 val;
+	int ret;
+
+	ret = readl_poll_timeout_atomic(ecc->regs + ECC_ENCIDLE, val,
+					val & ENC_IDLE, 10, ECC_TIMEOUT);
+	if (ret)
+		dev_warn(dev, "encoder NOT idle\n");
+}
+
+static inline void mtk_ecc_decoder_idle(struct mtk_ecc *ecc)
+{
+	struct device *dev = ecc->dev;
+	u32 val;
+	int ret;
+
+	ret = readl_poll_timeout_atomic(ecc->regs + ECC_DECIDLE, val,
+					val & DEC_IDLE, 10, ECC_TIMEOUT);
+	if (ret)
+		dev_warn(dev, "decoder NOT idle\n");
+}
+
+static irqreturn_t mtk_ecc_irq(int irq, void *id)
+{
+	struct mtk_ecc *ecc = id;
+	u32 dec, enc;
+
+	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
+	enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
+
+	if (!(dec || enc))
+		return IRQ_NONE;
+
+	if (dec) {
+		dec = readw(ecc->regs + ECC_DECDONE);
+		if (dec & ecc->sec_mask) {
+			ecc->sec_mask = 0;
+			complete(&ecc->done);
+			writew(0, ecc->regs + ECC_DECIRQ_EN);
+		}
+	} else {
+		complete(&ecc->done);
+		writel(0, ecc->regs + ECC_ENCIRQ_EN);
+	}
+
+	return IRQ_HANDLED;
+}
+
+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);
+
+void mtk_ecc_enable_encode(struct mtk_ecc *ecc)
+{
+	mtk_ecc_encoder_idle(ecc);
+	writew(ENC_EN, ecc->regs + ECC_ENCCON);
+}
+EXPORT_SYMBOL(mtk_ecc_enable_encode);
+
+void mtk_ecc_disable_encode(struct mtk_ecc *ecc)
+{
+	writew(0, ecc->regs + ECC_ENCIRQ_EN);
+	mtk_ecc_encoder_idle(ecc);
+	writew(ENC_DE, ecc->regs + ECC_ENCCON);
+}
+EXPORT_SYMBOL(mtk_ecc_disable_encode);
+
+void mtk_ecc_enable_decode(struct mtk_ecc *ecc)
+{
+	mtk_ecc_decoder_idle(ecc);
+	writel(DEC_EN, ecc->regs + ECC_DECCON);
+}
+EXPORT_SYMBOL(mtk_ecc_enable_decode);
+
+void mtk_ecc_disable_decode(struct mtk_ecc *ecc)
+{
+	writew(0, ecc->regs + ECC_DECIRQ_EN);
+	mtk_ecc_decoder_idle(ecc);
+	writel(DEC_DE, ecc->regs + ECC_DECCON);
+}
+EXPORT_SYMBOL(mtk_ecc_disable_decode);
+
+void mtk_ecc_start_decode(struct mtk_ecc *ecc, int sectors)
+{
+	ecc->sec_mask = 1 << (sectors - 1);
+	init_completion(&ecc->done);
+	writew(DEC_IRQEN, ecc->regs + ECC_DECIRQ_EN);
+}
+EXPORT_SYMBOL(mtk_ecc_start_decode);
+
+int mtk_ecc_wait_decode(struct mtk_ecc *ecc)
+{
+	int ret;
+
+	ret = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
+	if (!ret) {
+		dev_err(ecc->dev, "decode timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(mtk_ecc_wait_decode);
+
+int mtk_ecc_start_encode(struct mtk_ecc *ecc, struct mtk_ecc_enc_data *d)
+{
+	dma_addr_t addr;
+	u32 *p, len;
+	u32 reg, i;
+	int rc, ret = 0;
+
+	addr = dma_map_single(ecc->dev, d->data, d->len, DMA_TO_DEVICE);
+	rc = dma_mapping_error(ecc->dev, addr);
+	if (rc) {
+		dev_err(ecc->dev, "dma mapping error\n");
+		return -EINVAL;
+	}
+
+	/* enable the encoder in DMA mode to calculate the ECC bytes  */
+	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
+	reg |= ECC_DMA_MODE;
+	writel(reg, ecc->regs + ECC_ENCCNFG);
+
+	writel(ENC_IRQEN, ecc->regs + ECC_ENCIRQ_EN);
+	writel(lower_32_bits(addr), ecc->regs + ECC_ENCDIADDR);
+
+	init_completion(&ecc->done);
+	writew(ENC_EN, ecc->regs + ECC_ENCCON);
+
+	rc = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
+	if (!rc) {
+		dev_err(ecc->dev, "encode timeout\n");
+		writel(0, ecc->regs + ECC_ENCIRQ_EN);
+		ret = -ETIMEDOUT;
+		goto timeout;
+	}
+
+	mtk_ecc_encoder_idle(ecc);
+
+	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
+	len = (d->strength * ECC_PARITY_BITS + 7) >> 3;
+	p = (u32 *) (d->data + d->len);
+
+	/* 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_ENCPAR0 + i * sizeof(u32));
+
+timeout:
+
+	dma_unmap_single(ecc->dev, addr, d->len, DMA_TO_DEVICE);
+
+	writew(0, ecc->regs + ECC_ENCCON);
+	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
+	reg |= ECC_NFI_MODE;
+	writel(reg, ecc->regs + ECC_ENCCNFG);
+
+	return ret;
+}
+EXPORT_SYMBOL(mtk_ecc_start_encode);
+
+void mtk_ecc_hw_init(struct mtk_ecc *ecc)
+{
+	mtk_ecc_encoder_idle(ecc);
+	writew(ENC_DE, ecc->regs + ECC_ENCCON);
+
+	mtk_ecc_decoder_idle(ecc);
+	writel(DEC_DE, ecc->regs + ECC_DECCON);
+}
+
+int mtk_ecc_config(struct mtk_ecc *ecc, struct mtk_ecc_config *config)
+{
+	u32 ecc_bit, dec_sz, enc_sz;
+	u32 reg;
+
+	switch (config->strength) {
+	case 4:
+		ecc_bit = ECC_CNFG_4BIT;
+		break;
+	case 12:
+		ecc_bit = ECC_CNFG_12BIT;
+		break;
+	case 24:
+		ecc_bit = ECC_CNFG_24BIT;
+		break;
+	default:
+		dev_err(ecc->dev, "invalid spare len per sector\n");
+		return -EINVAL;
+	}
+
+	/* configure ECC encoder (in bits) */
+	enc_sz = config->step_len << 3;
+	reg = ecc_bit | ECC_NFI_MODE | (enc_sz << ECC_MS_SHIFT);
+	writel(reg, ecc->regs + ECC_ENCCNFG);
+
+	/* configure ECC decoder (in bits) */
+	dec_sz = enc_sz + config->strength * ECC_PARITY_BITS;
+	reg = ecc_bit | ECC_NFI_MODE | (dec_sz << ECC_MS_SHIFT);
+	reg |= DEC_CNFG_CORRECT | DEC_EMPTY_EN;
+	writel(reg, ecc->regs + ECC_DECCNFG);
+
+	return 0;
+}
+EXPORT_SYMBOL(mtk_ecc_config);
+
+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;
+	}
+
+	mutex_init(&ecc->lock);
+
+	ecc->dev = dev;
+
+	platform_set_drvdata(pdev, ecc);
+
+	dev_info(dev, "driver 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@mediatek.com>");
+MODULE_AUTHOR("Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.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..d12bc5f
--- /dev/null
+++ b/drivers/mtd/nand/mtk_ecc.h
@@ -0,0 +1,56 @@
+/*
+ * MTK SDG1 ECC controller
+ *
+ * Copyright (c) 2016 Mediatek
+ * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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>
+
+struct device_node;
+struct mtk_ecc;
+
+/**
+ * @len: number of bytes in the data buffer
+ * @data: pointer to memory holding the data
+ * @strength: number of correctable bits
+ */
+struct mtk_ecc_enc_data {
+	unsigned int len;
+	int strength;
+	u8 *data;
+};
+
+struct mtk_ecc_stats {
+	u32 corrected;
+	u32 bitflips;
+	u32 failed;
+};
+
+struct mtk_ecc_config {
+	u32 strength;
+	u32 step_len;
+};
+
+void mtk_ecc_enable_decode(struct mtk_ecc *);
+void mtk_ecc_disable_decode(struct mtk_ecc *);
+
+int mtk_ecc_wait_decode(struct mtk_ecc *);
+void mtk_ecc_enable_encode(struct mtk_ecc *);
+void mtk_ecc_disable_encode(struct mtk_ecc *);
+int mtk_ecc_start_encode(struct mtk_ecc *, struct mtk_ecc_enc_data *);
+void mtk_ecc_hw_init(struct mtk_ecc *);
+int mtk_ecc_config(struct mtk_ecc *, struct mtk_ecc_config *);
+void mtk_ecc_release(struct mtk_ecc *);
+struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
+
+void mtk_ecc_start_decode(struct mtk_ecc *, int sectors);
+void mtk_ecc_get_stats(struct mtk_ecc *, struct mtk_ecc_stats *, int sectors);
+#endif
diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
new file mode 100644
index 0000000..048024f
--- /dev/null
+++ b/drivers/mtd/nand/mtk_nand.c
@@ -0,0 +1,1266 @@
+/*
+ * MTK NAND Flash controller driver.
+ * Copyright (C) 2016 MediaTek Inc.
+ * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
+ *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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_28	(3)
+#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_FDM_REG_SIZE	(8)
+#define NFI_FDM0L		(0xA0)
+#define NFI_FDM0M		(0xA4)
+#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)
+
+struct mtk_nfc_clk {
+	struct clk *nfi_clk;
+	struct clk *pad_clk;
+};
+
+struct mtk_nfc_nand_chip {
+	struct list_head node;
+	struct nand_chip nand;
+	u32 spare_per_sector;
+	int nsels;
+	u8 sels[0];
+};
+
+struct mtk_nfc {
+	struct nand_hw_control controller;
+	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)
+{
+	return chip->oob_poi + i * NFI_FDM_REG_SIZE;
+}
+
+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 int mtk_step_len(struct nand_chip *chip)
+{
+	return chip->ecc.size + NFI_FDM_REG_SIZE;
+}
+
+/* NFI register access */
+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;
+
+	nfi_writel(nfc, CON_FIFO_FLUSH | CON_NFI_RST, NFI_CON);
+
+	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);
+
+	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 = mtk_nand->spare_per_sector;
+	struct mtk_ecc_config config;
+
+	/* skip configuration when recognize NAND Flash */
+	if (!mtd->writesize)
+		return 0;
+
+	switch (mtd->writesize) {
+	case 512:
+		fmt = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
+		break;
+	case KB(2):
+		fmt = PAGEFMT_512_2K;
+		break;
+	case KB(4):
+		fmt = PAGEFMT_2K_4K;
+		break;
+	case KB(8):
+		fmt = PAGEFMT_4K_8K;
+		break;
+	default:
+		dev_err(nfc->dev, "invalid page len: %d\n", mtd->writesize);
+		return -EINVAL;
+	}
+
+	if (mtd->writesize > 512)
+		spare >>= 1;
+
+	switch (spare) {
+	case 16:
+		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
+		break;
+	case 28:
+		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
+		break;
+	default:
+		break;
+	}
+	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_SHIFT;
+	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_ECC_SHIFT;
+	nfi_writew(nfc, fmt, NFI_PAGEFMT);
+
+	config.step_len = mtk_step_len(chip);
+	config.strength = chip->ecc.strength;
+	mtk_ecc_config(nfc->ecc, &config);
+
+	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);
+
+		/* hardware issue work around:
+		 * The first byte of data may be wrong right after the trigger.
+		 * (The controller fetches data until the internal FIFO is full)
+		 */
+		udelay(10);
+	}
+
+	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_ecc_enc_data enc_data = {
+		.strength = chip->ecc.strength,
+		.len = mtk_step_len(chip),
+		.data = data,
+	};
+
+	return mtk_ecc_start_encode(nfc->ecc, &enc_data);
+}
+
+static int mtk_nfc_format_subpage(struct mtd_info *mtd, uint32_t offset,
+			uint32_t len, const uint8_t *buf, int oob_on)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	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 (i < start || i >= end)
+			continue;
+
+		if (oob_on)
+			memcpy(mtk_oob_ptr(chip, i), oob_ptr(chip, i),
+				NFI_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,
+				int oob_on)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	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 (oob_on)
+			memcpy(mtk_oob_ptr(chip, i), oob_ptr(chip, i),
+				NFI_FDM_REG_SIZE);
+	}
+}
+
+static inline void mtk_nfc_read_fdm(struct nand_chip *chip, 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, i);
+		p[0] = nfi_readl(nfc, NFI_FDM0L + i * NFI_FDM_REG_SIZE);
+		p[1] = nfi_readl(nfc, NFI_FDM0M + i * NFI_FDM_REG_SIZE);
+	}
+}
+
+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_FDM0L + i * NFI_FDM_REG_SIZE);
+		nfi_writel(nfc, p[1], NFI_FDM0M + i * NFI_FDM_REG_SIZE);
+	}
+}
+
+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 oob_on, int page, int raw)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(chip);
+	size_t len;
+	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);
+
+		mtk_ecc_enable_encode(nfc->ecc);
+
+		/* write OOB into the FDM registers (OOB area in MTK NAND) */
+		if (oob_on)
+			mtk_nfc_write_fdm(chip);
+	}
+
+	len = mtd->writesize + (raw ? mtd->oobsize : 0);
+	ret = mtk_nfc_do_write_page(mtd, chip, buf, page, len);
+
+	if (!raw)
+		mtk_ecc_disable_encode(nfc->ecc);
+
+	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, oob_on, 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, oob_on);
+	return mtk_nfc_write_page(mtd, chip, nfc->buffer, 0, 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, oob_on);
+	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, 0, page, 1);
+}
+
+static int mtk_nfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+				int page)
+{
+	u8 *data = chip->buffers->databuf;
+	int ret;
+
+	memset(data, 0xff, mtd->writesize);
+
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+
+	ret = mtk_nfc_write_page_hwecc(mtd, chip, data, 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_write_oob_raw(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_oob(struct mtd_info *mtd, struct nand_chip *chip,
+				u8 *buf, u32 sectors)
+{
+	struct mtk_nfc *nfc = nand_get_controller_data(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, NFI_FDM_REG_SIZE);
+
+		return 0;
+	}
+
+	mtk_nfc_read_fdm(chip, sectors);
+
+	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_block_markbad(struct mtd_info *mtd, loff_t ofs)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	u8 *buf = chip->buffers->databuf;
+	int page, rc, i;
+
+	memset(buf, 0x00, mtd->writesize + mtd->oobsize);
+
+	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
+		ofs += mtd->erasesize - mtd->writesize;
+
+	i = 0;
+	do {
+		page = (int)(ofs >> chip->page_shift);
+		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+		rc = mtk_nfc_write_page(mtd, chip, buf, 0, page, 1);
+		if (rc < 0)
+			return rc;
+
+		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+		rc = chip->waitfunc(mtd, chip);
+		rc = rc & NAND_STATUS_FAIL ? -EIO : 0;
+		if (rc < 0)
+			return rc;
+
+		ofs += mtd->writesize;
+		i++;
+
+	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
+
+	return 0;
+}
+
+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 column, sectors, start, end, reg;
+	u32 spare = mtk_nand->spare_per_sector;
+	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);
+
+		mtk_ecc_enable_decode(nfc->ecc);
+	} 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);
+
+	if (!raw)
+		mtk_ecc_start_decode(nfc->ecc, sectors);
+
+	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_decode(nfc->ecc);
+			bitflips = rc < 0 ? -ETIMEDOUT :
+				mtk_nfc_update_oob(mtd, chip, buf, sectors);
+		}
+	}
+
+	dma_unmap_single(nfc->dev, addr, len, DMA_FROM_DEVICE);
+
+	if (!raw)
+		mtk_ecc_disable_decode(nfc->ecc);
+
+	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_hwecc(mtd, chip, 0, mtd->writesize, p, pg);
+}
+
+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 *nfc = nand_get_controller_data(chip);
+	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++) {
+		if (buf)
+			memcpy(data_ptr(chip, buf, i), mtk_data_ptr(chip, i),
+							chip->ecc.size);
+		if (oob_on)
+			memcpy(oob_ptr(chip, i), mtk_oob_ptr(chip, i),
+							NFI_FDM_REG_SIZE);
+	}
+
+	return ret;
+}
+
+static int mtk_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+				int page)
+{
+	u8 *buf = chip->buffers->databuf;
+	struct mtd_ecc_stats stats;
+	int ret;
+
+	stats = mtd->ecc_stats;
+
+	memset(buf, 0xff, mtd->writesize);
+	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+
+	ret = mtk_nfc_read_page_hwecc(mtd, chip, buf, 1, page);
+
+	/* mark as invalid data 0x00 if UECC happens */
+	if ((mtd->ecc_stats.failed - stats.failed) > 0)
+		memset(chip->oob_poi, 0, mtd->oobsize);
+
+	if (ret < mtd->bitflip_threshold)
+		mtd->ecc_stats.corrected = stats.corrected;
+
+	return ret;
+}
+
+static int mtk_nfc_read_oob_raw(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);
+
+	if (section)
+		return -ERANGE;
+
+	oob_region->length = NFI_FDM_REG_SIZE * chip->ecc.steps;
+	oob_region->offset = 0;
+
+	return 0;
+}
+
+static int mtk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
+				struct mtd_oob_region *oob_region)
+{
+	struct mtd_oob_region free_region;
+
+	if (section)
+		return -ERANGE;
+
+	mtk_nfc_ooblayout_free(mtd, 0, &free_region);
+
+	oob_region->length = mtd->oobsize - free_region.length;
+	oob_region->offset = free_region.length;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops mtk_nfc_ooblayout_ops = {
+	.free = mtk_nfc_ooblayout_free,
+	.ecc = mtk_nfc_ooblayout_ecc,
+};
+
+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;
+	}
+
+	if (of_property_read_u32(np, "spare_per_sector",
+						&chip->spare_per_sector)) {
+		dev_err(dev, "missing spare_per_sector property in DT\n");
+		return -ENODEV;
+
+	}
+
+	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->block_markbad = mtk_nfc_block_markbad;
+	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_raw;
+	nand->ecc.write_oob = mtk_nfc_write_oob;
+
+	nand->ecc.read_subpage = mtk_nfc_read_subpage_hwecc;
+	nand->ecc.read_page_raw = mtk_nfc_read_page_raw;
+	nand->ecc.read_oob_raw = mtk_nfc_read_oob_raw;
+	nand->ecc.read_page = mtk_nfc_read_page_hwecc;
+	nand->ecc.read_oob = mtk_nfc_read_oob;
+
+	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;
+
+	/* TODO: add NAND_ECC_SOFT */
+	if (nand->ecc.mode != NAND_ECC_HW) {
+		dev_err(dev, "driver only supports NAND_ECC_HW\n");
+		return -ENODEV;
+	}
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		return -ENODEV;
+
+	len = mtd->writesize + mtd->oobsize;
+	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL);
+	if (!nfc->buffer)
+		return  -ENOMEM;
+
+	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@mediatek.com>");
+MODULE_AUTHOR("Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>");
+MODULE_DESCRIPTION("MTK Nand Flash Controller Driver");
-- 
2.5.0

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

[Boris Brezillon]

Hi Jorge,

On Mon, 11 Apr 2016 12:56:12 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.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@linaro.org>
> ---
>  drivers/mtd/nand/Kconfig    |    7 +
>  drivers/mtd/nand/Makefile   |    1 +
>  drivers/mtd/nand/mtk_ecc.c  |  449 +++++++++++++++
>  drivers/mtd/nand/mtk_ecc.h  |   56 ++
>  drivers/mtd/nand/mtk_nand.c | 1266 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 1779 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..627f0a7
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -0,0 +1,449 @@
> +/*
> + * MTK ECC controller driver.
> + * Copyright (C) 2016  MediaTek Inc.
> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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 "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_12BIT		(4)
> +#define		ECC_CNFG_24BIT		(10)
> +#define		ECC_NFI_MODE		BIT(5)
> +#define		ECC_DMA_MODE		(0)
> +#define		ECC_ENC_MODE_MASK	(0x3 << 5)
> +#define		ECC_MS_SHIFT		(16)
> +#define ECC_ENCDIADDR		(0x08)
> +#define ECC_ENCIDLE		(0x0C)
> +#define		ENC_IDLE		BIT(0)
> +#define ECC_ENCPAR0		(0x10)
> +#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_PARITY_BITS		(14)
> +
> +struct mtk_ecc {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct mutex lock;

You defined this lock, but don't use it. See below for a suggestion of
where it should be used...

> +	struct clk *clk;
> +
> +	struct completion done;
> +	u32 sec_mask;
> +};
> +
> +static inline void mtk_ecc_encoder_idle(struct mtk_ecc *ecc)

Just a detail, but the function does not exactly reflect that you're
waiting for the encoder to be idle. How about renaming it
mtk_ecc_encoder_wait_idle() ?

> +{
> +	struct device *dev = ecc->dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(ecc->regs + ECC_ENCIDLE, val,
> +					val & ENC_IDLE, 10, ECC_TIMEOUT);
> +	if (ret)
> +		dev_warn(dev, "encoder NOT idle\n");
> +}
> +
> +static inline void mtk_ecc_decoder_idle(struct mtk_ecc *ecc)

Ditto.

> +{
> +	struct device *dev = ecc->dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = readl_poll_timeout_atomic(ecc->regs + ECC_DECIDLE, val,
> +					val & DEC_IDLE, 10, ECC_TIMEOUT);
> +	if (ret)
> +		dev_warn(dev, "decoder NOT idle\n");
> +}
> +
> +static irqreturn_t mtk_ecc_irq(int irq, void *id)
> +{
> +	struct mtk_ecc *ecc = id;
> +	u32 dec, enc;
> +
> +	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
> +	enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
> +
> +	if (!(dec || enc))
> +		return IRQ_NONE;
> +
> +	if (dec) {
> +		dec = readw(ecc->regs + ECC_DECDONE);
> +		if (dec & ecc->sec_mask) {
> +			ecc->sec_mask = 0;
> +			complete(&ecc->done);

If you can really do enc and dec in parallel, then you should have two
waitqueues.

> +			writew(0, ecc->regs + ECC_DECIRQ_EN);
> +		}
> +	} else {
> +		complete(&ecc->done);
> +		writel(0, ecc->regs + ECC_ENCIRQ_EN);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +

[...]

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

_enable_encode*r*()?

> +{

Not sure this is needed right now, since the NAND driver is the only
user of the ECC engine (not even sure you can use the ECC engine
independently), and we do not support accessing chips in parallel, but
it may be more future proof to take a lock before using the ECC
encoder/decoder, and release it when the operation is finished.

This your controller seems capable of doing ECC encoding/decoding in
parallel, it might be worth having 2 different locks.

If you decide to not use any lock, please add something in the
documentation, stating that it's the ECC engine user responsibility to
ensure serialization, and forbid several mtk_ecc_get() on the same
device.

> +	mtk_ecc_encoder_idle(ecc);
> +	writew(ENC_EN, ecc->regs + ECC_ENCCON);
> +}
> +EXPORT_SYMBOL(mtk_ecc_enable_encode);
> +
> +void mtk_ecc_disable_encode(struct mtk_ecc *ecc)

_disable_encode*r*()?

> +{
> +	writew(0, ecc->regs + ECC_ENCIRQ_EN);
> +	mtk_ecc_encoder_idle(ecc);
> +	writew(ENC_DE, ecc->regs + ECC_ENCCON);

Release the lock here, if any.

> +}
> +EXPORT_SYMBOL(mtk_ecc_disable_encode);
> +
> +void mtk_ecc_enable_decode(struct mtk_ecc *ecc)

_decode*r*()

> +{
> +	mtk_ecc_decoder_idle(ecc);
> +	writel(DEC_EN, ecc->regs + ECC_DECCON);
> +}
> +EXPORT_SYMBOL(mtk_ecc_enable_decode);
> +
> +void mtk_ecc_disable_decode(struct mtk_ecc *ecc)

Ditto.

> +{
> +	writew(0, ecc->regs + ECC_DECIRQ_EN);
> +	mtk_ecc_decoder_idle(ecc);
> +	writel(DEC_DE, ecc->regs + ECC_DECCON);
> +}
> +EXPORT_SYMBOL(mtk_ecc_disable_decode);
> +
> +void mtk_ecc_start_decode(struct mtk_ecc *ecc, int sectors)

AFAIS, you're not really starting the decoding here. It's triggered by
the NAND engine when it forwards data to the ECC engine.
Can we make this clearer by rename the function into
mtk_ecc_prepare_decoding(). I know I'm quite picky on those function
names, but I like when names are reflecting what's really done :).

> +{
> +	ecc->sec_mask = 1 << (sectors - 1);
> +	init_completion(&ecc->done);
> +	writew(DEC_IRQEN, ecc->regs + ECC_DECIRQ_EN);
> +}
> +EXPORT_SYMBOL(mtk_ecc_start_decode);
> +
> +int mtk_ecc_wait_decode(struct mtk_ecc *ecc)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
> +	if (!ret) {
> +		dev_err(ecc->dev, "decode timeout\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mtk_ecc_wait_decode);
> +
> +int mtk_ecc_start_encode(struct mtk_ecc *ecc, struct mtk_ecc_enc_data *d)

And here you're doing more than just preparing the encoding request,
you're actually encoding data. So, how about mtd_ecc_encode()?

> +{
> +	dma_addr_t addr;
> +	u32 *p, len;
> +	u32 reg, i;
> +	int rc, ret = 0;
> +
> +	addr = dma_map_single(ecc->dev, d->data, d->len, DMA_TO_DEVICE);
> +	rc = dma_mapping_error(ecc->dev, addr);
> +	if (rc) {
> +		dev_err(ecc->dev, "dma mapping error\n");
> +		return -EINVAL;
> +	}
> +
> +	/* enable the encoder in DMA mode to calculate the ECC bytes  */
> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
> +	reg |= ECC_DMA_MODE;

When I compare the start_encode() to the start_decode() function I see
one big difference: the former is clearly operating in non-pipeline
mode (the data are retrieved using DMA and returned values are kept in
PARX registers), while, IFAICS, the latter is operating in pipeline
mode (data are directly retrieved from the NAND engine stream).

I'm perfectly fine supporting those 2 modes (at least it gives an
answer to one of my question: the ECC engine can be used independently
of the NAND engine), but the function names should reflect this.

> +	writel(reg, ecc->regs + ECC_ENCCNFG);
> +
> +	writel(ENC_IRQEN, ecc->regs + ECC_ENCIRQ_EN);
> +	writel(lower_32_bits(addr), ecc->regs + ECC_ENCDIADDR);
> +
> +	init_completion(&ecc->done);
> +	writew(ENC_EN, ecc->regs + ECC_ENCCON);
> +
> +	rc = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
> +	if (!rc) {
> +		dev_err(ecc->dev, "encode timeout\n");
> +		writel(0, ecc->regs + ECC_ENCIRQ_EN);
> +		ret = -ETIMEDOUT;
> +		goto timeout;
> +	}
> +
> +	mtk_ecc_encoder_idle(ecc);
> +
> +	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
> +	len = (d->strength * ECC_PARITY_BITS + 7) >> 3;
> +	p = (u32 *) (d->data + d->len);
> +
> +	/* 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_ENCPAR0 + i * sizeof(u32));

Please define an ECC_ENCPAR(x) macro, where X is the parity word index.

> +
> +timeout:
> +
> +	dma_unmap_single(ecc->dev, addr, d->len, DMA_TO_DEVICE);
> +
> +	writew(0, ecc->regs + ECC_ENCCON);
> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
> +	reg |= ECC_NFI_MODE;
> +	writel(reg, ecc->regs + ECC_ENCCNFG);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mtk_ecc_start_encode);
> +

[...]

> diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
> new file mode 100644
> index 0000000..d12bc5f
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_ecc.h
> @@ -0,0 +1,56 @@
> +/*
> + * MTK SDG1 ECC controller
> + *
> + * Copyright (c) 2016 Mediatek
> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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>
> +
> +struct device_node;
> +struct mtk_ecc;
> +
> +/**
> + * @len: number of bytes in the data buffer
> + * @data: pointer to memory holding the data
> + * @strength: number of correctable bits
> + */
> +struct mtk_ecc_enc_data {

Should probably not be suffixed with _enc_, since the same structure
could be used for decoding ops.

> +	unsigned int len;
> +	int strength;

You should already have this information (either stored in a copy of
the config passed to mtk_ecc_config() or retrieved from the registers
values).

> +	u8 *data;
> +};

This being said, I wonder if we really need a struct for that. Passing
those two parameters to mtd_ecc_start_encode() is acceptable.

> +
> +struct mtk_ecc_stats {
> +	u32 corrected;
> +	u32 bitflips;
> +	u32 failed;
> +};
> +
> +struct mtk_ecc_config {
> +	u32 strength;
> +	u32 step_len;
> +};
> +
> +void mtk_ecc_enable_decode(struct mtk_ecc *);
> +void mtk_ecc_disable_decode(struct mtk_ecc *);
> +
> +int mtk_ecc_wait_decode(struct mtk_ecc *);
> +void mtk_ecc_enable_encode(struct mtk_ecc *);
> +void mtk_ecc_disable_encode(struct mtk_ecc *);
> +int mtk_ecc_start_encode(struct mtk_ecc *, struct mtk_ecc_enc_data *);
> +void mtk_ecc_hw_init(struct mtk_ecc *);
> +int mtk_ecc_config(struct mtk_ecc *, struct mtk_ecc_config *);
> +void mtk_ecc_release(struct mtk_ecc *);
> +struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
> +
> +void mtk_ecc_start_decode(struct mtk_ecc *, int sectors);
> +void mtk_ecc_get_stats(struct mtk_ecc *, struct mtk_ecc_stats *, int sectors);
> +#endif
> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
> new file mode 100644
> index 0000000..048024f
> --- /dev/null
> +++ b/drivers/mtd/nand/mtk_nand.c
> @@ -0,0 +1,1266 @@
> +/*
> + * MTK NAND Flash controller driver.
> + * Copyright (C) 2016 MediaTek Inc.
> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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_28	(3)
> +#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_FDM_REG_SIZE	(8)
> +#define NFI_FDM0L		(0xA0)
> +#define NFI_FDM0M		(0xA4)
> +#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)
> +
> +struct mtk_nfc_clk {
> +	struct clk *nfi_clk;
> +	struct clk *pad_clk;
> +};
> +
> +struct mtk_nfc_nand_chip {
> +	struct list_head node;
> +	struct nand_chip nand;
> +	u32 spare_per_sector;

We already discussed that on IRC, and I don't think you really need
this field. AFAICT, all it's encoding is the number of ECC + free bytes
you have per data chunk (ECC step size). This can all be described
using the ecc->{pre,post}pad and ecc->bytes fields.

> +	int nsels;
> +	u8 sels[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 = mtk_nand->spare_per_sector;
> +	struct mtk_ecc_config config;
> +
> +	/* skip configuration when recognize NAND Flash */
> +	if (!mtd->writesize)
> +		return 0;
> +
> +	switch (mtd->writesize) {
> +	case 512:
> +		fmt = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
> +		break;
> +	case KB(2):
> +		fmt = PAGEFMT_512_2K;
> +		break;
> +	case KB(4):
> +		fmt = PAGEFMT_2K_4K;
> +		break;
> +	case KB(8):
> +		fmt = PAGEFMT_4K_8K;
> +		break;
> +	default:
> +		dev_err(nfc->dev, "invalid page len: %d\n", mtd->writesize);
> +		return -EINVAL;
> +	}
> +
> +	if (mtd->writesize > 512)
> +		spare >>= 1;
> +
> +	switch (spare) {
> +	case 16:
> +		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	case 28:
> +		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
> +		break;
> +	default:
> +		break;
> +	}
> +	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_SHIFT;
> +	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_ECC_SHIFT;
> +	nfi_writew(nfc, fmt, NFI_PAGEFMT);
> +
> +	config.step_len = mtk_step_len(chip);
> +	config.strength = chip->ecc.strength;
> +	mtk_ecc_config(nfc->ecc, &config);

If you follow my suggestion to add a lock to the ECC engine, then
mtk_ecc_config() should only be called after you've acquired this lock
(i.e. after calling mtk_ecc_enable_{encoder/decoder}()), and thus
should be moved in your ecc->{read,write}_page() implementations.

If you want to avoid those conversions on each NAND operations, just
store a copy of mtk_ecc_config into your mtk_nfc_nand_chip struct.

> +
> +	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;
> +		nfi_writel(nfc, reg, NFI_CON);
> +
> +		/* trigger to fetch data */
> +		nfi_writew(nfc, STAR_EN, NFI_STRDATA);
> +
> +		/* hardware issue work around:
> +		 * The first byte of data may be wrong right after the trigger.
> +		 * (The controller fetches data until the internal FIFO is full)

If this is an erratum and is clearly identified in the datasheet, then
you should mention the reference and quote the datasheet.
It looks like a bad timing configuration to me, but I might be wrong.

> +		 */
> +		udelay(10);
> +	}
> +
> +	mtk_nfc_wait_ioready(nfc);
> +
> +	return nfi_readb(nfc, NFI_DATAR);
> +}
> +

[...]

> +
> +static int mtk_nfc_block_markbad(struct mtd_info *mtd, loff_t ofs)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	u8 *buf = chip->buffers->databuf;
> +	int page, rc, i;
> +
> +	memset(buf, 0x00, mtd->writesize + mtd->oobsize);
> +
> +	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> +		ofs += mtd->erasesize - mtd->writesize;
> +
> +	i = 0;
> +	do {
> +		page = (int)(ofs >> chip->page_shift);
> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> +		rc = mtk_nfc_write_page(mtd, chip, buf, 0, page, 1);
> +		if (rc < 0)
> +			return rc;
> +
> +		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +		rc = chip->waitfunc(mtd, chip);
> +		rc = rc & NAND_STATUS_FAIL ? -EIO : 0;
> +		if (rc < 0)
> +			return rc;
> +
> +		ofs += mtd->writesize;
> +		i++;
> +
> +	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
> +
> +	return 0;
> +}

Why do you need this custom implementation?

[...]

> +
> +static int mtk_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +				int page)
> +{
> +	u8 *buf = chip->buffers->databuf;
> +	struct mtd_ecc_stats stats;
> +	int ret;
> +
> +	stats = mtd->ecc_stats;
> +
> +	memset(buf, 0xff, mtd->writesize);
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +
> +	ret = mtk_nfc_read_page_hwecc(mtd, chip, buf, 1, page);
> +
> +	/* mark as invalid data 0x00 if UECC happens */
> +	if ((mtd->ecc_stats.failed - stats.failed) > 0)
> +		memset(chip->oob_poi, 0, mtd->oobsize);

No, please leave the data as is. That's really useful to debug things.

> +
> +	if (ret < mtd->bitflip_threshold)
> +		mtd->ecc_stats.corrected = stats.corrected;

Hm, ->corrected has already been updated by mtk_nfc_read_page_hwecc(),
or am I missing something?
Moreover, you should have mtd->ecc_stats.corrected += stats.corrected.

> +
> +	return ret;
> +}
> +

[...]


> +
> +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);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oob_region->length = NFI_FDM_REG_SIZE * chip->ecc.steps;

Is NFI_FDM_REG_SIZE really fixed to 8, or does it depend on the
spare_per_sector and ecc->bytes information?

> +	oob_region->offset = 0;
> +
> +	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;
> +	}
> +
> +	if (of_property_read_u32(np, "spare_per_sector",
> +						&chip->spare_per_sector)) {

As already discussed on IRC, this should be deduced from the NAND page
info retrieved in nand_scan_ident().

> +		dev_err(dev, "missing spare_per_sector property in DT\n");
> +		return -ENODEV;
> +
> +	}
> +
> +	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->block_markbad = mtk_nfc_block_markbad;
> +	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_raw;
> +	nand->ecc.write_oob = mtk_nfc_write_oob;
> +
> +	nand->ecc.read_subpage = mtk_nfc_read_subpage_hwecc;
> +	nand->ecc.read_page_raw = mtk_nfc_read_page_raw;
> +	nand->ecc.read_oob_raw = mtk_nfc_read_oob_raw;
> +	nand->ecc.read_page = mtk_nfc_read_page_hwecc;
> +	nand->ecc.read_oob = mtk_nfc_read_oob;
> +
> +	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;
> +
> +	/* TODO: add NAND_ECC_SOFT */
> +	if (nand->ecc.mode != NAND_ECC_HW) {
> +		dev_err(dev, "driver only supports NAND_ECC_HW\n");
> +		return -ENODEV;
> +	}

Do you really support all kind of NANDs (even small and very large pages
NANDs). IMHO, it's safer to do a check after nand_scan_ident(), to
verify that your implementation support the chip requirements...

> +
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		return -ENODEV;
> +
> +	len = mtd->writesize + mtd->oobsize;
> +	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL);
> +	if (!nfc->buffer)
> +		return  -ENOMEM;
> +
> +	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;
> +}
> +

I tried to carefully review most of the driver, but I may have missed a
few things. Richard, Brian, if you have some time, could you please
have look?

Thanks,

Boris

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

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

[Jorge Ramirez-Ortiz]

On 04/17/2016 06:17 PM, Boris Brezillon wrote:
> Hi Jorge,

Hi Boris,

inlined some comments.
we have started preparing v4 following most of your suggestions - we need input 
on some of the others.



>
> On Mon, 11 Apr 2016 12:56:12 -0400
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.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@linaro.org>
>> ---
>>   drivers/mtd/nand/Kconfig    |    7 +
>>   drivers/mtd/nand/Makefile   |    1 +
>>   drivers/mtd/nand/mtk_ecc.c  |  449 +++++++++++++++
>>   drivers/mtd/nand/mtk_ecc.h  |   56 ++
>>   drivers/mtd/nand/mtk_nand.c | 1266 +++++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 1779 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..627f0a7
>> --- /dev/null
>> +++ b/drivers/mtd/nand/mtk_ecc.c
>> @@ -0,0 +1,449 @@
>> +/*
>> + * MTK ECC controller driver.
>> + * Copyright (C) 2016  MediaTek Inc.
>> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
>> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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 "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_12BIT		(4)
>> +#define		ECC_CNFG_24BIT		(10)
>> +#define		ECC_NFI_MODE		BIT(5)
>> +#define		ECC_DMA_MODE		(0)
>> +#define		ECC_ENC_MODE_MASK	(0x3 << 5)
>> +#define		ECC_MS_SHIFT		(16)
>> +#define ECC_ENCDIADDR		(0x08)
>> +#define ECC_ENCIDLE		(0x0C)
>> +#define		ENC_IDLE		BIT(0)
>> +#define ECC_ENCPAR0		(0x10)
>> +#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_PARITY_BITS		(14)
>> +
>> +struct mtk_ecc {
>> +	struct device *dev;
>> +	void __iomem *regs;
>> +	struct mutex lock;
> You defined this lock, but don't use it. See below for a suggestion of
> where it should be used...
>
>> +	struct clk *clk;
>> +
>> +	struct completion done;
>> +	u32 sec_mask;
>> +};
>> +
>> +static inline void mtk_ecc_encoder_idle(struct mtk_ecc *ecc)
> Just a detail, but the function does not exactly reflect that you're
> waiting for the encoder to be idle. How about renaming it
> mtk_ecc_encoder_wait_idle() ?

OK.

>
>> +{
>> +	struct device *dev = ecc->dev;
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = readl_poll_timeout_atomic(ecc->regs + ECC_ENCIDLE, val,
>> +					val & ENC_IDLE, 10, ECC_TIMEOUT);
>> +	if (ret)
>> +		dev_warn(dev, "encoder NOT idle\n");
>> +}
>> +
>> +static inline void mtk_ecc_decoder_idle(struct mtk_ecc *ecc)
> Ditto.

OK.

>
>> +{
>> +	struct device *dev = ecc->dev;
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = readl_poll_timeout_atomic(ecc->regs + ECC_DECIDLE, val,
>> +					val & DEC_IDLE, 10, ECC_TIMEOUT);
>> +	if (ret)
>> +		dev_warn(dev, "decoder NOT idle\n");
>> +}
>> +
>> +static irqreturn_t mtk_ecc_irq(int irq, void *id)
>> +{
>> +	struct mtk_ecc *ecc = id;
>> +	u32 dec, enc;
>> +
>> +	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
>> +	enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
>> +
>> +	if (!(dec || enc))
>> +		return IRQ_NONE;
>> +
>> +	if (dec) {
>> +		dec = readw(ecc->regs + ECC_DECDONE);
>> +		if (dec & ecc->sec_mask) {
>> +			ecc->sec_mask = 0;
>> +			complete(&ecc->done);
> If you can really do enc and dec in parallel, then you should have two
> waitqueues.


unfortunately no, we can't do parallel operations.

>
>> +			writew(0, ecc->regs + ECC_DECIRQ_EN);
>> +		}
>> +	} else {
>> +		complete(&ecc->done);
>> +		writel(0, ecc->regs + ECC_ENCIRQ_EN);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
> [...]
>
>> +
>> +void mtk_ecc_enable_encode(struct mtk_ecc *ecc)
> _enable_encode*r*()?

enable_encode is supposed to enable a encode operation.
but sure I am with with addressing the encoder - seems to make more sense.



>
>> +{
> Not sure this is needed right now, since the NAND driver is the only
> user of the ECC engine (not even sure you can use the ECC engine
> independently), and we do not support accessing chips in parallel, but
> it may be more future proof to take a lock before using the ECC
> encoder/decoder, and release it when the operation is finished.

Since it is not required per the current architecture (no parallel chip 
accesses) I do have doubts about it.

>
> This your controller seems capable of doing ECC encoding/decoding in
> parallel, it might be worth having 2 different locks.

I did double check with the design team and it can't.

>
> If you decide to not use any lock, please add something in the
> documentation, stating that it's the ECC engine user responsibility to
> ensure serialization, and forbid several mtk_ecc_get() on the same
> device.

ok.

>
>> +	mtk_ecc_encoder_idle(ecc);
>> +	writew(ENC_EN, ecc->regs + ECC_ENCCON);
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_enable_encode);
>> +
>> +void mtk_ecc_disable_encode(struct mtk_ecc *ecc)
> _disable_encode*r*()?

ok

>
>> +{
>> +	writew(0, ecc->regs + ECC_ENCIRQ_EN);
>> +	mtk_ecc_encoder_idle(ecc);
>> +	writew(ENC_DE, ecc->regs + ECC_ENCCON);
> Release the lock here, if any.
>
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_disable_encode);
>> +
>> +void mtk_ecc_enable_decode(struct mtk_ecc *ecc)
> _decode*r*()
>
>> +{
>> +	mtk_ecc_decoder_idle(ecc);
>> +	writel(DEC_EN, ecc->regs + ECC_DECCON);
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_enable_decode);
>> +
>> +void mtk_ecc_disable_decode(struct mtk_ecc *ecc)
> Ditto.
>
>> +{
>> +	writew(0, ecc->regs + ECC_DECIRQ_EN);
>> +	mtk_ecc_decoder_idle(ecc);
>> +	writel(DEC_DE, ecc->regs + ECC_DECCON);
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_disable_decode);
>> +
>> +void mtk_ecc_start_decode(struct mtk_ecc *ecc, int sectors)
> AFAIS, you're not really starting the decoding here. It's triggered by
> the NAND engine when it forwards data to the ECC engine.

yes

> Can we make this clearer by rename the function into
> mtk_ecc_prepare_decoding(). I know I'm quite picky on those function
> names, but I like when names are reflecting what's really done :).

sure makes sense. ok.

>
>> +{
>> +	ecc->sec_mask = 1 << (sectors - 1);
>> +	init_completion(&ecc->done);
>> +	writew(DEC_IRQEN, ecc->regs + ECC_DECIRQ_EN);
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_start_decode);
>> +
>> +int mtk_ecc_wait_decode(struct mtk_ecc *ecc)
>> +{
>> +	int ret;
>> +
>> +	ret = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
>> +	if (!ret) {
>> +		dev_err(ecc->dev, "decode timeout\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_wait_decode);
>> +
>> +int mtk_ecc_start_encode(struct mtk_ecc *ecc, struct mtk_ecc_enc_data *d)
> And here you're doing more than just preparing the encoding request,
> you're actually encoding data. So, how about mtd_ecc_encode()?

works for me :) ok

>
>> +{
>> +	dma_addr_t addr;
>> +	u32 *p, len;
>> +	u32 reg, i;
>> +	int rc, ret = 0;
>> +
>> +	addr = dma_map_single(ecc->dev, d->data, d->len, DMA_TO_DEVICE);
>> +	rc = dma_mapping_error(ecc->dev, addr);
>> +	if (rc) {
>> +		dev_err(ecc->dev, "dma mapping error\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* enable the encoder in DMA mode to calculate the ECC bytes  */
>> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
>> +	reg |= ECC_DMA_MODE;
> When I compare the start_encode() to the start_decode() function I see
> one big difference: the former is clearly operating in non-pipeline
> mode (the data are retrieved using DMA and returned values are kept in
> PARX registers), while, IFAICS, the latter is operating in pipeline
> mode (data are directly retrieved from the NAND engine stream).

yes

>
> I'm perfectly fine supporting those 2 modes (at least it gives an
> answer to one of my question: the ECC engine can be used independently
> of the NAND engine), but the function names should reflect this.

sure: ecc_prepare_decoder() and ecc_encode().

>
>> +	writel(reg, ecc->regs + ECC_ENCCNFG);
>> +
>> +	writel(ENC_IRQEN, ecc->regs + ECC_ENCIRQ_EN);
>> +	writel(lower_32_bits(addr), ecc->regs + ECC_ENCDIADDR);
>> +
>> +	init_completion(&ecc->done);
>> +	writew(ENC_EN, ecc->regs + ECC_ENCCON);
>> +
>> +	rc = wait_for_completion_timeout(&ecc->done, msecs_to_jiffies(500));
>> +	if (!rc) {
>> +		dev_err(ecc->dev, "encode timeout\n");
>> +		writel(0, ecc->regs + ECC_ENCIRQ_EN);
>> +		ret = -ETIMEDOUT;
>> +		goto timeout;
>> +	}
>> +
>> +	mtk_ecc_encoder_idle(ecc);
>> +
>> +	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
>> +	len = (d->strength * ECC_PARITY_BITS + 7) >> 3;
>> +	p = (u32 *) (d->data + d->len);
>> +
>> +	/* 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_ENCPAR0 + i * sizeof(u32));
> Please define an ECC_ENCPAR(x) macro, where X is the parity word index.

sure.
>
>> +
>> +timeout:
>> +
>> +	dma_unmap_single(ecc->dev, addr, d->len, DMA_TO_DEVICE);
>> +
>> +	writew(0, ecc->regs + ECC_ENCCON);
>> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
>> +	reg |= ECC_NFI_MODE;
>> +	writel(reg, ecc->regs + ECC_ENCCNFG);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(mtk_ecc_start_encode);
>> +
> [...]
>
>> diff --git a/drivers/mtd/nand/mtk_ecc.h b/drivers/mtd/nand/mtk_ecc.h
>> new file mode 100644
>> index 0000000..d12bc5f
>> --- /dev/null
>> +++ b/drivers/mtd/nand/mtk_ecc.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + * MTK SDG1 ECC controller
>> + *
>> + * Copyright (c) 2016 Mediatek
>> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
>> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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>
>> +
>> +struct device_node;
>> +struct mtk_ecc;
>> +
>> +/**
>> + * @len: number of bytes in the data buffer
>> + * @data: pointer to memory holding the data
>> + * @strength: number of correctable bits
>> + */
>> +struct mtk_ecc_enc_data {
> Should probably not be suffixed with _enc_, since the same structure
> could be used for decoding ops.

ok remove

>
>> +	unsigned int len;
>> +	int strength;
> You should already have this information (either stored in a copy of
> the config passed to mtk_ecc_config() or retrieved from the registers
> values).

ack


>
>> +	u8 *data;
>> +};
> This being said, I wonder if we really need a struct for that. Passing
> those two parameters to mtd_ecc_start_encode() is acceptable.

perfect. we will remove the structure and pass len and *data.


>
>> +
>> +struct mtk_ecc_stats {
>> +	u32 corrected;
>> +	u32 bitflips;
>> +	u32 failed;
>> +};
>> +
>> +struct mtk_ecc_config {
>> +	u32 strength;
>> +	u32 step_len;
>> +};
>> +
>> +void mtk_ecc_enable_decode(struct mtk_ecc *);
>> +void mtk_ecc_disable_decode(struct mtk_ecc *);
>> +
>> +int mtk_ecc_wait_decode(struct mtk_ecc *);
>> +void mtk_ecc_enable_encode(struct mtk_ecc *);
>> +void mtk_ecc_disable_encode(struct mtk_ecc *);
>> +int mtk_ecc_start_encode(struct mtk_ecc *, struct mtk_ecc_enc_data *);
>> +void mtk_ecc_hw_init(struct mtk_ecc *);
>> +int mtk_ecc_config(struct mtk_ecc *, struct mtk_ecc_config *);
>> +void mtk_ecc_release(struct mtk_ecc *);
>> +struct mtk_ecc *of_mtk_ecc_get(struct device_node *);
>> +
>> +void mtk_ecc_start_decode(struct mtk_ecc *, int sectors);
>> +void mtk_ecc_get_stats(struct mtk_ecc *, struct mtk_ecc_stats *, int sectors);
>> +#endif
>> diff --git a/drivers/mtd/nand/mtk_nand.c b/drivers/mtd/nand/mtk_nand.c
>> new file mode 100644
>> index 0000000..048024f
>> --- /dev/null
>> +++ b/drivers/mtd/nand/mtk_nand.c
>> @@ -0,0 +1,1266 @@
>> +/*
>> + * MTK NAND Flash controller driver.
>> + * Copyright (C) 2016 MediaTek Inc.
>> + * Authors:	Xiaolei Li		<xiaolei.li@mediatek.com>
>> + *		Jorge Ramirez-Ortiz	<jorge.ramirez-ortiz@linaro.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_28	(3)
>> +#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_FDM_REG_SIZE	(8)
>> +#define NFI_FDM0L		(0xA0)
>> +#define NFI_FDM0M		(0xA4)
>> +#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)
>> +
>> +struct mtk_nfc_clk {
>> +	struct clk *nfi_clk;
>> +	struct clk *pad_clk;
>> +};
>> +
>> +struct mtk_nfc_nand_chip {
>> +	struct list_head node;
>> +	struct nand_chip nand;
>> +	u32 spare_per_sector;
> We already discussed that on IRC, and I don't think you really need
> this field. AFAICT, all it's encoding is the number of ECC + free bytes
> you have per data chunk (ECC step size). This can all be described
> using the ecc->{pre,post}pad and ecc->bytes fields.

ack

>
>> +	int nsels;
>> +	u8 sels[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 = mtk_nand->spare_per_sector;
>> +	struct mtk_ecc_config config;
>> +
>> +	/* skip configuration when recognize NAND Flash */
>> +	if (!mtd->writesize)
>> +		return 0;
>> +
>> +	switch (mtd->writesize) {
>> +	case 512:
>> +		fmt = PAGEFMT_512_2K | PAGEFMT_SEC_SEL_512;
>> +		break;
>> +	case KB(2):
>> +		fmt = PAGEFMT_512_2K;
>> +		break;
>> +	case KB(4):
>> +		fmt = PAGEFMT_2K_4K;
>> +		break;
>> +	case KB(8):
>> +		fmt = PAGEFMT_4K_8K;
>> +		break;
>> +	default:
>> +		dev_err(nfc->dev, "invalid page len: %d\n", mtd->writesize);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (mtd->writesize > 512)
>> +		spare >>= 1;
>> +
>> +	switch (spare) {
>> +	case 16:
>> +		fmt |= (PAGEFMT_SPARE_16 << PAGEFMT_SPARE_SHIFT);
>> +		break;
>> +	case 28:
>> +		fmt |= (PAGEFMT_SPARE_28 << PAGEFMT_SPARE_SHIFT);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_SHIFT;
>> +	fmt |= NFI_FDM_REG_SIZE << PAGEFMT_FDM_ECC_SHIFT;
>> +	nfi_writew(nfc, fmt, NFI_PAGEFMT);
>> +
>> +	config.step_len = mtk_step_len(chip);
>> +	config.strength = chip->ecc.strength;
>> +	mtk_ecc_config(nfc->ecc, &config);
> If you follow my suggestion to add a lock to the ECC engine, then
> mtk_ecc_config() should only be called after you've acquired this lock
> (i.e. after calling mtk_ecc_enable_{encoder/decoder}()), and thus
> should be moved in your ecc->{read,write}_page() implementations.
thanks for pointing this out (I would have probably missed it otherwise).
I think we are going to skip the lock and rely on users to serialize the access 
since there can be no concurrent access to the ECC engine.

>
> If you want to avoid those conversions on each NAND operations, just
> store a copy of mtk_ecc_config into your mtk_nfc_nand_chip struct.
>
>> +
>> +	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;
>> +		nfi_writel(nfc, reg, NFI_CON);
>> +
>> +		/* trigger to fetch data */
>> +		nfi_writew(nfc, STAR_EN, NFI_STRDATA);
>> +
>> +		/* hardware issue work around:
>> +		 * The first byte of data may be wrong right after the trigger.
>> +		 * (The controller fetches data until the internal FIFO is full)
> If this is an erratum and is clearly identified in the datasheet, then
> you should mention the reference and quote the datasheet.
> It looks like a bad timing configuration to me, but I might be wrong.

yes you are right and thanks for raising this issue (we did confirm it with the 
controller designer)
we have fixed this in v4 by adding one additional nfi reset after the NFI_MASTER 
check.



>
>> +		 */
>> +		udelay(10);
>> +	}
>> +
>> +	mtk_nfc_wait_ioready(nfc);
>> +
>> +	return nfi_readb(nfc, NFI_DATAR);
>> +}
>> +
> [...]
>
>> +
>> +static int mtk_nfc_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	u8 *buf = chip->buffers->databuf;
>> +	int page, rc, i;
>> +
>> +	memset(buf, 0x00, mtd->writesize + mtd->oobsize);
>> +
>> +	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
>> +		ofs += mtd->erasesize - mtd->writesize;
>> +
>> +	i = 0;
>> +	do {
>> +		page = (int)(ofs >> chip->page_shift);
>> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
>> +		rc = mtk_nfc_write_page(mtd, chip, buf, 0, page, 1);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>> +		rc = chip->waitfunc(mtd, chip);
>> +		rc = rc & NAND_STATUS_FAIL ? -EIO : 0;
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		ofs += mtd->writesize;
>> +		i++;
>> +
>> +	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
>> +
>> +	return 0;
>> +}
> Why do you need this custom implementation?

the reason it our page layout: if we need to mark a bad block it is not possible 
for us to access the spare area (our layout is: sector + oob + sector + oob ...)
so we just mark the whole page.

is this acceptable?


>
> [...]
>
>> +
>> +static int mtk_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> +				int page)
>> +{
>> +	u8 *buf = chip->buffers->databuf;
>> +	struct mtd_ecc_stats stats;
>> +	int ret;
>> +
>> +	stats = mtd->ecc_stats;
>> +
>> +	memset(buf, 0xff, mtd->writesize);
>> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>> +
>> +	ret = mtk_nfc_read_page_hwecc(mtd, chip, buf, 1, page);
>> +
>> +	/* mark as invalid data 0x00 if UECC happens */
>> +	if ((mtd->ecc_stats.failed - stats.failed) > 0)
>> +		memset(chip->oob_poi, 0, mtd->oobsize);
> No, please leave the data as is. That's really useful to debug things.

ok

>
>> +
>> +	if (ret < mtd->bitflip_threshold)
>> +		mtd->ecc_stats.corrected = stats.corrected;
> Hm, ->corrected has already been updated by mtk_nfc_read_page_hwecc(),
> or am I missing something?
> Moreover, you should have mtd->ecc_stats.corrected += stats.corrected.

sorry, yes it might be a bit convoluted due to the inverted logic.
unless the number of bitflips returned by the read_page_hwecc is equal or over 
the threshold we don't update the corrected stats (so we just reset it to the 
previous value at this point, hence the = instead of the +=).

please let me know if it is not clear.

>
>> +
>> +	return ret;
>> +}
>> +
> [...]
>
>
>> +
>> +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);
>> +
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	oob_region->length = NFI_FDM_REG_SIZE * chip->ecc.steps;
> Is NFI_FDM_REG_SIZE really fixed to 8, or does it depend on the
> spare_per_sector and ecc->bytes information?

Its value can range between 0 and 8 and it depends on the spare area size and 
the ecc level.
each sector spare is: FDM + ECC parity + dummy (if there is dummy, the 
controller pads it)
ECC parity = 14 * ecc level (bits)

So we could do:

FDM size = spare size - (14 * ecc level + 7) / 8;
if (FDM size > 8)
FDM size = 8;


for SLC and MLC nand we do fix it to 8 in all cases.
for TLC nand, since we have less spare, we can only use 3 bytes.

do you think it is worth adding the FDM size as a function?

>
>> +	oob_region->offset = 0;
>> +
>> +	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;
>> +	}
>> +
>> +	if (of_property_read_u32(np, "spare_per_sector",
>> +						&chip->spare_per_sector)) {
> As already discussed on IRC, this should be deduced from the NAND page
> info retrieved in nand_scan_ident().

ack.

>
>> +		dev_err(dev, "missing spare_per_sector property in DT\n");
>> +		return -ENODEV;
>> +
>> +	}
>> +
>> +	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->block_markbad = mtk_nfc_block_markbad;
>> +	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_raw;
>> +	nand->ecc.write_oob = mtk_nfc_write_oob;
>> +
>> +	nand->ecc.read_subpage = mtk_nfc_read_subpage_hwecc;
>> +	nand->ecc.read_page_raw = mtk_nfc_read_page_raw;
>> +	nand->ecc.read_oob_raw = mtk_nfc_read_oob_raw;
>> +	nand->ecc.read_page = mtk_nfc_read_page_hwecc;
>> +	nand->ecc.read_oob = mtk_nfc_read_oob;
>> +
>> +	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;
>> +
>> +	/* TODO: add NAND_ECC_SOFT */
>> +	if (nand->ecc.mode != NAND_ECC_HW) {
>> +		dev_err(dev, "driver only supports NAND_ECC_HW\n");
>> +		return -ENODEV;
>> +	}
> Do you really support all kind of NANDs (even small and very large pages
> NANDs). IMHO, it's safer to do a check after nand_scan_ident(), to
> verify that your implementation support the chip requirements...

ack.

>
>> +
>> +	ret = nand_scan_tail(mtd);
>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	len = mtd->writesize + mtd->oobsize;
>> +	nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL);
>> +	if (!nfc->buffer)
>> +		return  -ENOMEM;
>> +
>> +	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;
>> +}
>> +
> I tried to carefully review most of the driver, but I may have missed a
> few things. Richard, Brian, if you have some time, could you please
> have look?
>
> Thanks,
>
> Boris
>

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

[Boris Brezillon]

Hi Jorge,

On Tue, 19 Apr 2016 10:26:55 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:

> >> +static irqreturn_t mtk_ecc_irq(int irq, void *id)
> >> +{
> >> +	struct mtk_ecc *ecc = id;
> >> +	u32 dec, enc;
> >> +
> >> +	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
> >> +	enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
> >> +
> >> +	if (!(dec || enc))
> >> +		return IRQ_NONE;
> >> +
> >> +	if (dec) {
> >> +		dec = readw(ecc->regs + ECC_DECDONE);
> >> +		if (dec & ecc->sec_mask) {
> >> +			ecc->sec_mask = 0;
> >> +			complete(&ecc->done);
> > If you can really do enc and dec in parallel, then you should have two
> > waitqueues.
> 
> 
> unfortunately no, we can't do parallel operations.

That's not a problem, as long a you get exclusive access to the engine
when launching an operation.

Given this new input, I'd suggest reworking a bit the ECC interface.
How about providing an mtk_ecc_enable() function where you'll pass the
direction, or even better, the config which would contain the direction
(encoding or decoding), the ECC strength and step-size (maybe we
should also pass the ECC_MODE: NFI, DMA or whatever).

Similarly to the mtk_ecc_enable() function you would have an
mtk_ecc_disable() which would disable everything.

[...]
> >
> >> +{
> > Not sure this is needed right now, since the NAND driver is the only
> > user of the ECC engine (not even sure you can use the ECC engine
> > independently), and we do not support accessing chips in parallel, but
> > it may be more future proof to take a lock before using the ECC
> > encoder/decoder, and release it when the operation is finished.
> 
> Since it is not required per the current architecture (no parallel chip 
> accesses) I do have doubts about it.

Hm, given that your engine can possibly be used outside of the NAND
pipeline, I'd recommend introducing a lock and using it.
BTW, I see ECC_NFI_MODE (where NFI probably means Nand Flash
Interface) and ECC_DMA_MODE, but the ECC_ENC_MODE mask is (3 << 5). I'm
just curious, what are the other modes (if any)?

> 
> >
> > This your controller seems capable of doing ECC encoding/decoding in
> > parallel, it might be worth having 2 different locks.
> 
> I did double check with the design team and it can't.

Then having a single lock is fine.

> 
> >
> > If you decide to not use any lock, please add something in the
> > documentation, stating that it's the ECC engine user responsibility to
> > ensure serialization, and forbid several mtk_ecc_get() on the same
> > device.
> 
> ok.

Honestly, it wouldn't be a blocking aspect if you decide to skip the
locking part, but given that implementing it seems quite easy and more
future proof, I'd prefer if you do it.

> 
> >
> >> +{
> >> +	dma_addr_t addr;
> >> +	u32 *p, len;
> >> +	u32 reg, i;
> >> +	int rc, ret = 0;
> >> +
> >> +	addr = dma_map_single(ecc->dev, d->data, d->len, DMA_TO_DEVICE);
> >> +	rc = dma_mapping_error(ecc->dev, addr);
> >> +	if (rc) {
> >> +		dev_err(ecc->dev, "dma mapping error\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* enable the encoder in DMA mode to calculate the ECC bytes  */
> >> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
> >> +	reg |= ECC_DMA_MODE;
> > When I compare the start_encode() to the start_decode() function I see
> > one big difference: the former is clearly operating in non-pipeline
> > mode (the data are retrieved using DMA and returned values are kept in
> > PARX registers), while, IFAICS, the latter is operating in pipeline
> > mode (data are directly retrieved from the NAND engine stream).
> 
> yes
> 
> >
> > I'm perfectly fine supporting those 2 modes (at least it gives an
> > answer to one of my question: the ECC engine can be used independently
> > of the NAND engine), but the function names should reflect this.
> 
> sure: ecc_prepare_decoder() and ecc_encode().

Maybe something mode explicit, like mtd_ecc_prepare_pipeline().
BTW, I had a second look at your implementation, and I wonder why
you're not putting the operations done in the prepare function directly
in the enable one.



> >> +
> >> +static int mtk_nfc_block_markbad(struct mtd_info *mtd, loff_t ofs)
> >> +{
> >> +	struct nand_chip *chip = mtd_to_nand(mtd);
> >> +	u8 *buf = chip->buffers->databuf;
> >> +	int page, rc, i;
> >> +
> >> +	memset(buf, 0x00, mtd->writesize + mtd->oobsize);
> >> +
> >> +	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
> >> +		ofs += mtd->erasesize - mtd->writesize;
> >> +
> >> +	i = 0;
> >> +	do {
> >> +		page = (int)(ofs >> chip->page_shift);
> >> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> >> +		rc = mtk_nfc_write_page(mtd, chip, buf, 0, page, 1);
> >> +		if (rc < 0)
> >> +			return rc;
> >> +
> >> +		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> >> +		rc = chip->waitfunc(mtd, chip);
> >> +		rc = rc & NAND_STATUS_FAIL ? -EIO : 0;
> >> +		if (rc < 0)
> >> +			return rc;
> >> +
> >> +		ofs += mtd->writesize;
> >> +		i++;
> >> +
> >> +	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
> >> +
> >> +	return 0;
> >> +}
> > Why do you need this custom implementation?
> 
> the reason it our page layout: if we need to mark a bad block it is not possible 
> for us to access the spare area (our layout is: sector + oob + sector + oob ...)
> so we just mark the whole page.
> 
> is this acceptable?

Oh, this means you're loosing the BBM as soon as you start using the
NAND. Don't you have a mechanism switching one or 2 bytes of in-band
with OOB data, so that the BBM remains intact and you can still mark
them as bad when needed?
I'd strongly suggest to do this (I know other drivers, like the GPMI
one, use the same trick).

BTW, how do you detect bad blocks marked by the NAND vendor. Those BBMs
will be in what you consider your in-band region.


> >
> >> +
> >> +	if (ret < mtd->bitflip_threshold)
> >> +		mtd->ecc_stats.corrected = stats.corrected;
> > Hm, ->corrected has already been updated by mtk_nfc_read_page_hwecc(),
> > or am I missing something?
> > Moreover, you should have mtd->ecc_stats.corrected += stats.corrected.
> 
> sorry, yes it might be a bit convoluted due to the inverted logic.
> unless the number of bitflips returned by the read_page_hwecc is equal or over 
> the threshold we don't update the corrected stats (so we just reset it to the 
> previous value at this point, hence the = instead of the +=).

What's the point? I mean, ->corrected should be updated each time you
read a page, even if the number of bitflips does not exceed the
bitflips_threshold.


> >> +
> >> +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);
> >> +
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	oob_region->length = NFI_FDM_REG_SIZE * chip->ecc.steps;
> > Is NFI_FDM_REG_SIZE really fixed to 8, or does it depend on the
> > spare_per_sector and ecc->bytes information?
> 
> Its value can range between 0 and 8 and it depends on the spare area size and 
> the ecc level.
> each sector spare is: FDM + ECC parity + dummy (if there is dummy, the 
> controller pads it)
> ECC parity = 14 * ecc level (bits)
> 
> So we could do:
> 
> FDM size = spare size - (14 * ecc level + 7) / 8;
> if (FDM size > 8)
> FDM size = 8;

Looks good to me.

> 
> 
> for SLC and MLC nand we do fix it to 8 in all cases.
> for TLC nand, since we have less spare, we can only use 3 bytes.
> 
> do you think it is worth adding the FDM size as a function?

Not necessarily a function, it can be a field somewhere in your private
mtk_nand_chip struct.


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

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

[Jorge Ramirez-Ortiz]

On 04/19/2016 12:42 PM, Boris Brezillon wrote:
> Hi Jorge,
>
> On Tue, 19 Apr 2016 10:26:55 -0400
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:
>
>>>> +static irqreturn_t mtk_ecc_irq(int irq, void *id)
>>>> +{
>>>> +	struct mtk_ecc *ecc = id;
>>>> +	u32 dec, enc;
>>>> +
>>>> +	dec = readw(ecc->regs + ECC_DECIRQ_STA) & DEC_IRQEN;
>>>> +	enc = readl(ecc->regs + ECC_ENCIRQ_STA) & ENC_IRQEN;
>>>> +
>>>> +	if (!(dec || enc))
>>>> +		return IRQ_NONE;
>>>> +
>>>> +	if (dec) {
>>>> +		dec = readw(ecc->regs + ECC_DECDONE);
>>>> +		if (dec & ecc->sec_mask) {
>>>> +			ecc->sec_mask = 0;
>>>> +			complete(&ecc->done);
>>> If you can really do enc and dec in parallel, then you should have two
>>> waitqueues.
>>
>> unfortunately no, we can't do parallel operations.
> That's not a problem, as long a you get exclusive access to the engine
> when launching an operation.
>
> Given this new input, I'd suggest reworking a bit the ECC interface.
> How about providing an mtk_ecc_enable() function where you'll pass the
> direction, or even better, the config which would contain the direction
> (encoding or decoding), the ECC strength and step-size (maybe we
> should also pass the ECC_MODE: NFI, DMA or whatever).

OK. that is simple enough (having said that, the mtk_ecc_irq will not change)


>
> Similarly to the mtk_ecc_enable() function you would have an
> mtk_ecc_disable() which would disable everything.

ok

>
> [...]
>>>> +{
>>> Not sure this is needed right now, since the NAND driver is the only
>>> user of the ECC engine (not even sure you can use the ECC engine
>>> independently), and we do not support accessing chips in parallel, but
>>> it may be more future proof to take a lock before using the ECC
>>> encoder/decoder, and release it when the operation is finished.
>> Since it is not required per the current architecture (no parallel chip
>> accesses) I do have doubts about it.
> Hm, given that your engine can possibly be used outside of the NAND
> pipeline, I'd recommend introducing a lock and using it.

I dont think it will ever will but I'll do the lock (just like jz4780_bch.c does)

> BTW, I see ECC_NFI_MODE (where NFI probably means Nand Flash
> Interface) and ECC_DMA_MODE, but the ECC_ENC_MODE mask is (3 << 5). I'm
> just curious, what are the other modes (if any)?

b11: reserved mode
b10: PIO mode
b01: NFI mode
b00: DMA mode


>
>>> This your controller seems capable of doing ECC encoding/decoding in
>>> parallel, it might be worth having 2 different locks.
>> I did double check with the design team and it can't.
> Then having a single lock is fine.
>
>>> If you decide to not use any lock, please add something in the
>>> documentation, stating that it's the ECC engine user responsibility to
>>> ensure serialization, and forbid several mtk_ecc_get() on the same
>>> device.
>> ok.
> Honestly, it wouldn't be a blocking aspect if you decide to skip the
> locking part, but given that implementing it seems quite easy and more
> future proof, I'd prefer if you do it.

ok.

>
>>>> +{
>>>> +	dma_addr_t addr;
>>>> +	u32 *p, len;
>>>> +	u32 reg, i;
>>>> +	int rc, ret = 0;
>>>> +
>>>> +	addr = dma_map_single(ecc->dev, d->data, d->len, DMA_TO_DEVICE);
>>>> +	rc = dma_mapping_error(ecc->dev, addr);
>>>> +	if (rc) {
>>>> +		dev_err(ecc->dev, "dma mapping error\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* enable the encoder in DMA mode to calculate the ECC bytes  */
>>>> +	reg = readl(ecc->regs + ECC_ENCCNFG) & ~ECC_ENC_MODE_MASK;
>>>> +	reg |= ECC_DMA_MODE;
>>> When I compare the start_encode() to the start_decode() function I see
>>> one big difference: the former is clearly operating in non-pipeline
>>> mode (the data are retrieved using DMA and returned values are kept in
>>> PARX registers), while, IFAICS, the latter is operating in pipeline
>>> mode (data are directly retrieved from the NAND engine stream).
>> yes
>>
>>> I'm perfectly fine supporting those 2 modes (at least it gives an
>>> answer to one of my question: the ECC engine can be used independently
>>> of the NAND engine), but the function names should reflect this.
>> sure: ecc_prepare_decoder() and ecc_encode().
> Maybe something mode explicit, like mtd_ecc_prepare_pipeline().

what is wrong with prepare decoder?

> BTW, I had a second look at your implementation, and I wonder why
> you're not putting the operations done in the prepare function directly
> in the enable one.

No reason - this was the sequence of register writes provided by the design team 
so we kept them this way.
I'll try to reorganize.

>
>
>
>>>> +
>>>> +static int mtk_nfc_block_markbad(struct mtd_info *mtd, loff_t ofs)
>>>> +{
>>>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>>>> +	u8 *buf = chip->buffers->databuf;
>>>> +	int page, rc, i;
>>>> +
>>>> +	memset(buf, 0x00, mtd->writesize + mtd->oobsize);
>>>> +
>>>> +	if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
>>>> +		ofs += mtd->erasesize - mtd->writesize;
>>>> +
>>>> +	i = 0;
>>>> +	do {
>>>> +		page = (int)(ofs >> chip->page_shift);
>>>> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
>>>> +		rc = mtk_nfc_write_page(mtd, chip, buf, 0, page, 1);
>>>> +		if (rc < 0)
>>>> +			return rc;
>>>> +
>>>> +		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>>>> +		rc = chip->waitfunc(mtd, chip);
>>>> +		rc = rc & NAND_STATUS_FAIL ? -EIO : 0;
>>>> +		if (rc < 0)
>>>> +			return rc;
>>>> +
>>>> +		ofs += mtd->writesize;
>>>> +		i++;
>>>> +
>>>> +	} while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
>>>> +
>>>> +	return 0;
>>>> +}
>>> Why do you need this custom implementation?
>> the reason it our page layout: if we need to mark a bad block it is not possible
>> for us to access the spare area (our layout is: sector + oob + sector + oob ...)
>> so we just mark the whole page.
>>
>> is this acceptable?
> Oh, this means you're loosing the BBM as soon as you start using the
> NAND. Don't you have a mechanism switching one or 2 bytes of in-band
> with OOB data, so that the BBM remains intact and you can still mark
> them as bad when needed?
> I'd strongly suggest to do this (I know other drivers, like the GPMI
> one, use the same trick).
>
> BTW, how do you detect bad blocks marked by the NAND vendor. Those BBMs
> will be in what you consider your in-band region.

let me come back to you on this. I realize it is important.

>
>
>>>> +
>>>> +	if (ret < mtd->bitflip_threshold)
>>>> +		mtd->ecc_stats.corrected = stats.corrected;
>>> Hm, ->corrected has already been updated by mtk_nfc_read_page_hwecc(),
>>> or am I missing something?
>>> Moreover, you should have mtd->ecc_stats.corrected += stats.corrected.
>> sorry, yes it might be a bit convoluted due to the inverted logic.
>> unless the number of bitflips returned by the read_page_hwecc is equal or over
>> the threshold we don't update the corrected stats (so we just reset it to the
>> previous value at this point, hence the = instead of the +=).
> What's the point? I mean, ->corrected should be updated each time you
> read a page, even if the number of bitflips does not exceed the
> bitflips_threshold.

ack

>
>
>>>> +
>>>> +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);
>>>> +
>>>> +	if (section)
>>>> +		return -ERANGE;
>>>> +
>>>> +	oob_region->length = NFI_FDM_REG_SIZE * chip->ecc.steps;
>>> Is NFI_FDM_REG_SIZE really fixed to 8, or does it depend on the
>>> spare_per_sector and ecc->bytes information?
>> Its value can range between 0 and 8 and it depends on the spare area size and
>> the ecc level.
>> each sector spare is: FDM + ECC parity + dummy (if there is dummy, the
>> controller pads it)
>> ECC parity = 14 * ecc level (bits)
>>
>> So we could do:
>>
>> FDM size = spare size - (14 * ecc level + 7) / 8;
>> if (FDM size > 8)
>> FDM size = 8;
> Looks good to me.

ok

>
>>
>> for SLC and MLC nand we do fix it to 8 in all cases.
>> for TLC nand, since we have less spare, we can only use 3 bytes.
>>
>> do you think it is worth adding the FDM size as a function?
> Not necessarily a function, it can be a field somewhere in your private
> mtk_nand_chip struct.

ok.

>
>

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

[Boris Brezillon]

On Mon, 11 Apr 2016 12:56:12 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.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@linaro.org>
> ---

[...]

> +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;
> +	}
> +
> +	if (of_property_read_u32(np, "spare_per_sector",
> +						&chip->spare_per_sector)) {
> +		dev_err(dev, "missing spare_per_sector property in DT\n");
> +		return -ENODEV;
> +
> +	}
> +
> +	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->block_markbad = mtk_nfc_block_markbad;
> +	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_raw;
> +	nand->ecc.write_oob = mtk_nfc_write_oob;
> +
> +	nand->ecc.read_subpage = mtk_nfc_read_subpage_hwecc;
> +	nand->ecc.read_page_raw = mtk_nfc_read_page_raw;
> +	nand->ecc.read_oob_raw = mtk_nfc_read_oob_raw;
> +	nand->ecc.read_page = mtk_nfc_read_page_hwecc;
> +	nand->ecc.read_oob = mtk_nfc_read_oob;
> +
> +	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;
> +
> +	/* TODO: add NAND_ECC_SOFT */
> +	if (nand->ecc.mode != NAND_ECC_HW) {
> +		dev_err(dev, "driver only supports NAND_ECC_HW\n");
> +		return -ENODEV;
> +	}

You also don't check the ecc->strength and ecc->step_size values, and
I'd recommend doing the ECC initialization in a separate function.
Moreover, as already explained on IRC, you should not make the
nand-ecc-strength and nand-ecc-step-size properties mandatory. Usually
those information are exposed by the NAND chip itself, and are
available in chip->ecc_strength_ds and chip->ecc_step_size_ds.
nand-ecc-strength and nand-ecc-step-size are only here to override the
ECC config (a typical usage is to override those information to match
the configuration hardcoded in the bootloader).

Here is an example of how it could be done [1].

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1188


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

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

[Boris Brezillon]

On Mon, 11 Apr 2016 12:56:12 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:


> +static int mtk_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +				int page)
> +{
> +	u8 *buf = chip->buffers->databuf;

You're touching ->databuf here, and the core uses this buffer for its
internal cache. If want to use this buffer you have to invalidate the
cache by setting chip->pagebuf to -1 first.

> +	struct mtd_ecc_stats stats;
> +	int ret;
> +
> +	stats = mtd->ecc_stats;
> +
> +	memset(buf, 0xff, mtd->writesize);
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +
> +	ret = mtk_nfc_read_page_hwecc(mtd, chip, buf, 1, page);
> +
> +	/* mark as invalid data 0x00 if UECC happens */
> +	if ((mtd->ecc_stats.failed - stats.failed) > 0)
> +		memset(chip->oob_poi, 0, mtd->oobsize);
> +
> +	if (ret < mtd->bitflip_threshold)
> +		mtd->ecc_stats.corrected = stats.corrected;
> +
> +	return ret;
> +}


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

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

[Jorge Ramirez-Ortiz]

On 04/19/2016 04:24 PM, Boris Brezillon wrote:
> On Mon, 11 Apr 2016 12:56:12 -0400
> Jorge Ramirez-Ortiz<jorge.ramirez-ortiz@linaro.org>  wrote:
>
>
>> >+static int mtk_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>> >+				int page)
>> >+{
>> >+	u8 *buf = chip->buffers->databuf;
> You're touching ->databuf here, and the core uses this buffer for its
> internal cache. If want to use this buffer you have to invalidate the
> cache by setting chip->pagebuf to -1 first.
>

ack. we will use our private buffer instead as discussed on IRC.

  static int mtk_nfc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
                                 int page)
  {
-   u8 *data = chip->buffers->databuf;
+ struct mtk_nfc *nfc = nand_get_controller_data(chip);
         int ret;

-   memset(data, 0xff, mtd->writesize);
+ memset(nfc->buffer, 0xff, mtd->writesize);

         chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);

-   ret = mtk_nfc_write_page_hwecc(mtd, chip, data, 1, page);
+ ret = mtk_nfc_write_page_hwecc(mtd, chip, nfc->buffer, 1, page);
         if (ret < 0)
                 return -EIO;