[U-Boot] [PATCH 0/8] Bring NAND support to Nintendo NES Classic

[U-Boot] [PATCH 0/8] Bring NAND support to Nintendo NES Classic

[Miquel Raynal]

Hello,

This series adds fixes to sunxi NAND drivers (SPL and U-Boot), in order
to later migrate the SPL driver to use PIO instead of DMA to have more
generic code (working also on a A33). Finally, NAND support is added to
Nintendo NES Classic through Kconfig and DT additions.

Thanks,
Miquèl

Miquel Raynal (8):
  mtd: nand: sunxi: Fix strength minimum value
  spl: nand: sunxi: Fix second case of modulo by zero error
  sunxi: Allow SPL to be compiled for sun8i platforms
  spl: nand: sunxi: Enhancements and cleaning
  spl: nand: sunxi: use PIO instead of DMA
  configs: Add NAND support for NES Classic
  sunxi: dts: Add NAND node to sun8i DTSI
  sunxi: dts: Enable NAND on NES classic

 arch/arm/dts/sun8i-a23-a33.dtsi                    |  31 +++
 arch/arm/dts/sun8i-a33.dtsi                        |   8 +
 .../dts/sun8i-r16-nintendo-nes-classic-edition.dts |  14 ++
 arch/arm/include/asm/arch-sunxi/clock_sun6i.h      |   6 +
 board/sunxi/board.c                                |   4 +-
 configs/Nintendo_NES_Classic_Edition_defconfig     |   4 +
 drivers/mtd/nand/Kconfig                           |   2 +-
 drivers/mtd/nand/sunxi_nand.c                      |   1 +
 drivers/mtd/nand/sunxi_nand_spl.c                  | 236 +++++++++++----------
 9 files changed, 188 insertions(+), 118 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH 1/8] mtd: nand: sunxi: Fix strength minimum value

[Miquel Raynal]

When no requirement in Device Tree is given about the ECC strength and
step size, the engine should fallback on the minimal working case for
this engine (16b/1024B) instead of the NAND chip requirement which might
be simply unreachable.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index 8bc3828854..e8e7ad8ac5 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
 		goto err;
 	}
 
+	ecc->strength = strengths[i];
 	data->mode = i;
 
 	/* HW ECC always request ECC bytes for 1024 bytes blocks */
-- 
2.11.0

[U-Boot] [PATCH 1/8] mtd: nand: sunxi: Fix strength minimum value

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:47AM +0100, Miquel Raynal wrote:
> When no requirement in Device Tree is given about the ECC strength and
> step size, the engine should fallback on the minimal working case for
> this engine (16b/1024B) instead of the NAND chip requirement which might
> be simply unreachable.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/sunxi_nand.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> index 8bc3828854..e8e7ad8ac5 100644
> --- a/drivers/mtd/nand/sunxi_nand.c
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
>  		goto err;
>  	}
>  
> +	ecc->strength = strengths[i];

You should have a comment here as well mentionning why you're doing
this (your commit log would be a good start :))

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/bf0b9e09/attachment.sig>

[U-Boot] [PATCH 1/8] mtd: nand: sunxi: Fix strength minimum value

[Miquel Raynal]

Hello Maxime,

On Wed, 24 Jan 2018 08:46:18 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:47AM +0100, Miquel Raynal wrote:
> > When no requirement in Device Tree is given about the ECC strength and
> > step size, the engine should fallback on the minimal working case for
> > this engine (16b/1024B) instead of the NAND chip requirement which might
> > be simply unreachable.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/sunxi_nand.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> > index 8bc3828854..e8e7ad8ac5 100644
> > --- a/drivers/mtd/nand/sunxi_nand.c
> > +++ b/drivers/mtd/nand/sunxi_nand.c
> > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
> >  		goto err;
> >  	}
> >  
> > +	ecc->strength = strengths[i];  
> 
> You should have a comment here as well mentionning why you're doing
> this (your commit log would be a good start :))

Sure, I also moved that line above, where the strength is selected
(with a comment). This way it seems more clear to me why this is needed.

Thanks,
Miquèl

[U-Boot] [PATCH 1/8] mtd: nand: sunxi: Fix strength minimum value

[Boris Brezillon]

On Wed, 24 Jan 2018 01:44:47 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> When no requirement in Device Tree is given about the ECC strength and
> step size, the engine should fallback on the minimal working case for
> this engine (16b/1024B) instead of the NAND chip requirement which might
> be simply unreachable.

Actually, this is not what this patch is fixing. It fixes all cases
where the requested ECC strength does not exactly match the strengths
supported by the ECC engine. In this case, the driver is selecting the
closest strength meeting the 'selected_strength > requested_strength'
constraint. The problem was that we were not updating ecc->strength
with the real strength, which is what you're fixing here.

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/sunxi_nand.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> index 8bc3828854..e8e7ad8ac5 100644
> --- a/drivers/mtd/nand/sunxi_nand.c
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
>  		goto err;
>  	}
>  
> +	ecc->strength = strengths[i];
>  	data->mode = i;
>  
>  	/* HW ECC always request ECC bytes for 1024 bytes blocks */

[U-Boot] [PATCH 1/8] mtd: nand: sunxi: Fix strength minimum value

[Miquel Raynal]

Hello Boris,

On Wed, 24 Jan 2018 08:57:23 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 24 Jan 2018 01:44:47 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > When no requirement in Device Tree is given about the ECC strength and
> > step size, the engine should fallback on the minimal working case for
> > this engine (16b/1024B) instead of the NAND chip requirement which might
> > be simply unreachable.  
> 
> Actually, this is not what this patch is fixing. It fixes all cases
> where the requested ECC strength does not exactly match the strengths
> supported by the ECC engine. In this case, the driver is selecting the
> closest strength meeting the 'selected_strength > requested_strength'
> constraint. The problem was that we were not updating ecc->strength
> with the real strength, which is what you're fixing here.

That is entirely right, I kept one explanation on the easiest case
where this is needed: when there is not ECC-related property in the DT.

> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/sunxi_nand.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> > index 8bc3828854..e8e7ad8ac5 100644
> > --- a/drivers/mtd/nand/sunxi_nand.c
> > +++ b/drivers/mtd/nand/sunxi_nand.c
> > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
> >  		goto err;
> >  	}
> >  
> > +	ecc->strength = strengths[i];
> >  	data->mode = i;
> >  
> >  	/* HW ECC always request ECC bytes for 1024 bytes blocks */  
> 



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

[U-Boot] [PATCH 2/8] spl: nand: sunxi: Fix second case of modulo by zero error

[Miquel Raynal]

In the nand_read_buffer() step, the seed is calculated by doing a modulo
by conf->nseeds which is always zero when not using the randomizer (most
of SLC NANDs).

This situation turns out to lead to a run time freeze.

