[RFC PATCH v2 0/2] mtd: rawnand: meson: add Amlogic NAND driver support

[RFC PATCH v2 0/2] mtd: rawnand: meson: add Amlogic NAND driver support

[Yixun Lan]

  These two patches try to add initial NAND driver support for Amlogic Meson
SoCs, current it has been tested on GXL(p212) and AXG(s400) platform.

  Note this patch series actually depend on the eMMC clkc patch[2] which
still not merged.

Changes since v1 at [1]:
   - adopt property amlogic,nand-enable-scrambler - thanks Martin
   - drop nand pins in DT
   - convert clk access to emmc clkc model 
   - fix regiser field definition alignment
   - convert devm_kmalloc to kmalloc API
   - drop nand-user-mode 
   - parse cs id from DT
   - rework n2m, m2n function
   - explain why insert two "IDLE" command
   - implement exec_op()
   - drop meson_nfc_get_nand_chip_dts()
   - release resource once error occur in meson_nfc_nand_chips_init(), 
   - call nand_cleanup(nand) once mtd_device_register fail

Items not addressed in this version:
   - convert to ECC conf helper() 
   - how to construct mtd->name

[1] https://lkml.kernel.org/r/20180613161314.14894-1-yixun.lan@amlogic.com
[2] https://lkml.kernel.org/r/20180712211244.11428-1-yixun.lan@amlogic.com

Liang Yang (1):
  dt-bindings: nand: meson: add Amlogic NAND controller driver

Yixun Lan (1):
  mtd: rawnand: meson: add support for Amlogic NAND flash controller

 .../bindings/mtd/amlogic,meson-nand.txt       |   95 ++
 drivers/mtd/nand/raw/Kconfig                  |   10 +
 drivers/mtd/nand/raw/Makefile                 |    1 +
 drivers/mtd/nand/raw/meson_nand.c             | 1333 +++++++++++++++++
 4 files changed, 1439 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
 create mode 100644 drivers/mtd/nand/raw/meson_nand.c

-- 
2.18.0

[RFC PATCH v2 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver

[Yixun Lan]

From: Liang Yang <liang.yang@amlogic.com>

Add Amlogic NAND controller dt-bindings for Meson SoC,
Current this driver support GXBB/GXL/AXG platform.

Signed-off-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 .../bindings/mtd/amlogic,meson-nand.txt       | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
new file mode 100644
index 000000000000..31f910dcd27a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
@@ -0,0 +1,95 @@
+Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
+
+This file documents the properties in addition to those available in
+the MTD NAND bindings.
+
+Required properties:
+- compatible : contains one of:
+  - "amlogic,meson-gxl-nfc"
+  - "amlogic,meson-axg-nfc"
+- clocks     :
+	A list of phandle + clock-specifier pairs for the clocks listed
+	in clock-names.
+
+- clock-names: Should contain the following:
+	"core" - NFC module gate clock
+	"device" - device clock from eMMC sub clock controller
+
+- pins     : Select pins which NFC need.
+- nand_pins: Detail NAND pins information.
+- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
+				controller port C
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+Optional properties:
+- amlogic,nand-enable-scrambler: enable the NAND scrambler feature.
+	- (absent) = scrambler is disabled
+	- (present) = scrambler is enabled
+
+
+Other properties:
+see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
+
+Example demonstrate on AXG SoC:
+
+	sd_emmc_c_clkc: mmc@7000 {
+		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
+		reg = <0x0 0x7000 0x0 0x800>;
+		status = "okay";
+	};
+
+	nand: nfc@7800 {
+		compatible = "amlogic,meson-axg-nfc";
+		reg = <0x0 0x7800 0x0 0x100>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
+		status = "disabled";
+
+		clocks = <&clkc CLKID_SD_EMMC_C>,
+			<&sd_emmc_c_clkc CLKID_MMC_DIV>;
+		clock-names = "core", "device";
+		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
+
+		status = "okay";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&nand_pins>;
+
+		nand@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			nand-on-flash-bbt;
+			nand-ecc-mode = "hw";
+			nand-ecc-strength = <8>;
+			nand-ecc-step-size = <1024>;
+
+			amlogic,nand-enable-scrambler;
+
+			partition@0 {
+				label = "boot";
+				reg = <0x00000000 0x00200000>;
+				read-only;
+			};
+			partition@200000 {
+				label = "env";
+				reg = <0x00200000 0x00400000>;
+			};
+			partition@600000 {
+				label = "system";
+				reg = <0x00600000 0x00a00000>;
+			};
+			partition@1000000 {
+				label = "rootfs";
+				reg = <0x01000000 0x03000000>;
+			};
+			partition@4000000 {
+				label = "media";
+				reg = <0x04000000 0x8000000>;
+			};
+		};
+	};
-- 
2.18.0

Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver

[Boris Brezillon]

On Thu, 19 Jul 2018 17:46:11 +0800
Yixun Lan <yixun.lan@amlogic.com> wrote:

> From: Liang Yang <liang.yang@amlogic.com>
> 
> Add Amlogic NAND controller dt-bindings for Meson SoC,
> Current this driver support GXBB/GXL/AXG platform.
> 
> Signed-off-by: Liang Yang <liang.yang@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  .../bindings/mtd/amlogic,meson-nand.txt       | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> new file mode 100644
> index 000000000000..31f910dcd27a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
> @@ -0,0 +1,95 @@
> +Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
> +
> +This file documents the properties in addition to those available in
> +the MTD NAND bindings.
> +
> +Required properties:
> +- compatible : contains one of:
> +  - "amlogic,meson-gxl-nfc"
> +  - "amlogic,meson-axg-nfc"
> +- clocks     :
> +	A list of phandle + clock-specifier pairs for the clocks listed
> +	in clock-names.
> +
> +- clock-names: Should contain the following:
> +	"core" - NFC module gate clock
> +	"device" - device clock from eMMC sub clock controller
> +
> +- pins     : Select pins which NFC need.
> +- nand_pins: Detail NAND pins information.

You should document pinctrl-0 and pinctrl-names, not pins and nand_pins.

> +- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
> +				controller port C

Are you sure this is still needed, even after exposing MMC/NAND clks
through the CCF?

You forgot
- #address-cells
- #size-cells
- reg
- interrupts
- 

> +
> +Optional children nodes:
> +Children nodes represent the available nand chips.
> +
> +Optional properties:
> +- amlogic,nand-enable-scrambler: enable the NAND scrambler feature.
> +	- (absent) = scrambler is disabled
> +	- (present) = scrambler is enabled

I keep thinking this is not needed if you have the NAND chip properly
defined (NAND_NEED_SCRAMBLING flag set in chip->options).

> +
> +
> +Other properties:
> +see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
> +
> +Example demonstrate on AXG SoC:
> +
> +	sd_emmc_c_clkc: mmc@7000 {
> +		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
> +		reg = <0x0 0x7000 0x0 0x800>;
> +		status = "okay";
> +	};
> +
> +	nand: nfc@7800 {
> +		compatible = "amlogic,meson-axg-nfc";
> +		reg = <0x0 0x7800 0x0 0x100>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> +		status = "disabled";
> +
> +		clocks = <&clkc CLKID_SD_EMMC_C>,
> +			<&sd_emmc_c_clkc CLKID_MMC_DIV>;
> +		clock-names = "core", "device";
> +		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
> +
> +		status = "okay";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&nand_pins>;
> +
> +		nand@0 {
> +			reg = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			nand-on-flash-bbt;
> +			nand-ecc-mode = "hw";
> +			nand-ecc-strength = <8>;
> +			nand-ecc-step-size = <1024>;

I'd recommend not forcing a specific ECC config in the example.

> +
> +			amlogic,nand-enable-scrambler;
> +
> +			partition@0 {
> +				label = "boot";
> +				reg = <0x00000000 0x00200000>;
> +				read-only;
> +			};

Blank line here.

> +			partition@200000 {
> +				label = "env";
> +				reg = <0x00200000 0x00400000>;
> +			};
> +			partition@600000 {
> +				label = "system";
> +				reg = <0x00600000 0x00a00000>;
> +			};
> +			partition@1000000 {
> +				label = "rootfs";
> +				reg = <0x01000000 0x03000000>;
> +			};
> +			partition@4000000 {
> +				label = "media";
> +				reg = <0x04000000 0x8000000>;
> +			};
> +		};
> +	};

Re: [RFC PATCH v2 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver

[Yixun Lan]

Hi Boris:
 see my comments, thanks for the quick response

On 07/19/18 17:57, Boris Brezillon wrote:
> On Thu, 19 Jul 2018 17:46:11 +0800
> Yixun Lan <yixun.lan@amlogic.com> wrote:
> 
>> From: Liang Yang <liang.yang@amlogic.com>
>>
>> Add Amlogic NAND controller dt-bindings for Meson SoC,
>> Current this driver support GXBB/GXL/AXG platform.
>>
>> Signed-off-by: Liang Yang <liang.yang@amlogic.com>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  .../bindings/mtd/amlogic,meson-nand.txt       | 95 +++++++++++++++++++
>>  1 file changed, 95 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> new file mode 100644
>> index 000000000000..31f910dcd27a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.txt
>> @@ -0,0 +1,95 @@
>> +Amlogic NAND Flash Controller (NFC) for GXBB/GXL/AXG family SoCs
>> +
>> +This file documents the properties in addition to those available in
>> +the MTD NAND bindings.
>> +
>> +Required properties:
>> +- compatible : contains one of:
>> +  - "amlogic,meson-gxl-nfc"
>> +  - "amlogic,meson-axg-nfc"
>> +- clocks     :
>> +	A list of phandle + clock-specifier pairs for the clocks listed
>> +	in clock-names.
>> +
>> +- clock-names: Should contain the following:
>> +	"core" - NFC module gate clock
>> +	"device" - device clock from eMMC sub clock controller
>> +
>> +- pins     : Select pins which NFC need.
>> +- nand_pins: Detail NAND pins information.
> 
> You should document pinctrl-0 and pinctrl-names, not pins and nand_pins.
> 
Ok, will fix this

>> +- amlogic,mmc-syscon	: Required for NAND clocks, it's shared with SD/eMMC
>> +				controller port C
> 
> Are you sure this is still needed, even after exposing MMC/NAND clks
> through the CCF?
> 
yes, the SD_EMMC_CLOCK register from eMMC space explore a few other bit
that not fit well into clock model, thus we still need to access them
from NAND driver, we know it's kind of ugly..

#define SD_EMMC_CLOCK                   0x00
#define   CLK_ALWAYS_ON                 BIT(28)
#define   CLK_SELECT_NAND BIT(31)
#define   CLK_DIV_MASK                  GENMASK(5, 0)

we probably could get rid of CLK_DIV_MASK, but need to keep other two


> You forgot
> - #address-cells
> - #size-cells
> - reg
> - interrupts
> - 
> 
will fix these
>> +
>> +Optional children nodes:
>> +Children nodes represent the available nand chips.
>> +
>> +Optional properties:
>> +- amlogic,nand-enable-scrambler: enable the NAND scrambler feature.
>> +	- (absent) = scrambler is disabled
>> +	- (present) = scrambler is enabled
> 
> I keep thinking this is not needed if you have the NAND chip properly
> defined (NAND_NEED_SCRAMBLING flag set in chip->options).
> 
Ok, we will try this flag
>> +
>> +
>> +Other properties:
>> +see Documentation/devicetree/bindings/mtd/nand.txt for generic bindings.
>> +
>> +Example demonstrate on AXG SoC:
>> +
>> +	sd_emmc_c_clkc: mmc@7000 {
>> +		compatible = "amlogic,meson-axg-mmc-clkc", "syscon";
>> +		reg = <0x0 0x7000 0x0 0x800>;
>> +		status = "okay";
>> +	};
>> +
>> +	nand: nfc@7800 {
>> +		compatible = "amlogic,meson-axg-nfc";
>> +		reg = <0x0 0x7800 0x0 0x100>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
>> +		status = "disabled";
>> +
>> +		clocks = <&clkc CLKID_SD_EMMC_C>,
>> +			<&sd_emmc_c_clkc CLKID_MMC_DIV>;
>> +		clock-names = "core", "device";
>> +		amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
>> +
>> +		status = "okay";
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&nand_pins>;
>> +
>> +		nand@0 {
>> +			reg = <0>;
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +
>> +			nand-on-flash-bbt;
>> +			nand-ecc-mode = "hw";
>> +			nand-ecc-strength = <8>;
>> +			nand-ecc-step-size = <1024>;
> 
> I'd recommend not forcing a specific ECC config in the example.
> 
Ok, will fix

>> +
>> +			amlogic,nand-enable-scrambler;
>> +
>> +			partition@0 {
>> +				label = "boot";
>> +				reg = <0x00000000 0x00200000>;
>> +				read-only;
>> +			};
> 
> Blank line here.
will fix
> 
>> +			partition@200000 {
>> +				label = "env";
>> +				reg = <0x00200000 0x00400000>;
>> +			};
>> +			partition@600000 {
>> +				label = "system";
>> +				reg = <0x00600000 0x00a00000>;
>> +			};
>> +			partition@1000000 {
>> +				label = "rootfs";
>> +				reg = <0x01000000 0x03000000>;
>> +			};
>> +			partition@4000000 {
>> +				label = "media";
>> +				reg = <0x04000000 0x8000000>;
>> +			};
>> +		};
>> +	};
> 
> .
> 

[RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Yixun Lan]

Add initial support for the Amlogic NAND flash controller which found
in the Meson-GXBB/GXL/AXG SoCs.

Signed-off-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/mtd/nand/raw/Kconfig      |   10 +
 drivers/mtd/nand/raw/Makefile     |    1 +
 drivers/mtd/nand/raw/meson_nand.c | 1333 +++++++++++++++++++++++++++++
 3 files changed, 1344 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/meson_nand.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 6871ff0fd300..d4a72b258b44 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -530,4 +530,14 @@ config MTD_NAND_MTK
 	  Enables support for NAND controller on MTK SoCs.
 	  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
 
+config MTD_NAND_MESON
+	tristate "Support for NAND controller on Amlogic's Meson SoCs"
+	depends on ARCH_MESON || COMPILE_TEST
+	depends on COMMON_CLK_AMLOGIC
+	select COMMON_CLK_REGMAP_MESON
+	select MFD_SYSCON
+	help
+	  Enables support for NAND controller on Amlogic's Meson SoCs.
+	  This controller is found on Meson GXBB, GXL, AXG SoCs.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 165b7ef9e9a1..6e9101f7b855 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -56,6 +56,7 @@ 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_ecc.o mtk_nand.o
+obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_amd.o
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
new file mode 100644
index 000000000000..2458312f22fa
--- /dev/null
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -0,0 +1,1333 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson Nand Flash Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Liang Yang <liang.yang@amlogic.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/clk.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/iopoll.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#define NFC_REG_CMD		0x00
+#define   NFC_CMD_DRD		(0x8 << 14)
+#define   NFC_CMD_IDLE		(0xc << 14)
+#define   NFC_CMD_DWR		(0x4 << 14)
+#define   NFC_CMD_CLE		(0x5 << 14)
+#define   NFC_CMD_ALE		(0x6 << 14)
+#define   NFC_CMD_ADL		((0 << 16) | (3 << 20))
+#define   NFC_CMD_ADH		((1 << 16) | (3 << 20))
+#define   NFC_CMD_AIL		((2 << 16) | (3 << 20))
+#define   NFC_CMD_AIH		((3 << 16) | (3 << 20))
+#define   NFC_CMD_SEED		((8 << 16) | (3 << 20))
+#define   NFC_CMD_M2N		((0 << 17) | (2 << 20))
+#define   NFC_CMD_N2M		((1 << 17) | (2 << 20))
+#define   NFC_CMD_RB		BIT(20)
+#define   NFC_CMD_IO6		((0xb << 10) | (1 << 18))
+
+#define NFC_REG_CFG		0x04
+#define NFC_REG_DADR		0x08
+#define NFC_REG_IADR		0x0c
+#define NFC_REG_BUF		0x10
+#define NFC_REG_INFO		0x14
+#define NFC_REG_DC		0x18
+#define NFC_REG_ADR		0x1c
+#define NFC_REG_DL		0x20
+#define NFC_REG_DH		0x24
+#define NFC_REG_CADR		0x28
+#define NFC_REG_SADR		0x2c
+#define NFC_REG_PINS		0x30
+#define NFC_REG_VER		0x38
+
+#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages)	\
+	(								\
+		(cmd_dir)			|			\
+		((ran) << 19)			|			\
+		((bch) << 14)			|			\
+		((short_mode) << 13)		|			\
+		(((page_size) & 0x7f) << 6)	|			\
+		((pages) & 0x3f)					\
+	)
+
+#define GENCMDDADDRL(adl, addr)		((adl) | ((addr) & 0xffff))
+#define GENCMDDADDRH(adh, addr)		((adh) | (((addr) >> 16) & 0xffff))
+#define GENCMDIADDRL(ail, addr)		((ail) | ((addr) & 0xffff))
+#define GENCMDIADDRH(aih, addr)		((aih) | (((addr) >> 16) & 0xffff))
+
+#define RB_STA(x)			(1 << (26 + (x)))
+#define	DMA_DIR(dir)			((dir) ? NFC_CMD_N2M : NFC_CMD_M2N)
+
+#define NAND_TWB_TIME_CYCLE		10
+
+#define ECC_CHECK_RETURN_FF		(-1)
+
+#define NAND_CE0			(0xe << 10)
+#define NAND_CE1			(0xd << 10)
+
+#define DMA_BUSY_TIMEOUT		0x100000
+
+#define MAX_CE_NUM			2
+#define RAN_ENABLE			1
+
+/* eMMC clock register, misc control */
+#define SD_EMMC_CLOCK			0x00
+#define   CLK_ALWAYS_ON			BIT(28)
+#define   CLK_SELECT_NAND		BIT(31)
+#define   CLK_DIV_MASK			GENMASK(5, 0)
+
+#define NFC_CLK_CYCLE			6
+
+/* nand flash controller delay 3 ns */
+#define NFC_DEFAULT_DELAY		3000
+
+#define MAX_ECC_INDEX			10
+
+#define MUX_CLK_NUM_PARENTS		2
+
+#define ROW_ADDER(page, index)		(((page) >> (8 * (index))) & 0xff)
+#define MAX_CYCLE_ROW_ADDRS		3
+#define MAX_CYCLE_COLUMN_ADDRS		2
+#define DIRREAD				1
+#define DIRWRITE			0
+
+struct meson_nfc_info_format {
+	u16 info_bytes;
+
+	/* bit[5:0] are valid */
+	u8 zero_cnt;
+	struct ecc_sta {
+		u8 eccerr_cnt : 6;
+		u8 notused : 1;
+		u8 completed : 1;
+	} ecc;
+	u32 reserved;
+};
+
+#define	PER_INFO_BYTE	(sizeof(struct meson_nfc_info_format))
+
+struct meson_nfc_nand_chip {
+	struct list_head node;
+	struct nand_chip nand;
+	bool is_scramble;
+	int bch_mode;
+	int nsels;
+	u8 sels[0];
+};
+
+struct meson_nand_ecc {
+	int bch;
+	int strength;
+	int parity;
+};
+
+struct meson_nfc_data {
+	const struct meson_nand_ecc *ecc;
+	int ecc_num;
+	int bch_mode;
+	int short_bch;
+};
+
+struct meson_nfc_param {
+	int chip_select;
+	int rb_select;
+};
+
+struct nand_rw_cmd {
+	int cmd0;
+	int col[MAX_CYCLE_COLUMN_ADDRS];
+	int row[MAX_CYCLE_ROW_ADDRS];
+	int cmd1;
+};
+
+struct meson_nfc {
+	struct nand_hw_control controller;
+	struct clk *core_clk;
+	struct clk *device_clk;
+
+	struct device *dev;
+	void __iomem *reg_base;
+	struct regmap *reg_clk;
+
+	struct completion completion;
+	struct list_head chips;
+	struct meson_nfc_data *data;
+	struct meson_nfc_param param;
+	union {
+		int cmd[32];
+		struct nand_rw_cmd rw;
+	} cmdfifo;
+
+	dma_addr_t data_dma;
+	dma_addr_t info_dma;
+
+	u8 *data_buf;
+	u8 *info_buf;
+};
+
+enum {
+	NFC_ECC_NONE	= 0,
+	NFC_ECC_BCH8,		/* bch8 with ecc page size of 512B */
+	NFC_ECC_BCH8_1K,	/* bch8 with ecc page size of 1024B */
+	NFC_ECC_BCH24_1K,
+	NFC_ECC_BCH30_1K,
+	NFC_ECC_BCH40_1K,
+	NFC_ECC_BCH50_1K,
+	NFC_ECC_BCH60_1K,
+	NFC_ECC_BCH_SHORT,
+};
+
+#define MESON_ECC_DATA(b, s, p)				\
+	{ .bch = (b),	.strength = (s),	.parity = (p) }
+
+static const struct meson_nand_ecc meson_gxl_ecc[] = {
+	MESON_ECC_DATA(NFC_ECC_NONE, 0, 0),
+	MESON_ECC_DATA(NFC_ECC_BCH8, 8, 14),
+	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 14),
+	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 42),
+	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 54),
+	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 70),
+	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 88),
+	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 106),
+	MESON_ECC_DATA(NFC_ECC_BCH_SHORT, 0xff, 0xff),
+};
+
+static const struct meson_nand_ecc meson_axg_ecc[] = {
+	MESON_ECC_DATA(NFC_ECC_NONE, 0, 0),
+	MESON_ECC_DATA(NFC_ECC_BCH8, 8, 14),
+	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 14),
+	MESON_ECC_DATA(NFC_ECC_BCH_SHORT, 0xff, 0xff),
+};
+
+static inline struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
+{
+	return container_of(nand, struct meson_nfc_nand_chip, nand);
+}
+
+static void meson_nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct meson_nfc *nfc = nand_get_controller_data(nand);
+
+	if (chip < 0 || chip > MAX_CE_NUM)
+		return;
+
+	nfc->param.chip_select = meson_chip->sels[chip] ? NAND_CE1 : NAND_CE0;
+	nfc->param.rb_select = nfc->param.chip_select;
+}
+
+static inline void meson_nfc_cmd_idle(struct meson_nfc *nfc, u32 time)
+{
+	writel(nfc->param.chip_select | NFC_CMD_IDLE | (time & 0x3ff),
+	       nfc->reg_base + NFC_REG_CMD);
+}
+
+static inline void meson_nfc_cmd_seed(struct meson_nfc *nfc, u32 seed)
+{
+	writel(NFC_CMD_SEED | (0xc2 + (seed & 0x7fff)),
+	       nfc->reg_base + NFC_REG_CMD);
+}
+
+static void meson_nfc_cmd_access(struct meson_nfc *nfc,
+				 struct mtd_info *mtd, int raw, bool dir)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	u32 cmd, pagesize, pages, shortm = 0;
+	int bch = meson_chip->bch_mode;
+	int len = mtd->writesize;
+	int scramble = meson_chip->is_scramble ? 1 : 0;
+
+	pagesize = nand->ecc.size;
+
+	if (raw) {
+		bch = NAND_ECC_NONE;
+		len = mtd->writesize + mtd->oobsize;
+		cmd = (len & 0x3fff) | (scramble << 19) |
+			DMA_DIR(dir);
+		writel(cmd, nfc->reg_base + NFC_REG_CMD);
+		return;
+	}
+
+	pages = len / nand->ecc.size;
+
+	cmd = CMDRWGEN(DMA_DIR(dir), scramble, bch, shortm,
+		       pagesize, pages);
+
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+}
+
+static inline void meson_nfc_drain_cmd(struct meson_nfc *nfc)
+{
+	/*
+	 * Insert two commands to make sure all valid commands are finished.
+	 *
+	 * The Nand flash controller is designed as two stages pipleline -
+	 *  a) fetch and b) excute.
+	 * There might be cases when the driver see command queue is empty,
+	 * but the Nand flash controller still has two commands buffered,
+	 * one is fetched into NFC request queue (ready to run), and another
+	 * is actively executing. So pushing 2 "IDLE" commands guarantees that
+	 * the pipeline is emptied.
+	 */
+	meson_nfc_cmd_idle(nfc, 0);
+	meson_nfc_cmd_idle(nfc, 0);
+}
+
+static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc,
+				     unsigned int timeout_ms)
+{
+	u32 cmd_size = 0;
+	int ret;
+
+	/* wait cmd fifo is empty */
+	ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size,
+				 !((cmd_size >> 22) & 0x1f),
+				 10, timeout_ms * 1000);
+	if (ret)
+		dev_err(nfc->dev, "wait for empty cmd FIFO time out\n");
+
+	return ret;
+}
+
+static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc)
+{
+	meson_nfc_drain_cmd(nfc);
+
+	return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT);
+}
+
+static inline struct meson_nfc_info_format *nfc_info_ptr(struct meson_nfc *nfc,
+							 int index)
+{
+	return (struct meson_nfc_info_format *)&nfc->info_buf[index * 8];
+}
+
+static u8 *meson_nfc_oob_ptr(struct meson_nfc *nfc, struct mtd_info *mtd, int i)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	int len;
+
+	len = (nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i);
+
+	return nfc->data_buf + len;
+}
+
+static u8 *meson_nfc_data_ptr(struct meson_nfc *nfc,
+			      struct mtd_info *mtd, int i)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	int len;
+	int temp;
+
+	temp = nand->ecc.size + nand->ecc.bytes;
+	len = (temp + 2) * i;
+
+	return nfc->data_buf + len;
+}
+
+static void meson_nfc_prase_data_oob(struct meson_nfc *nfc,
+				     struct mtd_info *mtd, u8 *buf, u8 *oobbuf)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	int i, oob_len = 0;
+	u8 *dsrc, *osrc;
+
+	for (i = 0; i < mtd->writesize / nand->ecc.size; i++) {
+		if (buf) {
+			dsrc = meson_nfc_data_ptr(nfc, mtd, i);
+			memcpy(buf, dsrc, nand->ecc.size);
+			buf += nand->ecc.size;
+		}
+		oob_len = nand->ecc.bytes + 2;
+		osrc = meson_nfc_oob_ptr(nfc, mtd, i);
+		memcpy(oobbuf, osrc, oob_len);
+		oobbuf += oob_len;
+	}
+}
+
+static void
+meson_nfc_format_data_oob(struct meson_nfc *nfc,
+			  struct mtd_info *mtd, const u8 *buf, u8 *oobbuf)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	int i, oob_len = 0;
+	u8 *dsrc, *osrc;
+
+	for (i = 0; i < mtd->writesize / nand->ecc.size; i++) {
+		if (buf) {
+			dsrc = meson_nfc_data_ptr(nfc, mtd, i);
+			memcpy(dsrc, buf, nand->ecc.size);
+			buf += nand->ecc.size;
+		}
+		oob_len = nand->ecc.bytes + 2;
+		osrc = meson_nfc_oob_ptr(nfc, mtd, i);
+		memcpy(osrc, oobbuf, oob_len);
+		oobbuf += oob_len;
+	}
+}
+
+static int meson_nfc_queue_rb(struct meson_nfc *nfc)
+{
+	u32 cmd, cfg;
+	int ret = 0;
+
+	init_completion(&nfc->completion);
+
+	cfg = readl(nfc->reg_base + NFC_REG_CFG);
+	cfg |= (1 << 21);
+	writel(cfg, nfc->reg_base + NFC_REG_CFG);
+
+	meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+	cmd = nfc->param.chip_select | NFC_CMD_CLE | (NAND_CMD_STATUS & 0xff);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+	meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+
+	cmd = NFC_CMD_RB | NFC_CMD_IO6 | (1 << 16) | (0x18 & 0x1f);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+	meson_nfc_cmd_idle(nfc, 2);
+
+	ret = wait_for_completion_timeout(&nfc->completion,
+					  msecs_to_jiffies(1000));
+	if (ret == 0) {
+		dev_err(nfc->dev, "wait nand irq timeout\n");
+		ret = -1;
+	}
+
+	return ret;
+}
+
+static void meson_nfc_set_user_byte(struct mtd_info *mtd,
+				    struct nand_chip *chip, u8 *oob_buf)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	struct meson_nfc_info_format *info;
+	int i, count;
+
+	for (i = 0, count = 0; i < chip->ecc.steps; i++, count += 2) {
+		info = nfc_info_ptr(nfc, i);
+		info->info_bytes =
+			oob_buf[count] | (oob_buf[count + 1] << 8);
+	}
+}
+
+static void meson_nfc_get_user_byte(struct mtd_info *mtd,
+				    struct nand_chip *chip, u8 *oob_buf)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	struct meson_nfc_info_format *info;
+	int i, count;
+
+	for (i = 0, count = 0; i < chip->ecc.steps; i++, count += 2) {
+		info = nfc_info_ptr(nfc, i);
+		oob_buf[count] = info->info_bytes & 0xff;
+		oob_buf[count + 1] = (info->info_bytes >> 8) & 0xff;
+	}
+}
+
+static int meson_nfc_ecc_correct(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(chip);
+	struct meson_nfc_info_format *info;
+	u32 bitflips = 0, i;
+	u8 zero_cnt;
+
+	for (i = 0; i < chip->ecc.steps; i++) {
+		info = nfc_info_ptr(nfc, i);
+		if (info->ecc.eccerr_cnt == 0x3f) {
+			zero_cnt = info->zero_cnt & 0x3f;
+			if (meson_chip->is_scramble &&
+			    zero_cnt < chip->ecc.strength)
+				return ECC_CHECK_RETURN_FF;
+			mtd->ecc_stats.failed++;
+			continue;
+		}
+		mtd->ecc_stats.corrected += info->ecc.eccerr_cnt;
+		bitflips = max_t(u32, bitflips, info->ecc.eccerr_cnt);
+	}
+
+	return bitflips;
+}
+
+static inline u8 meson_nfc_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	u32 cmd;
+
+	cmd = nfc->param.chip_select | NFC_CMD_DRD | 0;
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	meson_nfc_drain_cmd(nfc);
+
+	meson_nfc_wait_cmd_finish(nfc, 1000);
+
+	return readb(nfc->reg_base + NFC_REG_BUF);
+}
+
+static void meson_nfc_read_buf(struct mtd_info *mtd, u8 *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		buf[i] = meson_nfc_read_byte(mtd);
+}
+
+static void meson_nfc_write_byte(struct mtd_info *mtd, u8 byte)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(mtd_to_nand(mtd));
+	u32 cmd;
+
+	meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+
+	cmd = nfc->param.chip_select | NFC_CMD_DWR | (byte & 0xff);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+	meson_nfc_cmd_idle(nfc, 0);
+
+	meson_nfc_wait_cmd_finish(nfc, 1000);
+}
+
+static void meson_nfc_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		meson_nfc_write_byte(mtd, buf[i]);
+}
+
+static int
+meson_nfc_rw_cmd_prepare_and_execute(struct meson_nfc *nfc,
+				     struct nand_chip *chip, int page, bool in)
+{
+	const struct nand_sdr_timings *sdr =
+		nand_get_sdr_timings(&chip->data_interface);
+	int cs = nfc->param.chip_select;
+	int i, cmd0, cmd_num;
+	int ret = 0;
+
+	cmd0 = (in) ? NAND_CMD_READ0 : NAND_CMD_SEQIN;
+	cmd_num = sizeof(struct nand_rw_cmd) / sizeof(int);
+	if (!in)
+		cmd_num--;
+
+	nfc->cmdfifo.rw.cmd0 = cs | NFC_CMD_CLE | cmd0;
+	for (i = 0; i < MAX_CYCLE_COLUMN_ADDRS; i++)
+		nfc->cmdfifo.rw.col[i] = cs | NFC_CMD_ALE | 0;
+
+	for (i = 0; i < MAX_CYCLE_ROW_ADDRS; i++)
+		nfc->cmdfifo.rw.row[i] = cs | NFC_CMD_ALE | ROW_ADDER(page, i);
+
+	nfc->cmdfifo.rw.cmd1 = cs | NFC_CMD_CLE | NAND_CMD_READSTART;
+
+	for (i = 0; i < cmd_num; i++)
+		writel(nfc->cmdfifo.cmd[i], nfc->reg_base + NFC_REG_CMD);
+
+	meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+
+	usleep_range(150, 200);
+	if (in)
+		ret = nand_soft_waitrdy(chip, PSEC_TO_MSEC(sdr->tR_max));
+
+	return ret;
+}
+
+static int meson_nfc_write_page_sub(struct mtd_info *mtd,
+				    struct nand_chip *chip, const u8 *buf,
+				    int page, int raw)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	dma_addr_t daddr, iaddr;
+	u32 cmd, ecc_step;
+	int ret;
+
+	ecc_step = mtd->writesize / chip->ecc.size;
+
+	daddr = dma_map_single(nfc->dev, (void *)nfc->data_buf,
+			       mtd->writesize + mtd->oobsize,
+			       DMA_TO_DEVICE);
+	ret = dma_mapping_error(nfc->dev, daddr);
+	if (ret) {
+		dev_err(nfc->dev, "dma mapping error\n");
+		goto err;
+	}
+
+	iaddr = dma_map_single(nfc->dev, (void *)nfc->info_buf,
+			       ecc_step * PER_INFO_BYTE,
+			       DMA_TO_DEVICE);
+	ret = dma_mapping_error(nfc->dev, iaddr);
+	if (ret) {
+		dev_err(nfc->dev, "dma mapping error\n");
+		goto err_map;
+	}
+
+	meson_nfc_rw_cmd_prepare_and_execute(nfc, chip, page, DIRWRITE);
+
+	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	meson_nfc_cmd_seed(nfc, page);
+	meson_nfc_cmd_access(nfc, mtd, raw, DIRWRITE);
+	ret = meson_nfc_wait_dma_finish(nfc);
+
+	ret = nand_prog_page_end_op(chip);
+
+	dma_unmap_single(nfc->dev, iaddr,
+			 ecc_step * PER_INFO_BYTE, DMA_TO_DEVICE);
+err_map:
+	dma_unmap_single(nfc->dev, daddr,
+			 mtd->writesize + mtd->oobsize, DMA_TO_DEVICE);
+err:
+	return ret;
+}
+
+static int meson_nfc_write_page_raw(struct mtd_info *mtd,
+				    struct nand_chip *chip, const u8 *buf,
+				    int oob_required, int page)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	u8 *oob_buf = chip->oob_poi;
+
+	meson_nfc_format_data_oob(nfc, mtd, buf, oob_buf);
+
+	return meson_nfc_write_page_sub(mtd, chip, nfc->data_buf, page, 1);
+}
+
+static int meson_nfc_write_page_hwecc(struct mtd_info *mtd,
+				      struct nand_chip *chip, const u8 *buf,
+				      int oob_required, int page)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	u8 *oob_buf = chip->oob_poi;
+
+	memcpy(nfc->data_buf, buf, mtd->writesize);
+	meson_nfc_set_user_byte(mtd, chip, oob_buf);
+
+	return meson_nfc_write_page_sub(mtd, chip, nfc->data_buf, page, 0);
+}
+
+static void meson_nfc_check_ecc_pages_valid(struct meson_nfc *nfc,
+					    struct mtd_info *mtd, int raw)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct meson_nfc_info_format *info;
+	int neccpages, i;
+
+	neccpages = raw ? 1 : (mtd->writesize / chip->ecc.size);
+
+	for (i = 0; i < neccpages; i++) {
+		info = nfc_info_ptr(nfc, neccpages - 1);
+		if (info->ecc.completed == 0)
+			dev_err(nfc->dev, "seems eccpage is invalid\n");
+	}
+}
+
+static int meson_nfc_read_page_sub(struct mtd_info *mtd,
+				   struct nand_chip *chip, const u8 *buf,
+				   int page, int raw)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	dma_addr_t daddr, iaddr;
+	u32 cmd, ecc_step;
+	int ret;
+
+	ecc_step =  mtd->writesize / chip->ecc.size;
+
+	daddr = dma_map_single(nfc->dev, (void *)nfc->data_buf,
+			       mtd->writesize + mtd->oobsize, DMA_FROM_DEVICE);
+	ret = dma_mapping_error(nfc->dev, daddr);
+	if (ret) {
+		dev_err(nfc->dev, "dma mapping error\n");
+		goto err;
+	}
+
+	iaddr = dma_map_single(nfc->dev, (void *)nfc->info_buf,
+			       ecc_step * PER_INFO_BYTE, DMA_FROM_DEVICE);
+	ret = dma_mapping_error(nfc->dev, iaddr);
+	if (ret) {
+		dev_err(nfc->dev, "dma mapping error\n");
+		goto err_map_daddr;
+	}
+
+	ret = meson_nfc_rw_cmd_prepare_and_execute(nfc, chip, page, DIRREAD);
+	if (ret)
+		goto err_map_iaddr;
+
+	cmd = GENCMDDADDRL(NFC_CMD_ADL, daddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDDADDRH(NFC_CMD_ADH, daddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDIADDRL(NFC_CMD_AIL, iaddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	cmd = GENCMDIADDRH(NFC_CMD_AIH, iaddr);
+	writel(cmd, nfc->reg_base + NFC_REG_CMD);
+
+	meson_nfc_cmd_seed(nfc, page);
+	meson_nfc_cmd_access(nfc, mtd, raw, DIRREAD);
+	ret = meson_nfc_wait_dma_finish(nfc);
+	meson_nfc_queue_rb(nfc);
+	meson_nfc_check_ecc_pages_valid(nfc, mtd, raw);
+
+err_map_iaddr:
+	dma_unmap_single(nfc->dev, iaddr,
+			 ecc_step * PER_INFO_BYTE, DMA_FROM_DEVICE);
+err_map_daddr:
+	dma_unmap_single(nfc->dev, daddr,
+			 mtd->writesize + mtd->oobsize, DMA_FROM_DEVICE);
+err:
+	return ret;
+}
+
+static int meson_nfc_read_page_raw(struct mtd_info *mtd,
+				   struct nand_chip *chip, u8 *buf,
+				   int oob_required, int page)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	u8 *oob_buf = chip->oob_poi;
+	int ret;
+
+	ret = meson_nfc_read_page_sub(mtd, chip, nfc->data_buf, page, 1);
+	if (ret)
+		return ret;
+
+	meson_nfc_prase_data_oob(nfc, mtd, buf, oob_buf);
+
+	return 0;
+}
+
+static int meson_nfc_read_page_hwecc(struct mtd_info *mtd,
+				     struct nand_chip *chip, u8 *buf,
+				     int oob_required, int page)
+{
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	u8 *oob_buf = chip->oob_poi;
+	int ret;
+
+	ret = meson_nfc_read_page_sub(mtd, chip, nfc->data_buf, page, 0);
+	if (ret)
+		return ret;
+
+	meson_nfc_get_user_byte(mtd, chip, oob_buf);
+
+	ret = meson_nfc_ecc_correct(mtd, chip);
+	if (ret == ECC_CHECK_RETURN_FF) {
+		if (buf)
+			memset(buf, 0xff, mtd->writesize);
+
+		memset(oob_buf, 0xff, mtd->oobsize);
+		return 0;
+	}
+
+	if (buf && buf != nfc->data_buf)
+		memcpy(buf, nfc->data_buf, mtd->writesize);
+
+	return ret;
+}
+
+static int meson_nfc_read_oob_raw(struct mtd_info *mtd,
+				  struct nand_chip *chip, int page)
+{
+	return meson_nfc_read_page_raw(mtd, chip, NULL, 1, page);
+}
+
+static int meson_nfc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+			      int page)
+{
+	return meson_nfc_read_page_hwecc(mtd, chip, NULL, 1, page);
+}
+
+static int meson_nfc_exec_op(struct nand_chip *chip,
+			     const struct nand_operation *op, bool check_only)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct meson_nfc *nfc = nand_get_controller_data(chip);
+	const struct nand_op_instr *instr = NULL;
+	int ret = 0, cmd;
+	unsigned int op_id;
+	int i;
+
+	for (op_id = 0; op_id < op->ninstrs; op_id++) {
+		instr = &op->instrs[op_id];
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			cmd = nfc->param.chip_select | NFC_CMD_CLE;
+			cmd |= instr->ctx.cmd.opcode & 0xff;
+			writel(cmd, nfc->reg_base + NFC_REG_CMD);
+			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
+			meson_nfc_drain_cmd(nfc);
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
+				cmd = nfc->param.chip_select | NFC_CMD_ALE;
+				cmd |= instr->ctx.addr.addrs[i] & 0xff;
+				writel(cmd, nfc->reg_base + NFC_REG_CMD);
+			}
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
+					   instr->ctx.data.len);
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
+					    instr->ctx.data.len);
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			mdelay(instr->ctx.waitrdy.timeout_ms);
+			ret = nand_soft_waitrdy(chip,
+						instr->ctx.waitrdy.timeout_ms);
+			break;
+		}
+	}
+	return ret;
+}
+
+static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
+			       struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int free_oob;
+
+	if (section >= chip->ecc.steps)
+		return -ERANGE;
+
+	free_oob = (section + 1) * 2;
+	oobregion->offset = section * chip->ecc.bytes + free_oob;
+	oobregion->length = chip->ecc.bytes;
+
+	return 0;
+}
+
+static int meson_ooblayout_free(struct mtd_info *mtd, int section,
+				struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section >= chip->ecc.steps)
+		return -ERANGE;
+
+	oobregion->offset = section * (2 + chip->ecc.bytes);
+	oobregion->length = 2;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
+	.ecc = meson_ooblayout_ecc,
+	.free = meson_ooblayout_free,
+};
+
+static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
+	struct meson_nfc *nfc = nand_get_controller_data(nand);
+	const struct meson_nand_ecc *meson_ecc = nfc->data->ecc;
+	int num = nfc->data->ecc_num;
+	int nsectors, i, bytes;
+
+	/* support only ecc hw mode */
+	if (nand->ecc.mode != NAND_ECC_HW) {
+		dev_err(dev, "ecc.mode not supported\n");
+		return -EINVAL;
+	}
+
+	if (!nand->ecc.size || !nand->ecc.strength) {
+		/* use datasheet requirements */
+		nand->ecc.strength = nand->ecc_strength_ds;
+		nand->ecc.size = nand->ecc_step_ds;
+	}
+
+	if (nand->ecc.options & NAND_ECC_MAXIMIZE) {
+		nand->ecc.size = 1024;
+		nsectors = mtd->writesize / nand->ecc.size;
+		bytes = mtd->oobsize - 2 * nsectors;
+		bytes /= nsectors;
+
+		/* and bytes has to be even. */
+		if (bytes % 2)
+			bytes--;
+
+		nand->ecc.strength = bytes * 8 / fls(8 * nand->ecc.size);
+	} else {
+		if (nand->ecc.strength > meson_ecc[num - 1].strength) {
+			dev_err(dev, "not support ecc strength\n");
+			return -EINVAL;
+		}
+	}
+
+	for (i = 0; i < num; i++) {
+		if (meson_ecc[i].strength == 0xff ||
+		    nand->ecc.strength < meson_ecc[i].strength)
+			break;
+	}
+
+	nand->ecc.strength = meson_ecc[i - 1].strength;
+	nand->ecc.bytes = meson_ecc[i - 1].parity;
+
+	meson_chip->bch_mode = meson_ecc[i - 1].bch;
+
+	if (nand->ecc.size != 512 && nand->ecc.size != 1024)
+		return -EINVAL;
+
+	nsectors = mtd->writesize / nand->ecc.size;
+	bytes = nsectors * 2;
+
+	if (mtd->oobsize < (nand->ecc.bytes * nsectors + bytes))
+		return -EINVAL;
+
+	mtd_set_ooblayout(mtd, &meson_ooblayout_ops);
+
+	return 0;
+}
+
+static int meson_nfc_clk_init(struct meson_nfc *nfc)
+{
+	int ret;
+
+	/* request core clock */
+	nfc->core_clk = devm_clk_get(nfc->dev, "core");
+	if (IS_ERR(nfc->core_clk)) {
+		dev_err(nfc->dev, "failed to get core clk\n");
+		return PTR_ERR(nfc->core_clk);
+	}
+
+	nfc->device_clk = devm_clk_get(nfc->dev, "device");
+	if (IS_ERR(nfc->device_clk)) {
+		dev_err(nfc->dev, "failed to get device clk\n");
+		return PTR_ERR(nfc->device_clk);
+	}
+
+	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
+	regmap_update_bits(nfc->reg_clk, 0,
+			   CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
+			   CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
+
+	ret = clk_prepare_enable(nfc->core_clk);
+	if (ret) {
+		dev_err(nfc->dev, "failed to enable core clk\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(nfc->device_clk);
+	if (ret) {
+		dev_err(nfc->dev, "failed to enable device clk\n");
+		clk_disable_unprepare(nfc->core_clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void meson_nfc_disable_clk(struct meson_nfc *nfc)
+{
+	clk_disable_unprepare(nfc->device_clk);
+	clk_disable_unprepare(nfc->core_clk);
+}
+
+static int meson_nfc_buffer_init(struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct meson_nfc *nfc = nand_get_controller_data(nand);
+	int info_bytes, page_bytes;
+	int nsectors;
+
+	nsectors = mtd->writesize / nand->ecc.size;
+	info_bytes = nsectors * PER_INFO_BYTE;
+	page_bytes = mtd->writesize + mtd->oobsize;
+
+	if (nfc->data_buf && nfc->info_buf)
+		return 0;
+
+	nfc->data_buf = kmalloc(page_bytes, GFP_KERNEL);
+	if (!nfc->data_buf)
+		return -ENOMEM;
+
+	nfc->info_buf = kmalloc(info_bytes, GFP_KERNEL);
+	if (!nfc->info_buf) {
+		kfree(nfc->data_buf);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
+				     int rc_min, int rea_max, int rhoh_min)
+{
+	int div, bt_min, bt_max, bus_timing;
+	int ret;
+
+	div = DIV_ROUND_UP((rc_min / 1000), NFC_CLK_CYCLE);
+	ret = clk_set_rate(nfc->device_clk, 1000000000 / div);
+	if (ret) {
+		dev_err(nfc->dev, "failed to set nand clock rate\n");
+		return ret;
+	}
+
+	bt_min = (rea_max + NFC_DEFAULT_DELAY) / div;
+	bt_max = (NFC_DEFAULT_DELAY + rhoh_min + rc_min / 2) / div;
+
+	bt_min = DIV_ROUND_UP(bt_min, 1000);
+	bt_max = DIV_ROUND_UP(bt_max, 1000);
+
+	if (bt_max < bt_min)
+		return -EINVAL;
+
+	bus_timing = (bt_min + bt_max) / 2 + 1;
+
+	writel((1 << 21), nfc->reg_base + NFC_REG_CFG);
+	writel((NFC_CLK_CYCLE - 1) | (bus_timing << 5),
+	       nfc->reg_base + NFC_REG_CFG);
+
+	writel((1 << 31), nfc->reg_base + NFC_REG_CMD);
+
+	return 0;
+}
+
+static int
+meson_nfc_setup_data_interface(struct mtd_info *mtd, int csline,
+			       const struct nand_data_interface *conf)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct meson_nfc *nfc = nand_get_controller_data(nand);
+	const struct nand_sdr_timings *timings;
+
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return -ENOTSUPP;
+
+	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+		return 0;
+
+	meson_nfc_calc_set_timing(nfc, timings->tRC_min,
+				  timings->tREA_max, timings->tRHOH_min);
+	return 0;
+}
+
+static int
+meson_nfc_nand_chip_init(struct device *dev,
+			 struct meson_nfc *nfc, struct device_node *np)
+{
+	struct meson_nfc_nand_chip *chip;
+	struct nand_chip *nand;
+	struct mtd_info *mtd;
+	int ret, nsels, i, len = 0;
+	char cs_id[16];
+	u32 tmp;
+
+	if (!of_get_property(np, "reg", &nsels))
+		return -EINVAL;
+
+	nsels /= sizeof(u32);
+	if (!nsels || nsels > MAX_CE_NUM) {
+		dev_err(dev, "invalid reg property size\n");
+		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, "could not retrieve reg property: %d\n",
+				ret);
+			return ret;
+		}
+		chip->sels[i] = tmp;
+		len += snprintf(cs_id + len, 16, "%s%d", i ? "-" : ".", tmp);
+	}
+
+	chip->is_scramble =
+		of_property_read_bool(np, "amlogic,nand-enable-scrambler");
+
+	nand = &chip->nand;
+	nand_set_flash_node(nand, np);
+	nand_set_controller_data(nand, nfc);
+
+	nand->options |= NAND_USE_BOUNCE_BUFFER;
+	nand->select_chip = meson_nfc_select_chip;
+	nand->exec_op = meson_nfc_exec_op;
+	nand->setup_data_interface = meson_nfc_setup_data_interface;
+
+	nand->ecc.mode = NAND_ECC_HW;
+
+	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
+	nand->ecc.write_page = meson_nfc_write_page_hwecc;
+	nand->ecc.write_oob_raw = nand_write_oob_std;
+	nand->ecc.write_oob = nand_write_oob_std;
+
+	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
+	nand->ecc.read_page = meson_nfc_read_page_hwecc;
+	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
+	nand->ecc.read_oob = meson_nfc_read_oob;
+
+	mtd = nand_to_mtd(nand);
+	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = dev;
+
+	ret = nand_scan_ident(mtd, 1, NULL);
+	if (ret) {
+		dev_err(dev, "failed to can ident\n");
+		return -ENODEV;
+	}
+
+	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s:nand%s",
+				   dev_name(dev), cs_id);
+
+	/* store bbt magic in page, cause OOB is not protected */
+	if (nand->bbt_options & NAND_BBT_USE_FLASH)
+		nand->bbt_options |= NAND_BBT_NO_OOB;
+
+	nand->options |= NAND_NO_SUBPAGE_WRITE;
+
+	ret = meson_nfc_ecc_init(dev, mtd);
+	if (ret) {
+		dev_err(dev, "failed to ecc init\n");
+		return -EINVAL;
+	}
+
+	if (nand->options & NAND_BUSWIDTH_16) {
+		dev_err(dev, "16bits buswidth not supported");
+		return -EINVAL;
+	}
+
+	ret = meson_nfc_buffer_init(mtd);
+	if (ret)
+		return -ENOMEM;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		return -ENODEV;
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (ret) {
+		dev_err(dev, "failed to register mtd device: %d\n", ret);
+		nand_cleanup(nand);
+		return ret;
+	}
+
+	list_add_tail(&chip->node, &nfc->chips);
+
+	return 0;
+}
+
+static int meson_nfc_nand_chip_cleanup(struct meson_nfc *nfc)
+{
+	struct meson_nfc_nand_chip *chip;
+	struct mtd_info *mtd;
+	int ret;
+
+	while (!list_empty(&nfc->chips)) {
+		chip = list_first_entry(&nfc->chips, struct meson_nfc_nand_chip,
+					node);
+		mtd = nand_to_mtd(&chip->nand);
+		ret = mtd_device_unregister(mtd);
+		if (ret)
+			return ret;
+
+		nand_cleanup(&chip->nand);
+		list_del(&chip->node);
+	}
+
+	return 0;
+}
+
+static int meson_nfc_nand_chips_init(struct device *dev, struct meson_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 = meson_nfc_nand_chip_init(dev, nfc, nand_np);
+		if (ret) {
+			meson_nfc_nand_chip_cleanup(nfc);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static irqreturn_t meson_nfc_irq(int irq, void *id)
+{
+	struct meson_nfc *nfc = id;
+	u32 cfg;
+
+	cfg = readl(nfc->reg_base + NFC_REG_CFG);
+	cfg |= (1 << 21);
+	writel(cfg, nfc->reg_base + NFC_REG_CFG);
+
+	complete(&nfc->completion);
+	return IRQ_HANDLED;
+}
+
+static const struct meson_nfc_data meson_gxl_data = {
+	.short_bch	= NFC_ECC_BCH60_1K,
+	.ecc		= meson_gxl_ecc,
+	.ecc_num	= ARRAY_SIZE(meson_gxl_ecc),
+};
+
+static const struct meson_nfc_data meson_axg_data = {
+	.short_bch	= NFC_ECC_BCH8_1K,
+	.ecc		= meson_axg_ecc,
+	.ecc_num	= ARRAY_SIZE(meson_axg_ecc),
+};
+
+static const struct of_device_id meson_nfc_id_table[] = {
+	{
+		.compatible = "amlogic,meson-gxl-nfc",
+		.data = &meson_gxl_data,
+	}, {
+		.compatible = "amlogic,meson-axg-nfc",
+		.data = &meson_axg_data,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
+
+static int meson_nfc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct meson_nfc *nfc;
+	struct resource *res;
+	int ret, irq;
+
+	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	nfc->data =
+		(struct meson_nfc_data *)of_device_get_match_data(&pdev->dev);
+	if (!nfc->data)
+		return -ENODEV;
+
+	spin_lock_init(&nfc->controller.lock);
+	init_waitqueue_head(&nfc->controller.wq);
+	INIT_LIST_HEAD(&nfc->chips);
+
+	nfc->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Failed to nfc reg resource\n");
+		return -EINVAL;
+	}
+
+	nfc->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(nfc->reg_base)) {
+		dev_err(dev, "Failed to lookup nfi reg base\n");
+		return PTR_ERR(nfc->reg_base);
+	}
+
+	nfc->reg_clk =
+		syscon_regmap_lookup_by_phandle(dev->of_node,
+						"amlogic,mmc-syscon");
+	if (IS_ERR(nfc->reg_clk)) {
+		dev_err(dev, "Failed to lookup clock base\n");
+		return PTR_ERR(nfc->reg_clk);
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no nfi irq resource\n");
+		return -EINVAL;
+	}
+
+	ret = meson_nfc_clk_init(nfc);
+	if (ret) {
+		dev_err(dev, "failed to initialize nand clk\n");
+		goto err_clk;
+	}
+
+	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
+	if (ret) {
+		dev_err(dev, "failed to request nfi irq\n");
+		ret = -EINVAL;
+		goto err_clk;
+	}
+
+	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(dev, "failed to set dma mask\n");
+		goto err_clk;
+	}
+
+	platform_set_drvdata(pdev, nfc);
+
+	ret = meson_nfc_nand_chips_init(dev, nfc);
+	if (ret) {
+		dev_err(dev, "failed to init nand chips\n");
+		goto err_clk;
+	}
+
+	return 0;
+
+err_clk:
+	meson_nfc_disable_clk(nfc);
+	return ret;
+}
+
+static int meson_nfc_remove(struct platform_device *pdev)
+{
+	struct meson_nfc *nfc = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = meson_nfc_nand_chip_cleanup(nfc);
+	if (ret)
+		return ret;
+
+	meson_nfc_disable_clk(nfc);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver meson_nfc_driver = {
+	.probe  = meson_nfc_probe,
+	.remove = meson_nfc_remove,
+	.driver = {
+		.name  = "meson-nand",
+		.of_match_table = meson_nfc_id_table,
+	},
+};
+module_platform_driver(meson_nfc_driver);
+
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_AUTHOR("Liang Yang <liang.yang@amlogic.com>");
+MODULE_DESCRIPTION("Amlogic's Meson NAND Flash Controller driver");
-- 
2.18.0

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

Hi Yixun,

On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan <yixun.lan@amlogic.com> wrote:

I haven't finished reviewing the driver yet (I'll try to do that later
this week), but I already pointed a few things to fix/improve.

> +
> +static int meson_nfc_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op, bool check_only)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct meson_nfc *nfc = nand_get_controller_data(chip);
> +	const struct nand_op_instr *instr = NULL;
> +	int ret = 0, cmd;
> +	unsigned int op_id;
> +	int i;
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
> +			cmd |= instr->ctx.cmd.opcode & 0xff;
> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);

This is not necessarily TWB you have to wait after a CMD cycle. It can
be tWHR. And you should definitely not hardcode the value, since,
AFAIR, it depends on the selected SDR timings. Probably something you
should calculate in ->setup_data_interface().

> +			meson_nfc_drain_cmd(nfc);

I don't know exactly how the NAND controller works, but it's usually
not a good idea to execute the operation right away, especially if you
have address/cmd/data cycles following this cmd and those can be
packed in the same controller operation. 

> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
> +			}
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
> +					   instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
> +					    instr->ctx.data.len);

