[PATCH V2 0/4] MXS nand fixes in SPL

[PATCH V2 0/4] MXS nand fixes in SPL

[Michael Trimarchi]

Those patches come after some testing of failing in factory on some
unit. We found out that the bootrom imx loader was not able to handling
badblock. This can be a limit of the implementation right now in imx8mn.
Anyway not all the imx platform has the support of this loader. I found
some problems on the implementation so I have fixed it up according the
experience of Sitara. I tested only using a Fit Image as a flash
container. This version add in the series the fix of cmd_nandbcb and
the fix of spl_nand load. I can imagine that a lot of boards and users
are affected. I have started to backport this changes in some older
uboot and adapt it

Michael Trimarchi (4):
  nand: raw: mxs_nand: Fix specific hook registration
  mtd: nand: mxs_nand_spl: Fix bad block skipping
  arm: mach-imx: cmd_nandbcb fix bad block handling
  spl: spl_nand: Fix bad block handling in fitImage

 arch/arm/mach-imx/cmd_nandbcb.c     | 21 +++----
 common/spl/spl_nand.c               |  5 +-
 drivers/mtd/nand/raw/mxs_nand.c     | 32 +++++-----
 drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
 4 files changed, 73 insertions(+), 75 deletions(-)

-- 
2.25.1

[PATCH V2 1/4] nand: raw: mxs_nand: Fix specific hook registration

[Michael Trimarchi]

Move the hook after nand_scan_tail is called. The hook must be replaced
to the mxs specific one but those must to be assignment later in the
probe function.

With this fix markbad is working again. Before this change:

nand markbad 0xDEC00
NXS NAND: Writing OOB isn't supported
NXS NAND: Writing OOB isn't supported
block 0x000dec00 NOT marked as bad! ERROR 0

Cc: Han Xu <han.xu@nxp.com>
Cc: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
V1->V2:
	- Adjust the commit message
	- Add Cc Han Xu and Fabio
---
 drivers/mtd/nand/raw/mxs_nand.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
index ee5d7fde9c..53f24b9c4b 100644
--- a/drivers/mtd/nand/raw/mxs_nand.c
+++ b/drivers/mtd/nand/raw/mxs_nand.c
@@ -1246,22 +1246,6 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
 	/* Enable BCH complete interrupt */
 	writel(BCH_CTRL_COMPLETE_IRQ_EN, &bch_regs->hw_bch_ctrl_set);
 
-	/* Hook some operations at the MTD level. */
-	if (mtd->_read_oob != mxs_nand_hook_read_oob) {
-		nand_info->hooked_read_oob = mtd->_read_oob;
-		mtd->_read_oob = mxs_nand_hook_read_oob;
-	}
-
-	if (mtd->_write_oob != mxs_nand_hook_write_oob) {
-		nand_info->hooked_write_oob = mtd->_write_oob;
-		mtd->_write_oob = mxs_nand_hook_write_oob;
-	}
-
-	if (mtd->_block_markbad != mxs_nand_hook_block_markbad) {
-		nand_info->hooked_block_markbad = mtd->_block_markbad;
-		mtd->_block_markbad = mxs_nand_hook_block_markbad;
-	}
-
 	return 0;
 }
 
@@ -1467,6 +1451,22 @@ int mxs_nand_init_ctrl(struct mxs_nand_info *nand_info)
 	if (err)
 		goto err_free_buffers;
 
+	/* Hook some operations at the MTD level. */
+	if (mtd->_read_oob != mxs_nand_hook_read_oob) {
+		nand_info->hooked_read_oob = mtd->_read_oob;
+		mtd->_read_oob = mxs_nand_hook_read_oob;
+	}
+
+	if (mtd->_write_oob != mxs_nand_hook_write_oob) {
+		nand_info->hooked_write_oob = mtd->_write_oob;
+		mtd->_write_oob = mxs_nand_hook_write_oob;
+	}
+
+	if (mtd->_block_markbad != mxs_nand_hook_block_markbad) {
+		nand_info->hooked_block_markbad = mtd->_block_markbad;
+		mtd->_block_markbad = mxs_nand_hook_block_markbad;
+	}
+
 	err = nand_register(0, mtd);
 	if (err)
 		goto err_free_buffers;
-- 
2.25.1

Re: [PATCH V2 1/4] nand: raw: mxs_nand: Fix specific hook registration

[Han Xu]

On 22/04/27 07:50AM, Michael Trimarchi wrote:
> Move the hook after nand_scan_tail is called. The hook must be replaced
> to the mxs specific one but those must to be assignment later in the
> probe function.
> 
> With this fix markbad is working again. Before this change:
> 
> nand markbad 0xDEC00
> NXS NAND: Writing OOB isn't supported
> NXS NAND: Writing OOB isn't supported
> block 0x000dec00 NOT marked as bad! ERROR 0
> 
> Cc: Han Xu <han.xu@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Acked-by: Han Xu <han.xu@nxp.com>

> ---
> V1->V2:
> 	- Adjust the commit message
> 	- Add Cc Han Xu and Fabio
> ---
>  drivers/mtd/nand/raw/mxs_nand.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
> index ee5d7fde9c..53f24b9c4b 100644
> --- a/drivers/mtd/nand/raw/mxs_nand.c
> +++ b/drivers/mtd/nand/raw/mxs_nand.c
> @@ -1246,22 +1246,6 @@ int mxs_nand_setup_ecc(struct mtd_info *mtd)
>  	/* Enable BCH complete interrupt */
>  	writel(BCH_CTRL_COMPLETE_IRQ_EN, &bch_regs->hw_bch_ctrl_set);
>  
> -	/* Hook some operations at the MTD level. */
> -	if (mtd->_read_oob != mxs_nand_hook_read_oob) {
> -		nand_info->hooked_read_oob = mtd->_read_oob;
> -		mtd->_read_oob = mxs_nand_hook_read_oob;
> -	}
> -
> -	if (mtd->_write_oob != mxs_nand_hook_write_oob) {
> -		nand_info->hooked_write_oob = mtd->_write_oob;
> -		mtd->_write_oob = mxs_nand_hook_write_oob;
> -	}
> -
> -	if (mtd->_block_markbad != mxs_nand_hook_block_markbad) {
> -		nand_info->hooked_block_markbad = mtd->_block_markbad;
> -		mtd->_block_markbad = mxs_nand_hook_block_markbad;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1467,6 +1451,22 @@ int mxs_nand_init_ctrl(struct mxs_nand_info *nand_info)
>  	if (err)
>  		goto err_free_buffers;
>  
> +	/* Hook some operations at the MTD level. */
> +	if (mtd->_read_oob != mxs_nand_hook_read_oob) {
> +		nand_info->hooked_read_oob = mtd->_read_oob;
> +		mtd->_read_oob = mxs_nand_hook_read_oob;
> +	}
> +
> +	if (mtd->_write_oob != mxs_nand_hook_write_oob) {
> +		nand_info->hooked_write_oob = mtd->_write_oob;
> +		mtd->_write_oob = mxs_nand_hook_write_oob;
> +	}
> +
> +	if (mtd->_block_markbad != mxs_nand_hook_block_markbad) {
> +		nand_info->hooked_block_markbad = mtd->_block_markbad;
> +		mtd->_block_markbad = mxs_nand_hook_block_markbad;
> +	}
> +
>  	err = nand_register(0, mtd);
>  	if (err)
>  		goto err_free_buffers;
> -- 
> 2.25.1
> 

[PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Trimarchi]

The specific implementation was having bug. Those bugs are since
the beginning of the implementation. Some manufactures can receive
this bug in their SPL code. This bug start to be more visible on
architecture that has complicated boot process like imx8mn. Older
version of uboot has the same problem only if the bad block
appear in correspoding of befine of u-boot image. In order to
adjust the function we scan from the first block. The logic
is not changed to have a simple way to fix without get regression.

The problematic part of old code was in this part:

while (is_badblock(mtd, offs, 1)) {
           page = page + nand_page_per_block;
          /* Check i we've reached the end of flash. */
          if (page >= mtd->size >> chip->page_shift) {
                      free(page_buf);
                      return -ENOMEM;
         }
}

Even we fix it adding increment of the offset of one erase block size
we don't fix the problem, because the first erase block where the
image start is not checked. The code was tested on an imx8mn where
the boot rom api was not able to skip it. Apart of that other
architecure are using this code and all boards that has nand as boot
device can be affected

Cc: Han Xu <han.xu@nxp.com>
Cc: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
V1->V2:
	- Adjust the commit message
	- Add Cc Han Xu and Fabio
	- fix size >= 0 to > 0
---
 drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c b/drivers/mtd/nand/raw/mxs_nand_spl.c
index 59a67ee414..2bfb181007 100644
--- a/drivers/mtd/nand/raw/mxs_nand_spl.c
+++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
@@ -218,14 +218,14 @@ void nand_init(void)
 	mxs_nand_setup_ecc(mtd);
 }
 
