[PATCH] ARM: formalize an IPI for CPU wake-ups

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

Many CPUs are implementing power saving states via the WFI mode.
In some cases, WFI must be entered in order to suspend, or even reset,
a CPU.

Whether a CPU needs to be awaken, brought out of WFI during secondary
boot, or to cancel a reset state before it is effective, the CPU must
receive an interrupt to exit its WFI state.

Let's formalize an IPI number for this purpose, and move over existing
users over to a convenience function for it.

This also has the side effect of removing the spurious "Unknown IPI
message 1" each time a CPU is awaken.

Signed-off-by: Nicolas Pitre <nico@linaro.org>

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index ae29293270..5b1ddd6ca5 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -90,6 +90,8 @@ extern void platform_cpu_die(unsigned int cpu);
 extern int platform_cpu_kill(unsigned int cpu);
 extern void platform_cpu_enable(unsigned int cpu);
 
+extern void arm_send_ping_ipi(int cpu);
+
 extern void arch_send_call_function_single_ipi(int cpu);
 extern void arch_send_call_function_ipi_mask(const struct cpumask *mask);
 
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 2c7217d971..f0a060651d 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -51,7 +51,8 @@
 struct secondary_data secondary_data;
 
 enum ipi_msg_type {
-	IPI_TIMER = 2,
+	IPI_PING = 1,
+	IPI_TIMER,
 	IPI_RESCHEDULE,
 	IPI_CALL_FUNC,
 	IPI_CALL_FUNC_SINGLE,
@@ -336,6 +337,11 @@ void __init set_smp_cross_call(void (*fn)(const struct cpumask *, unsigned int))
 	smp_cross_call = fn;
 }
 
