Thread (81 messages) 81 messages, 12 authors, 2012-06-05

Re: [UPDATED PATCH 21/27] sparc32, smpboot: Use generic SMP booting infrastructure

From: Srivatsa S. Bhat <hidden>
Date: 2012-06-02 15:59:36
Also in: lkml, sparclinux

On 06/02/2012 08:43 PM, Sam Ravnborg wrote:
quoted
quoted
I will try to dive into this and see if I on the sparc32 side
can make leon behave like the other platforms, and then 
unify some of the smp cpu boot stuff such that we then
can introduce the generalization from your patch.
This is what I came up with.
I modelled this over your patch-set to make it
simpler to convert to the generic scheme.

Thanks a lot for doing this :-)
I realised a bit more how CPU's are started up on sparc32.
So I think all of leon, sun4m and sun4d should work with this.

But my SUN box is defunct atm. so I have not tested it.
Comments appreciated!

PS. I used the ugly __names for now.
As soon as you have found a better set of names this should
be fixed here too.

As I mentioned in my other mail, I am thinking of changing them to
arch_cpu_pre_starting(), arch_cpu_pre_online() and arch_cpu_post_online().
Let me know what you think of those names.

Would you kindly add a changelog and your sign-off to this patch?
quoted hunk ↗ jump to hunk
diff --git a/arch/sparc/kernel/kernel.h b/arch/sparc/kernel/kernel.h
index 291bb5d..2bc1c6d 100644
--- a/arch/sparc/kernel/kernel.h
+++ b/arch/sparc/kernel/kernel.h
@@ -48,6 +48,10 @@ extern void sun4m_init_IRQ(void);
 extern void sun4m_unmask_profile_irq(void);
 extern void sun4m_clear_profile_irq(int cpu);

+/* sun4m_smp.c */
+void sun4m_cpu_pre_starting(void);
+void sun4m_cpu_pre_online(void);
+
 /* sun4d_irq.c */
 extern spinlock_t sun4d_imsk_lock;
@@ -60,6 +64,14 @@ extern int show_sun4d_interrupts(struct seq_file *, void *);
 extern void sun4d_distribute_irqs(void);
 extern void sun4d_free_irq(unsigned int irq, void *dev_id);

+/* sun4d_smp.c */
+void sun4d_cpu_pre_starting(void);
+void sun4d_cpu_pre_online(void);
+
+/* leon_smp.c */
+void leon_cpu_pre_starting(void);
+void leon_cpu_pre_online(void);
+
 /* head_32.S */
 extern unsigned int t_nmi[];
 extern unsigned int linux_trap_ipi15_sun4d[];
diff --git a/arch/sparc/kernel/leon_smp.c b/arch/sparc/kernel/leon_smp.c
index 0f3fb6d..0b028b3 100644
--- a/arch/sparc/kernel/leon_smp.c
+++ b/arch/sparc/kernel/leon_smp.c
@@ -69,31 +69,20 @@ static inline unsigned long do_swap(volatile unsigned long *ptr,
 	return val;
 }

-void __cpuinit leon_callin(void)
+void __cpuinit leon_cpu_pre_starting(void)
 {
-	int cpuid = hard_smp_processor_id();
-
-	local_ops->cache_all();
-	local_ops->tlb_all();
 	leon_configure_cache_smp();
+}

-	notify_cpu_starting(cpuid);
-
-	/* Get our local ticker going. */
-	register_percpu_ce(cpuid);
-
-	calibrate_delay();
-	smp_store_cpu_info(cpuid);

-	local_ops->cache_all();
-	local_ops->tlb_all();
+void __cpuinit cpu_cpu_pre_starting(void *unused)

