[U-Boot] [PATCH v7 0/5] mtd: nand: omap: optimize and clean-up of OMAP NAND driver

[U-Boot] [PATCH v7 0/5] mtd: nand: omap: optimize and clean-up of OMAP NAND driver

[Pekon Gupta]

*changes in v7*
[PATCH 1/5]
	- omap_gpmc.c: fix: free bytes in OOB (ecclayout->oobfree[0].length)
	- omap_gpmc.c: cleanup: redundant code added in previous patch versions
	- am335x_evm.h: cleanup: redundant code added in previous patch versions
	- tricorder.h: fix: CONFIG_NAND_OMAP_ECCSCHEME
[PATCH 2/5] removed: re-configuration of gpmc.config1[dev_width] added in
		previous version of patch
[PATCH 3/5] <no update>
[PATCH 4/5] <no update>
[PATCH 5/5] minor fix: missing '$' in ${loadaddr}

 
*changes in v6*
[PATCH 1/5] incorporated feedbacks from Scott Wood <scottwood@freescale.com>
	- renamed CONFIG_SYS_NAND_ECCSCHEME to CONFIG_NAND_OMAP_ECCSCHEME
	- updated omap_select_ecc_scheme() to handle error conditions without
		depending on caller.
	- renamed OMAP_ECC_HAM1_CODE_HW_ROMCODE to OMAP_ECC_HAM1_CODE_HW 
		to keep it naming compatible to linux kernel
	- updated doc/README.nand and doc/README.omap3
[PATCH 2/5] minor code clean-up
[PATCH 3/5] minor code clean-up 
[PATCH 4/5] <no update>
[PATCH 5/5] <moved CONFIG_NAND_OMAP_ECCSCHEME description to README.nand>


*changes in v5*
This version of patch is tested on am335x-evm with x8 NAND device, and boots
SPL and u-boot from NAND
[PATCH 1/5]
	- re-added omap_read_page_bch(): needed proper sequence of while reading
	DATA and ECC from NAND page, so that calc_ecc generated from GPMC
	is understood by ELM.
	- added check to see if NAND OOB can accomodate ECC for entire page
[PATCH 2/5] fixed device-width in GPMC_CONFIG1_X to support x16 devices
[PATCH 3/5] code clean-up for OMAP_ECC_BCH8_CODE_HW_DETECTION_SW mode
[PATCH 4/5]
	- fixed omap_correct_data_bch() for correcting bit-flips using ELM
	- code-cleanup + added omap_reverse_list()
[PATCH 5/5] incorporated feedbacks from Peter Korsgaard <jacmet@sunsite.dk>


*changes in v4*
[PATCH 1/5]
	- removed omap_read_page_bch(): chip->ecc.read_page uses default API
		nand_read_page_hwecc() in nand_base.c
	- updated tricorder.h: added new CONFIGS for ECCSCHEME & ONFI_DETECTION
	- converted printf("ECC-SCHEME") to debug("ECC-SCHEME")
[PATCH 2/5] minor code clean-up
[PATCH 3/5] <no update>
[PATCH 4/5] <no update>
[PATCH 5/5] updated README as per feedbacks from trini at ti.com


*changes in v3*
[PATCH 1/5] (complete change) 
	- ecc-scheme is selection is controller by s/w, not CONFIG_NAND_xx
	- added omap_select_ecc_scheme(), as common function to handle all
	  ecc-scheme related configurations for both board_nand_init() &
	  omap_nand_switch_ecc().
	- removed un-used defines from asm/arch-am33xx/omap_gpmc.h
	- updated doc/REAME.nand
[PATCH 2/5] removed un-used defines from asm/omap_gpmc.h
[PATCH 3/5] removed omap_calculate_ecc_bch_sw() and omap_calculate_ecc()
		and merged their logic into omap_calculate_ecc_bch()
[PATCH 4/5] updated return error-code in-case of invalid conditions
[PATCH 5/5] (new) for board/ti/am335x/README


*changes in v2*
- added documentation for CONFIG_NAND_OMAP_xx in doc/README.nand
- added CONFIG_BCH along with CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
  to include software library lib/bch.c
- fixed board_nand_init() and omap_enable_hwecc()


*original v1*
This patch series updates BCH8_ECC schemes in mtd/nand/omap_gpmc.c driver
- adds scalability for higher ECC schemes in future.
- removes CONFIG_AM335x and it makes it generic for all platforms.
- optimizes read_data paths

Pekon Gupta (5):
[PATCH 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform
[PATCH 2/5] mtd: nand: omap: optimize chip->ecc.hwctl() for H/W ECC schemes
[PATCH 3/5] mtd: nand: omap: optimize chip->ecc.calculate() for H/W ECC schemes
[PATCH 4/5] mtd: nand: omap: optimized chip->ecc.correct() for H/W ECC schemes
[PATCH 5/5] board/ti/am335x/README: update for NAND boot

 arch/arm/include/asm/arch-am33xx/omap_gpmc.h |  52 +-
 arch/arm/include/asm/omap_gpmc.h             |   7 -
 board/ti/am335x/README                       |  53 +-
 doc/README.nand                              |  30 +
 doc/README.omap3                             |   4 +-
 drivers/mtd/nand/omap_gpmc.c                 | 822 +++++++++++----------------
 include/configs/am335x_evm.h                 |  11 +-
 include/configs/tricorder.h                  |   3 +-
 8 files changed, 416 insertions(+), 566 deletions(-)

-- 
1.8.1

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Pekon Gupta]

BCH8_ECC scheme implemented in omap_gpmc.c driver has following favours
+-----------------------------------+-----------------+-----------------+
|ECC Scheme                         | ECC Calculation | Error Detection |
+-----------------------------------+-----------------+-----------------+
|OMAP_ECC_BCH8_CODE_HW              |GPMC             |ELM H/W engine   |
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |GPMC             |S/W BCH library  |
+-----------------------------------+-----------------+-----------------+

Current implementation limits the BCH8_CODE_HW only for AM33xx device family.
(using CONFIG_AM33XX). However, other SoC families (like TI81xx) also have
ELM hardware module, and can support ECC error detection using ELM.

This patch
- removes CONFIG_AM33xx
	Thus this driver can be reused by all devices having ELM h/w engine.
- removes CONFIG_NAND_OMAP_BCH8 and adds new CONFIG_NAND_OMAP_ECCSCHEME
	CONFIG_NAND_OMAP_ECCSCHEME determines ecc-scheme to be used
	refer doc/README.nand and board/ti/am335x/README
- adds omap_select_ecc_scheme()
	A common function to handle ecc-scheme related configurations. This
	can be used both during device-probe and via user-space u-boot commads
	to change ecc-scheme. During device probe ecc-scheme is selected based
	on CONFIG_NAND_OMAP_ECCSCHEME.
- enables CONFIG_BCH
	S/W library (lib/bch.c) required by OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
  	is enabled by CONFIG_BCH.
- enables CONFIG_SYS_NAND_ONFI_DETECTION
	for auto-detection of ONFI compliant NAND devices
- updates following README doc
	doc/README.nand
	board/ti/am335x/README
	doc/README.omap3

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 arch/arm/include/asm/arch-am33xx/omap_gpmc.h |  52 ++---
 doc/README.nand                              |  30 +++
 doc/README.omap3                             |   4 +-
 drivers/mtd/nand/omap_gpmc.c                 | 287 +++++++++++++++------------
 include/configs/am335x_evm.h                 |  11 +-
 include/configs/tricorder.h                  |   3 +-
 6 files changed, 216 insertions(+), 171 deletions(-)

diff --git a/arch/arm/include/asm/arch-am33xx/omap_gpmc.h b/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
index 00ad1d0..62f608e 100644
--- a/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
+++ b/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
@@ -7,42 +7,20 @@
 #ifndef __ASM_ARCH_OMAP_GPMC_H
 #define __ASM_ARCH_OMAP_GPMC_H
 