+void arm_send_ping_ipi(int cpu)
+{
+	smp_cross_call(cpumask_of(cpu), IPI_PING);
+}
+
 void arch_send_call_function_ipi_mask(const struct cpumask *mask)
 {
 	smp_cross_call(mask, IPI_CALL_FUNC);
@@ -504,6 +510,9 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 		__inc_irq_stat(cpu, ipi_irqs[ipinr - IPI_TIMER]);
 
 	switch (ipinr) {
+	case IPI_PING:
+		break;
+
 	case IPI_TIMER:
 		irq_enter();
 		ipi_timer();
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 36c3984aaa..444001c9aa 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -139,7 +139,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 		__raw_writel(virt_to_phys(exynos4_secondary_startup),
 			CPU1_BOOT_REG);
-		gic_raise_softirq(cpumask_of(cpu), 1);
+		arm_send_ping_ipi(cpu);
 
 		if (pen_release == -1)
 			break;
diff --git a/arch/arm/mach-msm/platsmp.c b/arch/arm/mach-msm/platsmp.c
index db0117ec55..9be8bed59f 100644
--- a/arch/arm/mach-msm/platsmp.c
+++ b/arch/arm/mach-msm/platsmp.c
@@ -127,7 +127,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * the boot monitor to read the system wide flags register,
 	 * and branch to the address found there.
 	 */
-	gic_raise_softirq(cpumask_of(cpu), 1);
+	arm_send_ping_ipi(cpu);
 
 	timeout = jiffies + (1 * HZ);
 	while (time_before(jiffies, timeout)) {
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
index deffbf1c96..604051c169 100644
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -111,7 +111,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 		booted = true;
 	}
 
-	gic_raise_softirq(cpumask_of(cpu), 1);
+	arm_send_ping_ipi(cpu);
 
 	/*
 	 * Now the secondary core is starting up let it run its
diff --git a/arch/arm/mach-shmobile/smp-emev2.c b/arch/arm/mach-shmobile/smp-emev2.c
index 6a35c4a31e..9e55c54b90 100644
--- a/arch/arm/mach-shmobile/smp-emev2.c
+++ b/arch/arm/mach-shmobile/smp-emev2.c
@@ -82,7 +82,7 @@ int __cpuinit emev2_boot_secondary(unsigned int cpu)
 	/* Tell ROM loader about our vector (in headsmp.S) */
 	emev2_set_boot_vector(__pa(shmobile_secondary_vector));
 
-	gic_raise_softirq(cpumask_of(cpu), 1);
+	arm_send_ping_ipi(cpu);
 	return 0;
 }
 
diff --git a/arch/arm/plat-versatile/platsmp.c b/arch/arm/plat-versatile/platsmp.c
index 49c7db48c7..53864bd0d0 100644
--- a/arch/arm/plat-versatile/platsmp.c
+++ b/arch/arm/plat-versatile/platsmp.c
@@ -85,7 +85,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * the boot monitor to read the system wide flags register,
 	 * and branch to the address found there.
 	 */
-	gic_raise_softirq(cpumask_of(cpu), 1);
+	arm_send_ping_ipi(cpu);
 
 	timeout = jiffies + (1 * HZ);
 	while (time_before(jiffies, timeout)) {

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 11:54:03AM -0400, Nicolas Pitre wrote:
> 
> Many CPUs are implementing power saving states via the WFI mode.
> In some cases, WFI must be entered in order to suspend, or even reset,
> a CPU.
> 
> Whether a CPU needs to be awaken, brought out of WFI during secondary
> boot, or to cancel a reset state before it is effective, the CPU must
> receive an interrupt to exit its WFI state.
> 
> Let's formalize an IPI number for this purpose, and move over existing
> users over to a convenience function for it.
> 
> This also has the side effect of removing the spurious "Unknown IPI
> message 1" each time a CPU is awaken.

I want people to move over to using IPI 0 for wakeup.

The problem is, as ever, no one bloody well listens and they all continue
on copying Realview.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:

> On Mon, Jun 11, 2012 at 11:54:03AM -0400, Nicolas Pitre wrote:
> > 
> > Many CPUs are implementing power saving states via the WFI mode.
> > In some cases, WFI must be entered in order to suspend, or even reset,
> > a CPU.
> > 
> > Whether a CPU needs to be awaken, brought out of WFI during secondary
> > boot, or to cancel a reset state before it is effective, the CPU must
> > receive an interrupt to exit its WFI state.
> > 
> > Let's formalize an IPI number for this purpose, and move over existing
> > users over to a convenience function for it.
> > 
> > This also has the side effect of removing the spurious "Unknown IPI
> > message 1" each time a CPU is awaken.
> 
> I want people to move over to using IPI 0 for wakeup.
> 
> The problem is, as ever, no one bloody well listens and they all continue
> on copying Realview.

No problem -- with this patch the actual IPI number is abstracted away 
and we can modify it as we see fit in a single location.

I can provide a follow-up patch doing just that.  What do you think?


Nicolas

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 12:44:34PM -0400, Nicolas Pitre wrote:
> On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> 
> > On Mon, Jun 11, 2012 at 11:54:03AM -0400, Nicolas Pitre wrote:
> > > 
> > > Many CPUs are implementing power saving states via the WFI mode.
> > > In some cases, WFI must be entered in order to suspend, or even reset,
> > > a CPU.
> > > 
> > > Whether a CPU needs to be awaken, brought out of WFI during secondary
> > > boot, or to cancel a reset state before it is effective, the CPU must
> > > receive an interrupt to exit its WFI state.
> > > 
> > > Let's formalize an IPI number for this purpose, and move over existing
> > > users over to a convenience function for it.
> > > 
> > > This also has the side effect of removing the spurious "Unknown IPI
> > > message 1" each time a CPU is awaken.
> > 
> > I want people to move over to using IPI 0 for wakeup.
> > 
> > The problem is, as ever, no one bloody well listens and they all continue
> > on copying Realview.
> 
> No problem -- with this patch the actual IPI number is abstracted away 
> and we can modify it as we see fit in a single location.

No.  The reason I haven't simply been through the code and done a forceful
replace of '1' to '0' is because we don't know how the platforms themselves
will react, and whether any have their boot protocol expecting SGI1.

I've explained all this before, whenever the whinge about the "Unknown IPI"
message comes up.  Unfortunately, no one has yet listened to what I've said
and done anything to test whether SGI0 works.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:

> On Mon, Jun 11, 2012 at 12:44:34PM -0400, Nicolas Pitre wrote:
> > On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> > 
> > > On Mon, Jun 11, 2012 at 11:54:03AM -0400, Nicolas Pitre wrote:
> > > > 
> > > > Many CPUs are implementing power saving states via the WFI mode.
> > > > In some cases, WFI must be entered in order to suspend, or even reset,
> > > > a CPU.
> > > > 
> > > > Whether a CPU needs to be awaken, brought out of WFI during secondary
> > > > boot, or to cancel a reset state before it is effective, the CPU must
> > > > receive an interrupt to exit its WFI state.
> > > > 
> > > > Let's formalize an IPI number for this purpose, and move over existing
> > > > users over to a convenience function for it.
> > > > 
> > > > This also has the side effect of removing the spurious "Unknown IPI
> > > > message 1" each time a CPU is awaken.
> > > 
> > > I want people to move over to using IPI 0 for wakeup.
> > > 
> > > The problem is, as ever, no one bloody well listens and they all continue
> > > on copying Realview.
> > 
> > No problem -- with this patch the actual IPI number is abstracted away 
> > and we can modify it as we see fit in a single location.
> 
> No.  The reason I haven't simply been through the code and done a forceful
> replace of '1' to '0' is because we don't know how the platforms themselves
> will react, and whether any have their boot protocol expecting SGI1.

I'm all in favor of having people find this out the hard way.  As you 
know I'm sure, pushing out such changes and see if anyone complains is 
often the only way to go.  And I'll be glad to have those complaints 
sent my way if I'm credited for this.

> I've explained all this before, whenever the whinge about the "Unknown IPI"
> message comes up.  Unfortunately, no one has yet listened to what I've said
> and done anything to test whether SGI0 works.

It certainly works for all the targets I'm looking at right now.  In 
those cases the boot protocol couldn't care less about the IPI number as 
the point is only to kick the target CPU out of WFI.

And that message certainly has to go as it may be triggered multiple 
times per second in some use cases here.


Nicolas

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 06/11/12 10:18, Nicolas Pitre wrote:
>
> It certainly works for all the targets I'm looking at right now.  In 
> those cases the boot protocol couldn't care less about the IPI number as 
> the point is only to kick the target CPU out of WFI.
>
> And that message certainly has to go as it may be triggered multiple 
> times per second in some use cases here.
>
>

Last time this came up I thought we covered most of the platforms?

OMAP, Realview, Versatile Express, and MSM are covered by this thread

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/039275.html

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 10:41:15AM -0700, Stephen Boyd wrote:
> On 06/11/12 10:18, Nicolas Pitre wrote:
> >
> > It certainly works for all the targets I'm looking at right now.  In 
> > those cases the boot protocol couldn't care less about the IPI number as 
> > the point is only to kick the target CPU out of WFI.
> >
> > And that message certainly has to go as it may be triggered multiple 
> > times per second in some use cases here.
> >
> >
> 
> Last time this came up I thought we covered most of the platforms?
> 
> OMAP, Realview, Versatile Express, and MSM are covered by this thread
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/039275.html

Ok, in that case I've just committed three patches to make those platforms
use SGI0 instead of SGI1.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 06/11/12 12:25, Russell King - ARM Linux wrote:
> On Mon, Jun 11, 2012 at 10:41:15AM -0700, Stephen Boyd wrote:
>> On 06/11/12 10:18, Nicolas Pitre wrote:
>>> It certainly works for all the targets I'm looking at right now.  In 
>>> those cases the boot protocol couldn't care less about the IPI number as 
>>> the point is only to kick the target CPU out of WFI.
>>>
>>> And that message certainly has to go as it may be triggered multiple 
>>> times per second in some use cases here.
>>>
>>>
>> Last time this came up I thought we covered most of the platforms?
>>
>> OMAP, Realview, Versatile Express, and MSM are covered by this thread
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/039275.html
> Ok, in that case I've just committed three patches to make those platforms
> use SGI0 instead of SGI1.

Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the only
one left to move to SGI0.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Kukjin Kim]

Stephen Boyd wrote:
> 
> On 06/11/12 12:25, Russell King - ARM Linux wrote:
> > On Mon, Jun 11, 2012 at 10:41:15AM -0700, Stephen Boyd wrote:
> >> On 06/11/12 10:18, Nicolas Pitre wrote:
> >>> It certainly works for all the targets I'm looking at right now.  In
> >>> those cases the boot protocol couldn't care less about the IPI number
> as
> >>> the point is only to kick the target CPU out of WFI.
> >>>
> >>> And that message certainly has to go as it may be triggered multiple
> >>> times per second in some use cases here.
> >>>
> >>>
> >> Last time this came up I thought we covered most of the platforms?
> >>
> >> OMAP, Realview, Versatile Express, and MSM are covered by this thread
> >>
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-
> January/039275.html
> > Ok, in that case I've just committed three patches to make those
> platforms
> > use SGI0 instead of SGI1.
> 
> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the only
> one left to move to SGI0.
> 
Yeah, EXYNOS can use SGI0 instead of SGI1 :)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 07/10/12 23:34, Kukjin Kim wrote:
> Stephen Boyd wrote:
>> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the only
>> one left to move to SGI0.
>>
> Yeah, EXYNOS can use SGI0 instead of SGI1 :)
>
>

