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

[Miquel Raynal]

Hi Patrick,

Patrick DELAUNAY <patrick.delaunay at st.com> wrote on Fri, 27 Sep 2019
11:23:09 +0000:

> 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.

I understand, I'm fine with the cmd/mtd.c to be changed only.

Thanks,
Miquèl