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

[Marek Vasut]

On 10/30/2018 04:00 PM, Simon Goldschmidt wrote:
> 
> 
> Marek Vasut <marex at denx.de <mailto:marex at denx.de>> schrieb am Di., 30.
> Okt. 2018, 14:24:
> 
>     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> <mailto: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>
>     >> <mailto: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
> 
> 
> Right. I'll check how the SPL size check works with different offsets then.
> 
> So for this patch:
> Acked-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com
> <mailto:simon.k.r.goldschmidt at gmail.com>>
> 
> Since this is a bugfix, will it be merged for 2018.11?

I'll pick it if Stefan is fine with it (I guess he is) ?

-- 
Best regards,
Marek Vasut