Message ID | 1442158965-29962-5-git-send-email-hdegoede@redhat.com |
---|---|
State | Accepted |
Delegated to: | Hans de Goede |
Headers | show |
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? > + 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. Regards, Simon
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
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) { + 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;
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(+)