Simon Goldschmidt simon.k.r.goldschmidt@gmail.com schrieb am Di., 30. Okt. 2018, 11:34:
Stefan Roese sr@denx.de schrieb am Di., 30. Okt. 2018, 10:36:
Hi Simon,
On 30.10.18 10:07, Simon Goldschmidt wrote:
Stefan Roese <sr@denx.de mailto:sr@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 thusthe
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 internalSRAM
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.
Drop the maybe. I'll test your patch and drop mine, but see the comment below.
Simon
Thanks, Stefan
Simon
Signed-off-by: Stefan Roese <sr@denx.de <mailto:sr@denx.de>> Cc: Marek Vasut <marex@denx.de <mailto:marex@denx.de>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com <mailto:simon.k.r.goldschmidt@gmail.com>>
--- include/configs/socfpga_common.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/configs/socfpga_common.hb/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 theinternal
+ * SRAM as bootcounter storage. Make sure to not put the stackdirectly
+ * 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_ADDRCONFIG_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.
Simon
#define CONFIG_SYS_SDRAM_BASE PHYS_SDRAM_1 -- 2.19.1Viele 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@denx.de