diff mbox

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

Message ID 1368684720-26763-3-git-send-email-pekon@ti.com
State Not Applicable
Headers show

Commit Message

pekon gupta May 16, 2013, 6:12 a.m. UTC 
From: "Gupta, Pekon" <pekon@ti.com>

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: Gupta, Pekon <pekon@ti.com>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          | 37 +++++++++++++++-------
 arch/arm/boot/dts/am335x-evm.dts                   |  2 +-
 arch/arm/mach-omap2/gpmc.c                         | 12 ++++---
 3 files changed, 34 insertions(+), 17 deletions(-)

Comments

jean-philippe francois May 16, 2013, 7:26 a.m. UTC | #1 
2013/5/16 Gupta, Pekon <pekon@ti.com>:
> From: "Gupta, Pekon" <pekon@ti.com>
>
> 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
>

OMAP_ECC_BCH4_CODE_HW_DETECTION_SW and  OMAP_ECC_BCH4_CODE_HW
seems to exist in the code, but are not in the changelog, and not in
the device tree binding documentation.


> Signed-off-by: Gupta, Pekon <pekon@ti.com>
> ---
>  .../devicetree/bindings/mtd/gpmc-nand.txt          | 37 +++++++++++++++-------
>  arch/arm/boot/dts/am335x-evm.dts                   |  2 +-
>  arch/arm/mach-omap2/gpmc.c                         | 12 ++++---
>  3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> index e7f8d7e..de180be 100644
> --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -17,20 +17,35 @@ 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_sw"            1-bit Hamming ECC using software
> +
> +       "hamming_hw"            1-bit Hamming ECC using hardware
> +
> +       "hamming_hw_romcode"    1-bit Hamming ECC using hardware
> +                               - ECC layout compatible to ROM code
> +
> +       "bch8_hw_detection_sw"  8-bit BCH with ECC calculation in hardware
> +                               and error detection in software
> +                               - requires Kconfig CONFIG_MTD_NAND_ECC_BCH
> +
> +       "bch8_hw"               8-bit BCH with ECC calculation in hardware
> +                               and error detection in hardware
> +                               - requires <elm_id> to be specified
> +                               - requires Kconfig CONFIG_MTD_NAND_OMAP_BCH
> +
> +
> +
> + - elm_id:                     Specifies elm device node. This is required to
> +                               support some BCH ECC schemes mentioned above.
>
> - - 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/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index 0b8f161..60e8f59 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -135,7 +135,7 @@
>                         nand@0,0 {
>                                 reg = <0 0 0>; /* CS0, offset 0 */
>                                 nand-bus-width = <8>;
> -                               ti,nand-ecc-opt = "bch8";
> +                               ti,nand-ecc-opt = "bch8_hw";
>
>                                 gpmc,sync-clk = <0>;
>                                 gpmc,cs-on = <0>;
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 410e1ba..03b8027 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1205,11 +1205,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_sw",
> +       [OMAP_ECC_HAMMING_CODE_HW]              = "hamming_hw",
> +       [OMAP_ECC_HAMMING_CODE_HW_ROMCODE]      = "hamming_hw_romcode",
> +       [OMAP_ECC_BCH4_CODE_HW]                 = "bch4_hw",
> +       [OMAP_ECC_BCH4_CODE_HW_DETECTION_SW]    = "bch4_hw_detection_sw",
> +       [OMAP_ECC_BCH8_CODE_HW]                 = "bch8_hw",
> +       [OMAP_ECC_BCH8_CODE_HW_DETECTION_SW]    = "bch8_hw_detection_sw"
>  };
>
>  static int gpmc_probe_nand_child(struct platform_device *pdev,
> --
> 1.8.1
>
> --
> 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
pekon gupta May 16, 2013, 7:52 a.m. UTC | #2 
> >
> 
> OMAP_ECC_BCH4_CODE_HW_DETECTION_SW and
> OMAP_ECC_BCH4_CODE_HW
> seems to exist in the code, but are not in the changelog, and not in
> the device tree binding documentation.
> 

Yes, I plan to omit them from code also, in next series as it does not
make sense to support both BCH4 and BCH8 at same time, when most
users would opt for BCH8.
Also, BCH4 was kept for legacy purposes, and was not tested on all devices.
Therefore I have purposely omitted it from documentation.

with regards, pekon
jean-philippe francois May 16, 2013, 8:29 a.m. UTC | #3 
2013/5/16 Gupta, Pekon <pekon@ti.com>:
>> >
>>
>> OMAP_ECC_BCH4_CODE_HW_DETECTION_SW and
>> OMAP_ECC_BCH4_CODE_HW
>> seems to exist in the code, but are not in the changelog, and not in
>> the device tree binding documentation.
>>
>
> Yes, I plan to omit them from code also, in next series as it does not
> make sense to support both BCH4 and BCH8 at same time, when most
> users would opt for BCH8.
> Also, BCH4 was kept for legacy purposes, and was not tested on all devices.
> Therefore I have purposely omitted it from documentation.
>

We have shipped devices with BCH4 nand.  This would be a regression for us.

Regards,
Jean-Philippe François.

> with regards, pekon
pekon gupta May 16, 2013, 8:51 a.m. UTC | #4 
> 2013/5/16 Gupta, Pekon <pekon@ti.com>:

> >> >

> >>

> >> OMAP_ECC_BCH4_CODE_HW_DETECTION_SW and

> >> OMAP_ECC_BCH4_CODE_HW

> >> seems to exist in the code, but are not in the changelog, and not in

> >> the device tree binding documentation.

> >>

> >

> > Yes, I plan to omit them from code also, in next series as it does not

> > make sense to support both BCH4 and BCH8 at same time, when most

> > users would opt for BCH8.

> > Also, BCH4 was kept for legacy purposes, and was not tested on all

> devices.

> > Therefore I have purposely omitted it from documentation.

> >

> 

> We have shipped devices with BCH4 nand.  This would be a regression

> for us.

> 

[Pekon]: May I know the following details so that I can prioritize BCH4 testing
- Which TI device have you productized ?
- Which kernel version you are using ? (Is it from mainline or SDK release)
- Which BCH4 ECC implementation you are using ?
	BCH4_HW (using both GPMC and ELM H/W engines)
	BCH4_HW_DETECTION_SW (using GPMC H/W and bch.h S/W libraries)
- Is there a specific reason why 	you opted for BCH4 instead of BCH8 ?
	(Though its only recent that OMAP_ECC_BCHx support is mainlined 
	But, BCH8 support was available in TI SDK releases from quite sometime.)

I'll try to see if I can help you here, but going forward its always recommended 
to use higher ECC schemes (like BCH8), so that flash's lifetime is extended on field.


> Regards,

> Jean-Philippe François.

> 

with regards, pekon
jean-philippe francois May 16, 2013, 9:34 a.m. UTC | #5 
2013/5/16 Gupta, Pekon <pekon@ti.com>:
>
>> 2013/5/16 Gupta, Pekon <pekon@ti.com>:
>> >> >
>> >>
>> >> OMAP_ECC_BCH4_CODE_HW_DETECTION_SW and
>> >> OMAP_ECC_BCH4_CODE_HW
>> >> seems to exist in the code, but are not in the changelog, and not in
>> >> the device tree binding documentation.
>> >>
>> >
>> > Yes, I plan to omit them from code also, in next series as it does not
>> > make sense to support both BCH4 and BCH8 at same time, when most
>> > users would opt for BCH8.
>> > Also, BCH4 was kept for legacy purposes, and was not tested on all
>> devices.
>> > Therefore I have purposely omitted it from documentation.
>> >
>>
>> We have shipped devices with BCH4 nand.  This would be a regression
>> for us.
>>
> [Pekon]: May I know the following details so that I can prioritize BCH4 testing
> - Which TI device have you productized ?

dm3730

> - Which kernel version you are using ? (Is it from mainline or SDK release)

3.6.11, currently moving to 3.9

> - Which BCH4 ECC implementation you are using ?
>         BCH4_HW (using both GPMC and ELM H/W engines)
>         BCH4_HW_DETECTION_SW (using GPMC H/W and bch.h S/W libraries)

I guess it is BCH4_HW_DETECTION_SW. Here is the relevant part of the
machine board file patch :

+static struct omap_nand_platform_data cydm_nand_data = {
+     .cs = 0,
+     .ecc_opt = OMAP_ECC_BCH4_CODE_HW,
+};



> - Is there a specific reason why        you opted for BCH4 instead of BCH8 ?
>         (Though its only recent that OMAP_ECC_BCHx support is mainlined
>         But, BCH8 support was available in TI SDK releases from quite sometime.)
>

No, I figured BCH4 was sufficient for my nand. In fact my previous ecc
scheme (1 bit hamming) was not
offering enough protection. I started investigating bch, and read that
while the ti dvsdk bch code
for computing the bch was ok, the error correction code was not, and
the new BCH4 scheme
resulted in all FF OOB data for an empty page, which was great. There
was no real reason for
choosing BCH4 over BCH8, but I thought the simpler the better.

> I'll try to see if I can help you here, but going forward its always recommended
> to use higher ECC schemes (like BCH8), so that flash's lifetime is extended on field.
>

>
>> Regards,
>> Jean-Philippe François.
>>
> with regards, pekon
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index e7f8d7e..de180be 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -17,20 +17,35 @@  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_sw"          	1-bit Hamming ECC using software
+
+	"hamming_hw"          	1-bit Hamming ECC using hardware
+
+	"hamming_hw_romcode"  	1-bit Hamming ECC using hardware
+				- ECC layout compatible to ROM code
+
+	"bch8_hw_detection_sw" 	8-bit BCH with ECC calculation in hardware
+				and error detection in software
+				- requires Kconfig CONFIG_MTD_NAND_ECC_BCH
+
+	"bch8_hw"             	8-bit BCH with ECC calculation in hardware
+				and error detection in hardware
+				- requires <elm_id> to be specified
+				- requires Kconfig CONFIG_MTD_NAND_OMAP_BCH
+
+
+
+ - elm_id:			Specifies elm device node. This is required to
+				support some BCH ECC schemes mentioned above.
 
- - 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/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 0b8f161..60e8f59 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -135,7 +135,7 @@ 
 			nand@0,0 {
 				reg = <0 0 0>; /* CS0, offset 0 */
 				nand-bus-width = <8>;
-				ti,nand-ecc-opt = "bch8";
+				ti,nand-ecc-opt = "bch8_hw";
 
 				gpmc,sync-clk = <0>;
 				gpmc,cs-on = <0>;
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 410e1ba..03b8027 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1205,11 +1205,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_sw",
+	[OMAP_ECC_HAMMING_CODE_HW]		= "hamming_hw",
+	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	= "hamming_hw_romcode",
+	[OMAP_ECC_BCH4_CODE_HW]			= "bch4_hw",
+	[OMAP_ECC_BCH4_CODE_HW_DETECTION_SW]	= "bch4_hw_detection_sw",
+	[OMAP_ECC_BCH8_CODE_HW]			= "bch8_hw",
+	[OMAP_ECC_BCH8_CODE_HW_DETECTION_SW]	= "bch8_hw_detection_sw"
 };
 
 static int gpmc_probe_nand_child(struct platform_device *pdev,