Well, I'm not entirely sure what happens when you call
read/write_buf(), but it seems you're doing that one byte at a time,
and that sounds not so efficient given the operation you do for each
byte read/written. Don't you have a way to tell the engine that you
want to read/write X bytes?

> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			mdelay(instr->ctx.waitrdy.timeout_ms);
> +			ret = nand_soft_waitrdy(chip,
> +						instr->ctx.waitrdy.timeout_ms);

Hm, i'd be surprised if the controller does not have a way to optimize
waits on R/B transitions.

> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
> +			       struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	int free_oob;
> +
> +	if (section >= chip->ecc.steps)
> +		return -ERANGE;
> +
> +	free_oob = (section + 1) * 2;
> +	oobregion->offset = section * chip->ecc.bytes + free_oob;

Hm, this offset calculation looks weird. Are you sure it's correct?
I'd bet on something like:

	oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));

> +	oobregion->length = chip->ecc.bytes;
> +
> +	return 0;
> +}
> +
> +static int meson_ooblayout_free(struct mtd_info *mtd, int section,
> +				struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section >= chip->ecc.steps)
> +		return -ERANGE;
> +
> +	oobregion->offset = section * (2 + chip->ecc.bytes);
> +	oobregion->length = 2;
> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
> +	.ecc = meson_ooblayout_ecc,
> +	.free = meson_ooblayout_free,
> +};
> +
> +static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	const struct meson_nand_ecc *meson_ecc = nfc->data->ecc;
> +	int num = nfc->data->ecc_num;
> +	int nsectors, i, bytes;
> +
> +	/* support only ecc hw mode */
> +	if (nand->ecc.mode != NAND_ECC_HW) {
> +		dev_err(dev, "ecc.mode not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!nand->ecc.size || !nand->ecc.strength) {
> +		/* use datasheet requirements */
> +		nand->ecc.strength = nand->ecc_strength_ds;
> +		nand->ecc.size = nand->ecc_step_ds;
> +	}
> +
> +	if (nand->ecc.options & NAND_ECC_MAXIMIZE) {
> +		nand->ecc.size = 1024;
> +		nsectors = mtd->writesize / nand->ecc.size;
> +		bytes = mtd->oobsize - 2 * nsectors;
> +		bytes /= nsectors;
> +
> +		/* and bytes has to be even. */
> +		if (bytes % 2)
> +			bytes--;
> +
> +		nand->ecc.strength = bytes * 8 / fls(8 * nand->ecc.size);
> +	} else {
> +		if (nand->ecc.strength > meson_ecc[num - 1].strength) {
> +			dev_err(dev, "not support ecc strength\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		if (meson_ecc[i].strength == 0xff ||
> +		    nand->ecc.strength < meson_ecc[i].strength)
> +			break;
> +	}

I'd suggest that you look at nand_match_ecc_req(). It's likely that the
selection logic you have here can be replaced by the generic function.

> +
> +	nand->ecc.strength = meson_ecc[i - 1].strength;
> +	nand->ecc.bytes = meson_ecc[i - 1].parity;
> +
> +	meson_chip->bch_mode = meson_ecc[i - 1].bch;
> +
> +	if (nand->ecc.size != 512 && nand->ecc.size != 1024)
> +		return -EINVAL;
> +
> +	nsectors = mtd->writesize / nand->ecc.size;
> +	bytes = nsectors * 2;
> +
> +	if (mtd->oobsize < (nand->ecc.bytes * nsectors + bytes))
> +		return -EINVAL;
> +
> +	mtd_set_ooblayout(mtd, &meson_ooblayout_ops);
> +
> +	return 0;
> +}
> +
> +static int meson_nfc_clk_init(struct meson_nfc *nfc)
> +{
> +	int ret;
> +
> +	/* request core clock */
> +	nfc->core_clk = devm_clk_get(nfc->dev, "core");
> +	if (IS_ERR(nfc->core_clk)) {
> +		dev_err(nfc->dev, "failed to get core clk\n");
> +		return PTR_ERR(nfc->core_clk);
> +	}
> +
> +	nfc->device_clk = devm_clk_get(nfc->dev, "device");
> +	if (IS_ERR(nfc->device_clk)) {
> +		dev_err(nfc->dev, "failed to get device clk\n");
> +		return PTR_ERR(nfc->device_clk);
> +	}
> +
> +	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
> +	regmap_update_bits(nfc->reg_clk, 0,
> +			   CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK,
> +			   CLK_SELECT_NAND | CLK_ALWAYS_ON | CLK_DIV_MASK);
> +
> +	ret = clk_prepare_enable(nfc->core_clk);
> +	if (ret) {
> +		dev_err(nfc->dev, "failed to enable core clk\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(nfc->device_clk);
> +	if (ret) {
> +		dev_err(nfc->dev, "failed to enable device clk\n");
> +		clk_disable_unprepare(nfc->core_clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void meson_nfc_disable_clk(struct meson_nfc *nfc)
> +{
> +	clk_disable_unprepare(nfc->device_clk);
> +	clk_disable_unprepare(nfc->core_clk);
> +}
> +
> +static int meson_nfc_buffer_init(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	int info_bytes, page_bytes;
> +	int nsectors;
> +
> +	nsectors = mtd->writesize / nand->ecc.size;
> +	info_bytes = nsectors * PER_INFO_BYTE;
> +	page_bytes = mtd->writesize + mtd->oobsize;
> +
> +	if (nfc->data_buf && nfc->info_buf)
> +		return 0;
> +
> +	nfc->data_buf = kmalloc(page_bytes, GFP_KERNEL);

I'm pretty sure that does not work if you have several chips. Either
you have one buffer tied to the NFC, and it has to be large enough to
handle the NAND with the largest page, or you have one buffer per chip.

> +	if (!nfc->data_buf)
> +		return -ENOMEM;
> +
> +	nfc->info_buf = kmalloc(info_bytes, GFP_KERNEL);
> +	if (!nfc->info_buf) {
> +		kfree(nfc->data_buf);
> +		return -ENOMEM;
> +	}
> +

Those buffers are not removed in the cleanup/error path.

> +	return 0;
> +}
> +
> +static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
> +				     int rc_min, int rea_max, int rhoh_min)
> +{
> +	int div, bt_min, bt_max, bus_timing;
> +	int ret;
> +
> +	div = DIV_ROUND_UP((rc_min / 1000), NFC_CLK_CYCLE);
> +	ret = clk_set_rate(nfc->device_clk, 1000000000 / div);
> +	if (ret) {
> +		dev_err(nfc->dev, "failed to set nand clock rate\n");
> +		return ret;
> +	}
> +
> +	bt_min = (rea_max + NFC_DEFAULT_DELAY) / div;
> +	bt_max = (NFC_DEFAULT_DELAY + rhoh_min + rc_min / 2) / div;
> +
> +	bt_min = DIV_ROUND_UP(bt_min, 1000);
> +	bt_max = DIV_ROUND_UP(bt_max, 1000);
> +
> +	if (bt_max < bt_min)
> +		return -EINVAL;
> +
> +	bus_timing = (bt_min + bt_max) / 2 + 1;
> +
> +	writel((1 << 21), nfc->reg_base + NFC_REG_CFG);
> +	writel((NFC_CLK_CYCLE - 1) | (bus_timing << 5),
> +	       nfc->reg_base + NFC_REG_CFG);
> +
> +	writel((1 << 31), nfc->reg_base + NFC_REG_CMD);

Nothing special to setup when operating in EDO mode (tRC < 20ns)?

> +
> +	return 0;
> +}
> +
> +static int
> +meson_nfc_setup_data_interface(struct mtd_info *mtd, int csline,
> +			       const struct nand_data_interface *conf)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	const struct nand_sdr_timings *timings;
> +
> +	timings = nand_get_sdr_timings(conf);
> +	if (IS_ERR(timings))
> +		return -ENOTSUPP;
> +
> +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> +		return 0;

Are you sure you support all SDR timing modes, even EDO ones (tRC <
20ns)?

> +
> +	meson_nfc_calc_set_timing(nfc, timings->tRC_min,
> +				  timings->tREA_max, timings->tRHOH_min);
> +	return 0;
> +}
> +
> +static int
> +meson_nfc_nand_chip_init(struct device *dev,
> +			 struct meson_nfc *nfc, struct device_node *np)
> +{
> +	struct meson_nfc_nand_chip *chip;
> +	struct nand_chip *nand;
> +	struct mtd_info *mtd;
> +	int ret, nsels, i, len = 0;
> +	char cs_id[16];
> +	u32 tmp;
> +
> +	if (!of_get_property(np, "reg", &nsels))
> +		return -EINVAL;
> +
> +	nsels /= sizeof(u32);
> +	if (!nsels || nsels > MAX_CE_NUM) {
> +		dev_err(dev, "invalid reg property size\n");
> +		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, "could not retrieve reg property: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		chip->sels[i] = tmp;

You should probably keep track of all the already assigned CS lines, to
prevent situations where the same controller-CS is used twice
(copy&paste error when writing the DT).

> +		len += snprintf(cs_id + len, 16, "%s%d", i ? "-" : ".", tmp);

Hm, do we really need to be that accurate? I'd suggest using the first
CS only.

> +	}
> +
> +	chip->is_scramble =
> +		of_property_read_bool(np, "amlogic,nand-enable-scrambler");

I think I already complained about that :P. If you think this is still
needed (meaning that the autodetection + NAND_NEED_SCRAMBLING flag are
not enough), I'll need a detailed explanation ;-).

> +
> +	nand = &chip->nand;
> +	nand_set_flash_node(nand, np);
> +	nand_set_controller_data(nand, nfc);
> +
> +	nand->options |= NAND_USE_BOUNCE_BUFFER;
> +	nand->select_chip = meson_nfc_select_chip;
> +	nand->exec_op = meson_nfc_exec_op;
> +	nand->setup_data_interface = meson_nfc_setup_data_interface;
> +
> +	nand->ecc.mode = NAND_ECC_HW;
> +
> +	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
> +	nand->ecc.write_page = meson_nfc_write_page_hwecc;
> +	nand->ecc.write_oob_raw = nand_write_oob_std;
> +	nand->ecc.write_oob = nand_write_oob_std;
> +
> +	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
> +	nand->ecc.read_page = meson_nfc_read_page_hwecc;
> +	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
> +	nand->ecc.read_oob = meson_nfc_read_oob;

We usually setup the ECC fields after the identification phase. This
way, if you ever want/need to support SW ECC, the code is already
properly placed.

> +
> +	mtd = nand_to_mtd(nand);
> +	mtd->owner = THIS_MODULE;
> +	mtd->dev.parent = dev;
> +
> +	ret = nand_scan_ident(mtd, 1, NULL);
> +	if (ret) {
> +		dev_err(dev, "failed to can ident\n");
> +		return -ENODEV;
> +	}
> +
> +	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s:nand%s",
> +				   dev_name(dev), cs_id);

So, if you follow my suggestion, it should be:


You should make that conditional, because the core might have retrieved
a user-friendly from the DT (label prop defined to the NAND chip node).

So, if you follow my suggestion to just use the first CS for the nand
id, that gives:

	if (!mtd->name) {
		mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
					   "%s:nand%d",
					   dev_name(dev),
					   chip->sels[i]);
		if (!mtd->name)
			return -ENOMEM;
	}
> +
> +	/* store bbt magic in page, cause OOB is not protected */
> +	if (nand->bbt_options & NAND_BBT_USE_FLASH)
> +		nand->bbt_options |= NAND_BBT_NO_OOB;
> +
> +	nand->options |= NAND_NO_SUBPAGE_WRITE;
> +
> +	ret = meson_nfc_ecc_init(dev, mtd);
> +	if (ret) {
> +		dev_err(dev, "failed to ecc init\n");
> +		return -EINVAL;
> +	}
> +
> +	if (nand->options & NAND_BUSWIDTH_16) {
> +		dev_err(dev, "16bits buswidth not supported");
> +		return -EINVAL;
> +	}
> +
> +	ret = meson_nfc_buffer_init(mtd);
> +	if (ret)
> +		return -ENOMEM;
> +
> +	ret = nand_scan_tail(mtd);

Miquel has reworked the nand_scan() infrastructure recently. Now you
have to use nand_scan() and define ->attach_chip() hook to do all the
init that happens between nand_scan_ident() and nand_scan_tail() in
your code. And of course, all resources allocated in the
->attach_chip() hook should be freed in ->detach_chip().

> +	if (ret)
> +		return -ENODEV;
> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to register mtd device: %d\n", ret);
> +		nand_cleanup(nand);
> +		return ret;
> +	}
> +
> +	list_add_tail(&chip->node, &nfc->chips);
> +
> +	return 0;
> +}
> +
> +static int meson_nfc_nand_chip_cleanup(struct meson_nfc *nfc)

			     ^ chips

> +{
> +	struct meson_nfc_nand_chip *chip;
> +	struct mtd_info *mtd;
> +	int ret;
> +
> +	while (!list_empty(&nfc->chips)) {
> +		chip = list_first_entry(&nfc->chips, struct meson_nfc_nand_chip,
> +					node);
> +		mtd = nand_to_mtd(&chip->nand);
> +		ret = mtd_device_unregister(mtd);
> +		if (ret)
> +			return ret;
> +
> +		nand_cleanup(&chip->nand);
> +		list_del(&chip->node);
> +	}
> +
> +	return 0;
> +}
> +
> +static int meson_nfc_nand_chips_init(struct device *dev, struct meson_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 = meson_nfc_nand_chip_init(dev, nfc, nand_np);
> +		if (ret) {
> +			meson_nfc_nand_chip_cleanup(nfc);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t meson_nfc_irq(int irq, void *id)
> +{
> +	struct meson_nfc *nfc = id;
> +	u32 cfg;
> +
> +	cfg = readl(nfc->reg_base + NFC_REG_CFG);
> +	cfg |= (1 << 21);
> +	writel(cfg, nfc->reg_base + NFC_REG_CFG);
> +
> +	complete(&nfc->completion);
> +	return IRQ_HANDLED;

Can you check if at least one event has been generated, and if not
return IRQ_NONE?

> +}
> +
> +static const struct meson_nfc_data meson_gxl_data = {
> +	.short_bch	= NFC_ECC_BCH60_1K,
> +	.ecc		= meson_gxl_ecc,
> +	.ecc_num	= ARRAY_SIZE(meson_gxl_ecc),
> +};
> +
> +static const struct meson_nfc_data meson_axg_data = {
> +	.short_bch	= NFC_ECC_BCH8_1K,
> +	.ecc		= meson_axg_ecc,
> +	.ecc_num	= ARRAY_SIZE(meson_axg_ecc),
> +};
> +
> +static const struct of_device_id meson_nfc_id_table[] = {
> +	{
> +		.compatible = "amlogic,meson-gxl-nfc",
> +		.data = &meson_gxl_data,
> +	}, {
> +		.compatible = "amlogic,meson-axg-nfc",
> +		.data = &meson_axg_data,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
> +
> +static int meson_nfc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct meson_nfc *nfc;
> +	struct resource *res;
> +	int ret, irq;
> +
> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	nfc->data =
> +		(struct meson_nfc_data *)of_device_get_match_data(&pdev->dev);

You don't need the cast if you declare nfc->data as const in the struct
def.

> +	if (!nfc->data)
> +		return -ENODEV;
> +
> +	spin_lock_init(&nfc->controller.lock);
> +	init_waitqueue_head(&nfc->controller.wq);

There's a helper for that (nand_controller_init()).

> +	INIT_LIST_HEAD(&nfc->chips);
> +
> +	nfc->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "Failed to nfc reg resource\n");
> +		return -EINVAL;
> +	}

No need to do this check, just pass res to devm_ioremap_resource() and
it will do the check for you.

> +
> +	nfc->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(nfc->reg_base)) {
> +		dev_err(dev, "Failed to lookup nfi reg base\n");

This error message is not needed, devm_ioremap_resource() complains
already.

Just do:

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	nfc->reg_base = devm_ioremap_resource(dev, res);
	if (IS_ERR(nfc->reg_base))
		return PTR_ERR(nfc->reg_base);

> +		return PTR_ERR(nfc->reg_base);
> +	}
> +
> +	nfc->reg_clk =
> +		syscon_regmap_lookup_by_phandle(dev->of_node,
> +						"amlogic,mmc-syscon");
> +	if (IS_ERR(nfc->reg_clk)) {
> +		dev_err(dev, "Failed to lookup clock base\n");
> +		return PTR_ERR(nfc->reg_clk);
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "no nfi irq resource\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = meson_nfc_clk_init(nfc);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize nand clk\n");
> +		goto err_clk;
> +	}
> +
> +	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);

