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

[Scott Wood]

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.

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

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

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

-Scott