[U-Boot] malloc_simple: Allow malloc_simple to be used with non stack RAM

[U-Boot] malloc_simple: Allow malloc_simple to be used with non stack RAM

[Hans de Goede]

Hi Tom and Simon,

Here is an old patch of mine, which at the time I
send it got reviewed by Simon, but never got merged.

I've just brushed this patch of, because it shrinks the SPL
by about 3k, and with the spl nand support we've recently
added we are going over the size limit with the SPL on
some sunxi boards.

So I would like to see this merged. Simon, does your ack still
stand for this patch ? Tom, if so can you merge this please ?

Note this patch has close to 0 chance of causing regressions as it
is a nop unless the new CONFIG_SYS_MALLOC_F_BASE define is defined.

Patch 2 / 3 are merely added as example / for Ian to review :)
I will merge these through the sunxi tree once patch 1 is merged.

Thanks & Regards,

Hans

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

[Hans de Goede]

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

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

[Simon Glass]

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

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

[Hans de Goede]

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

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

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

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

[Hans de Goede]

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

[U-Boot] [PATCH 2/3] sunxi: Switch to using malloc_simple for the spl

[Hans de Goede]

common/dlmalloc.c is quite big, both in .text and .data usage. E.g. for a
Mele_M9 sun6i board build this reduces .text from 0x4214 to 0x3b94 bytes, and
.data from 0x54c to 0x144 bytes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/configs/sunxi-common.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 837feaf..1776609 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -80,7 +80,7 @@
 #define CONFIG_SYS_LOAD_ADDR		0x22000000 /* default load address */
 #define CONFIG_SYS_TEXT_BASE		0x2a000000
 #define CONFIG_PRE_CON_BUF_ADDR		0x2f000000
-#define CONFIG_SYS_SPL_MALLOC_START	0x2ff00000
+#define CONFIG_SPL_MALLOC_F_BASE	0x2ff00000
 #define CONFIG_SPL_BSS_START_ADDR	0x2ff80000
 #else
 #define SDRAM_OFFSET(x) 0x4##x
@@ -88,12 +88,19 @@
 #define CONFIG_SYS_LOAD_ADDR		0x42000000 /* default load address */
 #define CONFIG_SYS_TEXT_BASE		0x4a000000
 #define CONFIG_PRE_CON_BUF_ADDR		0x4f000000
-#define CONFIG_SYS_SPL_MALLOC_START	0x4ff00000
+#define CONFIG_SPL_MALLOC_F_BASE	0x4ff00000
 #define CONFIG_SPL_BSS_START_ADDR	0x4ff80000
 #endif
 
 #define CONFIG_SPL_BSS_MAX_SIZE		0x00080000 /* 512 KiB */
-#define CONFIG_SYS_SPL_MALLOC_SIZE	0x00080000 /* 512 KiB */
+#define CONFIG_SPL_MALLOC_F_LEN		0x00080000 /* 512 KiB */
+
+#ifdef CONFIG_SPL_BUILD
+#define CONFIG_SYS_MALLOC_SIMPLE	/* Use malloc_simple to save space */
+#undef CONFIG_SYS_MALLOC_F_LEN
+#define CONFIG_SYS_MALLOC_F_LEN		CONFIG_SPL_MALLOC_F_LEN
+#define CONFIG_SYS_MALLOC_F_BASE	CONFIG_SPL_MALLOC_F_BASE
+#endif
 
 #ifdef CONFIG_MACH_SUN9I
 /*
-- 
2.4.3

[U-Boot] [PATCH 2/3] sunxi: Switch to using malloc_simple for the spl

[Ian Campbell]

On Mon, 2015-08-17 at 18:08 +0200, Hans de Goede wrote:
> common/dlmalloc.c is quite big, both in .text and .data usage. E.g. 
> for a
> Mele_M9 sun6i board build this reduces .text from 0x4214 to 0x3b94 
> bytes, and
> .data from 0x54c to 0x144 bytes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Acked-by: Ian Campbell <ijc@hellion.org.uk>

[U-Boot] [PATCH 3/3] sunxi: sunxi-common.h cleanup

[Hans de Goede]

Move some #define-s around from one #ifdef block to another to
reduce the number of #ifdef blocks (note this causes no functional
changes even though the conditions are not always exactly the same)
and move generic #include statements to the top.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/configs/sunxi-common.h | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 1776609..915b597 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -13,6 +13,7 @@
 #ifndef _SUNXI_COMMON_CONFIG_H
 #define _SUNXI_COMMON_CONFIG_H
 
+#include <asm/arch/cpu.h>
 #include <linux/stringify.h>
 
 #ifdef CONFIG_OLD_SUNXI_KERNEL_COMPAT
@@ -35,27 +36,15 @@
  * High Level Configuration Options
  */
 #define CONFIG_SUNXI		/* sunxi family */
-#ifdef CONFIG_SPL_BUILD
-#define CONFIG_SYS_THUMB_BUILD	/* Thumbs mode to save space in SPL */
-#endif
-
-#include <asm/arch/cpu.h>	/* get chip and board defs */
-
-#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_DM_SERIAL)
-# define CONFIG_DW_SERIAL
-#endif
-
-/*
- * Display CPU information
- */
-#define CONFIG_DISPLAY_CPUINFO
 
 /* Serial & console */
 #define CONFIG_SYS_NS16550
 #define CONFIG_SYS_NS16550_SERIAL
 /* ns16550 reg in the low bits of cpu reg */
 #define CONFIG_SYS_NS16550_CLK		24000000
-#ifndef CONFIG_DM_SERIAL
+#ifdef CONFIG_DM_SERIAL
+# define CONFIG_DW_SERIAL
+#else
 # define CONFIG_SYS_NS16550_REG_SIZE	-4
 # define CONFIG_SYS_NS16550_COM1		SUNXI_UART0_BASE
 # define CONFIG_SYS_NS16550_COM2		SUNXI_UART1_BASE
@@ -65,6 +54,7 @@
 #endif
 
 /* CPU */
+#define CONFIG_DISPLAY_CPUINFO
 #define CONFIG_SYS_CACHELINE_SIZE	64
 
 /*
@@ -96,6 +86,7 @@
 #define CONFIG_SPL_MALLOC_F_LEN		0x00080000 /* 512 KiB */
 
 #ifdef CONFIG_SPL_BUILD
+#define CONFIG_SYS_THUMB_BUILD		/* Thumbs mode to save space in SPL */
 #define CONFIG_SYS_MALLOC_SIMPLE	/* Use malloc_simple to save space */
 #undef CONFIG_SYS_MALLOC_F_LEN
 #define CONFIG_SYS_MALLOC_F_LEN		CONFIG_SPL_MALLOC_F_LEN
-- 
2.4.3

[U-Boot] [PATCH 3/3] sunxi: sunxi-common.h cleanup

[Ian Campbell]

On Mon, 2015-08-17 at 18:08 +0200, Hans de Goede wrote:
> Move some #define-s around from one #ifdef block to another to
> reduce the number of #ifdef blocks (note this causes no functional
> changes even though the conditions are not always exactly the same)
> and move generic #include statements to the top.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ian Campbell <ijc@hellion.org.uk>