You should make sure all irqs are masked/cleared before setting up your
irq handler.

> +	if (ret) {
> +		dev_err(dev, "failed to request nfi irq\n");
> +		ret = -EINVAL;
> +		goto err_clk;
> +	}
> +
> +	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(dev, "failed to set dma mask\n");
> +		goto err_clk;
> +	}
> +
> +	platform_set_drvdata(pdev, nfc);
> +
> +	ret = meson_nfc_nand_chips_init(dev, nfc);
> +	if (ret) {
> +		dev_err(dev, "failed to init nand chips\n");
> +		goto err_clk;
> +	}
> +
> +	return 0;
> +
> +err_clk:
> +	meson_nfc_disable_clk(nfc);
> +	return ret;
> +}

Regards,

Boris

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Yixun Lan]

Hi Boris


On 08/02/2018 05:50 AM, Boris Brezillon wrote:
> Hi Yixun,
> 
> On Thu, 19 Jul 2018 17:46:12 +0800
> Yixun Lan <yixun.lan@amlogic.com> wrote:
> 
> I haven't finished reviewing the driver yet (I'll try to do that later
> this week), but I already pointed a few things to fix/improve.
> 

thanks for the fully review, we really appreciate your time ;-)

I will comment on a few general items first, then clarify others after
talking to the NAND/ASIC team

