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

[Scott Wood]

On 04/26/2013 03:02:36 AM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu at faraday-tech.com>
> 
> Faraday FTNANDC021 is a integrated NAND flash controller.
> It use a build-in command table to abstract the underlying
> NAND flash control logic.
> 
> For example:
> 
> Issuing a command 0x10 to FTNANDC021 would result in
> a page write + a read status operation.
> 
> Signed-off-by: Kuo-Jung Su <dantesu at faraday-tech.com>
> CC: Scott Wood <scottwood at freescale.com>
> ---
>  README                        |    7 +
>  drivers/mtd/nand/Makefile     |    1 +
>  drivers/mtd/nand/ftnandc021.c |  724  
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/ftnandc021.h |  137 ++++++++
>  include/faraday/nand.h        |   34 ++
>  5 files changed, 903 insertions(+)
>  create mode 100644 drivers/mtd/nand/ftnandc021.c
>  create mode 100644 drivers/mtd/nand/ftnandc021.h
>  create mode 100644 include/faraday/nand.h
> 
> diff --git a/README b/README
> index 862bb3e..adc198f 100644
> --- a/README
> +++ b/README
> @@ -3872,6 +3872,13 @@ Low Level (hardware related) configuration  
> options:
>  		- drivers/mtd/nand/ndfc.c
>  		- drivers/mtd/nand/mxc_nand.c
> 
> +- CONFIG_SYS_NAND_TIMING
> +		Defined to tell the NAND controller that the NAND chip  
> is using
> +		a customized timing parameters.
> +		Not all NAND drivers use this symbol.
> +		Example of drivers that use it:
> +		- drivers/mtd/nand/ftnandc021.c

This doesn't seem to have any standardized meaning (even if that meaning
is applicable to only a subset of controllers), so please call it
CONFIG_SYS_FTNANDC021_TIMING and document the ftnandc021-specific
semantics.

> diff --git a/drivers/mtd/nand/ftnandc021.c  
> b/drivers/mtd/nand/ftnandc021.c
> new file mode 100644
> index 0000000..39c181f
> --- /dev/null
> +++ b/drivers/mtd/nand/ftnandc021.c
> @@ -0,0 +1,724 @@
> +/*
> + * 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/errno.h>
> +#include <asm/io.h>
> +#include <asm/unaligned.h>
> +#include <nand.h>
> +#include <malloc.h>
> +#include <faraday/nand.h>
> +
> +#include "ftnandc021.h"
> +
> +#define CFG_HWECC	/* Enable hardware ECC */

If this is really to be optional, call it CONFIG_FTNANDC021_ECC (HWECC
suggests the alternative is SW ECC, not no ECC) and define it in the
board config file (or better, invert it to CONFIG_FTNANDC021_NO_ECC so
ECC is the default).

If it's not meant to be optional, just remove the non-ECC code.

If the ECC doesn't cause major problems in a reasonable use case, I'd
rather see the non-ECC code just removed.

> +static struct nand_ecclayout ftnandc021_ecclayout[] = {
> +	{ /* page size = 512 (oob size = 16) */
> +		.eccbytes = 6,
> +		.eccpos = { 0, 1, 2, 3, 6, 7 },
> +		.oobfree = {
> +#ifdef CFG_HWECC
> +			{ 9, 3 },
> +#else
> +			{ 8, 4 },
> +#endif
> +		}
> +	},
> +	{ /* page size = 2048 (oob size = 64) */
> +		.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 = {
> +#ifdef CFG_HWECC
> +			{ 9, 3 },
> +#else
> +			{ 8, 4 },
> +#endif
> +		},
> +	},
> +	{ /* page size = 4096 (oob size = 128) */
> +		.eccbytes = 48,
> +		.eccpos = {
> +			80, 81, 82, 83, 84, 85, 86, 87,
> +			88, 89, 90, 91, 92, 93, 94, 95,
> +			96, 97, 98, 99, 100, 101, 102, 103,
> +			104, 105, 106, 107, 108, 109, 110, 111,
> +			112, 113, 114, 115, 116, 117, 118, 119,
> +			120, 121, 122, 123, 124, 125, 126, 127
> +		},
> +		.oobfree = {
> +#ifdef CFG_HWECC
> +			{ 9, 7 },
> +#else
> +			{ 8, 8 },
> +#endif
> +		},
> +	},
> +};

Shouldn't .eccpos depend on HWECC?

> +static int ftnandc021_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
> +	uint8_t *read_ecc, uint8_t *calc_ecc)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct faraday_nand_chip *info = chip->priv;
> +	struct ftnandc021_chip *priv = info->priv;
> +	struct ftnandc021_regs __iomem *regs = priv->regs;
> +	uint32_t st = readl(&regs->ecc_sr);
> +	int ret = 0;
> +
> +	if (st & ECC_SR_CERR) {
> +		printf("ftnandc021: ecc corection error\n");
> +		ret = -EIO;
> +	} else if (st & ECC_SR_ERR) {
> +		printf("ftnandc021: ecc error\n");
> +		ret = -EIO;
> +	}
> +
> +	return ret;

Can you detect correctable errors?

> +#ifdef CFG_HWECC
> +	chip->ecc.bytes          = chip->ecc.layout->eccbytes;
> +	chip->ecc.size           = info->pgsz;
> +	chip->ecc.steps          = 1;

Is it really all in one step, regardless of page size?

-Scott