[PATCHv2 for soc 4/4] arm: socfpga: Add SMP support for actual socfpga harware

[Dinh Nguyen]

Hi Pavel,

On Sun, 2013-02-03 at 19:36 +0100, ZY - pavel wrote:
> Hi!
> 
> > > > Point taken...thanks Russell.
> > > 
> > > Well, I don't think we normally check dtbs for validity with
> > > user-helpful error messages, but it is relatively easy in this case.
> > > 
> > > ---cut here---
> > > 
> > > Continue booting with second core disabled if cpu1-start-addr is not
> > > present in .dtb.
> > > 
> > > Signed-off-by: Pavel Machek <pavel at denx.de>
> > > 
> > > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-
> > > socfpga/platsmp.c
> > > index 81e0da0..90facdd 100644
> > > --- a/arch/arm/mach-socfpga/platsmp.c
> > > +++ b/arch/arm/mach-socfpga/platsmp.c
> > > @@ -82,6 +82,9 @@ static void __init socfpga_smp_init_cpus(void)
> > >  		ncores = 1;
> > >  	}
> > >  #endif
> > > +	if (!cpu1start_addr)
> > > +		ncores = 1;
> > > +
> > 
> > This will not work because of commit 5587164eea4aad88fcb79d9b21dc8f14fea598cd 
> > I sent out a V3 series of this patch, CPU1 will simply fail to come online if 
> > cpu1-start-addr is not defined.
> 
> Are you sure? As far as I can see you still need such a line in v3 of
> the patch:
> 
> @@ -72,6 +73,11 @@ void __init socfpga_sysmgr_init(void)
>         struct device_node *np;
> 
>         np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr");
> +       
> +       if (of_property_read_u32(np, "cpu1-start-addr",
> +                       (u32 *) &cpu1start_addr))
> +               pr_err("SMP: Need cpu1-start-addr in device tree.\n");
> +
>         sys_manager_base_addr = of_iomap(np, 0);
> 
>         np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");
> 
> ...so cpu1-start-addr is not there, cpu1start_addr is NULL but you
> continue booting.
> 
>  ENTRY(secondary_trampoline)
> -       movw    r0, #:lower16:CPU1_START_ADDR
> -       movt  r0, #:upper16:CPU1_START_ADDR
> +       movw    r2, #:lower16:cpu1start_addr
> +       movt  r2, #:upper16:cpu1start_addr
> +                       
> +       /* The socfpga VT cannot handle a 0xC0000000 page offset when
> loading
> +               the cpu1start_addr, we bit clear it. Tested on HW and
> VT. */
> +       bic     r2, r2, #0x40000000
> 
> +       ldr     r0, [r2]
>         ldr     r1, [r0]
>         bx      r1
> 
> ...and you'll dereference the NULL pointer here, no?

You're right, somehow my initial test did not catch this error. For v4,
I'm just going to wrap everything in sofpga_boot_secodardy in a 

if (cpu1start_addr){}


Dinh

> 
> Sorry for the noise, this really is not all that important...
> 									Pavel