>> +
>> +static int meson_nfc_exec_op(struct nand_chip *chip,
>> +			     const struct nand_operation *op, bool check_only)
>> +{
>> +
>> +static int meson_nfc_buffer_init(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	int info_bytes, page_bytes;
>> +	int nsectors;
>> +
>> +	nsectors = mtd->writesize / nand->ecc.size;
>> +	info_bytes = nsectors * PER_INFO_BYTE;
>> +	page_bytes = mtd->writesize + mtd->oobsize;
>> +
>> +	if (nfc->data_buf && nfc->info_buf)
>> +		return 0;
>> +
>> +	nfc->data_buf = kmalloc(page_bytes, GFP_KERNEL);
> 
> I'm pretty sure that does not work if you have several chips. Either
> you have one buffer tied to the NFC, and it has to be large enough to
> handle the NAND with the largest page, or you have one buffer per chip.
> 
em, we will fix this in next version,

>> +	if (!nfc->data_buf)
>> +		return -ENOMEM;
>> +
>> +	nfc->info_buf = kmalloc(info_bytes, GFP_KERNEL);
>> +	if (!nfc->info_buf) {
>> +		kfree(nfc->data_buf);
>> +		return -ENOMEM;
>> +	}
>> +
> 
> Those buffers are not removed in the cleanup/error path.
> 
indeed, thanks for pointing out.
we actually realized this error after sent out this patch ..
>> +	return 0;
>> +}
>> +
>> +static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
>> +				     int rc_min, int rea_max, int rhoh_min)
..

