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