MCPM backend for Exynos5420

From: Andrew Bresticker <[email protected]>

Do not enable the big.LITTLE switcher on systems that do not have an ARM CCI (cache-coherent interconnect) present. Since the CCI is used to maintain cache coherency between multiple clusters and peripherals, it is unlikely that a system without CCI would support big.LITTLE.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 arch/arm/common/bL_switcher.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/arm/common/bL_switcher.c +12 -0

index 5774b6e..c4fec1f 100644
@@ -33,6 +33,7 @@
 #include <linux/sysfs.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 
 #include <asm/smp_plat.h>
 #include <asm/cputype.h>

@@ -796,6 +797,17 @@ core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
 static int __init bL_switcher_init(void)
 {
 	int ret;
+	struct device_node *dn;
+
+	/*
+	 * We don't want to set up the bL switcher if the machine doesn't
+	 * support bL, so use the presence of a CCI to indicate whether or
+	 * not bL is supported on this device.
+	 */
+	dn = of_find_compatible_node(NULL, NULL, "arm,cci-400");
+	if (!dn)
+		return 0;
+	of_node_put(dn);
 
 	if (MAX_NR_CLUSTERS != 2) {
 		pr_err("%s: only dual cluster systems are supported\n", __func__);

Signature · 1 line

1.7.9.5

Quoted 9 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

Do not enable the big.LITTLE switcher on systems that do not have an ARM CCI (cache-coherent interconnect) present. Since the CCI is used to maintain cache coherency between multiple clusters and peripherals, it is unlikely that a system without CCI would support big.LITTLE.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

The b.L switcher depends on MCPM, and it also expects only 2 clusters which is evaluated at run time or it bails out.

There might be in theory other ways than the CCI to enforce coherency between clusters. And that should all be encapsulated by the MCPM layer. The switcher module should not be concerned at all by the underlying hardware mechanism.

So if the goal is to determine at run time whether or not the switcher should be activated in a multi-config kernel, then the criteria should be whether or not MCPM is initialized, and not if there is a CCI.

Quoted 36 lines

---
 arch/arm/common/bL_switcher.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/arm/common/bL_switcher.c +12 -1

index 5774b6e..c4fec1f 100644
@@ -33,6 +33,7 @@
 #include <linux/sysfs.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 
 #include <asm/smp_plat.h>
 #include <asm/cputype.h>

@@ -796,6 +797,17 @@ core_param(no_bL_switcher, no_bL_switcher, bool, 0644);
 static int __init bL_switcher_init(void)
 {
      int ret;
+     struct device_node *dn;
+
+     /*
+      * We don't want to set up the bL switcher if the machine doesn't
+      * support bL, so use the presence of a CCI to indicate whether or
+      * not bL is supported on this device.
+      */
+     dn = of_find_compatible_node(NULL, NULL, "arm,cci-400");
+     if (!dn)
+             return 0;
+     of_node_put(dn);
 
      if (MAX_NR_CLUSTERS != 2) {
              pr_err("%s: only dual cluster systems are supported\n", __func__);
--

Quoted 2 lines

1.7.9.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Nicolas,

Quoted 15 lines · Nicolas Pitre

Quoted 9 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

Do not enable the big.LITTLE switcher on systems that do not have an ARM CCI (cache-coherent interconnect) present. Since the CCI is used to maintain cache coherency between multiple clusters and peripherals, it is unlikely that a system without CCI would support big.LITTLE.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

The b.L switcher depends on MCPM, and it also expects only 2 clusters which is evaluated at run time or it bails out.

Quoted 8 lines

There might be in theory other ways than the CCI to enforce coherency between clusters. And that should all be encapsulated by the MCPM layer. The switcher module should not be concerned at all by the underlying hardware mechanism.

So if the goal is to determine at run time whether or not the switcher should be activated in a multi-config kernel, then the criteria should be whether or not MCPM is initialized, and not if there is a CCI.

Yes, we have a multi-SoC enabled kernel (with MCPM and BL_SWITCHER configs enabled); one SoC has a single cluster while the other is a dual cluster. We wanted a run-time check to prevent bL_switcher from running on the single cluster one and zeroed in on CCI. But, I get your point as to why CCI should not be used as a distinguishing factor for switcher initialization.

For now, I can use the no_bL_switcher parameter to disable it on certain platforms. However, can you elaborate on the MCPM initialization check you suggested ?

Quoted 35 lines · Abhilash Kesavan

Hi Nicolas,

Quoted 15 lines · Nicolas Pitre

Quoted 9 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

Do not enable the big.LITTLE switcher on systems that do not have an ARM CCI (cache-coherent interconnect) present. Since the CCI is used to maintain cache coherency between multiple clusters and peripherals, it is unlikely that a system without CCI would support big.LITTLE.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

The b.L switcher depends on MCPM, and it also expects only 2 clusters which is evaluated at run time or it bails out.

Quoted 8 lines

There might be in theory other ways than the CCI to enforce coherency between clusters. And that should all be encapsulated by the MCPM layer. The switcher module should not be concerned at all by the underlying hardware mechanism.

So if the goal is to determine at run time whether or not the switcher should be activated in a multi-config kernel, then the criteria should be whether or not MCPM is initialized, and not if there is a CCI.

Yes, we have a multi-SoC enabled kernel (with MCPM and BL_SWITCHER configs enabled); one SoC has a single cluster while the other is a dual cluster. We wanted a run-time check to prevent bL_switcher from running on the single cluster one and zeroed in on CCI. But, I get your point as to why CCI should not be used as a distinguishing factor for switcher initialization.

Quoted 3 lines

For now, I can use the no_bL_switcher parameter to disable it on certain platforms. However, can you elaborate on the MCPM initialization check you suggested ?

Here's what I mean:

----- >8

From: Nicolas Pitre <[email protected]>
Date: Wed, 16 Apr 2014 15:43:59 -0400
Subject: [PATCH] ARM: bL_switcher: fix validation check before its activation

The switcher should not depend on MAX_CLUSTER which is a build time limit to determine ifit should be activated or not. In a multiplatform kernel binary it is possible to have dual-cluster and quad-cluster platforms configured in. In that case MAX_CLUSTER should be 4 and that shouldn't prevent the switcher from working if the kernel is booted on a b.L dual-cluster system.

In bL_switcher_halve_cpus() we already have a runtime validation check to make sure we're dealing with only two clusters, so booting on a quad cluster system will be caught and switcher activation aborted.

However, the b.L switcher must ensure the MCPM layer is initialized on the booted hardware before doing anything. The mcpm_is_available() function is added to that effect.

Signed-off-by Nicolas Pitre <[email protected]>

arch/arm/common/bL_switcher.c +2 -4

index 5774b6ea7a..f01c0ee0c8 100644
@@ -797,10 +797,8 @@ static int __init bL_switcher_init(void)
 {
        int ret;
 
-       if (MAX_NR_CLUSTERS != 2) {
-               pr_err("%s: only dual cluster systems are supported\n", __func__);
-               return -EINVAL;
-       }
+       if (!mcpm_is_available())
+               return -ENODEV;
 
        cpu_notifier(bL_switcher_hotplug_callback, 0);
 

arch/arm/common/mcpm_entry.c +5 -0

index 1e361abc29..86fd60fefb 100644
@@ -48,6 +48,11 @@ int __init mcpm_platform_register(const struct mcpm_platform_ops *ops)
        return 0;
 }
 
+bool mcpm_is_available(void)
+{
+       return (platform_ops) ? true : false;
+}
+
 int mcpm_cpu_power_up(unsigned int cpu, unsigned int cluster)
 {
        if (!platform_ops)

arch/arm/include/asm/mcpm.h +7 -0

index 608516ebab..a5ff410dcd 100644
@@ -54,6 +54,13 @@ void mcpm_set_early_poke(unsigned cpu, unsigned cluster,
  */
 
 /**
+ * mcpm_is_available - returns whether MCPM is initialized and available
+ *
+ * This returns true or false accordingly.
+ */
+bool mcpm_is_available(void);
+
+/**
  * mcpm_cpu_power_up - make given CPU in given cluster runable
  *
  * @cpu: CPU number within given cluster

Hi Nicolas,

Quoted 23 lines · Nicolas Pitre

Here's what I mean:

----- >8

From: Nicolas Pitre <[email protected]>
Date: Wed, 16 Apr 2014 15:43:59 -0400
Subject: [PATCH] ARM: bL_switcher: fix validation check before its activation

The switcher should not depend on MAX_CLUSTER which is a build time limit to determine ifit should be activated or not. In a multiplatform kernel binary it is possible to have dual-cluster and quad-cluster platforms configured in. In that case MAX_CLUSTER should be 4 and that shouldn't prevent the switcher from working if the kernel is booted on a b.L dual-cluster system.

In bL_switcher_halve_cpus() we already have a runtime validation check to make sure we're dealing with only two clusters, so booting on a quad cluster system will be caught and switcher activation aborted.

However, the b.L switcher must ensure the MCPM layer is initialized on the booted hardware before doing anything. The mcpm_is_available() function is added to that effect.

Quoted 1 line

Signed-off-by Nicolas Pitre <[email protected]>

Thank you for the explanation and patch. I have tested this on our multi-platform configuration and it works fine - activating the switcher on one SoC and not on the other.

Even though it is not the case now, could we have a scenario where we may use mcpm for only secondary core boot-up on one SoC and for switching on another ? I would then have mcpm ops populated for both but still want bL switcher activated for only one of them.

From: Andrew Bresticker <[email protected]>

cci_disable_port_by_cpu() can be used to disable an arbitrary ACE port, but there is no C-callable way to enable an ACE port. Change cci_disable_port_by_cpu() to cci_control_port_by_cpu() to allow us to disable and enable a CPU's ACE port.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/bus/arm-cci.c   |   13 +++++++------
 include/linux/arm-cci.h |    7 +++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

drivers/bus/arm-cci.c +7 -6

index 5a86da9..5668b40 100644
@@ -763,10 +763,11 @@ static void notrace cci_port_control(unsigned int port, bool enable)
 }
 
 /**
- * cci_disable_port_by_cpu() - function to disable a CCI port by CPU
+ * cci_control_port_by_cpu() - function to control a CCI port by CPU
  *			       reference
  *
- * @mpidr: mpidr of the CPU whose CCI port should be disabled
+ * @mpidr: mpidr of the CPU whose CCI port should be enabled/disabled
+ * @enable: if true enables the port, if false disables it
  *
  * Disabling a CCI port for a CPU implies disabling the CCI port
  * controlling that CPU cluster. Code disabling CPU CCI ports

@@ -777,20 +778,20 @@ static void notrace cci_port_control(unsigned int port, bool enable)
  *	0 on success
  *	-ENODEV on port look-up failure
  */
-int notrace cci_disable_port_by_cpu(u64 mpidr)
+int notrace cci_control_port_by_cpu(u64 mpidr, bool enable)
 {
 	int cpu;
 	bool is_valid;
 	for (cpu = 0; cpu < nr_cpu_ids; cpu++) {
 		is_valid = cpu_port_is_valid(&cpu_port[cpu]);
 		if (is_valid && cpu_port_match(&cpu_port[cpu], mpidr)) {
-			cci_port_control(cpu_port[cpu].port, false);
+			cci_port_control(cpu_port[cpu].port, enable);
 			return 0;
 		}
 	}
 	return -ENODEV;
 }
-EXPORT_SYMBOL_GPL(cci_disable_port_by_cpu);
+EXPORT_SYMBOL_GPL(cci_control_port_by_cpu);
 
 /**
  * cci_enable_port_for_self() - enable a CCI port for calling CPU

@@ -936,7 +937,7 @@ int notrace __cci_control_port_by_index(u32 port, bool enable)
 	/*
 	 * CCI control for ports connected to CPUS is extremely fragile
 	 * and must be made to go through a specific and controlled
-	 * interface (ie cci_disable_port_by_cpu(); control by general purpose
+	 * interface (ie cci_control_port_by_cpu(); control by general purpose
 	 * indexing is therefore disabled for ACE ports.
 	 */
 	if (ports[port].type == ACE_PORT)

include/linux/arm-cci.h +5 -2

index 79d6edf..c6c3ed0 100644
@@ -29,7 +29,7 @@ struct device_node;
 #ifdef CONFIG_ARM_CCI
 extern bool cci_probed(void);
 extern int cci_ace_get_port(struct device_node *dn);
-extern int cci_disable_port_by_cpu(u64 mpidr);
+extern int cci_control_port_by_cpu(u64 mpidr, bool enable);
 extern int __cci_control_port_by_device(struct device_node *dn, bool enable);
 extern int __cci_control_port_by_index(u32 port, bool enable);
 #else

@@ -38,7 +38,10 @@ static inline int cci_ace_get_port(struct device_node *dn)
 {
 	return -ENODEV;
 }
-static inline int cci_disable_port_by_cpu(u64 mpidr) { return -ENODEV; }
+static inline int cci_disable_port_by_cpu(u64 mpidr, bool enable)
+{
+	return -ENODEV;
+}
 static inline int __cci_control_port_by_device(struct device_node *dn,
 					       bool enable)
 {

Signature · 1 line

1.7.9.5

Quoted 9 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

cci_disable_port_by_cpu() can be used to disable an arbitrary ACE port, but there is no C-callable way to enable an ACE port. Change cci_disable_port_by_cpu() to cci_control_port_by_cpu() to allow us to disable and enable a CPU's ACE port.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

As soon as this patch is applied, it'll break compilation for arch/arm/mach-vexpress/dcscb.c and arch/arm/mach-vexpress/tc2_pm.c.

I suggest you add a compatibility macro in include/linux/arm-cci.h that may look like:

#define cci_disable_port_by_cpu(mpidr) cci_control_port_by_cpu(mpidr, false)

... or at least update existing callers.

Quoted 84 lines

---
 drivers/bus/arm-cci.c   |   13 +++++++------
 include/linux/arm-cci.h |    7 +++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

drivers/bus/arm-cci.c +7 -6

index 5a86da9..5668b40 100644
@@ -763,10 +763,11 @@ static void notrace cci_port_control(unsigned int port, bool enable)
 }
 
 /**
- * cci_disable_port_by_cpu() - function to disable a CCI port by CPU
+ * cci_control_port_by_cpu() - function to control a CCI port by CPU
  *                          reference
  *
- * @mpidr: mpidr of the CPU whose CCI port should be disabled
+ * @mpidr: mpidr of the CPU whose CCI port should be enabled/disabled
+ * @enable: if true enables the port, if false disables it
  *
  * Disabling a CCI port for a CPU implies disabling the CCI port
  * controlling that CPU cluster. Code disabling CPU CCI ports

@@ -777,20 +778,20 @@ static void notrace cci_port_control(unsigned int port, bool enable)
  *   0 on success
  *   -ENODEV on port look-up failure
  */
-int notrace cci_disable_port_by_cpu(u64 mpidr)
+int notrace cci_control_port_by_cpu(u64 mpidr, bool enable)
 {
      int cpu;
      bool is_valid;
      for (cpu = 0; cpu < nr_cpu_ids; cpu++) {
              is_valid = cpu_port_is_valid(&cpu_port[cpu]);
              if (is_valid && cpu_port_match(&cpu_port[cpu], mpidr)) {
-                     cci_port_control(cpu_port[cpu].port, false);
+                     cci_port_control(cpu_port[cpu].port, enable);
                      return 0;
              }
      }
      return -ENODEV;
 }
-EXPORT_SYMBOL_GPL(cci_disable_port_by_cpu);
+EXPORT_SYMBOL_GPL(cci_control_port_by_cpu);
 
 /**
  * cci_enable_port_for_self() - enable a CCI port for calling CPU

@@ -936,7 +937,7 @@ int notrace __cci_control_port_by_index(u32 port, bool enable)
      /*
       * CCI control for ports connected to CPUS is extremely fragile
       * and must be made to go through a specific and controlled
-      * interface (ie cci_disable_port_by_cpu(); control by general purpose
+      * interface (ie cci_control_port_by_cpu(); control by general purpose
       * indexing is therefore disabled for ACE ports.
       */
      if (ports[port].type == ACE_PORT)

include/linux/arm-cci.h +5 -3

index 79d6edf..c6c3ed0 100644
@@ -29,7 +29,7 @@ struct device_node;
 #ifdef CONFIG_ARM_CCI
 extern bool cci_probed(void);
 extern int cci_ace_get_port(struct device_node *dn);
-extern int cci_disable_port_by_cpu(u64 mpidr);
+extern int cci_control_port_by_cpu(u64 mpidr, bool enable);
 extern int __cci_control_port_by_device(struct device_node *dn, bool enable);
 extern int __cci_control_port_by_index(u32 port, bool enable);
 #else

@@ -38,7 +38,10 @@ static inline int cci_ace_get_port(struct device_node *dn)
 {
      return -ENODEV;
 }
-static inline int cci_disable_port_by_cpu(u64 mpidr) { return -ENODEV; }
+static inline int cci_disable_port_by_cpu(u64 mpidr, bool enable)
+{
+     return -ENODEV;
+}
 static inline int __cci_control_port_by_device(struct device_node *dn,
                                             bool enable)
 {
--

Quoted 2 lines

1.7.9.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On the Exynos5420 non-secure SYSRAM is used for secondary CPU bring-up, so add a mapping for it.

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 arch/arm/mach-exynos/exynos.c           |   11 +++++++++++
 arch/arm/mach-exynos/include/mach/map.h |    1 +
 2 files changed, 12 insertions(+)

arch/arm/mach-exynos/exynos.c +0 -0

index b32a907..b1cf9d5 100644
@@ -159,6 +159,15 @@ static struct map_desc exynos5250_iodesc[] __initdata = {
 	},
 };

Quoted 17 lines · Abhilash Kesavan

On the Exynos5420 non-secure SYSRAM is used for secondary CPU bring-up, so add a mapping for it.

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 arch/arm/mach-exynos/exynos.c           |   11 +++++++++++
 arch/arm/mach-exynos/include/mach/map.h |    1 +
 2 files changed, 12 insertions(+)

arch/arm/mach-exynos/exynos.c +0 -0

index b32a907..b1cf9d5 100644
@@ -159,6 +159,15 @@ static struct map_desc exynos5250_iodesc[] __initdata = {
      },
 };

arch/arm/mach-exynos/exynos.c static struct map_desc exynos5250_iodesc[] __initdata = { · line 170

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +9 -0

 
+static struct map_desc exynos5420_iodesc[] __initdata = {
+     {
+             .virtual        = (unsigned long)S5P_VA_SYSRAM_NS,
+             .pfn            = __phys_to_pfn(EXYNOS5420_PA_SYSRAM_NS),
+             .length         = SZ_4K,
+             .type           = MT_DEVICE,
+     },
+};
+

Regarding Arnd's NAK in
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/246936.html,
this looks like another instance of the same thing.

Arnd, what was your preferred solution? Should this be described in DT?

Quoted 33 lines · On Monday 14 April 2014 12:25:20 Dave Martin wrote:

Quoted 17 lines · Abhilash Kesavan

On the Exynos5420 non-secure SYSRAM is used for secondary CPU bring-up, so add a mapping for it.

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 arch/arm/mach-exynos/exynos.c           |   11 +++++++++++
 arch/arm/mach-exynos/include/mach/map.h |    1 +
 2 files changed, 12 insertions(+)

arch/arm/mach-exynos/exynos.c +0 -0

index b32a907..b1cf9d5 100644
@@ -159,6 +159,15 @@ static struct map_desc exynos5250_iodesc[] __initdata = {
      },
 };

arch/arm/mach-exynos/exynos.c static struct map_desc exynos5250_iodesc[] __initdata = { · line 170

Quoted 10 lines

arch/arm/mach-exynos/exynos.c +9 -0

 
+static struct map_desc exynos5420_iodesc[] __initdata = {
+     {
+             .virtual        = (unsigned long)S5P_VA_SYSRAM_NS,
+             .pfn            = __phys_to_pfn(EXYNOS5420_PA_SYSRAM_NS),
+             .length         = SZ_4K,
+             .type           = MT_DEVICE,
+     },
+};
+

Regarding Arnd's NAK in
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/246936.html,
this looks like another instance of the same thing.

Quoted 1 line

Arnd, what was your preferred solution? Should this be described in DT?

Yes. There are already patches on the list to do this right.

Quoted 3 lines · Arnd Bergmann

Quoted 1 line · On Monday 14 April 2014 12:25:20 Dave Martin wrote:

Arnd, what was your preferred solution? Should this be described in DT?

Quoted 1 line

Yes. There are already patches on the list to do this right.

This is http://www.spinics.net/lists/linux-samsung-soc/msg27647.html,
right?

If so, it looks like exynos5420 should definitely follow the same approach.

Quoted 6 lines · On Monday 14 April 2014 12:59:11 Dave Martin wrote:

Quoted 4 lines · Arnd Bergmann

Quoted 1 line · On Monday 14 April 2014 12:25:20 Dave Martin wrote:

Arnd, what was your preferred solution? Should this be described in DT?

Yes. There are already patches on the list to do this right.

Quoted 6 lines

This is http://www.spinics.net/lists/linux-samsung-soc/msg27647.html,
right?

If so, it looks like exynos5420 should definitely follow the same approach.

Yes, exactly.

Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 arch/arm/mach-exynos/Kconfig             |    9 +
 arch/arm/mach-exynos/Makefile            |    2 +
 arch/arm/mach-exynos/common.h            |    1 +
 arch/arm/mach-exynos/exynos.c            |    1 +
 arch/arm/mach-exynos/mcpm-exynos-setup.S |   35 +++
 arch/arm/mach-exynos/mcpm-exynos.c       |  444 ++++++++++++++++++++++++++++++
 arch/arm/mach-exynos/platsmp.c           |   19 ++
 arch/arm/mach-exynos/regs-pmu.h          |    2 +
 8 files changed, 513 insertions(+)
 create mode 100644 arch/arm/mach-exynos/mcpm-exynos-setup.S
 create mode 100644 arch/arm/mach-exynos/mcpm-exynos.c

arch/arm/mach-exynos/Kconfig +0 -0

index fc8bf18..a921a80 100644
@@ -110,4 +110,13 @@ config SOC_EXYNOS5440
 

arch/arm/mach-exynos/Kconfig +0 -0

 endif

arch/arm/mach-exynos/Makefile +2 -0

index a656dbe..776fcbd 100644
@@ -29,3 +29,5 @@ obj-$(CONFIG_ARCH_EXYNOS)	+= firmware.o
 
 plus_sec := $(call as-instr,.arch_extension sec,+sec)
 AFLAGS_exynos-smc.o		:=-Wa,-march=armv7-a$(plus_sec)
+
+obj-$(CONFIG_EXYNOS5420_MCPM)	+= mcpm-exynos.o mcpm-exynos-setup.o

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

index 347afc2..a023ccc 100644
@@ -51,6 +51,7 @@ static inline void exynos_pm_init(void) {}
 extern void exynos_cpu_resume(void);
 
 extern struct smp_operations exynos_smp_ops;
+extern bool exynos_smp_init(void);
 
 extern void exynos_cpu_die(unsigned int cpu);
 

arch/arm/mach-exynos/exynos.c +1 -0

index b1cf9d5..5b72b5e 100644
@@ -412,6 +412,7 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
 	/* Maintainer: Thomas Abraham <[email protected]> */
 	/* Maintainer: Kukjin Kim <[email protected]> */
 	.smp		= smp_ops(exynos_smp_ops),
+	.smp_init	= smp_init_ops(exynos_smp_init),
 	.map_io		= exynos_init_io,
 	.init_early	= exynos_firmware_init,
 	.init_machine	= exynos_dt_machine_init,

arch/arm/mach-exynos/mcpm-exynos-setup.S new file +25 -0

new file mode 100644
index 0000000..990c0d5
@@ -0,0 +1,35 @@
+/*
+ * Exynos low-level MCPM setup
+ *
+ * Copyright (C) 2013-2014 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+
+ENTRY(exynos_power_up_setup)
+
+	cmp	r0, #0			@ check affinity level
+	beq	1f
+
+/*
+ * Enable cluster-level coherency, in preparation for turning on the MMU.
+ * The ACTLR SMP bit does not need to be set here, because cpu_resume()

arch/arm/mach-exynos/mcpm-exynos.c new file +53 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * arch/arm/mach-exynos/mcpm-exynos.c
+ *
+ * Based on arch/arm/mach-vexpress/dcscb.c
+ *
+ * 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/kernel.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+#include <linux/fs.h>
+#include <linux/delay.h>
+#include <linux/arm-cci.h>
+
+#include <asm/mcpm.h>
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+
+#include <plat/cpu.h>
+#include "regs-pmu.h"
+
+#define EXYNOS5420_CPUS_PER_CLUSTER	4
+#define EXYNOS5420_NR_CLUSTERS		2
+
+/* Secondary CPU entry point */
+#define REG_ENTRY_ADDR		(S5P_VA_SYSRAM_NS + 0x1C)
+
+#define EXYNOS_CORE_LOCAL_PWR_EN		0x3
+#define EXYNOS_CORE_LOCAL_PWR_DIS		0x0
+
+#define EXYNOS_ARM_COMMON_CONFIGURATION		S5P_PMUREG(0x2500)
+#define EXYNOS_ARM_L2_CONFIGURATION		S5P_PMUREG(0x2600)
+
+#define EXYNOS_ARM_CORE_CONFIGURATION(_nr)	\
+			(S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS_ARM_CORE_STATUS(_nr)		\
+			(S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+
+#define EXYNOS_COMMON_CONFIGURATION(_nr)	\
+			(EXYNOS_ARM_COMMON_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS_COMMON_STATUS(_nr)		\
+			(EXYNOS_COMMON_CONFIGURATION(_nr) + 0x4)
+
+#define EXYNOS_L2_CONFIGURATION(_nr)		\
+			(EXYNOS_ARM_L2_CONFIGURATION + ((_nr) * 0x80))

arch/arm/mach-exynos/mcpm-exynos.c new file +41 -0

+static int
+cpu_use_count[EXYNOS5420_CPUS_PER_CLUSTER][EXYNOS5420_NR_CLUSTERS];
+
+static bool exynos_core_power_state(unsigned int cpu, unsigned int cluster)
+{
+	unsigned int val;
+	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
+
+	val = __raw_readl(EXYNOS_ARM_CORE_STATUS(cpunr)) &
+				EXYNOS_CORE_LOCAL_PWR_EN;
+	return !!val;
+}
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+						int enable)
+{
+	unsigned int val = EXYNOS_CORE_LOCAL_PWR_DIS;
+	unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
+
+	if (exynos_core_power_state(cpu, cluster) == enable)
+		return;
+
+	if (enable)
+		val = EXYNOS_CORE_LOCAL_PWR_EN;
+	__raw_writel(val, EXYNOS_ARM_CORE_CONFIGURATION(cpunr));
+}
+
+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(20);
+	unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+	if (enable)
+		val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+	status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+	if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+		return;
+
+	__raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+	/* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c new file +22 -0

+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+	unsigned long mpidr;
+
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+		cluster >= EXYNOS5420_NR_CLUSTERS)
+		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(&bl_lock);
+
+	cpu_use_count[cpu][cluster]++;
+	if (cpu_use_count[cpu][cluster] == 1) {
+		bool was_cluster_down =
+			__mcpm_cluster_state(cluster) == CLUSTER_DOWN;

arch/arm/mach-exynos/mcpm-exynos.c new file +71 -0

+		/* CPU should be powered up there */
+		/* Also setup Cluster Power Sequence here */
+	} else if (cpu_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(&bl_lock);
+	local_irq_enable();
+
+	return 0;
+}
+
+static void exynos_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster, cpumask;
+	bool last_man = false, skip_wfi = false;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+	cpumask = (1 << cpu);
+
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+	BUG_ON(cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+			cluster >= EXYNOS5420_NR_CLUSTERS);
+
+	__mcpm_cpu_going_down(cpu, cluster);
+
+	arch_spin_lock(&bl_lock);
+	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+	cpu_use_count[cpu][cluster]--;
+	if (cpu_use_count[cpu][cluster] == 0) {
+		int cnt = 0, i;
+
+		exynos_core_power_control(cpu, cluster, 0);
+		for (i = 0; i < EXYNOS5420_CPUS_PER_CLUSTER; i++)
+			cnt += cpu_use_count[i][cluster];
+		if (cnt == 0)
+			last_man = true;
+	} else if (cpu_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(&bl_lock);
+
+		/*
+		 * Flush all cache levels for this cluster.
+		 *
+		 * To do so we do:
+		 * - Clear the SCTLR.C bit to prevent further cache allocations
+		 * - Flush the whole cache
+		 * - Clear the ACTLR "SMP" bit to disable local coherency
+		 *
+		 * Let's do it in the safest possible way i.e. with

arch/arm/mach-exynos/mcpm-exynos.c new file +19 -0

+		asm volatile(
+		"stmfd	sp!, {fp, ip}\n\t"
+		"mrc	p15, 0, r0, c1, c0, 0	@ get SCTLR\n\t"
+		"bic	r0, r0, #"__stringify(CR_C)"\n\t"
+		"mcr	p15, 0, r0, c1, c0, 0	@ set SCTLR\n\t"
+		"isb\n\t"
+		"bl	v7_flush_dcache_all\n\t"
+		"clrex\n\t"
+		"mrc	p15, 0, r0, c1, c0, 1	@ get ACTLR\n\t"
+		"bic	r0, r0, #(1 << 6)	@ disable local coherency\n\t"
+		/* Dummy Load of a device register to avoid Erratum 799270 */
+		"ldr	r4, [%0]\n\t"
+		"and	r4, r4, #0\n\t"
+		"orr	r0, r0, r4\n\t"
+		"mcr	p15, 0, r0, c1, c0, 1	@ set ACTLR\n\t"
+		"isb\n\t"
+		"dsb\n\t"
+		"ldmfd	sp!, {fp, ip}"
+		:

arch/arm/mach-exynos/mcpm-exynos.c new file +21 -0

+
+		__mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+	} else {
+		arch_spin_unlock(&bl_lock);
+
+		/*
+		 * Flush the local CPU cache.
+		 * Let's do it in the safest possible way as above.
+		 */
+		asm volatile(
+		"stmfd	sp!, {fp, ip}\n\t"
+		"mrc	p15, 0, r0, c1, c0, 0	@ get SCTLR\n\t"
+		"bic	r0, r0, #"__stringify(CR_C)"\n\t"
+		"mcr	p15, 0, r0, c1, c0, 0	@ set SCTLR\n\t"
+		"isb\n\t"
+		"bl	v7_flush_dcache_louis\n\t"
+		"clrex\n\t"
+		"mrc	p15, 0, r0, c1, c0, 1	@ get ACTLR\n\t"
+		"bic	r0, r0, #(1 << 6)	@ disable local coherency\n\t"
+		/* Dummy Load of a device register to avoid Erratum 799270 */
+		"ldr	r4, [%0]\n\t"

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+	if (!skip_wfi)
+		wfi();
+
+	/* Not dead at this point?  Let our caller cope. */
+}

arch/arm/mach-exynos/mcpm-exynos.c new file +16 -0

+
+	return 0; /* success: the CPU is halted */
+}
+
+static const struct mcpm_platform_ops exynos_power_ops = {
+	.power_up		= exynos_power_up,
+	.power_down		= exynos_power_down,
+	.power_down_finish	= exynos_power_down_finish,
+};
+
+static void __init exynos_mcpm_usage_count_init(void)
+{
+	unsigned int mpidr, cpu, cluster, i;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);

arch/arm/mach-exynos/mcpm-exynos.c new file +39 -0

+	cpu_use_count[cpu][cluster] = 1;
+}
+
+static size_t bL_check_status(char *info)
+{
+	size_t len = 0;
+	int i;
+
+	len += sprintf(&info[len], "\t0 1 2 3 L2\n");
+	len += sprintf(&info[len], "[A15]   ");
+	for (i = 0; i < 4; i++) {
+		len += sprintf(&info[len], "%d ",
+			(readl(EXYNOS_ARM_CORE_STATUS(i)) & 0x3) == 3 ? 1 : 0);
+	}
+	len += sprintf(&info[len], "%d\n",
+			(readl(EXYNOS_COMMON_STATUS(0)) & 0x3) == 3 ? 1 : 0);
+
+	len += sprintf(&info[len], "[A7]    ");
+	for (i = 4; i < 8; i++)
+		len += sprintf(&info[len], "%d ",
+			(readl(EXYNOS_ARM_CORE_STATUS(i)) & 0x3) == 3 ? 1 : 0);
+	len += sprintf(&info[len], "%d\n\n",
+			(readl(EXYNOS_COMMON_STATUS(1)) & 0x3) == 3 ? 1 : 0);
+
+	return len;
+}
+
+static ssize_t bL_status_read(struct file *file, char __user *buf,
+			size_t len, loff_t *pos)
+{
+	size_t count = 0;
+	char info[100];
+
+	count = bL_check_status(info);
+	if (count < 0)
+		return -EINVAL;
+
+	return simple_read_from_buffer(buf, len, pos, info, count);
+}

arch/arm/mach-exynos/mcpm-exynos.c new file +9 -0

+	 * To increase the stability of KFC reset we need to program
+	 * the PMU SPARE3 register
+	 */
+	__raw_writel(EXYNOS5420_SWRESET_KFC_SEL, S5P_PMU_SPARE3);
+
+	exynos_mcpm_usage_count_init();
+
+	ret = mcpm_platform_register(&exynos_power_ops);
+	if (!ret)

arch/arm/mach-exynos/mcpm-exynos.c new file +20 -0

+
+	return ret;
+}
+
+early_initcall(exynos_mcpm_init);
+
+static int __init exynos_bl_status_init(void)
+{
+	int ret;
+
+	if (!soc_is_exynos5420())
+		return 0;
+
+	ret = misc_register(&bL_status_device);
+	if (ret)
+		pr_info("bl_status not available\n");
+	return 0;
+}
+
+late_initcall(exynos_bl_status_init);

arch/arm/mach-exynos/platsmp.c +8 -0

index 03e5e9f..4f14457 100644
@@ -25,6 +25,7 @@
 #include <asm/smp_plat.h>
 #include <asm/smp_scu.h>
 #include <asm/firmware.h>
+#include <asm/mcpm.h>
 
 #include <plat/cpu.h>
 

@@ -226,6 +227,24 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
+bool __init exynos_smp_init(void)
+{
+#ifdef CONFIG_MCPM
+	/*
+	 * The best way to detect a multi-cluster configuration at the moment
+	 * is to look for the presence of a CCI in the system.
+	 * Override the default exynos_smp_ops if so.

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

+
 struct smp_operations exynos_smp_ops __initdata = {
 	.smp_init_cpus		= exynos_smp_init_cpus,
 	.smp_prepare_cpus	= exynos_smp_prepare_cpus,

arch/arm/mach-exynos/regs-pmu.h +2 -0

index cfbfc575..43fe7a0 100644
@@ -38,6 +38,7 @@
 #define S5P_INFORM5				S5P_PMUREG(0x0814)
 #define S5P_INFORM6				S5P_PMUREG(0x0818)
 #define S5P_INFORM7				S5P_PMUREG(0x081C)
+#define S5P_PMU_SPARE3				S5P_PMUREG(0x090C)
 
 #define EXYNOS_IROM_DATA2			S5P_PMUREG(0x0988)
 

@@ -540,5 +541,6 @@
 #define EXYNOS5420_KFC_USE_STANDBY_WFE3				(1 << 23)
 
 #define DUR_WAIT_RESET				0xF
+#define EXYNOS5420_SWRESET_KFC_SEL		0x3
 
 #endif /* __ASM_ARCH_REGS_PMU_H */

Signature · 1 line

1.7.9.5

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(20);
+	unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+	if (enable)
+		val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+	status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+	if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+		return;
+
+	__raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+	/* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+	while (time_before(jiffies, timeout)) {
+		status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+		if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+			return;
+
+		cpu_relax();
+	}
+	pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+		enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 23 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +22 -0

+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+	unsigned long mpidr;
+
+	pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+	if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+		cluster >= EXYNOS5420_NR_CLUSTERS)
+		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(&bl_lock);
+
+	cpu_use_count[cpu][cluster]++;
+	if (cpu_use_count[cpu][cluster] == 1) {
+		bool was_cluster_down =
+			__mcpm_cluster_state(cluster) == CLUSTER_DOWN;

arch/arm/mach-exynos/mcpm-exynos.c line 146

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+
+		if (was_cluster_down)
+			exynos_cluster_power_control(cluster, 1);
+		exynos_core_power_control(cpu, cluster, 1);
+
+		if (was_cluster_down) {
+			mpidr = read_cpuid_mpidr();
+			udelay(10);
+			cci_control_port_by_cpu(mpidr, true);
+		}

This is completely wrong. Is this why you created the patch to introduce cci_control_port_by_cpu()? If so I'm NAKing that other patch as well.

This is going to be completely ineffective with concurrent usage by cpuidle where CPUs in the other cluster are awaken by an interrupt and not by calling the cpu_up method. The current cluster will therefore not be aware of the other cluster coming online and system memory corruption will occur.

I see below that you do turn off the CCI port for the current cluster and the other cluster together, hence the need to enable back the CCI for the current cluster here. Please don't do that. That's not the proper way to achieve that and there are many race conditions to take care of before this can be done. And if we were to go that route, I want to be convinced it is worth the needed complexity first i.e. I want to see evidence this actually does save power or improve performance by a non negligible margin.

Quoted 20 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +19 -0

+		/* CPU should be powered up there */
+		/* Also setup Cluster Power Sequence here */
+	} else if (cpu_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(&bl_lock);
+	local_irq_enable();
+
+	return 0;
+}

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 31 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(20);
+	unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+	if (enable)
+		val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+	status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+	if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+		return;
+
+	__raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+	/* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+	while (time_before(jiffies, timeout)) {
+		status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+		if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+			return;
+
+		cpu_relax();
+	}
+	pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+		enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

Quoted 2 lines

The actual power status should be polled for in the mcpm_finish() method only.

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

If this is the case, it might be necessary to ensure before a power-up request, that the power controller has caught up and reports the cluster/CPU as down. Putting this poll before the write to the power controller maximises the chance of pipelining useful work in the meantime. Putting the poll after the write is the worst case.

Quoted 62 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 33 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(20);
+	unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+	if (enable)
+		val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+	status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+	if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+		return;
+
+	__raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+	/* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+	while (time_before(jiffies, timeout)) {
+		status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+		if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+			return;
+
+		cpu_relax();
+	}
+	pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+		enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 5 lines

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

Given the way the code is structured now, I suppose that has not been the case so far.

Quoted 6 lines

If this is the case, it might be necessary to ensure before a power-up request, that the power controller has caught up and reports the cluster/CPU as down. Putting this poll before the write to the power controller maximises the chance of pipelining useful work in the meantime. Putting the poll after the write is the worst case.

More useful might be a check on the actual _control_ bit. If the control bits may be read back then I expect the indicated state will happen even if delayed.

Quoted 65 lines · Nicolas Pitre

Quoted 62 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 33 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+       unsigned long timeout = jiffies + msecs_to_jiffies(20);
+       unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+       if (enable)
+               val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+       status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+       if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+               return;
+
+       __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+       /* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+       while (time_before(jiffies, timeout)) {
+               status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+               if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+                       return;
+
+               cpu_relax();
+       }
+       pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+               enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 8 lines

Quoted 5 lines

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

Given the way the code is structured now, I suppose that has not been the case so far.

It depends on the purpose of the polling loop. If it was added to resolve a race between a power-up and a power-down that is complete in MCPM but still pending in the hardware, than that would suggest the power controller does have this behaviour.

But I'm just guessing.

Abhilash, can you comment?

Quoted 11 lines

Quoted 7 lines

If this is the case, it might be necessary to ensure before a power-up request, that the power controller has caught up and reports the cluster/CPU as down. Putting this poll before the write to the power controller maximises the chance of pipelining useful work in the meantime. Putting the poll after the write is the worst case.

More useful might be a check on the actual _control_ bit. If the control bits may be read back then I expect the indicated state will happen even if delayed.

Quite likely, yes. It just depends on what the hardware specs say.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Dave,

Quoted 80 lines · Dave Martin

Quoted 65 lines · Nicolas Pitre

Quoted 62 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 33 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+       unsigned long timeout = jiffies + msecs_to_jiffies(20);
+       unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+       if (enable)
+               val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+       status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+       if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+               return;
+
+       __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+       /* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+       while (time_before(jiffies, timeout)) {
+               status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+               if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+                       return;
+
+               cpu_relax();
+       }
+       pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+               enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 8 lines

Quoted 5 lines

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

Given the way the code is structured now, I suppose that has not been the case so far.

It depends on the purpose of the polling loop. If it was added to resolve a race between a power-up and a power-down that is complete in MCPM but still pending in the hardware, than that would suggest the power controller does have this behaviour.

Quoted 3 lines

But I'm just guessing.

Abhilash, can you comment?

The polling loop in the above function was added to ensure that the cluster is switched on before we power on the individual cores in exynos_power_up. This is only required when the first man comes up.

Quoted 85 lines · Abhilash Kesavan

Hi Dave,

Quoted 82 lines · Dave Martin

Quoted 65 lines · Nicolas Pitre

Quoted 62 lines · Dave Martin

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 33 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+       unsigned long timeout = jiffies + msecs_to_jiffies(20);
+       unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+       if (enable)
+               val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+       status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+       if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+               return;
+
+       __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+       /* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+       while (time_before(jiffies, timeout)) {
+               status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+               if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+                       return;
+
+               cpu_relax();
+       }
+       pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+               enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 8 lines

Quoted 5 lines

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

Given the way the code is structured now, I suppose that has not been the case so far.

It depends on the purpose of the polling loop. If it was added to resolve a race between a power-up and a power-down that is complete in MCPM but still pending in the hardware, than that would suggest the power controller does have this behaviour.

But I'm just guessing.

Quoted 4 lines

Quoted 1 line

Abhilash, can you comment?

The polling loop in the above function was added to ensure that the cluster is switched on before we power on the individual cores in exynos_power_up. This is only required when the first man comes up.

Is this only required on power-up, not power-down? I don't see a call to exynos_cluster_power_control(..., 0), but maybe I'm looking in the wrong place.

I wonder whether this is needed, though. It imposes a requirement on the OS to waste time polling for the cluster to come online. Can the hardware not cope with a CPU poweron request being pending already before the cluster is powered on? If that were possible, there would be no need for an extra poll.

How long do you expect to need to poll for?

Cheers
---Dave

Hi Dave,

Quoted 5 lines · Dave Martin

Quoted 3 lines · Abhilash Kesavan

The polling loop in the above function was added to ensure that the cluster is switched on before we power on the individual cores in exynos_power_up. This is only required when the first man comes up.

Quoted 3 lines

Is this only required on power-up, not power-down? I don't see a call to exynos_cluster_power_control(..., 0), but maybe I'm looking in the wrong place.

No, you have not missed it. As of now, we are turning on both the L2s for 8 core bring-up and then not turning it off (if all 4 cores in the clusters are down). Turning the L2 off (A15 L2 mainly) brings in significant power savings and I will be looking at adding support for this next.

Quoted 5 lines

I wonder whether this is needed, though. It imposes a requirement on the OS to waste time polling for the cluster to come online. Can the hardware not cope with a CPU poweron request being pending already before the cluster is powered on? If that were possible, there would be no need for an extra poll.

No, we need to ensure that the cluster L2 is turned On before we can turn the core on, the hardware does not handle it.

Quoted 2 lines

How long do you expect to need to poll for?

In internal testing when we are turning the cluster on from an off state, I have seen the loop being executed 3-4 times occasionally during ageing tests. Generally, the status returns as being powered ON immediately.

Quoted 6 lines · Nicolas Pitre

Quoted 4 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

I won't duplicate Nico's review, but I have a couple of extra comments/ questions, below.

[...]

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +0 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@

[...]

Quoted 31 lines

Quoted 14 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+   unsigned long timeout = jiffies + msecs_to_jiffies(20);
+   unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+   if (enable)
+           val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+   status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+   if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+           return;
+
+   __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+   /* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+   while (time_before(jiffies, timeout)) {
+           status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+           if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+                   return;
+
+           cpu_relax();
+   }
+   pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+           enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

Quoted 2 lines

The actual power status should be polled for in the mcpm_finish() method only.

Depending on the power controller, it might be possible for writes to the controller to be lost or not acted upon, if a previous change is still pending.

Does this issue apply to the exynos power controller?

If this is the case, it might be necessary to ensure before a power-up request, that the power controller has caught up and reports the cluster/CPU as down. Putting this poll before the write to the power controller maximises the chance of pipelining useful work in the meantime. Putting the poll after the write is the worst case.

Quoted 44 lines

Quoted 23 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +22 -0

+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+   unsigned long mpidr;
+
+   pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+   if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+           cluster >= EXYNOS5420_NR_CLUSTERS)
+           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(&bl_lock);
+
+   cpu_use_count[cpu][cluster]++;
+   if (cpu_use_count[cpu][cluster] == 1) {
+           bool was_cluster_down =
+                   __mcpm_cluster_state(cluster) == CLUSTER_DOWN;

arch/arm/mach-exynos/mcpm-exynos.c line 146

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+
+           if (was_cluster_down)
+                   exynos_cluster_power_control(cluster, 1);
+           exynos_core_power_control(cpu, cluster, 1);
+
+           if (was_cluster_down) {
+                   mpidr = read_cpuid_mpidr();
+                   udelay(10);
+                   cci_control_port_by_cpu(mpidr, true);
+           }

This is completely wrong. Is this why you created the patch to introduce cci_control_port_by_cpu()? If so I'm NAKing that other patch as well.

This is going to be completely ineffective with concurrent usage by cpuidle where CPUs in the other cluster are awaken by an interrupt and not by calling the cpu_up method. The current cluster will therefore not be aware of the other cluster coming online and system memory corruption will occur.

Quoted 8 lines

I see below that you do turn off the CCI port for the current cluster and the other cluster together, hence the need to enable back the CCI for the current cluster here. Please don't do that. That's not the proper way to achieve that and there are many race conditions to take care of before this can be done. And if we were to go that route, I want to be convinced it is worth the needed complexity first i.e. I want to see evidence this actually does save power or improve performance by a non negligible margin.

Agreed.

The fact that there is no C interface for enabling ACE ports is deliberate. For CPUs connected to ACE and managed via MCPM, it is incorrect to enable CCI via C code, since the safe window is the window during which all outbound CPUs have reached CLUSTER_DOWN and all inbound CPUs have not turned their MMU on yet (and thus cannot execute any general Linux C code).

There might be scenarios involving GPUs and other non-CPU devices connected to ACE ports where the device cannot enable CCI snoops for itself -- but this would require a holding-pen protocol to enable the device to wait and signal a CPU to enable CCI snoops on the device's behalf before the device proceeds. It is not the correct solution for CPU clusters attached to ACE, precisely because we can be more efficient in that case.

In fact, because you implement a power_up_setup method that calls cci_enable_port_for_self, CCI snoops are actually enabled twice, making
the above code appear redundant. Have I missed something?

Cheers
---Dave

Quoted 12 lines

Quoted 12 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +11 -0

+           /* CPU should be powered up there */
+           /* Also setup Cluster Power Sequence here */
+   } else if (cpu_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.
+            */

Quoted 8 lines

arch/arm/mach-exynos/mcpm-exynos.c line 166

Quoted 8 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +8 -0

+           BUG();
+   }
+
+   arch_spin_unlock(&bl_lock);
+   local_irq_enable();
+
+   return 0;
+}

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 15 lines

The fact that there is no C interface for enabling ACE ports is deliberate. For CPUs connected to ACE and managed via MCPM, it is incorrect to enable CCI via C code, since the safe window is the window during which all outbound CPUs have reached CLUSTER_DOWN and all inbound CPUs have not turned their MMU on yet (and thus cannot execute any general Linux C code).

There might be scenarios involving GPUs and other non-CPU devices connected to ACE ports where the device cannot enable CCI snoops for itself -- but this would require a holding-pen protocol to enable the device to wait and signal a CPU to enable CCI snoops on the device's behalf before the device proceeds. It is not the correct solution for CPU clusters attached to ACE, precisely because we can be more efficient in that case.

Quoted 3 lines

In fact, because you implement a power_up_setup method that calls cci_enable_port_for_self, CCI snoops are actually enabled twice, making
the above code appear redundant. Have I missed something?

When a cluster is being turned off the snoops for both the clusters are being turned off. When the other cluster comes back the snoops are being turned on for the incoming cluster via power_up_setup and here for the other cluster. As previously mentioned, I will be dropping this change.

On Thu, Apr 17, 2014 at 12:41:22AM +0530, Abhilash Kesavan wrote:

[...]

Quoted 15 lines

Quoted 15 lines

The fact that there is no C interface for enabling ACE ports is deliberate. For CPUs connected to ACE and managed via MCPM, it is incorrect to enable CCI via C code, since the safe window is the window during which all outbound CPUs have reached CLUSTER_DOWN and all inbound CPUs have not turned their MMU on yet (and thus cannot execute any general Linux C code).

There might be scenarios involving GPUs and other non-CPU devices connected to ACE ports where the device cannot enable CCI snoops for itself -- but this would require a holding-pen protocol to enable the device to wait and signal a CPU to enable CCI snoops on the device's behalf before the device proceeds. It is not the correct solution for CPU clusters attached to ACE, precisely because we can be more efficient in that case.

Quoted 8 lines

Quoted 3 lines

In fact, because you implement a power_up_setup method that calls cci_enable_port_for_self, CCI snoops are actually enabled twice, making
the above code appear redundant. Have I missed something?

When a cluster is being turned off the snoops for both the clusters are being turned off. When the other cluster comes back the snoops are being turned on for the incoming cluster via power_up_setup and here for the other cluster. As previously mentioned, I will be dropping this change.

That's a fair point. If there is only one cluster alive, turning off snoops for it should be safe, because there is no second cluster for it to maintain coherency with.

Quoted 28 lines · Dave Martin

On Thu, Apr 17, 2014 at 12:41:22AM +0530, Abhilash Kesavan wrote:

[...]

Quoted 15 lines

Quoted 15 lines

The fact that there is no C interface for enabling ACE ports is deliberate. For CPUs connected to ACE and managed via MCPM, it is incorrect to enable CCI via C code, since the safe window is the window during which all outbound CPUs have reached CLUSTER_DOWN and all inbound CPUs have not turned their MMU on yet (and thus cannot execute any general Linux C code).

There might be scenarios involving GPUs and other non-CPU devices connected to ACE ports where the device cannot enable CCI snoops for itself -- but this would require a holding-pen protocol to enable the device to wait and signal a CPU to enable CCI snoops on the device's behalf before the device proceeds. It is not the correct solution for CPU clusters attached to ACE, precisely because we can be more efficient in that case.

Quoted 8 lines

Quoted 3 lines

In fact, because you implement a power_up_setup method that calls cci_enable_port_for_self, CCI snoops are actually enabled twice, making
the above code appear redundant. Have I missed something?

When a cluster is being turned off the snoops for both the clusters are being turned off. When the other cluster comes back the snoops are being turned on for the incoming cluster via power_up_setup and here for the other cluster. As previously mentioned, I will be dropping this change.

Quoted 3 lines

That's a fair point. If there is only one cluster alive, turning off snoops for it should be safe, because there is no second cluster for it to maintain coherency with.

But that's not that simple as I explained in a previous email. If the other cluster has gone down via cpuidle, it may come back up at any moment without warning. We do have the infrastructure in place to cope with possible races handling the CCI within a cluster. We do not have anything for cross cluster races. And before we do, it is necessary to know if it is worth it.

Quoted 34 lines · Nicolas Pitre

Quoted 28 lines · Dave Martin

On Thu, Apr 17, 2014 at 12:41:22AM +0530, Abhilash Kesavan wrote:

[...]

Quoted 15 lines

Quoted 15 lines

The fact that there is no C interface for enabling ACE ports is deliberate. For CPUs connected to ACE and managed via MCPM, it is incorrect to enable CCI via C code, since the safe window is the window during which all outbound CPUs have reached CLUSTER_DOWN and all inbound CPUs have not turned their MMU on yet (and thus cannot execute any general Linux C code).

There might be scenarios involving GPUs and other non-CPU devices connected to ACE ports where the device cannot enable CCI snoops for itself -- but this would require a holding-pen protocol to enable the device to wait and signal a CPU to enable CCI snoops on the device's behalf before the device proceeds. It is not the correct solution for CPU clusters attached to ACE, precisely because we can be more efficient in that case.

Quoted 8 lines

Quoted 3 lines

In fact, because you implement a power_up_setup method that calls cci_enable_port_for_self, CCI snoops are actually enabled twice, making
the above code appear redundant. Have I missed something?

When a cluster is being turned off the snoops for both the clusters are being turned off. When the other cluster comes back the snoops are being turned on for the incoming cluster via power_up_setup and here for the other cluster. As previously mentioned, I will be dropping this change.

Quoted 3 lines

That's a fair point. If there is only one cluster alive, turning off snoops for it should be safe, because there is no second cluster for it to maintain coherency with.

Quoted 6 lines

But that's not that simple as I explained in a previous email. If the other cluster has gone down via cpuidle, it may come back up at any moment without warning. We do have the infrastructure in place to cope with possible races handling the CCI within a cluster. We do not have anything for cross cluster races. And before we do, it is necessary to know if it is worth it.

Agreed. It could be done, perhaps by the approach I already considered for handling multilevel clusters, but it is far from trivial, and I would like to see some measurement of the potential benefit before getting into it.

Cheers
---Dave

Quoted 8 lines · Abhilash Kesavan

Add machine-dependent MCPM call-backs for Exynos5420. These are used to power up/down the secondary CPUs during boot, shutdown, s2r and switching.

Signed-off-by Thomas Abraham <[email protected]>

Signed-off-by Inderpal Singh <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

See comments inline.

Quoted 21 lines

---
 arch/arm/mach-exynos/Kconfig             |    9 +
 arch/arm/mach-exynos/Makefile            |    2 +
 arch/arm/mach-exynos/common.h            |    1 +
 arch/arm/mach-exynos/exynos.c            |    1 +
 arch/arm/mach-exynos/mcpm-exynos-setup.S |   35 +++
 arch/arm/mach-exynos/mcpm-exynos.c       |  444 ++++++++++++++++++++++++++++++
 arch/arm/mach-exynos/platsmp.c           |   19 ++
 arch/arm/mach-exynos/regs-pmu.h          |    2 +
 8 files changed, 513 insertions(+)
 create mode 100644 arch/arm/mach-exynos/mcpm-exynos-setup.S
 create mode 100644 arch/arm/mach-exynos/mcpm-exynos.c

arch/arm/mach-exynos/Kconfig +0 -0

index fc8bf18..a921a80 100644
@@ -110,4 +110,13 @@ config SOC_EXYNOS5440
 
 endmenu
 

arch/arm/mach-exynos/Kconfig config SOC_EXYNOS5440 · line 120

Quoted 8 lines

arch/arm/mach-exynos/Kconfig +8 -0

+config EXYNOS5420_MCPM
+     bool "Exynos5420 Multi-Cluster PM support"
+     depends on MCPM && SOC_EXYNOS5420
+     select ARM_CCI
+     help
+       Support for Dual Cluster Switching (A15/A7) on Exynos5420.
+       This is needed to provide CPU and cluster power management
+       on Exynos5420 implementing big.LITTLE.

MCPM is not about "cluster switching". It is about cluster-wide power-up/power-down coordination and race avoidance. MCPM is relied upon by the big.LITTLE switcher, but it is also needed by cpuidle, CPU hotplug, etc. Therefore the first line of the help text is wrong and could be omitted entirely.

Quoted 73 lines

arch/arm/mach-exynos/Kconfig +0 -0

 endif

arch/arm/mach-exynos/Makefile +2 -0

index a656dbe..776fcbd 100644
@@ -29,3 +29,5 @@ obj-$(CONFIG_ARCH_EXYNOS)   += firmware.o
 
 plus_sec := $(call as-instr,.arch_extension sec,+sec)
 AFLAGS_exynos-smc.o          :=-Wa,-march=armv7-a$(plus_sec)
+
+obj-$(CONFIG_EXYNOS5420_MCPM)        += mcpm-exynos.o mcpm-exynos-setup.o

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

index 347afc2..a023ccc 100644
@@ -51,6 +51,7 @@ static inline void exynos_pm_init(void) {}
 extern void exynos_cpu_resume(void);
 
 extern struct smp_operations exynos_smp_ops;
+extern bool exynos_smp_init(void);
 
 extern void exynos_cpu_die(unsigned int cpu);
 

arch/arm/mach-exynos/exynos.c +1 -0

index b1cf9d5..5b72b5e 100644
@@ -412,6 +412,7 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
      /* Maintainer: Thomas Abraham <[email protected]> */
      /* Maintainer: Kukjin Kim <[email protected]> */
      .smp            = smp_ops(exynos_smp_ops),
+     .smp_init       = smp_init_ops(exynos_smp_init),
      .map_io         = exynos_init_io,
      .init_early     = exynos_firmware_init,
      .init_machine   = exynos_dt_machine_init,

arch/arm/mach-exynos/mcpm-exynos-setup.S new file +31 -0

new file mode 100644
index 0000000..990c0d5
@@ -0,0 +1,35 @@
+/*
+ * Exynos low-level MCPM setup
+ *
+ * Copyright (C) 2013-2014 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+
+ENTRY(exynos_power_up_setup)
+
+     cmp     r0, #0                  @ check affinity level
+     beq     1f
+
+/*
+ * Enable cluster-level coherency, in preparation for turning on the MMU.
+ * The ACTLR SMP bit does not need to be set here, because cpu_resume()
+ * already restores that.
+ */
+     b       cci_enable_port_for_self
+
+1:   @ Implementation-specific local CPU setup operations should go here,
+     @ if any.  In this case, there is nothing to do.

arch/arm/mach-exynos/mcpm-exynos-setup.S line 35

Quoted 4 lines

arch/arm/mach-exynos/mcpm-exynos-setup.S new file +4 -0

+
+     bx      lr
+
+ENDPROC(exynos_power_up_setup)

Given this is so simple, I'd suggest you simply copy the TC2 version for the above code and dispense with this file altogether. See tc2_pm_power_up_setup() in mach-vexpress/tc2_pm.c.

Quoted 65 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +58 -0

new file mode 100644
index 0000000..46d4968
@@ -0,0 +1,444 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ *           http://www.samsung.com
+ *
+ * arch/arm/mach-exynos/mcpm-exynos.c
+ *
+ * Based on arch/arm/mach-vexpress/dcscb.c
+ *
+ * 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/kernel.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/miscdevice.h>
+#include <linux/fs.h>
+#include <linux/delay.h>
+#include <linux/arm-cci.h>
+
+#include <asm/mcpm.h>
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+
+#include <plat/cpu.h>
+#include "regs-pmu.h"
+
+#define EXYNOS5420_CPUS_PER_CLUSTER  4
+#define EXYNOS5420_NR_CLUSTERS               2
+
+/* Secondary CPU entry point */
+#define REG_ENTRY_ADDR               (S5P_VA_SYSRAM_NS + 0x1C)
+
+#define EXYNOS_CORE_LOCAL_PWR_EN             0x3
+#define EXYNOS_CORE_LOCAL_PWR_DIS            0x0
+
+#define EXYNOS_ARM_COMMON_CONFIGURATION              S5P_PMUREG(0x2500)
+#define EXYNOS_ARM_L2_CONFIGURATION          S5P_PMUREG(0x2600)
+
+#define EXYNOS_ARM_CORE_CONFIGURATION(_nr)   \
+                     (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS_ARM_CORE_STATUS(_nr)          \
+                     (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+
+#define EXYNOS_COMMON_CONFIGURATION(_nr)     \
+                     (EXYNOS_ARM_COMMON_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS_COMMON_STATUS(_nr)            \
+                     (EXYNOS_COMMON_CONFIGURATION(_nr) + 0x4)
+
+#define EXYNOS_L2_CONFIGURATION(_nr)         \
+                     (EXYNOS_ARM_L2_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS_L2_STATUS(_nr)                        \
+                     (EXYNOS_L2_CONFIGURATION(_nr) + 0x4)
+
+/*
+ * We can't use regular spinlocks. In the switcher case, it is possible

arch/arm/mach-exynos/mcpm-exynos.c line 63

Quoted 5 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+ * 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 bl_lock = __ARCH_SPIN_LOCK_UNLOCKED;

the bl prefix in "bl_lock" might be confusing. I'd suggest you name this "exynos_mcpm_lock" or similar.

Quoted 42 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +41 -0

+static int
+cpu_use_count[EXYNOS5420_CPUS_PER_CLUSTER][EXYNOS5420_NR_CLUSTERS];
+
+static bool exynos_core_power_state(unsigned int cpu, unsigned int cluster)
+{
+     unsigned int val;
+     unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
+
+     val = __raw_readl(EXYNOS_ARM_CORE_STATUS(cpunr)) &
+                             EXYNOS_CORE_LOCAL_PWR_EN;
+     return !!val;
+}
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+                                             int enable)
+{
+     unsigned int val = EXYNOS_CORE_LOCAL_PWR_DIS;
+     unsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
+
+     if (exynos_core_power_state(cpu, cluster) == enable)
+             return;
+
+     if (enable)
+             val = EXYNOS_CORE_LOCAL_PWR_EN;
+     __raw_writel(val, EXYNOS_ARM_CORE_CONFIGURATION(cpunr));
+}
+
+static void exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+     unsigned long timeout = jiffies + msecs_to_jiffies(20);
+     unsigned int status, val = EXYNOS_CORE_LOCAL_PWR_DIS;
+
+     if (enable)
+             val = EXYNOS_CORE_LOCAL_PWR_EN;
+
+     status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+     if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+             return;
+
+     __raw_writel(val, EXYNOS_COMMON_CONFIGURATION(cluster));
+     /* Wait until cluster power control is applied */

arch/arm/mach-exynos/mcpm-exynos.c line 114

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+     while (time_before(jiffies, timeout)) {
+             status = __raw_readl(EXYNOS_COMMON_STATUS(cluster));
+
+             if ((status & EXYNOS_CORE_LOCAL_PWR_EN) == val)
+                     return;
+
+             cpu_relax();
+     }
+     pr_warn("timed out waiting for cluster %u to power %s\n", cluster,
+             enable ? "on" : "off");

You should not have to wait for the power status to change here. Simply signaling the desired state and returning is all that is expected. And because IRQs are turned off, it is likely that time_before(jiffies, timeout) will always be true anyway because jiffies are not updated if there is no other CPU to service the timer interrupt.

The actual power status should be polled for in the mcpm_finish() method only.

Quoted 28 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +28 -0

+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long mpidr;
+
+     pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+     if (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+             cluster >= EXYNOS5420_NR_CLUSTERS)
+             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(&bl_lock);
+
+     cpu_use_count[cpu][cluster]++;
+     if (cpu_use_count[cpu][cluster] == 1) {
+             bool was_cluster_down =
+                     __mcpm_cluster_state(cluster) == CLUSTER_DOWN;
+
+             if (was_cluster_down)
+                     exynos_cluster_power_control(cluster, 1);
+             exynos_core_power_control(cpu, cluster, 1);
+
+             if (was_cluster_down) {

arch/arm/mach-exynos/mcpm-exynos.c line 146

Quoted 4 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +4 -0

+                     mpidr = read_cpuid_mpidr();
+                     udelay(10);
+                     cci_control_port_by_cpu(mpidr, true);
+             }

This is completely wrong. Is this why you created the patch to introduce cci_control_port_by_cpu()? If so I'm NAKing that other patch as well.

This is going to be completely ineffective with concurrent usage by cpuidle where CPUs in the other cluster are awaken by an interrupt and not by calling the cpu_up method. The current cluster will therefore not be aware of the other cluster coming online and system memory corruption will occur.

I see below that you do turn off the CCI port for the current cluster and the other cluster together, hence the need to enable back the CCI for the current cluster here. Please don't do that. That's not the proper way to achieve that and there are many race conditions to take care of before this can be done. And if we were to go that route, I want to be convinced it is worth the needed complexity first i.e. I want to see evidence this actually does save power or improve performance by a non negligible margin.

Quoted 77 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +76 -0

+             /* CPU should be powered up there */
+             /* Also setup Cluster Power Sequence here */
+     } else if (cpu_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(&bl_lock);
+     local_irq_enable();
+
+     return 0;
+}
+
+static void exynos_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster, cpumask;
+     bool last_man = false, skip_wfi = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+     cpumask = (1 << cpu);
+
+     pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+     BUG_ON(cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+                     cluster >= EXYNOS5420_NR_CLUSTERS);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     arch_spin_lock(&bl_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     cpu_use_count[cpu][cluster]--;
+     if (cpu_use_count[cpu][cluster] == 0) {
+             int cnt = 0, i;
+
+             exynos_core_power_control(cpu, cluster, 0);
+             for (i = 0; i < EXYNOS5420_CPUS_PER_CLUSTER; i++)
+                     cnt += cpu_use_count[i][cluster];
+             if (cnt == 0)
+                     last_man = true;
+     } else if (cpu_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(&bl_lock);
+
+             /*
+              * Flush all cache levels for this cluster.
+              *
+              * To do so we do:
+              * - Clear the SCTLR.C bit to prevent further cache allocations
+              * - Flush the whole cache
+              * - Clear the ACTLR "SMP" bit to disable local coherency
+              *
+              * Let's do it in the safest possible way i.e. with
+              * no memory access within the following sequence
+              * including to the stack.
+              *
+              * Note: fp is preserved to the stack explicitly prior doing
+              * this since adding it to the clobber list is incompatible

arch/arm/mach-exynos/mcpm-exynos.c line 228

Quoted 5 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+              * with having CONFIG_FRAME_POINTER=y.
+              *
+              * The common v7_exit_coherency_flush API that could not be
+              * used because of the Erratum 799270 workaround.
+              */

Bummer. Could you at least create a macro locally, similar to v7_exit_coherency_flush, so not to duplicate this code twice please?

Quoted 20 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +19 -0

+             asm volatile(
+             "stmfd  sp!, {fp, ip}\n\t"
+             "mrc    p15, 0, r0, c1, c0, 0   @ get SCTLR\n\t"
+             "bic    r0, r0, #"__stringify(CR_C)"\n\t"
+             "mcr    p15, 0, r0, c1, c0, 0   @ set SCTLR\n\t"
+             "isb\n\t"
+             "bl     v7_flush_dcache_all\n\t"
+             "clrex\n\t"
+             "mrc    p15, 0, r0, c1, c0, 1   @ get ACTLR\n\t"
+             "bic    r0, r0, #(1 << 6)       @ disable local coherency\n\t"
+             /* Dummy Load of a device register to avoid Erratum 799270 */
+             "ldr    r4, [%0]\n\t"
+             "and    r4, r4, #0\n\t"
+             "orr    r0, r0, r4\n\t"
+             "mcr    p15, 0, r0, c1, c0, 1   @ set ACTLR\n\t"
+             "isb\n\t"
+             "dsb\n\t"
+             "ldmfd  sp!, {fp, ip}"
+             :

arch/arm/mach-exynos/mcpm-exynos.c line 257

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+             : "Ir" (S5P_INFORM0)
+             : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
+                   "r9", "r10", "lr", "memory");
+
+             /*
+              * This is a harmless no-op.  On platforms with a real
+              * outer cache this might either be needed or not,
+              * depending on where the outer cache sits.
+              */
+             outer_flush_all();

If you have no actual outer cache, please remove this call. In most cases with existing outer cache controllers this call is wrong anyway. I've already sent a patch removing it from dcscb.c to RMK.

arch/arm/mach-exynos/mcpm-exynos.c line 264

Quoted 7 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +6 -0

+             /*
+              * Disable cluster-level coherency by masking
+              * incoming snoops and DVM messages:
+              */
+             cci_control_port_by_cpu(mpidr, false);
+             cci_control_port_by_cpu(mpidr ^ (1 << 8), false);

See my comment above for not disabling the remote cluster.

Quoted 26 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +25 -0

+
+             __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+     } else {
+             arch_spin_unlock(&bl_lock);
+
+             /*
+              * Flush the local CPU cache.
+              * Let's do it in the safest possible way as above.
+              */
+             asm volatile(
+             "stmfd  sp!, {fp, ip}\n\t"
+             "mrc    p15, 0, r0, c1, c0, 0   @ get SCTLR\n\t"
+             "bic    r0, r0, #"__stringify(CR_C)"\n\t"
+             "mcr    p15, 0, r0, c1, c0, 0   @ set SCTLR\n\t"
+             "isb\n\t"
+             "bl     v7_flush_dcache_louis\n\t"
+             "clrex\n\t"
+             "mrc    p15, 0, r0, c1, c0, 1   @ get ACTLR\n\t"
+             "bic    r0, r0, #(1 << 6)       @ disable local coherency\n\t"
+             /* Dummy Load of a device register to avoid Erratum 799270 */
+             "ldr    r4, [%0]\n\t"
+             "and    r4, r4, #0\n\t"
+             "orr    r0, r0, r4\n\t"
+             "mcr    p15, 0, r0, c1, c0, 1   @ set ACTLR\n\t"
+             "isb\n\t"

arch/arm/mach-exynos/mcpm-exynos.c line 250

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +6 -0

+             "dsb\n\t"
+             "ldmfd  sp!, {fp, ip}"
+             :
+             : "Ir" (S5P_INFORM0)
+             : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
+                   "r9", "r10", "lr", "memory");

See my note above for not duplicating this code.

arch/arm/mach-exynos/mcpm-exynos.c line 301

Quoted 7 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +6 -0

+     }
+
+     __mcpm_cpu_down(cpu, cluster);
+
+     /* Now we are prepared for power-down, do it: */
+     dsb();

This dsb is redundant. The cache maintenance implied by __mcpm_cpu_down() already contains a dsb.

Quoted 6 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+     if (!skip_wfi)
+             wfi();
+
+     /* Not dead at this point?  Let our caller cope. */
+}

arch/arm/mach-exynos/mcpm-exynos.c line 316

Quoted 10 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +10 -0

+
+static int exynos_power_down_finish(unsigned int cpu, unsigned int cluster)
+{
+     pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+     BUG_ON(cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
+                     cluster >= EXYNOS5420_NR_CLUSTERS);
+
+     /* Wait for the core state to be OFF */
+     while (exynos_core_power_state(cpu, cluster) != 0x0)
+             ;

Unlike above, here is the proper location to implement a timeout.

Quoted 21 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +21 -0

+
+     return 0; /* success: the CPU is halted */
+}
+
+static const struct mcpm_platform_ops exynos_power_ops = {
+     .power_up               = exynos_power_up,
+     .power_down             = exynos_power_down,
+     .power_down_finish      = exynos_power_down_finish,
+};
+
+static void __init exynos_mcpm_usage_count_init(void)
+{
+     unsigned int mpidr, cpu, cluster, i;
+
+     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 >= EXYNOS5420_CPUS_PER_CLUSTER  ||
+                     cluster >= EXYNOS5420_NR_CLUSTERS);

arch/arm/mach-exynos/mcpm-exynos.c line 342

Quoted 5 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+
+     for (i = 0; i < EXYNOS5420_CPUS_PER_CLUSTER; i++) {
+             cpu_use_count[i][0] = 0;
+             cpu_use_count[i][1] = 0;
+     }

Global non-initialized variables are already initialized to zero by default. You therefore don't need to do it here.

Quoted 45 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +44 -0

+     cpu_use_count[cpu][cluster] = 1;
+}
+
+static size_t bL_check_status(char *info)
+{
+     size_t len = 0;
+     int i;
+
+     len += sprintf(&info[len], "\t0 1 2 3 L2\n");
+     len += sprintf(&info[len], "[A15]   ");
+     for (i = 0; i < 4; i++) {
+             len += sprintf(&info[len], "%d ",
+                     (readl(EXYNOS_ARM_CORE_STATUS(i)) & 0x3) == 3 ? 1 : 0);
+     }
+     len += sprintf(&info[len], "%d\n",
+                     (readl(EXYNOS_COMMON_STATUS(0)) & 0x3) == 3 ? 1 : 0);
+
+     len += sprintf(&info[len], "[A7]    ");
+     for (i = 4; i < 8; i++)
+             len += sprintf(&info[len], "%d ",
+                     (readl(EXYNOS_ARM_CORE_STATUS(i)) & 0x3) == 3 ? 1 : 0);
+     len += sprintf(&info[len], "%d\n\n",
+                     (readl(EXYNOS_COMMON_STATUS(1)) & 0x3) == 3 ? 1 : 0);
+
+     return len;
+}
+
+static ssize_t bL_status_read(struct file *file, char __user *buf,
+                     size_t len, loff_t *pos)
+{
+     size_t count = 0;
+     char info[100];
+
+     count = bL_check_status(info);
+     if (count < 0)
+             return -EINVAL;
+
+     return simple_read_from_buffer(buf, len, pos, info, count);
+}
+
+static const struct file_operations bL_status_fops = {
+     .read           = bL_status_read,
+};
+

arch/arm/mach-exynos/mcpm-exynos.c line 391

Quoted 5 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+static struct miscdevice bL_status_device = {
+     MISC_DYNAMIC_MINOR,
+     "bL_status",
+     &bL_status_fops
+};

Please split this debug code out to a separate patch so it can be evaluated on its own.

arch/arm/mach-exynos/mcpm-exynos.c line 403

Quoted 12 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +11 -0

+extern void exynos_power_up_setup(unsigned int affinity_level);
+
+static int __init exynos_mcpm_init(void)
+{
+     int ret = 0;
+
+     if (!cci_probed())
+             return -ENODEV;
+
+     if (!soc_is_exynos5420())
+             return 0;

You should return -ENODEV here as well. Also this would be better to test this before calling cci_probed().

Quoted 15 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +14 -0

+      * To increase the stability of KFC reset we need to program
+      * the PMU SPARE3 register
+      */
+     __raw_writel(EXYNOS5420_SWRESET_KFC_SEL, S5P_PMU_SPARE3);
+
+     exynos_mcpm_usage_count_init();
+
+     ret = mcpm_platform_register(&exynos_power_ops);
+     if (!ret)
+             ret = mcpm_sync_init(exynos_power_up_setup);
+
+     pr_info("Exynos MCPM support installed\n");
+
+     /*

arch/arm/mach-exynos/mcpm-exynos.c line 424

Quoted 5 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +5 -0

+      * Future entries into the kernel can now go
+      * through the cluster entry vectors.
+      */
+     __raw_writel(virt_to_phys(mcpm_entry_point),
+             REG_ENTRY_ADDR);

You should probably do the positive pr_info() and change the vector entry point only when ret is equal to 0.

Quoted 51 lines

arch/arm/mach-exynos/mcpm-exynos.c new file +20 -0

+
+     return ret;
+}
+
+early_initcall(exynos_mcpm_init);
+
+static int __init exynos_bl_status_init(void)
+{
+     int ret;
+
+     if (!soc_is_exynos5420())
+             return 0;
+
+     ret = misc_register(&bL_status_device);
+     if (ret)
+             pr_info("bl_status not available\n");
+     return 0;
+}
+
+late_initcall(exynos_bl_status_init);

arch/arm/mach-exynos/platsmp.c +14 -0

index 03e5e9f..4f14457 100644
@@ -25,6 +25,7 @@
 #include <asm/smp_plat.h>
 #include <asm/smp_scu.h>
 #include <asm/firmware.h>
+#include <asm/mcpm.h>
 
 #include <plat/cpu.h>
 

@@ -226,6 +227,24 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
      }
 }
 
+bool __init exynos_smp_init(void)
+{
+#ifdef CONFIG_MCPM
+     /*
+      * The best way to detect a multi-cluster configuration at the moment
+      * is to look for the presence of a CCI in the system.
+      * Override the default exynos_smp_ops if so.
+      */
+     struct device_node *node;
+     node = of_find_compatible_node(NULL, NULL, "arm,cci-400");
+     if (node && of_device_is_available(node)) {
+             mcpm_smp_set_ops();
+             return true;