ARM: OMAP3: Add memory and syslib common files, add NAND support

PATCH Superseded dirk.behme at googlemail.com 4 participants 13 messages Oct 3, 2008 – Oct 7, 2008

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

dirk.behme at googlemail.com

author

2008-10-03 10:40 UTC #0 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Subject: [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

From: Dirk Behme <[email protected]>

Add memory and syslib common files, add NAND support

Signed-off-by Dirk Behme <[email protected]>

---
Changes in version v2:

- Move common ARM Cortex A8 code to cpu/arm_cortexa8/ and OMAP3 SoC specific common code to cpu/arm_cortexa8/omap3 as proposed by Wolfgang.

 common/cmd_nand.c               |   29 ++
 cpu/arm_cortexa8/omap3/Makefile |    2 
 cpu/arm_cortexa8/omap3/mem.c    |  301 ++++++++++++++++++++++++++++++
 cpu/arm_cortexa8/omap3/nand.c   |  399 ++++++++++++++++++++++++++++++++++++++++
 cpu/arm_cortexa8/omap3/syslib.c |   72 +++++++
 drivers/mtd/nand/nand_base.c    |  145 ++++++++++++++
 examples/Makefile               |    6 
 7 files changed, 951 insertions(+), 3 deletions(-)

cpu/arm_cortexa8/omap3/mem.c new file +301 -0

Index: u-boot-arm/cpu/arm_cortexa8/omap3/mem.c
===================================================================
@@ -0,0 +1,301 @@
+/*
+ * (C) Copyright 2008
+ * Texas Instruments, <www.ti.com>
+ *
+ * Author :
+ *     Manikandan Pillai <[email protected]>
+ *
+ * Initial Code from:
+ *     Richard Woodruff <[email protected]>
+ *     Syed Mohammed Khasim <[email protected]>
+ *
+ * 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 <asm/io.h>
+#include <asm/arch/bits.h>
+#include <asm/arch/mem.h>
+#include <asm/arch/sys_proto.h>
+#include <command.h>
+
+/* Only One NAND allowed on board at a time.
+ * The GPMC CS Base for the same
+ */
+unsigned int boot_flash_base;
+unsigned int boot_flash_off;
+unsigned int boot_flash_sec;
+unsigned int boot_flash_type;
+volatile unsigned int boot_flash_env_addr;
+
+/* help common/env_flash.c */
+#ifdef ENV_IS_VARIABLE
+
+uchar(*boot_env_get_char_spec) (int index);
+int (*boot_env_init) (void);
+int (*boot_saveenv) (void);
+void (*boot_env_relocate_spec) (void);
+
+/* 16 bit NAND */
+uchar env_get_char_spec(int index);
+int env_init(void);
+int saveenv(void);
+void env_relocate_spec(void);
+extern char *env_name_spec;
+
+#if defined(CONFIG_CMD_NAND)
+u8 is_nand;
+#endif
+
+#if defined(CONFIG_CMD_ONENAND)
+u8 is_onenand;
+#endif
+
+#endif /* ENV_IS_VARIABLE */
+
+#if defined(CONFIG_CMD_NAND)
+static u32 gpmc_m_nand[GPMC_MAX_REG] = {
+	M_NAND_GPMC_CONFIG1,
+	M_NAND_GPMC_CONFIG2,
+	M_NAND_GPMC_CONFIG3,
+	M_NAND_GPMC_CONFIG4,
+	M_NAND_GPMC_CONFIG5,
+	M_NAND_GPMC_CONFIG6, 0
+};
+unsigned int nand_cs_base;
+#endif
+
+#if defined(CONFIG_CMD_ONENAND)
+static u32 gpmc_onenand[GPMC_MAX_REG] = {
+	ONENAND_GPMC_CONFIG1,
+	ONENAND_GPMC_CONFIG2,
+	ONENAND_GPMC_CONFIG3,
+	ONENAND_GPMC_CONFIG4,
+	ONENAND_GPMC_CONFIG5,
+	ONENAND_GPMC_CONFIG6, 0
+};
+unsigned int onenand_cs_base;
+
+#endif
+
+/**************************************************************************
+ * make_cs1_contiguous() - for es2 and above remap cs1 behind cs0 to allow
+ *  command line mem=xyz use all memory with out discontinuous support
+ *  compiled in.  Could do it at the ATAG, but there really is two banks...
+ * Called as part of 2nd phase DDR init.
+ **************************************************************************/
+void make_cs1_contiguous(void)
+{
+	u32 size, a_add_low, a_add_high;
+
+	size = get_sdr_cs_size(SDRC_CS0_OSET);
+	size /= SZ_32M;		        /* find size to offset CS1 */
+	a_add_high = (size & 3) << 8;	/* set up low field */
+	a_add_low = (size & 0x3C) >> 2;	/* set up high field */
+	__raw_writel((a_add_high | a_add_low), SDRC_CS_CFG);
+
+}
+
+/********************************************************
+ *  mem_ok() - test used to see if timings are correct
+ *             for a part. Helps in guessing which part
+ *             we are currently using.
+ *******************************************************/
+u32 mem_ok(void)
+{
+	u32 val1, val2, addr;
+	u32 pattern = 0x12345678;
+
+	addr = OMAP34XX_SDRC_CS0;
+
+	__raw_writel(0x0, addr + 0x400);  /* clear pos A */
+	__raw_writel(pattern, addr);	  /* pattern to pos B */
+	__raw_writel(0x0, addr + 4);	  /* remove pattern off the bus */
+	val1 = __raw_readl(addr + 0x400); /* get pos A value */
+	val2 = __raw_readl(addr);	  /* get val2 */
+
+	if ((val1 != 0) || (val2 != pattern))  /* see if pos A value changed */
+		return 0;
+	else
+		return 1;
+}
+
+/********************************************************
+ *  sdrc_init() - init the sdrc chip selects CS0 and CS1
+ *  - early init routines, called from flash or
+ *  SRAM.
+ *******************************************************/
+void sdrc_init(void)
+{
+	/* only init up first bank here */
+	do_sdrc_init(SDRC_CS0_OSET, EARLY_INIT);
+}
+
+/*************************************************************************
+ * do_sdrc_init(): initialize the SDRAM for use.
+ *  -code sets up SDRAM basic SDRC timings for CS0
+ *  -optimal settings can be placed here, or redone after i2c
+ *      inspection of board info
+ *
+ *  - code called ones in C-Stack only context for CS0 and a possible 2nd
+ *      time depending on memory configuration from stack+global context
+ **************************************************************************/
+
+void do_sdrc_init(u32 offset, u32 early)
+{
+
+	/* reset sdrc controller */
+	__raw_writel(SOFTRESET, SDRC_SYSCONFIG);
+	wait_on_value(BIT0, BIT0, SDRC_STATUS, 12000000);
+	__raw_writel(0, SDRC_SYSCONFIG);
+
+	/* setup sdrc to ball mux */
+	__raw_writel(SDP_SDRC_SHARING, SDRC_SHARING);
+
+	/* SDRC_MCFG0 register */
+	(*(unsigned int *) 0x6D000080) = 0x02584099; /* from Micron */
+
+	/* SDRC_RFR_CTRL0 register */
+	(*(unsigned int *) 0x6D0000a4) = 0x54601;    /* for 166M */
+
+	/* SDRC_ACTIM_CTRLA0 register */
+	(*(unsigned int *) 0x6D00009c) = 0xa29db4c6; /* for 166M */
+
+	/* SDRC_ACTIM_CTRLB0 register */
+	(*(unsigned int *) 0x6D0000a0) = 0x12214;    /* for 166M */
+
+	/* Disble Power Down of CKE cuz of 1 CKE on combo part */
+	(*(unsigned int *) 0x6D000070) = 0x00000081;
+
+	/* SDRC_Manual command register */
+	(*(unsigned int *) 0x6D0000a8) = 0x00000000; /* NOP command */
+	(*(unsigned int *) 0x6D0000a8) = 0x00000001; /* Precharge command */
+	(*(unsigned int *) 0x6D0000a8) = 0x00000002; /* Auto-refresh command */
+	(*(unsigned int *) 0x6D0000a8) = 0x00000002; /* Auto-refresh command */
+
+	/* SDRC MR0 register */
+	(*(int *) 0x6D000084) = 0x00000032;	/*  Burst length = 4 */
+	/* CAS latency = 3, Write Burst = Read Burst Serial Mode */
+
+	/* SDRC DLLA control register */
+	(*(unsigned int *) 0x6D000060) = 0x0000A;
+	sdelay(0x20000);
+}
+
+void enable_gpmc_config(u32 *gpmc_config, u32 gpmc_base, u32 base, u32 size)
+{
+	__raw_writel(0, GPMC_CONFIG7 + gpmc_base);
+	sdelay(1000);
+	/* Delay for settling */
+	__raw_writel(gpmc_config[0], GPMC_CONFIG1 + gpmc_base);
+	__raw_writel(gpmc_config[1], GPMC_CONFIG2 + gpmc_base);
+	__raw_writel(gpmc_config[2], GPMC_CONFIG3 + gpmc_base);
+	__raw_writel(gpmc_config[3], GPMC_CONFIG4 + gpmc_base);
+	__raw_writel(gpmc_config[4], GPMC_CONFIG5 + gpmc_base);
+	__raw_writel(gpmc_config[5], GPMC_CONFIG6 + gpmc_base);
+	/* Enable the config */
+	__raw_writel((((size & 0xF) << 8) | ((base >> 24) & 0x3F) |
+		      (1 << 6)), GPMC_CONFIG7 + gpmc_base);
+	sdelay(2000);
+}
+
+/*****************************************************
+ * gpmc_init(): init gpmc bus
+ * Init GPMC for x16, MuxMode (SDRAM in x32).
+ * This code can only be executed from SRAM or SDRAM.
+ *****************************************************/
+void gpmc_init(void)
+{
+	/* putting a blanket check on GPMC based on ZeBu for now */
+	u32 mux = 0, mwidth;
+	u32 *gpmc_config = NULL;
+	u32 gpmc_base = 0;
+	u32 base = 0;
+	u32 size = 0;
+	u32 f_off = CONFIG_SYS_MONITOR_LEN;
+	u32 f_sec = 0;
+	u32 config = 0;
+
+	mux = BIT9;
+	mwidth = get_gpmc0_width();
+
+	/* global settings */
+	__raw_writel(0x0, GPMC_IRQENABLE);	/* isr's sources masked */
+	__raw_writel(0, GPMC_TIMEOUT_CONTROL);	/* timeout disable */
+
+	config = __raw_readl(GPMC_CONFIG);
+	config &= (~0xf00);
+	__raw_writel(config, GPMC_CONFIG);
+
+	/* Disable the GPMC0 config set by ROM code
+	 * It conflicts with our MPDB (both@0x08000000)
+	 */
+	__raw_writel(0, GPMC_CONFIG7 + GPMC_CONFIG_CS0);
+	sdelay(1000);
+
+#if defined(CONFIG_CMD_NAND)    /* CS 0 */
+	gpmc_config = gpmc_m_nand;
+#if defined(CONFIG_ENV_IS_IN_NAND)
+	gpmc_base = GPMC_CONFIG_CS0 + (0 * GPMC_CONFIG_WIDTH);
+#else
+	gpmc_base = GPMC_CONFIG_CS0 + (1 * GPMC_CONFIG_WIDTH);
+#endif
+	base = PISMO1_NAND_BASE;
+	size = PISMO1_NAND_SIZE;
+	enable_gpmc_config(gpmc_config, gpmc_base, base, size);
+	is_nand = 1;
+	nand_cs_base = gpmc_base;
+#if defined(CONFIG_ENV_IS_IN_NAND)
+	f_off = SMNAND_ENV_OFFSET;
+	f_sec = SZ_128K;
+	/* env setup */
+	boot_flash_base = base;
+	boot_flash_off = f_off;
+	boot_flash_sec = f_sec;
+	boot_flash_env_addr = f_off;
+#endif
+#endif
+
+#if defined(CONFIG_CMD_ONENAND)
+	gpmc_config = gpmc_onenand;
+#if defined(CONFIG_ENV_IS_IN_ONENAND)
+	gpmc_base = GPMC_CONFIG_CS0 + (0 * GPMC_CONFIG_WIDTH);
+#else
+	gpmc_base = GPMC_CONFIG_CS0 + (1 * GPMC_CONFIG_WIDTH);
+#endif
+	base = PISMO1_ONEN_BASE;
+	size = PISMO1_ONEN_SIZE;
+	enable_gpmc_config(gpmc_config, gpmc_base, base, size);
+	is_onenand = 1;
+	onenand_cs_base = gpmc_base;
+#if defined(CONFIG_ENV_IS_IN_ONENAND)
+	f_off = ONENAND_ENV_OFFSET;
+	f_sec = SZ_128K;
+	/* env setup */
+	boot_flash_base = base;
+	boot_flash_off = f_off;
+	boot_flash_sec = f_sec;
+	boot_flash_env_addr = f_off;
+#endif
+#endif
+
+#ifdef ENV_IS_VARIABLE
+	boot_env_get_char_spec = env_get_char_spec;
+	boot_env_init = env_init;
+	boot_saveenv = saveenv;
+	boot_env_relocate_spec = env_relocate_spec;
+#endif
+}

