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

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

[Joakim Tjernlund]

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

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 lib_ppc/board.c |  123 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 75 insertions(+), 48 deletions(-)

diff --git a/lib_ppc/board.c b/lib_ppc/board.c
index 765f97a..f0160e6 100644
--- a/lib_ppc/board.c
+++ b/lib_ppc/board.c
@@ -157,7 +157,6 @@ ulong monitor_flash_len;
  * argument, and returns an integer return code, where 0 means
  * "continue" and != 0 means "fatal error, hang the system".
  */
-typedef int (init_fnc_t) (void);
 
 /************************************************************************
  * Init Utilities							*
@@ -236,17 +235,17 @@ static int init_func_watchdog_init (void)
 	WATCHDOG_RESET ();
 	return (0);
 }
-# define INIT_FUNC_WATCHDOG_INIT	init_func_watchdog_init,
+# define INIT_FUNC_WATCHDOG_INIT	init_func_watchdog_init()
 
 static int init_func_watchdog_reset (void)
 {
 	WATCHDOG_RESET ();
 	return (0);
 }
-# define INIT_FUNC_WATCHDOG_RESET	init_func_watchdog_reset,
+# define INIT_FUNC_WATCHDOG_RESET	init_func_watchdog_reset()
 #else
-# define INIT_FUNC_WATCHDOG_INIT	/* undef */
-# define INIT_FUNC_WATCHDOG_RESET	/* undef */
+# define INIT_FUNC_WATCHDOG_INIT	0 /* undef */
+# define INIT_FUNC_WATCHDOG_RESET	0 /* undef */
 #endif /* CONFIG_WATCHDOG */
 
 /************************************************************************
@@ -254,76 +253,110 @@ static int init_func_watchdog_reset (void)
  ************************************************************************
  */
 
-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;
 #endif
 #ifdef CONFIG_SYS_ALLOC_DPRAM
 #if !defined(CONFIG_CPM2)
-	dpram_init,
+	if (dpram_init())
+		goto err_out;
 #endif
 #endif
 #if defined(CONFIG_BOARD_POSTCLK_INIT)
-	board_postclk_init,
+	if (board_postclk_init())
+		goto err_out;
 #endif
-	env_init,
+	if (env_init())
+		goto err_out;
 #if defined(CONFIG_8xx_CPUCLK_DEFAULT)
-	get_clocks_866,		/* get CPU and bus clocks according to the environment variable */
-	sdram_adjust_866,	/* adjust sdram refresh rate according to the new clock */
-	init_timebase,
-#endif
-	init_baudrate,
-	serial_init,
-	console_init_f,
-	display_options,
+	if (get_clocks_866())
+		goto err_out;	/* get CPU and bus clocks according to the environment variable */
+	if (sdram_adjust_866())
+		goto err_out;	/* adjust sdram refresh rate according to the new clock */
+	if (init_timebase())
+		goto err_out;
+#endif
+	if (init_baudrate())
+		goto err_out;
+	if (serial_init())
+		goto err_out;
+	if (console_init_f())
+		goto err_out;
+	if (display_options())
+		goto err_out;
 #if defined(CONFIG_8260)
-	prt_8260_rsr,
-	prt_8260_clks,
+	if (prt_8260_rsr())
+		goto err_out;
+	if (prt_8260_clks())
+		goto err_out;
 #endif /* CONFIG_8260 */
 #if defined(CONFIG_MPC83xx)
-	prt_83xx_rsr,
+	if (prt_83xx_rsr())
+		goto err_out;
 #endif
-	checkcpu,
+	if (checkcpu())
+		goto err_out;
 #if defined(CONFIG_MPC5xxx)
-	prt_mpc5xxx_clks,
+	if (prt_mpc5xxx_clks())
+		goto err_out;
 #endif /* CONFIG_MPC5xxx */
 #if defined(CONFIG_MPC8220)
-	prt_mpc8220_clks,
+	if (prt_mpc8220_clks())
+		goto err_out;
 #endif
-	checkboard,
-	INIT_FUNC_WATCHDOG_INIT
+	if (checkboard())
+		goto err_out;
+	if (INIT_FUNC_WATCHDOG_INIT)
+		goto err_out;
 #if defined(CONFIG_MISC_INIT_F)
-	misc_init_f,
+	if (misc_init_f())
+		goto err_out;
 #endif
-	INIT_FUNC_WATCHDOG_RESET
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
 #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
-	init_func_i2c,
+	if (init_func_i2c())
+		goto err_out;
 #endif
 #if defined(CONFIG_HARD_SPI)
-	init_func_spi,
+	if (init_func_spi())
+		goto err_out;
 #endif
 #ifdef CONFIG_POST
-	post_init_f,
+	if (post_init_f())
+		goto err_out;
 #endif
-	INIT_FUNC_WATCHDOG_RESET
-	init_func_ram,
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
+	if (init_func_ram())
+		goto err_out;
 #if defined(CONFIG_SYS_DRAM_TEST)
-	testdram,
+	if (testdram())
+		goto err_out;
 #endif /* CONFIG_SYS_DRAM_TEST */
-	INIT_FUNC_WATCHDOG_RESET
-
-	NULL,			/* Terminate this list */
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
+ 	return;
+err_out:
+	hang();
 };
 
 ulong get_effective_memsize(void)
@@ -366,7 +399,6 @@ void board_init_f (ulong bootflag)
 	ulong len, addr, addr_sp;
 	ulong *s;
 	gd_t *id;
-	init_fnc_t **init_fnc_ptr;
 #ifdef CONFIG_PRAM
 	int i;
 	ulong reg;
@@ -383,12 +415,7 @@ void board_init_f (ulong bootflag)
 	/* Clear initial global data */
 	memset ((void *) gd, 0, sizeof (gd_t));
 #endif
-
-	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
-		if ((*init_fnc_ptr) () != 0) {
-			hang ();
-		}
-	}
+	init_sequence();
 
 	/*
 	 * Now that we have DRAM mapped and working, we can
-- 
1.6.4.4

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <1259317926-9820-1-git-send-email-Joakim.Tjernlund@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.

> -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.

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.

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?

Thank you, but no.

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
An Elephant is a mouse with an Operating System.              - Knuth

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 27/11/2009 15:06:34:
>
> Dear Joakim Tjernlund,
>
> In message <1259317926-9820-1-git-send-email-Joakim.Tjernlund@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 :)

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OF012B1DA7.446FFBCD-ONC125767B.004DD4EE-C125767B.00508FF3@transmode.se> you wrote:
>
> > 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.

Well, remove the #ifdef's from the pointer array, and it will be
beautiful, too?

> > 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.

Umm... you can set breakpoints on any of the functions now, too?
And printing the funtion pointer will show you easily where you are,
right?

> 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.

Of all the problems I ever had with bringing up new  hardware,  relo-
cation  has  never  been  one  of them. Relocation is pretty hardware
indepentent - either it works, or it doesn't.

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

I am not convinced yet that the new code is actually an improvement.
Changing the array of pointers into a list of function calls does not
solve any of the real issues we have with the init seuqnece - like
that some board need the PCI initialization early, and others later,
etc.

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
In an organization, each person rises to the level of his own  incom-
petency                                         - The Peter Principle

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 27/11/2009 21:18:28:

> From: Wolfgang Denk <wd@denx.de>
> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Cc: u-boot at lists.denx.de
> Date: 27/11/2009 21:18
> Subject: Re: [U-Boot] [PATCH] ppc: transform init_sequence into a function.
>
> Dear Joakim Tjernlund,
>
> In message <OF012B1DA7.446FFBCD-ONC125767B.004DD4EE-C125767B.
> 00508FF3 at transmode.se> you wrote:
> >
> > > 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.
>
> Well, remove the #ifdef's from the pointer array, and it will be
> beautiful, too?

Both versions will look a lot better when the #ifdefs are gone. I find
the current #ifdefs more ugly than my new function. You don't
obviously as otherwise you would not have the #ifdefs in the first place.

>
> > > 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.
>
> Umm... you can set breakpoints on any of the functions now, too?
> And printing the funtion pointer will show you easily where you are,
> right?

Yes, but it isn't as easy as placing bp's in the new function.
To find the problem function you would have print the fptr, stepover
repeat until done. You probably need to do this a few times as
mistakes are common and you will have to reboot and start from the beginning.
Also, depending on the board and emulator, it might not be possible/easy to continue
after you hit a BP because the emulator flushes the cache(or something else), then
you have to find each function source code(lots of unwanted typing) and place
the BP inside the function. If you are very lucky, it was the right one, otherwise
go find some other function and repeat.
Finally, if you don't have debugger handy, you will have stick some debug code
into each function to see which one that causes the problem as opposed to
add some debug code into my new function and move it around a bit until
you have identified the problematic function.

While it is possible to do all this with the current way, it isn't
as easy as with my new function, period.

>
> > 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.
>
> Of all the problems I ever had with bringing up new  hardware,  relo-
> cation  has  never  been  one  of them. Relocation is pretty hardware
> indepentent - either it works, or it doesn't.
>
> > Anyhow, I don't feel too strongly about this. If you want to drop it, I won't
> > cry :)
>
> I am not convinced yet that the new code is actually an improvement.
> Changing the array of pointers into a list of function calls does not
> solve any of the real issues we have with the init seuqnece - like
> that some board need the PCI initialization early, and others later,
> etc.

What has PCI initialization with my new function to do? I never
claimed it would solve such problems. You are side tracking
the point here which is:
- reduce code size.
- easier debugging.
against keeping a beautiful list of function ptrs, while it is
pretty, it is not as useful as my new function.

 Jocke

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 27/11/2009 21:18:28:
> I am not convinced yet that the new code is actually an improvement.
> Changing the array of pointers into a list of function calls does not
> solve any of the real issues we have with the init seuqnece - like
> that some board need the PCI initialization early, and others later,
> etc.

Somewhat offtopic, but you could add a few weak empty dummy functions
at strategic places in the board_X funcs. Any board that
needs some extra init sequence could define the appropriate function
which will replace the weak one.
Even my new init_sequence function could be made weak.

    Jocke

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OF9F9C7491.8E6AB1CE-ONC125767E.00448FDA-C125767E.00453C54@transmode.se> you wrote:
> Wolfgang Denk <wd@denx.de> wrote on 27/11/2009 21:18:28:
> > I am not convinced yet that the new code is actually an improvement.
> > Changing the array of pointers into a list of function calls does not
> > solve any of the real issues we have with the init seuqnece - like
> > that some board need the PCI initialization early, and others later,
> > etc.
> 
> Somewhat offtopic, but you could add a few weak empty dummy functions
> at strategic places in the board_X funcs. Any board that
> needs some extra init sequence could define the appropriate function
> which will replace the weak one.

Yes. And all boards that don't need it will suffer from the increased
memory footprint.

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
There's an old story about the person who wished his computer were as
easy to use as his telephone. That wish has come  true,  since  I  no
longer know how to use my telephone.

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 30/11/2009 22:02:44:
>
>
> Dear Joakim Tjernlund,
>
> In message <OF9F9C7491.8E6AB1CE-ONC125767E.00448FDA-C125767E.
> 00453C54 at transmode.se> you wrote:
> > Wolfgang Denk <wd@denx.de> wrote on 27/11/2009 21:18:28:
> > > I am not convinced yet that the new code is actually an improvement.
> > > Changing the array of pointers into a list of function calls does not
> > > solve any of the real issues we have with the init seuqnece - like
> > > that some board need the PCI initialization early, and others later,
> > > etc.
> >
> > Somewhat offtopic, but you could add a few weak empty dummy functions
> > at strategic places in the board_X funcs. Any board that
> > needs some extra init sequence could define the appropriate function
> > which will replace the weak one.
>
> Yes. And all boards that don't need it will suffer from the increased
> memory footprint.

Sure, but I won't adding these extra call sites as an array of
fptrs also add size? Since the new function as smaller than the current
list, I would not be surprised if my function idea is smaller
in total. Perhaps I am misunderstanding something?

I am just illustrating one way, one that will allow boards
better control too as then can define this function as they like/need.

 Jocke

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OF5A3AF402.57EFBD26-ONC125767E.007A9CC5-C125767E.007B6427@transmode.se> you wrote:
>
> > Yes. And all boards that don't need it will suffer from the increased
> > memory footprint.
> 
> Sure, but I won't adding these extra call sites as an array of
> fptrs also add size? Since the new function as smaller than the current
> list, I would not be surprised if my function idea is smaller
> in total. Perhaps I am misunderstanding something?
> 
> I am just illustrating one way, one that will allow boards
> better control too as then can define this function as they like/need.

The idea is that boards that want such contrrol can redefine the
whole init sequence list - adding what they really need, and omitting
what they don't. Zero overhead.

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
There is an order of things in this universe.
	-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

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

[Detlev Zundel]

Hi Wolfgang,

> Dear Joakim Tjernlund,
>
> In message <OF5A3AF402.57EFBD26-ONC125767E.007A9CC5-C125767E.007B6427@transmode.se> you wrote:
>>
>> > Yes. And all boards that don't need it will suffer from the increased
>> > memory footprint.
>> 
>> Sure, but I won't adding these extra call sites as an array of
>> fptrs also add size? Since the new function as smaller than the current
>> list, I would not be surprised if my function idea is smaller
>> in total. Perhaps I am misunderstanding something?
>> 
>> I am just illustrating one way, one that will allow boards
>> better control too as then can define this function as they like/need.
>
> The idea is that boards that want such contrrol can redefine the
> whole init sequence list - adding what they really need, and omitting
> what they don't. Zero overhead.

A little bit late, but reading and pondering on Jockes suggestion, in
the meantime I lean somewhat into the direction of Jockes init
function.  Probably the heaviest argument is that once a board comes
along which redefines the whole init list, this will effectively be a
snapshot of the then current init-list shuffled around for this specific
board.  Now when we add more board-independent sub-systems needed
initialization, we will always have to remember that there are copies of
this pretty central data structure needing to be updated also, which I
do not think to be very nice.

Moreover, if we have such a central place, when adding stuff, we always
now which user may have a problem with new code without going through
all board configs.

Apart from that, if up to today no board actually did such a
redefinition of the whole array, then one could argue that the chances
for something like that are pretty slim.  And even if such a thing
happens, I rather like to see the exception for such a conceptually
important thing in a central place rather than a board config file.

Just my 0.02 of your favourite currency
  Detlev

-- 
Some mathematicians become so tense these days that they do not go to
sleep during seminars.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

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

[Alessandro Rubini]

> 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.

I think the approach I took with lib_arm/board.c may apply as well.
A simple weak init_nop(), no change at all in board code and
no more "CONFIG_BOARD_EARLY_INIT_F" kind of stuff.

>> 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.

I agree it is uglier (and I've discussed with Joakim offlist), this is
a very strong point. I never had problems with the u-boot init
function, I had serious issues with kernel initcalls.

Whils debug_initcall is great and good in the kernel, we can't do with
printf ini u-boot, not in the first steps, at least.

So while I strongly prefer data structures to a burst of "if () goto error;",
I think Joakim has a point here -- after I argued against it offlist.

/alessandro

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

[Joakim Tjernlund]

rubini at gnudd.com wrote on 27/11/2009 15:54:57:
>
> > 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.
>
> I think the approach I took with lib_arm/board.c may apply as well.
> A simple weak init_nop(), no change at all in board code and
> no more "CONFIG_BOARD_EARLY_INIT_F" kind of stuff.
>
> >> 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.
>
> I agree it is uglier (and I've discussed with Joakim offlist), this is
> a very strong point. I never had problems with the u-boot init
> function, I had serious issues with kernel initcalls.

I have and it can be very time consuming to iterate back and fourth
just to get to the actual crash point even with a debugger like BDI2000

 Jocke

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

[Joakim Tjernlund]

init_sequence is an array with function pointers which
are really hard to follow when you need to debug this area.
Turn it into plain function calls instead which makes
the code a bit uglier but I find the simpler debugging
much more valuable.

An added bonus is that it is smaller too:
   text	   data	    bss	    dec	    hex	filename
   1268	    212	      0	   1480	    5c8	lib_ppc/board.org
   1224	     92	      0	   1316	    524	lib_ppc/board.new

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---

 So I had to do debug this area again and I am getting really
 tiered of following function pointers so here goes
 my old patch again.

 arch/powerpc/lib/board.c |  123 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 75 insertions(+), 48 deletions(-)

diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c
index 765f97a..f0160e6 100644
--- a/arch/powerpc/lib/board.c
+++ b/arch/powerpc/lib/board.c
@@ -157,7 +157,6 @@ ulong monitor_flash_len;
  * argument, and returns an integer return code, where 0 means
  * "continue" and != 0 means "fatal error, hang the system".
  */
