[U-Boot] [PATCH v2 0/7] spl: nand: sunxi: implement auto-detection

[U-Boot] [PATCH v2 0/7] spl: nand: sunxi: implement auto-detection

[Boris Brezillon]

Hello,

This patch series aims at adding support for NAND auto-detection to
the sunxi SPL NAND driver.

As explained in patch 7, this auto-detection is nothing more than a
dumb "trial and error" logic, but it allows one to use the same
SPL binary for all kind of sunxi boards booting from NAND.
Of course, this approach might increase a bit the boot-time, but this
is something we could address by adding a "default NAND config",
that would be tested before launching the auto-detection procedure.

Now let's detail a bit what's inside this patch-set.
Patch 1 is a cleanup removing support for BootROM configs, which in
my opinion are not only inefficient but also not reliable (at least
the current implementation does not guarantee that you'll be using
the correct configuration when reading the NAND).

Piotr, Hans, any comment?
Is this a real problem if we get rid of syndrome/BootROM configs?
I mean, are you really using this mode? If that's not the case, I'd
prefer dropping support for this feature. ITOH, if you really
need this mode, then I'd recommend adding Kconfig options to specify
the exact config to be used rather than randomly testing configs
(see my explanation in patch 1).

Patch 2 is renaming the SYS_NAND_U_BOOT_OFFS Kconfig option to make it
usable on all platforms (not only sunxi) and avoid conflicts when
one board is defining CONFIG_SYS_NAND_U_BOOT_OFFS in its
include/configs/<board>.h header.

Patch 3 is adding generic support for redundant u-boot images, which
is particularly useful on modern NANDs where corruptions is likely to
happen.
Patch 4 is just getting rid of the open-coded version of redundant
u-boot image support in the sunxi NAND driver.

Patch 5 is a simple improvement of the NAND controller status polling
loop, which is really important to make the "trial and error"
approach efficient (we try to limit the impact on boot-time here).
Patch 6 and 7 are implementing the auto-detection logic.

Best Regards,

Boris

Changes since v1:
- added Hans ack
- fixed 2 typos in patch 7

Boris Brezillon (7):
  spl: nand: sunxi: remove support for so-called 'syndrome' mode
  spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option
  spl: nand: support redundant u-boot image
  spl: nand: sunxi: stop guessing the redundant u-boot offset
  spl: nand: sunxi: rework status polling loop
  spl: nand: sunxi: split 'load page' and 'read page' logic
  spl: nand: sunxi: add support for NAND config auto-detection

 common/spl/spl_nand.c             |  12 +
 drivers/mtd/nand/Kconfig          |  15 +-
 drivers/mtd/nand/sunxi_nand_spl.c | 480 ++++++++++++++++++++++++--------------
 3 files changed, 332 insertions(+), 175 deletions(-)

-- 
2.7.4

[U-Boot] [PATCH v2 1/7] spl: nand: sunxi: remove support for so-called 'syndrome' mode

[Boris Brezillon]

The sunxi SPL NAND controller driver supports use 'BootROM'-like configs,
that is, configs where the ECC bytes and real data are interleaved in the
page instead of putting ECC bytes in the OOB area.

Doing that has several drawbacks:
- since you're interleaving data and ECC bytes you can't use the whole page
  otherwise you might override the bad block marker with non-FF bytes.
- to solve the bad block marker problem, the ROM code supports partially
  using the page, but this introduces a huge penalty both in term of read
  speed and NAND memory usage. While this is fine for rather small
  binaries(like the SPL one which is at maximum 24KB large), it becomes
  non-negligible for the bootloader image (several hundred of KB).
- auto-detection of the page size is not reliable (this is in my opinion
  the biggest problem). If you get the page size wrong, you'll end up
  reading data at a different offset than what was specified by the caller
  and the reading may succeed (if valid data were written at this address).

For all those reasons I think it's wiser to completely remove support for
'syndrome' configs. If we ever need to support it again, then I'd recommend
specifying all the config parameters through Kconfig options.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 56 ++++++++++-----------------------------
 1 file changed, 14 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index b0e07aa..1739da2 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -119,9 +119,6 @@ const uint16_t random_seed[128] = {
 	0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
 };
 
-/* random seed used for syndrome calls */
-const uint16_t random_seed_syndrome = 0x4a80;
-
 #define MAX_RETRIES 10
 
 static int check_value_inner(int offset, int expected_bits,
@@ -183,7 +180,7 @@ void nand_init(void)
 }
 
 static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
-	int addr_cycles, uint32_t real_addr, dma_addr_t dst, int syndrome)
+	int addr_cycles, uint32_t real_addr, dma_addr_t dst)
 {
 	uint32_t val;
 	int i, ecc_off = 0;
@@ -209,17 +206,11 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 	page = real_addr / page_size;
 	column = real_addr % page_size;
 
-	if (syndrome)
-		column += (column / ecc_page_size) * ecc_off;
-
 	/* clear ecc status */
 	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
 
 	/* Choose correct seed */
-	if (syndrome)
-		rand_seed = random_seed_syndrome;
-	else
-		rand_seed = random_seed[page % 128];
+	rand_seed = random_seed[page % 128];
 
 	writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
 		| NFC_ECC_PIPELINE | (ecc_mode << 12),
@@ -228,9 +219,8 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 	val = readl(SUNXI_NFC_BASE + NFC_CTL);
 	writel(val | NFC_CTL_RAM_METHOD, SUNXI_NFC_BASE + NFC_CTL);
 
-	if (!syndrome)
-		writel(page_size + (column / ecc_page_size) * ecc_off,
-		       SUNXI_NFC_BASE + NFC_SPARE_AREA);
+	writel(page_size + (column / ecc_page_size) * ecc_off,
+	       SUNXI_NFC_BASE + NFC_SPARE_AREA);
 
 	flush_dcache_range(dst, ALIGN(dst + ecc_page_size, ARCH_DMA_MINALIGN));
 
@@ -266,7 +256,7 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
 		NFC_PAGE_CMD | NFC_WAIT_FLAG |
 		((addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) |
-		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD | (syndrome ? NFC_SEQ : 0),
+		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD,
 		SUNXI_NFC_BASE + NFC_CMD);
 
 	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
@@ -292,7 +282,7 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 }
 
 static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size,
-	int addr_cycles, uint32_t offs, uint32_t size, void *dest, int syndrome)
+	int addr_cycles, uint32_t offs, uint32_t size, void *dest)
 {
 	void *end = dest + size;
 
@@ -301,16 +291,14 @@ static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size,
 
 	for ( ;dest < end; dest += ecc_page_size, offs += ecc_page_size) {
 		if (nand_read_page(page_size, ecc_strength, ecc_page_size,
-				   addr_cycles, offs, (dma_addr_t)dest,
-				   syndrome))
+				   addr_cycles, offs, (dma_addr_t)dest))
 			return -1;
 	}
 
 	return 0;
 }
 
