arm: socfpga: check total size of SPL

PATCH New Simon Goldschmidt 3 participants 13 messages Oct 30, 2018 – Nov 2, 2018

Share & tools

Simon Goldschmidt

author

2018-10-30 21:23 UTC #0 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).

The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).

Signed-off-by Simon Goldschmidt <[email protected]>

---

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

include/configs/socfpga_common.h +7 -0

index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
 
+/* Check total size of SPL including BSS, malloc area and gd */
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT	(CONFIG_SYS_INIT_SP_ADDR - \
+					 CONFIG_SYS_INIT_RAM_ADDR - \
+					 CONFIG_SYS_MALLOC_F_LEN - \
+					 GENERATED_GBL_DATA_SIZE)
+
 #if defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
 /* SPL memory allocation configuration, this is for FAT implementation */
 #ifndef CONFIG_SYS_SPL_MALLOC_START

Signature · 1 line

2.17.1

Marek Vasut

2018-10-30 21:26 UTC #1 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 18 lines · Simon Goldschmidt

Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
 include/configs/socfpga_common.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250

Quoted 9 lines

include/configs/socfpga_common.h +6 -0

@@ -242,6 +242,13 @@ 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
 
+/* Check total size of SPL including BSS, malloc area and gd */
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT	(CONFIG_SYS_INIT_SP_ADDR - \
+					 CONFIG_SYS_INIT_RAM_ADDR - \
+					 CONFIG_SYS_MALLOC_F_LEN - \
+					 GENERATED_GBL_DATA_SIZE)

Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?

Signature · 2 lines

Best regards,
Marek Vasut

Simon Goldschmidt

author

2018-10-30 21:30 UTC #2 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 22 lines · Marek Vasut

Quoted 21 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/configs/socfpga_common.h | 7 +++++++
  1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
 

Quoted 9 lines

include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250
Quoted 6 lines
include/configs/socfpga_common.h +6 -0
+/* Check total size of SPL including BSS, malloc area and gd */
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT	(CONFIG_SYS_INIT_SP_ADDR - \
+					 CONFIG_SYS_INIT_RAM_ADDR - \
+					 CONFIG_SYS_MALLOC_F_LEN - \
+					 GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?

Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".

But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.

Simon

Marek Vasut

2018-10-30 21:36 UTC #3 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 33 lines · Simon Goldschmidt

Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
include/configs/socfpga_common.h | 7 +++++++
  1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
+/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?

Quoted 7 lines

Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.

Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...

Signature · 2 lines

Best regards,
Marek Vasut

Simon Goldschmidt

author

2018-10-31 05:44 UTC #4 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/configs/socfpga_common.h | 7 +++++++
  1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
  +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".

Quoted 5 lines

Quoted 2 lines
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...

You're probably right. I'll send a V2 making this more clear.

Simon

Marek Vasut

2018-10-31 10:00 UTC #5 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 41 lines · Simon Goldschmidt

Quoted 40 lines · Marek Vasut
Quoted 38 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/configs/socfpga_common.h | 7 +++++++
  1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
  +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.

Thanks

Signature · 2 lines

Best regards,
Marek Vasut

Simon Goldschmidt

author

2018-11-01 19:26 UTC #6 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 40 lines · Marek Vasut

Quoted 39 lines · Simon Goldschmidt
Quoted 38 lines · Marek Vasut
Quoted 37 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
   include/configs/socfpga_common.h | 7 +++++++
   1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
   +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 250
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".

Quoted 6 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.
Thanks

Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?

Simon

Marek Vasut

2018-11-01 19:51 UTC #7 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 51 lines · Simon Goldschmidt

Quoted 50 lines · Marek Vasut
Quoted 49 lines · Simon Goldschmidt
Quoted 47 lines · Marek Vasut
Quoted 35 lines · Simon Goldschmidt
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
include/configs/socfpga_common.h | 7 +++++++
   1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
+/* Check total size of SPL including BSS, malloc area and gd */
include/configs/socfpga_common.h +2 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
Quoted 8 lines
Quoted 3 lines
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the
SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or
something ?
Quoted 8 lines
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please,
otherwise the confusion will continue ...

Quoted 6 lines

Quoted 2 lines
Quoted 1 line
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?

Stack just grows down, that's all, there's no limit.

Signature · 2 lines

Best regards,
Marek Vasut

Simon Goldschmidt

author

2018-11-02 21:33 UTC #8 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Am Fr., 2. Nov. 2018, 01:08 hat Marek Vasut <[email protected]> geschrieben:

Quoted 52 lines · Simon Goldschmidt

Quoted 51 lines · Marek Vasut
Quoted 50 lines · Simon Goldschmidt
Quoted 49 lines
Quoted 47 lines · Marek Vasut
Quoted 35 lines · Simon Goldschmidt
Quoted 26 lines · Marek Vasut
Quoted 25 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
   include/configs/socfpga_common.h | 7 +++++++
   1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +2 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
   +/* Check total size of SPL including BSS, malloc area and gd */
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
Quoted 8 lines
include/configs/socfpga_common.h unsigned int · line 250
Quoted 3 lines
include/configs/socfpga_common.h +3 -0
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the
SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or
something ?
Quoted 8 lines
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please,
otherwise the confusion will continue ...

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
Quoted 1 line
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?
Stack just grows down, that's all, there's no limit.

Right. But that sounds kind of dangerous...

And it would make sense to let heap and stack grow against each other instead of letting heap grow up against end of ram and stack down against bss_end or the devicetree...

To achieve that, 'gd' could be placed as low as possible, but the common startup code doesn't seem to make that easy, especially as there is no linker symbol for the devicetree start/end of it is not embedded.

Simon

Quoted 1 line

Simon Goldschmidt

author

2018-11-01 20:31 UTC #9 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

In reply to Simon Goldschmidt

Quoted 44 lines · Simon Goldschmidt

Quoted 42 lines · Marek Vasut
Quoted 41 lines · Simon Goldschmidt
Quoted 40 lines · Marek Vasut
Quoted 39 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
   include/configs/socfpga_common.h | 7 +++++++
   1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
   +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 249
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?

And to make matters worse, the devicetree is also not included in the size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select CONFIG_OF_EMBED.

While this would be OK for SPL, I think it wouldn't be for U-Boot. How do other platforms handle this size check?

Simon

Marek Vasut

2018-11-01 20:59 UTC #10 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 55 lines · Simon Goldschmidt

Quoted 44 lines · Simon Goldschmidt
Quoted 42 lines · Marek Vasut
Quoted 41 lines · Simon Goldschmidt
Quoted 40 lines · Marek Vasut
Quoted 39 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
   include/configs/socfpga_common.h | 7 +++++++
   1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
   +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 249
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?

Quoted 6 lines

And to make matters worse, the devicetree is also not included in the size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select CONFIG_OF_EMBED.
While this would be OK for SPL, I think it wouldn't be for U-Boot. How do other platforms handle this size check?

Can git grep help here ?

Signature · 2 lines

Best regards,
Marek Vasut

Simon Goldschmidt

author

2018-11-01 21:13 UTC #11 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 53 lines · Marek Vasut

Quoted 52 lines · Simon Goldschmidt
Quoted 50 lines · Simon Goldschmidt
Quoted 40 lines · Marek Vasut
Quoted 39 lines · Simon Goldschmidt
Quoted 38 lines · Marek Vasut
Quoted 37 lines · Simon Goldschmidt
Quoted 23 lines · Marek Vasut
Quoted 22 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by Simon Goldschmidt <[email protected]>
---
    include/configs/socfpga_common.h | 7 +++++++
    1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
    +/* Check total size of SPL including BSS, malloc area and gd */
Quoted 8 lines
include/configs/socfpga_common.h unsigned int cm_get_qspi_controller_clk_hz(void); · line 249
Quoted 5 lines
include/configs/socfpga_common.h +5 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Now that's confusing :-) Add a comment explaining this into a V2 please, otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?

Quoted 7 lines

Quoted 6 lines
And to make matters worse, the devicetree is also not included in the size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select CONFIG_OF_EMBED.
While this would be OK for SPL, I think it wouldn't be for U-Boot. How do other platforms handle this size check?
Can git grep help here ?

Not really. If so, I wouldn't have asked ;-)

I can only git grep for known strings and I only see constants for CONFIG_SPL_MAX_SIZE.
CONFIG_SPL_MAX_FOOTPRINT seems to be rarely used and no platform seems to be taking the devicetree into account.

Stack usage seems to be covered in one or two boards, but I expect this to be quite platform specific (given he different drivers at least), so we might need an additional runtime check, which I haven't seen so far? In my (smaller) embedded projects, I often enable runtime stack checks to be on the safe side...

Maybe printing free memory at build-time and printing stack usage at runtime would be a solution? But I only know U-Boot from the socfpga gen5 perspective, that's why I ask.

Simon

Marek Vasut

2018-11-01 21:21 UTC #12 lore raw

PATCH 1/1 arm: socfpga: check total size of SPL

Quoted 68 lines · Simon Goldschmidt

Quoted 67 lines · Marek Vasut
Quoted 66 lines · Simon Goldschmidt
Quoted 50 lines · Simon Goldschmidt
Quoted 49 lines · Marek Vasut
Quoted 48 lines · Simon Goldschmidt
Quoted 46 lines · Marek Vasut
Quoted 45 lines · Simon Goldschmidt
Quoted 28 lines · Marek Vasut
Quoted 27 lines · Simon Goldschmidt
Correctly define CONFIG_SPL_MAX_FOOTPRINT to make the default arm linker script for SPL check the total SRAM size available for SPL (code, data, bss, heap, global data).
The previously existing define CONFIG_SPL_MAX_SIZE seems to only check the binary size (which is without bss, heap and gd).
Signed-off-by: Simon Goldschmidt
<[email protected]>
---
include/configs/socfpga_common.h | 7 +++++++
    1 file changed, 7 insertions(+)
include/configs/socfpga_common.h +0 -0
index 2330143cf1..9103d0a966 100644
@@ -242,6 +242,13 @@ 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
+/* Check total size of SPL including BSS, malloc area and gd */
include/configs/socfpga_common.h +2 -0
+#include <generated/generic-asm-offsets.h>
+#define CONFIG_SPL_MAX_FOOTPRINT    (CONFIG_SYS_INIT_SP_ADDR - \
Quoted 8 lines
Quoted 3 lines
+                     CONFIG_SYS_INIT_RAM_ADDR - \
+                     CONFIG_SYS_MALLOC_F_LEN - \
+                     GENERATED_GBL_DATA_SIZE)
Are you sure this calculation is correct ? INIT_SP_ADDR is I think the
SRAM offset in the address space. Shouldn't that contain INIT_SP_SIZE or
something ?
Yes, I'm pretty sure. INIT_SP_ADDR is defined as INIT_RAM_ADDR + INIT_RAM_SIZE.
So by subtracting INIT_RAM_ADDR again, I effectively get "INIT_RAM_ADDR
- MALLOC_F_LEN - GBL_DATA_SIZE".
But I did it this way to keep it working after Stefan's fix for reserving the boot counter location is applied.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Now that's confusing :-) Add a comment explaining this into a V2 please,
otherwise the confusion will continue ...
You're probably right. I'll send a V2 making this more clear.
Thanks
Re-checking this, I found the check is still not correct as it would leave 4 bytes for the stack only. Is there an option that controls how much stack should be preserved for SPL somewhere?
And to make matters worse, the devicetree is also not included in the size check done by 'arch/arm/cpu/u-boot-spl.lds' unless we select CONFIG_OF_EMBED.
While this would be OK for SPL, I think it wouldn't be for U-Boot. How do other platforms handle this size check?

Quoted 8 lines

Quoted 1 line
Can git grep help here ?
Not really. If so, I wouldn't have asked ;-)
I can only git grep for known strings and I only see constants for CONFIG_SPL_MAX_SIZE.
CONFIG_SPL_MAX_FOOTPRINT seems to be rarely used and no platform seems to be taking the devicetree into account.

Ouch :(

Quoted 5 lines

Stack usage seems to be covered in one or two boards, but I expect this to be quite platform specific (given he different drivers at least), so we might need an additional runtime check, which I haven't seen so far? In my (smaller) embedded projects, I often enable runtime stack checks to be on the safe side...

Makes sense.

Quoted 3 lines

Maybe printing free memory at build-time and printing stack usage at runtime would be a solution? But I only know U-Boot from the socfpga gen5 perspective, that's why I ask.

Wouldn't the stack usage change with usage too ? I think it might, albeit slightly. I'm not sure this would be really useful then.

Signature · 2 lines

Best regards,
Marek Vasut