| Message ID | 1368791418-14330-5-git-send-email-pekon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
> 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 -- 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
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; }