[PATCH 0/5] Second round of MCPM refactoring

[PATCH 0/5] Second round of MCPM refactoring

[Nicolas Pitre]

This is the sequel to the first episode that started here:

	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353

Now that most of the first set has been merged upstream, here's the last
set to complete the cleanup. This mainly includes the removal of the
backward compatibility support plus some cosmetic changes.

The hisi04 backend, though, is instead converted to raw SMP operations
as it currently doesn't benefit from MCPM at all and doesn't fit well
with the new backend structure.

This series can also be obtained from the following Git repository:

	http://git.linaro.org/people/nicolas.pitre/linux.git mcpm

The diffstat shows more code removal again:

 arch/arm/common/mcpm_entry.c         | 281 +++++++++++++----------------
 arch/arm/include/asm/mcpm.h          |  73 +++-----
 arch/arm/mach-exynos/suspend.c       |   8 +-
 arch/arm/mach-hisi/platmcpm.c        | 127 +++++--------
 drivers/cpuidle/cpuidle-big_little.c |   8 +-
 5 files changed, 198 insertions(+), 299 deletions(-)

[PATCH 1/5] ARM: hisi/hip04: remove the MCPM overhead

[Nicolas Pitre]

This platform is currently relying on the MCPM infrastructure for no
apparent reason.  The MCPM concurrency handling brings no benefits here
as there is no asynchronous CPU wake-ups to be concerned about (this is
used for CPU hotplug and secondary boot only, not for CPU idle).

This platform is also different from the other MCPM users because a given
CPU can't shut itself down completely without the assistance of another
CPU. This is at odds with the on-going MCPM backend refactoring.

To simplify things, this is converted to hook directly into the
smp_operations callbacks, bypassing the MCPM infrastructure.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/mach-hisi/platmcpm.c | 127 ++++++++++++++----------------------------
 1 file changed, 42 insertions(+), 85 deletions(-)

diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c
index 280f3f14f7..880cbfa9c3 100644
--- a/arch/arm/mach-hisi/platmcpm.c
+++ b/arch/arm/mach-hisi/platmcpm.c
@@ -6,6 +6,8 @@
  * under the terms and conditions of the GNU General Public License,
  * version 2, as published by the Free Software Foundation.
  */
+#include <linux/init.h>
+#include <linux/smp.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/memblock.h>
@@ -13,7 +15,9 @@
 
 #include <asm/cputype.h>
 #include <asm/cp15.h>
-#include <asm/mcpm.h>
+#include <asm/cacheflush.h>
+#include <asm/smp.h>
+#include <asm/smp_plat.h>
 
 #include "core.h"
 
@@ -94,11 +98,16 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
 	} while (data != readl_relaxed(fabric + FAB_SF_MODE));
 }
 
-static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+static int hip04_boot_secondary(unsigned int l_cpu, struct task_struct *idle)
 {
+	unsigned int mpidr, cpu, cluster;
 	unsigned long data;
 	void __iomem *sys_dreq, *sys_status;
 
+	mpidr = cpu_logical_map(l_cpu);
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
 	if (!sysctrl)
 		return -ENODEV;
 	if (cluster >= HIP04_MAX_CLUSTERS || cpu >= HIP04_MAX_CPUS_PER_CLUSTER)
@@ -118,6 +127,7 @@ static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
 			cpu_relax();
 			data = readl_relaxed(sys_status);
 		} while (data & CLUSTER_DEBUG_RESET_STATUS);
+		hip04_set_snoop_filter(cluster, 1);
 	}
 
 	data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
@@ -126,11 +136,15 @@ static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
 	do {
 		cpu_relax();
 	} while (data == readl_relaxed(sys_status));
+
 	/*
 	 * We may fail to power up core again without this delay.
 	 * It's not mentioned in document. It's found by test.
 	 */
 	udelay(20);
+
+	arch_send_wakeup_ipi_mask(cpumask_of(l_cpu));
+
 out:
 	hip04_cpu_table[cluster][cpu]++;
 	spin_unlock_irq(&boot_lock);
@@ -138,31 +152,29 @@ out:
 	return 0;
 }
 
-static void hip04_mcpm_power_down(void)
+static void hip04_cpu_die(unsigned int l_cpu)
 {
 	unsigned int mpidr, cpu, cluster;
-	bool skip_wfi = false, last_man = false;
+	bool last_man;
 
-	mpidr = read_cpuid_mpidr();
+	mpidr = cpu_logical_map(l_cpu);
 	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
 
-	__mcpm_cpu_going_down(cpu, cluster);
-
 	spin_lock(&boot_lock);
-	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
 	hip04_cpu_table[cluster][cpu]--;
 	if (hip04_cpu_table[cluster][cpu] == 1) {
 		/* A power_up request went ahead of us. */
-		skip_wfi = true;
+		spin_unlock(&boot_lock);
+		return;
 	} else if (hip04_cpu_table[cluster][cpu] > 1) {
 		pr_err("Cluster %d CPU%d boots multiple times\n", cluster, cpu);
 		BUG();
 	}
 
 	last_man = hip04_cluster_is_down(cluster);
-	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
-		spin_unlock(&boot_lock);
+	spin_unlock(&boot_lock);
+	if (last_man) {
 		/* Since it's Cortex A15, disable L2 prefetching. */
 		asm volatile(
 		"mcr	p15, 1, %0, c15, c0, 3 \n\t"
@@ -170,34 +182,30 @@ static void hip04_mcpm_power_down(void)
 		"dsb	"
 		: : "r" (0x400) );
 		v7_exit_coherency_flush(all);
-		hip04_set_snoop_filter(cluster, 0);
-		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
 	} else {
-		spin_unlock(&boot_lock);
 		v7_exit_coherency_flush(louis);
 	}
 
-	__mcpm_cpu_down(cpu, cluster);
-
-	if (!skip_wfi)
+	for (;;)
 		wfi();
 }
 
-static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int cluster)
+static int hip04_cpu_kill(unsigned int l_cpu)
 {
+	unsigned int mpidr, cpu, cluster;
 	unsigned int data, tries, count;
-	int ret = -ETIMEDOUT;
 
+	mpidr = cpu_logical_map(l_cpu);
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
 	BUG_ON(cluster >= HIP04_MAX_CLUSTERS ||
 	       cpu >= HIP04_MAX_CPUS_PER_CLUSTER);
 
 	count = TIMEOUT_MSEC / POLL_MSEC;
 	spin_lock_irq(&boot_lock);
 	for (tries = 0; tries < count; tries++) {
-		if (hip04_cpu_table[cluster][cpu]) {
-			ret = -EBUSY;
+		if (hip04_cpu_table[cluster][cpu])
 			goto err;
-		}
 		cpu_relax();
 		data = readl_relaxed(sysctrl + SC_CPU_RESET_STATUS(cluster));
 		if (data & CORE_WFI_STATUS(cpu))
@@ -220,64 +228,19 @@ static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int cluster)
 	}
 	if (tries >= count)
 		goto err;
+	if (hip04_cluster_is_down(cluster))
+		hip04_set_snoop_filter(cluster, 0);
 	spin_unlock_irq(&boot_lock);
-	return 0;
+	return 1;
 err:
 	spin_unlock_irq(&boot_lock);
