Re: [PATCH v2 2/7] common: binman: Calling initr_binman() when BINMAN_FDT

[Michal Simek <michal.simek@amd.com>]

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 <trini@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@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@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@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@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.

I have sent RFC with this here.

https://lore.kernel.org/r/9786c6124c959ca230dd2c23e8da794680f09867.1733837980.git.michal.simek@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