Derive this seed only when the randomizer is enabled (and conf->nseeds
logically not zero).

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index eed4472bdc..06695fc15f 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -475,11 +475,12 @@ static int nand_detect_config(struct nfc_config *conf, u32 offs, void *dest)
 static int nand_read_buffer(struct nfc_config *conf, uint32_t offs,
 			    unsigned int size, void *dest)
 {
-	int first_seed, page, ret;
+	int first_seed = 0, page, ret;
 
 	size = ALIGN(size, conf->page_size);
 	page = offs / conf->page_size;
-	first_seed = page % conf->nseeds;
+	if (conf->randomize)
+		first_seed = page % conf->nseeds;
 
 	for (; size; size -= conf->page_size) {
 		if (nand_load_page(conf, offs))
-- 
2.11.0

[U-Boot] [PATCH 2/8] spl: nand: sunxi: Fix second case of modulo by zero error

[Maxime Ripard]

On Wed, Jan 24, 2018 at 01:44:48AM +0100, Miquel Raynal wrote:
> In the nand_read_buffer() step, the seed is calculated by doing a modulo
> by conf->nseeds which is always zero when not using the randomizer (most
> of SLC NANDs).
> 
> This situation turns out to lead to a run time freeze.
> 
> Derive this seed only when the randomizer is enabled (and conf->nseeds
> logically not zero).

Mentionning the first occurence of that issue in your commit log would
be great, especially since the situation depends on configuration
options.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/08e917a1/attachment.sig>

[U-Boot] [PATCH 2/8] spl: nand: sunxi: Fix second case of modulo by zero error

[Miquel Raynal]

Hi Maxime,

On Wed, 24 Jan 2018 08:47:31 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> On Wed, Jan 24, 2018 at 01:44:48AM +0100, Miquel Raynal wrote:
> > In the nand_read_buffer() step, the seed is calculated by doing a modulo
> > by conf->nseeds which is always zero when not using the randomizer (most
> > of SLC NANDs).
> > 
> > This situation turns out to lead to a run time freeze.
> > 
> > Derive this seed only when the randomizer is enabled (and conf->nseeds
> > logically not zero).  
> 
> Mentionning the first occurence of that issue in your commit log would
> be great, especially since the situation depends on configuration
> options.

Absolutely, I added a reference in the commit message to:
ea3f750c73e3 ("nand: sunxi: Fix modulo by zero error")

Thanks,
Miquèl

[U-Boot] [PATCH 3/8] sunxi: Allow SPL to be compiled for sun8i platforms

[Miquel Raynal]

Add some clocks/PLL definitions and the dependency on MACH_SUN8I in
Kconfig so the SPL could be compiled to boards using A33 SoCs.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 arch/arm/include/asm/arch-sunxi/clock_sun6i.h | 6 ++++++
 drivers/mtd/nand/Kconfig                      | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
index d328df9597..d35aa479f7 100644
--- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
+++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
@@ -192,6 +192,7 @@ struct sunxi_ccm_reg {
 #define ATB_DIV_1			0
 #define ATB_DIV_2			1
 #define ATB_DIV_4			2
+#define AHB_DIV_1			0
 #define CPU_CLK_SRC_OSC24M		1
 #define CPU_CLK_SRC_PLL1		2
 
@@ -317,6 +318,11 @@ struct sunxi_ccm_reg {
 #define AHB_GATE_OFFSET_LCD0		3
 #endif
 
+#define CCM_NAND_CTRL_M(x)		((x) - 1)
+#define CCM_NAND_CTRL_N(x)		((x) << 16)
+#define CCM_NAND_CTRL_PLL6		(0x1 << 24)
+#define CCM_NAND_CTRL_ENABLE		(0x1 << 31)
+
 #define CCM_MMC_CTRL_M(x)		((x) - 1)
 #define CCM_MMC_CTRL_OCLK_DLY(x)	((x) << 8)
 #define CCM_MMC_CTRL_N(x)		((x) << 16)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 85b26d6088..2f37a8014d 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -78,7 +78,7 @@ config NAND_PXA3XX
 
 config NAND_SUNXI
 	bool "Support for NAND on Allwinner SoCs"
-	depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
+	depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I || MACH_SUN8I
 	select SYS_NAND_SELF_INIT
 	select SYS_NAND_U_BOOT_LOCATIONS
 	imply CMD_NAND
-- 
2.11.0

[U-Boot] [PATCH 3/8] sunxi: Allow SPL to be compiled for sun8i platforms

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:49AM +0100, Miquel Raynal wrote:
> Add some clocks/PLL definitions and the dependency on MACH_SUN8I in
> Kconfig so the SPL could be compiled to boards using A33 SoCs.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

The SPL can already be compiled for these platforms. What you're doing
is allowing the NAND support to be compiled in the SPL.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/3341abde/attachment.sig>

[U-Boot] [PATCH 3/8] sunxi: Allow SPL to be compiled for sun8i platforms

[Miquel Raynal]

Hi Maxime,

On Wed, 24 Jan 2018 08:49:10 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:49AM +0100, Miquel Raynal wrote:
> > Add some clocks/PLL definitions and the dependency on MACH_SUN8I in
> > Kconfig so the SPL could be compiled to boards using A33 SoCs.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>  
> 
> The SPL can already be compiled for these platforms. What you're doing
> is allowing the NAND support to be compiled in the SPL.

Corrected.

Thanks,
Miquèl

> 
> Maxime
> 



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

[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

[Miquel Raynal]

Do some cleaning in sunxi NAND SPL driver like adding helpers and
clearing flags at the right spot

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 64 +++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 06695fc15f..2488d5cb51 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -55,8 +55,8 @@
 
 
 #define NFC_ADDR_NUM_OFFSET        16
-#define NFC_SEND_ADR               (1 << 19)
 #define NFC_ACCESS_DIR             (1 << 20)
+#define NFC_SEND_ADDR              (1 << 19)
 #define NFC_DATA_TRANS             (1 << 21)
 #define NFC_SEND_CMD1              (1 << 22)
 #define NFC_WAIT_FLAG              (1 << 23)
@@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int unexpected_bits,
 	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
 }
 
+static int nand_wait_cmd_fifo_empty(void)
+{
+	if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT,
+				 DEFAULT_TIMEOUT_US)) {
+		printf("nand: timeout waiting for empty cmd FIFO\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int nand_wait_int(void)
+{
+	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
+			 DEFAULT_TIMEOUT_US)) {
+		printf("nand: timeout waiting for interruption\n");
+		return -ETIMEDOUT;
+	}
+
+	udelay(1);
+
+	return 0;
+}
+
 void nand_init(void)
 {
 	uint32_t val;
@@ -172,22 +196,21 @@ void nand_init(void)
 	}
 
 	/* reset NAND */
+	nand_wait_cmd_fifo_empty();
+
 	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
 	       SUNXI_NFC_BASE + NFC_CMD);
 
-	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
-			 DEFAULT_TIMEOUT_US)) {
-		printf("Error timeout waiting for nand reset\n");
-		return;
-	}
-	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
+	nand_wait_int();
 }
 
 static void nand_apply_config(const struct nfc_config *conf)
 {
 	u32 val;
 
+	nand_wait_cmd_fifo_empty();
+
 	val = readl(SUNXI_NFC_BASE + NFC_CTL);
 	val &= ~NFC_CTL_PAGE_SIZE_MASK;
 	writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
@@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
 {
 	int page = offs / conf->page_size;
 
+	nand_wait_cmd_fifo_empty();
+
 	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
 	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
 	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
@@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
 	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
 	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
-	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
+	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,
 	       SUNXI_NFC_BASE + NFC_CMD);
 
-	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
-			 DEFAULT_TIMEOUT_US)) {
-		printf("Error while initializing dma interrupt\n");
-		return -EIO;
-	}
-
-	return 0;
+	return nand_wait_int();
 }
 
 static int nand_reset_column(void)
 {
+	nand_wait_cmd_fifo_empty();
+
 	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
 	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
 	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
 	       SUNXI_NFC_BASE + NFC_RCMD_SET);
+	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 	writel(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
 	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
-	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
+	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
 	       SUNXI_NFC_BASE + NFC_CMD);
 
-	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
-			 DEFAULT_TIMEOUT_US)) {
-		printf("Error while initializing dma interrupt\n");
-		return -1;
-	}
+	return nand_wait_int();
 
-	return 0;
 }
 
+static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
+
 static int nand_read_page(const struct nfc_config *conf, u32 offs,
 			  void *dest, int len)
 {
@@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
 
 static int nand_max_ecc_strength(struct nfc_config *conf)
 {
-	static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116 };
 	int max_oobsize, max_ecc_bytes;
 	int nsectors = conf->page_size / conf->ecc_size;
 	int i;
-- 
2.11.0

[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:50AM +0100, Miquel Raynal wrote:
> Do some cleaning in sunxi NAND SPL driver like adding helpers and
> clearing flags at the right spot
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

I'm not really fond of these kind of wildcard patches, especially
since..

> ---
>  drivers/mtd/nand/sunxi_nand_spl.c | 64 +++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> index 06695fc15f..2488d5cb51 100644
> --- a/drivers/mtd/nand/sunxi_nand_spl.c
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -55,8 +55,8 @@
>  
>  
>  #define NFC_ADDR_NUM_OFFSET        16
> -#define NFC_SEND_ADR               (1 << 19)
>  #define NFC_ACCESS_DIR             (1 << 20)
> +#define NFC_SEND_ADDR              (1 << 19)
>  #define NFC_DATA_TRANS             (1 << 21)
>  #define NFC_SEND_CMD1              (1 << 22)
>  #define NFC_WAIT_FLAG              (1 << 23)

... here you reorder a single value, which means that the set of these
values is not sorted anymore ...

> @@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int unexpected_bits,
>  	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
>  }
>  
> +static int nand_wait_cmd_fifo_empty(void)
> +{
> +	if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT,
> +				 DEFAULT_TIMEOUT_US)) {
> +		printf("nand: timeout waiting for empty cmd FIFO\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nand_wait_int(void)
> +{
> +	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> +			 DEFAULT_TIMEOUT_US)) {
> +		printf("nand: timeout waiting for interruption\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	udelay(1);
> +
> +	return 0;
> +}
> +

... here you add the helpers that you talked about in your commit log,
good. but ...

>  void nand_init(void)
>  {
>  	uint32_t val;
> @@ -172,22 +196,21 @@ void nand_init(void)
>  	}
>  
>  	/* reset NAND */
> +	nand_wait_cmd_fifo_empty();
> +

... here you call one of these helpers where you didn't have that code
before, and that's not mentionned or explained in your commit log.

>  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
>  	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error timeout waiting for nand reset\n");
> -		return;
> -	}
> -	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +	nand_wait_int();
>  }
>  
>  static void nand_apply_config(const struct nfc_config *conf)
>  {
>  	u32 val;
>  
> +	nand_wait_cmd_fifo_empty();
> +

Ditto.

>  	val = readl(SUNXI_NFC_BASE + NFC_CTL);
>  	val &= ~NFC_CTL_PAGE_SIZE_MASK;
>  	writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
> @@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
>  {
>  	int page = offs / conf->page_size;
>  
> +	nand_wait_cmd_fifo_empty();
> +

Ditto.

>  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
>  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
>  	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
> @@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
>  	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
>  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
>  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
> -	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
> +	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,

I guess it was a typo fix then?

>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -EIO;
> -	}
> -
> -	return 0;
> +	return nand_wait_int();
>  }
>  
>  static int nand_reset_column(void)
>  {
> +	nand_wait_cmd_fifo_empty();
> +

Not mentionned or explained in your commit log.

>  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
>  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
>  	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
>  	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);

Ditto

