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

[Mario Six]

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.

>>
>>         /* 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