-int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
 {
-	struct nand_chip *chip;
-	unsigned int page;
+	unsigned int sz;
+	unsigned int block, lastblock;
+	unsigned int page, page_offset;
 	unsigned int nand_page_per_block;
-	unsigned int sz = 0;
+	struct nand_chip *chip;
 	u8 *page_buf = NULL;
-	u32 page_off;
 
 	chip = mtd_to_nand(mtd);
 	if (!chip->numchips)
@@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
 	if (!page_buf)
 		return -ENOMEM;
 
-	page = offs >> chip->page_shift;
-	page_off = offs & (mtd->writesize - 1);
+	/* offs has to be aligned to a page address! */
+	block = offs / mtd->erasesize;
+	lastblock = (offs + size - 1) / mtd->erasesize;
+	page = (offs % mtd->erasesize) / mtd->writesize;
+	page_offset = offs % mtd->writesize;
 	nand_page_per_block = mtd->erasesize / mtd->writesize;
 
-	debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
-
-	while (size) {
-		if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
-			return -1;
-
-		if (size > (mtd->writesize - page_off))
-			sz = (mtd->writesize - page_off);
-		else
-			sz = size;
-
-		memcpy(buf, page_buf + page_off, sz);
-
-		offs += mtd->writesize;
-		page++;
-		buf += (mtd->writesize - page_off);
-		page_off = 0;
-		size -= sz;
-
-		/*
-		 * Check if we have crossed a block boundary, and if so
-		 * check for bad block.
-		 */
-		if (!(page % nand_page_per_block)) {
-			/*
-			 * Yes, new block. See if this block is good. If not,
-			 * loop until we find a good block.
-			 */
-			while (is_badblock(mtd, offs, 1)) {
-				page = page + nand_page_per_block;
-				/* Check i we've reached the end of flash. */
-				if (page >= mtd->size >> chip->page_shift) {
+	while (block <= lastblock && size > 0) {
+		if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
+			/* Skip bad blocks */
+			while (page < nand_page_per_block) {
+				int curr_page = nand_page_per_block * block + page;
+
+				if (mxs_read_page_ecc(mtd, page_buf, curr_page) < 0) {
 					free(page_buf);
-					return -ENOMEM;
+					return -EIO;
 				}
+
+				if (size > (mtd->writesize - page_offset))
+					sz = (mtd->writesize - page_offset);
+				else
+					sz = size;
+
+				memcpy(dst, page_buf + page_offset, sz);
+				dst += sz;
+				size -= sz;
+				page_offset = 0;
+				page++;
 			}
+
+			page = 0;
+		} else {
+			lastblock++;
 		}
+
+		block++;
 	}
 
 	free(page_buf);
@@ -294,6 +289,19 @@ void nand_deselect(void)
 
 u32 nand_spl_adjust_offset(u32 sector, u32 offs)
 {
-	/* Handle the offset adjust in nand_spl_load_image,*/
+	unsigned int block, lastblock;
+
+	block = sector / mtd->erasesize;
+	lastblock = (sector + offs) / mtd->erasesize;
+
+	while (block <= lastblock) {
+		if (is_badblock(mtd, block * mtd->erasesize, 1)) {
+			offs += mtd->erasesize;
+			lastblock++;
+		}
+
+		block++;
+	}
+
 	return offs;
 }
-- 
2.25.1

RE: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Han Xu]

> -----Original Message-----
> From: Michael Trimarchi <michael@amarulasolutions.com>
> Sent: Wednesday, April 27, 2022 12:50 AM
> To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> <jagan@amarulasolutions.com>; Ariel D'Alessandro
> <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> 
> The specific implementation was having bug. Those bugs are since the beginning
> of the implementation. Some manufactures can receive this bug in their SPL code.
> This bug start to be more visible on architecture that has complicated boot
> process like imx8mn. Older version of uboot has the same problem only if the
> bad block appear in correspoding of befine of u-boot image. In order to adjust
> the function we scan from the first block. The logic is not changed to have a
> simple way to fix without get regression.
> 
> The problematic part of old code was in this part:
> 
> while (is_badblock(mtd, offs, 1)) {
>            page = page + nand_page_per_block;
>           /* Check i we've reached the end of flash. */
>           if (page >= mtd->size >> chip->page_shift) {
>                       free(page_buf);
>                       return -ENOMEM;
>          }
> }
> 
> Even we fix it adding increment of the offset of one erase block size we don't fix
> the problem, because the first erase block where the image start is not checked.

Could you please describe more details about your test? Thanks.

> The code was tested on an imx8mn where the boot rom api was not able to skip
> it. Apart of that other architecure are using this code and all boards that has
> nand as boot device can be affected
> 
> Cc: Han Xu <han.xu@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
> V1->V2:
> 	- Adjust the commit message
> 	- Add Cc Han Xu and Fabio
> 	- fix size >= 0 to > 0
> ---
>  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> b/drivers/mtd/nand/raw/mxs_nand_spl.c
> index 59a67ee414..2bfb181007 100644
> --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> @@ -218,14 +218,14 @@ void nand_init(void)
>  	mxs_nand_setup_ecc(mtd);
>  }
> 
> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
>  {
> -	struct nand_chip *chip;
> -	unsigned int page;
> +	unsigned int sz;
> +	unsigned int block, lastblock;
> +	unsigned int page, page_offset;
>  	unsigned int nand_page_per_block;
> -	unsigned int sz = 0;
> +	struct nand_chip *chip;
>  	u8 *page_buf = NULL;
> -	u32 page_off;
> 
>  	chip = mtd_to_nand(mtd);
>  	if (!chip->numchips)
> @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> size, void *buf)
>  	if (!page_buf)
>  		return -ENOMEM;
> 
> -	page = offs >> chip->page_shift;
> -	page_off = offs & (mtd->writesize - 1);
> +	/* offs has to be aligned to a page address! */
> +	block = offs / mtd->erasesize;
> +	lastblock = (offs + size - 1) / mtd->erasesize;
> +	page = (offs % mtd->erasesize) / mtd->writesize;
> +	page_offset = offs % mtd->writesize;
>  	nand_page_per_block = mtd->erasesize / mtd->writesize;
> 
> -	debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> -
> -	while (size) {
> -		if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> -			return -1;
> -
> -		if (size > (mtd->writesize - page_off))
> -			sz = (mtd->writesize - page_off);
> -		else
> -			sz = size;
> -
> -		memcpy(buf, page_buf + page_off, sz);
> -
> -		offs += mtd->writesize;
> -		page++;
> -		buf += (mtd->writesize - page_off);
> -		page_off = 0;
> -		size -= sz;
> -
> -		/*
> -		 * Check if we have crossed a block boundary, and if so
> -		 * check for bad block.
> -		 */
> -		if (!(page % nand_page_per_block)) {
> -			/*
> -			 * Yes, new block. See if this block is good. If not,
> -			 * loop until we find a good block.
> -			 */
> -			while (is_badblock(mtd, offs, 1)) {
> -				page = page + nand_page_per_block;
> -				/* Check i we've reached the end of flash. */
> -				if (page >= mtd->size >> chip->page_shift) {
> +	while (block <= lastblock && size > 0) {
> +		if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> +			/* Skip bad blocks */
> +			while (page < nand_page_per_block) {
> +				int curr_page = nand_page_per_block * block +
> page;
> +
> +				if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> < 0) {
>  					free(page_buf);
> -					return -ENOMEM;
> +					return -EIO;
>  				}
> +
> +				if (size > (mtd->writesize - page_offset))
> +					sz = (mtd->writesize - page_offset);
> +				else
> +					sz = size;
> +
> +				memcpy(dst, page_buf + page_offset, sz);
> +				dst += sz;
> +				size -= sz;
> +				page_offset = 0;
> +				page++;
>  			}
> +
> +			page = 0;
> +		} else {
> +			lastblock++;
>  		}
> +
> +		block++;
>  	}
> 
>  	free(page_buf);
> @@ -294,6 +289,19 @@ void nand_deselect(void)
> 
>  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> -	/* Handle the offset adjust in nand_spl_load_image,*/
> +	unsigned int block, lastblock;
> +
> +	block = sector / mtd->erasesize;
> +	lastblock = (sector + offs) / mtd->erasesize;
> +
> +	while (block <= lastblock) {
> +		if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> +			offs += mtd->erasesize;
> +			lastblock++;
> +		}
> +
> +		block++;
> +	}
> +
>  	return offs;
>  }
> --
> 2.25.1

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Nazzareno Trimarchi]

Hi

On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han.xu@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Michael Trimarchi <michael@amarulasolutions.com>
> > Sent: Wednesday, April 27, 2022 12:50 AM
> > To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> > <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> > Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> > <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> > <jagan@amarulasolutions.com>; Ariel D'Alessandro
> > <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> > Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> >
> > The specific implementation was having bug. Those bugs are since the beginning
> > of the implementation. Some manufactures can receive this bug in their SPL code.
> > This bug start to be more visible on architecture that has complicated boot
> > process like imx8mn. Older version of uboot has the same problem only if the
> > bad block appear in correspoding of befine of u-boot image. In order to adjust
> > the function we scan from the first block. The logic is not changed to have a
> > simple way to fix without get regression.
> >
> > The problematic part of old code was in this part:
> >
> > while (is_badblock(mtd, offs, 1)) {
> >            page = page + nand_page_per_block;
> >           /* Check i we've reached the end of flash. */
> >           if (page >= mtd->size >> chip->page_shift) {
> >                       free(page_buf);
> >                       return -ENOMEM;
> >          }
> > }
> >
> > Even we fix it adding increment of the offset of one erase block size we don't fix
> > the problem, because the first erase block where the image start is not checked.
>
> Could you please describe more details about your test? Thanks.

Suppose you have a badblock on 5 or 6. Let's start to have only 6
and you write uboot from 5 and let's the uboot be enough big to cover 5, 6, 7, 8


Case 1)
When you write the block 6 the code will skip it as bad during
programming. THe image of uboot (or flash.bin) will
be on 5 7 8 9, because the 6 is skipped. The while loop on spl will
read (from raw offset the 5) and then he will found the
bad block on next erase block in the while loop and will exists at the
end of the flash because the test
is done on the offset and not on the page that is not incremented

Case 2)

Now same example but let's suppose to have block 5 bad. So you write
your image and it will start
from a raw offset 5 but it will be written starting from 6. The spl
loader will fail because it will not skip
the first block and then will fail anyway to read the image. The patch
try to fix the above behavior

Case 3) can be any combination

Michael



>
> > The code was tested on an imx8mn where the boot rom api was not able to skip
> > it. Apart of that other architecure are using this code and all boards that has
> > nand as boot device can be affected
> >
> > Cc: Han Xu <han.xu@nxp.com>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> > V1->V2:
> >       - Adjust the commit message
> >       - Add Cc Han Xu and Fabio
> >       - fix size >= 0 to > 0
> > ---
> >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> >  1 file changed, 49 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > index 59a67ee414..2bfb181007 100644
> > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > @@ -218,14 +218,14 @@ void nand_init(void)
> >       mxs_nand_setup_ecc(mtd);
> >  }
> >
> > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> >  {
> > -     struct nand_chip *chip;
> > -     unsigned int page;
> > +     unsigned int sz;
> > +     unsigned int block, lastblock;
> > +     unsigned int page, page_offset;
> >       unsigned int nand_page_per_block;
> > -     unsigned int sz = 0;
> > +     struct nand_chip *chip;
> >       u8 *page_buf = NULL;
> > -     u32 page_off;
> >
> >       chip = mtd_to_nand(mtd);
> >       if (!chip->numchips)
> > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> > size, void *buf)
> >       if (!page_buf)
> >               return -ENOMEM;
> >
> > -     page = offs >> chip->page_shift;
> > -     page_off = offs & (mtd->writesize - 1);
> > +     /* offs has to be aligned to a page address! */
> > +     block = offs / mtd->erasesize;
> > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > +     page_offset = offs % mtd->writesize;
> >       nand_page_per_block = mtd->e
Copy Link
MA
Copy Link
NA
Copy Link
rasesize / mtd->writesize;
> >
> > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > -
> > -     while (size) {
> > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > -                     return -1;
> > -
> > -             if (size > (mtd->writesize - page_off))
> > -                     sz = (mtd->writesize - page_off);
> > -             else
> > -                     sz = size;
> > -
> > -             memcpy(buf, page_buf + page_off, sz);
> > -
> > -             offs += mtd->writesize;
> > -             page++;
> > -             buf += (mtd->writesize - page_off);
> > -             page_off = 0;
> > -             size -= sz;
> > -
> > -             /*
> > -              * Check if we have crossed a block boundary, and if so
> > -              * check for bad block.
> > -              */
> > -             if (!(page % nand_page_per_block)) {
> > -                     /*
> > -                      * Yes, new block. See if this block is good. If not,
> > -                      * loop until we find a good block.
> > -                      */
> > -                     while (is_badblock(mtd, offs, 1)) {
> > -                             page = page + nand_page_per_block;
> > -                             /* Check i we've reached the end of flash. */
> > -                             if (page >= mtd->size >> chip->page_shift) {
> > +     while (block <= lastblock && size > 0) {
> > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > +                     /* Skip bad blocks */
> > +                     while (page < nand_page_per_block) {
> > +                             int curr_page = nand_page_per_block * block +
> > page;
> > +
> > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> > < 0) {
> >                                       free(page_buf);
> > -                                     return -ENOMEM;
> > +                                     return -EIO;
> >                               }
> > +
> > +                             if (size > (mtd->writesize - page_offset))
> > +                                     sz = (mtd->writesize - page_offset);
> > +                             else
> > +                                     sz = size;
> > +
> > +                             memcpy(dst, page_buf + page_offset, sz);
> > +                             dst += sz;
> > +                             size -= sz;
> > +                             page_offset = 0;
> > +                             page++;
> >                       }
> > +
> > +                     page = 0;
> > +             } else {
> > +                     lastblock++;
> >               }
> > +
> > +             block++;
> >       }
> >
> >       free(page_buf);
> > @@ -294,6 +289,19 @@ void nand_deselect(void)
> >
> >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > +     unsigned int block, lastblock;
> > +
> > +     block = sector / mtd->erasesize;
> > +     lastblock = (sector + offs) / mtd->erasesize;
> > +
> > +     while (block <= lastblock) {
> > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > +                     offs += mtd->erasesize;
> > +                     lastblock++;
> > +             }
> > +
> > +             block++;
> > +     }
> > +
> >       return offs;
> >  }
> > --
> > 2.25.1
>