-	return ret;
-}
-
-static void hip04_mcpm_powered_up(void)
-{
-	unsigned int mpidr, cpu, cluster;
-
-	mpidr = read_cpuid_mpidr();
-	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
-	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
-
-	spin_lock(&boot_lock);
-	if (!hip04_cpu_table[cluster][cpu])
-		hip04_cpu_table[cluster][cpu] = 1;
-	spin_unlock(&boot_lock);
-}
-
-static void __naked hip04_mcpm_power_up_setup(unsigned int affinity_level)
-{
-	asm volatile ("			\n"
-"	cmp	r0, #0			\n"
-"	bxeq	lr			\n"
-	/* calculate fabric phys address */
-"	adr	r2, 2f			\n"
-"	ldmia	r2, {r1, r3}		\n"
-"	sub	r0, r2, r1		\n"
-"	ldr	r2, [r0, r3]		\n"
-	/* get cluster id from MPIDR */
-"	mrc	p15, 0, r0, c0, c0, 5	\n"
-"	ubfx	r1, r0, #8, #8		\n"
-	/* 1 << cluster id */
-"	mov	r0, #1			\n"
-"	mov	r3, r0, lsl r1		\n"
-"	ldr	r0, [r2, #"__stringify(FAB_SF_MODE)"]	\n"
-"	tst	r0, r3			\n"
-"	bxne	lr			\n"
-"	orr	r1, r0, r3		\n"
-"	str	r1, [r2, #"__stringify(FAB_SF_MODE)"]	\n"
-"1:	ldr	r0, [r2, #"__stringify(FAB_SF_MODE)"]	\n"
-"	tst	r0, r3			\n"
-"	beq	1b			\n"
-"	bx	lr			\n"
-
-"	.align	2			\n"
-"2:	.word	.			\n"
-"	.word	fabric_phys_addr	\n"
-	);
+	return 0;
 }
 
-static const struct mcpm_platform_ops hip04_mcpm_ops = {
-	.power_up		= hip04_mcpm_power_up,
-	.power_down		= hip04_mcpm_power_down,
-	.wait_for_powerdown	= hip04_mcpm_wait_for_powerdown,
-	.powered_up		= hip04_mcpm_powered_up,
+static struct smp_operations __initdata hip04_smp_ops = {
+	.smp_boot_secondary	= hip04_boot_secondary,
+	.cpu_die		= hip04_cpu_die,
+	.cpu_kill		= hip04_cpu_kill,
 };
 
 static bool __init hip04_cpu_table_init(void)
@@ -298,7 +261,7 @@ static bool __init hip04_cpu_table_init(void)
 	return true;
 }
 
-static int __init hip04_mcpm_init(void)
+static int __init hip04_smp_init(void)
 {
 	struct device_node *np, *np_sctl, *np_fab;
 	struct resource fab_res;
@@ -353,10 +316,6 @@ static int __init hip04_mcpm_init(void)
 		ret = -EINVAL;
 		goto err_table;
 	}
-	ret = mcpm_platform_register(&hip04_mcpm_ops);
-	if (ret) {
-		goto err_table;
-	}
 
 	/*
 	 * Fill the instruction address that is used after secondary core
@@ -364,13 +323,11 @@ static int __init hip04_mcpm_init(void)
 	 */
 	writel_relaxed(hip04_boot_method[0], relocation);
 	writel_relaxed(0xa5a5a5a5, relocation + 4);	/* magic number */
-	writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+	writel_relaxed(virt_to_phys(secondary_startup), relocation + 8);
 	writel_relaxed(0, relocation + 12);
 	iounmap(relocation);
 
-	mcpm_sync_init(hip04_mcpm_power_up_setup);
-	mcpm_smp_set_ops();
-	pr_info("HiP04 MCPM initialized\n");
+	smp_set_ops(&hip04_smp_ops);
 	return ret;
 err_table:
 	iounmap(fabric);
@@ -383,4 +340,4 @@ err_reloc:
 err:
 	return ret;
 }
-early_initcall(hip04_mcpm_init);
+early_initcall(hip04_smp_init);
-- 
2.1.0

[PATCH 2/5] ARM: MCPM: remove backward compatibility code

[Nicolas Pitre]

Now that no one uses the old callbacks anymore, let's remove them
and associated support code.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/mcpm_entry.c | 45 +++++---------------------------------------
 arch/arm/include/asm/mcpm.h  |  6 ------
 2 files changed, 5 insertions(+), 46 deletions(-)

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index 5f8a52ac7e..0908f96278 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -78,16 +78,11 @@ int mcpm_cpu_power_up(unsigned int cpu, unsigned int cluster)
 	bool cpu_is_down, cluster_is_down;
 	int ret = 0;
 
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
 	if (!platform_ops)
 		return -EUNATCH; /* try not to shadow power_up errors */
 	might_sleep();
 
-	/* backward compatibility callback */
-	if (platform_ops->power_up)
-		return platform_ops->power_up(cpu, cluster);
-
-	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
-
 	/*
 	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
 	 * variant exists, we need to disable IRQs manually here.
@@ -128,29 +123,17 @@ void mcpm_cpu_power_down(void)
 	bool cpu_going_down, last_man;
 	phys_reset_t phys_reset;
 
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
 	if (WARN_ON_ONCE(!platform_ops))
 	       return;
 	BUG_ON(!irqs_disabled());
 
-	/*
-	 * Do this before calling into the power_down method,
-	 * as it might not always be safe to do afterwards.
-	 */
 	setup_mm_for_reboot();
 
-	/* backward compatibility callback */
-	if (platform_ops->power_down) {
-		platform_ops->power_down();
-		goto not_dead;
-	}
-
-	mpidr = read_cpuid_mpidr();
-	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
-	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
-	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
-
 	__mcpm_cpu_going_down(cpu, cluster);
-
 	arch_spin_lock(&mcpm_lock);
 	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
 
@@ -187,7 +170,6 @@ void mcpm_cpu_power_down(void)
 	if (cpu_going_down)
 		wfi();
 
-not_dead:
 	/*
 	 * It is possible for a power_up request to happen concurrently
 	 * with a power_down request for the same CPU. In this case the
@@ -224,17 +206,6 @@ void mcpm_cpu_suspend(u64 expected_residency)
 	if (WARN_ON_ONCE(!platform_ops))
 		return;
 
-	/* backward compatibility callback */
-	if (platform_ops->suspend) {
-		phys_reset_t phys_reset;
-		BUG_ON(!irqs_disabled());
-		setup_mm_for_reboot();
-		platform_ops->suspend(expected_residency);
-		phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
-		phys_reset(virt_to_phys(mcpm_entry_point));
-		BUG();
-	}
-
 	/* Some platforms might have to enable special resume modes, etc. */
 	if (platform_ops->cpu_suspend_prepare) {
 		unsigned int mpidr = read_cpuid_mpidr();
@@ -256,12 +227,6 @@ int mcpm_cpu_powered_up(void)
 	if (!platform_ops)
 		return -EUNATCH;
 
-	/* backward compatibility callback */
-	if (platform_ops->powered_up) {
-		platform_ops->powered_up();
-		return 0;
-	}
-
 	mpidr = read_cpuid_mpidr();
 	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
index 50b378f59e..e2118c941d 100644
--- a/arch/arm/include/asm/mcpm.h
+++ b/arch/arm/include/asm/mcpm.h
@@ -234,12 +234,6 @@ struct mcpm_platform_ops {
 	void (*cpu_is_up)(unsigned int cpu, unsigned int cluster);
 	void (*cluster_is_up)(unsigned int cluster);
 	int (*wait_for_powerdown)(unsigned int cpu, unsigned int cluster);
-
-	/* deprecated callbacks */
-	int (*power_up)(unsigned int cpu, unsigned int cluster);
-	void (*power_down)(void);
-	void (*suspend)(u64);
-	void (*powered_up)(void);
 };
 
 /**
-- 
2.1.0

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Nicolas Pitre]

This concerns the following helpers:

	__mcpm_cpu_going_down()
	__mcpm_cpu_down()
	__mcpm_outbound_enter_critical()
	__mcpm_outbound_leave_critical()
	__mcpm_cluster_state()

They are and should only be used by the core code now.  Therefore their
declarations are removed from mcpm.h and their definitions are made
static, hence the need to move them before their users which accounts
for the bulk of this patch.

This left the mcpm_sync_struct definition at an odd location, therefore
it is moved as well with some comment clarifications.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/mcpm_entry.c | 229 ++++++++++++++++++++++---------------------
 arch/arm/include/asm/mcpm.h  |  52 +++++-----
 2 files changed, 138 insertions(+), 143 deletions(-)

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index 0908f96278..c5fe2e33e6 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -20,6 +20,121 @@
 #include <asm/cputype.h>
 #include <asm/suspend.h>
 
+
+struct sync_struct mcpm_sync;
+
+/*
+ * __mcpm_cpu_going_down: Indicates that the cpu is being torn down.
+ *    This must be called at the point of committing to teardown of a CPU.
+ *    The CPU cache (SCTRL.C bit) is expected to still be active.
+ */
+static void __mcpm_cpu_going_down(unsigned int cpu, unsigned int cluster)
+{
+	mcpm_sync.clusters[cluster].cpus[cpu].cpu = CPU_GOING_DOWN;
+	sync_cache_w(&mcpm_sync.clusters[cluster].cpus[cpu].cpu);
+}
+
+/*
+ * __mcpm_cpu_down: Indicates that cpu teardown is complete and that the
+ *    cluster can be torn down without disrupting this CPU.
+ *    To avoid deadlocks, this must be called before a CPU is powered down.
+ *    The CPU cache (SCTRL.C bit) is expected to be off.
+ *    However L2 cache might or might not be active.
+ */
+static void __mcpm_cpu_down(unsigned int cpu, unsigned int cluster)
+{
+	dmb();
+	mcpm_sync.clusters[cluster].cpus[cpu].cpu = CPU_DOWN;
+	sync_cache_w(&mcpm_sync.clusters[cluster].cpus[cpu].cpu);
+	sev();
+}
+
+/*
+ * __mcpm_outbound_leave_critical: Leave the cluster teardown critical section.
+ * @state: the final state of the cluster:
+ *     CLUSTER_UP: no destructive teardown was done and the cluster has been
+ *         restored to the previous state (CPU cache still active); or
+ *     CLUSTER_DOWN: the cluster has been torn-down, ready for power-off
+ *         (CPU cache disabled, L2 cache either enabled or disabled).
+ */
+static void __mcpm_outbound_leave_critical(unsigned int cluster, int state)
+{
+	dmb();
+	mcpm_sync.clusters[cluster].cluster = state;
+	sync_cache_w(&mcpm_sync.clusters[cluster].cluster);
+	sev();
+}
+
+/*
+ * __mcpm_outbound_enter_critical: Enter the cluster teardown critical section.
+ * This function should be called by the last man, after local CPU teardown
+ * is complete.  CPU cache expected to be active.
+ *
+ * Returns:
+ *     false: the critical section was not entered because an inbound CPU was
+ *         observed, or the cluster is already being set up;
+ *     true: the critical section was entered: it is now safe to tear down the
+ *         cluster.
+ */
+static bool __mcpm_outbound_enter_critical(unsigned int cpu, unsigned int cluster)
+{
+	unsigned int i;
+	struct mcpm_sync_struct *c = &mcpm_sync.clusters[cluster];
+
+	/* Warn inbound CPUs that the cluster is being torn down: */
+	c->cluster = CLUSTER_GOING_DOWN;
+	sync_cache_w(&c->cluster);
+
+	/* Back out if the inbound cluster is already in the critical region: */
+	sync_cache_r(&c->inbound);
+	if (c->inbound == INBOUND_COMING_UP)
+		goto abort;
+
+	/*
+	 * Wait for all CPUs to get out of the GOING_DOWN state, so that local
+	 * teardown is complete on each CPU before tearing down the cluster.
+	 *
+	 * If any CPU has been woken up again from the DOWN state, then we
+	 * shouldn't be taking the cluster down at all: abort in that case.
+	 */
+	sync_cache_r(&c->cpus);
+	for (i = 0; i < MAX_CPUS_PER_CLUSTER; i++) {
+		int cpustate;
+
+		if (i == cpu)
+			continue;
+
+		while (1) {
+			cpustate = c->cpus[i].cpu;
+			if (cpustate != CPU_GOING_DOWN)
+				break;
+
+			wfe();
+			sync_cache_r(&c->cpus[i].cpu);
+		}
+
+		switch (cpustate) {
+		case CPU_DOWN:
+			continue;
+
+		default:
+			goto abort;
+		}
+	}
+
+	return true;
+
+abort:
+	__mcpm_outbound_leave_critical(cluster, CLUSTER_UP);
+	return false;
+}
+
+static int __mcpm_cluster_state(unsigned int cluster)
+{
+	sync_cache_r(&mcpm_sync.clusters[cluster].cluster);
+	return mcpm_sync.clusters[cluster].cluster;
+}
+
 extern unsigned long mcpm_entry_vectors[MAX_NR_CLUSTERS][MAX_CPUS_PER_CLUSTER];
 
 void mcpm_set_entry_vector(unsigned cpu, unsigned cluster, void *ptr)
@@ -299,120 +414,6 @@ int __init mcpm_loopback(void (*cache_disable)(void))
 
 #endif
 
-struct sync_struct mcpm_sync;
-
-/*
- * __mcpm_cpu_going_down: Indicates that the cpu is being torn down.
- *    This must be called at the point of committing to teardown of a CPU.
- *    The CPU cache (SCTRL.C bit) is expected to still be active.
- */
-void __mcpm_cpu_going_down(unsigned int cpu, unsigned int cluster)
-{
-	mcpm_sync.clusters[cluster].cpus[cpu].cpu = CPU_GOING_DOWN;
-	sync_cache_w(&mcpm_sync.clusters[cluster].cpus[cpu].cpu);
-}
-
-/*
- * __mcpm_cpu_down: Indicates that cpu teardown is complete and that the
- *    cluster can be torn down without disrupting this CPU.
- *    To avoid deadlocks, this must be called before a CPU is powered down.
- *    The CPU cache (SCTRL.C bit) is expected to be off.
- *    However L2 cache might or might not be active.
- */
-void __mcpm_cpu_down(unsigned int cpu, unsigned int cluster)
-{
-	dmb();
-	mcpm_sync.clusters[cluster].cpus[cpu].cpu = CPU_DOWN;
-	sync_cache_w(&mcpm_sync.clusters[cluster].cpus[cpu].cpu);
-	sev();
-}
-
-/*
- * __mcpm_outbound_leave_critical: Leave the cluster teardown critical section.
- * @state: the final state of the cluster:
- *     CLUSTER_UP: no destructive teardown was done and the cluster has been
- *         restored to the previous state (CPU cache still active); or
- *     CLUSTER_DOWN: the cluster has been torn-down, ready for power-off
- *         (CPU cache disabled, L2 cache either enabled or disabled).
- */
-void __mcpm_outbound_leave_critical(unsigned int cluster, int state)
-{
-	dmb();
-	mcpm_sync.clusters[cluster].cluster = state;
-	sync_cache_w(&mcpm_sync.clusters[cluster].cluster);
-	sev();
-}
-
-/*
- * __mcpm_outbound_enter_critical: Enter the cluster teardown critical section.
- * This function should be called by the last man, after local CPU teardown
- * is complete.  CPU cache expected to be active.
- *
- * Returns:
- *     false: the critical section was not entered because an inbound CPU was
- *         observed, or the cluster is already being set up;
- *     true: the critical section was entered: it is now safe to tear down the
- *         cluster.
- */
-bool __mcpm_outbound_enter_critical(unsigned int cpu, unsigned int cluster)
-{
-	unsigned int i;
-	struct mcpm_sync_struct *c = &mcpm_sync.clusters[cluster];
-
-	/* Warn inbound CPUs that the cluster is being torn down: */
-	c->cluster = CLUSTER_GOING_DOWN;
-	sync_cache_w(&c->cluster);
-
-	/* Back out if the inbound cluster is already in the critical region: */
-	sync_cache_r(&c->inbound);
-	if (c->inbound == INBOUND_COMING_UP)
-		goto abort;
-
-	/*
-	 * Wait for all CPUs to get out of the GOING_DOWN state, so that local
-	 * teardown is complete on each CPU before tearing down the cluster.
-	 *
-	 * If any CPU has been woken up again from the DOWN state, then we
-	 * shouldn't be taking the cluster down at all: abort in that case.
-	 */
-	sync_cache_r(&c->cpus);
-	for (i = 0; i < MAX_CPUS_PER_CLUSTER; i++) {
-		int cpustate;
-
-		if (i == cpu)
-			continue;
-
-		while (1) {
-			cpustate = c->cpus[i].cpu;
-			if (cpustate != CPU_GOING_DOWN)
-				break;
-
-			wfe();
-			sync_cache_r(&c->cpus[i].cpu);
-		}
-
-		switch (cpustate) {
-		case CPU_DOWN:
-			continue;
-
-		default:
-			goto abort;
-		}
-	}
-
-	return true;
-
-abort:
-	__mcpm_outbound_leave_critical(cluster, CLUSTER_UP);
-	return false;
-}
-
-int __mcpm_cluster_state(unsigned int cluster)
-{
-	sync_cache_r(&mcpm_sync.clusters[cluster].cluster);
-	return mcpm_sync.clusters[cluster].cluster;
-}
-
 extern unsigned long mcpm_power_up_setup_phys;
 
 int __init mcpm_sync_init(
diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
index e2118c941d..6a40d5f8db 100644
--- a/arch/arm/include/asm/mcpm.h
+++ b/arch/arm/include/asm/mcpm.h
@@ -245,35 +245,6 @@ struct mcpm_platform_ops {
  */
 int __init mcpm_platform_register(const struct mcpm_platform_ops *ops);
 
-/* Synchronisation structures for coordinating safe cluster setup/teardown: */
-
-/*
- * When modifying this structure, make sure you update the MCPM_SYNC_ defines
- * to match.
- */
-struct mcpm_sync_struct {
-	/* individual CPU states */
-	struct {
-		s8 cpu __aligned(__CACHE_WRITEBACK_GRANULE);
-	} cpus[MAX_CPUS_PER_CLUSTER];
-
-	/* cluster state */
-	s8 cluster __aligned(__CACHE_WRITEBACK_GRANULE);
-
-	/* inbound-side state */
-	s8 inbound __aligned(__CACHE_WRITEBACK_GRANULE);
-};
-
-struct sync_struct {
-	struct mcpm_sync_struct clusters[MAX_NR_CLUSTERS];
-};
-
-void __mcpm_cpu_going_down(unsigned int cpu, unsigned int cluster);
-void __mcpm_cpu_down(unsigned int cpu, unsigned int cluster);
-void __mcpm_outbound_leave_critical(unsigned int cluster, int state);
-bool __mcpm_outbound_enter_critical(unsigned int this_cpu, unsigned int cluster);
-int __mcpm_cluster_state(unsigned int cluster);
-
 /**
  * mcpm_sync_init - Initialize the cluster synchronization support
  *
@@ -312,6 +283,29 @@ int __init mcpm_loopback(void (*cache_disable)(void));
 
 void __init mcpm_smp_set_ops(void);
 
+/*
+ * Synchronisation structures for coordinating safe cluster setup/teardown.
+ * This is private to the MCPM core code and shared between C and assembly.
+ * When modifying this structure, make sure you update the MCPM_SYNC_ defines
+ * to match.
+ */
+struct mcpm_sync_struct {
+	/* individual CPU states */
+	struct {
+		s8 cpu __aligned(__CACHE_WRITEBACK_GRANULE);
+	} cpus[MAX_CPUS_PER_CLUSTER];
+
+	/* cluster state */
+	s8 cluster __aligned(__CACHE_WRITEBACK_GRANULE);
+
+	/* inbound-side state */
+	s8 inbound __aligned(__CACHE_WRITEBACK_GRANULE);
+};
+
+struct sync_struct {
+	struct mcpm_sync_struct clusters[MAX_NR_CLUSTERS];
+};
+
 #else
 
 /* 
-- 
2.1.0

[PATCH 4/5] ARM: MCPM: add references to the available documentation in the code

[Nicolas Pitre]

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/mcpm_entry.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index c5fe2e33e6..492467587c 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -20,6 +20,11 @@
 #include <asm/cputype.h>
 #include <asm/suspend.h>
 
+/*
+ * The public API for this code is documented in arch/arm/include/asm/mcpm.h.
+ * For a comprehensive description of the main algorithm used here, please
+ * see Documentation/arm/cluster-pm-race-avoidance.txt.
+ */
 
 struct sync_struct mcpm_sync;
 
-- 
2.1.0

[PATCH 5/5] ARM: MCPM: remove residency argument from mcpm_cpu_suspend()

[Nicolas Pitre]

This is currently unused.

If a suspend must be limited to CPU level only by preventing the last man
from triggering a cluster level suspend then this should be determined
according to many other criteria the MCPM layer is currently not aware of.
It is unlikely that mcpm_cpu_suspend() would be the proper conduit for
that information anyway.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/mcpm_entry.c         |  2 +-
 arch/arm/include/asm/mcpm.h          | 15 +++++----------
 arch/arm/mach-exynos/suspend.c       |  8 +-------
 drivers/cpuidle/cpuidle-big_little.c |  8 +-------
 4 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index 492467587c..a923524d10 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -321,7 +321,7 @@ int mcpm_wait_for_cpu_powerdown(unsigned int cpu, unsigned int cluster)
 	return ret;
 }
 
-void mcpm_cpu_suspend(u64 expected_residency)
+void mcpm_cpu_suspend(void)
 {
 	if (WARN_ON_ONCE(!platform_ops))
 		return;
diff --git a/arch/arm/include/asm/mcpm.h b/arch/arm/include/asm/mcpm.h
index 6a40d5f8db..acd4983d9b 100644
--- a/arch/arm/include/asm/mcpm.h
+++ b/arch/arm/include/asm/mcpm.h
@@ -137,17 +137,12 @@ int mcpm_wait_for_cpu_powerdown(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
- *			to remain suspended, or 0 if unknown/infinity.
- *
- * The calling CPU is suspended.  The expected residency argument is used
- * as a hint by the platform specific backend to implement the appropriate
- * sleep state level according to the knowledge it has on wake-up latency
- * for the given hardware.
+ * The calling CPU is suspended.  This is similar to mcpm_cpu_power_down()
+ * except for possible extra platform specific configuration steps to allow
+ * an asynchronous wake-up e.g. with a pending interrupt.
  *
  * If this CPU is found to be the "last man standing" in the cluster
- * then the cluster may be prepared for power-down too, if the expected
- * residency makes it worthwhile.
+ * then the cluster may be prepared for power-down too.
  *
  * This must be called with interrupts disabled.
  *
@@ -157,7 +152,7 @@ int mcpm_wait_for_cpu_powerdown(unsigned int cpu, unsigned int cluster);
  * This will return if mcpm_platform_register() has not been called
  * previously in which case the caller should take appropriate action.
  */
-void mcpm_cpu_suspend(u64 expected_residency);
+void mcpm_cpu_suspend(void);
 
 /**
  * mcpm_cpu_powered_up - housekeeping workafter a CPU has been powered up
diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index 3e6aea7f83..372bd0b099 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -311,13 +311,7 @@ static int exynos5420_cpu_suspend(unsigned long arg)
 
 	if (IS_ENABLED(CONFIG_EXYNOS5420_MCPM)) {
 		mcpm_set_entry_vector(cpu, cluster, exynos_cpu_resume);
-
-		/*
-		 * Residency value passed to mcpm_cpu_suspend back-end
-		 * has to be given clear semantics. Set to 0 as a
-		 * temporary value.
-		 */
-		mcpm_cpu_suspend(0);
+		mcpm_cpu_suspend();
 	}
 
 	pr_info("Failed to suspend the system\n");
diff --git a/drivers/cpuidle/cpuidle-big_little.c b/drivers/cpuidle/cpuidle-big_little.c
index 40c34faffe..db2ede565f 100644
--- a/drivers/cpuidle/cpuidle-big_little.c
+++ b/drivers/cpuidle/cpuidle-big_little.c
@@ -108,13 +108,7 @@ static int notrace bl_powerdown_finisher(unsigned long arg)
 	unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 
 	mcpm_set_entry_vector(cpu, cluster, cpu_resume);
-
-	/*
-	 * Residency value passed to mcpm_cpu_suspend back-end
-	 * has to be given clear semantics. Set to 0 as a
-	 * temporary value.
-	 */
-	mcpm_cpu_suspend(0);
+	mcpm_cpu_suspend();
 
 	/* return value != 0 means failure */
 	return 1;
-- 
2.1.0

[PATCH 0/5] Second round of MCPM refactoring

[Russell King - ARM Linux]

On Fri, May 01, 2015 at 12:06:27PM -0400, Nicolas Pitre wrote:
> This is the sequel to the first episode that started here:
> 
> 	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353
> 
> Now that most of the first set has been merged upstream, here's the last
> set to complete the cleanup. This mainly includes the removal of the
> backward compatibility support plus some cosmetic changes.
> 
> The hisi04 backend, though, is instead converted to raw SMP operations
> as it currently doesn't benefit from MCPM at all and doesn't fit well
> with the new backend structure.

Looks like all good stuff.  What's the merging plan for this?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 0/5] Second round of MCPM refactoring

[Nicolas Pitre]

On Fri, 1 May 2015, Russell King - ARM Linux wrote:

> On Fri, May 01, 2015 at 12:06:27PM -0400, Nicolas Pitre wrote:
> > This is the sequel to the first episode that started here:
> > 
> > 	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353
> > 
> > Now that most of the first set has been merged upstream, here's the last
> > set to complete the cleanup. This mainly includes the removal of the
> > backward compatibility support plus some cosmetic changes.
> > 
> > The hisi04 backend, though, is instead converted to raw SMP operations
> > as it currently doesn't benefit from MCPM at all and doesn't fit well
> > with the new backend structure.
> 
> Looks like all good stuff.  What's the merging plan for this?

I'm not sure yet.  Except for the hisi04 patch, this is all generic 
stuff.  That could go upstream either via your tree or arm-soc I 
suppose.

Of course if new MCPM-using platforms are to be merged this cycle, 
they should be using the new interface. But that's already in mainline 
so that doesn't depend on this series.

What do you think?


Nicolas

[PATCH 0/5] Second round of MCPM refactoring

[Wei Xu]

On 5/1/2015 5:06 PM, Nicolas Pitre wrote:
> This is the sequel to the first episode that started here:
> 
> 	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353
> 
> Now that most of the first set has been merged upstream, here's the last
> set to complete the cleanup. This mainly includes the removal of the
> backward compatibility support plus some cosmetic changes.
> 
> The hisi04 backend, though, is instead converted to raw SMP operations
> as it currently doesn't benefit from MCPM at all and doesn't fit well
> with the new backend structure.
> 
> This series can also be obtained from the following Git repository:
> 
> 	http://git.linaro.org/people/nicolas.pitre/linux.git mcpm
> 
> The diffstat shows more code removal again:
> 
>  arch/arm/common/mcpm_entry.c         | 281 +++++++++++++----------------
>  arch/arm/include/asm/mcpm.h          |  73 +++-----
>  arch/arm/mach-exynos/suspend.c       |   8 +-
>  arch/arm/mach-hisi/platmcpm.c        | 127 +++++--------
>  drivers/cpuidle/cpuidle-big_little.c |   8 +-
>  5 files changed, 198 insertions(+), 299 deletions(-)
> 
> .
> 

Tested by: Wei Xu <xuwei5@hisilicon.com>

Built and booted all the 16 cores on D01 board based on v4.1-rc2.
And confirmed CPU hotplug works well on D01 board.

Thanks!

Best Regards,
Wei

[PATCH 0/5] Second round of MCPM refactoring

[Dave P Martin]

On Fri, May 01, 2015 at 05:06:27PM +0100, Nicolas Pitre wrote:
> This is the sequel to the first episode that started here:
> 
> 	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353
> 
> Now that most of the first set has been merged upstream, here's the last
> set to complete the cleanup. This mainly includes the removal of the
> backward compatibility support plus some cosmetic changes.
> 
> The hisi04 backend, though, is instead converted to raw SMP operations
> as it currently doesn't benefit from MCPM at all and doesn't fit well
> with the new backend structure.
> 
> This series can also be obtained from the following Git repository:
> 
> 	http://git.linaro.org/people/nicolas.pitre/linux.git mcpm
> 
> The diffstat shows more code removal again:

Good stuff.

FWIW: except for the hip04 patch which I'll leave to the relevant
maintainer,

Acked-by: Dave Martin <Dave.Martin@arm.com>

I have not reviewed this stuff in detail, but it looks like sensible
cleanup to me.

Cheers
---Dave

> 
>  arch/arm/common/mcpm_entry.c         | 281 +++++++++++++----------------
>  arch/arm/include/asm/mcpm.h          |  73 +++-----
>  arch/arm/mach-exynos/suspend.c       |   8 +-
>  arch/arm/mach-hisi/platmcpm.c        | 127 +++++--------
>  drivers/cpuidle/cpuidle-big_little.c |   8 +-
>  5 files changed, 198 insertions(+), 299 deletions(-)
> 

[PATCH 0/5] Second round of MCPM refactoring

[Nicolas Pitre]

On Wed, 6 May 2015, Wei Xu wrote:

> 
> 
> On 5/1/2015 5:06 PM, Nicolas Pitre wrote:
> > This is the sequel to the first episode that started here:
> > 
> > 	http://article.gmane.org/gmane.linux.ports.arm.kernel/401353
> > 
> > Now that most of the first set has been merged upstream, here's the last
> > set to complete the cleanup. This mainly includes the removal of the
> > backward compatibility support plus some cosmetic changes.
> > 
> > The hisi04 backend, though, is instead converted to raw SMP operations
> > as it currently doesn't benefit from MCPM at all and doesn't fit well
> > with the new backend structure.
> 
> Tested by: Wei Xu <xuwei5@hisilicon.com>
> 
> Built and booted all the 16 cores on D01 board based on v4.1-rc2.
> And confirmed CPU hotplug works well on D01 board.

Thanks for testing.


Nicolas

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Chen-Yu Tsai]

Hi,

On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> This concerns the following helpers:
>
>         __mcpm_cpu_going_down()
>         __mcpm_cpu_down()
>         __mcpm_outbound_enter_critical()
>         __mcpm_outbound_leave_critical()
>         __mcpm_cluster_state()
>
> They are and should only be used by the core code now.  Therefore their
> declarations are removed from mcpm.h and their definitions are made
> static, hence the need to move them before their users which accounts
> for the bulk of this patch.

I'm looking for some advice. On the Allwinner A80, at least on mainline,
there is no external PMU or embedded controller in charge of power
controls. What this means is that I'm doing power sequencing in the
kernel as part of the MCPM calls, specifically powering down cores and
clusters in the .wait_for_powerdown callback. (I don't think it's
reasonable or even possible to power down stuff in .*_powerdown_prepare)

Previously I was using __mcpm_cluster_state() to check if the last core
in a cluster was to be powered off, and thus the whole cluster could be
turned off as well. I could also check if the individual power gates or
resets are asserted, but if a core was already scheduled to be brought
up, and MCPM common framework didn't call .cluster_powerup, there might
be a problem.

Any suggestions? Maybe export __mcpm_cluster_state() so platform code
can know what's going to happen?

Thanks


Regards
ChenYu

> This left the mcpm_sync_struct definition at an odd location, therefore
> it is moved as well with some comment clarifications.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Dave Martin]

On Fri, Jul 24, 2015 at 11:54:18AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > This concerns the following helpers:
> >
> >         __mcpm_cpu_going_down()
> >         __mcpm_cpu_down()
> >         __mcpm_outbound_enter_critical()
> >         __mcpm_outbound_leave_critical()
> >         __mcpm_cluster_state()
> >
> > They are and should only be used by the core code now.  Therefore their
> > declarations are removed from mcpm.h and their definitions are made
> > static, hence the need to move them before their users which accounts
> > for the bulk of this patch.
> 
> I'm looking for some advice. On the Allwinner A80, at least on mainline,
> there is no external PMU or embedded controller in charge of power
> controls. What this means is that I'm doing power sequencing in the
> kernel as part of the MCPM calls, specifically powering down cores and
> clusters in the .wait_for_powerdown callback. (I don't think it's
> reasonable or even possible to power down stuff in .*_powerdown_prepare)
> 
> Previously I was using __mcpm_cluster_state() to check if the last core
> in a cluster was to be powered off, and thus the whole cluster could be
> turned off as well. I could also check if the individual power gates or
> resets are asserted, but if a core was already scheduled to be brought
> up, and MCPM common framework didn't call .cluster_powerup, there might
> be a problem.

It's been a while since I looked at this stuff in detail, so Nico
may want to put me right ... but here's my 2 cents:


__mcpm_cluster_state() should be considered an implementation detail of
mcpm.  If you feel you need to call it from your driver code, that's
a warning that your code is probably racy -- unless you can explain
otherwise.

When you say you have no external power controller, does this mean
that you need to poll in Linux for the affected cpus/clusters to reach
WFI and then hit the relevant clamps, clocks and/or regulators
yourself?

If so you're effectively implementing a multithreaded power controller
in software, suggesting that you need some state tracking and locking
between the various mcpm methods in your code.  However, you can use
the normal locking and atomics APIs for this.

Since the MCPM lock is held when calling all the relevant methods
except for wait_for_powerdown(), this nearly satisfies the locking
requirement by itself.  You are correct that the hardware operations
associated with physically powering things down will need to be
done in wait_for_powerdown() though, and the MCPM lock is not held
when calling that.


This would motivate two solutions that I can see:

 a) Expose the mcpm lock so that driver code can lock/unlock it
    during critical regions in wait_for_powerdown().

 b) Refactor wait_for_powerdown() so that the locking, sleeping
    and timeout code is moved into the mcpm generic code, and
    the wait_for_powerdown() method is replaced with something
    like "cpu_is_powered_down()".