>> +
>> +static int
>> +meson_nfc_nand_chip_init(struct device *dev,
>> +			 struct meson_nfc *nfc, struct device_node *np)
>> +{
>> +	struct meson_nfc_nand_chip *chip;
>> +	struct nand_chip *nand;
>> +	struct mtd_info *mtd;
>> +	int ret, nsels, i, len = 0;
>> +	char cs_id[16];
>> +	u32 tmp;
>> +
>> +	if (!of_get_property(np, "reg", &nsels))
>> +		return -EINVAL;
>> +
>> +	nsels /= sizeof(u32);
>> +	if (!nsels || nsels > MAX_CE_NUM) {
>> +		dev_err(dev, "invalid reg property size\n");
>> +		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, "could not retrieve reg property: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +		chip->sels[i] = tmp;
> 
> You should probably keep track of all the already assigned CS lines, to
> prevent situations where the same controller-CS is used twice
> (copy&paste error when writing the DT).
> 

will do in next version, we would consider to use a bitmap for tracking
this ..
>> +		len += snprintf(cs_id + len, 16, "%s%d", i ? "-" : ".", tmp);
> 
> Hm, do we really need to be that accurate? I'd suggest using the first
> CS only.
> 
ok, this would much simple..
thanks for the suggestion and the detail sample code in the following
section ;-)
>> +	}
>> +
>> +	chip->is_scramble =
>> +		of_property_read_bool(np, "amlogic,nand-enable-scrambler");
> 
> I think I already complained about that :P. If you think this is still
> needed (meaning that the autodetection + NAND_NEED_SCRAMBLING flag are
> not enough), I'll need a detailed explanation ;-).
> 

yes, we saw this kind comment in DT patch already, we will try to fix this..
>> +
>> +	nand = &chip->nand;
>> +	nand_set_flash_node(nand, np);
>> +	nand_set_controller_data(nand, nfc);
>> +
>> +	nand->options |= NAND_USE_BOUNCE_BUFFER;
>> +	nand->select_chip = meson_nfc_select_chip;
>> +	nand->exec_op = meson_nfc_exec_op;
>> +	nand->setup_data_interface = meson_nfc_setup_data_interface;
>> +
>> +	nand->ecc.mode = NAND_ECC_HW;
>> +
>> +	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>> +	nand->ecc.write_page = meson_nfc_write_page_hwecc;
>> +	nand->ecc.write_oob_raw = nand_write_oob_std;
>> +	nand->ecc.write_oob = nand_write_oob_std;
>> +
>> +	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>> +	nand->ecc.read_page = meson_nfc_read_page_hwecc;
>> +	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
>> +	nand->ecc.read_oob = meson_nfc_read_oob;
> 
> We usually setup the ECC fields after the identification phase. This
> way, if you ever want/need to support SW ECC, the code is already
> properly placed.
> 
>> +
>> +	mtd = nand_to_mtd(nand);
>> +	mtd->owner = THIS_MODULE;
>> +	mtd->dev.parent = dev;
>> +
>> +	ret = nand_scan_ident(mtd, 1, NULL);
>> +	if (ret) {
>> +		dev_err(dev, "failed to can ident\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s:nand%s",
>> +				   dev_name(dev), cs_id);
> 
> So, if you follow my suggestion, it should be:
> 
> 
> You should make that conditional, because the core might have retrieved
> a user-friendly from the DT (label prop defined to the NAND chip node).
> 
> So, if you follow my suggestion to just use the first CS for the nand
> id, that gives:
> 
> 	if (!mtd->name) {
> 		mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> 					   "%s:nand%d",
> 					   dev_name(dev),
> 					   chip->sels[i]);
> 		if (!mtd->name)
> 			return -ENOMEM;
> 	}
sure
>> +
>> +	/* store bbt magic in page, cause OOB is not protected */
>> +	if (nand->bbt_options & NAND_BBT_USE_FLASH)
>> +		nand->bbt_options |= NAND_BBT_NO_OOB;
>> +
>> +	nand->options |= NAND_NO_SUBPAGE_WRITE;
>> +
>> +	ret = meson_nfc_ecc_init(dev, mtd);
>> +	if (ret) {
>> +		dev_err(dev, "failed to ecc init\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (nand->options & NAND_BUSWIDTH_16) {
>> +		dev_err(dev, "16bits buswidth not supported");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = meson_nfc_buffer_init(mtd);
>> +	if (ret)
>> +		return -ENOMEM;
>> +
>> +	ret = nand_scan_tail(mtd);
> 
> Miquel has reworked the nand_scan() infrastructure recently. Now you
> have to use nand_scan() and define ->attach_chip() hook to do all the
> init that happens between nand_scan_ident() and nand_scan_tail() in
> your code. And of course, all resources allocated in the
> ->attach_chip() hook should be freed in ->detach_chip().
> 
thanks, will look into this

>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register mtd device: %d\n", ret);
>> +		nand_cleanup(nand);
>> +		return ret;
>> +	}
>> +
>> +	list_add_tail(&chip->node, &nfc->chips);
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_nfc_nand_chip_cleanup(struct meson_nfc *nfc)
> 
> 			     ^ chips
> 
sure

>> +{
>> +	struct meson_nfc_nand_chip *chip;
>> +	struct mtd_info *mtd;
>> +	int ret;
>> +
>> +	while (!list_empty(&nfc->chips)) {
>> +		chip = list_first_entry(&nfc->chips, struct meson_nfc_nand_chip,
>> +					node);
>> +		mtd = nand_to_mtd(&chip->nand);
>> +		ret = mtd_device_unregister(mtd);
>> +		if (ret)
>> +			return ret;
>> +
>> +		nand_cleanup(&chip->nand);
>> +		list_del(&chip->node);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_nfc_nand_chips_init(struct device *dev, struct meson_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 = meson_nfc_nand_chip_init(dev, nfc, nand_np);
>> +		if (ret) {
>> +			meson_nfc_nand_chip_cleanup(nfc);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t meson_nfc_irq(int irq, void *id)
>> +{
>> +	struct meson_nfc *nfc = id;
>> +	u32 cfg;
>> +
>> +	cfg = readl(nfc->reg_base + NFC_REG_CFG);
>> +	cfg |= (1 << 21);
>> +	writel(cfg, nfc->reg_base + NFC_REG_CFG);
>> +
>> +	complete(&nfc->completion);
>> +	return IRQ_HANDLED;
> 
> Can you check if at least one event has been generated, and if not
> return IRQ_NONE?
> 
sure, will address this in next version
>> +}
>> +
>> +static const struct meson_nfc_data meson_gxl_data = {
>> +	.short_bch	= NFC_ECC_BCH60_1K,
>> +	.ecc		= meson_gxl_ecc,
>> +	.ecc_num	= ARRAY_SIZE(meson_gxl_ecc),
>> +};
>> +
>> +static const struct meson_nfc_data meson_axg_data = {
>> +	.short_bch	= NFC_ECC_BCH8_1K,
>> +	.ecc		= meson_axg_ecc,
>> +	.ecc_num	= ARRAY_SIZE(meson_axg_ecc),
>> +};
>> +
>> +static const struct of_device_id meson_nfc_id_table[] = {
>> +	{
>> +		.compatible = "amlogic,meson-gxl-nfc",
>> +		.data = &meson_gxl_data,
>> +	}, {
>> +		.compatible = "amlogic,meson-axg-nfc",
>> +		.data = &meson_axg_data,
>> +	},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
>> +
>> +static int meson_nfc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct meson_nfc *nfc;
>> +	struct resource *res;
>> +	int ret, irq;
>> +
>> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>> +	if (!nfc)
>> +		return -ENOMEM;
>> +
>> +	nfc->data =
>> +		(struct meson_nfc_data *)of_device_get_match_data(&pdev->dev);
> 
> You don't need the cast if you declare nfc->data as const in the struct
> def.
> 
ok

>> +	if (!nfc->data)
>> +		return -ENODEV;
>> +
>> +	spin_lock_init(&nfc->controller.lock);
>> +	init_waitqueue_head(&nfc->controller.wq);
> 
> There's a helper for that (nand_controller_init()).
> 
ok
>> +	INIT_LIST_HEAD(&nfc->chips);
>> +
>> +	nfc->dev = dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "Failed to nfc reg resource\n");
>> +		return -EINVAL;
>> +	}
> 
> No need to do this check, just pass res to devm_ioremap_resource() and
> it will do the check for you.
> 
>> +
>> +	nfc->reg_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(nfc->reg_base)) {
>> +		dev_err(dev, "Failed to lookup nfi reg base\n");
> 
> This error message is not needed, devm_ioremap_resource() complains
> already.
> 
> Just do:
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	nfc->reg_base = devm_ioremap_resource(dev, res);
> 	if (IS_ERR(nfc->reg_base))
> 		return PTR_ERR(nfc->reg_base);
> 

em.. indeed, thanks


>> +		return PTR_ERR(nfc->reg_base);
>> +	}
>> +
>> +	nfc->reg_clk =
>> +		syscon_regmap_lookup_by_phandle(dev->of_node,
>> +						"amlogic,mmc-syscon");
>> +	if (IS_ERR(nfc->reg_clk)) {
>> +		dev_err(dev, "Failed to lookup clock base\n");
>> +		return PTR_ERR(nfc->reg_clk);
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(dev, "no nfi irq resource\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = meson_nfc_clk_init(nfc);
>> +	if (ret) {
>> +		dev_err(dev, "failed to initialize nand clk\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
> 
> You should make sure all irqs are masked/cleared before setting up your
> irq handler.
> 
ok, will do
>> +	if (ret) {
>> +		dev_err(dev, "failed to request nfi irq\n");
>> +		ret = -EINVAL;
>> +		goto err_clk;
>> +	}
>> +
>> +	ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>> +	if (ret) {
>> +		dev_err(dev, "failed to set dma mask\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, nfc);
>> +
>> +	ret = meson_nfc_nand_chips_init(dev, nfc);
>> +	if (ret) {
>> +		dev_err(dev, "failed to init nand chips\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_clk:
>> +	meson_nfc_disable_clk(nfc);
>> +	return ret;
>> +}
> 
> Regards,
> 
> Boris
> 
> .
> 

Yixun

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

Hi Boris,
On 2018/8/2 5:50, Boris Brezillon wrote:

> Hi Yixun,
>
> On Thu, 19 Jul 2018 17:46:12 +0800
> Yixun Lan <yixun.lan@amlogic.com> wrote:
>
> I haven't finished reviewing the driver yet (I'll try to do that later
> this week), but I already pointed a few things to fix/improve.
>
>> +
>> +static int meson_nfc_exec_op(struct nand_chip *chip,
>> +			     const struct nand_operation *op, bool check_only)
>> +{
>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>> +	struct meson_nfc *nfc = nand_get_controller_data(chip);
>> +	const struct nand_op_instr *instr = NULL;
>> +	int ret = 0, cmd;
>> +	unsigned int op_id;
>> +	int i;
>> +
>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> +		instr = &op->instrs[op_id];
>> +		switch (instr->type) {
>> +		case NAND_OP_CMD_INSTR:
>> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
>> +			cmd |= instr->ctx.cmd.opcode & 0xff;
>> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
> This is not necessarily TWB you have to wait after a CMD cycle. It can
> be tWHR. And you should definitely not hardcode the value, since,
> AFAIR, it depends on the selected SDR timings. Probably something you
> should calculate in ->setup_data_interface().

Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
so I will delete it.

>> +			meson_nfc_drain_cmd(nfc);
> I don't know exactly how the NAND controller works, but it's usually
> not a good idea to execute the operation right away, especially if you
> have address/cmd/data cycles following this cmd and those can be
> packed in the same controller operation.

it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version

>> +			break;
>> +
>> +		case NAND_OP_ADDR_INSTR:
>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
>> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
>> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
>> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
>> +			}
>> +			break;
>> +
>> +		case NAND_OP_DATA_IN_INSTR:
>> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
>> +					   instr->ctx.data.len);
>> +			break;
>> +
>> +		case NAND_OP_DATA_OUT_INSTR:
>> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
>> +					    instr->ctx.data.len);
> Well, I'm not entirely sure what happens when you call
> read/write_buf(), but it seems you're doing that one byte at a time,
> and that sounds not so efficient given the operation you do for each
> byte read/written. Don't you have a way to tell the engine that you
> want to read/write X bytes?

As i known, there is no way to read/write X bytes once.

>> +			break;
>> +
>> +		case NAND_OP_WAITRDY_INSTR:
>> +			mdelay(instr->ctx.waitrdy.timeout_ms);
>> +			ret = nand_soft_waitrdy(chip,
>> +						instr->ctx.waitrdy.timeout_ms);
> Hm, i'd be surprised if the controller does not have a way to optimize
> waits on R/B transitions.

When i delete the delay here, erasing operation will be failed.
Does it mean NFC send 0x70 to nand device when rb is busy(low)?
If so, i will ask our NFC designer for comfirmation or grasping the waveform.

>> +			break;
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +			       struct mtd_oob_region *oobregion)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	int free_oob;
>> +
>> +	if (section >= chip->ecc.steps)
>> +		return -ERANGE;
>> +
>> +	free_oob = (section + 1) * 2;
>> +	oobregion->offset = section * chip->ecc.bytes + free_oob;
> Hm, this offset calculation looks weird. Are you sure it's correct?
> I'd bet on something like:
>
> 	oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));

Each ecc page have 2 user bytes. Assume one 2KB+64B page size nand
flash using ECC8/1KB which ecc parity bytes is 14B.
     _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    |             |  |      |             |  |     |  not  |
    |	1KB      |2B| 14B  |     1KB     |2B| 14B | used  |  (layout on nand)
    |_ _ _ _ _ _ _|_ |_ _ _ | _ _ _ _ _ _ |_ |_ _ _|_ _ _ _|
		    (2KB + 64B)
when reading from nand, I will format the page as follow:
     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _
    |             |             |  |     |  |      |  not  |
    |	1KB      |    1KB      |2B| 14B |2B|  14B | used  |(layout on ddr)
    |_ _ _ _ _ _ _|_ _ _ _ _ _ _|_ |_ _ _|_ |_ _ _ |_ _ _ _|
		    (2KB + 64B)
So i get "oobregion->offset = section * chip->ecc.bytes + free_oob".
Maybe i don't get what does 'section' mean. i think it means the ecc page number.

>> +	oobregion->length = chip->ecc.bytes;
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>> +				struct mtd_oob_region *oobregion)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +
>> +	if (section >= chip->ecc.steps)
>> +		return -ERANGE;
>> +
>> +	oobregion->offset = section * (2 + chip->ecc.bytes);
>> +	oobregion->length = 2;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops meson_ooblayout_ops = {
>> +	.ecc = meson_ooblayout_ecc,
>> +	.free = meson_ooblayout_free,
>> +};
>> +
>> +static int meson_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
>> +	const struct meson_nand_ecc *meson_ecc = nfc->data->ecc;
>> +	int num = nfc->data->ecc_num;
>> +	int nsectors, i, bytes;
>> +
>> +	/* support only ecc hw mode */
>> +	if (nand->ecc.mode != NAND_ECC_HW) {
>> +		dev_err(dev, "ecc.mode not supported\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!nand->ecc.size || !nand->ecc.strength) {
>> +		/* use datasheet requirements */
>> +		nand->ecc.strength = nand->ecc_strength_ds;
>> +		nand->ecc.size = nand->ecc_step_ds;
>> +	}
>> +
>> +	if (nand->ecc.options & NAND_ECC_MAXIMIZE) {
>> +		nand->ecc.size = 1024;
>> +		nsectors = mtd->writesize / nand->ecc.size;
>> +		bytes = mtd->oobsize - 2 * nsectors;
>> +		bytes /= nsectors;
>> +
>> +		/* and bytes has to be even. */
>> +		if (bytes % 2)
>> +			bytes--;
>> +
>> +		nand->ecc.strength = bytes * 8 / fls(8 * nand->ecc.size);
>> +	} else {
>> +		if (nand->ecc.strength > meson_ecc[num - 1].strength) {
>> +			dev_err(dev, "not support ecc strength\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (meson_ecc[i].strength == 0xff ||
>> +		    nand->ecc.strength < meson_ecc[i].strength)
>> +			break;
>> +	}
> I'd suggest that you look at nand_match_ecc_req(). It's likely that the
> selection logic you have here can be replaced by the generic function.

em, I will try it next version.

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

On Fri, 17 Aug 2018 21:03:59 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> Hi Boris,
> On 2018/8/2 5:50, Boris Brezillon wrote:
> 
> > Hi Yixun,
> >
> > On Thu, 19 Jul 2018 17:46:12 +0800
> > Yixun Lan <yixun.lan@amlogic.com> wrote:
> >
> > I haven't finished reviewing the driver yet (I'll try to do that later
> > this week), but I already pointed a few things to fix/improve.
> >  
> >> +
> >> +static int meson_nfc_exec_op(struct nand_chip *chip,
> >> +			     const struct nand_operation *op, bool check_only)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> +	struct meson_nfc *nfc = nand_get_controller_data(chip);
> >> +	const struct nand_op_instr *instr = NULL;
> >> +	int ret = 0, cmd;
> >> +	unsigned int op_id;
> >> +	int i;
> >> +
> >> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> >> +		instr = &op->instrs[op_id];
> >> +		switch (instr->type) {
> >> +		case NAND_OP_CMD_INSTR:
> >> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
> >> +			cmd |= instr->ctx.cmd.opcode & 0xff;
> >> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);  
> > This is not necessarily TWB you have to wait after a CMD cycle. It can
> > be tWHR. And you should definitely not hardcode the value, since,
> > AFAIR, it depends on the selected SDR timings. Probably something you
> > should calculate in ->setup_data_interface().  
> 
> Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
> so I will delete it.

Are you sure the engine always applies a tWHR delay, even when tWB is
expected? tWB should be applied everytime you are about to wait for a
R/B transition. tWHR is about switching IO pins from input to output on
the NAND chip side.

> 
> >> +			meson_nfc_drain_cmd(nfc);  
> > I don't know exactly how the NAND controller works, but it's usually
> > not a good idea to execute the operation right away, especially if you
> > have address/cmd/data cycles following this cmd and those can be
> > packed in the same controller operation.  
> 
> it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version

What's the CMD queue depth? I think you'll have to ensure the requested
op fits in the CMD FIFO and split things in several sub-ops if it does
not.

> 
> >> +			break;
> >> +
> >> +		case NAND_OP_ADDR_INSTR:
> >> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
> >> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
> >> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
> >> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >> +			}
> >> +			break;
> >> +
> >> +		case NAND_OP_DATA_IN_INSTR:
> >> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
> >> +					   instr->ctx.data.len);
> >> +			break;
> >> +
> >> +		case NAND_OP_DATA_OUT_INSTR:
> >> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
> >> +					    instr->ctx.data.len);  
> > Well, I'm not entirely sure what happens when you call
> > read/write_buf(), but it seems you're doing that one byte at a time,
> > and that sounds not so efficient given the operation you do for each
> > byte read/written. Don't you have a way to tell the engine that you
> > want to read/write X bytes?  
> 
> As i known, there is no way to read/write X bytes once.

Okay, then maybe you can queue several byte read/write reqs before
flushing the queue (meson_nfc_drain_cmd() +
meson_nfc_wait_cmd_finish()).

> 
> >> +			break;
> >> +
> >> +		case NAND_OP_WAITRDY_INSTR:
> >> +			mdelay(instr->ctx.waitrdy.timeout_ms);
> >> +			ret = nand_soft_waitrdy(chip,
> >> +						instr->ctx.waitrdy.timeout_ms);  
> > Hm, i'd be surprised if the controller does not have a way to optimize
> > waits on R/B transitions.  
> 
> When i delete the delay here, erasing operation will be failed.
> Does it mean NFC send 0x70 to nand device when rb is busy(low)?

I was not even talking about the delay, but yes, mdelay() seems way too
big. Remember that it's a timeout, and you usually don't have to wait
that much. You can do ndelay(instr->ctx.delay_ns) before calling
nand_soft_waitrdy() to make sure tWB is enforced.

Anyway, that's not what I was initially referring to. What I meant is
that nand_soft_waitrdy() should be replaced by native R/B pin or status
polling wait logic so that the CPU is released while waiting for a R/B
transition.

> If so, i will ask our NFC designer for comfirmation or grasping the waveform.

You have to wait tWB, that's for sure.

> 
> >> +			break;
> >> +		}
> >> +	}
> >> +	return ret;
> >> +}
> >> +
> >> +static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +			       struct mtd_oob_region *oobregion)
> >> +{
> >> +	struct nand_chip *chip = mtd_to_nand(mtd);
> >> +	int free_oob;
> >> +
> >> +	if (section >= chip->ecc.steps)
> >> +		return -ERANGE;
> >> +
> >> +	free_oob = (section + 1) * 2;
> >> +	oobregion->offset = section * chip->ecc.bytes + free_oob;  
> > Hm, this offset calculation looks weird. Are you sure it's correct?
> > I'd bet on something like:
> >
> > 	oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));  
> 
> Each ecc page have 2 user bytes. Assume one 2KB+64B page size nand
> flash using ECC8/1KB which ecc parity bytes is 14B.
>      _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
>     |             |  |      |             |  |     |  not  |
>     |	1KB      |2B| 14B  |     1KB     |2B| 14B | used  |  (layout on nand)
>     |_ _ _ _ _ _ _|_ |_ _ _ | _ _ _ _ _ _ |_ |_ _ _|_ _ _ _|
> 		    (2KB + 64B)
> when reading from nand, I will format the page as follow:
>      _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _
>     |             |             |  |     |  |      |  not  |
>     |	1KB      |    1KB      |2B| 14B |2B|  14B | used  |(layout on ddr)
>     |_ _ _ _ _ _ _|_ _ _ _ _ _ _|_ |_ _ _|_ |_ _ _ |_ _ _ _|
> 		    (2KB + 64B)
> So i get "oobregion->offset = section * chip->ecc.bytes + free_oob".