-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Nazzareno Trimarchi]

Dear Han and Fabio

On Thu, Apr 28, 2022 at 7:01 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi
>
> On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han.xu@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Michael Trimarchi <michael@amarulasolutions.com>
> > > Sent: Wednesday, April 27, 2022 12:50 AM
> > > To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > > Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> > > <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> > > Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> > > <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> > > <jagan@amarulasolutions.com>; Ariel D'Alessandro
> > > <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> > > Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> > >
> > > The specific implementation was having bug. Those bugs are since the beginning
> > > of the implementation. Some manufactures can receive this bug in their SPL code.
> > > This bug start to be more visible on architecture that has complicated boot
> > > process like imx8mn. Older version of uboot has the same problem only if the
> > > bad block appear in correspoding of befine of u-boot image. In order to adjust
> > > the function we scan from the first block. The logic is not changed to have a
> > > simple way to fix without get regression.
> > >
> > > The problematic part of old code was in this part:
> > >
> > > while (is_badblock(mtd, offs, 1)) {
> > >            page = page + nand_page_per_block;
> > >           /* Check i we've reached the end of flash. */
> > >           if (page >= mtd->size >> chip->page_shift) {
> > >                       free(page_buf);
> > >                       return -ENOMEM;
> > >          }
> > > }
> > >
> > > Even we fix it adding increment of the offset of one erase block size we don't fix
> > > the problem, because the first erase block where the image start is not checked.
> >
> > Could you please describe more details about your test? Thanks.
>
> Suppose you have a badblock on 5 or 6. Let's start to have only 6
> and you write uboot from 5 and let's the uboot be enough big to cover 5, 6, 7, 8
>
>
> Case 1)
> When you write the block 6 the code will skip it as bad during
> programming. THe image of uboot (or flash.bin) will
> be on 5 7 8 9, because the 6 is skipped. The while loop on spl will
> read (from raw offset the 5) and then he will found the
> bad block on next erase block in the while loop and will exists at the
> end of the flash because the test
> is done on the offset and not on the page that is not incremented
>
> Case 2)
>
> Now same example but let's suppose to have block 5 bad. So you write
> your image and it will start
> from a raw offset 5 but it will be written starting from 6. The spl
> loader will fail because it will not skip
> the first block and then will fail anyway to read the image. The patch
> try to fix the above behavior
>
> Case 3) can be any combination
>

Do I need to resend v3?

Michael

> Michael
>
>
>
> >
> > > The code was tested on an imx8mn where the boot rom api was not able to skip
> > > it. Apart of that other architecure are using this code and all boards that has
> > > nand as boot device can be affected
> > >
> > > Cc: Han Xu <han.xu@nxp.com>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > ---
> > > V1->V2:
> > >       - Adjust the commit message
> > >       - Add Cc Han Xu and Fabio
> > >       - fix size >= 0 to > 0
> > > ---
> > >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> > >  1 file changed, 49 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > index 59a67ee414..2bfb181007 100644
> > > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > @@ -218,14 +218,14 @@ void nand_init(void)
> > >       mxs_nand_setup_ecc(mtd);
> > >  }
> > >
> > > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > >  {
> > > -     struct nand_chip *chip;
> > > -     unsigned int page;
> > > +     unsigned int sz;
> > > +     unsigned int block, lastblock;
> > > +     unsigned int page, page_offset;
> > >       unsigned int nand_page_per_block;
> > > -     unsigned int sz = 0;
> > > +     struct nand_chip *chip;
> > >       u8 *page_buf = NULL;
> > > -     u32 page_off;
> > >
> > >       chip = mtd_to_nand(mtd);
> > >       if (!chip->numchips)
> > > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> > > size, void *buf)
> > >       if (!page_buf)
> > >               return -ENOMEM;
> > >
> > > -     page = offs >> chip->page_shift;
> > > -     page_off = offs & (mtd->writesize - 1);
> > > +     /* offs has to be aligned to a page address! */
> > > +     block = offs / mtd->erasesize;
> > > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > > +     page_offset = offs % mtd->writesize;
> > >       nand_page_per_block = mtd->e
> Copy Link
> MA
> Copy Link
> NA
> Copy Link
> rasesize / mtd->writesize;
> > >
> > > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > > -
> > > -     while (size) {
> > > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > > -                     return -1;
> > > -
> > > -             if (size > (mtd->writesize - page_off))
> > > -                     sz = (mtd->writesize - page_off);
> > > -             else
> > > -                     sz = size;
> > > -
> > > -             memcpy(buf, page_buf + page_off, sz);
> > > -
> > > -             offs += mtd->writesize;
> > > -             page++;
> > > -             buf += (mtd->writesize - page_off);
> > > -             page_off = 0;
> > > -             size -= sz;
> > > -
> > > -             /*
> > > -              * Check if we have crossed a block boundary, and if so
> > > -              * check for bad block.
> > > -              */
> > > -             if (!(page % nand_page_per_block)) {
> > > -                     /*
> > > -                      * Yes, new block. See if this block is good. If not,
> > > -                      * loop until we find a good block.
> > > -                      */
> > > -                     while (is_badblock(mtd, offs, 1)) {
> > > -                             page = page + nand_page_per_block;
> > > -                             /* Check i we've reached the end of flash. */
> > > -                             if (page >= mtd->size >> chip->page_shift) {
> > > +     while (block <= lastblock && size > 0) {
> > > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > > +                     /* Skip bad blocks */
> > > +                     while (page < nand_page_per_block) {
> > > +                             int curr_page = nand_page_per_block * block +
> > > page;
> > > +
> > > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> > > < 0) {
> > >                                       free(page_buf);
> > > -                                     return -ENOMEM;
> > > +                                     return -EIO;
> > >                               }
> > > +
> > > +                             if (size > (mtd->writesize - page_offset))
> > > +                                     sz = (mtd->writesize - page_offset);
> > > +                             else
> > > +                                     sz = size;
> > > +
> > > +                             memcpy(dst, page_buf + page_offset, sz);
> > > +                             dst += sz;
> > > +                             size -= sz;
> > > +                             page_offset = 0;
> > > +                             page++;
> > >                       }
> > > +
> > > +                     page = 0;
> > > +             } else {
> > > +                     lastblock++;
> > >               }
> > > +
> > > +             block++;
> > >       }
> > >
> > >       free(page_buf);
> > > @@ -294,6 +289,19 @@ void nand_deselect(void)
> > >
> > >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> > > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > > +     unsigned int block, lastblock;
> > > +
> > > +     block = sector / mtd->erasesize;
> > > +     lastblock = (sector + offs) / mtd->erasesize;
> > > +
> > > +     while (block <= lastblock) {
> > > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > > +                     offs += mtd->erasesize;
> > > +                     lastblock++;
> > > +             }
> > > +
> > > +             block++;
> > > +     }
> > > +
> > >       return offs;
> > >  }
> > > --
> > > 2.25.1
> >
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Han Xu]

On 22/05/01 08:36AM, Michael Nazzareno Trimarchi wrote:
> Dear Han and Fabio
> 
> On Thu, Apr 28, 2022 at 7:01 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi
> >
> > On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han.xu@nxp.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Michael Trimarchi <michael@amarulasolutions.com>
> > > > Sent: Wednesday, April 27, 2022 12:50 AM
> > > > To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > > > Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> > > > <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> > > > Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> > > > <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> > > > <jagan@amarulasolutions.com>; Ariel D'Alessandro
> > > > <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> > > > Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> > > >
> > > > The specific implementation was having bug. Those bugs are since the beginning
> > > > of the implementation. Some manufactures can receive this bug in their SPL code.
> > > > This bug start to be more visible on architecture that has complicated boot
> > > > process like imx8mn. Older version of uboot has the same problem only if the
> > > > bad block appear in correspoding of befine of u-boot image. In order to adjust
> > > > the function we scan from the first block. The logic is not changed to have a
> > > > simple way to fix without get regression.
> > > >
> > > > The problematic part of old code was in this part:
> > > >
> > > > while (is_badblock(mtd, offs, 1)) {
> > > >            page = page + nand_page_per_block;
> > > >           /* Check i we've reached the end of flash. */
> > > >           if (page >= mtd->size >> chip->page_shift) {
> > > >                       free(page_buf);
> > > >                       return -ENOMEM;
> > > >          }
> > > > }
> > > >
> > > > Even we fix it adding increment of the offset of one erase block size we don't fix
> > > > the problem, because the first erase block where the image start is not checked.
> > >
> > > Could you please describe more details about your test? Thanks.
> >
> > Suppose you have a badblock on 5 or 6. Let's start to have only 6
> > and you write uboot from 5 and let's the uboot be enough big to cover 5, 6, 7, 8
> >
> >
> > Case 1)
> > When you write the block 6 the code will skip it as bad during
> > programming. THe image of uboot (or flash.bin) will
> > be on 5 7 8 9, because the 6 is skipped. The while loop on spl will
> > read (from raw offset the 5) and then he will found the
> > bad block on next erase block in the while loop and will exists at the
> > end of the flash because the test
> > is done on the offset and not on the page that is not incremented
> >
> > Case 2)
> >
> > Now same example but let's suppose to have block 5 bad. So you write
> > your image and it will start
> > from a raw offset 5 but it will be written starting from 6. The spl
> > loader will fail because it will not skip
> > the first block and then will fail anyway to read the image. The patch
> > try to fix the above behavior
> >
> > Case 3) can be any combination
> >
> 
> Do I need to resend v3?
> 
> Michael

It's not about the v3. I need to discuss some details with ROM team but
unfortunately they are all in vacation till May 5th. I will respond after the
discussion.