Russell, can we apply something like this?

diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 36c3984..090e32b 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -139,7 +139,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 		__raw_writel(virt_to_phys(exynos4_secondary_startup),
 			CPU1_BOOT_REG);
-		gic_raise_softirq(cpumask_of(cpu), 1);
+		gic_raise_softirq(cpumask_of(cpu), 0);
 
 		if (pen_release == -1)
 			break;


I see we have another user of SGI1. Magnus/Rafael, can we move
smp-emev2.c to use SGI0 instead of SGI1?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Kukjin Kim]

Stephen Boyd wrote:
> 
> On 07/10/12 23:34, Kukjin Kim wrote:
> > Stephen Boyd wrote:
> >> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the
> only
> >> one left to move to SGI0.
> >>
> > Yeah, EXYNOS can use SGI0 instead of SGI1 :)
> >
> >
> 
> Russell, can we apply something like this?
> 
Please feel free to add my ack on following change.

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-
> exynos/platsmp.c
> index 36c3984..090e32b 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -139,7 +139,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct
> task_struct *idle)
> 
>  		__raw_writel(virt_to_phys(exynos4_secondary_startup),
>  			CPU1_BOOT_REG);
> -		gic_raise_softirq(cpumask_of(cpu), 1);
> +		gic_raise_softirq(cpumask_of(cpu), 0);
> 
>  		if (pen_release == -1)
>  			break;
> 
> 
> I see we have another user of SGI1. Magnus/Rafael, can we move
> smp-emev2.c to use SGI0 instead of SGI1?
> 
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Colin Cross]