Okay, but I prefer when it's written like that:

	oobregion->offset = 2 + (section * (2 + chip->ecc.bytes));

> Maybe i don't get what does 'section' mean. i think it means the ecc page number.

Section is just the free OOB or ECC section number. It starts at 0 and
goes up to N - 1, where N usually is the number of ECC steps you have in
a page.

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

Hi Boris,

On 8/17/2018 9:56 PM, Boris Brezillon wrote:
> On Fri, 17 Aug 2018 21:03:59 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> Hi Boris,
>> On 2018/8/2 5:50, Boris Brezillon wrote:
>>
>>> Hi Yixun,
>>>
>>> On Thu, 19 Jul 2018 17:46:12 +0800
>>> Yixun Lan <yixun.lan@amlogic.com> wrote:
>>>
>>> I haven't finished reviewing the driver yet (I'll try to do that later
>>> this week), but I already pointed a few things to fix/improve.
>>>   
>>>> +
>>>> +static int meson_nfc_exec_op(struct nand_chip *chip,
>>>> +			     const struct nand_operation *op, bool check_only)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(chip);
>>>> +	const struct nand_op_instr *instr = NULL;
>>>> +	int ret = 0, cmd;
>>>> +	unsigned int op_id;
>>>> +	int i;
>>>> +
>>>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>>>> +		instr = &op->instrs[op_id];
>>>> +		switch (instr->type) {
>>>> +		case NAND_OP_CMD_INSTR:
>>>> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
>>>> +			cmd |= instr->ctx.cmd.opcode & 0xff;
>>>> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
>>> This is not necessarily TWB you have to wait after a CMD cycle. It can
>>> be tWHR. And you should definitely not hardcode the value, since,
>>> AFAIR, it depends on the selected SDR timings. Probably something you
>>> should calculate in ->setup_data_interface().
>>
>> Indeed. TWB is not necessarily. And tWHR will be promised by NFC.
>> so I will delete it.
> 
> Are you sure the engine always applies a tWHR delay, even when tWB is
> expected? tWB should be applied everytime you are about to wait for a
> R/B transition. tWHR is about switching IO pins from input to output on
> the NAND chip side.
> 

it seems work well even do not add tWHR, but software needs to promise 
tWHR, also the same as TWB. I will check the code and add them.

>>
>>>> +			meson_nfc_drain_cmd(nfc);
>>> I don't know exactly how the NAND controller works, but it's usually
>>> not a good idea to execute the operation right away, especially if you
>>> have address/cmd/data cycles following this cmd and those can be
>>> packed in the same controller operation.
>>
>> it doesn't need meson_nfc_drain_cmd(nfc) here. i will delete it next version
> 
> What's the CMD queue depth? I think you'll have to ensure the requested
> op fits in the CMD FIFO and split things in several sub-ops if it does
> not.
> 

there are maximum 32 commands.
I think it should be enough to promise ONE maximum number of 
operations(cmd - addr - dma - 2 ilde commands).

>>
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_ADDR_INSTR:
>>>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
>>>> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
>>>> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
>>>> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +			}
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_DATA_IN_INSTR:
>>>> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
>>>> +					   instr->ctx.data.len);
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_DATA_OUT_INSTR:
>>>> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
>>>> +					    instr->ctx.data.len);
>>> Well, I'm not entirely sure what happens when you call
>>> read/write_buf(), but it seems you're doing that one byte at a time,
>>> and that sounds not so efficient given the operation you do for each
>>> byte read/written. Don't you have a way to tell the engine that you
>>> want to read/write X bytes?
>>
>> As i known, there is no way to read/write X bytes once.
> 
> Okay, then maybe you can queue several byte read/write reqs before
> flushing the queue (meson_nfc_drain_cmd() +
> meson_nfc_wait_cmd_finish()).
> 
>>
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_WAITRDY_INSTR:
>>>> +			mdelay(instr->ctx.waitrdy.timeout_ms);
>>>> +			ret = nand_soft_waitrdy(chip,
>>>> +						instr->ctx.waitrdy.timeout_ms);
>>> Hm, i'd be surprised if the controller does not have a way to optimize
>>> waits on R/B transitions.
>>
>> When i delete the delay here, erasing operation will be failed.
>> Does it mean NFC send 0x70 to nand device when rb is busy(low)?
> 
> I was not even talking about the delay, but yes, mdelay() seems way too
> big. Remember that it's a timeout, and you usually don't have to wait
> that much. You can do ndelay(instr->ctx.delay_ns) before calling
> nand_soft_waitrdy() to make sure tWB is enforced.
> 
> Anyway, that's not what I was initially referring to. What I meant is
> that nand_soft_waitrdy() should be replaced by native R/B pin or status
> polling wait logic so that the CPU is released while waiting for a R/B
> transition.
> 
>> If so, i will ask our NFC designer for comfirmation or grasping the waveform.
> 
> You have to wait tWB, that's for sure.
> 

Indeed.