cpu/arm_cortexa8/omap3/syslib.c new file +72 -0

72 lines not on this page · raw message

cpu/arm_cortexa8/omap3/nand.c new file +399 -0

399 lines not on this page · raw message

cpu/arm_cortexa8/omap3/Makefile +1 -1

Index: u-boot-arm/cpu/arm_cortexa8/omap3/Makefile
===================================================================
@@ -26,7 +26,7 @@ include $(TOPDIR)/config.mk
 LIB	= lib$(SOC).a
 
 SOBJS	:= lowlevel_init.o
-OBJS	:= sys_info.o board.o clock.o interrupts.o
+OBJS	:= sys_info.o board.o clock.o interrupts.o mem.o syslib.o nand.o
 
 all:	.depend $(LIB)

examples/Makefile +4 -2

Index: u-boot-arm/examples/Makefile
===================================================================
@@ -30,10 +30,12 @@ LOAD_ADDR = 0x40000
 endif
 
 ifeq ($(ARCH),arm)
+LOAD_ADDR = 0xc100000
 ifeq ($(BOARD),omap2420h4)
 LOAD_ADDR = 0x80300000
-else
-LOAD_ADDR = 0xc100000
+endif
+ifeq ($(CPU),omap3)
+LOAD_ADDR = 0x80300000
 endif
 endif

common/cmd_nand.c +29 -0

