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

[Dirk Behme]

Dirk Behme wrote:
> Scott Wood wrote:
> 
>> On Tue, Oct 07, 2008 at 06:25:11AM -0500, Nishanth Menon wrote:
>>
>>> Dirk Behme said the following on 10/07/2008 04:42 AM:
>>>
>>>> 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.
>>
>>
>>
>> Correct, NULL methods will be filled in with defaults if applicable.
> 
> 
> ok, will do so, this is easy :)
> 
>>>>>> +/*
>>>>>> + *  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.
>>
>>
>> Right, I'd just like to see an explicit statement that this is the 
>> only way
>> to do HW ECC that the hardware supports (following verification of that
>> fact, of course), along with a pointer to where in the code the ECC error
>> when reading an empty page is dealt with.
> 
> 
> Will add Nishanth's explanation to comment and check code for this.
> 
>>>>>> +    .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,
>>
>>
>>
>> Why not offset 14, length 50?
> 
> 
> Seems I need a closer look what we are talking about here ;)
> 
>>>> 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..
>>
>>
>>
>> Agreed.
> 
> 
> As already mentioned in previous post, I think for the the moment we 
> have to go with both ways.
> 
> To summarize the open points I will look at:
> 
> a) Remove unnecessary nand->read_buf init

Removed in attachment

> b) Add comment and check code for HW ecc issue with erased page

Added comment. Not sure if we have to do anything in code, though.

> c) Fix offset 14, length 50 issue

Changed in attachment.

> d) Extend MTD API with a call to switch HW/SW ecc, remove 
> platform-specific ifdefs in generic files

Removed nand ecc command from cmd_nand.c and added "nandecc" command 
to board file (Scott: Thanks for the hint!):

--- /dev/null
+++ u-boot-arm/cpu/arm_cortexa8/omap3/board.c
...
+#ifdef CONFIG_NAND_OMAP3
+/******************************************************************************
+ * OMAP3 specific command to switch between NAND HW and SW ecc
+ 
*****************************************************************************/
+static int do_switch_ecc(cmd_tbl_t * cmdtp, int flag, int argc, char 
*argv[])
+{
+	if (argc != 2)
+		goto usage;
+	if (strncmp(argv[1], "hw", 2) == 0)
+		omap_nand_switch_ecc(1);
+	else if (strncmp(argv[1], "sw", 2) == 0)
+		omap_nand_switch_ecc(0);
+	else
+		goto usage;
+
+	return 0;
+
+usage:
+	printf ("Usage: nandecc %s\n", cmdtp->help);
+	return 1;
+}
+
+U_BOOT_CMD(
+	nandecc, 2, 1,	do_switch_ecc,
+	"nandecc - switch OMAP3 NAND ECC calculation algorithm\n",
+	"[hw/sw] - Switch between NAND hardware (hw) or software (sw) ecc 
algorithm\n"
+	);
+
+#endif /* CONFIG_NAND_OMAP3 */

See attachment for latest version of NAND patch. Please let us know if 
anything else has to be changed. If not, I will prepare OMAP3 v3 patch 
set.

Thanks

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/20081007/88d2c45d/attachment-0001.txt