[U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

[Nishanth Menon]

Dirk Behme said the following on 10/07/2008 04:42 AM:
>
> Scott Wood wrote:
>> On Fri, Oct 03, 2008 at 12:40:25PM +0200, dirk.behme at googlemail.com
>> wrote:
>>
>>> +#include <common.h>
>>> +#include <asm/io.h>
>>> +#include <asm/arch/mem.h>
>>> +#include <linux/mtd/nand_ecc.h>
>>> +
>>> +#if defined(CONFIG_CMD_NAND)
>>> +
>>> +#include <nand.h>
>>
>>
>> Move the #ifdef to the Makefile.
>
> Did this. Additionally, I moved OMAP3 NAND driver to
> drivers/mtd/nand/omap3.c.
I would recommend having a nand_omap_gpmc.c if the driver can be made
generic enough.
>
>>> +/*
>>> + * nand_read_buf16 - [DEFAULT] read chip data into buffer
>>> + * @mtd:    MTD device structure
>>> + * @buf:    buffer to store date
>>> + * @len:    number of bytes to read
>>> + *
>>> + * Default read function for 16bit buswith
>>> + */
>>> +static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf,
>>> int len)
>>> +{
>>> +    int i;
>>> +    struct nand_chip *this = mtd->priv;
>>> +    u16 *p = (u16 *) buf;
>>> +    len >>= 1;
>>> +
>>> +    for (i = 0; i < len; i++)
>>> +        p[i] = readw(this->IO_ADDR_R);
>>> +}
>>
>>
>> How does this differ from the default implementation?
>
> It doesn't differ ;)
>
> So I removed this and tried to use default nand_read_buf16() instead:
>
> nand->read_buf = nand_read_buf16;
>
> in board_nand_init(). But this doesn't compile as nand_read_buf16() is
> static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it
> as FIXME in patch.
Probably does not need an explicit initialization, mtd nand_scan should
populate that. in fact IMHO, the read/write buf ops might be duplicating
mtd's implementation..  :(

>
>
>>> +/*
>>> + *  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.
>>
>>
>> Is this a hardware limitation?  If so, say so in the comment.  If not,
>> why do it this way?
>
> Don't know.
>
> All: Any help?
The issue is simple: assume we read a page of 0xFF's(fresh erased), IF
using H/w ECC engine within GPMC, the result of read will be 0x0 while
the ECC offsets of the spare area will be 0xFF which will result in an
ECC mismatch.
>>> +    .eccbytes = 12,
>>> +    .eccpos = {
>>> +           2, 3, 4, 5,
>>> +           6, 7, 8, 9,
>>> +           10, 11, 12, 13},
>>> +    .oobfree = { {20, 50} }    /* don't care */
>>
>>
>> Bytes 64-69 of a 64-byte OOB are free?
>> What don't we care about?
> +static struct nand_ecclayout hw_nand_oob_64 = {
>
> Don't know (or understand?).
>
> All: Any help?
I do not get it either.. ECCPOS is in offset bytes, and oobfree should
be {.offset=20,.length=44} /*I always hated struct initialization done
as above..*/, but then,
>
>>> ===================================================================
>>> --- u-boot-arm.orig/drivers/mtd/nand/nand_base.c
>>> +++ u-boot-arm/drivers/mtd/nand/nand_base.c
>>> @@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
>>>     return 0;
>>> }
>>>
>>> +#if defined(CONFIG_OMAP) && (defined(CONFIG_OMAP3_BEAGLE) \
>>> +    || defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
>>> +void nand_switch_ecc(struct mtd_info *mtd)
>>
>>
>> NACK.  First, explain why you need to be able to dynamically switch ECC
>> modes.
>
> Two topics here, changes in cmd_nand.c and nand_base.c.
>
> cmd_nand.c:
>
> We need to be able to switch ECC at runtime cause some images have to
> be written to NAND with HW ECC and some with SW ECC. This depends on
> what user (reader) of these parts expect.
>
> OMAP3 has a boot ROM which is able to read a second level loader
> (called x-loader) from NAND and start/execute it. This 2nd level
> loader has to be written by U-Boot using HW ECC as ROM code does HW
> ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC
> as default. For this we have to use SW ECC to write images, then.
>
> Therefore we add an additional user command in cmd_nand.c to switch
> ECC depending on what user wants to write.
why not use h/w ecc which rom code understands in kernel and u-boot. H/w
ecc will be faster in comparison to doing s/w ecc and there is good
support in MTD to do it, then there would be no reason for s/w ecc IMHO..

Regards,
Nishanth Menon