Index: u-boot-arm/common/cmd_nand.c
===================================================================
@@ -38,6 +38,11 @@ int find_dev_and_part(const char *id, st
 		      u8 *part_num, struct part_info **part);
 #endif
 
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE)\
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+extern void omap_nand_switch_ecc(nand_info_t *nand, int hardware);
+#endif
+
 static int nand_dump(nand_info_t *nand, ulong off, int only_oob)
 {
 	int i;

@@ -234,6 +239,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
 	    strncmp(cmd, "read", 4) != 0 && strncmp(cmd, "write", 5) != 0 &&
 	    strcmp(cmd, "scrub") != 0 && strcmp(cmd, "markbad") != 0 &&
 	    strcmp(cmd, "biterr") != 0 &&
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE)\
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+	    strncmp(cmd, "ecc", 3) != 0 &&
+#endif
 	    strcmp(cmd, "lock") != 0 && strcmp(cmd, "unlock") != 0 )
 		goto usage;

@@ -318,6 +327,22 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
 
 	}
 
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+	if (strncmp(cmd, "ecc", 3) == 0) {
+		if (argc < 2)
+			goto usage;
+		if (strncmp(argv[2], "hw", 2) == 0)
+			omap_nand_switch_ecc(nand, 1);
+		else if (strncmp(argv[2], "sw", 2) == 0)
+			omap_nand_switch_ecc(nand, 0);
+		else
+			goto usage;
+
+		return 0;
+	}
+#endif
+
 	if (strncmp(cmd, "read", 4) == 0 || strncmp(cmd, "write", 5) == 0) {
 		int read;

@@ -483,6 +508,10 @@ U_BOOT_CMD(nand, 5, 1, do_nand,
 	   "nand scrub - really clean NAND erasing bad blocks (UNSAFE)\n"
 	   "nand markbad off - mark bad block at offset (UNSAFE)\n"
 	   "nand biterr off - make a bit error@offset (UNSAFE)\n"
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+	   "nand ecc [hw/sw] - switch the ecc calculation algorithm \n"
+#endif
 	   "nand lock [tight] [status]\n"
 	   "    bring nand to lock state or display locked pages\n"
 	   "nand unlock [offset] [size] - unlock section\n");

drivers/mtd/nand/nand_base.c +145 -0

151 lines not on this page · raw message

Scott Wood

2008-10-03 16:52 UTC #1 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

cpu/arm_cortexa8/omap3/nand.c line 31

Quoted 8 lines · dirk.behme at googlemail.com

cpu/arm_cortexa8/omap3/nand.c new file +8 -0

+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/mem.h>
+#include <linux/mtd/nand_ecc.h>
+
+#if defined(CONFIG_CMD_NAND)
+
+#include <nand.h>

Move the #ifdef to the Makefile.

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c new file +8 -0

+/*
+ * nand_read_buf16 - [DEFAULT] read chip data into buffer
+ * @mtd:	MTD device structure
+ * @buf:	buffer to store date
+ * @len:	number of bytes to read
+ *
+ * Default read function for 16bit buswith
+ */

cpu/arm_cortexa8/omap3/nand.c line 136

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+	u16 *p = (u16 *) buf;
+	len >>= 1;
+
+	for (i = 0; i < len; i++)
+		p[i] = readw(this->IO_ADDR_R);
+}

How does this differ from the default implementation?

Quoted 22 lines

cpu/arm_cortexa8/omap3/nand.c new file +22 -0

+static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+				int len)
+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		writeb(buf[i], chip->IO_ADDR_W);
+		for (j = 0; j < 10; j++) ;
+	}
+
+}
+
+/*
+ * omap_nand_read_buf - read data from NAND controller into buffer
+ * @mtd:        MTD device structure
+ * @buf:        buffer to store date
+ * @len:        number of bytes to read
+ *
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)

cpu/arm_cortexa8/omap3/nand.c line 177

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = readb(chip->IO_ADDR_R);
+		while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS) & GPMC_BUF_FULL));
+	}
+}

I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL] when writing, but with 8-bit NAND, you check it when reading? And 8-bit writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads have no delay at all?

cpu/arm_cortexa8/omap3/nand.c line 195

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+static void omap_hwecc_init(struct nand_chip *chip)
+{
+	unsigned long val = 0x0;
+
+	/* Init ECC Control Register */
+	/* Clear all ECC  | Enable Reg1 */
+	val = ((0x00000001 << 8) | 0x00000001);
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
+	__raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
+}

Why raw?

Quoted 4 lines

cpu/arm_cortexa8/omap3/nand.c new file +4 -0

+/*
+ * omap_correct_data - Compares the ecc read from nand spare area with
+ *                     ECC registers values
+ *			and corrects one bit error if it has occured

cpu/arm_cortexa8/omap3/nand.c line 210

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+ * @mtd:		 MTD device structure
+ * @dat:		 page data
+ * @read_ecc:		 ecc read from nand flash
+ * @calc_ecc: 		 ecc read from ECC registers
+ */
+static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
+			     u_char *read_ecc, u_char *calc_ecc)
+{
+	return 0;
+}

This obviously is not correcting anything. If the errors are corrected automatically by the controller, say so in the comments.

Quoted 8 lines

+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *

+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.

Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?

Quoted 7 lines

+ *  @mtd:	MTD structure
+ *  @dat:	unused
+ *  @ecc_code:	ecc_code buffer

+ */

cpu/arm_cortexa8/omap3/nand.c new file +3 -0

+static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+			      u_char *ecc_code)
+{

cpu/arm_cortexa8/omap3/nand.c line 236

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+	unsigned long val = 0x0;
+	unsigned long reg;
+
+	/* Start Reading from HW ECC1_Result = 0x200 */
+	reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
+	val = __raw_readl(reg);
+
+	*ecc_code++ = ECC_P1_128_E(val);
+	*ecc_code++ = ECC_P1_128_O(val);
+	*ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val) << 4;

These macros are used nowhere else; why obscure what it's doing by moving it to the top of the file?

Quoted 19 lines

cpu/arm_cortexa8/omap3/nand.c new file +19 -0

+static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG);
+	unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
+
+	switch (mode) {
+	case NAND_ECC_READ:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_READSYN:
+		__raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_WRITE:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);

cpu/arm_cortexa8/omap3/nand.c line 274

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	default:
+		printf("Error: Unrecognized Mode[%d]!\n", mode);
+		break;
+	}
+
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONFIG);
+}

Is it OK if config gets written before control, or if this whole thing gets done out of order with respect to other raw writes?

cpu/arm_cortexa8/omap3/nand.c line 282

Quoted 7 lines

cpu/arm_cortexa8/omap3/nand.c new file +7 -0

+static struct nand_ecclayout hw_nand_oob_64 = {
+	.eccbytes = 12,
+	.eccpos = {
+		   2, 3, 4, 5,
+		   6, 7, 8, 9,
+		   10, 11, 12, 13},
+	.oobfree = { {20, 50} }	/* don't care */

Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?

cpu/arm_cortexa8/omap3/nand.c line 326

Quoted 7 lines

cpu/arm_cortexa8/omap3/nand.c new file +7 -0

+	if (nand->options & NAND_BUSWIDTH_16) {
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 2);
+		if (nand->ecc.layout->eccbytes & 0x01)
+			mtd->oobavail--;
+	} else
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 1);
+}

oobavail is calculated by the generic NAND code. You don't need to do it here.

Quoted 9 lines

+/*
+ * Board-specific NAND initialization. The following members of the
+ * argument are board-specific (per include/linux/mtd/nand_new.h):
+ * - IO_ADDR_R?: address to read the 8 I/O lines of the flash device
+ * - IO_ADDR_W?: address to write the 8 I/O lines of the flash device
+ * - hwcontrol: hardwarespecific function for accesing control-lines
+ * - dev_ready: hardwarespecific function for accesing device ready/busy line
+ * - enable_hwecc?: function to enable (reset) hardware ecc generator. Must
+ * only be provided if a hardware ECC is available

Quoted 8 lines

+ * - eccmode: mode of ecc, see defines
+ * - chip_delay: chip dependent delay for transfering data from array to
+ * read regs (tR)
+ * - options: various chip options. They can partly be set to inform
+ * nand_scan about special functionality. See the defines for further
+ * explanation
+ * Members with a "?" were not set in the merged testing-NAND branch,
+ * so they are not set here either.

IO_ADDR_R and IO_ADDR_W have a "?" but are set here. If you have a question about the API, ask it on the list, rather than encoding it in the source.

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 2735

Quoted 10 lines

===================================================================

drivers/mtd/nand/nand_base.c +3 -0

@@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
 	return 0;
 }
 
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+void nand_switch_ecc(struct mtd_info *mtd)

NACK. First, explain why you need to be able to dynamically switch ECC modes.

Then, if it is justified, implement it in a separate patch, without all the duplication of code, and without platform-specific #ifdefs.

-Scott

Dirk Behme

author

2008-10-03 17:08 UTC #2 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Scott,

many thanks for the review!

As this code is directly taken from some TI code, it seems I have to find somebody who can answer your questions and rework the code now. Will do so now. Unfortunately, I don't know a lot about NAND.

Thanks

Dirk

Quoted 255 lines · Scott Wood

cpu/arm_cortexa8/omap3/nand.c line 31
Quoted 8 lines · dirk.behme at googlemail.com
cpu/arm_cortexa8/omap3/nand.c new file +8 -0
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/mem.h>
+#include <linux/mtd/nand_ecc.h>
+
+#if defined(CONFIG_CMD_NAND)
+
+#include <nand.h>
Move the #ifdef to the Makefile.
Quoted 8 lines
cpu/arm_cortexa8/omap3/nand.c new file +8 -0
+/*
+ * nand_read_buf16 - [DEFAULT] read chip data into buffer
+ * @mtd:	MTD device structure
+ * @buf:	buffer to store date
+ * @len:	number of bytes to read
+ *
+ * Default read function for 16bit buswith
+ */
cpu/arm_cortexa8/omap3/nand.c line 136
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+	u16 *p = (u16 *) buf;
+	len >>= 1;
+
+	for (i = 0; i < len; i++)
+		p[i] = readw(this->IO_ADDR_R);
+}
How does this differ from the default implementation?
Quoted 22 lines
cpu/arm_cortexa8/omap3/nand.c new file +22 -0
+static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+				int len)
+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		writeb(buf[i], chip->IO_ADDR_W);
+		for (j = 0; j < 10; j++) ;
+	}
+
+}
+
+/*
+ * omap_nand_read_buf - read data from NAND controller into buffer
+ * @mtd:        MTD device structure
+ * @buf:        buffer to store date
+ * @len:        number of bytes to read
+ *
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
cpu/arm_cortexa8/omap3/nand.c line 177
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = readb(chip->IO_ADDR_R);
+		while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS) & GPMC_BUF_FULL));
+	}
+}
I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL] when writing, but with 8-bit NAND, you check it when reading? And 8-bit writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads have no delay at all?
cpu/arm_cortexa8/omap3/nand.c line 195
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+static void omap_hwecc_init(struct nand_chip *chip)
+{
+	unsigned long val = 0x0;
+
+	/* Init ECC Control Register */
+	/* Clear all ECC  | Enable Reg1 */
+	val = ((0x00000001 << 8) | 0x00000001);
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
+	__raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
+}
Why raw?
Quoted 4 lines
cpu/arm_cortexa8/omap3/nand.c new file +4 -0
+/*
+ * omap_correct_data - Compares the ecc read from nand spare area with
+ *                     ECC registers values
+ *			and corrects one bit error if it has occured
cpu/arm_cortexa8/omap3/nand.c line 210
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+ * @mtd:		 MTD device structure
+ * @dat:		 page data
+ * @read_ecc:		 ecc read from nand flash
+ * @calc_ecc: 		 ecc read from ECC registers
+ */
+static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
+			     u_char *read_ecc, u_char *calc_ecc)
+{
+	return 0;
+}
This obviously is not correcting anything. If the errors are corrected automatically by the controller, say so in the comments.
Quoted 8 lines
+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.
Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?
Quoted 7 lines
+ *  @mtd:	MTD structure
+ *  @dat:	unused
+ *  @ecc_code:	ecc_code buffer
+ */
cpu/arm_cortexa8/omap3/nand.c new file +3 -0
+static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+			      u_char *ecc_code)
+{
cpu/arm_cortexa8/omap3/nand.c line 236
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+	unsigned long val = 0x0;
+	unsigned long reg;
+
+	/* Start Reading from HW ECC1_Result = 0x200 */
+	reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
+	val = __raw_readl(reg);
+
+	*ecc_code++ = ECC_P1_128_E(val);
+	*ecc_code++ = ECC_P1_128_O(val);
+	*ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val) << 4;
These macros are used nowhere else; why obscure what it's doing by moving it to the top of the file?
Quoted 19 lines
cpu/arm_cortexa8/omap3/nand.c new file +19 -0
+static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG);
+	unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
+
+	switch (mode) {
+	case NAND_ECC_READ:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_READSYN:
+		__raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_WRITE:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
cpu/arm_cortexa8/omap3/nand.c line 274
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c new file +10 -0
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	default:
+		printf("Error: Unrecognized Mode[%d]!\n", mode);
+		break;
+	}
+
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONFIG);
+}
Is it OK if config gets written before control, or if this whole thing gets done out of order with respect to other raw writes?
cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 7 lines
cpu/arm_cortexa8/omap3/nand.c new file +7 -0
+static struct nand_ecclayout hw_nand_oob_64 = {
+	.eccbytes = 12,
+	.eccpos = {
+		   2, 3, 4, 5,
+		   6, 7, 8, 9,
+		   10, 11, 12, 13},
+	.oobfree = { {20, 50} }	/* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?
cpu/arm_cortexa8/omap3/nand.c line 326
Quoted 7 lines
cpu/arm_cortexa8/omap3/nand.c new file +7 -0
+	if (nand->options & NAND_BUSWIDTH_16) {
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 2);
+		if (nand->ecc.layout->eccbytes & 0x01)
+			mtd->oobavail--;
+	} else
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 1);
+}
oobavail is calculated by the generic NAND code. You don't need to do it here.
Quoted 9 lines
+/*
+ * Board-specific NAND initialization. The following members of the
+ * argument are board-specific (per include/linux/mtd/nand_new.h):
+ * - IO_ADDR_R?: address to read the 8 I/O lines of the flash device
+ * - IO_ADDR_W?: address to write the 8 I/O lines of the flash device
+ * - hwcontrol: hardwarespecific function for accesing control-lines
+ * - dev_ready: hardwarespecific function for accesing device ready/busy line
+ * - enable_hwecc?: function to enable (reset) hardware ecc generator. Must
+ * only be provided if a hardware ECC is available
Quoted 8 lines
+ * - eccmode: mode of ecc, see defines
+ * - chip_delay: chip dependent delay for transfering data from array to
+ * read regs (tR)
+ * - options: various chip options. They can partly be set to inform
+ * nand_scan about special functionality. See the defines for further
+ * explanation
+ * Members with a "?" were not set in the merged testing-NAND branch,
+ * so they are not set here either.
IO_ADDR_R and IO_ADDR_W have a "?" but are set here. If you have a question about the API, ask it on the list, rather than encoding it in the source.
drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 2735
Quoted 10 lines
===================================================================
drivers/mtd/nand/nand_base.c +3 -0
@@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
 	return 0;
 }
 
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+void nand_switch_ecc(struct mtd_info *mtd)
NACK. First, explain why you need to be able to dynamically switch ECC modes.
Then, if it is justified, implement it in a separate patch, without all the duplication of code, and without platform-specific #ifdefs.
-Scott

Dirk Behme

author

2008-10-07 09:42 UTC #3 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

In reply to Scott Wood

Unfortunately, except Nishanth's comments, I didn't get any further help (e.g. from TI) for this yet. So I started to look at this myself. Please forgive everything I missed as I'm no NAND expert. Maybe you like to explain some additional details regarding what I missed ;)

First version of updated NAND patch in attachment. This is RFC only. I'd like to fix anything still open. When this is done and when everybody is fine with this, I will send an updated v3 of whole OMAP3 patch set including the then final NAND patch (and the fixes for all other comments).

Quoted 13 lines · Scott Wood

cpu/arm_cortexa8/omap3/nand.c line 31
Quoted 8 lines · dirk.behme at googlemail.com
cpu/arm_cortexa8/omap3/nand.c new file +8 -0
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/mem.h>
+#include <linux/mtd/nand_ecc.h>
+
+#if defined(CONFIG_CMD_NAND)
+
+#include <nand.h>
Move the #ifdef to the Makefile.

Did this. Additionally, I moved OMAP3 NAND driver to drivers/mtd/nand/omap3.c.

Quoted 13 lines

cpu/arm_cortexa8/omap3/nand.c new file +13 -0

+/*
+ * nand_read_buf16 - [DEFAULT] read chip data into buffer
+ * @mtd:	MTD device structure
+ * @buf:	buffer to store date
+ * @len:	number of bytes to read
+ *
+ * Default read function for 16bit buswith
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+	u16 *p = (u16 *) buf;

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c line 136
Quoted 5 lines
cpu/arm_cortexa8/omap3/nand.c new file +5 -0
+	len >>= 1;
+
+	for (i = 0; i < len; i++)
+		p[i] = readw(this->IO_ADDR_R);
+}
How does this differ from the default implementation?

It doesn't differ ;)

So I removed this and tried to use default nand_read_buf16() instead:

nand->read_buf = nand_read_buf16;

in board_nand_init(). But this doesn't compile as nand_read_buf16() is static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it as FIXME in patch.

Quoted 30 lines

cpu/arm_cortexa8/omap3/nand.c new file +30 -0

+static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+				int len)
+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		writeb(buf[i], chip->IO_ADDR_W);
+		for (j = 0; j < 10; j++) ;
+	}
+
+}
+
+/*
+ * omap_nand_read_buf - read data from NAND controller into buffer
+ * @mtd:        MTD device structure
+ * @buf:        buffer to store date
+ * @len:        number of bytes to read
+ *
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	int i;
+	int j = 0;
+	struct nand_chip *chip = mtd->priv;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = readb(chip->IO_ADDR_R);
+		while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS) & GPMC_BUF_FULL));

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+	}
+}
I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL] when writing, but with 8-bit NAND, you check it when reading? And 8-bit writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads have no delay at all?

Removed 8-bit stuff completely as it isn't used.

Quoted 5 lines

cpu/arm_cortexa8/omap3/nand.c new file +5 -0

+static void omap_hwecc_init(struct nand_chip *chip)
+{
+	unsigned long val = 0x0;
+
+	/* Init ECC Control Register */

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c line 195

Quoted 5 lines

cpu/arm_cortexa8/omap3/nand.c new file +5 -0

+	/* Clear all ECC  | Enable Reg1 */
+	val = ((0x00000001 << 8) | 0x00000001);
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
+	__raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
+}

