[U-Boot] [PATCH 6/6] EA20: do not use subpage write for NAND

[Ben Gardiner]

Hi Stefano,

Thanks for sharing this patch -- I have been using the "-O 2048" (VID
header offset) option to prevent subpages here.

On Sat, Apr 9, 2011 at 2:05 PM, Stefano Babic <sbabic at denx.de> wrote:
> The NAND controller does not support subpage accessing. This is
> not used at all for MLC NAND, but it is set for SLC NAND. UBI tries
> to access to subpages, and because it fails, it starts "torture tests"

"tries to access subpages" is maybe a little too vague; I think Jon
Povey described the problem quite succinctly:

"the problem is that the ECC generated for an all-FFs page is not
all-FFs, and subpage writes are handled by
nand_do_write_ops() by writing a full page with FFs in the unset data
areas." [1]

> on the whole page that are always successful, making an endless loop
> with the UBI background task taking most CPU time.
> On the console, the issue is recognized by the messages:
>
> UBI error: ubi_io_read: error -74 (ECC error) while reading 512 bytes from
> PEB 37:512, read 512 bytes
> UBI: run torture test for PEB 37
> UBI: PEB 37 passed torture test, do not mark it a bad
>
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> CC: Ben Gardiner<bengardiner at nanometrics.ca>
> CC: Sandeep Paulraj <s-paulraj at ti.com>
> CC: Scott Wood <scottwood at freescale.com>

Is it worth mentioning that this will create UBI partitions that are
not usable in Linux with a similar patch or a VID header offset
workaround?

> ---
>  README                          |    4 ++++
>  drivers/mtd/nand/davinci_nand.c |    3 +++
>  include/configs/ea20.h          |    1 +
>  include/linux/mtd/nand.h        |    3 ++-
>  4 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/README b/README
> index 21cd71b..8d664eb 100644
> --- a/README
> +++ b/README
> @@ -2907,6 +2907,10 @@ Low Level (hardware related) configuration options:
>                that is executed before the actual U-Boot. E.g. when
>                compiling a NAND SPL.
>
> +- CONFIG_SYS_NAND_NO_SUBPAGE
> +               Some drivers (davinci) do not support access to NAND subpage.
> +
> +
>  Building the Software:
>  ======================
>
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index d41579c..f6c7d09 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand)
>  #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
>        nand->options     |= NAND_USE_FLASH_BBT;
>  #endif
> +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE
> +       nand->options     |= NAND_NO_SUBPAGE_WRITE;
> +#endif
>  #ifdef CONFIG_SYS_NAND_HW_ECC
>        nand->ecc.mode = NAND_ECC_HW;
>        nand->ecc.size = 512;
> diff --git a/include/configs/ea20.h b/include/configs/ea20.h
> index 1843dae..9e5dda4 100644
> --- a/include/configs/ea20.h
> +++ b/include/configs/ea20.h
> @@ -171,6 +171,7 @@
>
>  #define CONFIG_NAND_DAVINCI
>  #define        CONFIG_SYS_NAND_PAGE_2K
> +#define CONFIG_SYS_NAND_NO_SUBPAGE
>  #define CONFIG_SYS_NAND_CS             2
>  #define CONFIG_SYS_NAND_BASE           DAVINCI_ASYNC_EMIF_DATA_CE2_BASE
>  #undef CONFIG_SYS_NAND_HW_ECC
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 987a2ec..215e781 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -193,7 +193,8 @@ typedef enum {
>                                        && (chip->page_shift > 9))
>
>  /* Mask to zero out the chip options, which come from the id table */
> -#define NAND_CHIPOPTIONS_MSK   (0x0000ffff & ~NAND_NO_AUTOINCR)
> +#define NAND_CHIPOPTIONS_MSK   (0x0000ffff & ~NAND_NO_AUTOINCR & \
> +                               ~NAND_NO_SUBPAGE_WRITE)

First let me say that I would be happy to have a fix for this merged
-- so I think the change is "good enough" since it makes UBI work
without specifying a VID header offset equal to the NAND page size.
What follows is more topical musings that any particular criticisms
that should be considered blockers of your patch.

I'm wondering about the long-term path for davinci NAND and subpage writes...

But the sentiment I've heard about adding an exception to
NAND_CHIPOPTIONS_MSK as above is that we are mixing features of the
NAND chip and features of the NAND controller (If you have a modern
SLC NAND then your chip probably supports subpage writes whereas it is
the controller that needs to be limited).

What's more is that the davinci nand controller could do subpage
writing if the writing operation were informed of the extents of the
subpage being written instead of being handed a buffer with 0xFF in
the non-target page areas. Which, I believe is Artem's primary
motivation for the introduction of nand_write_subpage() to the davinci
NAND controller driver [2].

So if the nand_write_subpage family of functions was introduced to the
Linux kernel instead of adding another exception to
NAND_CHIPOPTIONS_MSK then we would have 3 ways to use UBI on davinci
NAND:
1) no patch: VID offset <page size> required
2) chip NAND_NO_SUBPAGE_WRITE patch
3) subpage writes support with nand_write_subpage()

systems with 2) or 3) could always use UBI as in 1) and a UBI volume
made with 2) would need to be attached as in 1) on systems with 2) or
3) ; but a UBI volume made with 3) could not be used on systems with
1) or 2) which means that we could not make use of the efficiency
benefits of nand_write_subpage() if/when it is available on systems
which need access to UBI from U-boot.

I've cc'd Artem and Jon in the hopes that they can correct any of my
leaps above -- what I'd really like is to understand a migration
strategy to actually using subpage writes with davinci NAND in the
future.

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.linux.davinci/19853
[2] http://article.gmane.org/gmane.linux.drivers.mtd/31581

---
Nanometrics Inc.
http://www.nanometrics.ca