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

[Simon Goldschmidt]

Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> schrieb am Di., 30.
Okt. 2018, 11:34:

>
>
> 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.
>

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 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
>>
>
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.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
>>
>