[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 06:45, Simon Glass <sjg at chromium.org> wrote:
> 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().

(Little update - this is not strictly true - with your patch you
needed to change board_init_f_mem(), but with my thoughts below you
don't need to)

>
> 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