-typedef int (init_fnc_t) (void);
 
 /************************************************************************
  * Init Utilities							*
@@ -236,17 +235,17 @@ static int init_func_watchdog_init (void)
 	WATCHDOG_RESET ();
 	return (0);
 }
-# define INIT_FUNC_WATCHDOG_INIT	init_func_watchdog_init,
+# define INIT_FUNC_WATCHDOG_INIT	init_func_watchdog_init()
 
 static int init_func_watchdog_reset (void)
 {
 	WATCHDOG_RESET ();
 	return (0);
 }
-# define INIT_FUNC_WATCHDOG_RESET	init_func_watchdog_reset,
+# define INIT_FUNC_WATCHDOG_RESET	init_func_watchdog_reset()
 #else
-# define INIT_FUNC_WATCHDOG_INIT	/* undef */
-# define INIT_FUNC_WATCHDOG_RESET	/* undef */
+# define INIT_FUNC_WATCHDOG_INIT	0 /* undef */
+# define INIT_FUNC_WATCHDOG_RESET	0 /* undef */
 #endif /* CONFIG_WATCHDOG */
 
 /************************************************************************
@@ -254,76 +253,110 @@ static int init_func_watchdog_reset (void)
  ************************************************************************
  */
 
-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;
 #endif
 #ifdef CONFIG_SYS_ALLOC_DPRAM
 #if !defined(CONFIG_CPM2)
-	dpram_init,
+	if (dpram_init())
+		goto err_out;
 #endif
 #endif
 #if defined(CONFIG_BOARD_POSTCLK_INIT)
-	board_postclk_init,
+	if (board_postclk_init())
+		goto err_out;
 #endif
