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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
