Patchwork [v7,1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes

login
register
mail settings
Submitter pekon gupta
Date Oct. 4, 2013, 7:49 p.m.
Message ID <1380916188-24206-2-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/280722/
State New
Headers show

Comments

pekon gupta - Oct. 4, 2013, 7:49 p.m.
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>
---
 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(-)
Mark Rutland - Oct. 7, 2013, 11:27 a.m.
On Fri, Oct 04, 2013 at 08:49:43PM +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.

If I have my NAND formatted with one of the existing ECC schemes (e.g.
OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new
OMAP_ECC_HAM1_CODE_HW scheme?

Are they all compatible?

> 
> Signed-off-by: Pekon Gupta <pekon@ti.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 9f4795a..1c45b72 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",

Won't this break existing dts which have "sw", "hw", or "hw-romcode"?

Someone may try to use a new kernel with an old dt, and we marked them
as deprecated, not removed.

Thanks,
Mark.
pekon gupta - Oct. 7, 2013, 11:42 a.m.
Hi,

> From: Mark Rutland [mailto:mark.rutland@arm.com]
> > On Fri, Oct 04, 2013 at 08:49:43PM +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.
> 
> If I have my NAND formatted with one of the existing ECC schemes (e.g.
> OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new
> OMAP_ECC_HAM1_CODE_HW scheme?
> 
> Are they all compatible?
> 
Yes, they all are 1-bit hamming code, the only difference between 
xx_Default and xx_HW was who was doing the ECC calculation.
For xx_DEFAULT: ECC calculation was done on CPU via s/w library
For xx_HW: ECC calculation was done by in-build h/w engine.
So, all HAMMING_xx can be replaced by HAM1_HW.

[snip]

> > @@ -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",
> 
> Won't this break existing dts which have "sw", "hw", or "hw-romcode"?
> 
> Someone may try to use a new kernel with an old dt, and we marked them
> as deprecated, not removed.
> 
HAMMING_xx ECC scheme was used only on legacy platforms, when
BCH8 was not available, I have not seen anyone using this scheme
*from mainline kernel* from quite a long time. So, it's safe to remove them.

This is what was concluded as per below email.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048876.html

with regards, pekon
pekon gupta - Oct. 11, 2013, 10:17 a.m.
Dear MTD Maintainers,

> >
> > If I have my NAND formatted with one of the existing ECC schemes (e.g.
> > OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new
> > OMAP_ECC_HAM1_CODE_HW scheme?
> >
> > Are they all compatible?
> >
> Yes, they all are 1-bit hamming code, the only difference between
> xx_Default and xx_HW was who was doing the ECC calculation.
> For xx_DEFAULT: ECC calculation was done on CPU via s/w library
> For xx_HW: ECC calculation was done by in-build h/w engine.
> So, all HAMMING_xx can be replaced by HAM1_HW.
> 
> [snip]
> 
> > > @@ -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",
> >
> > Won't this break existing dts which have "sw", "hw", or "hw-romcode"?
> >
> > Someone may try to use a new kernel with an old dt, and we marked them
> > as deprecated, not removed.
> >
> HAMMING_xx ECC scheme was used only on legacy platforms, when
> BCH8 was not available, I have not seen anyone using this scheme
> *from mainline kernel* from quite a long time. So, it's safe to remove them.
> 
> This is what was concluded as per below email.
> http://lists.infradead.org/pipermail/linux-mtd/2013-September/048876.html
> 

This patch-series and its follow-on series has already missed many merge
windows, And the above discussion has reached a stalled state (infinite loop)
where, I cannot revert some DT binding updates to and fro to keep all legacy
DT bindings backward compatible forever.
However, I can assure that these DT updates make binding stable for long-term.

So now it's your discretion to whether to accept or leave following 2 series:

http://lists.infradead.org/pipermail/linux-mtd/2013-October/048983.html

http://lists.infradead.org/pipermail/linux-mtd/2013-October/049008.html


AFAIK no-one is using Hamming based ecc-scheme on OMAP platforms
*from mainline kernel*. So this DT update actually does not affect users
I know of. Rather these patch series was intended for long term scalability
and clean-up so that more OMAP users migrate to mainline kernel easily.


with regards, pekon

Patch

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 9f4795a..1c45b72 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 */
 };