-/* These GPMC_NAND_HW_BCHx_ECC_LAYOUT defines are based on AM33xx ELM */
-#define GPMC_NAND_HW_BCH4_ECC_LAYOUT {\
-	.eccbytes = 32,\
-	.eccpos = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
-				16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27,\
-				28, 29, 30, 31, 32, 33},\
-	.oobfree = {\
-		{.offset = 34,\
-		 .length = 30 } } \
-}
+enum omap_ecc {
+	/* 1-bit  ECC calculation by Software, Error detection by Software */
+	OMAP_ECC_HAM1_CODE_SW = 1, /* avoid un-initialized int can be 0x0 */
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	/* ECC layout compatible to legacy ROMCODE. */
+	OMAP_ECC_HAM1_CODE_HW,
+	/* 4-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH4_CODE_HW_DETECTION_SW,
+	/* 4-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH4_CODE_HW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH8_CODE_HW_DETECTION_SW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH8_CODE_HW,
+};
 
-#define GPMC_NAND_HW_BCH8_ECC_LAYOUT {\
-	.eccbytes = 56,\
-	.eccpos = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
-				16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27,\
-				28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39,\
-				40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51,\
-				52, 53, 54, 55, 56, 57},\
-	.oobfree = {\
-		{.offset = 58,\
-		 .length = 6 } } \
-}
-
-#define GPMC_NAND_HW_BCH16_ECC_LAYOUT {\
-	.eccbytes = 104,\
-	.eccpos = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
-				16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27,\
-				28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39,\
-				40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51,\
-				52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63,\
-				64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75,\
-				76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87,\
-				88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99,\
-				100, 101, 102, 103, 104, 105},\
-	.oobfree = {\
-		{.offset = 106,\
-		 .length = 8 } } \
-}
 #endif /* __ASM_ARCH_OMAP_GPMC_H */
diff --git a/doc/README.nand b/doc/README.nand
index 913e9b5..f9c921e 100644
--- a/doc/README.nand
+++ b/doc/README.nand
@@ -169,6 +169,36 @@ Configuration Options:
       Please convert your driver even if you don't need the extra
       flexibility, so that one day we can eliminate the old mechanism.
 
+   CONFIG_SYS_NAND_ONFI_DETECTION
+	Enables detection of ONFI compliant devices during probe.
+	And fetching device parameters flashed on device, by parsing
+	ONFI parameter page.
+
+   CONFIG_BCH
+	Enables software based BCH ECC algorithm present in lib/bch.c
+	This is used by SoC platforms which do not have built-in ELM
+	hardware engine required for BCH ECC correction.
+
+Platform specific options
+=========================
+
+   CONFIG_NAND_OMAP_ECCSCHEME
+	On OMAP platforms, this specifies NAND ECC scheme.
+	1 - HAM1_SW	1-bit Hamming code using software library
+			(for legacy devices only)
+	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
+			(for legacy devices only)
+	3 - BCH4_SW	4-bit BCH code (unsupported)
+	4 - BCH4_HW	4-bit BCH code (unsupported)
+	5 - BCH8_SW	8-bit BCH code with
+			- ecc calculation using GPMC hardware engine,
+			- error detection using software library.
+			- requires CONFIG_BCH to enable software BCH library
+			(For legacy device which do not have ELM h/w engine)
+	6 - BCH8_HW	8-bit BCH code with
+			- ecc calculation using GPMC hardware engine,
+			- error detection using ELM hardware engine.
+
 NOTE:
 =====
 
diff --git a/doc/README.omap3 b/doc/README.omap3
index 1fbe79d..fd85b22 100644
--- a/doc/README.omap3
+++ b/doc/README.omap3
@@ -161,8 +161,8 @@ BCH8
 
 To enable hardware assisted BCH8 (8-bit BCH [Bose, Chaudhuri, Hocquenghem]) on
 OMAP3 devices we can use the BCH library in lib/bch.c. To do so add CONFIG_BCH
-to enable the library and CONFIG_NAND_OMAP_BCH8 to to enable hardware assisted
-syndrom generation to your board config.
+to enable the library and set CONFIG_NAND_OMAP_ECCSCHEME=5 (as per README.nand)
+to enable BCH8_SW ecc scheme.
 The NAND OOB layout is the same as in linux kernel, if the linux kernel BCH8
 implementation for OMAP3 works for you so the u-boot version should also.
 When you require the SPL to read with BCH8 there are two more configs to
diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index ec1787f..7396bee 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -15,15 +15,13 @@
 #include <linux/bch.h>
 #include <linux/compiler.h>
 #include <nand.h>
-#ifdef CONFIG_AM33XX
 #include <asm/arch/elm.h>
-#endif
+
+#define BADBLOCK_MARKER_LENGTH	2
+#define SECTOR_BYTES		512
 
 static uint8_t cs;
-static __maybe_unused struct nand_ecclayout hw_nand_oob =
-	GPMC_NAND_HW_ECC_LAYOUT;
-static __maybe_unused struct nand_ecclayout hw_bch8_nand_oob =
-	GPMC_NAND_HW_BCH8_ECC_LAYOUT;
+static __maybe_unused struct nand_ecclayout omap_ecclayout;
 
 /*
  * omap_nand_hwcontrol - Set the address pointers corretly for the
@@ -233,6 +231,7 @@ struct nand_bch_priv {
 	uint8_t type;
 	uint8_t nibbles;
 	struct bch_control *control;
+	enum omap_ecc ecc_scheme;
 };
 
 /* bch types */
@@ -274,17 +273,15 @@ static void omap_hwecc_init_bch(struct nand_chip *chip, int32_t mode)
 {
 	uint32_t val;
 	uint32_t dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
-#ifdef CONFIG_AM33XX
 	uint32_t unused_length = 0;
-#endif
 	uint32_t wr_mode = BCH_WRAPMODE_6;
 	struct nand_bch_priv *bch = chip->priv;
 
 	/* Clear the ecc result registers, select ecc reg as 1 */
 	writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
 
-#ifdef CONFIG_AM33XX
-	wr_mode = BCH_WRAPMODE_1;
+	if (bch->ecc_scheme == OMAP_ECC_BCH8_CODE_HW) {
+		wr_mode = BCH_WRAPMODE_1;
 
 	switch (bch->nibbles) {
 	case ECC_BCH4_NIBBLES:
@@ -320,7 +317,7 @@ static void omap_hwecc_init_bch(struct nand_chip *chip, int32_t mode)
 		val |= (unused_length << 22);
 		break;
 	}
-#else
+	} else {
 	/*
 	 * This ecc_size_config setting is for BCH sw library.
 	 *
@@ -333,7 +330,7 @@ static void omap_hwecc_init_bch(struct nand_chip *chip, int32_t mode)
 	 *  size1 = 32 (skip 32 nibbles = 16 bytes per sector in spare area)
 	 */
 	val = (32 << 22) | (0 << 12);
-#endif
+	}
 	/* ecc size configuration */
 	writel(val, &gpmc_cfg->ecc_size_config);
 
@@ -376,9 +373,8 @@ static void __maybe_unused omap_ecc_disable(struct mtd_info *mtd)
 }
 
 /*
- * BCH8 support (needs ELM and thus AM33xx-only)
+ * BCH support using ELM module
  */
-#ifdef CONFIG_AM33XX
 /*
  * omap_read_bch8_result - Read BCH result for BCH8 level
  *
@@ -631,20 +627,19 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 	return 0;
 }
-#endif /* CONFIG_AM33XX */
 
 /*
  * OMAP3 BCH8 support (with BCH library)
  */
-#ifdef CONFIG_NAND_OMAP_BCH8
+#ifdef CONFIG_BCH
 /*
- *  omap_calculate_ecc_bch - Read BCH ECC result
+ *  omap_calculate_ecc_bch_sw - Read BCH ECC result
  *
  *  @mtd:	MTD device structure
  *  @dat:	The pointer to data on which ecc is computed (unused here)
  *  @ecc:	The ECC output buffer
  */
-static int omap_calculate_ecc_bch(struct mtd_info *mtd, const uint8_t *dat,
+static int omap_calculate_ecc_bch_sw(struct mtd_info *mtd, const uint8_t *dat,
 				uint8_t *ecc)
 {
 	int ret = 0;
@@ -689,13 +684,13 @@ static int omap_calculate_ecc_bch(struct mtd_info *mtd, const uint8_t *dat,
 }
 
 /**
- * omap_correct_data_bch - Decode received data and correct errors
+ * omap_correct_data_bch_sw - Decode received data and correct errors
  * @mtd: MTD device structure
  * @data: page data
  * @read_ecc: ecc read from nand flash
  * @calc_ecc: ecc read from HW ECC registers
  */
-static int omap_correct_data_bch(struct mtd_info *mtd, u_char *data,
+static int omap_correct_data_bch_sw(struct mtd_info *mtd, u_char *data,
 				 u_char *read_ecc, u_char *calc_ecc)
 {
 	int i, count;
@@ -752,7 +747,115 @@ static void __maybe_unused omap_free_bch(struct mtd_info *mtd)
 		chip_priv->control = NULL;
 	}
 }
-#endif /* CONFIG_NAND_OMAP_BCH8 */
+#endif /* CONFIG_BCH */
+
+/**
+ * omap_select_ecc_scheme - configures driver for particular ecc-scheme
+ * @nand: NAND chip device structure
+ * @ecc_scheme: ecc scheme to configure
+ * @pagesize: number of main-area bytes per page of NAND device
+ * @oobsize: number of OOB/spare bytes per page of NAND device
+ */
+static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
+		unsigned int pagesize, unsigned int oobsize) {
+	struct nand_bch_priv	*bch		= nand->priv;
+	struct nand_ecclayout	*ecclayout	= nand->ecc.layout;
+	int i;
+
+	switch (ecc_scheme) {
+	case OMAP_ECC_HAM1_CODE_SW:
+		debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n");
+		nand->ecc.mode		= NAND_ECC_SOFT;
+		nand->ecc.layout	= NULL;
+		nand->ecc.size		= 0;
+		nand->ecc.strength	= 1;
+		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_SW;
+		break;
+	case OMAP_ECC_HAM1_CODE_HW:
+		debug("nand: selected OMAP_ECC_HAM1_CODE_HW\n");
+		nand->ecc.mode		= NAND_ECC_HW;
+		nand->ecc.strength	= 1;
+		nand->ecc.size		= 512;
+		nand->ecc.bytes		= 3;
+		nand->ecc.hwctl		= omap_enable_hwecc;
+		nand->ecc.correct	= omap_correct_data;
+		nand->ecc.calculate	= omap_calculate_ecc;
+		/* define ecc-layout */
+		ecclayout->eccbytes	= nand->ecc.bytes *
+						(pagesize / SECTOR_BYTES);
+		for (i = 0; i < ecclayout->eccbytes; i++)
+			ecclayout->eccpos[i] = i + BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree[0].offset = i + BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
+						BADBLOCK_MARKER_LENGTH;
+		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_HW;
+		break;
+#ifdef CONFIG_BCH
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+		debug("nand: selected OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
+		nand->ecc.mode		= NAND_ECC_HW;
+		nand->ecc.strength	= 8;
+		nand->ecc.size		= 512;
+		nand->ecc.bytes		= 13;
+		nand->ecc.hwctl		= omap_enable_ecc_bch;
+		nand->ecc.correct	= omap_correct_data_bch_sw;
+		nand->ecc.calculate	= omap_calculate_ecc_bch_sw;
+		/* BCH SW library is used for error detection */
+		bch_priv.control	= init_bch(13, 8, 0x201b);
+		if (!bch_priv.control) {
+			printf("nand: error: could not init_bch()\n");
+			return -ENODEV;
+		}
+		bch_priv.type		= ECC_BCH8;
+		/* define ecc-layout */
+		ecclayout->eccbytes	= nand->ecc.bytes *
+						(pagesize / SECTOR_BYTES);
+		for (i = 0; i < ecclayout->eccbytes; i++)
+			ecclayout->eccpos[i] = i + (oobsize -
+						ecclayout->eccbytes);
+		ecclayout->oobfree[0].offset = BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
+						BADBLOCK_MARKER_LENGTH;
+		omap_hwecc_init_bch(nand, NAND_ECC_READ);
+		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
+		break;
+#endif
+	case OMAP_ECC_BCH8_CODE_HW:
+		debug("nand: selected OMAP_ECC_BCH8_CODE_HW\n");
+		nand->ecc.mode		= NAND_ECC_HW;
+		nand->ecc.strength	= 8;
+		nand->ecc.size		= 512;
+		nand->ecc.bytes		= 14;
+		nand->ecc.hwctl		= omap_enable_ecc_bch;
+		nand->ecc.correct	= omap_correct_data_bch;
+		nand->ecc.calculate	= omap_calculate_ecc_bch;
+		nand->ecc.read_page	= omap_read_page_bch;
+		/* ELM is used for ECC error detection */
+		elm_init();
+		bch_priv.type		= ECC_BCH8;
+		/* define ecc-layout */
+		ecclayout->eccbytes	= nand->ecc.bytes *
+						(pagesize / SECTOR_BYTES);
+		for (i = 0; i < ecclayout->eccbytes; i++)
+			ecclayout->eccpos[i] = i + BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree[0].offset = i + BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
+						BADBLOCK_MARKER_LENGTH;
+		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW;
+		break;
+	default:
+		debug("nand: error: ecc scheme not enabled or supported\n");
+		return -EINVAL;
+	}
+
+	/* check if NAND spare/OOB has enough bytes to accomodate ecclayout */
+	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize) {
+		printf("nand: error: insufficient OOB bytes. require=%d\n", (
+				ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH));
+		return -EINVAL;
+	}
+	return 0;
+}
 
 #ifndef CONFIG_SPL_BUILD
 /*
@@ -763,77 +866,52 @@ static void __maybe_unused omap_free_bch(struct mtd_info *mtd)
  * @eccstrength		- the number of bits that could be corrected
  *			  (1 - hamming, 4 - BCH4, 8 - BCH8, 16 - BCH16)
  */
-void omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength)
+int omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength)
 {
 	struct nand_chip *nand;
 	struct mtd_info *mtd;
+	struct nand_bch_priv *bch;
+	int err = 0;
 
 	if (nand_curr_device < 0 ||
 	    nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
 	    !nand_info[nand_curr_device].name) {
-		printf("Error: Can't switch ecc, no devices available\n");
-		return;
+		printf("nand: error: no NAND devices found\n");
+		return -ENODEV;
 	}
 
 	mtd = &nand_info[nand_curr_device];
 	nand = mtd->priv;
-
+	bch = nand->priv;
 	nand->options |= NAND_OWN_BUFFERS;
-
-	/* Reset ecc interface */
-	nand->ecc.mode = NAND_ECC_NONE;
-	nand->ecc.read_page = NULL;
-	nand->ecc.write_page = NULL;
-	nand->ecc.read_oob = NULL;
-	nand->ecc.write_oob = NULL;
-	nand->ecc.hwctl = NULL;
-	nand->ecc.correct = NULL;
-	nand->ecc.calculate = NULL;
-	nand->ecc.strength = eccstrength;
-
 	/* Setup the ecc configurations again */
 	if (hardware) {
 		if (eccstrength == 1) {
-			nand->ecc.mode = NAND_ECC_HW;
-			nand->ecc.layout = &hw_nand_oob;
-			nand->ecc.size = 512;
-			nand->ecc.bytes = 3;
-			nand->ecc.hwctl = omap_enable_hwecc;
-			nand->ecc.correct = omap_correct_data;
-			nand->ecc.calculate = omap_calculate_ecc;
-			omap_hwecc_init(nand);
-			printf("1-bit hamming HW ECC selected\n");
-		}
-#if defined(CONFIG_AM33XX) || defined(CONFIG_NAND_OMAP_BCH8)
-		else if (eccstrength == 8) {
-			nand->ecc.mode = NAND_ECC_HW;
-			nand->ecc.layout = &hw_bch8_nand_oob;
-			nand->ecc.size = 512;
-#ifdef CONFIG_AM33XX
-			nand->ecc.bytes = 14;
-			nand->ecc.read_page = omap_read_page_bch;
-#else
-			nand->ecc.bytes = 13;
-#endif
-			nand->ecc.hwctl = omap_enable_ecc_bch;
-			nand->ecc.correct = omap_correct_data_bch;
-			nand->ecc.calculate = omap_calculate_ecc_bch;
-			omap_hwecc_init_bch(nand, NAND_ECC_READ);
-			printf("8-bit BCH HW ECC selected\n");
+			err = omap_select_ecc_scheme(nand,
+					OMAP_ECC_HAM1_CODE_HW,
+					mtd->writesize, mtd->oobsize);
+		} else if (eccstrength == 8) {
+			err = omap_select_ecc_scheme(nand,
+					OMAP_ECC_BCH8_CODE_HW,
+					mtd->writesize, mtd->oobsize);
+		} else {
+			printf("nand: error: unsupported ECC scheme\n");
+			return -EINVAL;
 		}
-#endif
 	} else {
-		nand->ecc.mode = NAND_ECC_SOFT;
-		/* Use mtd default settings */
-		nand->ecc.layout = NULL;
-		nand->ecc.size = 0;
-		printf("SW ECC selected\n");
+		err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW,
+					mtd->writesize, mtd->oobsize);
+	}
+	if (err) {
+		printf("nand: error: could not switch ecc, reverting\n");
+		omap_select_ecc_scheme(nand, bch->ecc_scheme,
+				       mtd->writesize, mtd->oobsize);
+		return -EINVAL;
 	}
 
 	/* Update NAND handling after ECC mode switch */
-	nand_scan_tail(mtd);
-
-	nand->options &= ~NAND_OWN_BUFFERS;
+	err = nand_scan_tail(mtd);
+	return err;
 }
 #endif /* CONFIG_SPL_BUILD */
 
