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

[Scott Wood <scottwood@freescale.com>]

Alessandro Rubini wrote:
>> 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.

Hmm, I've e-mailed the postmaster inquiring as to why.  Is there a 
specific IP address or range that is being blocked?  Is there a 
rejection message?

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

OK.

>>> +	__asm__ __volatile__(
> 
>> Why is this volatile?
>> The underscores are unnecessary, BTW.
> 
> Both for my own pedantry.

volatile should be left off of pure calculations; you're just removing 
optimization opportunity.

And I think the underscores are ugly. :-)

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

OK, good.

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

Makes what available?  If it makes the calculated ECC available, you 
should only need to do the code in ecc_correct, not ecc512.

> However, I haven't been able to make the hardware work (nor original vendor
> code did actually use the hardware).

OK, that seems to be the more relevant reason. :-)

Include a comment to that effect.

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

It's not wrong as long as the free bytes you do claim meet all expected 
needs; I was just curious.

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

Suppose it only wants to read a few bytes of OOB.

>>> -#define CONFIG_SYS_NAND_BASE		0x40000000
>>> +#define CONFIG_SYS_NAND_BASE		0x40000000 /* SMPS0n */
>> What is "SMPS0n"?
> 
> It's the chip select.

OK.  Just making sure it wasn't something left in by accident.

-Scott