From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 1 Oct 2015 14:19:10 +0200 Subject: [U-Boot] [PATCH] rockchip: Reconfigure the malloc based to point to system memory In-Reply-To: <1443698758.31869.104.camel@collabora.co.uk> References: <1443690640-7568-1-git-send-email-sjoerd.simons@collabora.co.uk> <560D0629.5070003@redhat.com> <1443698758.31869.104.camel@collabora.co.uk> Message-ID: <560D24BE.2040403@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, On 01-10-15 13:25, Sjoerd Simons wrote: > 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 >>> >>> --- >>> 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 > . > > . > 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) :) Ah yes you're right, and since the stack grows downwards I guess that CONFIG_SPL_STACK_ADDR_R already is not 0 for rockchip :) You should probably still reset gd->malloc_ptr to 0, otherwise the new DRAM heap will begin at CONFIG_SPL_STACK_ADDR_R + gd->malloc_ptr and it will be only CONFIG_SYS_MALLOC_F_LEN - gd->malloc_ptr bytes large. Regards, Hans