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

[Albert ARIBAUD]

Hi Scott,

Le Mon, 9 Mar 2015 18:51:03 -0500, Scott Wood <scottwood at freescale.com>
a écrit :

> On Thu, 2015-03-05 at 07:46 +0100, Albert ARIBAUD (3ADEV) wrote:
> > diff --git a/drivers/mtd/nand/lpc32xx_nand_mlc.c b/drivers/mtd/nand/lpc32xx_nand_mlc.c
> > new file mode 100644
> > index 0000000..cb23972
> > --- /dev/null
> > +++ b/drivers/mtd/nand/lpc32xx_nand_mlc.c
> > @@ -0,0 +1,589 @@
> > +/*
> > + * LPC32xx MLC NAND flash controller driver
> > + *
> > + * (C) Copyright 2014 3ADEV <http://3adev.com>
> > + * Written by Albert ARIBAUD <albert.aribaud at 3adev.fr>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + *
> > + * NOTE:
> > + *
> > + * The MLC NAND flash controller provides hardware Reed-Solomon ECC
> > + * covering in- and out-of-band data together. Therefore, in- and out-
> > + * of-band data must be written together in order to have a valid ECC.
> > + *
> > + * Consequently, pages with meaningful in-band data are written with
> > + * blank (all-ones) out-of-band data and a valid ECC, and any later
> > + * out-of-band data write will void the ECC.
> > + *
> > + * Therefore, code which reads such late-written out-of-band data
> > + * should not rely on the ECC validity.
> > + */
> 
> When are you expecting later out-of-band writes?

I am not actually expecting OOB writes to happen, but they might do
regardless of my expectations (bad block marking, for instance?);
However, I do know the once an OOB write happens, OOB as well as
in-band reads will fail ECC, so I am just warning whoever it might
happen to.

> > +static struct lpc32xx_nand_mlc_registers *lpc32xx_nand_mlc_registers
> > +	= (struct lpc32xx_nand_mlc_registers *)MLC_NAND_BASE;
> 
> __iomem
> 
> > +				   unsigned int ctrl)
> > +{
> > +	if (cmd == NAND_CMD_NONE)
> > +		return;
> > +
> > +	if (ctrl & NAND_CLE)
> > +		writeb(cmd & 0Xff, &lpc32xx_nand_mlc_registers->cmd);
> > +	else if (ctrl & NAND_ALE)
> > +		writeb(cmd & 0Xff, &lpc32xx_nand_mlc_registers->addr);
> > +	else
> > +		return;
> > +}
> 
> That last return is superfluous.

Will fix in v4.

> > +/**
> > + * ECC layout -- this is needed whatever ECC mode we are using.
> > + * In a 2KB (4*512B) page, R/S codes occupy 40 (4*10) bytes.
> > + * To make U-Boot's life easier, we pack 'useable' OOB at the
> > + * front and R/S ECC at the back.
> > + */
> > +
> > +static struct nand_ecclayout lpc32xx_largepage_ecclayout = {
> > +	.eccbytes = 40,
> > +	.eccpos = {24, 25, 26, 27, 28, 29, 30, 31, 32, 33,
> > +		   34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
> > +		   44, 45, 46, 47, 48, 48, 50, 51, 52, 53,
> > +		   54, 55, 56, 57, 58, 59, 60, 61, 62, 63,
> > +		   },
> > +	.oobfree = {
> > +		{
> > +			.offset = 0,
> > +			.length = 24
> > +		},
> > +	}
> > +};
> 
> Is offset zero really free?  Where does the bad block marker go?

My bad -- I had not realized that the ECC layout struct had to factor
out the bad block marker location. Since this is a large block NAND,
that should be...

			.offset = 2,
			.length = 22

... Right? Will fix in v4.

> > +	for (i = 0; i < 4; i++) {
> > +		/* start data input */
> > +		oob[6*i] = ((0xfe << i) & 0xff);
> > +		oob[6*i+5] = ((0x7f >> i) & 0xff);
> > +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x200+0x210*i, page);
> > +		/* copy 6 non-ECC out-of-band bytes directly into NAND */
> > +		memcpy(lpc32xx_nand_mlc_registers->data, oob+6*i, 6);
> 
> Lots of magic numbers...

Will turn these into symbols in v4.

> > +		/* wait for NAND to return to ready state */
> > +		while (!(readl(&lpc32xx_nand_mlc_registers->isr)
> > +			& ISR_NAND_READY))
> > +			;
> 
> Timeout?

Will fix in v4.
 
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * lpc32xx_waitfunc - wait until a command is done
> > + * @mtd: MTD device structure
> > + * @chip: NAND chip structure
> > + *
> > + * Wait for controller and FLASH to both be ready.
> > + */
> > +
> > +static int lpc32xx_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
> > +{
> > +	/* FIXME: put a time-out here */
> > +	int status;
> > +	/* wait until both controller and NAND are ready */
> > +	do {
> > +		status = readl(&lpc32xx_nand_mlc_registers->isr);
> > +	} while ((status & (ISR_CONTROLLER_READY || ISR_NAND_READY))
> > +		 != (ISR_CONTROLLER_READY || ISR_NAND_READY));
> 
> Timeout?  Etc.

Will fix in v4.
 
> > +/*
> > + * Initialize the controller
> > + */
> > +
> > +int board_nand_init(struct nand_chip *nand)
> > +{
> 
> It'd be nice if new NAND drivers used CONFIG_SYS_NAND_SELF_INIT.

Will use in v4.

> > +
> > +	/* BBT options: read from last two pages, don't write */
> > +	nand->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_LASTBLOCK
> > +		| NAND_BBT_SCANLASTPAGE | NAND_BBT_SCAN2NDPAGE
> > +		| NAND_BBT_WRITE;
> 
> Don't write?

Obsolete comment. Will fix comment in v4.

> > +#define LARGE_PAGE_SIZE 2048
> > +
> > +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
> > +{
> > +	unsigned int page = offs / LARGE_PAGE_SIZE;
> > +	unsigned int left = (size + LARGE_PAGE_SIZE - 1) / LARGE_PAGE_SIZE;
> 
> DIV_ROUND_UP()

Will use in v4.

> > +	while (left) {
> > +		if (read_single_page(dst, page) >= 0) {
> > +			dst += LARGE_PAGE_SIZE;
> > +			page++;
> > +			left--;
> > +		}
> > +	}
> 
> No bad block skipping?

Hmm... actually the 'left--' should be just after the 'if' block,
otherwise not only will the code not skip a bad block, it will actually
loop infinitely trying to read it. Will fix in v4. Thanks for pointing
this out!

> -Scott

Thanks for your review.

Cordialement,
Albert ARIBAUD
3ADEV