Re: [PATCH] mtd: spinand: Wait after erase in spinand_markbad

[Boris Brezillon <bbrezillon@kernel.org>]

On Sat, 5 Jan 2019 22:01:44 -0800
Emil Lenngren <emil.lenngren@gmail.com> wrote:

> Hi,
> 
> Den lör 5 jan. 2019 kl 05:59 skrev Boris Brezillon <bbrezillon@kernel.org>:
> >
> > On Fri, 21 Dec 2018 12:58:14 +0100
> > Emil Lenngren <emil.lenngren@gmail.com> wrote:
> >  
> > > SPI NAND flashes don't accept new commands while an erase is ongoing.
> > > Make sure to wait until the device is ready before writing the marker.
> > >
> > > Just as with the erase op, no error check is performed since we want
> > > to continue writing the marker even if the erase fails.
> > >
> > > Signed-off-by: Emil Lenngren <emil.lenngren@gmail.com>
> > > ---
> > >  drivers/mtd/nand/spi/core.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > index 479c2f2cf1..c2724d34e6 100644
> > > --- a/drivers/mtd/nand/spi/core.c
> > > +++ b/drivers/mtd/nand/spi/core.c
> > > @@ -685,6 +685,8 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos)
> > >
> > >       spinand_erase_op(spinand, pos);
> > >
> > > +     spinand_wait(spinand, NULL);
> > > +  
> >
> > After thinking a bit more about it, I think we should simply write the
> > BBM and skip the erase operation. Marking a block bad is just about
> > writing 0 to the first 2 bytes of the OOB area, and we don't need the
> > block to be erased to do that.
> >  
> 
> I compared with the raw and implementation in
> nand_block_markbad_lowlevel, it also erases first, ignoring a
> potential error.

Yes, and I think this implementation was inspired by the rawnand one,
but I'm not sure the rawnand implem is correct.

> 
> On the other hand, a common spi flash chip MX35LF1GE4AB states in the
> datasheet that it's not recommended to erase a bad block, but no
> reason why.

Because the erase might succeed and reset the BBM to 0xff, thus marking
the block good even if it's unreliable.

> At the same time, it's generally disallowed to write the
> same page twice...

That's only if you care about the data you write to the page. Marking a
block bad is just about setting the BBM to 0x0, which should always work
even if the page you're writing to has already been written, simply
because a 1 -> 0 cell transition does not require an erase (only a 0 ->
1 transition does).

> 
> But in the end I also think the best way is to avoid the erase
> operation and simply write 0 0 as a raw write.

I don't know why the erase op was added to
nand_block_markbad_lowlevel() in the first place but I don't want to
risk breaking platforms that might depend on this behavior. It's
different for SPI NAND: the subsystem has just been created and I think
we should get rid of this erase call until someone explicitly asks for
it with a good explanation on why this is needed.

> 
> > >       memset(spinand->oobbuf, 0, 2);
> > >       return spinand_write_page(spinand, &req);
> > >  }  
> >  
> 
> /Emil