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

[Wolfgang Denk]

Dear Kyungmin Park,

In message <9c9fda240907192016i32c7312dh490629f2f2bb31be at mail.gmail.com> you wrote:
> 
> >>  /* 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.

OK, so it is SoC specific code in a common file - that's just
marginally better.

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

It would be great if we can get rid of the #ifdef.

> > Also, please use I/O accessor functions instead of register accesses.
>
> readl(...)? If yes, I agree it.

Yes.

> >> @@ -114,6 +129,9 @@ int onenand_read_block(unsigned char *buf)
> >>
> >>       erasesize = ONENAND_PAGES_PER_BLOCK * pagesize;
> >>       nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize - 1) >> eras> e_shift;
> >> +#ifdef CONFIG_S5PC1XX
> >> +     nblocks = 1;
> >> +#endif
> >
> > Again: why do we need such board specific code here?
>
> It should be fixed.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Just about every computer on the market today runs Unix,  except  the
Mac (and nobody cares about it).                   - Bill Joy 6/21/85