Mario,
> On 27 Jul 2017, at 13:45, Mario Six <mariosix1...@gmail.com> wrote:
>
> Hi Phillip,
>
> On Thu, Jul 27, 2017 at 12:48 PM, Philipp Tomsich
> <philipp.toms...@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....@rock-chips.com>
>>> Acked-by: Philipp Tomsich <philipp.toms...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
