[U-Boot] [PATCH v2] pcm058: fix NAND flash not using badblock table

[Harald Seiler]

Hello Stefano,

On Fri, 2018-12-07 at 19:55 +0100, Stefano Babic wrote:
> Hi Harald,
> 
> On 07/12/18 13:18, Marek Vasut wrote:
> > On 12/07/2018 01:15 PM, Harald Seiler wrote:
> > > Hello Marek,
> > 
> > Hi,
> > 
> > > On Fri, 2018-12-07 at 12:48 +0100, Marek Vasut wrote:
> > > > On 12/07/2018 10:19 AM, Harald Seiler wrote:
> > > > > Currently, U-Boot ignores the BBT stored in the last 4 blocks of NAND
> > > > > flash because the NAND_BBT_USE_FLASH flag is not set.  This leads to
> > > > > two issues:
> > > > > 
> > > > > * U-Boot silently uses a memory-only BBT which is initialized with all
> > > > >   blocks marked as good.  This means, actual bad blocks are marked good
> > > > >   and U-Boot might try writing to or reading from them.
> > > > > * The BBT in flash, which will be created once Linux boots up, is not
> > > > >   off limits for a driver ontop, like UBI.  While it does not seem to
> > > > >   consistently produce an error, sometimes UBI will fail to attach
> > > > >   because the BBT blocks obviously don't contain valid UBI data.
> > > > > 
> > > > > To fix this, this patch sets the CONFIG_SYS_NAND_USE_FLASH_BBT option,
> > > > > which is used in ./drivers/mtd/nand/raw/mxs_nand.c to decide whether
> > > > > a BBT in flash is used.
> > > > > 
> > > > > Signed-off-by: Harald Seiler <hws at denx.de>
> > > > 
> > > > V2 Changelog is missing.
> > > > 
> > > > > ---
> > > > >  include/configs/pcm058.h | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/include/configs/pcm058.h b/include/configs/pcm058.h
> > > > > index 49048c163f..b9bc08b388 100644
> > > > > --- a/include/configs/pcm058.h
> > > > > +++ b/include/configs/pcm058.h
> > > > > @@ -55,6 +55,7 @@
> > > > >  #define CONFIG_SYS_NAND_BASE		0x40000000
> > > > >  #define CONFIG_SYS_NAND_5_ADDR_CYCLE
> > > > >  #define CONFIG_SYS_NAND_ONFI_DETECTION
> > > > > +#define CONFIG_SYS_NAND_USE_FLASH_BBT
> > > > 
> > > > Shouldn't this be enabled on all boards with GPMI NAND ?
> > > > 
> > > 
> > > I looked at other boards and they all defined this config, so I
> > > assumed this was the way to go ...
> 
> Let me understand. If Isearch for CONFIG_NAND_MXS, I get 27 boards using
> this drivers, with different SOC (mx28, mx6[Dual|Quad|Solo], mx6sx,
> mx6ull). But none of them is setting  CONFIG_SYS_NAND_USE_FLASH_BBT.
> 
> When does it happen the issue ? It should happen if we create a UBI
> container and its volumes in U-Boot. If UBI is generated in linux, this
> should not happen. Is it the case or does it happen in any condition ?

Linux will write the badblock table to the last 4 blocks by default which
U-Boot ignores at the moment.  So the issue happens if you use the default
kernel config (which you pointed out below).

> I think the issue happens because there is a disalignment between kernel
> and u-boot. Kernel mainline for this board (file
> imx6qdl-phytec-phycore-som.dtsi) sets "nand-on-flash-bbt", while U-Boot
> not. That mean that this can always happen if kernel and U-Boot does not
> use the same setup for the driver.
> 
> Maybe with previous kernel versions there was no problem because Linux
> used the same setup as U-Boot.

This is exactly what I think is going on ...

> Anyway, this discourage setting CONFIG_SYS_NAND_USE_FLASH_BBT
> unconditionally for all boards with GPMI driver.
> 

I agree, it only makes sense in cases where Linux also does it this way.
Although, not doing it this way means not having any persistent badblock
table at all, which I feel is never a good idea ...

> > But yes, as far as I understand,
> > > it would make sense to have it enabled most of the time.  Although
> > > there is some code which makes this configuration from the
> > > devicetree, which might be an even better solution.
> 
> Device tree has already this option as I see in
> drivers/mtd/nand/raw/nand_base.c. The driver enforces this if no DT (as
> in this case) is used.

What do you mean by enforce? If no devicetree is used, the code does not
set any flags as far as I understand it, which also aligns with the results
of my experimentation ... (Currently bbt_options is 0)

> > Cool, if this can be fixed in a more general manner, that'd be awesome
> > and would help others too. Better yet, it'd get rid of possibly
> > hazardous duplication. Can you research it ?
> > 
> 
> Best regards,
> Stefano
> 

-- 
Harald

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-62  Fax: +49-8142-66989-80   Email: hws at denx.de