Re: [PATCH v3 3/4] mtd: nand: omap: optimize chip->ecc.hwctl() for H/W ECC schemes

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

On Wed, Nov 13, 2013 at 02:03:30PM -0800, Brian Norris wrote:
> On Sat, Nov 02, 2013 at 03:16:15PM +0530, Pekon Gupta wrote:
> > chip->ecc.hwctl() is used for preparing the H/W controller before read/write
> > NAND accesses (like assigning data-buf, enabling ECC scheme configs, etc.)
> > 
> > Though all ECC schemes in OMAP NAND driver use GPMC controller for generating
> > ECC syndrome (for both Read/Write accesses). But but in current code
> 
> s/But but/but/
> 
> :)
> 
> > HAM1_ECC and BCHx_ECC schemes implement individual function to achieve this.
> > This patch merges the GPMC configuration code for all ECC schemes into
> > single omap_enable_hwecc(), thus adding scalability for future ECC schemes.
> > 
> >  omap_enable_hwecc() + omap3_enable_hwecc_bch() -> omap_enable_hwecc()
> > 
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > ---
> >  drivers/mtd/nand/omap2.c | 212 +++++++++++++++++------------------------------
> >  1 file changed, 74 insertions(+), 138 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 1f59505..3a99e29 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -33,6 +33,10 @@
> >  #define	DRIVER_NAME	"omap2-nand"
> >  #define	OMAP_NAND_TIMEOUT_MS	5000
> >  
> > +#define GPMC_ECC_READ		0 /* Reset Hardware ECC for read */
> > +#define GPMC_ECC_WRITE		1 /* Reset Hardware ECC for write */
> > +#define GPMC_ECC_READSYN	2 /* Reset before syndrom is read back */
> > +
> >  #define NAND_Ecc_P1e		(1 << 0)
> >  #define NAND_Ecc_P2e		(1 << 1)
> >  #define NAND_Ecc_P4e		(1 << 2)
> > @@ -101,13 +105,9 @@
> >  #define P4o_s(a)	(TF(a & NAND_Ecc_P4o)		<< 1)
> >  
> >  #define	PREFETCH_CONFIG1_CS_SHIFT	24
> > -#define	ECC_CONFIG_CS_SHIFT		1
> 
> You stopped using this macro in a different patch, so this removal is
> unrelated to this patch. But I'm not sure the code is better without
> these macros anyway... I'll leave it to your discretion, I guess.
> 
> >  #define	CS_MASK				0x7
> >  #define	ENABLE_PREFETCH			(0x1 << 7)
> >  #define	DMA_MPU_MODE_SHIFT		2
> > -#define	ECCSIZE0_SHIFT			12
> > -#define	ECCSIZE1_SHIFT			22
> > -#define	ECC1RESULTSIZE			0x1
> >  #define	ECCCLEAR			0x100
> >  #define	ECC1				0x1
> >  #define	PREFETCH_FIFOTHRESHOLD_MAX	0x40
> 
> ...
> 
> The rest looks OK but very hard for me to verify. Do we have any
> testers? Any platforms to check for regressions?
> 

I have 8-bit and 16-bit NAND modules to test this.

Pekon: Are you sending a new version of the whole patchset?
Please Cc me and I'll see about testing it.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com