-static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest,
-			    int syndrome)
+static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest)
 {
 	const struct {
 		int page_size;
@@ -337,7 +325,7 @@ static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest,
 					  nand_configs[i].ecc_strength,
 					  nand_configs[i].ecc_page_size,
 					  nand_configs[i].addr_cycles,
-					  offs, size, dest, syndrome) == 0) {
+					  offs, size, dest) == 0) {
 				debug("success\n");
 				nand_config = i;
 				return 0;
@@ -351,48 +339,32 @@ static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest,
 			     nand_configs[nand_config].ecc_strength,
 			     nand_configs[nand_config].ecc_page_size,
 			     nand_configs[nand_config].addr_cycles,
-			     offs, size, dest, syndrome);
+			     offs, size, dest);
 }
 
 int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
 {
-#if CONFIG_SYS_NAND_U_BOOT_OFFS == CONFIG_SPL_PAD_TO
-	/*
-	 * u-boot-dtb.bin appended to SPL, use syndrome (like the BROM does)
-	 * and try different erase block sizes to find the backup.
-	 */
-	const uint32_t boot_offsets[] = {
-		0 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,
-		1 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,
-		2 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,
-		4 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,
-	};
-	const int syndrome = 1;
-#else
 	/*
-	 * u-boot-dtb.bin on its own partition, do not use syndrome, u-boot
-	 * partition sits after 2 eraseblocks (spl, spl-backup), look for
-	 * backup u-boot 1 erase block further.
+	 * u-boot partition sits after 2 eraseblocks (spl, spl-backup), look
+	 * for backup u-boot 1 erase block further.
 	 */
 	const uint32_t eraseblock_size = CONFIG_SYS_NAND_U_BOOT_OFFS / 2;
 	const uint32_t boot_offsets[] = {
 		CONFIG_SYS_NAND_U_BOOT_OFFS,
 		CONFIG_SYS_NAND_U_BOOT_OFFS + eraseblock_size,
 	};
-	const int syndrome = 0;
-#endif
 	int i;
 
 	if (offs == CONFIG_SYS_NAND_U_BOOT_OFFS) {
 		for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) {
 			if (nand_read_buffer(boot_offsets[i], size,
-					     dest, syndrome) == 0)
+					     dest) == 0)
 				return 0;
 		}
 		return -1;
 	}
 
-	return nand_read_buffer(offs, size, dest, syndrome);
+	return nand_read_buffer(offs, size, dest);
 }
 
 void nand_deselect(void)
-- 
2.7.4

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon]

The SYS_NAND_U_BOOT_OFFS is quite generic, but the Kconfig entry is forced
to explicitly depend on platforms that are not already defining it in their
include/configs/<board>.h header.

Rename this Kconfig option into SPL_NAND_U_BOOT_OFFS, remove the dependency
on NAND_SUNXI and make it dependent on SPL selection.

common/spl/spl_nand.c now sets CONFIG_SYS_NAND_U_BOOT_OFFS to
CONFIG_SPL_NAND_U_BOOT_OFFS only when it's undefined. This way we stay
compatible with the existing behavior.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 common/spl/spl_nand.c             | 4 ++++
 drivers/mtd/nand/Kconfig          | 9 +++++----
 drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index bbd9546..612bd4a 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -10,6 +10,10 @@
 #include <asm/io.h>
 #include <nand.h>
 
+#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
+#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
+#endif
+
 #if defined(CONFIG_SPL_NAND_RAW_ONLY)
 int spl_nand_load_image(void)
 {
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 2fc73ef..4b0f92c 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -99,16 +99,17 @@ config SYS_NAND_BUSWIDTH_16BIT
 	    not available while configuring controller. So a static CONFIG_NAND_xx
 	    is needed to know the device's bus-width in advance.
 
-# Enhance depends when converting drivers to Kconfig which use this config
-config SYS_NAND_U_BOOT_OFFS
+if SPL
+
+# This option should be used in replacement of CONFIG_SYS_NAND_U_BOOT_OFFS.
+# CONFIG_SYS_NAND_U_BOOT_OFFS is still preferred if defined.
+config SPL_NAND_U_BOOT_OFFS
 	hex "Location in NAND to read U-Boot from"
 	default 0x8000 if NAND_SUNXI
-	depends on NAND_SUNXI
 	help
 	Set the offset from the start of the nand where u-boot should be
 	loaded from.
 
-if SPL
 
 config SPL_NAND_DENALI
 	bool "Support Denali NAND controller for SPL"
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 1739da2..85cb127 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -348,14 +348,14 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
 	 * u-boot partition sits after 2 eraseblocks (spl, spl-backup), look
 	 * for backup u-boot 1 erase block further.
 	 */
-	const uint32_t eraseblock_size = CONFIG_SYS_NAND_U_BOOT_OFFS / 2;
+	const uint32_t eraseblock_size = CONFIG_SPL_NAND_U_BOOT_OFFS / 2;
 	const uint32_t boot_offsets[] = {
-		CONFIG_SYS_NAND_U_BOOT_OFFS,
-		CONFIG_SYS_NAND_U_BOOT_OFFS + eraseblock_size,
+		CONFIG_SPL_NAND_U_BOOT_OFFS,
+		CONFIG_SPL_NAND_U_BOOT_OFFS + eraseblock_size,
 	};
 	int i;
 
-	if (offs == CONFIG_SYS_NAND_U_BOOT_OFFS) {
+	if (offs == CONFIG_SPL_NAND_U_BOOT_OFFS) {
 		for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) {
 			if (nand_read_buffer(boot_offsets[i], size,
 					     dest) == 0)
-- 
2.7.4

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Scott Wood]

On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> The SYS_NAND_U_BOOT_OFFS is quite generic, but the Kconfig entry is forced
> to explicitly depend on platforms that are not already defining it in their
> include/configs/<board>.h header.
> 
> Rename this Kconfig option into SPL_NAND_U_BOOT_OFFS, remove the dependency
> on NAND_SUNXI and make it dependent on SPL selection.
> 
> common/spl/spl_nand.c now sets CONFIG_SYS_NAND_U_BOOT_OFFS to
> CONFIG_SPL_NAND_U_BOOT_OFFS only when it's undefined. This way we stay
> compatible with the existing behavior.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/spl/spl_nand.c             | 4 ++++
>  drivers/mtd/nand/Kconfig          | 9 +++++----
>  drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++----
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index bbd9546..612bd4a 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -10,6 +10,10 @@
>  #include <asm/io.h>
>  #include <nand.h>
>  
> +#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> +#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> +#endif
[snip]
> -# Enhance depends when converting drivers to Kconfig which use this config
> -config SYS_NAND_U_BOOT_OFFS
> +if SPL
> +
> +# This option should be used in replacement of CONFIG_SYS_NAND_U_BOOT_OFFS.
> +# CONFIG_SYS_NAND_U_BOOT_OFFS is still preferred if defined.
> +config SPL_NAND_U_BOOT_OFFS
>  	hex "Location in NAND to read U-Boot from"
>  	default 0x8000 if NAND_SUNXI
> -	depends on NAND_SUNXI
>  	help
>  	Set the offset from the start of the nand where u-boot should be
>  	loaded from.

This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined when SPL is defined, and the user will be forced to enter a value before kconfig will continue (or kconfig will error out in an automated build).

If you want to do this there needs to be a separate bool config that controls whether the hex config exists.  And there'd be no need to rename hex symbol.

-Scott

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon]

On Fri, 03 Jun 2016 20:08:49 -0500
Scott Wood <oss@buserror.net> wrote:

> On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> > The SYS_NAND_U_BOOT_OFFS is quite generic, but the Kconfig entry is forced
> > to explicitly depend on platforms that are not already defining it in their
> > include/configs/<board>.h header.
> > 
> > Rename this Kconfig option into SPL_NAND_U_BOOT_OFFS, remove the dependency
> > on NAND_SUNXI and make it dependent on SPL selection.
> > 
> > common/spl/spl_nand.c now sets CONFIG_SYS_NAND_U_BOOT_OFFS to
> > CONFIG_SPL_NAND_U_BOOT_OFFS only when it's undefined. This way we stay
> > compatible with the existing behavior.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  common/spl/spl_nand.c             | 4 ++++
> >  drivers/mtd/nand/Kconfig          | 9 +++++----
> >  drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++----
> >  3 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> > index bbd9546..612bd4a 100644
> > --- a/common/spl/spl_nand.c
> > +++ b/common/spl/spl_nand.c
> > @@ -10,6 +10,10 @@
> >  #include <asm/io.h>
> >  #include <nand.h>
> >  
> > +#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> > +#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > +#endif  
> [snip]
> > -# Enhance depends when converting drivers to Kconfig which use this config
> > -config SYS_NAND_U_BOOT_OFFS
> > +if SPL
> > +
> > +# This option should be used in replacement of CONFIG_SYS_NAND_U_BOOT_OFFS.
> > +# CONFIG_SYS_NAND_U_BOOT_OFFS is still preferred if defined.
> > +config SPL_NAND_U_BOOT_OFFS
> >  	hex "Location in NAND to read U-Boot from"
> >  	default 0x8000 if NAND_SUNXI
> > -	depends on NAND_SUNXI
> >  	help
> >  	Set the offset from the start of the nand where u-boot should be
> >  	loaded from.  
> 
> This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined when SPL is defined, and the user will be forced to enter a value before kconfig will continue (or kconfig will error out in an automated build).

Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
file.
And for the "user will forced to enter a value before Kconfig
continue" comment, we could just have

config SPL_NAND_U_BOOT_OFFS
  	hex "Location in NAND to read U-Boot from"
  	default 0x8000 if NAND_SUNXI
	default 0x0
	...

> 
> If you want to do this there needs to be a separate bool config that controls whether the hex config exists.

I can add an extra Kconfig option, but is it really necessary:
if people are relying on it they will choose a valid value, and leave
it to 0 otherwise.
It's just a detail, so I'm fine adding this extra option if you think
it's really useful.

> And there'd be no need to rename hex symbol.

Well, functionally there's no problem keeping the existing SYS_ prefix
if we add this extra option to activate the U_OFFS config in Kconfig,
but I'm not sure this is a good idea to reuse config header names in
Kconfig.

And what happens if the user enabled this option (some like to enable
everything :-)) and CONFIG_SYS_NAND_U_BOOT_OFFS is also defined in the
board config header?
That's what I was trying to avoid. By renaming the Kconfig option we
make sure the value defined in include/configs/<board>.h always take
precedence over the Kconfig value.



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

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Scott Wood]

