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

[Albert ARIBAUD]

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.

> > +static int read_single_page(uint8_t *dest, int page,
> > +	struct lpc32xx_oob *oob)
> > +{
> > +	int status, i, timeout, err, max_bitflips = 0;
> > +
> > +	/* enter read mode */
> > +	writel(NAND_CMD_READ0, &lpc32xx_nand_mlc_registers->cmd);
> > +	/* send column (lsb then MSB) and page (lsb to MSB) */
> > +	writel(0, &lpc32xx_nand_mlc_registers->addr);
> > +	writel(0, &lpc32xx_nand_mlc_registers->addr);
> > +	writel(page & 0xff, &lpc32xx_nand_mlc_registers->addr);
> > +	writel((page>>8) & 0xff, &lpc32xx_nand_mlc_registers->addr);
> > +	writel((page>>16) & 0xff, &lpc32xx_nand_mlc_registers->addr);
> > +	/* start reading */
> > +	writel(NAND_CMD_READSTART, &lpc32xx_nand_mlc_registers->cmd);
> > +
> > +	/* large page auto decode read */
> > +	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))
> > +			;
> > +		/* return -1 if hard error */
> > +		if (status & ISR_DECODER_FAILURE)
> > +			return -1;
> > +		/* keep count of maximum bitflips performed */
> > +		if (status & ISR_DECODER_ERROR) {
> > +			err = ISR_DECODER_ERRORS(status);
> > +			if (err > max_bitflips)
> > +				max_bitflips = err;
> > +		}
> > +		/* copy first 512 bytes into buffer */
> > +		memcpy(dest+i*512, lpc32xx_nand_mlc_registers->buff, 512);
> > +		/* copy next 6 bytes bytes into OOB buffer */
> > +		memcpy(&oob->free[i], lpc32xx_nand_mlc_registers->buff, 6);
> > +	}
> > +	return max_bitflips;
> > +}
> > +
> 
> Why keep track of max_bitflips if the caller doesn't use it?

Because I modeled read_single_page from another read function, and I
preferred to minimize modifications and keep returning as much info
as I have, which can help debugging and cannot cause harm as it does
not affect the caller indeed.

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

> Also, if you fail ECC, that should be a fatal error, not something to
> silently skip.

Will fix.

> -Scott

Cordialement,
Albert ARIBAUD
3ADEV