[PATCH v5 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
Gupta, Pekon
pekon at ti.com
Thu Aug 22 03:56:57 EDT 2013
>
> 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 at 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 at 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 at 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
More information about the linux-mtd
mailing list