[U-Boot] [PATCH 08/13 v3] ARM: OMAP3: Add NAND support

[Scott Wood]

On Sat, Oct 11, 2008 at 10:55:37AM +0200, Dirk Behme wrote:
> >It's looking decent.  I have some concerns about the ECC switching,
> >though.
> 
> Yes, I know, agreed. But changing existing kernels isn't easy, too.

I meant the implementation (specifically, the re-init of things done by
nand_scan_tail()).

> >>+static unsigned char cs;
> >>+static void __iomem *gpmc_cs_base_add;
> >
> >
> >I'd prefer an actual data type rather than void, but I won't hold it up
> >for that.
> 
> I took the data type of IO_ADDR_W & IO_ADDR_R where gpmc_cs_base_add 
> is assigned to.
> 
> void  __iomem	*IO_ADDR_W;

But the actual data is of a certain size, not void.

> >Is GPMC_BASE an integer or a pointer?
> 
> Nothing. A macro:
> 
> #define OMAP34XX_GPMC_BASE                (0x6E000000)
> #define GPMC_BASE		(OMAP34XX_GPMC_BASE)

So it's an integer.

> It's then casted to volatile pointer by ARM's readx/writex.

The cast should be done by the driver, or you'll get warnings if
readx/writex ever become inline functions (as they are on other arches).

> >Make sure that anything that the generic layer calculates that would
> >change is updated (e.g. oobavail, read_page, write_page, read_oob,
> >write_oob).
> 
> What about calling nand_scan_tail() for this? Proposal in attachment 
> (omap_nand_switch_ecc()).

You'll leak chip->buffers.  Better to factor nand_scan_tail() into two
functions, one which allocates memory and any other non-repeatable init,
and another which does init based on ECC type.  The former function would
call the latter.

> +static void omap_nand_hwcontrol(struct mtd_info *mtd, int cmd,
> +				unsigned int ctrl)
> +{
> +	register struct nand_chip *this = mtd->priv;

The "register" keyword is superfluous when not specifying a specific
register (such as for use with inline assembly).

> +	/* Check if calc_ecc corresponds to a blank page.
> +	 * If so, treat read_ecc as correct. See comment at omap_calculate_ecc.
> +	 */
> +	if((*(unsigned int *)calc_ecc == 0x00000000) &&
> +	   (*(unsigned int *)read_ecc == 0xFFFFFFFF))
> +		return 0;

This assumes 32-bit alignment of the ECC -- and aren't there four
steps of 3 bytes each?

Also, not being overly familiar with ECC, is it possible for one or two
bit flips to cause the computed ECC to go from all-ones to all-zeroes? 
If it is, then we may want to follow this up by checking the data.

-Scott