Why raw?

Removed all _raw_ . At ARM, it seems that __raw_writex()/readx() are the same as writex()/readx().

Quoted 10 lines

cpu/arm_cortexa8/omap3/nand.c new file +10 -0

+/*
+ * omap_correct_data - Compares the ecc read from nand spare area with
+ *                     ECC registers values
+ *			and corrects one bit error if it has occured
+ * @mtd:		 MTD device structure
+ * @dat:		 page data
+ * @read_ecc:		 ecc read from nand flash
+ * @calc_ecc: 		 ecc read from ECC registers
+ */
+static int omap_correct_data(struct mtd_info *mtd, u_char *dat,

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c line 210
Quoted 4 lines
cpu/arm_cortexa8/omap3/nand.c new file +4 -0
+			     u_char *read_ecc, u_char *calc_ecc)
+{
+	return 0;
+}
This obviously is not correcting anything. If the errors are corrected automatically by the controller, say so in the comments.

I replaced this with the version from Nishanth's U-Boot v2:

http://git.denx.de/?p=u-boot/u-boot-v2.git;a=blob;f=drivers/nand/nand_omap_gpmc.c;h=afd676a74194c63a1e3cf581d210235a9d4c78ba;hb=HEAD

Quoted 10 lines

Quoted 8 lines

+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *

+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.

Quoted 2 lines

Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?

Don't know.

All: Any help?

Quoted 13 lines

+ *  @mtd:	MTD structure
+ *  @dat:	unused
+ *  @ecc_code:	ecc_code buffer

+ */

cpu/arm_cortexa8/omap3/nand.c new file +9 -0

+static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+			      u_char *ecc_code)
+{
+	unsigned long val = 0x0;
+	unsigned long reg;
+
+	/* Start Reading from HW ECC1_Result = 0x200 */
+	reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
+	val = __raw_readl(reg);

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c line 236
Quoted 4 lines
cpu/arm_cortexa8/omap3/nand.c new file +4 -0
+
+	*ecc_code++ = ECC_P1_128_E(val);
+	*ecc_code++ = ECC_P1_128_O(val);
+	*ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val) << 4;
These macros are used nowhere else; why obscure what it's doing by moving it to the top of the file?

I replaced this with the version from Nishanth's U-Boot v2 (see link above)

Quoted 25 lines

cpu/arm_cortexa8/omap3/nand.c new file +25 -0

+static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG);
+	unsigned int dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
+
+	switch (mode) {
+	case NAND_ECC_READ:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_READSYN:
+		__raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	case NAND_ECC_WRITE:
+		__raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
+		/* ECC col width | CS | ECC Enable */
+		val = (dev_width << 7) | (cs << 1) | (0x1);
+		break;
+	default:
+		printf("Error: Unrecognized Mode[%d]!\n", mode);
+		break;

Quoted 8 lines

cpu/arm_cortexa8/omap3/nand.c line 274
Quoted 4 lines
cpu/arm_cortexa8/omap3/nand.c new file +4 -0
+	}
+
+	__raw_writel(val, GPMC_BASE + GPMC_ECC_CONFIG);
+}
Is it OK if config gets written before control, or if this whole thing gets done out of order with respect to other raw writes?

Hmm. I replaced this with the version from Nishanth's U-Boot v2 (see link above). If this isn't ok, can you explain a little more?

Quoted 11 lines

cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 7 lines
cpu/arm_cortexa8/omap3/nand.c new file +7 -0
+static struct nand_ecclayout hw_nand_oob_64 = {
+	.eccbytes = 12,
+	.eccpos = {
+		   2, 3, 4, 5,
+		   6, 7, 8, 9,
+		   10, 11, 12, 13},
+	.oobfree = { {20, 50} }	/* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?

Don't know (or understand?).

All: Any help?

Quoted 11 lines

cpu/arm_cortexa8/omap3/nand.c line 326
Quoted 7 lines
cpu/arm_cortexa8/omap3/nand.c new file +7 -0
+	if (nand->options & NAND_BUSWIDTH_16) {
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 2);
+		if (nand->ecc.layout->eccbytes & 0x01)
+			mtd->oobavail--;
+	} else
+		mtd->oobavail = mtd->oobsize - (nand->ecc.layout->eccbytes + 1);
+}
oobavail is calculated by the generic NAND code. You don't need to do it here.

Removed it.

Quoted 19 lines

Quoted 9 lines
+/*
+ * Board-specific NAND initialization. The following members of the
+ * argument are board-specific (per include/linux/mtd/nand_new.h):
+ * - IO_ADDR_R?: address to read the 8 I/O lines of the flash device
+ * - IO_ADDR_W?: address to write the 8 I/O lines of the flash device
+ * - hwcontrol: hardwarespecific function for accesing control-lines
+ * - dev_ready: hardwarespecific function for accesing device ready/busy line
+ * - enable_hwecc?: function to enable (reset) hardware ecc generator. Must
+ * only be provided if a hardware ECC is available
Quoted 8 lines
+ * - eccmode: mode of ecc, see defines
+ * - chip_delay: chip dependent delay for transfering data from array to
+ * read regs (tR)
+ * - options: various chip options. They can partly be set to inform
+ * nand_scan about special functionality. See the defines for further
+ * explanation
+ * Members with a "?" were not set in the merged testing-NAND branch,
+ * so they are not set here either.

Quoted 3 lines

IO_ADDR_R and IO_ADDR_W have a "?" but are set here. If you have a question about the API, ask it on the list, rather than encoding it in the source.

Seems that this was only a comment style issue (?). Cleaned up the comment. Better?

Quoted 7 lines

Quoted 7 lines
===================================================================
drivers/mtd/nand/nand_base.c +0 -0
@@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
 	return 0;
 }
 

Quoted 7 lines

drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 2735
Quoted 3 lines
drivers/mtd/nand/nand_base.c +3 -0
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+	|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+void nand_switch_ecc(struct mtd_info *mtd)
NACK. First, explain why you need to be able to dynamically switch ECC modes.

Two topics here, changes in cmd_nand.c and nand_base.c.

cmd_nand.c:

We need to be able to switch ECC at runtime cause some images have to be written to NAND with HW ECC and some with SW ECC. This depends on what user (reader) of these parts expect.

OMAP3 has a boot ROM which is able to read a second level loader (called x-loader) from NAND and start/execute it. This 2nd level loader has to be written by U-Boot using HW ECC as ROM code does HW ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC as default. For this we have to use SW ECC to write images, then.

Therefore we add an additional user command in cmd_nand.c to switch ECC depending on what user wants to write.

Quoted 2 lines

Then, if it is justified, implement it in a separate patch, without all the duplication of code, and without platform-specific #ifdefs.

nand_base.c:

The large nand_switch_ecc() in nand_base.c seems to be a merge error. Thanks for finding this. I *removed* it completely.

I.e. with patch in attachment we don't touch nand_base.c any more, but still have an #ifdeffed user command in cmd_nand.c. If you don't like this, please give a hint how to better do this.

Many thanks for your review and help

Dirk

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 08_omap3_nand.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081007/a9ab7378/attachment-0001.txt

Nishanth Menon

2008-10-07 11:25 UTC #4 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Dirk Behme said the following on 10/07/2008 04:42 AM:

Quoted 17 lines

Quoted 14 lines · Scott Wood
cpu/arm_cortexa8/omap3/nand.c line 31
Quoted 8 lines · dirk.behme at googlemail.com
cpu/arm_cortexa8/omap3/nand.c new file +8 -0
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/mem.h>
+#include <linux/mtd/nand_ecc.h>
+
+#if defined(CONFIG_CMD_NAND)
+
+#include <nand.h>
Move the #ifdef to the Makefile.

Quoted 2 lines

Did this. Additionally, I moved OMAP3 NAND driver to drivers/mtd/nand/omap3.c.

I would recommend having a nand_omap_gpmc.c if the driver can be made generic enough.

Quoted 26 lines

Quoted 15 lines
Quoted 15 lines
cpu/arm_cortexa8/omap3/nand.c new file +9 -0
+/*
+ * nand_read_buf16 - [DEFAULT] read chip data into buffer
+ * @mtd:    MTD device structure
+ * @buf:    buffer to store date
+ * @len:    number of bytes to read
+ *
+ * Default read function for 16bit buswith
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf,
int len)
cpu/arm_cortexa8/omap3/nand.c new file +4 -0
+{
+    int i;
+    struct nand_chip *this = mtd->priv;
+    u16 *p = (u16 *) buf;
Quoted 8 lines
cpu/arm_cortexa8/omap3/nand.c line 136
Quoted 5 lines
cpu/arm_cortexa8/omap3/nand.c new file +5 -0
+    len >>= 1;
+
+    for (i = 0; i < len; i++)
+        p[i] = readw(this->IO_ADDR_R);
+}
How does this differ from the default implementation?
It doesn't differ ;)

Quoted 7 lines

So I removed this and tried to use default nand_read_buf16() instead:
nand->read_buf = nand_read_buf16;
in board_nand_init(). But this doesn't compile as nand_read_buf16() is static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it as FIXME in patch.

Probably does not need an explicit initialization, mtd nand_scan should populate that. in fact IMHO, the read/write buf ops might be duplicating mtd's implementation.. :(

Quoted 12 lines

Quoted 10 lines

+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *

+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.

Quoted 6 lines

Quoted 2 lines
Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?
Don't know.
All: Any help?

The issue is simple: assume we read a page of 0xFF's(fresh erased), IF using H/w ECC engine within GPMC, the result of read will be 0x0 while the ECC offsets of the spare area will be 0xFF which will result in an ECC mismatch.

Quoted 8 lines

Quoted 6 lines

cpu/arm_cortexa8/omap3/nand.c new file +6 -0

+    .eccbytes = 12,
+    .eccpos = {
+           2, 3, 4, 5,
+           6, 7, 8, 9,
+           10, 11, 12, 13},
+    .oobfree = { {20, 50} }    /* don't care */

Quoted 7 lines

Quoted 2 lines
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?
+static struct nand_ecclayout hw_nand_oob_64 = {
Don't know (or understand?).
All: Any help?

I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,

Quoted 24 lines

Quoted 8 lines
Quoted 8 lines
===================================================================
drivers/mtd/nand/nand_base.c +0 -0
@@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
    return 0;
 }
 
Quoted 7 lines
drivers/mtd/nand/nand_base.c int nand_scan_tail(struct mtd_info *mtd) · line 2735
Quoted 3 lines
drivers/mtd/nand/nand_base.c +3 -0
+#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
+    || defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
+void nand_switch_ecc(struct mtd_info *mtd)
NACK. First, explain why you need to be able to dynamically switch ECC modes.
Two topics here, changes in cmd_nand.c and nand_base.c.
cmd_nand.c:
We need to be able to switch ECC at runtime cause some images have to be written to NAND with HW ECC and some with SW ECC. This depends on what user (reader) of these parts expect.

Quoted 8 lines

OMAP3 has a boot ROM which is able to read a second level loader (called x-loader) from NAND and start/execute it. This 2nd level loader has to be written by U-Boot using HW ECC as ROM code does HW ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC as default. For this we have to use SW ECC to write images, then.
Therefore we add an additional user command in cmd_nand.c to switch ECC depending on what user wants to write.

why not use h/w ecc which rom code understands in kernel and u-boot. H/w ecc will be faster in comparison to doing s/w ecc and there is good support in MTD to do it, then there would be no reason for s/w ecc IMHO..

Regards,
Nishanth Menon

Scott Wood

2008-10-07 17:30 UTC #5 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 5 lines · Nishanth Menon

Dirk Behme said the following on 10/07/2008 04:42 AM:
Quoted 4 lines
It doesn't differ ;)
So I removed this and tried to use default nand_read_buf16() instead:

Quoted 7 lines

Quoted 5 lines
nand->read_buf = nand_read_buf16;
in board_nand_init(). But this doesn't compile as nand_read_buf16() is static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it as FIXME in patch.
Probably does not need an explicit initialization, mtd nand_scan should populate that.

Correct, NULL methods will be filled in with defaults if applicable.

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.
Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?

Quoted 7 lines

Quoted 3 lines
Don't know.
All: Any help?
The issue is simple: assume we read a page of 0xFF's(fresh erased), IF using H/w ECC engine within GPMC, the result of read will be 0x0 while the ECC offsets of the spare area will be 0xFF which will result in an ECC mismatch.

Right, I'd just like to see an explicit statement that this is the only way to do HW ECC that the hardware supports (following verification of that fact, of course), along with a pointer to where in the code the ECC error when reading an empty page is dealt with.

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 6 lines
cpu/arm_cortexa8/omap3/nand.c new file +6 -0
+    .eccbytes = 12,
+    .eccpos = {
+           2, 3, 4, 5,
+           6, 7, 8, 9,
+           10, 11, 12, 13},
+    .oobfree = { {20, 50} }    /* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?

Quoted 8 lines

Quoted 5 lines
+static struct nand_ecclayout hw_nand_oob_64 = {
Don't know (or understand?).
All: Any help?
I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,

Why not offset 14, length 50?

Quoted 10 lines

Quoted 10 lines
We need to be able to switch ECC at runtime cause some images have to be written to NAND with HW ECC and some with SW ECC. This depends on what user (reader) of these parts expect.
OMAP3 has a boot ROM which is able to read a second level loader (called x-loader) from NAND and start/execute it. This 2nd level loader has to be written by U-Boot using HW ECC as ROM code does HW ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC as default. For this we have to use SW ECC to write images, then.

Quoted 5 lines

Quoted 2 lines
Therefore we add an additional user command in cmd_nand.c to switch ECC depending on what user wants to write.
why not use h/w ecc which rom code understands in kernel and u-boot. H/w ecc will be faster in comparison to doing s/w ecc and there is good support in MTD to do it, then there would be no reason for s/w ecc IMHO..

Agreed.

-Scott

Menon

2008-10-07 17:45 UTC #6 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 19 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Tuesday, October 07, 2008 12:30 PM
To: Nishanth Menon
Cc: Dirk Behme; u-boot at lists.denx.de; Kamat, Nishant; Menon, Nishanth
Subject: Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support
Quoted 12 lines
Quoted 12 lines
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 6 lines
cpu/arm_cortexa8/omap3/nand.c new file +6 -0
+    .eccbytes = 12,
+    .eccpos = {
+           2, 3, 4, 5,
+           6, 7, 8, 9,
+           10, 11, 12, 13},
+    .oobfree = { {20, 50} }    /* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?
+static struct nand_ecclayout hw_nand_oob_64 = {

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Don't know (or understand?).
All: Any help?
I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,
Why not offset 14, length 50?

How about this part being used by ubi/jffs2 or some fs.. I cant think off-hand if there an implication, but yep, I suspect there is no reason why not..

Regards,
Nishanth Menon

Scott Wood

2008-10-07 17:47 UTC #7 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 5 lines · Nishanth

Quoted 4 lines
Quoted 3 lines
I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,
Why not offset 14, length 50?
How about this part being used by ubi/jffs2 or some fs.. I cant think off-hand if there an implication, but yep, I suspect there is no reason why not..

The higher levels allocate their OOB usage from the regions that the low-level driver claims are free.

-Scott

Dirk Behme

author

2008-10-07 17:56 UTC #8 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

In reply to Scott Wood

Quoted 14 lines · Scott Wood

Quoted 12 lines · Nishanth Menon
Dirk Behme said the following on 10/07/2008 04:42 AM:
Quoted 9 lines
It doesn't differ ;)
So I removed this and tried to use default nand_read_buf16() instead:
nand->read_buf = nand_read_buf16;
in board_nand_init(). But this doesn't compile as nand_read_buf16() is static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it as FIXME in patch.

Quoted 5 lines

Quoted 2 lines
Probably does not need an explicit initialization, mtd nand_scan should populate that.
Correct, NULL methods will be filled in with defaults if applicable.

ok, will do so, this is easy :)

Quoted 22 lines

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 8 lines
+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.
Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?
Quoted 8 lines
Quoted 3 lines
Don't know.
All: Any help?
The issue is simple: assume we read a page of 0xFF's(fresh erased), IF using H/w ECC engine within GPMC, the result of read will be 0x0 while the ECC offsets of the spare area will be 0xFF which will result in an ECC mismatch.

Quoted 4 lines

Right, I'd just like to see an explicit statement that this is the only way to do HW ECC that the hardware supports (following verification of that fact, of course), along with a pointer to where in the code the ECC error when reading an empty page is dealt with.

Will add Nishanth's explanation to comment and check code for this.

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 6 lines
cpu/arm_cortexa8/omap3/nand.c new file +6 -0
+    .eccbytes = 12,
+    .eccpos = {
+           2, 3, 4, 5,
+           6, 7, 8, 9,
+           10, 11, 12, 13},
+    .oobfree = { {20, 50} }    /* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?
+static struct nand_ecclayout hw_nand_oob_64 = {
Don't know (or understand?).

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
All: Any help?
I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,
Why not offset 14, length 50?

Seems I need a closer look what we are talking about here ;)

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
We need to be able to switch ECC at runtime cause some images have to be written to NAND with HW ECC and some with SW ECC. This depends on what user (reader) of these parts expect.
OMAP3 has a boot ROM which is able to read a second level loader (called x-loader) from NAND and start/execute it. This 2nd level loader has to be written by U-Boot using HW ECC as ROM code does HW ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC as default. For this we have to use SW ECC to write images, then.
Therefore we add an additional user command in cmd_nand.c to switch ECC depending on what user wants to write.

Quoted 6 lines

Quoted 3 lines
why not use h/w ecc which rom code understands in kernel and u-boot. H/w ecc will be faster in comparison to doing s/w ecc and there is good support in MTD to do it, then there would be no reason for s/w ecc IMHO..
Agreed.

As already mentioned in previous post, I think for the the moment we have to go with both ways.

To summarize the open points I will look at:

a) Remove unnecessary nand->read_buf init
b) Add comment and check code for HW ecc issue with erased page
c) Fix offset 14, length 50 issue
d) Extend MTD API with a call to switch HW/SW ecc, remove platform-specific ifdefs in generic files

Nishanth M.: Would be nice to get some help for these changes. E.g. can you say if this HW ecc issue with erased page is handled correctly in exisiting code?

Thanks for the help

Dirk

Dirk Behme

author

2008-10-07 20:47 UTC #9 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 120 lines · Dirk Behme

Quoted 15 lines · Scott Wood
Quoted 13 lines · Nishanth Menon
Dirk Behme said the following on 10/07/2008 04:42 AM:
Quoted 9 lines
It doesn't differ ;)
So I removed this and tried to use default nand_read_buf16() instead:
nand->read_buf = nand_read_buf16;
in board_nand_init(). But this doesn't compile as nand_read_buf16() is static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it as FIXME in patch.
Quoted 6 lines
Quoted 2 lines
Probably does not need an explicit initialization, mtd nand_scan should populate that.
Correct, NULL methods will be filled in with defaults if applicable.
ok, will do so, this is easy :)
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
Quoted 14 lines
Quoted 11 lines
+/*
+ * omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ * Using noninverted ECC can be considered ugly since writing a blank
+ * page ie. padding will clear the ECC bytes. This is no problem as
+ * long nobody is trying to write data on the seemingly unused page.
+ * Reading an erased page will produce an ECC mismatch between
+ * generated and read ECC bytes that has to be dealt with separately.
Quoted 2 lines
Is this a hardware limitation? If so, say so in the comment. If not, why do it this way?
Don't know.
Quoted 7 lines
Quoted 1 line
All: Any help?
The issue is simple: assume we read a page of 0xFF's(fresh erased), IF using H/w ECC engine within GPMC, the result of read will be 0x0 while the ECC offsets of the spare area will be 0xFF which will result in an ECC mismatch.
Quoted 5 lines
Right, I'd just like to see an explicit statement that this is the only way
to do HW ECC that the hardware supports (following verification of that fact, of course), along with a pointer to where in the code the ECC error when reading an empty page is dealt with.
Will add Nishanth's explanation to comment and check code for this.
Quoted 20 lines
Quoted 20 lines
Quoted 18 lines
Quoted 11 lines
cpu/arm_cortexa8/omap3/nand.c line 282
Quoted 6 lines
cpu/arm_cortexa8/omap3/nand.c new file +6 -0
+    .eccbytes = 12,
+    .eccpos = {
+           2, 3, 4, 5,
+           6, 7, 8, 9,
+           10, 11, 12, 13},
+    .oobfree = { {20, 50} }    /* don't care */
Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?
+static struct nand_ecclayout hw_nand_oob_64 = {
Don't know (or understand?).
All: Any help?
Quoted 7 lines
Quoted 3 lines
I do not get it either.. ECCPOS is in offset bytes, and oobfree should be {.offset=20,.length=44} /*I always hated struct initialization done as above..*/, but then,
Why not offset 14, length 50?
Seems I need a closer look what we are talking about here ;)
Quoted 14 lines
Quoted 14 lines
Quoted 12 lines
We need to be able to switch ECC at runtime cause some images have to be written to NAND with HW ECC and some with SW ECC. This depends on what user (reader) of these parts expect.
OMAP3 has a boot ROM which is able to read a second level loader (called x-loader) from NAND and start/execute it. This 2nd level loader has to be written by U-Boot using HW ECC as ROM code does HW ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC as default. For this we have to use SW ECC to write images, then.
Therefore we add an additional user command in cmd_nand.c to switch ECC depending on what user wants to write.
Quoted 8 lines
Quoted 4 lines
why not use h/w ecc which rom code understands in kernel and u-boot. H/w ecc will be faster in comparison to doing s/w ecc and there is good support in MTD to do it, then there would be no reason for s/w ecc IMHO..
Agreed.

Quoted 6 lines

As already mentioned in previous post, I think for the the moment we have to go with both ways.
To summarize the open points I will look at:
a) Remove unnecessary nand->read_buf init

