diff mbox

[U-Boot,v1,1/4] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

Message ID 1375782920-25935-2-git-send-email-pekon@ti.com
State Superseded
Headers show

Commit Message

pekon gupta Aug. 6, 2013, 9:55 a.m. UTC 
BCH8_ECC scheme implemented in omap_gpmc.c driver has following two 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 enables of BCH8_CODE_HW only for AM33xx SoC 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
- replaces CONFIG_AM33xx define with generic CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
  so that all device families having required h/w capability can use ELM for
  error detection in ECC_BCHx schemes.

- replaces CONFIG_NAND_OMAP_BCH8 with CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
  and separates out code for above mentioned BCH8_ECC implementations so that
  driver can be build independently using anyone of them.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap_gpmc.c | 117 +++++++++++++++++++++++--------------------
 include/configs/am335x_evm.h |   1 +
 include/configs/ti814x_evm.h |   2 +-
 include/configs/tricorder.h  |   2 +-
 4 files changed, 65 insertions(+), 57 deletions(-)

Comments

Scott Wood Aug. 10, 2013, 12:45 a.m. UTC | #1 
On Tue, 2013-08-06 at 15:25 +0530, Pekon Gupta wrote:
> BCH8_ECC scheme implemented in omap_gpmc.c driver has following two 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 enables of BCH8_CODE_HW only for AM33xx SoC 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
> - replaces CONFIG_AM33xx define with generic CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
>   so that all device families having required h/w capability can use ELM for
>   error detection in ECC_BCHx schemes.
> 
> - replaces CONFIG_NAND_OMAP_BCH8 with CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
>   and separates out code for above mentioned BCH8_ECC implementations so that
>   driver can be build independently using anyone of them.

Please document these CONFIG symbols in the README.

Is the choice of ECC mode dictated by the hardware, or is it software's
choice?  If the former, it should be CONFIG_SYS rather than just CONFIG.

> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/omap_gpmc.c | 117 +++++++++++++++++++++++--------------------
>  include/configs/am335x_evm.h |   1 +
>  include/configs/ti814x_evm.h |   2 +-
>  include/configs/tricorder.h  |   2 +-
>  4 files changed, 65 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
> index ec1787f..c6c5cec 100644
> --- a/drivers/mtd/nand/omap_gpmc.c
> +++ b/drivers/mtd/nand/omap_gpmc.c
> @@ -12,11 +12,12 @@
>  #include <asm/arch/cpu.h>
>  #include <asm/omap_gpmc.h>
>  #include <linux/mtd/nand_ecc.h>
> -#include <linux/bch.h>
>  #include <linux/compiler.h>
>  #include <nand.h>
> -#ifdef CONFIG_AM33XX
> +#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
>  #include <asm/arch/elm.h>
> +#elif defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
> +#include <linux/bch.h>
>  #endif

Normally includes don't get ifdeffed...  and what is the connection
between a particular ECC mode and asm/arch/elm.h?

-Scott
Scott Wood Aug. 13, 2013, 12:40 a.m. UTC | #2 
On Mon, 2013-08-12 at 13:31 +0000, Gupta, Pekon wrote:
> Hi,
> > 
> > On Tue, 2013-08-06 at 15:25 +0530, Pekon Gupta wrote:
> > > This patch
> > > - replaces CONFIG_AM33xx define with generic
> > CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
> > >   so that all device families having required h/w capability can use ELM for
> > >   error detection in ECC_BCHx schemes.
> > >
> > > - replaces CONFIG_NAND_OMAP_BCH8 with
> > CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
> > >   and separates out code for above mentioned BCH8_ECC implementations
> > so that
> > >   driver can be build independently using anyone of them.
> > 
> > Please document these CONFIG symbols in the README.
> > 
> [Pekon]: should I add them to doc/README.nand ? 

Yes.

> Also, in broad sense these define suggest which flavor of ECC scheme to use
> BCH8_HW, BCH8_SW, BCH4_HW, BCH4_SW, etc..
> So in-order to make them generic for use across all vendors I can rename
> them to CONFIG_SYS_NAND_ECC_xx (omitting OMAP in name).

I don't think this would be appropriate, especially with the "SYS" in
the name.  The generic approach is that this is configured at runtime.
Whether a specific controller driver uses CONFIG symbols or other
mechanisms to determine what sort of ECC to use is up to the controller
driver (most controller drivers probably only support one type for any
given type of NAND chip).

