[U-Boot] [PATCH 1/3] arm: socfpga: fix comment about SPL memory layout

[U-Boot] [PATCH 1/3] arm: socfpga: fix comment about SPL memory layout

[Simon Goldschmidt]

The comment about SPL memory layout for socfpga gen5 is outdated: the
initial malloc memory is now at the end of the SRAM, gd is below it
(see board_init_f_alloc_reserve).

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 include/configs/socfpga_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index c9cbf8f5e3..a3fbca0a5d 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -237,9 +237,9 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
  *
  * 0xFFFF_0000 ...... Start of SRAM
  * 0xFFFF_xxxx ...... Top of stack (grows down)
- * 0xFFFF_yyyy ...... Malloc area
- * 0xFFFF_zzzz ...... Global Data
- * 0xFFFF_FF00 ...... End of SRAM
+ * 0xFFFF_yyyy ...... Global Data
+ * 0xFFFF_zzzz ...... Malloc area
+ * 0xFFFF_FFFF ...... End of SRAM
  *
  * SRAM Memory layout for Arria 10:
  * 0xFFE0_0000 ...... Start of SRAM (bottom)
-- 
2.17.1

[U-Boot] [PATCH 2/3] arm: socfpga: a10: move SPL stack size to Kconfig

[Simon Goldschmidt]

Instead of fixing the SPL stack to 64 KiB in the board config header via
CONFIG_SYS_SPL_MALLOC_SIZE, let's just use CONFIG_SPL_SYS_MALLOC_F_LEN
in the defconfig.

This also has the advandage that it removes sub-mach specific ifdefs in
socfpga_common.h.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 configs/socfpga_arria10_defconfig |  1 +
 include/configs/socfpga_common.h  | 14 --------------
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
index f321a0ac3b..8d0479cc05 100644
--- a/configs/socfpga_arria10_defconfig
+++ b/configs/socfpga_arria10_defconfig
@@ -2,6 +2,7 @@ CONFIG_ARM=y
 CONFIG_ARCH_SOCFPGA=y
 CONFIG_SYS_TEXT_BASE=0x01000040
 CONFIG_SYS_MALLOC_F_LEN=0x2000
+CONFIG_SPL_SYS_MALLOC_F_LEN=0x10000
 CONFIG_TARGET_SOCFPGA_ARRIA10_SOCDK=y
 CONFIG_SPL=y
 CONFIG_IDENT_STRING="socfpga_arria10"
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index a3fbca0a5d..c23b34186a 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -251,16 +251,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 #define CONFIG_SPL_TEXT_BASE		CONFIG_SYS_INIT_RAM_ADDR
 #define CONFIG_SPL_MAX_SIZE		CONFIG_SYS_INIT_RAM_SIZE
 
-#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
-/* SPL memory allocation configuration, this is for FAT implementation */
-#ifndef CONFIG_SYS_SPL_MALLOC_START
-#define CONFIG_SYS_SPL_MALLOC_SIZE	0x00010000
-#define CONFIG_SYS_SPL_MALLOC_START	(CONFIG_SYS_INIT_RAM_SIZE - \
-					 CONFIG_SYS_SPL_MALLOC_SIZE + \
-					 CONFIG_SYS_INIT_RAM_ADDR)
-#endif
-#endif
-
 /* SPL SDMMC boot support */
 #ifdef CONFIG_SPL_MMC_SUPPORT
 #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4)
@@ -287,11 +277,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 /*
  * Stack setup
  */
-#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
 #define CONFIG_SPL_STACK		CONFIG_SYS_INIT_SP_ADDR
-#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
-#define CONFIG_SPL_STACK		CONFIG_SYS_SPL_MALLOC_START
-#endif
 
 /* Extra Environment */
 #ifndef CONFIG_SPL_BUILD
-- 
2.17.1

[U-Boot] [PATCH 3/3] arm: socfpga: put initial U-Boot stack into DDR

[Simon Goldschmidt]

If SPL pre-reloc stage puts the stack into DDR, U-Boot should be able to
do that, too.

The reason to do so is that this way, U-Boot initial stack can be larger
than SPL initial stack. In situations where we want to save the SPL
in SRAM for next boot without reloading, this prevents overwriting the
SPL DTB in SRAM if U-Boot stack usage gets too high.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 include/configs/socfpga_common.h | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index c23b34186a..7ae3db233f 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -38,12 +38,23 @@
 #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
