[PATCH] configs: at91: sama5d2_icp_mmc: Enable CONFIG_LTO

[Eugen Hristev]

On 4/27/23 19:55, Pali Rohár wrote:
> On Thursday 27 April 2023 12:30:43 Stefan Roese wrote:
>> On 4/27/23 11:51, Eugen Hristev wrote:
>>> On 4/27/23 12:26, Stefan Roese wrote:
>>>> Hi Eugen,
>>>>
>>>> On 4/27/23 11:19, Eugen Hristev wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> This I guess is a workaround such that you can add a bit more of code.
>>>>> In the end, it's not scalable, and we have to find a better way,
>>>>> probably by removing some of the code to make the SPL smaller.
>>>>
>>>> U-Boot image size increase resulting in overflowing some limits is
>>>> a common problem, especially in SPL. Enabling LTO gives quite some good
>>>> improvements in image size decrease. So I don't think it's an
>>>> workaround.
>>>
>>> If this was not needed until today, and not adding any new
>>> functionality, I would call this a workaround to avoid shrinking the
>>> size to fit in the SRAM.
>>> When we are adding more and more, and eventually hit this problem again,
>>> LTO already enabled, what we will do ?
>>> That's why I call this a workaround because we are not solving the
>>> problem, just postponing so we can add more things today.
>>
>> This is what's happening since many years. But okay, let's call it a
>> workaround.
>>
>>>>
>>>>> How does this impact the size? How much we are gaining ?
>>>>
>>>> I did not measure this. I just checked that this target compiles clean
>>>> again with LTO enabled and the MMC related patches applied.
>>>>
>>>> Could you (or some college?) please investigate here, how the results
>>>> are in image size?
>>>
>>> No, you are submitting the patch, I assume you could give some numbers
>>> to support your patch.
>>
>> Sorry, my time is limited and frankly, I don't feel very much motivated
>> (any more) to do additional work here. Even if it's not that much of
>> effort.
>>
>>>>
>>>>> We can perhaps have a look to see which code is removed and
>>>>> guard it by #ifndef SPL_BUILD and that might lower the size. (if
>>>>> ofcourse, this code should really be removed)
>>>>
>>>> Sure, other improvements in image size decrease are of course always a
>>>> good idea.
>>>>
>>>>> Also, I don't have a board at hand to test this, so it has to be
>>>>> tested first to make sure the board doesn't break.
>>>>
>>>> Agreed. I assume/hope that one of your colleges will be able to test
>>>> this?
>>>
>>> Someone from Microchip can, or other people using the board from the
>>> community
>>>
>>> I no longer work for Microchip, but I am still maintaining the AT91
>>> custodian tree
>>
>> Okay. Let's see, where this goes.
> 
> Well, if nobody wants to care about this board, go ahead with this
> change and if it is not enough that drop support for this board.

Hi Pali,

I kind of dislike this attitude. If a patchset breaks a board, a 
patchset should be changed. Or rejected.
I don't agree with removing boards just because in a few days nobody 
tested one patch.
And applying untested patches is again something which I disagree upon.

> 
>> Thanks,
>> Stefan
>>
>>> Eugen
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>> Eugen
>>>>>
>>>>>
>>>>> On 4/27/23 11:59, Stefan Roese wrote:
>>>>>> Adding just a tiny bit more code for sama5d2_icp_mmc leads to a SRAM
>>>>>> image overflow. Fix this by enabling LTO for this board, so that such
>>>>>> changes still can be made to the common U-Boot code.
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>>> Cc: Tudor Ambarus <tudor.ambarus at microchip.com>
>>>>>> Cc: Eugen Hristev <eugen.hristev at microchip.com>
>>>>>> Cc: Sergiu Moga <sergiu.moga at microchip.com>
>>>>>> Cc: Pali Rohár <pali at kernel.org>
>>>>>> ---
>>>>>>    configs/sama5d2_icp_mmc_defconfig | 5 +++--
>>>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/configs/sama5d2_icp_mmc_defconfig
>>>>>> b/configs/sama5d2_icp_mmc_defconfig
>>>>>> index e1b602d8e5ec..a3c57a3f1250 100644
>>>>>> --- a/configs/sama5d2_icp_mmc_defconfig
>>>>>> +++ b/configs/sama5d2_icp_mmc_defconfig
>>>>>> @@ -9,9 +9,11 @@ CONFIG_SPL_LIBCOMMON_SUPPORT=y
>>>>>>    CONFIG_SPL_LIBGENERIC_SUPPORT=y
>>>>>>    CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>>    CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x20003ef0
>>>>>> +CONFIG_SF_DEFAULT_SPEED=66000000
>>>>>>    CONFIG_ENV_SIZE=0x4000
>>>>>>    CONFIG_DM_GPIO=y
>>>>>>    CONFIG_DEFAULT_DEVICE_TREE="at91-sama5d2_icp"
>>>>>> +CONFIG_OF_LIBFDT_OVERLAY=y
>>>>>>    CONFIG_SPL_MMC=y
>>>>>>    CONFIG_SPL_SERIAL=y
>>>>>>    CONFIG_SPL_DRIVERS_MISC=y
>>>>>> @@ -24,6 +26,7 @@ CONFIG_SPL_FS_FAT=y
>>>>>>    CONFIG_SPL_LIBDISK_SUPPORT=y
>>>>>>    CONFIG_SYS_LOAD_ADDR=0x22000000
>>>>>>    CONFIG_DEBUG_UART=y
>>>>>> +CONFIG_LTO=y
>>>>>>    CONFIG_ENV_VARS_UBOOT_CONFIG=y
>>>>>>    CONFIG_SYS_MONITOR_LEN=524288
>>>>>>    CONFIG_FIT=y
>>>>>> @@ -86,7 +89,6 @@ CONFIG_MMC_SDHCI=y
>>>>>>    CONFIG_MMC_SDHCI_ATMEL=y
>>>>>>    CONFIG_DM_SPI_FLASH=y
>>>>>>    CONFIG_SF_DEFAULT_BUS=2
>>>>>> -CONFIG_SF_DEFAULT_SPEED=66000000
>>>>>>    CONFIG_SPI_FLASH_SFDP_SUPPORT=y
>>>>>>    CONFIG_SPI_FLASH_ATMEL=y
>>>>>>    CONFIG_SPI_FLASH_MACRONIX=y
>>>>>> @@ -110,5 +112,4 @@ CONFIG_TIMER=y
>>>>>>    CONFIG_SPL_TIMER=y
>>>>>>    CONFIG_ATMEL_TCB_TIMER=y
>>>>>>    CONFIG_SPL_ATMEL_TCB_TIMER=y
>>>>>> -CONFIG_OF_LIBFDT_OVERLAY=y
>>>>>>    # CONFIG_EFI_LOADER_HII is not set
>>>>>
>>>>
>>>> Viele Grüße,
>>>> Stefan Roese
>>>>
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Erika Unter
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de