Plus, this way you only need to focus on the options that are relevant
to OMAP.

> > Is the choice of ECC mode dictated by the hardware, or is it software's
> > choice?  If the former, it should be CONFIG_SYS rather than just CONFIG.
> > 
> [Pekon] Some older OMAP device do not support ELM hardware. So in those
> CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW is default.
> In newer devices, supporting ELM hardware
> CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW is default.

What does "DETECTION_SW" mean?

> > > -#ifdef CONFIG_AM33XX
> > > +#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
> > >  #include <asm/arch/elm.h>
> > > +#elif
> > defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
> > > +#include <linux/bch.h>
> > >  #endif
> > 
> > Normally includes don't get ifdeffed...  and what is the connection
> > between a particular ECC mode and asm/arch/elm.h?
> > 
> [Pekon] Both the Config do the same BCH8 ECC scheme, but implementation
> is different. One uses S/W library, while other uses ELM hardware engine.

Why can't you include both headers?  They don't appear to be exposing
the same interface...

> - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
> Include API for S/W library (lib/bch.c) so included that here..
> But this ECC scheme should be used only for older OMAP platforms where
>  ELM is not present
> 
> - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
> Include declarations for ELM functions. So new OMAP platforms which have
> ELM hardware engine, need not include whole bch.c library in their code.

Maybe it should just be something like CONFIG_SYS_OMAP_ELM?

-Scott
Scott Wood Aug. 13, 2013, 8:03 p.m. UTC | #3 
On Tue, 2013-08-13 at 05:00 +0000, Gupta, Pekon wrote:
> > > - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
> > > Include API for S/W library (lib/bch.c) so included that here..
> > > But this ECC scheme should be used only for older OMAP platforms where
> > >  ELM is not present
> > >
> > > - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
> > > Include declarations for ELM functions. So new OMAP platforms which
> > have
> > > ELM hardware engine, need not include whole bch.c library in their code.
> > 
> > Maybe it should just be something like CONFIG_SYS_OMAP_ELM?
> > 
> Actually is not just decides between using ELM or not. CONFIGs also
> selects which all ECC scheme to support (multiple selection allowed).
> 
> Following ECC schemes can be supported on omap-nand driver..
> [1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/101028
> 
> So to keep code foot-print small, configs are used to select out some
> Un-needed ECC schemes on newer devices.

If the hardware doesn't dictate a specific choice, use CONFIG rather
than CONFIG_SYS, even if not all hardware supports all options.

-Scott
diff mbox

Patch

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index ec1787f..c6c5cec 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -12,11 +12,12 @@ 
 #include <asm/arch/cpu.h>
 #include <asm/omap_gpmc.h>
 #include <linux/mtd/nand_ecc.h>
-#include <linux/bch.h>
 #include <linux/compiler.h>
 #include <nand.h>
-#ifdef CONFIG_AM33XX
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
 #include <asm/arch/elm.h>
+#elif defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
+#include <linux/bch.h>
 #endif
 
 static uint8_t cs;
@@ -274,7 +275,7 @@  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
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
 	uint32_t unused_length = 0;
 #endif
 	uint32_t wr_mode = BCH_WRAPMODE_6;
@@ -283,7 +284,7 @@  static void omap_hwecc_init_bch(struct nand_chip *chip, int32_t mode)
 	/* Clear the ecc result registers, select ecc reg as 1 */
 	writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
 
-#ifdef CONFIG_AM33XX
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
 	wr_mode = BCH_WRAPMODE_1;
 
 	switch (bch->nibbles) {
@@ -375,10 +376,10 @@  static void __maybe_unused omap_ecc_disable(struct mtd_info *mtd)
 	writel((readl(&gpmc_cfg->ecc_config) & ~0x1), &gpmc_cfg->ecc_config);
 }
 
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
 /*
  * BCH8 support (needs ELM and thus AM33xx-only)
  */
-#ifdef CONFIG_AM33XX
 /*
  * omap_read_bch8_result - Read BCH result for BCH8 level
  *
@@ -631,12 +632,12 @@  static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 	return 0;
 }
-#endif /* CONFIG_AM33XX */
+#endif /* CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW */
 
 /*
  * OMAP3 BCH8 support (with BCH library)
  */
-#ifdef CONFIG_NAND_OMAP_BCH8
+#ifdef CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
 /*
  *  omap_calculate_ecc_bch - Read BCH ECC result
  *
@@ -752,7 +753,7 @@  static void __maybe_unused omap_free_bch(struct mtd_info *mtd)
 		chip_priv->control = NULL;
 	}
 }
-#endif /* CONFIG_NAND_OMAP_BCH8 */
+#endif /* CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW */
 
 #ifndef CONFIG_SPL_BUILD
 /*
@@ -803,25 +804,32 @@  void omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength)
 			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 if (eccstrength == 8) {
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
+			nand->ecc.mode		= NAND_ECC_HW;
+			nand->ecc.layout	= &hw_bch8_nand_oob;
+			nand->ecc.size		= 512;
+			nand->ecc.bytes		= 14;
+			nand->ecc.read_page	= omap_read_page_bch;
+			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("using OMAP_ECC_BCH8_CODE_HW\n");
+#elif defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
+			nand->ecc.mode		= NAND_ECC_HW;
+			nand->ecc.layout	= &hw_bch8_nand_oob;
+			nand->ecc.size		= 512;
+			nand->ecc.bytes		= 13;
+			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("using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
 #else
-			nand->ecc.bytes = 13;
+			printf("selected ECC not supported or not enabled\n");
 #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");
 		}
-#endif
 	} else {
 		nand->ecc.mode = NAND_ECC_SOFT;
 		/* Use mtd default settings */
@@ -894,44 +902,44 @@  int board_nand_init(struct nand_chip *nand)
 
 	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 */
+#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
+	printf("nand: using OMAP_ECC_BCH8_CODE_HW\n");
+	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;
+	nand->ecc.read_page	= omap_read_page_bch;
+	omap_hwecc_init_bch(nand, NAND_ECC_READ);
+	/* ELM is used for ECC error detection */
 	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 */);
