[U-Boot] [PATCH] ARM: socfpga: Put stack at the end of SRAM

[Simon Goldschmidt]

On 14.05.2018 21:43, Simon Goldschmidt wrote:
> 
> 
> On 14.05.2018 17:51, Stefan Roese wrote:
>> On 14.05.2018 11:06, Marek Vasut wrote:
>>> On 05/14/2018 11:01 AM, Simon Goldschmidt wrote:
>>>>
>>>>
>>>> On 14.05.2018 10:17, Marek Vasut wrote:
>>>>> On 05/14/2018 10:03 AM, Simon Goldschmidt wrote:
>>>>>> On 12.05.2018 22:27, Marek Vasut wrote:
>>>>>>> The global data are in the .data section, so there's no point in
>>>>>>> reserving any space for it above stack. Put stack at the end of
>>>>>>> SRAM.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>> Cc: Chin Liang See <chin.liang.see at intel.com>
>>>>>>> Cc: Dinh Nguyen <dinguyen at kernel.org>
>>>>>>> ---
>>>>>>>     include/configs/socfpga_common.h | 4 +---
>>>>>>>     1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/configs/socfpga_common.h
>>>>>>> b/include/configs/socfpga_common.h
>>>>>>> index 4de2aa7929..cb67d539b1 100644
>>>>>>> --- a/include/configs/socfpga_common.h
>>>>>>> +++ b/include/configs/socfpga_common.h
>>>>>>> @@ -35,10 +35,8 @@
>>>>>>>     #define CONFIG_SYS_INIT_RAM_ADDR    0xFFE00000
>>>>>>>     #define CONFIG_SYS_INIT_RAM_SIZE    0x40000 /* 256KB */
>>>>>>>     #endif
>>>>>>> -#define CONFIG_SYS_INIT_SP_OFFSET        \
>>>>>>> -    (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE)
>>>>>>>     #define CONFIG_SYS_INIT_SP_ADDR            \
>>>>>>> -    (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET)
>>>>>>> +    (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)

Anyway, after reading up on this in the code and the resulting binaries, 
I do think this patch is correct, so:

Reviewed-by: Simon Goldschmidt <sgoldschmidt at de.pepperl-fuchs.com>

Sorry for the confusion, but this really helped to clarify my 
understanding of the initial memory allocation ;-)

Thanks,
Simon

>>>>>>
>>>>>> I think this interferes with the bootcount being stored at the end of
>>>>>> SRAM. See the defconfigs of 'is1' and 'sr1500' boards - or my local
>>>>>> defconfig here :-)
>>>>>>
>>>>>> I'd really like to keep some room here for bootcounter and some 
>>>>>> related
>>>>>> things I store here.
>>>>>
>>>>> So if I understand it correctly, before this patch, the boards 
>>>>> overwrote
>>>>> the global data ?
>>>>
>>>> Ehrm, reading up on the init code path in the S and C files, it seems
>>>> like that, yes. I just used the two boards as an example. Are they
>>>> actively maintained? get_maintainer.pl shows the boards are maintained
>>>> by denx, so maybe you could clarify this?
>>>
>>> +CC Stefan.
>>
>> Thanks. At the time I added the bootcounter support on this platform,
>> it used to work. I might have missed something though. Right now,
>> I don't have access to such a platform, so its hard for me to test
>> something. If you have some patches to fix any potential overlapping
>> memory usages, I'll gladly review them.
> 
> I just checked out ae996c80 and from what I can tell, the RAM layout was 
> the same at that time: include/generated/generated-asm-offsets.h contains:
> - #define GENERATED_GBL_DATA_SIZE 208
> - #define GD_SIZE 200
> 
> That leaves us with an 8 byte alignment gap at the end of the SRAM and 
> that's where the boot counter is located. So the global data is not 
> overwritten, but this is more or less by chance, any change to the 
> struct definition might change this.
> 
> Looking at just another example: the x600 board defines its SRAM_SIZE 8 
> bytes smaller than what it really is. That seems like a good idea for 
> socfpga, too. I'll see if I can provide a patch.
> 
> Slightly off-topic: I have a constellation where the boot-counter is not 
> reset although the power has been pulled. This device has big capacitors 
> and the SRAM content seems to survive although the processor is reset. 
> To me it seems as if looking at the boot reason might be safer than 
> checking for a magic word... Any ideas?
> 
> Thanks,
> Simon
> 
>>
>>>> Chances are high that these bytes just fit into the 16-byte alignment
>>>> allocated by GENERATED_GBL_DATA_SIZE (and on my board it *is* like 
>>>> that).
>>>>
>>>>>
>>>>> Anyway, if you need some extra space, look at
>>>>> board_init_f_alloc_reserve() .
>>>>>
>>>>
>>>> OK, but that function does not seem to be overridable? How can I 
>>>> reserve
>>>> my own memory here?
>>>
>>> Add a __weak there I guess ?
>>>
>>>> And reading this function, I guess your commit message is a bit
>>>> misleading. It seems that the purpose of CONFIG_SYS_INIT_SP_OFFSET like
>>>> it was was to reserve memory for the initial gd (before relocation),
>>>> which is now done in board_init_f_alloc_reserve().
>>>
>>> The code is confusing as hell, I give you that.
>>
>> Thanks,
>> Stefan
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot