[U-Boot] [RFC v1 PATCH 1/1] mpc85xx: Enable pre-relocation malloc for MPC85xx

[Mario Six]

Quoting York Sun <york.sun at nxp.com>:

> On 03/30/2016 11:29 PM, Mario Six wrote:
>>
>> Quoting York Sun <york.sun at nxp.com>:
>>
>>> On 03/29/2016 11:53 PM, Mario Six wrote:
>>>> To enable DM on MPC85xx, we need pre-relocation malloc, which is
>>>> implemented in this patch.
>>>>
>>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>>> Cc: York Sun <york.sun at nxp.com>
>>>> Cc: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>  arch/powerpc/cpu/mpc85xx/cpu_init_early.c |  8 --------
>>>>  arch/powerpc/cpu/mpc85xx/start.S          | 28  
>>>> ++++++++++++++++++++++++++++
>>>>  2 files changed, 28 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>>>> b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>>>> index 235a635..e6e1688 100644
>>>> --- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>>>> +++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>>>> @@ -82,7 +82,6 @@ void setup_ifc(void)
>>>>  void cpu_init_early_f(void *fdt)
>>>>  {
>>>>  	u32 mas0, mas1, mas2, mas3, mas7;
>>>> -	int i;
>>>>  #ifdef CONFIG_SYS_FSL_ERRATUM_P1010_A003549
>>>>  	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>>>>  #endif
>>>> @@ -95,13 +94,6 @@ void cpu_init_early_f(void *fdt)
>>>>  	/* Pointer is writable since we allocated a register for it */
>>>>  	gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
>>>>
>>>> -	/*
>>>> -	 * Clear initial global data
>>>> -	 *   we don't use memset so we can share this code with NAND_SPL
>>>> -	 */
>>>> -	for (i = 0; i < sizeof(gd_t); i++)
>>>> -		((char *)gd)[i] = 0;
>>>> -
>>>>  #ifdef CONFIG_QEMU_E500
>>>>  	/*
>>>>  	 * CONFIG_SYS_CCSRBAR_PHYS below may use gd->fdt_blob on ePAPR systems,
>>>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S
>>>> b/arch/powerpc/cpu/mpc85xx/start.S
>>>> index d867e2a..e6b5203 100644
>>>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>>>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>>>> @@ -1152,6 +1152,34 @@ _start_cont:
>>>>  	/* Setup the stack in initial RAM,could be L2-as-SRAM or L1 dcache*/
>>>>  	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
>>>> +	/* Leave 16+ byte for back chain termination and NULL return address */
>>>> +	subi	r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf)
>>>> +
>>>> +	/* End of RAM */
>>>> +	lis	r4,(CONFIG_SYS_INIT_RAM_ADDR)@h
>>>> +	ori	r4,r4,(CONFIG_SYS_INIT_RAM_SIZE)@l
>>>> +
>>>> +	li	r0,0
>>>> +
>>>> +1: 	subi 	r4,r4,4
>>>> +	stw 	r0,0(r4)
>>>> +	cmplw 	r4,r3
>>>> +	bne	1b
>>>> +
>>>> +	lis	r4,(CONFIG_SYS_INIT_RAM_ADDR)@h
>>>> +	ori	r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l
>>>> +
>>>> +	addi	r3,r3,16	/* Pre-relocation malloc area */
>>>> +	stw	r3,GD_MALLOC_BASE(r4)
>>>> +	subi	r3,r3,16
>>>> +
>>>> +	/* Fix issue with exception handler alignment */
>>>> +	nop
>>>> +	nop
>>>> +	nop
>>>
>>> Why do you need this? Does the code get too long and enters
>>> exception handler space?
>>>
>>
>> Those are the reason I sent the patch as RFC: There seems to be some kind of
>> alignment issue with the exception vectors. If the nops are not there, the
>> board crashes as soon as the timer_interrupt is raised for the first
>> time.  The
>> commit 96d2bb952bbf2e5a14f6ad668312cbce3cc4485a (powerpc/mpc85xx: Don't
>> relocate exception vectors), among other things, removed the explicit
>> alignment
>> of the vectors for E500. If you add those back in (and remove the nops), the
>> code works too. So maybe some kind of alignment for the vectors is
>> needed after
>> all?
>
> There is requirement for alignment. For e500 core, the interrupt  
> vector offset
> registers (IVORs) have lowest 4 bit cleared. So the vectors must be  
> aligned to
> 16 bytes. For legacy cores, the exception vectors are fixed. You have to make
> sure the vectors are exactly where they should be.
>
> I think you can use .align 4. Try it.
>

Tried it and works perfectly, thanks. Will include this in v2, too.

>>
>>>> +#endif
>>>>  	li	r0,0
>>>>  	stw	r0,0(r3)	/* Terminate Back Chain */
>>>>  	stw	r0,+4(r3)	/* NULL return address. */
>>>> --
>>>
>>> This patch presumes stack is right under GD, which is OK. But
>>> CONFIG_SYS_MALLOC_F_LEN has not been used by powerpc. Presumption of
>>> (CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE) <
>>> CONFIG_SYS_INIT_RAM_SIZE
>>> may not be true. Would it be better to check at compiling time to
>>> make sure we
>>> have enough init ram for the malloc len?
>>>
>>
>> Yes, good idea; I'll add an appropriate check in v2 of the patch. Maybe
>> something like
>>
>> #if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE + 0x80 >
>> CONFIG_SYS_INIT_RAM_SIZE
>> #error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM."
>> #endif
>>
>> to ensure we leave at least a minimum of ~128 byte stack space, too?
>>
>
> I don't think so. We know the size of GD, and we know the size of MALLOC_LEN.
> But we don't know the depth of stack until we compile it. I would  
> put GD on top,
> followed by MALLOC, and leave the rest to stack. I know the DDR driver we use
> needs way more than 128 bytes for stack.
>

OK, something like

#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."
#endif

then? The stack might end up awfully small, but if there are drivers that need
a lot of stack, we can't do a much better test anyway.

Also, from what I see, the GD is on top for all MPC85xx boards (except for
T4240QDS, apparently?), because they define either

#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_INIT_RAM_SIZE -  
GENERATED_GBL_DATA_SIZE)

or

#define CONFIG_SYS_GBL_DATA_OFFSET	(CONFIG_SYS_INIT_RAM_END -  
GENERATED_GBL_DATA_SIZE)

and then

#define	CONFIG_SYS_INIT_SP_OFFSET	CONFIG_SYS_GBL_DATA_OFFSET

So the memory layout is as you said: GD on top, followed by MALLOC, and the
rest for the stack.

Best regards,

Mario