From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Tue, 22 Sep 2015 11:52:00 +0200 Subject: [U-Boot] [PATCH 4/7] malloc_simple: Add support for switching to DRAM heap In-Reply-To: References: <1442158965-29962-1-git-send-email-hdegoede@redhat.com> <1442158965-29962-5-git-send-email-hdegoede@redhat.com> Message-ID: <560124C0.80408@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >> --- >> 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