Hi Hans,
On 18 August 2015 at 06:45, Simon Glass <s...@chromium.org> wrote:
> 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().
(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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