+	nand->priv = &bch_priv;
+#elif defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
+	printf("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
+	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;
+	omap_hwecc_init_bch(nand, NAND_ECC_READ);
+	/* BCH SW library is used for error detection */
+	bch_priv.control	= init_bch(13, 8, 0x201b /* hw polynominal */);
 	if (!bch_priv.control) {
 		puts("Could not init_bch()\n");
 		return -ENODEV;
 	}
-#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)
+#elif !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_NAND_SOFTECC)
+	printf("nand: using OMAP_ECC_HAM1_CODE_SW\n");
 	nand->ecc.mode = NAND_ECC_SOFT;
 #else
+	printf("nand: using OMAP_ECC_HAM1_CODE_HW\n");
 	nand->ecc.mode = NAND_ECC_HW;
 	nand->ecc.layout = &hw_nand_oob;
 	nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE;
@@ -942,7 +950,6 @@  int board_nand_init(struct nand_chip *nand)
 	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 c5a6d4b..8f3bd54 100644
--- a/include/configs/am335x_evm.h
+++ b/include/configs/am335x_evm.h
@@ -493,6 +493,7 @@ 
 #define CONFIG_NAND
 /* NAND support */
 #ifdef CONFIG_NAND
+#define CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
 #define CONFIG_CMD_NAND
 #define CONFIG_CMD_MTDPARTS
 #define MTDIDS_DEFAULT			"nand0=omap2-nand.0"
diff --git a/include/configs/ti814x_evm.h b/include/configs/ti814x_evm.h
index b6fafc7..546cc07 100644
--- a/include/configs/ti814x_evm.h
+++ b/include/configs/ti814x_evm.h
@@ -286,7 +286,7 @@ 
 #define CONFIG_NAND
 /* NAND support */
 #ifdef CONFIG_NAND
-#define CONFIG_MTD_NAND_OMAP_BCH
+#define CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
 #define CONFIG_CMD_NAND
 #define CONFIG_CMD_MTDPARTS
 #define MTDIDS_DEFAULT			"nand0=omap2-nand.0"
diff --git a/include/configs/tricorder.h b/include/configs/tricorder.h
index 4e2cb65..dc4f6dc 100644
--- a/include/configs/tricorder.h
+++ b/include/configs/tricorder.h
@@ -109,7 +109,7 @@ 
 
 #define CONFIG_SYS_MAX_NAND_DEVICE	1		/* Max number of NAND */
 							/* devices */
-#define CONFIG_NAND_OMAP_BCH8
+#define CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
 #define CONFIG_BCH
 
 /* commands to include */