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

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

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

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

• }
• 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.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.

-Scott