[PATCH 0/2] MCPM backend for Virtual Express TC2

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Nicolas Pitre]

This is the MCPM backend enabling SMP secondary boot and CPU hotplug
on the VExpress TC2 big.LITTLE platform.  The method to allow CPU suspend
is also implemented.

Those patches have the following prerequisites:

- the vexpress/dual-cluster branch currently in ARM-SOC
  (commit 033a899c9b06)

- the VExpress SPC patches from Lorenzo here:
  http://news.gmane.org/group/gmane.linux.drivers.devicetree/thread=37483

For convenience I've pushed everything here:

  git://git.linaro.org/people/nico/linux TC2

Diffstat:


 arch/arm/mach-vexpress/Kconfig  |   9 ++
 arch/arm/mach-vexpress/Makefile |   1 +
 arch/arm/mach-vexpress/tc2_pm.c | 262 ++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
aka TC2.  This provides cluster management for SMP secondary boot and
CPU hotplug.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/mach-vexpress/Kconfig  |   9 ++
 arch/arm/mach-vexpress/Makefile |   1 +
 arch/arm/mach-vexpress/tc2_pm.c | 243 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+)
 create mode 100644 arch/arm/mach-vexpress/tc2_pm.c

diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index b8bbabec63..e7a825d7df 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -66,4 +66,13 @@ config ARCH_VEXPRESS_DCSCB
 	  This is needed to provide CPU and cluster power management
 	  on RTSM implementing big.LITTLE.
 
+config ARCH_VEXPRESS_TC2
+	bool "Versatile Express TC2 power management"
+	depends on MCPM
+	select VEXPRESS_SPC
+	select ARM_CCI
+	help
+	  Support for CPU and cluster power management on Versatile Express
+	  with a TC2 (A15x2 A7x3) big.LITTLE core tile.
+
 endmenu
diff --git a/arch/arm/mach-vexpress/Makefile b/arch/arm/mach-vexpress/Makefile
index 48ba89a814..b1cf227fa5 100644
--- a/arch/arm/mach-vexpress/Makefile
+++ b/arch/arm/mach-vexpress/Makefile
@@ -7,5 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 obj-y					:= v2m.o
 obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)	+= ct-ca9x4.o
 obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)	+= dcscb.o	dcscb_setup.o
+obj-$(CONFIG_ARCH_VEXPRESS_TC2)		+= tc2_pm.o
 obj-$(CONFIG_SMP)			+= platsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
new file mode 100644
index 0000000000..a3ea524372
--- /dev/null
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -0,0 +1,243 @@
+/*
+ * arch/arm/mach-vexpress/tc2_pm.c - TC2 power management support
+ *
+ * Created by:	Nicolas Pitre, October 2012
+ * Copyright:	(C) 2012-2013  Linaro Limited
+ *
+ * Some portions of this file were originally written by Achin Gupta
+ * Copyright:   (C) 2012  ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/errno.h>
+
+#include <asm/mcpm.h>
+#include <asm/proc-fns.h>
+#include <asm/cacheflush.h>
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+
+#include <mach/motherboard.h>
+
+#include <linux/vexpress.h>
+#include <linux/arm-cci.h>
+
+/*
+ * 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
+ * is already live using the same logical CPU number which trips lockdep
+ * debugging.
+ */
+static arch_spinlock_t tc2_pm_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+static int tc2_pm_use_count[3][2];
+
+static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
+{
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+	if (cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster))
+		return -EINVAL;
+
+	/*
+	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
+	 * variant exists, we need to disable IRQs manually here.
+	 */
+	local_irq_disable();
+	arch_spin_lock(&tc2_pm_lock);
+
+	if (!tc2_pm_use_count[0][cluster] &&
+	    !tc2_pm_use_count[1][cluster] &&
+	    !tc2_pm_use_count[2][cluster])
+		vexpress_spc_powerdown_enable(cluster, 0);
+
+	tc2_pm_use_count[cpu][cluster]++;
+	if (tc2_pm_use_count[cpu][cluster] == 1) {
+		vexpress_spc_write_resume_reg(cluster, cpu,
+					      virt_to_phys(mcpm_entry_point));
+		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
+	} else if (tc2_pm_use_count[cpu][cluster] != 2) {
+		/*
+		 * The only possible values are:
+		 * 0 = CPU down
+		 * 1 = CPU (still) up
+		 * 2 = CPU requested to be up before it had a chance
+		 *     to actually make itself down.
+		 * Any other value is a bug.
+		 */
+		BUG();
+	}
+
+	arch_spin_unlock(&tc2_pm_lock);
+	local_irq_enable();
+
+	return 0;
+}
+
+static void tc2_pm_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster;
+	bool last_man = false, skip_wfi = false;
+
+	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);
+	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));
+
+	__mcpm_cpu_going_down(cpu, cluster);
+
+	arch_spin_lock(&tc2_pm_lock);
+	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+	tc2_pm_use_count[cpu][cluster]--;
+	if (tc2_pm_use_count[cpu][cluster] == 0) {
+		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
+		if (!tc2_pm_use_count[0][cluster] &&
+		    !tc2_pm_use_count[1][cluster] &&
+		    !tc2_pm_use_count[2][cluster]) {
+			vexpress_spc_powerdown_enable(cluster, 1);
+			vexpress_spc_set_global_wakeup_intr(1);
+			last_man = true;
+		}
+	} else if (tc2_pm_use_count[cpu][cluster] == 1) {
+		/*
+		 * A power_up request went ahead of us.
+		 * Even if we do not want to shut this CPU down,
+		 * the caller expects a certain state as if the WFI
+		 * was aborted.  So let's continue with cache cleaning.
+		 */
+		skip_wfi = true;
+	} else
+		BUG();
+
+	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
+		arch_spin_unlock(&tc2_pm_lock);
+
+		set_cr(get_cr() & ~CR_C);
+		flush_cache_all();
+		asm volatile ("clrex");
+		set_auxcr(get_auxcr() & ~(1 << 6));
+
+		cci_disable_port_by_cpu(mpidr);
+
+		/*
+		 * Ensure that both C & I bits are disabled in the SCTLR
+		 * before disabling ACE snoops. This ensures that no
+		 * coherency traffic will originate from this cpu after
+		 * ACE snoops are turned off.
+		 */
+		cpu_proc_fin();
+
+		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+	} else {
+		/*
+		 * If last man then undo any setup done previously.
+		 */
+		if (last_man) {
+			vexpress_spc_powerdown_enable(cluster, 0);
+			vexpress_spc_set_global_wakeup_intr(0);
+		}
+
+		arch_spin_unlock(&tc2_pm_lock);
+
+		set_cr(get_cr() & ~CR_C);
+		flush_cache_louis();
+		asm volatile ("clrex");
+		set_auxcr(get_auxcr() & ~(1 << 6));
+	}
+
+	__mcpm_cpu_down(cpu, cluster);
+
+	/* Now we are prepared for power-down, do it: */
+	if (!skip_wfi)
+		wfi();
+
+	/* Not dead@this point?  Let our caller cope. */
+}
+
+static void tc2_pm_powered_up(void)
+{
+	unsigned int mpidr, cpu, cluster;
+	unsigned long flags;
+
+	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);
+	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));
+
+	local_irq_save(flags);
+	arch_spin_lock(&tc2_pm_lock);
+
+	if (!tc2_pm_use_count[0][cluster] &&
+	    !tc2_pm_use_count[1][cluster] &&
+	    !tc2_pm_use_count[2][cluster]) {
+		vexpress_spc_powerdown_enable(cluster, 0);
+		vexpress_spc_set_global_wakeup_intr(0);
+	}
+
+	if (!tc2_pm_use_count[cpu][cluster])
+		tc2_pm_use_count[cpu][cluster] = 1;
+
+	vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 0);
+	vexpress_spc_write_resume_reg(cluster, cpu, 0);
+
+	arch_spin_unlock(&tc2_pm_lock);
+	local_irq_restore(flags);
+}
+
+static const struct mcpm_platform_ops tc2_pm_power_ops = {
+	.power_up	= tc2_pm_power_up,
+	.power_down	= tc2_pm_power_down,
+	.powered_up	= tc2_pm_powered_up,
+};
+
+static void __init tc2_pm_usage_count_init(void)
+{
+	unsigned int mpidr, cpu, cluster;
+
+	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);
+	BUG_ON(cpu >= 3 || cluster >= 2);
+	tc2_pm_use_count[cpu][cluster] = 1;
+}
+
+/*
+ * Enable cluster-level coherency, in preparation for turning on the MMU.
+ */
+static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
+{
+	asm volatile (" \n"
+"	cmp	r0, #1 \n"
+"	beq	cci_enable_port_for_self \n"
+"	bx	lr ");
+}
+
+static int __init tc2_pm_init(void)
+{
+	int ret;
+
+	if (!vexpress_spc_check_loaded())
+		return -ENODEV;
+
+	tc2_pm_usage_count_init();
+
+	ret = mcpm_platform_register(&tc2_pm_power_ops);
+	if (!ret)
+		ret = mcpm_sync_init(tc2_pm_power_up_setup);
+	if (!ret)
+		pr_info("TC2 power management initialized\n");
+	return ret;
+}
+
+early_initcall(tc2_pm_init);
-- 
1.8.1.2

[PATCH 2/2] ARM: vexpress/TC2: implement PM suspend method

[Nicolas Pitre]

From: Nicolas Pitre <nico@linaro.org>

This is simplistic for the moment as the expected residency is used to
prevent the shutting down of L2 and the cluster if the residency for
the last man is lower than 5 ms.  To make this right, the shutdown
latency for each CPU would need to be recorded and taken into account.

On a suspend, the firmware mailbox address has to be set prior entering
low power mode.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/mach-vexpress/tc2_pm.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index a3ea524372..7901528566 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -79,7 +79,7 @@ static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
 	return 0;
 }
 
