Hi Hans,
On 18 August 2015 at 03:23, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 18-08-15 03:59, Simon Glass wrote:
Hi Hans,
On 17 August 2015 at 10:08, Hans de Goede hdegoede@redhat.com wrote:
Before this patch malloc_simple would always allocate a chunk of RAM from the stack. This commit adds a CONFIG_SYS_MALLOC_F_BASE define, which when set directly specifies the memory address to use for the heap with malloc_simple.
Signed-off-by: Hans de Goede hdegoede@redhat.com Reviewed-by: Simon Glass sjg@chromium.org
arch/arm/lib/crt0.S | 2 +- common/board_f.c | 4 ++++ common/dlmalloc.c | 4 ++++ common/spl/spl.c | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index afd4f10..5e6619f 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -96,7 +96,7 @@ clr_gd: strlo r0, [r1] /* clear 32-bit GD word */ addlo r1, r1, #4 /* move to next */ blo clr_gd -#if defined(CONFIG_SYS_MALLOC_F_LEN) +#if defined(CONFIG_SYS_MALLOC_F_LEN) && !defined(CONFIG_SYS_MALLOC_F_BASE) sub sp, sp, #CONFIG_SYS_MALLOC_F_LEN str sp, [r9, #GD_MALLOC_BASE] #endif diff --git a/common/board_f.c b/common/board_f.c index c959774..7f3b96f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -1050,9 +1050,13 @@ ulong board_init_f_mem(ulong top) arch_setup_gd(gd_ptr);
#ifdef CONFIG_SYS_MALLOC_F_LEN +#if defined(CONFIG_SYS_MALLOC_F_BASE)
gd->malloc_base = CONFIG_SYS_MALLOC_F_BASE;+#else top -= CONFIG_SYS_MALLOC_F_LEN; gd->malloc_base = top; #endif +#endif
return top;} diff --git a/common/dlmalloc.c b/common/dlmalloc.c index b5bb051..9b14033 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -3264,7 +3264,11 @@ int mALLOPt(param_number, value) int param_number; int value; int initf_malloc(void) { #ifdef CONFIG_SYS_MALLOC_F_LEN +#if defined(CONFIG_SYS_MALLOC_F_BASE)
gd->malloc_base = CONFIG_SYS_MALLOC_F_BASE;+#else assert(gd->malloc_base); /* Set up by crt0.S */ +#endif gd->malloc_limit = CONFIG_SYS_MALLOC_F_LEN; gd->malloc_ptr = 0; #endif diff --git a/common/spl/spl.c b/common/spl/spl.c index 94b01da..811452b 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -156,6 +156,9 @@ int spl_init(void) #if defined(CONFIG_SYS_MALLOC_F_LEN) gd->malloc_limit = CONFIG_SYS_MALLOC_F_LEN; gd->malloc_ptr = 0; +#if defined(CONFIG_SYS_MALLOC_F_BASE)
gd->malloc_base = CONFIG_SYS_MALLOC_F_BASE;+#endif #endif if (IS_ENABLED(CONFIG_OF_CONTROL) && !IS_ENABLED(CONFIG_SPL_DISABLE_OF_CONTROL)) { -- 2.4.3
Why does this save memory?
See patch 2/3, which does #define CONFIG_SYS_MALLOC_SIMPLE when building the SPL, removing common/dlmalloc.c and only using common/malloc_simple.c both pre and post reloc.
We need this patch to do this because we do not have room on the stack (which sits in SRAM) and setting CONFIG_SYS_MALLOC_F_LEN normally puts the malloc_simple heap there.
We do however have room in DRAM and the SPL (which does not use device- model on sunxi) does not need malloc until after DRAM has been brought up, so we use this to point the malloc_simple.c heap at DRAM (far far away from where u-boot.bin will be loaded).
In general we should move away from hard-coding specific addresses I think, and just work out the memory from a single address, subtracting space for each area we need.
I understand and agree, but I've been unable to find another easy solution for this, and now that we are adding nand support we are really running out of space in the SPL on sunxi, and could really use the circa 3k (out of 24k total) dlmalloc is costing us.
OK thanks for the explanation.
You say that you are trying to change this in SPL but your patch changes U-Boot proper also. If it is just SPL you should not need to change board_init_f_mem() and initf_malloc().
For SPL we now have spl_relocate_stack_gd() which sets up the stack in SDRAM before calling board_init_r(). This implements the CONFIG_SPL_STACK_R option.
We should avoid hard-coding an address if we can. I wonder if we could have bool CONFIG_SPL_MALLOC_R and hex CONFIG_SPL_MALLOC_R_LEN (or hopefully you can think of better names). Then you change could go in spl_relocate_stack_gd(), something like:
if (IS_ENABLED(CONFIG_SPL_STACK_R)) { ptr -= CONFIG_SPL_MALLOC_R_LEN; gd->malloc_base = ptr; }
You'll unfortunately need to add another conditional to the top of spl_init() since gd->malloc_limit will need to be set to a different value.
Regards, Simon