[U-Boot] [PATCH v3 09/11] S3C24XX: Add NAND Flash driver

["José Miguel Gonçalves" <jose.goncalves@inov.pt>]

On 18-09-2012 19:02, Scott Wood wrote:
> On 09/18/2012 12:40:36 PM, Jos? Miguel Gon?alves wrote:
>> NAND Flash driver with HW ECC for the S3C24XX SoCs.
>> Currently it only supports SLC NAND chips.
>>
>> Signed-off-by: Jos? Miguel Gon?alves <jose.goncalves@inov.pt>
>> ---
>> Changes for v2:
>>    - Coding style cleanup
>>    - Use of clrsetbits_le32()
>>    - Use of register bit macros instead of magic numbers
>>
>> Changes for v3:
>>    - Removed magic numbers
>>    - Removed a macro to declare a void printf()
>>    - Replaced one printf() with a puts()
>> ---
>>  drivers/mtd/nand/Makefile       |    1 +
>>  drivers/mtd/nand/s3c24xx_nand.c |  246 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 247 insertions(+)
>>  create mode 100644 drivers/mtd/nand/s3c24xx_nand.c
>>
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index 29dc20e..791ec44 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -60,6 +60,7 @@ COBJS-$(CONFIG_NAND_MXS) += mxs_nand.o
>>  COBJS-$(CONFIG_NAND_NDFC) += ndfc.o
>>  COBJS-$(CONFIG_NAND_NOMADIK) += nomadik.o
>>  COBJS-$(CONFIG_NAND_S3C2410) += s3c2410_nand.o
>> +COBJS-$(CONFIG_NAND_S3C24XX) += s3c24xx_nand.o
>>  COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o
>>  COBJS-$(CONFIG_NAND_SPEAR) += spr_nand.o
>>  COBJS-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
>> diff --git a/drivers/mtd/nand/s3c24xx_nand.c b/drivers/mtd/nand/s3c24xx_nand.c
>> new file mode 100644
>> index 0000000..3c13709
>> --- /dev/null
>> +++ b/drivers/mtd/nand/s3c24xx_nand.c
>> @@ -0,0 +1,246 @@
>> +/*
>> + * (C) Copyright 2012 INOV - INESC Inovacao
>> + * Jose Goncalves <jose.goncalves@inov.pt>
>> + *
>> + * Based on drivers/mtd/nand/s3c64xx.c and U-Boot 1.3.4 from Samsung.
>> + * Supports only SLC NAND Flash chips.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <nand.h>
>> +#include <asm/io.h>
>> +#include <asm/errno.h>
>> +#include <asm/arch/s3c24xx_cpu.h>
>> +
>> +#define TACLS_VAL    7    /* CLE & ALE duration setting (0~7) */
>> +#define    TWRPH0_VAL    7    /* TWRPH0 duration setting (0~7) */
>> +#define TWRPH1_VAL    7    /* TWRPH1 duration setting (0~7) */
>
> Please use space, not tab, as a word separator (after the second #define).

OK.

>
>> +
>> +#define MAX_CHIPS    2
>> +static int nand_cs[MAX_CHIPS] = { 0, 1 };
>
> This needs explanation (and const).  Better would be to use a priv struct, as 
> discussed before.
>
>> +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +    struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>> +    u_long nfcont;
>
> s/u_long/u32/

Didn't catch this...

>
>> +
>> +    nfcont = readl(&nand->nfcont);
>> +
>> +    switch (chip) {
>> +    case -1:
>> +        nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
>> +        break;
>> +    case 0:
>> +        nfcont &= ~NFCONT_NCE0;
>> +        break;
>> +    case 1:
>> +        nfcont &= ~NFCONT_NCE1;
>> +        break;
>> +    default:
>> +        return;
>
> error message on default?

OK.

>
>> +    }
>> +
>> +    writel(nfcont, &nand->nfcont);
>> +}
>> +
>> +/*
>> + * Hardware specific access to control-lines function
>> + */
>> +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>> +{
>> +    struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>> +    struct nand_chip *this = mtd->priv;
>> +
>> +    if (ctrl & NAND_CTRL_CHANGE) {
>> +        if (ctrl & NAND_CLE)
>> +            this->IO_ADDR_W = &nand->nfcmmd;
>> +        else if (ctrl & NAND_ALE)
>> +            this->IO_ADDR_W = &nand->nfaddr;
>> +        else
>> +            this->IO_ADDR_W = &nand->nfdata;
>> +        if (ctrl & NAND_NCE)
>> +            s3c_nand_select_chip(mtd, *(int *)this->priv);
>> +        else
>> +            s3c_nand_select_chip(mtd, -1);
>> +    }
>> +
>> +    if (cmd != NAND_CMD_NONE)
>> +        writeb(cmd, this->IO_ADDR_W);
>> +}
>
> As discussed earlier, do you really need to mess with IO_ADDR_W or can you do it 
> the way ndfc.c does?