-static void tc2_pm_power_down(void)
+static void tc2_pm_down(u64 residency)
 {
 	unsigned int mpidr, cpu, cluster;
 	bool last_man = false, skip_wfi = false;
@@ -100,7 +100,8 @@ static void tc2_pm_power_down(void)
 		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
 		if (!tc2_pm_use_count[0][cluster] &&
 		    !tc2_pm_use_count[1][cluster] &&
-		    !tc2_pm_use_count[2][cluster]) {
+		    !tc2_pm_use_count[2][cluster] &&
+		    (!residency || residency > 5000)) {
 			vexpress_spc_powerdown_enable(cluster, 1);
 			vexpress_spc_set_global_wakeup_intr(1);
 			last_man = true;
@@ -161,6 +162,23 @@ static void tc2_pm_power_down(void)
 	/* Not dead at this point?  Let our caller cope. */
 }
 
+static void tc2_pm_power_down(void)
+{
+	tc2_pm_down(0);
+}
+
+static void tc2_pm_suspend(u64 residency)
+{
+	unsigned int mpidr, cpu, cluster;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+	vexpress_spc_write_resume_reg(cluster, cpu,
+				      virt_to_phys(mcpm_entry_point));
+	tc2_pm_down(residency);
+}
+
 static void tc2_pm_powered_up(void)
 {
 	unsigned int mpidr, cpu, cluster;
@@ -196,6 +214,7 @@ 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,
 };
 
-- 
1.8.1.2

[PATCH 2/2] ARM: vexpress/TC2: implement PM suspend method

[Lorenzo Pieralisi]

Hi Nico,

On Fri, Jun 07, 2013 at 07:39:12AM +0100, Nicolas Pitre wrote:
> From: Nicolas Pitre <nico@linaro.org>
> 
> This is simplistic for the moment as the expected residency is used to
> prevent the shutting down of L2 and the cluster if the residency for
> the last man is lower than 5 ms.  To make this right, the shutdown
> latency for each CPU would need to be recorded and taken into account.
> 
> On a suspend, the firmware mailbox address has to be set prior entering
> low power mode.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/arm/mach-vexpress/tc2_pm.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> index a3ea524372..7901528566 100644
> --- a/arch/arm/mach-vexpress/tc2_pm.c
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -79,7 +79,7 @@ static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
>  	return 0;
>  }
>  
> -static void tc2_pm_power_down(void)
> +static void tc2_pm_down(u64 residency)
>  {
>  	unsigned int mpidr, cpu, cluster;
>  	bool last_man = false, skip_wfi = false;
> @@ -100,7 +100,8 @@ static void tc2_pm_power_down(void)
>  		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
>  		if (!tc2_pm_use_count[0][cluster] &&
>  		    !tc2_pm_use_count[1][cluster] &&
> -		    !tc2_pm_use_count[2][cluster]) {
> +		    !tc2_pm_use_count[2][cluster] &&
> +		    (!residency || residency > 5000)) {

I would remove the line above altogether. It is ok to have parameter (we
should still define what that residency actually is), I do not think it
makes sense to add policy at this stage, I have a patchset to queue for
that.

Thanks,
Lorenzo

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Lorenzo Pieralisi]

On Fri, Jun 07, 2013 at 07:39:11AM +0100, Nicolas Pitre wrote:
> This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
> aka TC2.  This provides cluster management for SMP secondary boot and
> CPU hotplug.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/arm/mach-vexpress/Kconfig  |   9 ++
>  arch/arm/mach-vexpress/Makefile |   1 +
>  arch/arm/mach-vexpress/tc2_pm.c | 243 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 253 insertions(+)
>  create mode 100644 arch/arm/mach-vexpress/tc2_pm.c
> 
> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> index b8bbabec63..e7a825d7df 100644
> --- a/arch/arm/mach-vexpress/Kconfig
> +++ b/arch/arm/mach-vexpress/Kconfig
> @@ -66,4 +66,13 @@ config ARCH_VEXPRESS_DCSCB
>  	  This is needed to provide CPU and cluster power management
>  	  on RTSM implementing big.LITTLE.
>  
> +config ARCH_VEXPRESS_TC2
> +	bool "Versatile Express TC2 power management"
> +	depends on MCPM
> +	select VEXPRESS_SPC
> +	select ARM_CCI
> +	help
> +	  Support for CPU and cluster power management on Versatile Express
> +	  with a TC2 (A15x2 A7x3) big.LITTLE core tile.
> +
>  endmenu
> diff --git a/arch/arm/mach-vexpress/Makefile b/arch/arm/mach-vexpress/Makefile
> index 48ba89a814..b1cf227fa5 100644
> --- a/arch/arm/mach-vexpress/Makefile
> +++ b/arch/arm/mach-vexpress/Makefile
> @@ -7,5 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
>  obj-y					:= v2m.o
>  obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)	+= ct-ca9x4.o
>  obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)	+= dcscb.o	dcscb_setup.o
> +obj-$(CONFIG_ARCH_VEXPRESS_TC2)		+= tc2_pm.o
>  obj-$(CONFIG_SMP)			+= platsmp.o
>  obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> new file mode 100644
> index 0000000000..a3ea524372
> --- /dev/null
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -0,0 +1,243 @@
> +/*
> + * arch/arm/mach-vexpress/tc2_pm.c - TC2 power management support
> + *
> + * Created by:	Nicolas Pitre, October 2012
> + * Copyright:	(C) 2012-2013  Linaro Limited
> + *
> + * Some portions of this file were originally written by Achin Gupta
> + * Copyright:   (C) 2012  ARM Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include <mach/motherboard.h>

Is the include above needed ?

> +#include <linux/vexpress.h>
> +#include <linux/arm-cci.h>
> +
> +/*
> + * 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
> + * is already live using the same logical CPU number which trips lockdep
> + * debugging.
> + */
> +static arch_spinlock_t tc2_pm_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +
> +static int tc2_pm_use_count[3][2];
> +
> +static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	if (cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster))
> +		return -EINVAL;

We could stash (vexpress_spc_get_nb_cpus()), it never changes.

> +	/*
> +	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
> +	 * variant exists, we need to disable IRQs manually here.
> +	 */
> +	local_irq_disable();
> +	arch_spin_lock(&tc2_pm_lock);
> +
> +	if (!tc2_pm_use_count[0][cluster] &&
> +	    !tc2_pm_use_count[1][cluster] &&
> +	    !tc2_pm_use_count[2][cluster])
> +		vexpress_spc_powerdown_enable(cluster, 0);
> +
> +	tc2_pm_use_count[cpu][cluster]++;
> +	if (tc2_pm_use_count[cpu][cluster] == 1) {
> +		vexpress_spc_write_resume_reg(cluster, cpu,
> +					      virt_to_phys(mcpm_entry_point));
> +		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +	} else if (tc2_pm_use_count[cpu][cluster] != 2) {
> +		/*
> +		 * The only possible values are:
> +		 * 0 = CPU down
> +		 * 1 = CPU (still) up
> +		 * 2 = CPU requested to be up before it had a chance
> +		 *     to actually make itself down.
> +		 * Any other value is a bug.
> +		 */
> +		BUG();
> +	}
> +
> +	arch_spin_unlock(&tc2_pm_lock);
> +	local_irq_enable();
> +
> +	return 0;
> +}
> +
> +static void tc2_pm_power_down(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	bool last_man = false, skip_wfi = false;
> +
> +	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);
> +	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));

Ditto, see above.

> +	__mcpm_cpu_going_down(cpu, cluster);
> +
> +	arch_spin_lock(&tc2_pm_lock);
> +	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> +	tc2_pm_use_count[cpu][cluster]--;
> +	if (tc2_pm_use_count[cpu][cluster] == 0) {
> +		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +		if (!tc2_pm_use_count[0][cluster] &&
> +		    !tc2_pm_use_count[1][cluster] &&
> +		    !tc2_pm_use_count[2][cluster]) {
> +			vexpress_spc_powerdown_enable(cluster, 1);
> +			vexpress_spc_set_global_wakeup_intr(1);
> +			last_man = true;
> +		}
> +	} else if (tc2_pm_use_count[cpu][cluster] == 1) {
> +		/*
> +		 * A power_up request went ahead of us.
> +		 * Even if we do not want to shut this CPU down,
> +		 * the caller expects a certain state as if the WFI
> +		 * was aborted.  So let's continue with cache cleaning.
> +		 */
> +		skip_wfi = true;
> +	} else
> +		BUG();
> +
> +	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> +		arch_spin_unlock(&tc2_pm_lock);
> +
> +		set_cr(get_cr() & ~CR_C);

We must disable L2 prefetching on A15 before cleaning L2.

> +		flush_cache_all();
> +		asm volatile ("clrex");
> +		set_auxcr(get_auxcr() & ~(1 << 6));

I think we should add comments here to avoid copy'n'paste mayhem. The
code above is safe on cpus like A15/A7 (I know this back-end can just
be run on those processors) that hit in the cache with C-bit in SCTLR
cleared, it would explode on processors (eg A9) that do not hit in the
cache with C-bit cleared. I am wondering if it is better to write inline
asm and jump to v7 cache functions that do not need stack push/pop
straight away.

> +		cci_disable_port_by_cpu(mpidr);
> +
> +		/*
> +		 * Ensure that both C & I bits are disabled in the SCTLR
> +		 * before disabling ACE snoops. This ensures that no
> +		 * coherency traffic will originate from this cpu after
> +		 * ACE snoops are turned off.
> +		 */
> +		cpu_proc_fin();

Mmm, C bit is already cleared, why clear the I bit (and the A bit) ?
I do not think cpu_proc_fin() is needed and I am really keen on getting
the power down procedure right to avoid copy'n'paste induced error from
the start.

> +
> +		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> +	} else {
> +		/*
> +		 * If last man then undo any setup done previously.
> +		 */
> +		if (last_man) {
> +			vexpress_spc_powerdown_enable(cluster, 0);
> +			vexpress_spc_set_global_wakeup_intr(0);
> +		}
> +
> +		arch_spin_unlock(&tc2_pm_lock);
> +
> +		set_cr(get_cr() & ~CR_C);
> +		flush_cache_louis();
> +		asm volatile ("clrex");
> +		set_auxcr(get_auxcr() & ~(1 << 6));
> +	}
> +
> +	__mcpm_cpu_down(cpu, cluster);
> +
> +	/* Now we are prepared for power-down, do it: */
> +	if (!skip_wfi)
> +		wfi();
> +
> +	/* Not dead at this point?  Let our caller cope. */

This function should disable the GIC CPU IF, but I guess you will add
the code when CPUidle is merged.

> +static void tc2_pm_powered_up(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	unsigned long flags;
> +
> +	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);
> +	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));
> +
> +	local_irq_save(flags);
> +	arch_spin_lock(&tc2_pm_lock);
> +
> +	if (!tc2_pm_use_count[0][cluster] &&
> +	    !tc2_pm_use_count[1][cluster] &&
> +	    !tc2_pm_use_count[2][cluster]) {
> +		vexpress_spc_powerdown_enable(cluster, 0);
> +		vexpress_spc_set_global_wakeup_intr(0);
> +	}
> +
> +	if (!tc2_pm_use_count[cpu][cluster])
> +		tc2_pm_use_count[cpu][cluster] = 1;
> +
> +	vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 0);
> +	vexpress_spc_write_resume_reg(cluster, cpu, 0);
> +
> +	arch_spin_unlock(&tc2_pm_lock);
> +	local_irq_restore(flags);
> +}
> +
> +static const struct mcpm_platform_ops tc2_pm_power_ops = {
> +	.power_up	= tc2_pm_power_up,
> +	.power_down	= tc2_pm_power_down,
> +	.powered_up	= tc2_pm_powered_up,
> +};
> +
> +static void __init tc2_pm_usage_count_init(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +
> +	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);
> +	BUG_ON(cpu >= 3 || cluster >= 2);
> +	tc2_pm_use_count[cpu][cluster] = 1;
> +}
> +
> +/*
> + * Enable cluster-level coherency, in preparation for turning on the MMU.
> + */
> +static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
> +{
> +	asm volatile (" \n"
> +"	cmp	r0, #1 \n"
> +"	beq	cci_enable_port_for_self \n"
> +"	bx	lr ");
> +}

We could write a function like the above (stackless and inline) for the
sequence:

1- clear C bit
2- flush cache all/louis
3- exit coherency

Again, the current implementation is right on TC2 but we should not let
people think that's correct for all v7 processors

Lorenzo

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Pawel Moll]

On Fri, 2013-06-07 at 07:39 +0100, Nicolas Pitre wrote:
> This is the MCPM backend enabling SMP secondary boot and CPU hotplug
> on the VExpress TC2 big.LITTLE platform.  The method to allow CPU suspend
> is also implemented.

With my Naming Purist (TM) hat on, I must point out that there is no
such thing as TC2... The platform you're referring to is V2P-CA15_A7.
TC2 is just an internal name for the chip (as in: Test Chip 2, which
followed 1 and was supposed to be followed by 3 ;-). And considering the
fact that substantial part of the power management system lives outside
of the main chip anyway, the naming is not really precise.

Now, I appreciate that "v2p_ca15_a7_" namespace is a bit cumbersome
comparing to "tc2_" and no, I don't really have better ideas now...

Other than that:

Acked-by: Pawel Moll <pawel.moll@arm.com>

Cheers!

Pawe?

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Nicolas Pitre]

On Fri, 7 Jun 2013, Pawel Moll wrote:

> On Fri, 2013-06-07 at 07:39 +0100, Nicolas Pitre wrote:
> > This is the MCPM backend enabling SMP secondary boot and CPU hotplug
> > on the VExpress TC2 big.LITTLE platform.  The method to allow CPU suspend
> > is also implemented.
> 
> With my Naming Purist (TM) hat on, I must point out that there is no
> such thing as TC2... The platform you're referring to is V2P-CA15_A7.
> TC2 is just an internal name for the chip (as in: Test Chip 2, which
> followed 1 and was supposed to be followed by 3 ;-). And considering the
> fact that substantial part of the power management system lives outside
> of the main chip anyway, the naming is not really precise.

Well, in people's mind, TC2 refers to the whole system now, for better 
or for worse.

> Now, I appreciate that "v2p_ca15_a7_" namespace is a bit cumbersome
> comparing to "tc2_" and no, I don't really have better ideas now...

Exactly.  If ARM wants names that stick, they'll have to be much more 
practical.  Otherwise people will simply grab the first sensible 
alternative name around.

All the ARM people I talk to always refer to v2p_ca15_a7 core tile as 
simply TC2.  So using another name at this point would only *create* 
confusion.  It is therefore probably best to keep it.

You should do like some other companies does i.e. have an official name, 
and an alias which is short and borrowed from an animal, or some 
geographical region, etc.

> Other than that:
> 
> Acked-by: Pawel Moll <pawel.moll@arm.com>

Thanks.


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Fri, 7 Jun 2013, Lorenzo Pieralisi wrote:

> On Fri, Jun 07, 2013 at 07:39:11AM +0100, Nicolas Pitre wrote:
> > +#include <mach/motherboard.h>
> 
> Is the include above needed ?

Apparently not.

> > +static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> > +{
> > +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> > +	if (cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster))
> > +		return -EINVAL;
> 
> We could stash (vexpress_spc_get_nb_cpus()), it never changes.

Well... sure.  However, given cpu_up is not a very hot path and because 
the cpu argument is externally provided in this case, I'm inclined to 
leave this instance as is.  I'll hardcode a constant in the other cases 
where the cpu number is obtained from the MPIDR and validated with 
BUG_ON() where this is done only to prevent overflowing the 
tc2_pm_use_count array if something very wrong happens.

Oh and I'll use defines for those constants as well.

> > +	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> > +		arch_spin_unlock(&tc2_pm_lock);
> > +
> > +		set_cr(get_cr() & ~CR_C);
> 
> We must disable L2 prefetching on A15 before cleaning L2.

OK.

> > +		flush_cache_all();
> > +		asm volatile ("clrex");
> > +		set_auxcr(get_auxcr() & ~(1 << 6));
> 
> I think we should add comments here to avoid copy'n'paste mayhem. The
> code above is safe on cpus like A15/A7 (I know this back-end can just
> be run on those processors) that hit in the cache with C-bit in SCTLR
> cleared, it would explode on processors (eg A9) that do not hit in the
> cache with C-bit cleared. I am wondering if it is better to write inline
> asm and jump to v7 cache functions that do not need stack push/pop
> straight away.

Well... yeah.  This is unfortunate because we moved this part from 
assembly to C over time to clean it up.  But better use something safer 
in case it gets copied as you say.

> 
> > +		cci_disable_port_by_cpu(mpidr);
> > +
> > +		/*
> > +		 * Ensure that both C & I bits are disabled in the SCTLR
> > +		 * before disabling ACE snoops. This ensures that no
> > +		 * coherency traffic will originate from this cpu after
> > +		 * ACE snoops are turned off.
> > +		 */
> > +		cpu_proc_fin();
> 
> Mmm, C bit is already cleared, why clear the I bit (and the A bit) ?
> I do not think cpu_proc_fin() is needed and I am really keen on getting
> the power down procedure right to avoid copy'n'paste induced error from
> the start.

I trusted the above and its accompanying comment from Achin's initial 
cluster shutdown code.  But I suppose icache lookups don't create snoop 
requests?

> > +		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> > +	} else {
> > +		/*
> > +		 * If last man then undo any setup done previously.
> > +		 */
> > +		if (last_man) {
> > +			vexpress_spc_powerdown_enable(cluster, 0);
> > +			vexpress_spc_set_global_wakeup_intr(0);
> > +		}
> > +
> > +		arch_spin_unlock(&tc2_pm_lock);
> > +
> > +		set_cr(get_cr() & ~CR_C);
> > +		flush_cache_louis();
> > +		asm volatile ("clrex");
> > +		set_auxcr(get_auxcr() & ~(1 << 6));
> > +	}
> > +
> > +	__mcpm_cpu_down(cpu, cluster);
> > +
> > +	/* Now we are prepared for power-down, do it: */
> > +	if (!skip_wfi)
> > +		wfi();
> > +
> > +	/* Not dead at this point?  Let our caller cope. */
> 
> This function should disable the GIC CPU IF, but I guess you will add
> the code when CPUidle is merged.

The GIC code does not provide a hook for doing that at the moment.  That 
needs to be sorted out there before that can be added here.

[...]

OK, here's another version:

From: Nicolas Pitre <nicolas.pitre@linaro.org>
Date: Fri, 19 Oct 2012 20:48:50 -0400
Subject: [PATCH] ARM: vexpress/TC2: basic PM support

This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
aka TC2.  This provides cluster management for SMP secondary boot and
CPU hotplug.

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

diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index b8bbabec63..e7a825d7df 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -66,4 +66,13 @@ config ARCH_VEXPRESS_DCSCB
 	  This is needed to provide CPU and cluster power management
 	  on RTSM implementing big.LITTLE.
 
+config ARCH_VEXPRESS_TC2
+	bool "Versatile Express TC2 power management"
+	depends on MCPM
+	select VEXPRESS_SPC
+	select ARM_CCI
+	help
+	  Support for CPU and cluster power management on Versatile Express
+	  with a TC2 (A15x2 A7x3) big.LITTLE core tile.
+
 endmenu
diff --git a/arch/arm/mach-vexpress/Makefile b/arch/arm/mach-vexpress/Makefile
index 48ba89a814..b1cf227fa5 100644
--- a/arch/arm/mach-vexpress/Makefile
+++ b/arch/arm/mach-vexpress/Makefile
@@ -7,5 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 obj-y					:= v2m.o
 obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)	+= ct-ca9x4.o
 obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)	+= dcscb.o	dcscb_setup.o
+obj-$(CONFIG_ARCH_VEXPRESS_TC2)		+= tc2_pm.o
 obj-$(CONFIG_SMP)			+= platsmp.o
 obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
new file mode 100644
index 0000000000..f0673b4814
--- /dev/null
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -0,0 +1,275 @@
+/*
+ * arch/arm/mach-vexpress/tc2_pm.c - TC2 power management support
+ *
+ * Created by:	Nicolas Pitre, October 2012
+ * Copyright:	(C) 2012-2013  Linaro Limited
+ *
+ * Some portions of this file were originally written by Achin Gupta
+ * Copyright:   (C) 2012  ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/errno.h>
+
+#include <asm/mcpm.h>
+#include <asm/proc-fns.h>
+#include <asm/cacheflush.h>
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+
+#include <linux/vexpress.h>
+#include <linux/arm-cci.h>
+
+/*
+ * 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
+ * is already live using the same logical CPU number which trips lockdep
+ * debugging.
+ */
+static arch_spinlock_t tc2_pm_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+#define TC2_CLUSTERS			2
+#define TC2_MAX_CPUS_PER_CLUSTER	3
+static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];
+
+static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
+{
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+	if (cluster >= TC2_CLUSTERS || cpu >= vexpress_spc_get_nb_cpus(cluster))
+		return -EINVAL;
+
+	/*
+	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
+	 * variant exists, we need to disable IRQs manually here.
+	 */
+	local_irq_disable();
+	arch_spin_lock(&tc2_pm_lock);
+
+	if (!tc2_pm_use_count[0][cluster] &&
+	    !tc2_pm_use_count[1][cluster] &&
+	    !tc2_pm_use_count[2][cluster])
+		vexpress_spc_powerdown_enable(cluster, 0);
+
+	tc2_pm_use_count[cpu][cluster]++;
+	if (tc2_pm_use_count[cpu][cluster] == 1) {
+		vexpress_spc_write_resume_reg(cluster, cpu,
+					      virt_to_phys(mcpm_entry_point));
+		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
+	} else if (tc2_pm_use_count[cpu][cluster] != 2) {
+		/*
+		 * The only possible values are:
+		 * 0 = CPU down
+		 * 1 = CPU (still) up
+		 * 2 = CPU requested to be up before it had a chance
+		 *     to actually make itself down.
+		 * Any other value is a bug.
+		 */
+		BUG();
+	}
+
+	arch_spin_unlock(&tc2_pm_lock);
+	local_irq_enable();
+
+	return 0;
+}
+
+static void tc2_pm_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster;
+	bool last_man = false, skip_wfi = false;
+
+	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);
+	BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
+
+	__mcpm_cpu_going_down(cpu, cluster);
+
+	arch_spin_lock(&tc2_pm_lock);
+	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+	tc2_pm_use_count[cpu][cluster]--;
+	if (tc2_pm_use_count[cpu][cluster] == 0) {
+		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
+		if (!tc2_pm_use_count[0][cluster] &&
+		    !tc2_pm_use_count[1][cluster] &&
+		    !tc2_pm_use_count[2][cluster]) {
+			vexpress_spc_powerdown_enable(cluster, 1);
+			vexpress_spc_set_global_wakeup_intr(1);
+			last_man = true;
+		}
+	} else if (tc2_pm_use_count[cpu][cluster] == 1) {
+		/*
+		 * A power_up request went ahead of us.
+		 * Even if we do not want to shut this CPU down,
+		 * the caller expects a certain state as if the WFI
+		 * was aborted.  So let's continue with cache cleaning.
+		 */
+		skip_wfi = true;
+	} else
+		BUG();
+
+	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
+		arch_spin_unlock(&tc2_pm_lock);
+
+		if ((read_cpuid_id() & 0xf0) == 0xf0) {
+			/*
+			 * On the Cortex-A15 we need to disable
+			 * L2 prefetching before flushing the cache.
+			 */
+			asm volatile(
+			"mcr	p15, 1, %0, c15, c0, 3 \n\t"
+			"isb	\n\t"
+			"dsb	"
+			: : "r" (0x400) );
+		}
+
+		/*
+		 * We need to disable and flush the whole (L1 and L2) cache.
+		 * Let's do it in the safest possible way i.e. with
+		 * no memory access within the following sequence,
+		 * including the stack.
+		 */
+		asm volatile(
+		"mrc	p15, 0, r0, c1, c0, 0	@ get CR \n\t"
+		"bic	r0, r0, #"__stringify(CR_C)" \n\t"
+		"mcr	p15, 0, r0, c1, c0, 0	@ set CR \n\t"
+		"isb	\n\t"
+		"bl	v7_flush_dcache_all \n\t"
+		"clrex	\n\t"
+		"mrc	p15, 0, r0, c1, c0, 1	@ get AUXCR \n\t"
+		"bic	r0, r0, #(1 << 6)	@ disable local coherency \n\t"
+		"mcr	p15, 0, r0, c1, c0, 1	@ set AUXCR \n\t"
+		"isb	"
+		: : : "r0","r1","r2","r3","r4","r5","r6","r7",
+		      "r9","r10","r11","lr","memory");
+
+		cci_disable_port_by_cpu(mpidr);
+
+		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+	} else {
+		/*
+		 * If last man then undo any setup done previously.
+		 */
+		if (last_man) {
+			vexpress_spc_powerdown_enable(cluster, 0);
+			vexpress_spc_set_global_wakeup_intr(0);
+		}
+
+		arch_spin_unlock(&tc2_pm_lock);
+
+		/*
+		 * We need to disable and flush only the L1 cache.
+		 * Let's do it in the safest possible way as above.
+		 */
+		asm volatile(
+		"mrc	p15, 0, r0, c1, c0, 0	@ get CR \n\t"
+		"bic	r0, r0, #"__stringify(CR_C)" \n\t"
+		"mcr	p15, 0, r0, c1, c0, 0	@ set CR \n\t"
+		"isb	\n\t"
+		"bl	v7_flush_dcache_louis \n\t"
+		"clrex	\n\t"
+		"mrc	p15, 0, r0, c1, c0, 1	@ get AUXCR \n\t"
+		"bic	r0, r0, #(1 << 6)	@ disable local coherency \n\t"
+		"mcr	p15, 0, r0, c1, c0, 1	@ set AUXCR \n\t"
+		"isb	"
+		: : : "r0","r1","r2","r3","r4","r5","r6","r7",
+		      "r9","r10","r11","lr","memory");
+	}
+
+	__mcpm_cpu_down(cpu, cluster);
+
+	/* Now we are prepared for power-down, do it: */
+	if (!skip_wfi)
+		wfi();
+
+	/* Not dead@this point?  Let our caller cope. */
+}
+
+static void tc2_pm_powered_up(void)
+{
+	unsigned int mpidr, cpu, cluster;
+	unsigned long flags;
+
+	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);
+	BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
+
+	local_irq_save(flags);
+	arch_spin_lock(&tc2_pm_lock);
+
+	if (!tc2_pm_use_count[0][cluster] &&
+	    !tc2_pm_use_count[1][cluster] &&
+	    !tc2_pm_use_count[2][cluster]) {
+		vexpress_spc_powerdown_enable(cluster, 0);
+		vexpress_spc_set_global_wakeup_intr(0);
+	}
+
+	if (!tc2_pm_use_count[cpu][cluster])
+		tc2_pm_use_count[cpu][cluster] = 1;
+
+	vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 0);
+	vexpress_spc_write_resume_reg(cluster, cpu, 0);
+
+	arch_spin_unlock(&tc2_pm_lock);
+	local_irq_restore(flags);
+}
+
+static const struct mcpm_platform_ops tc2_pm_power_ops = {
+	.power_up	= tc2_pm_power_up,
+	.power_down	= tc2_pm_power_down,
+	.powered_up	= tc2_pm_powered_up,
+};
+
+static void __init tc2_pm_usage_count_init(void)
+{
+	unsigned int mpidr, cpu, cluster;
+
+	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);
+	BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
+	tc2_pm_use_count[cpu][cluster] = 1;
+}
+
+/*
+ * Enable cluster-level coherency, in preparation for turning on the MMU.
+ */
+static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
+{
+	asm volatile (" \n"
+"	cmp	r0, #1 \n"
+"	bxne	lr \n"
+"	b	cci_enable_port_for_self ");
+}
+
+static int __init tc2_pm_init(void)
+{
+	int ret;
+
+	if (!vexpress_spc_check_loaded())
+		return -ENODEV;
+
+	tc2_pm_usage_count_init();
+
+	ret = mcpm_platform_register(&tc2_pm_power_ops);
+	if (!ret)
+		ret = mcpm_sync_init(tc2_pm_power_up_setup);
+	if (!ret)
+		pr_info("TC2 power management initialized\n");
+	return ret;
+}
+
+early_initcall(tc2_pm_init);