+#define CONFIG_SPL_STACK		CONFIG_SYS_BOOTCOUNT_ADDR
 #else
-#define CONFIG_SYS_INIT_SP_ADDR			\
+#define CONFIG_SPL_STACK			\
 	(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
 #endif
 
+/*
+ * U-Boot stack setup: if SPL post-reloc uses DDR stack, use it in pre-reloc
+ * phase of U-Boot, too. This prevents overwriting SPL data if stack/heap usage
+ * in U-Boot pre-reloc is higher than in SPL.
+ */
+#if defined(CONFIG_SPL_STACK_R_ADDR) && CONFIG_SPL_STACK_R_ADDR
+#define CONFIG_SYS_INIT_SP_ADDR		CONFIG_SPL_STACK_R_ADDR
+#else
+#define CONFIG_SYS_INIT_SP_ADDR		CONFIG_SPL_STACK
+#endif
+
 #define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM_1
 
 /*
@@ -274,11 +285,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 #define CONFIG_SYS_NAND_U_BOOT_OFFS	0x40000
 #endif
 
-/*
- * Stack setup
- */
-#define CONFIG_SPL_STACK		CONFIG_SYS_INIT_SP_ADDR
-
 /* Extra Environment */
 #ifndef CONFIG_SPL_BUILD
 
-- 
2.17.1

[U-Boot] [PATCH 2/3] arm: socfpga: a10: move SPL stack size to Kconfig

[Marek Vasut]

On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
> Instead of fixing the SPL stack to 64 KiB in the board config header via
> CONFIG_SYS_SPL_MALLOC_SIZE, let's just use CONFIG_SPL_SYS_MALLOC_F_LEN
> in the defconfig.
> 
> This also has the advandage that it removes sub-mach specific ifdefs in
> socfpga_common.h.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Can't we default these stack sizes with imply in Kconfig files instead ?
Then they won't have to be in the defconfigs either.

> ---
> 
>  configs/socfpga_arria10_defconfig |  1 +
>  include/configs/socfpga_common.h  | 14 --------------
>  2 files changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
> index f321a0ac3b..8d0479cc05 100644
> --- a/configs/socfpga_arria10_defconfig
> +++ b/configs/socfpga_arria10_defconfig
> @@ -2,6 +2,7 @@ CONFIG_ARM=y
>  CONFIG_ARCH_SOCFPGA=y
>  CONFIG_SYS_TEXT_BASE=0x01000040
>  CONFIG_SYS_MALLOC_F_LEN=0x2000
> +CONFIG_SPL_SYS_MALLOC_F_LEN=0x10000
>  CONFIG_TARGET_SOCFPGA_ARRIA10_SOCDK=y
>  CONFIG_SPL=y
>  CONFIG_IDENT_STRING="socfpga_arria10"
> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> index a3fbca0a5d..c23b34186a 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -251,16 +251,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>  #define CONFIG_SPL_TEXT_BASE		CONFIG_SYS_INIT_RAM_ADDR
>  #define CONFIG_SPL_MAX_SIZE		CONFIG_SYS_INIT_RAM_SIZE
>  
> -#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> -/* SPL memory allocation configuration, this is for FAT implementation */
> -#ifndef CONFIG_SYS_SPL_MALLOC_START
> -#define CONFIG_SYS_SPL_MALLOC_SIZE	0x00010000
> -#define CONFIG_SYS_SPL_MALLOC_START	(CONFIG_SYS_INIT_RAM_SIZE - \
> -					 CONFIG_SYS_SPL_MALLOC_SIZE + \
> -					 CONFIG_SYS_INIT_RAM_ADDR)
> -#endif
> -#endif
> -
>  /* SPL SDMMC boot support */
>  #ifdef CONFIG_SPL_MMC_SUPPORT
>  #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4)
> @@ -287,11 +277,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>  /*
>   * Stack setup
>   */
> -#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>  #define CONFIG_SPL_STACK		CONFIG_SYS_INIT_SP_ADDR
> -#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> -#define CONFIG_SPL_STACK		CONFIG_SYS_SPL_MALLOC_START
> -#endif
>  
>  /* Extra Environment */
>  #ifndef CONFIG_SPL_BUILD
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] arm: socfpga: put initial U-Boot stack into DDR

[Marek Vasut]

On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
> If SPL pre-reloc stage puts the stack into DDR, U-Boot should be able to
> do that, too.

Here you mention SPL pre-reloc stage, while about mid-way through the
patch you have a comment talking about SPL post-reloc stage, which one
is it ? I think the commit message needs rewording, it's not clear what
the patch does based on it.

On a separate note, this is likely a fix for current release, right ?

Finally , keep in mind the A10 needs to load the FPGA before DRAM
becomes available, so the stack shenanigans there become even more nasty.

> The reason to do so is that this way, U-Boot initial stack can be larger
> than SPL initial stack. In situations where we want to save the SPL
> in SRAM for next boot without reloading, this prevents overwriting the
> SPL DTB in SRAM if U-Boot stack usage gets too high.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>  include/configs/socfpga_common.h | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> index c23b34186a..7ae3db233f 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -38,12 +38,23 @@
>  #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
> +#define CONFIG_SPL_STACK		CONFIG_SYS_BOOTCOUNT_ADDR
>  #else
> -#define CONFIG_SYS_INIT_SP_ADDR			\
> +#define CONFIG_SPL_STACK			\
>  	(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
>  #endif
>  
> +/*
> + * U-Boot stack setup: if SPL post-reloc uses DDR stack, use it in pre-reloc
> + * phase of U-Boot, too. This prevents overwriting SPL data if stack/heap usage
> + * in U-Boot pre-reloc is higher than in SPL.
> + */
> +#if defined(CONFIG_SPL_STACK_R_ADDR) && CONFIG_SPL_STACK_R_ADDR
> +#define CONFIG_SYS_INIT_SP_ADDR		CONFIG_SPL_STACK_R_ADDR
> +#else
> +#define CONFIG_SYS_INIT_SP_ADDR		CONFIG_SPL_STACK
> +#endif
> +
>  #define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM_1
>  
>  /*
> @@ -274,11 +285,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>  #define CONFIG_SYS_NAND_U_BOOT_OFFS	0x40000
>  #endif
>  
> -/*
> - * Stack setup
> - */
> -#define CONFIG_SPL_STACK		CONFIG_SYS_INIT_SP_ADDR
> -
>  /* Extra Environment */
>  #ifndef CONFIG_SPL_BUILD
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 2/3] arm: socfpga: a10: move SPL stack size to Kconfig

[Simon Goldschmidt]

On Thu, Feb 28, 2019 at 10:44 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
> > Instead of fixing the SPL stack to 64 KiB in the board config header via
> > CONFIG_SYS_SPL_MALLOC_SIZE, let's just use CONFIG_SPL_SYS_MALLOC_F_LEN
> > in the defconfig.
> >
> > This also has the advandage that it removes sub-mach specific ifdefs in
> > socfpga_common.h.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>
> Can't we default these stack sizes with imply in Kconfig files instead ?
> Then they won't have to be in the defconfigs either.

I guess you mean default the *malloc* sizes, not *stack* sizes? Yes, that
should work.

I started this whole series because all gen5 defconfigs set the initial malloc
size to 8 KiB, while I monitored socrates to only need a bit more than 1 KiB.
With my reset patches, something around 1.5 KiB is still enough. So I'd
move that to imply 2 KiB for gen5 and 64 KiB for a10 in a patch 4/4 for v2.

Regards,
Simon