@@ -873,7 +951,7 @@ int board_nand_init(struct nand_chip *nand)
 		cs++;
 	}
 	if (cs >= GPMC_MAX_CS) {
-		printf("NAND: Unable to find NAND settings in "
+		printf("nand: error: Unable to find NAND settings in "
 			"GPMC Configuration - quitting\n");
 		return -ENODEV;
 	}
@@ -885,64 +963,23 @@ int board_nand_init(struct nand_chip *nand)
 
 	nand->IO_ADDR_R = (void __iomem *)&gpmc_cfg->cs[cs].nand_dat;
 	nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
-
-	nand->cmd_ctrl = omap_nand_hwcontrol;
-	nand->options = NAND_NO_PADDING | NAND_CACHEPRG;
+	nand->priv	= &bch_priv;
+	nand->cmd_ctrl	= omap_nand_hwcontrol;
+	nand->options	|= NAND_NO_PADDING | NAND_CACHEPRG;
 	/* If we are 16 bit dev, our gpmc config tells us that */
 	if ((readl(&gpmc_cfg->cs[cs].config1) & 0x3000) == 0x1000)
 		nand->options |= NAND_BUSWIDTH_16;
 
 	nand->chip_delay = 100;
-
-#if defined(CONFIG_AM33XX) || defined(CONFIG_NAND_OMAP_BCH8)
-#ifdef CONFIG_AM33XX
-	/* AM33xx uses the ELM */
-	/* required in case of BCH */
-	elm_init();
-#else
-	/*
-	 * Whereas other OMAP based SoC do not have the ELM, they use the BCH
-	 * SW library.
-	 */
-	bch_priv.control = init_bch(13, 8, 0x201b /* hw polynominal */);
-	if (!bch_priv.control) {
-		puts("Could not init_bch()\n");
-		return -ENODEV;
+	nand->ecc.layout = &omap_ecclayout;
+
+	/* select ECC scheme */
+	if (omap_select_ecc_scheme(nand, CONFIG_NAND_OMAP_ECCSCHEME,
+				   CONFIG_SYS_NAND_PAGE_SIZE,
+				   CONFIG_SYS_NAND_OOBSIZE)) {
+		printf("nand: error: unsupported ecc scheme specified\n");
+		return -EINVAL;
 	}
-#endif
-	/* BCH info that will be correct for SPL or overridden otherwise. */
-	nand->priv = &bch_priv;
-#endif
-
-	/* Default ECC mode */
-#if defined(CONFIG_AM33XX) || defined(CONFIG_NAND_OMAP_BCH8)
-	nand->ecc.mode = NAND_ECC_HW;
-	nand->ecc.layout = &hw_bch8_nand_oob;
-	nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
-	nand->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES;
-	nand->ecc.strength = 8;
-	nand->ecc.hwctl = omap_enable_ecc_bch;
-	nand->ecc.correct = omap_correct_data_bch;
-	nand->ecc.calculate = omap_calculate_ecc_bch;
-#ifdef CONFIG_AM33XX
-	nand->ecc.read_page = omap_read_page_bch;
-#endif
-	omap_hwecc_init_bch(nand, NAND_ECC_READ);
-#else
-#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_NAND_SOFTECC)
-	nand->ecc.mode = NAND_ECC_SOFT;
-#else
-	nand->ecc.mode = NAND_ECC_HW;
-	nand->ecc.layout = &hw_nand_oob;
-	nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
-	nand->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES;
-	nand->ecc.hwctl = omap_enable_hwecc;
-	nand->ecc.correct = omap_correct_data;
-	nand->ecc.calculate = omap_calculate_ecc;
-	nand->ecc.strength = 1;
-	omap_hwecc_init(nand);
-#endif
-#endif
 
 #ifdef CONFIG_SPL_BUILD
 	if (nand->options & NAND_BUSWIDTH_16)
diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
index 3de30fc..994e235 100644
--- a/include/configs/am335x_evm.h
+++ b/include/configs/am335x_evm.h
@@ -216,11 +216,11 @@
 
 #ifdef CONFIG_NAND
 #define CONFIG_SYS_NAND_5_ADDR_CYCLE
-#define CONFIG_SYS_NAND_PAGE_COUNT	(CONFIG_SYS_NAND_BLOCK_SIZE / \
-					 CONFIG_SYS_NAND_PAGE_SIZE)
+#define CONFIG_SYS_NAND_BLOCK_SIZE	131072
 #define CONFIG_SYS_NAND_PAGE_SIZE	2048
 #define CONFIG_SYS_NAND_OOBSIZE		64
-#define CONFIG_SYS_NAND_BLOCK_SIZE	(128*1024)
+#define CONFIG_SYS_NAND_PAGE_COUNT	(CONFIG_SYS_NAND_BLOCK_SIZE / \
+					 CONFIG_SYS_NAND_PAGE_SIZE)
 #define CONFIG_SYS_NAND_BAD_BLOCK_POS	NAND_LARGE_BADBLOCK_POS
 #define CONFIG_SYS_NAND_ECCPOS		{ 2, 3, 4, 5, 6, 7, 8, 9, \
 					 10, 11, 12, 13, 14, 15, 16, 17, \
@@ -232,9 +232,8 @@
 
 #define CONFIG_SYS_NAND_ECCSIZE		512
 #define CONFIG_SYS_NAND_ECCBYTES	14
-
-#define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
-#define CONFIG_SYS_NAND_U_BOOT_OFFS	0x80000
+#define CONFIG_SYS_NAND_ONFI_DETECTION
+#define CONFIG_NAND_OMAP_ECCSCHEME	6
 #endif
 #endif
 
diff --git a/include/configs/tricorder.h b/include/configs/tricorder.h
index a9b2714..27db618 100644
--- a/include/configs/tricorder.h
+++ b/include/configs/tricorder.h
@@ -110,7 +110,6 @@
 
 #define CONFIG_SYS_MAX_NAND_DEVICE	1		/* Max number of NAND */
 							/* devices */
-#define CONFIG_NAND_OMAP_BCH8
 #define CONFIG_BCH
 
 /* commands to include */
@@ -298,6 +297,8 @@
 
 #define CONFIG_SYS_NAND_ECCSIZE		512
 #define CONFIG_SYS_NAND_ECCBYTES	13
+#define CONFIG_NAND_OMAP_ECCSCHEME	5
+#define CONFIG_SYS_NAND_ONFI_DETECTION
 
 #define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
 
-- 
1.8.1

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Stefan Roese]

Hi Pekon,

On 30.09.2013 16:13, Pekon Gupta wrote:
> BCH8_ECC scheme implemented in omap_gpmc.c driver has following favours
> +-----------------------------------+-----------------+-----------------+
> |ECC Scheme                         | ECC Calculation | Error Detection |
> +-----------------------------------+-----------------+-----------------+
> |OMAP_ECC_BCH8_CODE_HW              |GPMC             |ELM H/W engine   |
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |GPMC             |S/W BCH library  |
> +-----------------------------------+-----------------+-----------------+
> 
> Current implementation limits the BCH8_CODE_HW only for AM33xx device family.
> (using CONFIG_AM33XX). However, other SoC families (like TI81xx) also have
> ELM hardware module, and can support ECC error detection using ELM.
> 
> This patch
> - removes CONFIG_AM33xx
> 	Thus this driver can be reused by all devices having ELM h/w engine.

Do you have an overview, which TI devices actually have this ELM h/w
engine integrated? Especially if its integrated in the OMAP3530 SoC? I'm
currently scanning the users manual and can't find any reference to this
IP block.

Thanks,
Stefan

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Scott Wood]

On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> +Platform specific options
> +=========================
> +
> +   CONFIG_NAND_OMAP_ECCSCHEME
> +	On OMAP platforms, this specifies NAND ECC scheme.
> +	1 - HAM1_SW	1-bit Hamming code using software library
> +			(for legacy devices only)
> +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
> +			(for legacy devices only)
> +	3 - BCH4_SW	4-bit BCH code (unsupported)
> +	4 - BCH4_HW	4-bit BCH code (unsupported)
> +	5 - BCH8_SW	8-bit BCH code with
> +			- ecc calculation using GPMC hardware engine,
> +			- error detection using software library.
> +			- requires CONFIG_BCH to enable software BCH library
> +			(For legacy device which do not have ELM h/w engine)
> +	6 - BCH8_HW	8-bit BCH code with
> +			- ecc calculation using GPMC hardware engine,
> +			- error detection using ELM hardware engine.

You should document the symbols, not the numbers that happen to be
assigned to them.

> +/**
> + * omap_select_ecc_scheme - configures driver for particular ecc-scheme
> + * @nand: NAND chip device structure
> + * @ecc_scheme: ecc scheme to configure
> + * @pagesize: number of main-area bytes per page of NAND device
> + * @oobsize: number of OOB/spare bytes per page of NAND device
> + */
> +static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
> +		unsigned int pagesize, unsigned int oobsize) {

s/int ecc_scheme/enum omap_ecc ecc_scheme/

> +	struct nand_bch_priv	*bch		= nand->priv;
> +	struct nand_ecclayout	*ecclayout	= nand->ecc.layout;
> +	int i;
> +
> +	switch (ecc_scheme) {
> +	case OMAP_ECC_HAM1_CODE_SW:
> +		debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n");
> +		nand->ecc.mode		= NAND_ECC_SOFT;
> +		nand->ecc.layout	= NULL;
> +		nand->ecc.size		= 0;
> +		nand->ecc.strength	= 1;
> +		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_SW;
> +		break;
> +	case OMAP_ECC_HAM1_CODE_HW:
> +		debug("nand: selected OMAP_ECC_HAM1_CODE_HW\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 1;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 3;
> +		nand->ecc.hwctl		= omap_enable_hwecc;
> +		nand->ecc.correct	= omap_correct_data;
> +		nand->ecc.calculate	= omap_calculate_ecc;
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i + BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].offset = i + BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_HW;
> +		break;
> +#ifdef CONFIG_BCH
> +	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
> +		debug("nand: selected OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 8;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 13;
> +		nand->ecc.hwctl		= omap_enable_ecc_bch;
> +		nand->ecc.correct	= omap_correct_data_bch_sw;
> +		nand->ecc.calculate	= omap_calculate_ecc_bch_sw;
> +		/* BCH SW library is used for error detection */
> +		bch_priv.control	= init_bch(13, 8, 0x201b);
> +		if (!bch_priv.control) {
> +			printf("nand: error: could not init_bch()\n");
> +			return -ENODEV;
> +		}
> +		bch_priv.type		= ECC_BCH8;
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i + (oobsize -
> +						ecclayout->eccbytes);
> +		ecclayout->oobfree[0].offset = BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		omap_hwecc_init_bch(nand, NAND_ECC_READ);
> +		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
> +		break;
> +#endif
> +	case OMAP_ECC_BCH8_CODE_HW:
> +		debug("nand: selected OMAP_ECC_BCH8_CODE_HW\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 8;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 14;
> +		nand->ecc.hwctl		= omap_enable_ecc_bch;
> +		nand->ecc.correct	= omap_correct_data_bch;
> +		nand->ecc.calculate	= omap_calculate_ecc_bch;
> +		nand->ecc.read_page	= omap_read_page_bch;
> +		/* ELM is used for ECC error detection */
> +		elm_init();
> +		bch_priv.type		= ECC_BCH8;
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i + BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].offset = i + BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW;
> +		break;
> +	default:
> +		debug("nand: error: ecc scheme not enabled or supported\n");
> +		return -EINVAL;
> +	}
> +
> +	/* check if NAND spare/OOB has enough bytes to accomodate ecclayout */
> +	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize) {
> +		printf("nand: error: insufficient OOB bytes. require=%d\n", (
> +				ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH));
> +		return -EINVAL;
> +	}

Check this before you make any changes to the current ECC setup.

> +		err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW,
> +					mtd->writesize, mtd->oobsize);
> +	}
> +	if (err) {
> +		printf("nand: error: could not switch ecc, reverting\n");
> +		omap_select_ecc_scheme(nand, bch->ecc_scheme,
> +				       mtd->writesize, mtd->oobsize);
> +		return -EINVAL;
>  	}

You won't need to "revert" here if omap_select_ecc_scheme doesn't damage
anything in error cases.

-Scott

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Gupta, Pekon]

Hi,
Please see the replies inline..

> From: Scott Wood [mailto:scottwood at freescale.com]
> > On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> > +Platform specific options
> > +=========================
> > +
> > +   CONFIG_NAND_OMAP_ECCSCHEME
> > +	On OMAP platforms, this specifies NAND ECC scheme.
> > +	1 - HAM1_SW	1-bit Hamming code using software library
> > +			(for legacy devices only)
> > +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
> > +			(for legacy devices only)
> > +	3 - BCH4_SW	4-bit BCH code (unsupported)
> > +	4 - BCH4_HW	4-bit BCH code (unsupported)
> > +	5 - BCH8_SW	8-bit BCH code with
> > +			- ecc calculation using GPMC hardware engine,
> > +			- error detection using software library.
> > +			- requires CONFIG_BCH to enable software BCH
> library
> > +			(For legacy device which do not have ELM h/w
> engine)
> > +	6 - BCH8_HW	8-bit BCH code with
> > +			- ecc calculation using GPMC hardware engine,
> > +			- error detection using ELM hardware engine.
> 
> You should document the symbols, not the numbers that happen to be
> assigned to them.
> 
Sorry din't get you. This is based on your below feedback
http://lists.denx.de/pipermail/u-boot/2013-September/162773.html

Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
It is this number is what user needs to specify in include/configs/*.h
Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
not be exposed to user, user-interface should remain constant. This is
similar to DT binding approach used in linux. Internal symbols are not
exposed to users.


> > +/**
> > + * omap_select_ecc_scheme - configures driver for particular ecc-scheme
> > + * @nand: NAND chip device structure
> > + * @ecc_scheme: ecc scheme to configure
> > + * @pagesize: number of main-area bytes per page of NAND device
> > + * @oobsize: number of OOB/spare bytes per page of NAND device
> > + */
> > +static int omap_select_ecc_scheme(struct nand_chip *nand, int
> ecc_scheme,
> > +		unsigned int pagesize, unsigned int oobsize) {
> 
> s/int ecc_scheme/enum omap_ecc ecc_scheme/
> 
If this is only the cosmetic change, may be I'll take it separately
in another patch. Also 'omap_select_ecc_scheme()' has default
statement in last, which gracefully handles all un-supported ecc-schemes.

[snip]

> > +	/* check if NAND spare/OOB has enough bytes to accomodate
> ecclayout */
> > +	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize)
> {
> > +		printf("nand: error: insufficient OOB bytes. require=%d\n", (
> > +				ecclayout->eccbytes +
> BADBLOCK_MARKER_LENGTH));
> > +		return -EINVAL;
> > +	}
> 
> Check this before you make any changes to the current ECC setup.
> 
'ecclayout->eccbytes' depends on ECC scheme selected, therefore
this check cannot be done before selecting ECC scheme first.