[PATCH 2/2] ARM: vexpress/TC2: implement PM suspend method

[Nicolas Pitre]

On Fri, 7 Jun 2013, Lorenzo Pieralisi wrote:

> Hi Nico,
> 
> On Fri, Jun 07, 2013 at 07:39:12AM +0100, Nicolas Pitre wrote:
> > From: Nicolas Pitre <nico@linaro.org>
> > 
> > This is simplistic for the moment as the expected residency is used to
> > prevent the shutting down of L2 and the cluster if the residency for
> > the last man is lower than 5 ms.  To make this right, the shutdown
> > latency for each CPU would need to be recorded and taken into account.
> > 
> > On a suspend, the firmware mailbox address has to be set prior entering
> > low power mode.
> > 
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> >  arch/arm/mach-vexpress/tc2_pm.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> > index a3ea524372..7901528566 100644
> > --- a/arch/arm/mach-vexpress/tc2_pm.c
> > +++ b/arch/arm/mach-vexpress/tc2_pm.c
> > @@ -79,7 +79,7 @@ static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> >  	return 0;
> >  }
> >  
> > -static void tc2_pm_power_down(void)
> > +static void tc2_pm_down(u64 residency)
> >  {
> >  	unsigned int mpidr, cpu, cluster;
> >  	bool last_man = false, skip_wfi = false;
> > @@ -100,7 +100,8 @@ static void tc2_pm_power_down(void)
> >  		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> >  		if (!tc2_pm_use_count[0][cluster] &&
> >  		    !tc2_pm_use_count[1][cluster] &&
> > -		    !tc2_pm_use_count[2][cluster]) {
> > +		    !tc2_pm_use_count[2][cluster] &&
> > +		    (!residency || residency > 5000)) {
> 
> I would remove the line above altogether. It is ok to have parameter (we
> should still define what that residency actually is), I do not think it
> makes sense to add policy at this stage, I have a patchset to queue for
> that.

OK, let's discuss that separately.  I'll simply let the whole cluster go 
down in all cases initially.  That's what we have right now anyway as 
the cpuidle driver is not passing anything interesting at the moment 
other than 0.


Nicolas

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Pawel Moll]

On Mon, 2013-06-10 at 04:21 +0100, Nicolas Pitre wrote:
> You should do like some other companies does i.e. have an official name, 
> and an alias which is short and borrowed from an animal, or some 
> geographical region, etc.

Funny you mention this - this test chip _does_ have a code-name (and it
is a bird indeed, just mythical ;-). Somehow it didn't stuck.

Pawe?

[PATCH 2/2] ARM: vexpress/TC2: implement PM suspend method

[Lorenzo Pieralisi]

On Mon, Jun 10, 2013 at 04:56:12AM +0100, Nicolas Pitre wrote:
> On Fri, 7 Jun 2013, Lorenzo Pieralisi wrote:
> 
> > Hi Nico,
> > 
> > On Fri, Jun 07, 2013 at 07:39:12AM +0100, Nicolas Pitre wrote:
> > > From: Nicolas Pitre <nico@linaro.org>
> > > 
> > > This is simplistic for the moment as the expected residency is used to
> > > prevent the shutting down of L2 and the cluster if the residency for
> > > the last man is lower than 5 ms.  To make this right, the shutdown
> > > latency for each CPU would need to be recorded and taken into account.
> > > 
> > > On a suspend, the firmware mailbox address has to be set prior entering
> > > low power mode.
> > > 
> > > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > > ---
> > >  arch/arm/mach-vexpress/tc2_pm.c | 23 +++++++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> > > index a3ea524372..7901528566 100644
> > > --- a/arch/arm/mach-vexpress/tc2_pm.c
> > > +++ b/arch/arm/mach-vexpress/tc2_pm.c
> > > @@ -79,7 +79,7 @@ static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> > >  	return 0;
> > >  }
> > >  
> > > -static void tc2_pm_power_down(void)
> > > +static void tc2_pm_down(u64 residency)
> > >  {
> > >  	unsigned int mpidr, cpu, cluster;
> > >  	bool last_man = false, skip_wfi = false;
> > > @@ -100,7 +100,8 @@ static void tc2_pm_power_down(void)
> > >  		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> > >  		if (!tc2_pm_use_count[0][cluster] &&
> > >  		    !tc2_pm_use_count[1][cluster] &&
> > > -		    !tc2_pm_use_count[2][cluster]) {
> > > +		    !tc2_pm_use_count[2][cluster] &&
> > > +		    (!residency || residency > 5000)) {
> > 
> > I would remove the line above altogether. It is ok to have parameter (we
> > should still define what that residency actually is), I do not think it
> > makes sense to add policy at this stage, I have a patchset to queue for
> > that.
> 
> OK, let's discuss that separately.  I'll simply let the whole cluster go 
> down in all cases initially.  That's what we have right now anyway as 
> the cpuidle driver is not passing anything interesting at the moment 
> other than 0.

Agreed, thanks.

Lorenzo

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Sudeep KarkadaNagesha]

On 07/06/13 07:39, Nicolas Pitre wrote:
> This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
> aka TC2.  This provides cluster management for SMP secondary boot and
> CPU hotplug.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/arm/mach-vexpress/Kconfig  |   9 ++
>  arch/arm/mach-vexpress/Makefile |   1 +
>  arch/arm/mach-vexpress/tc2_pm.c | 243 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 253 insertions(+)
>  create mode 100644 arch/arm/mach-vexpress/tc2_pm.c
> 
[...]
> +/*
> + * Enable cluster-level coherency, in preparation for turning on the MMU.
> + */
> +static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
> +{
> +	asm volatile (" \n"
> +"	cmp	r0, #1 \n"
You may need Thumb2 if-then(IT) instruction to support longer branch
range here when compiled in THUMB2 mode.
"       it      eq \n"

> +"	beq	cci_enable_port_for_self \n"
> +"	bx	lr ");
> +}
> +

Regards,
Sudeep

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Lorenzo Pieralisi]

On Mon, Jun 10, 2013 at 04:53:12AM +0100, Nicolas Pitre wrote:
> On Fri, 7 Jun 2013, Lorenzo Pieralisi wrote:
> 
> > On Fri, Jun 07, 2013 at 07:39:11AM +0100, Nicolas Pitre wrote:
> > > +#include <mach/motherboard.h>
> >
> > Is the include above needed ?
> 
> Apparently not.
> 
> > > +static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> > > +{
> > > +   pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> > > +   if (cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster))
> > > +           return -EINVAL;
> >
> > We could stash (vexpress_spc_get_nb_cpus()), it never changes.
> 
> Well... sure.  However, given cpu_up is not a very hot path and because
> the cpu argument is externally provided in this case, I'm inclined to
> leave this instance as is.  I'll hardcode a constant in the other cases
> where the cpu number is obtained from the MPIDR and validated with
> BUG_ON() where this is done only to prevent overflowing the
> tc2_pm_use_count array if something very wrong happens.
> 
> Oh and I'll use defines for those constants as well.

It was just a minor nitpick, my fear was that the SPC read could be
stalled by a background write by the microcontroller (to another
SPC register - ie DVFS), but that should not happen. I will follow up.

> > > +   if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> > > +           arch_spin_unlock(&tc2_pm_lock);
> > > +
> > > +           set_cr(get_cr() & ~CR_C);
> >
> > We must disable L2 prefetching on A15 before cleaning L2.
> 
> OK.
> 
> > > +           flush_cache_all();
> > > +           asm volatile ("clrex");
> > > +           set_auxcr(get_auxcr() & ~(1 << 6));
> >
> > I think we should add comments here to avoid copy'n'paste mayhem. The
> > code above is safe on cpus like A15/A7 (I know this back-end can just
> > be run on those processors) that hit in the cache with C-bit in SCTLR
> > cleared, it would explode on processors (eg A9) that do not hit in the
> > cache with C-bit cleared. I am wondering if it is better to write inline
> > asm and jump to v7 cache functions that do not need stack push/pop
> > straight away.
> 
> Well... yeah.  This is unfortunate because we moved this part from
> assembly to C over time to clean it up.  But better use something safer
> in case it gets copied as you say.

Ok, thanks.

