[U-Boot] [PATCH 2/4] s5pc1xx: support onenand driver

[Wolfgang Denk]

Dear Kyungmin Park,

In message <9c9fda240909040406m2a8597d4q4b10ef55d33d628a at mail.gmail.com> you wrote:
> 
> > PLease see comments to previous patch about usage of register plus
> > offset versus using proper C structures.
> 
> I have different opinion. How much register map is required? I mean in

Size does actually not matter as you do not  create  any  objects  of
such  type,  you  just  set  a  pointer  to allow the compiler to the
arithmetics and type checking for you.

> case of read/write cmd, it has about 256MiB range. then how do you
> cast is as C structures.

Well, the OneNAND Controller offsets are all withion a single 1 kB
range, right?

> Only some small range for example. 1K or less. C structures possible.
> but more than this. it's too huge to case it.

Where exactly do you think is a data structure too big to be handled
this way?


> > Please change this code to use structs, too.
> >
> >> +static unsigned int s5pc100_mem_addr(int fba, int fpa, int fsa)
> >> +{
> >> +     return (fba << 13) | (fpa << 7) | (fsa << 5);
> >> +}
> >
> > This function needs explanation. Please add a comment what it does,
> > and how.
>
> It's mandatory. no need to shift. It's fixed. with this reason. we
> covers alomst 256MiB memory range.

You don't understand me.

I asked you to add a comment that documents what this function  does,
whet  the  arguments mean, which value it returns, and what the magic
constants used in the code mean.

> >> +#ifdef CONFIG_S5PC1XX
> >> +     /* S5PC100 specific values */
> >> +     onenand->base = (void *) 0xE7100000;
> >> +     onenand->ahb_addr = (void *) 0xB0000000;
> >> +     onenand->mem_addr = s5pc100_mem_addr;
> >> +#else
> >> +#error Please fix it at s3c6410
> >> +#endif
> >
> > What does this mean?
>
> Now we got two version of samsung.c one for s3c6410, another is this
> one. No problem to add it for s3c6410

Ah! Two versions of samsung.c? This is bad. Please avoid such code
duplication. Create a common file that covers both systems.


> >> +/*
> >> + * OneNAND Controller
> >> + */
> >> +#define MEM_CFG_OFFSET               0x0000
> >> +#define BURST_LEN_OFFSET     0x0010
> >> +#define MEM_RESET_OFFSET     0x0020
> >> +#define INT_ERR_STAT_OFFSET  0x0030
> >> +#define INT_ERR_MASK_OFFSET  0x0040
> >> +#define INT_ERR_ACK_OFFSET   0x0050
> >
> > Please use a C struct instead.
>
> The end address is 0x400. Are you really need it to cast? I don't think so.

I don't understand what you complain about. What's wrong about to have
a structure for that? That its sparse populated? No problem.

Keep in mind that we do not create any such objects, we just use the
defintion to desctibe the structure of your hardware, so the compiler
can to the address calculation and typoe checking for us.

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
Vor allem kein Gedanke! Nichts ist kompromittierender als ein  Gedan-
ke!            - Friedrich Wilhelm Nietzsche _Der Fall Wagner_ (1888)