Re: [PATCH 1/2] NAND: Adding new ECC mode - NAND_ECC_HW_OOB_FIRST for TI DaVinci devices

[David Brownell <david-b@pacbell.net>]

On Wednesday 06 May 2009, nsnehaprabha@ti.com wrote:
> From: Sneha Narnakaje <nsnehaprabha@ti.com>
> 
> This patch adds new ECC mode NAND_ECC_HW_OOB_FIRST in the nand core to
> support 4-bit ECC on TI DaVinci devices with large page NAND chips.

And it seems quite straightforward too:

  (a) add new "page" parameter to all the NAND read_page methods,
  (b) define a new nand_read_page_hwecc_oob_first() method, and
      kick it in for ECC_HW_OOB_FIRST

I can't see anything going wrong with (a), though some might prefer
to see it in a separate patch.  And (b) looks obvious too, given
that design.

Comments from any MTD folk?


> This ECC mode is similar to NAND_ECC_HW, with the exception of read_page
> handle that should first read the OOB area, read the data in chunks, feed the
> ECC from OOB area to the ECC hw engine and perform any correction on the data
> as per the ECC status reported by hw engine. In order to read the OOB area
> before the data area, the read_page handle has to send READOOB command and
> thus the read_page/read_page_raw APIs are changed to pass the page argument.
> 
> Note: This patch is dependent on [patch 2.6.30-rc1] NAND: don't walk past
> end of oobfree[] from David Brownell:
> http://lists.infradead.org/pipermail/linux-mtd/2009-April/025205.html

... both of which went from Andrew to David Woodhouse, and I'd
hope they would get into the MTD queue soon.


> 
> Signed-off-by: Sneha Narnakaje <nsnehaprabha@ti.com>
> ---
>  drivers/mtd/nand/atmel_nand.c    |    2 +-
>  drivers/mtd/nand/cafe_nand.c     |    2 +-
>  drivers/mtd/nand/fsl_elbc_nand.c |    3 +-
>  drivers/mtd/nand/nand_base.c     |   72 +++++++++++++++++++++++++++++++++----
>  drivers/mtd/nand/sh_flctl.c      |    2 +-
>  include/linux/mtd/nand.h         |    5 ++-
>  6 files changed, 72 insertions(+), 14 deletions(-)
> 
> 	... snip ...
>
> 
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> 	... deletia ...
> 
> @@ -980,6 +980,49 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
>  }
>  
>  /**
> + * nand_read_page_hwecc_oob_first - [REPLACABLE] hw ecc, read oob first
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @buf:	buffer to store read data
> + *
> + * Hardware ECC for large page chips, require OOB to be read first

I'd explain this a bit here.  A key point is that unlike the
ECC_HW_SYNDROME support when multiple ECC "steps" are involved,
this doesn't clobber manufacturer bad block markings by using
an "infix ECC" scheme:  it puts ECC data *only* into the OOB.


> + */


And this is the guts of it all.  "Looks obvious":

> +static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
> +	struct nand_chip *chip, uint8_t *buf, int page)
> +{
> +	int i, eccsize = chip->ecc.size;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	uint8_t *p = buf;
> +	uint8_t *ecc_code = chip->buffers->ecccode;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> +
> +	/* Read the OOB area first */
> +	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +
> +	for (i = 0; i < chip->ecc.total; i++)
> +		ecc_code[i] = chip->oob_poi[eccpos[i]];
> +
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		int stat;
> +
> +		chip->ecc.hwctl(mtd, NAND_ECC_READ);
> +		chip->read_buf(mtd, p, eccsize);
> +		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> +
> +		stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);
> +		if (stat < 0)
> +			mtd->ecc_stats.failed++;
> +		else
> +			mtd->ecc_stats.corrected += stat;
> +	}
> +	return 0;
> +}
> +
> +/**
>   * nand_read_page_syndrome - [REPLACABLE] hardware ecc syndrom based page read
>   * @mtd:	mtd info structure
>   * @chip:	nand chip info structure
>
>	... deletia ...
>
> @@ -2678,6 +2723,17 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	case NAND_ECC_HW3_512:
>  	case NAND_ECC_HW3_256:

You should reissue the patch against current GIT code.  Those two
constants are long gone.


>  #endif
> +	case NAND_ECC_HW_OOB_FIRST:
> +		/* Similar to NAND_ECC_HW, but a separate read_page handle */
> +		if (!chip->ecc.calculate || !chip->ecc.correct ||
> +		     !chip->ecc.hwctl) {
> +			printk(KERN_WARNING "No ECC functions supplied, "
> +			       "Hardware ECC not possible\n");
> +			BUG();
> +		}
> +		if (!chip->ecc.read_page)
> +			chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> +
>  	case NAND_ECC_HW:
>  		/* Use standard hwecc read page function ? */
>  		if (!chip->ecc.read_page)