On Wed, Aug 1, 2012 at 2:42 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Stephen Boyd wrote:
>>
>> On 07/10/12 23:34, Kukjin Kim wrote:
>> > Stephen Boyd wrote:
>> >> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the
>> only
>> >> one left to move to SGI0.
>> >>
>> > Yeah, EXYNOS can use SGI0 instead of SGI1 :)
>> >
>> >
>>
>> Russell, can we apply something like this?
>>
> Please feel free to add my ack on following change.
>
> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>

When testing coupled cpuidle on Exynos5, I found that sending an IPI
does not successfully wake up CPU1.  CPU1 is in a wfe with interrupts
masked, not a wfi, so an interrupt is not able to wake it up.  It
tends to wake up anyways because the next time a spin lock is
unlocked, often during a timer interrupt on CPU0, CPU0 executes an sev
which wakes up CPU1.  You can see that the IPI is unnecessary by
removing the gic_raise_softirq and replacing it with dsb_sev().

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Aug 06, 2012 at 01:41:06PM -0700, Colin Cross wrote:
> When testing coupled cpuidle on Exynos5, I found that sending an IPI
> does not successfully wake up CPU1.  CPU1 is in a wfe with interrupts
> masked, not a wfi, so an interrupt is not able to wake it up.  It
> tends to wake up anyways because the next time a spin lock is
> unlocked, often during a timer interrupt on CPU0, CPU0 executes an sev
> which wakes up CPU1.  You can see that the IPI is unnecessary by
> removing the gic_raise_softirq and replacing it with dsb_sev().

That's good news; that supports my assertion that the SMP bringup code
we see on various platforms as copies from the Realview code was done
without much thought about what is actually required.

It's good that you're looking into this, and fixing the code on the CPUs
you have access to.  I rather wish we'd made it a requirement for people
to _document_ the actual bringup conditions in the kernel for their
hardware before we accepted their SMP support code.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Shilimkar, Santosh]

On Tue, Aug 7, 2012 at 10:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Aug 06, 2012 at 01:41:06PM -0700, Colin Cross wrote:
>> When testing coupled cpuidle on Exynos5, I found that sending an IPI
>> does not successfully wake up CPU1.  CPU1 is in a wfe with interrupts
>> masked, not a wfi, so an interrupt is not able to wake it up.  It
>> tends to wake up anyways because the next time a spin lock is
>> unlocked, often during a timer interrupt on CPU0, CPU0 executes an sev
>> which wakes up CPU1.  You can see that the IPI is unnecessary by
>> removing the gic_raise_softirq and replacing it with dsb_sev().
>
> That's good news; that supports my assertion that the SMP bringup code
> we see on various platforms as copies from the Realview code was done
> without much thought about what is actually required.
>
> It's good that you're looking into this, and fixing the code on the CPUs
> you have access to.  I rather wish we'd made it a requirement for people
> to _document_ the actual bringup conditions in the kernel for their
> hardware before we accepted their SMP support code.
>
Movement to SGI0 for all ARM platform was also discussed when I
was trying to fix the "Unknown IPI message 0x1" warning with patch [1]

For OMAP too, the SGI0 works.

Regards
Santosh

[1 ] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/038430.html

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Kukjin Kim]

Colin Cross wrote:
> 
> On Wed, Aug 1, 2012 at 2:42 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Stephen Boyd wrote:
> >>
> >> On 07/10/12 23:34, Kukjin Kim wrote:
> >> > Stephen Boyd wrote:
> >> >> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the
> >> only
> >> >> one left to move to SGI0.
> >> >>
> >> > Yeah, EXYNOS can use SGI0 instead of SGI1 :)
> >> >
> >> >
> >>
> >> Russell, can we apply something like this?
> >>
> > Please feel free to add my ack on following change.
> >
> > Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> >
> 
> When testing coupled cpuidle on Exynos5, I found that sending an IPI
> does not successfully wake up CPU1.  CPU1 is in a wfe with interrupts
> masked, not a wfi, so an interrupt is not able to wake it up.  It
> tends to wake up anyways because the next time a spin lock is
> unlocked, often during a timer interrupt on CPU0, CPU0 executes an sev
> which wakes up CPU1.  You can see that the IPI is unnecessary by
> removing the gic_raise_softirq and replacing it with dsb_sev().

