> > 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@xxxxxx> > > --- > > .../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@xxxxxxxxx> 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@xxxxxx> 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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f