Further to this, this "software power controller" really just maps one
power sequencing model onto another, so there's a chance it could be
a generic mcpm driver than can be reused across multiple platforms.

Cheers
---Dave

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Nicolas Pitre]

On Fri, 24 Jul 2015, Chen-Yu Tsai wrote:

> Hi,
> 
> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > This concerns the following helpers:
> >
> >         __mcpm_cpu_going_down()
> >         __mcpm_cpu_down()
> >         __mcpm_outbound_enter_critical()
> >         __mcpm_outbound_leave_critical()
> >         __mcpm_cluster_state()
> >
> > They are and should only be used by the core code now.  Therefore their
> > declarations are removed from mcpm.h and their definitions are made
> > static, hence the need to move them before their users which accounts
> > for the bulk of this patch.
> 
> I'm looking for some advice. On the Allwinner A80, at least on mainline,
> there is no external PMU or embedded controller in charge of power
> controls. What this means is that I'm doing power sequencing in the
> kernel as part of the MCPM calls, specifically powering down cores and
> clusters in the .wait_for_powerdown callback. (I don't think it's
> reasonable or even possible to power down stuff in .*_powerdown_prepare)

Can you tell me more about the power control knobs at your disposal?  Do 
power gates become effective immediately or only when WFI is asserted? 

