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

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>
---
 drivers/mtd/nand/Makefile     |    1 +
 drivers/mtd/nand/ftnandc021.c |  544 +++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/ftnandc021.h |  132 ++++++++++
 include/faraday/nand.h        |   16 ++
 4 files changed, 693 insertions(+)
 create mode 100644 drivers/mtd/nand/ftnandc021.c
 create mode 100644 drivers/mtd/nand/ftnandc021.h
 create mode 100644 include/faraday/nand.h

--
1.7.9.5

Dear Kuo-Jung Su,

In message <1366277139-29728-8-git-send-email-dantesu@gmail.com> you wrote:
...
> +/* 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)

As before: drop these.

> +	/* wait until chip ready */
> +	while (NAND_READ(&regs->srr) & SRR_CHIP_RESET)
> +		;

Please add a timeout (and fix similar locations in the rest of the
code if there are such).

> +	switch (priv->bksz / priv->pgsz) {
> +	case 16:
> +		bk = 0;
> +		break;
> +	case 32:
> +		bk = 1;
> +		break;
> +	case 64:
> +		bk = 2;
> +		break;
> +	case 128:
> +		bk = 3;
> +		break;
> +	}

	bk = ffs(priv->bksz / priv->pgsz) - 4;

?

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

	ac = priv->adrc - 3;

?


Best regards,

Wolfgang Denk

2013/4/18 Wolfgang Denk <wd@denx.de>:
> Dear Kuo-Jung Su,
>
> In message <1366277139-29728-8-git-send-email-dantesu@gmail.com> you wrote:
> ...
>> +/* 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)
>
> As before: drop these.
>

Got it, thanks

>> +     /* wait until chip ready */
>> +     while (NAND_READ(&regs->srr) & SRR_CHIP_RESET)
>> +             ;
>
> Please add a timeout (and fix similar locations in the rest of the
> code if there are such).
>

Got it, thanks

>> +     switch (priv->bksz / priv->pgsz) {
>> +     case 16:
>> +             bk = 0;
>> +             break;
>> +     case 32:
>> +             bk = 1;
>> +             break;
>> +     case 64:
>> +             bk = 2;
>> +             break;
>> +     case 128:
>> +             bk = 3;
>> +             break;
>> +     }
>
>         bk = ffs(priv->bksz / priv->pgsz) - 4;
>
> ?
>

Got it, thanks

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

Got it, thanks

> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "You're just jealous." "What, of an overgrown puppy  with  a  single-
> figure IQ?"                      - Terry Pratchett, _Moving Pictures_



--
Best wishes,
Kuo-Jung Su

2013/4/19 Scott Wood <scottwood@freescale.com>:
> 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@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;
>
>

Got it, thanks.

>> +       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.
>
>

No, we don't need these wrappers, they're here only because my
miss-understanding
about readl()/writel(), I don't know they've already done these for me.
I'll drop these wrappers at next patch.

>> +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).
>
>

Sorry, it's my bad, I forget to clean it up.
In my private base, it's possible to turn-on ECC support to FTNADC021
with the following issues:

1. When ECC is enabled, FTNANDC021 would always do ECC on every data block,
   including the blank block(all 0xff).
   What I mean is 'With ECC enabled, we'll have ECC errors on non-ecc
proected data blocks or even the blank block'.

2. FTNANDC021 would stop upon ECC errors, a chip reset is required to
make it work again.

3. There are only 4 bytes data + 1 byte Bad Block Info available to software,
   and because of the ECC issues above.
   I'll have to use 1 byte of data to tag if this block is 'not blank',
   to make it possible to cancel the operation on these blocks
   to prevent the chip to be stopped. (It means that I don't have to reset chip)
   And thus if the ECC is enabled, there are only 3 data bytes
available to software,
   which makes it not possible to support BBT. (BBT requres 4 data bytes in OOB)

So in this patch, I'll prefer ECC disabled.
BTW, because of the issues above, this chip has been faced out many years ago.
It would only be available to A369 or QEMU, and never in mass production.
And thus I think it's ok to have ECC disabled in this patch.

>> +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.
>
>

Got it, thanks

>> +       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.
>
>

Got it, thanks.
I'll make it a fatal error when we got troubles to parse system straps.

>> +#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.
>
>

Got it, thanks

>> +       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?
>
>

Got it, thanks.
I'll add a timeout for it.

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

Got it, thanks

>> +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.
>
>

It's for my personal debugging purpose, and it would be removed at next patch.

>> +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?
>
>

Sorry, I don't know, too. :P
It would be fixed latter.

>> +       default:
>> +               debug("ftnandc021_read_byte: unknown cmd(0x%02X)\n",
>> +                       priv->cmd);
>
>
> This is an error, not just debug info.  Use printf.
>
>

Got it, thanks

>> +/**
>> + * 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?
>
>

From the MTD design: The answer is 'NO'.
From the 'nand' command of u-boot: The answer is likely a 'YES'.
The code was written at 4 years ago.
I'll see if there is a better way, and it'll be updated at next version
when I got luck.

>> +       /* 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?
>
>

Because there is no any hardware access in 'read_byte'.
The FTNANDC021 simplily does not support byte access by default.
(I mean it's possible to update hardware RTL code,
but it's not the case to A369)
So I add some tricks to READ_ID, READ_OOB and READ_STATUS.
The actual hardware access is performed at ftnandc021_cmdfunc(),
The ftnandc021_read_byte() simpily access the cached data buffer.

>> +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?

It's a historic issue, I think.
It's too long (4 years..) to me to recall the reason,
and it's obviously not necessary right now, so it would be
updated at next version.

> Can you not read a byte at a time here?
>
>

No, the FTNANDC021 does not support byte access.

>> +/**
>> + * 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.
>
>

Got it, thanks

>> +       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?
>
>

Please see the comments above.
And it would be clean up at next version.

>> 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@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



--
Best wishes,
Kuo-Jung Su

2013/4/23 Scott Wood <scottwood@freescale.com>:
> On 04/21/2013 09:45:00 PM, Kuo-Jung Su wrote:
>>
>> Sorry, it's my bad, I forget to clean it up.
>> In my private base, it's possible to turn-on ECC support to FTNADC021
>> with the following issues:
>>
>> 1. When ECC is enabled, FTNANDC021 would always do ECC on every data
>> block,
>>    including the blank block(all 0xff).
>>    What I mean is 'With ECC enabled, we'll have ECC errors on non-ecc
>> proected data blocks or even the blank block'.
>
>
> fsl_ifc_nand is similar -- when we see an uncorrectable error, we check
> whether the page is blank.
>
>

Good, I'm not alone....

>> 2. FTNANDC021 would stop upon ECC errors, a chip reset is required to
>> make it work again.
>
>
> This sounds bad, though.  I assume that this is more than just resetting the
> NAND controller -- that you're resetting the entire system on a chip or
> similar?
>
>

No, just reset the NAND controller. Since it's simply a peripheral device
attached to the bus, and supports only PIO mode.

>> 3. There are only 4 bytes data + 1 byte Bad Block Info available to
>> software,
>>    and because of the ECC issues above.
>>    I'll have to use 1 byte of data to tag if this block is 'not blank',
>>    to make it possible to cancel the operation on these blocks
>>    to prevent the chip to be stopped. (It means that I don't have to reset
>> chip)
>>    And thus if the ECC is enabled, there are only 3 data bytes
>> available to software,
>>    which makes it not possible to support BBT. (BBT requres 4 data bytes
>> in OOB)
>
>
> You could define a custom BBT descriptor that uses fewer bytes.
>
>

As far as I know, it's O.K to Linux, but requires some source file updated
to u-boot in old release. (i.e. u-boot-2009.01, which was the original
target release to FTNANDC021)
I'll check if it's now O.K to do that in u-boot. (I means no source
code needs to be modified, except FTNANDC021.[ch])

>> So in this patch, I'll prefer ECC disabled.
>> BTW, because of the issues above, this chip has been faced out many years
>> ago.
>> It would only be available to A369 or QEMU, and never in mass production.
>> And thus I think it's ok to have ECC disabled in this patch.
>
>
> Why do we need support in U-Boot, then?  Couldn't you just have QEMU model
> saner hardware?  What is A369?
>
>

A369 is a short for 'Faraday A369 SoC Evaluation Platform' with IC
design companies as
its target customers, which means A369 should never be the ASIC used
in mass production.
And the FTNANDC021 is actually designed for SSD applications, the
general software (e.g. linux)
is never put into consideration during IP/CHIP design process.

It's an an accident that the FTNANDC021 get integrated into the A369.
Although the NAND controller of A369 is considered a bad chip, the
NAND flash is still a primary storage
to A369. So it still needs to be integrated to U-Boot to make
everything work correctly.

The QEMU model for A369 is out several month ago, its primary target
is to provide ROM code development
environment for myself.
However 80% of the peripheral chips have been modeled at the end. (The
20% is MPEG4/H.264 and SATA)

It's now known to work properly with U-boot, eCos, Linux and even Android Linux.
So I integrate the QEMU model into my open-source Faraday SDK, and thus
the QEMU modeled A369 would be the primary target platform from now on.

That's why I think the ECC function is optional to FTNANDC021, and the
reason why I want it to be integrated to U-Boot.

BTW, I'm still struggling for QEMU contribution. So all the stuff are
still available at my own github account only.
And of course, they're not the up-to-date source tree, because the
github is actually my secondary storage.

P.S: My github account is 'dantesu1218', there you could find the
stuff described above.
If you really want to have a try on it, I'll suggest starting from
'falinux', it's a copy-cat of uClinuc & buildroot.
Which means user only needs to run 'make menuconfig', falinux would
gather all the stuff for him.

>> >> +       /* 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?
>> >
>> >
>>
>> Because there is no any hardware access in 'read_byte'.
>> The FTNANDC021 simplily does not support byte access by default.
>> (I mean it's possible to update hardware RTL code,
>> but it's not the case to A369)
>> So I add some tricks to READ_ID, READ_OOB and READ_STATUS.
>> The actual hardware access is performed at ftnandc021_cmdfunc(),
>> The ftnandc021_read_byte() simpily access the cached data buffer.
>
>
> OK, so you're assuming that the OOB will never be larger than the 256-byte
> software buffer?
>
> -Scott



--
Best wishes,
Kuo-Jung Su

2013/4/23 Scott Wood <scottwood@freescale.com>:
> On 04/21/2013 09:45:00 PM, Kuo-Jung Su wrote:
>>
>> Sorry, it's my bad, I forget to clean it up.
>> In my private base, it's possible to turn-on ECC support to FTNADC021
>> with the following issues:
>>
>> 1. When ECC is enabled, FTNANDC021 would always do ECC on every data
>> block,
>>    including the blank block(all 0xff).
>>    What I mean is 'With ECC enabled, we'll have ECC errors on non-ecc
>> proected data blocks or even the blank block'.
>
>
> fsl_ifc_nand is similar -- when we see an uncorrectable error, we check
> whether the page is blank.
>
>
>> 2. FTNANDC021 would stop upon ECC errors, a chip reset is required to
>> make it work again.
>
>
> This sounds bad, though.  I assume that this is more than just resetting the
> NAND controller -- that you're resetting the entire system on a chip or
> similar?
>
>
>> 3. There are only 4 bytes data + 1 byte Bad Block Info available to
>> software,
>>    and because of the ECC issues above.
>>    I'll have to use 1 byte of data to tag if this block is 'not blank',
>>    to make it possible to cancel the operation on these blocks
>>    to prevent the chip to be stopped. (It means that I don't have to reset
>> chip)
>>    And thus if the ECC is enabled, there are only 3 data bytes
>> available to software,
>>    which makes it not possible to support BBT. (BBT requres 4 data bytes
>> in OOB)
>
>
> You could define a custom BBT descriptor that uses fewer bytes.
>
>
>> So in this patch, I'll prefer ECC disabled.
>> BTW, because of the issues above, this chip has been faced out many years
>> ago.
>> It would only be available to A369 or QEMU, and never in mass production.
>> And thus I think it's ok to have ECC disabled in this patch.
>
>
> Why do we need support in U-Boot, then?  Couldn't you just have QEMU model
> saner hardware?  What is A369?
>
>
>> >> +       /* 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?
>> >
>> >
>>
>> Because there is no any hardware access in 'read_byte'.
>> The FTNANDC021 simplily does not support byte access by default.
>> (I mean it's possible to update hardware RTL code,
>> but it's not the case to A369)
>> So I add some tricks to READ_ID, READ_OOB and READ_STATUS.
>> The actual hardware access is performed at ftnandc021_cmdfunc(),
>> The ftnandc021_read_byte() simpily access the cached data buffer.
>
>
> OK, so you're assuming that the OOB will never be larger than the 256-byte
> software buffer?
>

No, that's my bad.
I don't really understand NAND flash protocol 4 years ago, I'm now
re-writing the read/write stuff
with correct column address handling.

> -Scott



--
Best wishes,
Kuo-Jung Su

2013/4/24 Scott Wood <scottwood@freescale.com>:
> On 04/22/2013 08:19:18 PM, Kuo-Jung Su wrote:
>>
>> 2013/4/23 Scott Wood <scottwood@freescale.com>:
>> > On 04/21/2013 09:45:00 PM, Kuo-Jung Su wrote:
>> >> 2. FTNANDC021 would stop upon ECC errors, a chip reset is required to
>> >> make it work again.
>> >
>> >
>> > This sounds bad, though.  I assume that this is more than just resetting
>> > the
>> > NAND controller -- that you're resetting the entire system on a chip or
>> > similar?
>> >
>> >
>>
>> No, just reset the NAND controller. Since it's simply a peripheral device
>> attached to the bus, and supports only PIO mode.
>
>
> OK, so what's the downside to resetting the chip on an ECC error?
> Performance when reading a bunch of blank pages?
>
>

1. I'm lazy and coward.
2. It do impact the performance very much, because the ECC error is currently
   detected by a ~200ms NAND flash command timeout.

Now I plan to enable ECC in the next patch, and I'll check
if it's possible to detect ECC error more directly and cancel command wait.

P.S:
Both the RTL and datasheet of FTNANDC021 are buggy,
and the IP designer has quit from Faraday years.
I have to do it by trial and error.... so I'll always say 'I'll try....'

>> > You could define a custom BBT descriptor that uses fewer bytes.
>> >
>> >
>>
>> As far as I know, it's O.K to Linux, but requires some source file updated
>> to u-boot in old release. (i.e. u-boot-2009.01, which was the original
>> target release to FTNANDC021)
>> I'll check if it's now O.K to do that in u-boot. (I means no source
>> code needs to be modified, except FTNANDC021.[ch])
>
>
> The NAND code has been updated since then (currently corresponds to Linux
> 3.0), though I'm not sure what the problem would have been even back then.
>
>

That sounds good!
But I've found that BBT support has been dropped from default settings of Linux
by myself years ago, I simply forget it.
It means that I don't even need to create a customized BBT header.

P.S:
The BBT was turned on to accelerate bad block scan in old days.
Because the former driver written by one of my colleague, always read
a full page rather than oob only. It takes +5 min to scan bad blocks
from 256MB flash.
However the driver has been re-written with reading oob only at
several years ago,
it now takes < 1 sec to scan bad blocks from 512MB flash, so I drop
the BBT support by then.

>> A369 is a short for 'Faraday A369 SoC Evaluation Platform' with IC
>> design companies as
>> its target customers, which means A369 should never be the ASIC used
>> in mass production.
>> And the FTNANDC021 is actually designed for SSD applications, the
>> general software (e.g. linux)
>> is never put into consideration during IP/CHIP design process.
>>
>> It's an an accident that the FTNANDC021 get integrated into the A369.
>> Although the NAND controller of A369 is considered a bad chip, the
>> NAND flash is still a primary storage
>> to A369. So it still needs to be integrated to U-Boot to make
>> everything work correctly.
>>
>> The QEMU model for A369 is out several month ago, its primary target
>> is to provide ROM code development
>> environment for myself.
>> However 80% of the peripheral chips have been modeled at the end. (The
>> 20% is MPEG4/H.264 and SATA)
>>
>> It's now known to work properly with U-boot, eCos, Linux and even Android
>> Linux.
>> So I integrate the QEMU model into my open-source Faraday SDK, and thus
>> the QEMU modeled A369 would be the primary target platform from now on.
>>
>> That's why I think the ECC function is optional to FTNANDC021, and the
>> reason why I want it to be integrated to U-Boot.
>
>
> It could still be a problem during evaluation, though I guess you don't
> support MLC NAND chips which would need it the most.  I won't block it, but
> at least make sure it prints a prominent warning message.
>

Don't worry about that, I've made my mind to enable ECC at next patch.
Sorry for the mess that I've done.

> -Scott


--
Best wishes,
Kuo-Jung Su