[U-Boot] [PATCH] [OneNAND IPL] OneNAND board init support

[Kyungmin Park]

On Sat, Sep 19, 2009 at 4:26 AM, Scott Wood <scottwood at freescale.com> wrote:
> On Sat, Aug 29, 2009 at 01:00:59PM +0900, Kyungmin Park wrote:
>>  #define READ_INTERRUPT()                                                \
>> -     onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
>> +                             onenand_readw(ONENAND_REG_INTERRUPT)
>
> You could get rid of the newline now...

It exceeds the 80 columns.

>
>> +enum {
>> +     ONENAND_USE_DEFAULT,
>> +     ONENAND_USE_GENERIC,
>> +};
>
> What is this?  Add a comment, and possibly more specific names.

I see redefine the specific names and comments.

>
>> +extern int (*onenand_read_page)(ulong block, ulong page,
>> +                             u_char *buf, int pagesize);
>
> Maybe use a weak function instead?  Or an #ifdef
> CONFIG_SYS_ONENAND_BOARD_READ_PAGE that will keep the code for the
> generic version from being in the image (it'd be nice if we could
> optimize out replaced weak functions).  It seems especially odd that you
> use one method for init and another for read page.

I tried to use weak function but it produces more than expected. as
you know it got size limitation.
When use the weak function. the apollon board will be broken.
and I don't want to use #ifdef. since Now we support two different
CPUs, s5pc100, s5pc110. these accesses different way. s5pc100 use own
OneNAND controller. but s5pc110 use generic OneNAND method.
That's reason to define the function pointer.

>
>>  /* read a page with ECC */
>> -static inline int onenand_read_page(ulong block, ulong page,
>> +static int generic_onenand_read_page(ulong block, ulong page,
>>                               u_char * buf, int pagesize)
>
> Is the "generic" code really generic, or is it just one specific
> controller?

The 'generic' means the original OneNAND access method. Use NOR
interface and use OneNAND registers.
Most and Most generic method.

Only Samsung SoCs, s3c64xx series, and s5pc100 uses their own OneNAND
controller.

>
>> +#ifdef CONFIG_ONENAND_BOARD_INIT
>
> This should probably be CONFIG_SYS_ONENAND_BOARD_INIT -- it's not
> tweakable by the end user.
>
> How is this different from the existing CONFIG_USE_ONENAND_BOARD_INIT?

Okay l try to consider how to use same configurations.

>
>> +     onenand_board_init(&page_is_4KiB, &page);
>> +#else
>> +     onenand_generic_init(&page_is_4KiB, &page);
>> +#endif
>>
>> -     if (onenand_readw(ONENAND_REG_TECHNOLOGY)) {
>> -             pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */
>> +     if (page_is_4KiB) {
>> +             pagesize = 4096; /* OneNAND has 4KiB pagesize */
>>               erase_shift = 18;
>> -     } else {
>> -             pagesize = 2048;
>> -             erase_shift = 17;
>>       }
>
> I don't understand why you move the pagesize/erase_shift init before
> onenand_board_init, suggesting that the init code change it if it needs
> changing -- but then leave the page_is_4KiB stuff in the generic code.
>
> This should probably just be filled in by the init code without anything
> here.

No different. basically I assume OneNAND has 2KiB pagesize and
In special case, MLC, and 4KiB pagesize OneNAND set the 4KiB pagesize.

If you want to leave as before. no problem.

Please consider the code size and don't want to break exsiting board support.
That's reason I can't use weak function and use #ifdef at onenand_board_init.

Thank you,
Kyungmin Park