> > > +           cci_disable_port_by_cpu(mpidr);
> > > +
> > > +           /*
> > > +            * Ensure that both C & I bits are disabled in the SCTLR
> > > +            * before disabling ACE snoops. This ensures that no
> > > +            * coherency traffic will originate from this cpu after
> > > +            * ACE snoops are turned off.
> > > +            */
> > > +           cpu_proc_fin();
> >
> > Mmm, C bit is already cleared, why clear the I bit (and the A bit) ?
> > I do not think cpu_proc_fin() is needed and I am really keen on getting
> > the power down procedure right to avoid copy'n'paste induced error from
> > the start.
> 
> I trusted the above and its accompanying comment from Achin's initial
> cluster shutdown code.  But I suppose icache lookups don't create snoop
> requests?

I-cache misses trigger snoop requests to other cluster(s), but this is
still safe, since CCI disables the incoming snoops, not the outgoing
and the lines returned are driven on the AXI read channel, not through
the ACE port.

We can safely remove cpu_proc_fin.

> > > +           __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> > > +   } else {
> > > +           /*
> > > +            * If last man then undo any setup done previously.
> > > +            */
> > > +           if (last_man) {
> > > +                   vexpress_spc_powerdown_enable(cluster, 0);
> > > +                   vexpress_spc_set_global_wakeup_intr(0);
> > > +           }
> > > +
> > > +           arch_spin_unlock(&tc2_pm_lock);
> > > +
> > > +           set_cr(get_cr() & ~CR_C);
> > > +           flush_cache_louis();
> > > +           asm volatile ("clrex");
> > > +           set_auxcr(get_auxcr() & ~(1 << 6));
> > > +   }
> > > +
> > > +   __mcpm_cpu_down(cpu, cluster);
> > > +
> > > +   /* Now we are prepared for power-down, do it: */
> > > +   if (!skip_wfi)
> > > +           wfi();
> > > +
> > > +   /* Not dead at this point?  Let our caller cope. */
> >
> > This function should disable the GIC CPU IF, but I guess you will add
> > the code when CPUidle is merged.
> 
> The GIC code does not provide a hook for doing that at the moment.  That
> needs to be sorted out there before that can be added here.

Yes, that's agreed, it is not a big deal either.

> 
> [...]
> 
> OK, here's another version:
> 
> From: Nicolas Pitre <nicolas.pitre@linaro.org>
> Date: Fri, 19 Oct 2012 20:48:50 -0400
> Subject: [PATCH] ARM: vexpress/TC2: basic PM support
> 
> This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
> aka TC2.  This provides cluster management for SMP secondary boot and
> CPU hotplug.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> 
> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> index b8bbabec63..e7a825d7df 100644
> --- a/arch/arm/mach-vexpress/Kconfig
> +++ b/arch/arm/mach-vexpress/Kconfig
> @@ -66,4 +66,13 @@ config ARCH_VEXPRESS_DCSCB
>           This is needed to provide CPU and cluster power management
>           on RTSM implementing big.LITTLE.
> 
> +config ARCH_VEXPRESS_TC2
> +       bool "Versatile Express TC2 power management"
> +       depends on MCPM
> +       select VEXPRESS_SPC
> +       select ARM_CCI
> +       help
> +         Support for CPU and cluster power management on Versatile Express
> +         with a TC2 (A15x2 A7x3) big.LITTLE core tile.
> +
>  endmenu
> diff --git a/arch/arm/mach-vexpress/Makefile b/arch/arm/mach-vexpress/Makefile
> index 48ba89a814..b1cf227fa5 100644
> --- a/arch/arm/mach-vexpress/Makefile
> +++ b/arch/arm/mach-vexpress/Makefile
> @@ -7,5 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
>  obj-y                                  := v2m.o
>  obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)      += ct-ca9x4.o
>  obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)      += dcscb.o      dcscb_setup.o
> +obj-$(CONFIG_ARCH_VEXPRESS_TC2)                += tc2_pm.o
>  obj-$(CONFIG_SMP)                      += platsmp.o
>  obj-$(CONFIG_HOTPLUG_CPU)              += hotplug.o
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> new file mode 100644
> index 0000000000..f0673b4814
> --- /dev/null
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -0,0 +1,275 @@
> +/*
> + * arch/arm/mach-vexpress/tc2_pm.c - TC2 power management support
> + *
> + * Created by: Nicolas Pitre, October 2012
> + * Copyright:  (C) 2012-2013  Linaro Limited
> + *
> + * Some portions of this file were originally written by Achin Gupta
> + * Copyright:   (C) 2012  ARM Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include <linux/vexpress.h>
> +#include <linux/arm-cci.h>
> +
> +/*
> + * 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
> + * is already live using the same logical CPU number which trips lockdep
> + * debugging.
> + */
> +static arch_spinlock_t tc2_pm_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +
> +#define TC2_CLUSTERS                   2
> +#define TC2_MAX_CPUS_PER_CLUSTER       3
> +static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];
> +
> +static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +       pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +       if (cluster >= TC2_CLUSTERS || cpu >= vexpress_spc_get_nb_cpus(cluster))
> +               return -EINVAL;
> +
> +       /*
> +        * Since this is called with IRQs enabled, and no arch_spin_lock_irq
> +        * variant exists, we need to disable IRQs manually here.
> +        */
> +       local_irq_disable();
> +       arch_spin_lock(&tc2_pm_lock);
> +
> +       if (!tc2_pm_use_count[0][cluster] &&
> +           !tc2_pm_use_count[1][cluster] &&
> +           !tc2_pm_use_count[2][cluster])
> +               vexpress_spc_powerdown_enable(cluster, 0);
> +
> +       tc2_pm_use_count[cpu][cluster]++;
> +       if (tc2_pm_use_count[cpu][cluster] == 1) {
> +               vexpress_spc_write_resume_reg(cluster, cpu,
> +                                             virt_to_phys(mcpm_entry_point));
> +               vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +       } else if (tc2_pm_use_count[cpu][cluster] != 2) {
> +               /*
> +                * The only possible values are:
> +                * 0 = CPU down
> +                * 1 = CPU (still) up
> +                * 2 = CPU requested to be up before it had a chance
> +                *     to actually make itself down.
> +                * Any other value is a bug.
> +                */
> +               BUG();
> +       }
> +
> +       arch_spin_unlock(&tc2_pm_lock);
> +       local_irq_enable();
> +
> +       return 0;
> +}
> +
> +static void tc2_pm_power_down(void)
> +{
> +       unsigned int mpidr, cpu, cluster;
> +       bool last_man = false, skip_wfi = false;
> +
> +       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);
> +       BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
> +
> +       __mcpm_cpu_going_down(cpu, cluster);
> +
> +       arch_spin_lock(&tc2_pm_lock);
> +       BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> +       tc2_pm_use_count[cpu][cluster]--;
> +       if (tc2_pm_use_count[cpu][cluster] == 0) {
> +               vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +               if (!tc2_pm_use_count[0][cluster] &&
> +                   !tc2_pm_use_count[1][cluster] &&
> +                   !tc2_pm_use_count[2][cluster]) {
> +                       vexpress_spc_powerdown_enable(cluster, 1);
> +                       vexpress_spc_set_global_wakeup_intr(1);
> +                       last_man = true;
> +               }
> +       } else if (tc2_pm_use_count[cpu][cluster] == 1) {
> +               /*
> +                * A power_up request went ahead of us.
> +                * Even if we do not want to shut this CPU down,
> +                * the caller expects a certain state as if the WFI
> +                * was aborted.  So let's continue with cache cleaning.
> +                */
> +               skip_wfi = true;
> +       } else
> +               BUG();
> +
> +       if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> +               arch_spin_unlock(&tc2_pm_lock);
> +
> +               if ((read_cpuid_id() & 0xf0) == 0xf0) {
> +                       /*
> +                        * On the Cortex-A15 we need to disable
> +                        * L2 prefetching before flushing the cache.
> +                        */
> +                       asm volatile(
> +                       "mcr    p15, 1, %0, c15, c0, 3 \n\t"
> +                       "isb    \n\t"
> +                       "dsb    "
> +                       : : "r" (0x400) );
> +               }
> +
> +               /*
> +                * We need to disable and flush the whole (L1 and L2) cache.
> +                * Let's do it in the safest possible way i.e. with
> +                * no memory access within the following sequence,
> +                * including the stack.
> +                */
> +               asm volatile(
> +               "mrc    p15, 0, r0, c1, c0, 0   @ get CR \n\t"
> +               "bic    r0, r0, #"__stringify(CR_C)" \n\t"
> +               "mcr    p15, 0, r0, c1, c0, 0   @ set CR \n\t"
> +               "isb    \n\t"
> +               "bl     v7_flush_dcache_all \n\t"
> +               "clrex  \n\t"
> +               "mrc    p15, 0, r0, c1, c0, 1   @ get AUXCR \n\t"
> +               "bic    r0, r0, #(1 << 6)       @ disable local coherency \n\t"
> +               "mcr    p15, 0, r0, c1, c0, 1   @ set AUXCR \n\t"
> +               "isb    "

We need a dsb here, I know there is one before returning from the flush
routine though. Maybe add it as a comment please.

> +               : : : "r0","r1","r2","r3","r4","r5","r6","r7",
> +                     "r9","r10","r11","lr","memory");
> +
> +               cci_disable_port_by_cpu(mpidr);
> +
> +               __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> +       } else {
> +               /*
> +                * If last man then undo any setup done previously.
> +                */
> +               if (last_man) {
> +                       vexpress_spc_powerdown_enable(cluster, 0);
> +                       vexpress_spc_set_global_wakeup_intr(0);
> +               }
> +
> +               arch_spin_unlock(&tc2_pm_lock);
> +
> +               /*
> +                * We need to disable and flush only the L1 cache.
> +                * Let's do it in the safest possible way as above.
> +                */
> +               asm volatile(
> +               "mrc    p15, 0, r0, c1, c0, 0   @ get CR \n\t"
> +               "bic    r0, r0, #"__stringify(CR_C)" \n\t"
> +               "mcr    p15, 0, r0, c1, c0, 0   @ set CR \n\t"
> +               "isb    \n\t"
> +               "bl     v7_flush_dcache_louis \n\t"
> +               "clrex  \n\t"
> +               "mrc    p15, 0, r0, c1, c0, 1   @ get AUXCR \n\t"
> +               "bic    r0, r0, #(1 << 6)       @ disable local coherency \n\t"
> +               "mcr    p15, 0, r0, c1, c0, 1   @ set AUXCR \n\t"
> +               "isb    "

Ditto.

> +               : : : "r0","r1","r2","r3","r4","r5","r6","r7",
> +                     "r9","r10","r11","lr","memory");
> +       }
> +
> +       __mcpm_cpu_down(cpu, cluster);
> +
> +       /* Now we are prepared for power-down, do it: */
> +       if (!skip_wfi)
> +               wfi();
> +
> +       /* Not dead at this point?  Let our caller cope. */
> +}
> +
> +static void tc2_pm_powered_up(void)
> +{
> +       unsigned int mpidr, cpu, cluster;
> +       unsigned long flags;
> +
> +       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);
> +       BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
> +
> +       local_irq_save(flags);
> +       arch_spin_lock(&tc2_pm_lock);
> +
> +       if (!tc2_pm_use_count[0][cluster] &&
> +           !tc2_pm_use_count[1][cluster] &&
> +           !tc2_pm_use_count[2][cluster]) {
> +               vexpress_spc_powerdown_enable(cluster, 0);
> +               vexpress_spc_set_global_wakeup_intr(0);
> +       }
> +
> +       if (!tc2_pm_use_count[cpu][cluster])
> +               tc2_pm_use_count[cpu][cluster] = 1;
> +
> +       vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 0);
> +       vexpress_spc_write_resume_reg(cluster, cpu, 0);
> +
> +       arch_spin_unlock(&tc2_pm_lock);
> +       local_irq_restore(flags);
> +}
> +
> +static const struct mcpm_platform_ops tc2_pm_power_ops = {
> +       .power_up       = tc2_pm_power_up,
> +       .power_down     = tc2_pm_power_down,
> +       .powered_up     = tc2_pm_powered_up,
> +};
> +
> +static void __init tc2_pm_usage_count_init(void)
> +{
> +       unsigned int mpidr, cpu, cluster;
> +
> +       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);
> +       BUG_ON(cluster >= TC2_CLUSTERS || cpu >= TC2_MAX_CPUS_PER_CLUSTER);
> +       tc2_pm_use_count[cpu][cluster] = 1;
> +}
> +
> +/*
> + * Enable cluster-level coherency, in preparation for turning on the MMU.
> + */
> +static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
> +{
> +       asm volatile (" \n"
> +"      cmp     r0, #1 \n"
> +"      bxne    lr \n"
> +"      b       cci_enable_port_for_self ");
> +}
> +
> +static int __init tc2_pm_init(void)
> +{
> +       int ret;
> +
> +       if (!vexpress_spc_check_loaded())
> +               return -ENODEV;
> +
> +       tc2_pm_usage_count_init();
> +
> +       ret = mcpm_platform_register(&tc2_pm_power_ops);
> +       if (!ret)
> +               ret = mcpm_sync_init(tc2_pm_power_up_setup);

Minor nit: mcpm_sync_init always returns 0, so maybe we should remove that
return value altogether, otherwise it looks like we are not undoing the
platform registration on error.

> +       if (!ret)
> +               pr_info("TC2 power management initialized\n");
> +       return ret;
> +}
> +
> +early_initcall(tc2_pm_init);

