[PATCH v2 2/7] common: binman: Calling initr_binman() when BINMAN_FDT
Tom Rini
trini at konsulko.com
Mon Dec 9 21:08:18 CET 2024
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 <trini at konsulko.com> 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 <trini at konsulko.com> 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 <michal.simek at amd.com> 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 <michal.simek at amd.com>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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 <sjg at chromium.org>
> > > > > > >
> > > > > > > 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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241209/d8490231/attachment.sig>
More information about the U-Boot
mailing list