[U-Boot] [PATCH] Add support s3c2440 NAND controller

[Marek Vasut]

Dear Scott Wood,

> On 04/19/2012 08:53 AM, Ilya Averyanov wrote:
> >  arch/arm/include/asm/arch-s3c24x0/s3c2410.h |    4 +-
> >  arch/arm/include/asm/arch-s3c24x0/s3c2440.h |    4 +-
> >  arch/arm/include/asm/arch-s3c24x0/s3c24x0.h |   13 +-
> >  drivers/mtd/nand/Makefile                   |    3 +-
> >  drivers/mtd/nand/s3c2410_nand.c             |  189 -------------------
> >  drivers/mtd/nand/s3c24x0_nand.c             |  268
> > 
> > +++++++++++++++++++++++++++
> > 
> >  6 files changed, 283 insertions(+), 198 deletions(-)
> >  delete mode 100644 drivers/mtd/nand/s3c2410_nand.c
> >  create mode 100644 drivers/mtd/nand/s3c24x0_nand.c
> > 
> > diff --git a/arch/arm/include/asm/arch-s3c24x0/s3c2410.h
> > b/arch/arm/include/asm/arch-s3c24x0/s3c2410.h
> > index 4fbdf20..71eb33e 100644
> > --- a/arch/arm/include/asm/arch-s3c24x0/s3c2410.h
> > +++ b/arch/arm/include/asm/arch-s3c24x0/s3c2410.h
> > @@ -99,9 +99,9 @@ static inline struct s3c24x0_lcd
> > *s3c24x0_get_base_lcd(void)
> > 
> >      return (struct s3c24x0_lcd *)S3C24X0_LCD_BASE;
> >  
> >  }
> > 
> > -static inline struct s3c2410_nand *s3c2410_get_base_nand(void)
> > +static inline struct s3c24x0_nand *s3c24x0_get_base_nand(void)
> > 
> >  {
> > 
> > -    return (struct s3c2410_nand *)S3C2410_NAND_BASE;
> > +    return (struct s3c24x0_nand *)S3C2410_NAND_BASE;
> > 
> >  }
> 
> Patch is whitespace damaged.
> 
> > diff --git a/drivers/mtd/nand/s3c2410_nand.c
> > b/drivers/mtd/nand/s3c2410_nand.c
> > deleted file mode 100644
> > index e1a459b..0000000
> > --- a/drivers/mtd/nand/s3c2410_nand.c
> > +++ /dev/null
> > @@ -1,189 +0,0 @@
> > -/*
> > - * (C) Copyright 2006 OpenMoko, Inc.
> > - * Author: Harald Welte <laforge at openmoko.org>
> > - *
> > - * 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.
> 
> Please pass -M -C to git format-patch so it detects renames and copies.
> 
> > +#ifdef CONFIG_NAND_SPL
> 
> Please try to make the new SPL subsystem work.  How tight is your space
> restriction?

Blasting 4k!!! He has so much space he can swim in it!

> 
> > +#ifdef    CONFIG_S3C2440
> 
> One space after #ifdef.
> 
> > +static void s3c2440_hwcontrol(struct mtd_info *mtd, int cmd, unsigned
> > int ctrl)
> > +{
> > +    struct nand_chip *chip = mtd->priv;
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +
> > +
> > +    debug("hwcontrol(): 0x%02x 0x%02x\n", cmd, ctrl);
> > +
> > +    if (ctrl & NAND_CTRL_CHANGE) {
> > +        ulong IO_ADDR_W = (ulong)nand;
> > +
> > +        if (!(ctrl & NAND_CLE))
> > +            IO_ADDR_W |= S3C2440_ADDR_NCLE;
> > +        if (!(ctrl & NAND_ALE))
> > +            IO_ADDR_W |= S3C2440_ADDR_NALE;
> > +
> > +        chip->IO_ADDR_W = (void *)IO_ADDR_W;
> > +
> > +        if (ctrl & NAND_NCE)
> > +            writel(readl(&nand->nfcont) & ~S3C2440_NFCONF_nFCE,
> > +                   &nand->nfcont);
> > +        else
> > +            writel(readl(&nand->nfcont) | S3C2440_NFCONF_nFCE,
> > +                   &nand->nfcont);
> > +    }
> > +
> > +    if (cmd != NAND_CMD_NONE)
> > +        writeb(cmd, chip->IO_ADDR_W);
> > +}
> > +
> > +static int s3c2440_dev_ready(struct mtd_info *mtd)
> > +{
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +    debug("dev_ready\n");
> > +    return readl(&nand->nfstat) & 0x01;
> > +}
> > +#else
> > +static void s3c2410_hwcontrol(struct mtd_info *mtd, int cmd, unsigned
> > int ctrl)
> > +{
> > +    struct nand_chip *chip = mtd->priv;
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +
> > +
> > +    debug("hwcontrol(): 0x%02x 0x%02x\n", cmd, ctrl);
> > +
> > +    if (ctrl & NAND_CTRL_CHANGE) {
> > +        ulong IO_ADDR_W = (ulong)nand;
> > +
> > +        if (!(ctrl & NAND_CLE))
> > +            IO_ADDR_W |= S3C2410_ADDR_NCLE;
> > +        if (!(ctrl & NAND_ALE))
> > +            IO_ADDR_W |= S3C2410_ADDR_NALE;
> > +
> > +        chip->IO_ADDR_W = (void *)IO_ADDR_W;
> > +
> > +        if (ctrl & NAND_NCE)
> > +            writel(readl(&nand->nfconf) & ~S3C2410_NFCONF_nFCE,
> > +                   &nand->nfconf);
> > +        else
> > +            writel(readl(&nand->nfconf) | S3C2410_NFCONF_nFCE,
> > +                   &nand->nfconf);
> > +    }
> > +
> > +    if (cmd != NAND_CMD_NONE)
> > +        writeb(cmd, chip->IO_ADDR_W);
> > +}
> > +
> > +static int s3c2410_dev_ready(struct mtd_info *mtd)
> > +{
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +    debug("dev_ready\n");
> > +    return readl(&nand->nfstat) & 0x01;
> > +}
> > +#endif
> 
> Do you really need to duplicate the code?  Can't you just put the
> relevant hardware constants in an ifdef, that redefines them with a
> non-hardware-specific name?
> 
> > +#ifdef CONFIG_SYS_S3C2440_NAND_HWECC
> > +void s3c2440_nand_enable_hwecc(struct mtd_info *mtd, int mode)
> > +{
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +    debug("s3c2410_nand_enable_hwecc(%p, %d)\n", mtd, mode);
> > +    writel((readl(&nand->nfcont) & ~(1 << 5)) | S3C2440_NFCONT_INITECC,
> > &nand->nfcont);
> > +}
> 
> Move that readl out of the writel for readability.
> 
> Symbolically define what 1 << 5 means (likewise for other magic numbers).
> 
> > +{
> > +    struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> > +    ecc_code[0] = readb(&nand->nfecc);
> > +    ecc_code[1] = readb(&nand->nfecc + 1);
> > +    ecc_code[2] = readb(&nand->nfecc + 2);
> > +    debug("s3c2410_nand_calculate_hwecc(%p,): 0x%02x 0x%02x 0x%02x\n",
> > +           mtd , ecc_code[0], ecc_code[1], ecc_code[2]);
> 
> No space before ,
> 
> > +#ifdef    CONFIG_S3C2440
> > +    writel(readl(&nand_reg->nfconf) | ((tacls-1)<<12)|((twrph0 -
> > 1)<<8)|((twrph1 - 1)<<4)|(0<<0), &nand_reg->nfconf);
> > +
> > writel((0<<13)|(0<<12)|(0<<10)|(0<<9)|(0<<8)|(1<<6)|(1<<5)|(1<<4)|(1<<1)|
> > (1<<0), &nand_reg->nfcont);
> > +#else
> > +    writel(S3C2410_NFCONF_EN|S3C2410_NFCONF_TACLS(tacls -
> > 1)|S3C2410_NFCONF_TWRPH0(twrph0 - 1)|S3C2410_NFCONF_TWRPH1(twrph1 - 1) ,
> > +            &nand_reg->nfconf);
> > +#endif
> 
> Come on, really?
> 
> > +#ifdef CONFIG_S3C2410_NAND_BBT
> > +    nand->options = NAND_USE_FLASH_BBT;
> > +#else
> > +    nand->options = 0;
> > +#endif
> 
> #ifdef CONFIG_S3C2410_NAND_BBT
> 	nand->options |= NAND_USE_FLASH_BBT;
> #endif
> 
> -Scott
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot