* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
@ 2018-10-30 9:00 Stefan Roese
2018-10-30 9:07 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 9:00 UTC (permalink / raw)
To: u-boot
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
--
2.19.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 9:00 [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM Stefan Roese
@ 2018-10-30 9:07 ` Simon Goldschmidt
2018-10-30 9:36 ` Stefan Roese
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 9:07 UTC (permalink / raw)
To: u-boot
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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 9:07 ` Simon Goldschmidt
@ 2018-10-30 9:36 ` Stefan Roese
2018-10-30 10:24 ` Marek Vasut
2018-10-30 10:34 ` Simon Goldschmidt
0 siblings, 2 replies; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 9:36 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 30.10.18 10:07, Simon Goldschmidt wrote:
>
>
> Stefan Roese <sr at 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 at denx.de <mailto:sr@denx.de>>
> Cc: Marek Vasut <marex at denx.de <mailto:marex@denx.de>>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at 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
--
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 9:36 ` Stefan Roese
@ 2018-10-30 10:24 ` Marek Vasut
2018-10-30 10:34 ` Simon Goldschmidt
1 sibling, 0 replies; 16+ messages in thread
From: Marek Vasut @ 2018-10-30 10:24 UTC (permalink / raw)
To: u-boot
On 10/30/2018 10:36 AM, Stefan Roese wrote:
> Hi Simon,
>
> On 30.10.18 10:07, Simon Goldschmidt wrote:
>>
>>
>> Stefan Roese <sr at 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.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 9:36 ` Stefan Roese
2018-10-30 10:24 ` Marek Vasut
@ 2018-10-30 10:34 ` Simon Goldschmidt
2018-10-30 11:17 ` Simon Goldschmidt
1 sibling, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 10:34 UTC (permalink / raw)
To: u-boot
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 at 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.
>
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@denx.de>>
> > Cc: Marek Vasut <marex at denx.de <mailto:marex@denx.de>>
> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt@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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 10:34 ` Simon Goldschmidt
@ 2018-10-30 11:17 ` Simon Goldschmidt
2018-10-30 11:28 ` Stefan Roese
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 11:17 UTC (permalink / raw)
To: u-boot
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 at 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.
>>
>
> 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@denx.de>>
>> > Cc: Marek Vasut <marex at denx.de <mailto:marex@denx.de>>
>> > Cc: Simon Goldschmidt <simon.k.r.goldschmidt@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
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 11:17 ` Simon Goldschmidt
@ 2018-10-30 11:28 ` Stefan Roese
2018-10-30 12:37 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 11:28 UTC (permalink / raw)
To: u-boot
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?
Thanks,
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 11:28 ` Stefan Roese
@ 2018-10-30 12:37 ` Simon Goldschmidt
2018-10-30 12:50 ` Stefan Roese
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 12:37 UTC (permalink / raw)
To: u-boot
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)
> > > +#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?
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 12:37 ` Simon Goldschmidt
@ 2018-10-30 12:50 ` Stefan Roese
2018-10-30 13:02 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 12:50 UTC (permalink / raw)
To: u-boot
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)
>>> > +#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.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 12:50 ` Stefan Roese
@ 2018-10-30 13:02 ` Simon Goldschmidt
2018-10-30 13:13 ` Stefan Roese
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 13:02 UTC (permalink / raw)
To: u-boot
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.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...
Simon
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 13:02 ` Simon Goldschmidt
@ 2018-10-30 13:13 ` Stefan Roese
2018-10-30 13:24 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 13:13 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 13:13 ` Stefan Roese
@ 2018-10-30 13:24 ` Marek Vasut
2018-10-30 15:00 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-10-30 13:24 UTC (permalink / raw)
To: u-boot
On 10/30/2018 02:13 PM, Stefan Roese wrote:
> 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@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
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 13:24 ` Marek Vasut
@ 2018-10-30 15:00 ` Simon Goldschmidt
2018-10-30 15:18 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-10-30 15:00 UTC (permalink / raw)
To: u-boot
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 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@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
>
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 15:00 ` Simon Goldschmidt
@ 2018-10-30 15:18 ` Marek Vasut
2018-10-30 15:33 ` Stefan Roese
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-10-30 15:18 UTC (permalink / raw)
To: u-boot
On 10/30/2018 04:00 PM, Simon Goldschmidt wrote:
>
>
> Marek Vasut <marex at 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 at 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 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).
>
> 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) ?
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 15:18 ` Marek Vasut
@ 2018-10-30 15:33 ` Stefan Roese
2018-10-30 16:59 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Stefan Roese @ 2018-10-30 15:33 UTC (permalink / raw)
To: u-boot
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. ;)
Thanks,
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM
2018-10-30 15:33 ` Stefan Roese
@ 2018-10-30 16:59 ` Marek Vasut
0 siblings, 0 replies; 16+ messages in thread
From: Marek Vasut @ 2018-10-30 16:59 UTC (permalink / raw)
To: u-boot
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.
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-10-30 16:59 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 9:00 [U-Boot] [PATCH] arm: socfpga: Fix bootcounter located at the end of internal SRAM Stefan Roese
2018-10-30 9:07 ` Simon Goldschmidt
2018-10-30 9:36 ` Stefan Roese
2018-10-30 10:24 ` Marek Vasut
2018-10-30 10:34 ` Simon Goldschmidt
2018-10-30 11:17 ` Simon Goldschmidt
2018-10-30 11:28 ` Stefan Roese
2018-10-30 12:37 ` Simon Goldschmidt
2018-10-30 12:50 ` Stefan Roese
2018-10-30 13:02 ` Simon Goldschmidt
2018-10-30 13:13 ` Stefan Roese
2018-10-30 13:24 ` Marek Vasut
2018-10-30 15:00 ` Simon Goldschmidt
2018-10-30 15:18 ` Marek Vasut
2018-10-30 15:33 ` Stefan Roese
2018-10-30 16:59 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.