[U-Boot] [PATCH v2 07/12] mtd: nand: add Faraday FTNANDC021 NAND controller support
Kuo-Jung Su
dantesu at gmail.com
Mon Apr 22 04:45:00 CEST 2013
2013/4/19 Scott Wood <scottwood at 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 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;
>
>
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, ®s->atr[0]);
>> +#endif
>> +#ifdef CONFIG_FTNANDC021_ACTIMING_2
>> + NAND_WRITE(CONFIG_FTNANDC021_ACTIMING_2, ®s->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, ®s->ier);
>> + NAND_WRITE(0, ®s->pir);
>> + NAND_WRITE(0xff, ®s->bbiwr);
>> + NAND_WRITE(0xffffffff, ®s->lsnwr);
>> + NAND_WRITE(0xffffffff, ®s->crcwr);
>> +
>> + if (chip->options & NAND_BUSWIDTH_16)
>> + NAND_WRITE(FCR_SWCRC | FCR_IGNCRC | FCR_16BIT,
>> ®s->fcr);
>> + else
>> + NAND_WRITE(FCR_SWCRC | FCR_IGNCRC, ®s->fcr);
>> +
>> + /* chip reset */
>> + NAND_WRITE(SRR_CHIP_RESET, ®s->srr);
>> +
>> + /* wait until chip ready */
>> + while (NAND_READ(®s->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(®s->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(®s->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?
>
>
More information about the U-Boot
mailing list