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

[Scott Wood]

On Fri, Oct 10, 2008 at 08:58:41AM +0200, Dirk Behme wrote:
> >>+/*
> >>+ *  omap_calculate_ecc - Generate non-inverted ECC bytes.
> >>+ *
> >>+ *  Using noninverted ECC can be considered ugly since writing a blank
> >>+ *  page ie. padding will clear the ECC bytes. This is no problem as
> >>+ *  long nobody is trying to write data on the seemingly unused page.
> >>+ *  Reading an erased page will produce an ECC mismatch between
> >>+ *  generated and read ECC bytes that has to be dealt with separately.
> >
> >
> >Where is it dealt with separately?
> 
> We already talked about this and extended the comment. To my 
> understanding this special handling can't be done in 
> omap_calculate_ecc() as it is called from generic NAND code and 
> doesn't know if ECC it calculates is correct or not?
> 
> Do you have any proposals where and how to handle this?

Perhaps it could be done in omap_correct_data()?  If you get a calc_ecc
that corresponds to a blank page, treat a read_ecc of all-bits-set as
correct.

> To summarize: If you agree with changes in attachment, last open point 
> is the "ECC mismatch" issue. Do you agree?

It's looking decent.  I have some concerns about the ECC switching,
though.

> +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.

> +static void omap_hwecc_init(struct nand_chip *chip)
> +{
> +	/* Init ECC Control Register */
> +	/* Clear all ECC | Enable Reg1 */
> +	writel(ECCCLEAR | ECCRESULTREG1, GPMC_BASE + GPMC_ECC_CONTROL);
> +	writel(ECCSIZE1 | ECCSIZE0 | ECCSIZE0SEL,
> +	       GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
> +}

Is GPMC_BASE an integer or a pointer?

> +	if (!hardware) {
> +		nand->ecc.mode = NAND_ECC_SOFT;
> +		nand->ecc.layout = &sw_nand_oob_64;
> +		nand->ecc.size = 256;	/* set default eccsize */
> +		nand->ecc.bytes = 3;
> +		nand->ecc.steps = 8;
> +		nand->ecc.hwctl = 0;
> +		nand->ecc.calculate = nand_calculate_ecc;
> +		nand->ecc.correct = nand_correct_data;
> +	} else {
> +		nand->ecc.mode = NAND_ECC_HW;
> +		nand->ecc.layout = &hw_nand_oob_64;
> +		nand->ecc.size = 512;
> +		nand->ecc.bytes = 3;
> +		nand->ecc.steps = 4;
> +		nand->ecc.hwctl = omap_enable_hwecc;
> +		nand->ecc.correct = omap_correct_data;
> +		nand->ecc.calculate = omap_calculate_ecc;
> +		omap_hwecc_init(nand);
> +	}
> +}

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).

-Scott