Re: [PATCH] configs: at91: sama5d2_icp_mmc: Enable CONFIG_LTO

[<Nicolas.Ferre@microchip.com>]

On 03/05/2023 at 11:51, Eugen Hristev wrote:
> [You don't often get email from eugen.hristev@collabora.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

I'm monitoring that, with help from Eugen.

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

Well, give us some time, please!

> 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 Eugen for your support.

We didn't ask for this situation, so Pali, understand that we need to 
get organized before giving an answer / testing.

Best regards,
   Nicolas

>>>>>> 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@denx.de>
>>>>>>> Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>>>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>> Cc: Sergiu Moga <sergiu.moga@microchip.com>
>>>>>>> Cc: Pali Rohár <pali@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@denx.de
> 

-- 
Nicolas Ferre