diff mbox series

[v2,2/5] board_init_f(): use static calls

Message ID 252364e53d56fab1c77bda74563b24ad8a17d635.1734537135.git.jerome.forissier@linaro.org
State New
Headers show
Series Static initcalls | expand

Commit Message

Jerome Forissier Dec. 18, 2024, 3:53 p.m. UTC
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(-)

Comments

Quentin Schulz Dec. 18, 2024, 5:27 p.m. UTC | #1
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
Jerome Forissier Dec. 18, 2024, 5:38 p.m. UTC | #2
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,
Quentin Schulz Dec. 18, 2024, 5:47 p.m. UTC | #3
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
Jerome Forissier Dec. 18, 2024, 6:21 p.m. UTC | #4
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,
Ilias Apalodimas Dec. 21, 2024, 8:10 a.m. UTC | #5
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
>
Jerome Forissier Dec. 21, 2024, 12:08 p.m. UTC | #6
Hi Ilias,

On 12/21/24 09:10, Ilias Apalodimas wrote:
> 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?

Good catch, it's a mistake. I'll fix in v3.

Thanks,
diff mbox series

Patch

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