I will take a look at ndfc.c. Most of this driver was copy-paste from s3c64xx.c 
driver and an older patched U-Boot sources from Samsung, so I did not make any 
real code examination after it started to work...

>
> I.e.:
>
> static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
> {
>     struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>     struct nand_chip *this = mtd->priv;
>
>     if (cmd == NAND_CMD_NONE)
>         return;
>
>     if (ctrl & NAND_CLE)
>         writeb(cmd, &nand->nfcmmd);
>     else
>         writeb(cmd, &nand->nfaddr);
> }
>
>> +/*
>> + * Board-specific NAND initialization.
>> + */
>> +int board_nand_init(struct nand_chip *nand)
>> +{
>> +    static int chip_n;
>> +    struct s3c24xx_nand *const nand_reg = s3c24xx_get_base_nand();
>> +
>> +    if (chip_n == 0) {
>> +        /* Extend NAND timings to the maximum */
>> +        clrsetbits_le32(&nand_reg->nfconf,
>> +                NFCONF_TACLS_MASK | NFCONF_TWRPH0_MASK |
>> +                NFCONF_TWRPH1_MASK,
>> +                NFCONF_TACLS(TACLS_VAL) |
>> +                NFCONF_TWRPH0(TWRPH0_VAL) |
>> +                NFCONF_TWRPH1(TWRPH1_VAL));
>> +
>> +        /* Disable chip selects and soft lock, enable controller */
>> +        clrsetbits_le32(&nand_reg->nfcont, NFCONT_WP,
>> +                NFCONT_NCE1 | NFCONT_NCE0 | NFCONT_ENABLE);
>> +    } else if (chip_n >= MAX_CHIPS) {
>> +        return -ENODEV;
>> +    }
>> +
>> +    nand->IO_ADDR_R = &nand_reg->nfdata;
>> +    nand->IO_ADDR_W = &nand_reg->nfdata;
>> +    nand->cmd_ctrl = s3c_nand_hwcontrol;
>> +    nand->dev_ready = s3c_nand_device_ready;
>> +    nand->select_chip = s3c_nand_select_chip;
>> +    nand->options = 0;
>> +#ifdef CONFIG_SPL_BUILD
>> +    nand->read_buf = nand_read_buf;
>> +#endif
>> +
>> +#ifdef CONFIG_S3C24XX_NAND_HWECC
>> +    nand->ecc.hwctl = s3c_nand_enable_hwecc;
>> +    nand->ecc.calculate = s3c_nand_calculate_ecc;
>> +    nand->ecc.correct = s3c_nand_correct_data;
>> +    nand->ecc.mode = NAND_ECC_HW_OOB_FIRST;
>> +    nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
>> +    nand->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES;
>> +#else
>> +    nand->ecc.mode = NAND_ECC_SOFT;
>> +#endif /* ! CONFIG_S3C24XX_NAND_HWECC */
>> +
>> +    nand->priv = nand_cs + chip_n++;
>> +
>> +    return 0;
>> +}
>
> Please consider using the new SELF_INIT mechanism.
>

Can you explain and/or point_to_resources for what this means (I'm a U-Boot newbie)...

Best regards,
Jos? Gon?alves