On Sat, 2016-06-04 at 08:06 +0200, Boris Brezillon wrote:
> On Fri, 03 Jun 2016 20:08:49 -0500
> Scott Wood <oss@buserror.net> wrote:
>
> > This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined
> > when SPL is defined, and the user will be forced to enter a value before
> > kconfig will continue (or kconfig will error out in an automated build).
> 
> Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
> used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
> file.
> And for the "user will forced to enter a value before Kconfig
> continue" comment, we could just have
> 
> config SPL_NAND_U_BOOT_OFFS
>   	hex "Location in NAND to read U-Boot from"
>   	default 0x8000 if NAND_SUNXI
> 	default 0x0
> 	...

If you do that, then that zero will override CONFIG_SYS_NAND_U_BOOT_OFFS from
the header.

> > If you want to do this there needs to be a separate bool config that
> > controls whether the hex config exists.
> 
> I can add an extra Kconfig option, but is it really necessary:
> if people are relying on it they will choose a valid value, and leave
> it to 0 otherwise.
> It's just a detail, so I'm fine adding this extra option if you think
> it's really useful.

Zero *is* a valid value.  Several boards already have that value for this
symbol.  Even if that weren't the case,  we want a mechanism for migrating
from header value to kconfig value that works for more than just this one
specific symbol.

> 
> > And there'd be no need to rename hex symbol.
> 
> Well, functionally there's no problem keeping the existing SYS_ prefix
> if we add this extra option to activate the U_OFFS config in Kconfig,
> but I'm not sure this is a good idea to reuse config header names in
> Kconfig.
> 
> And what happens if the user enabled this option (some like to enable
> everything :-)) and CONFIG_SYS_NAND_U_BOOT_OFFS is also defined in the
> board config header?

Then the build fails with a redefined symbol, and the user learns their
lesson. :-)

The "SYS" in CONFIG_SYS means it's not a user-tunable knob.  From README:

> There are two classes of configuration variables:
> 
> * Configuration _OPTIONS_:
>   These are selectable by the user and have names beginning with
>   "CONFIG_".
> 
> * Configuration _SETTINGS_:
>   These depend on the hardware etc. and should not be meddled with if
>   you don't know what you're doing; they have names beginning with
>   "CONFIG_SYS_".

-Scott

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon]

On Sat, 04 Jun 2016 02:14:09 -0500
Scott Wood <oss@buserror.net> wrote:

> On Sat, 2016-06-04 at 08:06 +0200, Boris Brezillon wrote:
> > On Fri, 03 Jun 2016 20:08:49 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >  
> > > This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined
> > > when SPL is defined, and the user will be forced to enter a value before
> > > kconfig will continue (or kconfig will error out in an automated build).  
> > 
> > Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
> > used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
> > file.
> > And for the "user will forced to enter a value before Kconfig
> > continue" comment, we could just have
> > 
> > config SPL_NAND_U_BOOT_OFFS
> >   	hex "Location in NAND to read U-Boot from"
> >   	default 0x8000 if NAND_SUNXI
> > 	default 0x0
> > 	...  
> 
> If you do that, then that zero will override CONFIG_SYS_NAND_U_BOOT_OFFS from
> the header.

Nope, because the condition is

#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
#endif

and not

#ifdef CONFIG_SPL_NAND_U_BOOT_OFFS
#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
#endif

So CONFIG_SYS_NAND_U_BOOT_OFFS is always preferred over
CONFIG_SPL_NAND_U_BOOT_OFFS if it's defined.

> 
> > > If you want to do this there needs to be a separate bool config that
> > > controls whether the hex config exists.  
> > 
> > I can add an extra Kconfig option, but is it really necessary:
> > if people are relying on it they will choose a valid value, and leave
> > it to 0 otherwise.
> > It's just a detail, so I'm fine adding this extra option if you think
> > it's really useful.  
> 
> Zero *is* a valid value.  Several boards already have that value for this
> symbol.  Even if that weren't the case,  we want a mechanism for migrating
> from header value to kconfig value that works for more than just this one
> specific symbol.

Sure, 0 is a perfectly valid value. The "default 0" is just here to
prevent make from blocking because of a missing definition in the
_defconfig.

> 
> >   
> > > And there'd be no need to rename hex symbol.  
> > 
> > Well, functionally there's no problem keeping the existing SYS_ prefix
> > if we add this extra option to activate the U_OFFS config in Kconfig,
> > but I'm not sure this is a good idea to reuse config header names in
> > Kconfig.
> > 
> > And what happens if the user enabled this option (some like to enable
> > everything :-)) and CONFIG_SYS_NAND_U_BOOT_OFFS is also defined in the
> > board config header?  
> 
> Then the build fails with a redefined symbol, and the user learns their
> lesson. :-)

Fair enough.

> 
> The "SYS" in CONFIG_SYS means it's not a user-tunable knob.  From README:
> 
> > There are two classes of configuration variables:
> > 
> > * Configuration _OPTIONS_:
> >   These are selectable by the user and have names beginning with
> >   "CONFIG_".
> > 
> > * Configuration _SETTINGS_:
> >   These depend on the hardware etc. and should not be meddled with if
> >   you don't know what you're doing; they have names beginning with
> >   "CONFIG_SYS_".  