Hmm...could be. Let me check you commented in detail. And if any updates,
I'll let you know.

Thanks for your information.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Magnus Damm]

On Tue, Jul 31, 2012 at 3:19 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/10/12 23:34, Kukjin Kim wrote:
>> Stephen Boyd wrote:
>>> Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the only
>>> one left to move to SGI0.
>>>
>> Yeah, EXYNOS can use SGI0 instead of SGI1 :)
>>
>>
>
> Russell, can we apply something like this?
>
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 36c3984..090e32b 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -139,7 +139,7 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
>
>                 __raw_writel(virt_to_phys(exynos4_secondary_startup),
>                         CPU1_BOOT_REG);
> -               gic_raise_softirq(cpumask_of(cpu), 1);
> +               gic_raise_softirq(cpumask_of(cpu), 0);
>
>                 if (pen_release == -1)
>                         break;
>
>
> I see we have another user of SGI1. Magnus/Rafael, can we move
> smp-emev2.c to use SGI0 instead of SGI1?

So I finally managed to try this out on my KZM9D board. The SMP bring
up code continues to work as expected, so it seems that SGI0 can be
used instead of SGI1 on EMEV2. Let's just say that the boot protocol
implemented by the mask ROM is rather poorly documented. Anyway, to
test I used the following patch on top of linux git 42a579a.

--- 0001/arch/arm/mach-shmobile/smp-emev2.c
+++ work/arch/arm/mach-shmobile/smp-emev2.c	2012-08-08 15:42:54.000000000 +0900
@@ -82,7 +82,7 @@ int __cpuinit emev2_boot_secondary(unsig
 	/* Tell ROM loader about our vector (in headsmp.S) */
 	emev2_set_boot_vector(__pa(shmobile_secondary_vector));

-	gic_raise_softirq(cpumask_of(cpu), 1);
+	gic_raise_softirq(cpumask_of(cpu), 0);
 	return 0;
 }

So if/where needed, please add my:

Acked-by: Magnus Damm <damm@opensource.se>

Thanks,

/ magnus

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 08/07/12 23:52, Magnus Damm wrote:
> So I finally managed to try this out on my KZM9D board. The SMP bring
> up code continues to work as expected, so it seems that SGI0 can be
> used instead of SGI1 on EMEV2. Let's just say that the boot protocol
> implemented by the mask ROM is rather poorly documented. Anyway, to
> test I used the following patch on top of linux git 42a579a.
>
> --- 0001/arch/arm/mach-shmobile/smp-emev2.c
> +++ work/arch/arm/mach-shmobile/smp-emev2.c	2012-08-08 15:42:54.000000000 +0900
> @@ -82,7 +82,7 @@ int __cpuinit emev2_boot_secondary(unsig
>  	/* Tell ROM loader about our vector (in headsmp.S) */
>  	emev2_set_boot_vector(__pa(shmobile_secondary_vector));
>
> -	gic_raise_softirq(cpumask_of(cpu), 1);
> +	gic_raise_softirq(cpumask_of(cpu), 0);
>  	return 0;
>  }
>
> So if/where needed, please add my:
>
> Acked-by: Magnus Damm <damm@opensource.se>
>

Thanks Magnus, would you be able to merge this through the shmobile
tree? Otherwise I can put it in the patch tracker.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 08/13/12 15:09, Stephen Boyd wrote:
> On 08/07/12 23:52, Magnus Damm wrote:
>> So I finally managed to try this out on my KZM9D board. The SMP bring
>> up code continues to work as expected, so it seems that SGI0 can be
>> used instead of SGI1 on EMEV2. Let's just say that the boot protocol
>> implemented by the mask ROM is rather poorly documented. Anyway, to
>> test I used the following patch on top of linux git 42a579a.
>>
>> --- 0001/arch/arm/mach-shmobile/smp-emev2.c
>> +++ work/arch/arm/mach-shmobile/smp-emev2.c	2012-08-08 15:42:54.000000000 +0900
>> @@ -82,7 +82,7 @@ int __cpuinit emev2_boot_secondary(unsig
>>  	/* Tell ROM loader about our vector (in headsmp.S) */
>>  	emev2_set_boot_vector(__pa(shmobile_secondary_vector));
>>
>> -	gic_raise_softirq(cpumask_of(cpu), 1);
>> +	gic_raise_softirq(cpumask_of(cpu), 0);
>>  	return 0;
>>  }
>>
>> So if/where needed, please add my:
>>
>> Acked-by: Magnus Damm <damm@opensource.se>
>>
> Thanks Magnus, would you be able to merge this through the shmobile
> tree? Otherwise I can put it in the patch tracker.
>

Ok, I've put it in the patch tracker.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Magnus Damm]