>  	writel(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
>  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> -	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
> +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -1;
> -	}
> +	return nand_wait_int();
>  
> -	return 0;
>  }
>  
> +static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> +
>  static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  			  void *dest, int len)
>  {
> @@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  
>  static int nand_max_ecc_strength(struct nfc_config *conf)
>  {
> -	static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116 };


Ditto.

Overall, I think it would be great to split that patch into 4:
  - the first one to fix the ADR vs ADDR typo (without changing the order)
  - the second one to introduce the nand_wait_int helper
  - the third one to introduce the nand_wait_cmd_fifo_empty, with a
    proper commit log explaining why this needs to be done and which
    issues is it fixing
  - And maybe if justified moving the ecc_bytes array to global scope

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/784879f1/attachment.sig>

[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

[Miquel Raynal]

Hi Maxime,

On Wed, 24 Jan 2018 08:56:38 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:50AM +0100, Miquel Raynal wrote:
> > Do some cleaning in sunxi NAND SPL driver like adding helpers and
> > clearing flags at the right spot
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>  
> 
> I'm not really fond of these kind of wildcard patches, especially
> since..
> 

[...]

> 
> Overall, I think it would be great to split that patch into 4:
>   - the first one to fix the ADR vs ADDR typo (without changing the order)
>   - the second one to introduce the nand_wait_int helper
>   - the third one to introduce the nand_wait_cmd_fifo_empty, with a
>     proper commit log explaining why this needs to be done and which
>     issues is it fixing
>   - And maybe if justified moving the ecc_bytes array to global scope

I split this patch into 4 logical peaces, as requested.

Thanks,
Miquèl

[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

[Boris Brezillon]

On Wed, 24 Jan 2018 01:44:50 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Do some cleaning in sunxi NAND SPL driver like adding helpers and
> clearing flags at the right spot
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/sunxi_nand_spl.c | 64 +++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> index 06695fc15f..2488d5cb51 100644
> --- a/drivers/mtd/nand/sunxi_nand_spl.c
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -55,8 +55,8 @@
>  
>  
>  #define NFC_ADDR_NUM_OFFSET        16
> -#define NFC_SEND_ADR               (1 << 19)
>  #define NFC_ACCESS_DIR             (1 << 20)
> +#define NFC_SEND_ADDR              (1 << 19)

This typo fix probably deserves a separate patch.

>  #define NFC_DATA_TRANS             (1 << 21)
>  #define NFC_SEND_CMD1              (1 << 22)
>  #define NFC_WAIT_FLAG              (1 << 23)
> @@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int unexpected_bits,
>  	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
>  }
>  
> +static int nand_wait_cmd_fifo_empty(void)
> +{
> +	if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT,
> +				 DEFAULT_TIMEOUT_US)) {
> +		printf("nand: timeout waiting for empty cmd FIFO\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nand_wait_int(void)
> +{
> +	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> +			 DEFAULT_TIMEOUT_US)) {
> +		printf("nand: timeout waiting for interruption\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	udelay(1);

I'm pretty sure this udelay() is not required. Probably some leftover
of your debug session ;-).

> +
> +	return 0;
> +}
> +
>  void nand_init(void)
>  {
>  	uint32_t val;
> @@ -172,22 +196,21 @@ void nand_init(void)
>  	}
>  
>  	/* reset NAND */
> +	nand_wait_cmd_fifo_empty();
> +
>  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
>  	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error timeout waiting for nand reset\n");
> -		return;
> -	}
> -	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +	nand_wait_int();

I would go even further and create an helper that takes care of the
'wait_fifo_empty+clear_int+write_cmd+wait_int' sequence:

static int nand_exec_cmd(u32 cmd)
{
	int ret;

	ret = nand_wait_cmd_fifo_empty();
	if (ret)
		return ret;

	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
	writel(cmd, SUNXI_NFC_BASE + NFC_CMD);

	return nand_wait_int();
}

>  }
>  
>  static void nand_apply_config(const struct nfc_config *conf)
>  {
>  	u32 val;
>  
> +	nand_wait_cmd_fifo_empty();
> +
>  	val = readl(SUNXI_NFC_BASE + NFC_CTL);
>  	val &= ~NFC_CTL_PAGE_SIZE_MASK;
>  	writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
> @@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
>  {
>  	int page = offs / conf->page_size;
>  
> +	nand_wait_cmd_fifo_empty();
> +
>  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
>  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
>  	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
> @@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
>  	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
>  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
>  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
> -	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
> +	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -EIO;
> -	}
> -
> -	return 0;
> +	return nand_wait_int();
>  }
>  
>  static int nand_reset_column(void)
>  {
> +	nand_wait_cmd_fifo_empty();
> +
>  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
>  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
>  	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
>  	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
>  	writel(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
>  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> -	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
> +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -1;
> -	}
> +	return nand_wait_int();

Here, I think you should wait, just to make sure we don't read data
before tCCS. udelay(1) should be enough.

>  
> -	return 0;
>  }
>  
> +static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> +

Putting ecc_bytes out of nand_max_ecc_strength() is not really related
to this patch, and should probably go in patch 5.

>  static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  			  void *dest, int len)
>  {
> @@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  
>  static int nand_max_ecc_strength(struct nfc_config *conf)
>  {
> -	static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116 };
>  	int max_oobsize, max_ecc_bytes;
>  	int nsectors = conf->page_size / conf->ecc_size;
>  	int i;

[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

[Miquel Raynal]

Hi Boris,

On Wed, 24 Jan 2018 09:06:38 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 24 Jan 2018 01:44:50 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > Do some cleaning in sunxi NAND SPL driver like adding helpers and
> > clearing flags at the right spot
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/sunxi_nand_spl.c | 64 +++++++++++++++++++++++++--------------
> >  1 file changed, 41 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 06695fc15f..2488d5cb51 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -55,8 +55,8 @@
> >  
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_SEND_ADR               (1 << 19)
> >  #define NFC_ACCESS_DIR             (1 << 20)
> > +#define NFC_SEND_ADDR              (1 << 19)  
> 
> This typo fix probably deserves a separate patch.

Indeed.

> 
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> > @@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int unexpected_bits,
> >  	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
> >  }
> >  
> > +static int nand_wait_cmd_fifo_empty(void)
> > +{
> > +	if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT,
> > +				 DEFAULT_TIMEOUT_US)) {
> > +		printf("nand: timeout waiting for empty cmd FIFO\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int nand_wait_int(void)
> > +{
> > +	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > +			 DEFAULT_TIMEOUT_US)) {
> > +		printf("nand: timeout waiting for interruption\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	udelay(1);  
> 
> I'm pretty sure this udelay() is not required. Probably some leftover
> of your debug session ;-).

That is right -> removed.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  void nand_init(void)
> >  {
> >  	uint32_t val;
> > @@ -172,22 +196,21 @@ void nand_init(void)
> >  	}
> >  
> >  	/* reset NAND */
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> >  	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error timeout waiting for nand reset\n");
> > -		return;
> > -	}
> > -	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +	nand_wait_int();  
> 
> I would go even further and create an helper that takes care of the
> 'wait_fifo_empty+clear_int+write_cmd+wait_int' sequence:
> 
> static int nand_exec_cmd(u32 cmd)
> {
> 	int ret;
> 
> 	ret = nand_wait_cmd_fifo_empty();
> 	if (ret)
> 		return ret;
> 
> 	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> 	writel(cmd, SUNXI_NFC_BASE + NFC_CMD);
> 
> 	return nand_wait_int();
> }

It makes sense, I added another patch to introduce this helper.

> 
> >  }
> >  
> >  static void nand_apply_config(const struct nfc_config *conf)
> >  {
> >  	u32 val;
> >  
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	val = readl(SUNXI_NFC_BASE + NFC_CTL);
> >  	val &= ~NFC_CTL_PAGE_SIZE_MASK;
> >  	writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
> > @@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
> >  {
> >  	int page = offs / conf->page_size;
> >  
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> >  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> >  	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
> > @@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
> >  	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
> >  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> >  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
> > -	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
> > +	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -EIO;
> > -	}
> > -
> > -	return 0;
> > +	return nand_wait_int();
> >  }
> >  
> >  static int nand_reset_column(void)
> >  {
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> >  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> >  	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> >  	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> > +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> >  	writel(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> >  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> > -	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
> > +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -1;
> > -	}
> > +	return nand_wait_int();  
> 
> Here, I think you should wait, just to make sure we don't read data
> before tCCS. udelay(1) should be enough.

Done and explained in another patch.

> 
> >  
> > -	return 0;
> >  }
> >  
> > +static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> > +  
> 
> Putting ecc_bytes out of nand_max_ecc_strength() is not really related
> to this patch, and should probably go in patch 5.

Right, I will move this to the patch where it belongs.

> 
> >  static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  			  void *dest, int len)
> >  {
> > @@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  
> >  static int nand_max_ecc_strength(struct nfc_config *conf)
> >  {
> > -	static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116 };
> >  	int max_oobsize, max_ecc_bytes;
> >  	int nsectors = conf->page_size / conf->ecc_size;
> >  	int i;  
> 

Thanks,
Miquèl

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Miquel Raynal]

Because using DMA implementation is not generic and was not developped
to work on SoCs like A33, migrate the SPL to use PIO.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 board/sunxi/board.c               |   4 +-
 drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
 2 files changed, 79 insertions(+), 92 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 70e01437c4..512e2c17a6 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -266,11 +266,13 @@ static void nand_clock_setup(void)
 	struct sunxi_ccm_reg *const ccm =
 		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
 
-	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
+	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
+	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));
 #ifdef CONFIG_MACH_SUN9I
 	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
 #else
 	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
+	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));
 #endif
 	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
 }
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 2488d5cb51..5de6825994 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -10,6 +10,7 @@
 #include <common.h>
 #include <config.h>
 #include <nand.h>