>
> > ---
> >
> >  configs/socfpga_arria10_defconfig |  1 +
> >  include/configs/socfpga_common.h  | 14 --------------
> >  2 files changed, 1 insertion(+), 14 deletions(-)
> >
> > diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
> > index f321a0ac3b..8d0479cc05 100644
> > --- a/configs/socfpga_arria10_defconfig
> > +++ b/configs/socfpga_arria10_defconfig
> > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >  CONFIG_ARCH_SOCFPGA=y
> >  CONFIG_SYS_TEXT_BASE=0x01000040
> >  CONFIG_SYS_MALLOC_F_LEN=0x2000
> > +CONFIG_SPL_SYS_MALLOC_F_LEN=0x10000
> >  CONFIG_TARGET_SOCFPGA_ARRIA10_SOCDK=y
> >  CONFIG_SPL=y
> >  CONFIG_IDENT_STRING="socfpga_arria10"
> > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> > index a3fbca0a5d..c23b34186a 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -251,16 +251,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
> >  #define CONFIG_SPL_TEXT_BASE         CONFIG_SYS_INIT_RAM_ADDR
> >  #define CONFIG_SPL_MAX_SIZE          CONFIG_SYS_INIT_RAM_SIZE
> >
> > -#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > -/* SPL memory allocation configuration, this is for FAT implementation */
> > -#ifndef CONFIG_SYS_SPL_MALLOC_START
> > -#define CONFIG_SYS_SPL_MALLOC_SIZE   0x00010000
> > -#define CONFIG_SYS_SPL_MALLOC_START  (CONFIG_SYS_INIT_RAM_SIZE - \
> > -                                      CONFIG_SYS_SPL_MALLOC_SIZE + \
> > -                                      CONFIG_SYS_INIT_RAM_ADDR)
> > -#endif
> > -#endif
> > -
> >  /* SPL SDMMC boot support */
> >  #ifdef CONFIG_SPL_MMC_SUPPORT
> >  #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4)
> > @@ -287,11 +277,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
> >  /*
> >   * Stack setup
> >   */
> > -#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> >  #define CONFIG_SPL_STACK             CONFIG_SYS_INIT_SP_ADDR
> > -#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > -#define CONFIG_SPL_STACK             CONFIG_SYS_SPL_MALLOC_START
> > -#endif
> >
> >  /* Extra Environment */
> >  #ifndef CONFIG_SPL_BUILD
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 3/3] arm: socfpga: put initial U-Boot stack into DDR

[Simon Goldschmidt]

On Thu, Feb 28, 2019 at 10:44 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
> > If SPL pre-reloc stage puts the stack into DDR, U-Boot should be able to
> > do that, too.
>
> Here you mention SPL pre-reloc stage, while about mid-way through the
> patch you have a comment talking about SPL post-reloc stage, which one
> is it ? I think the commit message needs rewording, it's not clear what
> the patch does based on it.

Ehrm right. The commit message is wrong. Post-reloc is correct.

> On a separate note, this is likely a fix for current release, right ?

Not really. For A10, this should not change anything, just ensure the common
header file does not use 2 different ways to achieve the same thing.
Note that A10 does *not* enable a different stack/malloc area for SPL
post-reloc!

For gen5, yes there is a change, but only in U-Boot, not in SPL. But this is
not a fix, it's an improvement to be able to make the SPL malloc pool smaller
without U-Boot overwriting it. It's currently not overwritten as both
malloc sizes
are the same. I'd change that in a follow-up patch which reduces SPL malloc
size for gen5.

> Finally , keep in mind the A10 needs to load the FPGA before DRAM
> becomes available, so the stack shenanigans there become even more nasty.

Right, that's why even the initial malloc size for SPL is 64 KiB,
right? As written
above, this patch should not change anything for A10, so this should still work.

From reading the A10 SPL code, I didn't quite get how this works though, as
it seems the simple-malloc members in gd are only initialized after
spl_early_init(), so after initializing DT/DM? Or is A10 not yet
converted to DT?

Regards,
Simon

> > The reason to do so is that this way, U-Boot initial stack can be larger
> > than SPL initial stack. In situations where we want to save the SPL
> > in SRAM for next boot without reloading, this prevents overwriting the
> > SPL DTB in SRAM if U-Boot stack usage gets too high.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > ---
> >
> >  include/configs/socfpga_common.h | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> > index c23b34186a..7ae3db233f 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -38,12 +38,23 @@
> >  #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
> > +#define CONFIG_SPL_STACK             CONFIG_SYS_BOOTCOUNT_ADDR
> >  #else
> > -#define CONFIG_SYS_INIT_SP_ADDR                      \
> > +#define CONFIG_SPL_STACK                     \
> >       (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
> >  #endif
> >
> > +/*
> > + * U-Boot stack setup: if SPL post-reloc uses DDR stack, use it in pre-reloc
> > + * phase of U-Boot, too. This prevents overwriting SPL data if stack/heap usage
> > + * in U-Boot pre-reloc is higher than in SPL.
> > + */
> > +#if defined(CONFIG_SPL_STACK_R_ADDR) && CONFIG_SPL_STACK_R_ADDR
> > +#define CONFIG_SYS_INIT_SP_ADDR              CONFIG_SPL_STACK_R_ADDR
> > +#else
> > +#define CONFIG_SYS_INIT_SP_ADDR              CONFIG_SPL_STACK
> > +#endif
> > +
> >  #define CONFIG_SYS_SDRAM_BASE                PHYS_SDRAM_1
> >
> >  /*
> > @@ -274,11 +285,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
> >  #define CONFIG_SYS_NAND_U_BOOT_OFFS  0x40000
> >  #endif
> >
> > -/*
> > - * Stack setup
> > - */
> > -#define CONFIG_SPL_STACK             CONFIG_SYS_INIT_SP_ADDR
> > -
> >  /* Extra Environment */
> >  #ifndef CONFIG_SPL_BUILD
> >
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 2/3] arm: socfpga: a10: move SPL stack size to Kconfig

