Hi,
On 12/9/24 21:08, Tom Rini wrote:
On Mon, Dec 09, 2024 at 12:27:31PM -0700, Simon Glass wrote:
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)
We should probably just do a follow-up cleanup to, as you note
CONFIG_IS_ENABLED(FOO, (initr_foo,))
I don't think a more complex macro in there is worth while, but saving a
few bytes overall here for free would be good. We can figure out later
the cost/benefit on moving stuff here to events.
I have sent RFC with this here.
https://lore.kernel.org/r/9786c6124c959ca230dd2c23e8da794680f09867.1733837980.git.michal.si...@amd.com
There are likely some other steps should be happen on the top of this because
there are some entries which depends on more symbols. Also some macros are not
converted yet to CONFIG_ (CFG_) and there are functions which don't setup any
dependency but should be there.
But likely this should be done with steps.
Thanks,
Michal
