[U-Boot] [PATCH 4/7] mxc_nand: add nand driver for MX2/MX3

[Scott Wood]

On Mon, Jun 08, 2009 at 04:12:48AM +0400, Ilya Yanok wrote:
> Driver for NFC NAND controller found on Freescale's MX2 and MX3
> processors. Ported from Linux. Tested only with i.MX27 but should
> works with other MX2 and MX3 processors too.
> 
> Signed-off-by: Ilya Yanok <yanok at emcraft.com>

Please look at drivers/mtd/nand/mpc5121_nfc.c, which AFAICT is very
similar hardware, and see if anything can be factored out into common
code, and try to keep the rest looking the same except where the hardware
actually differs.

> +static struct nand_ecclayout nand_hw_eccoob_16 = {
> +	.eccbytes = 5,
> +	.eccpos = {6, 7, 8, 9, 10},
> +	.oobfree = {{0, 6}, {12, 4}, }
> +};

This implies the bad block marker is one byte at offset 11 (it's all
that's left), but I don't see any override of the bad block pattern.

> +static void *mxc_nand_memcpy32(void *dest, void *source, size_t size)
> +{
> +	uint32_t *s = source, *d = dest;
> +
> +	size >>= 2;
> +	while (size--)
> +		*d++ = *s++;
> +	return dest;
> +}

This should probably be using I/O accessors (possibly raw) -- and should
take uint32_t *, not void *.

> +/*
> + * This function requests the NANDFC to perform a read of the
> + * NAND device status and returns the current status.
> + */
> +static uint16_t get_dev_status(struct mxc_nand_host *host)
> +{
> +	void __iomem *main_buf = host->regs->main_area1;
> +	uint32_t store;
> +	uint16_t ret, tmp;
> +	/* Issue status request to NAND device */
> +
> +	/* store the main area1 first word, later do recovery */
> +	store = readl(main_buf);
> +	/*
> +	 * NANDFC buffer 1 is used for device status to prevent
> +	 * corruption of read/write buffer on status requests.
> +	 */
> +	writew(1, &host->regs->nfc_buf_addr);

But it looks like buffer 1 is used for data with large page flash.

Other drivers don't seem to have any problem with status reads clobbering
the buffer...

> +/* This functions is used by upper layer to checks if device is ready */
> +static int mxc_nand_dev_ready(struct mtd_info *mtd)

"This functions is"?  I'd say this comment is pretty redundant with respect
to the function name anyway...

> +static u_char mxc_nand_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
> +	uint8_t ret = 0;
> +	uint16_t col, rd_word;
> +	uint16_t __iomem *main_buf =
> +		(uint16_t __iomem *)host->regs->main_area0;
> +	uint16_t __iomem *spare_buf =
> +		(uint16_t __iomem *)host->regs->spare_area0;

According to Magnus Lilja, "the nand flash controller can only handle 32
bit read/write operations, any other size will cause an abort (or
something like that)".  But now we're accessing it as 16-bit?

> +static uint16_t mxc_nand_read_word(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
> +	uint16_t col, rd_word, ret;
> +	uint16_t __iomem *p;
> +
> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +	      "mxc_nand_read_word(col = %d)\n", host->col_addr);
> +
> +	col = host->col_addr;
> +	/* Adjust saved column address */
> +	if (col < mtd->writesize && host->spare_only)
> +		col += mtd->writesize;
> +
> +	if (col < mtd->writesize) {
> +		p = (uint16_t __iomem *)(host->regs->main_area0 + (col >> 1));
> +	} else {
> +		p = (uint16_t __iomem *)(host->regs->spare_area0 +
> +				((col - mtd->writesize) >> 1));
> +	}
> +
> +	if (col & 1) {
> +		rd_word = readw(p);
> +		ret = (rd_word >> 8) & 0xff;
> +		rd_word = readw(&p[1]);
> +		ret |= (rd_word << 8) & 0xff00;
> +

col should never be odd if you're reading words.

> +/*
> + * Write data of length len to buffer buf. The data to be
> + * written on NAND Flash is first copied to RAMbuffer. After the Data Input
> + * Operation by the NFC, the data is written to NAND Flash
> + */
> +static void mxc_nand_write_buf(struct mtd_info *mtd,
> +				const u_char *buf, int len)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct mxc_nand_host *host = nand_chip->priv;
> +	int n, col, i = 0;
> +
> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +	      "mxc_nand_write_buf(col = %d, len = %d)\n", host->col_addr,
> +	      len);
> +
> +	col = host->col_addr;
> +
> +	/* Adjust saved column address */
> +	if (col < mtd->writesize && host->spare_only)
> +		col += mtd->writesize;
> +
> +	n = mtd->writesize + mtd->oobsize - col;
> +	n = min(len, n);
> +
> +	MTDDEBUG(MTD_DEBUG_LEVEL3,
> +	      "%s:%d: col = %d, n = %d\n", __func__, __LINE__, col, n);
> +
> +	while (n) {

Safer to do while (n > 0), especially when the code is this complex.

> +		void __iomem *p;
> +
> +		if (col < mtd->writesize) {
> +			p = host->regs->main_area0 + (col & ~3);
> +		} else {
> +			p = host->regs->spare_area0 -
> +						mtd->writesize + (col & ~3);
> +		}
> +
> +		MTDDEBUG(MTD_DEBUG_LEVEL3, "%s:%d: p = %p\n", __func__,
> +		      __LINE__, p);
> +
> +		if (((col | (int)&buf[i]) & 3) || n < 16) {

Do not cast pointers to "int".  Use "uintptr_t" or "unsigned long" if you
must.

Why < 16 and not < 4?

> +			uint32_t data = 0;
> +
> +			if (col & 3 || n < 4)
> +				data = readl(p);

If that condition doesn't hold, the data we use is zero?

> +
> +			switch (col & 3) {
> +			case 0:
> +				if (n) {
> +					data = (data & 0xffffff00) |
> +					    (buf[i++] << 0);
> +					n--;
> +					col++;
> +				}
> +			case 1:
> +				if (n) {
> +					data = (data & 0xffff00ff) |
> +					    (buf[i++] << 8);
> +					n--;
> +					col++;
> +				}
> +			case 2:
> +				if (n) {
> +					data = (data & 0xff00ffff) |
> +					    (buf[i++] << 16);
> +					n--;
> +					col++;
> +				}
> +			case 3:
> +				if (n) {
> +					data = (data & 0x00ffffff) |
> +					    (buf[i++] << 24);
> +					n--;
> +					col++;
> +				}
> +			}
> +			writel(data, p);

Might I suggest this?

union {
	uint32_t word;
	uint8_t bytes[4];
} nfc_word;

nfc_word.word = readl(p);
nfc_word.bytes[col & 3] = buf[i++];
n--;
col++;

writel(nfc_word.word, p);

As a side benefit, you lose the endian dependency.

> +			mxc_nand_memcpy32(p, (void *)&buf[i], m);

Unnecessary cast.

> +	/* Update saved column address */
> +	host->col_addr = col;
> +
> +}

No blank lines before the brace at the end of a block.

> +
> +/*
> + * Used by the upper layer to verify the data in NAND Flash
> + * with the data in the buf.
> + */
> +static int mxc_nand_verify_buf(struct mtd_info *mtd,
> +				const u_char *buf, int len)
> +{
> +	return -EFAULT;
> +}

Umm...

> +	case NAND_CMD_SEQIN:
> +		if (column >= mtd->writesize) {
> +			/*
> +			 * FIXME: before send SEQIN command for write OOB,
> +			 * We must read one page out.
> +			 * For K9F1GXX has no READ1 command to set current HW
> +			 * pointer to spare area, we must write the whole page
> +			 * including OOB together.
> +			 */
> +			if (host->pagesize_2k) {
> +				/* call ourself to read a page */
> +				mxc_nand_command(mtd, NAND_CMD_READ0, 0,
> +						page_addr);
> +			}

Should this be #ifdef HWECC?  And update the comment to indicate the
actual problem, which is the unusual hardware ECC implementation.  I
don't see what the lack of a "READ1" command has to do with it.

And is this actually a FIXME if it's already being done?

> +#ifdef CONFIG_MXC_NAND_HWECC
> +	this->ecc.calculate = mxc_nand_calculate_ecc;
> +	this->ecc.hwctl = mxc_nand_enable_hwecc;
> +	this->ecc.correct = mxc_nand_correct_data;
> +	this->ecc.mode = NAND_ECC_HW;
> +	this->ecc.size = 512;
> +	this->ecc.bytes = 3;
> +	this->ecc.layout = &nand_hw_eccoob_8;
> +	tmp = readw(&host->regs->nfc_config1);
> +	tmp |= NFC_ECC_EN;
> +	writew(tmp, &host->regs->nfc_config1);
> +#else
> +	this->ecc.size = 512;
> +	this->ecc.bytes = 3;
> +	this->ecc.layout = &nand_hw_eccoob_8;
> +	this->ecc.mode = NAND_ECC_SOFT;
> +	tmp = readw(&host->regs->nfc_config1);
> +	tmp &= ~NFC_ECC_EN;
> +	writew(tmp, &host->regs->nfc_config1);
> +#endif

Soft ECC only supports 256-byte ECC blocks (anything you set here to the
contrary will just be overwritten), and you'll need a layout that
accommodates that with enough ECC bytes.

Alternatively, you can implement 512-byte soft ECC if you don't want to
waste those 3 extra OOB bytes. :-)

> +	host->pagesize_2k = 0;

So large page is currently unsupported?

-Scott