Hi Hans,
On 18 August 2015 at 03:23, Hans de Goede <hdego...@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 <hdego...@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 <hdego...@redhat.com>
>>> Reviewed-by: Simon Glass <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