And can you configure things so a core may be powered up asynchronously 
from an IRQ?

> Previously I was using __mcpm_cluster_state() to check if the last core
> in a cluster was to be powered off, and thus the whole cluster could be
> turned off as well.
> I could also check if the individual power gates or
> resets are asserted, but if a core was already scheduled to be brought
> up, and MCPM common framework didn't call .cluster_powerup, there might
> be a problem.

I fail to see how a core could be scheduled to be brought up without 
deasserting its reset line somehow though.

> Any suggestions? Maybe export __mcpm_cluster_state() so platform code
> can know what's going to happen?

The cluster state may change unexpectedly.  There is a special locking 
sequence and state machine needed to make this information reliable.  
Simply returning the current state wouldn't be enough to ensure 
it can be used race free.

As Dave stated, we might have to supplement the MCPM core code with 
special methods involving a surviving CPU to perform the power-down 
operation on the dying CPU's behalf.  Doing this in .wait_for_powerdown 
is just an abuse of the API.

It also brings up the question if MCPM is actually necessary in that 
case or if you can do without its complexity.  For example, you may look 
at commit 905cdf9dda5d for such a case.  It mainly depends on whether or 
not cores (and the cluster) may be awaken asynchronously upon assertion 
of an IRQ in the context of cpuidle. If the hardware doesn't support 
that then MCPM doesn't bring you any actual benefit.

