[U-Boot] [PATCH v3] Nand driver for Nomadik SoC

[Alessandro Rubini <rubini-list@gnudd.com>]

> From: Scott Wood <scottwood@freescale.com>

Unfortunately freescale.com i.e. mail.global.frontbridge.com i.e. microsoft
has blacklisted me. I'm trying to do what they say but I fear you won't
get direct email.

>> +static inline int parity(int b) /* b is really a byte; returns 0 or ~0 */
> 
> If it's really a byte, then why not tell the compiler this with uint8_t?

Because otherwise it will add instructions to mask the value.
 
>> +	__asm__ __volatile__(

> Why is this volatile?
> The underscores are unnecessary, BTW.

Both for my own pedantry.

> Have you verified that this is noticeably better than C code?

Well... it looked like I only checked without -O. I rechecked and the
result is the same. Ok, will switch to the C version.

>> +/*
>> + * This is the ECC routine used in hardware, according to the manual.
>> + * HW claims to make the calculation but not the correction; so we must
>> + * recalculate the bytes for a comparison.
>> + */
> 
> Why must you recalculate?  What does the hardware do with the ECC it
> calculates?

It only makes it available. You must recalculate and compare. However,
I haven't been able to make the hardware work (nor original vendor
code did actually use the hardware). I'm waiting for an errata sheet
or direct clarification. Meanwhile this code is working and it's the
best I can do (I can't use ECC_SOFT as the ECC layout would be
different from shipped devices).
 
>> +	.oobfree = { {0x08, 0x08}, {0x18, 0x08}, {0x28, 0x08}, {0x38, 0x08} },
>> +};
> 
> Any particular reason why bytes 0x05-0x07, 0x10-0x11, 0x15-0x17,
> etc. aren't marked free?

Since most other ECC routines use 2..7 I chose to leave open the
possibility to switch over from 2..4. Is that wrong?

>> +	len >>= 2;
>
> What if "len" isn't a multiple of 4?

I thought it never is. This always reads either 512 or 64
bytes. Aligned, too.

>> -#define CONFIG_SYS_NAND_BASE		0x40000000
>> +#define CONFIG_SYS_NAND_BASE		0x40000000 /* SMPS0n */
> 
> What is "SMPS0n"?

It's the chip select. Usually who would use this code will have the
manuals, so all the strange names can be looked up.  Specifically,
it's never spelled out. It's something like "Static Memory Pccard/nand
Select". Kind of black magic, like most vendor names (all vendors).

thanks
/alessandro