[Marek Vasut]

On 3/1/19 8:48 AM, Simon Goldschmidt wrote:
> On Thu, Feb 28, 2019 at 10:44 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
>>> Instead of fixing the SPL stack to 64 KiB in the board config header via
>>> CONFIG_SYS_SPL_MALLOC_SIZE, let's just use CONFIG_SPL_SYS_MALLOC_F_LEN
>>> in the defconfig.
>>>
>>> This also has the advandage that it removes sub-mach specific ifdefs in
>>> socfpga_common.h.
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>
>> Can't we default these stack sizes with imply in Kconfig files instead ?
>> Then they won't have to be in the defconfigs either.
> 
> I guess you mean default the *malloc* sizes, not *stack* sizes? Yes, that
> should work.

Yes, that's what I meant.

> I started this whole series because all gen5 defconfigs set the initial malloc
> size to 8 KiB, while I monitored socrates to only need a bit more than 1 KiB.
> With my reset patches, something around 1.5 KiB is still enough. So I'd
> move that to imply 2 KiB for gen5 and 64 KiB for a10 in a patch 4/4 for v2.

Sounds good!

> Regards,
> Simon
> 
>>
>>> ---
>>>
>>>  configs/socfpga_arria10_defconfig |  1 +
>>>  include/configs/socfpga_common.h  | 14 --------------
>>>  2 files changed, 1 insertion(+), 14 deletions(-)
>>>
>>> diff --git a/configs/socfpga_arria10_defconfig b/configs/socfpga_arria10_defconfig
>>> index f321a0ac3b..8d0479cc05 100644
>>> --- a/configs/socfpga_arria10_defconfig
>>> +++ b/configs/socfpga_arria10_defconfig
>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
>>>  CONFIG_ARCH_SOCFPGA=y
>>>  CONFIG_SYS_TEXT_BASE=0x01000040
>>>  CONFIG_SYS_MALLOC_F_LEN=0x2000
>>> +CONFIG_SPL_SYS_MALLOC_F_LEN=0x10000
>>>  CONFIG_TARGET_SOCFPGA_ARRIA10_SOCDK=y
>>>  CONFIG_SPL=y
>>>  CONFIG_IDENT_STRING="socfpga_arria10"
>>> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
>>> index a3fbca0a5d..c23b34186a 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -251,16 +251,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>  #define CONFIG_SPL_TEXT_BASE         CONFIG_SYS_INIT_RAM_ADDR
>>>  #define CONFIG_SPL_MAX_SIZE          CONFIG_SYS_INIT_RAM_SIZE
>>>
>>> -#if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> -/* SPL memory allocation configuration, this is for FAT implementation */
>>> -#ifndef CONFIG_SYS_SPL_MALLOC_START
>>> -#define CONFIG_SYS_SPL_MALLOC_SIZE   0x00010000
>>> -#define CONFIG_SYS_SPL_MALLOC_START  (CONFIG_SYS_INIT_RAM_SIZE - \
>>> -                                      CONFIG_SYS_SPL_MALLOC_SIZE + \
>>> -                                      CONFIG_SYS_INIT_RAM_ADDR)
>>> -#endif
>>> -#endif
>>> -
>>>  /* SPL SDMMC boot support */
>>>  #ifdef CONFIG_SPL_MMC_SUPPORT
>>>  #if defined(CONFIG_SPL_FS_FAT) || defined(CONFIG_SPL_FS_EXT4)
>>> @@ -287,11 +277,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>  /*
>>>   * Stack setup
>>>   */
>>> -#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>  #define CONFIG_SPL_STACK             CONFIG_SYS_INIT_SP_ADDR
>>> -#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> -#define CONFIG_SPL_STACK             CONFIG_SYS_SPL_MALLOC_START
>>> -#endif
>>>
>>>  /* Extra Environment */
>>>  #ifndef CONFIG_SPL_BUILD
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] arm: socfpga: put initial U-Boot stack into DDR

