[U-Boot] [PATCH v2 07/12] mtd: nand: add Faraday FTNANDC021 NAND controller support

[Scott Wood]

On 04/18/2013 04:25:34 AM, Kuo-Jung Su wrote:
> diff --git a/drivers/mtd/nand/ftnandc021.c  
> b/drivers/mtd/nand/ftnandc021.c
> new file mode 100644
> index 0000000..58863dc
> --- /dev/null
> +++ b/drivers/mtd/nand/ftnandc021.c
> @@ -0,0 +1,544 @@
> +/*
> + * Faraday NAND Flash Controller
> + *
> + * (C) Copyright 2010 Faraday Technology
> + * Dante Su <dantesu at faraday-tech.com>
> + *
> + * This file is released under the terms of GPL v2 and any later  
> version.
> + * See the file COPYING in the root directory of the source tree for  
> details.
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/unaligned.h>
> +#include <nand.h>
> +#include <malloc.h>
> +
> +#include "ftnandc021.h"
> +
> +/* common bitmask of nand flash status register */
> +#define NAND_IOSTATUS_ERROR		BIT_MASK(0)
> +#define NAND_IOSTATUS_READY		BIT_MASK(6)
> +#define NAND_IOSTATUS_UNPROTCT	BIT_MASK(7)
> +
> +struct ftnandc021_chip {
> +	void    *iobase;

struct ftnandc021_regs __iomem *iobase;

> +	uint32_t cmd;
> +
> +	uint32_t pgidx;
> +
> +	uint32_t off;
> +	uint8_t  buf[256];
> +
> +	uint32_t adrc;	/* address cycle */
> +	uint32_t pgsz;	/* page size */
> +	uint32_t bksz;	/* block size */
> +};
> +
> +/* Register access macros */
> +#define NAND_READ(r)		le32_to_cpu(readl(r))
> +#define NAND_WRITE(v, r)	writel(cpu_to_le32(v), r)
> +#define NAND_SETBITS(m, r)	setbits_le32(r, m)
> +#define NAND_CLRBITS(m, r)	clrbits_le32(r, m)

Do we really need these wrappers?  At least use inline functions (with  
lowercase names) rather than ALLCAPS MACROS, but I don't see the point  
once you get rid of the byteswapping, which is broken.  readl() reads a  
little-endian register and returns a CPU-ordered value, and then you  
pass that CPU ordered value to a function that wants to take a little  
endian value and swap it again.  Likewise with writel, in reverse.

> +static struct nand_ecclayout ftnandc021_oob_2k = {
> +	.eccbytes = 24,
> +	.eccpos = {
> +		40, 41, 42, 43, 44, 45, 46, 47,
> +		48, 49, 50, 51, 52, 53, 54, 55,
> +		56, 57, 58, 59, 60, 61, 62, 63
> +	},
> +	.oobfree = {
> +		{
> +			.offset = 9,
> +			.length = 3
> +		}
> +	}
> +};