+#include <linux/ctype.h>
 
 /* registers */
 #define NFC_CTL                    0x00000000
@@ -45,32 +46,22 @@
 #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
 #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
 
-
 #define NFC_ECC_EN                 (1 << 0)
-#define NFC_ECC_PIPELINE           (1 << 3)
 #define NFC_ECC_EXCEPTION          (1 << 4)
 #define NFC_ECC_BLOCK_SIZE         (1 << 5)
 #define NFC_ECC_RANDOM_EN          (1 << 9)
-#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
-
 
 #define NFC_ADDR_NUM_OFFSET        16
-#define NFC_ACCESS_DIR             (1 << 20)
 #define NFC_SEND_ADDR              (1 << 19)
 #define NFC_DATA_TRANS             (1 << 21)
 #define NFC_SEND_CMD1              (1 << 22)
 #define NFC_WAIT_FLAG              (1 << 23)
 #define NFC_SEND_CMD2              (1 << 24)
-#define NFC_SEQ                    (1 << 25)
-#define NFC_DATA_SWAP_METHOD       (1 << 26)
-#define NFC_ROW_AUTO_INC           (1 << 27)
-#define NFC_SEND_CMD3              (1 << 28)
-#define NFC_SEND_CMD4              (1 << 29)
 #define NFC_RAW_CMD                (0 << 30)
-#define NFC_PAGE_CMD               (2 << 30)
+#define NFC_ECC_CMD                (1 << 30)
 
 #define NFC_ST_CMD_INT_FLAG        (1 << 1)
-#define NFC_ST_DMA_INT_FLAG        (1 << 2)
+#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
 
 #define NFC_READ_CMD_OFFSET         0
 #define NFC_RANDOM_READ_CMD0_OFFSET 8
@@ -80,22 +71,6 @@
 #define NFC_CMD_RNDOUT             0x05
 #define NFC_CMD_READSTART          0x30
 
-#define SUNXI_DMA_CFG_REG0              0x300
-#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
-#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
-#define SUNXI_DMA_DDMA_BC_REG0          0x30C
-#define SUNXI_DMA_DDMA_PARA_REG0        0x318
-
-#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
-#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
-#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
-#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
-#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
-#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
-
-#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
-#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
-
 struct nfc_config {
 	int page_size;
 	int ecc_strength;
@@ -254,7 +229,23 @@ static int nand_reset_column(void)
 	       SUNXI_NFC_BASE + NFC_CMD);
 
 	return nand_wait_int();
+}
 
+static int nand_change_column(u16 column)
+{
+	nand_wait_cmd_fifo_empty();
+
+	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
+	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
+	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
+	       SUNXI_NFC_BASE + NFC_RCMD_SET);
+	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
+	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
+	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
+	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
+	       SUNXI_NFC_BASE + NFC_CMD);
+
+	return nand_wait_int();
 }
 
 static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
@@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
 static int nand_read_page(const struct nfc_config *conf, u32 offs,
 			  void *dest, int len)
 {
-	dma_addr_t dst = (dma_addr_t)dest;
 	int nsectors = len / conf->ecc_size;
 	u16 rand_seed = 0;
-	u32 val;
-	int page;
-
-	page = offs / conf->page_size;
+	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
+	int page = offs / conf->page_size;
+	u32 ecc_st;
+	int i;
 
 	if (offs % conf->page_size || len % conf->ecc_size ||
 	    len > conf->page_size || len < 0)
 		return -EINVAL;
 
-	/* clear ecc status */
-	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
-
 	/* Choose correct seed if randomized */
 	if (conf->randomize)
 		rand_seed = random_seed[page % conf->nseeds];
 
-	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
-		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
-		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
-		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
-		SUNXI_NFC_BASE + NFC_ECC_CTL);
-
-	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
-
-	/* SUNXI_DMA */
-	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
-	/* read from REG_IO_DATA */
-	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
-	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
-	/* read to RAM */
-	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
-	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
-	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
-	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
-	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
-	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
-	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
-	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
-	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
-	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
-	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
-	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
-
-	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
-	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
-	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
-	       SUNXI_NFC_BASE + NFC_CMD);
+	/* Retrieve data from SRAM (PIO) */
+	for (i = 0; i < nsectors; i++) {
+		int data_off = i * conf->ecc_size;
+		int oob_off = conf->page_size + (i * oob_chunk_sz);
+		u8 *data = dest + data_off;
+
+		/* Clear ECC status and restart ECC engine */
+		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
+		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
+		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
+		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
+		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
+		       SUNXI_NFC_BASE + NFC_ECC_CTL);
+
+		/* Move the data in SRAM */
+		nand_change_column(data_off);
+		nand_wait_cmd_fifo_empty();
+		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
+		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
+		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
+		nand_wait_int();
 
-	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
-			 DEFAULT_TIMEOUT_US)) {
-		printf("Error while initializing dma interrupt\n");
-		return -EIO;
-	}
-	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
+		/*
+		 * Let the ECC engine consume the ECC bytes and possibly correct
+		 * the data.
+		 */
+		nand_change_column(oob_off);
+		nand_wait_cmd_fifo_empty();
+		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
+		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
+		nand_wait_int();
 
-	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
-				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
-				 DEFAULT_TIMEOUT_US)) {
-		printf("Error while waiting for dma transfer to finish\n");
-		return -EIO;
-	}
+		/* Get the ECC status */
+		ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
 
-	invalidate_dcache_range(dst,
-				ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
+		/* Retrieve the data from SRAM */
+		memcpy_fromio(data, SUNXI_NFC_BASE + NFC_RAM0_BASE,
+			      conf->ecc_size);
 
-	val = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
+		/* Stop ECC engine */
+		writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
+		       SUNXI_NFC_BASE + NFC_ECC_CTL);
 
-	/* ECC error detected. */
-	if (val & 0xffff)
-		return -EIO;
+		/* ECC error detected. */
+		if (ecc_st & 0xffff)
+			return -EIO;
 
-	/*
-	 * Return 1 if the page is empty.
-	 * We consider the page as empty if the first ECC block is marked
-	 * empty.
-	 */
-	return (val & 0x10000) ? 1 : 0;
+		/*
+		 * Return 1 if the page is empty. We consider the page as empty
+		 * if the first ECC block is marked empty.
+		 */
+		if (ecc_st & 0x10000)
+			return 1;
+
+		if (data_off + conf->ecc_size >= len)
+			break;
+	}
+
+	return 0;
 }
 
 static int nand_max_ecc_strength(struct nfc_config *conf)
-- 
2.11.0

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:51AM +0100, Miquel Raynal wrote:
> Because using DMA implementation is not generic and was not developped
> to work on SoCs like A33, migrate the SPL to use PIO.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

I guess the issue is slightly different than what you're saying, or at
least should be expressed differently.

The issue is that the SPL support has be done to support only the
earlier generations of Allwinner SoCs, and only really enabled on the
A13 / GR8. However, those old SoCs had a DMA engine that has been
replaced since the A31 by another DMA controller that is no longer
compatible.

Since the code directly uses that DMA controller, it cannot operate
properly on the later SoCs, while the NAND controller hasn't changed.

There's two pathes forward, the first one would be to add support for
that DMA controller too, or you can just remove the DMA usage entirely
and rely on PIO that will make the driver simpler.

Explaining why you chose PIO would be great too.

> ---
>  board/sunxi/board.c               |   4 +-
>  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
>  2 files changed, 79 insertions(+), 92 deletions(-)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 70e01437c4..512e2c17a6 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
>  	struct sunxi_ccm_reg *const ccm =
>  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>  
> -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));

That has nothing to do with PIO vs DMA. It should be in another patch.

>  #ifdef CONFIG_MACH_SUN9I
>  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
>  #else
>  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));

So you're de-asserting the reset line for the DMA controller, while
the commit is about removing the need for that DMA controller ? :)

That whole block can actually be removed (in a separate patch) now
that you're not using DMA anymore.

>  #endif
>  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
>  }
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> index 2488d5cb51..5de6825994 100644
> --- a/drivers/mtd/nand/sunxi_nand_spl.c
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <config.h>
>  #include <nand.h>
> +#include <linux/ctype.h>
>  
>  /* registers */
>  #define NFC_CTL                    0x00000000
> @@ -45,32 +46,22 @@
>  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
>  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
>  
> -
>  #define NFC_ECC_EN                 (1 << 0)
> -#define NFC_ECC_PIPELINE           (1 << 3)
>  #define NFC_ECC_EXCEPTION          (1 << 4)
>  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
>  #define NFC_ECC_RANDOM_EN          (1 << 9)
> -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> -
>  
>  #define NFC_ADDR_NUM_OFFSET        16
> -#define NFC_ACCESS_DIR             (1 << 20)
>  #define NFC_SEND_ADDR              (1 << 19)
>  #define NFC_DATA_TRANS             (1 << 21)
>  #define NFC_SEND_CMD1              (1 << 22)
>  #define NFC_WAIT_FLAG              (1 << 23)
>  #define NFC_SEND_CMD2              (1 << 24)
> -#define NFC_SEQ                    (1 << 25)
> -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> -#define NFC_ROW_AUTO_INC           (1 << 27)
> -#define NFC_SEND_CMD3              (1 << 28)
> -#define NFC_SEND_CMD4              (1 << 29)
>  #define NFC_RAW_CMD                (0 << 30)
> -#define NFC_PAGE_CMD               (2 << 30)
> +#define NFC_ECC_CMD                (1 << 30)
>  
>  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
>  
>  #define NFC_READ_CMD_OFFSET         0
>  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> @@ -80,22 +71,6 @@
>  #define NFC_CMD_RNDOUT             0x05
>  #define NFC_CMD_READSTART          0x30
>  
> -#define SUNXI_DMA_CFG_REG0              0x300
> -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> -
> -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> -
> -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> -