> > +		err = omap_select_ecc_scheme(nand,
> OMAP_ECC_HAM1_CODE_SW,
> > +					mtd->writesize, mtd->oobsize);
> > +	}
> > +	if (err) {
> > +		printf("nand: error: could not switch ecc, reverting\n");
> > +		omap_select_ecc_scheme(nand, bch->ecc_scheme,
> > +				       mtd->writesize, mtd->oobsize);
> > +		return -EINVAL;
> >  	}
> 
> You won't need to "revert" here if omap_select_ecc_scheme doesn't
> damage anything in error cases.
> 
There are two cases when ECC selection could fail half-way:
(1) when if(ecclayout->eccbytes + BADBLOCK > oobsize), as this check
  can only be done after selecting ECC scheme so it can fail

(2) if 'bch_priv.control	= init_bch(13, 8, 0x201b);' fails.
This check is also is done during ecc-scheme selection.

In both above cases, nand_chip.ecc information is half-configured
so this reverting back to old ecc-scheme is essential.

with regards, pekon

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Scott Wood]

On Sat, 2013-10-05 at 06:11 +0000, Gupta, Pekon wrote:
> Hi,
> Please see the replies inline..
> 
> > From: Scott Wood [mailto:scottwood at freescale.com]
> > > On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> > > +Platform specific options
> > > +=========================
> > > +
> > > +   CONFIG_NAND_OMAP_ECCSCHEME
> > > +	On OMAP platforms, this specifies NAND ECC scheme.
> > > +	1 - HAM1_SW	1-bit Hamming code using software library
> > > +			(for legacy devices only)
> > > +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
> > > +			(for legacy devices only)
> > > +	3 - BCH4_SW	4-bit BCH code (unsupported)
> > > +	4 - BCH4_HW	4-bit BCH code (unsupported)
> > > +	5 - BCH8_SW	8-bit BCH code with
> > > +			- ecc calculation using GPMC hardware engine,
> > > +			- error detection using software library.
> > > +			- requires CONFIG_BCH to enable software BCH
> > library
> > > +			(For legacy device which do not have ELM h/w
> > engine)
> > > +	6 - BCH8_HW	8-bit BCH code with
> > > +			- ecc calculation using GPMC hardware engine,
> > > +			- error detection using ELM hardware engine.
> > 
> > You should document the symbols, not the numbers that happen to be
> > assigned to them.
> > 
> Sorry din't get you. This is based on your below feedback
> http://lists.denx.de/pipermail/u-boot/2013-September/162773.html
> 
> Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
> It is this number is what user needs to specify in include/configs/*.h
> Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
> not be exposed to user, user-interface should remain constant.

I disagree.  The user should specify OMAP_ECC_BCH8_CODE_SW.  It's more
greppable, and it's more likely that the number will change than that
the name will.

>  This is similar to DT binding approach used in linux. Internal symbols are not
> exposed to users.

This is a shortcoming of device trees that we should not emulate in C
code.

> > > +/**
> > > + * omap_select_ecc_scheme - configures driver for particular ecc-scheme
> > > + * @nand: NAND chip device structure
> > > + * @ecc_scheme: ecc scheme to configure
> > > + * @pagesize: number of main-area bytes per page of NAND device
> > > + * @oobsize: number of OOB/spare bytes per page of NAND device
> > > + */
> > > +static int omap_select_ecc_scheme(struct nand_chip *nand, int
> > ecc_scheme,
> > > +		unsigned int pagesize, unsigned int oobsize) {
> > 
> > s/int ecc_scheme/enum omap_ecc ecc_scheme/
> > 
> If this is only the cosmetic change, may be I'll take it separately
> in another patch. Also 'omap_select_ecc_scheme()' has default
> statement in last, which gracefully handles all un-supported ecc-schemes.
> 
> [snip]
> 
> > > +	/* check if NAND spare/OOB has enough bytes to accomodate
> > ecclayout */
> > > +	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize)
> > {
> > > +		printf("nand: error: insufficient OOB bytes. require=%d\n", (
> > > +				ecclayout->eccbytes +
> > BADBLOCK_MARKER_LENGTH));
> > > +		return -EINVAL;
> > > +	}
> > 
> > Check this before you make any changes to the current ECC setup.
> > 
> 'ecclayout->eccbytes' depends on ECC scheme selected, therefore
> this check cannot be done before selecting ECC scheme first.

It certainly can be done before you make any changes to the ECC struct.

You could either do it in each individual case, or you could have two
separate switch statements with this code in between them.

> > > +		err = omap_select_ecc_scheme(nand,
> > OMAP_ECC_HAM1_CODE_SW,
> > > +					mtd->writesize, mtd->oobsize);
> > > +	}
> > > +	if (err) {
> > > +		printf("nand: error: could not switch ecc, reverting\n");
> > > +		omap_select_ecc_scheme(nand, bch->ecc_scheme,
> > > +				       mtd->writesize, mtd->oobsize);
> > > +		return -EINVAL;
> > >  	}
> > 
> > You won't need to "revert" here if omap_select_ecc_scheme doesn't
> > damage anything in error cases.
> > 
> There are two cases when ECC selection could fail half-way:
> (1) when if(ecclayout->eccbytes + BADBLOCK > oobsize), as this check
>   can only be done after selecting ECC scheme so it can fail
> 
> (2) if 'bch_priv.control	= init_bch(13, 8, 0x201b);' fails.
> This check is also is done during ecc-scheme selection.
> 
> In both above cases, nand_chip.ecc information is half-configured
> so this reverting back to old ecc-scheme is essential.

If you really can't avoid the damage in error cases, move the cleanup
code into omap_select_ecc_scheme().

-Scott

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/07/2013 09:06 PM, Scott Wood wrote:
> On Sat, 2013-10-05 at 06:11 +0000, Gupta, Pekon wrote:
>> Hi,
>> Please see the replies inline..
>>
>>> From: Scott Wood [mailto:scottwood at freescale.com]
>>>> On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
>>>> +Platform specific options
>>>> +=========================
>>>> +
>>>> +   CONFIG_NAND_OMAP_ECCSCHEME
>>>> +	On OMAP platforms, this specifies NAND ECC scheme.
>>>> +	1 - HAM1_SW	1-bit Hamming code using software library
>>>> +			(for legacy devices only)
>>>> +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
>>>> +			(for legacy devices only)
>>>> +	3 - BCH4_SW	4-bit BCH code (unsupported)
>>>> +	4 - BCH4_HW	4-bit BCH code (unsupported)
>>>> +	5 - BCH8_SW	8-bit BCH code with
>>>> +			- ecc calculation using GPMC hardware engine,
>>>> +			- error detection using software library.
>>>> +			- requires CONFIG_BCH to enable software BCH
>>> library
>>>> +			(For legacy device which do not have ELM h/w
>>> engine)
>>>> +	6 - BCH8_HW	8-bit BCH code with
>>>> +			- ecc calculation using GPMC hardware engine,
>>>> +			- error detection using ELM hardware engine.
>>>
>>> You should document the symbols, not the numbers that happen to be
>>> assigned to them.
>>>
>> Sorry din't get you. This is based on your below feedback
>> http://lists.denx.de/pipermail/u-boot/2013-September/162773.html
>>
>> Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
>> It is this number is what user needs to specify in include/configs/*.h
>> Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
>> not be exposed to user, user-interface should remain constant.
> 
> I disagree.  The user should specify OMAP_ECC_BCH8_CODE_SW.  It's more
> greppable, and it's more likely that the number will change than that
> the name will.

I agree with Scott, enum (if it's not already one) and use the name so
it's clear what you're picking.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSU2cEAAoJENk4IS6UOR1WnOQP/30AInl+d0QVU6QHq+DsGC/q
Wh/yfXbN/EkWVPEvpASS5gftTSd+UjLSzUSWca+TcQxUVdzD64H1u4j8An2cSwJp
vQuX1QZzMWA+g6D+u75BKMs/cIXb6rwX9A5is5R/k5cU4mew+ecLmBeRLcTENfpM
+ROD7XfSJsXYff+dLhpOeVWC6ELzjgmHWajoqpdZNefrMtsrRHOeLd9ccbqYxoNZ
Td56vwMYfk8f+BBRmGlFRdAf+xN+9ssou1CipK6E/H0QHnexRkuPINRdmPD4/p0o
yP+opvOUq8XMYiO76HOGb37tMJvjgziEcZUy4S3Q0gbhRlC9EXz1D0qxs6ti1EFs
9M/Wsl8bsKx4rgdf12hj8iebFxvTdT1lXejJGNk9ThHqsHE0pl+K1cyouZ/GL0NA
sk/UCPe7DL3RIAtkUy+pLT7A3nOKIhqJY1kTsPN24qTF7UVlatWov6C33d42jDzp
YH5CkdP1FRffkRj1in1ex0CiVB/3h3SuI4kA+hceWdx0rGYRT/PRKQA3qSulU1p4
lx3jtzt07fSflPdeX9PxW5k9CqAmXut4rw5USU1OAdpRKANzp1FPXcKXjEVd2uQ1
zkfgUB4MeR6xs6HE6q5xBbaaRhy9zJX415ZG10NtKUXCrVBIoif51EQQmlvehgcy
ufn39bBOmsjkXYXzJI8W
=tQHU
-----END PGP SIGNATURE-----

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Gupta, Pekon]

> From: Scott Wood [mailto:scottwood at freescale.com]
> > On Sat, 2013-10-05 at 06:11 +0000, Gupta, Pekon wrote:
> > Hi,
> > Please see the replies inline..
> >
> > > From: Scott Wood [mailto:scottwood at freescale.com]
> > > > On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> > > > +Platform specific options
> > > > +=========================
> > > > +
> > > > +   CONFIG_NAND_OMAP_ECCSCHEME
> > > > +	On OMAP platforms, this specifies NAND ECC scheme.
> > > > +	1 - HAM1_SW	1-bit Hamming code using software library
> > > > +			(for legacy devices only)
> > > > +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
> > > > +			(for legacy devices only)
> > > > +	3 - BCH4_SW	4-bit BCH code (unsupported)
> > > > +	4 - BCH4_HW	4-bit BCH code (unsupported)
> > > > +	5 - BCH8_SW	8-bit BCH code with
> > > > +			- ecc calculation using GPMC hardware engine,
> > > > +			- error detection using software library.
> > > > +			- requires CONFIG_BCH to enable software BCH
> > > library
> > > > +			(For legacy device which do not have ELM h/w
> > > engine)
> > > > +	6 - BCH8_HW	8-bit BCH code with
> > > > +			- ecc calculation using GPMC hardware engine,
> > > > +			- error detection using ELM hardware engine.
> > >
> > > You should document the symbols, not the numbers that happen to be
> > > assigned to them.
> > >
> > Sorry din't get you. This is based on your below feedback
> > http://lists.denx.de/pipermail/u-boot/2013-September/162773.html
> >
> > Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
> > It is this number is what user needs to specify in include/configs/*.h
> > Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
> > not be exposed to user, user-interface should remain constant.
> 
> I disagree.  The user should specify OMAP_ECC_BCH8_CODE_SW.  It's more
> greppable, and it's more likely that the number will change than that
> the name will.
> 
Ok, accepted.


> >  This is similar to DT binding approach used in linux. Internal symbols are not
> > exposed to users.
> 
> This is a shortcoming of device trees that we should not emulate in C
> code.
> 
Don't know if its short-coming of device-tree or difference between
u-boot and kernel ideologies. But use of internal symbols (enums) in DT
was  opposed by one of the kernel maintainers. Refer below:
http://lists.infradead.org/pipermail/linux-mtd/2013-May/047030.html


> > > > +/**
> > > > + * omap_select_ecc_scheme - configures driver for particular ecc-
> scheme
> > > > + * @nand: NAND chip device structure
> > > > + * @ecc_scheme: ecc scheme to configure
> > > > + * @pagesize: number of main-area bytes per page of NAND device
> > > > + * @oobsize: number of OOB/spare bytes per page of NAND device
> > > > + */
> > > > +static int omap_select_ecc_scheme(struct nand_chip *nand, int
> > > ecc_scheme,
> > > > +		unsigned int pagesize, unsigned int oobsize) {
> > >
> > > s/int ecc_scheme/enum omap_ecc ecc_scheme/
> > >
> > If this is only the cosmetic change, may be I'll take it separately
> > in another patch. Also 'omap_select_ecc_scheme()' has default
> > statement in last, which gracefully handles all un-supported ecc-schemes.
> >
> > [snip]
> >
> > > > +	/* check if NAND spare/OOB has enough bytes to accomodate
> > > ecclayout */
> > > > +	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize)
> > > {
> > > > +		printf("nand: error: insufficient OOB bytes. require=%d\n", (
> > > > +				ecclayout->eccbytes +
> > > BADBLOCK_MARKER_LENGTH));
> > > > +		return -EINVAL;
> > > > +	}
> > >
> > > Check this before you make any changes to the current ECC setup.
> > >
> > 'ecclayout->eccbytes' depends on ECC scheme selected, therefore
> > this check cannot be done before selecting ECC scheme first.
> 
> It certainly can be done before you make any changes to the ECC struct.
> 
> You could either do it in each individual case, or you could have two
> separate switch statements with this code in between them.
> 
Having 2 switch statements would clutter the code. Isn't it ?
(1) first switch-case would do a dry-run to check the ecc requirements.
(2) second switch-case would do actual assignments.

