Patchwork ARM: imx: disable cpu in .cpu_kill hook

login
register
mail settings
Submitter Shawn Guo
Date Jan. 14, 2013, 1:09 p.m.
Message ID <1358168977-9631-1-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/211782/
State New
Headers show

Comments

Shawn Guo - Jan. 14, 2013, 1:09 p.m.
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 <shawn.guo@linaro.org>
---
 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(-)
Russell King - ARM Linux - Jan. 14, 2013, 1:32 p.m.
On Mon, Jan 14, 2013 at 09:09:37PM +0800, Shawn Guo wrote:
>  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);

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 - Jan. 14, 2013, 1:48 p.m.
On Mon, Jan 14, 2013 at 01:32:08PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 14, 2013 at 09:09:37PM +0800, Shawn Guo wrote:
> >  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);
> 
> 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 - Jan. 14, 2013, 1:55 p.m.
On 01/14/2013 07:09 AM, Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
> index 7191ab4..fa36fb8 100644
> --- a/arch/arm/mach-imx/common.h
> +++ b/arch/arm/mach-imx/common.h
> @@ -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);
> diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c
> index 3dec962..a9b4096 100644
> --- a/arch/arm/mach-imx/hotplug.c
> +++ b/arch/arm/mach-imx/hotplug.c
> @@ -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"

Can't you do:

set_cr(get_cr() | CR_C);

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

> +	  : "=&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;
>  }
> diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
> index 3777b80..66fae88 100644
> --- a/arch/arm/mach-imx/platsmp.c
> +++ b/arch/arm/mach-imx/platsmp.c
> @@ -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 - Jan. 14, 2013, 2 p.m.
On Mon, Jan 14, 2013 at 07:55:36AM -0600, Rob Herring wrote:
> 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

> > +	"	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 - Jan. 14, 2013, 4:29 p.m.
On Mon, 14 Jan 2013, Rob Herring wrote:

> 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 - Jan. 14, 2013, 4:46 p.m.
On Mon, Jan 14, 2013 at 11:29:13AM -0500, Nicolas Pitre wrote:
> On Mon, 14 Jan 2013, Rob Herring wrote:
> 
> > 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 - Jan. 14, 2013, 4:57 p.m.
On Mon, 14 Jan 2013, Russell King - ARM Linux wrote:

> On Mon, Jan 14, 2013 at 11:29:13AM -0500, Nicolas Pitre wrote:
> > On Mon, 14 Jan 2013, Rob Herring wrote:
> > 
> > > 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

Patch

diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h
index 7191ab4..fa36fb8 100644
--- a/arch/arm/mach-imx/common.h
+++ b/arch/arm/mach-imx/common.h
@@ -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);
diff --git a/arch/arm/mach-imx/hotplug.c b/arch/arm/mach-imx/hotplug.c
index 3dec962..a9b4096 100644
--- a/arch/arm/mach-imx/hotplug.c
+++ b/arch/arm/mach-imx/hotplug.c
@@ -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;
 }
diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 3777b80..66fae88 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -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
 };