[U-Boot] [U-Boot, v3, 03/10] powerpc: spl and normal u-boot stage set SYS_MALLOC_F_LEN indepently

Thu Jul 27 12:05:05 UTC 2017

Mario,

> On 27 Jul 2017, at 13:45, Mario Six <mariosix1986 at gmail.com> wrote:
> 
> Hi Phillip,
> 
> On Thu, Jul 27, 2017 at 12:48 PM, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>> 
>> 
>> On Mon, 24 Jul 2017, Andy Yan wrote:
>> 
>>> Some platforms has very small sram to run spl code, so
>>> it may have no enough sapce for so much malloc pool before
>>> relocation in spl stage as the normal u-boot stage.
>>> Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition.
>>> 
>>> Signed-off-by: Andy Yan <andy.yan at rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>>> 
>>> ---
>>> 
>>> Changes in v3:
>>> - use CONFIG_VAL(), which suggested by Simon
>>> 
>>> Changes in v2: None
>>> 
>>> arch/powerpc/cpu/mpc83xx/start.S |  8 ++++----
>>> arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------
>>> 2 files changed, 9 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/cpu/mpc83xx/start.S
>>> b/arch/powerpc/cpu/mpc83xx/start.S
>>> index 2fed4a1..1c3c737 100644
>>> --- a/arch/powerpc/cpu/mpc83xx/start.S
>>> +++ b/arch/powerpc/cpu/mpc83xx/start.S
>>> @@ -274,14 +274,14 @@ in_flash:
>>>        cmplw   r3, r4
>>>        bne     1b
>>> 
>>> -#ifdef CONFIG_SYS_MALLOC_F_LEN
>>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN)
>>> 
>>> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE >
>>> CONFIG_SYS_INIT_RAM_SIZE
>>> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM."
>>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE >
>>> CONFIG_SYS_INIT_RAM_SIZE
>>> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM."
>>> #endif
>>> 
>>>        /* r3 = new stack pointer / pre-reloc malloc area */
>>> -       subi    r3, r3, CONFIG_SYS_MALLOC_F_LEN
>>> +       subi    r3, r3, CONFIG_VAL(SYS_MALLOC_F_LEN)
>>> 
>>>        /* Set pointer to pre-reloc malloc area in GD */
>>>        stw     r3, GD_MALLOC_BASE(r4)
>>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S
>>> b/arch/powerpc/cpu/mpc85xx/start.S
>>> index 63fdffd..58cb9fc 100644
>>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>>> @@ -1183,14 +1183,13 @@ _start_cont:
>>>        lis     r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
>>>        ori     r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to
>>> 16 */
>>> 
>>> -#ifdef CONFIG_SYS_MALLOC_F_LEN
>>> -
>>> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE >
>>> CONFIG_SYS_INIT_RAM_SIZE
>>> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM."
>>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN)
>>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE >
>>> CONFIG_SYS_INIT_RAM_SIZE
>>> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM."
>>> #endif
>>> 
>>>        /* Leave 16+ byte for back chain termination and NULL return
>>> address */
>>> -       subi    r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf)
>>> +       subi    r3,r3,((CONFIG_VAL(SYS_MALLOC_F_LEN)+16+15)&~0xf)
>>> #endif
>> 
>> 
>> Could we now just drop the outermost "#if CONFIG_VAL(SYS_MALLOC_F_LEN)"
>> guard?
>> 
>> (And as a question to someone with knowledge of the history of this code:)
>> Why are those 16 bytes ("for back chain termination and NULL return
>> address") consumed only when CONFIG_SYS_MALLOC_F_LEN is defined? Shouldn't
>> this be required both with SYS_MALLOC_F_LEN and without?
>> 
> 
> The code is a bit tricky here: The actual back chain termination and NULL
> return address setting is further down:
> 
> stw     r0,0(r3)        /* Terminate Back Chain *
> stw     r0,+4(r3)       /* NULL return address. */
> 
> Now, if we don't have a pre-reloc malloc area, the r3 we computed via
> 
> lis     r3,(CONFIG_SYS_INIT_RAM_ADDR)@h
> ori     r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */
> 
> will just become our stack pointer.
> 
> The global data are starts right after the stack area
> (CONFIG_SYS_INIT_SP_OFFSET == CONFIG_SYS_GBL_DATA_OFFSET), which is OK at first
> glance, since the stack "grows downwards", but since the back chain termination
> and null address setting write beyond the stack pointer (see above), they would
> write into the GD area (and might be overwritten). But since we subtracted 16
> in the above statement, we have room left to store them.
> 
> The case where we have a pre-reloc malloc area is similar: Here we subtract the
> pre-reloc malloc area's size from the computed r3 and also a bit more to keep
> enough space for the back chain termination and null address:
> 
> subi    r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf)
> 
> Otherwise, the addresses of these would lie in the pre-reloc malloc area, and
> might be overwritten.
> 
> So the statement basically exists to "fix" what the substraction of the
> pre-reloc malloc area size disturbed. If we don't have pre-reloc malloc, the
> initial setting of r3 (with the -16 offset) takes care of that already.

Thanks for reviewing.

So how do you want to handle this: should I just merge this as-is (as it’s in
a series that will go through the rockchip tree) or should we try to simplify
this code in the same go?

> 
>>> 
>>>        /* End of RAM */
>>> @@ -1204,7 +1203,7 @@ _start_cont:
>>>        cmplw   r4,r3
>>>        bne     1b
>>> 
>>> -#ifdef CONFIG_SYS_MALLOC_F_LEN
>>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN)
>>>        lis     r4,(CONFIG_SYS_INIT_RAM_ADDR)@h
>>>        ori     r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l
>>> 
>>> 
> 
> Best regards,
> 
> Mario