ARM: imx: disable cpu in .cpu_kill hook

PATCH New Shawn Guo 4 participants 8 messages Jan 14, 2013

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Shawn Guo

author

2013-01-14 13:09 UTC #0 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

It's buggy to disable the cpu that is being hot-unplugged in .cpu_die hook which runs on the cpu itself. Instead, it should be done in .cpu_kill which runs on the thread (another cpu) that asks for shutting down the cpu. Move imx_enable_cpu(cpu, false) call into .cpu_kill hook, and leave the cpu to be hot-unplugged in WFI with a recovery call cpu_leave_lowpower(), so that we can get a more stable cpu hot-plug operation.

Signed-off-by Shawn Guo <[email protected]>

---
 arch/arm/mach-imx/common.h  |    1 +
 arch/arm/mach-imx/hotplug.c |   31 +++++++++++++++++++++++++++----
 arch/arm/mach-imx/platsmp.c |    1 +
 3 files changed, 29 insertions(+), 4 deletions(-)

arch/arm/mach-imx/common.h +1 -0

index 7191ab4..fa36fb8 100644
@@ -142,6 +142,7 @@ extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode);
 extern void imx6q_clock_map_io(void);
 
 extern void imx_cpu_die(unsigned int cpu);
+extern int imx_cpu_kill(unsigned int cpu);
 
 #ifdef CONFIG_PM
 extern void imx6q_pm_init(void);

arch/arm/mach-imx/hotplug.c +27 -4

index 3dec962..a9b4096 100644
@@ -38,6 +38,22 @@ static inline void cpu_enter_lowpower(void)
 	  : "cc");
 }
 
+static inline void cpu_leave_lowpower(void)
+{
+	unsigned int v;
+
+	asm volatile(
+		"mrc	p15, 0, %0, c1, c0, 0\n"
+	"	orr	%0, %0, %1\n"
+	"	mcr	p15, 0, %0, c1, c0, 0\n"
+	"	mrc	p15, 0, %0, c1, c0, 1\n"
+	"	orr	%0, %0, %2\n"
+	"	mcr	p15, 0, %0, c1, c0, 1\n"
+	  : "=&r" (v)
+	  : "Ir" (CR_C), "Ir" (0x40)
+	  : "cc");
+}
+
 /*
  * platform-specific code to shutdown a CPU
  *

@@ -45,10 +61,17 @@ static inline void cpu_enter_lowpower(void)
  */
 void imx_cpu_die(unsigned int cpu)
 {
+	static int spurious;
+
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+	cpu_leave_lowpower();
 
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
+}
+
+int imx_cpu_kill(unsigned int cpu)
+{
+	imx_enable_cpu(cpu, false);
+	return 1;
 }

arch/arm/mach-imx/platsmp.c +1 -0

index 3777b80..66fae88 100644
@@ -92,5 +92,6 @@ struct smp_operations  imx_smp_ops __initdata = {
 	.smp_boot_secondary	= imx_boot_secondary,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_die		= imx_cpu_die,
+	.cpu_kill		= imx_cpu_kill,
 #endif
 };

Signature · 1 line

1.7.9.5

Russell King - ARM Linux

2013-01-14 13:32 UTC #1 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

arch/arm/mach-imx/hotplug.c static inline void cpu_enter_lowpower(void) · line 70

Quoted 13 lines · Shawn Guo

void imx_cpu_die(unsigned int cpu)
{

arch/arm/mach-imx/hotplug.c +5 -4

+	static int spurious;
+
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+	cpu_leave_lowpower();
 
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);

Returning from the platform cpu die implementation should only be done in extreme situations, particularly where there's no possibility for the CPU to actually be taken offline (in other words, the ARM development platforms where there is _no_ possibility of powering down or resetting the secondary CPUs individually.)

I'm actually thinking about supplementing cpu_die() in arch/arm/kernel/smp.c with a warning:

@@ -259,6 +259,9 @@ void __ref cpu_die(void)

         */
        platform_cpu_die(cpu);

+ pr_warn("CPU%u: platform cpu_die() returned, trying to resuscitate\n",
+ cpu);
+

so that people get warned of this buggy behaviour.

Shawn Guo

author

2013-01-14 13:48 UTC #2 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · Shawn Guo
void imx_cpu_die(unsigned int cpu)
{
arch/arm/mach-imx/hotplug.c +4 -1
+	static int spurious;
+
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+	cpu_leave_lowpower();

Quoted 11 lines

arch/arm/mach-imx/hotplug.c static inline void cpu_enter_lowpower(void) · line 70
Quoted 4 lines
arch/arm/mach-imx/hotplug.c +1 -3
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
Returning from the platform cpu die implementation should only be done in extreme situations, particularly where there's no possibility for the CPU to actually be taken offline (in other words, the ARM development platforms where there is _no_ possibility of powering down or resetting the secondary CPUs individually.)

Indeed. I have to admit that I have copied the code from Vexpress but never seen it actually runs on my platform. Will remove the recovery completely.

Shawn

Rob Herring

2013-01-14 13:55 UTC #3 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

In reply to Shawn Guo

Quoted 33 lines · Shawn Guo

It's buggy to disable the cpu that is being hot-unplugged in .cpu_die hook which runs on the cpu itself. Instead, it should be done in .cpu_kill which runs on the thread (another cpu) that asks for shutting down the cpu. Move imx_enable_cpu(cpu, false) call into .cpu_kill hook, and leave the cpu to be hot-unplugged in WFI with a recovery call cpu_leave_lowpower(), so that we can get a more stable cpu hot-plug operation.
Signed-off-by Shawn Guo <[email protected]>
---
 arch/arm/mach-imx/common.h  |    1 +
 arch/arm/mach-imx/hotplug.c |   31 +++++++++++++++++++++++++++----
 arch/arm/mach-imx/platsmp.c |    1 +
 3 files changed, 29 insertions(+), 4 deletions(-)
arch/arm/mach-imx/common.h +1 -0
index 7191ab4..fa36fb8 100644
@@ -142,6 +142,7 @@ extern int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode);
 extern void imx6q_clock_map_io(void);
 
 extern void imx_cpu_die(unsigned int cpu);
