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

[Albert ARIBAUD]

Hi Scott,

Le Mon, 16 Mar 2015 16:30:29 -0500, Scott Wood
<scottwood at freescale.com> a écrit :

> 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)?

The udelay(1) in the loop is ok with me -- I'll put it in v6.

> > > > +#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.

Thanks -- I'll dig into the BBT code to see how it's handled in my
target and put a fix in v6 based on that.

> -Scott

Cordialement,
Albert ARIBAUD
3ADEV