We should probably keep those values. They're not used anymore, but
the NAND controller has been under-documented on most of the datasheet
we've had. It's good to keep it at least for reference.

>  struct nfc_config {
>  	int page_size;
>  	int ecc_strength;
> @@ -254,7 +229,23 @@ static int nand_reset_column(void)
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
>  	return nand_wait_int();
> +}
>  
> +static int nand_change_column(u16 column)
> +{
> +	nand_wait_cmd_fifo_empty();
> +
> +	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> +	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> +	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> +	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> +	       SUNXI_NFC_BASE + NFC_CMD);
> +
> +	return nand_wait_int();
>  }
>  
>  static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> @@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
>  static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  			  void *dest, int len)
>  {
> -	dma_addr_t dst = (dma_addr_t)dest;
>  	int nsectors = len / conf->ecc_size;
>  	u16 rand_seed = 0;
> -	u32 val;
> -	int page;
> -
> -	page = offs / conf->page_size;
> +	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
> +	int page = offs / conf->page_size;
> +	u32 ecc_st;
> +	int i;
>  
>  	if (offs % conf->page_size || len % conf->ecc_size ||
>  	    len > conf->page_size || len < 0)
>  		return -EINVAL;
>  
> -	/* clear ecc status */
> -	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> -
>  	/* Choose correct seed if randomized */
>  	if (conf->randomize)
>  		rand_seed = random_seed[page % conf->nseeds];
>  
> -	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> -		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> -		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> -		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
> -		SUNXI_NFC_BASE + NFC_ECC_CTL);
> -
> -	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> -
> -	/* SUNXI_DMA */
> -	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
> -	/* read from REG_IO_DATA */
> -	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
> -	/* read to RAM */
> -	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
> -	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
> -	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
> -	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
> -	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
> -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
> -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
> -
> -	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
> -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> -	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
> -	       SUNXI_NFC_BASE + NFC_CMD);
> +	/* Retrieve data from SRAM (PIO) */
> +	for (i = 0; i < nsectors; i++) {
> +		int data_off = i * conf->ecc_size;
> +		int oob_off = conf->page_size + (i * oob_chunk_sz);
> +		u8 *data = dest + data_off;
> +
> +		/* Clear ECC status and restart ECC engine */
> +		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> +		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> +		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> +		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> +		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
> +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> +
> +		/* Move the data in SRAM */
> +		nand_change_column(data_off);
> +		nand_wait_cmd_fifo_empty();
> +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
> +		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
> +		nand_wait_int();
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -EIO;
> -	}
> -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);

It seems that you're also converting this code to some of your new
helpers. This should be done in the patch introducing them.

> +		/*
> +		 * Let the ECC engine consume the ECC bytes and possibly correct
> +		 * the data.
> +		 */
> +		nand_change_column(oob_off);
> +		nand_wait_cmd_fifo_empty();
> +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
> +		nand_wait_int();
>  
> -	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
> -				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
> -				 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while waiting for dma transfer to finish\n");
> -		return -EIO;
> -	}

Ditto.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/163a0380/attachment.sig>

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Boris Brezillon]

On Wed, 24 Jan 2018 09:06:26 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:51AM +0100, Miquel Raynal wrote:
> > Because using DMA implementation is not generic and was not developped
> > to work on SoCs like A33, migrate the SPL to use PIO.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>  
> 
> I guess the issue is slightly different than what you're saying, or at
> least should be expressed differently.
> 
> The issue is that the SPL support has be done to support only the
> earlier generations of Allwinner SoCs, and only really enabled on the
> A13 / GR8. However, those old SoCs had a DMA engine that has been
> replaced since the A31 by another DMA controller that is no longer
> compatible.
> 
> Since the code directly uses that DMA controller, it cannot operate
> properly on the later SoCs, while the NAND controller hasn't changed.
> 
> There's two pathes forward, the first one would be to add support for
> that DMA controller too, or you can just remove the DMA usage entirely
> and rely on PIO that will make the driver simpler.
> 
> Explaining why you chose PIO would be great too.
> 
> > ---
> >  board/sunxi/board.c               |   4 +-
> >  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
> >  2 files changed, 79 insertions(+), 92 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 70e01437c4..512e2c17a6 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
> >  	struct sunxi_ccm_reg *const ccm =
> >  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >  
> > -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));  
> 
> That has nothing to do with PIO vs DMA. It should be in another patch.
> 
> >  #ifdef CONFIG_MACH_SUN9I
> >  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
> >  #else
> >  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));  
> 
> So you're de-asserting the reset line for the DMA controller, while
> the commit is about removing the need for that DMA controller ? :)
> 
> That whole block can actually be removed (in a separate patch) now
> that you're not using DMA anymore.
> 
> >  #endif
> >  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
> >  }
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 2488d5cb51..5de6825994 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <config.h>
> >  #include <nand.h>
> > +#include <linux/ctype.h>
> >  
> >  /* registers */
> >  #define NFC_CTL                    0x00000000
> > @@ -45,32 +46,22 @@
> >  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
> >  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
> >  
> > -
> >  #define NFC_ECC_EN                 (1 << 0)
> > -#define NFC_ECC_PIPELINE           (1 << 3)
> >  #define NFC_ECC_EXCEPTION          (1 << 4)
> >  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
> >  #define NFC_ECC_RANDOM_EN          (1 << 9)
> > -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> > -
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_ACCESS_DIR             (1 << 20)
> >  #define NFC_SEND_ADDR              (1 << 19)
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> >  #define NFC_SEND_CMD2              (1 << 24)
> > -#define NFC_SEQ                    (1 << 25)
> > -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> > -#define NFC_ROW_AUTO_INC           (1 << 27)
> > -#define NFC_SEND_CMD3              (1 << 28)
> > -#define NFC_SEND_CMD4              (1 << 29)
> >  #define NFC_RAW_CMD                (0 << 30)
> > -#define NFC_PAGE_CMD               (2 << 30)
> > +#define NFC_ECC_CMD                (1 << 30)
> >  
> >  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> > -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> > +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)

Oh, I didn't notice you were removing some definitions. I
agree what Maxime says below, it's definitely not a good move.

> >  
> >  #define NFC_READ_CMD_OFFSET         0
> >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > @@ -80,22 +71,6 @@
> >  #define NFC_CMD_RNDOUT             0x05
> >  #define NFC_CMD_READSTART          0x30
> >  
> > -#define SUNXI_DMA_CFG_REG0              0x300
> > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > -
> > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > -
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > -  
> 
> We should probably keep those values. They're not used anymore, but
> the NAND controller has been under-documented on most of the datasheet
> we've had. It's good to keep it at least for reference.
> 

Hm, your statement is valid for the NFC_ defs, but these are purely DMA
engine related definitions, so I think we can get rid of them.

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Maxime Ripard]

On Wed, Jan 24, 2018 at 09:26:37AM +0100, Boris Brezillon wrote:
> > >  
> > >  #define NFC_READ_CMD_OFFSET         0
> > >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > > @@ -80,22 +71,6 @@
> > >  #define NFC_CMD_RNDOUT             0x05
> > >  #define NFC_CMD_READSTART          0x30
> > >  
> > > -#define SUNXI_DMA_CFG_REG0              0x300
> > > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > > -
> > > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > > -
> > > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > > -  
> > 
> > We should probably keep those values. They're not used anymore, but
> > the NAND controller has been under-documented on most of the datasheet
> > we've had. It's good to keep it at least for reference.
> > 
> 
> Hm, your statement is valid for the NFC_ defs, but these are purely DMA
> engine related definitions, so I think we can get rid of them.

Oh, right, we can totally remove those.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/cc809528/attachment.sig>

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Miquel Raynal]

Hi Maxime,

I have some doubts about your comments, see below.

On Wed, 24 Jan 2018 09:06:26 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:51AM +0100, Miquel Raynal wrote:
> > Because using DMA implementation is not generic and was not developped
> > to work on SoCs like A33, migrate the SPL to use PIO.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>  
> 
> I guess the issue is slightly different than what you're saying, or at
> least should be expressed differently.
> 
> The issue is that the SPL support has be done to support only the
> earlier generations of Allwinner SoCs, and only really enabled on the
> A13 / GR8. However, those old SoCs had a DMA engine that has been
> replaced since the A31 by another DMA controller that is no longer
> compatible.
> 
> Since the code directly uses that DMA controller, it cannot operate
> properly on the later SoCs, while the NAND controller hasn't changed.
> 
> There's two pathes forward, the first one would be to add support for
> that DMA controller too, or you can just remove the DMA usage entirely
> and rely on PIO that will make the driver simpler.
> 
> Explaining why you chose PIO would be great too.
> 
> > ---
> >  board/sunxi/board.c               |   4 +-
> >  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
> >  2 files changed, 79 insertions(+), 92 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 70e01437c4..512e2c17a6 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
> >  	struct sunxi_ccm_reg *const ccm =
> >  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >  
> > -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));  
> 
> That has nothing to do with PIO vs DMA. It should be in another patch.

Ok.

> 
> >  #ifdef CONFIG_MACH_SUN9I
> >  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
> >  #else
> >  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));  
> 
> So you're de-asserting the reset line for the DMA controller, while
> the commit is about removing the need for that DMA controller ? :)
> 
> That whole block can actually be removed (in a separate patch) now
> that you're not using DMA anymore.

Shall I remove only what is after the else statement? Or does MACH_SUN9I
use the same code so I can remove the whole #ifdef/#else/#endif
statement?

> 
> >  #endif
> >  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
> >  }
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 2488d5cb51..5de6825994 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <config.h>
> >  #include <nand.h>
> > +#include <linux/ctype.h>
> >  
> >  /* registers */
> >  #define NFC_CTL                    0x00000000
> > @@ -45,32 +46,22 @@
> >  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
> >  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
> >  
> > -
> >  #define NFC_ECC_EN                 (1 << 0)
> > -#define NFC_ECC_PIPELINE           (1 << 3)
> >  #define NFC_ECC_EXCEPTION          (1 << 4)
> >  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
> >  #define NFC_ECC_RANDOM_EN          (1 << 9)
> > -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> > -
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_ACCESS_DIR             (1 << 20)
> >  #define NFC_SEND_ADDR              (1 << 19)
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> >  #define NFC_SEND_CMD2              (1 << 24)
> > -#define NFC_SEQ                    (1 << 25)
> > -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> > -#define NFC_ROW_AUTO_INC           (1 << 27)
> > -#define NFC_SEND_CMD3              (1 << 28)
> > -#define NFC_SEND_CMD4              (1 << 29)
> >  #define NFC_RAW_CMD                (0 << 30)
> > -#define NFC_PAGE_CMD               (2 << 30)
> > +#define NFC_ECC_CMD                (1 << 30)
> >  
> >  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> > -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> > +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
> >  
> >  #define NFC_READ_CMD_OFFSET         0
> >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > @@ -80,22 +71,6 @@
> >  #define NFC_CMD_RNDOUT             0x05
> >  #define NFC_CMD_READSTART          0x30
> >  
> > -#define SUNXI_DMA_CFG_REG0              0x300
> > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > -
> > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > -
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > -  
> 
> We should probably keep those values. They're not used anymore, but
> the NAND controller has been under-documented on most of the datasheet
> we've had. It's good to keep it at least for reference.

I will remove DMA related defines, and keep the other ones.

> 
> >  struct nfc_config {
> >  	int page_size;
> >  	int ecc_strength;
> > @@ -254,7 +229,23 @@ static int nand_reset_column(void)
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> >  	return nand_wait_int();
> > +}
> >  
> > +static int nand_change_column(u16 column)
> > +{
> > +	nand_wait_cmd_fifo_empty();
> > +
> > +	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> > +	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> > +	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> > +	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> > +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> > +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> > +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> > +	       SUNXI_NFC_BASE + NFC_CMD);
> > +
> > +	return nand_wait_int();
> >  }
> >  
> >  static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> > @@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> >  static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  			  void *dest, int len)
> >  {
> > -	dma_addr_t dst = (dma_addr_t)dest;
> >  	int nsectors = len / conf->ecc_size;
> >  	u16 rand_seed = 0;
> > -	u32 val;
> > -	int page;
> > -
> > -	page = offs / conf->page_size;
> > +	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
> > +	int page = offs / conf->page_size;
> > +	u32 ecc_st;
> > +	int i;
> >  
> >  	if (offs % conf->page_size || len % conf->ecc_size ||
> >  	    len > conf->page_size || len < 0)
> >  		return -EINVAL;
> >  
> > -	/* clear ecc status */
> > -	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > -
> >  	/* Choose correct seed if randomized */
> >  	if (conf->randomize)
> >  		rand_seed = random_seed[page % conf->nseeds];
> >  
> > -	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > -		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > -		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > -		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
> > -		SUNXI_NFC_BASE + NFC_ECC_CTL);
> > -
> > -	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> > -
> > -	/* SUNXI_DMA */
> > -	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
> > -	/* read from REG_IO_DATA */
> > -	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
> > -	/* read to RAM */
> > -	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
> > -	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
> > -	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
> > -	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
> > -	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
> > -
> > -	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > -	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
> > -	       SUNXI_NFC_BASE + NFC_CMD);
> > +	/* Retrieve data from SRAM (PIO) */
> > +	for (i = 0; i < nsectors; i++) {
> > +		int data_off = i * conf->ecc_size;
> > +		int oob_off = conf->page_size + (i * oob_chunk_sz);
> > +		u8 *data = dest + data_off;
> > +
> > +		/* Clear ECC status and restart ECC engine */
> > +		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > +		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > +		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > +		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > +		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
> > +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> > +
> > +		/* Move the data in SRAM */
> > +		nand_change_column(data_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
> > +		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -EIO;
> > -	}
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);  
> 
> It seems that you're also converting this code to some of your new
> helpers. This should be done in the patch introducing them.

Actually no, it is close but this is DMA specific, and does not fit
with the previously created helpers. The first reason why this block is
removed is because it is related to DMA.

> 
> > +		/*
> > +		 * Let the ECC engine consume the ECC bytes and possibly correct
> > +		 * the data.
> > +		 */
> > +		nand_change_column(oob_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
> > -				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
> > -				 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while waiting for dma transfer to finish\n");
> > -		return -EIO;
> > -	}  
> 
> Ditto.

Same as above.

Thanks for reviewing,
Miquèl

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Boris Brezillon]

On Wed, 24 Jan 2018 01:44:51 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Because using DMA implementation is not generic and was not developped
> to work on SoCs like A33, migrate the SPL to use PIO.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  board/sunxi/board.c               |   4 +-
>  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
>  2 files changed, 79 insertions(+), 92 deletions(-)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 70e01437c4..512e2c17a6 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
>  	struct sunxi_ccm_reg *const ccm =
>  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
>  
> -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));
>  #ifdef CONFIG_MACH_SUN9I
>  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
>  #else
>  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));
>  #endif
>  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
>  }
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> index 2488d5cb51..5de6825994 100644
> --- a/drivers/mtd/nand/sunxi_nand_spl.c
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <config.h>
>  #include <nand.h>
> +#include <linux/ctype.h>
>  
>  /* registers */
>  #define NFC_CTL                    0x00000000
> @@ -45,32 +46,22 @@
>  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
>  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
>  
> -
>  #define NFC_ECC_EN                 (1 << 0)
> -#define NFC_ECC_PIPELINE           (1 << 3)
>  #define NFC_ECC_EXCEPTION          (1 << 4)
>  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
>  #define NFC_ECC_RANDOM_EN          (1 << 9)
> -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> -
>  
>  #define NFC_ADDR_NUM_OFFSET        16
> -#define NFC_ACCESS_DIR             (1 << 20)
>  #define NFC_SEND_ADDR              (1 << 19)
>  #define NFC_DATA_TRANS             (1 << 21)
>  #define NFC_SEND_CMD1              (1 << 22)
>  #define NFC_WAIT_FLAG              (1 << 23)
>  #define NFC_SEND_CMD2              (1 << 24)
> -#define NFC_SEQ                    (1 << 25)
> -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> -#define NFC_ROW_AUTO_INC           (1 << 27)
> -#define NFC_SEND_CMD3              (1 << 28)
> -#define NFC_SEND_CMD4              (1 << 29)
>  #define NFC_RAW_CMD                (0 << 30)
> -#define NFC_PAGE_CMD               (2 << 30)
> +#define NFC_ECC_CMD                (1 << 30)
>  
>  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
>  
>  #define NFC_READ_CMD_OFFSET         0
>  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> @@ -80,22 +71,6 @@
>  #define NFC_CMD_RNDOUT             0x05
>  #define NFC_CMD_READSTART          0x30
>  
> -#define SUNXI_DMA_CFG_REG0              0x300
> -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> -
> -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> -
> -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> -
>  struct nfc_config {
>  	int page_size;
>  	int ecc_strength;
> @@ -254,7 +229,23 @@ static int nand_reset_column(void)
>  	       SUNXI_NFC_BASE + NFC_CMD);
>  
>  	return nand_wait_int();
> +}
>  
> +static int nand_change_column(u16 column)
> +{
> +	nand_wait_cmd_fifo_empty();
> +
> +	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> +	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> +	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> +	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> +	       SUNXI_NFC_BASE + NFC_CMD);
> +
> +	return nand_wait_int();
>  }

nand_reset_column() and nand_change_column() are almost the same,
except one is taking an argument to specify where you want to place the
pointer. I'd recommend getting rid of nand_reset_column() entirely and
patching all call sites to call nand_change_column(0) instead.

>  
>  static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> @@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
>  static int nand_read_page(const struct nfc_config *conf, u32 offs,
>  			  void *dest, int len)
>  {
> -	dma_addr_t dst = (dma_addr_t)dest;
>  	int nsectors = len / conf->ecc_size;
>  	u16 rand_seed = 0;
> -	u32 val;
> -	int page;
> -
> -	page = offs / conf->page_size;
> +	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
> +	int page = offs / conf->page_size;
> +	u32 ecc_st;
> +	int i;
>  
>  	if (offs % conf->page_size || len % conf->ecc_size ||
>  	    len > conf->page_size || len < 0)
>  		return -EINVAL;
>  
> -	/* clear ecc status */
> -	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> -
>  	/* Choose correct seed if randomized */
>  	if (conf->randomize)
>  		rand_seed = random_seed[page % conf->nseeds];
>  
> -	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> -		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> -		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> -		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
> -		SUNXI_NFC_BASE + NFC_ECC_CTL);
> -
> -	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> -
> -	/* SUNXI_DMA */
> -	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
> -	/* read from REG_IO_DATA */
> -	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
> -	/* read to RAM */
> -	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
> -	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
> -	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
> -	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
> -	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
> -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
> -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
> -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> -	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
> -
> -	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
> -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> -	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
> -	       SUNXI_NFC_BASE + NFC_CMD);
> +	/* Retrieve data from SRAM (PIO) */
> +	for (i = 0; i < nsectors; i++) {
> +		int data_off = i * conf->ecc_size;
> +		int oob_off = conf->page_size + (i * oob_chunk_sz);
> +		u8 *data = dest + data_off;
> +
> +		/* Clear ECC status and restart ECC engine */
> +		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> +		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> +		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> +		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> +		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
> +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> +
> +		/* Move the data in SRAM */
> +		nand_change_column(data_off);
> +		nand_wait_cmd_fifo_empty();
> +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
> +		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
> +		nand_wait_int();
>  
> -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
> -			 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while initializing dma interrupt\n");
> -		return -EIO;
> -	}
> -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +		/*
> +		 * Let the ECC engine consume the ECC bytes and possibly correct
> +		 * the data.
> +		 */
> +		nand_change_column(oob_off);
> +		nand_wait_cmd_fifo_empty();
> +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> +		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
> +		nand_wait_int();
>  
> -	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
> -				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
> -				 DEFAULT_TIMEOUT_US)) {
> -		printf("Error while waiting for dma transfer to finish\n");
> -		return -EIO;
> -	}
> +		/* Get the ECC status */
> +		ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
>  
> -	invalidate_dcache_range(dst,
> -				ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> +		/* Retrieve the data from SRAM */
> +		memcpy_fromio(data, SUNXI_NFC_BASE + NFC_RAM0_BASE,
> +			      conf->ecc_size);

Not a big deal, but should we really copy the data if the ECC engine
reports an error? Maybe you can move this memcpy_fromio() after checking
ecc_st.

>  
> -	val = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
> +		/* Stop ECC engine */
> +		writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
> +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
>  
> -	/* ECC error detected. */
> -	if (val & 0xffff)
> -		return -EIO;
> +		/* ECC error detected. */
> +		if (ecc_st & 0xffff)
> +			return -EIO;
>  
> -	/*
> -	 * Return 1 if the page is empty.
> -	 * We consider the page as empty if the first ECC block is marked
> -	 * empty.
> -	 */
> -	return (val & 0x10000) ? 1 : 0;
> +		/*
> +		 * Return 1 if the page is empty. We consider the page as empty
> +		 * if the first ECC block is marked empty.
> +		 */
> +		if (ecc_st & 0x10000)
> +			return 1;

This is no longer valid, because now you iterate over all ECC chunks
manually. The test should be:

		if (!i && (ecc_st & 0x10000))

> +
> +		if (data_off + conf->ecc_size >= len)
> +			break;
> +	}
> +
> +	return 0;
>  }
>  
>  static int nand_max_ecc_strength(struct nfc_config *conf)

[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Miquel Raynal]

Hi Boris,

On Wed, 24 Jan 2018 09:16:19 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 24 Jan 2018 01:44:51 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > Because using DMA implementation is not generic and was not developped
> > to work on SoCs like A33, migrate the SPL to use PIO.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  board/sunxi/board.c               |   4 +-
> >  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
> >  2 files changed, 79 insertions(+), 92 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 70e01437c4..512e2c17a6 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
> >  	struct sunxi_ccm_reg *const ccm =
> >  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >  
> > -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));
> >  #ifdef CONFIG_MACH_SUN9I
> >  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
> >  #else
> >  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));
> >  #endif
> >  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
> >  }
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 2488d5cb51..5de6825994 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <config.h>
> >  #include <nand.h>
> > +#include <linux/ctype.h>
> >  
> >  /* registers */
> >  #define NFC_CTL                    0x00000000
> > @@ -45,32 +46,22 @@
> >  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
> >  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
> >  
> > -
> >  #define NFC_ECC_EN                 (1 << 0)
> > -#define NFC_ECC_PIPELINE           (1 << 3)
> >  #define NFC_ECC_EXCEPTION          (1 << 4)
> >  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
> >  #define NFC_ECC_RANDOM_EN          (1 << 9)
> > -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> > -
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_ACCESS_DIR             (1 << 20)
> >  #define NFC_SEND_ADDR              (1 << 19)
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> >  #define NFC_SEND_CMD2              (1 << 24)
> > -#define NFC_SEQ                    (1 << 25)
> > -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> > -#define NFC_ROW_AUTO_INC           (1 << 27)
> > -#define NFC_SEND_CMD3              (1 << 28)
> > -#define NFC_SEND_CMD4              (1 << 29)
> >  #define NFC_RAW_CMD                (0 << 30)
> > -#define NFC_PAGE_CMD               (2 << 30)
> > +#define NFC_ECC_CMD                (1 << 30)
> >  
> >  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> > -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> > +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
> >  
> >  #define NFC_READ_CMD_OFFSET         0
> >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > @@ -80,22 +71,6 @@
> >  #define NFC_CMD_RNDOUT             0x05
> >  #define NFC_CMD_READSTART          0x30
> >  
> > -#define SUNXI_DMA_CFG_REG0              0x300
> > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > -
> > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > -
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > -
> >  struct nfc_config {
> >  	int page_size;
> >  	int ecc_strength;
> > @@ -254,7 +229,23 @@ static int nand_reset_column(void)
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> >  	return nand_wait_int();
> > +}
> >  
> > +static int nand_change_column(u16 column)
> > +{
> > +	nand_wait_cmd_fifo_empty();
> > +
> > +	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> > +	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> > +	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> > +	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> > +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> > +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> > +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> > +	       SUNXI_NFC_BASE + NFC_CMD);
> > +
> > +	return nand_wait_int();
> >  }  
> 
> nand_reset_column() and nand_change_column() are almost the same,
> except one is taking an argument to specify where you want to place the
> pointer. I'd recommend getting rid of nand_reset_column() entirely and
> patching all call sites to call nand_change_column(0) instead.

Done in a separate commit.


