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

[Hans de Goede]

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 <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) :)

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