So it depends on your hardware capabilities.


Nicolas

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Chen-Yu Tsai]

On Fri, Jul 24, 2015 at 7:15 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> On Fri, Jul 24, 2015 at 11:54:18AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > This concerns the following helpers:
>> >
>> >         __mcpm_cpu_going_down()
>> >         __mcpm_cpu_down()
>> >         __mcpm_outbound_enter_critical()
>> >         __mcpm_outbound_leave_critical()
>> >         __mcpm_cluster_state()
>> >
>> > They are and should only be used by the core code now.  Therefore their
>> > declarations are removed from mcpm.h and their definitions are made
>> > static, hence the need to move them before their users which accounts
>> > for the bulk of this patch.
>>
>> I'm looking for some advice. On the Allwinner A80, at least on mainline,
>> there is no external PMU or embedded controller in charge of power
>> controls. What this means is that I'm doing power sequencing in the
>> kernel as part of the MCPM calls, specifically powering down cores and
>> clusters in the .wait_for_powerdown callback. (I don't think it's
>> reasonable or even possible to power down stuff in .*_powerdown_prepare)
>>
>> Previously I was using __mcpm_cluster_state() to check if the last core
>> in a cluster was to be powered off, and thus the whole cluster could be
>> turned off as well. I could also check if the individual power gates or
>> resets are asserted, but if a core was already scheduled to be brought
>> up, and MCPM common framework didn't call .cluster_powerup, there might
>> be a problem.
>
> It's been a while since I looked at this stuff in detail, so Nico
> may want to put me right ... but here's my 2 cents:
>
>
> __mcpm_cluster_state() should be considered an implementation detail of
> mcpm.  If you feel you need to call it from your driver code, that's
> a warning that your code is probably racy -- unless you can explain
> otherwise.
>
> When you say you have no external power controller, does this mean
> that you need to poll in Linux for the affected cpus/clusters to reach
> WFI and then hit the relevant clamps, clocks and/or regulators
> yourself?