+extern int imx_cpu_kill(unsigned int cpu);
 
 #ifdef CONFIG_PM
 extern void imx6q_pm_init(void);
arch/arm/mach-imx/hotplug.c +0 -0
index 3dec962..a9b4096 100644
@@ -38,6 +38,22 @@ static inline void cpu_enter_lowpower(void)
 	  : "cc");

arch/arm/mach-imx/hotplug.c static inline void cpu_enter_lowpower(void) · line 48

Quoted 10 lines

arch/arm/mach-imx/hotplug.c +8 -0

 }
 
+static inline void cpu_leave_lowpower(void)
+{
+	unsigned int v;
+
+	asm volatile(
+		"mrc	p15, 0, %0, c1, c0, 0\n"
+	"	orr	%0, %0, %1\n"
+	"	mcr	p15, 0, %0, c1, c0, 0\n"

Can't you do:

set_cr(get_cr() | CR_C);

arch/arm/mach-imx/hotplug.c static inline void cpu_enter_lowpower(void) · line 51

Quoted 3 lines

arch/arm/mach-imx/hotplug.c +3 -0

+	"	mrc	p15, 0, %0, c1, c0, 1\n"
+	"	orr	%0, %0, %2\n"
+	"	mcr	p15, 0, %0, c1, c0, 1\n"

Setting or clearing the SMP bit is showing up in multiple places (big.LITTLE series, Tegra cpuidle). I did inline versions of set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.

Rob

Quoted 42 lines

arch/arm/mach-imx/hotplug.c +16 -4

+	  : "=&r" (v)
+	  : "Ir" (CR_C), "Ir" (0x40)
+	  : "cc");
+}
+
 /*
  * platform-specific code to shutdown a CPU
  *

@@ -45,10 +61,17 @@ static inline void cpu_enter_lowpower(void)
  */
 void imx_cpu_die(unsigned int cpu)
 {
+	static int spurious;
+
 	cpu_enter_lowpower();
-	imx_enable_cpu(cpu, false);
+	cpu_do_idle();
+	cpu_leave_lowpower();
 
-	/* spin here until hardware takes it down */
-	while (1)
-		;
+	pr_warn("CPU%u: %u spurious wakeup calls\n", cpu, ++spurious);
+}
+
+int imx_cpu_kill(unsigned int cpu)
+{
+	imx_enable_cpu(cpu, false);
+	return 1;
 }

arch/arm/mach-imx/platsmp.c +1 -0

index 3777b80..66fae88 100644
@@ -92,5 +92,6 @@ struct smp_operations  imx_smp_ops __initdata = {
 	.smp_boot_secondary	= imx_boot_secondary,
 #ifdef CONFIG_HOTPLUG_CPU
 	.cpu_die		= imx_cpu_die,
+	.cpu_kill		= imx_cpu_kill,
 #endif
 };

Shawn Guo

author

2013-01-14 14:00 UTC #4 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

Quoted 4 lines · Rob Herring

Can't you do:
set_cr(get_cr() | CR_C);

Yes, this is nice. But I'm taking Russell's suggestion to remove the recovery piece, which is unnecessary for imx6q.

Shawn

Quoted 8 lines

arch/arm/mach-imx/hotplug.c static inline void cpu_enter_lowpower(void) · line 51
Quoted 3 lines
arch/arm/mach-imx/hotplug.c +3 -0
+	"	mrc	p15, 0, %0, c1, c0, 1\n"
+	"	orr	%0, %0, %2\n"
+	"	mcr	p15, 0, %0, c1, c0, 1\n"
Setting or clearing the SMP bit is showing up in multiple places (big.LITTLE series, Tegra cpuidle). I did inline versions of set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.

Nicolas Pitre

2013-01-14 16:29 UTC #5 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

In reply to Rob Herring

Quoted 3 lines · Rob Herring

Setting or clearing the SMP bit is showing up in multiple places (big.LITTLE series, Tegra cpuidle). I did inline versions of set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.

Could you do that like now and send it to RMK for his stable branch? This way I could rebase the b.L series on top.

Nicolas

Russell King - ARM Linux

2013-01-14 16:46 UTC #6 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

Quoted 8 lines · Nicolas Pitre

Quoted 3 lines · Rob Herring
Setting or clearing the SMP bit is showing up in multiple places (big.LITTLE series, Tegra cpuidle). I did inline versions of set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.
Could you do that like now and send it to RMK for his stable branch? This way I could rebase the b.L series on top.

Just be clear: I don't think this is something for the stable trees.

Nicolas Pitre

2013-01-14 16:57 UTC #7 lore raw

PATCH 1/1 ARM: imx: disable cpu in .cpu_kill hook

Quoted 11 lines · Russell King - ARM Linux

Quoted 8 lines · Nicolas Pitre
Quoted 3 lines · Rob Herring
Setting or clearing the SMP bit is showing up in multiple places (big.LITTLE series, Tegra cpuidle). I did inline versions of set_auxcr/get_auxcr for highbank cpuidle. We should be make those common.
Could you do that like now and send it to RMK for his stable branch? This way I could rebase the b.L series on top.
Just be clear: I don't think this is something for the stable trees.

Right, not the stable tree but more precisely the devel-stable branch.

Nicolas