[RFC PATCH 0/3] MCPM/TC2 support for CPU powerdown synchronisation

[RFC PATCH 0/3] MCPM/TC2 support for CPU powerdown synchronisation

[Dave Martin]

This series adds MCPM support for detecting when a CPU is safely powered
down, and provides an implementation for TC2.

This is sufficient to for working kexec with real power management on
TC2.  To test it, you'll also need:

  * CONFIG_KEXEC=y
  * CONFIG_PROC_DEVICE_TREE=y
  * CONFIG_MCPM=y
  * CONFIG_ARCH_VEXPRESS_TC2_PM=y
  * sufficiently new kexec-tools
    (git://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git
    v2.0.4 worked for me)

To prevent CPUs from running off into the weeds across kexec, this
series requires Lorenzo's patch
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200917.html
(arm: vexpress: tc2: fix hotplug/idle/kexec race on cluster power down).

Dave Martin (3):
  ARM: mcpm: Factor out logical-to-physical CPU translation
  ARM: mcpm: Implement cpu_kill() to synchronise on powerdown
  ARM: vexpress/TC2: Implement MCPM power_down_finish()

 arch/arm/common/mcpm_entry.c    |    5 ++++
 arch/arm/common/mcpm_platsmp.c  |   27 +++++++++++++++---
 arch/arm/include/asm/mcpm.h     |   31 ++++++++++++++++++++
 arch/arm/mach-vexpress/spc.c    |   39 +++++++++++++++++++++++++
 arch/arm/mach-vexpress/spc.h    |    1 +
 arch/arm/mach-vexpress/tc2_pm.c |   60 +++++++++++++++++++++++++++++++++++----
 6 files changed, 154 insertions(+), 9 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/3] ARM: mcpm: Factor out logical-to-physical CPU translation

[Dave Martin]

This patch factors the logical-to-physical CPU translation out of
mcpm_boot_secondary(), so that it can be reused elsewhere.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/common/mcpm_platsmp.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/common/mcpm_platsmp.c b/arch/arm/common/mcpm_platsmp.c
index 1bc34c7..c0c3cd7 100644
--- a/arch/arm/common/mcpm_platsmp.c
+++ b/arch/arm/common/mcpm_platsmp.c
@@ -19,14 +19,23 @@
 #include <asm/smp.h>
 #include <asm/smp_plat.h>
 
+static void cpu_to_pcpu(unsigned int cpu,
+			unsigned int *pcpu, unsigned int *pcluster)
+{
+	unsigned int mpidr;
+
+	mpidr = cpu_logical_map(cpu);
+	*pcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	*pcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+}
+
 static int mcpm_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	unsigned int mpidr, pcpu, pcluster, ret;
+	unsigned int pcpu, pcluster, ret;
 	extern void secondary_startup(void);
 
-	mpidr = cpu_logical_map(cpu);
-	pcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
-	pcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+	cpu_to_pcpu(cpu, &pcpu, &pcluster);
+
 	pr_debug("%s: logical CPU %d is physical CPU %d cluster %d\n",
 		 __func__, cpu, pcpu, pcluster);
 
-- 
1.7.9.5

[RFC PATCH 2/3] ARM: mcpm: Implement cpu_kill() to synchronise on powerdown

[Dave Martin]

CPU hotplug and kexec rely on smp_ops.cpu_kill(), which is supposed
to wait for the CPU to park or power down, and perform the last
rites (such as disabling clocks etc., where the platform doesn't do
this automatically).

kexec in particular is unsafe without performing this
synchronisation to park secondaries.  Without it, the secondaries
might not be parked when kexec trashes the kernel.

There is no generic way to do this synchronisation, so a new mcpm
platform_ops method power_down_finish() is added by this patch.

The new method is mandatory.  A platform which provides no way to
detect when CPUs are parked is likely broken.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm/common/mcpm_entry.c   |    5 +++++
 arch/arm/common/mcpm_platsmp.c |   10 ++++++++++
 arch/arm/include/asm/mcpm.h    |   31 +++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index 370236d..b722027 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -89,6 +89,11 @@ void mcpm_cpu_power_down(void)
 	BUG();
 }
 
