[U-Boot] [PATCH 1/3] malloc_simple: Allow malloc_simple to be used with non stack RAM

[Simon Glass]

Hi Hans,

On 18 August 2015 at 03:23, Hans de Goede <hdegoede at 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 at 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 at redhat.com>
>>> Reviewed-by: Simon Glass <sjg at 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