-	env_init,
+	if (env_init())
+		goto err_out;
 #if defined(CONFIG_8xx_CPUCLK_DEFAULT)
-	get_clocks_866,		/* get CPU and bus clocks according to the environment variable */
-	sdram_adjust_866,	/* adjust sdram refresh rate according to the new clock */
-	init_timebase,
-#endif
-	init_baudrate,
-	serial_init,
-	console_init_f,
-	display_options,
+	if (get_clocks_866())
+		goto err_out;	/* get CPU and bus clocks according to the environment variable */
+	if (sdram_adjust_866())
+		goto err_out;	/* adjust sdram refresh rate according to the new clock */
+	if (init_timebase())
+		goto err_out;
+#endif
+	if (init_baudrate())
+		goto err_out;
+	if (serial_init())
+		goto err_out;
+	if (console_init_f())
+		goto err_out;
+	if (display_options())
+		goto err_out;
 #if defined(CONFIG_8260)
-	prt_8260_rsr,
-	prt_8260_clks,
+	if (prt_8260_rsr())
+		goto err_out;
+	if (prt_8260_clks())
+		goto err_out;
 #endif /* CONFIG_8260 */
 #if defined(CONFIG_MPC83xx)
-	prt_83xx_rsr,
+	if (prt_83xx_rsr())
+		goto err_out;
 #endif
-	checkcpu,
+	if (checkcpu())
+		goto err_out;
 #if defined(CONFIG_MPC5xxx)
-	prt_mpc5xxx_clks,
+	if (prt_mpc5xxx_clks())
+		goto err_out;
 #endif /* CONFIG_MPC5xxx */
 #if defined(CONFIG_MPC8220)
-	prt_mpc8220_clks,
+	if (prt_mpc8220_clks())
+		goto err_out;
 #endif
-	checkboard,
-	INIT_FUNC_WATCHDOG_INIT
+	if (checkboard())
+		goto err_out;
+	if (INIT_FUNC_WATCHDOG_INIT)
+		goto err_out;
 #if defined(CONFIG_MISC_INIT_F)
-	misc_init_f,
+	if (misc_init_f())
+		goto err_out;
 #endif
-	INIT_FUNC_WATCHDOG_RESET
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
 #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
-	init_func_i2c,
+	if (init_func_i2c())
+		goto err_out;
 #endif
 #if defined(CONFIG_HARD_SPI)
-	init_func_spi,
+	if (init_func_spi())
+		goto err_out;
 #endif
 #ifdef CONFIG_POST
-	post_init_f,
+	if (post_init_f())
+		goto err_out;
 #endif
-	INIT_FUNC_WATCHDOG_RESET
-	init_func_ram,
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
+	if (init_func_ram())
+		goto err_out;
 #if defined(CONFIG_SYS_DRAM_TEST)
-	testdram,
+	if (testdram())
+		goto err_out;
 #endif /* CONFIG_SYS_DRAM_TEST */
-	INIT_FUNC_WATCHDOG_RESET
-
-	NULL,			/* Terminate this list */
+	if (INIT_FUNC_WATCHDOG_RESET)
+		goto err_out;
+	return;
+err_out:
+	hang();
 };
 
 ulong get_effective_memsize(void)
@@ -366,7 +399,6 @@ void board_init_f (ulong bootflag)
 	ulong len, addr, addr_sp;
 	ulong *s;
 	gd_t *id;
-	init_fnc_t **init_fnc_ptr;
 #ifdef CONFIG_PRAM
 	int i;
 	ulong reg;
@@ -383,12 +415,7 @@ void board_init_f (ulong bootflag)
 	/* Clear initial global data */
 	memset ((void *) gd, 0, sizeof (gd_t));
 #endif