+int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster)
+{
+	return platform_ops->power_down_finish(cpu, cluster);
+}
+
 void mcpm_cpu_suspend(u64 expected_residency)
 {
 	phys_reset_t phys_reset;
diff --git a/arch/arm/common/mcpm_platsmp.c b/arch/arm/common/mcpm_platsmp.c
index c0c3cd7..ac48cc7 100644
--- a/arch/arm/common/mcpm_platsmp.c
+++ b/arch/arm/common/mcpm_platsmp.c
@@ -56,6 +56,15 @@ static void mcpm_secondary_init(unsigned int cpu)
 
 #ifdef CONFIG_HOTPLUG_CPU
 
+static int mcpm_cpu_kill(unsigned int cpu)
+{
+	unsigned int pcpu, pcluster;
+
+	cpu_to_pcpu(cpu, &pcpu, &pcluster);
+
+	return mcpm_cpu_power_down_finish(pcpu, pcluster);
+}
+
 static int mcpm_cpu_disable(unsigned int cpu)
 {
 	/*
@@ -82,6 +91,7 @@ static struct smp_operations __initdata mcpm_smp_ops = {
 	.smp_boot_secondary	= mcpm_boot_secondary,
 	.smp_secondary_init	= mcpm_secondary_init,
 #ifdef CONFIG_HOTPLUG_CPU
+	.cpu_kill		= mcpm_cpu_kill,
 	.cpu_disable		= mcpm_cpu_disable,
 	.cpu_die		= mcpm_cpu_die,
 #endif
diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
index 0f7b762..22cdc6f 100644
--- a/arch/arm/include/asm/mcpm.h
+++ b/arch/arm/include/asm/mcpm.h
@@ -78,10 +78,40 @@ int mcpm_cpu_power_up(unsigned int cpu, unsigned int cluster);
  *
  * This does not return.  Re-entry in the kernel is expected via
  * mcpm_entry_point.
+ *
+ * The CPU is not necessarily truly halted until
+ * mcpu_cpu_power_down_finish() subsequently returns non-zero for the
+ * specified cpu.  Until then, other CPUs should make sure they do not
+ * trash memory the target CPU might be executing/accessing.
  */
 void mcpm_cpu_power_down(void);
 
 /**
+ * mcpm_cpu_power_down_finish - wait for a specified CPU to halt, and
+ *	make sure it is powered off
+ *
+ * @cpu: CPU number within given cluster
+ * @cluster: cluster number for the CPU
+ *
+ * Call this function to ensure that a pending powerdown has taken
+ * effect and the CPU is safely parked before performing non-mcpm
+ * operations that may affect the CPU (such as kexec trashing the
+ * kernel text).
+ *
+ * It is *not* necessary to call this function if you only need to
+ * serialise a pending powerdown with mcpu_cpu_power_up() or a wakeup
+ * event.
+ *
+ * Do not call this function unless the specified CPU has already
+ * called mcpu_cpu_power_down() or has committed to doing so.
+ *
+ * @return:
+ *	- zero if the CPU is in a safely parked state
+ *	- nonzero otherwise (e.g., timeout)
+ */
+int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster);
+
+/**
  * mcpm_cpu_suspend - bring the calling CPU in a suspended state
  *
  * @expected_residency: duration in microseconds the CPU is expected
@@ -120,6 +150,7 @@ int mcpm_cpu_powered_up(void);
 struct mcpm_platform_ops {
 	int (*power_up)(unsigned int cpu, unsigned int cluster);
 	void (*power_down)(void);
+	int (*power_down_finish)(unsigned int cpu, unsigned int cluster);
 	void (*suspend)(u64);
 	void (*powered_up)(void);
 };
-- 
1.7.9.5

[RFC PATCH 3/3] ARM: vexpress/TC2: Implement MCPM power_down_finish()

[Dave Martin]

This patch implements the power_down_finish() method for TC2, to
enable the kernel to confirm when CPUs are safely powered down.

The information required for determining when a CPU is parked
cannot be obtained from any single place, so a few sources of
information must be combined:

  * mcpm_cpu_power_down() must be pending for the CPU, so that we
    don't get confused by false STANDBYWFI positives arising from
    CPUidle.  This is detected by waiting for the tc2_pm use count
    for the target CPU to reach 0.

  * Either the SPC must report that the CPU has asserted
    STANDBYWFI, or the TC2 tile's reset control logic must be
    holding the CPU in reset.

    Just checking for STANDBYWFI is not sufficient, because this
    signal is not latched when the the cluster is clamped off and
    powered down: the relevant status bits just drop to zero.  This
    means that STANDBYWFI status cannot be used for reliable
    detection of the last CPU in a cluster reaching WFI.

This patch is required in order for kexec to work with MCPM on TC2.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---

The mdelay(1) in tc2_pm_power_down_finish() is arbitrary.  The power
controller can show millisecond response times in the worst case, and
CPU hotplug is not expected to be performance-critical.

It may be wise to add a timeout to this function, but that's open to
discussion.


 arch/arm/mach-vexpress/spc.c    |   39 +++++++++++++++++++++++++
 arch/arm/mach-vexpress/spc.h    |    1 +
 arch/arm/mach-vexpress/tc2_pm.c |   60 +++++++++++++++++++++++++++++++++++----
 3 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-vexpress/spc.c b/arch/arm/mach-vexpress/spc.c
index eefb029..6f6ac56 100644
--- a/arch/arm/mach-vexpress/spc.c
+++ b/arch/arm/mach-vexpress/spc.c
@@ -35,6 +35,10 @@
 /* SPC per-CPU mailboxes */
 #define A15_BX_ADDR0		0x68
 #define A7_BX_ADDR0		0x78
+/* SPC CPU/cluster reset statue */
+#define STANDBYWFI_STAT		0x3c
+#define STANDBYWFI_STAT_A15_CPU_MASK(cpu)	(1 << (cpu))
+#define STANDBYWFI_STAT_A7_CPU_MASK(cpu)	(1 << (3 + (cpu)))
 
 /* wake-up interrupt masks */
 #define GBL_WAKEUP_INT_MSK	(0x3 << 10)
@@ -157,6 +161,41 @@ void ve_spc_powerdown(u32 cluster, bool enable)
 	writel_relaxed(enable, info->baseaddr + pwdrn_reg);
 }
 