On Thu, Aug 23, 2012 at 4:05 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/13/12 15:09, Stephen Boyd wrote:
>> On 08/07/12 23:52, Magnus Damm wrote:
>>> So I finally managed to try this out on my KZM9D board. The SMP bring
>>> up code continues to work as expected, so it seems that SGI0 can be
>>> used instead of SGI1 on EMEV2. Let's just say that the boot protocol
>>> implemented by the mask ROM is rather poorly documented. Anyway, to
>>> test I used the following patch on top of linux git 42a579a.
>>>
>>> --- 0001/arch/arm/mach-shmobile/smp-emev2.c
>>> +++ work/arch/arm/mach-shmobile/smp-emev2.c  2012-08-08 15:42:54.000000000 +0900
>>> @@ -82,7 +82,7 @@ int __cpuinit emev2_boot_secondary(unsig
>>>      /* Tell ROM loader about our vector (in headsmp.S) */
>>>      emev2_set_boot_vector(__pa(shmobile_secondary_vector));
>>>
>>> -    gic_raise_softirq(cpumask_of(cpu), 1);
>>> +    gic_raise_softirq(cpumask_of(cpu), 0);
>>>      return 0;
>>>  }
>>>
>>> So if/where needed, please add my:
>>>
>>> Acked-by: Magnus Damm <damm@opensource.se>
>>>
>> Thanks Magnus, would you be able to merge this through the shmobile
>> tree? Otherwise I can put it in the patch tracker.
>>
>
> Ok, I've put it in the patch tracker.

Oh, thanks a lot! We're currently in the middle of rearranging our
kernel repository handling so things move a bit slower than usual...

Thanks for your help!

/ magnus

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Stephen Boyd]

On 8/22/2012 12:05 PM, Stephen Boyd wrote:
> Ok, I've put it in the patch tracker.
>

Russell, what's involved in getting rid of the printk now? Can we just
shift everything down by 1 and then formalize 0 as the wakeup ipi? Or do
you want to completely remove that ipi from the stats? It's not clear to
me what we should do. Something like the following untested patch would
be a possibility.

----8<------->8------

diff --git a/arch/arm/include/asm/hardirq.h b/arch/arm/include/asm/hardirq.h
index 436e60b..2740c2a2 100644
--- a/arch/arm/include/asm/hardirq.h
+++ b/arch/arm/include/asm/hardirq.h
@@ -5,7 +5,7 @@
 #include <linux/threads.h>
 #include <asm/irq.h>

