From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Mon, 09 Feb 2009 12:10:12 -0600
Subject: [U-Boot] [PATCH v3] Nand driver for Nomadik SoC
In-Reply-To: <20090209175652.GA1206@mail.gnudd.com>
References: <20090209171641.GA27824@ld0162-tx32.am.freescale.net>	<20090207150450.GA19180@mail.gnudd.com>	<20090207044347.GA10759@mail.gnudd.com>	<20090207231956.GA27872@mail.gnudd.com>
	<20090209175652.GA1206@mail.gnudd.com>
Message-ID: <49907184.9000201@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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