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

Scott Wood
2013-04-26T23:41:30Z

On 04/26/2013 03:02:36 AM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su <dantesu@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@faraday-tech.com>
> CC: Scott Wood <scottwood@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@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