-
-	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
-		if ((*init_fnc_ptr) () != 0) {
-			hang ();
-		}
-	}
+	init_sequence();
 
 	/*
 	 * Now that we have DRAM mapped and working, we can
-- 
1.6.4.4

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> init_sequence is an array with function pointers which
> are really hard to follow when you need to debug this area.
> Turn it into plain function calls instead which makes
> the code a bit uglier but I find the simpler debugging
> much more valuable.

This is indeed much uglier.  What exactly is your problem with
debugging the existing code?

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
Another war ... must it always be so?  How many comrades have we lost
in this way? ...  Obedience.  Duty.  Death, and more death ...
	-- Romulan Commander, "Balance of Terror", stardate 1709.2

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47:
>
> Dear Joakim Tjernlund,
>
> In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> > init_sequence is an array with function pointers which
> > are really hard to follow when you need to debug this area.
> > Turn it into plain function calls instead which makes
> > the code a bit uglier but I find the simpler debugging
> > much more valuable.
>
> This is indeed much uglier.  What exactly is your problem with
> debugging the existing code?

Whenever I screw up so that one of the init funcs crashes, often without
any trace on the RS232 port you don't know which one. Single stepping
though the loop is cumbersome and not easy as BDI tends to
flush the cache when it stops so you loose your stack.
The other way is to look up one those funs and set a BP there and hope
for the best. Then repeat with the next function and so on.
Compare that with just setting a BP in the new init_sequence(), it is
fast and easy to move around.

I don't think you have been chasing bugs in this area for a long time,
if you had, you would appreciate how easy it is with functions
compared with a bunch of function ptrs.
I don't think this is much uglier, just a bit, but far more
useful and I have a hard time buying into "beautiful trumps usefulness".

 Jocke

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

[Scott Wood]

On Mon, 6 Dec 2010 21:42:28 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47:
> >
> > Dear Joakim Tjernlund,
> >
> > In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> > > init_sequence is an array with function pointers which
> > > are really hard to follow when you need to debug this area.
> > > Turn it into plain function calls instead which makes
> > > the code a bit uglier but I find the simpler debugging
> > > much more valuable.
> >
> > This is indeed much uglier.  What exactly is your problem with
> > debugging the existing code?
> 
> Whenever I screw up so that one of the init funcs crashes, often without
> any trace on the RS232 port you don't know which one. Single stepping
> though the loop is cumbersome and not easy as BDI tends to
> flush the cache when it stops so you loose your stack.
> The other way is to look up one those funs and set a BP there and hope
> for the best. Then repeat with the next function and so on.
> Compare that with just setting a BP in the new init_sequence(), it is
> fast and easy to move around.
> 
> I don't think you have been chasing bugs in this area for a long time,
> if you had, you would appreciate how easy it is with functions
> compared with a bunch of function ptrs.
> I don't think this is much uglier, just a bit, but far more
> useful and I have a hard time buying into "beautiful trumps usefulness".

I think it's easier with the function pointers -- if you want to debug
a hang in that phase of the boot, just have the loop print the address
of each function before it calls it.

-Scott

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

[Graeme Russ]

On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, 6 Dec 2010 21:42:28 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
>> Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 21:09:47:
>> >
>> > Dear Joakim Tjernlund,
>> >
>> > In message <1291658370-26367-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
>> > > init_sequence is an array with function pointers which
>> > > are really hard to follow when you need to debug this area.
>> > > Turn it into plain function calls instead which makes
>> > > the code a bit uglier but I find the simpler debugging
>> > > much more valuable.
>> >
>> > This is indeed much uglier. ?What exactly is your problem with
>> > debugging the existing code?
>>
>> Whenever I screw up so that one of the init funcs crashes, often without
>> any trace on the RS232 port you don't know which one. Single stepping
>> though the loop is cumbersome and not easy as BDI tends to
>> flush the cache when it stops so you loose your stack.
>> The other way is to look up one those funs and set a BP there and hope
>> for the best. Then repeat with the next function and so on.
>> Compare that with just setting a BP in the new init_sequence(), it is
>> fast and easy to move around.
>>
>> I don't think you have been chasing bugs in this area for a long time,
>> if you had, you would appreciate how easy it is with functions
>> compared with a bunch of function ptrs.
>> I don't think this is much uglier, just a bit, but far more
>> useful and I have a hard time buying into "beautiful trumps usefulness".
>
> I think it's easier with the function pointers -- if you want to debug
> a hang in that phase of the boot, just have the loop print the address
> of each function before it calls it.

I agree, but you can't print the address before you have console output. I
notice that console_init_f() can be up to 13th in the list of initialisation
functions - How often is that the case? There seems to be a lot of SDRAM
initialisation prior to getting console output which, to me, seems a little
strange - surely console output can be achieved earlier (even if it is using
a hard-coded baud rate)

I've now got x86 Cache-As-RAM working and console output is in about
position four in the init sequence - from there I debug using printf()

I don't think it will be long before board_init_f() gets unified into common
code and I would hate to see this uglyness spread to all arches.

Regards,

Graeme

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

[Scott Wood]

On Tue, 7 Dec 2010 09:36:40 +1100
Graeme Russ <graeme.russ@gmail.com> wrote:

> On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> > I think it's easier with the function pointers -- if you want to debug
> > a hang in that phase of the boot, just have the loop print the address
> > of each function before it calls it.
> 
> I agree, but you can't print the address before you have console output.

It's usually not too hard to hack something together, even if it's too
early for normal console output -- but I'd expect most problems to be
either before the initialization list, or after the console is working.

> I notice that console_init_f() can be up to 13th in the list of initialisation
> functions - How often is that the case? There seems to be a lot of SDRAM
> initialisation prior to getting console output which, to me, seems a little
> strange - surely console output can be achieved earlier (even if it is using
> a hard-coded baud rate)

I don't see "a lot of SDRAM initialization" -- there's
adjust_sdram_tbs_8xx, but that's really just setting up a couple clock
registers (and it's only for 8xx).  It's not the real SDRAM init.

-Scott

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

[Graeme Russ]

On Tue, Dec 7, 2010 at 9:49 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 7 Dec 2010 09:36:40 +1100
> Graeme Russ <graeme.russ@gmail.com> wrote:
>
>> On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
>> > I think it's easier with the function pointers -- if you want to debug
>> > a hang in that phase of the boot, just have the loop print the address
>> > of each function before it calls it.
>>
>> I agree, but you can't print the address before you have console output.
>
> It's usually not too hard to hack something together, even if it's too
> early for normal console output -- but I'd expect most problems to be
> either before the initialization list, or after the console is working.
>
>> I notice that console_init_f() can be up to 13th in the list of initialisation
>> functions - How often is that the case? There seems to be a lot of SDRAM
>> initialisation prior to getting console output which, to me, seems a little
>> strange - surely console output can be achieved earlier (even if it is using
>> a hard-coded baud rate)
>
> I don't see "a lot of SDRAM initialization" -- there's
> adjust_sdram_tbs_8xx, but that's really just setting up a couple clock
> registers (and it's only for 8xx). ?It's not the real SDRAM init.

And sdram_adjust_866() - True, not 'a lot'

But then there is dpram_init() as well which does not look like a
pre-req to console

Regards,

Graeme

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

[Scott Wood]

On Tue, 7 Dec 2010 10:04:23 +1100
Graeme Russ <graeme.russ@gmail.com> wrote:

> And sdram_adjust_866() - True, not 'a lot'
> 
> But then there is dpram_init() as well which does not look like a
> pre-req to console

DPRAM is not general purpose memory -- it actually is used by the
serial hardware.  And dpram_init() is virtually too simple to fail --
just setting two gd_t fields to constants.

-Scott

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <AANLkTimuPTiR6USC7k9srnGtSXYep+0DxGz9FWHVsgJd@mail.gmail.com> you wrote:
>
> And sdram_adjust_866() - True, not 'a lot'
> 
> But then there is dpram_init() as well which does not look like a
> pre-req to console

It is. On some 8xx system you have to load microcode and thus need to
initialize the DPRAM before you can configure the console port.

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
Unix is supported by IBM, like a hanging man is supported by rope.
		        - Don Libes & Sandy Ressler: _Life With Unix_

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

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04:
>
> On Tue, 7 Dec 2010 09:36:40 +1100
> Graeme Russ <graeme.russ@gmail.com> wrote:
>
> > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> > > I think it's easier with the function pointers -- if you want to debug
> > > a hang in that phase of the boot, just have the loop print the address
> > > of each function before it calls it.
> >
> > I agree, but you can't print the address before you have console output.
>
> It's usually not too hard to hack something together, even if it's too
> early for normal console output -- but I'd expect most problems to be
> either before the initialization list, or after the console is working.
>
> > I notice that console_init_f() can be up to 13th in the list of initialisation
> > functions - How often is that the case? There seems to be a lot of SDRAM
> > initialisation prior to getting console output which, to me, seems a little
> > strange - surely console output can be achieved earlier (even if it is using
> > a hard-coded baud rate)
>
> I don't see "a lot of SDRAM initialization" -- there's
> adjust_sdram_tbs_8xx, but that's really just setting up a couple clock
> registers (and it's only for 8xx).  It's not the real SDRAM init.

Scott, listen to yourself. You are proposing that one should turn the
code inside out and scan the map files just do this simple thing.

What is so valuable with func ptrs that you think it is worth it?

 Jocke

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

[Scott Wood]

On Tue, 7 Dec 2010 01:07:30 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04:
> >
> > On Tue, 7 Dec 2010 09:36:40 +1100
> > Graeme Russ <graeme.russ@gmail.com> wrote:
> >
> > > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> > > > I think it's easier with the function pointers -- if you want to debug
> > > > a hang in that phase of the boot, just have the loop print the address
> > > > of each function before it calls it.
> > >
> > > I agree, but you can't print the address before you have console output.
> >
> > It's usually not too hard to hack something together, even if it's too
> > early for normal console output -- but I'd expect most problems to be
> > either before the initialization list, or after the console is working.
> >
> > > I notice that console_init_f() can be up to 13th in the list of initialisation
> > > functions - How often is that the case? There seems to be a lot of SDRAM
> > > initialisation prior to getting console output which, to me, seems a little
> > > strange - surely console output can be achieved earlier (even if it is using
> > > a hard-coded baud rate)
> >
> > I don't see "a lot of SDRAM initialization" -- there's
> > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock
> > registers (and it's only for 8xx).  It's not the real SDRAM init.
> 
> Scott, listen to yourself. You are proposing that one should turn the
> code inside out and scan the map files just do this simple thing.

It looks like you're the one turning this code inside out. :-)

No need to scan anything yourself; let the tools (gdb/addr2line/etc) do
it.

> What is so valuable with func ptrs that you think it is worth it?

As I said, I think it's at least as easy to debug the way it is.  And
you admitted you found the new way uglier...

Plus, maybe someday we'll get real section-list initfuncs.

-Scott

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

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 2010/12/07 01:21:44:
>
> On Tue, 7 Dec 2010 01:07:30 +0100
> Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
>
> > Scott Wood <scottwood@freescale.com> wrote on 2010/12/06 23:49:04:
> > >
> > > On Tue, 7 Dec 2010 09:36:40 +1100
> > > Graeme Russ <graeme.russ@gmail.com> wrote:
> > >
> > > > On Tue, Dec 7, 2010 at 8:33 AM, Scott Wood <scottwood@freescale.com> wrote:
> > > > > I think it's easier with the function pointers -- if you want to debug
> > > > > a hang in that phase of the boot, just have the loop print the address
> > > > > of each function before it calls it.
> > > >
> > > > I agree, but you can't print the address before you have console output.
> > >
> > > It's usually not too hard to hack something together, even if it's too
> > > early for normal console output -- but I'd expect most problems to be
> > > either before the initialization list, or after the console is working.
> > >
> > > > I notice that console_init_f() can be up to 13th in the list of initialisation
> > > > functions - How often is that the case? There seems to be a lot of SDRAM
> > > > initialisation prior to getting console output which, to me, seems a little
> > > > strange - surely console output can be achieved earlier (even if it is using
> > > > a hard-coded baud rate)
> > >
> > > I don't see "a lot of SDRAM initialization" -- there's
> > > adjust_sdram_tbs_8xx, but that's really just setting up a couple clock
> > > registers (and it's only for 8xx).  It's not the real SDRAM init.
> >
> > Scott, listen to yourself. You are proposing that one should turn the
> > code inside out and scan the map files just do this simple thing.
>
> It looks like you're the one turning this code inside out. :-)

Some days it feels so :)

>
> No need to scan anything yourself; let the tools (gdb/addr2line/etc) do
> it.

gdb yes, addr2line/etc no. Needing to resort to external tools
when gdb and a sensible code layout would do the trick? No thanks.

>
> > What is so valuable with func ptrs that you think it is worth it?
>
> As I said, I think it's at least as easy to debug the way it is.  And
> you admitted you found the new way uglier...

You haven't tried my way yet. Suppose we littered the code with
function ptrs, would still feel it is easier using addr2line/etc each and
every function call?

>
> Plus, maybe someday we'll get real section-list initfuncs.

One day that hasn't come fore years yet and may never come and probably
fixable with weak functions.

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

[Scott Wood]

On Tue, 7 Dec 2010 01:41:22 +0100
Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:

> Scott Wood <scottwood@freescale.com> wrote on 2010/12/07 01:21:44:
> >
> > On Tue, 7 Dec 2010 01:07:30 +0100
> > Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> > > Scott, listen to yourself. You are proposing that one should turn the
> > > code inside out and scan the map files just do this simple thing.
> >
> > It looks like you're the one turning this code inside out. :-)
> 
> Some days it feels so :)
> 
> >
> > No need to scan anything yourself; let the tools (gdb/addr2line/etc) do
> > it.
> 
> gdb yes, addr2line/etc no. Needing to resort to external tools
> when gdb and a sensible code layout would do the trick? No thanks.

gdb is an external tool for me most of the time. :-)

It's what I use for lookup anyway (usually I'll want more context), I
just mentioned addr2line as an alternative.  Either one beats looking
at the map file (why?).

> > > What is so valuable with func ptrs that you think it is worth it?
> >
> > As I said, I think it's at least as easy to debug the way it is.  And
> > you admitted you found the new way uglier...
> 
> You haven't tried my way yet.

I usually don't even have a BDI hooked up. :-P

I've done breakpoint/step debugging.  Sometimes it's the right tool.
Usually printf works better for me, particularly if there's overhead
to setting up a debugger.

> > Plus, maybe someday we'll get real section-list initfuncs.
> 
> One day that hasn't come fore years yet and may never come

I can still hope. :-)

> and probably fixable with weak functions.

Weak functions are not a good substitute.

-Scott

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

[Wolfgang Denk]

Dear Graeme Russ,

In message <AANLkTikH3O9gZWsuEaKn-BK+8iAhUeRyk2iL+ezZz-y1@mail.gmail.com> you wrote:
>
> I agree, but you can't print the address before you have console output. I
> notice that console_init_f() can be up to 13th in the list of initialisation
> functions - How often is that the case? There seems to be a lot of SDRAM
> initialisation prior to getting console output which, to me, seems a little

No, this is only on broken system that don't play by the rules.  See
http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#6_Keep_it_Debuggable

Board / architecture maintainers that ignore the design principles
deserve some punishment ;-)

> I don't think it will be long before board_init_f() gets unified into common
> code and I would hate to see this uglyness spread to all arches.

Full ACK.

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
When a woman marries again it is because she detested her first  hus-
band.  When  a  man  marries again, it is because he adored his first
wife.                                                  -- Oscar Wilde

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote:
>
> > This is indeed much uglier.  What exactly is your problem with
> > debugging the existing code?
> 
> Whenever I screw up so that one of the init funcs crashes, often without
> any trace on the RS232 port you don't know which one. Single stepping

Well, we turn on the serial ports as early as possible, so ther eis
only a pretty small collection that is candidate for such a situation.

> though the loop is cumbersome and not easy as BDI tends to
> flush the cache when it stops so you loose your stack.

Does it? On which architecture / processor is this?

> The other way is to look up one those funs and set a BP there and hope
> for the best. Then repeat with the next function and so on.
> Compare that with just setting a BP in the new init_sequence(), it is
> fast and easy to move around.

...but ugly to read.  And if you really want to introduce this style,
it has to be done for all architectures, as I want to see this code to
become common across architectures.

> I don't think you have been chasing bugs in this area for a long time,
> if you had, you would appreciate how easy it is with functions
> compared with a bunch of function ptrs.

I haven't, lately, that's true, or when I had, I always knew pretty
well where I had to expect the issues.  But I've been there before,
many, many times.

> I don't think this is much uglier, just a bit, but far more
> useful and I have a hard time buying into "beautiful trumps usefulness".

I'm not convinced that your code is actually a general improvement.  I
understand that it's useful for you, and your style of debugging.
If you are really hanging in this area, then maybe a debug() is more
prowerfull - and less invasive.

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
In a survey taken several years ago, all  incoming  freshmen  at  MIT
were  asked  if  they  expected  to graduate in the top half of their
class. Ninety-seven percent responded that they did.

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

[Graeme Russ]

On Tue, Dec 7, 2010 at 9:36 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Joakim Tjernlund,
>
> In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote:
>>

[snip]

>> The other way is to look up one those funs and set a BP there and hope
>> for the best. Then repeat with the next function and so on.
>> Compare that with just setting a BP in the new init_sequence(), it is
>> fast and easy to move around.
>
> ...but ugly to read. ?And if you really want to introduce this style,
> it has to be done for all architectures, as I want to see this code to
> become common across architectures.
>

Moving away from the array based init sequence also promotes the posibility
to break away from the very strict function prototype (void parameter
returning int) which will lead to even more ugliness in the future

Regards,

Graeme

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/12/06 23:36:21:
>
> Dear Joakim Tjernlund,
>
> In message <OFEE7ADEEA.47556DD1-ONC12577F1.006FAF8D-C12577F1.0071C097@transmode.se> you wrote:
> >
> > > This is indeed much uglier.  What exactly is your problem with
> > > debugging the existing code?
> >
> > Whenever I screw up so that one of the init funcs crashes, often without
> > any trace on the RS232 port you don't know which one. Single stepping
>
> Well, we turn on the serial ports as early as possible, so ther eis
> only a pretty small collection that is candidate for such a situation.

But that won't help much. When you don't have any output it can be any
reason. I really want to see where it breaks down. That will tell me more
about what I did wrong earlier. It is easy to guess where when
you a playing with these function, but when you are trying to understand why
your latest relocation fixes doesn't work, it is a different game.

>
> > though the loop is cumbersome and not easy as BDI tends to
> > flush the cache when it stops so you loose your stack.
>
> Does it? On which architecture / processor is this?

MPC8321 and BDI2000. I have to play games with some internal
BDI command called SAP. Maybe they have fixed this in later models?

>
> > The other way is to look up one those funs and set a BP there and hope
> > for the best. Then repeat with the next function and so on.
> > Compare that with just setting a BP in the new init_sequence(), it is
> > fast and easy to move around.
>
> ...but ugly to read.  And if you really want to introduce this style,
> it has to be done for all architectures, as I want to see this code to
> become common across architectures.

That is the next issue, lets not stray from the path ..

>
> > I don't think you have been chasing bugs in this area for a long time,
> > if you had, you would appreciate how easy it is with functions
> > compared with a bunch of function ptrs.
>
> I haven't, lately, that's true, or when I had, I always knew pretty
> well where I had to expect the issues.  But I've been there before,
> many, many times.

And you never found it annoying when you hit these function ptrs?

>
> > I don't think this is much uglier, just a bit, but far more
> > useful and I have a hard time buying into "beautiful trumps usefulness".
>
> I'm not convinced that your code is actually a general improvement.  I
> understand that it's useful for you, and your style of debugging.
> If you are really hanging in this area, then maybe a debug() is more
> prowerfull - and less invasive.

Now you are proposing that I instrument the code, much like Scott and his
print of address idea. This is exactly what I want to avoid as it would
probably have to be adapted from case to case and there is always the
case when there is no output and then I am back to square zero again.

The argument that it is uglier alone is not a very good one.
Image the roles were reversed, I am sure you would dismiss me as
"less beautiful" is really not an argument alone.

 Jocke

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OF9FF97B1B.1FC2BD05-ONC12577F2.0000D034-C12577F2.0002F125@transmode.se> you wrote:
>
> > > though the loop is cumbersome and not easy as BDI tends to
> > > flush the cache when it stops so you loose your stack.
> >
> > Does it? On which architecture / processor is this?
> 
> MPC8321 and BDI2000. I have to play games with some internal
> BDI command called SAP. Maybe they have fixed this in later models?

Are you using the latest firmware, i. . 1.31 from October 2009?

> > ...but ugly to read.  And if you really want to introduce this style,
> > it has to be done for all architectures, as I want to see this code to
> > become common across architectures.
> 
> That is the next issue, lets not stray from the path ..

Right, that's the next issue, lets not stray from the path and put
additional road blocks into our way when we already know where we are
heading to.

> > I haven't, lately, that's true, or when I had, I always knew pretty
> > well where I had to expect the issues.  But I've been there before,
> > many, many times.
> 
> And you never found it annoying when you hit these function ptrs?

Actually, no.

In an earlier life I've been programming in Forth for some time, so I
find a list of addresses to process quite natural and efficient.

> Now you are proposing that I instrument the code, much like Scott and his
> print of address idea. This is exactly what I want to avoid as it would
> probably have to be adapted from case to case and there is always the
> case when there is no output and then I am back to square zero again.

Don't you use oneliners in the shell that you just make up as needed
and then throw away, and reinvent when you need them again?

Same here. Adding a printf() or similar to some code under test is
something I do several times a day. Even when running under a
debugger, and even when I'm not executing a list of function pointers.
I see no issue with adding this on a case-by-case base, had-tailored
to the specific problem I'm tracking down.

> The argument that it is uglier alone is not a very good one.
> Image the roles were reversed, I am sure you would dismiss me as
> "less beautiful" is really not an argument alone.

Ugliness was only one of several arguments, and there were several
votes againt yours.

Please consider the case closed.  I already marked the patch as NAKed.

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
The complexity of software is an essential property, not an  acciden-
tal  one. Hence, descriptions of a software entity that abstract away
its complexity often abstract away its essence.    - Fred Brooks, Jr.

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/12/07 07:34:49:
>
> Dear Joakim Tjernlund,
>
> In message <OF9FF97B1B.1FC2BD05-ONC12577F2.0000D034-C12577F2.0002F125@transmode.se> you wrote:
> >
> > > > though the loop is cumbersome and not easy as BDI tends to
> > > > flush the cache when it stops so you loose your stack.
> > >
> > > Does it? On which architecture / processor is this?
> >
> > MPC8321 and BDI2000. I have to play games with some internal
> > BDI command called SAP. Maybe they have fixed this in later models?
>
> Are you using the latest firmware, i. . 1.31 from October 2009?

No, got an earlier one, 1.24

>
> > > ...but ugly to read.  And if you really want to introduce this style,
> > > it has to be done for all architectures, as I want to see this code to
> > > become common across architectures.
> >
> > That is the next issue, lets not stray from the path ..
>
> Right, that's the next issue, lets not stray from the path and put
> additional road blocks into our way when we already know where we are
> heading to.

hmm, not sure how to read this. Irony? I was not trying to dodge
the subject, just wanted to keep the discussion focused. Of course
this can be adapted on all archs.

>
> > > I haven't, lately, that's true, or when I had, I always knew pretty
> > > well where I had to expect the issues.  But I've been there before,
> > > many, many times.
> >
> > And you never found it annoying when you hit these function ptrs?
>
> Actually, no.
>
> In an earlier life I've been programming in Forth for some time, so I
> find a list of addresses to process quite natural and efficient.
>
> > Now you are proposing that I instrument the code, much like Scott and his
> > print of address idea. This is exactly what I want to avoid as it would
> > probably have to be adapted from case to case and there is always the
> > case when there is no output and then I am back to square zero again.
>
> Don't you use oneliners in the shell that you just make up as needed
> and then throw away, and reinvent when you need them again?
>
> Same here. Adding a printf() or similar to some code under test is
> something I do several times a day. Even when running under a
> debugger, and even when I'm not executing a list of function pointers.
> I see no issue with adding this on a case-by-case base, had-tailored
> to the specific problem I'm tracking down.

Yes, this is a useful technique but it costs time. You don't
add these printout unless you need them, right? This is my
point. If you move to pure function calls you don't have to add
them just to identify what function crashes or lookup addresses etc.
Just move a BPs around without recompiling and reloading the code.

>
> > The argument that it is uglier alone is not a very good one.
> > Image the roles were reversed, I am sure you would dismiss me as
> > "less beautiful" is really not an argument alone.
>
> Ugliness was only one of several arguments, and there were several
> votes againt yours.

There was votes against with false claims that is is just as easy
to debug function ptrs.
And there was Scott which one day hope to have "real section-list initfuncs"
Bottom line argument is that the new code is uglier and that trumps
any of my arguments.

>
> Please consider the case closed.  I already marked the patch as NAKed.

I see, thanks for your time.

 Jocke

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

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OFB493743B.4A802F8E-ONC12577F2.002BC478-C12577F2.003241B1@transmode.se> you wrote:
>
> > > MPC8321 and BDI2000. I have to play games with some internal
> > > BDI command called SAP. Maybe they have fixed this in later models?
> >
> > Are you using the latest firmware, i. . 1.31 from October 2009?
> 
> No, got an earlier one, 1.24

Well, 1.24 is from April 2006.  Information about the MPC8321 must
have been pretty limited at that time.  Please update.  [Let me know
if you want me to send you a quote for the latest firmware :-) ]

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
"...this does not mean that some of us should not want, in  a  rather
dispassionate sort of way, to put a bullet through csh's head."
                   - Larry Wall in <1992Aug6.221512.5963@netlabs.com>

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

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/12/07 14:07:56:
>
> Dear Joakim Tjernlund,
>
> In message <OFB493743B.4A802F8E-ONC12577F2.002BC478-C12577F2.003241B1@transmode.se> you wrote:
> >
> > > > MPC8321 and BDI2000. I have to play games with some internal
> > > > BDI command called SAP. Maybe they have fixed this in later models?
> > >
> > > Are you using the latest firmware, i. . 1.31 from October 2009?
> >
> > No, got an earlier one, 1.24
>
> Well, 1.24 is from April 2006.  Information about the MPC8321 must

No surprise there considering we were beta testers of MPC8321. We
actually had to nag on Freescale to send Abatron an eval board so
we could buy a BDI2000 with MPC832x support :)

> have been pretty limited at that time.  Please update.  [Let me know
> if you want me to send you a quote for the latest firmware :-) ]

He, I sent an email to Abatron and they already replied with the
firmware we are entitled to which is somewhat newer than 1.24 so I will
try that one first :)

 Jocke