And omap_select_ecc_scheme() would be called only once in whole
lifetime, because:
(1) omap devices have deprecated the dynamic switching of ecc-schemes.
(2) with falcon-mode where SPL directly loads kernel, most driver configs
   would be statically derived from CONFIGS_xx.

Anyways I would take the changes if you wish so..
But request you to please provide comments on all the patches, before
I send next revision. This would help me consolidate all changes.


> > > > +		err = omap_select_ecc_scheme(nand,
> > > OMAP_ECC_HAM1_CODE_SW,
> > > > +					mtd->writesize, mtd->oobsize);
> > > > +	}
> > > > +	if (err) {
> > > > +		printf("nand: error: could not switch ecc, reverting\n");
> > > > +		omap_select_ecc_scheme(nand, bch->ecc_scheme,
> > > > +				       mtd->writesize, mtd->oobsize);
> > > > +		return -EINVAL;
> > > >  	}
> > >
> > > You won't need to "revert" here if omap_select_ecc_scheme doesn't
> > > damage anything in error cases.
> > >
> > There are two cases when ECC selection could fail half-way:
> > (1) when if(ecclayout->eccbytes + BADBLOCK > oobsize), as this check
> >   can only be done after selecting ECC scheme so it can fail
> >
> > (2) if 'bch_priv.control	= init_bch(13, 8, 0x201b);' fails.
> > This check is also is done during ecc-scheme selection.
> >
> > In both above cases, nand_chip.ecc information is half-configured
> > so this reverting back to old ecc-scheme is essential.
> 
> If you really can't avoid the damage in error cases, move the cleanup
> code into omap_select_ecc_scheme().
> 
As described above, this has dependency on ecc information
So again I have to go by 2 switch statement approach which would
clutter the code. I'm okay if you want it so..

But request you to review and provide comments to other patches
of this series, OR else atleast Ack/Nak them. This way my effort on
re-working and testing the patches again and again would reduce.

So, should I wait for review of other remaining patches before
sending next version v8 of this series ?


with regards, pekon

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Scott Wood]

On Tue, 2013-10-08 at 05:30 +0000, Gupta, Pekon wrote:
> > From: Scott Wood [mailto:scottwood at freescale.com]
> > > On Sat, 2013-10-05 at 06:11 +0000, Gupta, Pekon wrote:
> > > Hi,
> > > Please see the replies inline..
> > >
> > > > From: Scott Wood [mailto:scottwood at freescale.com]
> > > > > On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> > > > > +Platform specific options
> > > > > +=========================
> > > > > +
> > > > > +   CONFIG_NAND_OMAP_ECCSCHEME
> > > > > +	On OMAP platforms, this specifies NAND ECC scheme.
> > > > > +	1 - HAM1_SW	1-bit Hamming code using software library
> > > > > +			(for legacy devices only)
> > > > > +	2 - HAM1_HW	1-bit Hamming code using GPMC hardware engine
> > > > > +			(for legacy devices only)
> > > > > +	3 - BCH4_SW	4-bit BCH code (unsupported)
> > > > > +	4 - BCH4_HW	4-bit BCH code (unsupported)
> > > > > +	5 - BCH8_SW	8-bit BCH code with
> > > > > +			- ecc calculation using GPMC hardware engine,
> > > > > +			- error detection using software library.
> > > > > +			- requires CONFIG_BCH to enable software BCH
> > > > library
> > > > > +			(For legacy device which do not have ELM h/w
> > > > engine)
> > > > > +	6 - BCH8_HW	8-bit BCH code with
> > > > > +			- ecc calculation using GPMC hardware engine,
> > > > > +			- error detection using ELM hardware engine.
> > > >
> > > > You should document the symbols, not the numbers that happen to be
> > > > assigned to them.
> > > >
> > > Sorry din't get you. This is based on your below feedback
> > > http://lists.denx.de/pipermail/u-boot/2013-September/162773.html
> > >
> > > Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
> > > It is this number is what user needs to specify in include/configs/*.h
> > > Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
> > > not be exposed to user, user-interface should remain constant.
> > 
> > I disagree.  The user should specify OMAP_ECC_BCH8_CODE_SW.  It's more
> > greppable, and it's more likely that the number will change than that
> > the name will.
> > 
> Ok, accepted.
> 
> 
> > >  This is similar to DT binding approach used in linux. Internal symbols are not
> > > exposed to users.
> > 
> > This is a shortcoming of device trees that we should not emulate in C
> > code.
> > 
> Don't know if its short-coming of device-tree or difference between
> u-boot and kernel ideologies. But use of internal symbols (enums) in DT
> was  opposed by one of the kernel maintainers. Refer below:
> http://lists.infradead.org/pipermail/linux-mtd/2013-May/047030.html

It's a different situation.  The device tree is supposed to be stable
ABI.  This isn't.  The device tree is also supposed to be OS-independent
and thus taking Linux identifiers as-is is frowned upon (as the Linux
identifiers may change, plus device tree properties use a different
style for naming).

It's not a general difference between U-Boot and Linux.

> > > > > +/**
> > > > > + * omap_select_ecc_scheme - configures driver for particular ecc-
> > scheme
> > > > > + * @nand: NAND chip device structure
> > > > > + * @ecc_scheme: ecc scheme to configure
> > > > > + * @pagesize: number of main-area bytes per page of NAND device
> > > > > + * @oobsize: number of OOB/spare bytes per page of NAND device
> > > > > + */
> > > > > +static int omap_select_ecc_scheme(struct nand_chip *nand, int
> > > > ecc_scheme,
> > > > > +		unsigned int pagesize, unsigned int oobsize) {
> > > >
> > > > s/int ecc_scheme/enum omap_ecc ecc_scheme/
> > > >
> > > If this is only the cosmetic change, may be I'll take it separately
> > > in another patch. Also 'omap_select_ecc_scheme()' has default
> > > statement in last, which gracefully handles all un-supported ecc-schemes.
> > >
> > > [snip]
> > >
> > > > > +	/* check if NAND spare/OOB has enough bytes to accomodate
> > > > ecclayout */
> > > > > +	if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize)
> > > > {
> > > > > +		printf("nand: error: insufficient OOB bytes. require=%d\n", (
> > > > > +				ecclayout->eccbytes +
> > > > BADBLOCK_MARKER_LENGTH));
> > > > > +		return -EINVAL;
> > > > > +	}
> > > >
> > > > Check this before you make any changes to the current ECC setup.
> > > >
> > > 'ecclayout->eccbytes' depends on ECC scheme selected, therefore
> > > this check cannot be done before selecting ECC scheme first.
> > 
> > It certainly can be done before you make any changes to the ECC struct.
> > 
> > You could either do it in each individual case, or you could have two
> > separate switch statements with this code in between them.
> > 
> Having 2 switch statements would clutter the code. Isn't it ?
> (1) first switch-case would do a dry-run to check the ecc requirements.
> (2) second switch-case would do actual assignments.

It wouldn't be that bad.  It would eliminate the need for recovery code
and substantially reduce the complexity of a (likely little-tested, at
least long term) error path.

> And omap_select_ecc_scheme() would be called only once in whole
> lifetime, because:
> (1) omap devices have deprecated the dynamic switching of ecc-schemes.
> (2) with falcon-mode where SPL directly loads kernel, most driver configs
>    would be statically derived from CONFIGS_xx.
> 
> Anyways I would take the changes if you wish so..
> But request you to please provide comments on all the patches, before
> I send next revision. This would help me consolidate all changes.

I'm working on it, but I also have a lot of other patches to review, and
doing nothing but patch reviews all day long can be very draining (not
to mention starving other tasks that need to be done), and reviewing
patches for hardware I'm not familiar with is even worse.  I realize
that the delays can be frustrating from your end, but I only have so
much time and energy to expend.  Is there nobody else who's familiar
with this driver that can help review?  Looking at the commit history no
single person stands out as a logical maintainer for this driver.  It
would be nice to designate someone.

> So, should I wait for review of other remaining patches before
> sending next version v8 of this series ?

Yes.

-Scott

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Scott Wood]

On Tue, 2013-10-08 at 11:52 -0500, Scott Wood wrote:
> On Tue, 2013-10-08 at 05:30 +0000, Gupta, Pekon wrote:
> > Anyways I would take the changes if you wish so..
> > But request you to please provide comments on all the patches, before
> > I send next revision. This would help me consolidate all changes.
> 
> I'm working on it, but I also have a lot of other patches to review, and
> doing nothing but patch reviews all day long can be very draining (not
> to mention starving other tasks that need to be done), and reviewing
> patches for hardware I'm not familiar with is even worse.  I realize
> that the delays can be frustrating from your end, but I only have so
> much time and energy to expend.  Is there nobody else who's familiar
> with this driver that can help review?  Looking at the commit history no
> single person stands out as a logical maintainer for this driver.  It
> would be nice to designate someone.
> 
> > So, should I wait for review of other remaining patches before
> > sending next version v8 of this series ?
> 
> Yes.

OK, I've scanned the rest of the patches and given some minor feedback. 

-Scott

[U-Boot] [PATCH v7 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Gupta, Pekon]

Hi,

> On Tue, 2013-10-08 at 11:52 -0500, Scott Wood wrote:
> > On Tue, 2013-10-08 at 05:30 +0000, Gupta, Pekon wrote:
> > Anyways I would take the changes if you wish so..
> > But request you to please provide comments on all the patches, before
> > I send next revision. This would help me consolidate all changes.
>
> I'm working on it, but I also have a lot of other patches to review, and
> doing nothing but patch reviews all day long can be very draining (not
> to mention starving other tasks that need to be done), and reviewing
> patches for hardware I'm not familiar with is even worse.  I realize
> that the delays can be frustrating from your end, but I only have so
> much time and energy to expend.
> 
Firstly my apologies for being bit aggressive in getting patches
reviewed. Yes, I was bit irritated as patch re-submissions were
taking much of my energy and time as well, while lot of other things
are still pending on me to cleanup this driver and add functionality to it.

And I understand the patch reviewing is tough because reviewer
needs to first understand the driver & hardware context, and then
reply from a broader point of view of generic frame-work.


>   Is there nobody else who's familiar
> with this driver that can help review?  Looking at the commit history no
> single person stands out as a logical maintainer for this driver.  It
> would be nice to designate someone.
> 
Currently I'm the only supporting NAND driver in both kernel and
u-boot from omap side. And therefore, I plan to get both u-boot and 
kernel nand drivers cleaned-up and optimized, so that it becomes
scalable for future enhancements.
Aim is to get most of end-users to boot from mainline, so that they
are self dependent and up-to-date to new frameworks.

Once I'm done with my current commitments, I can help you in getting
some enhancements for generic framework.
For example, following patch adds checks for automatic bus-width
detection in generic NAND framework.
http://lists.denx.de/pipermail/u-boot/2013-September/163882.html

Hope such things help reduce some of your load.
And thanks again for the reviews, I'll submit next revision of patch soon.


with regards, pekon

[U-Boot] [PATCH v7 2/5] mtd: nand: omap: optimize chip->ecc.hwctl() for H/W ECC schemes

[Pekon Gupta]

chip->ecc.hwctl() is used for preparing the H/W controller before read/write
NAND accesses (like assigning data-buf, enabling ECC scheme configs, etc.)

Though all ECC schemes in OMAP NAND driver use GPMC controller for generating
ECC syndrome (for both Read/Write accesses). But but in current code
HAM1_ECC and BCHx_ECC schemes implement individual function to achieve this.
This patch
(1) removes omap_hwecc_init() and omap_hwecc_init_bch()
as chip->ecc.hwctl will re-initializeGPMC before every read/write call.
omap_hwecc_init_bch() -> omap_enable_ecc_bch()

(2) merges the GPMC configuration code for all ECC schemes into
single omap_enable_hwecc(), thus adding scalability for future ECC schemes.
omap_enable_hwecc() + omap_enable_ecc_bch() -> omap_enable_hwecc()

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 arch/arm/include/asm/omap_gpmc.h |   7 --
 drivers/mtd/nand/omap_gpmc.c     | 192 +++++++++++----------------------------
 2 files changed, 54 insertions(+), 145 deletions(-)

