| Message ID | 20130202192409.GA17736@amd.pavel.ucw.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pavel, > -----Original Message----- > From: ZY - pavel > Sent: Saturday, February 02, 2013 1:24 PM > To: Dinh Nguyen > Cc: Russell King - ARM Linux; olof@lixom.net; linux-arm- > kernel@lists.infradead.org; arnd@arndb.de > Subject: Re: [PATCHv2 for soc 4/4] arm: socfpga: Add SMP support for > actual socfpga harware > > Hi! > > > > > > I actually thought about that... but could not think of non- > ugly way > > > > > of doing that. I hope dts will normally be "right" for any > production > > > > > system... > > > > > > > > I think a panic is better just for the reason that if someone is > > > > expecting SMP, but missed the warning message, and later finds > out that > > > > the secondary core never came up, it would save some debugging > time. > > > > > > > > Since I have to send out a v3 from the 1st patch anyways, let me > verify > > > > that I can get the early warning. > > > > > > The choice is between a panic() at a point where the only way to > find > > > out is to throw in printascii() or a working printk, and ending up > with > > > an unbootable kernel, vs continuing the boot and having an almost > > > working system which can be logged into and the messages viewed. > > > > > > If you have an application which relies on the second CPU coming > up, > > > why not have it verify that the second CPU came up (it's quite easy > > > to do - there's POSIX standard libc calls to get the number of > online > > > CPUs). > > > > 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@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. Dinh > for (i = 0; i < ncores; i++) > set_cpu_possible(i, true); > > diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach- > socfpga/socfpga.c > index 334c330..c3cd88b 100644 > --- a/arch/arm/mach-socfpga/socfpga.c > +++ b/arch/arm/mach-socfpga/socfpga.c > @@ -74,10 +74,9 @@ static void __init socfpga_sysmgr_init(void) > > np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr"); > > - if (of_property_read_u32(np, "cpu1-start-addr", (u32 *) > &cpu1start_addr)) { > - early_printk("Need cpu1-start-addr in device tree.\n"); > - panic("Need cpu1-start-addr in device tree.\n"); > - } > + if (of_property_read_u32(np, "cpu1-start-addr", (u32 *) > &cpu1start_addr)) > + printk(KERN_ALERT "Need cpu1-start-addr in device tree for > SMP operation.\n"); > + > sys_manager_base_addr = of_iomap(np, 0); > > np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr"); > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html Confidentiality Notice. This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.
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@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
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@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
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; + for (i = 0; i < ncores; i++) set_cpu_possible(i, true); diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c index 334c330..c3cd88b 100644 --- a/arch/arm/mach-socfpga/socfpga.c +++ b/arch/arm/mach-socfpga/socfpga.c @@ -74,10 +74,9 @@ static void __init socfpga_sysmgr_init(void) np = of_find_compatible_node(NULL, NULL, "altr,sys-mgr"); - if (of_property_read_u32(np, "cpu1-start-addr", (u32 *) &cpu1start_addr)) { - early_printk("Need cpu1-start-addr in device tree.\n"); - panic("Need cpu1-start-addr in device tree.\n"); - } + if (of_property_read_u32(np, "cpu1-start-addr", (u32 *) &cpu1start_addr)) + printk(KERN_ALERT "Need cpu1-start-addr in device tree for SMP operation.\n"); + sys_manager_base_addr = of_iomap(np, 0); np = of_find_compatible_node(NULL, NULL, "altr,rst-mgr");