[PATCH v9 0/9] mtd:nand:omap2: clean-up of supported ECC schemes

[PATCH v9 0/9] mtd:nand:omap2: clean-up of supported ECC schemes

[Pekon Gupta]

*changes v8 -> v9*
[PATCH 1/9] <no update from [PATCH v8 1/6]>
[PATCH 2/9] <only commit log updated from [PATCH v8 2/6]>
 As per feedbacks from Brian Norris <computersforpeace@gmail.com> previous
 revision [PATCH v8 3/6] and [PATCH 4/6] are split into following sub-patches:
- [PATCH 3/9] <new> replaces local reference with generic names (mtd, nand_chip)
- [PATCH 4/9] <new> enables auto-detection of bus-width
- [PATCH 5/9] <new> removes omap3_init_bch: populates ecc-scheme data
- [PATCH 6/9] <new> removes omap3_init_bch_tail: populates ecc-layout
- [PATCH 7/9] <new> replaces lib/bch.c with nand_bch.c wrapper
[PATCH 8/9] <no update same as [PATCH v8 5/6]
[PATCH 9/9] removed devm_free_xx functions


*Changes v7 -> v8*
[PATCH 1/6] <no updates>
[PATCH 2/6]
	- updated DT parsing of "ti,nand-ecc-opts" so that its "ham1" remains
		compatible to "sw","hw","hw-romcode"
	- updated DT parsing of "ti,elm-id" to retain compatibility to "elm_id"
	- using of_parse_phandle() to get ELM device pointer from DT
[PATCH 3..6/6] <commit log updates>


*Changes v6 -> v7*
[PATCH 1/6] <NEW> split from [PATCH v6 2/4] as per feedbacks from Brian Norris <computersforpeace@gmail.com>
[PATCH 2/6] incorporated feedbacks from DT maintainers
[PATCH 3/6] cleaned and incorporated feedbacks from Brian Norris <computersforpeace@gmail.com>
[PATCH 4/6] rebasing changes and cleanup
[PATCH 5/6] updated omap3430-sdp.dts
[PATCH 6/6] <NEW> updated for devm_xx


*Changes v5 -> v6*
[PATCH 1/4]: 
	- updated DT binding for gpmc-nand based on 'Olof Johansson's feedbacks
	http://lists.infradead.org/pipermail/linux-mtd/2013-August/048394.html
	- detection of ELM device via ti,elm-id DT node, moved to gpmc.c driver
[PATCH 2/4]
	- removed: support for following obselete ECC schemes
	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC)
	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme)
	- updated: using omap_oobinfo as chip->ecc.layout for all ecc-schemes
	- clean: error messages
[PATCH 3/4] cleaned to include changes for OMAP_ECC_BCH8_CODE_HW only
[PATCH 4/4] updated to include DT property changes


*Changes v4 -> v5*
- Rebased to linux-next 
IMPORTANT: Need to revert commit fb1585b, [PATCH 2/4] part of previous version
	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html

- Swapped PATCH-1 & PATCH-2 to maintain bisectibility & compilation dependency
	http://lists.infradead.org/pipermail/linux-mtd/2013-July/047461.html

- PATCH-2: re-ordered call to is_elm_present() for later updates ELM driver
	- dropped changes in include/linux/platform_data/elm.h (not needed)
- PATCH-3: re-ordered call to is_elm_present() for later updates ELM driver
- Re-formated patch description (replaced tabs with white-spaces)


*Changes v3 -> v4*
(Resent with CC: devicetree-discuss@lists.ozlabs.org)
- [Patch 1/3] removed MTD_NAND_OMAP_BCH8 & MTD_NAND_OMAP_BCH4 from nand/Kconfig
	ECC scheme selectable via nand DT (nand-ecc-opt).
- [*] rebased for l2-mtd.git


*Changes v2 -> v3*
(Resent with Author Name fixed)
- PATCH-1: re-arranged code to remove redundancy, added NAND_BUSWIDTH_AUTO
- PATCH-2: updated nand-ecc-opt DT mapping and Documentation
- PATCH-3: code-cleaning + changes to match PATCH-1
- PATCH-4 <DROPPED> update DT attribute for ti,nand-ecc-opt 
	- received feedback to keep DT mapping independent of linuxism
- PATCH-4:<NEW> : ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
	- independent patch for AM335x-evm.dts update based on PATCH-2


*Changes v1 -> v2*
	added 	[PATCH 3/4] and [PATCH 4/4]


After this patch series, omap2-nand driver will supports following ECC schemes:
+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAM1_CODE_HW                  |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
| (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
|                                       |               |               |
|OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
| (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
|        ti,elm-id)                     |               |               |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
| (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
|                                       |               |               |
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
| (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
|        ti,elm-id)                     |               |               |
+---------------------------------------+---------------+---------------+


Pekon Gupta (9):
  mtd: nand: omap: combine different flavours of 1-bit hamming ecc
    schemes
  ARM: OMAP2+: cleaned-up DT support of various ECC schemes
  mtd: nand: omap: cleanup: replace local references with generic
    framework names
  mtd: nand: omap: enable auto-detection of bus-width for omap-nand
    drivers
  mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in
    device_probe
  mtd: nand: omap: clean-up ecc layout for BCH ecc schemes
  mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC
    instead of lib/bch.c
  ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
  mtd: nand: omap: updated devm_xx for all resource allocation and free
    calls

 .../devicetree/bindings/mtd/gpmc-nand.txt          |  16 +-
 arch/arm/boot/dts/am335x-evm.dts                   |   3 +-
 arch/arm/boot/dts/omap3430-sdp.dts                 |   2 +-
 arch/arm/mach-omap2/board-flash.c                  |   2 +-
 arch/arm/mach-omap2/gpmc.c                         |  47 +-
 drivers/mtd/nand/Kconfig                           |  30 +-
 drivers/mtd/nand/omap2.c                           | 623 +++++++++------------
 include/linux/platform_data/mtd-nand-omap2.h       |  18 +-
 8 files changed, 339 insertions(+), 402 deletions(-)

-- 
1.8.1

[PATCH v9 1/9] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes

[Pekon Gupta]

OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
ecc-scheme, like:
- OMAP_ECC_HAMMING_CODE_DEFAULT
	1-bit hamming ecc code using software library
- OMAP_ECC_HAMMING_CODE_HW
	1-bit hamming ecc-code using GPMC h/w engine
- OMAP_ECC_HAMMING_CODE_HW_ROMCODE
	1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible
	to ROM code.

This patch combines above multiple ecc-schemes into single implementation:
- OMAP_ECC_HAM1_CODE_HW
	1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible
	ecc-layout.

Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 ++++----
 arch/arm/mach-omap2/board-flash.c                   | 2 +-
 arch/arm/mach-omap2/gpmc.c                          | 4 +---
 drivers/mtd/nand/omap2.c                            | 9 +++------
 include/linux/platform_data/mtd-nand-omap2.h        | 6 +-----
 5 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index df338cb..25ee232 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -22,10 +22,10 @@ Optional properties:
 				width of 8 is assumed.
 
  - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
-
-		"sw"		Software method (default)
-		"hw"		Hardware method
-		"hw-romcode"	gpmc hamming mode method & romcode layout
+		"sw"		<deprecated> use "ham1" instead
+		"hw"		<deprecated> use "ham1" instead
+		"hw-romcode"	<deprecated> use "ham1" instead
+		"ham1"		1-bit Hamming ecc code
 		"bch4"		4-bit BCH ecc code
 		"bch8"		8-bit BCH ecc code
 
diff --git a/arch/arm/mach-omap2/board-flash.c b/arch/arm/mach-omap2/board-flash.c
index fc20a61..ac82512 100644
--- a/arch/arm/mach-omap2/board-flash.c
+++ b/arch/arm/mach-omap2/board-flash.c
@@ -142,7 +142,7 @@ __init board_nand_init(struct mtd_partition *nand_parts, u8 nr_parts, u8 cs,
 	board_nand_data.nr_parts	= nr_parts;
 	board_nand_data.devsize		= nand_type;
 
-	board_nand_data.ecc_opt = OMAP_ECC_HAMMING_CODE_DEFAULT;
+	board_nand_data.ecc_opt = OMAP_ECC_BCH8_CODE_HW;
 	gpmc_nand_init(&board_nand_data, gpmc_t);
 }
 #endif /* CONFIG_MTD_NAND_OMAP2 || CONFIG_MTD_NAND_OMAP2_MODULE */
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 579697a..c9fb353 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1342,9 +1342,7 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
 #ifdef CONFIG_MTD_NAND
 
 static const char * const nand_ecc_opts[] = {
-	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "sw",
-	[OMAP_ECC_HAMMING_CODE_HW]		= "hw",
-	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hw-romcode",
+	[OMAP_ECC_HAM1_CODE_HW]			= "ham1",
 	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
 	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
 };
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ecf0e5..8d521aa 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1993,10 +1993,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* select the ecc type */
-	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_DEFAULT)
-		info->nand.ecc.mode = NAND_ECC_SOFT;
-	else if ((pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW) ||
-		(pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE)) {
+	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
 		info->nand.ecc.bytes            = 3;
 		info->nand.ecc.size             = 512;
 		info->nand.ecc.strength         = 1;
@@ -2025,7 +2022,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* rom code layout */
-	if (pdata->ecc_opt == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) {
+	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
 
 		if (info->nand.options & NAND_BUSWIDTH_16)
 			offset = 2;
@@ -2033,7 +2030,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 			offset = 1;
 			info->nand.badblock_pattern = &bb_descrip_flashbased;
 		}
-		omap_oobinfo.eccbytes = 3 * (info->mtd.oobsize/16);
+		omap_oobinfo.eccbytes = 3 * (info->mtd.writesize / 512);
 		for (i = 0; i < omap_oobinfo.eccbytes; i++)
 			omap_oobinfo.eccpos[i] = i+offset;
 
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 6bf9ef4..cb5a54a 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,11 +23,7 @@ enum nand_io {
 };
 
 enum omap_ecc {
-		/* 1-bit ecc: stored at end of spare area */
-	OMAP_ECC_HAMMING_CODE_DEFAULT = 0, /* Default, s/w method */
-	OMAP_ECC_HAMMING_CODE_HW, /* gpmc to detect the error */
-		/* 1-bit ecc: stored at beginning of spare area as romcode */
-	OMAP_ECC_HAMMING_CODE_HW_ROMCODE, /* gpmc method & romcode layout */
+	OMAP_ECC_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
 	OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
 	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
 };
-- 
1.8.1

Re: [PATCH v9 1/9] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes

[Mark Rutland]

Hi Pekon,

On Tue, Oct 15, 2013 at 06:49:49AM +0100, Pekon Gupta wrote:
> OMAP NAND driver currently supports multiple flavours of 1-bit Hamming
> ecc-scheme, like:
> - OMAP_ECC_HAMMING_CODE_DEFAULT
> 	1-bit hamming ecc code using software library
> - OMAP_ECC_HAMMING_CODE_HW
> 	1-bit hamming ecc-code using GPMC h/w engine
> - OMAP_ECC_HAMMING_CODE_HW_ROMCODE
> 	1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible
> 	to ROM code.
> 
> This patch combines above multiple ecc-schemes into single implementation:
> - OMAP_ECC_HAM1_CODE_HW
> 	1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible
> 	ecc-layout.
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> Reviewed-by: Felipe Balbi <balbi@ti.com>
> Acked-by: Tony Lindgren <tony@atomide.com>
> ---
>  Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 ++++----
>  arch/arm/mach-omap2/board-flash.c                   | 2 +-
>  arch/arm/mach-omap2/gpmc.c                          | 4 +---
>  drivers/mtd/nand/omap2.c                            | 9 +++------
>  include/linux/platform_data/mtd-nand-omap2.h        | 6 +-----
>  5 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index df338cb..25ee232 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -22,10 +22,10 @@ Optional properties:
>  				width of 8 is assumed.
>  
>   - ti,nand-ecc-opt:		A string setting the ECC layout to use. One of:
> -
> -		"sw"		Software method (default)
> -		"hw"		Hardware method
> -		"hw-romcode"	gpmc hamming mode method & romcode layout
> +		"sw"		<deprecated> use "ham1" instead
> +		"hw"		<deprecated> use "ham1" instead
> +		"hw-romcode"	<deprecated> use "ham1" instead
> +		"ham1"		1-bit Hamming ecc code
>  		"bch4"		4-bit BCH ecc code
>  		"bch8"		8-bit BCH ecc code

[...]

> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 579697a..c9fb353 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1342,9 +1342,7 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
>  #ifdef CONFIG_MTD_NAND
>  
>  static const char * const nand_ecc_opts[] = {
> -	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "sw",
> -	[OMAP_ECC_HAMMING_CODE_HW]		= "hw",
> -	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hw-romcode",
> +	[OMAP_ECC_HAM1_CODE_HW]			= "ham1",
>  	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
>  	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
>  };

As the parsing isn't updated until the next patch, doesn't this
temporarily break DTBs with the deprecated ti,nand-ecc-opt values?

Thanks,
Mark.

RE: [PATCH v9 1/9] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes

[Gupta, Pekon]

Hi Mark,

> From: Mark Rutland
> [...]
> 
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-
> omap2/gpmc.c
> > index 579697a..c9fb353 100644
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -1342,9 +1342,7 @@ static void __maybe_unused
> gpmc_read_timings_dt(struct device_node *np,
> >  #ifdef CONFIG_MTD_NAND
> >
> >  static const char * const nand_ecc_opts[] = {
> > -	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "sw",
> > -	[OMAP_ECC_HAMMING_CODE_HW]		= "hw",
> > -	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hw-
> romcode",
> > +	[OMAP_ECC_HAM1_CODE_HW]			= "ham1",
> >  	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
> >  	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
> >  };
> 
> As the parsing isn't updated until the next patch, doesn't this
> temporarily break DTBs with the deprecated ti,nand-ecc-opt values?
> 
Ok then I'll swap the [Patch 1/9] and [Patch 2/9] so that DT parsing updates
(with backward compatibility) happen before the deprecation of DT values.
This way old DT binding would work all through.

And as DT parsing is kept backward compatible, so .dts changes would not
break the DTBs even if updated in separate patch.

with regards, pekon

[PATCH v9 2/9] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Pekon Gupta]

OMAP NAND driver support multiple ECC scheme, which can used in different
flavours, depending on in-build Hardware engines present on SoC.

This patch updates following in DT bindings related to sectionion of ecc-schemes
- ti,elm-id: replaces elm_id (maintains backward compatibility)
- ti,nand-ecc-opts: selection of h/w or s/w implementation of an ecc-scheme
	depends on ti,elm-id. (supported values ham1, bch4, and bch8)
- maintain backward compatibility to deprecated DT bindings (sw, hw, hw-romcode)

Below table shows different flavours of ecc-schemes supported by OMAP devices
+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAM1_CODE_HW                  |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
|(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
|(requires CONFIG_MTD_NAND_OMAP_BCH &&  |               |               |
| ti,elm-id in DT)                      |               |               |
+---------------------------------------+---------------+---------------+

To optimize footprint of omap2-nand driver, selection of some ECC schemes
also require enabling following Kconfigs, in addition to setting appropriate
DT bindings
- Kconfig:CONFIG_MTD_NAND_ECC_BCH        error detection done in software
- Kconfig:CONFIG_MTD_NAND_OMAP_BCH       error detection done by h/w engine

Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  8 +++-
 arch/arm/mach-omap2/gpmc.c                         | 45 ++++++++++++++++------
 include/linux/platform_data/mtd-nand-omap2.h       | 14 +++++--
 3 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index 25ee232..5e1f31b 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -36,8 +36,12 @@ Optional properties:
 		"prefetch-dma"		Prefetch enabled sDMA mode
 		"prefetch-irq"		Prefetch enabled irq mode
 
- - elm_id:	Specifies elm device node. This is required to support BCH
- 		error correction using ELM module.
+ - elm_id:	<deprecated> use "ti,elm-id" instead
+ - ti,elm-id:	Specifies phandle of the ELM devicetree node.
+		ELM is an on-chip hardware engine on TI SoC which is used for
+		locating ECC errors for BCHx algorithms. SoC devices which have
+		ELM hardware engines should specify this device node in .dtsi
+		Using ELM for ECC error correction frees some CPU cycles.
 
 For inline partiton table parsing (optional):
 
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index c9fb353..9473c9f 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1341,12 +1341,6 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
 
 #ifdef CONFIG_MTD_NAND
 
-static const char * const nand_ecc_opts[] = {
-	[OMAP_ECC_HAM1_CODE_HW]			= "ham1",
-	[OMAP_ECC_BCH4_CODE_HW]			= "bch4",
-	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
-};
-
 static const char * const nand_xfer_types[] = {
 	[NAND_OMAP_PREFETCH_POLLED]		= "prefetch-polled",
 	[NAND_OMAP_POLLED]			= "polled",
@@ -1376,12 +1370,39 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
 	gpmc_nand_data->cs = val;
 	gpmc_nand_data->of_node = child;
 
-	if (!of_property_read_string(child, "ti,nand-ecc-opt", &s))
-		for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++)
-			if (!strcasecmp(s, nand_ecc_opts[val])) {
-				gpmc_nand_data->ecc_opt = val;
-				break;
-			}
+	/* Detect availability of ELM module */
+	gpmc_nand_data->elm_of_node = of_parse_phandle(child, "ti,elm-id", 0);
+	if (gpmc_nand_data->elm_of_node == NULL)
+		gpmc_nand_data->elm_of_node =
+					of_parse_phandle(child, "elm_id", 0);
+	if (gpmc_nand_data->elm_of_node == NULL)
+		pr_warn("%s: ti,elm-id property not found\n", __func__);
+
+	/* select NAND ecc-opt */
+	if (of_property_read_string(child, "ti,nand-ecc-opt", &s)) {
+		pr_err("%s: ti,nand-ecc-opt not found\n", __func__);
+		return -ENODEV;
+	}
+	if (!strcmp(s, "ham1") || !strcmp(s, "sw") ||
+		!strcmp(s, "hw") || !strcmp(s, "hw-romcode"))
+		gpmc_nand_data->ecc_opt =
+				OMAP_ECC_HAM1_CODE_HW;
+	else if (!strcmp(s, "bch4"))
+		if (gpmc_nand_data->elm_of_node)
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH4_CODE_HW;
+		else
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH4_CODE_HW_DETECTION_SW;
+	else if (!strcmp(s, "bch8"))
+		if (gpmc_nand_data->elm_of_node)
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH8_CODE_HW;
+		else
+			gpmc_nand_data->ecc_opt =
+				OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
+	else
+		pr_err("%s: ti,nand-ecc-opt invalid value\n", __func__);
 
 	if (!of_property_read_string(child, "ti,nand-xfer-type", &s))
 		for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++)
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index cb5a54a..4da5bfa 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,9 +23,16 @@ enum nand_io {
 };
 
 enum omap_ecc {
-	OMAP_ECC_HAM1_CODE_HW = 0, /* 1-bit Hamming ecc code */
-	OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
-	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_HAM1_CODE_HW = 0,
+	/* 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,
 };
 
 struct gpmc_nand_regs {
@@ -59,5 +66,6 @@ struct omap_nand_platform_data {
 
 	/* for passing the partitions */
 	struct device_node	*of_node;
+	struct device_node	*elm_of_node;
 };
 #endif
-- 
1.8.1

[PATCH v9 3/9] mtd: nand: omap: cleanup: replace local references with generic framework names

[Pekon Gupta]

This patch updates following in omap_nand_probe() and omap_nand_remove()
- replaces "info->nand" with "nand_chip" (struct nand_chip *nand_chip)
- replaces "info->mtd" with "mtd" (struct mtd_info *mtd)
- white-space and formatting cleanup

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 112 ++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 55 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 8d521aa..5596368 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1824,10 +1824,12 @@ static int omap_nand_probe(struct platform_device *pdev)
 {
 	struct omap_nand_info		*info;
 	struct omap_nand_platform_data	*pdata;
+	struct mtd_info			*mtd;
+	struct nand_chip		*nand_chip;
 	int				err;
 	int				i, offset;
-	dma_cap_mask_t mask;
-	unsigned sig;
+	dma_cap_mask_t			mask;
+	unsigned			sig;
 	struct resource			*res;
 	struct mtd_part_parser_data	ppdata = {};
 
@@ -1846,17 +1848,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 	spin_lock_init(&info->controller.lock);
 	init_waitqueue_head(&info->controller.wq);
 
-	info->pdev = pdev;
-
+	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
-
-	info->mtd.priv		= &info->nand;
-	info->mtd.name		= dev_name(&pdev->dev);
-	info->mtd.owner		= THIS_MODULE;
-
-	info->nand.options	= pdata->devsize;
-	info->nand.options	|= NAND_SKIP_BBTSCAN;
+	mtd			= &info->mtd;
+	mtd->priv		= &info->nand;
+	mtd->name		= dev_name(&pdev->dev);
+	mtd->owner		= THIS_MODULE;
+	nand_chip		= &info->nand;
+	nand_chip->options	= pdata->devsize;
+	nand_chip->options	|= NAND_SKIP_BBTSCAN;
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 	info->of_node		= pdata->of_node;
 #endif
@@ -1877,16 +1878,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_free_info;
 	}
 
-	info->nand.IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
-	if (!info->nand.IO_ADDR_R) {
+	nand_chip->IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
+	if (!nand_chip->IO_ADDR_R) {
 		err = -ENOMEM;
 		goto out_release_mem_region;
 	}
 
-	info->nand.controller = &info->controller;
+	nand_chip->controller = &info->controller;
 
-	info->nand.IO_ADDR_W = info->nand.IO_ADDR_R;
-	info->nand.cmd_ctrl  = omap_hwcontrol;
+	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
+	nand_chip->cmd_ctrl  = omap_hwcontrol;
 
 	/*
 	 * If RDY/BSY line is connected to OMAP then use the omap ready
@@ -1896,26 +1897,26 @@ static int omap_nand_probe(struct platform_device *pdev)
 	 * device and read status register until you get a failure or success
 	 */
 	if (pdata->dev_ready) {
-		info->nand.dev_ready = omap_dev_ready;
-		info->nand.chip_delay = 0;
+		nand_chip->dev_ready = omap_dev_ready;
+		nand_chip->chip_delay = 0;
 	} else {
-		info->nand.waitfunc = omap_wait;
-		info->nand.chip_delay = 50;
+		nand_chip->waitfunc = omap_wait;
+		nand_chip->chip_delay = 50;
 	}
 
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
-		info->nand.read_buf   = omap_read_buf_pref;
-		info->nand.write_buf  = omap_write_buf_pref;
+		nand_chip->read_buf   = omap_read_buf_pref;
+		nand_chip->write_buf  = omap_write_buf_pref;
 		break;
 
 	case NAND_OMAP_POLLED:
-		if (info->nand.options & NAND_BUSWIDTH_16) {
-			info->nand.read_buf   = omap_read_buf16;
-			info->nand.write_buf  = omap_write_buf16;
+		if (nand_chip->options & NAND_BUSWIDTH_16) {
+			nand_chip->read_buf   = omap_read_buf16;
+			nand_chip->write_buf  = omap_write_buf16;
 		} else {
-			info->nand.read_buf   = omap_read_buf8;
-			info->nand.write_buf  = omap_write_buf8;
+			nand_chip->read_buf   = omap_read_buf8;
+			nand_chip->write_buf  = omap_write_buf8;
 		}
 		break;
 
@@ -1944,8 +1945,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 					err);
 				goto out_release_mem_region;
 			}
-			info->nand.read_buf   = omap_read_buf_dma_pref;
-			info->nand.write_buf  = omap_write_buf_dma_pref;
+			nand_chip->read_buf   = omap_read_buf_dma_pref;
+			nand_chip->write_buf  = omap_write_buf_dma_pref;
 		}
 		break;
 
@@ -1980,8 +1981,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 			goto out_release_mem_region;
 		}
 
-		info->nand.read_buf  = omap_read_buf_irq_pref;
-		info->nand.write_buf = omap_write_buf_irq_pref;
+		nand_chip->read_buf  = omap_read_buf_irq_pref;
+		nand_chip->write_buf = omap_write_buf_irq_pref;
 
 		break;
 
@@ -1994,16 +1995,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	/* select the ecc type */
 	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
-		info->nand.ecc.bytes            = 3;
-		info->nand.ecc.size             = 512;
-		info->nand.ecc.strength         = 1;
-		info->nand.ecc.calculate        = omap_calculate_ecc;
-		info->nand.ecc.hwctl            = omap_enable_hwecc;
-		info->nand.ecc.correct          = omap_correct_data;
-		info->nand.ecc.mode             = NAND_ECC_HW;
+		nand_chip->ecc.bytes            = 3;
+		nand_chip->ecc.size             = 512;
+		nand_chip->ecc.strength         = 1;
+		nand_chip->ecc.calculate        = omap_calculate_ecc;
+		nand_chip->ecc.hwctl            = omap_enable_hwecc;
+		nand_chip->ecc.correct          = omap_correct_data;
+		nand_chip->ecc.mode             = NAND_ECC_HW;
 	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
 		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
-		err = omap3_init_bch(&info->mtd, pdata->ecc_opt);
+		err = omap3_init_bch(mtd, pdata->ecc_opt);
 		if (err) {
 			err = -EINVAL;
 			goto out_release_mem_region;
@@ -2013,9 +2014,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 	/* DIP switches on some boards change between 8 and 16 bit
 	 * bus widths for flash.  Try the other width if the first try fails.
 	 */
-	if (nand_scan_ident(&info->mtd, 1, NULL)) {
-		info->nand.options ^= NAND_BUSWIDTH_16;
-		if (nand_scan_ident(&info->mtd, 1, NULL)) {
+	if (nand_scan_ident(mtd, 1, NULL)) {
+		nand_chip->options ^= NAND_BUSWIDTH_16;
+		if (nand_scan_ident(mtd, 1, NULL)) {
 			err = -ENXIO;
 			goto out_release_mem_region;
 		}
@@ -2024,25 +2025,25 @@ static int omap_nand_probe(struct platform_device *pdev)
 	/* rom code layout */
 	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
 
-		if (info->nand.options & NAND_BUSWIDTH_16)
+		if (nand_chip->options & NAND_BUSWIDTH_16) {
 			offset = 2;
-		else {
+		} else {
 			offset = 1;
-			info->nand.badblock_pattern = &bb_descrip_flashbased;
+			nand_chip->badblock_pattern = &bb_descrip_flashbased;
 		}
-		omap_oobinfo.eccbytes = 3 * (info->mtd.writesize / 512);
+		omap_oobinfo.eccbytes = 3 * (mtd->writesize / 512);
 		for (i = 0; i < omap_oobinfo.eccbytes; i++)
 			omap_oobinfo.eccpos[i] = i+offset;
 
 		omap_oobinfo.oobfree->offset = offset + omap_oobinfo.eccbytes;
-		omap_oobinfo.oobfree->length = info->mtd.oobsize -
+		omap_oobinfo.oobfree->length = mtd->oobsize -
 					(offset + omap_oobinfo.eccbytes);
 
-		info->nand.ecc.layout = &omap_oobinfo;
+		nand_chip->ecc.layout = &omap_oobinfo;
 	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
 		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
 		/* build OOB layout for BCH ECC correction */
-		err = omap3_init_bch_tail(&info->mtd);
+		err = omap3_init_bch_tail(mtd);
 		if (err) {
 			err = -EINVAL;
 			goto out_release_mem_region;
@@ -2050,16 +2051,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* second phase scan */
-	if (nand_scan_tail(&info->mtd)) {
+	if (nand_scan_tail(mtd)) {
 		err = -ENXIO;
 		goto out_release_mem_region;
 	}
 
 	ppdata.of_node = pdata->of_node;
-	mtd_device_parse_register(&info->mtd, NULL, &ppdata, pdata->parts,
+	mtd_device_parse_register(mtd, NULL, &ppdata, pdata->parts,
 				  pdata->nr_parts);
 
-	platform_set_drvdata(pdev, &info->mtd);
+	platform_set_drvdata(pdev, mtd);
 
 	return 0;
 
@@ -2080,9 +2081,10 @@ out_free_info:
 static int omap_nand_remove(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
+	struct nand_chip *nand_chip = mtd->priv;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
-	omap3_free_bch(&info->mtd);
+	omap3_free_bch(mtd);
 
 	if (info->dma)
 		dma_release_channel(info->dma);
@@ -2093,8 +2095,8 @@ static int omap_nand_remove(struct platform_device *pdev)
 		free_irq(info->gpmc_irq_fifo, info);
 
 	/* Release NAND device, its internal structures and partitions */
-	nand_release(&info->mtd);
-	iounmap(info->nand.IO_ADDR_R);
+	nand_release(mtd);
+	iounmap(nand_chip->IO_ADDR_R);
 	release_mem_region(info->phys_base, info->mem_size);
 	kfree(info);
 	return 0;
-- 
1.8.1

Re: [PATCH v9 3/9] mtd: nand: omap: cleanup: replace local references with generic framework names

[Brian Norris]

Hi Pekon,

On Tue, Oct 15, 2013 at 11:19:51AM +0530, Pekon Gupta wrote:
> This patch updates following in omap_nand_probe() and omap_nand_remove()
> - replaces "info->nand" with "nand_chip" (struct nand_chip *nand_chip)
> - replaces "info->mtd" with "mtd" (struct mtd_info *mtd)
> - white-space and formatting cleanup
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>

This patch looks good. Thanks for being consistent.

> ---
>  drivers/mtd/nand/omap2.c | 112 ++++++++++++++++++++++++-----------------------
>  1 file changed, 57 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 8d521aa..5596368 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c

...

> @@ -1846,17 +1848,16 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	spin_lock_init(&info->controller.lock);
>  	init_waitqueue_head(&info->controller.wq);
>  
> -	info->pdev = pdev;
> -
> +	info->pdev		= pdev;
>  	info->gpmc_cs		= pdata->cs;
>  	info->reg		= pdata->reg;
> -
> -	info->mtd.priv		= &info->nand;
> -	info->mtd.name		= dev_name(&pdev->dev);
> -	info->mtd.owner		= THIS_MODULE;
> -
> -	info->nand.options	= pdata->devsize;
> -	info->nand.options	|= NAND_SKIP_BBTSCAN;
> +	mtd			= &info->mtd;
> +	mtd->priv		= &info->nand;
> +	mtd->name		= dev_name(&pdev->dev);
> +	mtd->owner		= THIS_MODULE;
> +	nand_chip		= &info->nand;
> +	nand_chip->options	= pdata->devsize;

Trivial side comment: the 'devsize' field is not named very
informatively. It is apparently just a way to specify nand_chip.options,
and it is only actually used for setting a buswidth. (It is also badly
named nand_type in other places.) Not sure if it's worth improving, or
even dropping at some later point in time.

> +	nand_chip->options	|= NAND_SKIP_BBTSCAN;
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH
>  	info->of_node		= pdata->of_node;
>  #endif

[...]

Brian

RE: [PATCH v9 3/9] mtd: nand: omap: cleanup: replace local references with generic framework names

[Gupta, Pekon]

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Tue, Oct 15, 2013 at 11:19:51AM +0530, Pekon Gupta wrote:
> > This patch updates following in omap_nand_probe() and
> omap_nand_remove()
> > - replaces "info->nand" with "nand_chip" (struct nand_chip *nand_chip)
> > - replaces "info->mtd" with "mtd" (struct mtd_info *mtd)
> > - white-space and formatting cleanup
> >
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
> 
> This patch looks good. Thanks for being consistent.
> 

> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 8d521aa..5596368 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> 
> ...
> 
> > @@ -1846,17 +1848,16 @@ static int omap_nand_probe(struct
> > -
> > -	info->nand.options	= pdata->devsize;
> > -	info->nand.options	|= NAND_SKIP_BBTSCAN;
> > +	mtd			= &info->mtd;
> > +	mtd->priv		= &info->nand;
> > +	mtd->name		= dev_name(&pdev->dev);
> > +	mtd->owner		= THIS_MODULE;
> > +	nand_chip		= &info->nand;
> > +	nand_chip->options	= pdata->devsize;
> 
> Trivial side comment: the 'devsize' field is not named very
> informatively. It is apparently just a way to specify nand_chip.options,
> and it is only actually used for setting a buswidth. (It is also badly
> named nand_type in other places.) Not sure if it's worth improving, or
> even dropping at some later point in time.
> 
For now I'm keeping this name consistent for now, as this would require
touching GPMC driver also, where 'devsize' is used to configure
GPMC controller registers.

I'll keep this feedback as pending for independent patch where I probe
driver with "NAND_BUSWIDTH_AUTO".


with regards, pekon

[PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Pekon Gupta]

Autodetection of NAND device bus-width was added in generic NAND driver as
part of following commit
	commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
	Author:     Matthieu CASTET <matthieu.castet@parrot.com>
	AuthorDate: 2012-11-06
	mtd: nand: add NAND_BUSWIDTH_AUTO to autodetect bus width
This patch enables this feature for OMAP2 NAND driver

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5596368..57a3f51 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1856,8 +1856,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	mtd->name		= dev_name(&pdev->dev);
 	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
-	nand_chip->options	= pdata->devsize;
-	nand_chip->options	|= NAND_SKIP_BBTSCAN;
+	nand_chip->options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 	info->of_node		= pdata->of_node;
 #endif
@@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->chip_delay = 50;
 	}
 
+	/* scan for NAND device connected to chip controller */
+	if (nand_scan_ident(mtd, 1, NULL)) {
+		err = -ENXIO;
+		goto out_release_mem_region;
+	}
+	if ((nand_chip->options & NAND_BUSWIDTH_16) !=
+			(pdata->devsize & NAND_BUSWIDTH_16)) {
+		pr_err("%s: detected %s device but driver configured for %s\n",
+			DRIVER_NAME,
+			(nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8",
+			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8");
+		err = -EINVAL;
+		goto out_release_mem_region;
+	}
+
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
 		nand_chip->read_buf   = omap_read_buf_pref;
@@ -2011,17 +2025,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* DIP switches on some boards change between 8 and 16 bit
-	 * bus widths for flash.  Try the other width if the first try fails.
-	 */
-	if (nand_scan_ident(mtd, 1, NULL)) {
-		nand_chip->options ^= NAND_BUSWIDTH_16;
-		if (nand_scan_ident(mtd, 1, NULL)) {
-			err = -ENXIO;
-			goto out_release_mem_region;
-		}
-	}
-
 	/* rom code layout */
 	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
 
-- 
1.8.1

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Brian Norris]

On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
> Autodetection of NAND device bus-width was added in generic NAND driver as
> part of following commit
> 	commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
> 	Author:     Matthieu CASTET <matthieu.castet@parrot.com>
> 	AuthorDate: 2012-11-06
> 	mtd: nand: add NAND_BUSWIDTH_AUTO to autodetect bus width
> This patch enables this feature for OMAP2 NAND driver
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/omap2.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5596368..57a3f51 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c

[...]

> @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		nand_chip->chip_delay = 50;
>  	}
>  
> +	/* scan for NAND device connected to chip controller */
> +	if (nand_scan_ident(mtd, 1, NULL)) {
> +		err = -ENXIO;
> +		goto out_release_mem_region;
> +	}
> +	if ((nand_chip->options & NAND_BUSWIDTH_16) !=
> +			(pdata->devsize & NAND_BUSWIDTH_16)) {
> +		pr_err("%s: detected %s device but driver configured for %s\n",
> +			DRIVER_NAME,
> +			(nand_chip->options & NAND_BUSWIDTH_16) ? "x16" : "x8",
> +			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : "x8");

I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO,
do you even want to try to configure the buswidth by platform data?
You're not really getting the full use out of NAND_BUSWIDTH_AUTO because
the platform data has to guess the same buswidth that nand_base.c
determines. This is worse than the previous behavior, in which you would
try both buswidths if the specified type failed.

IOW, what are you trying to achieve with auto-buswidth? Automatic
determination of the buswidth or just automatic validation? What do you
achieve with platform data's selection of buswidth? Specification of the
buswidth or just a hint? Do the goals conflict? Perhaps you can just
warn, and not error out if the two selections don't match? Or maybe you
really wanted to replace the platform data selection mechanism entirely
with auto-buswidth?

> +		err = -EINVAL;
> +		goto out_release_mem_region;
> +	}
> +
>  	switch (pdata->xfer_type) {
>  	case NAND_OMAP_PREFETCH_POLLED:
>  		nand_chip->read_buf   = omap_read_buf_pref;

[...]

Brian

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Gupta, Pekon]

Hi Brian,

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
> > Autodetection of NAND device bus-width was added in generic NAND
> driver as
[...]
> > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct
> platform_device *pdev)
> >  		nand_chip->chip_delay = 50;
> >  	}
> >
> > +	/* scan for NAND device connected to chip controller */
> > +	if (nand_scan_ident(mtd, 1, NULL)) {
> > +		err = -ENXIO;
> > +		goto out_release_mem_region;
> > +	}
> > +	if ((nand_chip->options & NAND_BUSWIDTH_16) !=
> > +			(pdata->devsize & NAND_BUSWIDTH_16)) {
> > +		pr_err("%s: detected %s device but driver configured for
> %s\n",
> > +			DRIVER_NAME,
> > +			(nand_chip->options & NAND_BUSWIDTH_16) ?
> "x16" : "x8",
> > +			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" :
> "x8");
> 
> I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO,
> do you even want to try to configure the buswidth by platform data?
> You're not really getting the full use out of NAND_BUSWIDTH_AUTO because
> the platform data has to guess the same buswidth that nand_base.c
> determines. This is worse than the previous behavior, in which you would
> try both buswidths if the specified type failed.
> 
> IOW, what are you trying to achieve with auto-buswidth? Automatic
> determination of the buswidth or just automatic validation? What do you
> achieve with platform data's selection of buswidth? Specification of the
> buswidth or just a hint? Do the goals conflict? Perhaps you can just
> warn, and not error out if the two selections don't match? Or maybe you
> really wanted to replace the platform data selection mechanism entirely
> with auto-buswidth?
> 
This is 'automatic validation' of value set in DT binding "nand-bus-width"
So this approach is other way round, where controller is configured
based on DT binding "nand-bus-width" and then it checks whether
device actual buswidth matches DT binding value or not.

- GPMC controller is pre-configured to support "x8" or "x16" device based
   on DT binding "nand-bus-width".
   Reference: $LINUX/arch/arm/mach-omap2/gpmc.c
  @@gpmc_probe_nand_child()
	val = of_get_nand_bus_width(child);

- But, bus-width of NAND device is only known during NAND driver
   Probe in nand_scan_ident().

Going forward when all NAND devices are compliant to ONFI,
then we can deprecate "nand-bus-width".


with regards, pekon

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Brian Norris]

On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote:
> > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
> > > Autodetection of NAND device bus-width was added in generic NAND
> > driver as
> [...]
> > > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct
> > platform_device *pdev)
> > >  		nand_chip->chip_delay = 50;
> > >  	}
> > >
> > > +	/* scan for NAND device connected to chip controller */
> > > +	if (nand_scan_ident(mtd, 1, NULL)) {
> > > +		err = -ENXIO;
> > > +		goto out_release_mem_region;
> > > +	}
> > > +	if ((nand_chip->options & NAND_BUSWIDTH_16) !=
> > > +			(pdata->devsize & NAND_BUSWIDTH_16)) {
> > > +		pr_err("%s: detected %s device but driver configured for
> > %s\n",
> > > +			DRIVER_NAME,
> > > +			(nand_chip->options & NAND_BUSWIDTH_16) ?
> > "x16" : "x8",
> > > +			(pdata->devsize & NAND_BUSWIDTH_16) ? "x16" :
> > "x8");
> > 
> > I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO,
> > do you even want to try to configure the buswidth by platform data?
> > You're not really getting the full use out of NAND_BUSWIDTH_AUTO because
> > the platform data has to guess the same buswidth that nand_base.c
> > determines. This is worse than the previous behavior, in which you would
> > try both buswidths if the specified type failed.
> > 
> > IOW, what are you trying to achieve with auto-buswidth? Automatic
> > determination of the buswidth or just automatic validation? What do you
> > achieve with platform data's selection of buswidth? Specification of the
> > buswidth or just a hint? Do the goals conflict? Perhaps you can just
> > warn, and not error out if the two selections don't match? Or maybe you
> > really wanted to replace the platform data selection mechanism entirely
> > with auto-buswidth?
> > 
> This is 'automatic validation' of value set in DT binding "nand-bus-width"

You mean you intend for the patched driver to simply validate, not
configure the buswidth?

> So this approach is other way round, where controller is configured
> based on DT binding "nand-bus-width" and then it checks whether
> device actual buswidth matches DT binding value or not.

If this is the case, then you really don't want to use
NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the
pre-selected buswidth and exits with an error, in much the same way that
you're doing here (you're just duplicating the functionality).
NAND_BUSWIDTH_AUTO is useful if you want to turn control of the buswidth
configuration entirely over to nand_base.c.

> - GPMC controller is pre-configured to support "x8" or "x16" device based
>    on DT binding "nand-bus-width".
>    Reference: $LINUX/arch/arm/mach-omap2/gpmc.c
>   @@gpmc_probe_nand_child()
> 	val = of_get_nand_bus_width(child);
> 
> - But, bus-width of NAND device is only known during NAND driver
>    Probe in nand_scan_ident().
> 
> Going forward when all NAND devices are compliant to ONFI,
> then we can deprecate "nand-bus-width".

Buswidth auto-detection is not actually restricted to ONFI. nand_base.c
has (corretly, AFAIK) been detecting buswidths for a long time, using
some form of ID decode detection. But it was just that: detection. It
did not actually configure the buswidth, since it left that
responsibility to the low-level driver to get right.

Another thing to consider: I think this current patch is a regression
from previous behavior. Previously, you would run nand_scan_ident()
twice if the first one failed; once with the platform-configured
buswidth and once with the opposite. But now, you only run
nand_scan_ident() once, and then you quit if it detects differently than
you expected.

My opinion: the platform- and device-tree-provided buswidth is
unnecessary; it was previouisly only a suggestion which your driver
would readily discard, and it isn't really needed now. You can probably
get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the
auto-configured buswidth is different than the platform specified.

But the real point: you need to clearly communicate what you are
choosing in this patch. Either you want to

 (1) strictly follow the buswidth provided by the platform or

 (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration.

Trying to mix both (as your patch currently does) just makes everything
worse.

Brian

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Gupta, Pekon]

Hi Brain,

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote:
> > > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
[snip]
> > So this approach is other way round, where controller is configured
> > based on DT binding "nand-bus-width" and then it checks whether
> > device actual buswidth matches DT binding value or not.
> 
> If this is the case, then you really don't want to use
> NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the
> pre-selected buswidth and exits with an error, in much the same way that
> you're doing here (you're just duplicating the functionality).
> NAND_BUSWIDTH_AUTO is useful if you want to turn control of the
> buswidth
> configuration entirely over to nand_base.c.
> 
> > - GPMC controller is pre-configured to support "x8" or "x16" device based
> >    on DT binding "nand-bus-width".
> >    Reference: $LINUX/arch/arm/mach-omap2/gpmc.c
> >   @@gpmc_probe_nand_child()
> > 	val = of_get_nand_bus_width(child);
> >
> > - But, bus-width of NAND device is only known during NAND driver
> >    Probe in nand_scan_ident().
> >
> > Going forward when all NAND devices are compliant to ONFI,
> > then we can deprecate "nand-bus-width".
> 
> Buswidth auto-detection is not actually restricted to ONFI. nand_base.c
> has (corretly, AFAIK) been detecting buswidths for a long time, using
> some form of ID decode detection. But it was just that: detection. It
> did not actually configure the buswidth, since it left that
> responsibility to the low-level driver to get right.
> 
> Another thing to consider: I think this current patch is a regression
> from previous behavior. Previously, you would run nand_scan_ident()
> twice if the first one failed; once with the platform-configured
> buswidth and once with the opposite. But now, you only run
> nand_scan_ident() once, and then you quit if it detects differently than
> you expected.
> 
Actually having nand_scan_ident() called again if first time it fails, is 
_not_ the right way, it was a quick-fix work-around.
It should not have been approved..


> My opinion: the platform- and device-tree-provided buswidth is
> unnecessary; it was previouisly only a suggestion which your driver
> would readily discard, and it isn't really needed now. You can probably
> get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the
> auto-configured buswidth is different than the platform specified.
> 
> But the real point: you need to clearly communicate what you are
> choosing in this patch. Either you want to
> 
>  (1) strictly follow the buswidth provided by the platform or
> 
>  (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration.
> 
Ideally, I would like to go with (2), but that would need other changes
in-order to re-configure GPMC with newly parsed ONFI data, which
can be a separate patch-set.

Thus, I would drop this patch for now. And go with (1), with following
updates in  omap_nand_probe().

(a) perform nand_scan_ident() in "x8" mode, so that ONFI params are
    read and device-info (chip->writesize, chip->oobsize) are populated.

(b) Then switch to "x8" or "x16" mode based on "nand-bus-width"
    as passed from GPMC driver to NAND driver via platform-data.
   And re-populate mtd->byte, mtd-read_buf, mtd->write_buf.


with regards, pekon

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Gupta, Pekon]

> 
> Hi Brain,
> 
Oop sorry .. 
s/Brain/Brian

synonymous though :-)


with regards, pekon

Re: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Brian Norris]

On Thu, Oct 17, 2013 at 09:00:27PM +0000, Pekon Gupta wrote:
> > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote:
> > > > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > > > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote:
> [snip]
> > > So this approach is other way round, where controller is configured
> > > based on DT binding "nand-bus-width" and then it checks whether
> > > device actual buswidth matches DT binding value or not.
> > 
> > If this is the case, then you really don't want to use
> > NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the
> > pre-selected buswidth and exits with an error, in much the same way that
> > you're doing here (you're just duplicating the functionality).
> > NAND_BUSWIDTH_AUTO is useful if you want to turn control of the
> > buswidth
> > configuration entirely over to nand_base.c.
> > 
> > > - GPMC controller is pre-configured to support "x8" or "x16" device based
> > >    on DT binding "nand-bus-width".
> > >    Reference: $LINUX/arch/arm/mach-omap2/gpmc.c
> > >   @@gpmc_probe_nand_child()
> > > 	val = of_get_nand_bus_width(child);
> > >
> > > - But, bus-width of NAND device is only known during NAND driver
> > >    Probe in nand_scan_ident().
> > >
> > > Going forward when all NAND devices are compliant to ONFI,
> > > then we can deprecate "nand-bus-width".
> > 
> > Buswidth auto-detection is not actually restricted to ONFI. nand_base.c
> > has (corretly, AFAIK) been detecting buswidths for a long time, using
> > some form of ID decode detection. But it was just that: detection. It
> > did not actually configure the buswidth, since it left that
> > responsibility to the low-level driver to get right.
> > 
> > Another thing to consider: I think this current patch is a regression
> > from previous behavior. Previously, you would run nand_scan_ident()
> > twice if the first one failed; once with the platform-configured
> > buswidth and once with the opposite. But now, you only run
> > nand_scan_ident() once, and then you quit if it detects differently than
> > you expected.
> > 
> Actually having nand_scan_ident() called again if first time it fails, is 
> _not_ the right way, it was a quick-fix work-around.
> It should not have been approved..

OK, fair enough. I agree.

> > My opinion: the platform- and device-tree-provided buswidth is
> > unnecessary; it was previouisly only a suggestion which your driver
> > would readily discard, and it isn't really needed now. You can probably
> > get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the
> > auto-configured buswidth is different than the platform specified.
> > 
> > But the real point: you need to clearly communicate what you are
> > choosing in this patch. Either you want to
> > 
> >  (1) strictly follow the buswidth provided by the platform or
> > 
> >  (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration.
> > 
> Ideally, I would like to go with (2), but that would need other changes
> in-order to re-configure GPMC with newly parsed ONFI data, which
> can be a separate patch-set.

What exactly needs changed to support this? It looks like this omap2
NAND driver doesn't make assumptions about 8-bit vs. 16-bit until after
nand_scan_ident(). But maybe there is something I missed.

> Thus, I would drop this patch for now. And go with (1),

OK, but to drop this patch entirely would still not be (1); it would
still leave the possibility of calling nand_scan_ident() twice, and it
puts you in a middle ground between (1) and (2). That's fine if it works
for you, but I just want to acknowledge that now: this driver requires
changes to become either (1) or (2).

Does your series need a rebase if we're removing this patch? Or you're
rewriting/simplifying it to the following two points? (Perhaps it's
best to separate this portion to its own patch (set) and discussion,
since it is independent of your ECC rewrite.)

> with following
> updates in  omap_nand_probe().
> 
> (a) perform nand_scan_ident() in "x8" mode, so that ONFI params are
>     read and device-info (chip->writesize, chip->oobsize) are populated.

OK.

> (b) Then switch to "x8" or "x16" mode based on "nand-bus-width"
>     as passed from GPMC driver to NAND driver via platform-data.
>    And re-populate mtd->byte, mtd-read_buf, mtd->write_buf.

I'm not sure if switching buswidth after nand_scan_ident() is really
supported, but I'll hold off until I see the code you're proposing.

Brian

RE: [PATCH v9 4/9] mtd: nand: omap: enable auto-detection of bus-width for omap-nand drivers

[Gupta, Pekon]

Hi Brian,

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> On Thu, Oct 17, 2013 at 09:00:27PM +0000, Pekon Gupta wrote:
> > > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > > > On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote:
 [...]

> > > But the real point: you need to clearly communicate what you are
> > > choosing in this patch. Either you want to
> > >
> > >  (1) strictly follow the buswidth provided by the platform or
> > >
> > >  (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration.
> > >
> > Ideally, I would like to go with (2), but that would need other changes
> > in-order to re-configure GPMC with newly parsed ONFI data, which
> > can be a separate patch-set.
> 
> What exactly needs changed to support this? It looks like this omap2
> NAND driver doesn't make assumptions about 8-bit vs. 16-bit until after
> nand_scan_ident(). But maybe there is something I missed.
> 
The GPMC driver will be touched by NAND_BUSWIDTH_AUTO change.
There are two set of configurations in GPMC controller..
(a) device based configurations:
  These are static configurations derived on DT bindings for each
  chip-select (like NAND signal timings, etc). These done on GPMC probe.
(b) ecc-scheme based configurations:
  These are dynamic configurations done in NAND driver in
  chip->ecc.hwctl() and are refreshed at each NAND accesss.

However, 'nand-bus-width' configuration is part of both (1) and (2).
Thus, both 'OMAP NAND driver' and 'GPMC driver' need to be
consistent in programming  'nand-bus-width' otherwise ecc-engine
would not work.
Thus, I'm proceeding with (1) DT based parsing of 'nand-bus-width'.


> > Thus, I would drop this patch for now. And go with (1),
> 
> OK, but to drop this patch entirely would still not be (1); it would
> still leave the possibility of calling nand_scan_ident() twice, and it
> puts you in a middle ground between (1) and (2). That's fine if it works
> for you, but I just want to acknowledge that now: this driver requires
> changes to become either (1) or (2).
> 
I have re-posted [PATCH v10 4/10] with this updates. However, please
take into account that some limitation of dual programming require
such probe. New patch also call nand_scan_ident() twice, but only
on certain pre-determined conditions, not in all failure.
Once I convert to NAND_BUSWIDTH_AUTO these should get clean,
 as I would update both GPMC and NAND driver for this.
(Till then this was most optimal solution I could think of)..


> Does your series need a rebase if we're removing this patch? Or you're
> rewriting/simplifying it to the following two points? (Perhaps it's
> best to separate this portion to its own patch (set) and discussion,
> since it is independent of your ECC rewrite.)
> 
> > with following
> > updates in  omap_nand_probe().
> >
> > (a) perform nand_scan_ident() in "x8" mode, so that ONFI params are
> >     read and device-info (chip->writesize, chip->oobsize) are populated.
> 
> OK.
> 
> > (b) Then switch to "x8" or "x16" mode based on "nand-bus-width"
> >     as passed from GPMC driver to NAND driver via platform-data.
> >    And re-populate mtd->byte, mtd-read_buf, mtd->write_buf.
> 
> I'm not sure if switching buswidth after nand_scan_ident() is really
> supported, but I'll hold off until I see the code you're proposing.
> 
I have re-posted [PATCH v10 4/10] with this updates.
Please accept this ...


with regards, pekon

[PATCH v9 5/9] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Pekon Gupta]

current implementation in omap3_init_bch() has some redundant code like:
(1) omap3_init_bch() re-probes the DT-binding to detect presence of ELM h/w
    engine on SoC. And based on that it selects implemetation of ecc-scheme.
    However, this is already done as part of GPMC DT parsing.
(2) As omap3_init_bch() serves as common function for configuring all types of
    BCHx ecc-schemes, so there are multiple levels of redudant if..then..else
    checks while populating nand_chip->ecc.

This patch make following changes to OMAP NAND driver:
(1) removes omap3_init_bch(): each ecc-scheme is individually configured in
    omap_nand_probe() there by removing redundant if..then..else checks.
(2) adds is_elm_present(): re-probing of ELM device via DT is not required as
    it's done in GPMC driver probe. Thus is_elm_present() just initializes ELM
    driver with NAND probe data, when ecc-scheme with h/w based error-detection
    is used.
(3) separates out configuration of different flavours of "BCH4" and "BCH8"
    ecc-schemes as given in below table
(4) conditionally compiles callbacks implementations of ecc.hwctl(),
    ecc.calculate(), ecc.correct() to avoid warning of un-used functions.

+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAM1_CODE_HW                  |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
| (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
|                                       |               |               |
|OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
| (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
|        ti,elm-id)                     |               |               |
+---------------------------------------+---------------+---------------+
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
| (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
|                                       |               |               |
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
| (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
|        ti,elm-id)                     |               |               |
+---------------------------------------+---------------+---------------+

- 'CONFIG_MTD_NAND_ECC_BCH' is generic KConfig required to build lib/bch.c
    which is required for ECC error detection done in software.
    (mainly used for legacy platforms which do not have on-chip ELM engine)

- 'CONFIG_MTD_NAND_OMAP_BCH' is OMAP specific Kconfig to detemine presence
    on ELM h/w engine on SoC.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 281 ++++++++++++++++++++++++++---------------------
 1 file changed, 158 insertions(+), 123 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 57a3f51..09ce85c 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -25,10 +25,8 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 #include <linux/bch.h>
 #include <linux/platform_data/elm.h>
-#endif
 
 #include <linux/platform_data/mtd-nand-omap2.h>
 
@@ -141,6 +139,8 @@
 #define BCH_ECC_SIZE0		0x0	/* ecc_size0 = 0, no oob protection */
 #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
 
+#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
+
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
 	0xac, 0x6b, 0xff, 0x99, 0x7b};
@@ -182,14 +182,12 @@ struct omap_nand_info {
 	u_char				*buf;
 	int					buf_len;
 	struct gpmc_nand_regs		reg;
-
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
+	/* fields specific for BCHx_HW ECC scheme */
 	struct bch_control             *bch;
 	struct nand_ecclayout           ecclayout;
 	bool				is_elm_used;
 	struct device			*elm_dev;
 	struct device_node		*of_node;
-#endif
 };
 
 /**
@@ -1058,8 +1056,7 @@ static int omap_dev_ready(struct mtd_info *mtd)
 	}
 }
 
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-
+#if defined(CONFIG_MTD_NAND_ECC_BCH) || defined(CONFIG_MTD_NAND_OMAP_BCH)
 /**
  * omap3_enable_hwecc_bch - Program OMAP3 GPMC to perform BCH ECC correction
  * @mtd: MTD device structure
@@ -1140,7 +1137,9 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
 	/* Clear ecc and enable bits */
 	writel(ECCCLEAR | ECC1, info->reg.gpmc_ecc_control);
 }
+#endif
 
+#ifdef CONFIG_MTD_NAND_ECC_BCH
 /**
  * omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
  * @mtd: MTD device structure
@@ -1225,7 +1224,9 @@ static int omap3_calculate_ecc_bch8(struct mtd_info *mtd, const u_char *dat,
 
 	return 0;
 }
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
 
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
 /**
  * omap3_calculate_ecc_bch - Generate bytes of ECC bytes
  * @mtd:	MTD device structure
@@ -1517,7 +1518,9 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 
 	return stat;
 }
+#endif /* CONFIG_MTD_NAND_OMAP_BCH */
 
+#ifdef CONFIG_MTD_NAND_ECC_BCH
 /**
  * omap3_correct_data_bch - Decode received data and correct errors
  * @mtd: MTD device structure
@@ -1549,7 +1552,9 @@ static int omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
 	}
 	return count;
 }
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
 
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
 /**
  * omap_write_page_bch - BCH ecc based write page function for entire page
  * @mtd:		mtd info structure
@@ -1637,118 +1642,48 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
- * omap3_free_bch - Release BCH ecc resources
- * @mtd: MTD device structure
+ * is_elm_present - checks for presence of ELM module by scanning DT nodes
+ * @omap_nand_info: NAND device structure containing platform data
+ * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16
  */
-static void omap3_free_bch(struct mtd_info *mtd)
+static int is_elm_present(struct omap_nand_info *info,
+			struct device_node *elm_node, enum bch_ecc bch_type)
 {
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	if (info->bch) {
-		free_bch(info->bch);
-		info->bch = NULL;
+	struct platform_device *pdev;
+	info->is_elm_used = false;
+	/* check whether elm-id is passed via DT */
+	if (!elm_node) {
+		pr_err("nand: error: ELM DT node not found\n");
+		return -ENODEV;
+	}
+	pdev = of_find_device_by_node(elm_node);
+	/* check whether ELM device is registered */
+	if (!pdev) {
+		pr_err("nand: error: ELM device not found\n");
+		return -ENODEV;
 	}
+	/* ELM module available, now configure it */
+	info->elm_dev = &pdev->dev;
+	if (elm_config(info->elm_dev, bch_type))
+		return -ENODEV;
+	info->is_elm_used = true;
+	return 0;
 }
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
 
+#ifdef CONFIG_MTD_NAND_ECC_BCH
 /**
- * omap3_init_bch - Initialize BCH ECC
+ * omap3_free_bch - Release BCH ecc resources
  * @mtd: MTD device structure
- * @ecc_opt: OMAP ECC mode (OMAP_ECC_BCH4_CODE_HW or OMAP_ECC_BCH8_CODE_HW)
  */
-static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
+static void omap3_free_bch(struct mtd_info *mtd)
 {
-	int max_errors;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 						   mtd);
-#ifdef CONFIG_MTD_NAND_OMAP_BCH8
-	const int hw_errors = BCH8_MAX_ERROR;
-#else
-	const int hw_errors = BCH4_MAX_ERROR;
-#endif
-	enum bch_ecc bch_type;
-	const __be32 *parp;
-	int lenp;
-	struct device_node *elm_node;
-
-	info->bch = NULL;
-
-	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
-		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
-	if (max_errors != hw_errors) {
-		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
-		       max_errors, hw_errors);
-		goto fail;
-	}
-
-	info->nand.ecc.size = 512;
-	info->nand.ecc.hwctl = omap3_enable_hwecc_bch;
-	info->nand.ecc.mode = NAND_ECC_HW;
-	info->nand.ecc.strength = max_errors;
-
-	if (hw_errors == BCH8_MAX_ERROR)
-		bch_type = BCH8_ECC;
-	else
-		bch_type = BCH4_ECC;
-
-	/* Detect availability of ELM module */
-	parp = of_get_property(info->of_node, "elm_id", &lenp);
-	if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) {
-		pr_err("Missing elm_id property, fall back to Software BCH\n");
-		info->is_elm_used = false;
-	} else {
-		struct platform_device *pdev;
-
-		elm_node = of_find_node_by_phandle(be32_to_cpup(parp));
-		pdev = of_find_device_by_node(elm_node);
-		info->elm_dev = &pdev->dev;
-
-		if (elm_config(info->elm_dev, bch_type) == 0)
-			info->is_elm_used = true;
-	}
-
-	if (info->is_elm_used && (mtd->writesize <= 4096)) {
-
-		if (hw_errors == BCH8_MAX_ERROR)
-			info->nand.ecc.bytes = BCH8_SIZE;
-		else
-			info->nand.ecc.bytes = BCH4_SIZE;
-
-		info->nand.ecc.correct = omap_elm_correct_data;
-		info->nand.ecc.calculate = omap3_calculate_ecc_bch;
-		info->nand.ecc.read_page = omap_read_page_bch;
-		info->nand.ecc.write_page = omap_write_page_bch;
-	} else {
-		/*
-		 * software bch library is only used to detect and
-		 * locate errors
-		 */
-		info->bch = init_bch(13, max_errors,
-				0x201b /* hw polynomial */);
-		if (!info->bch)
-			goto fail;
-
-		info->nand.ecc.correct = omap3_correct_data_bch;
-
-		/*
-		 * The number of corrected errors in an ecc block that will
-		 * trigger block scrubbing defaults to the ecc strength (4 or 8)
-		 * Set mtd->bitflip_threshold here to define a custom threshold.
-		 */
-
-		if (max_errors == 8) {
-			info->nand.ecc.bytes = 13;
-			info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
-		} else {
-			info->nand.ecc.bytes = 7;
-			info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
-		}
+	if (info->bch) {
+		free_bch(info->bch);
+		info->bch = NULL;
 	}
-
-	pr_info("enabling NAND BCH ecc with %d-bit correction\n", max_errors);
-	return 0;
-fail:
-	omap3_free_bch(mtd);
-	return -1;
 }
 
 /**
@@ -1806,11 +1741,6 @@ fail:
 }
 
 #else
-static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
-{
-	pr_err("CONFIG_MTD_NAND_OMAP_BCH is not enabled\n");
-	return -1;
-}
 static int omap3_init_bch_tail(struct mtd_info *mtd)
 {
 	return -1;
@@ -1818,7 +1748,7 @@ static int omap3_init_bch_tail(struct mtd_info *mtd)
 static void omap3_free_bch(struct mtd_info *mtd)
 {
 }
-#endif /* CONFIG_MTD_NAND_OMAP_BCH */
+#endif /* CONFIG_MTD_NAND_ECC_BCH */
 
 static int omap_nand_probe(struct platform_device *pdev)
 {
@@ -1851,15 +1781,14 @@ static int omap_nand_probe(struct platform_device *pdev)
 	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
+	info->bch		= NULL;
+	info->of_node		= pdata->of_node;
 	mtd			= &info->mtd;
 	mtd->priv		= &info->nand;
 	mtd->name		= dev_name(&pdev->dev);
 	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
 	nand_chip->options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
-	info->of_node		= pdata->of_node;
-#endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
@@ -2007,22 +1936,125 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
-	/* select the ecc type */
-	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
+	/* populate MTD interface based on ECC scheme */
+	switch (pdata->ecc_opt) {
+	case OMAP_ECC_HAM1_CODE_HW:
+		pr_info("nand: using OMAP_ECC_HAM1_CODE_HW\n");
+		nand_chip->ecc.mode             = NAND_ECC_HW;
 		nand_chip->ecc.bytes            = 3;
 		nand_chip->ecc.size             = 512;
 		nand_chip->ecc.strength         = 1;
 		nand_chip->ecc.calculate        = omap_calculate_ecc;
 		nand_chip->ecc.hwctl            = omap_enable_hwecc;
 		nand_chip->ecc.correct          = omap_correct_data;
-		nand_chip->ecc.mode             = NAND_ECC_HW;
-	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
-		err = omap3_init_bch(mtd, pdata->ecc_opt);
-		if (err) {
+		break;
+
+	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW_DETECTION_SW\n");
+		nand_chip->ecc.mode		= NAND_ECC_HW;
+		nand_chip->ecc.size		= 512;
+		nand_chip->ecc.bytes		= 7;
+		nand_chip->ecc.strength		= 4;
+		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
+		nand_chip->ecc.correct		= omap3_correct_data_bch;
+		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch4;
+		/* software bch library is used for locating errors */
+		info->bch = init_bch(nand_chip->ecc.bytes,
+					nand_chip->ecc.strength,
+					OMAP_ECC_BCH8_POLYNOMIAL);
+		if (!info->bch) {
+			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
+		}
+		break;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+
+	case OMAP_ECC_BCH4_CODE_HW:
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+		pr_info("nand: using OMAP_ECC_BCH4_CODE_HW ECC scheme\n");
+		nand_chip->ecc.mode		= NAND_ECC_HW;
+		nand_chip->ecc.size		= 512;
+		/* 14th bit is kept reserved for ROM-code compatibility */
+		nand_chip->ecc.bytes		= 7 + 1;
+		nand_chip->ecc.strength		= 4;
+		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
+		nand_chip->ecc.correct		= omap_elm_correct_data;
+		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch;
+		nand_chip->ecc.read_page	= omap_read_page_bch;
+		nand_chip->ecc.write_page	= omap_write_page_bch;
+		/* This ECC scheme requires ELM H/W block */
+		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
+			pr_err("nand: error: could not initialize ELM\n");
+			err = -ENODEV;
 			goto out_release_mem_region;
 		}
+		break;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+
+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
+#ifdef CONFIG_MTD_NAND_ECC_BCH
+		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
+		nand_chip->ecc.mode		= NAND_ECC_HW;
+		nand_chip->ecc.size		= 512;
+		nand_chip->ecc.bytes		= 13;
+		nand_chip->ecc.strength		= 8;
+		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
+		nand_chip->ecc.correct		= omap3_correct_data_bch;
+		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch8;
+		/* software bch library is used for locating errors */
+		info->bch = init_bch(nand_chip->ecc.bytes,
+					nand_chip->ecc.strength,
+					OMAP_ECC_BCH8_POLYNOMIAL);
+		if (!info->bch) {
+			pr_err("nand: error: unable to use s/w BCH library\n");
+			err = -EINVAL;
+			goto out_release_mem_region;
+		}
+		break;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_ECC_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+
+	case OMAP_ECC_BCH8_CODE_HW:
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+		pr_info("nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme\n");
+		nand_chip->ecc.mode		= NAND_ECC_HW;
+		nand_chip->ecc.size		= 512;
+		/* 14th bit is kept reserved for ROM-code compatibility */
+		nand_chip->ecc.bytes		= 13 + 1;
+		nand_chip->ecc.strength		= 8;
+		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
+		nand_chip->ecc.correct		= omap_elm_correct_data;
+		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch;
+		nand_chip->ecc.read_page	= omap_read_page_bch;
+		nand_chip->ecc.write_page	= omap_write_page_bch;
+		/* This ECC scheme requires ELM H/W block */
+		if (is_elm_present(info, pdata->elm_of_node, BCH8_ECC) < 0) {
+			pr_err("nand: error: could not initialize ELM\n");
+			goto out_release_mem_region;
+		}
+		break;
+#else
+		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+#endif
+
+	default:
+		pr_err("nand: error: invalid or unsupported ECC scheme\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
 	}
 
 	/* rom code layout */
@@ -2044,6 +2076,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 		nand_chip->ecc.layout = &omap_oobinfo;
 	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
+		   (pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
+		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW) ||
 		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
 		/* build OOB layout for BCH ECC correction */
 		err = omap3_init_bch_tail(mtd);
@@ -2076,6 +2110,7 @@ out_release_mem_region:
 		free_irq(info->gpmc_irq_fifo, info);
 	release_mem_region(info->phys_base, info->mem_size);
 out_free_info:
+	omap3_free_bch(mtd);
 	kfree(info);
 
 	return err;
-- 
1.8.1

Re: [PATCH v9 5/9] mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC configurations in device_probe

[Brian Norris]

On Tue, Oct 15, 2013 at 11:19:53AM +0530, Pekon Gupta wrote:
> current implementation in omap3_init_bch() has some redundant code like:
> (1) omap3_init_bch() re-probes the DT-binding to detect presence of ELM h/w
>     engine on SoC. And based on that it selects implemetation of ecc-scheme.
>     However, this is already done as part of GPMC DT parsing.
> (2) As omap3_init_bch() serves as common function for configuring all types of
>     BCHx ecc-schemes, so there are multiple levels of redudant if..then..else
>     checks while populating nand_chip->ecc.
> 
> This patch make following changes to OMAP NAND driver:
> (1) removes omap3_init_bch(): each ecc-scheme is individually configured in
>     omap_nand_probe() there by removing redundant if..then..else checks.
> (2) adds is_elm_present(): re-probing of ELM device via DT is not required as
>     it's done in GPMC driver probe. Thus is_elm_present() just initializes ELM
>     driver with NAND probe data, when ecc-scheme with h/w based error-detection
>     is used.
> (3) separates out configuration of different flavours of "BCH4" and "BCH8"
>     ecc-schemes as given in below table
> (4) conditionally compiles callbacks implementations of ecc.hwctl(),
>     ecc.calculate(), ecc.correct() to avoid warning of un-used functions.
> 
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAM1_CODE_HW                  |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH4_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
> | (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
> |                                       |               |               |
> |OMAP_ECC_BCH4_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> | (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
> |        ti,elm-id)                     |               |               |
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W (lib/bch.c)|
> | (needs CONFIG_MTD_NAND_ECC_BCH)       |               |               |
> |                                       |               |               |
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> | (needs CONFIG_MTD_NAND_OMAP_BCH &&    |               |               |
> |        ti,elm-id)                     |               |               |
> +---------------------------------------+---------------+---------------+
> 
> - 'CONFIG_MTD_NAND_ECC_BCH' is generic KConfig required to build lib/bch.c
>     which is required for ECC error detection done in software.
>     (mainly used for legacy platforms which do not have on-chip ELM engine)
> 
> - 'CONFIG_MTD_NAND_OMAP_BCH' is OMAP specific Kconfig to detemine presence
>     on ELM h/w engine on SoC.
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>

This patch looks good to me.

Brian

[PATCH v9 6/9] mtd: nand: omap: clean-up ecc layout for BCH ecc schemes

[Pekon Gupta]

In current implementation omap3_init_bch_tail() is a common function to
define ecc layout for different BCHx ecc schemes.This patch:
(1) removes omap3_init_bch_tail() and defines ecc layout for individual
    ecc-schemes along with populating their nand_chip->ecc data in
    omap_nand_probe(). This improves the readability and scalability of
    code for add new ecc schemes in future.
(2) removes 'struct nand_bbt_descr bb_descrip_flashbased' because default
    nand_bbt_descr in nand_bbt.c matches the same (.len=1 for x8 devices).
(3) add the check to see if NAND device has enough OOB/Spare bytes to
    store ECC signature of whole page, as defined by ecc-scheme.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 162 ++++++++++++++++++-----------------------------
 1 file changed, 63 insertions(+), 99 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 09ce85c..5f6e621 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -139,6 +139,7 @@
 #define BCH_ECC_SIZE0		0x0	/* ecc_size0 = 0, no oob protection */
 #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
 
+#define BADBLOCK_MARKER_LENGTH		2
 #define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
 
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
@@ -149,17 +150,6 @@ static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
 
 /* oob info generated runtime depending on ecc algorithm and layout selected */
 static struct nand_ecclayout omap_oobinfo;
-/* Define some generic bad / good block scan pattern which are used
- * while scanning a device for factory marked good / bad blocks
- */
-static uint8_t scan_ff_pattern[] = { 0xff };
-static struct nand_bbt_descr bb_descrip_flashbased = {
-	.options = NAND_BBT_SCANALLPAGES,
-	.offs = 0,
-	.len = 1,
-	.pattern = scan_ff_pattern,
-};
-
 
 struct omap_nand_info {
 	struct nand_hw_control		controller;
@@ -184,7 +174,6 @@ struct omap_nand_info {
 	struct gpmc_nand_regs		reg;
 	/* fields specific for BCHx_HW ECC scheme */
 	struct bch_control             *bch;
-	struct nand_ecclayout           ecclayout;
 	bool				is_elm_used;
 	struct device			*elm_dev;
 	struct device_node		*of_node;
@@ -1686,65 +1675,8 @@ static void omap3_free_bch(struct mtd_info *mtd)
 	}
 }
 
-/**
- * omap3_init_bch_tail - Build an oob layout for BCH ECC correction.
- * @mtd: MTD device structure
- */
-static int omap3_init_bch_tail(struct mtd_info *mtd)
-{
-	int i, steps, offset;
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	struct nand_ecclayout *layout = &info->ecclayout;
-
-	/* build oob layout */
-	steps = mtd->writesize/info->nand.ecc.size;
-	layout->eccbytes = steps*info->nand.ecc.bytes;
-
-	/* do not bother creating special oob layouts for small page devices */
-	if (mtd->oobsize < 64) {
-		pr_err("BCH ecc is not supported on small page devices\n");
-		goto fail;
-	}
-
-	/* reserve 2 bytes for bad block marker */
-	if (layout->eccbytes+2 > mtd->oobsize) {
-		pr_err("no oob layout available for oobsize %d eccbytes %u\n",
-		       mtd->oobsize, layout->eccbytes);
-		goto fail;
-	}
-
-	/* ECC layout compatible with RBL for BCH8 */
-	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
-		offset = 2;
-	else
-		offset = mtd->oobsize - layout->eccbytes;
-
-	/* put ecc bytes at oob tail */
-	for (i = 0; i < layout->eccbytes; i++)
-		layout->eccpos[i] = offset + i;
-
-	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
-		layout->oobfree[0].offset = 2 + layout->eccbytes * steps;
-	else
-		layout->oobfree[0].offset = 2;
-
-	layout->oobfree[0].length = mtd->oobsize-2-layout->eccbytes;
-	info->nand.ecc.layout = layout;
-
-	if (!(info->nand.options & NAND_BUSWIDTH_16))
-		info->nand.badblock_pattern = &bb_descrip_flashbased;
-	return 0;
-fail:
-	omap3_free_bch(mtd);
-	return -1;
-}
-
 #else
-static int omap3_init_bch_tail(struct mtd_info *mtd)
-{
-	return -1;
-}
+
 static void omap3_free_bch(struct mtd_info *mtd)
 {
 }
@@ -1756,8 +1688,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 	struct omap_nand_platform_data	*pdata;
 	struct mtd_info			*mtd;
 	struct nand_chip		*nand_chip;
+	struct nand_ecclayout		*ecclayout;
 	int				err;
-	int				i, offset;
+	int				i;
 	dma_cap_mask_t			mask;
 	unsigned			sig;
 	struct resource			*res;
@@ -1847,6 +1780,14 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
+	/* check for small page devices */
+	if ((mtd->oobsize < 64) && (pdata->ecc_opt != OMAP_ECC_HAM1_CODE_HW)) {
+		pr_err("small page devices are not supported\n");
+		err = -EINVAL;
+		goto out_release_mem_region;
+	}
+
+	/* populate read & write API based on xfer_type selected */
 	switch (pdata->xfer_type) {
 	case NAND_OMAP_PREFETCH_POLLED:
 		nand_chip->read_buf   = omap_read_buf_pref;
@@ -1937,6 +1878,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* populate MTD interface based on ECC scheme */
+	nand_chip->ecc.layout	= &omap_oobinfo;
+	ecclayout		= &omap_oobinfo;
 	switch (pdata->ecc_opt) {
 	case OMAP_ECC_HAM1_CODE_HW:
 		pr_info("nand: using OMAP_ECC_HAM1_CODE_HW\n");
@@ -1947,6 +1890,16 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.calculate        = omap_calculate_ecc;
 		nand_chip->ecc.hwctl            = omap_enable_hwecc;
 		nand_chip->ecc.correct          = omap_correct_data;
+		/* define ECC layout */
+		ecclayout->eccbytes		= nand_chip->ecc.bytes *
+							(mtd->writesize /
+							nand_chip->ecc.size);
+		if (nand_chip->options & NAND_BUSWIDTH_16)
+			ecclayout->eccpos[0]	= BADBLOCK_MARKER_LENGTH;
+		else
+			ecclayout->eccpos[0]	= 1;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
 		break;
 
 	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
@@ -1959,6 +1912,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
 		nand_chip->ecc.correct		= omap3_correct_data_bch;
 		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch4;
+		/* define ECC layout */
+		ecclayout->eccbytes		= nand_chip->ecc.bytes *
+							(mtd->writesize /
+							nand_chip->ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
 		/* software bch library is used for locating errors */
 		info->bch = init_bch(nand_chip->ecc.bytes,
 					nand_chip->ecc.strength,
@@ -1987,6 +1947,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch;
 		nand_chip->ecc.read_page	= omap_read_page_bch;
 		nand_chip->ecc.write_page	= omap_write_page_bch;
+		/* define ECC layout */
+		ecclayout->eccbytes		= nand_chip->ecc.bytes *
+							(mtd->writesize /
+							nand_chip->ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
 		/* This ECC scheme requires ELM H/W block */
 		if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) {
 			pr_err("nand: error: could not initialize ELM\n");
@@ -2010,6 +1977,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
 		nand_chip->ecc.correct		= omap3_correct_data_bch;
 		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch8;
+		/* define ECC layout */
+		ecclayout->eccbytes		= nand_chip->ecc.bytes *
+							(mtd->writesize /
+							nand_chip->ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
 		/* software bch library is used for locating errors */
 		info->bch = init_bch(nand_chip->ecc.bytes,
 					nand_chip->ecc.strength,
@@ -2044,6 +2018,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 			pr_err("nand: error: could not initialize ELM\n");
 			goto out_release_mem_region;
 		}
+		/* define ECC layout */
+		ecclayout->eccbytes		= nand_chip->ecc.bytes *
+							(mtd->writesize /
+							nand_chip->ecc.size);
+		ecclayout->eccpos[0]		= BADBLOCK_MARKER_LENGTH;
+		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
+							ecclayout->eccbytes;
 		break;
 #else
 		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
@@ -2057,34 +2038,17 @@ static int omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
-	/* rom code layout */
-	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
-
-		if (nand_chip->options & NAND_BUSWIDTH_16) {
-			offset = 2;
-		} else {
-			offset = 1;
-			nand_chip->badblock_pattern = &bb_descrip_flashbased;
-		}
-		omap_oobinfo.eccbytes = 3 * (mtd->writesize / 512);
-		for (i = 0; i < omap_oobinfo.eccbytes; i++)
-			omap_oobinfo.eccpos[i] = i+offset;
-
-		omap_oobinfo.oobfree->offset = offset + omap_oobinfo.eccbytes;
-		omap_oobinfo.oobfree->length = mtd->oobsize -
-					(offset + omap_oobinfo.eccbytes);
-
-		nand_chip->ecc.layout = &omap_oobinfo;
-	} else if ((pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH4_CODE_HW_DETECTION_SW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW_DETECTION_SW) ||
-		   (pdata->ecc_opt == OMAP_ECC_BCH8_CODE_HW)) {
-		/* build OOB layout for BCH ECC correction */
-		err = omap3_init_bch_tail(mtd);
-		if (err) {
-			err = -EINVAL;
-			goto out_release_mem_region;
-		}
+	/* populate remaining ECC layout data */
+	ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH +
+							ecclayout->eccbytes);
+	for (i = 1; i < ecclayout->eccbytes; i++)
+		ecclayout->eccpos[i] = ecclayout->eccpos[0] + i;
+	/* check if NAND device's OOB is enough to store ECC signatures */
+	if (mtd->oobsize < (ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH)) {
+		pr_err("not enough OOB bytes required = %d, available=%d\n",
+					   ecclayout->eccbytes, mtd->oobsize);
+		err = -EINVAL;
+		goto out_release_mem_region;
 	}
 
 	/* second phase scan */
-- 
1.8.1

Re: [PATCH v9 6/9] mtd: nand: omap: clean-up ecc layout for BCH ecc schemes

[Brian Norris]

On Tue, Oct 15, 2013 at 11:19:54AM +0530, Pekon Gupta wrote:
> In current implementation omap3_init_bch_tail() is a common function to
> define ecc layout for different BCHx ecc schemes.This patch:
> (1) removes omap3_init_bch_tail() and defines ecc layout for individual
>     ecc-schemes along with populating their nand_chip->ecc data in
>     omap_nand_probe(). This improves the readability and scalability of
>     code for add new ecc schemes in future.
> (2) removes 'struct nand_bbt_descr bb_descrip_flashbased' because default
>     nand_bbt_descr in nand_bbt.c matches the same (.len=1 for x8 devices).
> (3) add the check to see if NAND device has enough OOB/Spare bytes to
>     store ECC signature of whole page, as defined by ecc-scheme.
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>

This patch also looks OK.

Brian

[PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[Pekon Gupta]

generic frame-work in mtd/nand/nand_bch.c is a wrapper above lib/bch.h which
encapsulates all control information specific to BCH ecc algorithm in software.
Thus this patch:
(1) replace omap specific implementations with equivalent wrapper in nand_bch.c
    so that more generic code is re-used. like;
        omap3_correct_data_bch() -> nand_bch_correct_data()
        omap3_free_bch() -> nand_bch_free()
(2) replace direct calls to lib/bch.c with wrapper functions defined in nand_bch.c
	init_bch() -> nand_bch_init()
(3) removes selection between BCH8 and BCH4 h/w ecc-schemes via KConfig.
    This selection is now based on ti,nand-ecc-opt and ti,elm-id DT bindings.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/Kconfig | 30 ++-------------
 drivers/mtd/nand/omap2.c | 96 +++++++++++-------------------------------------
 2 files changed, 26 insertions(+), 100 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index d885298..5836039 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
 
 config MTD_NAND_OMAP_BCH
 	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
-	tristate "Enable support for hardware BCH error correction"
+	tristate "Support hardware based BCH error correction"
 	default n
 	select BCH
-	select BCH_CONST_PARAMS
 	help
-	 Support for hardware BCH error correction.
-
-choice
-	prompt "BCH error correction capability"
-	depends on MTD_NAND_OMAP_BCH
-
-config MTD_NAND_OMAP_BCH8
-	bool "8 bits / 512 bytes (recommended)"
-	help
-	 Support correcting up to 8 bitflips per 512-byte block.
-	 This will use 13 bytes of spare area per 512 bytes of page data.
-	 This is the recommended mode, as 4-bit mode does not work
-	 on some OMAP3 revisions, due to a hardware bug.
-
-config MTD_NAND_OMAP_BCH4
-	bool "4 bits / 512 bytes"
-	help
-	 Support correcting up to 4 bitflips per 512-byte block.
-	 This will use 7 bytes of spare area per 512 bytes of page data.
-	 Note that this mode does not work on some OMAP3 revisions, due to a
-	 hardware bug. Please check your OMAP datasheet before selecting this
-	 mode.
-
-endchoice
+	  Some devices have built-in ELM hardware engine, which can be used to
+	  locate and correct errors when using BCH ECC scheme. This enables the
+	  driver support for same.
 
 if MTD_NAND_OMAP_BCH
 config BCH_CONST_M
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5f6e621..769ff65 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -25,7 +25,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 
-#include <linux/bch.h>
+#include <linux/mtd/nand_bch.h>
 #include <linux/platform_data/elm.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>
@@ -140,7 +140,6 @@
 #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
 
 #define BADBLOCK_MARKER_LENGTH		2
-#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
 
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
@@ -173,7 +172,6 @@ struct omap_nand_info {
 	int					buf_len;
 	struct gpmc_nand_regs		reg;
 	/* fields specific for BCHx_HW ECC scheme */
-	struct bch_control             *bch;
 	bool				is_elm_used;
 	struct device			*elm_dev;
 	struct device_node		*of_node;
@@ -1507,43 +1505,7 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
 
 	return stat;
 }
-#endif /* CONFIG_MTD_NAND_OMAP_BCH */
 
-#ifdef CONFIG_MTD_NAND_ECC_BCH
-/**
- * omap3_correct_data_bch - 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 omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
-				  u_char *read_ecc, u_char *calc_ecc)
-{
-	int i, count;
-	/* cannot correct more than 8 errors */
-	unsigned int errloc[8];
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-
-	count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
-			   errloc);
-	if (count > 0) {
-		/* correct errors */
-		for (i = 0; i < count; i++) {
-			/* correct data only, not ecc bytes */
-			if (errloc[i] < 8*512)
-				data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
-			pr_debug("corrected bitflip %u\n", errloc[i]);
-		}
-	} else if (count < 0) {
-		pr_err("ecc unrecoverable error\n");
-	}
-	return count;
-}
-#endif /* CONFIG_MTD_NAND_ECC_BCH */
-
-#ifdef CONFIG_MTD_NAND_OMAP_BCH
 /**
  * omap_write_page_bch - BCH ecc based write page function for entire page
  * @mtd:		mtd info structure
@@ -1660,28 +1622,6 @@ static int is_elm_present(struct omap_nand_info *info,
 }
 #endif /* CONFIG_MTD_NAND_ECC_BCH */
 
-#ifdef CONFIG_MTD_NAND_ECC_BCH
-/**
- * omap3_free_bch - Release BCH ecc resources
- * @mtd: MTD device structure
- */
-static void omap3_free_bch(struct mtd_info *mtd)
-{
-	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
-						   mtd);
-	if (info->bch) {
-		free_bch(info->bch);
-		info->bch = NULL;
-	}
-}
-
-#else
-
-static void omap3_free_bch(struct mtd_info *mtd)
-{
-}
-#endif /* CONFIG_MTD_NAND_ECC_BCH */
-
 static int omap_nand_probe(struct platform_device *pdev)
 {
 	struct omap_nand_info		*info;
@@ -1714,13 +1654,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
 	info->reg		= pdata->reg;
-	info->bch		= NULL;
 	info->of_node		= pdata->of_node;
 	mtd			= &info->mtd;
 	mtd->priv		= &info->nand;
 	mtd->name		= dev_name(&pdev->dev);
 	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
+	nand_chip->ecc.priv	= NULL;
 	nand_chip->options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1910,7 +1850,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.bytes		= 7;
 		nand_chip->ecc.strength		= 4;
 		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
-		nand_chip->ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.correct		= nand_bch_correct_data;
 		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch4;
 		/* define ECC layout */
 		ecclayout->eccbytes		= nand_chip->ecc.bytes *
@@ -1920,10 +1860,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
 							ecclayout->eccbytes;
 		/* software bch library is used for locating errors */
-		info->bch = init_bch(nand_chip->ecc.bytes,
-					nand_chip->ecc.strength,
-					OMAP_ECC_BCH8_POLYNOMIAL);
-		if (!info->bch) {
+		info->nand.ecc.priv		= nand_bch_init(mtd,
+							info->nand.ecc.size,
+							info->nand.ecc.bytes,
+							&info->nand.ecc.layout);
+		if (!info->nand.ecc.priv) {
 			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 		}
@@ -1975,7 +1916,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.bytes		= 13;
 		nand_chip->ecc.strength		= 8;
 		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
-		nand_chip->ecc.correct		= omap3_correct_data_bch;
+		info->nand.ecc.correct		= nand_bch_correct_data;
 		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch8;
 		/* define ECC layout */
 		ecclayout->eccbytes		= nand_chip->ecc.bytes *
@@ -1985,10 +1926,11 @@ static int omap_nand_probe(struct platform_device *pdev)
 		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
 							ecclayout->eccbytes;
 		/* software bch library is used for locating errors */
-		info->bch = init_bch(nand_chip->ecc.bytes,
-					nand_chip->ecc.strength,
-					OMAP_ECC_BCH8_POLYNOMIAL);
-		if (!info->bch) {
+		info->nand.ecc.priv		= nand_bch_init(mtd,
+							info->nand.ecc.size,
+							info->nand.ecc.bytes,
+							&info->nand.ecc.layout);
+		if (!info->nand.ecc.priv) {
 			pr_err("nand: error: unable to use s/w BCH library\n");
 			err = -EINVAL;
 			goto out_release_mem_region;
@@ -2074,7 +2016,10 @@ out_release_mem_region:
 		free_irq(info->gpmc_irq_fifo, info);
 	release_mem_region(info->phys_base, info->mem_size);
 out_free_info:
-	omap3_free_bch(mtd);
+	if (info->nand.ecc.priv) {
+		nand_bch_free(info->nand.ecc.priv);
+		info->nand.ecc.priv = NULL;
+	}
 	kfree(info);
 
 	return err;
@@ -2086,7 +2031,10 @@ static int omap_nand_remove(struct platform_device *pdev)
 	struct nand_chip *nand_chip = mtd->priv;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
-	omap3_free_bch(mtd);
+	if (info->nand.ecc.priv) {
+		nand_bch_free(info->nand.ecc.priv);
+		info->nand.ecc.priv = NULL;
+	}
 
 	if (info->dma)
 		dma_release_channel(info->dma);
-- 
1.8.1

Re: [PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[Brian Norris]

+ Ivan, as he has touched this stuff before

On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
> generic frame-work in mtd/nand/nand_bch.c is a wrapper above lib/bch.h which
> encapsulates all control information specific to BCH ecc algorithm in software.
> Thus this patch:
> (1) replace omap specific implementations with equivalent wrapper in nand_bch.c
>     so that more generic code is re-used. like;
>         omap3_correct_data_bch() -> nand_bch_correct_data()
>         omap3_free_bch() -> nand_bch_free()
> (2) replace direct calls to lib/bch.c with wrapper functions defined in nand_bch.c
> 	init_bch() -> nand_bch_init()
> (3) removes selection between BCH8 and BCH4 h/w ecc-schemes via KConfig.
>     This selection is now based on ti,nand-ecc-opt and ti,elm-id DT bindings.
> 
> Signed-off-by: Pekon Gupta <pekon-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/mtd/nand/Kconfig | 30 ++-------------
>  drivers/mtd/nand/omap2.c | 96 +++++++++++-------------------------------------
>  2 files changed, 26 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index d885298..5836039 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
>  
>  config MTD_NAND_OMAP_BCH
>  	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> -	tristate "Enable support for hardware BCH error correction"
> +	tristate "Support hardware based BCH error correction"
>  	default n
>  	select BCH
> -	select BCH_CONST_PARAMS

Do you know what will happen now if someone configures BCH_CONST_PARAMS?
Would this cause problems?

>  	help
> -	 Support for hardware BCH error correction.
> -
> -choice
> -	prompt "BCH error correction capability"
> -	depends on MTD_NAND_OMAP_BCH
> -
> -config MTD_NAND_OMAP_BCH8
> -	bool "8 bits / 512 bytes (recommended)"
> -	help
> -	 Support correcting up to 8 bitflips per 512-byte block.
> -	 This will use 13 bytes of spare area per 512 bytes of page data.
> -	 This is the recommended mode, as 4-bit mode does not work
> -	 on some OMAP3 revisions, due to a hardware bug.
> -
> -config MTD_NAND_OMAP_BCH4
> -	bool "4 bits / 512 bytes"
> -	help
> -	 Support correcting up to 4 bitflips per 512-byte block.
> -	 This will use 7 bytes of spare area per 512 bytes of page data.
> -	 Note that this mode does not work on some OMAP3 revisions, due to a
> -	 hardware bug. Please check your OMAP datasheet before selecting this
> -	 mode.
> -
> -endchoice
> +	  Some devices have built-in ELM hardware engine, which can be used to
> +	  locate and correct errors when using BCH ECC scheme. This enables the
> +	  driver support for same.
>  
>  if MTD_NAND_OMAP_BCH
>  config BCH_CONST_M

Do you need to to also kill of the Kconfig stuff for BCH_CONST_M and
BCH_CONST_T, which were tied to the MTD_NAND_OMAP_BCH4 and
MTD_NAND_OMAP_BCH8 configs you just removed?

> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5f6e621..769ff65 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -25,7 +25,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  
> -#include <linux/bch.h>
> +#include <linux/mtd/nand_bch.h>
>  #include <linux/platform_data/elm.h>
>  
>  #include <linux/platform_data/mtd-nand-omap2.h>
> @@ -140,7 +140,6 @@
>  #define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
>  
>  #define BADBLOCK_MARKER_LENGTH		2
> -#define OMAP_ECC_BCH8_POLYNOMIAL	0x201b
>  
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH
>  static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
> @@ -173,7 +172,6 @@ struct omap_nand_info {
>  	int					buf_len;
>  	struct gpmc_nand_regs		reg;
>  	/* fields specific for BCHx_HW ECC scheme */
> -	struct bch_control             *bch;
>  	bool				is_elm_used;
>  	struct device			*elm_dev;
>  	struct device_node		*of_node;
> @@ -1507,43 +1505,7 @@ static int omap_elm_correct_data(struct mtd_info *mtd, u_char *data,
>  
>  	return stat;
>  }
> -#endif /* CONFIG_MTD_NAND_OMAP_BCH */
>  
> -#ifdef CONFIG_MTD_NAND_ECC_BCH
> -/**
> - * omap3_correct_data_bch - 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 omap3_correct_data_bch(struct mtd_info *mtd, u_char *data,
> -				  u_char *read_ecc, u_char *calc_ecc)
> -{
> -	int i, count;
> -	/* cannot correct more than 8 errors */
> -	unsigned int errloc[8];
> -	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> -						   mtd);
> -
> -	count = decode_bch(info->bch, NULL, 512, read_ecc, calc_ecc, NULL,
> -			   errloc);
> -	if (count > 0) {
> -		/* correct errors */
> -		for (i = 0; i < count; i++) {
> -			/* correct data only, not ecc bytes */
> -			if (errloc[i] < 8*512)
> -				data[errloc[i]/8] ^= 1 << (errloc[i] & 7);
> -			pr_debug("corrected bitflip %u\n", errloc[i]);
> -		}
> -	} else if (count < 0) {
> -		pr_err("ecc unrecoverable error\n");
> -	}
> -	return count;
> -}
> -#endif /* CONFIG_MTD_NAND_ECC_BCH */
> -
> -#ifdef CONFIG_MTD_NAND_OMAP_BCH
>  /**
>   * omap_write_page_bch - BCH ecc based write page function for entire page
>   * @mtd:		mtd info structure
> @@ -1660,28 +1622,6 @@ static int is_elm_present(struct omap_nand_info *info,
>  }
>  #endif /* CONFIG_MTD_NAND_ECC_BCH */
>  
> -#ifdef CONFIG_MTD_NAND_ECC_BCH
> -/**
> - * omap3_free_bch - Release BCH ecc resources
> - * @mtd: MTD device structure
> - */
> -static void omap3_free_bch(struct mtd_info *mtd)
> -{
> -	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> -						   mtd);
> -	if (info->bch) {
> -		free_bch(info->bch);
> -		info->bch = NULL;
> -	}
> -}
> -
> -#else
> -
> -static void omap3_free_bch(struct mtd_info *mtd)
> -{
> -}
> -#endif /* CONFIG_MTD_NAND_ECC_BCH */
> -
>  static int omap_nand_probe(struct platform_device *pdev)
>  {
>  	struct omap_nand_info		*info;
> @@ -1714,13 +1654,13 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	info->pdev		= pdev;
>  	info->gpmc_cs		= pdata->cs;
>  	info->reg		= pdata->reg;
> -	info->bch		= NULL;
>  	info->of_node		= pdata->of_node;
>  	mtd			= &info->mtd;
>  	mtd->priv		= &info->nand;
>  	mtd->name		= dev_name(&pdev->dev);
>  	mtd->owner		= THIS_MODULE;
>  	nand_chip		= &info->nand;
> +	nand_chip->ecc.priv	= NULL;
>  	nand_chip->options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1910,7 +1850,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		nand_chip->ecc.bytes		= 7;
>  		nand_chip->ecc.strength		= 4;
>  		nand_chip->ecc.hwctl		= omap3_enable_hwecc_bch;
> -		nand_chip->ecc.correct		= omap3_correct_data_bch;
> +		info->nand.ecc.correct		= nand_bch_correct_data;
>  		nand_chip->ecc.calculate	= omap3_calculate_ecc_bch4;
>  		/* define ECC layout */
>  		ecclayout->eccbytes		= nand_chip->ecc.bytes *
> @@ -1920,10 +1860,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
>  							ecclayout->eccbytes;
>  		/* software bch library is used for locating errors */
> -		info->bch = init_bch(nand_chip->ecc.bytes,
> -					nand_chip->ecc.strength,
> -					OMAP_ECC_BCH8_POLYNOMIAL);
> -		if (!info->bch) {
> +		info->nand.ecc.priv		= nand_bch_init(mtd,
> +							info->nand.ecc.size,
> +							info->nand.ecc.bytes,
> +							&info->nand.ecc.layout);

Are you sure nand_bch_init() is a proper drop-in replacement for the
implementation you had based on init_bch()? It looks to me like they at
least use a differnt polynomial value (0x201b vs. 0). Is this a problem
for backwards compatibility?

> +		if (!info->nand.ecc.priv) {
>  			pr_err("nand: error: unable to use s/w BCH library\n");
>  			err = -EINVAL;
>  		}

[...]

> @@ -1985,10 +1926,11 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		ecclayout->oobfree->offset	= ecclayout->eccpos[0] +
>  							ecclayout->eccbytes;
>  		/* software bch library is used for locating errors */
> -		info->bch = init_bch(nand_chip->ecc.bytes,
> -					nand_chip->ecc.strength,
> -					OMAP_ECC_BCH8_POLYNOMIAL);
> -		if (!info->bch) {
> +		info->nand.ecc.priv		= nand_bch_init(mtd,
> +							info->nand.ecc.size,
> +							info->nand.ecc.bytes,
> +							&info->nand.ecc.layout);

Same questions apply.

> +		if (!info->nand.ecc.priv) {
>  			pr_err("nand: error: unable to use s/w BCH library\n");
>  			err = -EINVAL;
>  			goto out_release_mem_region;

[...]

A related question: do we have any current users of this driver that can
provide testing results for this series? Or is this work just tested
with new hardware?

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[Gupta, Pekon]

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
[snip]

> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > index d885298..5836039 100644
> > --- a/drivers/mtd/nand/Kconfig
> > +++ b/drivers/mtd/nand/Kconfig
> > @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
> >
> >  config MTD_NAND_OMAP_BCH
> >  	depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
> > -	tristate "Enable support for hardware BCH error correction"
> > +	tristate "Support hardware based BCH error correction"
> >  	default n
> >  	select BCH
> > -	select BCH_CONST_PARAMS
> 
> Do you know what will happen now if someone configures
> BCH_CONST_PARAMS?
> Would this cause problems?
> 
As per comments in lib/bch.c 
--------------------------- 
* Option CONFIG_BCH_CONST_PARAMS can be used to force fixed values of
 * parameters m and t; thus allowing extra compiler optimizations and providing
 * better (up to 2x) encoding performance. Using this option makes sense when
 * (m,t) are fixed and known in advance, e.g. when using BCH error correction
 * on a particular NAND flash device.
---------------------------
'BCH_CONST_PARAMS' is required for optimization when BCH algorithm
is fixed. But in omap-nand case selection of type of BCH algorithm
(BCH4 or BCH8) comes from DT binding "ti,nand-ecc-opts".

If enabled (CONFIG_BCH_CONST_PARAMS) will optimize lib/bch.c
for BCH8 algorithm by default, so 
CASE: if BCH8 is selected by DT, then no issues
CASE: if BCH4 is selected  then nand_bch_init() fails with following error
+ if (!info->nand.ecc.priv) {
		pr_err("nand: error: unable to use s/w BCH library\n");
		err = -EINVAL;
		goto out_release_mem_region;
}

[snip] 

> >  if MTD_NAND_OMAP_BCH
> >  config BCH_CONST_M
> 
> Do you need to to also kill of the Kconfig stuff for BCH_CONST_M and
> BCH_CONST_T, which were tied to the MTD_NAND_OMAP_BCH4 and
> MTD_NAND_OMAP_BCH8 configs you just removed?
> 
Thanks, good catch. I dint really notice.
So, the driver is now updated to separate out two flavours of BCHx scheme.
(a) OMAP_ECC_BCHx_CODE_HW: which uses ELM hardware
(b) OMAP_ECC_BCHx_CODE_HW_DETECTION_SW: which uses lib/bch.c
These BCH_CONST_M and BCH_CONST_T now belongs to (b) family only.
-  if MTD_NAND_OMAP_BCH
+ if MTD_NAND_ECC_BCH
       config BCH_CONST_M
(I'll update that)..


> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
[snip]
> > -		info->bch = init_bch(nand_chip->ecc.bytes,
> > -					nand_chip->ecc.strength,
> > -					OMAP_ECC_BCH8_POLYNOMIAL);
> > -		if (!info->bch) {
> > +		info->nand.ecc.priv	= nand_bch_init(mtd,
> > +						info->nand.ecc.size,
> > +						info->nand.ecc.bytes,
> > +						&info->nand.ecc.layout);
> 
> Are you sure nand_bch_init() is a proper drop-in replacement for the
> implementation you had based on init_bch()? It looks to me like they at
> least use a differnt polynomial value (0x201b vs. 0). Is this a problem
> for backwards compatibility?
> 
It's not the polynomial value = 0. Rather 0x201b is selected in both cases
Refer below code.
---------------
When init_bch(m,t, 0) is called from nand_bch_init() then,
lib/bch.c @@ init_bch(int m, int t, unsigned int prim_poly)
(a)	/* default primitive polynomials */
	static const unsigned int prim_poly_tab[] = {
		0x25, 0x43, 0x83, 0x11d, 0x211, 0x409, 0x805, 0x1053, 0x201b,
		0x402b, 0x8003,
	};
(b)	/* select a primitive polynomial for generating GF(2^m) */
	if (prim_poly == 0)
		prim_poly = prim_poly_tab[m-min_m];
(c) And, const int min_m = 5;

So, for BCH8 m=13, min_m=5; So 
prim_poly = prim_poly_tab[13-5] = prim_poly_tab[8] = 0x201b
---------------

Hence, there is no change in polynomial, it's just that instead of
hard-coding the value, polynomial selection depends on 'm' and 't'.



> [...]
> 
> A related question: do we have any current users of this driver that can
> provide testing results for this series? Or is this work just tested
> with new hardware?
> 
Got a tested-by: jp.francois@cynove.com for BCH4
But that was in May,2013 :-)
http://lists.infradead.org/pipermail/linux-mtd/2013-May/047065.html


with regards, pekon

Re: [PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[jean-philippe francois]

2013/10/17 Gupta, Pekon <pekon@ti.com>:
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>> > On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
> [snip]
>
>> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> > index d885298..5836039 100644
>> > --- a/drivers/mtd/nand/Kconfig
>> > +++ b/drivers/mtd/nand/Kconfig
>> > @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
>> >
>> >  config MTD_NAND_OMAP_BCH
>> >     depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
>> > -   tristate "Enable support for hardware BCH error correction"
>> > +   tristate "Support hardware based BCH error correction"
>> >     default n
>> >     select BCH
>> > -   select BCH_CONST_PARAMS
>>
>> Do you know what will happen now if someone configures
>> BCH_CONST_PARAMS?
>> Would this cause problems?
>>
> As per comments in lib/bch.c
> ---------------------------
> * Option CONFIG_BCH_CONST_PARAMS can be used to force fixed values of
>  * parameters m and t; thus allowing extra compiler optimizations and providing
>  * better (up to 2x) encoding performance. Using this option makes sense when
>  * (m,t) are fixed and known in advance, e.g. when using BCH error correction
>  * on a particular NAND flash device.
> ---------------------------
> 'BCH_CONST_PARAMS' is required for optimization when BCH algorithm
> is fixed. But in omap-nand case selection of type of BCH algorithm
> (BCH4 or BCH8) comes from DT binding "ti,nand-ecc-opts".
>
> If enabled (CONFIG_BCH_CONST_PARAMS) will optimize lib/bch.c
> for BCH8 algorithm by default, so
> CASE: if BCH8 is selected by DT, then no issues
> CASE: if BCH4 is selected  then nand_bch_init() fails with following error
> + if (!info->nand.ecc.priv) {
>                 pr_err("nand: error: unable to use s/w BCH library\n");
>                 err = -EINVAL;
>                 goto out_release_mem_region;
> }
>
> [snip]
>
>> >  if MTD_NAND_OMAP_BCH
>> >  config BCH_CONST_M
>>
>> Do you need to to also kill of the Kconfig stuff for BCH_CONST_M and
>> BCH_CONST_T, which were tied to the MTD_NAND_OMAP_BCH4 and
>> MTD_NAND_OMAP_BCH8 configs you just removed?
>>
> Thanks, good catch. I dint really notice.
> So, the driver is now updated to separate out two flavours of BCHx scheme.
> (a) OMAP_ECC_BCHx_CODE_HW: which uses ELM hardware
> (b) OMAP_ECC_BCHx_CODE_HW_DETECTION_SW: which uses lib/bch.c
> These BCH_CONST_M and BCH_CONST_T now belongs to (b) family only.
> -  if MTD_NAND_OMAP_BCH
> + if MTD_NAND_ECC_BCH
>        config BCH_CONST_M
> (I'll update that)..
>
>
>> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> [snip]
>> > -           info->bch = init_bch(nand_chip->ecc.bytes,
>> > -                                   nand_chip->ecc.strength,
>> > -                                   OMAP_ECC_BCH8_POLYNOMIAL);
>> > -           if (!info->bch) {
>> > +           info->nand.ecc.priv     = nand_bch_init(mtd,
>> > +                                           info->nand.ecc.size,
>> > +                                           info->nand.ecc.bytes,
>> > +                                           &info->nand.ecc.layout);
>>
>> Are you sure nand_bch_init() is a proper drop-in replacement for the
>> implementation you had based on init_bch()? It looks to me like they at
>> least use a differnt polynomial value (0x201b vs. 0). Is this a problem
>> for backwards compatibility?
>>
> It's not the polynomial value = 0. Rather 0x201b is selected in both cases
> Refer below code.
> ---------------
> When init_bch(m,t, 0) is called from nand_bch_init() then,
> lib/bch.c @@ init_bch(int m, int t, unsigned int prim_poly)
> (a)     /* default primitive polynomials */
>         static const unsigned int prim_poly_tab[] = {
>                 0x25, 0x43, 0x83, 0x11d, 0x211, 0x409, 0x805, 0x1053, 0x201b,
>                 0x402b, 0x8003,
>         };
> (b)     /* select a primitive polynomial for generating GF(2^m) */
>         if (prim_poly == 0)
>                 prim_poly = prim_poly_tab[m-min_m];
> (c) And, const int min_m = 5;
>
> So, for BCH8 m=13, min_m=5; So
> prim_poly = prim_poly_tab[13-5] = prim_poly_tab[8] = 0x201b
> ---------------
>
> Hence, there is no change in polynomial, it's just that instead of
> hard-coding the value, polynomial selection depends on 'm' and 't'.
>
>
>
>> [...]
>>
>> A related question: do we have any current users of this driver that can
>> provide testing results for this series? Or is this work just tested
>> with new hardware?
>>
> Got a tested-by: jp.francois@cynove.com for BCH4
> But that was in May,2013 :-)
> http://lists.infradead.org/pipermail/linux-mtd/2013-May/047065.html
>
>
> with regards, pekon

Yes, but I am currently stuck on 3.10, and I need to migrate my out of tree
board file to a device tree, and I currently have no bandwidth for this, so I
won't be able to test the whole serie, but may be some parts are
testable independently ?

I will take a look and tell you.

Regards,
Jean-Philippe François
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[Brian Norris]

On Thu, Oct 17, 2013 at 3:14 AM, Gupta, Pekon <pekon@ti.com> wrote:
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>> > On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
> [snip]
>
>> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> > index d885298..5836039 100644
>> > --- a/drivers/mtd/nand/Kconfig
>> > +++ b/drivers/mtd/nand/Kconfig
>> > @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
>> >
>> >  config MTD_NAND_OMAP_BCH
>> >     depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
>> > -   tristate "Enable support for hardware BCH error correction"
>> > +   tristate "Support hardware based BCH error correction"
>> >     default n
>> >     select BCH
>> > -   select BCH_CONST_PARAMS
>>
>> Do you know what will happen now if someone configures
>> BCH_CONST_PARAMS?
>> Would this cause problems?
>>
> As per comments in lib/bch.c
> ---------------------------
> * Option CONFIG_BCH_CONST_PARAMS can be used to force fixed values of
>  * parameters m and t; thus allowing extra compiler optimizations and providing
>  * better (up to 2x) encoding performance. Using this option makes sense when
>  * (m,t) are fixed and known in advance, e.g. when using BCH error correction
>  * on a particular NAND flash device.
> ---------------------------
> 'BCH_CONST_PARAMS' is required for optimization when BCH algorithm
> is fixed. But in omap-nand case selection of type of BCH algorithm
> (BCH4 or BCH8) comes from DT binding "ti,nand-ecc-opts".
>
> If enabled (CONFIG_BCH_CONST_PARAMS) will optimize lib/bch.c
> for BCH8 algorithm by default, so
> CASE: if BCH8 is selected by DT, then no issues
> CASE: if BCH4 is selected  then nand_bch_init() fails with following error
> + if (!info->nand.ecc.priv) {
>                 pr_err("nand: error: unable to use s/w BCH library\n");
>                 err = -EINVAL;
>                 goto out_release_mem_region;
> }

Ah, so lib/bch.c handles this gracefully enough:

#if defined(CONFIG_BCH_CONST_PARAMS)
        if ((m != (CONFIG_BCH_CONST_M)) || (t != (CONFIG_BCH_CONST_T))) {
                printk(KERN_ERR "bch encoder/decoder was configured to support "
                       "parameters m=%d, t=%d only!\n",
                       CONFIG_BCH_CONST_M, CONFIG_BCH_CONST_T);
                goto fail;
        }
#endif

>> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> [snip]
>> > -           info->bch = init_bch(nand_chip->ecc.bytes,
>> > -                                   nand_chip->ecc.strength,
>> > -                                   OMAP_ECC_BCH8_POLYNOMIAL);
>> > -           if (!info->bch) {
>> > +           info->nand.ecc.priv     = nand_bch_init(mtd,
>> > +                                           info->nand.ecc.size,
>> > +                                           info->nand.ecc.bytes,
>> > +                                           &info->nand.ecc.layout);
>>
>> Are you sure nand_bch_init() is a proper drop-in replacement for the
>> implementation you had based on init_bch()? It looks to me like they at
>> least use a differnt polynomial value (0x201b vs. 0). Is this a problem
>> for backwards compatibility?
>>
> It's not the polynomial value = 0. Rather 0x201b is selected in both cases
> Refer below code.
> ---------------
> When init_bch(m,t, 0) is called from nand_bch_init() then,
> lib/bch.c @@ init_bch(int m, int t, unsigned int prim_poly)
> (a)     /* default primitive polynomials */
>         static const unsigned int prim_poly_tab[] = {
>                 0x25, 0x43, 0x83, 0x11d, 0x211, 0x409, 0x805, 0x1053, 0x201b,
>                 0x402b, 0x8003,
>         };
> (b)     /* select a primitive polynomial for generating GF(2^m) */
>         if (prim_poly == 0)
>                 prim_poly = prim_poly_tab[m-min_m];
> (c) And, const int min_m = 5;
>
> So, for BCH8 m=13, min_m=5; So
> prim_poly = prim_poly_tab[13-5] = prim_poly_tab[8] = 0x201b
> ---------------
>
> Hence, there is no change in polynomial, it's just that instead of
> hard-coding the value, polynomial selection depends on 'm' and 't'.

I missed that. Thanks for pointing it out.

>> [...]
>>
>> A related question: do we have any current users of this driver that can
>> provide testing results for this series? Or is this work just tested
>> with new hardware?
>>
> Got a tested-by: jp.francois@cynove.com for BCH4
> But that was in May,2013 :-)
> http://lists.infradead.org/pipermail/linux-mtd/2013-May/047065.html

OK, well that's good to know. I suppose the operation of the driver
probably hasn't changed a lot in the meantime, just the DT bindings
and a few other smaller details. But we still can't call it
"Tested-by:" in its current form.

Brian

[PATCH v9 8/9] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt

[Pekon Gupta]

Updated DTS to replace deprecated binding with newer values
Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt

Signed-off-by: Pekon Gupta <pekon@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
---
 arch/arm/boot/dts/am335x-evm.dts   | 3 +--
 arch/arm/boot/dts/omap3430-sdp.dts | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index e8ec875..1aee6ac 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -269,7 +269,6 @@
 				reg = <0 0 0>; /* CS0, offset 0 */
 				nand-bus-width = <8>;
 				ti,nand-ecc-opt = "bch8";
-				gpmc,device-nand = "true";
 				gpmc,device-width = <1>;
 				gpmc,sync-clk-ps = <0>;
 				gpmc,cs-on-ns = <0>;
@@ -296,7 +295,7 @@
 
 				#address-cells = <1>;
 				#size-cells = <1>;
-				elm_id = <&elm>;
+				ti,elm-id = <&elm>;
 
 				/* MTD partition table */
 				partition@0 {
diff --git a/arch/arm/boot/dts/omap3430-sdp.dts b/arch/arm/boot/dts/omap3430-sdp.dts
index e2249bc..501f863 100644
--- a/arch/arm/boot/dts/omap3430-sdp.dts
+++ b/arch/arm/boot/dts/omap3430-sdp.dts
@@ -105,7 +105,7 @@
 		reg = <1 0 0x08000000>;
 		nand-bus-width = <8>;
 
-		ti,nand-ecc-opt = "sw";
+		ti,nand-ecc-opt = "ham1";
 		gpmc,cs-on-ns = <0>;
 		gpmc,cs-rd-off-ns = <36>;
 		gpmc,cs-wr-off-ns = <36>;
-- 
1.8.1

[PATCH v9 9/9] mtd: nand: omap: updated devm_xx for all resource allocation and free calls

[Pekon Gupta]

"Managed Device Resource" or devm_xx calls takes care of automatic freeing
of the resource in case of:
- failure during driver probe
- failure during resource allocation
- detaching or unloading of driver module (rmmod)
Reference: Documentation/driver-model/devres.txt

Though OMAP NAND driver handles freeing of resource allocation in most of
the cases, but using devm_xx provides more clean and effortless approach
to handle all such cases.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 769ff65..0ed0d6f 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1642,7 +1642,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	info = kzalloc(sizeof(struct omap_nand_info), GFP_KERNEL);
+	info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
+				GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -1667,19 +1668,20 @@ static int omap_nand_probe(struct platform_device *pdev)
 	if (res == NULL) {
 		err = -EINVAL;
 		dev_err(&pdev->dev, "error getting memory resource\n");
-		goto out_free_info;
+		goto out_release_mem_region;
 	}
 
 	info->phys_base = res->start;
 	info->mem_size = resource_size(res);
 
-	if (!request_mem_region(info->phys_base, info->mem_size,
-				pdev->dev.driver->name)) {
+	if (!devm_request_mem_region(&pdev->dev, info->phys_base,
+				info->mem_size,	pdev->dev.driver->name)) {
 		err = -EBUSY;
-		goto out_free_info;
+		goto out_release_mem_region;
 	}
 
-	nand_chip->IO_ADDR_R = ioremap(info->phys_base, info->mem_size);
+	nand_chip->IO_ADDR_R = devm_ioremap(&pdev->dev, info->phys_base,
+						info->mem_size);
 	if (!nand_chip->IO_ADDR_R) {
 		err = -ENOMEM;
 		goto out_release_mem_region;
@@ -1781,8 +1783,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 			err = -ENODEV;
 			goto out_release_mem_region;
 		}
-		err = request_irq(info->gpmc_irq_fifo,	omap_nand_irq,
-					IRQF_SHARED, "gpmc-nand-fifo", info);
+		err = devm_request_irq(&pdev->dev, info->gpmc_irq_fifo,
+					omap_nand_irq, IRQF_SHARED,
+					"gpmc-nand-fifo", info);
 		if (err) {
 			dev_err(&pdev->dev, "requesting irq(%d) error:%d",
 						info->gpmc_irq_fifo, err);
@@ -1796,8 +1799,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 			err = -ENODEV;
 			goto out_release_mem_region;
 		}
-		err = request_irq(info->gpmc_irq_count,	omap_nand_irq,
-					IRQF_SHARED, "gpmc-nand-count", info);
+		err = devm_request_irq(&pdev->dev, info->gpmc_irq_count,
+					omap_nand_irq, IRQF_SHARED,
+					"gpmc-nand-count", info);
 		if (err) {
 			dev_err(&pdev->dev, "requesting irq(%d) error:%d",
 						info->gpmc_irq_count, err);
@@ -2010,45 +2014,25 @@ static int omap_nand_probe(struct platform_device *pdev)
 out_release_mem_region:
 	if (info->dma)
 		dma_release_channel(info->dma);
-	if (info->gpmc_irq_count > 0)
-		free_irq(info->gpmc_irq_count, info);
-	if (info->gpmc_irq_fifo > 0)
-		free_irq(info->gpmc_irq_fifo, info);
-	release_mem_region(info->phys_base, info->mem_size);
-out_free_info:
 	if (info->nand.ecc.priv) {
 		nand_bch_free(info->nand.ecc.priv);
 		info->nand.ecc.priv = NULL;
 	}
-	kfree(info);
-
 	return err;
 }
 
 static int omap_nand_remove(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct nand_chip *nand_chip = mtd->priv;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
 	if (info->nand.ecc.priv) {
 		nand_bch_free(info->nand.ecc.priv);
 		info->nand.ecc.priv = NULL;
 	}
-
 	if (info->dma)
 		dma_release_channel(info->dma);
-
-	if (info->gpmc_irq_count > 0)
-		free_irq(info->gpmc_irq_count, info);
-	if (info->gpmc_irq_fifo > 0)
-		free_irq(info->gpmc_irq_fifo, info);
-
-	/* Release NAND device, its internal structures and partitions */
 	nand_release(mtd);
-	iounmap(nand_chip->IO_ADDR_R);
-	release_mem_region(info->phys_base, info->mem_size);
-	kfree(info);
 	return 0;
 }
 
-- 
1.8.1

Re: [PATCH v9 9/9] mtd: nand: omap: updated devm_xx for all resource allocation and free calls

[Brian Norris]

On Tue, Oct 15, 2013 at 11:19:57AM +0530, Pekon Gupta wrote:
> "Managed Device Resource" or devm_xx calls takes care of automatic freeing
> of the resource in case of:
> - failure during driver probe
> - failure during resource allocation
> - detaching or unloading of driver module (rmmod)
> Reference: Documentation/driver-model/devres.txt
> 
> Though OMAP NAND driver handles freeing of resource allocation in most of
> the cases, but using devm_xx provides more clean and effortless approach
> to handle all such cases.
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/omap2.c | 44 ++++++++++++++------------------------------
>  1 file changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 769ff65..0ed0d6f 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c

[...]

> @@ -1796,8 +1799,9 @@ static int omap_nand_probe(struct platform_device *pdev)
>  			err = -ENODEV;
>  			goto out_release_mem_region;
>  		}
> -		err = request_irq(info->gpmc_irq_count,	omap_nand_irq,
> -					IRQF_SHARED, "gpmc-nand-count", info);
> +		err = devm_request_irq(&pdev->dev, info->gpmc_irq_count,
> +					omap_nand_irq, IRQF_SHARED,
> +					"gpmc-nand-count", info);
>  		if (err) {
>  			dev_err(&pdev->dev, "requesting irq(%d) error:%d",
>  						info->gpmc_irq_count, err);
> @@ -2010,45 +2014,25 @@ static int omap_nand_probe(struct platform_device *pdev)
>  out_release_mem_region:

Now that you've simplified this error path, you don't need so verbose of
a name (it could just be 'out'), but that's not worth another revision.

>  	if (info->dma)
>  		dma_release_channel(info->dma);
> -	if (info->gpmc_irq_count > 0)
> -		free_irq(info->gpmc_irq_count, info);
> -	if (info->gpmc_irq_fifo > 0)
> -		free_irq(info->gpmc_irq_fifo, info);
> -	release_mem_region(info->phys_base, info->mem_size);
> -out_free_info:
>  	if (info->nand.ecc.priv) {
>  		nand_bch_free(info->nand.ecc.priv);
>  		info->nand.ecc.priv = NULL;
>  	}
> -	kfree(info);
> -
>  	return err;
>  }

[...]

This patch version looks much better. Thanks for the rewrite.

Brian

RE: [PATCH v9 0/9] mtd:nand:omap2: clean-up of supported ECC schemes

[Gupta, Pekon]

Hi Brian,

> 
> *changes v8 -> v9*
> [PATCH 1/9] <no update from [PATCH v8 1/6]>
> [PATCH 2/9] <only commit log updated from [PATCH v8 2/6]>
>  As per feedbacks from Brian Norris <computersforpeace@gmail.com>
> previous
>  revision [PATCH v8 3/6] and [PATCH 4/6] are split into following sub-patches:
> - [PATCH 3/9] <new> replaces local reference with generic names (mtd,
> nand_chip)
> - [PATCH 4/9] <new> enables auto-detection of bus-width
> - [PATCH 5/9] <new> removes omap3_init_bch: populates ecc-scheme data
> - [PATCH 6/9] <new> removes omap3_init_bch_tail: populates ecc-layout
> - [PATCH 7/9] <new> replaces lib/bch.c with nand_bch.c wrapper
> [PATCH 8/9] <no update same as [PATCH v8 5/6]
> [PATCH 9/9] removed devm_free_xx functions
> 

Request you to have a look at v9.
If this is acceptable, then I would work to rebase the other pending
series on top of this one.


with regards, pekon

Re: [PATCH v9 0/9] mtd:nand:omap2: clean-up of supported ECC schemes

[Brian Norris]

Hi,

On Tue, Oct 15, 2013 at 11:19:48AM +0530, Pekon Gupta wrote:
> 
> *changes v8 -> v9*
> [PATCH 1/9] <no update from [PATCH v8 1/6]>
> [PATCH 2/9] <only commit log updated from [PATCH v8 2/6]>
>  As per feedbacks from Brian Norris <computersforpeace@gmail.com> previous
>  revision [PATCH v8 3/6] and [PATCH 4/6] are split into following sub-patches:
> - [PATCH 3/9] <new> replaces local reference with generic names (mtd, nand_chip)
> - [PATCH 4/9] <new> enables auto-detection of bus-width
> - [PATCH 5/9] <new> removes omap3_init_bch: populates ecc-scheme data
> - [PATCH 6/9] <new> removes omap3_init_bch_tail: populates ecc-layout
> - [PATCH 7/9] <new> replaces lib/bch.c with nand_bch.c wrapper
> [PATCH 8/9] <no update same as [PATCH v8 5/6]
> [PATCH 9/9] removed devm_free_xx functions

[...]

I had a few comments on patches 4 and 7, but everything else is looking
a lot better.

Thanks,
Brian