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

From: Stefan Roese <sr@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
Date: Tue, 30 Oct 2018 14:13:56 +0100	[thread overview]
Message-ID: <f85ce9ba-f6a1-7387-2093-e78b94870fbd@denx.de> (raw)
In-Reply-To: <CAAh8qswFzRnEtCWy=p0m-Xm1Em5DyzwZCyod365UK4LifexSmQ@mail.gmail.com>

On 30.10.18 14:02, Simon Goldschmidt wrote:
> 
> 
> Stefan Roese <sr at denx.de <mailto:sr@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@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).

Thanks,
Stefan