Okay. I'll switch back to SYS_NAND_U_BOOT_OFFS, add an intermediate
option to unlock this one in the config menu and rename
CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND into
CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND.


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

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Scott Wood]

On Sat, 2016-06-04 at 13:06 +0200, Boris Brezillon wrote:
> On Sat, 04 Jun 2016 02:14:09 -0500
> Scott Wood <oss@buserror.net> wrote:
> 
> > On Sat, 2016-06-04 at 08:06 +0200, Boris Brezillon wrote:
> > > On Fri, 03 Jun 2016 20:08:49 -0500
> > > Scott Wood <oss@buserror.net> wrote:
> > >  
> > > > This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined
> > > > when SPL is defined, and the user will be forced to enter a value
> > > > before
> > > > kconfig will continue (or kconfig will error out in an automated
> > > > build).  
> > > 
> > > Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
> > > used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
> > > file.
> > > And for the "user will forced to enter a value before Kconfig
> > > continue" comment, we could just have
> > > 
> > > config SPL_NAND_U_BOOT_OFFS
> > >   	hex "Location in NAND to read U-Boot from"
> > >   	default 0x8000 if NAND_SUNXI
> > > 	default 0x0
> > > 	...  
> > 
> > If you do that, then that zero will override CONFIG_SYS_NAND_U_BOOT_OFFS
> > from
> > the header.
> 
> Nope, because the condition is
> 
> #ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> #endif
> 
> and not
> 
> #ifdef CONFIG_SPL_NAND_U_BOOT_OFFS
> #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> #endif
> 
> So CONFIG_SYS_NAND_U_BOOT_OFFS is always preferred over
> CONFIG_SPL_NAND_U_BOOT_OFFS if it's defined.

Ah, right.  Still, I think it would be less confusing to not have two
different names for the same thing, both of which would be present (albeit
only one is used) in the legacy case -- especially if we start adding
references directly to the SPL name in some drivers.  The bool could
eventually be reversed so that only the legacy targets need it.

-Scott

[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon]

On Mon, 06 Jun 2016 12:16:33 -0500
Scott Wood <oss@buserror.net> wrote:

> On Sat, 2016-06-04 at 13:06 +0200, Boris Brezillon wrote:
> > On Sat, 04 Jun 2016 02:14:09 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >   
> > > On Sat, 2016-06-04 at 08:06 +0200, Boris Brezillon wrote:  
> > > > On Fri, 03 Jun 2016 20:08:49 -0500
> > > > Scott Wood <oss@buserror.net> wrote:
> > > >    
> > > > > This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined
> > > > > when SPL is defined, and the user will be forced to enter a value
> > > > > before
> > > > > kconfig will continue (or kconfig will error out in an automated
> > > > > build).    
> > > > 
> > > > Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
> > > > used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
> > > > file.
> > > > And for the "user will forced to enter a value before Kconfig
> > > > continue" comment, we could just have
> > > > 
> > > > config SPL_NAND_U_BOOT_OFFS
> > > >   	hex "Location in NAND to read U-Boot from"
> > > >   	default 0x8000 if NAND_SUNXI
> > > > 	default 0x0
> > > > 	...    
> > > 
> > > If you do that, then that zero will override CONFIG_SYS_NAND_U_BOOT_OFFS
> > > from
> > > the header.  
> > 
> > Nope, because the condition is
> > 
> > #ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> > #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > #endif
> > 
> > and not
> > 
> > #ifdef CONFIG_SPL_NAND_U_BOOT_OFFS
> > #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > #endif
> > 
> > So CONFIG_SYS_NAND_U_BOOT_OFFS is always preferred over
> > CONFIG_SPL_NAND_U_BOOT_OFFS if it's defined.  
> 
> Ah, right.  Still, I think it would be less confusing to not have two
> different names for the same thing, both of which would be present (albeit
> only one is used) in the legacy case -- especially if we start adding
> references directly to the SPL name in some drivers.  The bool could
> eventually be reversed so that only the legacy targets need it.

I posted a new version with the extra bool option this morning ;).

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

[U-Boot] [PATCH v2 3/7] spl: nand: support redundant u-boot image

[Boris Brezillon]

On modern NAND it's more than recommended to have a backup copy of the
u-boot binary to recover from corruption: bitflips are quite common on
MLC NANDs, and the read-disturbance will corrupt your u-boot partitition
more quickly than what you would see on an SLC NAND.

Add an extra Kconfig option to specify the offset of the redundant u-boot
image.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 common/spl/spl_nand.c    | 8 ++++++++
 drivers/mtd/nand/Kconfig | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 612bd4a..0bf0848 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -12,6 +12,9 @@
 
 #ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
 #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
+#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND
+#else
+#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND CONFIG_SYS_NAND_U_BOOT_OFFS
 #endif
 
 #if defined(CONFIG_SPL_NAND_RAW_ONLY)
@@ -111,6 +114,11 @@ int spl_nand_load_image(void)
 #endif
 	/* Load u-boot */
 	err = spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS, header);
+#if CONFIG_SYS_NAND_U_BOOT_OFFS != CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND
+	if (err)
+		err = spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND,
+					    header);
+#endif
 	nand_deselect();
 	return err;
 }
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 4b0f92c..2f1d1f7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -110,6 +110,12 @@ config SPL_NAND_U_BOOT_OFFS
 	Set the offset from the start of the nand where u-boot should be
 	loaded from.
 
+config SPL_NAND_U_BOOT_OFFS_REDUND
+	hex "Location in NAND to read U-Boot from"
+	default SPL_NAND_U_BOOT_OFFS
+	help
+	Set the offset from the start of the nand where u-boot should be
+	loaded from.
 
 config SPL_NAND_DENALI
 	bool "Support Denali NAND controller for SPL"
-- 
2.7.4

[U-Boot] [PATCH v2 3/7] spl: nand: support redundant u-boot image

[Scott Wood]

On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> On modern NAND it's more than recommended to have a backup copy of the
> u-boot binary to recover from corruption: bitflips are quite common on
> MLC NANDs, and the read-disturbance will corrupt your u-boot partitition
> more quickly than what you would see on an SLC NAND.

Wouldn't the same happen to the SPL itself?  Or is the boot block implemented
in a different, more robust manner?

> Add an extra Kconfig option to specify the offset of the redundant u-boot
> image.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  common/spl/spl_nand.c    | 8 ++++++++
>  drivers/mtd/nand/Kconfig | 6 ++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 612bd4a..0bf0848 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -12,6 +12,9 @@
>  
>  #ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
>  #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> +#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND
> CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND
> +#else
> +#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND CONFIG_SYS_NAND_U_BOOT_OFFS
>  #endif
>  
>  #if defined(CONFIG_SPL_NAND_RAW_ONLY)
> @@ -111,6 +114,11 @@ int spl_nand_load_image(void)
>  #endif
>  	/* Load u-boot */
>  	err = spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS, header);
> +#if CONFIG_SYS_NAND_U_BOOT_OFFS != CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND
> +	if (err)
> +		err =
> spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND,
> +					    header);
> +#endif

If one of the images has failed, doesn't it need to be reflashed before the
other one goes bad as well?  How does the failure get communicated to later
parts of the system that would be responsible for such reflashing?

-Scott

[U-Boot] [PATCH v2 3/7] spl: nand: support redundant u-boot image

[Boris Brezillon]

On Fri, 03 Jun 2016 20:15:16 -0500
Scott Wood <oss@buserror.net> wrote:

