[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Tim Harvey]

in order to use env in the SPL (CONFIG_SPL_ENV_SUPPORT) nand_info,
mtd_block_isbad, and mtd_read must be available.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/mtd/nand/mxs_nand_spl.c | 112 ++++++++++++++++++++++------------------
 1 file changed, 61 insertions(+), 51 deletions(-)

diff --git a/drivers/mtd/nand/mxs_nand_spl.c b/drivers/mtd/nand/mxs_nand_spl.c
index 0e7c364..d9d1811 100644
--- a/drivers/mtd/nand/mxs_nand_spl.c
+++ b/drivers/mtd/nand/mxs_nand_spl.c
@@ -8,7 +8,7 @@
 #include <nand.h>
 #include <malloc.h>
 
-static nand_info_t mtd;
+nand_info_t nand_info[1];
 static struct nand_chip nand_chip;
 
 static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
@@ -123,14 +123,11 @@ static int mxs_read_page_ecc(struct mtd_info *mtd, void *buf, unsigned int page)
 	return 0;
 }
 
-static int is_badblock(struct mtd_info *mtd, loff_t offs, int allowbbt)
+int mtd_block_isbad(struct mtd_info *mtd, loff_t offs)
 {
 	register struct nand_chip *chip = mtd->priv;
-	unsigned int block = offs >> chip->phys_erase_shift;
 	unsigned int page = offs >> chip->page_shift;
 
-	debug("%s offs=0x%08x block:%d page:%d\n", __func__, (int)offs, block,
-	      page);
 	chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page);
 	memset(chip->oob_poi, 0, mtd->oobsize);
 	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
@@ -138,62 +135,34 @@ static int is_badblock(struct mtd_info *mtd, loff_t offs, int allowbbt)
 	return chip->oob_poi[0] != 0xff;
 }
 