> 
> > Michael
> >
> >
> >
> > >
> > > > The code was tested on an imx8mn where the boot rom api was not able to skip
> > > > it. Apart of that other architecure are using this code and all boards that has
> > > > nand as boot device can be affected
> > > >
> > > > Cc: Han Xu <han.xu@nxp.com>
> > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > ---
> > > > V1->V2:
> > > >       - Adjust the commit message
> > > >       - Add Cc Han Xu and Fabio
> > > >       - fix size >= 0 to > 0
> > > > ---
> > > >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> > > >  1 file changed, 49 insertions(+), 41 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > index 59a67ee414..2bfb181007 100644
> > > > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > @@ -218,14 +218,14 @@ void nand_init(void)
> > > >       mxs_nand_setup_ecc(mtd);
> > > >  }
> > > >
> > > > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > >  {
> > > > -     struct nand_chip *chip;
> > > > -     unsigned int page;
> > > > +     unsigned int sz;
> > > > +     unsigned int block, lastblock;
> > > > +     unsigned int page, page_offset;
> > > >       unsigned int nand_page_per_block;
> > > > -     unsigned int sz = 0;
> > > > +     struct nand_chip *chip;
> > > >       u8 *page_buf = NULL;
> > > > -     u32 page_off;
> > > >
> > > >       chip = mtd_to_nand(mtd);
> > > >       if (!chip->numchips)
> > > > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> > > > size, void *buf)
> > > >       if (!page_buf)
> > > >               return -ENOMEM;
> > > >
> > > > -     page = offs >> chip->page_shift;
> > > > -     page_off = offs & (mtd->writesize - 1);
> > > > +     /* offs has to be aligned to a page address! */
> > > > +     block = offs / mtd->erasesize;
> > > > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > > > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > > > +     page_offset = offs % mtd->writesize;
> > > >       nand_page_per_block = mtd->e
> > Copy Link
> > MA
> > Copy Link
> > NA
> > Copy Link
> > rasesize / mtd->writesize;
> > > >
> > > > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > > > -
> > > > -     while (size) {
> > > > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > > > -                     return -1;
> > > > -
> > > > -             if (size > (mtd->writesize - page_off))
> > > > -                     sz = (mtd->writesize - page_off);
> > > > -             else
> > > > -                     sz = size;
> > > > -
> > > > -             memcpy(buf, page_buf + page_off, sz);
> > > > -
> > > > -             offs += mtd->writesize;
> > > > -             page++;
> > > > -             buf += (mtd->writesize - page_off);
> > > > -             page_off = 0;
> > > > -             size -= sz;
> > > > -
> > > > -             /*
> > > > -              * Check if we have crossed a block boundary, and if so
> > > > -              * check for bad block.
> > > > -              */
> > > > -             if (!(page % nand_page_per_block)) {
> > > > -                     /*
> > > > -                      * Yes, new block. See if this block is good. If not,
> > > > -                      * loop until we find a good block.
> > > > -                      */
> > > > -                     while (is_badblock(mtd, offs, 1)) {
> > > > -                             page = page + nand_page_per_block;
> > > > -                             /* Check i we've reached the end of flash. */
> > > > -                             if (page >= mtd->size >> chip->page_shift) {
> > > > +     while (block <= lastblock && size > 0) {
> > > > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > > > +                     /* Skip bad blocks */
> > > > +                     while (page < nand_page_per_block) {
> > > > +                             int curr_page = nand_page_per_block * block +
> > > > page;
> > > > +
> > > > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> > > > < 0) {
> > > >                                       free(page_buf);
> > > > -                                     return -ENOMEM;
> > > > +                                     return -EIO;
> > > >                               }
> > > > +
> > > > +                             if (size > (mtd->writesize - page_offset))
> > > > +                                     sz = (mtd->writesize - page_offset);
> > > > +                             else
> > > > +                                     sz = size;
> > > > +
> > > > +                             memcpy(dst, page_buf + page_offset, sz);
> > > > +                             dst += sz;
> > > > +                             size -= sz;
> > > > +                             page_offset = 0;
> > > > +                             page++;
> > > >                       }
> > > > +
> > > > +                     page = 0;
> > > > +             } else {
> > > > +                     lastblock++;
> > > >               }
> > > > +
> > > > +             block++;
> > > >       }
> > > >
> > > >       free(page_buf);
> > > > @@ -294,6 +289,19 @@ void nand_deselect(void)
> > > >
> > > >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> > > > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > > > +     unsigned int block, lastblock;
> > > > +
> > > > +     block = sector / mtd->erasesize;
> > > > +     lastblock = (sector + offs) / mtd->erasesize;
> > > > +
> > > > +     while (block <= lastblock) {
> > > > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > > > +                     offs += mtd->erasesize;
> > > > +                     lastblock++;
> > > > +             }
> > > > +
> > > > +             block++;
> > > > +     }
> > > > +
> > > >       return offs;
> > > >  }
> > > > --
> > > > 2.25.1
> > >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0
> 
> 
> 
> -- 
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Nazzareno Trimarchi]

Hi Han

On Mon, May 2, 2022 at 11:32 PM Han Xu <han.xu@nxp.com> wrote:
>
> On 22/05/01 08:36AM, Michael Nazzareno Trimarchi wrote:
> > Dear Han and Fabio
> >
> > On Thu, Apr 28, 2022 at 7:01 AM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han.xu@nxp.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > Sent: Wednesday, April 27, 2022 12:50 AM
> > > > > To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > > > > Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> > > > > <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> > > > > Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> > > > > <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> > > > > <jagan@amarulasolutions.com>; Ariel D'Alessandro
> > > > > <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> > > > > Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> > > > >
> > > > > The specific implementation was having bug. Those bugs are since the beginning
> > > > > of the implementation. Some manufactures can receive this bug in their SPL code.
> > > > > This bug start to be more visible on architecture that has complicated boot
> > > > > process like imx8mn. Older version of uboot has the same problem only if the
> > > > > bad block appear in correspoding of befine of u-boot image. In order to adjust
> > > > > the function we scan from the first block. The logic is not changed to have a
> > > > > simple way to fix without get regression.
> > > > >
> > > > > The problematic part of old code was in this part:
> > > > >
> > > > > while (is_badblock(mtd, offs, 1)) {
> > > > >            page = page + nand_page_per_block;
> > > > >           /* Check i we've reached the end of flash. */
> > > > >           if (page >= mtd->size >> chip->page_shift) {
> > > > >                       free(page_buf);
> > > > >                       return -ENOMEM;
> > > > >          }
> > > > > }
> > > > >
> > > > > Even we fix it adding increment of the offset of one erase block size we don't fix
> > > > > the problem, because the first erase block where the image start is not checked.
> > > >
> > > > Could you please describe more details about your test? Thanks.
> > >
> > > Suppose you have a badblock on 5 or 6. Let's start to have only 6
> > > and you write uboot from 5 and let's the uboot be enough big to cover 5, 6, 7, 8
> > >
> > >
> > > Case 1)
> > > When you write the block 6 the code will skip it as bad during
> > > programming. THe image of uboot (or flash.bin) will
> > > be on 5 7 8 9, because the 6 is skipped. The while loop on spl will
> > > read (from raw offset the 5) and then he will found the
> > > bad block on next erase block in the while loop and will exists at the
> > > end of the flash because the test
> > > is done on the offset and not on the page that is not incremented
> > >
> > > Case 2)
> > >
> > > Now same example but let's suppose to have block 5 bad. So you write
> > > your image and it will start
> > > from a raw offset 5 but it will be written starting from 6. The spl
> > > loader will fail because it will not skip
> > > the first block and then will fail anyway to read the image. The patch
> > > try to fix the above behavior
> > >
> > > Case 3) can be any combination
> > >
> >
> > Do I need to resend v3?
> >
> > Michael
>
> It's not about the v3. I need to discuss some details with ROM team but
> unfortunately they are all in vacation till May 5th. I will respond after the
> discussion.
>

No problem I will wait, but the code is used by other nxp
architectures that don't use the
rom api.

Michael

Definit
> >
> > > Michael
> > >
> > >
> > >
> > > >
> > > > > The code was tested on an imx8mn where the boot rom api was not able to skip
> > > > > it. Apart of that other architecure are using this code and all boards that has
> > > > > nand as boot device can be affected
> > > > >
> > > > > Cc: Han Xu <han.xu@nxp.com>
> > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > ---
> > > > > V1->V2:
> > > > >       - Adjust the commit message
> > > > >       - Add Cc Han Xu and Fabio
> > > > >       - fix size >= 0 to > 0
> > > > > ---
> > > > >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> > > > >  1 file changed, 49 insertions(+), 41 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > index 59a67ee414..2bfb181007 100644
> > > > > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > @@ -218,14 +218,14 @@ void nand_init(void)
> > > > >       mxs_nand_setup_ecc(mtd);
> > > > >  }
> > > > >
> > > > > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > > > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > > >  {
> > > > > -     struct nand_chip *chip;
> > > > > -     unsigned int page;
> > > > > +     unsigned int sz;
> > > > > +     unsigned int block, lastblock;
> > > > > +     unsigned int page, page_offset;
> > > > >       unsigned int nand_page_per_block;
> > > > > -     unsigned int sz = 0;
> > > > > +     struct nand_chip *chip;
> > > > >       u8 *page_buf = NULL;
> > > > > -     u32 page_off;
> > > > >
> > > > >       chip = mtd_to_nand(mtd);
> > > > >       if (!chip->numchips)
> > > > > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> > > > > size, void *buf)
> > > > >       if (!page_buf)
> > > > >               return -ENOMEM;
> > > > >
> > > > > -     page = offs >> chip->page_shift;
> > > > > -     page_off = offs & (mtd->writesize - 1);
> > > > > +     /* offs has to be aligned to a page address! */
> > > > > +     block = offs / mtd->erasesize;
> > > > > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > > > > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > > > > +     page_offset = offs % mtd->writesize;
> > > > >       nand_page_per_block = mtd->e
> > > Copy Link
> > > MA
> > > Copy Link
> > > NA
> > > Copy Link
> > > rasesize / mtd->writesize;
> > > > >
> > > > > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > > > > -
> > > > > -     while (size) {
> > > > > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > > > > -                     return -1;
> > > > > -
> > > > > -             if (size > (mtd->writesize - page_off))
> > > > > -                     sz = (mtd->writesize - page_off);
> > > > > -             else
> > > > > -                     sz = size;
> > > > > -
> > > > > -             memcpy(buf, page_buf + page_off, sz);
> > > > > -
> > > > > -             offs += mtd->writesize;
> > > > > -             page++;
> > > > > -             buf += (mtd->writesize - page_off);
> > > > > -             page_off = 0;
> > > > > -             size -= sz;
> > > > > -
> > > > > -             /*
> > > > > -              * Check if we have crossed a block boundary, and if so
> > > > > -              * check for bad block.
> > > > > -              */
> > > > > -             if (!(page % nand_page_per_block)) {
> > > > > -                     /*
> > > > > -                      * Yes, new block. See if this block is good. If not,
> > > > > -                      * loop until we find a good block.
> > > > > -                      */
> > > > > -                     while (is_badblock(mtd, offs, 1)) {
> > > > > -                             page = page + nand_page_per_block;
> > > > > -                             /* Check i we've reached the end of flash. */
> > > > > -                             if (page >= mtd->size >> chip->page_shift) {
> > > > > +     while (block <= lastblock && size > 0) {
> > > > > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > > > > +                     /* Skip bad blocks */
> > > > > +                     while (page < nand_page_per_block) {
> > > > > +                             int curr_page = nand_page_per_block * block +
> > > > > page;
> > > > > +
> > > > > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> > > > > < 0) {
> > > > >                                       free(page_buf);
> > > > > -                                     return -ENOMEM;
> > > > > +                                     return -EIO;
> > > > >                               }
> > > > > +
> > > > > +                             if (size > (mtd->writesize - page_offset))
> > > > > +                                     sz = (mtd->writesize - page_offset);
> > > > > +                             else
> > > > > +                                     sz = size;
> > > > > +
> > > > > +                             memcpy(dst, page_buf + page_offset, sz);
> > > > > +                             dst += sz;
> > > > > +                             size -= sz;
> > > > > +                             page_offset = 0;
> > > > > +                             page++;
> > > > >                       }
> > > > > +
> > > > > +                     page = 0;
> > > > > +             } else {
> > > > > +                     lastblock++;
> > > > >               }
> > > > > +
> > > > > +             block++;
> > > > >       }
> > > > >
> > > > >       free(page_buf);
> > > > > @@ -294,6 +289,19 @@ void nand_deselect(void)
> > > > >
> > > > >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> > > > > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > > > > +     unsigned int block, lastblock;
> > > > > +
> > > > > +     block = sector / mtd->erasesize;
> > > > > +     lastblock = (sector + offs) / mtd->erasesize;
> > > > > +
> > > > > +     while (block <= lastblock) {
> > > > > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > > > > +                     offs += mtd->erasesize;
> > > > > +                     lastblock++;
> > > > > +             }
> > > > > +
> > > > > +             block++;
> > > > > +     }
> > > > > +
> > > > >       return offs;
> > > > >  }
> > > > > --
> > > > > 2.25.1
> > > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Nazzareno Trimarchi]

