RE: [PATCH v5 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 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�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f





[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux