[U-Boot] [PATCH] arm: socfpga: check total size of SPL

[Marek Vasut]

On 11/01/2018 09:31 PM, Simon Goldschmidt wrote:
> On Thu, Nov 1, 2018 at 8:26 PM Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
>>
>> On 31.10.2018 11:00, Marek Vasut wrote:
>>> On 10/31/2018 06:44 AM, Simon Goldschmidt wrote:
>>>> On Tue, Oct 30, 2018 at 11:02 PM Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>> On 10/30/2018 10:30 PM, Simon Goldschmidt wrote:
>>>>>> On 30.10.2018 22:26, Marek Vasut wrote:
>>>>>>> On 10/30/2018 10:23 PM, Simon Goldschmidt wrote:
>>>>>>>> Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm
>>>>>>>> linker script for SPL check the total SRAM size available for SPL
>>>>>>>> (code, data, bss, heap, global data).
>>>>>>>>
>>>>>>>> The previously existing define CONFIG_SPL_MAX_SIZE seems to only
>>>>>>>> check the binary size (which is without bss, heap and gd).
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>    include/configs/socfpga_common.h | 7 +++++++
>>>>>>>>    1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/configs/socfpga_common.h
>>>>>>>> b/include/configs/socfpga_common.h
>>>>>>>> index 2330143cf1..9103d0a966 100644
>>>>>>>> --- a/include/configs/socfpga_common.h
>>>>>>>> +++ b/include/configs/socfpga_common.h
>>>>>>>> @@ -242,6 +242,13 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>>>>>>    #define CONFIG_SPL_TEXT_BASE        CONFIG_SYS_INIT_RAM_ADDR
>>>>>>>>    #define CONFIG_SPL_MAX_SIZE        CONFIG_SYS_INIT_RAM_SIZE
>>>>>>>>    +/* Check total size of SPL including BSS, malloc area and gd */
>>>>>>>> +#include <generated/generic-asm-offsets.h>
>>>>>>>> +#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
>>>>>>>> +                     CONFIG_SYS_INIT_RAM_ADDR - \
>>>>>>>> +                     CONFIG_SYS_MALLOC_F_LEN - \
>>>>>>>> +                     GENERATED_GBL_DATA_SIZE)
>>>>>>> Are you sure this calculation is correct ? INIT_SP_ADDR is I think the
>>>>>>> SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or
>>>>>>> something ?
>>>>>> Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR +
>>>>>> INIT_RAM_SIZE.
>>>>>> So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
>>>>>> - MALLOC_F_LEN - GBL_DATA_SIZE".
>>>>>>
>>>>>> But I did it this way to keep it working after Stefan's fix for
>>>>>> reserving the boot counter location is applied.
>>>>> Now that's confusing :-) Add a comment explaining this into a V2 please,
>>>>> otherwise the confusion will continue ...
>>>> You're probably right. I'll send a V2 making this more clear.
>>> Thanks
>>
>> Re-checking this, I found the check is still not correct as it would
>> leave 4 bytes for the stack only. Is there an option that controls how
>> much stack should be preserved for SPL somewhere?
> 
> And to make matters worse, the devicetree is also not included in the
> size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select
> CONFIG_OF_EMBED.
> 
> While this would be OK for SPL, I think it wouldn't be for U-Boot. How
> do other platforms handle this size check?

Can git grep help here ?

-- 
Best regards,
Marek Vasut