Hi Han
Any update?
Michael
On Tue, May 3, 2022 at 7:14 AM Michael Nazzareno Trimarchi
<mich...@amarulasolutions.com> wrote:
>
> Hi Han
>
> On Mon, May 2, 2022 at 11:32 PM Han Xu <han...@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
> > > <mich...@amarulasolutions.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Thu, Apr 28, 2022 at 2:27 AM Han Xu <han...@nxp.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Michael Trimarchi <mich...@amarulasolutions.com>
> > > > > > Sent: Wednesday, April 27, 2022 12:50 AM
> > > > > > To: Han Xu <han...@nxp.com>; U-Boot-Denx <u-boot@lists.denx.de>
> > > > > > Cc: Ye Li <ye...@nxp.com>; Stefano Babic <sba...@denx.de>; Miquel
> > > > > > Raynal
> > > > > > <miquel.ray...@bootlin.com>; Fabio Estevam <feste...@denx.de>; Dario
> > > > > > Binacchi <dario.binac...@amarulasolutions.com>; Sean Anderson
> > > > > > <sean.ander...@seco.com>; linux-ker...@amarulasolutions.com; Jagan
> > > > > > Teki
> > > > > > <ja...@amarulasolutions.com>; Ariel D'Alessandro
> > > > > > <ariel.dalessan...@collabora.com>; Fabio Estevam
> > > > > > <feste...@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...@nxp.com>
> > > > > > Cc: Fabio Estevam <feste...@gmail.com>
> > > > > > Signed-off-by: Michael Trimarchi <mich...@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
> > > > mich...@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > i...@amarulasolutions.com
> > > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&reserved=0
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > mich...@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > i...@amarulasolutions.com
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.amarulasolutions.com%2F&data=05%7C01%7Chan.xu%40nxp.com%7C81091e4443d745ce460708da2b3cec6a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637869837927785927%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=I5jvEx5qmQLzO8QrQT9aMYttDPZhdvbGi0Z2lk77bio%3D&reserved=0
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> mich...@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> i...@amarulasolutions.com
> www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
mich...@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
i...@amarulasolutions.com
www.amarulasolutions.com