That's right. Polling a specific register to check for WFI, and gating
the cores. Clocks and external regulators haven't been implemented yet.

(I've also run into a problem where the cores in cluster 0 don't stay
 in WFI, but I think that's probably an implementation bug on my end.)

> If so you're effectively implementing a multithreaded power controller
> in software, suggesting that you need some state tracking and locking
> between the various mcpm methods in your code.  However, you can use
> the normal locking and atomics APIs for this.
>
> Since the MCPM lock is held when calling all the relevant methods
> except for wait_for_powerdown(), this nearly satisfies the locking
> requirement by itself.  You are correct that the hardware operations
> associated with physically powering things down will need to be
> done in wait_for_powerdown() though, and the MCPM lock is not held
> when calling that.

This was my reason for choosing MCPM, not having to handle all the
locking myself.

> This would motivate two solutions that I can see:
>
>  a) Expose the mcpm lock so that driver code can lock/unlock it
>     during critical regions in wait_for_powerdown().

>From a users' standpoint, having to do the locking myself is not
so appealing, as the interactions with the rest of the framework
is not as clear, and an overall view of the framework is lacking
for now.

>  b) Refactor wait_for_powerdown() so that the locking, sleeping
>     and timeout code is moved into the mcpm generic code, and
>     the wait_for_powerdown() method is replaced with something
>     like "cpu_is_powered_down()".

This seems to be reversing some patch? Maybe a new (optional)
callback for people who need this? AFAIK other platforms seem
to have some embedded controller that deals with this.

The A80 actually hase an embedded controller, but no documents
are available, and the original firmware from Allwinner is not
open source. Furthermore, in Allwinner's kernel, the kernel was
in charge of loading the firmware, a bit backwards IMHO.

Thanks!


Regards
ChenYu

> Further to this, this "software power controller" really just maps one
> power sequencing model onto another, so there's a chance it could be
> a generic mcpm driver than can be reused across multiple platforms.
>
> Cheers
> ---Dave
>

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Chen-Yu Tsai]

On Fri, Jul 24, 2015 at 11:44 PM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:
> On Fri, 24 Jul 2015, Chen-Yu Tsai wrote:
>
>> Hi,
>>
>> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > This concerns the following helpers:
>> >
>> >         __mcpm_cpu_going_down()
>> >         __mcpm_cpu_down()
>> >         __mcpm_outbound_enter_critical()
>> >         __mcpm_outbound_leave_critical()
>> >         __mcpm_cluster_state()
>> >
>> > They are and should only be used by the core code now.  Therefore their
>> > declarations are removed from mcpm.h and their definitions are made
>> > static, hence the need to move them before their users which accounts
>> > for the bulk of this patch.
>>
>> I'm looking for some advice. On the Allwinner A80, at least on mainline,
>> there is no external PMU or embedded controller in charge of power
>> controls. What this means is that I'm doing power sequencing in the
>> kernel as part of the MCPM calls, specifically powering down cores and
>> clusters in the .wait_for_powerdown callback. (I don't think it's
>> reasonable or even possible to power down stuff in .*_powerdown_prepare)
>
> Can you tell me more about the power control knobs at your disposal?  Do
> power gates become effective immediately or only when WFI is asserted?
>
> And can you configure things so a core may be powered up asynchronously
> from an IRQ?

The above probably wasn't clear enough. Power gates, reset controls and
SMP/WFI/WFE status are mapped to various mmio registers. The controls
are effective immediately.

The power gates and reset controls can only be manually controlled.
There is no mainline support for the embedded controller yet, and I
doubt Allwinner's firmware supports it either, as their kernel also
does power sequencing itself. In a nutshell, the kernel is on its
own, we do not support wakeups with IRQs.

>> Previously I was using __mcpm_cluster_state() to check if the last core
>> in a cluster was to be powered off, and thus the whole cluster could be
>> turned off as well.
>> I could also check if the individual power gates or
>> resets are asserted, but if a core was already scheduled to be brought
>> up, and MCPM common framework didn't call .cluster_powerup, there might
>> be a problem.
>
> I fail to see how a core could be scheduled to be brought up without
> deasserting its reset line somehow though.

My point is could there be a race condition in the sequence of events?
Say .*_powerup() deasserted the reset lines _after_ we checked them
in .wait_for_powerdown(). As Dave mentioned, .wait_for_powerdown() is
not called with the MCPM lock held.

But I've resolved to waiting for L2 WFI before powering off clusters.

>> Any suggestions? Maybe export __mcpm_cluster_state() so platform code
>> can know what's going to happen?
>
> The cluster state may change unexpectedly.  There is a special locking
> sequence and state machine needed to make this information reliable.
> Simply returning the current state wouldn't be enough to ensure
> it can be used race free.

I see.

> As Dave stated, we might have to supplement the MCPM core code with
> special methods involving a surviving CPU to perform the power-down
> operation on the dying CPU's behalf.  Doing this in .wait_for_powerdown
> is just an abuse of the API.

The other users I looked at all had other pieces of hardware taking care
of this, so I couldn't really understand where I could put this.

If adding another callback to handle this is acceptable, then I can
look into it.

> It also brings up the question if MCPM is actually necessary in that
> case or if you can do without its complexity.  For example, you may look
> at commit 905cdf9dda5d for such a case.  It mainly depends on whether or
> not cores (and the cluster) may be awaken asynchronously upon assertion
> of an IRQ in the context of cpuidle. If the hardware doesn't support
> that then MCPM doesn't bring you any actual benefit.
>
> So it depends on your hardware capabilities.

If they're just in WFI, then yes (I think). If they're powered down, they
need surviving cores to power them up.

But I had the impression that MCPM was trying to host common code for
multi-cluster management, such as core reference counting, proper locking
and maybe other stuff. With the MCPM framework, I reimplemented SMP
bringup and CPU hotplugging on the A80 with just half of LOC compared to
Allwinner's implementation, which is just a variation of another
platform's custom MCPM code.


Regards
ChenYu

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Nicolas Pitre]

On Sat, 25 Jul 2015, Chen-Yu Tsai wrote:

> On Fri, Jul 24, 2015 at 11:44 PM, Nicolas Pitre
> <nicolas.pitre@linaro.org> wrote:
> > On Fri, 24 Jul 2015, Chen-Yu Tsai wrote:
> >
> >> Hi,
> >>
> >> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >> > This concerns the following helpers:
> >> >
> >> >         __mcpm_cpu_going_down()
> >> >         __mcpm_cpu_down()
> >> >         __mcpm_outbound_enter_critical()
> >> >         __mcpm_outbound_leave_critical()
> >> >         __mcpm_cluster_state()
> >> >
> >> > They are and should only be used by the core code now.  Therefore their
> >> > declarations are removed from mcpm.h and their definitions are made
> >> > static, hence the need to move them before their users which accounts
> >> > for the bulk of this patch.
> >>
> >> I'm looking for some advice. On the Allwinner A80, at least on mainline,
> >> there is no external PMU or embedded controller in charge of power
> >> controls. What this means is that I'm doing power sequencing in the
> >> kernel as part of the MCPM calls, specifically powering down cores and
> >> clusters in the .wait_for_powerdown callback. (I don't think it's
> >> reasonable or even possible to power down stuff in .*_powerdown_prepare)
> >
> > Can you tell me more about the power control knobs at your disposal?  Do
> > power gates become effective immediately or only when WFI is asserted?
> >
> > And can you configure things so a core may be powered up asynchronously
> > from an IRQ?
> 
> The above probably wasn't clear enough. Power gates, reset controls and
> SMP/WFI/WFE status are mapped to various mmio registers. The controls
> are effective immediately.
> 
> The power gates and reset controls can only be manually controlled.
> There is no mainline support for the embedded controller yet, and I
> doubt Allwinner's firmware supports it either, as their kernel also
> does power sequencing itself. In a nutshell, the kernel is on its
> own, we do not support wakeups with IRQs.
> 
> >> Previously I was using __mcpm_cluster_state() to check if the last core
> >> in a cluster was to be powered off, and thus the whole cluster could be
> >> turned off as well.
> >> I could also check if the individual power gates or
> >> resets are asserted, but if a core was already scheduled to be brought
> >> up, and MCPM common framework didn't call .cluster_powerup, there might
> >> be a problem.
> >
> > I fail to see how a core could be scheduled to be brought up without
> > deasserting its reset line somehow though.
> 
> My point is could there be a race condition in the sequence of events?
> Say .*_powerup() deasserted the reset lines _after_ we checked them
> in .wait_for_powerdown(). As Dave mentioned, .wait_for_powerdown() is
> not called with the MCPM lock held.

