2013/4/19 Scott Wood <[email protected]>:
> 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 <[email protected]>
>> + *
>> + * 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?
>
>
>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(®s->ior) & IOR_READY))
>> + ;
>> + *(uint32_t *)(buf + off) = NAND_READ(®s->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(®s->idr[0]),
>> + priv->buf);
>> + put_unaligned_le32(NAND_READ(®s->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 <[email protected]>
>> + *
>> + * 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
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
