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

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

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.

> 
> Michal, see also [1] in case you can work out why it 'stopped
> working'. I could have sworn inlining the function was a win when it
> was applied, but no amount of toolchain juggling could make it be a
> win when I came back to it later.

Are you saying that it worked in past?

Thanks,
Michal