[U-Boot] [PATCH] cmd: mtd: solve bad block support in erase command

[Patrick DELAUNAY]

Hi Miquel

> From: Miquel Raynal <miquel.raynal at bootlin.com>
> Sent: jeudi 26 septembre 2019 11:43
> 
> Hi Patrick,
> 
> Patrick DELAUNAY <patrick.delaunay at st.com> wrote on Thu, 26 Sep 2019
> 09:31:46 +0000:
> 
> > Hi Stefan,
> >
> > > From: Stefan Roese <sr at denx.de>
> > > Sent: vendredi 20 septembre 2019 11:20
> > >
> > > Hi Patrick,
> > >
> > > On 20.09.19 09:20, Patrick Delaunay wrote:
> > > > This patch modify the loop in mtd erase command to erase one by
> > > > one the blocks in the requested area.
> > > >
> > > > It solves issue on "mtd erase" command on nand with existing bad
> > > > block, the command is interrupted on the first bad block with the trace:
> > > > 	"Skipping bad block at 0xffffffffffffffff"
> > > >
> > > > In MTD driver (nand/raw), when a bad block is present on the MTD
> > > > device, the erase_op.fail_addr is not updated and we have the
> > > > initial value MTD_FAIL_ADDR_UNKNOWN = (ULL)-1.
> > >
> > > So here is the difference? I remember testing this on a board with
> > > SPI NAND and here it worked correctly. But your test case is with RAW
> NAND?
> >
> > Yes with RAW nand.
> >
> > it is the difference the U-Boot code, for SPI nan use:
> > 	int nanddev_mtd_erase()
> >
> > the fail address is always updated
> > 	=> einfo->fail_addr = nanddev_pos_to_offs(nand, &pos);
> >
> >
> > > Do you have a change to also test this on a board with SPI NAND?
> >
> > I do the test  a SPI-NAND today.
> >
> > The mtd erase command was functional on SPI-ANND before my patch :
> > I create 2 bad block manually and they are correctly skipped.
> >
> > STM32MP> mtd list
> > List of MTD devices:
> > * spi-nand0
> >   - device: spi-nand at 0
> >   - parent: qspi at 58003000
> >   - driver: spi_nand
> >   - type: NAND flash
> >   - block size: 0x20000 bytes
> >   - min I/O: 0x800 bytes
> >   - OOB size: 128 bytes
> >   - OOB available: 62 bytes
> >   - 0x000000000000-0x000010000000 : "spi-nand0"
> > 	  - 0x000000000000-0x000000200000 : "fsbl"
> > 	  - 0x000000200000-0x000000400000 : "ssbl1"
> > 	  - 0x000000400000-0x000000600000 : "ssbl2"
> > 	  - 0x000000600000-0x000010000000 : "UBI"
> >
> > STM32MP> mtd erase spi-nand0 0x00000000 0x10000000
> > Erasing 0x00000000 ... 0x0fffffff (2048 eraseblock(s))
> > 0x0fd00000: bad block
> > 0x0fd20000: bad block
> > attempt to erase a bad/reserved block @fd00000 Skipping bad block at
> > 0x0fd00000 attempt to erase a bad/reserved block @fd20000 Skipping bad
> > block at 0x0fd20000
> > 0x0fd00000: bad block
> > 0x0fd20000: bad block
> >
> >
> > > Thanks,
> > > Stefan
> > >
> >
> > What it is the better solution for you ?
> >
> >  update the MTD command (my patch) or allign the behavior of the 2 MTD
> > devices
> > - MTD RAW NAND (nand_base.c:: nand_erase_nand)
> > - MTD SPI NAND (core.c:: nanddev_mtd_erase)
> 
> Do you think it is easy to make use of nanddev_mtd_erase() from the raw NAND
> core? It is probably a little bit more elegant (and efficient) to do all in one go than
> iterating over each block (while there is a helper in the core to do that).


Yes, I agree: it will be more elegant.

But,  I am not comfortable with MTD Raw NAND framework.

Based on a quick check between Linux Kernel 5.3 and U-Boot, it seems that U-Boot
Raw NAND framework is aligned with Kernel 4.19 Raw NAND framework.
To be able to use nanddev_mtd_erase API, we should backport Raw NAND framework
from Kernel 5.3 because nanddev_mtd_erase can be used only if memorg structure
is properly set (has been done on Kernel 5.2).

I have not checked all potential impacts to use this API, but a backport form Kernel
Raw NAND framework is needed in U-Boot in a first step.

As  I am not comfortable with MTD frameworks, I think that my patch could be currently
applied to solve this issue, and in a second step, when a MTD expert will backport the
framework, it could be removed.

PS: A other solution with minimize the impacts in MTD, it is to change
      only nand_base.c:nand_erase_nand(), to update the fail_addr:

----------------------- drivers/mtd/nand/raw/nand_base.c ----------------------- index aba8ac019d..50542a2b9a 100644 @@ -3554,6 +3554,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 			pr_warn("%s: attempt to erase a bad block at page 0x%08x\n",
 				    __func__, page);
 			instr->state = MTD_ERASE_FAILED;
+			instr->fail_addr =
+				((loff_t)page << chip->page_shift);
 			goto erase_exit;
 		}

But as it is also a common MTD part with Linux kernel so I continue to prefer
my patch on U-Boot only code.

> 
> Thanks,
> Miquèl

Regards
Patrick