Other than that, FWIW:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Mon, 10 Jun 2013, Sudeep KarkadaNagesha wrote:

> On 07/06/13 07:39, Nicolas Pitre wrote:
> > This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
> > aka TC2.  This provides cluster management for SMP secondary boot and
> > CPU hotplug.
> > 
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> > ---
> >  arch/arm/mach-vexpress/Kconfig  |   9 ++
> >  arch/arm/mach-vexpress/Makefile |   1 +
> >  arch/arm/mach-vexpress/tc2_pm.c | 243 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 253 insertions(+)
> >  create mode 100644 arch/arm/mach-vexpress/tc2_pm.c
> > 
> [...]
> > +/*
> > + * Enable cluster-level coherency, in preparation for turning on the MMU.
> > + */
> > +static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
> > +{
> > +	asm volatile (" \n"
> > +"	cmp	r0, #1 \n"
> > +"	beq	cci_enable_port_for_self \n"
> > +"	bx	lr ");
> You may need Thumb2 if-then(IT) instruction to support longer branch
> range here when compiled in THUMB2 mode.
> "       it      eq \n"

Yeah, I noticed, and therefore reworked it in my follow-up patch as 
follows:

	cmp	r0, #1
	bxne	lr
	b	cci_enable_port_for_self \n"


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Mon, 10 Jun 2013, Lorenzo Pieralisi wrote:

> > +               /*
> > +                * We need to disable and flush the whole (L1 and L2) cache.
> > +                * Let's do it in the safest possible way i.e. with
> > +                * no memory access within the following sequence,
> > +                * including the stack.
> > +                */
> > +               asm volatile(
> > +               "mrc    p15, 0, r0, c1, c0, 0   @ get CR \n\t"
> > +               "bic    r0, r0, #"__stringify(CR_C)" \n\t"
> > +               "mcr    p15, 0, r0, c1, c0, 0   @ set CR \n\t"
> > +               "isb    \n\t"
> > +               "bl     v7_flush_dcache_all \n\t"
> > +               "clrex  \n\t"
> > +               "mrc    p15, 0, r0, c1, c0, 1   @ get AUXCR \n\t"
> > +               "bic    r0, r0, #(1 << 6)       @ disable local coherency \n\t"
> > +               "mcr    p15, 0, r0, c1, c0, 1   @ set AUXCR \n\t"
> > +               "isb    "
> 
> We need a dsb here, I know there is one before returning from the flush
> routine though. Maybe add it as a comment please.

Is the dsb in v7_flush_dcache_all sufficient (hence a comment) or it 
needs to be located after a particular operation?

> Other than that, FWIW:
> 
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Thanks!


Nicolas

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Jon Medhurst (Tixy)]

On Fri, 2013-06-07 at 02:39 -0400, Nicolas Pitre wrote:
> This is the MCPM backend enabling SMP secondary boot and CPU hotplug
> on the VExpress TC2 big.LITTLE platform.  The method to allow CPU suspend
> is also implemented.

Are we leaving out for now Lorenzo's patch "ARM: TC2: reset CPUs
spuriously woken up on cluster power up"?

I got reminded of this after reading the comments on Mark Rutland's
"[PATCH] arm: versatile: don't mark pen as __INIT"

-- 
Tixy

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Lorenzo Pieralisi]

