[U-Boot] [PATCH v5 2/8] lpc32xx: mtd: nand: add MLC NAND controller

[Scott Wood]

On Sat, 2015-03-14 at 15:27 +0100, Albert ARIBAUD wrote:
> Bonjour Scott,
> 
> Le Fri, 13 Mar 2015 16:57:33 -0500, Scott Wood
> <scottwood at freescale.com> a écrit :
> 
> > On Fri, 2015-03-13 at 09:04 +0100, Albert ARIBAUD (3ADEV) wrote:
> > > +	/* go through all four small pages */
> > > +	for (i = 0; i < 4; i++) {
> > > +		/* start auto decode (reads 528 NAND bytes) */
> > > +		writel(0, &lpc32xx_nand_mlc_registers->ecc_auto_dec_reg);
> > > +		/* wait for controller to return to ready state */
> > > +		timeout = LPC32X_NAND_TIMEOUT;
> > > +		do {
> > > +			if (timeout-- == 0)
> > > +				return -1;
> > > +			status = readl(&lpc32xx_nand_mlc_registers->isr);
> > > +		} while (!(status & ISR_CONTROLLER_READY));
> > 
> > How much time does 10000 reads of this register equate to?  Are you sure
> > it's enough?  Timeouts should generally be in terms of time, not loop
> > iterations.
> 
> I followed the examples in several drivers where timeouts are by
> iteration. Note that  -- while this does not void your point --  I did
> not use 10000 but 100000, which at a CPU clock of 208 MHz, and assuming
> an optimistic one instruction per cycle and two instructions per loop,
> makes the loop last at least 960 us, well over the 600 us which the
> NAND takes for any page programming.

What if this driver ends up being used on hardware that runs
significantly faster than 208 MHz?  I could understand if it's hugely
space-constrained SPL code (like the ones that have to fit in 4K), but
otherwise why not make use of the timekeeping code that exists in U-Boot
(either by reading the timer, or by putting udelay(1) in the loop body)?

> > > +#define LARGE_PAGE_SIZE 2048
> > > +
> > > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > > +{
> > > +	struct lpc32xx_oob oob;
> > > +	unsigned int page = offs / LARGE_PAGE_SIZE;
> > > +	unsigned int left = DIV_ROUND_UP(size, LARGE_PAGE_SIZE);
> > > +
> > > +	while (left) {
> > > +		int res = read_single_page(dst, page, &oob);
> > > +		page++;
> > > +		/* if read succeeded, even if fixed by ECC */
> > > +		if (res >= 0) {
> > > +			/* skip bad block */
> > > +			if (oob.free[0].free_oob_bytes[0] != 0xff)
> > > +				continue;
> > > +			if (oob.free[0].free_oob_bytes[1] != 0xff)
> > > +				continue;
> > > +			/* page is good, keep it */
> > > +			dst += LARGE_PAGE_SIZE;
> > > +			left--;
> > > +		}
> > 
> > You should be checking the designated page(s) of the block, rather than
> > the current page, for the bad block markers -- and skipping the entire
> > block if it's bad.
> 
> Will fix this -- is there any helper function in the bad block
> management code for this? I could not find one, but I'm no NAND expert.

I don't know of any such helper -- outside of SPL it's handled via the
BBT.  fsl_ifc_spl.c is an example that checks for bad block markers, but
it's hardcoded to assume the first two pages of a block which is a bit
simpler than checking at the end of the block.

-Scott