Hi Han

Any update?

Michael

On Tue, May 3, 2022 at 7:14 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Han
>
> On Mon, May 2, 2022 at 11:32 PM Han Xu <han.xu@nxp.com> wrote:
> >
> > On 22/05/01 08:36AM, Michael Nazzareno Trimarchi wrote:
> > > Dear Han and Fabio
> > >
> > > On Thu, Apr 28, 2022 at 7:01 AM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han.xu@nxp.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > Sent: Wednesday, April 27, 2022 12:50 AM
> > > > > > To: Han Xu <han.xu@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > > > > > Cc: Ye Li <ye.li@nxp.com>; Stefano Babic <sbabic@denx.de>; Miquel Raynal
> > > > > > <miquel.raynal@bootlin.com>; Fabio Estevam <festevam@denx.de>; Dario
> > > > > > Binacchi <dario.binacchi@amarulasolutions.com>; Sean Anderson
> > > > > > <sean.anderson@seco.com>; linux-kernel@amarulasolutions.com; Jagan Teki
> > > > > > <jagan@amarulasolutions.com>; Ariel D'Alessandro
> > > > > > <ariel.dalessandro@collabora.com>; Fabio Estevam <festevam@gmail.com>
> > > > > > Subject: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping
> > > > > >
> > > > > > The specific implementation was having bug. Those bugs are since the beginning
> > > > > > of the implementation. Some manufactures can receive this bug in their SPL code.
> > > > > > This bug start to be more visible on architecture that has complicated boot
> > > > > > process like imx8mn. Older version of uboot has the same problem only if the
> > > > > > bad block appear in correspoding of befine of u-boot image. In order to adjust
> > > > > > the function we scan from the first block. The logic is not changed to have a
> > > > > > simple way to fix without get regression.
> > > > > >
> > > > > > The problematic part of old code was in this part:
> > > > > >
> > > > > > while (is_badblock(mtd, offs, 1)) {
> > > > > >            page = page + nand_page_per_block;
> > > > > >           /* Check i we've reached the end of flash. */
> > > > > >           if (page >= mtd->size >> chip->page_shift) {
> > > > > >                       free(page_buf);
> > > > > >                       return -ENOMEM;
> > > > > >          }
> > > > > > }
> > > > > >
> > > > > > Even we fix it adding increment of the offset of one erase block size we don't fix
> > > > > > the problem, because the first erase block where the image start is not checked.
> > > > >
> > > > > Could you please describe more details about your test? Thanks.
> > > >
> > > > Suppose you have a badblock on 5 or 6. Let's start to have only 6
> > > > and you write uboot from 5 and let's the uboot be enough big to cover 5, 6, 7, 8
> > > >
> > > >
> > > > Case 1)
> > > > When you write the block 6 the code will skip it as bad during
> > > > programming. THe image of uboot (or flash.bin) will
> > > > be on 5 7 8 9, because the 6 is skipped. The while loop on spl will
> > > > read (from raw offset the 5) and then he will found the
> > > > bad block on next erase block in the while loop and will exists at the
> > > > end of the flash because the test
> > > > is done on the offset and not on the page that is not incremented
> > > >
> > > > Case 2)
> > > >
> > > > Now same example but let's suppose to have block 5 bad. So you write
> > > > your image and it will start
> > > > from a raw offset 5 but it will be written starting from 6. The spl
> > > > loader will fail because it will not skip
> > > > the first block and then will fail anyway to read the image. The patch
> > > > try to fix the above behavior
> > > >
> > > > Case 3) can be any combination
> > > >
> > >
> > > Do I need to resend v3?
> > >
> > > Michael
> >
> > It's not about the v3. I need to discuss some details with ROM team but
> > unfortunately they are all in vacation till May 5th. I will respond after the
> > discussion.
> >
>
> No problem I will wait, but the code is used by other nxp
> architectures that don't use the
> rom api.
>
> Michael
>
> Definit
> > >
> > > > Michael
> > > >
> > > >
> > > >
> > > > >
> > > > > > The code was tested on an imx8mn where the boot rom api was not able to skip
> > > > > > it. Apart of that other architecure are using this code and all boards that has
> > > > > > nand as boot device can be affected
> > > > > >
> > > > > > Cc: Han Xu <han.xu@nxp.com>
> > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > ---
> > > > > > V1->V2:
> > > > > >       - Adjust the commit message
> > > > > >       - Add Cc Han Xu and Fabio
> > > > > >       - fix size >= 0 to > 0
> > > > > > ---
> > > > > >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> > > > > >  1 file changed, 49 insertions(+), 41 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > > b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > > index 59a67ee414..2bfb181007 100644
> > > > > > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > > > > > @@ -218,14 +218,14 @@ void nand_init(void)
> > > > > >       mxs_nand_setup_ecc(mtd);
> > > > > >  }
> > > > > >
> > > > > > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > > > > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > > > >  {
> > > > > > -     struct nand_chip *chip;
> > > > > > -     unsigned int page;
> > > > > > +     unsigned int sz;
> > > > > > +     unsigned int block, lastblock;
> > > > > > +     unsigned int page, page_offset;
> > > > > >       unsigned int nand_page_per_block;
> > > > > > -     unsigned int sz = 0;
> > > > > > +     struct nand_chip *chip;
> > > > > >       u8 *page_buf = NULL;
> > > > > > -     u32 page_off;
> > > > > >
> > > > > >       chip = mtd_to_nand(mtd);
> > > > > >       if (!chip->numchips)
> > > > > > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int
> > > > > > size, void *buf)
> > > > > >       if (!page_buf)
> > > > > >               return -ENOMEM;
> > > > > >
> > > > > > -     page = offs >> chip->page_shift;
> > > > > > -     page_off = offs & (mtd->writesize - 1);
> > > > > > +     /* offs has to be aligned to a page address! */
> > > > > > +     block = offs / mtd->erasesize;
> > > > > > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > > > > > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > > > > > +     page_offset = offs % mtd->writesize;
> > > > > >       nand_page_per_block = mtd->e
> > > > Copy Link
> > > > MA
> > > > Copy Link
> > > > NA
> > > > Copy Link
> > > > rasesize / mtd->writesize;
> > > > > >
> > > > > > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > > > > > -
> > > > > > -     while (size) {
> > > > > > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > > > > > -                     return -1;
> > > > > > -
> > > > > > -             if (size > (mtd->writesize - page_off))
> > > > > > -                     sz = (mtd->writesize - page_off);
> > > > > > -             else
> > > > > > -                     sz = size;
> > > > > > -
> > > > > > -             memcpy(buf, page_buf + page_off, sz);
> > > > > > -
> > > > > > -             offs += mtd->writesize;
> > > > > > -             page++;
> > > > > > -             buf += (mtd->writesize - page_off);
> > > > > > -             page_off = 0;
> > > > > > -             size -= sz;
> > > > > > -
> > > > > > -             /*
> > > > > > -              * Check if we have crossed a block boundary, and if so
> > > > > > -              * check for bad block.
> > > > > > -              */
> > > > > > -             if (!(page % nand_page_per_block)) {
> > > > > > -                     /*
> > > > > > -                      * Yes, new block. See if this block is good. If not,
> > > > > > -                      * loop until we find a good block.
> > > > > > -                      */
> > > > > > -                     while (is_badblock(mtd, offs, 1)) {
> > > > > > -                             page = page + nand_page_per_block;
> > > > > > -                             /* Check i we've reached the end of flash. */
> > > > > > -                             if (page >= mtd->size >> chip->page_shift) {
> > > > > > +     while (block <= lastblock && size > 0) {
> > > > > > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > > > > > +                     /* Skip bad blocks */
> > > > > > +                     while (page < nand_page_per_block) {
> > > > > > +                             int curr_page = nand_page_per_block * block +
> > > > > > page;
> > > > > > +
> > > > > > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page)
> > > > > > < 0) {
> > > > > >                                       free(page_buf);
> > > > > > -                                     return -ENOMEM;
> > > > > > +                                     return -EIO;
> > > > > >                               }
> > > > > > +
> > > > > > +                             if (size > (mtd->writesize - page_offset))
> > > > > > +                                     sz = (mtd->writesize - page_offset);
> > > > > > +                             else
> > > > > > +                                     sz = size;
> > > > > > +
> > > > > > +                             memcpy(dst, page_buf + page_offset, sz);
> > > > > > +                             dst += sz;
> > > > > > +                             size -= sz;
> > > > > > +                             page_offset = 0;
> > > > > > +                             page++;
> > > > > >                       }
> > > > > > +
> > > > > > +                     page = 0;
> > > > > > +             } else {
> > > > > > +                     lastblock++;
> > > > > >               }
> > > > > > +
> > > > > > +             block++;
> > > > > >       }
> > > > > >
> > > > > >       free(page_buf);
> > > > > > @@ -294,6 +289,19 @@ void nand_deselect(void)
> > > > > >
> > > > > >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)  {
> > > > > > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > > > > > +     unsigned int block, lastblock;
> > > > > > +
> > > > > > +     block = sector / mtd->erasesize;
> > > > > > +     lastblock = (sector + offs) / mtd->erasesize;
> > > > > > +
> > > > > > +     while (block <= lastblock) {
> > > > > > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > > > > > +                     offs += mtd->erasesize;
> > > > > > +                     lastblock++;
> > > > > > +             }
> > > > > > +
> > > > > > +             block++;
> > > > > > +     }
> > > > > > +
> > > > > >       return offs;
> > > > > >  }
> > > > > > --
> > > > > > 2.25.1
> > > > >
> > > >
> > > >
> > > > --
> > > > Michael Nazzareno Trimarchi
> > > > Co-Founder & Chief Executive Officer
> > > > M. +39 347 913 2170
> > > > michael@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > info@amarulasolutions.com
> > > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&amp;data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&amp;reserved=0
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Han Xu]