>>
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int meson_ooblayout_ecc(struct mtd_info *mtd, int section,
>>>> +			       struct mtd_oob_region *oobregion)
>>>> +{
>>>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>>>> +	int free_oob;
>>>> +
>>>> +	if (section >= chip->ecc.steps)
>>>> +		return -ERANGE;
>>>> +
>>>> +	free_oob = (section + 1) * 2;
>>>> +	oobregion->offset = section * chip->ecc.bytes + free_oob;
>>> Hm, this offset calculation looks weird. Are you sure it's correct?
>>> I'd bet on something like:
>>>
>>> 	oobregion->offset = 2 + (section * (chip->ecc.bytes + 4));
>>
>> Each ecc page have 2 user bytes. Assume one 2KB+64B page size nand
>> flash using ECC8/1KB which ecc parity bytes is 14B.
>>       _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
>>      |             |  |      |             |  |     |  not  |
>>      |	1KB      |2B| 14B  |     1KB     |2B| 14B | used  |  (layout on nand)
>>      |_ _ _ _ _ _ _|_ |_ _ _ | _ _ _ _ _ _ |_ |_ _ _|_ _ _ _|
>> 		    (2KB + 64B)
>> when reading from nand, I will format the page as follow:
>>       _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _
>>      |             |             |  |     |  |      |  not  |
>>      |	1KB      |    1KB      |2B| 14B |2B|  14B | used  |(layout on ddr)
>>      |_ _ _ _ _ _ _|_ _ _ _ _ _ _|_ |_ _ _|_ |_ _ _ |_ _ _ _|
>> 		    (2KB + 64B)
>> So i get "oobregion->offset = section * chip->ecc.bytes + free_oob".
> 
> Okay, but I prefer when it's written like that:
> 
> 	oobregion->offset = 2 + (section * (2 + chip->ecc.bytes));

em, I prefer too. it looks better.
> >> Maybe i don't get what does 'section' mean. i think it means the ecc 
page number.
> 
> Section is just the free OOB or ECC section number. It starts at 0 and
> goes up to N - 1, where N usually is the number of ECC steps you have in
> a page.
> 
> .
> 

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

Hi Boris,

There is a question below. please see my comments.

Thanks.

On 8/17/2018 9:56 PM, Boris Brezillon wrote:
> On Fri, 17 Aug 2018 21:03:59 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> Hi Boris,
>> On 2018/8/2 5:50, Boris Brezillon wrote:
>>
>>> Hi Yixun,
>>>
>>> On Thu, 19 Jul 2018 17:46:12 +0800
>>> Yixun Lan <yixun.lan@amlogic.com> wrote:
>>>
>>> I haven't finished reviewing the driver yet (I'll try to do that later
>>> this week), but I already pointed a few things to fix/improve.
>>>   
>>>> +
>>>> +static int meson_nfc_exec_op(struct nand_chip *chip,
>>>> +			     const struct nand_operation *op, bool check_only)
>>>> +{
>>>> +	struct mtd_info *mtd = nand_to_mtd(chip);
>>>> +	struct meson_nfc *nfc = nand_get_controller_data(chip);
>>>> +	const struct nand_op_instr *instr = NULL;
>>>> +	int ret = 0, cmd;
>>>> +	unsigned int op_id;
>>>> +	int i;
>>>> +
>>>> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
>>>> +		instr = &op->instrs[op_id];
>>>> +		switch (instr->type) {
>>>> +		case NAND_OP_CMD_INSTR:
>>>> +			cmd = nfc->param.chip_select | NFC_CMD_CLE;
>>>> +			cmd |= instr->ctx.cmd.opcode & 0xff;
>>>> +			writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +			meson_nfc_cmd_idle(nfc, NAND_TWB_TIME_CYCLE);
>>
>>>> +			meson_nfc_drain_cmd(nfc);
>>> I don't know exactly how the NAND controller works, but it's usually
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_ADDR_INSTR:
>>>> +			for (i = 0; i < instr->ctx.addr.naddrs; i++) {
>>>> +				cmd = nfc->param.chip_select | NFC_CMD_ALE;
>>>> +				cmd |= instr->ctx.addr.addrs[i] & 0xff;
>>>> +				writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> +			}
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_DATA_IN_INSTR:
>>>> +			meson_nfc_read_buf(mtd, instr->ctx.data.buf.in,
>>>> +					   instr->ctx.data.len);
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_DATA_OUT_INSTR:
>>>> +			meson_nfc_write_buf(mtd, instr->ctx.data.buf.out,
>>>> +					    instr->ctx.data.len);
> 
>>
>>>> +			break;
>>>> +
>>>> +		case NAND_OP_WAITRDY_INSTR:
>>>> +			mdelay(instr->ctx.waitrdy.timeout_ms);
>>>> +			ret = nand_soft_waitrdy(chip,
>>>> +						instr->ctx.waitrdy.timeout_ms);
>>> Hm, i'd be surprised if the controller does not have a way to optimize
>>> waits on R/B transitions.
>>
>> When i delete the delay here, erasing operation will be failed.
>> Does it mean NFC send 0x70 to nand device when rb is busy(low)?
> 
> I was not even talking about the delay, but yes, mdelay() seems way too
> big. Remember that it's a timeout, and you usually don't have to wait
> that much. You can do ndelay(instr->ctx.delay_ns) before calling
> nand_soft_waitrdy() to make sure tWB is enforced.
> 
> Anyway, that's not what I was initially referring to. What I meant is
> that nand_soft_waitrdy() should be replaced by native R/B pin or status
> polling wait logic so that the CPU is released while waiting for a R/B
> transition.
> 
>> If so, i will ask our NFC designer for comfirmation or grasping the waveform.
> 
> You have to wait tWB, that's for sure.
> 
we have a maximum 32 commands fifo. when command is written into 
NFC_REG_CMD, it doesn't mean that command is executing right now, maybe 
it is buffering on the queue.Assume one ERASE operation, when 2nd 
command(0xd0) is written into NFC_REG_CMD and then come into 
NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be 
wrong because 0xd0 may not being executed. it is unusual unless 
buffering two many command.
so it seems that i still need to use nand_soft_waitrdy or wait cmd is 
executed somewhere.
>>
> 

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

On Wed, 22 Aug 2018 22:08:42 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> > You have to wait tWB, that's for sure.
> >   
> we have a maximum 32 commands fifo. when command is written into 
> NFC_REG_CMD, it doesn't mean that command is executing right now, maybe 
> it is buffering on the queue.Assume one ERASE operation, when 2nd 
> command(0xd0) is written into NFC_REG_CMD and then come into 
> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be 
> wrong because 0xd0 may not being executed. it is unusual unless 
> buffering two many command.

You should flush the queue and wait for it to empty at the end of
->exec_op().

> so it seems that i still need to use nand_soft_waitrdy or wait cmd is 
> executed somewhere.

Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
probably what you should use for tWB.

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

Hi Boris,

On 8/24/2018 8:48 PM, Boris Brezillon wrote:
> On Wed, 22 Aug 2018 22:08:42 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>>> You have to wait tWB, that's for sure.
>>>    
>> we have a maximum 32 commands fifo. when command is written into
>> NFC_REG_CMD, it doesn't mean that command is executing right now, maybe
>> it is buffering on the queue.Assume one ERASE operation, when 2nd
>> command(0xd0) is written into NFC_REG_CMD and then come into
>> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
>> wrong because 0xd0 may not being executed. it is unusual unless
>> buffering two many command.
> 
> You should flush the queue and wait for it to empty at the end of
> ->exec_op().
> 
>> so it seems that i still need to use nand_soft_waitrdy or wait cmd is
>> executed somewhere.
> 
> Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
> NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
> probably what you should use for tWB.
> 
> em, I can wait for RB by reading the status from register now. but when 
calling nand_soft_waitrdy, i really met a problem. One *jiffies* is 
about 4ms. When programming, it pass 1ms to 
instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one 
*jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at 
the tail of 4ms interval, it may only wait 100us and next jiffies 
arrive. Is it correct?
> 

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

On Tue, 28 Aug 2018 21:21:48 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> Hi Boris,
> 
> On 8/24/2018 8:48 PM, Boris Brezillon wrote:
> > On Wed, 22 Aug 2018 22:08:42 +0800
> > Liang Yang <liang.yang@amlogic.com> wrote:
> >   
> >>> You have to wait tWB, that's for sure.
> >>>      
> >> we have a maximum 32 commands fifo. when command is written into
> >> NFC_REG_CMD, it doesn't mean that command is executing right now, maybe
> >> it is buffering on the queue.Assume one ERASE operation, when 2nd
> >> command(0xd0) is written into NFC_REG_CMD and then come into
> >> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
> >> wrong because 0xd0 may not being executed. it is unusual unless
> >> buffering two many command.  
> > 
> > You should flush the queue and wait for it to empty at the end of  
> > ->exec_op().  
> >   
> >> so it seems that i still need to use nand_soft_waitrdy or wait cmd is
> >> executed somewhere.  
> > 
> > Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
> > NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
> > probably what you should use for tWB.
> > 
> > em, I can wait for RB by reading the status from register now. but when   
> calling nand_soft_waitrdy, i really met a problem. One *jiffies* is 
> about 4ms. When programming, it pass 1ms to 
> instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one 
> *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at 
> the tail of 4ms interval, it may only wait 100us and next jiffies 
> arrive. Is it correct?

Hm, no. If you initialize the time you compare to (using time_before()
or time_after()) correctly it should not happen. Anyway, I keep thinking
this is not how it should be done. Did you try NFC_CMD_RB? Did you ask
HW designers what it was created for?

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

On 8/28/2018 9:26 PM, Boris Brezillon wrote:
> On Tue, 28 Aug 2018 21:21:48 +0800
> Liang Yang <liang.yang@amlogic.com> wrote:
> 
>> Hi Boris,
>>
>> On 8/24/2018 8:48 PM, Boris Brezillon wrote:
>>> On Wed, 22 Aug 2018 22:08:42 +0800
>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>    
>>>>> You have to wait tWB, that's for sure.
>>>>>       
>>>> we have a maximum 32 commands fifo. when command is written into
>>>> NFC_REG_CMD, it doesn't mean that command is executing right now, maybe
>>>> it is buffering on the queue.Assume one ERASE operation, when 2nd
>>>> command(0xd0) is written into NFC_REG_CMD and then come into
>>>> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
>>>> wrong because 0xd0 may not being executed. it is unusual unless
>>>> buffering two many command.
>>>
>>> You should flush the queue and wait for it to empty at the end of
>>> ->exec_op().
>>>    
>>>> so it seems that i still need to use nand_soft_waitrdy or wait cmd is
>>>> executed somewhere.
>>>
>>> Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
>>> NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
>>> probably what you should use for tWB.
>>>
>>> em, I can wait for RB by reading the status from register now. but when
>> calling nand_soft_waitrdy, i really met a problem. One *jiffies* is
>> about 4ms. When programming, it pass 1ms to
>> instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one
>> *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at
>> the tail of 4ms interval, it may only wait 100us and next jiffies
>> arrive. Is it correct?
> 
> Hm, no. If you initialize the time you compare to (using time_before()
> or time_after()) correctly it should not happen. Anyway, I keep thinking
> this is not how it should be done. Did you try NFC_CMD_RB? Did you ask
> HW designers what it was created for?
> 
I am using NFC_CMD_RB and checking with irq. it is ok now.
> .
> 

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Liang Yang]

On 8/29/2018 6:08 PM, Liang Yang wrote:
> 
> On 8/28/2018 9:26 PM, Boris Brezillon wrote:
>> On Tue, 28 Aug 2018 21:21:48 +0800
>> Liang Yang <liang.yang@amlogic.com> wrote:
>>
>>> Hi Boris,
>>>
>>> On 8/24/2018 8:48 PM, Boris Brezillon wrote:
>>>> On Wed, 22 Aug 2018 22:08:42 +0800
>>>> Liang Yang <liang.yang@amlogic.com> wrote:
>>>>>> You have to wait tWB, that's for sure.
>>>>> we have a maximum 32 commands fifo. when command is written into
>>>>> NFC_REG_CMD, it doesn't mean that command is executing right now, 
>>>>> maybe
>>>>> it is buffering on the queue.Assume one ERASE operation, when 2nd
>>>>> command(0xd0) is written into NFC_REG_CMD and then come into
>>>>> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
>>>>> wrong because 0xd0 may not being executed. it is unusual unless
>>>>> buffering two many command.
>>>>
>>>> You should flush the queue and wait for it to empty at the end of
>>>> ->exec_op().
>>>>> so it seems that i still need to use nand_soft_waitrdy or wait cmd is
>>>>> executed somewhere.
>>>>
>>>> Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
>>>> NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
>>>> probably what you should use for tWB.
>>>>
>>>> em, I can wait for RB by reading the status from register now. but when
>>> calling nand_soft_waitrdy, i really met a problem. One *jiffies* is
>>> about 4ms. When programming, it pass 1ms to
>>> instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one
>>> *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at
>>> the tail of 4ms interval, it may only wait 100us and next jiffies
>>> arrive. Is it correct?
>>
>> Hm, no. If you initialize the time you compare to (using time_before()
>> or time_after()) correctly it should not happen. Anyway, I keep thinking
>> this is not how it should be done. Did you try NFC_CMD_RB? Did you ask
>> HW designers what it was created for?
>>
> I am using NFC_CMD_RB and checking with irq. it is ok now.
there are two usages for NFC_CMD_RB. One reads the data status 
continuously by hardware after sending 0x70 command; the other checks 
the r/b IO status continuously.both can send irq when r/b is ready.

>> .
>>

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

On Wed, 29 Aug 2018 18:29:05 +0800
Liang Yang <liang.yang@amlogic.com> wrote:

> On 8/29/2018 6:08 PM, Liang Yang wrote:
> > 
> > On 8/28/2018 9:26 PM, Boris Brezillon wrote:  
> >> On Tue, 28 Aug 2018 21:21:48 +0800
> >> Liang Yang <liang.yang@amlogic.com> wrote:
> >>  
> >>> Hi Boris,
> >>>
> >>> On 8/24/2018 8:48 PM, Boris Brezillon wrote:  
> >>>> On Wed, 22 Aug 2018 22:08:42 +0800
> >>>> Liang Yang <liang.yang@amlogic.com> wrote:  
> >>>>>> You have to wait tWB, that's for sure.  
> >>>>> we have a maximum 32 commands fifo. when command is written into
> >>>>> NFC_REG_CMD, it doesn't mean that command is executing right now, 
> >>>>> maybe
> >>>>> it is buffering on the queue.Assume one ERASE operation, when 2nd
> >>>>> command(0xd0) is written into NFC_REG_CMD and then come into
> >>>>> NAND_OP_WAITRDY_INSTR, if I read the RB status by register, it may be
> >>>>> wrong because 0xd0 may not being executed. it is unusual unless
> >>>>> buffering two many command.  
> >>>>
> >>>> You should flush the queue and wait for it to empty at the end of  
> >>>> ->exec_op().
> >>>>> so it seems that i still need to use nand_soft_waitrdy or wait cmd is
> >>>>> executed somewhere.  
> >>>>
> >>>> Don't you have a WAIT_FOR_RB instruction? What is NFC_CMD_RB for? Also,
> >>>> NFC_CMD_IDLE seems to allow you to add an arbitrary delay, and that's
> >>>> probably what you should use for tWB.
> >>>>
> >>>> em, I can wait for RB by reading the status from register now. but when  
> >>> calling nand_soft_waitrdy, i really met a problem. One *jiffies* is
> >>> about 4ms. When programming, it pass 1ms to
> >>> instr->ctx.waitrdy.timeout_ms and nand_soft_waitrdy will be only one
> >>> *jiffies* to reach timeout. And then calling nand_soft_waitrdy maybe at
> >>> the tail of 4ms interval, it may only wait 100us and next jiffies
> >>> arrive. Is it correct?  
> >>
> >> Hm, no. If you initialize the time you compare to (using time_before()
> >> or time_after()) correctly it should not happen. Anyway, I keep thinking
> >> this is not how it should be done. Did you try NFC_CMD_RB? Did you ask
> >> HW designers what it was created for?
> >>  
> > I am using NFC_CMD_RB and checking with irq. it is ok now.  
> there are two usages for NFC_CMD_RB. One reads the data status 
> continuously by hardware after sending 0x70 command; the other checks 
> the r/b IO status continuously.both can send irq when r/b is ready.

Both should do what you expect, so I guess you're good.

Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

[Boris Brezillon]

Hi Yixun,

I know I said I would finish reviewing the driver, but I didn't have
time to do it, so feel free to send a new version addressing the
comments I already made.

On Thu, 19 Jul 2018 17:46:12 +0800
Yixun Lan <yixun.lan@amlogic.com> wrote:


> +static void meson_nfc_select_chip(struct mtd_info *mtd, int chip)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +
> +	if (chip < 0 || chip > MAX_CE_NUM)

			^ chip > meson_chip->nsels)

> +		return;
> +
> +	nfc->param.chip_select = meson_chip->sels[chip] ? NAND_CE1 : NAND_CE0;
> +	nfc->param.rb_select = nfc->param.chip_select;
> +}
> +

Regards,

Boris