[U-Boot] [PATCH] rockchip: Reconfigure the malloc based to point to system memory

[Sjoerd Simons]

Hey Hans,

On Thu, 2015-10-01 at 12:08 +0200, Hans de Goede wrote:
> Hi Sjoerd,
> 
> On 01-10-15 11:10, Sjoerd Simons wrote:
> > When malloc_base initially gets setup in the SPL it is based on the
> > current (early) stack pointer, which for rockchip is pointing into
> > SRAM.
> > This means simple memory allocations happen in SRAM space, which is
> > somewhat unfortunate. Specifically a bounce buffer for the mmc
> > allocated
> > in SRAM space seems to cause the mmc engine to stall/fail causing
> > timeouts and a failure to load the main u-boot image.
> > 
> > To resolve this, reconfigure the malloc_base to start at the
> > relocated
> > stack pointer after DRAM  has been setup.
> > 
> > For reference, things did work fine on rockchip before 596380db was
> > merged to fix memalign_simple due to a combination of rockchip
> > SDRAM
> > starting at address 0 and the dw_mmc driver not checking errors
> > from
> > bounce_buffer_start. As a result, when a bounce buffer needed to be
> > allocated mem_align simple would fail and return NULL. The mmc
> > driver
> > ignored the error and happily continued with the bounce buffer
> > address
> > being set to 0, which just happened to work fine..
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
> > 
> > ---
> > A potentially better fix for this issue would be to reconfigure the
> > malloc_base in spl_relocate_stack_gd following the same steps as is
> > done
> > for the initial setup.
> 
> I actually have a patch series pending for this:
> 
> http://patchwork.ozlabs.org/patch/517191/
> http://patchwork.ozlabs.org/patch/517194/
> http://patchwork.ozlabs.org/patch/517193/
> http://patchwork.ozlabs.org/patch/517195/
> http://patchwork.ozlabs.org/patch/517196/
> 
> (I've omitted 2 uninteresting patches)
> 
> Your review of / input on this series would be appreciated.

Cool, I'll try to make some time to give that a closer look.

>  > However at this point in the release cycle i
> > preferred to do a minimal rockchip only fix (so those boards become
> > bootable again) for this issue to minimize the potential impact on
> > other
> > boards.
> 
> I agree that a minimal rockchip only fix likely is best at this time,
> however your fix seems wrong:
> 
> >   arch/arm/mach-rockchip/board-spl.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm/mach-rockchip/board-spl.c b/arch/arm/mach
> > -rockchip/board-spl.c
> > index a241d96..5daced7 100644
> > --- a/arch/arm/mach-rockchip/board-spl.c
> > +++ b/arch/arm/mach-rockchip/board-spl.c
> > @@ -217,6 +217,10 @@ void board_init_f(ulong dummy)
> >   		debug("DRAM init failed: %d\n", ret);
> >   		return;
> >   	}
> > +
> > +	/* Now that DRAM is initialized setup base pointer for
> > simple malloc
> > +	 * into RAM */
> > +	gd->malloc_base = CONFIG_SPL_STACK_R_ADDR;
> >   }
> > 
> >   static int setup_led(void)
> 
> SPL_STACK_R_ADDR is where the stack will be put by
> spl_relocate_stack_gd
> so now you've the stack and the heap overlapping.

If i'm not mistaken the stack grows downward, while the heap grows
upwards so there shouldn't be a conflict. In my understanding the
memory layout after spl_relocate_stack_gd should look something like
this

0x0
.
<misc, other>
.
CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): relocated Stack pointer (growing downwards)
CONFIG_SPL_STACK_ADDR_R - sizeof(gd_t): global data
CONFIG_SPL_STACK_ADDR_R               : Start of heap (growing upward>
CONFIG_SPL_STACK_ADDR_R + CONFIG_SYS_MALLOC_F_LEN: End of heap

I'm pretty sure that's correct, well either that, or i'm missing
something obvious and spl_relocate_stack_gd doesn't make any sense (as
it als puts the new stack pointer to start at the gd location) :)


> You likely will want to make CONFIG_SPL_STACK_R_ADDR be something
> like 0x80000
> and then do:
> 
> 	gd->malloc_base = 0x00000000;
> 	gd->malloc_limit = CONFIG_SPL_STACK_R_ADDR;
> 	gd->malloc_ptr = 0;
> 
> Here to stop the 2 from overlapping, things are likely working with
> your patch since you're not resetting gd->malloc_ptr, so you keep
> space
> for the stack for whatever that is set to at that point.

Actually i've been tempted to put the malloc_base at 0x0, but that
would mean the first malloc_simple call will return 0 so is
indistinguishable from an error.


-- 
Sjoerd Simons
Collabora Ltd.