[Marek Vasut]

On 3/1/19 8:55 AM, Simon Goldschmidt wrote:
> On Thu, Feb 28, 2019 at 10:44 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/28/19 9:33 PM, Simon Goldschmidt wrote:
>>> If SPL pre-reloc stage puts the stack into DDR, U-Boot should be able to
>>> do that, too.
>>
>> Here you mention SPL pre-reloc stage, while about mid-way through the
>> patch you have a comment talking about SPL post-reloc stage, which one
>> is it ? I think the commit message needs rewording, it's not clear what
>> the patch does based on it.
> 
> Ehrm right. The commit message is wrong. Post-reloc is correct.
> 
>> On a separate note, this is likely a fix for current release, right ?
> 
> Not really. For A10, this should not change anything, just ensure the common
> header file does not use 2 different ways to achieve the same thing.
> Note that A10 does *not* enable a different stack/malloc area for SPL
> post-reloc!

OK

> For gen5, yes there is a change, but only in U-Boot, not in SPL. But this is
> not a fix, it's an improvement to be able to make the SPL malloc pool smaller
> without U-Boot overwriting it. It's currently not overwritten as both
> malloc sizes
> are the same. I'd change that in a follow-up patch which reduces SPL malloc
> size for gen5.

Sounds good

>> Finally , keep in mind the A10 needs to load the FPGA before DRAM
>> becomes available, so the stack shenanigans there become even more nasty.
> 
> Right, that's why even the initial malloc size for SPL is 64 KiB,
> right? As written
> above, this patch should not change anything for A10, so this should still work.

No, that's because the A10 does a lot of allocations in SPL iirc.

> From reading the A10 SPL code, I didn't quite get how this works though, as
> it seems the simple-malloc members in gd are only initialized after
> spl_early_init(), so after initializing DT/DM? Or is A10 not yet
> converted to DT?

It's semi-converted. The early part still needs some work.

> Regards,
> Simon
> 
>>> The reason to do so is that this way, U-Boot initial stack can be larger
>>> than SPL initial stack. In situations where we want to save the SPL
>>> in SRAM for next boot without reloading, this prevents overwriting the
>>> SPL DTB in SRAM if U-Boot stack usage gets too high.
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> ---
>>>
>>>  include/configs/socfpga_common.h | 20 +++++++++++++-------
>>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
>>> index c23b34186a..7ae3db233f 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -38,12 +38,23 @@
>>>  #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
>>> +#define CONFIG_SPL_STACK             CONFIG_SYS_BOOTCOUNT_ADDR
>>>  #else
>>> -#define CONFIG_SYS_INIT_SP_ADDR                      \
>>> +#define CONFIG_SPL_STACK                     \
>>>       (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_RAM_SIZE)
>>>  #endif
>>>
>>> +/*
>>> + * U-Boot stack setup: if SPL post-reloc uses DDR stack, use it in pre-reloc
>>> + * phase of U-Boot, too. This prevents overwriting SPL data if stack/heap usage
>>> + * in U-Boot pre-reloc is higher than in SPL.
>>> + */
>>> +#if defined(CONFIG_SPL_STACK_R_ADDR) && CONFIG_SPL_STACK_R_ADDR
>>> +#define CONFIG_SYS_INIT_SP_ADDR              CONFIG_SPL_STACK_R_ADDR
>>> +#else
>>> +#define CONFIG_SYS_INIT_SP_ADDR              CONFIG_SPL_STACK
>>> +#endif
>>> +
>>>  #define CONFIG_SYS_SDRAM_BASE                PHYS_SDRAM_1
>>>
>>>  /*
>>> @@ -274,11 +285,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>>>  #define CONFIG_SYS_NAND_U_BOOT_OFFS  0x40000
>>>  #endif
>>>
>>> -/*
>>> - * Stack setup
>>> - */
>>> -#define CONFIG_SPL_STACK             CONFIG_SYS_INIT_SP_ADDR
>>> -
>>>  /* Extra Environment */
>>>  #ifndef CONFIG_SPL_BUILD
>>>
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut