Patchwork [4/4,v2] ARM: OMAP2+: updated ECC scheme attributes for omap2-nand DT

login
register
mail settings
Submitter pekon gupta
Date May 17, 2013, 11:50 a.m.
Message ID <1368791418-14330-5-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/244619/
State Not Applicable
Headers show

Comments

pekon gupta - May 17, 2013, 11:50 a.m.
From: "Gupta, Pekon" <pekon@ti.com>

Updates ECC scheme selection string same to same as used in omap2-driver code.
This makes the DT configurations easy to understand and map to actual code.

Signed-off-by: Gupta, Pekon <pekon@ti.com>
---
 .../devicetree/bindings/mtd/gpmc-nand.txt          | 47 ++++++++++++++--------
 arch/arm/boot/dts/am335x-evm.dts                   |  2 +-
 arch/arm/mach-omap2/gpmc.c                         | 19 +++++----
 3 files changed, 43 insertions(+), 25 deletions(-)
Arnd Bergmann - May 17, 2013, 12:46 p.m.
On Friday 17 May 2013, Gupta, Pekon wrote:
> From: "Gupta, Pekon" <pekon@ti.com>
> 
> Updates ECC scheme selection string same to same as used in omap2-driver code.
> This makes the DT configurations easy to understand and map to actual code.
> 
> Signed-off-by: Gupta, Pekon <pekon@ti.com>

This moves the binding in the wrong direction. First of all, you should never
make incompatible changes to a specification document.

> -	"bch8_hw_detection_sw" 	8-bit BCH with ECC calculation in hardware
> -				and error detection in software
> -				- requires Kconfig CONFIG_MTD_NAND_ECC_BCH

The binding before your change is already broken since it refers to
Linux-specific Kconfig symbols, and you fail to fix that.

> +				"OMAP_ECC_BCH4_CODE_HW_DETECTION_SW"
> +					4-bit BCH with ECC calculation in
> +					hardware & error detection in software.
> +					- requires CONFIG_MTD_NAND_ECC_BCH

Instead you make it worse by using /more/ Linux-isms in the binding.

	Arnd
pekon gupta - May 18, 2013, 11:05 a.m.
> On Friday 17 May 2013, Gupta, Pekon wrote:
> > From: "Gupta, Pekon" <pekon@ti.com>
> >
> > Updates ECC scheme selection string same to same as used in omap2-
> driver code.
> > This makes the DT configurations easy to understand and map to actual
> code.
> >
> > Signed-off-by: Gupta, Pekon <pekon@ti.com>
> 
> This moves the binding in the wrong direction. First of all, you should
> never
> make incompatible changes to a specification document.
> 
> > -	"bch8_hw_detection_sw" 	8-bit BCH with ECC calculation in
> hardware
> > -				and error detection in software
> > -				- requires Kconfig
> CONFIG_MTD_NAND_ECC_BCH
> 
> The binding before your change is already broken since it refers to
> Linux-specific Kconfig symbols, and you fail to fix that.
> 
[Pekon]: There are two constrains due to which I could not remove
Kconfig dependency
(1) Some BCHx ECC schemes are using generic lib/bch.h and nand_bch.h 
generic libraries, which further depend on 'CONFIG_MTD_NAND_ECC_BCH'.
So I have to include that dependency, so that incorrect ECC scheme 
selection In DT does not break linker.

(2) Other alternative could have been to always include lib/bch.h and 
nand_bch.h. But that would have bloated the omap2-driver footprint,
especially when 80% of the time these libraries were not getting used.

(3) I did not want to change things so drastically that it breaks on legacy
devices. Some users are now migrating to latest kernel from older 
versions. I din't want to make migration more tough :-)

However, I tried to fix some existing issues due to incorrect selection of
Kconfigs, and I tried making things simpler from user's point-of-view.

Like, if you don't select a correct Kconfig along with matching ECC scheme
Then your device_initialization would cleanly exit giving you msg:
pr_err("selected ECC scheme not supported or not enabled\n");


when running the linker.> > +
> 	"OMAP_ECC_BCH4_CODE_HW_DETECTION_SW"
> > +					4-bit BCH with ECC calculation in
> > +					hardware & error detection in
> software.
> > +					- requires
> CONFIG_MTD_NAND_ECC_BCH
> 
> Instead you make it worse by using /more/ Linux-isms in the binding.
> 
> 	Arnd

