[PATCH] MTD-NAND: Changes to read_page APIs to support NAND_ECC_HW_SYNDROME mode on TI DaVinci DM355

[Narnakaje, Snehaprabha]

Thomas,

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: Tuesday, April 07, 2009 3:48 PM
> To: Narnakaje, Snehaprabha
> Cc: David Brownell; davinci-linux-open-source at linux.davincidsp.com; linux-
> mtd at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: RE: [PATCH] MTD-NAND: Changes to read_page APIs to support
> NAND_ECC_HW_SYNDROME mode on TI DaVinci DM355
> 
> On Tue, 7 Apr 2009, Narnakaje, Snehaprabha wrote:
> > > > On DaVinci DM355 device, we need to pass the ecc code/syndrome
> > > > extracted from OOB area, in order to read the HW ECC for each data
> > > > chunk. This is where we differ from NAND_ECC_HW mode.
> > >
> > > I have to admit that I'm slightly confused. Is the below description
> > > correct?
> > >
> > > On write you generate the ECC via hardware and store it in the OOB
> > > area, right ?
> 
> Your mailer still does not insert line breaks at around 78 chars.
> 
> > Yes. For a large page (e.g. 2K+64), ECC should be stored in the
> > 64bytes OOB region. The NAND controller on DaVinci DM355 device is
> > capable of generating HW ECC (4-bit) for every 512bytes. This means,
> > we have 4 eccsteps. The ECC should be generated by the HW for every
> > 512byte chunk write and stored temporarily in a buffer until all 4
> > eccsteps have been tried. The ECC from the temporary buffer is then
> > written to the OOB area.
> 
> That's exactly what NAND_ECC_HW does.
> 
> > >
> > > On read you read the oob data first and extract the ECC. Then you feed
> > > the extracted ECC into the HW generator and read the data. Is this
> > > correct ?
> 
> > Almost, read OOB, read data and then feed the extracted ECC into HW
> > generator. Read the ECC status to see any correction required on the
> > read data. Again, reading data, feeding the extracted ECC into HW
> > generator and the correction will have to be repeated for # of times
> > - eccsteps.
> 
> Ok.
> 
> > >
> > > > The read_page/write_page APIs for NAND_ECC_HW_SYNDROME have the
> > > > other problem that David mentioned - overwriting NAND manufacturers
> > > > bad block meta data, when used with large page NAND chips. These
> > > > APIs do not handle the "eccsteps" properly. The OOB is read/written
> > > > after every data chunk, thus you actually have overwritten the
> > > > factory bad block information, when these APIs handle the last data
> > > > chunk. OOB should be read/written after the entire data (a page) is
> > > > read/written.
> > >
> > > Again, that is _how_ the NAND_ECC_HW_SYNDROME functions work. And if
> > > your hardware needs a completely different mode, then we need to
> > > analyse what's the best solution for it.
> 
> > Based on the description above, NAND_ECC_HW_SYNDROME fitted well,
> 
> No, it does not fit well.
> 
> > with the exception of overriding the read_page/write_page APIs (to
> > fix the bugs mentioned above). I have not sent the actual NAND
> 
> Those are not bugs. You abused that interface and now you claim it has
> bugs. It does _not_. You introduced the bugs by using a function for
> something it was never designed for.
> 
> > driver to the linux-mtd yet, since the initial version (supported
> > only 1-bit HW ECC) submitted by David Brownell was still in review
> > (no comments and not approved yet). While coming up with read_page
> > API in the DaVinci NAND driver, we realized the need to pass "page"
> > parameter. The read_page API in the DaVinci NAND driver required to
> > call chip->cmdfunc to adjust the read location (page vs. OOB). The
> > "page" parameter has to be passed to the chip->cmdfunc.
> 
> This is the wrong approach.
> 
> What you need is a NAND_ECC_HW_OOB_FIRST mode, which uses the
> NAND_ECC_HW write function and implements a separate read function
> which reads the oob and then reads the data chunks.

OK, I will come up with the new ECC mode with the separate read_page handler.

However, I am still not clear on how I can avoid the change to read_page handler definition to pass page. This new read_page handler does require it, if it has to call the cmdfunc to adjust the read pointer to OOB area. Basically, I am referring to the code snippet below -

...
/* Read the OOB area first */
chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
chip->read_buf(mtd, oob, mtd->oobsize);
chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
...

The function that calls read_page handler has already called the cmdfunc to adjust the read pointer to the beginning of the page. The read_page handler for the new ECC mode will have to re-adjust the read pointer to read the OOB first.

Thanks
Sneha

> 
> Thanks,
> 
> 	tglx