On 22/04/27 07:50AM, Michael Trimarchi wrote:
> The specific implementation was having bug. Those bugs are since
> the beginning of the implementation. Some manufactures can receive
> this bug in their SPL code. This bug start to be more visible on
> architecture that has complicated boot process like imx8mn. Older
> version of uboot has the same problem only if the bad block
> appear in correspoding of befine of u-boot image. In order to
> adjust the function we scan from the first block. The logic
> is not changed to have a simple way to fix without get regression.
> 
> The problematic part of old code was in this part:
> 
> while (is_badblock(mtd, offs, 1)) {
>            page = page + nand_page_per_block;
>           /* Check i we've reached the end of flash. */
>           if (page >= mtd->size >> chip->page_shift) {
>                       free(page_buf);
>                       return -ENOMEM;
>          }
> }
> 
> Even we fix it adding increment of the offset of one erase block size
> we don't fix the problem, because the first erase block where the
> image start is not checked. The code was tested on an imx8mn where
> the boot rom api was not able to skip it. Apart of that other
> architecure are using this code and all boards that has nand as boot
> device can be affected
> 
> Cc: Han Xu <han.xu@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Acked-by: Han Xu <han.xu@nxp.com>

> ---
> V1->V2:
> 	- Adjust the commit message
> 	- Add Cc Han Xu and Fabio
> 	- fix size >= 0 to > 0
> ---
>  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c b/drivers/mtd/nand/raw/mxs_nand_spl.c
> index 59a67ee414..2bfb181007 100644
> --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> @@ -218,14 +218,14 @@ void nand_init(void)
>  	mxs_nand_setup_ecc(mtd);
>  }
>  
> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
>  {
> -	struct nand_chip *chip;
> -	unsigned int page;
> +	unsigned int sz;
> +	unsigned int block, lastblock;
> +	unsigned int page, page_offset;
>  	unsigned int nand_page_per_block;
> -	unsigned int sz = 0;
> +	struct nand_chip *chip;
>  	u8 *page_buf = NULL;
> -	u32 page_off;
>  
>  	chip = mtd_to_nand(mtd);
>  	if (!chip->numchips)
> @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>  	if (!page_buf)
>  		return -ENOMEM;
>  
> -	page = offs >> chip->page_shift;
> -	page_off = offs & (mtd->writesize - 1);
> +	/* offs has to be aligned to a page address! */
> +	block = offs / mtd->erasesize;
> +	lastblock = (offs + size - 1) / mtd->erasesize;
> +	page = (offs % mtd->erasesize) / mtd->writesize;
> +	page_offset = offs % mtd->writesize;
>  	nand_page_per_block = mtd->erasesize / mtd->writesize;
>  
> -	debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> -
> -	while (size) {
> -		if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> -			return -1;
> -
> -		if (size > (mtd->writesize - page_off))
> -			sz = (mtd->writesize - page_off);
> -		else
> -			sz = size;
> -
> -		memcpy(buf, page_buf + page_off, sz);
> -
> -		offs += mtd->writesize;
> -		page++;
> -		buf += (mtd->writesize - page_off);
> -		page_off = 0;
> -		size -= sz;
> -
> -		/*
> -		 * Check if we have crossed a block boundary, and if so
> -		 * check for bad block.
> -		 */
> -		if (!(page % nand_page_per_block)) {
> -			/*
> -			 * Yes, new block. See if this block is good. If not,
> -			 * loop until we find a good block.
> -			 */
> -			while (is_badblock(mtd, offs, 1)) {
> -				page = page + nand_page_per_block;
> -				/* Check i we've reached the end of flash. */
> -				if (page >= mtd->size >> chip->page_shift) {
> +	while (block <= lastblock && size > 0) {
> +		if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> +			/* Skip bad blocks */
> +			while (page < nand_page_per_block) {
> +				int curr_page = nand_page_per_block * block + page;
> +
> +				if (mxs_read_page_ecc(mtd, page_buf, curr_page) < 0) {
>  					free(page_buf);
> -					return -ENOMEM;
> +					return -EIO;
>  				}
> +
> +				if (size > (mtd->writesize - page_offset))
> +					sz = (mtd->writesize - page_offset);
> +				else
> +					sz = size;
> +
> +				memcpy(dst, page_buf + page_offset, sz);
> +				dst += sz;
> +				size -= sz;
> +				page_offset = 0;
> +				page++;
>  			}
> +
> +			page = 0;
> +		} else {
> +			lastblock++;
>  		}
> +
> +		block++;
>  	}
>  
>  	free(page_buf);
> @@ -294,6 +289,19 @@ void nand_deselect(void)
>  
>  u32 nand_spl_adjust_offset(u32 sector, u32 offs)
>  {
> -	/* Handle the offset adjust in nand_spl_load_image,*/
> +	unsigned int block, lastblock;
> +
> +	block = sector / mtd->erasesize;
> +	lastblock = (sector + offs) / mtd->erasesize;
> +
> +	while (block <= lastblock) {
> +		if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> +			offs += mtd->erasesize;
> +			lastblock++;
> +		}
> +
> +		block++;
> +	}
> +
>  	return offs;
>  }
> -- 
> 2.25.1
> 

Re: [PATCH V2 2/4] mtd: nand: mxs_nand_spl: Fix bad block skipping

[Michael Nazzareno Trimarchi]

Hi

Thank you. What is the reply from bootrom team?

Michael

On Fri, May 6, 2022 at 4:41 PM Han Xu <han.xu@nxp.com> wrote:
>
> On 22/04/27 07:50AM, Michael Trimarchi wrote:
> > The specific implementation was having bug. Those bugs are since
> > the beginning of the implementation. Some manufactures can receive
> > this bug in their SPL code. This bug start to be more visible on
> > architecture that has complicated boot process like imx8mn. Older
> > version of uboot has the same problem only if the bad block
> > appear in correspoding of befine of u-boot image. In order to
> > adjust the function we scan from the first block. The logic
> > is not changed to have a simple way to fix without get regression.
> >
> > The problematic part of old code was in this part:
> >
> > while (is_badblock(mtd, offs, 1)) {
> >            page = page + nand_page_per_block;
> >           /* Check i we've reached the end of flash. */
> >           if (page >= mtd->size >> chip->page_shift) {
> >                       free(page_buf);
> >                       return -ENOMEM;
> >          }
> > }
> >
> > Even we fix it adding increment of the offset of one erase block size
> > we don't fix the problem, because the first erase block where the
> > image start is not checked. The code was tested on an imx8mn where
> > the boot rom api was not able to skip it. Apart of that other
> > architecure are using this code and all boards that has nand as boot
> > device can be affected
> >
> > Cc: Han Xu <han.xu@nxp.com>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>
> Acked-by: Han Xu <han.xu@nxp.com>
>
> > ---
> > V1->V2:
> >       - Adjust the commit message
> >       - Add Cc Han Xu and Fabio
> >       - fix size >= 0 to > 0
> > ---
> >  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
> >  1 file changed, 49 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/mxs_nand_spl.c b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > index 59a67ee414..2bfb181007 100644
> > --- a/drivers/mtd/nand/raw/mxs_nand_spl.c
> > +++ b/drivers/mtd/nand/raw/mxs_nand_spl.c
> > @@ -218,14 +218,14 @@ void nand_init(void)
> >       mxs_nand_setup_ecc(mtd);
> >  }
> >
> > -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> >  {
> > -     struct nand_chip *chip;
> > -     unsigned int page;
> > +     unsigned int sz;
> > +     unsigned int block, lastblock;
> > +     unsigned int page, page_offset;
> >       unsigned int nand_page_per_block;
> > -     unsigned int sz = 0;
> > +     struct nand_chip *chip;
> >       u8 *page_buf = NULL;
> > -     u32 page_off;
> >
> >       chip = mtd_to_nand(mtd);
> >       if (!chip->numchips)
> > @@ -235,47 +235,42 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
> >       if (!page_buf)
> >               return -ENOMEM;
> >
> > -     page = offs >> chip->page_shift;
> > -     page_off = offs & (mtd->writesize - 1);
> > +     /* offs has to be aligned to a page address! */
> > +     block = offs / mtd->erasesize;
> > +     lastblock = (offs + size - 1) / mtd->erasesize;
> > +     page = (offs % mtd->erasesize) / mtd->writesize;
> > +     page_offset = offs % mtd->writesize;
> >       nand_page_per_block = mtd->erasesize / mtd->writesize;
> >
> > -     debug("%s offset:0x%08x len:%d page:%x\n", __func__, offs, size, page);
> > -
> > -     while (size) {
> > -             if (mxs_read_page_ecc(mtd, page_buf, page) < 0)
> > -                     return -1;
> > -
> > -             if (size > (mtd->writesize - page_off))
> > -                     sz = (mtd->writesize - page_off);
> > -             else
> > -                     sz = size;
> > -
> > -             memcpy(buf, page_buf + page_off, sz);
> > -
> > -             offs += mtd->writesize;
> > -             page++;
> > -             buf += (mtd->writesize - page_off);
> > -             page_off = 0;
> > -             size -= sz;
> > -
> > -             /*
> > -              * Check if we have crossed a block boundary, and if so
> > -              * check for bad block.
> > -              */
> > -             if (!(page % nand_page_per_block)) {
> > -                     /*
> > -                      * Yes, new block. See if this block is good. If not,
> > -                      * loop until we find a good block.
> > -                      */
> > -                     while (is_badblock(mtd, offs, 1)) {
> > -                             page = page + nand_page_per_block;
> > -                             /* Check i we've reached the end of flash. */
> > -                             if (page >= mtd->size >> chip->page_shift) {
> > +     while (block <= lastblock && size > 0) {
> > +             if (!is_badblock(mtd, mtd->erasesize * block, 1)) {
> > +                     /* Skip bad blocks */
> > +                     while (page < nand_page_per_block) {
> > +                             int curr_page = nand_page_per_block * block + page;
> > +
> > +                             if (mxs_read_page_ecc(mtd, page_buf, curr_page) < 0) {
> >                                       free(page_buf);
> > -                                     return -ENOMEM;
> > +                                     return -EIO;
> >                               }
> > +
> > +                             if (size > (mtd->writesize - page_offset))
> > +                                     sz = (mtd->writesize - page_offset);
> > +                             else
> > +                                     sz = size;
> > +
> > +                             memcpy(dst, page_buf + page_offset, sz);
> > +                             dst += sz;
> > +                             size -= sz;
> > +                             page_offset = 0;
> > +                             page++;
> >                       }
> > +
> > +                     page = 0;
> > +             } else {
> > +                     lastblock++;
> >               }
> > +
> > +             block++;
> >       }
> >
> >       free(page_buf);
> > @@ -294,6 +289,19 @@ void nand_deselect(void)
> >
> >  u32 nand_spl_adjust_offset(u32 sector, u32 offs)
> >  {
> > -     /* Handle the offset adjust in nand_spl_load_image,*/
> > +     unsigned int block, lastblock;
> > +
> > +     block = sector / mtd->erasesize;
> > +     lastblock = (sector + offs) / mtd->erasesize;
> > +
> > +     while (block <= lastblock) {
> > +             if (is_badblock(mtd, block * mtd->erasesize, 1)) {
> > +                     offs += mtd->erasesize;
> > +                     lastblock++;
> > +             }
> > +
> > +             block++;
> > +     }
> > +
> >       return offs;
> >  }
> > --
> > 2.25.1
> >



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

[PATCH V2 3/4] arm: mach-imx: cmd_nandbcb fix bad block handling

[Michael Trimarchi]

The badblock should be skipped properly in reading and writing.
Fix the logic. The bcb struct is written, skipping the bad block,
so we need to read using the same logic. This was tested create
bad block in the area and then flash it and read it back.

Cc: Han Xu <han.xu@nxp.com>
Cc: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
V1->V2:
	- Adjust the commit message
	- Add Cc Han Xu and Fabio
	- move out from RFC
---
 arch/arm/mach-imx/cmd_nandbcb.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-imx/cmd_nandbcb.c b/arch/arm/mach-imx/cmd_nandbcb.c
index f119e9f88d..c54f52b343 100644
--- a/arch/arm/mach-imx/cmd_nandbcb.c
+++ b/arch/arm/mach-imx/cmd_nandbcb.c
@@ -506,10 +506,6 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
 	int ret = 0;
 
 	mtd = boot_cfg->mtd;
-	if (mtd_block_isbad(mtd, off)) {
-		printf("Block %d is bad, skipped\n", (int)CONV_TO_BLOCKS(off));
-		return 1;
-	}
 
 	fcb_raw_page = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
 	if (!fcb_raw_page) {
@@ -530,7 +526,7 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
 		else if (plat_config.misc_flags & FCB_ENCODE_BCH_40b)
 			mxs_nand_mode_fcb_40bit(mtd);
 
-		ret = nand_read(mtd, off, &size, (u_char *)fcb);
+		ret = nand_read_skip_bad(mtd, off, &size, NULL, mtd->size, (u_char *)fcb);
 
 		/* switch BCH back */
 		mxs_nand_mode_normal(mtd);
@@ -617,6 +613,7 @@ static int write_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb)
 	for (i = 0; i < g_boot_search_count; i++) {
 		if (mtd_block_isbad(mtd, off)) {
 			printf("Block %d is bad, skipped\n", i);
+			off += mtd->erasesize;
 			continue;
 		}
 
@@ -676,20 +673,15 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
 		      void *dbbt_data_page, loff_t off)
 {
 	size_t size;
+	size_t actual_size;
 	struct mtd_info *mtd;
 	loff_t to;
 	int ret;
 
 	mtd = boot_cfg->mtd;
 
-	if (mtd_block_isbad(mtd, off)) {
-		printf("Block %d is bad, skipped\n",
-		       (int)CONV_TO_BLOCKS(off));
-		return 1;
-	}
-
 	size = sizeof(struct dbbt_block);
-	ret = nand_read(mtd, off, &size, (u_char *)dbbt);
+	ret = nand_read_skip_bad(mtd, off, &size, &actual_size, mtd->size, (u_char *)dbbt);
 	printf("NAND DBBT read from 0x%llx offset 0x%zx read: %s\n",
 	       off, size, ret ? "ERROR" : "OK");
 	if (ret)
@@ -697,9 +689,9 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
 
 	/* dbbtpages == 0 if no bad blocks */
 	if (dbbt->dbbtpages > 0) {
-		to = off + 4 * mtd->writesize;
+		to = off + 4 * mtd->writesize + actual_size - size;
 		size = mtd->writesize;
-		ret = nand_read(mtd, to, &size, dbbt_data_page);
+		ret = nand_read_skip_bad(mtd, to, &size, NULL, mtd->size, dbbt_data_page);
 		printf("DBBT data read from 0x%llx offset 0x%zx read: %s\n",
 		       to, size, ret ? "ERROR" : "OK");
 
@@ -729,6 +721,7 @@ static int write_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
 		if (mtd_block_isbad(mtd, off)) {
 			printf("Block %d is bad, skipped\n",
 			       (int)(i + CONV_TO_BLOCKS(off)));
+			off += mtd->erasesize;
 			continue;
 		}
 
-- 
2.25.1

Re: [PATCH V2 3/4] arm: mach-imx: cmd_nandbcb fix bad block handling

[Han Xu]

On 22/04/27 07:50AM, Michael Trimarchi wrote:
> The badblock should be skipped properly in reading and writing.
> Fix the logic. The bcb struct is written, skipping the bad block,
> so we need to read using the same logic. This was tested create
> bad block in the area and then flash it and read it back.
> 
> Cc: Han Xu <han.xu@nxp.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Acked-by: Han Xu <han.xu@nxp.com>

> ---
> V1->V2:
> 	- Adjust the commit message
> 	- Add Cc Han Xu and Fabio
> 	- move out from RFC
> ---
>  arch/arm/mach-imx/cmd_nandbcb.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/cmd_nandbcb.c b/arch/arm/mach-imx/cmd_nandbcb.c
> index f119e9f88d..c54f52b343 100644
> --- a/arch/arm/mach-imx/cmd_nandbcb.c
> +++ b/arch/arm/mach-imx/cmd_nandbcb.c
> @@ -506,10 +506,6 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
>  	int ret = 0;
>  
>  	mtd = boot_cfg->mtd;
> -	if (mtd_block_isbad(mtd, off)) {
> -		printf("Block %d is bad, skipped\n", (int)CONV_TO_BLOCKS(off));
> -		return 1;
> -	}
>  
>  	fcb_raw_page = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
>  	if (!fcb_raw_page) {
> @@ -530,7 +526,7 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
>  		else if (plat_config.misc_flags & FCB_ENCODE_BCH_40b)
>  			mxs_nand_mode_fcb_40bit(mtd);
>  
> -		ret = nand_read(mtd, off, &size, (u_char *)fcb);
> +		ret = nand_read_skip_bad(mtd, off, &size, NULL, mtd->size, (u_char *)fcb);
>  
>  		/* switch BCH back */
>  		mxs_nand_mode_normal(mtd);
> @@ -617,6 +613,7 @@ static int write_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb)
>  	for (i = 0; i < g_boot_search_count; i++) {
>  		if (mtd_block_isbad(mtd, off)) {
>  			printf("Block %d is bad, skipped\n", i);
> +			off += mtd->erasesize;
>  			continue;
>  		}
>  
> @@ -676,20 +673,15 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
>  		      void *dbbt_data_page, loff_t off)
>  {
>  	size_t size;
> +	size_t actual_size;
>  	struct mtd_info *mtd;
>  	loff_t to;
>  	int ret;
>  
>  	mtd = boot_cfg->mtd;
>  
> -	if (mtd_block_isbad(mtd, off)) {
> -		printf("Block %d is bad, skipped\n",
> -		       (int)CONV_TO_BLOCKS(off));
> -		return 1;
> -	}
> -
>  	size = sizeof(struct dbbt_block);
> -	ret = nand_read(mtd, off, &size, (u_char *)dbbt);
> +	ret = nand_read_skip_bad(mtd, off, &size, &actual_size, mtd->size, (u_char *)dbbt);
>  	printf("NAND DBBT read from 0x%llx offset 0x%zx read: %s\n",
>  	       off, size, ret ? "ERROR" : "OK");
>  	if (ret)
> @@ -697,9 +689,9 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
>  
>  	/* dbbtpages == 0 if no bad blocks */
>  	if (dbbt->dbbtpages > 0) {
> -		to = off + 4 * mtd->writesize;
> +		to = off + 4 * mtd->writesize + actual_size - size;
>  		size = mtd->writesize;
> -		ret = nand_read(mtd, to, &size, dbbt_data_page);
> +		ret = nand_read_skip_bad(mtd, to, &size, NULL, mtd->size, dbbt_data_page);
>  		printf("DBBT data read from 0x%llx offset 0x%zx read: %s\n",
>  		       to, size, ret ? "ERROR" : "OK");
>  
> @@ -729,6 +721,7 @@ static int write_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
>  		if (mtd_block_isbad(mtd, off)) {
>  			printf("Block %d is bad, skipped\n",
>  			       (int)(i + CONV_TO_BLOCKS(off)));
> +			off += mtd->erasesize;
>  			continue;
>  		}
>  
> -- 
> 2.25.1
> 

Re: [PATCH V2 3/4] arm: mach-imx: cmd_nandbcb fix bad block handling

[Michael Nazzareno Trimarchi]

Hi Tim

Do you have an alternative board to test?

Michael

On Fri, May 6, 2022 at 4:41 PM Han Xu <han.xu@nxp.com> wrote:
>
> On 22/04/27 07:50AM, Michael Trimarchi wrote:
> > The badblock should be skipped properly in reading and writing.
> > Fix the logic. The bcb struct is written, skipping the bad block,
> > so we need to read using the same logic. This was tested create
> > bad block in the area and then flash it and read it back.
> >
> > Cc: Han Xu <han.xu@nxp.com>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>
> Acked-by: Han Xu <han.xu@nxp.com>
>
> > ---
> > V1->V2:
> >       - Adjust the commit message
> >       - Add Cc Han Xu and Fabio
> >       - move out from RFC
> > ---
> >  arch/arm/mach-imx/cmd_nandbcb.c | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/cmd_nandbcb.c b/arch/arm/mach-imx/cmd_nandbcb.c
> > index f119e9f88d..c54f52b343 100644
> > --- a/arch/arm/mach-imx/cmd_nandbcb.c
> > +++ b/arch/arm/mach-imx/cmd_nandbcb.c
> > @@ -506,10 +506,6 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
> >       int ret = 0;
> >
> >       mtd = boot_cfg->mtd;
> > -     if (mtd_block_isbad(mtd, off)) {
> > -             printf("Block %d is bad, skipped\n", (int)CONV_TO_BLOCKS(off));
> > -             return 1;
> > -     }
> >
> >       fcb_raw_page = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> >       if (!fcb_raw_page) {
> > @@ -530,7 +526,7 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
> >               else if (plat_config.misc_flags & FCB_ENCODE_BCH_40b)
> >                       mxs_nand_mode_fcb_40bit(mtd);
> >
> > -             ret = nand_read(mtd, off, &size, (u_char *)fcb);
> > +             ret = nand_read_skip_bad(mtd, off, &size, NULL, mtd->size, (u_char *)fcb);
> >
> >               /* switch BCH back */
> >               mxs_nand_mode_normal(mtd);
> > @@ -617,6 +613,7 @@ static int write_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb)
> >       for (i = 0; i < g_boot_search_count; i++) {
> >               if (mtd_block_isbad(mtd, off)) {
> >                       printf("Block %d is bad, skipped\n", i);
> > +                     off += mtd->erasesize;
> >                       continue;
> >               }
> >
> > @@ -676,20 +673,15 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> >                     void *dbbt_data_page, loff_t off)
> >  {
> >       size_t size;
> > +     size_t actual_size;
> >       struct mtd_info *mtd;
> >       loff_t to;
> >       int ret;
> >
> >       mtd = boot_cfg->mtd;
> >
> > -     if (mtd_block_isbad(mtd, off)) {
> > -             printf("Block %d is bad, skipped\n",
> > -                    (int)CONV_TO_BLOCKS(off));
> > -             return 1;
> > -     }
> > -
> >       size = sizeof(struct dbbt_block);
> > -     ret = nand_read(mtd, off, &size, (u_char *)dbbt);
> > +     ret = nand_read_skip_bad(mtd, off, &size, &actual_size, mtd->size, (u_char *)dbbt);
> >       printf("NAND DBBT read from 0x%llx offset 0x%zx read: %s\n",
> >              off, size, ret ? "ERROR" : "OK");
> >       if (ret)
> > @@ -697,9 +689,9 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> >
> >       /* dbbtpages == 0 if no bad blocks */
> >       if (dbbt->dbbtpages > 0) {
> > -             to = off + 4 * mtd->writesize;
> > +             to = off + 4 * mtd->writesize + actual_size - size;
> >               size = mtd->writesize;
> > -             ret = nand_read(mtd, to, &size, dbbt_data_page);
> > +             ret = nand_read_skip_bad(mtd, to, &size, NULL, mtd->size, dbbt_data_page);
> >               printf("DBBT data read from 0x%llx offset 0x%zx read: %s\n",
> >                      to, size, ret ? "ERROR" : "OK");
> >
> > @@ -729,6 +721,7 @@ static int write_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> >               if (mtd_block_isbad(mtd, off)) {
> >                       printf("Block %d is bad, skipped\n",
> >                              (int)(i + CONV_TO_BLOCKS(off)));
> > +                     off += mtd->erasesize;
> >                       continue;
> >               }
> >
> > --
> > 2.25.1
> >



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH V2 3/4] arm: mach-imx: cmd_nandbcb fix bad block handling

[Tim Harvey]

On Tue, May 10, 2022 at 11:49 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi Tim
>
> Do you have an alternative board to test?
>

Michael,

Yes sorry... I tested the series and had the reply in my draft folder.
I sent it just now in response to the cover letter.

Best regards,

Tim

> Michael
>
> On Fri, May 6, 2022 at 4:41 PM Han Xu <han.xu@nxp.com> wrote:
> >
> > On 22/04/27 07:50AM, Michael Trimarchi wrote:
> > > The badblock should be skipped properly in reading and writing.
> > > Fix the logic. The bcb struct is written, skipping the bad block,
> > > so we need to read using the same logic. This was tested create
> > > bad block in the area and then flash it and read it back.
> > >
> > > Cc: Han Xu <han.xu@nxp.com>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> >
> > Acked-by: Han Xu <han.xu@nxp.com>
> >
> > > ---
> > > V1->V2:
> > >       - Adjust the commit message
> > >       - Add Cc Han Xu and Fabio
> > >       - move out from RFC
> > > ---
> > >  arch/arm/mach-imx/cmd_nandbcb.c | 21 +++++++--------------
> > >  1 file changed, 7 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-imx/cmd_nandbcb.c b/arch/arm/mach-imx/cmd_nandbcb.c
> > > index f119e9f88d..c54f52b343 100644
> > > --- a/arch/arm/mach-imx/cmd_nandbcb.c
> > > +++ b/arch/arm/mach-imx/cmd_nandbcb.c
> > > @@ -506,10 +506,6 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
> > >       int ret = 0;
> > >
> > >       mtd = boot_cfg->mtd;
> > > -     if (mtd_block_isbad(mtd, off)) {
> > > -             printf("Block %d is bad, skipped\n", (int)CONV_TO_BLOCKS(off));
> > > -             return 1;
> > > -     }
> > >
> > >       fcb_raw_page = kzalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
> > >       if (!fcb_raw_page) {
> > > @@ -530,7 +526,7 @@ static int read_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb,
> > >               else if (plat_config.misc_flags & FCB_ENCODE_BCH_40b)
> > >                       mxs_nand_mode_fcb_40bit(mtd);
> > >
> > > -             ret = nand_read(mtd, off, &size, (u_char *)fcb);
> > > +             ret = nand_read_skip_bad(mtd, off, &size, NULL, mtd->size, (u_char *)fcb);
> > >
> > >               /* switch BCH back */
> > >               mxs_nand_mode_normal(mtd);
> > > @@ -617,6 +613,7 @@ static int write_fcb(struct boot_config *boot_cfg, struct fcb_block *fcb)
> > >       for (i = 0; i < g_boot_search_count; i++) {
> > >               if (mtd_block_isbad(mtd, off)) {
> > >                       printf("Block %d is bad, skipped\n", i);
> > > +                     off += mtd->erasesize;
> > >                       continue;
> > >               }
> > >
> > > @@ -676,20 +673,15 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> > >                     void *dbbt_data_page, loff_t off)
> > >  {
> > >       size_t size;
> > > +     size_t actual_size;
> > >       struct mtd_info *mtd;
> > >       loff_t to;
> > >       int ret;
> > >
> > >       mtd = boot_cfg->mtd;
> > >
> > > -     if (mtd_block_isbad(mtd, off)) {
> > > -             printf("Block %d is bad, skipped\n",
> > > -                    (int)CONV_TO_BLOCKS(off));
> > > -             return 1;
> > > -     }
> > > -
> > >       size = sizeof(struct dbbt_block);
> > > -     ret = nand_read(mtd, off, &size, (u_char *)dbbt);
> > > +     ret = nand_read_skip_bad(mtd, off, &size, &actual_size, mtd->size, (u_char *)dbbt);
> > >       printf("NAND DBBT read from 0x%llx offset 0x%zx read: %s\n",
> > >              off, size, ret ? "ERROR" : "OK");
> > >       if (ret)
> > > @@ -697,9 +689,9 @@ static int read_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> > >
> > >       /* dbbtpages == 0 if no bad blocks */
> > >       if (dbbt->dbbtpages > 0) {
> > > -             to = off + 4 * mtd->writesize;
> > > +             to = off + 4 * mtd->writesize + actual_size - size;
> > >               size = mtd->writesize;
> > > -             ret = nand_read(mtd, to, &size, dbbt_data_page);
> > > +             ret = nand_read_skip_bad(mtd, to, &size, NULL, mtd->size, dbbt_data_page);
> > >               printf("DBBT data read from 0x%llx offset 0x%zx read: %s\n",
> > >                      to, size, ret ? "ERROR" : "OK");
> > >
> > > @@ -729,6 +721,7 @@ static int write_dbbt(struct boot_config *boot_cfg, struct dbbt_block *dbbt,
> > >               if (mtd_block_isbad(mtd, off)) {
> > >                       printf("Block %d is bad, skipped\n",
> > >                              (int)(i + CONV_TO_BLOCKS(off)));
> > > +                     off += mtd->erasesize;
> > >                       continue;
> > >               }
> > >
> > > --
> > > 2.25.1
> > >
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

[PATCH 4/4] spl: spl_nand: Fix bad block handling in fitImage

[Michael Trimarchi]

If the fitImage has some bad block in fit image area, the
offset must be recalulcated. This should be done always.
After implementing it in mxs now is possible to call the function
even for that platform.

Cc: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
V1->V2:
	- move out from RFC
---
 common/spl/spl_nand.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index fc61b447a5..82a10ffa63 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -43,15 +43,12 @@ static ulong spl_nand_fit_read(struct spl_load_info *load, ulong offs,
 			       ulong size, void *dst)
 {
 	int err;
-#ifdef CONFIG_SYS_NAND_BLOCK_SIZE
 	ulong sector;
 
 	sector = *(int *)load->priv;
-	offs = sector + nand_spl_adjust_offset(sector, offs - sector);
-#else
 	offs *= load->bl_len;
 	size *= load->bl_len;
-#endif
+	offs = sector + nand_spl_adjust_offset(sector, offs - sector);
 	err = nand_spl_load_image(offs, size, dst);
 	if (err)
 		return 0;
-- 
2.25.1

Re: [PATCH 4/4] spl: spl_nand: Fix bad block handling in fitImage

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Wed, Apr 27, 2022 at 07:50:25AM +0200, Michael Trimarchi wrote:

> If the fitImage has some bad block in fit image area, the
> offset must be recalulcated. This should be done always.
> After implementing it in mxs now is possible to call the function
> even for that platform.
> 
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/4] spl: spl_nand: Fix bad block handling in fitImage

[Han Xu]

On 22/04/27 07:50AM, Michael Trimarchi wrote:
> If the fitImage has some bad block in fit image area, the
> offset must be recalulcated. This should be done always.
> After implementing it in mxs now is possible to call the function
> even for that platform.
> 
> Cc: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>

Acked-by: Han Xu <han.xu@nxp.com>

> ---
> V1->V2:
> 	- move out from RFC
> ---
>  common/spl/spl_nand.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index fc61b447a5..82a10ffa63 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -43,15 +43,12 @@ static ulong spl_nand_fit_read(struct spl_load_info *load, ulong offs,
>  			       ulong size, void *dst)
>  {
>  	int err;
> -#ifdef CONFIG_SYS_NAND_BLOCK_SIZE
>  	ulong sector;
>  
>  	sector = *(int *)load->priv;
> -	offs = sector + nand_spl_adjust_offset(sector, offs - sector);
> -#else
>  	offs *= load->bl_len;
>  	size *= load->bl_len;
> -#endif
> +	offs = sector + nand_spl_adjust_offset(sector, offs - sector);
>  	err = nand_spl_load_image(offs, size, dst);
>  	if (err)
>  		return 0;
> -- 
> 2.25.1
> 

Re: [PATCH V2 0/4] MXS nand fixes in SPL

[Tim Harvey]

On Tue, Apr 26, 2022 at 10:50 PM Michael Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Those patches come after some testing of failing in factory on some
> unit. We found out that the bootrom imx loader was not able to handling
> badblock. This can be a limit of the implementation right now in imx8mn.
> Anyway not all the imx platform has the support of this loader. I found
> some problems on the implementation so I have fixed it up according the
> experience of Sitara. I tested only using a Fit Image as a flash
> container. This version add in the series the fix of cmd_nandbcb and
> the fix of spl_nand load. I can imagine that a lot of boards and users
> are affected. I have started to backport this changes in some older
> uboot and adapt it
>
> Michael Trimarchi (4):
>   nand: raw: mxs_nand: Fix specific hook registration
>   mtd: nand: mxs_nand_spl: Fix bad block skipping
>   arm: mach-imx: cmd_nandbcb fix bad block handling
>   spl: spl_nand: Fix bad block handling in fitImage
>
>  arch/arm/mach-imx/cmd_nandbcb.c     | 21 +++----
>  common/spl/spl_nand.c               |  5 +-
>  drivers/mtd/nand/raw/mxs_nand.c     | 32 +++++-----
>  drivers/mtd/nand/raw/mxs_nand_spl.c | 90 ++++++++++++++++-------------
>  4 files changed, 73 insertions(+), 75 deletions(-)
>
> --
> 2.25.1
>

Tested By: Tim Harvey <tharvey@gateworks.com>
#gwventana_nand_defconfig (imx6qdl NAND)

Best Regards,

Tim