Shouldn't this be leon_cpu_pre_online()? And the parameters
should be consistent too (here it expects a void*, but it is
called with no arguments...)
quoted hunk ↗ jump to hunk
+{
+	int cpuid = hard_smp_processor_id();

-	/*
-	 * Unblock the master CPU _only_ when the scheduler state
-	 * of all secondary CPUs will be up-to-date, so after
-	 * the SMP initialization the master will be just allowed
-	 * to call the scheduler code.
-	 * Allow master to continue.
+	/* Allow master to continue. The master will then give us the
+	 * go-ahead by setting the smp_commenced_mask and will wait without
+	 * timeouts until our setup is completed fully (signified by
+	 * our bit being set in the cpu_online_mask).
 	 */
 	do_swap(&cpu_callin_map[cpuid], 1);
@@ -110,9 +99,6 @@ void __cpuinit leon_callin(void)

 	while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
 		mb();
-
-	local_irq_enable();
-	set_cpu_online(cpuid, true);
 }

 /*
diff --git a/arch/sparc/kernel/smp_32.c b/arch/sparc/kernel/smp_32.c
index 79db45e..2e2990f 100644
--- a/arch/sparc/kernel/smp_32.c
+++ b/arch/sparc/kernel/smp_32.c
@@ -20,6 +20,7 @@
 #include <linux/seq_file.h>
 #include <linux/cache.h>
 #include <linux/delay.h>
+#include <linux/cpu.h>

 #include <asm/ptrace.h>
 #include <linux/atomic.h>
@@ -32,8 +33,10 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 #include <asm/cpudata.h>
+#include <asm/timer.h>
 #include <asm/leon.h>

+#include "kernel.h"
 #include "irq.h"

 volatile unsigned long cpu_callin_map[NR_CPUS] __cpuinitdata = {0,};
@@ -294,6 +297,89 @@ int __cpuinit __cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return ret;
 }

+void __cpuinit __cpu_pre_starting(void *unused)
+{
+	local_ops->cache_all();
+	local_ops->tlb_all();
+
+	switch(sparc_cpu_model) {
+	case sun4m:
+		sun4m_cpu_pre_starting();

Wouldn't it be better to have a provision to pass the
pointer down to these functions as well?
+		break;
+	case sun4d:
+		sun4d_cpu_pre_starting();
+		break;
+	case sparc_leon:
+		leon_cpu_pre_starting();
+		break;
+	default:
+		BUG();
+	}
+}
+
+void __cpuinit __cpu_pre_online(void *unused)
+{
+	unsigned int cpuid = hard_smp_processor_id();
+
+	register_percpu_ce(cpuid);
+
+	calibrate_delay();
+	smp_store_cpu_info(cpuid);
+
+	local_ops->cache_all();
+	local_ops->tlb_all();
+
+	switch(sparc_cpu_model) {
+	case sun4m:
+		sun4m_cpu_pre_online();
+		break;
+	case sun4d:
+		sun4d_cpu_pre_online();
+		break;
+	case sparc_leon:
+		leon_cpu_pre_starting();

Shouldn't this be leon_cpu_pre_online()?
+		break;
+	default:
+		BUG();
+	}
+}
+
+void __cpuinit sparc_start_secondary(void *unused)
+{
+	unsigned int cpu;
+
+	/*
+	 * SMP booting is extremely fragile in some architectures. So run
+	 * the cpu initialization code first before anything else.
+	 */
+	__cpu_pre_starting(NULL);
+

Why not pass the pointer to __cpu_pre_starting() as well as the rest of
the functions?
quoted hunk ↗ jump to hunk
+	preempt_disable();
+	cpu = smp_processor_id();
+
+	/* Invoke the CPU_STARTING notifier callbacks */
+	notify_cpu_starting(cpu);
+
+	__cpu_pre_online(NULL);
+
+	/* Set the CPU in the cpu_online_mask */
+	set_cpu_online(cpu, true);
+
+	/* Enable local interrupts now */
+	local_irq_enable();
+
+	wmb();
+	cpu_idle();
+
+	/* We should never reach here! */
+	BUG();
+}
+
+void __cpuinit smp_callin(void)
+{
+	sparc_start_secondary(NULL);
+}
+
 void smp_bogo(struct seq_file *m)
 {
 	int i;
diff --git a/arch/sparc/kernel/sun4d_smp.c b/arch/sparc/kernel/sun4d_smp.c
index ddaea31..bfbbc46 100644
--- a/arch/sparc/kernel/sun4d_smp.c
+++ b/arch/sparc/kernel/sun4d_smp.c
@@ -50,10 +50,9 @@ static inline void show_leds(int cpuid)
 			      "i" (ASI_M_CTL));
 }

-void __cpuinit smp4d_callin(void)
+void __cpuinit sun4d_cpu_pre_starting(void)
 {
 	int cpuid = hard_smp_processor_id();
-	unsigned long flags;

 	/* Show we are alive */
 	cpu_leds[cpuid] = 0x6;
@@ -61,24 +60,14 @@ void __cpuinit smp4d_callin(void)

 	/* Enable level15 interrupt, disable level14 interrupt for now */
 	cc_set_imsk((cc_get_imsk() & ~0x8000) | 0x4000);
+}

