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

[Pavel Machek]

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?

Sorry for the noise, this really is not all that important...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html