[U-Boot] [PATCH] ppc: transform init_sequence into a function.

[Joakim Tjernlund]

Wolfgang Denk <wd at denx.de> wrote on 27/11/2009 15:06:34:
>
> Dear Joakim Tjernlund,
>
> In message <1259317926-9820-1-git-send-email-Joakim.Tjernlund at transmode.se> you wrote:
> > init_sequence is an array with function pointers.
> > It produces lots of relocation data and it
> > is hard to debug when something fails.
> >
> > Transform it into a function, making it smaller
> > and easier to debug.
> >    text      data       bss       dec       hex   filename
> >    1268       212         0      1480       5c8   lib_ppc/board.org
> >    1224        92         0      1316       524   lib_ppc/board.new
>
> You know that I'm a really big fan of small code, and I tend to
> accept a certain amount of ugliness if it saves memory. But here I
> just disagree.

I think most of the ugliness comes from the #ifdef hell in this list.
replacing the #ifdefs is another matter so looking behind
the #ifdef mess, I don't think it looks too bad.

>
> > -init_fnc_t *init_sequence[] = {
> > +void init_sequence(void)
> > +{
> >  #if defined(CONFIG_MPC85xx) || defined(CONFIG_MPC86xx)
> > -   probecpu,
> > +   if (probecpu())
> > +      goto err_out;
> >  #endif
> >  #if defined(CONFIG_BOARD_EARLY_INIT_F)
> > -   board_early_init_f,
> > +   if (board_early_init_f())
> > +      goto err_out;
> >  #endif
> >  #if !defined(CONFIG_8xx_CPUCLK_DEFAULT)
> > -   get_clocks,      /* get CPU and bus clocks (etc.) */
> > +   if (get_clocks())
> > +      goto err_out;   /* get CPU and bus clocks (etc.) */
> >  #if defined(CONFIG_TQM8xxL) && !defined(CONFIG_TQM866M) \
> >      && !defined(CONFIG_TQM885D)
> > -   adjust_sdram_tbs_8xx,
> > +   if (adjust_sdram_tbs_8xx())
> > +      goto err_out;
> >  #endif
> > -   init_timebase,
> > +   if (init_timebase())
> > +      goto err_out;
>
> This is much more ugly, and I cannot see why it would be easier to
> debug.

You can set breakpoints anywhere you like. When it crashes in here
somewhere, it isn't easy to tell where it did so.

>
> The original idea of defining an array of function pointed was to
> introduce a bigger level of flexibility. There was a time when people
> complained about the fixed initialization sequence. So my thinking
> was that it should be possible to simply #define in you board config
> file a list of function pointers to initialize init_sequence[], i. e.
> allow for completely board specific init sequences.

Noted.

>
> OK, you can argument that nobody used this feature yeat, or that you
> could provide a weak implementation of the new init_sequence()
> function, or ... but just for saving 164 Bytes and adding a lot of
> ugliness?

There is one more minor advantage too. When bringing up a board, relocation
may be off and you will get an very early crash, avoiding relocs here might
get you a bit further, possibly making it easier to pin point the problem.
This is all speculation though.

Weak funs would be nice too, but that is another matter which could follow later.

Anyhow, I don't feel too strongly about this. If you want to drop it, I won't
cry :)