Removed in attachment

Quoted 1 line

b) Add comment and check code for HW ecc issue with erased page

Added comment. Not sure if we have to do anything in code, though.

Quoted 1 line

c) Fix offset 14, length 50 issue

Changed in attachment.

Quoted 2 lines

d) Extend MTD API with a call to switch HW/SW ecc, remove platform-specific ifdefs in generic files

Removed nand ecc command from cmd_nand.c and added "nandecc" command to board file (Scott: Thanks for the hint!):

--- /dev/null
+++ u-boot-arm/cpu/arm_cortexa8/omap3/board.c
...
+#ifdef CONFIG_NAND_OMAP3 +/******************************************************************************
+ * OMAP3 specific command to switch between NAND HW and SW ecc
+ *****************************************************************************/ +static int do_switch_ecc(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
+{
+ if (argc != 2)
+ goto usage;
+ if (strncmp(argv[1], "hw", 2) == 0)
+ omap_nand_switch_ecc(1);
+ else if (strncmp(argv[1], "sw", 2) == 0)
+ omap_nand_switch_ecc(0);
+ else
+ goto usage;
+
+ return 0;
+
+usage:
+ printf ("Usage: nandecc %s\n", cmdtp->help);
+ return 1;
+}
+
+U_BOOT_CMD(
+ nandecc, 2, 1, do_switch_ecc,
+ "nandecc - switch OMAP3 NAND ECC calculation algorithm\n",
+ "[hw/sw] - Switch between NAND hardware (hw) or software (sw) ecc
algorithm\n"
+ );
+
+#endif /* CONFIG_NAND_OMAP3 */

See attachment for latest version of NAND patch. Please let us know if anything else has to be changed. If not, I will prepare OMAP3 v3 patch set.

Thanks

Dirk

Scott Wood

2008-10-07 17:26 UTC #10 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

In reply to Dirk Behme

Quoted 5 lines · Dirk Behme

Quoted 2 lines
Is it OK if config gets written before control, or if this whole thing gets done out of order with respect to other raw writes?
Hmm. I replaced this with the version from Nishanth's U-Boot v2 (see link above). If this isn't ok, can you explain a little more?

I was referring to the raw accessors, which you removed, so it's OK.

Quoted 3 lines

I.e. with patch in attachment we don't touch nand_base.c any more, but still have an #ifdeffed user command in cmd_nand.c. If you don't like this, please give a hint how to better do this.

If at all possible, fix Linux to accept hardware ECC.

Otherwise, either add something to the MTD API that the nand command can call, or have your own board-specific command defined in the board file. Don't put platform-specific ifdefs in generic files.

-Scott

Dirk Behme

author

2008-10-07 17:55 UTC #11 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 10 lines · Scott Wood

Quoted 5 lines · Dirk Behme
Quoted 2 lines
Is it OK if config gets written before control, or if this whole thing gets done out of order with respect to other raw writes?
Hmm. I replaced this with the version from Nishanth's U-Boot v2 (see link above). If this isn't ok, can you explain a little more?
I was referring to the raw accessors, which you removed, so it's OK.

Quoted 6 lines

Quoted 3 lines
I.e. with patch in attachment we don't touch nand_base.c any more, but still have an #ifdeffed user command in cmd_nand.c. If you don't like this, please give a hint how to better do this.
If at all possible, fix Linux to accept hardware ECC.

Yes, as mentioned by Nishanth, too, this will be possible. But I don't think in the short term, and not with breaking compatibility with kernels already out there. So for the moment I like to have both options, HW & SW ECC.

Quoted 3 lines

Otherwise, either add something to the MTD API that the nand command can call, or have your own board-specific command defined in the board file. Don't put platform-specific ifdefs in generic files.

Do you have any example how to extend MTD API and/or can link to example where board-specific command is already used?

Thanks

Dirk

Scott Wood

2008-10-07 17:59 UTC #12 lore raw

PATCH v2 7/12 ARM: OMAP3: Add memory and syslib common files, add NAND support

Quoted 5 lines · Dirk Behme

Quoted 3 lines
Otherwise, either add something to the MTD API that the nand command can call, or have your own board-specific command defined in the board file. Don't put platform-specific ifdefs in generic files.
Do you have any example how to extend MTD API

You'd need to add callbacks in the device structures; since this is a workaround for legacy support, and not something that is desireable in most cases, I'd go with the latter option of a board-specific command.

Quoted 2 lines

and/or can link to
example where board-specific command is already used?

grep -rI U_BOOT_CMD board/

-Scott