[U-Boot] [PATCH 4/7] malloc_simple: Add support for switching to DRAM heap

[Hans de Goede <hdegoede@redhat.com>]

Hi,

Thanks for all the reviews.

On 22-09-15 06:00, Simon Glass wrote:
> Hi Hans,
>
> On 13 September 2015 at 09:42, Hans de Goede <hdegoede@redhat.com> wrote:
>> malloc_simple uses a part of the stack as heap, initially it uses
>> SYS_MALLOC_F_LEN bytes which typically is quite small as the initial
>> stacks sits in SRAM and we do not have that much SRAM to work with.
>>
>> When DRAM becomes available we may switch the stack from SRAM to DRAM
>> to give use more room. This commit adds support for also switching to
>> a new bigger malloc_simple heap located in the new stack.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   Kconfig          | 10 ++++++++++
>>   common/spl/spl.c |  9 +++++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/Kconfig b/Kconfig
>> index 0ae4fab..86088bc 100644
>> --- a/Kconfig
>> +++ b/Kconfig
>> @@ -142,6 +142,16 @@ config SPL_STACK_R_ADDR
>>            Specify the address in SDRAM for the SPL stack. This will be set up
>>            before board_init_r() is called.
>>
>> +config SPL_STACK_R_MALLOC_SIMPLE_LEN
>> +       depends on SPL_STACK_R && SPL_MALLOC_SIMPLE
>> +       hex "Size of malloc_simple heap after switching to DRAM SPL stack"
>> +       default 0x100000
>> +       help
>> +         Specify the amount of the stack to use as memory pool for
>> +         malloc_simple after switching the stack to DRAM. This may be set
>> +         to give board_init_r() a larger heap then the initial heap in
>> +         SRAM which is limited to SYS_MALLOC_F_LEN bytes.
>> +
>>   config TPL
>>          bool
>>          depends on SPL && SUPPORT_TPL
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index b09a626..8c2d109 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -347,6 +347,15 @@ ulong spl_relocate_stack_gd(void)
>>          memcpy(new_gd, (void *)gd, sizeof(gd_t));
>>          gd = new_gd;
>>
>> +#ifdef CONFIG_SPL_MALLOC_SIMPLE
>> +       if (CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN) {
>
> Do you think we could do:
>
> if (IS_ENABLED(CONFIG_SPL_MALLOC_SIMPLE) &&
> CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN)
>
> to avoid the #ifdef?

AFAIK we cannot do that because CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN will
not be defined if CONFIG_SPL_MALLOC_SIMPLE is not set, so then
the c compiler will end up looking for a symbol called
CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN and will not find it.

>> +               ptr -= CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN;
>> +               gd->malloc_base = ptr;
>> +               gd->malloc_limit = CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN;
>> +               gd->malloc_ptr = 0;
>> +       }
>> +#endif
>> +
>>          return ptr;
>>   #else
>>          return 0;
>> --
>> 2.4.3
>>
>
> I have to say I worry a little bit about combinatoric explosion with
> this series. But I can't immediately see a better way.

We could simply always relocate the heap when using malloc_simple and
CONFIG_SPL_STACK_R is set, code wise this would mean dropping the
CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN != 0 check, simplifying the code
somewhat (and allowing us to switch to using if (IS_ENABLED(CONFIG_SPL_MALLOC_SIMPLE)
instead #ifdef.

This will also half the number of memory layout variants we have in
the SPL, thus reducing the combinatoric explosion.

Downsides are:

1) If someone sets CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN to 0 things will
break. We can add text to the Kconfig help saying not to do that, which
IMHO is a good enough fix for this

2) This forces all users who use both SPL_STACK_R and SPL_SYS_MALLOC_SIMPLE
to also get their malloc_simple heap relocated, and I guess this may be
undesirable in some cases, although I cannot think of one.

2. is the reason why I wrote this patch as it is written, I have already
considered going the suggested route while writing the patch. I'm fine
either way though, if you think that making heap reloc mandatory when
using both SPL_STACK_R and SPL_SYS_MALLOC_SIMPLE that is fine with me.

Regards,

Hans