diff --git a/arch/arm/include/asm/omap_gpmc.h b/arch/arm/include/asm/omap_gpmc.h
index dd40cb6..9660842 100644
--- a/arch/arm/include/asm/omap_gpmc.h
+++ b/arch/arm/include/asm/omap_gpmc.h
@@ -14,13 +14,6 @@
 #define GPMC_BUF_EMPTY	0
 #define GPMC_BUF_FULL	1
 
-#define ECCCLEAR	(0x1 << 8)
-#define ECCRESULTREG1	(0x1 << 0)
-#define ECCSIZE512BYTE	0xFF
-#define ECCSIZE1	(ECCSIZE512BYTE << 22)
-#define ECCSIZE0	(ECCSIZE512BYTE << 12)
-#define ECCSIZE0SEL	(0x000 << 0)
-
 /* Generic ECC Layouts */
 /* Large Page x8 NAND device Layout */
 #ifdef GPMC_NAND_ECC_LP_x8_LAYOUT
diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 7396bee..67998ef 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -19,6 +19,8 @@
 
 #define BADBLOCK_MARKER_LENGTH	2
 #define SECTOR_BYTES		512
+#define ECCCLEAR		(0x1 << 8)
+#define ECCRESULTREG1		(0x1 << 0)
 
 static uint8_t cs;
 static __maybe_unused struct nand_ecclayout omap_ecclayout;
@@ -60,21 +62,6 @@ int omap_spl_dev_ready(struct mtd_info *mtd)
 }
 #endif
 
