diff mbox

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

Message ID 1439827706-1748-2-git-send-email-hdegoede@redhat.com
State Deferred
Delegated to: Tom Rini
Headers show

Commit Message

Hans de Goede Aug. 17, 2015, 4:08 p.m. UTC 
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(-)

Comments

Simon Glass Aug. 18, 2015, 1:59 a.m. UTC | #1 
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?

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.

Regards,
Simon
Hans de Goede Aug. 18, 2015, 9:23 a.m. UTC | #2 
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.

Regards,

Hans
Simon Glass Aug. 18, 2015, 12:45 p.m. UTC | #3 
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
Simon Glass Aug. 18, 2015, 3:46 p.m. UTC | #4 
Hi Hans,

On 18 August 2015 at 06:45, Simon Glass <sjg@chromium.org> wrote:
> 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().

(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
Hans de Goede Aug. 19, 2015, 11:40 a.m. UTC | #5 
Hi,

On 18-08-15 14:45, Simon Glass wrote:
> 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.

Sounds like a plan, I'll give this a try, may be a while before I get
around to doing this though.

Regards,

Hans
diff mbox

Patch

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