> On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> > On modern NAND it's more than recommended to have a backup copy of the
> > u-boot binary to recover from corruption: bitflips are quite common on
> > MLC NANDs, and the read-disturbance will corrupt your u-boot partitition
> > more quickly than what you would see on an SLC NAND.  
> 
> Wouldn't the same happen to the SPL itself?  Or is the boot block implemented
> in a different, more robust manner?

Nope, the same happens to the SPL image, and we're actually using the
same trick: the brom code search for a valid SPL image every 64
pages is duplicated every 64 pages (it tests the first 8 locations:
page 0, page 64, page 128, ..., page 448).

We usually fill 2 blocks with SPL images (repeating it several times in
each block).

> 
> > Add an extra Kconfig option to specify the offset of the redundant u-boot
> > image.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  common/spl/spl_nand.c    | 8 ++++++++
> >  drivers/mtd/nand/Kconfig | 6 ++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> > index 612bd4a..0bf0848 100644
> > --- a/common/spl/spl_nand.c
> > +++ b/common/spl/spl_nand.c
> > @@ -12,6 +12,9 @@
> >  
> >  #ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> >  #define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > +#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND
> > CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND
> > +#else
> > +#define CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND CONFIG_SYS_NAND_U_BOOT_OFFS
> >  #endif
> >  
> >  #if defined(CONFIG_SPL_NAND_RAW_ONLY)
> > @@ -111,6 +114,11 @@ int spl_nand_load_image(void)
> >  #endif
> >  	/* Load u-boot */
> >  	err = spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS, header);
> > +#if CONFIG_SYS_NAND_U_BOOT_OFFS != CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND
> > +	if (err)
> > +		err =
> > spl_nand_load_element(CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND,
> > +					    header);
> > +#endif  
> 
> If one of the images has failed, doesn't it need to be reflashed before the
> other one goes bad as well?

Yes, that's the idea.

> How does the failure get communicated to later
> parts of the system that would be responsible for such reflashing?

Linux is taking care of that (a script tries to read the u-boot
partition, and if fails it reflashes it with the content of the
u-boot-backup partition, or with a reference u-boot.bin file).
I guess u-boot could do it too.

Anyway, that's a different story ;).

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

[U-Boot] [PATCH v2 3/7] spl: nand: support redundant u-boot image

[Scott Wood]

On Sat, 2016-06-04 at 08:15 +0200, Boris Brezillon wrote:
> On Fri, 03 Jun 2016 20:15:16 -0500
> Scott Wood <oss@buserror.net> wrote:
> 
> > How does the failure get communicated to later
> > parts of the system that would be responsible for such reflashing?
> 
> Linux is taking care of that (a script tries to read the u-boot
> partition, and if fails it reflashes it with the content of the
> u-boot-backup partition, or with a reference u-boot.bin file).
> I guess u-boot could do it too.
> 
> Anyway, that's a different story ;).

It seemed like it would be good to export the information if possible (e.g. an
environment variable) rather than rereading and thus making failures happen
twice as quickly.  But that can wait until someone actually wants to use it
that way.

-Scott

[U-Boot] [PATCH v2 4/7] spl: nand: sunxi: stop guessing the redundant u-boot offset

[Boris Brezillon]

Use CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND value instead of trying to guess
where the redundant u-boot image is based on simple (and most of the time
erroneous) heuristics.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 85cb127..13e6eab 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -344,26 +344,6 @@ static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest)
 
 int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
 {
-	/*
-	 * u-boot partition sits after 2 eraseblocks (spl, spl-backup), look
-	 * for backup u-boot 1 erase block further.
-	 */
-	const uint32_t eraseblock_size = CONFIG_SPL_NAND_U_BOOT_OFFS / 2;
-	const uint32_t boot_offsets[] = {
-		CONFIG_SPL_NAND_U_BOOT_OFFS,
-		CONFIG_SPL_NAND_U_BOOT_OFFS + eraseblock_size,
-	};
-	int i;
-
-	if (offs == CONFIG_SPL_NAND_U_BOOT_OFFS) {
-		for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) {
-			if (nand_read_buffer(boot_offsets[i], size,
-					     dest) == 0)
-				return 0;
-		}
-		return -1;
-	}
-
 	return nand_read_buffer(offs, size, dest);
 }
 
-- 
2.7.4

[U-Boot] [PATCH v2 5/7] spl: nand: sunxi: rework status polling loop

[Boris Brezillon]

check_value_xxx() helpers are using a 1ms delay between each test, which
can be quite long for some operations (like a page read on an SLC NAND).
Since we don't have anything to do but to poll this register, reduce the
delay between each test to 1us.

While we're at it, rename the max_number_of_retries parameters and the
MAX_RETRIES macro into timeout_us and DEFAULT_TIMEOUT_US to reflect that
we're actually waiting a given amount of time and not only a number of
retries.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 13e6eab..55b3c8a 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -119,35 +119,31 @@ const uint16_t random_seed[128] = {
 	0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
 };
 
-#define MAX_RETRIES 10
+#define DEFAULT_TIMEOUT_US	100000
 
 static int check_value_inner(int offset, int expected_bits,
-				int max_number_of_retries, int negation)
+			     int timeout_us, int negation)
 {
-	int retries = 0;
 	do {
 		int val = readl(offset) & expected_bits;
 		if (negation ? !val : val)
 			return 1;
-		mdelay(1);
-		retries++;
-	} while (retries < max_number_of_retries);
+		udelay(1);
+	} while (--timeout_us);
 
 	return 0;
 }
 
 static inline int check_value(int offset, int expected_bits,
-				int max_number_of_retries)
+			      int timeout_us)
 {
-	return check_value_inner(offset, expected_bits,
-					max_number_of_retries, 0);
+	return check_value_inner(offset, expected_bits, timeout_us, 0);
 }
 
 static inline int check_value_negated(int offset, int unexpected_bits,
-					int max_number_of_retries)
+				      int timeout_us)
 {
-	return check_value_inner(offset, unexpected_bits,
-					max_number_of_retries, 1);
+	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
 }
 
 void nand_init(void)
@@ -162,7 +158,7 @@ void nand_init(void)
 	       SUNXI_NFC_BASE + NFC_CTL);
 
 	if (!check_value_negated(SUNXI_NFC_BASE + NFC_CTL,
-				 NFC_CTL_RESET, MAX_RETRIES)) {
+				 NFC_CTL_RESET, DEFAULT_TIMEOUT_US)) {
 		printf("Couldn't initialize nand\n");
 	}
 
@@ -172,7 +168,7 @@ void nand_init(void)
 	       SUNXI_NFC_BASE + NFC_CMD);
 
 	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
-			 MAX_RETRIES)) {
+			 DEFAULT_TIMEOUT_US)) {
 		printf("Error timeout waiting for nand reset\n");
 		return;
 	}
@@ -260,14 +256,15 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 		SUNXI_NFC_BASE + NFC_CMD);
 
 	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
-			 MAX_RETRIES)) {
+			 DEFAULT_TIMEOUT_US)) {
 		printf("Error while initializing dma interrupt\n");
 		return -1;
 	}
 	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 
 	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
-				 SUNXI_DMA_DDMA_CFG_REG_LOADING, MAX_RETRIES)) {
+				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
+				 DEFAULT_TIMEOUT_US)) {
 		printf("Error while waiting for dma transfer to finish\n");
 		return -1;
 	}
-- 
2.7.4

[U-Boot] [PATCH v2 6/7] spl: nand: sunxi: split 'load page' and 'read page' logic

[Boris Brezillon]

