| Message ID | 252364e53d56fab1c77bda74563b24ad8a17d635.1734537135.git.jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Static initcalls | expand |
Hi Jerome, On 12/18/24 4:53 PM, Jerome Forissier wrote: > Make static calls instead of iterating over the init_sequence_f arrays. > Tested on a KV260 board (xilinx_zynqmp_kria_defconfig). > > - With LTO enabled, the code size reported by bloat-o-meter is 1200 > bytes less (-0.11%) > - With LTO disabled, the code is 594 bytes smaller (-0.05%) > - Execution time does not change in a noticeable way > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > --- > common/board_f.c | 165 +++++++++++++++++++++++---------------------- > include/initcall.h | 27 ++++++++ > 2 files changed, 110 insertions(+), 82 deletions(-) > > diff --git a/common/board_f.c b/common/board_f.c > index a4d8850cb7d..cebed85ed4d 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -38,6 +38,7 @@ > #include <spl.h> > #include <status_led.h> > #include <sysreset.h> > +#include <time.h> > #include <timer.h> > #include <trace.h> > #include <upl.h> > @@ -870,58 +871,60 @@ static int initf_upl(void) > return 0; > } > > -static const init_fnc_t init_sequence_f[] = { > - setup_mon_len, > - CONFIG_IS_ENABLED(OF_CONTROL, (fdtdec_setup,)) > - CONFIG_IS_ENABLED(TRACE_EARLY, (trace_early_init,)) > - initf_malloc, > - initf_upl, > - log_init, > - initf_bootstage, /* uses its own timer, so does not need DM */ > - event_init, > - bloblist_maybe_init, > - setup_spl_handoff, > - CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, (console_record_init,)) > - INITCALL_EVENT(EVT_FSP_INIT_F), > - arch_cpu_init, /* basic arch cpu dependent setup */ > - mach_cpu_init, /* SoC/machine dependent CPU setup */ > - initf_dm, > - CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (board_early_init_f,)) > +static void initcall_run_f(void) > +{ > + INITCALL(setup_mon_len); > + CONFIG_IS_ENABLED(OF_CONTROL, (INITCALL(fdtdec_setup))); > + CONFIG_IS_ENABLED(TRACE_EARLY, (INITCALL(trace_early_init))); > + INITCALL(initf_malloc); > + INITCALL(initf_upl); > + INITCALL(log_init); > + INITCALL(initf_bootstage); /* uses its own timer, so does not need DM */ > + INITCALL(event_init); > + INITCALL(bloblist_maybe_init); > + INITCALL(setup_spl_handoff); > + CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, > + (INITCALL(console_record_init);)) > + INITCALL_EVT(EVT_FSP_INIT_F); > + INITCALL(arch_cpu_init); /* basic arch cpu dependent setup */ > + INITCALL(mach_cpu_init); /* SoC/machine dependent CPU setup */ > + INITCALL(initf_dm); > + CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (INITCALL(board_early_init_f);)) > #if defined(CONFIG_PPC) || defined(CONFIG_SYS_FSL_CLK) || defined(CONFIG_M68K) > /* get CPU and bus clocks according to the environment variable */ > - get_clocks, /* get CPU and bus clocks (etc.) */ > + INITCALL(get_clocks); /* get CPU and bus clocks (etc.) */ > #endif > #if !defined(CONFIG_M68K) || (defined(CONFIG_M68K) && !defined(CONFIG_MCFTMR)) > - timer_init, /* initialize timer */ > + INITCALL(timer_init); /* initialize timer */ > #endif > - CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, (board_postclk_init,)) > - env_init, /* initialize environment */ > - init_baud_rate, /* initialze baudrate settings */ > - serial_init, /* serial communications setup */ > - console_init_f, /* stage 1 init of console */ > - display_options, /* say that we are here */ > - display_text_info, /* show debugging info if required */ > - checkcpu, > - CONFIG_IS_ENABLED(SYSRESET, (print_resetinfo,)) > + CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, > + (INITCALL(board_postclk_init);)) > + INITCALL(env_init); /* initialize environment */ > + INITCALL(init_baud_rate); /* initialze baudrate settings */ > + INITCALL(serial_init); /* serial communications setup */ > + INITCALL(console_init_f); /* stage 1 init of console */ > + INITCALL(display_options); /* say that we are here */ > + INITCALL(display_text_info); /* show debugging info if required */ > + INITCALL(checkcpu); > + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) > /* display cpu info (and speed) */ > - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) > - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) > - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) > - INIT_FUNC_WATCHDOG_INIT > - INITCALL_EVENT(EVT_MISC_INIT_F), > - INIT_FUNC_WATCHDOG_RESET > - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) > - announce_dram_init, > - dram_init, /* configure available RAM banks */ > - CONFIG_IS_ENABLED(POST, (post_init_f,)) > - INIT_FUNC_WATCHDOG_RESET > + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) > + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) > + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) > + WATCHDOG_INIT(); > + INITCALL_EVT(EVT_MISC_INIT_F); > + WATCHDOG_RESET(); > + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) > + INITCALL(announce_dram_init); > + INITCALL(dram_init); /* configure available RAM banks */ > + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) > + WATCHDOG_INIT(); > #if defined(CFG_SYS_DRAM_TEST) > - testdram, > + INITCALL(testdram); > #endif /* CFG_SYS_DRAM_TEST */ > - INIT_FUNC_WATCHDOG_RESET > - > - CONFIG_IS_ENABLED(POST, (init_post,)) > - INIT_FUNC_WATCHDOG_RESET > + WATCHDOG_RESET(); > + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) > + WATCHDOG_RESET(); > /* > * Now that we have DRAM mapped and working, we can > * relocate the code and continue running from DRAM. > @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { > * - monitor code > * - board info struct > */ > - setup_dest_addr, > + INITCALL(setup_dest_addr); > #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) > - fix_fdt, > + INITCALL(fix_fdt); > #endif > #ifdef CFG_PRAM > - reserve_pram, > + INITCALL(reserve_pram); > #endif > - reserve_round_4k, > - setup_relocaddr_from_bloblist, > - arch_reserve_mmu, > - reserve_video, > - reserve_trace, > - reserve_uboot, > - reserve_malloc, > - reserve_board, > - reserve_global_data, > - reserve_fdt, > + INITCALL(reserve_round_4k); > + INITCALL(setup_relocaddr_from_bloblist); > + INITCALL(arch_reserve_mmu); > + INITCALL(reserve_video); > + INITCALL(reserve_trace); > + INITCALL(reserve_uboot); > + INITCALL(reserve_malloc); > + INITCALL(reserve_board); > + INITCALL(reserve_global_data); > + INITCALL(reserve_fdt); > #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) > - reloc_fdt, > - fix_fdt, > + INITCALL(reloc_fdt); > + INITCALL(fix_fdt); > #endif > - reserve_bootstage, > - reserve_bloblist, > - reserve_arch, > - reserve_stacks, > - dram_init_banksize, > - show_dram_config, > - INIT_FUNC_WATCHDOG_RESET > - setup_bdinfo, > - display_new_sp, > - INIT_FUNC_WATCHDOG_RESET > + INITCALL(reserve_bootstage); > + INITCALL(reserve_bloblist); > + INITCALL(reserve_arch); > + INITCALL(reserve_stacks); > + INITCALL(dram_init_banksize); > + INITCALL(show_dram_config); > + WATCHDOG_RESET(); > + INITCALL(setup_bdinfo); > + INITCALL(display_new_sp); > + WATCHDOG_RESET(); > #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) > - reloc_fdt, > + INITCALL(reloc_fdt); > #endif > - reloc_bootstage, > - reloc_bloblist, > - setup_reloc, > + INITCALL(reloc_bootstage); > + INITCALL(reloc_bloblist); > + INITCALL(setup_reloc); > #if defined(CONFIG_X86) || defined(CONFIG_ARC) > - copy_uboot_to_ram, > - do_elf_reloc_fixups, > + INITCALL(copy_uboot_to_ram); > + INITCALL(do_elf_reloc_fixups); > #endif > - clear_bss, > + INITCALL(clear_bss); > /* > * Deregister all cyclic functions before relocation, so that > * gd->cyclic_list does not contain any references to pre-relocation > @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { > * This should happen as late as possible so that the window where a > * watchdog device is not serviced is as small as possible. > */ > - cyclic_unregister_all, > + INITCALL(cyclic_unregister_all); > #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) > - jump_to_copy, > + INITCALL(jump_to_copy); > #endif > - NULL, > -}; > +} > > void board_init_f(ulong boot_flags) > { > @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) > gd->flags &= ~GD_FLG_HAVE_CONSOLE; > gd->boardf = &boardf; > > - if (initcall_run_list(init_sequence_f)) > - hang(); > + initcall_run_f(); > > #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ > !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ > diff --git a/include/initcall.h b/include/initcall.h > index 62d3bb67f08..9398a3ec7d5 100644 > --- a/include/initcall.h > +++ b/include/initcall.h > @@ -8,6 +8,7 @@ > > #include <asm/types.h> > #include <event.h> > +#include <hang.h> > > _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); > > @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); > */ > int initcall_run_list(const init_fnc_t init_sequence[]); > > +#define INITCALL(_call) \ > + do { \ > + if (_call()) { \ > + debug("%s(): calling %s() failed\n", __func__, \ > + #_call); \ > + hang(); \ > + } \ > + } while (0) > + > +#define INITCALL_EVT(_evt) \ > + do { \ > + if (event_notify_null(_evt)) { \ > + debug("%s(): event %d/%s failed\n", __func__, _evt, \ > + event_type_name(_evt)) ; \ > + hang(); \ > + } \ > + } while (0) > + initcall_run_list() currently prints (printf) whenever an initcall fails. It's happened to me a lot already while debugging/bringing up boards to have an initcall fail on me and that message wasn't really enough to put me on the right track, but at least I had something. Now I believe this would just hang without notifying you before. Is my understanding correct? Would it be possible to print an error message (or raise the loglevel from debug to error or something like that?) before hang()? Cheers, Quentin
Hi Quentin, On 12/18/24 18:27, Quentin Schulz wrote: > Hi Jerome, > > On 12/18/24 4:53 PM, Jerome Forissier wrote: >> Make static calls instead of iterating over the init_sequence_f arrays. >> Tested on a KV260 board (xilinx_zynqmp_kria_defconfig). >> >> - With LTO enabled, the code size reported by bloat-o-meter is 1200 >> bytes less (-0.11%) >> - With LTO disabled, the code is 594 bytes smaller (-0.05%) >> - Execution time does not change in a noticeable way >> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >> --- >> common/board_f.c | 165 +++++++++++++++++++++++---------------------- >> include/initcall.h | 27 ++++++++ >> 2 files changed, 110 insertions(+), 82 deletions(-) >> >> diff --git a/common/board_f.c b/common/board_f.c >> index a4d8850cb7d..cebed85ed4d 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -38,6 +38,7 @@ >> #include <spl.h> >> #include <status_led.h> >> #include <sysreset.h> >> +#include <time.h> >> #include <timer.h> >> #include <trace.h> >> #include <upl.h> >> @@ -870,58 +871,60 @@ static int initf_upl(void) >> return 0; >> } >> -static const init_fnc_t init_sequence_f[] = { >> - setup_mon_len, >> - CONFIG_IS_ENABLED(OF_CONTROL, (fdtdec_setup,)) >> - CONFIG_IS_ENABLED(TRACE_EARLY, (trace_early_init,)) >> - initf_malloc, >> - initf_upl, >> - log_init, >> - initf_bootstage, /* uses its own timer, so does not need DM */ >> - event_init, >> - bloblist_maybe_init, >> - setup_spl_handoff, >> - CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, (console_record_init,)) >> - INITCALL_EVENT(EVT_FSP_INIT_F), >> - arch_cpu_init, /* basic arch cpu dependent setup */ >> - mach_cpu_init, /* SoC/machine dependent CPU setup */ >> - initf_dm, >> - CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (board_early_init_f,)) >> +static void initcall_run_f(void) >> +{ >> + INITCALL(setup_mon_len); >> + CONFIG_IS_ENABLED(OF_CONTROL, (INITCALL(fdtdec_setup))); >> + CONFIG_IS_ENABLED(TRACE_EARLY, (INITCALL(trace_early_init))); >> + INITCALL(initf_malloc); >> + INITCALL(initf_upl); >> + INITCALL(log_init); >> + INITCALL(initf_bootstage); /* uses its own timer, so does not need DM */ >> + INITCALL(event_init); >> + INITCALL(bloblist_maybe_init); >> + INITCALL(setup_spl_handoff); >> + CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, >> + (INITCALL(console_record_init);)) >> + INITCALL_EVT(EVT_FSP_INIT_F); >> + INITCALL(arch_cpu_init); /* basic arch cpu dependent setup */ >> + INITCALL(mach_cpu_init); /* SoC/machine dependent CPU setup */ >> + INITCALL(initf_dm); >> + CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (INITCALL(board_early_init_f);)) >> #if defined(CONFIG_PPC) || defined(CONFIG_SYS_FSL_CLK) || defined(CONFIG_M68K) >> /* get CPU and bus clocks according to the environment variable */ >> - get_clocks, /* get CPU and bus clocks (etc.) */ >> + INITCALL(get_clocks); /* get CPU and bus clocks (etc.) */ >> #endif >> #if !defined(CONFIG_M68K) || (defined(CONFIG_M68K) && !defined(CONFIG_MCFTMR)) >> - timer_init, /* initialize timer */ >> + INITCALL(timer_init); /* initialize timer */ >> #endif >> - CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, (board_postclk_init,)) >> - env_init, /* initialize environment */ >> - init_baud_rate, /* initialze baudrate settings */ >> - serial_init, /* serial communications setup */ >> - console_init_f, /* stage 1 init of console */ >> - display_options, /* say that we are here */ >> - display_text_info, /* show debugging info if required */ >> - checkcpu, >> - CONFIG_IS_ENABLED(SYSRESET, (print_resetinfo,)) >> + CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, >> + (INITCALL(board_postclk_init);)) >> + INITCALL(env_init); /* initialize environment */ >> + INITCALL(init_baud_rate); /* initialze baudrate settings */ >> + INITCALL(serial_init); /* serial communications setup */ >> + INITCALL(console_init_f); /* stage 1 init of console */ >> + INITCALL(display_options); /* say that we are here */ >> + INITCALL(display_text_info); /* show debugging info if required */ >> + INITCALL(checkcpu); >> + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) >> /* display cpu info (and speed) */ >> - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) >> - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) >> - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) >> - INIT_FUNC_WATCHDOG_INIT >> - INITCALL_EVENT(EVT_MISC_INIT_F), >> - INIT_FUNC_WATCHDOG_RESET >> - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) >> - announce_dram_init, >> - dram_init, /* configure available RAM banks */ >> - CONFIG_IS_ENABLED(POST, (post_init_f,)) >> - INIT_FUNC_WATCHDOG_RESET >> + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) >> + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) >> + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) >> + WATCHDOG_INIT(); >> + INITCALL_EVT(EVT_MISC_INIT_F); >> + WATCHDOG_RESET(); >> + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) >> + INITCALL(announce_dram_init); >> + INITCALL(dram_init); /* configure available RAM banks */ >> + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) >> + WATCHDOG_INIT(); >> #if defined(CFG_SYS_DRAM_TEST) >> - testdram, >> + INITCALL(testdram); >> #endif /* CFG_SYS_DRAM_TEST */ >> - INIT_FUNC_WATCHDOG_RESET >> - >> - CONFIG_IS_ENABLED(POST, (init_post,)) >> - INIT_FUNC_WATCHDOG_RESET >> + WATCHDOG_RESET(); >> + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) >> + WATCHDOG_RESET(); >> /* >> * Now that we have DRAM mapped and working, we can >> * relocate the code and continue running from DRAM. >> @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { >> * - monitor code >> * - board info struct >> */ >> - setup_dest_addr, >> + INITCALL(setup_dest_addr); >> #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) >> - fix_fdt, >> + INITCALL(fix_fdt); >> #endif >> #ifdef CFG_PRAM >> - reserve_pram, >> + INITCALL(reserve_pram); >> #endif >> - reserve_round_4k, >> - setup_relocaddr_from_bloblist, >> - arch_reserve_mmu, >> - reserve_video, >> - reserve_trace, >> - reserve_uboot, >> - reserve_malloc, >> - reserve_board, >> - reserve_global_data, >> - reserve_fdt, >> + INITCALL(reserve_round_4k); >> + INITCALL(setup_relocaddr_from_bloblist); >> + INITCALL(arch_reserve_mmu); >> + INITCALL(reserve_video); >> + INITCALL(reserve_trace); >> + INITCALL(reserve_uboot); >> + INITCALL(reserve_malloc); >> + INITCALL(reserve_board); >> + INITCALL(reserve_global_data); >> + INITCALL(reserve_fdt); >> #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) >> - reloc_fdt, >> - fix_fdt, >> + INITCALL(reloc_fdt); >> + INITCALL(fix_fdt); >> #endif >> - reserve_bootstage, >> - reserve_bloblist, >> - reserve_arch, >> - reserve_stacks, >> - dram_init_banksize, >> - show_dram_config, >> - INIT_FUNC_WATCHDOG_RESET >> - setup_bdinfo, >> - display_new_sp, >> - INIT_FUNC_WATCHDOG_RESET >> + INITCALL(reserve_bootstage); >> + INITCALL(reserve_bloblist); >> + INITCALL(reserve_arch); >> + INITCALL(reserve_stacks); >> + INITCALL(dram_init_banksize); >> + INITCALL(show_dram_config); >> + WATCHDOG_RESET(); >> + INITCALL(setup_bdinfo); >> + INITCALL(display_new_sp); >> + WATCHDOG_RESET(); >> #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) >> - reloc_fdt, >> + INITCALL(reloc_fdt); >> #endif >> - reloc_bootstage, >> - reloc_bloblist, >> - setup_reloc, >> + INITCALL(reloc_bootstage); >> + INITCALL(reloc_bloblist); >> + INITCALL(setup_reloc); >> #if defined(CONFIG_X86) || defined(CONFIG_ARC) >> - copy_uboot_to_ram, >> - do_elf_reloc_fixups, >> + INITCALL(copy_uboot_to_ram); >> + INITCALL(do_elf_reloc_fixups); >> #endif >> - clear_bss, >> + INITCALL(clear_bss); >> /* >> * Deregister all cyclic functions before relocation, so that >> * gd->cyclic_list does not contain any references to pre-relocation >> @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { >> * This should happen as late as possible so that the window where a >> * watchdog device is not serviced is as small as possible. >> */ >> - cyclic_unregister_all, >> + INITCALL(cyclic_unregister_all); >> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) >> - jump_to_copy, >> + INITCALL(jump_to_copy); >> #endif >> - NULL, >> -}; >> +} >> void board_init_f(ulong boot_flags) >> { >> @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) >> gd->flags &= ~GD_FLG_HAVE_CONSOLE; >> gd->boardf = &boardf; >> - if (initcall_run_list(init_sequence_f)) >> - hang(); >> + initcall_run_f(); >> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ >> !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ >> diff --git a/include/initcall.h b/include/initcall.h >> index 62d3bb67f08..9398a3ec7d5 100644 >> --- a/include/initcall.h >> +++ b/include/initcall.h >> @@ -8,6 +8,7 @@ >> #include <asm/types.h> >> #include <event.h> >> +#include <hang.h> >> _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); >> @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); >> */ >> int initcall_run_list(const init_fnc_t init_sequence[]); >> +#define INITCALL(_call) \ >> + do { \ >> + if (_call()) { \ >> + debug("%s(): calling %s() failed\n", __func__, \ >> + #_call); \ >> + hang(); \ >> + } \ >> + } while (0) >> + >> +#define INITCALL_EVT(_evt) \ >> + do { \ >> + if (event_notify_null(_evt)) { \ >> + debug("%s(): event %d/%s failed\n", __func__, _evt, \ >> + event_type_name(_evt)) ; \ >> + hang(); \ >> + } \ >> + } while (0) >> + > > initcall_run_list() currently prints (printf) whenever an initcall fails. It's happened to me a lot already while debugging/bringing up boards to have an initcall fail on me and that message wasn't really enough to put me on the right track, but at least I had something. Now I believe this would just hang without notifying you before. Is my understanding correct? The debug print is still there, the line just before hang() ;-) Cheers,
Hi Jerome, On 12/18/24 6:38 PM, Jerome Forissier wrote: > Hi Quentin, > > On 12/18/24 18:27, Quentin Schulz wrote: >> Hi Jerome, >> >> On 12/18/24 4:53 PM, Jerome Forissier wrote: >>> Make static calls instead of iterating over the init_sequence_f arrays. >>> Tested on a KV260 board (xilinx_zynqmp_kria_defconfig). >>> >>> - With LTO enabled, the code size reported by bloat-o-meter is 1200 >>> bytes less (-0.11%) >>> - With LTO disabled, the code is 594 bytes smaller (-0.05%) >>> - Execution time does not change in a noticeable way >>> >>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >>> --- >>> common/board_f.c | 165 +++++++++++++++++++++++---------------------- >>> include/initcall.h | 27 ++++++++ >>> 2 files changed, 110 insertions(+), 82 deletions(-) >>> >>> diff --git a/common/board_f.c b/common/board_f.c >>> index a4d8850cb7d..cebed85ed4d 100644 >>> --- a/common/board_f.c >>> +++ b/common/board_f.c >>> @@ -38,6 +38,7 @@ >>> #include <spl.h> >>> #include <status_led.h> >>> #include <sysreset.h> >>> +#include <time.h> >>> #include <timer.h> >>> #include <trace.h> >>> #include <upl.h> >>> @@ -870,58 +871,60 @@ static int initf_upl(void) >>> return 0; >>> } >>> -static const init_fnc_t init_sequence_f[] = { >>> - setup_mon_len, >>> - CONFIG_IS_ENABLED(OF_CONTROL, (fdtdec_setup,)) >>> - CONFIG_IS_ENABLED(TRACE_EARLY, (trace_early_init,)) >>> - initf_malloc, >>> - initf_upl, >>> - log_init, >>> - initf_bootstage, /* uses its own timer, so does not need DM */ >>> - event_init, >>> - bloblist_maybe_init, >>> - setup_spl_handoff, >>> - CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, (console_record_init,)) >>> - INITCALL_EVENT(EVT_FSP_INIT_F), >>> - arch_cpu_init, /* basic arch cpu dependent setup */ >>> - mach_cpu_init, /* SoC/machine dependent CPU setup */ >>> - initf_dm, >>> - CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (board_early_init_f,)) >>> +static void initcall_run_f(void) >>> +{ >>> + INITCALL(setup_mon_len); >>> + CONFIG_IS_ENABLED(OF_CONTROL, (INITCALL(fdtdec_setup))); >>> + CONFIG_IS_ENABLED(TRACE_EARLY, (INITCALL(trace_early_init))); >>> + INITCALL(initf_malloc); >>> + INITCALL(initf_upl); >>> + INITCALL(log_init); >>> + INITCALL(initf_bootstage); /* uses its own timer, so does not need DM */ >>> + INITCALL(event_init); >>> + INITCALL(bloblist_maybe_init); >>> + INITCALL(setup_spl_handoff); >>> + CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, >>> + (INITCALL(console_record_init);)) >>> + INITCALL_EVT(EVT_FSP_INIT_F); >>> + INITCALL(arch_cpu_init); /* basic arch cpu dependent setup */ >>> + INITCALL(mach_cpu_init); /* SoC/machine dependent CPU setup */ >>> + INITCALL(initf_dm); >>> + CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (INITCALL(board_early_init_f);)) >>> #if defined(CONFIG_PPC) || defined(CONFIG_SYS_FSL_CLK) || defined(CONFIG_M68K) >>> /* get CPU and bus clocks according to the environment variable */ >>> - get_clocks, /* get CPU and bus clocks (etc.) */ >>> + INITCALL(get_clocks); /* get CPU and bus clocks (etc.) */ >>> #endif >>> #if !defined(CONFIG_M68K) || (defined(CONFIG_M68K) && !defined(CONFIG_MCFTMR)) >>> - timer_init, /* initialize timer */ >>> + INITCALL(timer_init); /* initialize timer */ >>> #endif >>> - CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, (board_postclk_init,)) >>> - env_init, /* initialize environment */ >>> - init_baud_rate, /* initialze baudrate settings */ >>> - serial_init, /* serial communications setup */ >>> - console_init_f, /* stage 1 init of console */ >>> - display_options, /* say that we are here */ >>> - display_text_info, /* show debugging info if required */ >>> - checkcpu, >>> - CONFIG_IS_ENABLED(SYSRESET, (print_resetinfo,)) >>> + CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, >>> + (INITCALL(board_postclk_init);)) >>> + INITCALL(env_init); /* initialize environment */ >>> + INITCALL(init_baud_rate); /* initialze baudrate settings */ >>> + INITCALL(serial_init); /* serial communications setup */ >>> + INITCALL(console_init_f); /* stage 1 init of console */ >>> + INITCALL(display_options); /* say that we are here */ >>> + INITCALL(display_text_info); /* show debugging info if required */ >>> + INITCALL(checkcpu); >>> + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) >>> /* display cpu info (and speed) */ >>> - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) >>> - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) >>> - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) >>> - INIT_FUNC_WATCHDOG_INIT >>> - INITCALL_EVENT(EVT_MISC_INIT_F), >>> - INIT_FUNC_WATCHDOG_RESET >>> - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) >>> - announce_dram_init, >>> - dram_init, /* configure available RAM banks */ >>> - CONFIG_IS_ENABLED(POST, (post_init_f,)) >>> - INIT_FUNC_WATCHDOG_RESET >>> + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) >>> + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) >>> + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) >>> + WATCHDOG_INIT(); >>> + INITCALL_EVT(EVT_MISC_INIT_F); >>> + WATCHDOG_RESET(); >>> + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) >>> + INITCALL(announce_dram_init); >>> + INITCALL(dram_init); /* configure available RAM banks */ >>> + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) >>> + WATCHDOG_INIT(); >>> #if defined(CFG_SYS_DRAM_TEST) >>> - testdram, >>> + INITCALL(testdram); >>> #endif /* CFG_SYS_DRAM_TEST */ >>> - INIT_FUNC_WATCHDOG_RESET >>> - >>> - CONFIG_IS_ENABLED(POST, (init_post,)) >>> - INIT_FUNC_WATCHDOG_RESET >>> + WATCHDOG_RESET(); >>> + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) >>> + WATCHDOG_RESET(); >>> /* >>> * Now that we have DRAM mapped and working, we can >>> * relocate the code and continue running from DRAM. >>> @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { >>> * - monitor code >>> * - board info struct >>> */ >>> - setup_dest_addr, >>> + INITCALL(setup_dest_addr); >>> #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) >>> - fix_fdt, >>> + INITCALL(fix_fdt); >>> #endif >>> #ifdef CFG_PRAM >>> - reserve_pram, >>> + INITCALL(reserve_pram); >>> #endif >>> - reserve_round_4k, >>> - setup_relocaddr_from_bloblist, >>> - arch_reserve_mmu, >>> - reserve_video, >>> - reserve_trace, >>> - reserve_uboot, >>> - reserve_malloc, >>> - reserve_board, >>> - reserve_global_data, >>> - reserve_fdt, >>> + INITCALL(reserve_round_4k); >>> + INITCALL(setup_relocaddr_from_bloblist); >>> + INITCALL(arch_reserve_mmu); >>> + INITCALL(reserve_video); >>> + INITCALL(reserve_trace); >>> + INITCALL(reserve_uboot); >>> + INITCALL(reserve_malloc); >>> + INITCALL(reserve_board); >>> + INITCALL(reserve_global_data); >>> + INITCALL(reserve_fdt); >>> #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) >>> - reloc_fdt, >>> - fix_fdt, >>> + INITCALL(reloc_fdt); >>> + INITCALL(fix_fdt); >>> #endif >>> - reserve_bootstage, >>> - reserve_bloblist, >>> - reserve_arch, >>> - reserve_stacks, >>> - dram_init_banksize, >>> - show_dram_config, >>> - INIT_FUNC_WATCHDOG_RESET >>> - setup_bdinfo, >>> - display_new_sp, >>> - INIT_FUNC_WATCHDOG_RESET >>> + INITCALL(reserve_bootstage); >>> + INITCALL(reserve_bloblist); >>> + INITCALL(reserve_arch); >>> + INITCALL(reserve_stacks); >>> + INITCALL(dram_init_banksize); >>> + INITCALL(show_dram_config); >>> + WATCHDOG_RESET(); >>> + INITCALL(setup_bdinfo); >>> + INITCALL(display_new_sp); >>> + WATCHDOG_RESET(); >>> #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) >>> - reloc_fdt, >>> + INITCALL(reloc_fdt); >>> #endif >>> - reloc_bootstage, >>> - reloc_bloblist, >>> - setup_reloc, >>> + INITCALL(reloc_bootstage); >>> + INITCALL(reloc_bloblist); >>> + INITCALL(setup_reloc); >>> #if defined(CONFIG_X86) || defined(CONFIG_ARC) >>> - copy_uboot_to_ram, >>> - do_elf_reloc_fixups, >>> + INITCALL(copy_uboot_to_ram); >>> + INITCALL(do_elf_reloc_fixups); >>> #endif >>> - clear_bss, >>> + INITCALL(clear_bss); >>> /* >>> * Deregister all cyclic functions before relocation, so that >>> * gd->cyclic_list does not contain any references to pre-relocation >>> @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { >>> * This should happen as late as possible so that the window where a >>> * watchdog device is not serviced is as small as possible. >>> */ >>> - cyclic_unregister_all, >>> + INITCALL(cyclic_unregister_all); >>> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) >>> - jump_to_copy, >>> + INITCALL(jump_to_copy); >>> #endif >>> - NULL, >>> -}; >>> +} >>> void board_init_f(ulong boot_flags) >>> { >>> @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) >>> gd->flags &= ~GD_FLG_HAVE_CONSOLE; >>> gd->boardf = &boardf; >>> - if (initcall_run_list(init_sequence_f)) >>> - hang(); >>> + initcall_run_f(); >>> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ >>> !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ >>> diff --git a/include/initcall.h b/include/initcall.h >>> index 62d3bb67f08..9398a3ec7d5 100644 >>> --- a/include/initcall.h >>> +++ b/include/initcall.h >>> @@ -8,6 +8,7 @@ >>> #include <asm/types.h> >>> #include <event.h> >>> +#include <hang.h> >>> _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); >>> @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); >>> */ >>> int initcall_run_list(const init_fnc_t init_sequence[]); >>> +#define INITCALL(_call) \ >>> + do { \ >>> + if (_call()) { \ >>> + debug("%s(): calling %s() failed\n", __func__, \ >>> + #_call); \ >>> + hang(); \ >>> + } \ >>> + } while (0) >>> + >>> +#define INITCALL_EVT(_evt) \ >>> + do { \ >>> + if (event_notify_null(_evt)) { \ >>> + debug("%s(): event %d/%s failed\n", __func__, _evt, \ >>> + event_type_name(_evt)) ; \ >>> + hang(); \ >>> + } \ >>> + } while (0) >>> + >> >> initcall_run_list() currently prints (printf) whenever an initcall fails. It's happened to me a lot already while debugging/bringing up boards to have an initcall fail on me and that message wasn't really enough to put me on the right track, but at least I had something. Now I believe this would just hang without notifying you before. Is my understanding correct? > > The debug print is still there, the line just before hang() ;-) > It's now a debug() instead of a printf() is what I meant. So it's not shown by default anymore I believe, unless we have #define DEBUG in the file? Cheers, Quentin
On 12/18/24 18:47, Quentin Schulz wrote: > Hi Jerome, > > On 12/18/24 6:38 PM, Jerome Forissier wrote: >> Hi Quentin, >> >> On 12/18/24 18:27, Quentin Schulz wrote: >>> Hi Jerome, >>> >>> On 12/18/24 4:53 PM, Jerome Forissier wrote: >>>> Make static calls instead of iterating over the init_sequence_f arrays. >>>> Tested on a KV260 board (xilinx_zynqmp_kria_defconfig). >>>> >>>> - With LTO enabled, the code size reported by bloat-o-meter is 1200 >>>> bytes less (-0.11%) >>>> - With LTO disabled, the code is 594 bytes smaller (-0.05%) >>>> - Execution time does not change in a noticeable way >>>> >>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >>>> --- >>>> common/board_f.c | 165 +++++++++++++++++++++++---------------------- >>>> include/initcall.h | 27 ++++++++ >>>> 2 files changed, 110 insertions(+), 82 deletions(-) >>>> >>>> diff --git a/common/board_f.c b/common/board_f.c >>>> index a4d8850cb7d..cebed85ed4d 100644 >>>> --- a/common/board_f.c >>>> +++ b/common/board_f.c >>>> @@ -38,6 +38,7 @@ >>>> #include <spl.h> >>>> #include <status_led.h> >>>> #include <sysreset.h> >>>> +#include <time.h> >>>> #include <timer.h> >>>> #include <trace.h> >>>> #include <upl.h> >>>> @@ -870,58 +871,60 @@ static int initf_upl(void) >>>> return 0; >>>> } >>>> -static const init_fnc_t init_sequence_f[] = { >>>> - setup_mon_len, >>>> - CONFIG_IS_ENABLED(OF_CONTROL, (fdtdec_setup,)) >>>> - CONFIG_IS_ENABLED(TRACE_EARLY, (trace_early_init,)) >>>> - initf_malloc, >>>> - initf_upl, >>>> - log_init, >>>> - initf_bootstage, /* uses its own timer, so does not need DM */ >>>> - event_init, >>>> - bloblist_maybe_init, >>>> - setup_spl_handoff, >>>> - CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, (console_record_init,)) >>>> - INITCALL_EVENT(EVT_FSP_INIT_F), >>>> - arch_cpu_init, /* basic arch cpu dependent setup */ >>>> - mach_cpu_init, /* SoC/machine dependent CPU setup */ >>>> - initf_dm, >>>> - CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (board_early_init_f,)) >>>> +static void initcall_run_f(void) >>>> +{ >>>> + INITCALL(setup_mon_len); >>>> + CONFIG_IS_ENABLED(OF_CONTROL, (INITCALL(fdtdec_setup))); >>>> + CONFIG_IS_ENABLED(TRACE_EARLY, (INITCALL(trace_early_init))); >>>> + INITCALL(initf_malloc); >>>> + INITCALL(initf_upl); >>>> + INITCALL(log_init); >>>> + INITCALL(initf_bootstage); /* uses its own timer, so does not need DM */ >>>> + INITCALL(event_init); >>>> + INITCALL(bloblist_maybe_init); >>>> + INITCALL(setup_spl_handoff); >>>> + CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, >>>> + (INITCALL(console_record_init);)) >>>> + INITCALL_EVT(EVT_FSP_INIT_F); >>>> + INITCALL(arch_cpu_init); /* basic arch cpu dependent setup */ >>>> + INITCALL(mach_cpu_init); /* SoC/machine dependent CPU setup */ >>>> + INITCALL(initf_dm); >>>> + CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (INITCALL(board_early_init_f);)) >>>> #if defined(CONFIG_PPC) || defined(CONFIG_SYS_FSL_CLK) || defined(CONFIG_M68K) >>>> /* get CPU and bus clocks according to the environment variable */ >>>> - get_clocks, /* get CPU and bus clocks (etc.) */ >>>> + INITCALL(get_clocks); /* get CPU and bus clocks (etc.) */ >>>> #endif >>>> #if !defined(CONFIG_M68K) || (defined(CONFIG_M68K) && !defined(CONFIG_MCFTMR)) >>>> - timer_init, /* initialize timer */ >>>> + INITCALL(timer_init); /* initialize timer */ >>>> #endif >>>> - CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, (board_postclk_init,)) >>>> - env_init, /* initialize environment */ >>>> - init_baud_rate, /* initialze baudrate settings */ >>>> - serial_init, /* serial communications setup */ >>>> - console_init_f, /* stage 1 init of console */ >>>> - display_options, /* say that we are here */ >>>> - display_text_info, /* show debugging info if required */ >>>> - checkcpu, >>>> - CONFIG_IS_ENABLED(SYSRESET, (print_resetinfo,)) >>>> + CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, >>>> + (INITCALL(board_postclk_init);)) >>>> + INITCALL(env_init); /* initialize environment */ >>>> + INITCALL(init_baud_rate); /* initialze baudrate settings */ >>>> + INITCALL(serial_init); /* serial communications setup */ >>>> + INITCALL(console_init_f); /* stage 1 init of console */ >>>> + INITCALL(display_options); /* say that we are here */ >>>> + INITCALL(display_text_info); /* show debugging info if required */ >>>> + INITCALL(checkcpu); >>>> + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) >>>> /* display cpu info (and speed) */ >>>> - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) >>>> - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) >>>> - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) >>>> - INIT_FUNC_WATCHDOG_INIT >>>> - INITCALL_EVENT(EVT_MISC_INIT_F), >>>> - INIT_FUNC_WATCHDOG_RESET >>>> - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) >>>> - announce_dram_init, >>>> - dram_init, /* configure available RAM banks */ >>>> - CONFIG_IS_ENABLED(POST, (post_init_f,)) >>>> - INIT_FUNC_WATCHDOG_RESET >>>> + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) >>>> + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) >>>> + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) >>>> + WATCHDOG_INIT(); >>>> + INITCALL_EVT(EVT_MISC_INIT_F); >>>> + WATCHDOG_RESET(); >>>> + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) >>>> + INITCALL(announce_dram_init); >>>> + INITCALL(dram_init); /* configure available RAM banks */ >>>> + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) >>>> + WATCHDOG_INIT(); >>>> #if defined(CFG_SYS_DRAM_TEST) >>>> - testdram, >>>> + INITCALL(testdram); >>>> #endif /* CFG_SYS_DRAM_TEST */ >>>> - INIT_FUNC_WATCHDOG_RESET >>>> - >>>> - CONFIG_IS_ENABLED(POST, (init_post,)) >>>> - INIT_FUNC_WATCHDOG_RESET >>>> + WATCHDOG_RESET(); >>>> + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) >>>> + WATCHDOG_RESET(); >>>> /* >>>> * Now that we have DRAM mapped and working, we can >>>> * relocate the code and continue running from DRAM. >>>> @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { >>>> * - monitor code >>>> * - board info struct >>>> */ >>>> - setup_dest_addr, >>>> + INITCALL(setup_dest_addr); >>>> #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) >>>> - fix_fdt, >>>> + INITCALL(fix_fdt); >>>> #endif >>>> #ifdef CFG_PRAM >>>> - reserve_pram, >>>> + INITCALL(reserve_pram); >>>> #endif >>>> - reserve_round_4k, >>>> - setup_relocaddr_from_bloblist, >>>> - arch_reserve_mmu, >>>> - reserve_video, >>>> - reserve_trace, >>>> - reserve_uboot, >>>> - reserve_malloc, >>>> - reserve_board, >>>> - reserve_global_data, >>>> - reserve_fdt, >>>> + INITCALL(reserve_round_4k); >>>> + INITCALL(setup_relocaddr_from_bloblist); >>>> + INITCALL(arch_reserve_mmu); >>>> + INITCALL(reserve_video); >>>> + INITCALL(reserve_trace); >>>> + INITCALL(reserve_uboot); >>>> + INITCALL(reserve_malloc); >>>> + INITCALL(reserve_board); >>>> + INITCALL(reserve_global_data); >>>> + INITCALL(reserve_fdt); >>>> #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) >>>> - reloc_fdt, >>>> - fix_fdt, >>>> + INITCALL(reloc_fdt); >>>> + INITCALL(fix_fdt); >>>> #endif >>>> - reserve_bootstage, >>>> - reserve_bloblist, >>>> - reserve_arch, >>>> - reserve_stacks, >>>> - dram_init_banksize, >>>> - show_dram_config, >>>> - INIT_FUNC_WATCHDOG_RESET >>>> - setup_bdinfo, >>>> - display_new_sp, >>>> - INIT_FUNC_WATCHDOG_RESET >>>> + INITCALL(reserve_bootstage); >>>> + INITCALL(reserve_bloblist); >>>> + INITCALL(reserve_arch); >>>> + INITCALL(reserve_stacks); >>>> + INITCALL(dram_init_banksize); >>>> + INITCALL(show_dram_config); >>>> + WATCHDOG_RESET(); >>>> + INITCALL(setup_bdinfo); >>>> + INITCALL(display_new_sp); >>>> + WATCHDOG_RESET(); >>>> #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) >>>> - reloc_fdt, >>>> + INITCALL(reloc_fdt); >>>> #endif >>>> - reloc_bootstage, >>>> - reloc_bloblist, >>>> - setup_reloc, >>>> + INITCALL(reloc_bootstage); >>>> + INITCALL(reloc_bloblist); >>>> + INITCALL(setup_reloc); >>>> #if defined(CONFIG_X86) || defined(CONFIG_ARC) >>>> - copy_uboot_to_ram, >>>> - do_elf_reloc_fixups, >>>> + INITCALL(copy_uboot_to_ram); >>>> + INITCALL(do_elf_reloc_fixups); >>>> #endif >>>> - clear_bss, >>>> + INITCALL(clear_bss); >>>> /* >>>> * Deregister all cyclic functions before relocation, so that >>>> * gd->cyclic_list does not contain any references to pre-relocation >>>> @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { >>>> * This should happen as late as possible so that the window where a >>>> * watchdog device is not serviced is as small as possible. >>>> */ >>>> - cyclic_unregister_all, >>>> + INITCALL(cyclic_unregister_all); >>>> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) >>>> - jump_to_copy, >>>> + INITCALL(jump_to_copy); >>>> #endif >>>> - NULL, >>>> -}; >>>> +} >>>> void board_init_f(ulong boot_flags) >>>> { >>>> @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) >>>> gd->flags &= ~GD_FLG_HAVE_CONSOLE; >>>> gd->boardf = &boardf; >>>> - if (initcall_run_list(init_sequence_f)) >>>> - hang(); >>>> + initcall_run_f(); >>>> #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ >>>> !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ >>>> diff --git a/include/initcall.h b/include/initcall.h >>>> index 62d3bb67f08..9398a3ec7d5 100644 >>>> --- a/include/initcall.h >>>> +++ b/include/initcall.h >>>> @@ -8,6 +8,7 @@ >>>> #include <asm/types.h> >>>> #include <event.h> >>>> +#include <hang.h> >>>> _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); >>>> @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); >>>> */ >>>> int initcall_run_list(const init_fnc_t init_sequence[]); >>>> +#define INITCALL(_call) \ >>>> + do { \ >>>> + if (_call()) { \ >>>> + debug("%s(): calling %s() failed\n", __func__, \ >>>> + #_call); \ >>>> + hang(); \ >>>> + } \ >>>> + } while (0) >>>> + >>>> +#define INITCALL_EVT(_evt) \ >>>> + do { \ >>>> + if (event_notify_null(_evt)) { \ >>>> + debug("%s(): event %d/%s failed\n", __func__, _evt, \ >>>> + event_type_name(_evt)) ; \ >>>> + hang(); \ >>>> + } \ >>>> + } while (0) >>>> + >>> >>> initcall_run_list() currently prints (printf) whenever an initcall fails. It's happened to me a lot already while debugging/bringing up boards to have an initcall fail on me and that message wasn't really enough to put me on the right track, but at least I had something. Now I believe this would just hang without notifying you before. Is my understanding correct? >> >> The debug print is still there, the line just before hang() ;-) >> > > It's now a debug() instead of a printf() is what I meant. So it's not shown by default anymore I believe, unless we have #define DEBUG in the file? Oh, right. There was a debug() before the initcall and a printf() in case of error. Sorry for overlooking this. I will change the debug() to a printf(). Thanks,
Hi Jerome [...] > + (INITCALL(board_postclk_init);)) > + INITCALL(env_init); /* initialize environment */ > + INITCALL(init_baud_rate); /* initialze baudrate settings */ > + INITCALL(serial_init); /* serial communications setup */ > + INITCALL(console_init_f); /* stage 1 init of console */ > + INITCALL(display_options); /* say that we are here */ > + INITCALL(display_text_info); /* show debugging info if required */ > + INITCALL(checkcpu); > + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) > /* display cpu info (and speed) */ > - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) > - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) > - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) > - INIT_FUNC_WATCHDOG_INIT > - INITCALL_EVENT(EVT_MISC_INIT_F), > - INIT_FUNC_WATCHDOG_RESET > - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) > - announce_dram_init, > - dram_init, /* configure available RAM banks */ > - CONFIG_IS_ENABLED(POST, (post_init_f,)) > - INIT_FUNC_WATCHDOG_RESET > + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) > + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) > + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) > + WATCHDOG_INIT(); > + INITCALL_EVT(EVT_MISC_INIT_F); > + WATCHDOG_RESET(); > + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) > + INITCALL(announce_dram_init); > + INITCALL(dram_init); /* configure available RAM banks */ > + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) > + WATCHDOG_INIT(); The previous function was calling init, reset, reset for the watchdog. The new code does init, reset, init, reset etc. Is this intentional? Was that a bug of the existing code? Thanks /Ilias > #if defined(CFG_SYS_DRAM_TEST) > - testdram, > + INITCALL(testdram); > #endif /* CFG_SYS_DRAM_TEST */ > - INIT_FUNC_WATCHDOG_RESET > - > - CONFIG_IS_ENABLED(POST, (init_post,)) > - INIT_FUNC_WATCHDOG_RESET > + WATCHDOG_RESET(); > + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) > + WATCHDOG_RESET(); > /* > * Now that we have DRAM mapped and working, we can > * relocate the code and continue running from DRAM. > @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { > * - monitor code > * - board info struct > */ > - setup_dest_addr, > + INITCALL(setup_dest_addr); > #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) > - fix_fdt, > + INITCALL(fix_fdt); > #endif > #ifdef CFG_PRAM > - reserve_pram, > + INITCALL(reserve_pram); > #endif > - reserve_round_4k, > - setup_relocaddr_from_bloblist, > - arch_reserve_mmu, > - reserve_video, > - reserve_trace, > - reserve_uboot, > - reserve_malloc, > - reserve_board, > - reserve_global_data, > - reserve_fdt, > + INITCALL(reserve_round_4k); > + INITCALL(setup_relocaddr_from_bloblist); > + INITCALL(arch_reserve_mmu); > + INITCALL(reserve_video); > + INITCALL(reserve_trace); > + INITCALL(reserve_uboot); > + INITCALL(reserve_malloc); > + INITCALL(reserve_board); > + INITCALL(reserve_global_data); > + INITCALL(reserve_fdt); > #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) > - reloc_fdt, > - fix_fdt, > + INITCALL(reloc_fdt); > + INITCALL(fix_fdt); > #endif > - reserve_bootstage, > - reserve_bloblist, > - reserve_arch, > - reserve_stacks, > - dram_init_banksize, > - show_dram_config, > - INIT_FUNC_WATCHDOG_RESET > - setup_bdinfo, > - display_new_sp, > - INIT_FUNC_WATCHDOG_RESET > + INITCALL(reserve_bootstage); > + INITCALL(reserve_bloblist); > + INITCALL(reserve_arch); > + INITCALL(reserve_stacks); > + INITCALL(dram_init_banksize); > + INITCALL(show_dram_config); > + WATCHDOG_RESET(); > + INITCALL(setup_bdinfo); > + INITCALL(display_new_sp); > + WATCHDOG_RESET(); > #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) > - reloc_fdt, > + INITCALL(reloc_fdt); > #endif > - reloc_bootstage, > - reloc_bloblist, > - setup_reloc, > + INITCALL(reloc_bootstage); > + INITCALL(reloc_bloblist); > + INITCALL(setup_reloc); > #if defined(CONFIG_X86) || defined(CONFIG_ARC) > - copy_uboot_to_ram, > - do_elf_reloc_fixups, > + INITCALL(copy_uboot_to_ram); > + INITCALL(do_elf_reloc_fixups); > #endif > - clear_bss, > + INITCALL(clear_bss); > /* > * Deregister all cyclic functions before relocation, so that > * gd->cyclic_list does not contain any references to pre-relocation > @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { > * This should happen as late as possible so that the window where a > * watchdog device is not serviced is as small as possible. > */ > - cyclic_unregister_all, > + INITCALL(cyclic_unregister_all); > #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) > - jump_to_copy, > + INITCALL(jump_to_copy); > #endif > - NULL, > -}; > +} > > void board_init_f(ulong boot_flags) > { > @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) > gd->flags &= ~GD_FLG_HAVE_CONSOLE; > gd->boardf = &boardf; > > - if (initcall_run_list(init_sequence_f)) > - hang(); > + initcall_run_f(); > > #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ > !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ > diff --git a/include/initcall.h b/include/initcall.h > index 62d3bb67f08..9398a3ec7d5 100644 > --- a/include/initcall.h > +++ b/include/initcall.h > @@ -8,6 +8,7 @@ > > #include <asm/types.h> > #include <event.h> > +#include <hang.h> > > _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); > > @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); > */ > int initcall_run_list(const init_fnc_t init_sequence[]); > > +#define INITCALL(_call) \ > + do { \ > + if (_call()) { \ > + debug("%s(): calling %s() failed\n", __func__, \ > + #_call); \ > + hang(); \ > + } \ > + } while (0) > + > +#define INITCALL_EVT(_evt) \ > + do { \ > + if (event_notify_null(_evt)) { \ > + debug("%s(): event %d/%s failed\n", __func__, _evt, \ > + event_type_name(_evt)) ; \ > + hang(); \ > + } \ > + } while (0) > + > +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) > +#define WATCHDOG_INIT() INITCALL(init_func_watchdog_init) > +#define WATCHDOG_RESET() INITCALL(init_func_watchdog_reset) > +#else > +#define WATCHDOG_INIT() > +#define WATCHDOG_RESET() > +#endif > + > #endif > -- > 2.43.0 >
diff --git a/common/board_f.c b/common/board_f.c index a4d8850cb7d..cebed85ed4d 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -38,6 +38,7 @@ #include <spl.h> #include <status_led.h> #include <sysreset.h> +#include <time.h> #include <timer.h> #include <trace.h> #include <upl.h> @@ -870,58 +871,60 @@ static int initf_upl(void) return 0; } -static const init_fnc_t init_sequence_f[] = { - setup_mon_len, - CONFIG_IS_ENABLED(OF_CONTROL, (fdtdec_setup,)) - CONFIG_IS_ENABLED(TRACE_EARLY, (trace_early_init,)) - initf_malloc, - initf_upl, - log_init, - initf_bootstage, /* uses its own timer, so does not need DM */ - event_init, - bloblist_maybe_init, - setup_spl_handoff, - CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, (console_record_init,)) - INITCALL_EVENT(EVT_FSP_INIT_F), - arch_cpu_init, /* basic arch cpu dependent setup */ - mach_cpu_init, /* SoC/machine dependent CPU setup */ - initf_dm, - CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (board_early_init_f,)) +static void initcall_run_f(void) +{ + INITCALL(setup_mon_len); + CONFIG_IS_ENABLED(OF_CONTROL, (INITCALL(fdtdec_setup))); + CONFIG_IS_ENABLED(TRACE_EARLY, (INITCALL(trace_early_init))); + INITCALL(initf_malloc); + INITCALL(initf_upl); + INITCALL(log_init); + INITCALL(initf_bootstage); /* uses its own timer, so does not need DM */ + INITCALL(event_init); + INITCALL(bloblist_maybe_init); + INITCALL(setup_spl_handoff); + CONFIG_IS_ENABLED(CONSOLE_RECORD_INIT_F, + (INITCALL(console_record_init);)) + INITCALL_EVT(EVT_FSP_INIT_F); + INITCALL(arch_cpu_init); /* basic arch cpu dependent setup */ + INITCALL(mach_cpu_init); /* SoC/machine dependent CPU setup */ + INITCALL(initf_dm); + CONFIG_IS_ENABLED(BOARD_EARLY_INIT_F, (INITCALL(board_early_init_f);)) #if defined(CONFIG_PPC) || defined(CONFIG_SYS_FSL_CLK) || defined(CONFIG_M68K) /* get CPU and bus clocks according to the environment variable */ - get_clocks, /* get CPU and bus clocks (etc.) */ + INITCALL(get_clocks); /* get CPU and bus clocks (etc.) */ #endif #if !defined(CONFIG_M68K) || (defined(CONFIG_M68K) && !defined(CONFIG_MCFTMR)) - timer_init, /* initialize timer */ + INITCALL(timer_init); /* initialize timer */ #endif - CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, (board_postclk_init,)) - env_init, /* initialize environment */ - init_baud_rate, /* initialze baudrate settings */ - serial_init, /* serial communications setup */ - console_init_f, /* stage 1 init of console */ - display_options, /* say that we are here */ - display_text_info, /* show debugging info if required */ - checkcpu, - CONFIG_IS_ENABLED(SYSRESET, (print_resetinfo,)) + CONFIG_IS_ENABLED(BOARD_POSTCLK_INIT, + (INITCALL(board_postclk_init);)) + INITCALL(env_init); /* initialize environment */ + INITCALL(init_baud_rate); /* initialze baudrate settings */ + INITCALL(serial_init); /* serial communications setup */ + INITCALL(console_init_f); /* stage 1 init of console */ + INITCALL(display_options); /* say that we are here */ + INITCALL(display_text_info); /* show debugging info if required */ + INITCALL(checkcpu); + CONFIG_IS_ENABLED(SYSRESET, (INITCALL(print_resetinfo);)) /* display cpu info (and speed) */ - CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (print_cpuinfo,)) - CONFIG_IS_ENABLED(DTB_RESELECT, (embedded_dtb_select,)) - CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (show_board_info,)) - INIT_FUNC_WATCHDOG_INIT - INITCALL_EVENT(EVT_MISC_INIT_F), - INIT_FUNC_WATCHDOG_RESET - CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (init_func_i2c,)) - announce_dram_init, - dram_init, /* configure available RAM banks */ - CONFIG_IS_ENABLED(POST, (post_init_f,)) - INIT_FUNC_WATCHDOG_RESET + CONFIG_IS_ENABLED(DISPLAY_CPUINFO, (INITCALL(print_cpuinfo);)) + CONFIG_IS_ENABLED(DTB_RESELECT, (INITCALL(embedded_dtb_select);)) + CONFIG_IS_ENABLED(DISPLAY_BOARDINFO, (INITCALL(show_board_info);)) + WATCHDOG_INIT(); + INITCALL_EVT(EVT_MISC_INIT_F); + WATCHDOG_RESET(); + CONFIG_IS_ENABLED(SYS_I2C_LEGACY, (INITCALL(init_func_i2c);)) + INITCALL(announce_dram_init); + INITCALL(dram_init); /* configure available RAM banks */ + CONFIG_IS_ENABLED(POST, (INITCALL(post_init_f);)) + WATCHDOG_INIT(); #if defined(CFG_SYS_DRAM_TEST) - testdram, + INITCALL(testdram); #endif /* CFG_SYS_DRAM_TEST */ - INIT_FUNC_WATCHDOG_RESET - - CONFIG_IS_ENABLED(POST, (init_post,)) - INIT_FUNC_WATCHDOG_RESET + WATCHDOG_RESET(); + CONFIG_IS_ENABLED(POST, (INITCALL(init_post);)) + WATCHDOG_RESET(); /* * Now that we have DRAM mapped and working, we can * relocate the code and continue running from DRAM. @@ -934,48 +937,48 @@ static const init_fnc_t init_sequence_f[] = { * - monitor code * - board info struct */ - setup_dest_addr, + INITCALL(setup_dest_addr); #if defined(CONFIG_OF_BOARD_FIXUP) && !defined(CONFIG_OF_INITIAL_DTB_READONLY) - fix_fdt, + INITCALL(fix_fdt); #endif #ifdef CFG_PRAM - reserve_pram, + INITCALL(reserve_pram); #endif - reserve_round_4k, - setup_relocaddr_from_bloblist, - arch_reserve_mmu, - reserve_video, - reserve_trace, - reserve_uboot, - reserve_malloc, - reserve_board, - reserve_global_data, - reserve_fdt, + INITCALL(reserve_round_4k); + INITCALL(setup_relocaddr_from_bloblist); + INITCALL(arch_reserve_mmu); + INITCALL(reserve_video); + INITCALL(reserve_trace); + INITCALL(reserve_uboot); + INITCALL(reserve_malloc); + INITCALL(reserve_board); + INITCALL(reserve_global_data); + INITCALL(reserve_fdt); #if defined(CONFIG_OF_BOARD_FIXUP) && defined(CONFIG_OF_INITIAL_DTB_READONLY) - reloc_fdt, - fix_fdt, + INITCALL(reloc_fdt); + INITCALL(fix_fdt); #endif - reserve_bootstage, - reserve_bloblist, - reserve_arch, - reserve_stacks, - dram_init_banksize, - show_dram_config, - INIT_FUNC_WATCHDOG_RESET - setup_bdinfo, - display_new_sp, - INIT_FUNC_WATCHDOG_RESET + INITCALL(reserve_bootstage); + INITCALL(reserve_bloblist); + INITCALL(reserve_arch); + INITCALL(reserve_stacks); + INITCALL(dram_init_banksize); + INITCALL(show_dram_config); + WATCHDOG_RESET(); + INITCALL(setup_bdinfo); + INITCALL(display_new_sp); + WATCHDOG_RESET(); #if !defined(CONFIG_OF_BOARD_FIXUP) || !defined(CONFIG_OF_INITIAL_DTB_READONLY) - reloc_fdt, + INITCALL(reloc_fdt); #endif - reloc_bootstage, - reloc_bloblist, - setup_reloc, + INITCALL(reloc_bootstage); + INITCALL(reloc_bloblist); + INITCALL(setup_reloc); #if defined(CONFIG_X86) || defined(CONFIG_ARC) - copy_uboot_to_ram, - do_elf_reloc_fixups, + INITCALL(copy_uboot_to_ram); + INITCALL(do_elf_reloc_fixups); #endif - clear_bss, + INITCALL(clear_bss); /* * Deregister all cyclic functions before relocation, so that * gd->cyclic_list does not contain any references to pre-relocation @@ -985,12 +988,11 @@ static const init_fnc_t init_sequence_f[] = { * This should happen as late as possible so that the window where a * watchdog device is not serviced is as small as possible. */ - cyclic_unregister_all, + INITCALL(cyclic_unregister_all); #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) - jump_to_copy, + INITCALL(jump_to_copy); #endif - NULL, -}; +} void board_init_f(ulong boot_flags) { @@ -1000,8 +1002,7 @@ void board_init_f(ulong boot_flags) gd->flags &= ~GD_FLG_HAVE_CONSOLE; gd->boardf = &boardf; - if (initcall_run_list(init_sequence_f)) - hang(); + initcall_run_f(); #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \ !defined(CONFIG_EFI_APP) && !CONFIG_IS_ENABLED(X86_64) && \ diff --git a/include/initcall.h b/include/initcall.h index 62d3bb67f08..9398a3ec7d5 100644 --- a/include/initcall.h +++ b/include/initcall.h @@ -8,6 +8,7 @@ #include <asm/types.h> #include <event.h> +#include <hang.h> _Static_assert(EVT_COUNT < 256, "Can only support 256 event types with 8 bits"); @@ -35,4 +36,30 @@ typedef int (*init_fnc_t)(void); */ int initcall_run_list(const init_fnc_t init_sequence[]); +#define INITCALL(_call) \ + do { \ + if (_call()) { \ + debug("%s(): calling %s() failed\n", __func__, \ + #_call); \ + hang(); \ + } \ + } while (0) + +#define INITCALL_EVT(_evt) \ + do { \ + if (event_notify_null(_evt)) { \ + debug("%s(): event %d/%s failed\n", __func__, _evt, \ + event_type_name(_evt)) ; \ + hang(); \ + } \ + } while (0) + +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) +#define WATCHDOG_INIT() INITCALL(init_func_watchdog_init) +#define WATCHDOG_RESET() INITCALL(init_func_watchdog_reset) +#else +#define WATCHDOG_INIT() +#define WATCHDOG_RESET() +#endif + #endif
Make static calls instead of iterating over the init_sequence_f arrays. Tested on a KV260 board (xilinx_zynqmp_kria_defconfig). - With LTO enabled, the code size reported by bloat-o-meter is 1200 bytes less (-0.11%) - With LTO disabled, the code is 594 bytes smaller (-0.05%) - Execution time does not change in a noticeable way Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> --- common/board_f.c | 165 +++++++++++++++++++++++---------------------- include/initcall.h | 27 ++++++++ 2 files changed, 110 insertions(+), 82 deletions(-)