Hi Tom, On Mon, 9 Dec 2024 at 12:23, Tom Rini <[email protected]> wrote: > > On Mon, Dec 09, 2024 at 07:34:34PM +0100, Michal Simek wrote: > > > > > > On 12/9/24 16:47, Simon Glass wrote: > > > Hi, > > > > > > On Mon, 9 Dec 2024 at 08:32, Tom Rini <[email protected]> wrote: > > > > > > > > On Mon, Dec 09, 2024 at 04:26:15PM +0100, Michal Simek wrote: > > > > > > > > > > > > > > > On 12/6/24 20:20, Simon Glass wrote: > > > > > > On Fri, 1 Nov 2024 at 03:18, Michal Simek <[email protected]> > > > > > > wrote: > > > > > > > > > > > > > > Calling empty function when BINMAN_FDT is adding +64B for nothing > > > > > > > which is > > > > > > > not helping on size sensitive configurations as Xilinx mini > > > > > > > configurations. > > > > > > > > > > > > > > Signed-off-by: Michal Simek <[email protected]> > > > > > > > --- > > > > > > > > > > > > > > Changes in v2: > > > > > > > - new patch > > > > > > > > > > > > > > From my perspective there is no reason to call empty function. > > > > > > > It is just > > > > > > > increase footprint for nothing and we are not far from that limit > > > > > > > now. > > > > > > > > > > > > > > --- > > > > > > > common/board_r.c | 7 +++---- > > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > > > > > Reviewed-by: Simon Glass <[email protected]> > > > > > > > > > > > > This is a bit odd, though. Do you have LTO enabled? > > > > > > > > > > > > > > > > yes LTO is enabled. And there are other candidates like this. > > > > > Is LTO able to fix function arrays which is calling empty function? > > > > > > > > > > (without this patch) > > > > > > > > > > 00000000fffc0eb4 <initr_of_live>: > > > > > fffc0eb4: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0eb8: d65f03c0 ret > > > > > > > > > > 00000000fffc0ebc <initr_dm_devices>: > > > > > fffc0ebc: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ec0: d65f03c0 ret > > > > > > > > > > 00000000fffc0ec4 <initr_bootstage>: > > > > > fffc0ec4: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ec8: d65f03c0 ret > > > > > > > > > > 00000000fffc0ecc <power_init_board>: > > > > > fffc0ecc: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ed0: d65f03c0 ret > > > > > > > > > > 00000000fffc0ed4 <initr_announce>: > > > > > fffc0ed4: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ed8: d65f03c0 ret > > > > > > > > > > 00000000fffc0edc <initr_binman>: > > > > > fffc0edc: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ee0: d65f03c0 ret > > > > > > > > > > 00000000fffc0ee4 <initr_status_led>: > > > > > fffc0ee4: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ee8: d65f03c0 ret > > > > > > > > > > 00000000fffc0eec <initr_boot_led_blink>: > > > > > fffc0eec: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ef0: d65f03c0 ret > > > > > > > > > > 00000000fffc0ef4 <initr_boot_led_on>: > > > > > fffc0ef4: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0ef8: d65f03c0 ret > > > > > > > > > > 00000000fffc0efc <initr_lmb>: > > > > > fffc0efc: 52800000 mov w0, #0x0 > > > > > // #0 > > > > > fffc0f00: d65f03c0 ret > > > > > > > > No, but maybe Simon would prefer if we marked all of the could-be-empty > > > > functions as __maybe_unused and did: > > > > CONFIG_IS_ENABLED(BINMAN_FDT, initr_binman), > > > > etc in the list instead? > > > > > > Yes that looks better. > > > > But we are talking about using macro inside array at best with using > > #ifdefs. > > Or maybe I am not seeing what you are saying. > > No, nevermind, sorry. I was hoping that: > diff --git a/common/board_r.c b/common/board_r.c > index ff9bce88dc93..c18997446dfa 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -610,9 +610,7 @@ static init_fnc_t init_sequence_r[] = { > noncached_init, > #endif > initr_of_live, > -#ifdef CONFIG_DM > - initr_dm, > -#endif > + CONFIG_IS_ENABLED(CONFIG_DM, initr_dm), > #ifdef CONFIG_ADDR_MAP > init_addr_map, > #endif > @@ -632,9 +630,7 @@ static init_fnc_t init_sequence_r[] = { > #ifdef CONFIG_EFI_LOADER > efi_memory_init, > #endif > -#ifdef CONFIG_BINMAN_FDT > - initr_binman, > -#endif > + CONFIG_IS_ENABLED(CONFIG_BINMAN_FDT, initr_binman), > #ifdef CONFIG_FSP_VERSION2 > arch_fsp_init_r, > #endif > > would work, but the macro doesn't evaluate how I'd hope it did and that > just blows up.
You should drop the CONFIG_ inside the brackets. Also, you need something like this, since the comma must not be present unless the option is enabled: CONFIG_IS_ENABLED(BINMAN_FDT, (initr_binman,)) If we want to do this more generally, we could: - convert more things to use an event (then the cost is just 4-8 bytes per item) - add a helper to initcall.h to make it easier, e.g. INITCALL(BINMAN_FDT, initr_binman) Regards, SImon
