[U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
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.
Signed-off-by: Stefan Roese sr@denx.de Cc: Marek Vasut marex@denx.de Cc: Simon Goldschmidt simon.k.r.goldschmidt@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
Stefan Roese 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 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.
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...
Simon
Signed-off-by: Stefan Roese sr@denx.de Cc: Marek Vasut marex@denx.de Cc: Simon Goldschmidt simon.k.r.goldschmidt@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
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 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.
And you probably need to add new defines to Kconfig as well (e.g. SOCFPGA_INIT_RAM_END_RESERVE).
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.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
On 10/30/2018 10:36 AM, Stefan Roese wrote:
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 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.
And you probably need to add new defines to Kconfig as well (e.g. SOCFPGA_INIT_RAM_END_RESERVE).
Thanks for discussing this and finding a better solution. I'm looking forward to the new patch.
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 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@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, ifthe
+ * 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 #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
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
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) > +#endifCan 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?
Thanks, Stefan
On Tue, Oct 30, 2018 at 12:28 PM Stefan Roese 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) > +#endifCan 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?
Simon
On 30.10.18 13:37, Simon Goldschmidt wrote:
On Tue, Oct 30, 2018 at 12:28 PM Stefan Roese 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) > +#endifCan 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.
Thanks, Stefan
Stefan Roese 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@denx.de wrote:
On 30.10.18 12:17, Simon Goldschmidt wrote:
<snip>
> 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 atthe end of the internal
> + * SRAM as bootcounter storage. Make sure to not putthe stack directly
> + * at this address to not overwrite the bootcounterby checking, if the
> + * bootcounter address is located in the internalSRAM.
> + */ > +#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)
> +#endifCan 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...
Simon
On 30.10.18 14:02, Simon Goldschmidt wrote:
Stefan Roese <sr@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@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
On 10/30/2018 02:13 PM, Stefan Roese wrote:
On 30.10.18 14:02, Simon Goldschmidt wrote:
Stefan Roese <sr@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@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).
INIT_RAM_SIZE should reflect reality IMO
Marek Vasut marex@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@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@denx.demailto: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).
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@gmail.com
Since this is a bugfix, will it be merged for 2018.11?
Simon
On 10/30/2018 04:00 PM, Simon Goldschmidt wrote:
Marek Vasut <marex@denx.de mailto:marex@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@denx.de <mailto:sr@denx.de> <mailto:sr@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@denx.de <mailto:sr@denx.de> >> <mailto:sr@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). INIT_RAM_SIZE should reflect reality IMORight. 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@gmail.com mailto:simon.k.r.goldschmidt@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) ?
On 30.10.18 16:18, Marek Vasut wrote:
> 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 IMORight. 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@gmail.com mailto:simon.k.r.goldschmidt@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) ?
Yes, I am. ;)
Thanks, Stefan
On 10/30/2018 04:33 PM, Stefan Roese wrote:
On 30.10.18 16:18, Marek Vasut wrote:
> 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@gmail.com mailto:simon.k.r.goldschmidt@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) ?
Yes, I am. ;)
Applied.
participants (3)
-
Marek Vasut -
Simon Goldschmidt -
Stefan Roese