> 
> >  
> >  static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> > @@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> >  static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  			  void *dest, int len)
> >  {
> > -	dma_addr_t dst = (dma_addr_t)dest;
> >  	int nsectors = len / conf->ecc_size;
> >  	u16 rand_seed = 0;
> > -	u32 val;
> > -	int page;
> > -
> > -	page = offs / conf->page_size;
> > +	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
> > +	int page = offs / conf->page_size;
> > +	u32 ecc_st;
> > +	int i;
> >  
> >  	if (offs % conf->page_size || len % conf->ecc_size ||
> >  	    len > conf->page_size || len < 0)
> >  		return -EINVAL;
> >  
> > -	/* clear ecc status */
> > -	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > -
> >  	/* Choose correct seed if randomized */
> >  	if (conf->randomize)
> >  		rand_seed = random_seed[page % conf->nseeds];
> >  
> > -	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > -		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > -		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > -		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
> > -		SUNXI_NFC_BASE + NFC_ECC_CTL);
> > -
> > -	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> > -
> > -	/* SUNXI_DMA */
> > -	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
> > -	/* read from REG_IO_DATA */
> > -	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
> > -	/* read to RAM */
> > -	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
> > -	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
> > -	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
> > -	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
> > -	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
> > -
> > -	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > -	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
> > -	       SUNXI_NFC_BASE + NFC_CMD);
> > +	/* Retrieve data from SRAM (PIO) */
> > +	for (i = 0; i < nsectors; i++) {
> > +		int data_off = i * conf->ecc_size;
> > +		int oob_off = conf->page_size + (i * oob_chunk_sz);
> > +		u8 *data = dest + data_off;
> > +
> > +		/* Clear ECC status and restart ECC engine */
> > +		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > +		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > +		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > +		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > +		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
> > +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> > +
> > +		/* Move the data in SRAM */
> > +		nand_change_column(data_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
> > +		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -EIO;
> > -	}
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		/*
> > +		 * Let the ECC engine consume the ECC bytes and possibly correct
> > +		 * the data.
> > +		 */
> > +		nand_change_column(oob_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
> > -				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
> > -				 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while waiting for dma transfer to finish\n");
> > -		return -EIO;
> > -	}
> > +		/* Get the ECC status */
> > +		ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
> >  
> > -	invalidate_dcache_range(dst,
> > -				ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> > +		/* Retrieve the data from SRAM */
> > +		memcpy_fromio(data, SUNXI_NFC_BASE + NFC_RAM0_BASE,
> > +			      conf->ecc_size);  
> 
> Not a big deal, but should we really copy the data if the ECC engine
> reports an error? Maybe you can move this memcpy_fromio() after checking
> ecc_st.

Ok.

> 
> >  
> > -	val = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
> > +		/* Stop ECC engine */
> > +		writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
> > +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> >  
> > -	/* ECC error detected. */
> > -	if (val & 0xffff)
> > -		return -EIO;
> > +		/* ECC error detected. */
> > +		if (ecc_st & 0xffff)
> > +			return -EIO;
> >  
> > -	/*
> > -	 * Return 1 if the page is empty.
> > -	 * We consider the page as empty if the first ECC block is marked
> > -	 * empty.
> > -	 */
> > -	return (val & 0x10000) ? 1 : 0;
> > +		/*
> > +		 * Return 1 if the page is empty. We consider the page as empty
> > +		 * if the first ECC block is marked empty.
> > +		 */
> > +		if (ecc_st & 0x10000)
> > +			return 1;  
> 
> This is no longer valid, because now you iterate over all ECC chunks
> manually. The test should be:
> 
> 		if (!i && (ecc_st & 0x10000))
> 

That is right and could have caused troubles, thanks for spotting it!

> > +
> > +		if (data_off + conf->ecc_size >= len)
> > +			break;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int nand_max_ecc_strength(struct nfc_config *conf)  
> 

Thanks for reviewing,
Miquèl

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

[U-Boot] [PATCH 6/8] configs: Add NAND support for NES Classic

[Miquel Raynal]

Add NAND parameters to the Nintendo NES Classic configuration file.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 configs/Nintendo_NES_Classic_Edition_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configs/Nintendo_NES_Classic_Edition_defconfig b/configs/Nintendo_NES_Classic_Edition_defconfig
index d05375d0db..467055279c 100644
--- a/configs/Nintendo_NES_Classic_Edition_defconfig
+++ b/configs/Nintendo_NES_Classic_Edition_defconfig
@@ -9,6 +9,10 @@ CONFIG_AXP_GPIO=y
 CONFIG_DEFAULT_DEVICE_TREE="sun8i-r16-nintendo-nes-classic-edition"
 # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
 CONFIG_SPL=y
+CONFIG_SPL_NAND_SUPPORT=y
+CONFIG_SYS_EXTRA_OPTIONS="SYS_NAND_BLOCK_SIZE=0x20000,SYS_NAND_PAGE_SIZE=2048,SYS_NAND_OOBSIZE=64"
+CONFIG_NAND=y
+CONFIG_NAND_SUNXI=y
 CONFIG_FASTBOOT_FLASH=y
 # CONFIG_CMD_IMLS is not set
 # CONFIG_CMD_FLASH is not set
-- 
2.11.0

[U-Boot] [PATCH 6/8] configs: Add NAND support for NES Classic

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:52AM +0100, Miquel Raynal wrote:
> Add NAND parameters to the Nintendo NES Classic configuration file.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  configs/Nintendo_NES_Classic_Edition_defconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configs/Nintendo_NES_Classic_Edition_defconfig b/configs/Nintendo_NES_Classic_Edition_defconfig
> index d05375d0db..467055279c 100644
> --- a/configs/Nintendo_NES_Classic_Edition_defconfig
> +++ b/configs/Nintendo_NES_Classic_Edition_defconfig
> @@ -9,6 +9,10 @@ CONFIG_AXP_GPIO=y
>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-r16-nintendo-nes-classic-edition"
>  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>  CONFIG_SPL=y
> +CONFIG_SPL_NAND_SUPPORT=y

This could be selected (or implied) by NAND_SUNXI

> +CONFIG_SYS_EXTRA_OPTIONS="SYS_NAND_BLOCK_SIZE=0x20000,SYS_NAND_PAGE_SIZE=2048,SYS_NAND_OOBSIZE=64"

Can you move this to Kconfig?

> +CONFIG_NAND=y
> +CONFIG_NAND_SUNXI=y

And we can add a default ARCH_SUNXI on NAND_SUNXI here, which means
that we'd only have the NAND parameters and the CONFIG_NAND in
defconfig, reducing the boilerplate.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/e0641c50/attachment.sig>

[U-Boot] [PATCH 7/8] sunxi: dts: Add NAND node to sun8i DTSI

[Miquel Raynal]

Add the NAND controller node, as well as the definition of the missing
pins and clock.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 arch/arm/dts/sun8i-a23-a33.dtsi | 31 +++++++++++++++++++++++++++++++
 arch/arm/dts/sun8i-a33.dtsi     |  8 ++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/arm/dts/sun8i-a23-a33.dtsi b/arch/arm/dts/sun8i-a23-a33.dtsi
index f97c38f097..fe6ea82cb3 100644
--- a/arch/arm/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/dts/sun8i-a23-a33.dtsi
@@ -325,6 +325,19 @@
 			#size-cells = <0>;
 		};
 
+		nfc: nand at 01c03000 {
+			compatible = "allwinner,sun4i-a10-nand";
+			reg = <0x01c03000 0x1000>;
+			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ahb1_gates 25>, <&nand_clk>;
+			clock-names = "ahb", "mod";
+			resets = <&ahb1_rst 10>;
+			reset-names = "ahb";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		ehci0: usb at 01c1a000 {
 			compatible = "allwinner,sun8i-a23-ehci", "generic-ehci";
 			reg = <0x01c1a000 0x100>;
@@ -364,6 +377,24 @@
 				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 			};
 
+			nand_pins_a: nand-base0 at 0 {
+				allwinner,pins = "PC0", "PC1", "PC2",
+						"PC5", "PC8", "PC9", "PC10",
+						"PC11", "PC12", "PC13", "PC14",
+						"PC15";
+				allwinner,function = "nand0";
+			};
+
+			nand_cs0_pins_a: nand-cs at 0 {
+				allwinner,pins = "PC4";
+				allwinner,function = "nand0";
+			};
+
+			nand_rb0_pins_a: nand-rb at 0 {
+				allwinner,pins = "PC6";
+				allwinner,function = "nand0";
+			};
+
 			mmc0_pins_a: mmc0 at 0 {
 				allwinner,pins = "PF0", "PF1", "PF2",
 						 "PF3", "PF4", "PF5";
diff --git a/arch/arm/dts/sun8i-a33.dtsi b/arch/arm/dts/sun8i-a33.dtsi
index 001d8402ca..2ef817a679 100644
--- a/arch/arm/dts/sun8i-a33.dtsi
+++ b/arch/arm/dts/sun8i-a33.dtsi
@@ -99,6 +99,14 @@
 					"ahb1_sat";
 		};
 
+		nand_clk: clk at 01c20080 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun4i-a10-mod0-clk";
+			reg = <0x01c20080 0x4>;
+			clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
+			clock-output-names = "nand";
+		};
+
 		ss_clk: clk at 01c2009c {
 			#clock-cells = <0>;
 			compatible = "allwinner,sun4i-a10-mod0-clk";
-- 
2.11.0

[U-Boot] [PATCH 7/8] sunxi: dts: Add NAND node to sun8i DTSI

[Maxime Ripard]

Hi,

On Wed, Jan 24, 2018 at 01:44:53AM +0100, Miquel Raynal wrote:
> Add the NAND controller node, as well as the definition of the missing
> pins and clock.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  arch/arm/dts/sun8i-a23-a33.dtsi | 31 +++++++++++++++++++++++++++++++
>  arch/arm/dts/sun8i-a33.dtsi     |  8 ++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/arch/arm/dts/sun8i-a23-a33.dtsi b/arch/arm/dts/sun8i-a23-a33.dtsi
> index f97c38f097..fe6ea82cb3 100644
> --- a/arch/arm/dts/sun8i-a23-a33.dtsi
> +++ b/arch/arm/dts/sun8i-a23-a33.dtsi
> @@ -325,6 +325,19 @@
>  			#size-cells = <0>;
>  		};
>  
> +		nfc: nand at 01c03000 {
> +			compatible = "allwinner,sun4i-a10-nand";
> +			reg = <0x01c03000 0x1000>;
> +			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ahb1_gates 25>, <&nand_clk>;
> +			clock-names = "ahb", "mod";
> +			resets = <&ahb1_rst 10>;
> +			reset-names = "ahb";
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +

That node is already there so that patch shouldn't be needed.

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180124/e4319633/attachment.sig>

[U-Boot] [PATCH 8/8] sunxi: dts: Enable NAND on NES classic

[Miquel Raynal]

Let the Nintendo NES Classic use the Macronix NAND chip on it.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts b/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
index dce688ec8e..b8535d3dac 100644
--- a/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
+++ b/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
@@ -61,3 +61,17 @@
 	pinctrl-0 = <&uart0_pins_a>;
 	status = "okay";
 };
+
+&nfc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&nand_pins_a &nand_cs0_pins_a &nand_rb0_pins_a>;
+	status = "okay";
+
+	nand at 0 {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		reg = <0>;
+		allwinner,rb = <0>;
+		nand-ecc-mode = "hw";
+	};
+};
-- 
2.11.0

[U-Boot] [PATCH 8/8] sunxi: dts: Enable NAND on NES classic

[Boris Brezillon]

On Wed, 24 Jan 2018 01:44:54 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Let the Nintendo NES Classic use the Macronix NAND chip on it.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts b/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
> index dce688ec8e..b8535d3dac 100644
> --- a/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
> +++ b/arch/arm/dts/sun8i-r16-nintendo-nes-classic-edition.dts
> @@ -61,3 +61,17 @@
>  	pinctrl-0 = <&uart0_pins_a>;
>  	status = "okay";
>  };
> +
> +&nfc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&nand_pins_a &nand_cs0_pins_a &nand_rb0_pins_a>;
> +	status = "okay";
> +
> +	nand at 0 {
> +		#address-cells = <2>;
> +		#size-cells = <2>;

I'm pretty sure size and address cells can be set to 1 (the NAND is
less 4GB).

> +		reg = <0>;
> +		allwinner,rb = <0>;
> +		nand-ecc-mode = "hw";
> +	};
> +};