In theory this should never happen.  Even if .wait_for_powerdown() was 
prevented from running concurrently with .*_powerup(), nothing would 
prevent .*_powerup() from running the moment .wait_for_powerdown() 
has returned.  So it is up to the higher level not to power up a CPU and 
wait for it to be down at the same time since this simply makes no 
sense... unless it really wants the CPU back right away.

> But I've resolved to waiting for L2 WFI before powering off clusters.
> 
> >> Any suggestions? Maybe export __mcpm_cluster_state() so platform code
> >> can know what's going to happen?
> >
> > The cluster state may change unexpectedly.  There is a special locking
> > sequence and state machine needed to make this information reliable.
> > Simply returning the current state wouldn't be enough to ensure
> > it can be used race free.
> 
> I see.
> 
> > As Dave stated, we might have to supplement the MCPM core code with
> > special methods involving a surviving CPU to perform the power-down
> > operation on the dying CPU's behalf.  Doing this in .wait_for_powerdown
> > is just an abuse of the API.
> 
> The other users I looked at all had other pieces of hardware taking care
> of this, so I couldn't really understand where I could put this.

I think what should be done in this case is simply to put the task of 
killing the CPU power on a work queue that gets executed by another CPU. 
Queueing the necessary work could be done from the MCPM core code the 
moment a special method exists in the machine backend structure. The 
work callback would take the MCPM lock, make sure the CPU/cluster state 
is "DOWN" and call that special method.

A call to .wait_for_powerdown() should not be responsible for the actual 
power down. It should remain optional.

> If adding another callback to handle this is acceptable, then I can
> look into it.

Please be my guest.


Nicolas

[PATCH 3/5] ARM: MCPM: make internal helpers private to the core code

[Dave Martin]

On Mon, Jul 27, 2015 at 12:43:28AM -0400, Nicolas Pitre wrote:
> On Sat, 25 Jul 2015, Chen-Yu Tsai wrote:
> 
> > On Fri, Jul 24, 2015 at 11:44 PM, Nicolas Pitre
> > <nicolas.pitre@linaro.org> wrote:
> > > On Fri, 24 Jul 2015, Chen-Yu Tsai wrote:
> > >
> > >> Hi,
> > >>
> > >> On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > >> > This concerns the following helpers:
> > >> >
> > >> >         __mcpm_cpu_going_down()
> > >> >         __mcpm_cpu_down()
> > >> >         __mcpm_outbound_enter_critical()
> > >> >         __mcpm_outbound_leave_critical()
> > >> >         __mcpm_cluster_state()
> > >> >
> > >> > They are and should only be used by the core code now.  Therefore their
> > >> > declarations are removed from mcpm.h and their definitions are made
> > >> > static, hence the need to move them before their users which accounts
> > >> > for the bulk of this patch.
> > >>
> > >> I'm looking for some advice. On the Allwinner A80, at least on mainline,
> > >> there is no external PMU or embedded controller in charge of power
> > >> controls. What this means is that I'm doing power sequencing in the
> > >> kernel as part of the MCPM calls, specifically powering down cores and
> > >> clusters in the .wait_for_powerdown callback. (I don't think it's
> > >> reasonable or even possible to power down stuff in .*_powerdown_prepare)
> > >
> > > Can you tell me more about the power control knobs at your disposal?  Do
> > > power gates become effective immediately or only when WFI is asserted?
> > >
> > > And can you configure things so a core may be powered up asynchronously
> > > from an IRQ?
> > 
> > The above probably wasn't clear enough. Power gates, reset controls and
> > SMP/WFI/WFE status are mapped to various mmio registers. The controls
> > are effective immediately.
> > 
> > The power gates and reset controls can only be manually controlled.
> > There is no mainline support for the embedded controller yet, and I
> > doubt Allwinner's firmware supports it either, as their kernel also
> > does power sequencing itself. In a nutshell, the kernel is on its
> > own, we do not support wakeups with IRQs.
> > 
> > >> Previously I was using __mcpm_cluster_state() to check if the last core
> > >> in a cluster was to be powered off, and thus the whole cluster could be
> > >> turned off as well.
> > >> I could also check if the individual power gates or
> > >> resets are asserted, but if a core was already scheduled to be brought
> > >> up, and MCPM common framework didn't call .cluster_powerup, there might
> > >> be a problem.
> > >
> > > I fail to see how a core could be scheduled to be brought up without
> > > deasserting its reset line somehow though.
> > 
> > My point is could there be a race condition in the sequence of events?
> > Say .*_powerup() deasserted the reset lines _after_ we checked them
> > in .wait_for_powerdown(). As Dave mentioned, .wait_for_powerdown() is
> > not called with the MCPM lock held.
> 
> In theory this should never happen.  Even if .wait_for_powerdown() was 
> prevented from running concurrently with .*_powerup(), nothing would 
> prevent .*_powerup() from running the moment .wait_for_powerdown() 
> has returned.  So it is up to the higher level not to power up a CPU and 
> wait for it to be down at the same time since this simply makes no 
> sense... unless it really wants the CPU back right away.
> 
> > But I've resolved to waiting for L2 WFI before powering off clusters.
> > 
> > >> Any suggestions? Maybe export __mcpm_cluster_state() so platform code
> > >> can know what's going to happen?
> > >
> > > The cluster state may change unexpectedly.  There is a special locking
> > > sequence and state machine needed to make this information reliable.
> > > Simply returning the current state wouldn't be enough to ensure
> > > it can be used race free.
> > 
> > I see.
> > 
> > > As Dave stated, we might have to supplement the MCPM core code with
> > > special methods involving a surviving CPU to perform the power-down
> > > operation on the dying CPU's behalf.  Doing this in .wait_for_powerdown
> > > is just an abuse of the API.
> > 
> > The other users I looked at all had other pieces of hardware taking care
> > of this, so I couldn't really understand where I could put this.
> 
> I think what should be done in this case is simply to put the task of 
> killing the CPU power on a work queue that gets executed by another CPU. 
> Queueing the necessary work could be done from the MCPM core code the 
> moment a special method exists in the machine backend structure. The 
> work callback would take the MCPM lock, make sure the CPU/cluster state 
> is "DOWN" and call that special method.
> 
> A call to .wait_for_powerdown() should not be responsible for the actual 
> power down. It should remain optional.
> 
> > If adding another callback to handle this is acceptable, then I can
> > look into it.
> 
> Please be my guest.

I think I originally named .wait_for_powerdown ".power_down_finish", or
something like that, with the expectation that this would both do the
serialisation and the last rites.

The dual role was a bit confusing though, and doesn't necessarily fit
the framework perfectly.

Having an optional callback for the "last rites" part probably would
be clearer.

Cheers
---Dave