+static u32 standbywfi_cpu_mask(u32 cpu, u32 cluster)
+{
+	return cluster_is_a15(cluster) ?
+		  STANDBYWFI_STAT_A15_CPU_MASK(cpu)
+		: STANDBYWFI_STAT_A7_CPU_MASK(cpu);
+}
+
+/**
+ * ve_spc_cpu_in_wfi(u32 cpu, u32 cluster)
+ *
+ * @cpu: mpidr[7:0] bitfield describing CPU affinity level within cluster
+ * @cluster: mpidr[15:8] bitfield describing cluster affinity level
+ *
+ * @return: non-zero if and only if the specified CPU is in WFI
+ *
+ * Take care when interpreting the result of this function: a CPU might
+ * be in WFI temporarily due to idle, and is not necessarily safely
+ * parked.
+ */
+int ve_spc_cpu_in_wfi(u32 cpu, u32 cluster)
+{
+	int ret;
+	u32 mask = standbywfi_cpu_mask(cpu, cluster);
+
+	if (cluster >= MAX_CLUSTERS)
+		return 1;
+
+	ret = readl_relaxed(info->baseaddr + STANDBYWFI_STAT);
+
+	pr_debug("%s: PCFGREG[0x%X] = 0x%08X, mask = 0x%X\n",
+		 __func__, STANDBYWFI_STAT, ret, mask);
+
+	return ret & mask;
+}
+
 int __init ve_spc_init(void __iomem *baseaddr, u32 a15_clusid)
 {
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
diff --git a/arch/arm/mach-vexpress/spc.h b/arch/arm/mach-vexpress/spc.h
index 5f7e4a4..edb1b06 100644
--- a/arch/arm/mach-vexpress/spc.h
+++ b/arch/arm/mach-vexpress/spc.h
@@ -20,5 +20,6 @@ void ve_spc_global_wakeup_irq(bool set);
 void ve_spc_cpu_wakeup_irq(u32 cluster, u32 cpu, bool set);
 void ve_spc_set_resume_addr(u32 cluster, u32 cpu, u32 addr);
 void ve_spc_powerdown(u32 cluster, bool enable);
+int ve_spc_cpu_in_wfi(u32 cpu, u32 cluster);
 
 #endif
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index e6eb481..0c56e72 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -12,6 +12,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -31,11 +32,17 @@
 #include "spc.h"
 
 /* SCC conf registers */
+#define RESET_CTRL		0x018
+#define RESET_A15_NCORERESET(cpu)	(1 << (2 + (cpu)))
+#define RESET_A7_NCORERESET(cpu)	(1 << (19 + (cpu)))
+
 #define A15_CONF		0x400
 #define A7_CONF			0x500
 #define SYS_INFO		0x700
 #define SPC_BASE		0xb00
 
+static void __iomem *scc;
+
 /*
  * We can't use regular spinlocks. In the switcher case, it is possible
  * for an outbound CPU to call power_down() after its inbound counterpart
@@ -233,6 +240,49 @@ static void tc2_pm_power_down(void)
 	tc2_pm_down(0);
 }
 
+static int tc2_core_in_reset(unsigned int cpu, unsigned int cluster)
+{
+	u32 mask = cluster ?
+		  RESET_A7_NCORERESET(cpu)
+		: RESET_A15_NCORERESET(cpu);
+
+	return !(readl_relaxed(scc + RESET_CTRL) & mask);
+}
+
+static int tc2_pm_power_down_finish(unsigned int cpu, unsigned int cluster)
+{
+	while (1) {
+		/*
+		 * In case this CPU raced too far ahead of the target CPU,
+		 * wait until tc2_pm_down() has really been entered:
+		 */
+		if (ACCESS_ONCE(tc2_pm_use_count[cpu][cluster]) != 0)
+			goto poll;
+
+		pr_debug("%s(cpu=%u, cluster=%u): RESET_CTRL = 0x%08X\n",
+			 __func__, cpu, cluster,
+			 readl_relaxed(scc + RESET_CTRL));
+
+		/*
+		 * We need the CPU to reach WFI, but the power
+		 * controller may immediately put the CPU in reset and
+		 * power the cluster off as soon as that happens, if
+		 * there are no other CPUs live on the cluster.  That
+		 * can cause the CPU's STANDBYWFI signal to disappear.
+		 *
+		 * So we need to check for both conditions:
+		 */
+		if (tc2_core_in_reset(cpu, cluster) ||
+		    ve_spc_cpu_in_wfi(cpu, cluster))
+			break;
+
+	poll:
+		msleep(1);
+	}
+		
+	return 1;
+}
+
 static void tc2_pm_suspend(u64 residency)
 {
 	unsigned int mpidr, cpu, cluster;
@@ -275,10 +325,11 @@ static void tc2_pm_powered_up(void)
 }
 
 static const struct mcpm_platform_ops tc2_pm_power_ops = {
-	.power_up	= tc2_pm_power_up,
-	.power_down	= tc2_pm_power_down,
-	.suspend	= tc2_pm_suspend,
-	.powered_up	= tc2_pm_powered_up,
+	.power_up		= tc2_pm_power_up,
+	.power_down		= tc2_pm_power_down,
+	.power_down_finish	= tc2_pm_power_down_finish,
+	.suspend		= tc2_pm_suspend,
+	.powered_up		= tc2_pm_powered_up,
 };
 
 static bool __init tc2_pm_usage_count_init(void)