Split the 'load page' and 'read page' logic in 2 different functions so
we can later load the page and test different ECC configs without the
penalty of reloading the same page in the NAND cache.

We also move common setup to a dedicated function (nand_apply_config()) to
avoid rewriting the same values in NFC registers each time we read a page.

These new functions are passed a pointer to an nfc_config struct to limit
the number of parameters.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mtd/nand/sunxi_nand_spl.c | 185 +++++++++++++++++++++++---------------
 1 file changed, 114 insertions(+), 71 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index 55b3c8a..b43f2af 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -66,6 +66,8 @@
 #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_ST_CMD_INT_FLAG        (1 << 1)
 #define NFC_ST_DMA_INT_FLAG        (1 << 2)
@@ -78,9 +80,6 @@
 #define NFC_CMD_RNDOUT             0x05
 #define NFC_CMD_READSTART          0x30
 
-
-#define NFC_PAGE_CMD               (2 << 30)
-
 #define SUNXI_DMA_CFG_REG0              0x300
 #define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
 #define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
@@ -97,6 +96,15 @@
 #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;
+	int ecc_size;
+	int addr_cycles;
+	int nseeds;
+	bool randomize;
+};
+
 /* minimal "boot0" style NAND support for Allwinner A20 */
 
 /* random seed used by linux */
@@ -175,50 +183,70 @@ void nand_init(void)
 	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 }
 
-static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
-	int addr_cycles, uint32_t real_addr, dma_addr_t dst)
+static void nand_apply_config(const struct nfc_config *conf)
 {
-	uint32_t val;
-	int i, ecc_off = 0;
-	uint16_t ecc_mode = 0;
-	uint16_t rand_seed;
-	uint32_t page;
-	uint16_t column;
-	static const u8 strengths[] = { 16, 24, 28, 32, 40, 48, 56, 60, 64 };
+	u32 val;
 
-	for (i = 0; i < ARRAY_SIZE(strengths); i++) {
-		if (ecc_strength == strengths[i]) {
-			ecc_mode = i;
-			break;
-		}
+	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),
+	       SUNXI_NFC_BASE + NFC_CTL);
+	writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
+	writel(conf->page_size, SUNXI_NFC_BASE + NFC_SPARE_AREA);
+}
+
+static int nand_load_page(const struct nfc_config *conf, u32 offs)
+{
+	int page = offs / conf->page_size;
+
+	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
+	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
+	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
+	       SUNXI_NFC_BASE + NFC_RCMD_SET);
+	writel(((page & 0xFFFF) << 16), SUNXI_NFC_BASE + NFC_ADDR_LOW);
+	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,
+	       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;
 	}
 
-	/* HW ECC always request ECC bytes for 1024 bytes blocks */
-	ecc_off = DIV_ROUND_UP(ecc_strength * fls(8 * 1024), 8);
-	/* HW ECC always work with even numbers of ECC bytes */
-	ecc_off += (ecc_off & 1);
-	ecc_off += 4; /* prepad */
+	return 0;
+}
+
+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;
+	u32 val;
+	int page;
+
+	page = offs / conf->page_size;
 
-	page = real_addr / page_size;
-	column = real_addr % page_size;
+	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 */
-	rand_seed = random_seed[page % 128];
+	rand_seed = random_seed[page % conf->nseeds];
 
-	writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
-		| NFC_ECC_PIPELINE | (ecc_mode << 12),
+	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);
 
-	val = readl(SUNXI_NFC_BASE + NFC_CTL);
-	writel(val | NFC_CTL_RAM_METHOD, SUNXI_NFC_BASE + NFC_CTL);
-
-	writel(page_size + (column / ecc_page_size) * ecc_off,
-	       SUNXI_NFC_BASE + NFC_SPARE_AREA);
-
-	flush_dcache_range(dst, ALIGN(dst + ecc_page_size, ARCH_DMA_MINALIGN));
+	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 */
@@ -227,38 +255,27 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 	       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(ecc_page_size,
-	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0); /* 1kB */
-	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((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET)
-		| (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET)
-		| (NFC_CMD_READSTART | NFC_READ_CMD_OFFSET), SUNXI_NFC_BASE
-			+ NFC_RCMD_SET);
-	writel(1, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
-	writel(((page & 0xFFFF) << 16) | column,
-	       SUNXI_NFC_BASE + NFC_ADDR_LOW);
-	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
+	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_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
-		NFC_PAGE_CMD | NFC_WAIT_FLAG |
-		((addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) |
-		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD,
-		SUNXI_NFC_BASE + NFC_CMD);
+	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
+	       SUNXI_NFC_BASE + NFC_CMD);
 
 	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
 			 DEFAULT_TIMEOUT_US)) {
 		printf("Error while initializing dma interrupt\n");
-		return -1;
+		return -EIO;
 	}
 	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
 
@@ -266,29 +283,55 @@ static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size,
 				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
 				 DEFAULT_TIMEOUT_US)) {
 		printf("Error while waiting for dma transfer to finish\n");
-		return -1;
+		return -EIO;
 	}
 
 	invalidate_dcache_range(dst,
-			        ALIGN(dst + ecc_page_size, ARCH_DMA_MINALIGN));
+				ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
 
-	if (readl(SUNXI_NFC_BASE + NFC_ECC_ST))
-		return -1;
+	val = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
 
