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