-	local_ops->cache_all();
-	local_ops->tlb_all();
-
-	notify_cpu_starting(cpuid);
-	/*
-	 * Unblock the master CPU _only_ when the scheduler state
-	 * of all secondary CPUs will be up-to-date, so after
-	 * the SMP initialization the master will be just allowed
-	 * to call the scheduler code.
-	 */
-	/* Get our local ticker going. */
-	register_percpu_ce(cpuid);
+void __cpuinit sun4d_cpu_pre_online(void)
+{
+	unsigned long flags;
+	int cpuid;

-	calibrate_delay();
-	smp_store_cpu_info(cpuid);
-	local_ops->cache_all();
-	local_ops->tlb_all();
+	cpuid = hard_smp_processor_id();

 	/* Allow master to continue. */
 	sun4d_swap((unsigned long *)&cpu_callin_map[cpuid], 1);
@@ -106,16 +95,12 @@ void __cpuinit smp4d_callin(void)
 	local_ops->cache_all();
 	local_ops->tlb_all();

-	local_irq_enable();	/* We don't allow PIL 14 yet */
-
 	while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
 		barrier();

 	spin_lock_irqsave(&sun4d_imsk_lock, flags);
 	cc_set_imsk(cc_get_imsk() & ~0x4000); /* Allow PIL 14 as well */
 	spin_unlock_irqrestore(&sun4d_imsk_lock, flags);
-	set_cpu_online(cpuid, true);
-
 }

 /*
diff --git a/arch/sparc/kernel/sun4m_smp.c b/arch/sparc/kernel/sun4m_smp.c
index 128af73..0cf6b4f 100644
--- a/arch/sparc/kernel/sun4m_smp.c
+++ b/arch/sparc/kernel/sun4m_smp.c
@@ -34,30 +34,19 @@ swap_ulong(volatile unsigned long *ptr, unsigned long val)
 	return val;
 }

-void __cpuinit smp4m_callin(void)
+void __cpuinit sun4m_cpu_pre_starting(void)
 {
-	int cpuid = hard_smp_processor_id();
-
-	local_ops->cache_all();
-	local_ops->tlb_all();
-
-	notify_cpu_starting(cpuid);
-
-	register_percpu_ce(cpuid);
-
-	calibrate_delay();
-	smp_store_cpu_info(cpuid);
+}

-	local_ops->cache_all();
-	local_ops->tlb_all();
+void __cpuinit sun4m_cpu_pre_online(void)
+{
+	int cpuid = hard_smp_processor_id();

-	/*
-	 * Unblock the master CPU _only_ when the scheduler state
-	 * of all secondary CPUs will be up-to-date, so after
-	 * the SMP initialization the master will be just allowed
-	 * to call the scheduler code.
+	/* Allow master to continue. The master will then give us the
+	 * go-ahead by setting the smp_commenced_mask and will wait without
+	 * timeouts until our setup is completed fully (signified by
+	 * our bit being set in the cpu_online_mask).
 	 */
-	/* Allow master to continue. */
 	swap_ulong(&cpu_callin_map[cpuid], 1);

 	/* XXX: What's up with all the flushes? */
@@ -75,10 +64,6 @@ void __cpuinit smp4m_callin(void)

 	while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
 		mb();
-
-	local_irq_enable();
-
-	set_cpu_online(cpuid, true);
 }

 /*
diff --git a/arch/sparc/kernel/trampoline_32.S b/arch/sparc/kernel/trampoline_32.S
index af27aca..6cdb08c 100644
--- a/arch/sparc/kernel/trampoline_32.S
+++ b/arch/sparc/kernel/trampoline_32.S
@@ -79,18 +79,15 @@ cpu3_startup:
 	 nop

 	/* Start this processor. */
-	call	smp4m_callin
+	call	smp_callin
 	 nop

-	b,a	smp_do_cpu_idle
+	b,a	smp_panic

 	.text
 	.align	4

-smp_do_cpu_idle:
-	call	cpu_idle
-	 mov	0, %o0
-
+smp_panic:
 	call	cpu_panic
 	 nop
@@ -144,10 +141,10 @@ sun4d_cpu_startup:
 	 nop

 	/* Start this processor. */
-	call	smp4d_callin
+	call	smp_callin
 	 nop

-	b,a	smp_do_cpu_idle
+	b,a	smp_panic

 	__CPUINIT
 	.align	4
@@ -201,7 +198,7 @@ leon_smp_cpu_startup:
 	 nop

 	/* Start this processor. */
-	call	leon_callin
+	call	smp_callin

I still didn't get how this solves the original problem of
not having sparc_cpu_model set to sparc_leon. You mentioned
that by the time we reach leon_smp_cpu_startup, that variable
is not set. Even inside leon_smp_cpu_startup, I don't immediately
see where it is set. Am I missing something?
 	 nop

-	b,a	smp_do_cpu_idle
+	b,a	smp_panic

Regards,
Srivatsa S. Bhat
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help