-	return 0;
+	/* ECC error detected. */
+	if (val & 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;
 }
 
 static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size,
 	int addr_cycles, uint32_t offs, uint32_t size, void *dest)
 {
 	void *end = dest + size;
+	static const u8 strengths[] = { 16, 24, 28, 32, 40, 48, 56, 60, 64 };
+	struct nfc_config conf = {
+		.page_size = page_size,
+		.ecc_size = ecc_page_size,
+		.addr_cycles = addr_cycles,
+		.nseeds = ARRAY_SIZE(random_seed),
+		.randomize = true,
+	};
+	int i;
 
-	clrsetbits_le32(SUNXI_NFC_BASE + NFC_CTL, NFC_CTL_PAGE_SIZE_MASK,
-			NFC_CTL_PAGE_SIZE(page_size));
+	for (i = 0; i < ARRAY_SIZE(strengths); i++) {
+		if (ecc_strength == strengths[i]) {
+			conf.ecc_strength = i;
+			break;
+		}
+	}
+
+
+	nand_apply_config(&conf);
+
+	for ( ;dest < end; dest += ecc_page_size, offs += page_size) {
+		if (nand_load_page(&conf, offs))
+			return -1;
 
-	for ( ;dest < end; dest += ecc_page_size, offs += ecc_page_size) {
-		if (nand_read_page(page_size, ecc_strength, ecc_page_size,
-				   addr_cycles, offs, (dma_addr_t)dest))
+		if (nand_read_page(&conf, offs, dest, page_size))
 			return -1;
 	}
 
-- 
2.7.4

[U-Boot] [PATCH v2 7/7] spl: nand: sunxi: add support for NAND config auto-detection

[Boris Brezillon]

NAND chips are supposed to expose their capabilities through advanced
mechanisms like READID, ONFI or JEDEC parameter tables. While those
methods are appropriate for the bootloader itself, it's way to
complicated and takes too much space to fit in the SPL.

Replace those mechanisms by a dumb 'trial and error' mechanism.

With this new approach we can get rid of the fixed config list that was
used in the sunxi NAND SPL driver.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
Changes since v1:
- fix the nand_max_ecc_strength()
---
 drivers/mtd/nand/sunxi_nand_spl.c | 262 +++++++++++++++++++++++++++++---------
 1 file changed, 204 insertions(+), 58 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
index b43f2af..1ef7366 100644
--- a/drivers/mtd/nand/sunxi_nand_spl.c
+++ b/drivers/mtd/nand/sunxi_nand_spl.c
@@ -103,6 +103,7 @@ struct nfc_config {
 	int addr_cycles;
 	int nseeds;
 	bool randomize;
+	bool valid;
 };
 
 /* minimal "boot0" style NAND support for Allwinner A20 */
@@ -219,6 +220,26 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
 	return 0;
 }
 
+static int nand_reset_column(void)
+{
+	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(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,
+	       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 0;
+}
+
 static int nand_read_page(const struct nfc_config *conf, u32 offs,
 			  void *dest, int len)
 {
@@ -303,88 +324,213 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
 	return (val & 0x10000) ? 1 : 0;
 }
 
-static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size,
-	int addr_cycles, uint32_t offs, uint32_t size, void *dest)
+static int nand_max_ecc_strength(struct nfc_config *conf)
 {
-	void *end = dest + size;
-	static const u8 strengths[] = { 16, 24, 28, 32, 40, 48, 56, 60, 64 };
-	struct nfc_config conf = {
-		.page_size = page_size,
-		.ecc_size = ecc_page_size,
-		.addr_cycles = addr_cycles,
-		.nseeds = ARRAY_SIZE(random_seed),
-		.randomize = true,
-	};
+	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;
 
-	for (i = 0; i < ARRAY_SIZE(strengths); i++) {
-		if (ecc_strength == strengths[i]) {
-			conf.ecc_strength = i;
+	/*
+	 * ECC strength is limited by the size of the OOB area which is
+	 * correlated with the page size.
+	 */
+	switch (conf->page_size) {
+	case 2048:
+		max_oobsize = 64;
+		break;
+	case 4096:
+		max_oobsize = 256;
+		break;
+	case 8192:
+		max_oobsize = 640;
+		break;
+	case 16384:
+		max_oobsize = 1664;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	max_ecc_bytes = max_oobsize / nsectors;
+
+	for (i = 0; i < ARRAY_SIZE(ecc_bytes); i++) {
+		if (ecc_bytes[i] > max_ecc_bytes)
 			break;
-		}
 	}
 
+	if (!i)
+		return -EINVAL;
 
-	nand_apply_config(&conf);
+	return i - 1;
+}
 
-	for ( ;dest < end; dest += ecc_page_size, offs += page_size) {
-		if (nand_load_page(&conf, offs))
-			return -1;
+static int nand_detect_ecc_config(struct nfc_config *conf, u32 offs,
+				  void *dest)
+{
+	/* NAND with pages > 4k will likely require 1k sector size. */
+	int min_ecc_size = conf->page_size > 4096 ? 1024 : 512;
+	int page = offs / conf->page_size;
+	int ret;
 
-		if (nand_read_page(&conf, offs, dest, page_size))
-			return -1;
+	/*
+	 * In most cases, 1k sectors are preferred over 512b ones, start
+	 * testing this config first.
+	 */
+	for (conf->ecc_size = 1024; conf->ecc_size >= min_ecc_size;
+	     conf->ecc_size >>= 1) {
+		int max_ecc_strength = nand_max_ecc_strength(conf);
+
+		nand_apply_config(conf);
+
+		/*
+		 * We are starting from the maximum ECC strength because
+		 * most of the time NAND vendors provide an OOB area that
+		 * barely meets the ECC requirements.
+		 */
+		for (conf->ecc_strength = max_ecc_strength;
+		     conf->ecc_strength >= 0;
+		     conf->ecc_strength--) {
+			conf->randomize = false;
+			if (nand_reset_column())
+				return -EIO;
+
+			/*
+			 * Only read the first sector to speedup detection.
+			 */
+			ret = nand_read_page(conf, offs, dest, conf->ecc_size);
+			if (!ret) {
+				return 0;
+			} else if (ret > 0) {
+				/*
+				 * If page is empty we can't deduce anything
+				 * about the ECC config => stop the detection.
+				 */
+				return -EINVAL;
+			}
+
+			conf->randomize = true;
+			conf->nseeds = ARRAY_SIZE(random_seed);
+			do {
+				if (nand_reset_column())
+					return -EIO;
+
+				if (!nand_read_page(conf, offs, dest,
+						    conf->ecc_size))
+					return 0;
+
+				/*
+				 * Find the next ->nseeds value that would
+				 * change the randomizer seed for the page
+				 * we're trying to read.
+				 */
+				while (conf->nseeds >= 16) {
+					int seed = page % conf->nseeds;
+
+					conf->nseeds >>= 1;
+					if (seed != page % conf->nseeds)
+						break;
+				}
+			} while (conf->nseeds >= 16);
+		}
 	}
 
-	return 0;
+	return -EINVAL;
 }
 
-static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest)
+static int nand_detect_config(struct nfc_config *conf, u32 offs, void *dest)
 {
-	const struct {
-		int page_size;
-		int ecc_strength;
-		int ecc_page_size;
-		int addr_cycles;
-	} nand_configs[] = {
-		{  8192, 40, 1024, 5 },
-		{ 16384, 56, 1024, 5 },
-		{  8192, 24, 1024, 5 },
-		{  4096, 24, 1024, 5 },
-	};
-	static int nand_config = -1;
-	int i;
+	if (conf->valid)
+		return 0;
 
-	if (nand_config == -1) {
-		for (i = 0; i < ARRAY_SIZE(nand_configs); i++) {
-			debug("nand: trying page %d ecc %d / %d addr %d: ",
-			      nand_configs[i].page_size,
-			      nand_configs[i].ecc_strength,
-			      nand_configs[i].ecc_page_size,
-			      nand_configs[i].addr_cycles);
-			if (nand_read_ecc(nand_configs[i].page_size,
-					  nand_configs[i].ecc_strength,
-					  nand_configs[i].ecc_page_size,
-					  nand_configs[i].addr_cycles,
-					  offs, size, dest) == 0) {
-				debug("success\n");
-				nand_config = i;
+	/*
+	 * Modern NANDs are more likely than legacy ones, so we start testing
+	 * with 5 address cycles.
+	 */
+	for (conf->addr_cycles = 5;
+	     conf->addr_cycles >= 4;
+	     conf->addr_cycles--) {
+		int max_page_size = conf->addr_cycles == 4 ? 2048 : 16384;
+
+		/*
+		 * Ignoring 1k pages cause I'm not even sure this case exist
+		 * in the real world.
+		 */
+		for (conf->page_size = 2048; conf->page_size <= max_page_size;
+		     conf->page_size <<= 1) {
+			if (nand_load_page(conf, offs))
+				return -1;
+
+			if (!nand_detect_ecc_config(conf, offs, dest)) {
+				conf->valid = true;
 				return 0;
 			}
-			debug("failed\n");
 		}
-		return -1;
 	}
 
-	return nand_read_ecc(nand_configs[nand_config].page_size,
-			     nand_configs[nand_config].ecc_strength,
-			     nand_configs[nand_config].ecc_page_size,
-			     nand_configs[nand_config].addr_cycles,
-			     offs, size, dest);
+	return -EINVAL;
+}
+
+static int nand_read_buffer(struct nfc_config *conf, uint32_t offs,
+			    unsigned int size, void *dest)
+{
+	int first_seed, page, ret;
+
+	size = ALIGN(size, conf->page_size);
+	page = offs / conf->page_size;
+	first_seed = page % conf->nseeds;
+
+	for (; size; size -= conf->page_size) {
+		if (nand_load_page(conf, offs))
+			return -1;
+
+		ret = nand_read_page(conf, offs, dest, conf->page_size);
+		/*
+		 * The ->nseeds value should be equal to the number of pages
+		 * in an eraseblock. Since we don't know this information in
+		 * advance we might have picked a wrong value.
+		 */
+		if (ret < 0 && conf->randomize) {
+			int cur_seed = page % conf->nseeds;
+
+			/*
+			 * We already tried all the seed values => we are
+			 * facing a real corruption.
+			 */
+			if (cur_seed < first_seed)
+				return -EIO;
+
+			/* Try to adjust ->nseeds and read the page again... */
+			conf->nseeds = cur_seed;
+
+			if (nand_reset_column())
+				return -EIO;
+
+			/* ... it still fails => it's a real corruption. */
+			if (nand_read_page(conf, offs, dest, conf->page_size))
+				return -EIO;
+		} else if (ret && conf->randomize) {
+			memset(dest, 0xff, conf->page_size);
+		}
+
+		page++;
+		offs += conf->page_size;
+		dest += conf->page_size;
+	}
+
+	return 0;
 }
 
 int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
 {
-	return nand_read_buffer(offs, size, dest);
+	static struct nfc_config conf = { };
+	int ret;
+
+	ret = nand_detect_config(&conf, offs, dest);
+	if (ret)
+		return ret;
+
+	return nand_read_buffer(&conf, offs, size, dest);
 }
 
 void nand_deselect(void)
-- 
2.7.4

[U-Boot] [linux-sunxi] [PATCH v2 7/7] spl: nand: sunxi: add support for NAND config auto-detection

[Siarhei Siamashka]

On Wed,  1 Jun 2016 13:23:24 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> NAND chips are supposed to expose their capabilities through advanced
> mechanisms like READID, ONFI or JEDEC parameter tables. While those
> methods are appropriate for the bootloader itself, it's way to
> complicated and takes too much space to fit in the SPL.
> 
> Replace those mechanisms by a dumb 'trial and error' mechanism.
> 
> With this new approach we can get rid of the fixed config list that was
> used in the sunxi NAND SPL driver.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>

We can also have these NAND parameters stored in the SPL header and
added there by a NAND image builder tool. This may save some precious
space in the SPL and also improve the reliability of detection.

Yes, this brings the necessity of the image builder tool into the
spotlight (something that converts the "u-boot-sunxi-with-spl.bin"
to a NAND image) but this has always been a problem. We have some
ongoing discussion about this in the linux-sunxi mailing list:
    https://groups.google.com/forum/#!topic/linux-sunxi/HsWRG-nuV-w

It also makes a lot of sense to have the NAND support functionality
enabled in the SPL for all sunxi boards by default, so the code size
does matter. We still do have the runtime decompression opportunity
as the strategic reserve [1], which should provide additional 4 or
5 KiB of space for the code. Still we need to be very careful about
using up this reserve, to ensure that it is well spent on something
useful (such as NAND support) instead of being just wasted by the
bloatware cultists :-)

[1] http://lists.denx.de/pipermail/u-boot/2015-September/226963.html

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [linux-sunxi] [PATCH v2 7/7] spl: nand: sunxi: add support for NAND config auto-detection

[Boris Brezillon]

On Wed, 1 Jun 2016 15:35:07 +0300
Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:

> On Wed,  1 Jun 2016 13:23:24 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > NAND chips are supposed to expose their capabilities through advanced
> > mechanisms like READID, ONFI or JEDEC parameter tables. While those
> > methods are appropriate for the bootloader itself, it's way to
> > complicated and takes too much space to fit in the SPL.
> > 
> > Replace those mechanisms by a dumb 'trial and error' mechanism.
> > 
> > With this new approach we can get rid of the fixed config list that was
> > used in the sunxi NAND SPL driver.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>  
> 
> We can also have these NAND parameters stored in the SPL header and
> added there by a NAND image builder tool. This may save some precious
> space in the SPL and also improve the reliability of detection.
> 
> Yes, this brings the necessity of the image builder tool into the
> spotlight (something that converts the "u-boot-sunxi-with-spl.bin"
> to a NAND image) but this has always been a problem. We have some
> ongoing discussion about this in the linux-sunxi mailing list:
>     https://groups.google.com/forum/#!topic/linux-sunxi/HsWRG-nuV-w
> 
> It also makes a lot of sense to have the NAND support functionality
> enabled in the SPL for all sunxi boards by default, so the code size
> does matter. We still do have the runtime decompression opportunity
> as the strategic reserve [1], which should provide additional 4 or
> 5 KiB of space for the code. Still we need to be very careful about
> using up this reserve, to ensure that it is well spent on something
> useful (such as NAND support) instead of being just wasted by the
> bloatware cultists :-)

Oh, come one! I just did the test, and we save 352 bytes when dropping
the auto-detection code. Do we really want to delay the NAND support
just because you want the perfect solution (which as I already said,
will not be trivial to implement).

I'm not telling that your approach is wrong, just that it's not a
priority right now, so let's move on and improve it iteratively.


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

[U-Boot] [linux-sunxi] [PATCH v2 7/7] spl: nand: sunxi: add support for NAND config auto-detection

[Maxime Ripard]

Hi,

On Wed, Jun 01, 2016 at 03:35:07PM +0300, Siarhei Siamashka wrote:
> On Wed,  1 Jun 2016 13:23:24 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > NAND chips are supposed to expose their capabilities through advanced
> > mechanisms like READID, ONFI or JEDEC parameter tables. While those
> > methods are appropriate for the bootloader itself, it's way to
> > complicated and takes too much space to fit in the SPL.
> > 
> > Replace those mechanisms by a dumb 'trial and error' mechanism.
> > 
> > With this new approach we can get rid of the fixed config list that was
> > used in the sunxi NAND SPL driver.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> We can also have these NAND parameters stored in the SPL header and
> added there by a NAND image builder tool. This may save some precious
> space in the SPL and also improve the reliability of detection.

So you want to favour a solution that hardcodes the NAND configuration
over a solution that runtime-detects what it needs?

I think we are aiming for two very different use-cases: we want to
have everything working ahead of time on various NAND chips during
production, you want to have the end-users being able to reflash
something after the production.

Boris' solution address both use-cases, yours address only the latter.

> Yes, this brings the necessity of the image builder tool into the
> spotlight (something that converts the "u-boot-sunxi-with-spl.bin"
> to a NAND image) but this has always been a problem.

I know you like it a lot, but I don't see how
u-boot-sunxi-with-spl.bin is relevant here. It's an image that has
always been built for the MMC.

We are talking about the NAND here, that will use a different image
format anyway to deal with the randomizer and the ECC, that will have
to use redundancy for the SPL, U-Boot and probably environment images,
that will have to be padded with random data, and doesn't want to
waste the useless (on NAND) padding that is used in that image.

> We have some
> ongoing discussion about this in the linux-sunxi mailing list:
>     https://groups.google.com/forum/#!topic/linux-sunxi/HsWRG-nuV-w
> 
> It also makes a lot of sense to have the NAND support functionality
> enabled in the SPL for all sunxi boards by default

On all the sunxi boards that have NAND at least.

> so the code size does matter. We still do have the runtime
> decompression opportunity as the strategic reserve [1], which should
> provide additional 4 or 5 KiB of space for the code. Still we need
> to be very careful about using up this reserve, to ensure that it is
> well spent on something useful (such as NAND support) instead of
> being just wasted by the bloatware cultists :-)

Good thing we are talking about NAND support and not some bloat then :)

Maxime

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