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

2016-06-01 Thread 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  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 
> > Acked-by: Hans de Goede 
> 
> 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

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

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

2016-06-01 Thread Boris Brezillon 
On Wed, 1 Jun 2016 15:35:07 +0300
Siarhei Siamashka  wrote:

> On Wed,  1 Jun 2016 13:23:24 +0200
> Boris Brezillon  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 
> > Acked-by: Hans de Goede   
> 
> 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

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

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

2016-06-01 Thread Siarhei Siamashka 
On Wed,  1 Jun 2016 13:23:24 +0200
Boris Brezillon  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 
> Acked-by: Hans de Goede 

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

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

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

2016-06-01 Thread 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 
Acked-by: Hans de Goede 
---
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 & 0x1) ? 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();
+   return i - 1;
+}
 
-   for ( ;dest < end; dest += ecc_page_size, offs += page_size) {
-   if (nand_load_page(, 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(, 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