[Pekon]: The update binding attribute string is done for following reasons:
(1) make it more user-friendly. The string you put in the DT is same, 
You see while browsing thru the driver code.
My assessment was that most mainline user are much tech-savy 
they themselves try to dig into the code and analyze the issue.
Thus for them, reducing the gap between driver nomenclature and 
DT bindings was my way of thinking.

(2) prefix of 'OMAP_ECC_xx' was required because I wanted to 
Differentiate Some ECC schemes implementations from the ones 
already present in generic driver (nand_base.c)
Example: there are following favours of same BCH-ECC scheme now..
- NAND_ECC_SOFT_BCH:  purely S/W based ECC scheme
- OMAP_ECC_BCH4_HW_DETECTION_SW: partly S/w partly H/W based
- OMAP_ECC_BCH4_MODE_HW: fully H/W based ECC scheme.

Anyway these are just my way of thinking.. 
If you have some suggestions, in following a particular nomenclature
OR particular way of streamlining the driver, please elaborate..
Any feedbacks would help to improve..


With regards, pekon

Patch

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
index de180be..c7a4add 100644
--- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -25,22 +25,37 @@  Optional properties:
  - 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
-
+				"OMAP_ECC_HAMMING_CODE_SW"
+					1-bit Hamming ECC using software.
+
+				"OMAP_ECC_HAMMING_CODE_HW"
+					1-bit Hamming ECC using hardware.
+
+				"OMAP_ECC_HAMMING_CODE_HW_ROMCODE"
+					1-bit Hamming ECC using hardware with
+					ECC layout compatible to ROM code.
+
+				"OMAP_ECC_BCH4_CODE_HW_DETECTION_SW"
+					4-bit BCH with ECC calculation in
+					hardware & error detection in software.
+					- requires CONFIG_MTD_NAND_ECC_BCH
+
+				"OMAP_ECC_BCH4_CODE_HW"
+					4-bit BCH with ECC calculation in
+					hardware & error detection in hardware.
+					- requires CONFIG_MTD_NAND_OMAP_BCH
+					- requires <elm_id> to be specified
+
+				"OMAP_ECC_BCH8_CODE_HW_DETECTION_SW"
+					8-bit BCH with ECC calculation in
+					hardware & error detection in software.
+					- requires CONFIG_MTD_NAND_ECC_BCH
+
+				"OMAP_ECC_BCH8_CODE_HW"
+					8-bit BCH with ECC calculation in
+					hardware & error detection in hardware.
+					- requires CONFIG_MTD_NAND_OMAP_BCH
+					- requires <elm_id> to be specified
 
 
  - elm_id:			Specifies elm device node. This is required to
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 60e8f59..1107761 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_hw";
+				ti,nand-ecc-opt = "OMAP_ECC_BCH8_CODE_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 03b8027..e3146c3 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1205,13 +1205,16 @@  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]		= "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"
+	[OMAP_ECC_HAMMING_CODE_DEFAULT]		= "OMAP_ECC_HAMMING_CODE_SW",
+	[OMAP_ECC_HAMMING_CODE_HW]		= "OMAP_ECC_HAMMING_CODE_HW",
+	[OMAP_ECC_HAMMING_CODE_HW_ROMCODE]	=
+					"OMAP_ECC_HAMMING_CODE_HW_ROMCODE",
+	[OMAP_ECC_BCH4_CODE_HW_DETECTION_SW]	=
+					"OMAP_ECC_BCH4_CODE_HW_DETECTION_SW",
+	[OMAP_ECC_BCH4_CODE_HW]			= "OMAP_ECC_BCH4_CODE_HW",
+	[OMAP_ECC_BCH8_CODE_HW_DETECTION_SW]	=
+					"OMAP_ECC_BCH8_CODE_HW_DETECTION_SW",
+	[OMAP_ECC_BCH8_CODE_HW]			= "OMAP_ECC_BCH8_CODE_HW"
 };
 
 static int gpmc_probe_nand_child(struct platform_device *pdev,
@@ -1238,7 +1241,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;
 			}