On Mon, Jun 10, 2013 at 11:39:11PM +0100, Nicolas Pitre wrote:
> On Mon, 10 Jun 2013, Lorenzo Pieralisi wrote:
> 
> > > +               /*
> > > +                * We need to disable and flush the whole (L1 and L2) cache.
> > > +                * Let's do it in the safest possible way i.e. with
> > > +                * no memory access within the following sequence,
> > > +                * including the stack.
> > > +                */
> > > +               asm volatile(
> > > +               "mrc    p15, 0, r0, c1, c0, 0   @ get CR \n\t"
> > > +               "bic    r0, r0, #"__stringify(CR_C)" \n\t"
> > > +               "mcr    p15, 0, r0, c1, c0, 0   @ set CR \n\t"
> > > +               "isb    \n\t"
> > > +               "bl     v7_flush_dcache_all \n\t"
> > > +               "clrex  \n\t"
> > > +               "mrc    p15, 0, r0, c1, c0, 1   @ get AUXCR \n\t"
> > > +               "bic    r0, r0, #(1 << 6)       @ disable local coherency \n\t"
> > > +               "mcr    p15, 0, r0, c1, c0, 1   @ set AUXCR \n\t"
> > > +               "isb    "
> > 
> > We need a dsb here, I know there is one before returning from the flush
> > routine though. Maybe add it as a comment please.
> 
> Is the dsb in v7_flush_dcache_all sufficient (hence a comment) or it 
> needs to be located after a particular operation?

Well I wanted to avoid too many dsb. But I think that a dsb after
exiting coherency is mandatory to avoid issues (after isb), hence it should
be added (I know as I said there is one just before returning from the

v7_flush_dcache_all/louis

functions), but we should not be playing with fire either, it boils down
to CPU microarchitectural details.

We cannot remove the dsb from the cache functions since they are meant
to be self-contained.

Lorenzo

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Tue, 11 Jun 2013, Lorenzo Pieralisi wrote:

> On Mon, Jun 10, 2013 at 11:39:11PM +0100, Nicolas Pitre wrote:
> > On Mon, 10 Jun 2013, Lorenzo Pieralisi wrote:
> > 
> > > > +               /*
> > > > +                * We need to disable and flush the whole (L1 and L2) cache.
> > > > +                * Let's do it in the safest possible way i.e. with
> > > > +                * no memory access within the following sequence,
> > > > +                * including the stack.
> > > > +                */
> > > > +               asm volatile(
> > > > +               "mrc    p15, 0, r0, c1, c0, 0   @ get CR \n\t"
> > > > +               "bic    r0, r0, #"__stringify(CR_C)" \n\t"
> > > > +               "mcr    p15, 0, r0, c1, c0, 0   @ set CR \n\t"
> > > > +               "isb    \n\t"
> > > > +               "bl     v7_flush_dcache_all \n\t"
> > > > +               "clrex  \n\t"
> > > > +               "mrc    p15, 0, r0, c1, c0, 1   @ get AUXCR \n\t"
> > > > +               "bic    r0, r0, #(1 << 6)       @ disable local coherency \n\t"
> > > > +               "mcr    p15, 0, r0, c1, c0, 1   @ set AUXCR \n\t"
> > > > +               "isb    "
> > > 
> > > We need a dsb here, I know there is one before returning from the flush
> > > routine though. Maybe add it as a comment please.
> > 
> > Is the dsb in v7_flush_dcache_all sufficient (hence a comment) or it 
> > needs to be located after a particular operation?
> 
> Well I wanted to avoid too many dsb. But I think that a dsb after
> exiting coherency is mandatory to avoid issues (after isb), hence it should
> be added (I know as I said there is one just before returning from the
> 
> v7_flush_dcache_all/louis
> 
> functions), but we should not be playing with fire either, it boils down
> to CPU microarchitectural details.
> 
> We cannot remove the dsb from the cache functions since they are meant
> to be self-contained.

OK, consider it added.


Nicolas

[PATCH 0/2] MCPM backend for Virtual Express TC2

[Nicolas Pitre]

On Tue, 11 Jun 2013, Jon Medhurst (Tixy) wrote:

> On Fri, 2013-06-07 at 02:39 -0400, Nicolas Pitre wrote:
> > This is the MCPM backend enabling SMP secondary boot and CPU hotplug
> > on the VExpress TC2 big.LITTLE platform.  The method to allow CPU suspend
> > is also implemented.
> 
> Are we leaving out for now Lorenzo's patch "ARM: TC2: reset CPUs
> spuriously woken up on cluster power up"?

I don't see spurious wake-ups with only hotplug operations.

So we'll have to reconsider if that patch is still needed once we merge
the cpuidle driver.  It certainly won't go into mainline in its current 
form in any case.

I do hope it is not required though -- that should be the job of the 
firmware.


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Olof Johansson]

Hi,


Some comments below.

On Fri, Jun 07, 2013 at 02:39:11AM -0400, Nicolas Pitre wrote:
> This is the MCPM backend for the Virtual Express A15x2 A7x3 CoreTile
> aka TC2.  This provides cluster management for SMP secondary boot and
> CPU hotplug.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>  arch/arm/mach-vexpress/Kconfig  |   9 ++
>  arch/arm/mach-vexpress/Makefile |   1 +
>  arch/arm/mach-vexpress/tc2_pm.c | 243 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 253 insertions(+)
>  create mode 100644 arch/arm/mach-vexpress/tc2_pm.c
> 
> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> index b8bbabec63..e7a825d7df 100644
> --- a/arch/arm/mach-vexpress/Kconfig
> +++ b/arch/arm/mach-vexpress/Kconfig
> @@ -66,4 +66,13 @@ config ARCH_VEXPRESS_DCSCB
>  	  This is needed to provide CPU and cluster power management
>  	  on RTSM implementing big.LITTLE.
>  
> +config ARCH_VEXPRESS_TC2
> +	bool "Versatile Express TC2 power management"

Should there be a _PM in the config option, perhaps? Or maybe take out the
power management one-line info if it's really just base support for the
platform.

> +	depends on MCPM
> +	select VEXPRESS_SPC
> +	select ARM_CCI
> +	help
> +	  Support for CPU and cluster power management on Versatile Express
> +	  with a TC2 (A15x2 A7x3) big.LITTLE core tile.
> +
>  endmenu
> diff --git a/arch/arm/mach-vexpress/Makefile b/arch/arm/mach-vexpress/Makefile
> index 48ba89a814..b1cf227fa5 100644
> --- a/arch/arm/mach-vexpress/Makefile
> +++ b/arch/arm/mach-vexpress/Makefile
> @@ -7,5 +7,6 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
>  obj-y					:= v2m.o
>  obj-$(CONFIG_ARCH_VEXPRESS_CA9X4)	+= ct-ca9x4.o
>  obj-$(CONFIG_ARCH_VEXPRESS_DCSCB)	+= dcscb.o	dcscb_setup.o
> +obj-$(CONFIG_ARCH_VEXPRESS_TC2)		+= tc2_pm.o
>  obj-$(CONFIG_SMP)			+= platsmp.o
>  obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> new file mode 100644
> index 0000000000..a3ea524372
> --- /dev/null
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -0,0 +1,243 @@
> +/*
> + * arch/arm/mach-vexpress/tc2_pm.c - TC2 power management support
> + *
> + * Created by:	Nicolas Pitre, October 2012
> + * Copyright:	(C) 2012-2013  Linaro Limited
> + *
> + * Some portions of this file were originally written by Achin Gupta
> + * Copyright:   (C) 2012  ARM Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include <mach/motherboard.h>
> +
> +#include <linux/vexpress.h>
> +#include <linux/arm-cci.h>
> +
> +/*
> + * 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
> + * is already live using the same logical CPU number which trips lockdep
> + * debugging.
> + */
> +static arch_spinlock_t tc2_pm_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +
> +static int tc2_pm_use_count[3][2];

A comment describing the purpose of this array would be much beneficial for
someone reading the driver.

> +
> +static int tc2_pm_power_up(unsigned int cpu, unsigned int cluster)
> +{
> +	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> +	if (cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster))
> +		return -EINVAL;
> +
> +	/*
> +	 * Since this is called with IRQs enabled, and no arch_spin_lock_irq
> +	 * variant exists, we need to disable IRQs manually here.
> +	 */
> +	local_irq_disable();
> +	arch_spin_lock(&tc2_pm_lock);
> +
> +	if (!tc2_pm_use_count[0][cluster] &&
> +	    !tc2_pm_use_count[1][cluster] &&
> +	    !tc2_pm_use_count[2][cluster])

I see this construct used three times open-coded like this in the file. Adding
a smaller helper with a descriptive name would be a good idea.

Same goes for other direct accesses to the array -- maybe a tiny helper to
set/get state? It might end up overabstracted that way though.

> +		vexpress_spc_powerdown_enable(cluster, 0);
> +
> +	tc2_pm_use_count[cpu][cluster]++;
> +	if (tc2_pm_use_count[cpu][cluster] == 1) {
> +		vexpress_spc_write_resume_reg(cluster, cpu,
> +					      virt_to_phys(mcpm_entry_point));
> +		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +	} else if (tc2_pm_use_count[cpu][cluster] != 2) {
> +		/*
> +		 * The only possible values are:
> +		 * 0 = CPU down
> +		 * 1 = CPU (still) up
> +		 * 2 = CPU requested to be up before it had a chance
> +		 *     to actually make itself down.
> +		 * Any other value is a bug.
> +		 */
> +		BUG();
> +	}
> +
> +	arch_spin_unlock(&tc2_pm_lock);
> +	local_irq_enable();
> +
> +	return 0;
> +}
> +
> +static void tc2_pm_power_down(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	bool last_man = false, skip_wfi = false;
> +
> +	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);
> +	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));
> +
> +	__mcpm_cpu_going_down(cpu, cluster);
> +
> +	arch_spin_lock(&tc2_pm_lock);
> +	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> +	tc2_pm_use_count[cpu][cluster]--;
> +	if (tc2_pm_use_count[cpu][cluster] == 0) {
> +		vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 1);
> +		if (!tc2_pm_use_count[0][cluster] &&
> +		    !tc2_pm_use_count[1][cluster] &&
> +		    !tc2_pm_use_count[2][cluster]) {
> +			vexpress_spc_powerdown_enable(cluster, 1);
> +			vexpress_spc_set_global_wakeup_intr(1);
> +			last_man = true;
> +		}
> +	} else if (tc2_pm_use_count[cpu][cluster] == 1) {
> +		/*
> +		 * A power_up request went ahead of us.
> +		 * Even if we do not want to shut this CPU down,
> +		 * the caller expects a certain state as if the WFI
> +		 * was aborted.  So let's continue with cache cleaning.
> +		 */
> +		skip_wfi = true;
> +	} else
> +		BUG();
> +
> +	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> +		arch_spin_unlock(&tc2_pm_lock);
> +
> +		set_cr(get_cr() & ~CR_C);
> +		flush_cache_all();
> +		asm volatile ("clrex");
> +		set_auxcr(get_auxcr() & ~(1 << 6));
> +
> +		cci_disable_port_by_cpu(mpidr);
> +
> +		/*
> +		 * Ensure that both C & I bits are disabled in the SCTLR
> +		 * before disabling ACE snoops. This ensures that no
> +		 * coherency traffic will originate from this cpu after
> +		 * ACE snoops are turned off.
> +		 */
> +		cpu_proc_fin();
> +
> +		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> +	} else {
> +		/*
> +		 * If last man then undo any setup done previously.
> +		 */
> +		if (last_man) {
> +			vexpress_spc_powerdown_enable(cluster, 0);
> +			vexpress_spc_set_global_wakeup_intr(0);
> +		}
> +
> +		arch_spin_unlock(&tc2_pm_lock);
> +
> +		set_cr(get_cr() & ~CR_C);
> +		flush_cache_louis();
> +		asm volatile ("clrex");
> +		set_auxcr(get_auxcr() & ~(1 << 6));
> +	}
> +
> +	__mcpm_cpu_down(cpu, cluster);
> +
> +	/* Now we are prepared for power-down, do it: */
> +	if (!skip_wfi)
> +		wfi();
> +
> +	/* Not dead at this point?  Let our caller cope. */
> +}
> +
> +static void tc2_pm_powered_up(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +	unsigned long flags;
> +
> +	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);
> +	BUG_ON(cluster >= 2 || cpu >= vexpress_spc_get_nb_cpus(cluster));
> +
> +	local_irq_save(flags);
> +	arch_spin_lock(&tc2_pm_lock);
> +
> +	if (!tc2_pm_use_count[0][cluster] &&
> +	    !tc2_pm_use_count[1][cluster] &&
> +	    !tc2_pm_use_count[2][cluster]) {
> +		vexpress_spc_powerdown_enable(cluster, 0);
> +		vexpress_spc_set_global_wakeup_intr(0);
> +	}
> +
> +	if (!tc2_pm_use_count[cpu][cluster])
> +		tc2_pm_use_count[cpu][cluster] = 1;
> +
> +	vexpress_spc_set_cpu_wakeup_irq(cpu, cluster, 0);
> +	vexpress_spc_write_resume_reg(cluster, cpu, 0);
> +
> +	arch_spin_unlock(&tc2_pm_lock);
> +	local_irq_restore(flags);
> +}
> +
> +static const struct mcpm_platform_ops tc2_pm_power_ops = {
> +	.power_up	= tc2_pm_power_up,
> +	.power_down	= tc2_pm_power_down,
> +	.powered_up	= tc2_pm_powered_up,
> +};
> +
> +static void __init tc2_pm_usage_count_init(void)
> +{
> +	unsigned int mpidr, cpu, cluster;
> +
> +	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);
> +	BUG_ON(cpu >= 3 || cluster >= 2);

A warning saying that this hardware configuration is not supported would maybe
be more informal for an user trying to boot some mythical future hardware with
a too-old kernel.

> +	tc2_pm_use_count[cpu][cluster] = 1;
> +}
> +
> +/*
> + * Enable cluster-level coherency, in preparation for turning on the MMU.
> + */
> +static void __naked tc2_pm_power_up_setup(unsigned int affinity_level)
> +{
> +	asm volatile (" \n"
> +"	cmp	r0, #1 \n"
> +"	beq	cci_enable_port_for_self \n"
> +"	bx	lr ");
> +}
> +
> +static int __init tc2_pm_init(void)
> +{
> +	int ret;
> +
> +	if (!vexpress_spc_check_loaded())
> +		return -ENODEV;
> +
> +	tc2_pm_usage_count_init();
> +
> +	ret = mcpm_platform_register(&tc2_pm_power_ops);
> +	if (!ret)
> +		ret = mcpm_sync_init(tc2_pm_power_up_setup);
> +	if (!ret)
> +		pr_info("TC2 power management initialized\n");
> +	return ret;
> +}
> +
> +early_initcall(tc2_pm_init);
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Thu, 13 Jun 2013, Olof Johansson wrote:

> > +config ARCH_VEXPRESS_TC2
> > +	bool "Versatile Express TC2 power management"
> 
> Should there be a _PM in the config option, perhaps? Or maybe take out the
> power management one-line info if it's really just base support for the
> platform.

CONFIG_ARCH_VEXPRESS_TC2_PM it now is.

> > +static int tc2_pm_use_count[3][2];
> 
> A comment describing the purpose of this array would be much beneficial for
> someone reading the driver.

You are right.  It now looks like this:

#define TC2_CLUSTERS                    2
#define TC2_MAX_CPUS_PER_CLUSTER        3

/* Keep per-cpu usage count to cope with unordered up/down requests */
static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];

#define tc2_cluster_unused(cluster) \
        (!tc2_pm_use_count[0][cluster] && \
         !tc2_pm_use_count[1][cluster] && \
         !tc2_pm_use_count[2][cluster])

> > +	if (!tc2_pm_use_count[0][cluster] &&
> > +	    !tc2_pm_use_count[1][cluster] &&
> > +	    !tc2_pm_use_count[2][cluster])
> 
> I see this construct used three times open-coded like this in the file. Adding
> a smaller helper with a descriptive name would be a good idea.

Good observation.  Being too close to this code makes those issues 
invisible.  Hence the tc2_cluster_unused() above.

> Same goes for other direct accesses to the array -- maybe a tiny helper to
> set/get state? It might end up overabstracted that way though.

I would think so.

> > +static void __init tc2_pm_usage_count_init(void)
> > +{
> > +	unsigned int mpidr, cpu, cluster;
> > +
> > +	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);
> > +	BUG_ON(cpu >= 3 || cluster >= 2);
> 
> A warning saying that this hardware configuration is not supported would maybe
> be more informal for an user trying to boot some mythical future hardware with
> a too-old kernel.

Hmmm... OK.


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Dave P Martin]

On Thu, Jun 13, 2013 at 06:31:32PM -0400, Nicolas Pitre wrote:
> On Thu, 13 Jun 2013, Olof Johansson wrote:

[...]

> /* Keep per-cpu usage count to cope with unordered up/down requests */
> static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];
> 
> #define tc2_cluster_unused(cluster) \
>         (!tc2_pm_use_count[0][cluster] && \
>          !tc2_pm_use_count[1][cluster] && \
>          !tc2_pm_use_count[2][cluster])
> 
> > > +	if (!tc2_pm_use_count[0][cluster] &&
> > > +	    !tc2_pm_use_count[1][cluster] &&
> > > +	    !tc2_pm_use_count[2][cluster])
> > 
> > I see this construct used three times open-coded like this in the file. Adding
> > a smaller helper with a descriptive name would be a good idea.
> 
> Good observation.  Being too close to this code makes those issues 
> invisible.  Hence the tc2_cluster_unused() above.
> 
> > Same goes for other direct accesses to the array -- maybe a tiny helper to
> > set/get state? It might end up overabstracted that way though.
> 
> I would think so.

True, but it's worth noting that most mcpm backends (certainly any
native backends) will end up duplicating this pattern.  We already
have it for the fast model and for TC2.


In this code, the use counts are doing basically the same thing as
the low-level mcpm coordination, but to coordinate things that can
happen while all affected CPUs' MMUs are on -- this we can use
proper spinlocks and be nice and efficient.  But there are still
parallels between the operations on the use counts array and the
low-level __mcpm_ state machine helpers.