This layout doesn't seem to match what the code does.  The code says
there is no ECC, and only writes to specific fixed bytes of the OOB
(which doesn't match 3 bytes at offset 9).

> +static int
> +ftnandc021_reset(struct nand_chip *chip)

We don't generally do the "function name starts in column 0" thing in  
U-Boot.

> +{
> +	struct ftnandc021_chip *priv = chip->priv;
> +	struct ftnandc021_regs *regs = priv->iobase;

struct ftnandc021_regs __iomem *regs

Here and elsewhere, for "sparse" checking.

> +	uint32_t bk = 2;	/* 64 pages */
> +	uint32_t pg = 1;	/* 2k */
> +	uint32_t ac = 2;	/* 5 */

When do you actually use these default values, other than when one of  
the
switch statements fail to match -- which seems like it would be an error
condition; even if you don't explicitly check for the error it doesn't
seem good to paper over it by providing common values that might work.

> +#ifdef CONFIG_FTNANDC021_ACTIMING_1
> +	NAND_WRITE(CONFIG_FTNANDC021_ACTIMING_1, &regs->atr[0]);
> +#endif
> +#ifdef CONFIG_FTNANDC021_ACTIMING_2
> +	NAND_WRITE(CONFIG_FTNANDC021_ACTIMING_2, &regs->atr[1]);
> +#endif

Use CONFIG_SYS_ for things which describe hardware rather than user  
preference.  Document all CONFIG symbols (including CONFIG_SYS symbols)  
in the README.

> +	NAND_WRITE(0, &regs->ier);
> +	NAND_WRITE(0, &regs->pir);
> +	NAND_WRITE(0xff, &regs->bbiwr);
> +	NAND_WRITE(0xffffffff, &regs->lsnwr);
> +	NAND_WRITE(0xffffffff, &regs->crcwr);
> +
> +	if (chip->options & NAND_BUSWIDTH_16)
> +		NAND_WRITE(FCR_SWCRC | FCR_IGNCRC | FCR_16BIT,  
> &regs->fcr);
> +	else
> +		NAND_WRITE(FCR_SWCRC | FCR_IGNCRC, &regs->fcr);
> +
> +	/* chip reset */
> +	NAND_WRITE(SRR_CHIP_RESET, &regs->srr);
> +
> +	/* wait until chip ready */
> +	while (NAND_READ(&regs->srr) & SRR_CHIP_RESET)
> +		;

Timeout?

> +	switch (priv->adrc) {
> +	case 3:
> +		ac = 0;
> +		break;
> +	case 4:
> +		ac = 1;
> +		break;
> +	case 5:
> +		ac = 2;
> +		break;
> +	}

ac = priv->adrc - 3;

> +static inline int
> +ftnandc021_ckst(struct ftnandc021_chip *priv)
> +{
> +	struct ftnandc021_regs *regs = priv->iobase;
> +	uint32_t st = NAND_READ(&regs->idr[1]);
> +
> +	if (st & NAND_IOSTATUS_ERROR)
> +		return -NAND_IOSTATUS_ERROR;
> +
> +	if (!(st & NAND_IOSTATUS_READY))
> +		return -NAND_IOSTATUS_READY;
> +
> +	if (!(st & NAND_IOSTATUS_UNPROTCT))
> +		return -NAND_IOSTATUS_UNPROTCT;
> +
> +	return 0;
> +}

Why the negation of NAND_IOSTATUS_*?  These aren't standard error
codes...  you don't even use the return value at all that I see, other
than checking zero or not-zero.

> +static uint8_t
> +ftnandc021_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct ftnandc021_chip *priv = chip->priv;
> +	struct ftnandc021_regs *regs = priv->iobase;
> +	uint8_t ret = 0xff;
> +
> +	switch (priv->cmd) {
> +	case NAND_CMD_READID:
> +	case NAND_CMD_READOOB:
> +		ret = priv->buf[priv->off % 256];
> +		priv->off += 1;
> +		break;
> +	case NAND_CMD_STATUS:
> +		ret = (uint8_t)(NAND_READ(&regs->idr[1]) & 0xff);
> +		break;

Why "% 256" in one case but "& 0xff" in the other?

> +	default:
> +		debug("ftnandc021_read_byte: unknown cmd(0x%02X)\n",
> +			priv->cmd);

This is an error, not just debug info.  Use printf.

> +/**
> + * Read data from NAND controller into buffer
> + * @mtd: MTD device structure
> + * @buf: buffer to store date
> + * @len: number of bytes to read
> + */
> +static void
> +ftnandc021_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct ftnandc021_chip *priv = chip->priv;
> +	struct ftnandc021_regs *regs = priv->iobase;
> +	ulong off;
> +
> +	/* oob read */
> +	if (len <= mtd->oobsize) {
> +		ftnandc021_read_oob(mtd, buf, len);
> +		return;
> +	}

Are you sure that a length smaller than the oobsize always means that
it's an oob read?

> +	/* page read */
> +	for (off = 0; len > 0; len -= 4, off += 4) {
> +		while (!(NAND_READ(&regs->ior) & IOR_READY))
> +			;
> +		*(uint32_t *)(buf + off) = NAND_READ(&regs->dr);
> +	}

Why do you need to check IOR_READY here but not in read_byte?

> +static void
> +ftnandc021_cmdfunc(struct mtd_info *mtd, unsigned cmd, int column,  
> int pgidx)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct ftnandc021_chip *priv = chip->priv;
> +	struct ftnandc021_regs *regs = priv->iobase;
> +
> +	priv->cmd   = cmd;
> +	priv->pgidx = pgidx;
> +
> +	switch (cmd) {
> +	case NAND_CMD_READID:	/* 0x90 */
> +		if (ftnandc021_command(priv, FTNANDC021_CMD_RDID)) {
> +			printf("ftnandc021: RDID failed.\n");
> +		} else {
> +			put_unaligned_le32(NAND_READ(&regs->idr[0]),
> +				priv->buf);
> +			put_unaligned_le32(NAND_READ(&regs->idr[1]),
> +				priv->buf + 4);
> +			priv->off = 0;
> +		}
> +		break;

Do error handling like this:

	if (ftnandc021_command(priv, FTNANDC021_CMD_RDID)) {
		printf(...);
		break;
	}

	put_unaligned...
	...

Why would it be unaligned?
Why _le32?
Can you not read a byte at a time here?

> +/**
> + * hardware specific access to control-lines
> + * @mtd: MTD device structure
> + * @cmd: command to device
> + * @ctrl:
> + * NAND_NCE: bit 0 -> don't care
> + * NAND_CLE: bit 1 -> Command Latch
> + * NAND_ALE: bit 2 -> Address Latch
> + *
> + * NOTE: boards may use different bits for these!!
> + */
> +static void
> +ftnandc021_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int  
> ctrl)
> +{
> +}

Just leave the function pointer NULL.

> +	chip->ecc.mode   = NAND_ECC_NONE;

Really, no ECC at all?  That is quite broken.  There is absolutely no  
way
to get access to the full OOB in order to do software ECC?

Or is it doing hardware ECC in a way that is transparent?  You should
still use NAND_ECC_HARD in that case.

> +	chip->ecc.layout = &ftnandc021_oob_2k;

What if it's not 2K NAND?

> diff --git a/include/faraday/nand.h b/include/faraday/nand.h
> new file mode 100644
> index 0000000..6d8efb2
> --- /dev/null
> +++ b/include/faraday/nand.h
> @@ -0,0 +1,16 @@
> +/*
> + * Faraday NAND Flash Controller
> + *
> + * (C) Copyright 2010 Faraday Technology
> + * Dante Su <dantesu at faraday-tech.com>
> + *
> + * This file is released under the terms of GPL v2 and any later  
> version.
> + * See the file COPYING in the root directory of the source tree for  
> details.
> + */
> +
> +#ifndef _FARADAY_NAND_H
> +#define _FARADAY_NAND_H
> +
> +int ftnandc021_probe(struct nand_chip *chip);

New drivers should use CONFIG_SYS_NAND_SELF_INIT

-Scott