-#define NR_IPI 5
+#define NR_IPI 6

 typedef struct {
        unsigned int __softirq_pending;
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index ebd8ad2..d98c37e 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -51,7 +51,8 @@
 struct secondary_data secondary_data;

 enum ipi_msg_type {
-       IPI_TIMER = 2,
+       IPI_WAKEUP,
+       IPI_TIMER,
        IPI_RESCHEDULE,
        IPI_CALL_FUNC,
        IPI_CALL_FUNC_SINGLE,
@@ -347,7 +348,8 @@ void arch_send_call_function_single_ipi(int cpu)
 }

 static const char *ipi_types[NR_IPI] = {
-#define S(x,s) [x - IPI_TIMER] = s
+#define S(x,s) [x] = s
+       S(IPI_WAKEUP, "CPU wakeup interrupts"),
        S(IPI_TIMER, "Timer broadcast interrupts"),
        S(IPI_RESCHEDULE, "Rescheduling interrupts"),
        S(IPI_CALL_FUNC, "Function call interrupts"),
@@ -500,10 +502,13 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
        unsigned int cpu = smp_processor_id();
        struct pt_regs *old_regs = set_irq_regs(regs);

-       if (ipinr >= IPI_TIMER && ipinr < IPI_TIMER + NR_IPI)
-               __inc_irq_stat(cpu, ipi_irqs[ipinr - IPI_TIMER]);
+       if (ipinr < NR_IPI)
+               __inc_irq_stat(cpu, ipi_irqs[ipinr]);

        switch (ipinr) {
+       case IPI_WAKEUP:
+               break;
+
        case IPI_TIMER:
                irq_enter();
                ipi_timer();

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Wed, 29 Aug 2012, Stephen Boyd wrote:

> 
> On 8/22/2012 12:05 PM, Stephen Boyd wrote:
> > Ok, I've put it in the patch tracker.
> >
> 
> Russell, what's involved in getting rid of the printk now? Can we just
> shift everything down by 1 and then formalize 0 as the wakeup ipi? Or do
> you want to completely remove that ipi from the stats? It's not clear to
> me what we should do. Something like the following untested patch would
> be a possibility.

Please make this into a proper patch and send it to the patch system 
with...

Reviewed-by: Nicolas Pitre <nico@linaro.org>

I do need this and would prefer to use a version likely to hit mainline 
rather than a local version.  And I don't think we should remove IPI 0 
from the stats.

> ----8<------->8------
> 
> diff --git a/arch/arm/include/asm/hardirq.h b/arch/arm/include/asm/hardirq.h
> index 436e60b..2740c2a2 100644
> --- a/arch/arm/include/asm/hardirq.h
> +++ b/arch/arm/include/asm/hardirq.h
> @@ -5,7 +5,7 @@
>  #include <linux/threads.h>
>  #include <asm/irq.h>
> 
> -#define NR_IPI 5
> +#define NR_IPI 6
> 
>  typedef struct {
>         unsigned int __softirq_pending;
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index ebd8ad2..d98c37e 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -51,7 +51,8 @@
>  struct secondary_data secondary_data;
> 
>  enum ipi_msg_type {
> -       IPI_TIMER = 2,
> +       IPI_WAKEUP,
> +       IPI_TIMER,
>         IPI_RESCHEDULE,
>         IPI_CALL_FUNC,
>         IPI_CALL_FUNC_SINGLE,
> @@ -347,7 +348,8 @@ void arch_send_call_function_single_ipi(int cpu)
>  }
> 
>  static const char *ipi_types[NR_IPI] = {
> -#define S(x,s) [x - IPI_TIMER] = s
> +#define S(x,s) [x] = s
> +       S(IPI_WAKEUP, "CPU wakeup interrupts"),
>         S(IPI_TIMER, "Timer broadcast interrupts"),
>         S(IPI_RESCHEDULE, "Rescheduling interrupts"),
>         S(IPI_CALL_FUNC, "Function call interrupts"),
> @@ -500,10 +502,13 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
>         unsigned int cpu = smp_processor_id();
>         struct pt_regs *old_regs = set_irq_regs(regs);
> 
> -       if (ipinr >= IPI_TIMER && ipinr < IPI_TIMER + NR_IPI)
> -               __inc_irq_stat(cpu, ipi_irqs[ipinr - IPI_TIMER]);
> +       if (ipinr < NR_IPI)
> +               __inc_irq_stat(cpu, ipi_irqs[ipinr]);
> 
>         switch (ipinr) {
> +       case IPI_WAKEUP:
> +               break;
> +
>         case IPI_TIMER:
>                 irq_enter();
>                 ipi_timer();
> 
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> 

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 01:18:28PM -0400, Nicolas Pitre wrote:
> And that message certainly has to go as it may be triggered multiple 
> times per second in some use cases here.

That, I interpret, as an attempt to use CPU hotplugging for power
saving.

No.  Do not do that.  Under any circumstances.  Not only is the CPU
hotplugging code racy in places, it also totally destroys the ability
to have threads bound to CPUs, or even interrupts to CPUs.

It's also racy with userspace.  Think of an application which wants to
know how many CPUs are online to create a number of threads, then you
take a CPU offline.  There's no way for it to be notified about that.
Or, conversely you bring a CPU online.  Again, no way for userspace to
be properly notified.

Then there's issues with reading /proc/interrupts while CPUs come and
go, especially when you have large numbers of interrupts which have to
be read across multiple reads.

Then there's all the driver model events sent into userspace when the
sysfs nodes for CPUs come and go.

So no, I'd point out that the message you get is the _least_ of your
problems if you're playing this idiotic game.  Don't do it.  Use CPU
idle instead.  That's what it's there for.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:

> On Mon, Jun 11, 2012 at 01:18:28PM -0400, Nicolas Pitre wrote:
> > And that message certainly has to go as it may be triggered multiple 
> > times per second in some use cases here.
> 
> That, I interpret, as an attempt to use CPU hotplugging for power
> saving.
> 
> No.  Do not do that.  Under any circumstances.  Not only is the CPU
> hotplugging code racy in places, it also totally destroys the ability
> to have threads bound to CPUs, or even interrupts to CPUs.

[ more comments ommitted ]

Russell, let me suggest you take the time to read this:

	http://lwn.net/Articles/481055/

then let's restart this conversation if you please.


Nicolas

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 02:40:24PM -0400, Nicolas Pitre wrote:
> On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> 
> > On Mon, Jun 11, 2012 at 01:18:28PM -0400, Nicolas Pitre wrote:
> > > And that message certainly has to go as it may be triggered multiple 
> > > times per second in some use cases here.
> > 
> > That, I interpret, as an attempt to use CPU hotplugging for power
> > saving.
> > 
> > No.  Do not do that.  Under any circumstances.  Not only is the CPU
> > hotplugging code racy in places, it also totally destroys the ability
> > to have threads bound to CPUs, or even interrupts to CPUs.
> 
> [ more comments ommitted ]
> 
> Russell, let me suggest you take the time to read this:
> 
> 	http://lwn.net/Articles/481055/
> 
> then let's restart this conversation if you please.

Same comments apply for exactly the reasons I stated.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:

> On Mon, Jun 11, 2012 at 02:40:24PM -0400, Nicolas Pitre wrote:
> > On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> > 
> > > On Mon, Jun 11, 2012 at 01:18:28PM -0400, Nicolas Pitre wrote:
> > > > And that message certainly has to go as it may be triggered multiple 
> > > > times per second in some use cases here.
> > > 
> > > That, I interpret, as an attempt to use CPU hotplugging for power
> > > saving.
> > > 
> > > No.  Do not do that.  Under any circumstances.  Not only is the CPU
> > > hotplugging code racy in places, it also totally destroys the ability
> > > to have threads bound to CPUs, or even interrupts to CPUs.
> > 
> > [ more comments ommitted ]
> > 
> > Russell, let me suggest you take the time to read this:
> > 
> > 	http://lwn.net/Articles/481055/
> > 
> > then let's restart this conversation if you please.
> 
> Same comments apply for exactly the reasons I stated.

No they don't.  They can't. The generic kernel code doesn't know what is 
happening behind the scene and may not even care if at the low level a 
complete CPU replacement happened.  Trust me, I happen to know one thing 
or two about those issues you mentioned and the code I'm working on will 
be posted in due course.

But that is not the point of this thread.  The point I want to bring 
forward is:

- IPIs are needed to wake up remote CPUs

- we should formalize a number for it and the actual number is the least 
  of my concern

- the warning must go

- and we shouldn't wait until the entire world comes forth to confirm 
  any arbitrary SGI number works for them or nothing will ever happen.

Can we make some progress with this please?


Nicolas

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 03:11:05PM -0400, Nicolas Pitre wrote:
> On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> 
> > On Mon, Jun 11, 2012 at 02:40:24PM -0400, Nicolas Pitre wrote:
> > > On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> > > 
> > > > On Mon, Jun 11, 2012 at 01:18:28PM -0400, Nicolas Pitre wrote:
> > > > > And that message certainly has to go as it may be triggered multiple 
> > > > > times per second in some use cases here.
> > > > 
> > > > That, I interpret, as an attempt to use CPU hotplugging for power
> > > > saving.
> > > > 
> > > > No.  Do not do that.  Under any circumstances.  Not only is the CPU
> > > > hotplugging code racy in places, it also totally destroys the ability
> > > > to have threads bound to CPUs, or even interrupts to CPUs.
> > > 
> > > [ more comments ommitted ]
> > > 
> > > Russell, let me suggest you take the time to read this:
> > > 
> > > 	http://lwn.net/Articles/481055/
> > > 
> > > then let's restart this conversation if you please.
> > 
> > Same comments apply for exactly the reasons I stated.
> 
> No they don't.  They can't. The generic kernel code doesn't know what is 
> happening behind the scene and may not even care if at the low level a 
> complete CPU replacement happened.  Trust me, I happen to know one thing 
> or two about those issues you mentioned and the code I'm working on will 
> be posted in due course.

Stop being bitchy.  Look, the information I have on bigLittle is limited
to what has been discussed, and what is in the specs.  That's it.  I don't
know what the hell you're doing with this.  I'm not a part of that effort.
I'm totally out of the loop (as seems to be per bloody normal with ARM Ltd.)

So please don't expect me to know what you're working on, or how your code
works.  I don't.  I don't have that information.

[PATCH] ARM: formalize an IPI for CPU wake-ups

[Nicolas Pitre]

On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:

> On Mon, Jun 11, 2012 at 03:11:05PM -0400, Nicolas Pitre wrote:
> > On Mon, 11 Jun 2012, Russell King - ARM Linux wrote:
> > 
> > > Same comments apply for exactly the reasons I stated.
> > 
> > No they don't.  They can't. The generic kernel code doesn't know what is 
> > happening behind the scene and may not even care if at the low level a 
> > complete CPU replacement happened.  Trust me, I happen to know one thing 
> > or two about those issues you mentioned and the code I'm working on will 
> > be posted in due course.
> 
> Stop being bitchy.  Look, the information I have on bigLittle is limited
> to what has been discussed, and what is in the specs.  That's it.  I don't
> know what the hell you're doing with this.  I'm not a part of that effort.

My intention is not to bitch.  I merely want to give you a heads up on a 
legitimate usage for multiple IPIs which are currently using SGI #1 and 
for which the kernel is generating unwanted warnings.  The actual SGI 
number is unimportant as long as it can be defined as no-op in the 
handle_IPI() context.

> I'm totally out of the loop (as seems to be per bloody normal with ARM Ltd.)

In this case this is a Linaro project, not ARM Ltd.  If you are 
interested by the details before they become public, we can discuss this 
offline.

> So please don't expect me to know what you're working on, or how your code
> works.  I don't.  I don't have that information.

I would expect that you know me well enough to trust me when I tell you 
that my code works.  Or at least give me the benefit of the doubt.


Nicolas