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

[Marek Vasut]

On 05/15/2018 09:26 AM, Simon Goldschmidt wrote:
> 
> 
> On 14.05.2018 22:43, Marek Vasut wrote:
>> On 05/14/2018 09:43 PM, 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)
>>>>>>>>
>>>>>>>> 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.
>>
>> This looks like a hack and a bogus one. Lying about RAM size is never a
>> good idea.
>>
>>> 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?
>>
>> What boot reason options do you get ?
> 
> The reset manager has a status register [1] that has various bits for
> different reasons of warm or cold reset. I'm just not sure if it's worth
> going through the hassle or if I'm alone with that problem of the
> bootcounter surviving a pull-and-replug.
> 
> [1] https://www.altera.com/hps/cyclone-v/topic/sfo1410067763568.html

Do you get warm or cold reset indication during plug-and-replug ?

-- 
Best regards,
Marek Vasut