[U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM

[Marek Vasut]

On 10/30/2018 02:13 PM, Stefan Roese wrote:
> On 30.10.18 14:02, Simon Goldschmidt wrote:
>>
>>
>> Stefan Roese <sr at denx.de <mailto:sr at denx.de>> schrieb am Di., 30. Okt.
>> 2018, 13:50:
>>
>>     On 30.10.18 13:37, Simon Goldschmidt wrote:
>>      > On Tue, Oct 30, 2018 at 12:28 PM Stefan Roese <sr at denx.de
>> <mailto:sr at denx.de>> wrote:
>>      >>
>>      >> On 30.10.18 12:17, Simon Goldschmidt wrote:
>>      >>
>>      >> <snip>
>>      >>
>>      >>>           >     diff --git a/include/configs/socfpga_common.h
>> b/include/configs/socfpga_common.h
>>      >>>           >     index 2330143cf1..bd8f5c8c41 100644
>>      >>>           >     --- a/include/configs/socfpga_common.h
>>      >>>           >     +++ b/include/configs/socfpga_common.h
>>      >>>           >     @@ -31,8 +31,21 @@
>>      >>>           >       #define CONFIG_SYS_INIT_RAM_ADDR     
>>  0xFFE00000
>>      >>>           >       #define CONFIG_SYS_INIT_RAM_SIZE     
>>  0x40000 /* 256KB */
>>      >>>           >       #endif
>>      >>>           >     +
>>      >>>           >     +/*
>>      >>>           >     + * Some boards (e.g. socfpga_sr1500) use 8
>> bytes at the end of the internal
>>      >>>           >     + * SRAM as bootcounter storage. Make sure to
>> not put the stack directly
>>      >>>           >     + * at this address to not overwrite the
>> bootcounter by checking, if the
>>      >>>           >     + * bootcounter address is located in the
>> internal SRAM.
>>      >>>           >     + */
>>      >>>           >     +#if ((CONFIG_SYS_BOOTCOUNT_ADDR >
>> CONFIG_SYS_INIT_RAM_ADDR) && \
>>      >>>           >     +     (CONFIG_SYS_BOOTCOUNT_ADDR <
>> (CONFIG_SYS_INIT_RAM_ADDR +  \
>>      >>>           >     +                                 
>> CONFIG_SYS_INIT_RAM_SIZE)))
>>      >>>           >     +#define CONFIG_SYS_INIT_SP_ADDR             
>>   CONFIG_SYS_BOOTCOUNT_ADDR
>>      >>>           >     +#else
>>      >>>           >       #define CONFIG_SYS_INIT_SP_ADDR           
>>             \
>>      >>>           >              (CONFIG_SYS_INIT_RAM_ADDR +
>> CONFIG_SYS_INIT_RAM_SIZE)
>>      >>>           >     +#endif
>>      >>>
>>      >>>
>>      >>> Can we have this check on CONFIG_INIT_RAM_SIZE instead of the
>>      >>> initial stack pointer?
>>      >>>
>>      >>> That would ensure the SPL size checks stay intact.
>>      >>
>>      >> I'm not really sure what you mean with this. Could you please
>>      >> explain in more detail?
>>      >
>>      > Sorry for being unclear. What I meant was: currently
>>      > CONFIG_SYS_INIT_RAM_SIZEis 0x10000 (the full 64 kByte).
>>      > So if CONFIG_SYS_BOOTCOUNT_ADDR is 0xfffffff8, I think we should
>>      > define CONFIG_SYS_INIT_RAM_SIZE to 0xfff8. That way, not only the
>>      > CONFIG_SYS_INIT_SP_ADDR define is correct but
>> CONFIG_SPL_MAX_SIZE is
>>      > checked to not overlap this address, too.
>>      >
>>      > Would that make sense to you?
>>
>>     Yes, I thought that you meant it this way. I'm not sure if we
>>     should go this way. As we would change CONFIG_SYS_INIT_RAM_SIZE
>>     to something that does not represent the physical size of the
>>     on-chip SRAM. This could be very confusing and misleading, if
>>     this define is used elsewhere.
>>
>>
>> Hmm, okay. I dont want to push you there. I just thought it would
>> be good to have the SPL binary size check correct...
> 
> Of course would this be good. But perhaps we can make this SPL
> binary size check "better" by not changing the INIT_RAM_SIZE
> define. Its definitely possible - I just don't know how hard
> (I didn't look into this).

INIT_RAM_SIZE should reflect reality IMO

-- 
Best regards,
Marek Vasut