Patchwork [v5,1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

login
register
mail settings
Submitter pekon gupta
Date July 13, 2013, 8:54 p.m.
Message ID <1373748891-7779-2-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/258838/
State New
Headers show

Comments

pekon gupta - July 13, 2013, 8:54 p.m.
ECC scheme on NAND devices can be implemented in multiple ways.Some using
Software algorithm, while others using in-build Hardware engines.
omap2-nand driver currently supports following flavours of ECC schemes,
selectable via DTB.

+---------------------------------------+---------------+---------------+
| ECC scheme                            |ECC calculation|Error detection|
+---------------------------------------+---------------+---------------+
|OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
|OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
|OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
|OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
+---------------------------------------+---------------+---------------+
|(requires CONFIG_MTD_NAND_OMAP_BCH)    |               |               |
|OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
+---------------------------------------+---------------+---------------+

Selection of some ECC schemes also require enabling following Kconfig options.
This was done to optimize footprint of omap2-nand driver.
-Kconfig:CONFIG_MTD_NAND_ECC_BCH	enables S/W based BCH ECC algorithm
-Kconfig:CONFIG_MTD_NAND_OMAP_BCH	enables H/W based BCH ECC algorithm

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          | 45 ++++++++++++++++------
 arch/arm/mach-omap2/gpmc.c                         | 14 ++++---
 include/linux/platform_data/mtd-nand-omap2.h       | 22 +++++++----
 3 files changed, 57 insertions(+), 24 deletions(-)
Brian Norris - Aug. 21, 2013, 1:26 a.m.
Including the correct device-tree list.

On Sun, Jul 14, 2013 at 02:24:48AM +0530, Pekon Gupta wrote:
> ECC scheme on NAND devices can be implemented in multiple ways.Some using
> Software algorithm, while others using in-build Hardware engines.
> omap2-nand driver currently supports following flavours of ECC schemes,
> selectable via DTB.
> 
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |(requires CONFIG_MTD_NAND_OMAP_BCH)    |               |               |
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> 
> Selection of some ECC schemes also require enabling following Kconfig options.
> This was done to optimize footprint of omap2-nand driver.
> -Kconfig:CONFIG_MTD_NAND_ECC_BCH	enables S/W based BCH ECC algorithm
> -Kconfig:CONFIG_MTD_NAND_OMAP_BCH	enables H/W based BCH ECC algorithm
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          | 45 ++++++++++++++++------
>  arch/arm/mach-omap2/gpmc.c                         | 14 ++++---
>  include/linux/platform_data/mtd-nand-omap2.h       | 22 +++++++----
>  3 files changed, 57 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index df338cb..c6551b6 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -17,17 +17,42 @@ Required properties:
>  
>  Optional properties:
>  
> - - nand-bus-width: 		Set this numeric value to 16 if the hardware
> -				is wired that way. If not specified, a bus
> -				width of 8 is assumed.
> + - nand-bus-width: 		Determines data-width of the connected device
> +				x16 = "16"
> +				x8  = "8" (default)
>  
> - - 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
> -		"bch4"		4-bit BCH ecc code
> -		"bch8"		8-bit BCH ecc code
> + - ti,nand-ecc-opt:		Determines the ECC scheme used by driver.
> +				It can be any of the following strings:
> +
> +	"hamming_code_sw"		1-bit Hamming ECC
> +					- ECC calculation in software
> +					- Error detection in software
> +
> +	"hamming_code_hw"		1-bit Hamming ECC
> +					- ECC calculation in hardware
> +					- Error detection in software
> +
> +	"hamming_code_hw_romcode"	1-bit Hamming ECC
> +					- ECC calculation in hardware
> +					- Error detection in software
> +					- ECC layout compatible to ROM code
> +
> +	"bch8_hw_code_detection_sw"	8-bit BCH ECC
> +					- ECC calculation in hardware
> +					- Error detection in software
> +					- depends on CONFIG_MTD_NAND_ECC_BCH
> +
> +	"bch8_code_hw"             	8-bit BCH ECC
> +					- ECC calculation in hardware
> +					- Error detection in hardware
> +					- depends on CONFIG_MTD_NAND_OMAP_BCH
> +					- requires <elm_id> to be specified

I'm pretty sure this is an ABI breakage, which is now considered
unacceptable. AFAIK, you can support new ECC types (or new aliases for
old ones), but you can't remove/rename old ones.

> +
> +
> + - elm_id:			Specifies elm device node. This is required to
> +				support some BCH ECC schemes mentioned above.
> +
>  
>   - ti,nand-xfer-type:		A string setting the data transfer type. One of:
>  
> @@ -36,8 +61,6 @@ 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.
>  
>  For inline partiton table parsing (optional):
>  
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 1c7969e..e19de21 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1342,11 +1342,13 @@ 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_BCH4_CODE_HW]			= "bch4",
> -	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
> +	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "hamming_code_sw",
> +	[OMAP_ECC_HAMMING_CODE_HW]		= "hamming_code_hw",
> +	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hamming_code_hw_romcode",
> +	[OMAP_ECC_BCH4_CODE_HW]			= "bch4_code_hw",
> +	[OMAP_ECC_BCH4_CODE_HW_DETECTION_SW]	= "bch4_code_hw_detection_sw",
> +	[OMAP_ECC_BCH8_CODE_HW]			= "bch8_code_hw",
> +	[OMAP_ECC_BCH8_CODE_HW_DETECTION_SW]	= "bch8_code_hw_detection_sw"
>  };
>  
>  static const char * const nand_xfer_types[] = {
> @@ -1380,7 +1382,7 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  
>  	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])) {
> +			if (!strcmp(s, nand_ecc_opts[val])) {
>  				gpmc_nand_data->ecc_opt = val;
>  				break;
>  			}
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
> index 6bf9ef4..ce74576 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -23,13 +23,21 @@ 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_BCH4_CODE_HW, /* 4-bit BCH ecc code */
> -	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
> +	/* 1-bit  ECC calculation by Software, Error detection by Software */
> +	OMAP_ECC_HAMMING_CODE_DEFAULT = 0,
> +	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
> +	OMAP_ECC_HAMMING_CODE_HW,
> +	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
> +	/* ECC layout compatible to legacy ROMCODE. */
> +	OMAP_ECC_HAMMING_CODE_HW_ROMCODE,
> +	/* 4-bit  ECC calculation by GPMC, Error detection by ELM */
> +	OMAP_ECC_BCH4_CODE_HW,
> +	/* 4-bit  ECC calculation by GPMC, Error detection by Software */
> +	OMAP_ECC_BCH4_CODE_HW_DETECTION_SW,
> +	/* 8-bit  ECC calculation by GPMC, Error detection by ELM */
> +	OMAP_ECC_BCH8_CODE_HW,
> +	/* 8-bit  ECC calculation by GPMC, Error detection by Software */
> +	OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
>  };
>  
>  struct gpmc_nand_regs {

Brian
Olof Johansson - Aug. 22, 2013, 4:54 a.m.
On Sun, Jul 14, 2013 at 02:24:48AM +0530, Pekon Gupta wrote:
> ECC scheme on NAND devices can be implemented in multiple ways.Some using
> Software algorithm, while others using in-build Hardware engines.
> omap2-nand driver currently supports following flavours of ECC schemes,
> selectable via DTB.
> 
> +---------------------------------------+---------------+---------------+
> | ECC scheme                            |ECC calculation|Error detection|
> +---------------------------------------+---------------+---------------+
> |OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> |OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W            |
> +---------------------------------------+---------------+---------------+
> |(requires CONFIG_MTD_NAND_OMAP_BCH)    |               |               |
> |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> +---------------------------------------+---------------+---------------+
> 
> Selection of some ECC schemes also require enabling following Kconfig options.
> This was done to optimize footprint of omap2-nand driver.
> -Kconfig:CONFIG_MTD_NAND_ECC_BCH	enables S/W based BCH ECC algorithm
> -Kconfig:CONFIG_MTD_NAND_OMAP_BCH	enables H/W based BCH ECC algorithm
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          | 45 ++++++++++++++++------
>  arch/arm/mach-omap2/gpmc.c                         | 14 ++++---
>  include/linux/platform_data/mtd-nand-omap2.h       | 22 +++++++----
>  3 files changed, 57 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index df338cb..c6551b6 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -17,17 +17,42 @@ Required properties:
>  
>  Optional properties:
>  
> - - nand-bus-width: 		Set this numeric value to 16 if the hardware
> -				is wired that way. If not specified, a bus
> -				width of 8 is assumed.
> + - nand-bus-width: 		Determines data-width of the connected device
> +				x16 = "16"
> +				x8  = "8" (default)

This change doesn't look like an improvement to me.

> - - 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
> -		"bch4"		4-bit BCH ecc code
> -		"bch8"		8-bit BCH ecc code
> + - ti,nand-ecc-opt:		Determines the ECC scheme used by driver.
> +				It can be any of the following strings:

The device tree should define the hardware, not configure the software.

Also, if it defines the scheme then you should probably call it something like
"ti,nand-ecc-scheme" instead.

> +
> +	"hamming_code_sw"		1-bit Hamming ECC
> +					- ECC calculation in software
> +					- Error detection in software
> +
> +	"hamming_code_hw"		1-bit Hamming ECC
> +					- ECC calculation in hardware
> +					- Error detection in software

Bzzt. Same here. It really doesn't matter to the hardware if the ECC is
calculated in HW or SW, and it doesn't belong in the device tree.
> +
> +	"hamming_code_hw_romcode"	1-bit Hamming ECC
> +					- ECC calculation in hardware
> +					- Error detection in software
> +					- ECC layout compatible to ROM code
> +
> +	"bch8_hw_code_detection_sw"	8-bit BCH ECC
> +					- ECC calculation in hardware
> +					- Error detection in software
> +					- depends on CONFIG_MTD_NAND_ECC_BCH

Configuration options don't belong in here either.

> +
> +	"bch8_code_hw"             	8-bit BCH ECC
> +					- ECC calculation in hardware
> +					- Error detection in hardware
> +					- depends on CONFIG_MTD_NAND_OMAP_BCH
> +					- requires <elm_id> to be specified

Some of the above clearly shouldn't be described in the device tree at all, but
it's also not very convenient to describe them as strings. Since you have
a binding, it's probably easier to give them integer values and just define
what those mean.

> +
> +
> + - elm_id:			Specifies elm device node. This is required to
> +				support some BCH ECC schemes mentioned above.

Use dashes instead of underscores for properties. if all other properties are
prepended with "ti,", then so should this.

What's an ELM device node? How is it specified? A phandle?



-Olof
pekon gupta - Aug. 22, 2013, 7:56 a.m.
> 
> On Sun, Jul 14, 2013 at 02:24:48AM +0530, Pekon Gupta wrote:
> > ECC scheme on NAND devices can be implemented in multiple ways.Some
> using
> > Software algorithm, while others using in-build Hardware engines.
> > omap2-nand driver currently supports following flavours of ECC schemes,
> > selectable via DTB.
> >
> > +---------------------------------------+---------------+---------------+
> > | ECC scheme                            |ECC calculation|Error detection|
> > +---------------------------------------+---------------+---------------+
> > |OMAP_ECC_HAMMING_CODE_DEFAULT          |S/W            |S/W            |
> > |OMAP_ECC_HAMMING_CODE_HW               |H/W (GPMC)     |S/W            |
> > |OMAP_ECC_HAMMING_CODE_HW_ROMCODE       |H/W (GPMC)     |S/W
> |
> > +---------------------------------------+---------------+---------------+
> > |(requires CONFIG_MTD_NAND_ECC_BCH)     |               |               |
> > |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW     |H/W (GPMC)     |S/W
> |
> > +---------------------------------------+---------------+---------------+
> > |(requires CONFIG_MTD_NAND_OMAP_BCH)    |               |               |
> > |OMAP_ECC_BCH8_CODE_HW                  |H/W (GPMC)     |H/W (ELM)      |
> > +---------------------------------------+---------------+---------------+
> >
> > Selection of some ECC schemes also require enabling following Kconfig
> options.
> > This was done to optimize footprint of omap2-nand driver.
> > -Kconfig:CONFIG_MTD_NAND_ECC_BCH	enables S/W based BCH ECC
> algorithm
> > -Kconfig:CONFIG_MTD_NAND_OMAP_BCH	enables H/W based BCH ECC
> algorithm
> >
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > ---
> >  .../devicetree/bindings/mtd/gpmc-nand.txt          | 45
> ++++++++++++++++------
> >  arch/arm/mach-omap2/gpmc.c                         | 14 ++++---
> >  include/linux/platform_data/mtd-nand-omap2.h       | 22 +++++++----
> >  3 files changed, 57 insertions(+), 24 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > index df338cb..c6551b6 100644
> > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> > @@ -17,17 +17,42 @@ Required properties:
> >
> >  Optional properties:
> >
> > - - nand-bus-width: 		Set this numeric value to 16 if the hardware
> > -				is wired that way. If not specified, a bus
> > -				width of 8 is assumed.
> > + - nand-bus-width: 		Determines data-width of the connected
> device
> > +				x16 = "16"
> > +				x8  = "8" (default)
> 
> This change doesn't look like an improvement to me.
> 
[Pekon]: Accepted. I'll drop this. However, this is not a new binding.
I was just elaborating & formatting the description because code allows only
two values "16" or "8".


> > - - 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
> > -		"bch4"		4-bit BCH ecc code
> > -		"bch8"		8-bit BCH ecc code
> > + - ti,nand-ecc-opt:		Determines the ECC scheme used by driver.
> > +				It can be any of the following strings:
> 
> The device tree should define the hardware, not configure the software.
> 
> Also, if it defines the scheme then you should probably call it something like
> "ti,nand-ecc-scheme" instead.
> 
[Pekon]: Again, ti,nand-ecc-opt is not new DT binding, This was added in
	Commit bc6b1e7b86f5d8e4a6fc1c0189e64bba4077efe0
	Daniel Mack <zonque@gmail.com>
	ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND
I just expanded its options..

Also I'll try to explain how below ecc-scheme selection is linked to TI Hardware.
TI SoC uses two separate H/W engines for calculating and correcting ECC.
(a) GPMC: General Purpose Memory Controller which calculates ECC also.
(b) ELM: Error Locator Module which just locates errors in BCHx code only.

*Reason-1*: All OMAP platforms have (a) GPMC h/w engine, but some 
legacy OMAP  platforms do not have (b) ELM h/w engine. Such older 
platforms use S/W lib/bch.c libraries for ECC error detection.
Therefore in-order to keep the driver consistent for all platforms we 
needed to keep so many ECC options alive. Like below you would see
two versions of BCH8 and BCH4
- bch8_code_hw (supported on new devices with ELM h/w engine)
- bch8_code_hw_detection_sw (kept for legacy devices)

*Reason-2*: Also H/W ECC schemes have different ECC layout, which is
compatible to ROM code. Thus end-to-end NAND boot would work
only with xx_code_hw schemes only.

Hence ECC selection is dependent on H/W, hence put as DT binding.
But I agree going forward we should deprecate most of legacy options
when there is common H/W platform for all devices.


> > +
> > +	"hamming_code_sw"		1-bit Hamming ECC
> > +					- ECC calculation in software
> > +					- Error detection in software
> > +
> > +	"hamming_code_hw"		1-bit Hamming ECC
> > +					- ECC calculation in hardware
> > +					- Error detection in software
> 
> Bzzt. Same here. It really doesn't matter to the hardware if the ECC is
> calculated in HW or SW, and it doesn't belong in the device tree.

[Pekon]: Not a new bindings option just renamed, and kept for legacy
 compatibility (please refer change diff below)
> > -		"sw"		Software method (default)
	renamed to "hamming_code_sw"

> > -		"hw"		Hardware method 
	renamed to "hamming_code_hw"


> > +
> > +	"hamming_code_hw_romcode"	1-bit Hamming ECC
> > +					- ECC calculation in hardware
> > +					- Error detection in software
> > +					- ECC layout compatible to ROM code
> > +
> > +	"bch8_hw_code_detection_sw"	8-bit BCH ECC
> > +					- ECC calculation in hardware
> > +					- Error detection in software
> > +					- depends on
> CONFIG_MTD_NAND_ECC_BCH
> 
> Configuration options don't belong in here either.
> 
[Pekon]: As explained above, legacy platforms do not have ELM h/w.
So they depend on lib/bch.c libraries for ECC detection. But that bloats
the driver code-footprint a lot, just to maintain compatibility for legacy
device platforms. Therefore I had to use KConfigs to keep driver
code-footprint small.

I can remove KConfig from documentation, but then it would confuse
the users when they get NAND probing errors because:
- bchx_hw_code requires ELM h/w engine, which is not present on 
	legacy devices.
-  bchx_hw_code_detection_sw requires lib/bch.c which is only
	 enabled by CONFIG_MTD_NAND_ECC_BCH.

Will removing KConfig from DT Documentation be acceptable ?

> > +
> > +	"bch8_code_hw"             	8-bit BCH ECC
> > +					- ECC calculation in hardware
> > +					- Error detection in hardware
> > +					- depends on
> CONFIG_MTD_NAND_OMAP_BCH
> > +					- requires <elm_id> to be specified
> 
> Some of the above clearly shouldn't be described in the device tree at all, but
> it's also not very convenient to describe them as strings. Since you have
> a binding, it's probably easier to give them integer values and just define
> what those mean.
> 
[Pekon]: Do you mean something like below ?
ti,nand-ecc-scheme(n)  where 'n' means
	0 ==  1-bit Hamming in S/W
	1 ==  1-bit Hamming in H/W
	2 ==  BCH4 with S/W detection
	3 ==  BCH4 all in H/W
	4 ==  BCH8 with S/W detection
	5 ==  BCH8 all in H/W

> > +
> > +
> > + - elm_id:			Specifies elm device node. This is required to
> > +				support some BCH ECC schemes mentioned
> above.
> 
> Use dashes instead of underscores for properties. if all other properties are
> prepended with "ti,", then so should this.
> 
[Pekon]: Accepted. But again this is not new DT binding. It was added in
	Commit 97c794a1e37b1ca128ef38f17c069186bfa5fb1b
	Philip Avinash <avinashphilip@ti.com>
	gpmc: Add device tree documentation for elm handle

> What's an ELM device node? How is it specified? A phandle?
> 
[Pekon]: ELM is a DT node specified in soc.dtsi (because ELM is not
 present in  legacy devices). Example Please refer:
$KERNEL/arch/arm/boot/dts/am33xx.dtsi


> 
> 
> -Olof

[Pekon]: Thanks much for review.
I have accepted some feedbacks, and given reasons for others..
In-case you are convinced, I'll send another version of patch with
all feedbacks taken in. 
So, request you to re-review based on reasons mentioned.


with regards, pekon
Olof Johansson - Aug. 27, 2013, 5:04 p.m.
On Thu, Aug 22, 2013 at 07:56:57AM +0000, Gupta, Pekon wrote:
> > This change doesn't look like an improvement to me.
> > 
> [Pekon]: Accepted. I'll drop this. However, this is not a new binding.
> I was just elaborating & formatting the description because code allows only
> two values "16" or "8".

No need to prefix your replies with [Pekon]. Based on quotation level it's easy
to see who said what.

> > > - - 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
> > > -		"bch4"		4-bit BCH ecc code
> > > -		"bch8"		8-bit BCH ecc code
> > > + - ti,nand-ecc-opt:		Determines the ECC scheme used by driver.
> > > +				It can be any of the following strings:
> > 
> > The device tree should define the hardware, not configure the software.
> > 
> > Also, if it defines the scheme then you should probably call it something like
> > "ti,nand-ecc-scheme" instead.
> > 
> [Pekon]: Again, ti,nand-ecc-opt is not new DT binding, This was added in
> 	Commit bc6b1e7b86f5d8e4a6fc1c0189e64bba4077efe0
> 	Daniel Mack <zonque@gmail.com>
> 	ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND
> I just expanded its options..

Yes, but it's not describing hardware.

If anything, the device entry should somehow describe the various ecc options
that the hardware implements (if you can't derive that from the compatible
value, which I think you can?).

> Also I'll try to explain how below ecc-scheme selection is linked to TI Hardware.
> TI SoC uses two separate H/W engines for calculating and correcting ECC.
> (a) GPMC: General Purpose Memory Controller which calculates ECC also.
> (b) ELM: Error Locator Module which just locates errors in BCHx code only.
> 
> *Reason-1*: All OMAP platforms have (a) GPMC h/w engine, but some 
> legacy OMAP  platforms do not have (b) ELM h/w engine. Such older 
> platforms use S/W lib/bch.c libraries for ECC error detection.

Ok, so then you just need a binary "elm-engine" property to indicate
that the hardware does have the engine.

> Therefore in-order to keep the driver consistent for all platforms we 
> needed to keep so many ECC options alive. Like below you would see
> two versions of BCH8 and BCH4
> - bch8_code_hw (supported on new devices with ELM h/w engine)
> - bch8_code_hw_detection_sw (kept for legacy devices)

All you need to specify is what ECC format is used. I.e. BCH8. Whether the
hardware or software will be used to calculate the checksums and detect/correct
errors is a driver decision, and not something that the device tree needs to
specify.

> *Reason-2*: Also H/W ECC schemes have different ECC layout, which is
> compatible to ROM code. Thus end-to-end NAND boot would work
> only with xx_code_hw schemes only.

So you should describe what the layout in use is. Wouldn't it be
possible to make the software handle the same layout as the hardware
engine if needed? I.e. the decision of using HW or SW is not a property
of the hardware and shouldn't be described in the device tree.

> Hence ECC selection is dependent on H/W, hence put as DT binding.
> But I agree going forward we should deprecate most of legacy options
> when there is common H/W platform for all devices.
> 
> 
> > > +
> > > +	"hamming_code_sw"		1-bit Hamming ECC
> > > +					- ECC calculation in software
> > > +					- Error detection in software
> > > +
> > > +	"hamming_code_hw"		1-bit Hamming ECC
> > > +					- ECC calculation in hardware
> > > +					- Error detection in software
> > 
> > Bzzt. Same here. It really doesn't matter to the hardware if the ECC is
> > calculated in HW or SW, and it doesn't belong in the device tree.
> 
> [Pekon]: Not a new bindings option just renamed, and kept for legacy
>  compatibility (please refer change diff below)

Same arguments as above.

> > > -		"sw"		Software method (default)
> 	renamed to "hamming_code_sw"
> 
> > > -		"hw"		Hardware method 
> 	renamed to "hamming_code_hw"
> 
> 
> > > +
> > > +	"hamming_code_hw_romcode"	1-bit Hamming ECC
> > > +					- ECC calculation in hardware
> > > +					- Error detection in software
> > > +					- ECC layout compatible to ROM code
> > > +
> > > +	"bch8_hw_code_detection_sw"	8-bit BCH ECC
> > > +					- ECC calculation in hardware
> > > +					- Error detection in software
> > > +					- depends on
> > CONFIG_MTD_NAND_ECC_BCH
> > 
> > Configuration options don't belong in here either.
> > 
> [Pekon]: As explained above, legacy platforms do not have ELM h/w.
> So they depend on lib/bch.c libraries for ECC detection. But that bloats
> the driver code-footprint a lot, just to maintain compatibility for legacy
> device platforms. Therefore I had to use KConfigs to keep driver
> code-footprint small.
>
> I can remove KConfig from documentation, but then it would confuse
> the users when they get NAND probing errors because:
> - bchx_hw_code requires ELM h/w engine, which is not present on 
> 	legacy devices.
> -  bchx_hw_code_detection_sw requires lib/bch.c which is only
> 	 enabled by CONFIG_MTD_NAND_ECC_BCH.
> 
> Will removing KConfig from DT Documentation be acceptable ?

Yes, please remove it. Bindings documentation should not document kernel config
options.

> > > +
> > > +	"bch8_code_hw"             	8-bit BCH ECC
> > > +					- ECC calculation in hardware
> > > +					- Error detection in hardware
> > > +					- depends on
> > CONFIG_MTD_NAND_OMAP_BCH
> > > +					- requires <elm_id> to be specified
> > 
> > Some of the above clearly shouldn't be described in the device tree at all, but
> > it's also not very convenient to describe them as strings. Since you have
> > a binding, it's probably easier to give them integer values and just define
> > what those mean.
> > 
> [Pekon]: Do you mean something like below ?
> ti,nand-ecc-scheme(n)  where 'n' means
> 	0 ==  1-bit Hamming in S/W
> 	1 ==  1-bit Hamming in H/W
> 	2 ==  BCH4 with S/W detection
> 	3 ==  BCH4 all in H/W
> 	4 ==  BCH8 with S/W detection
> 	5 ==  BCH8 all in H/W

Well, see above -- I think you can just describe the properties of the hardware
and make the driver pick appropriate setup based on it.

> > > + - elm_id:			Specifies elm device node. This is required to
> > > +				support some BCH ECC schemes mentioned
> > above.
> > 
> > Use dashes instead of underscores for properties. if all other properties are
> > prepended with "ti,", then so should this.
> > 
> [Pekon]: Accepted. But again this is not new DT binding. It was added in
> 	Commit 97c794a1e37b1ca128ef38f17c069186bfa5fb1b
> 	Philip Avinash <avinashphilip@ti.com>
> 	gpmc: Add device tree documentation for elm handle

Since you are revising the binding, this is the time to change it.

> > What's an ELM device node? How is it specified? A phandle?
> > 
> [Pekon]: ELM is a DT node specified in soc.dtsi (because ELM is not
>  present in  legacy devices). Example Please refer:
> $KERNEL/arch/arm/boot/dts/am33xx.dtsi

What I meant with a question like that is: Please document how it's specified.
It's not clear from reading it, since I had to ask.



-Olof
pekon gupta - Sept. 12, 2013, 11:57 a.m.
> On Thu, Aug 22, 2013 at 07:56:57AM +0000, Gupta, Pekon wrote:

> If anything, the device entry should somehow describe the various ecc
> options
> that the hardware implements (if you can't derive that from the compatible
> value, which I think you can?).
> 
> > Also I'll try to explain how below ecc-scheme selection is linked to TI
> Hardware.
> > TI SoC uses two separate H/W engines for calculating and correcting ECC.
> > (a) GPMC: General Purpose Memory Controller which calculates ECC also.
> > (b) ELM: Error Locator Module which just locates errors in BCHx code only.
> >
> > *Reason-1*: All OMAP platforms have (a) GPMC h/w engine, but some
> > legacy OMAP  platforms do not have (b) ELM h/w engine. Such older
> > platforms use S/W lib/bch.c libraries for ECC error detection.
> 
> Ok, so then you just need a binary "elm-engine" property to indicate
> that the hardware does have the engine.
> 
> > Therefore in-order to keep the driver consistent for all platforms we
> > needed to keep so many ECC options alive. Like below you would see
> > two versions of BCH8 and BCH4
> > - bch8_code_hw (supported on new devices with ELM h/w engine)
> > - bch8_code_hw_detection_sw (kept for legacy devices)
> 
> All you need to specify is what ECC format is used. I.e. BCH8. Whether the
> hardware or software will be used to calculate the checksums and
> detect/correct
> errors is a driver decision, and not something that the device tree needs to
> specify.
> 
> > *Reason-2*: Also H/W ECC schemes have different ECC layout, which is
> > compatible to ROM code. Thus end-to-end NAND boot would work
> > only with xx_code_hw schemes only.
> 
> So you should describe what the layout in use is. Wouldn't it be
> possible to make the software handle the same layout as the hardware
> engine if needed? I.e. the decision of using HW or SW is not a property
> of the hardware and shouldn't be described in the device tree.
> 

Sorry I was caught up in some other priority work, therefore could not
update this. But I have recently posted another version with your feedbacks
incorporated. Please review them, whenever possible.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048611.html

Patch series at: 
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048613.html


Thank you 
With regards, pekon

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index df338cb..c6551b6 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -17,17 +17,42 @@  Required properties:
 
 Optional properties:
 
- - nand-bus-width: 		Set this numeric value to 16 if the hardware
-				is wired that way. If not specified, a bus
-				width of 8 is assumed.
+ - nand-bus-width: 		Determines data-width of the connected device
+				x16 = "16"
+				x8  = "8" (default)
 
- - 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
-		"bch4"		4-bit BCH ecc code
-		"bch8"		8-bit BCH ecc code
+ - ti,nand-ecc-opt:		Determines the ECC scheme used by driver.
+				It can be any of the following strings:
+
+	"hamming_code_sw"		1-bit Hamming ECC
+					- ECC calculation in software
+					- Error detection in software
+
+	"hamming_code_hw"		1-bit Hamming ECC
+					- ECC calculation in hardware
+					- Error detection in software
+
+	"hamming_code_hw_romcode"	1-bit Hamming ECC
+					- ECC calculation in hardware
+					- Error detection in software
+					- ECC layout compatible to ROM code
+
+	"bch8_hw_code_detection_sw"	8-bit BCH ECC
+					- ECC calculation in hardware
+					- Error detection in software
+					- depends on CONFIG_MTD_NAND_ECC_BCH
+
+	"bch8_code_hw"             	8-bit BCH ECC
+					- ECC calculation in hardware
+					- Error detection in hardware
+					- depends on CONFIG_MTD_NAND_OMAP_BCH
+					- requires <elm_id> to be specified
+
+
+ - elm_id:			Specifies elm device node. This is required to
+				support some BCH ECC schemes mentioned above.
+
 
  - ti,nand-xfer-type:		A string setting the data transfer type. One of:
 
@@ -36,8 +61,6 @@  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.
 
 For inline partiton table parsing (optional):
 
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1c7969e..e19de21 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1342,11 +1342,13 @@  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_BCH4_CODE_HW]			= "bch4",
-	[OMAP_ECC_BCH8_CODE_HW]			= "bch8",
+	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "hamming_code_sw",
+	[OMAP_ECC_HAMMING_CODE_HW]		= "hamming_code_hw",
+	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hamming_code_hw_romcode",
+	[OMAP_ECC_BCH4_CODE_HW]			= "bch4_code_hw",
+	[OMAP_ECC_BCH4_CODE_HW_DETECTION_SW]	= "bch4_code_hw_detection_sw",
+	[OMAP_ECC_BCH8_CODE_HW]			= "bch8_code_hw",
+	[OMAP_ECC_BCH8_CODE_HW_DETECTION_SW]	= "bch8_code_hw_detection_sw"
 };
 
 static const char * const nand_xfer_types[] = {
@@ -1380,7 +1382,7 @@  static int gpmc_probe_nand_child(struct platform_device *pdev,
 
 	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])) {
+			if (!strcmp(s, nand_ecc_opts[val])) {
 				gpmc_nand_data->ecc_opt = val;
 				break;
 			}
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 6bf9ef4..ce74576 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -23,13 +23,21 @@  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_BCH4_CODE_HW, /* 4-bit BCH ecc code */
-	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
+	/* 1-bit  ECC calculation by Software, Error detection by Software */
+	OMAP_ECC_HAMMING_CODE_DEFAULT = 0,
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_HAMMING_CODE_HW,
+	/* 1-bit  ECC calculation by GPMC, Error detection by Software */
+	/* ECC layout compatible to legacy ROMCODE. */
+	OMAP_ECC_HAMMING_CODE_HW_ROMCODE,
+	/* 4-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH4_CODE_HW,
+	/* 4-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH4_CODE_HW_DETECTION_SW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by ELM */
+	OMAP_ECC_BCH8_CODE_HW,
+	/* 8-bit  ECC calculation by GPMC, Error detection by Software */
+	OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
 };
 
 struct gpmc_nand_regs {