[U-Boot] [PATCH 6/6] S5PC100: Add onenand_ipl for SMDKC100 support

[Kyungmin Park]

Hi,

On Mon, Jul 20, 2009 at 5:04 AM, Wolfgang Denk<wd at denx.de> wrote:
> Dear HeungJun Kim,
>
> In message <350d1ec30906250121q4e860812s35054aaee8c2016e at mail.gmail.com> you wrote:
>> The SMDKC100 Board has 256MB onenand.
>> So, It's bootable, if this patch is adapted thus the board use onenand_ipl.
>>
>> Signed-off-by: HeungJun, Kim <riverful.kim at samsung.com>
>>
>> ---
>>
>> This patch support onenand boot on SMDKC100 Board.
> ...
>> diff --git a/onenand_ipl/board/samsung/smdkc100/Makefile
>> b/onenand_ipl/board/samsung/smdkc100/Makefile
>> new file mode 100644
>> index 0000000..4301938
>> --- /dev/null
>> +++ b/onenand_ipl/board/samsung/smdkc100/Makefile
> ...
>> +ALL  = $(onenandobj)onenand-ipl $(onenandobj)onenand-ipl.bin
>> $(onenandobj)onenand-ipl-2k.bin $(onenandobj)onenand-ipl-4k.bin
>
> This patch is white-space corrupted. I gues this happened here because
> the line was way too long.
>
>> --- a/onenand_ipl/onenand_read.c
>> +++ b/onenand_ipl/onenand_read.c
>> @@ -37,10 +37,24 @@
>>  extern void *memcpy32(void *dest, void *src, int size);
>>  #endif
>>
>> +
>> +
>
> Please do not arbitrary empty lines.
>
>>  /* read a page with ECC */
>>  static inline int onenand_read_page(ulong block, ulong page,
>>                               u_char * buf, int pagesize)
>>  {
>> +#ifdef CONFIG_S5PC1XX
>> +     unsigned int *p = (unsigned int *) buf;
>> +     int mem_addr, i;
>> +
>> +     mem_addr = MEM_ADDR(block, page, 0);
>> +
>> +     pagesize >>= 2;
>> +
>> +     for (i = 0; i < pagesize; i++)
>> +             *p++ = *(volatile unsigned int *)(CMD_MAP_01(mem_addr));
>> +#else        /* CONFIG_S5PC1XX */
>> +
>>       unsigned long *base;
>
> I don't like to see such board specific code in global files.

I think it's not board specific code. S3C64XX and S5PC1XX series have
own OneNAND controller and to access the OneNAND, it should use the
this controller.

If you don't like the ifdef. we can separate the function but I'm not
sure it's really required.

>
> Also, please use I/O accessor functions instead of register accesses.

readl(...)? If yes, I agree it.
>
>> @@ -114,6 +129,9 @@ int onenand_read_block(unsigned char *buf)
>>
>>       erasesize = ONENAND_PAGES_PER_BLOCK * pagesize;
>>       nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize - 1) >> erase_shift;
>> +#ifdef CONFIG_S5PC1XX
>> +     nblocks = 1;
>> +#endif
>
> Again: why do we need such board specific code here?
>

It should be fixed.

Thank you,
Kyungmin Park