Dave, > -----Original Message----- > From: David Brownell [mailto:[email protected]] > Sent: Wednesday, May 13, 2009 5:04 AM > To: [email protected] > Cc: Narnakaje, Snehaprabha; [email protected]; > [email protected]; [email protected]; [email protected] > Subject: Re: [PATCH 1/2] NAND: Adding new ECC mode - NAND_ECC_HW_OOB_FIRST > for TI DaVinci devices > > On Wednesday 06 May 2009, [email protected] wrote: > > From: Sneha Narnakaje <[email protected]> > > > > 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.
I initially thought of keeping them in separate patches, but since the reason for change (a) is actually due to (b), I thought people might prefer to keep them together. > > 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 <[email protected]> > > --- > > 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. Will add the details here in the next patch. > > > > + */ > > > 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. OK. I am assuming it is linux-mtd next. Thanks Sneha > > > > #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) > _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