-/*
- * omap_hwecc_init - Initialize the Hardware ECC for NAND flash in
- *                   GPMC controller
- * @mtd:        MTD device structure
- *
- */
-static void __maybe_unused omap_hwecc_init(struct nand_chip *chip)
-{
-	/*
-	 * Init ECC Control Register
-	 * Clear all ECC | Enable Reg1
-	 */
-	writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
-	writel(ECCSIZE1 | ECCSIZE0 | ECCSIZE0SEL, &gpmc_cfg->ecc_size_config);
-}
 
 /*
  * gen_true_ecc - This function will generate true ECC value, which
@@ -192,38 +179,6 @@ static int __maybe_unused omap_calculate_ecc(struct mtd_info *mtd,
 }
 
 /*
- * omap_enable_ecc - This function enables the hardware ecc functionality
- * @mtd:        MTD device structure
- * @mode:       Read/Write mode
- */
-static void __maybe_unused omap_enable_hwecc(struct mtd_info *mtd, int32_t mode)
-{
-	struct nand_chip *chip = mtd->priv;
-	uint32_t val, dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
-
-	switch (mode) {
-	case NAND_ECC_READ:
-	case NAND_ECC_WRITE:
-		/* Clear the ecc result registers, select ecc reg as 1 */
-		writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
-
-		/*
-		 * Size 0 = 0xFF, Size1 is 0xFF - both are 512 bytes
-		 * tell all regs to generate size0 sized regs
-		 * we just have a single ECC engine for all CS
-		 */
-		writel(ECCSIZE1 | ECCSIZE0 | ECCSIZE0SEL,
-			&gpmc_cfg->ecc_size_config);
-		val = (dev_width << 7) | (cs << 1) | (0x1);
-		writel(val, &gpmc_cfg->ecc_config);
-		break;
-	default:
-		printf("Error: Unrecognized Mode[%d]!\n", mode);
-		break;
-	}
-}
-
-/*
  * Generic BCH interface
  */
 struct nand_bch_priv {
@@ -263,103 +218,65 @@ static __maybe_unused struct nand_bch_priv bch_priv = {
 };
 
 /*
- * omap_hwecc_init_bch - Initialize the BCH Hardware ECC for NAND flash in
- *				GPMC controller
+ * omap_enable_hwecc - configures GPMC as per ECC scheme before read/write
  * @mtd:	MTD device structure
  * @mode:	Read/Write mode
  */
 __maybe_unused
-static void omap_hwecc_init_bch(struct nand_chip *chip, int32_t mode)
+static void omap_enable_hwecc(struct mtd_info *mtd, int32_t mode)
 {
-	uint32_t val;
-	uint32_t dev_width = (chip->options & NAND_BUSWIDTH_16) >> 1;
-	uint32_t unused_length = 0;
-	uint32_t wr_mode = BCH_WRAPMODE_6;
-	struct nand_bch_priv *bch = chip->priv;
-
-	/* Clear the ecc result registers, select ecc reg as 1 */
-	writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
-
-	if (bch->ecc_scheme == OMAP_ECC_BCH8_CODE_HW) {
-		wr_mode = BCH_WRAPMODE_1;
-
-	switch (bch->nibbles) {
-	case ECC_BCH4_NIBBLES:
-		unused_length = 3;
-		break;
-	case ECC_BCH8_NIBBLES:
-		unused_length = 2;
-		break;
-	case ECC_BCH16_NIBBLES:
-		unused_length = 0;
+	struct nand_chip	*nand	= mtd->priv;
+	struct nand_bch_priv	*bch	= nand->priv;
+	unsigned int dev_width = (nand->options & NAND_BUSWIDTH_16) ? 1 : 0;
+	unsigned int ecc_algo = 0;
+	unsigned int bch_type = 0;
+	unsigned int eccsize1 = 0x00, eccsize0 = 0x00, bch_wrapmode = 0x00;
+	u32 ecc_size_config_val = 0;
+	u32 ecc_config_val = 0;
+
+	/* configure GPMC for specific ecc-scheme */
+	switch (bch->ecc_scheme) {
+	case OMAP_ECC_HAM1_CODE_SW:
+		return;
+	case OMAP_ECC_HAM1_CODE_HW:
+		ecc_algo = 0x0;
+		bch_type = 0x0;
+		bch_wrapmode = 0x00;
+		eccsize0 = 0xFF;
+		eccsize1 = 0xFF;
 		break;
-	}
-
-	/*
-	 * This is ecc_size_config for ELM mode.
-	 * Here we are using different settings for read and write access and
-	 * also depending on BCH strength.
-	 */
-	switch (mode) {
-	case NAND_ECC_WRITE:
-		/* write access only setup eccsize1 config */
-		val = ((unused_length + bch->nibbles) << 22);
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+	case OMAP_ECC_BCH8_CODE_HW:
+		ecc_algo = 0x1;
+		bch_type = 0x1;
+		if (mode == NAND_ECC_WRITE) {
+			bch_wrapmode = 0x01;
+			eccsize0 = 0;  /* extra bits in nibbles per sector */
+			eccsize1 = 28; /* OOB bits in nibbles per sector */
+		} else {
+			bch_wrapmode = 0x01;
+			eccsize0 = 26; /* ECC bits in nibbles per sector */
+			eccsize1 = 2;  /* non-ECC bits in nibbles per sector */
+		}
 		break;
-
-	case NAND_ECC_READ:
 	default:
-		/*
-		 * by default eccsize0 selected for ecc1resultsize
-		 * eccsize0 config.
-		 */
-		val  = (bch->nibbles << 12);
-		/* eccsize1 config */
-		val |= (unused_length << 22);
-		break;
-	}
-	} else {
-	/*
-	 * This ecc_size_config setting is for BCH sw library.
-	 *
-	 * Note: we only support BCH8 currently with BCH sw library!
-	 * Should be really easy to adobt to BCH4, however some omap3 have
-	 * flaws with BCH4.
-	 *
-	 * Here we are using wrapping mode 6 both for reading and writing, with:
-	 *  size0 = 0  (no additional protected byte in spare area)
-	 *  size1 = 32 (skip 32 nibbles = 16 bytes per sector in spare area)
-	 */
-	val = (32 << 22) | (0 << 12);
+		return;
 	}
-	/* ecc size configuration */
-	writel(val, &gpmc_cfg->ecc_size_config);
-
-	/*
-	 * Configure the ecc engine in gpmc
-	 * We assume 512 Byte sector pages for access to NAND.
-	 */
-	val  = (1 << 16);		/* enable BCH mode */
-	val |= (bch->type << 12);	/* setup BCH type */
-	val |= (wr_mode << 8);		/* setup wrapping mode */
-	val |= (dev_width << 7);	/* setup device width (16 or 8 bit) */
-	val |= (cs << 1);		/* setup chip select to work on */
-	debug("set ECC_CONFIG=0x%08x\n", val);
-	writel(val, &gpmc_cfg->ecc_config);
-}
-
-/*
- * omap_enable_ecc_bch - This function enables the bch h/w ecc functionality
- * @mtd:	MTD device structure
- * @mode:	Read/Write mode
- */
-__maybe_unused
-static void omap_enable_ecc_bch(struct mtd_info *mtd, int32_t mode)
-{
-	struct nand_chip *chip = mtd->priv;
-
-	omap_hwecc_init_bch(chip, mode);
-	/* enable ecc */
-	writel((readl(&gpmc_cfg->ecc_config) | 0x1), &gpmc_cfg->ecc_config);
+	/* Clear ecc and enable bits */
+	writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
+	/* Configure ecc size for BCH */
+	ecc_size_config_val = (eccsize1 << 22) | (eccsize0 << 12);
+	writel(ecc_size_config_val, &gpmc_cfg->ecc_size_config);
+
+	/* Configure device details for BCH engine */
+	ecc_config_val = ((ecc_algo << 16)	| /* HAM1 | BCHx */
+			(bch_type << 12)	| /* BCH4/BCH8/BCH16 */
+			(bch_wrapmode << 8)	| /* wrap mode */
+			(dev_width << 7)	| /* bus width */
+			(0x0 << 4)		| /* number of sectors */
+			(cs <<  1)		| /* ECC CS */
+			(0x1));			  /* enable ECC */
+	writel(ecc_config_val, &gpmc_cfg->ecc_config);
 }
 
 /*
@@ -797,7 +714,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
 		nand->ecc.strength	= 8;
 		nand->ecc.size		= 512;
 		nand->ecc.bytes		= 13;
-		nand->ecc.hwctl		= omap_enable_ecc_bch;
+		nand->ecc.hwctl		= omap_enable_hwecc;
 		nand->ecc.correct	= omap_correct_data_bch_sw;
 		nand->ecc.calculate	= omap_calculate_ecc_bch_sw;
 		/* BCH SW library is used for error detection */
@@ -816,7 +733,6 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
 		ecclayout->oobfree[0].offset = BADBLOCK_MARKER_LENGTH;
 		ecclayout->oobfree[0].length = oobsize - ecclayout->eccbytes -
 						BADBLOCK_MARKER_LENGTH;
-		omap_hwecc_init_bch(nand, NAND_ECC_READ);
 		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
 		break;
 #endif
@@ -826,7 +742,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
 		nand->ecc.strength	= 8;
 		nand->ecc.size		= 512;
 		nand->ecc.bytes		= 14;
-		nand->ecc.hwctl		= omap_enable_ecc_bch;
+		nand->ecc.hwctl		= omap_enable_hwecc;
 		nand->ecc.correct	= omap_correct_data_bch;
 		nand->ecc.calculate	= omap_calculate_ecc_bch;
 		nand->ecc.read_page	= omap_read_page_bch;
-- 
1.8.1

[U-Boot] [PATCH v7 3/5] mtd: nand: omap: optimize chip->ecc.calculate() for H/W ECC schemes

[Pekon Gupta]

chip->ecc.calculate() is used for calculating and fetching of ECC syndrome by
processing the data passed during Read/Write accesses.

All H/W based ECC schemes use GPMC controller to calculate ECC syndrome.
But each BCHx_ECC scheme has its own implemetation of post-processing and
fetching ECC syndrome from GPMC controller.

This patch updates OMAP_ECC_BCH8_CODE_HW ECC scheme in following way:
- merges multiple chip->calculate API for different ECC schemes
  omap_calculate_ecc() + omap_calculate_ecc_bch() + omap_calculate_ecc_bch_sw()
				==> omap_calculate_ecc()
- removes omap_ecc_disable() and instead uses it as inline.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap_gpmc.c | 220 ++++++++++++-------------------------------
 1 file changed, 59 insertions(+), 161 deletions(-)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 67998ef..76be758 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -21,7 +21,10 @@
 #define SECTOR_BYTES		512
 #define ECCCLEAR		(0x1 << 8)
 #define ECCRESULTREG1		(0x1 << 0)
-
+#ifdef CONFIG_BCH
+static u8  bch8_polynomial[] = {0xef, 0x51, 0x2e, 0x09, 0xed, 0x93, 0x9a, 0xc2,
+				0x97, 0x79, 0xe5, 0x24, 0xb5};
+#endif
 static uint8_t cs;
 static __maybe_unused struct nand_ecclayout omap_ecclayout;
 
@@ -143,42 +146,6 @@ static int __maybe_unused omap_correct_data(struct mtd_info *mtd, uint8_t *dat,
 }
 
 /*
- *  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.
- *  E.g. if page is 0xFF (fresh erased), and if HW ECC engine within GPMC
- *  is used, 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.
- *  @mtd:	MTD structure
- *  @dat:	unused
- *  @ecc_code:	ecc_code buffer
- */
-static int __maybe_unused omap_calculate_ecc(struct mtd_info *mtd,
-		const uint8_t *dat, uint8_t *ecc_code)
-{
-	u_int32_t val;
-
-	/* Start Reading from HW ECC1_Result = 0x200 */
-	val = readl(&gpmc_cfg->ecc1_result);
-
-	ecc_code[0] = val & 0xFF;
-	ecc_code[1] = (val >> 16) & 0xFF;
-	ecc_code[2] = ((val >> 8) & 0x0F) | ((val >> 20) & 0xF0);
-
-	/*
-	 * Stop reading anymore ECC vals and clear old results
-	 * enable will be called if more reads are required
-	 */
-	writel(0x000, &gpmc_cfg->ecc_config);
-
-	return 0;
-}
-
-/*
  * Generic BCH interface
  */
 struct nand_bch_priv {
@@ -194,12 +161,7 @@ struct nand_bch_priv {
 #define ECC_BCH8	1
 #define ECC_BCH16	2
 
-/* GPMC ecc engine settings */
-#define BCH_WRAPMODE_1		1	/* BCH wrap mode 1 */
-#define BCH_WRAPMODE_6		6	/* BCH wrap mode 6 */
-
 /* BCH nibbles for diff bch levels */
-#define NAND_ECC_HW_BCH ((uint8_t)(NAND_ECC_HW_OOB_FIRST) + 1)
 #define ECC_BCH4_NIBBLES	13
 #define ECC_BCH8_NIBBLES	26
 #define ECC_BCH16_NIBBLES	52
@@ -211,7 +173,6 @@ struct nand_bch_priv {
  * When some users with other BCH strength will exists this have to change!
  */
 static __maybe_unused struct nand_bch_priv bch_priv = {
-	.mode = NAND_ECC_HW_BCH,
 	.type = ECC_BCH8,
 	.nibbles = ECC_BCH8_NIBBLES,
 	.control = NULL
@@ -280,54 +241,71 @@ static void omap_enable_hwecc(struct mtd_info *mtd, int32_t mode)
 }
 
 /*
- * omap_ecc_disable - Disable H/W ECC calculation
- *
- * @mtd:	MTD device structure
- */
-static void __maybe_unused omap_ecc_disable(struct mtd_info *mtd)
-{
-	writel((readl(&gpmc_cfg->ecc_config) & ~0x1), &gpmc_cfg->ecc_config);
-}
-
-/*
- * BCH support using ELM module
- */
-/*
- * omap_read_bch8_result - Read BCH result for BCH8 level
+ *  omap_calculate_ecc - Read ECC result
+ *  @mtd:	MTD structure
+ *  @dat:	unused
+ *  @ecc_code:	ecc_code buffer
  *
- * @mtd:	MTD device structure
- * @big_endian:	When set read register 3 first
- * @ecc_code:	Read syndrome from BCH result registers
+ *  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.
+ *  E.g. if page is 0xFF (fresh erased), and if HW ECC engine within GPMC
+ *  is used, 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.
  */
-static void omap_read_bch8_result(struct mtd_info *mtd, uint8_t big_endian,
+static int omap_calculate_ecc(struct mtd_info *mtd, const uint8_t *dat,
 				uint8_t *ecc_code)
 {
-	uint32_t *ptr;
+	struct nand_chip *chip = mtd->priv;
+	struct nand_bch_priv *bch = chip->priv;
+	uint32_t *ptr, val = 0;
 	int8_t i = 0, j;
 
-	if (big_endian) {
+	switch (bch->ecc_scheme) {
+	case OMAP_ECC_HAM1_CODE_HW:
+		val = readl(&gpmc_cfg->ecc1_result);
+		ecc_code[0] = val & 0xFF;
+		ecc_code[1] = (val >> 16) & 0xFF;
+		ecc_code[2] = ((val >> 8) & 0x0F) | ((val >> 20) & 0xF0);
+		break;
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+	case OMAP_ECC_BCH8_CODE_HW:
 		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3];
-		ecc_code[i++] = readl(ptr) & 0xFF;
+		val = readl(ptr);
+		ecc_code[i++] = (val >>  0) & 0xFF;
 		ptr--;
 		for (j = 0; j < 3; j++) {
-			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
-			ecc_code[i++] = readl(ptr) & 0xFF;
+			val = readl(ptr);
+			ecc_code[i++] = (val >> 24) & 0xFF;
+			ecc_code[i++] = (val >> 16) & 0xFF;
+			ecc_code[i++] = (val >>  8) & 0xFF;
+			ecc_code[i++] = (val >>  0) & 0xFF;
 			ptr--;
 		}
-	} else {
-		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[0];
-		for (j = 0; j < 3; j++) {
-			ecc_code[i++] = readl(ptr) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
-			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;
-			ptr++;
-		}
-		ecc_code[i++] = readl(ptr) & 0xFF;
-		ecc_code[i++] = 0;	/* 14th byte is always zero */
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* ECC scheme specific syndrome customizations */
+	switch (bch->ecc_scheme) {
+	case OMAP_ECC_HAM1_CODE_HW:
+		break;
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+#ifdef CONFIG_BCH
+		for (i = 0; i < chip->ecc.bytes; i++)
+			*(ecc_code + i) = *(ecc_code + i) ^
+						bch8_polynomial[i];
+#endif
+		break;
+	case OMAP_ECC_BCH8_CODE_HW:
+		ecc_code[chip->ecc.bytes - 1] = 0x00;
+		break;
+	default:
+		return -EINVAL;
 	}
+	return 0;
 }
 
 /*
@@ -366,35 +344,6 @@ static void omap_rotate_ecc_bch(struct mtd_info *mtd, uint8_t *calc_ecc,
 }
 
 /*
- *  omap_calculate_ecc_bch - Read BCH ECC result
- *
- *  @mtd:	MTD structure
- *  @dat:	unused
- *  @ecc_code:	ecc_code buffer
- */
-static int omap_calculate_ecc_bch(struct mtd_info *mtd, const uint8_t *dat,
-				uint8_t *ecc_code)
-{
-	struct nand_chip *chip = mtd->priv;
-	struct nand_bch_priv *bch = chip->priv;
-	uint8_t big_endian = 1;
-	int8_t ret = 0;
-
-	if (bch->type == ECC_BCH8)
-		omap_read_bch8_result(mtd, big_endian, ecc_code);
-	else /* BCH4 and BCH16 currently not supported */
-		ret = -1;
-
-	/*
-	 * Stop reading anymore ECC vals and clear old results
-	 * enable will be called if more reads are required
-	 */
-	omap_ecc_disable(mtd);
-
-	return ret;
-}
-
-/*
  * omap_fix_errors_bch - Correct bch error in the data
  *
  * @mtd:	MTD device structure
@@ -549,57 +498,6 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
  * OMAP3 BCH8 support (with BCH library)
  */
 #ifdef CONFIG_BCH
-/*
- *  omap_calculate_ecc_bch_sw - Read BCH ECC result
- *
- *  @mtd:	MTD device structure
- *  @dat:	The pointer to data on which ecc is computed (unused here)
- *  @ecc:	The ECC output buffer
- */
-static int omap_calculate_ecc_bch_sw(struct mtd_info *mtd, const uint8_t *dat,
-				uint8_t *ecc)
-{
-	int ret = 0;
-	size_t i;
-	unsigned long nsectors, val1, val2, val3, val4;
-
-	nsectors = ((readl(&gpmc_cfg->ecc_config) >> 4) & 0x7) + 1;
-
-	for (i = 0; i < nsectors; i++) {
-		/* Read hw-computed remainder */
-		val1 = readl(&gpmc_cfg->bch_result_0_3[i].bch_result_x[0]);
-		val2 = readl(&gpmc_cfg->bch_result_0_3[i].bch_result_x[1]);
-		val3 = readl(&gpmc_cfg->bch_result_0_3[i].bch_result_x[2]);
-		val4 = readl(&gpmc_cfg->bch_result_0_3[i].bch_result_x[3]);
-
-		/*
-		 * Add constant polynomial to remainder, in order to get an ecc
-		 * sequence of 0xFFs for a buffer filled with 0xFFs.
-		 */
-		*ecc++ = 0xef ^ (val4 & 0xFF);
-		*ecc++ = 0x51 ^ ((val3 >> 24) & 0xFF);
-		*ecc++ = 0x2e ^ ((val3 >> 16) & 0xFF);
-		*ecc++ = 0x09 ^ ((val3 >> 8) & 0xFF);
-		*ecc++ = 0xed ^ (val3 & 0xFF);
-		*ecc++ = 0x93 ^ ((val2 >> 24) & 0xFF);
-		*ecc++ = 0x9a ^ ((val2 >> 16) & 0xFF);
-		*ecc++ = 0xc2 ^ ((val2 >> 8) & 0xFF);
-		*ecc++ = 0x97 ^ (val2 & 0xFF);
-		*ecc++ = 0x79 ^ ((val1 >> 24) & 0xFF);
-		*ecc++ = 0xe5 ^ ((val1 >> 16) & 0xFF);
-		*ecc++ = 0x24 ^ ((val1 >> 8) & 0xFF);
-		*ecc++ = 0xb5 ^ (val1 & 0xFF);
-	}
-
-	/*
-	 * Stop reading anymore ECC vals and clear old results
-	 * enable will be called if more reads are required
-	 */
-	omap_ecc_disable(mtd);
-
-	return ret;
-}
-
 /**
  * omap_correct_data_bch_sw - Decode received data and correct errors
  * @mtd: MTD device structure
@@ -716,7 +614,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
 		nand->ecc.bytes		= 13;
 		nand->ecc.hwctl		= omap_enable_hwecc;
 		nand->ecc.correct	= omap_correct_data_bch_sw;
-		nand->ecc.calculate	= omap_calculate_ecc_bch_sw;
+		nand->ecc.calculate	= omap_calculate_ecc;
 		/* BCH SW library is used for error detection */
 		bch_priv.control	= init_bch(13, 8, 0x201b);
 		if (!bch_priv.control) {
@@ -744,7 +642,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int ecc_scheme,
 		nand->ecc.bytes		= 14;
 		nand->ecc.hwctl		= omap_enable_hwecc;
 		nand->ecc.correct	= omap_correct_data_bch;
-		nand->ecc.calculate	= omap_calculate_ecc_bch;
+		nand->ecc.calculate	= omap_calculate_ecc;
 		nand->ecc.read_page	= omap_read_page_bch;
 		/* ELM is used for ECC error detection */
 		elm_init();
-- 
1.8.1

[U-Boot] [PATCH v7 3/5] mtd: nand: omap: optimize chip->ecc.calculate() for H/W ECC schemes

[Scott Wood]

On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> +	/* ECC scheme specific syndrome customizations */
> +	switch (bch->ecc_scheme) {
> +	case OMAP_ECC_HAM1_CODE_HW:
> +		break;
> +	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
> +#ifdef CONFIG_BCH
> +		for (i = 0; i < chip->ecc.bytes; i++)
> +			*(ecc_code + i) = *(ecc_code + i) ^
> +						bch8_polynomial[i];
> +#endif
> +		break;
> +	case OMAP_ECC_BCH8_CODE_HW:
> +		ecc_code[chip->ecc.bytes - 1] = 0x00;
> +		break;
> +	default:
> +		return -EINVAL;
>  	}

Shouldn't "case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:" be inside the
"#ifdef CONFIG_BCH" so that you get -EINVAL if the implementation isn't
there?

-Scott

[U-Boot] [PATCH v7 4/5] mtd: nand: omap: optimized chip->ecc.correct() for H/W ECC schemes

[Pekon Gupta]

chip->ecc.correct() is used for detecting and correcting bit-flips during read
operations. In omap-nand driver it implemented as:
(a) omap_correct_data(): for h/w based ECC_HAM1 scheme
(b) omap_correct_data_bch() + CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
	for ECC_BCH8 scheme using GPMC and software lib/bch.c
(c) omap_correct_data_bch() + CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
	for ECC_BCH8 scheme using GPMC and ELM

This patch updates (c)
- checks for calc_ecc[]==0x00 so that error_correction is not required for
  known good pages.
- adds scalability for other ECC_BCHx scheme by merging following
  omap_rotate_ecc_bch() + omap_fix_errors_bch() => omap_correct_data_bch()
- fixing logic for bit-flip correction based on error_loc[count]

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap_gpmc.c | 159 +++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 90 deletions(-)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 76be758..24e6f8f 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -21,6 +21,9 @@
 #define SECTOR_BYTES		512
 #define ECCCLEAR		(0x1 << 8)
 #define ECCRESULTREG1		(0x1 << 0)
+/* 4 bit padding to make byte aligned, 56 = 52 + 4 */
+#define BCH4_BIT_PAD		4
+
 #ifdef CONFIG_BCH
 static u8  bch8_polynomial[] = {0xef, 0x51, 0x2e, 0x09, 0xed, 0x93, 0x9a, 0xc2,
 				0x97, 0x79, 0xe5, 0x24, 0xb5};
@@ -179,6 +182,23 @@ static __maybe_unused struct nand_bch_priv bch_priv = {
 };
 
 /*
+ * omap_reverse_list - re-orders list elements in reverse order [internal]
+ * @list:	pointer to start of list
+ * @length:	length of list
+*/
+void omap_reverse_list(u8 *list, unsigned int length)
+{
+	unsigned int i, j;
+	unsigned int half_length = length / 2;
+	u8 tmp;
+	for (i = 0, j = length - 1; i < half_length; i++, j--) {
+		tmp = list[i];
+		list[i] = list[j];
+		list[j] = tmp;
+	}
+}
+
+/*
  * omap_enable_hwecc - configures GPMC as per ECC scheme before read/write
  * @mtd:	MTD device structure
  * @mode:	Read/Write mode
@@ -309,77 +329,6 @@ static int omap_calculate_ecc(struct mtd_info *mtd, const uint8_t *dat,
 }
 
 /*
- * omap_rotate_ecc_bch - Rotate the syndrome bytes
- *
- * @mtd:	MTD device structure
- * @calc_ecc:	ECC read from ECC registers
- * @syndrome:	Rotated syndrome will be retuned in this array
- *
- */
-static void omap_rotate_ecc_bch(struct mtd_info *mtd, uint8_t *calc_ecc,
-		uint8_t *syndrome)
-{
-	struct nand_chip *chip = mtd->priv;
-	struct nand_bch_priv *bch = chip->priv;
-	uint8_t n_bytes = 0;
-	int8_t i, j;
-
-	switch (bch->type) {
-	case ECC_BCH4:
-		n_bytes = 8;
-		break;
-
-	case ECC_BCH16:
-		n_bytes = 28;
-		break;
-
-	case ECC_BCH8:
-	default:
-		n_bytes = 13;
-		break;
-	}
-
-	for (i = 0, j = (n_bytes-1); i < n_bytes; i++, j--)
-		syndrome[i] =  calc_ecc[j];
-}
-
-/*
- * omap_fix_errors_bch - Correct bch error in the data
- *
- * @mtd:	MTD device structure
- * @data:	Data read from flash
- * @error_count:Number of errors in data
- * @error_loc:	Locations of errors in the data
- *
- */
-static void omap_fix_errors_bch(struct mtd_info *mtd, uint8_t *data,
-		uint32_t error_count, uint32_t *error_loc)
-{
-	struct nand_chip *chip = mtd->priv;
-	struct nand_bch_priv *bch = chip->priv;
-	uint8_t count = 0;
-	uint32_t error_byte_pos;
-	uint32_t error_bit_mask;
-	uint32_t last_bit = (bch->nibbles * 4) - 1;
-
-	/* Flip all bits as specified by the error location array. */
-	/* FOR( each found error location flip the bit ) */
-	for (count = 0; count < error_count; count++) {
-		if (error_loc[count] > last_bit) {
-			/* Remove the ECC spare bits from correction. */
-			error_loc[count] -= (last_bit + 1);
-			/* Offset bit in data region */
-			error_byte_pos = ((512 * 8) -
-					(error_loc[count]) - 1) / 8;
-			/* Error Bit mask */
-			error_bit_mask = 0x1 << (error_loc[count] % 8);
-			/* Toggle the error bit to make the correction. */
-			data[error_byte_pos] ^= error_bit_mask;
-		}
-	}
-}
-
-/*
  * omap_correct_data_bch - Compares the ecc read from nand spare area
  * with ECC registers values and corrects one bit error if it has occured
  *
@@ -395,40 +344,70 @@ static int omap_correct_data_bch(struct mtd_info *mtd, uint8_t *dat,
 {
 	struct nand_chip *chip = mtd->priv;
 	struct nand_bch_priv *bch = chip->priv;
-	uint8_t syndrome[28];
-	uint32_t error_count = 0;
+	uint32_t eccbytes = chip->ecc.bytes;
+	uint32_t error_count = 0, error_max;
 	uint32_t error_loc[8];
-	uint32_t i, ecc_flag;
+	uint32_t i, ecc_flag = 0;
+	uint8_t count, err = 0;
+	uint32_t byte_pos, bit_pos;
+
+	/* check calculated ecc */
+	for (i = 0; i < chip->ecc.bytes && !ecc_flag; i++)
+		if (calc_ecc[i] != 0x00)
+			ecc_flag = 1;
+	if (!ecc_flag)
+		return 0;
 
+	/* check for whether its a erased-page */
 	ecc_flag = 0;
-	for (i = 0; i < chip->ecc.bytes; i++)
+	for (i = 0; i < chip->ecc.bytes && !ecc_flag; i++)
 		if (read_ecc[i] != 0xff)
 			ecc_flag = 1;
-
 	if (!ecc_flag)
 		return 0;
 
-	elm_reset();
-	elm_config((enum bch_level)(bch->type));
-
 	/*
 	 * while reading ECC result we read it in big endian.
 	 * Hence while loading to ELM we have rotate to get the right endian.
 	 */
-	omap_rotate_ecc_bch(mtd, calc_ecc, syndrome);
-
+	switch (bch->ecc_scheme) {
+	case OMAP_ECC_BCH8_CODE_HW:
+		omap_reverse_list(calc_ecc, eccbytes - 1);
+		break;
+	default:
+		return -EINVAL;
+	}
 	/* use elm module to check for errors */
-	if (elm_check_error(syndrome, bch->nibbles, &error_count,
-				error_loc) != 0) {
-		printf("ECC: uncorrectable.\n");
-		return -1;
+	elm_config((enum bch_level)(bch->type));
+	if (elm_check_error(calc_ecc, bch->nibbles, &error_count, error_loc)) {
+		printf("nand: error: uncorrectable ECC errors\n");
+		return -EINVAL;
 	}
-
 	/* correct bch error */
-	if (error_count > 0)
-		omap_fix_errors_bch(mtd, dat, error_count, error_loc);
-
-	return 0;
+	for (count = 0; count < error_count; count++) {
+		switch (bch->type) {
+		case ECC_BCH8:
+			/* 14th byte in ECC is reserved to match ROM layout */
+			error_max = SECTOR_BYTES + (eccbytes - 1);
+			break;
+		default:
+			return -EINVAL;
+		}
+		byte_pos = error_max - (error_loc[count] / 8) - 1;
+		bit_pos  = error_loc[count] % 8;
+		if (byte_pos < SECTOR_BYTES) {
+			dat[byte_pos] ^= 1 << bit_pos;
+			printf("nand: bit-flip corrected @data=%d\n", byte_pos);
+		} else if (byte_pos < error_max) {
+			read_ecc[byte_pos - SECTOR_BYTES] = 1 << bit_pos;
+			printf("nand: bit-flip corrected @oob=%d\n", byte_pos -
+								SECTOR_BYTES);
+		} else {
+			err = -EBADMSG;
+			printf("nand: error: invalid bit-flip location\n");
+		}
+	}
+	return (err) ? err : error_count;
 }
 
 /**
-- 
1.8.1

[U-Boot] [PATCH v7 4/5] mtd: nand: omap: optimized chip->ecc.correct() for H/W ECC schemes

[Scott Wood]

On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> +	/* check calculated ecc */
> +	for (i = 0; i < chip->ecc.bytes && !ecc_flag; i++)
> +		if (calc_ecc[i] != 0x00)
> +			ecc_flag = 1;

U-Boot code style wants braces around multi-line bodies.

-Scott

[U-Boot] [PATCH v7 5/5] board/ti/am335x/README: update for NAND boot

[Pekon Gupta]

NAND boot mode on AM335x EVM has been verified, and steps
to use it has been documented and update in this README

Signed-off-by: Pekon Gupta <pekon@ti.com>
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Acked-by: Tom Rini <trini@ti.com>
---
 board/ti/am335x/README | 53 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/board/ti/am335x/README b/board/ti/am335x/README
index 2a30ab8..947305b 100644
--- a/board/ti/am335x/README
+++ b/board/ti/am335x/README
@@ -46,23 +46,42 @@ NAND
 The AM335x GP EVM ships with a 256MiB NAND available in most profiles.  In
 this example to program the NAND we assume that an SD card has been
 inserted with the files to write in the first SD slot and that mtdparts
-have been configured correctly for the board.  As a time saving measure we
-load MLO into memory in one location, copy it into the three locatations
-that the ROM checks for additional valid copies, then load U-Boot into
-memory.  We then write that whole section of memory to NAND.
-
-U-Boot # mmc rescan
-U-Boot # env default -f -a
-U-Boot # nand erase.chip
-U-Boot # saveenv
-U-Boot # load mmc 0 81000000 MLO
-U-Boot # cp.b 81000000 81020000 20000
-U-Boot # cp.b 81000000 81040000 20000
-U-Boot # cp.b 81000000 81060000 20000
-U-Boot # load mmc 0 81080000 u-boot.img
-U-Boot # nand write 81000000 0 260000
-U-Boot # load mmc 0 ${loadaddr} uImage
-U-Boot # nand write ${loadaddr} kernel 500000
+have been configured correctly for the board. All images are first loaded
+into memory, then written to NAND.
+
+Step-1: Building u-boot for NAND boot
+	Set following CONFIGxx options for NAND device.
+	CONFIG_SYS_NAND_PAGE_SIZE	number of main bytes in NAND page
+	CONFIG_SYS_NAND_OOBSIZE		number of OOB bytes in NAND page
+	CONFIG_SYS_NAND_BLOCK_SIZE	number of bytes in NAND erase-block
+	CONFIG_SYS_NAND_ECCPOS		ECC map for NAND page
+	CONFIG_NAND_OMAP_ECCSCHEME	(refer doc/README.nand)
+
+Step-2: Flashing NAND via MMC/SD
+	# select BOOTSEL to MMC/SD boot and boot from MMC/SD card
+	U-Boot # mmc rescan
+	# erase flash
+	U-Boot # nand erase.chip
+	U-Boot # env default -f -a
+	U-Boot # saveenv
+	# flash MLO. Redundant copies of MLO are kept for failsafe
+	U-Boot # load mmc 0 0x82000000 MLO
+	U-Boot # nand write 0x82000000 0x00000 0x20000
+	U-Boot # nand write 0x82000000 0x20000 0x20000
+	U-Boot # nand write 0x82000000 0x40000 0x20000
+	U-Boot # nand write 0x82000000 0x60000 0x20000
+	# flash u-boot.img
+	U-Boot # load mmc 0 0x82000000 u-boot.img
+	U-Boot # nand write 0x82000000 0x80000 0x60000
+	# flash kernel image
+	U-Boot # load mmc 0 0x82000000 uImage
+	U-Boot # nand write 0x82000000 ${nandsrcaddr} ${nandimgsize}
+	# flash filesystem image
+	U-Boot # load mmc 0 0x82000000 filesystem.img
+	U-Boot # nand write 0x82000000 ${loadaddress} 0x300000
+
+Step-3: Set BOOTSEL pin to select NAND boot, and POR the device.
+	The device should boot from images flashed on NAND device.
 
 NOR
 ===
-- 
1.8.1

[U-Boot] [PATCH v7 0/5] mtd: nand: omap: optimize and clean-up of OMAP NAND driver

[Gupta, Pekon]

Hi Scott, Tom,

> Subject: [PATCH v7 0/5] mtd: nand: omap: optimize and clean-up of OMAP
> NAND driver
> 
> *changes in v7*
> [PATCH 1/5]
> 	- omap_gpmc.c: fix: free bytes in OOB (ecclayout-
> >oobfree[0].length)
> 	- omap_gpmc.c: cleanup: redundant code added in previous patch
> versions
> 	- am335x_evm.h: cleanup: redundant code added in previous patch
> versions
> 	- tricorder.h: fix: CONFIG_NAND_OMAP_ECCSCHEME
> [PATCH 2/5] removed: re-configuration of gpmc.config1[dev_width] added
> in
> 		previous version of patch
> [PATCH 3/5] <no update>
> [PATCH 4/5] <no update>
> [PATCH 5/5] minor fix: missing '$' in ${loadaddr}
> 
> 

Request you to please accept this patch series if everything is ok ?
This one is more of driver clean-up and restructuring.

And is independent of another patch-series..
[PATCH v2 0/4] enable support for x16 NAND devices

with regards, pekon

[U-Boot] [PATCH v7 0/5] mtd: nand: omap: optimize and clean-up of OMAP NAND driver

[Tom Rini]

On Mon, Sep 30, 2013 at 07:43:33PM +0530, Pekon Gupta wrote:

> *changes in v7*
> [PATCH 1/5]
> 	- omap_gpmc.c: fix: free bytes in OOB (ecclayout->oobfree[0].length)
> 	- omap_gpmc.c: cleanup: redundant code added in previous patch versions
> 	- am335x_evm.h: cleanup: redundant code added in previous patch versions
> 	- tricorder.h: fix: CONFIG_NAND_OMAP_ECCSCHEME
> [PATCH 2/5] removed: re-configuration of gpmc.config1[dev_width] added in
> 		previous version of patch
> [PATCH 3/5] <no update>
> [PATCH 4/5] <no update>
> [PATCH 5/5] minor fix: missing '$' in ${loadaddr}

OK, with this series applied, please build all of the OMAP platforms,
and fix the various build problems that are introduced by this.

I see problems with: am3517_crane am335x_boneblack rut igep0020_nand
omap3_zoom2 omap3_overo dig297 omap3_evm omap3_logic igep0033 tricorder
mcx igep0030_nand twister pxm2 omap3_evm_quick_nand am43xx_evm
mt_ventoux ti816x_evm am335x_evm_spiboot pcm051 dxr2 eco5pk devkit8000
am3517_evm cm_t35 omap3_beagle omap3_zoom1 omap3_pandora

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131017/41a589af/attachment.pgp>