[U-Boot] [PATCH] ppc: transform init_sequence into a function.
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
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
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 :)
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
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@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@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
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
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
Wolfgang Denk wd@denx.de wrote on 30/11/2009 22:02:44:
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.
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
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
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
participants (4)
-
Detlev Zundel -
Joakim Tjernlund -
Joakim Tjernlund
-
Wolfgang Denk