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

[Dirk Behme <dirk.behme@googlemail.com>]

Scott Wood wrote:
> 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.

Thanks for this idea (you really help a lot)! Proposal in attachment 
(omap_correct_data()).

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

Yes, I know, agreed. But changing existing kernels isn't easy, too.

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

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

Nothing. A macro:

#define OMAP34XX_GPMC_BASE                (0x6E000000)
#define GPMC_BASE		(OMAP34XX_GPMC_BASE)

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

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

What about calling nand_scan_tail() for this? Proposal in attachment 
(omap_nand_switch_ecc()).

Many thanks for your help, reviews and patience,

Dirk

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 08_omap3_nand.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20081011/9374d331/attachment.txt