There are some interesting wrinkles here, such as the fact that
tc2_pm_powered_up() chooses a first man to perform some post-MMU
actions using regular spinlocking and the use counts... but this
isn't necessarily the same CPU selected as first man by the
low-level pre-MMU synchronisation.  And that's not a bug, because
we want the winner at each stage to dash across the finish line:
waiting for the runners-up just incurs latency.


Much about this feels like it's solving a common but non-trivial
problem, so there could be an argument for having more framework, or
otherwise documenting it a bit more.

We seem to have the following basic operations:

	lock_for_power_down(cpu, cluster) ->

		cluster and CPU need to be powered on, lock is held for both;
		CPU needs to be powered on, lock is held for it; or
		CPU is already on, so do nothing, lock is not held.

	lock_for_power_up(cpu, cluster) ->

		CPU and cluster need to be powered down (last man), lock is held for both; or
		CPU needs to be powered down, lock is held for it; or
		Someone is already turning the CPU on, so do nothing, lock is not held.

	lock_for_setup(cpu, cluster) ->

		cluster needs to be put into the powered-on state (post-MMU first man), lock is held for it.
		cluster is already in the powrered-on state, so do nothing, lock is not held.

	unlock()

		guess


These may not be great names, but hopefully you get the idea.

They correspond to the operations done in .power_up(), .power_down()
and .powered_up() respectively.

Cheers
---Dave

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Tue, 18 Jun 2013, Dave P Martin wrote:

> On Thu, Jun 13, 2013 at 06:31:32PM -0400, Nicolas Pitre wrote:
> > On Thu, 13 Jun 2013, Olof Johansson wrote:
> 
> [...]
> 
> > /* Keep per-cpu usage count to cope with unordered up/down requests */
> > static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];
> > 
> > #define tc2_cluster_unused(cluster) \
> >         (!tc2_pm_use_count[0][cluster] && \
> >          !tc2_pm_use_count[1][cluster] && \
> >          !tc2_pm_use_count[2][cluster])
> > 
> > > > +	if (!tc2_pm_use_count[0][cluster] &&
> > > > +	    !tc2_pm_use_count[1][cluster] &&
> > > > +	    !tc2_pm_use_count[2][cluster])
> > > 
> > > I see this construct used three times open-coded like this in the file. Adding
> > > a smaller helper with a descriptive name would be a good idea.
> > 
> > Good observation.  Being too close to this code makes those issues 
> > invisible.  Hence the tc2_cluster_unused() above.
> > 
> > > Same goes for other direct accesses to the array -- maybe a tiny helper to
> > > set/get state? It might end up overabstracted that way though.
> > 
> > I would think so.
> 
> True, but it's worth noting that most mcpm backends (certainly any
> native backends) will end up duplicating this pattern.  We already
> have it for the fast model and for TC2.

We do not in the PSCI backend though.

> In this code, the use counts are doing basically the same thing as
> the low-level mcpm coordination, but to coordinate things that can
> happen while all affected CPUs' MMUs are on -- this we can use
> proper spinlocks and be nice and efficient.  But there are still
> parallels between the operations on the use counts array and the
> low-level __mcpm_ state machine helpers.

Sort of.  The use counts are needed to determine who is becoming the 
last man.  That's the sole purpose of it.  And then only the last man is 
allowed to invoke MCPM state machine helpers changing the cluster state.

> There are some interesting wrinkles here, such as the fact that
> tc2_pm_powered_up() chooses a first man to perform some post-MMU
> actions using regular spinlocking and the use counts... but this
> isn't necessarily the same CPU selected as first man by the
> low-level pre-MMU synchronisation.  And that's not a bug, because
> we want the winner at each stage to dash across the finish line:
> waiting for the runners-up just incurs latency.

Exact.

> Much about this feels like it's solving a common but non-trivial
> problem, so there could be an argument for having more framework, or
> otherwise documenting it a bit more.

Well... right now I think it is hard to abstract things properly as we 
have very few backends.  I would like to see more of them first.

For something that can be done next, it is probably about moving that 
use count handling up the stack and out of backends.  I don't think we 
should create more helpers for backends to use and play more ping pong 
between the core and backends. The core MCPM layer could well take care 
of that last man determination directly with no abstraction at all and 
simply pass a last_man flag to simpler backends.  Initially the last man 
was determined directly from some hardware state but the handling of 
unordered up/down requests needed more than just a binary state.


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Olof Johansson]

I agree with most of the thread so far, but just wanted to highlight this:

On Tue, Jun 18, 2013 at 12:50 PM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:

> Well... right now I think it is hard to abstract things properly as we
> have very few backends.  I would like to see more of them first.

Yes. This.

And this is why it's critical, absolutely critical, that the first one
or two implementations go in are as simple and clean as possible.

Otherwise seeing the commonalities and how to abstract them out will
be a complete headache, since only the person writing the driver will
be able to get a good grasp of it (alternatively, it will take a lot
of effort for someone to figure out, clean up, and see the bigger
picture).


-Olof

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Nicolas Pitre]

On Tue, 18 Jun 2013, Olof Johansson wrote:

> I agree with most of the thread so far, but just wanted to highlight this:
> 
> On Tue, Jun 18, 2013 at 12:50 PM, Nicolas Pitre
> <nicolas.pitre@linaro.org> wrote:
> 
> > Well... right now I think it is hard to abstract things properly as we
> > have very few backends.  I would like to see more of them first.
> 
> Yes. This.
> 
> And this is why it's critical, absolutely critical, that the first one
> or two implementations go in are as simple and clean as possible.

I'll claim that this is rather utopic.  Simply because, once again, we 
don't know how to do so yet.  This is an iterative process and things 
will converge eventually.  But right now, even if the code is not 
necessarily as simple and clean as possible, it is certainly good enough 
for a start.

The perfect is the worst enemy of the good enough.

> Otherwise seeing the commonalities and how to abstract them out will
> be a complete headache, since only the person writing the driver will
> be able to get a good grasp of it (alternatively, it will take a lot
> of effort for someone to figure out, clean up, and see the bigger
> picture).

I don't see things as dramatically.  Probably because I'm one of the 
people who worked directly on this stuff.  And I don't intend to 
disappear after the code hits mainline either.  I therefore don't expect 
things to get out of control.  The job doesn't end there.

We do have ideas about improving things and the discussion is happening.  
But it would be wrong to wait until we can't think of any way to improve 
the code any further before merging it.  Otherwise even btrfs would 
still be out of tree.

The danger right now is that other people will reinvent their own layer 
because they have no model to follow at all.  That is even worse than 
having an imperfect model IMHO.

What we need is more exposure of this code.  It needs to be used and 
fiddled with by more people.  It has lived out of tree for a year 
already, and it would be about time its evolution happens in mainline 
from now on.

Let's also not forget that, right now, TC2 is the only fully functional 
big.LITTLE hardware implementation available to people.  Having it 
supported in mainline, even if not optimally at first, would greatly 
help other issues such as scheduler improvements for hybrid 
multiprocessing which are far more important than this code, as such 
platform could be distributed to scheduler people who might not always 
be bothered by low-level PM details and out-of-tree patches.

Please, understand that I fully appreciate the level of review and 
scrutiny that you provide and you do a very good job at it. However some 
accommodations for a compromise would be appreciated for the greater 
good here.


Nicolas

[PATCH 1/2] ARM: vexpress/TC2: basic PM support

[Dave P Martin]

On Tue, Jun 18, 2013 at 03:50:33PM -0400, Nicolas Pitre wrote:
> On Tue, 18 Jun 2013, Dave P Martin wrote:
> 
> > On Thu, Jun 13, 2013 at 06:31:32PM -0400, Nicolas Pitre wrote:
> > > On Thu, 13 Jun 2013, Olof Johansson wrote:
> > 
> > [...]
> > 
> > > /* Keep per-cpu usage count to cope with unordered up/down requests */
> > > static int tc2_pm_use_count[TC2_MAX_CPUS_PER_CLUSTER][TC2_CLUSTERS];
> > > 
> > > #define tc2_cluster_unused(cluster) \
> > >         (!tc2_pm_use_count[0][cluster] && \
> > >          !tc2_pm_use_count[1][cluster] && \
> > >          !tc2_pm_use_count[2][cluster])
> > > 
> > > > > +	if (!tc2_pm_use_count[0][cluster] &&
> > > > > +	    !tc2_pm_use_count[1][cluster] &&
> > > > > +	    !tc2_pm_use_count[2][cluster])
> > > > 
> > > > I see this construct used three times open-coded like this in the file. Adding
> > > > a smaller helper with a descriptive name would be a good idea.
> > > 
> > > Good observation.  Being too close to this code makes those issues 
> > > invisible.  Hence the tc2_cluster_unused() above.
> > > 
> > > > Same goes for other direct accesses to the array -- maybe a tiny helper to
> > > > set/get state? It might end up overabstracted that way though.
> > > 
> > > I would think so.
> > 
> > True, but it's worth noting that most mcpm backends (certainly any
> > native backends) will end up duplicating this pattern.  We already
> > have it for the fast model and for TC2.
> 
> We do not in the PSCI backend though.
> 
> > In this code, the use counts are doing basically the same thing as
> > the low-level mcpm coordination, but to coordinate things that can
> > happen while all affected CPUs' MMUs are on -- this we can use
> > proper spinlocks and be nice and efficient.  But there are still
> > parallels between the operations on the use counts array and the
> > low-level __mcpm_ state machine helpers.
> 
> Sort of.  The use counts are needed to determine who is becoming the 
> last man.  That's the sole purpose of it.  And then only the last man is 
> allowed to invoke MCPM state machine helpers changing the cluster state.
> 
> > There are some interesting wrinkles here, such as the fact that
> > tc2_pm_powered_up() chooses a first man to perform some post-MMU
> > actions using regular spinlocking and the use counts... but this
> > isn't necessarily the same CPU selected as first man by the
> > low-level pre-MMU synchronisation.  And that's not a bug, because
> > we want the winner at each stage to dash across the finish line:
> > waiting for the runners-up just incurs latency.
> 
> Exact.

(So, when you say the counts are only used to choose the last man, I
don't think it's quite that simple:

in connection with the spinlock, they also do first-man/last-man
coordination, and powerdown preemption at the CPU and cluster levels...
so it really does shadow the low-level state machine.  But that's just
my interpretation, and whether it's a good interpretation will depend on
whether it fits future backends ... which we don't fully know yet.)

> > Much about this feels like it's solving a common but non-trivial
> > problem, so there could be an argument for having more framework, or
> > otherwise documenting it a bit more.
> 
> Well... right now I think it is hard to abstract things properly as we 
> have very few backends.  I would like to see more of them first.

Agreed: that seems a reasonable stance.

This won't turn into a free-for-all unless we allow it to.

> For something that can be done next, it is probably about moving that 
> use count handling up the stack and out of backends.  I don't think we 
> should create more helpers for backends to use and play more ping pong 
> between the core and backends. The core MCPM layer could well take care 
> of that last man determination directly with no abstraction at all and 
> simply pass a last_man flag to simpler backends.  Initially the last man 
> was determined directly from some hardware state but the handling of 
> unordered up/down requests needed more than just a binary state.

Agreed.  Since all the mcpm methods start with code like this to determine
what to do and obtain some sort of lock, there's no reason why that can't
disappear into the framework.

A helper might be needed to do the unlock for the powerdown path, though
this might be combined with the existing helper methods to some extent.


This needs a bit of careful thought though, and I think if we wait for
one or two more backends to emerge, then that thought would be better
informed.

Cheers
---Dave