-/* setup mtd and nand structs and init mxs_nand driver */
-static int mxs_nand_init(void)
-{
-	/* return if already initalized */
-	if (nand_chip.numchips)
-		return 0;
-
-	/* init mxs nand driver */
-	board_nand_init(&nand_chip);
-	mtd.priv = &nand_chip;
-	/* set mtd functions */
-	nand_chip.cmdfunc = mxs_nand_command;
-	nand_chip.numchips = 1;
-
-	/* identify flash device */
-	puts("NAND : ");
-	if (mxs_flash_ident(&mtd)) {
-		printf("Failed to identify\n");
-		return -1;
-	}
-
-	/* allocate and initialize buffers */
-	nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
-				     sizeof(*nand_chip.buffers));
-	nand_chip.oob_poi = nand_chip.buffers->databuf + mtd.writesize;
-	/* setup flash layout (does not scan as we override that) */
-	mtd.size = nand_chip.chipsize;
-	nand_chip.scan_bbt(&mtd);
-
-	printf("%llu MiB\n", (mtd.size / (1024 * 1024)));
-	return 0;
-}
-
-int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
+int mtd_read(struct mtd_info *mtd, loff_t offs, size_t size, size_t *retlen,
+	     uchar *buf)
 {
 	struct nand_chip *chip;
 	unsigned int page;
 	unsigned int nand_page_per_block;
 	unsigned int sz = 0;
+	nand_info_t *info = &nand_info[0];
 
-	if (mxs_nand_init())
-		return -ENODEV;
-	chip = mtd.priv;
+	chip = info->priv;
 	page = offs >> chip->page_shift;
-	nand_page_per_block = mtd.erasesize / mtd.writesize;
+	nand_page_per_block = info->erasesize / info->writesize;
 
-	debug("%s offset:0x%08x len:%d page:%d\n", __func__, offs, size, page);
+	debug("%s offs:0x%08x len:0x%x page:%d\n", __func__, (int)offs, size,
+	      page);
 
-	size = roundup(size, mtd.writesize);
+	size = roundup(size, info->writesize);
+	if (retlen)
+		*retlen = 0;
 	while (sz < size) {
-		if (mxs_read_page_ecc(&mtd, buf, page) < 0)
+		if (mxs_read_page_ecc(info, buf, page) < 0)
 			return -1;
-		sz += mtd.writesize;
-		offs += mtd.writesize;
+		sz += info->writesize;
+		offs += info->writesize;
 		page++;
-		buf += mtd.writesize;
+		buf += info->writesize;
+		if (retlen)
+			*retlen += info->writesize;
 
 		/*
 		 * Check if we have crossed a block boundary, and if so
@@ -204,10 +173,10 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
 			 * Yes, new block. See if this block is good. If not,
 			 * loop until we find a good block.
 			 */
-			while (is_badblock(&mtd, offs, 1)) {
+			while (mtd_block_isbad(info, offs)) {
 				page = page + nand_page_per_block;
 				/* Check i we've reached the end of flash. */
-				if (page >= mtd.size >> chip->page_shift)
+				if (page >= info->size >> chip->page_shift)
 					return -ENOMEM;
 			}
 		}
@@ -216,6 +185,46 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
 	return 0;
 }
 
+/* setup mtd and nand structs and init mxs_nand driver */
+static int mxs_nand_init(void)
+{
+	nand_info_t *info = &nand_info[0];
+
+	/* return if already initalized */
+	if (nand_chip.numchips)
+		return 0;
+
+	/* init mxs nand driver */
+	board_nand_init(&nand_chip);
+	info->priv = &nand_chip;
+	/* set mtd functions */
+	nand_chip.cmdfunc = mxs_nand_command;
+	nand_chip.numchips = 1;
+
+	/* identify flash device */
+	puts("NAND : ");
+	if (mxs_flash_ident(info)) {
+		printf("Failed to identify\n");
+		return -1;
+	}
+
+	/* allocate and initialize buffers */
+	nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
+				     sizeof(*nand_chip.buffers));
+	nand_chip.oob_poi = nand_chip.buffers->databuf + info->writesize;
+	/* setup flash layout (does not scan as we override that) */
+	info->size = nand_chip.chipsize;
+	nand_chip.scan_bbt(info);
+
+	printf("%llu MiB\n", (info->size / (1024 * 1024)));
+	return 0;
+}
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
+{
+	return mtd_read(NULL, offs, size, NULL, buf);
+}
+
 int nand_default_bbt(struct mtd_info *mtd)
 {
 	return 0;
@@ -223,6 +232,7 @@ int nand_default_bbt(struct mtd_info *mtd)
 
 void nand_init(void)
 {
+	mxs_nand_init();
 }
 
 void nand_deselect(void)
-- 
1.9.1

[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Tim Harvey]

On Fri, May 8, 2015 at 2:39 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> in order to use env in the SPL (CONFIG_SPL_ENV_SUPPORT) nand_info,
> mtd_block_isbad, and mtd_read must be available.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/mtd/nand/mxs_nand_spl.c | 112 ++++++++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/mtd/nand/mxs_nand_spl.c b/drivers/mtd/nand/mxs_nand_spl.c
> index 0e7c364..d9d1811 100644
> --- a/drivers/mtd/nand/mxs_nand_spl.c
> +++ b/drivers/mtd/nand/mxs_nand_spl.c
> @@ -8,7 +8,7 @@
>  #include <nand.h>
>  #include <malloc.h>
>
> -static nand_info_t mtd;
> +nand_info_t nand_info[1];
>  static struct nand_chip nand_chip;
>
>  static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
> @@ -123,14 +123,11 @@ static int mxs_read_page_ecc(struct mtd_info *mtd, void *buf, unsigned int page)
>         return 0;
>  }
>
> -static int is_badblock(struct mtd_info *mtd, loff_t offs, int allowbbt)
> +int mtd_block_isbad(struct mtd_info *mtd, loff_t offs)
>  {
>         register struct nand_chip *chip = mtd->priv;
> -       unsigned int block = offs >> chip->phys_erase_shift;
>         unsigned int page = offs >> chip->page_shift;
>
> -       debug("%s offs=0x%08x block:%d page:%d\n", __func__, (int)offs, block,
> -             page);
>         chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page);
>         memset(chip->oob_poi, 0, mtd->oobsize);
>         chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> @@ -138,62 +135,34 @@ static int is_badblock(struct mtd_info *mtd, loff_t offs, int allowbbt)
>         return chip->oob_poi[0] != 0xff;
>  }
>
> -/* setup mtd and nand structs and init mxs_nand driver */
> -static int mxs_nand_init(void)
> -{
> -       /* return if already initalized */
> -       if (nand_chip.numchips)
> -               return 0;
> -
> -       /* init mxs nand driver */
> -       board_nand_init(&nand_chip);
> -       mtd.priv = &nand_chip;
> -       /* set mtd functions */
> -       nand_chip.cmdfunc = mxs_nand_command;
> -       nand_chip.numchips = 1;
> -
> -       /* identify flash device */
> -       puts("NAND : ");
> -       if (mxs_flash_ident(&mtd)) {
> -               printf("Failed to identify\n");
> -               return -1;
> -       }
> -
> -       /* allocate and initialize buffers */
> -       nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
> -                                    sizeof(*nand_chip.buffers));
> -       nand_chip.oob_poi = nand_chip.buffers->databuf + mtd.writesize;
> -       /* setup flash layout (does not scan as we override that) */
> -       mtd.size = nand_chip.chipsize;
> -       nand_chip.scan_bbt(&mtd);
> -
> -       printf("%llu MiB\n", (mtd.size / (1024 * 1024)));
> -       return 0;
> -}
> -
> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +int mtd_read(struct mtd_info *mtd, loff_t offs, size_t size, size_t *retlen,
> +            uchar *buf)
>  {
>         struct nand_chip *chip;
>         unsigned int page;
>         unsigned int nand_page_per_block;
>         unsigned int sz = 0;
> +       nand_info_t *info = &nand_info[0];
>
> -       if (mxs_nand_init())
> -               return -ENODEV;
> -       chip = mtd.priv;
> +       chip = info->priv;
>         page = offs >> chip->page_shift;
> -       nand_page_per_block = mtd.erasesize / mtd.writesize;
> +       nand_page_per_block = info->erasesize / info->writesize;
>
> -       debug("%s offset:0x%08x len:%d page:%d\n", __func__, offs, size, page);
> +       debug("%s offs:0x%08x len:0x%x page:%d\n", __func__, (int)offs, size,
> +             page);
>
> -       size = roundup(size, mtd.writesize);
> +       size = roundup(size, info->writesize);
> +       if (retlen)
> +               *retlen = 0;
>         while (sz < size) {
> -               if (mxs_read_page_ecc(&mtd, buf, page) < 0)
> +               if (mxs_read_page_ecc(info, buf, page) < 0)
>                         return -1;
> -               sz += mtd.writesize;
> -               offs += mtd.writesize;
> +               sz += info->writesize;
> +               offs += info->writesize;
>                 page++;
> -               buf += mtd.writesize;
> +               buf += info->writesize;
> +               if (retlen)
> +                       *retlen += info->writesize;
>
>                 /*
>                  * Check if we have crossed a block boundary, and if so
> @@ -204,10 +173,10 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>                          * Yes, new block. See if this block is good. If not,
>                          * loop until we find a good block.
>                          */
> -                       while (is_badblock(&mtd, offs, 1)) {
> +                       while (mtd_block_isbad(info, offs)) {
>                                 page = page + nand_page_per_block;
>                                 /* Check i we've reached the end of flash. */
> -                               if (page >= mtd.size >> chip->page_shift)
> +                               if (page >= info->size >> chip->page_shift)
>                                         return -ENOMEM;
>                         }
>                 }
> @@ -216,6 +185,46 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>         return 0;
>  }
>
> +/* setup mtd and nand structs and init mxs_nand driver */
> +static int mxs_nand_init(void)
> +{
> +       nand_info_t *info = &nand_info[0];
> +
> +       /* return if already initalized */
> +       if (nand_chip.numchips)
> +               return 0;
> +
> +       /* init mxs nand driver */
> +       board_nand_init(&nand_chip);
> +       info->priv = &nand_chip;
> +       /* set mtd functions */
> +       nand_chip.cmdfunc = mxs_nand_command;
> +       nand_chip.numchips = 1;
> +
> +       /* identify flash device */
> +       puts("NAND : ");
> +       if (mxs_flash_ident(info)) {
> +               printf("Failed to identify\n");
> +               return -1;
> +       }
> +
> +       /* allocate and initialize buffers */
> +       nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
> +                                    sizeof(*nand_chip.buffers));
> +       nand_chip.oob_poi = nand_chip.buffers->databuf + info->writesize;
> +       /* setup flash layout (does not scan as we override that) */
> +       info->size = nand_chip.chipsize;
> +       nand_chip.scan_bbt(info);
> +
> +       printf("%llu MiB\n", (info->size / (1024 * 1024)));
> +       return 0;
> +}
> +
> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +{
> +       return mtd_read(NULL, offs, size, NULL, buf);
> +}
> +
>  int nand_default_bbt(struct mtd_info *mtd)
>  {
>         return 0;
> @@ -223,6 +232,7 @@ int nand_default_bbt(struct mtd_info *mtd)
>
>  void nand_init(void)
>  {
> +       mxs_nand_init();
>  }
>
>  void nand_deselect(void)
> --
> 1.9.1
>

Stefano,

It appears that gw_ventana and cm_fx6 are the only users of this
(CONFIG_MX6 && CONFIG_NAND_MXS && building with SPL) so I'm adding
Nikita to Cc for him to test/comment.

Cc: Nikita Kiryanov <nikita@compulab.co.il>

Tim

[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Scott Wood]

On Fri, 2015-05-08 at 14:39 -0700, Tim Harvey wrote:
> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +int mtd_read(struct mtd_info *mtd, loff_t offs, size_t size, size_t *retlen,
> +	     uchar *buf)
>  {
>  	struct nand_chip *chip;
>  	unsigned int page;
>  	unsigned int nand_page_per_block;
>  	unsigned int sz = 0;
> +	nand_info_t *info = &nand_info[0];

Any reason not to use the passed-in mtd pointer (and fix the
nand_spl_load_image wrapper to pass in &nand_info[0])?

> +/* setup mtd and nand structs and init mxs_nand driver */
> +static int mxs_nand_init(void)
> +{
> +	nand_info_t *info = &nand_info[0];
> +
> +	/* return if already initalized */
> +	if (nand_chip.numchips)
> +		return 0;
> +
> +	/* init mxs nand driver */
> +	board_nand_init(&nand_chip);
> +	info->priv = &nand_chip;
> +	/* set mtd functions */
> +	nand_chip.cmdfunc = mxs_nand_command;
> +	nand_chip.numchips = 1;
> +
> +	/* identify flash device */
> +	puts("NAND : ");
> +	if (mxs_flash_ident(info)) {
> +		printf("Failed to identify\n");
> +		return -1;
> +	}
> +
> +	/* allocate and initialize buffers */
> +	nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
> +				     sizeof(*nand_chip.buffers));
> +	nand_chip.oob_poi = nand_chip.buffers->databuf + info->writesize;
> +	/* setup flash layout (does not scan as we override that) */
> +	info->size = nand_chip.chipsize;
> +	nand_chip.scan_bbt(info);
> +
> +	printf("%llu MiB\n", (info->size / (1024 * 1024)));
> +	return 0;
> +}

Why did this function need to be moved?

> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +{
> +	return mtd_read(NULL, offs, size, NULL, buf);
> +}

It'd be nice to keep the wrapper near the function it wraps.

I don't see any other such wrappers; is there no other driver that
currently works with SPL env?

>  int nand_default_bbt(struct mtd_info *mtd)
>  {
>  	return 0;
> @@ -223,6 +232,7 @@ int nand_default_bbt(struct mtd_info *mtd)
>  
>  void nand_init(void)
>  {
> +	mxs_nand_init();
>  }

Do you still need the "return if already initialized" check with this
change?  How is this change related to the rest?

-Scott

[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Tim Harvey]

On Wed, May 13, 2015 at 4:38 PM, Scott Wood <scottwood@freescale.com> wrote:
> On Fri, 2015-05-08 at 14:39 -0700, Tim Harvey wrote:
>> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>> +int mtd_read(struct mtd_info *mtd, loff_t offs, size_t size, size_t *retlen,
>> +          uchar *buf)
>>  {
>>       struct nand_chip *chip;
>>       unsigned int page;
>>       unsigned int nand_page_per_block;
>>       unsigned int sz = 0;
>> +     nand_info_t *info = &nand_info[0];
>
> Any reason not to use the passed-in mtd pointer (and fix the
> nand_spl_load_image wrapper to pass in &nand_info[0])?

Hi Scott,

No reason - I think I just got confused because I didn't realize
nand_info_t was a typedef for struct mtd_info.

>
>> +/* setup mtd and nand structs and init mxs_nand driver */
>> +static int mxs_nand_init(void)
>> +{
>> +     nand_info_t *info = &nand_info[0];
>> +
>> +     /* return if already initalized */
>> +     if (nand_chip.numchips)
>> +             return 0;
>> +
>> +     /* init mxs nand driver */
>> +     board_nand_init(&nand_chip);
>> +     info->priv = &nand_chip;
>> +     /* set mtd functions */
>> +     nand_chip.cmdfunc = mxs_nand_command;
>> +     nand_chip.numchips = 1;
>> +
>> +     /* identify flash device */
>> +     puts("NAND : ");
>> +     if (mxs_flash_ident(info)) {
>> +             printf("Failed to identify\n");
>> +             return -1;
>> +     }
>> +
>> +     /* allocate and initialize buffers */
>> +     nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
>> +                                  sizeof(*nand_chip.buffers));
>> +     nand_chip.oob_poi = nand_chip.buffers->databuf + info->writesize;
>> +     /* setup flash layout (does not scan as we override that) */
>> +     info->size = nand_chip.chipsize;
>> +     nand_chip.scan_bbt(info);
>> +
>> +     printf("%llu MiB\n", (info->size / (1024 * 1024)));
>> +     return 0;
>> +}
>
> Why did this function need to be moved?
>
>> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>> +{
>> +     return mtd_read(NULL, offs, size, NULL, buf);
>> +}
>
> It'd be nice to keep the wrapper near the function it wraps.

It doesn't need to be moved and moving the location of the wrapper as
well makes the patch much easier to read/review.

>
> I don't see any other such wrappers; is there no other driver that
> currently works with SPL env?

I don't know - this got me digging into why I needed mtd_read()
implemented all of the sudden once I enabled CONFIG_SPL_ENV_SUPPORT.

It turns out that this is needed because env_nand.c:readenv() calls
nand_read_skip_bad, which uses the mtd layer. For SPL the
functionality of readenv() is pretty much already in
nand_spl_load_image() so I find if do the following (instead of making
any changes to mxs_nand_spl.c) which allows the mtd layers to not be
used I save 4KB in the SPL:

diff --git a/common/env_nand.c b/common/env_nand.c
index cc7e979..97d10a2 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -260,6 +260,9 @@ int saveenv(void)
 }
 #endif /* CMD_SAVEENV */

+#if defined(CONFIG_SPL_BUILD)
+#define readenv(offset, buf) nand_spl_load_image(offset, CONFIG_ENV_SIZE, buf)
+#else
 static int readenv(size_t offset, u_char *buf)
 {
        size_t end = offset + CONFIG_ENV_RANGE;
@@ -295,6 +298,7 @@ static int readenv(size_t offset, u_char *buf)

        return 0;
 }
+#endif /* #if defined(CONFIG_SPL_BUILD) */

 #ifdef CONFIG_ENV_OFFSET_OOB
 int get_nand_env_oob(nand_info_t *nand, unsigned long *result)

I don't think I'm loosing any necessary functionality in the SPL by
doing the above and it saves me 4KB in the SPL which is precious
space.

>
>>  int nand_default_bbt(struct mtd_info *mtd)
>>  {
>>       return 0;
>> @@ -223,6 +232,7 @@ int nand_default_bbt(struct mtd_info *mtd)
>>
>>  void nand_init(void)
>>  {
>> +     mxs_nand_init();
>>  }
>
> Do you still need the "return if already initialized" check with this
> change?  How is this change related to the rest?

The check can now be eliminated as I'm moving the init to where it
likely should have been all along instead of in nand_spl_load_image().
In fact, what should be done is to simply rename the static int
mxs_nand_init() to nand_init() instead of calling the static function.

So while I could revise the above patch based on some of the things
you pointed out here it wouldn't be necessary (although I could do it
as a cleanup) with the above patch instead of env_nand.c.

What do you think?

Tim

[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Scott Wood]

On Thu, 2015-05-14 at 08:12 -0700, Tim Harvey wrote:
> It turns out that this is needed because env_nand.c:readenv() calls
> nand_read_skip_bad, which uses the mtd layer. For SPL the
> functionality of readenv() is pretty much already in
> nand_spl_load_image() so I find if do the following (instead of making
> any changes to mxs_nand_spl.c) which allows the mtd layers to not be
> used I save 4KB in the SPL:
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index cc7e979..97d10a2 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -260,6 +260,9 @@ int saveenv(void)
>  }
>  #endif /* CMD_SAVEENV */
> 
> +#if defined(CONFIG_SPL_BUILD)
> +#define readenv(offset, buf) nand_spl_load_image(offset, CONFIG_ENV_SIZE, buf)
> +#else
>  static int readenv(size_t offset, u_char *buf)
>  {
>         size_t end = offset + CONFIG_ENV_RANGE;
> @@ -295,6 +298,7 @@ static int readenv(size_t offset, u_char *buf)
> 
>         return 0;
>  }
> +#endif /* #if defined(CONFIG_SPL_BUILD) */
> 
>  #ifdef CONFIG_ENV_OFFSET_OOB
>  int get_nand_env_oob(nand_info_t *nand, unsigned long *result)
> 
> I don't think I'm loosing any necessary functionality in the SPL by
> doing the above and it saves me 4KB in the SPL which is precious
> space.

I'd prefer a function over a macro, but OK.

readenv() itself looks like it could be greatly simplified by dropping
the loop and bad block check, since nand_read_skip_bad() handles that.

-Scott