ARM: formalize an IPI for CPU wake-ups

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.

Quoted 12 lines · 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.

Quoted 2 lines

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.

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · 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.

Quoted 4 lines

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.

Quoted 23 lines · Nicolas Pitre

Quoted 20 lines · Russell King - ARM Linux

Quoted 9 lines · 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.

Quoted 5 lines

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.

Quoted 2 lines

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.

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.

Quoted 9 lines · Nicolas Pitre

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

Quoted 9 lines · Stephen Boyd

Quoted 8 lines · Nicolas Pitre

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.

Quoted 5 lines

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.

Quoted 4 lines · Russell King - ARM Linux

Quoted 3 lines · Stephen Boyd

Quoted 2 lines · Nicolas Pitre

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

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

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

Quoted 1 line

Quoted 1 line

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

January/039275.html

Quoted 1 line

Ok, in that case I've just committed three patches to make those

platforms

Quoted 1 line

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.

Quoted 3 lines · Stephen Boyd

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 :)

Quoted 2 lines · Kukjin Kim

Quoted 1 line · Stephen Boyd

Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the

only

Quoted 5 lines

Quoted 2 lines

one left to move to SGI0.

Yeah, EXYNOS can use SGI0 instead of SGI1 :)

Russell, can we apply something like this?

arch/arm/mach- renamed from arch/arm/mach-exynos/platsmp.c +0 -0

exynos/platsmp.c
index 36c3984..090e32b 100644

arch/arm/mach-exynos/platsmp.c +1 -1

@@ -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.

Quoted 11 lines · Stephen Boyd

Quoted 2 lines · Kukjin Kim

Quoted 1 line · Stephen Boyd

Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the

only

Quoted 5 lines

Quoted 2 lines

one left to move to SGI0.

Yeah, EXYNOS can use SGI0 instead of SGI1 :)

Quoted 2 lines

Russell, can we apply something like this?

Please feel free to add my ack on following change.

Acked-by Kukjin Kim <[email protected]>

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().

Quoted 7 lines · Colin Cross

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.

Quoted 12 lines · Kukjin Kim

Quoted 11 lines · Stephen Boyd

Quoted 2 lines · Kukjin Kim

Quoted 1 line · Stephen Boyd

Great. Kukjin Kim, can exynos use SGI0? It looks like exynos is the

only

Quoted 5 lines

Quoted 2 lines

one left to move to SGI0.

Yeah, EXYNOS can use SGI0 instead of SGI1 :)

Quoted 6 lines

Quoted 2 lines

Russell, can we apply something like this?

Please feel free to add my ack on following change.

Acked-by Kukjin Kim <[email protected]>

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().

Quoted 7 lines · Kukjin Kim

Quoted 3 lines · Stephen Boyd

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?

arch/arm/mach-exynos/platsmp.c +1 -1

index 36c3984..090e32b 100644
@@ -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.

arch/arm/mach-shmobile/smp-emev2.c +1 -1

@@ -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 <[email protected]>

Quoted 17 lines · Magnus Damm

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.

arch/arm/mach-shmobile/smp-emev2.c +1 -1

@@ -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;
 }
 

Quoted 4 lines

So if/where needed, please add my:

Acked-by Magnus Damm <[email protected]>

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

Quoted 20 lines · Stephen Boyd

Quoted 19 lines · Magnus Damm

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.

arch/arm/mach-shmobile/smp-emev2.c 2012-08-08 15:42:54.000000000 +0900 renamed from arch/arm/mach-shmobile/smp-emev2.c +1 -1

@@ -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:

Quoted 5 lines

Quoted 2 lines

Acked-by Magnus Damm <[email protected]>

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.

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

Quoted 2 lines · Stephen Boyd

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------

arch/arm/include/asm/hardirq.h +1 -1

index 436e60b..2740c2a2 100644
@@ -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;

arch/arm/kernel/smp.c +9 -4

index ebd8ad2..d98c37e 100644
@@ -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();

Signature · 3 lines

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

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

Quoted 2 lines · Nicolas Pitre

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.

Quoted 10 lines · Russell King - ARM Linux

Quoted 2 lines · Nicolas Pitre

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.

Quoted 15 lines · Nicolas Pitre

Quoted 10 lines · Russell King - ARM Linux

Quoted 2 lines · Nicolas Pitre

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 ]

Quoted 5 lines

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.

Quoted 22 lines · Russell King - ARM Linux

Quoted 20 lines · Nicolas Pitre

Quoted 10 lines · Russell King - ARM Linux

Quoted 2 lines · Nicolas Pitre

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.

Quoted 1 line

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.

Quoted 9 lines · Nicolas Pitre

Quoted 1 line · Russell King - ARM Linux

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.