| Message ID | 1373748891-7779-2-git-send-email-pekon@ti.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
> > 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
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
> 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
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 {
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(-)