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

[Simon Goldschmidt]

Stefan Roese <sr at denx.de> schrieb am Di., 30. Okt. 2018, 10:36:

> Hi Simon,
>
> On 30.10.18 10:07, Simon Goldschmidt wrote:
> >
> >
> > Stefan Roese <sr at denx.de <mailto:sr at denx.de>> schrieb am Di., 30. Okt.
> 2018, 10:00:
> >
> >     Commit 768f23dc8ae3 ("ARM: socfpga: Put stack at the end of SRAM")
> broke
> >     those socfpga boards that keep the bootcounter at the end of the
> >     internal SRAM as the bootcounter needs 8 bytes by default and thus
> the
> >     very first SPL call to board_init_f_alloc_reserve overwrites the
> >     bootcounter.
> >
> >     This patch allows to move the initial stack pointer down a bit by
> >     checking if CONFIG_SYS_BOOTCOUNT_ADDR is located in the internal SRAM
> >     area and then using this address as location for the start of the
> >     stack pointer.
> >
> >     No new macros / defines are added by this approach.
> >
> >
> > Ok, so no new macros are defined, but this is limited to the boot
> > counter. However, I need to store additional data that survives a
> > reboot somewhere, so I think an explicit range reservation would
> > be nicer.
>
> Ah, I was not aware that you have a different reasoning to allocate
> some memory in the internal SRAM.
>
> > I could still do what I want with your patch by allocating my data
> > above the bootcounter, but this works in a more or less implicit/
> > hidden way, not explicitly configured...
>
> Some implicit (hidden) means is definitely not good. You can go ahead
> with your approach. But please don't introduce new defines for things
> that are already configured - meaning 0x8 bytes reserved and 0xfffffff8
> as bootcounter location are redundant. This is a bit messy and might
> get out of hand, once those defines are not in sync any more.
>

Well, you configure the bootcounter location in defconfig as an address, so
you cannot really use this for a calculation other than your approach.


> And you probably need to add new defines to Kconfig as well
> (e.g. SOCFPGA_INIT_RAM_END_RESERVE).
>

An even cleaner solution would be to be able to override
CONFIG_SYS_INIT_RAM_SIZE, but since this is a globally used define, I don't
think I can provide a patch for this soon.

Maybe we'll start with your patch to fix the bootcounter in the mainline
boards and I'll provide a patch for my additional requirements once I find
the time.

Simon


> Thanks,
> Stefan
>
> > Simon
> >
> >
> >     Signed-off-by: Stefan Roese <sr at denx.de <mailto:sr at denx.de>>
> >     Cc: Marek Vasut <marex at denx.de <mailto:marex at denx.de>>
> >     Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com <mailto:
> simon.k.r.goldschmidt at gmail.com>>
> >     ---
> >       include/configs/socfpga_common.h | 13 +++++++++++++
> >       1 file changed, 13 insertions(+)
> >
> >     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
> >
> >       #define CONFIG_SYS_SDRAM_BASE          PHYS_SDRAM_1
> >
> >     --
> >     2.19.1
> >
>
> Viele Grüße,
> Stefan
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
>