@@ -312,7 +363,6 @@ static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
 static int __init tc2_pm_init(void)
 {
 	int ret;
-	void __iomem *scc;
 	u32 a15_cluster_id, a7_cluster_id, sys_info;
 	struct device_node *np;
 
-- 
1.7.9.5

[RFC PATCH 1/3] ARM: mcpm: Factor out logical-to-physical CPU translation

[Nicolas Pitre]

On Mon, 30 Sep 2013, Dave Martin wrote:

> This patch factors the logical-to-physical CPU translation out of
> mcpm_boot_secondary(), so that it can be reused elsewhere.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

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


> ---
>  arch/arm/common/mcpm_platsmp.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/common/mcpm_platsmp.c b/arch/arm/common/mcpm_platsmp.c
> index 1bc34c7..c0c3cd7 100644
> --- a/arch/arm/common/mcpm_platsmp.c
> +++ b/arch/arm/common/mcpm_platsmp.c
> @@ -19,14 +19,23 @@
>  #include <asm/smp.h>
>  #include <asm/smp_plat.h>
>  
> +static void cpu_to_pcpu(unsigned int cpu,
> +			unsigned int *pcpu, unsigned int *pcluster)
> +{
> +	unsigned int mpidr;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	*pcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	*pcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +}
> +
>  static int mcpm_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
> -	unsigned int mpidr, pcpu, pcluster, ret;
> +	unsigned int pcpu, pcluster, ret;
>  	extern void secondary_startup(void);
>  
> -	mpidr = cpu_logical_map(cpu);
> -	pcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> -	pcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +	cpu_to_pcpu(cpu, &pcpu, &pcluster);
> +
>  	pr_debug("%s: logical CPU %d is physical CPU %d cluster %d\n",
>  		 __func__, cpu, pcpu, pcluster);
>  
> -- 
> 1.7.9.5
> 

[RFC PATCH 2/3] ARM: mcpm: Implement cpu_kill() to synchronise on powerdown

[Nicolas Pitre]

On Mon, 30 Sep 2013, Dave Martin wrote:

> CPU hotplug and kexec rely on smp_ops.cpu_kill(), which is supposed
> to wait for the CPU to park or power down, and perform the last
> rites (such as disabling clocks etc., where the platform doesn't do
> this automatically).
> 
> kexec in particular is unsafe without performing this
> synchronisation to park secondaries.  Without it, the secondaries
> might not be parked when kexec trashes the kernel.
> 
> There is no generic way to do this synchronisation, so a new mcpm
> platform_ops method power_down_finish() is added by this patch.
> 
> The new method is mandatory.  A platform which provides no way to
> detect when CPUs are parked is likely broken.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm/common/mcpm_entry.c   |    5 +++++
>  arch/arm/common/mcpm_platsmp.c |   10 ++++++++++
>  arch/arm/include/asm/mcpm.h    |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
> index 370236d..b722027 100644
> --- a/arch/arm/common/mcpm_entry.c
> +++ b/arch/arm/common/mcpm_entry.c
> @@ -89,6 +89,11 @@ void mcpm_cpu_power_down(void)
>  	BUG();
>  }
>  
> +int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster)
> +{
> +	return platform_ops->power_down_finish(cpu, cluster);
> +}

Please make it return an error (non-zero return value) if platform_ops 
or platform_ops->power_down_finish is NULL.  Otherwise this would oops 
the kernel.
There is a patch already handling the other cases in RMK's patch system.

> +
>  void mcpm_cpu_suspend(u64 expected_residency)
>  {
>  	phys_reset_t phys_reset;
> diff --git a/arch/arm/common/mcpm_platsmp.c b/arch/arm/common/mcpm_platsmp.c
> index c0c3cd7..ac48cc7 100644
> --- a/arch/arm/common/mcpm_platsmp.c
> +++ b/arch/arm/common/mcpm_platsmp.c
> @@ -56,6 +56,15 @@ static void mcpm_secondary_init(unsigned int cpu)
>  
>  #ifdef CONFIG_HOTPLUG_CPU
>  
> +static int mcpm_cpu_kill(unsigned int cpu)
> +{
> +	unsigned int pcpu, pcluster;
> +
> +	cpu_to_pcpu(cpu, &pcpu, &pcluster);
> +
> +	return mcpm_cpu_power_down_finish(pcpu, pcluster);
> +}
> +
>  static int mcpm_cpu_disable(unsigned int cpu)
>  {
>  	/*
> @@ -82,6 +91,7 @@ static struct smp_operations __initdata mcpm_smp_ops = {
>  	.smp_boot_secondary	= mcpm_boot_secondary,
>  	.smp_secondary_init	= mcpm_secondary_init,
>  #ifdef CONFIG_HOTPLUG_CPU
> +	.cpu_kill		= mcpm_cpu_kill,
>  	.cpu_disable		= mcpm_cpu_disable,
>  	.cpu_die		= mcpm_cpu_die,
>  #endif
> diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
> index 0f7b762..22cdc6f 100644
> --- a/arch/arm/include/asm/mcpm.h
> +++ b/arch/arm/include/asm/mcpm.h
> @@ -78,10 +78,40 @@ int mcpm_cpu_power_up(unsigned int cpu, unsigned int cluster);
>   *
>   * This does not return.  Re-entry in the kernel is expected via
>   * mcpm_entry_point.
> + *
> + * The CPU is not necessarily truly halted until
> + * mcpu_cpu_power_down_finish() subsequently returns non-zero for the

s/mcpu/mcpm/

> + * specified cpu.  Until then, other CPUs should make sure they do not
> + * trash memory the target CPU might be executing/accessing.
>   */
>  void mcpm_cpu_power_down(void);
>  
>  /**
> + * mcpm_cpu_power_down_finish - wait for a specified CPU to halt, and
> + *	make sure it is powered off
> + *
> + * @cpu: CPU number within given cluster
> + * @cluster: cluster number for the CPU
> + *
> + * Call this function to ensure that a pending powerdown has taken
> + * effect and the CPU is safely parked before performing non-mcpm
> + * operations that may affect the CPU (such as kexec trashing the
> + * kernel text).
> + *
> + * It is *not* necessary to call this function if you only need to
> + * serialise a pending powerdown with mcpu_cpu_power_up() or a wakeup

Ditto.

> + * event.
> + *
> + * Do not call this function unless the specified CPU has already
> + * called mcpu_cpu_power_down() or has committed to doing so.

Ditto.

> + *
> + * @return:
> + *	- zero if the CPU is in a safely parked state
> + *	- nonzero otherwise (e.g., timeout)
> + */
> +int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster);
> +
> +/**
>   * mcpm_cpu_suspend - bring the calling CPU in a suspended state
>   *
>   * @expected_residency: duration in microseconds the CPU is expected
> @@ -120,6 +150,7 @@ int mcpm_cpu_powered_up(void);
>  struct mcpm_platform_ops {
>  	int (*power_up)(unsigned int cpu, unsigned int cluster);
>  	void (*power_down)(void);
> +	int (*power_down_finish)(unsigned int cpu, unsigned int cluster);
>  	void (*suspend)(u64);
>  	void (*powered_up)(void);
>  };
> -- 
> 1.7.9.5
> 

[RFC PATCH 3/3] ARM: vexpress/TC2: Implement MCPM power_down_finish()

[Nicolas Pitre]

On Mon, 30 Sep 2013, Dave Martin wrote:

> This patch implements the power_down_finish() method for TC2, to
> enable the kernel to confirm when CPUs are safely powered down.
> 
> The information required for determining when a CPU is parked
> cannot be obtained from any single place, so a few sources of
> information must be combined:
> 
>   * mcpm_cpu_power_down() must be pending for the CPU, so that we
>     don't get confused by false STANDBYWFI positives arising from
>     CPUidle.  This is detected by waiting for the tc2_pm use count
>     for the target CPU to reach 0.
> 
>   * Either the SPC must report that the CPU has asserted
>     STANDBYWFI, or the TC2 tile's reset control logic must be
>     holding the CPU in reset.
> 
>     Just checking for STANDBYWFI is not sufficient, because this
>     signal is not latched when the the cluster is clamped off and
>     powered down: the relevant status bits just drop to zero.  This
>     means that STANDBYWFI status cannot be used for reliable
>     detection of the last CPU in a cluster reaching WFI.
> 
> This patch is required in order for kexec to work with MCPM on TC2.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> 
> The mdelay(1) in tc2_pm_power_down_finish() is arbitrary.  The power
> controller can show millisecond response times in the worst case, and
> CPU hotplug is not expected to be performance-critical.
> 
> It may be wise to add a timeout to this function, but that's open to
> discussion.

That would be a good idea.  I'd suggest you reduce the polling loop to 
something like 10 ms and bail out after one second.   We've been 
affected by funny STANDBYWFI 
behaviors before.



Nicolas

[RFC PATCH 2/3] ARM: mcpm: Implement cpu_kill() to synchronise on powerdown

[Dave Martin]

On Mon, Sep 30, 2013 at 01:00:00PM -0400, Nicolas Pitre wrote:
> On Mon, 30 Sep 2013, Dave Martin wrote:
> 
> > CPU hotplug and kexec rely on smp_ops.cpu_kill(), which is supposed
> > to wait for the CPU to park or power down, and perform the last
> > rites (such as disabling clocks etc., where the platform doesn't do
> > this automatically).
> > 
> > kexec in particular is unsafe without performing this
> > synchronisation to park secondaries.  Without it, the secondaries
> > might not be parked when kexec trashes the kernel.
> > 
> > There is no generic way to do this synchronisation, so a new mcpm
> > platform_ops method power_down_finish() is added by this patch.
> > 
> > The new method is mandatory.  A platform which provides no way to
> > detect when CPUs are parked is likely broken.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm/common/mcpm_entry.c   |    5 +++++
> >  arch/arm/common/mcpm_platsmp.c |   10 ++++++++++
> >  arch/arm/include/asm/mcpm.h    |   31 +++++++++++++++++++++++++++++++
> >  3 files changed, 46 insertions(+)
> > 
> > diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
> > index 370236d..b722027 100644
> > --- a/arch/arm/common/mcpm_entry.c
> > +++ b/arch/arm/common/mcpm_entry.c
> > @@ -89,6 +89,11 @@ void mcpm_cpu_power_down(void)
> >  	BUG();
> >  }
> >  
> > +int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster)
> > +{
> > +	return platform_ops->power_down_finish(cpu, cluster);
> > +}
> 
> Please make it return an error (non-zero return value) if platform_ops 
> or platform_ops->power_down_finish is NULL.  Otherwise this would oops 
> the kernel.
> 
> There is a patch already handling the other cases in RMK's patch system.

OK -- I didn't pay enough attention as that went past.

I'll grab that patch and rebase onto it.


Do you have concerns about the mandatoriness of the new call?  The
alternative would be to check for it at registration time and set
smp_ops.cpu_kill to NULL if the backend method isn't there.

But I really wouldn't want to encourage that until someone comes
up with a good reason.

[...]

> > diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
> > index 0f7b762..22cdc6f 100644
> > --- a/arch/arm/include/asm/mcpm.h
> > +++ b/arch/arm/include/asm/mcpm.h
> > @@ -78,10 +78,40 @@ int mcpm_cpu_power_up(unsigned int cpu, unsigned int cluster);
> >   *
> >   * This does not return.  Re-entry in the kernel is expected via
> >   * mcpm_entry_point.
> > + *
> > + * The CPU is not necessarily truly halted until
> > + * mcpu_cpu_power_down_finish() subsequently returns non-zero for the
> 
> s/mcpu/mcpm/
> 
> > + * specified cpu.  Until then, other CPUs should make sure they do not
> > + * trash memory the target CPU might be executing/accessing.
> >   */
> >  void mcpm_cpu_power_down(void);
> >  
> >  /**
> > + * mcpm_cpu_power_down_finish - wait for a specified CPU to halt, and
> > + *	make sure it is powered off
> > + *
> > + * @cpu: CPU number within given cluster
> > + * @cluster: cluster number for the CPU
> > + *
> > + * Call this function to ensure that a pending powerdown has taken
> > + * effect and the CPU is safely parked before performing non-mcpm
> > + * operations that may affect the CPU (such as kexec trashing the
> > + * kernel text).
> > + *
> > + * It is *not* necessary to call this function if you only need to
> > + * serialise a pending powerdown with mcpu_cpu_power_up() or a wakeup
> 
> Ditto.
> 
> > + * event.
> > + *
> > + * Do not call this function unless the specified CPU has already
> > + * called mcpu_cpu_power_down() or has committed to doing so.
> 
> Ditto.

Ummm, duh.  Fixed -- thanks.

[...]

Cheers
---Dave

[RFC PATCH 3/3] ARM: vexpress/TC2: Implement MCPM power_down_finish()

[Dave Martin]

On Mon, Sep 30, 2013 at 01:14:38PM -0400, Nicolas Pitre wrote:
> On Mon, 30 Sep 2013, Dave Martin wrote:
> 
> > This patch implements the power_down_finish() method for TC2, to
> > enable the kernel to confirm when CPUs are safely powered down.
> > 
> > The information required for determining when a CPU is parked
> > cannot be obtained from any single place, so a few sources of
> > information must be combined:
> > 
> >   * mcpm_cpu_power_down() must be pending for the CPU, so that we
> >     don't get confused by false STANDBYWFI positives arising from
> >     CPUidle.  This is detected by waiting for the tc2_pm use count
> >     for the target CPU to reach 0.
> > 
> >   * Either the SPC must report that the CPU has asserted
> >     STANDBYWFI, or the TC2 tile's reset control logic must be
> >     holding the CPU in reset.
> > 
> >     Just checking for STANDBYWFI is not sufficient, because this
> >     signal is not latched when the the cluster is clamped off and
> >     powered down: the relevant status bits just drop to zero.  This
> >     means that STANDBYWFI status cannot be used for reliable
> >     detection of the last CPU in a cluster reaching WFI.
> > 
> > This patch is required in order for kexec to work with MCPM on TC2.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> > 
> > The mdelay(1) in tc2_pm_power_down_finish() is arbitrary.  The power
> > controller can show millisecond response times in the worst case, and
> > CPU hotplug is not expected to be performance-critical.
> > 
> > It may be wise to add a timeout to this function, but that's open to
> > discussion.
> 
> That would be a good idea.  I'd suggest you reduce the polling loop to 
> something like 10 ms and bail out after one second.   We've been 
> affected by funny STANDBYWFI 
> behaviors before.

OK, I'm happy to do that.  I don't have a strong opinion on the correct
answer here -- the main thing is to avoid thrashing the bus and wasting
power, so even quite a short delay will help considerably.

I'll just loop 100 times and then give up and return 0.

So far as I've seen, it's very unlikely in practice that repeated polling
is needed.  The most likely cause is that cluster powerdown involves a
lengthy drain of L2 within the blackout period.

Cheers
---Dave

[RFC PATCH 2/3] ARM: mcpm: Implement cpu_kill() to synchronise on powerdown

[Nicolas Pitre]

On Mon, 30 Sep 2013, Dave Martin wrote:

> On Mon, Sep 30, 2013 at 01:00:00PM -0400, Nicolas Pitre wrote:
> > On Mon, 30 Sep 2013, Dave Martin wrote:
> > 
> > > CPU hotplug and kexec rely on smp_ops.cpu_kill(), which is supposed
> > > to wait for the CPU to park or power down, and perform the last
> > > rites (such as disabling clocks etc., where the platform doesn't do
> > > this automatically).
> > > 
> > > kexec in particular is unsafe without performing this
> > > synchronisation to park secondaries.  Without it, the secondaries
> > > might not be parked when kexec trashes the kernel.
> > > 
> > > There is no generic way to do this synchronisation, so a new mcpm
> > > platform_ops method power_down_finish() is added by this patch.
> > > 
> > > The new method is mandatory.  A platform which provides no way to
> > > detect when CPUs are parked is likely broken.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm/common/mcpm_entry.c   |    5 +++++
> > >  arch/arm/common/mcpm_platsmp.c |   10 ++++++++++
> > >  arch/arm/include/asm/mcpm.h    |   31 +++++++++++++++++++++++++++++++
> > >  3 files changed, 46 insertions(+)
> > > 
> > > diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
> > > index 370236d..b722027 100644
> > > --- a/arch/arm/common/mcpm_entry.c
> > > +++ b/arch/arm/common/mcpm_entry.c
> > > @@ -89,6 +89,11 @@ void mcpm_cpu_power_down(void)
> > >  	BUG();
> > >  }
> > >  
> > > +int mcpm_cpu_power_down_finish(unsigned int cpu, unsigned int cluster)
> > > +{
> > > +	return platform_ops->power_down_finish(cpu, cluster);
> > > +}
> > 
> > Please make it return an error (non-zero return value) if platform_ops 
> > or platform_ops->power_down_finish is NULL.  Otherwise this would oops 
> > the kernel.
> > 
> > There is a patch already handling the other cases in RMK's patch system.
> 
> OK -- I didn't pay enough attention as that went past.
> 
> I'll grab that patch and rebase onto it.
> 
> 
> Do you have concerns about the mandatoriness of the new call?  The
> alternative would be to check for it at registration time and set
> smp_ops.cpu_kill to NULL if the backend method isn't there.
> But I really wouldn't want to encourage that until someone comes
> up with a good reason.

Agreed.  If for some reasons this method is unneeded on some platform 
then the backend just needs to return success unconditionally, ideally 
with a comment explaining why it is so.


Nicolas