Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

Hi,

On 11.11.2013 11:58, Tarek Dakhran wrote:



On Thu, Nov 07, 2013 at 11:51:49AM -0500, Nicolas Pitre wrote:
Samsung people: could you give us more info on the behavior of the power
controller please?


Nicolas

This is how the power controller works on exynos5410. For example for 
CORE0.


ARM_CORE0_STATUS register indicates the power state of Core 0 part of 
processor core.
0x3 indicates that power to Core 0 is turned-on. 0x0 indicates that 
power to Core 0 is turned-off.
All other values indicate that the power On/Off sequence of Core 0 in 
progress.


To turn Off the power of Core 0 power domain:

1. Set the LOCAL_POWER_CFG field of ARM_CORE0_CONFIGURATION register 
to 0x3.
2. After PMU detects a change in the LOCAL_POWER_CFG field, it waits 
for the execution of WFI.
3. After Core 0 executes the WFI instruction, PMU starts the 
power-down sequence.
4. The Status field of ARM_CORE0_STATUS register indicates the 
completion of the sequence.


That's why in the v1 of this patch exynos_core_power_control function 
was implemented as:


static int exynos_core_power_control(unsigned int cpu, unsigned int 
cluster,  int enable)

{
   unsigned long timeout = jiffies + msecs_to_jiffies(10);
   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;

   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset)) & 0x3) == value)
   return 0;

   __raw_writel(value, EXYNOS5410_CORE_CONFIGURATION(offset));
   do {
   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset)) & 0x3)
   == value)
   return 0;
   } while (time_before(jiffies, timeout));

   return -EDEADLK;
}

But, as i mentioned, this is no good using while here.
Now thinking about the problem.

Thank you,
Tarek Dakhran


What do you think about this way of solving the problem with races?

Add new edcs_power_controller_wait function.

static void edcs_power_controller_wait(unsigned int cpu, unsigned int 
cluster){


unsigned long timeout = jiffies + msecs_to_jiffies(10);
unsigned int offset = cluster * EDCS_CPUS_PER_CLUSTER + cpu;
void __iomem *status_reg = EDCS_CORE_STATUS(offset);

/* wait till core power controller finish the work */

do {
if ((readl_relaxed(status_reg) & 3) == 
edcs_use_count[cpu][cluster] ? 3 : 0)

return;
} while (time_before(jiffies, timeout));

/* Should never get here */
BUG();
}

Use it in:

static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
{
exynos_core_power_control(cpu, cluster, true);
edcs_power_controller_wait(cpu, cluster);
}

and in:

static void exynos_power_down(void)
{
bool last_man = false, skip_wfi = false;
unsigned int mpidr = read_cpuid_mpidr();
unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
unsigned int cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);


pr_debug("%s: CORE%d on CLUSTER %d\n", __func__, cpu, cluster);
BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER  || cluster >= EDCS_CLUSTERS);

__mcpm_cpu_going_down(cpu, cluster);

arch_spin_lock(_lock);
BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
edcs_use_count[cpu][cluster]--;
if (edcs_use_count[cpu][cluster] == 0) {
exynos_core_power_down(cpu, cluster);
--core_count[cluster];
if (core_count[cluster] == 0)
last_man = true;
[snip]
__mcpm_cpu_down(cpu, cluster);

if (!skip_wfi){
wfi();
}
edcs_power_controller_wait(cpu, cluster);
}

Comments appreciated. Thanks.

Best regards,
Tarek Dakhran.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

On 07.11.2013 17:01, Dave Martin wrote:

On Thu, Nov 07, 2013 at 08:12:48AM +, Vyacheslav Tyrtov wrote:

From: Tarek Dakhran 

Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.

Signed-off-by: Tarek Dakhran 
Signed-off-by: Vyacheslav Tyrtov 
---
  arch/arm/mach-exynos/Makefile |   2 +
  arch/arm/mach-exynos/edcs.c   | 278 ++
  2 files changed, 280 insertions(+)
  create mode 100644 arch/arm/mach-exynos/edcs.c

diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index 5369615..ba6efdb 100644
--- a/arch/arm/mach-exynos/Makefile
+++ b/arch/arm/mach-exynos/Makefile
@@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o   :=-Wa,-march=armv7-a$(plus_sec)
  
  obj-$(CONFIG_MACH_EXYNOS4_DT)		+= mach-exynos4-dt.o

  obj-$(CONFIG_MACH_EXYNOS5_DT) += mach-exynos5-dt.o
+
+obj-$(CONFIG_SOC_EXYNOS5410)   += edcs.o
diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
new file mode 100644
index 000..980bfdd
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,278 @@
+/*
+ * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran 
+ *
+ * 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.
+ *
+ * EDCS(exynos dual cluster support) for Exynos5410 SoC.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#define EDCS_CPUS_PER_CLUSTER  4
+#define EDCS_CLUSTERS  2
+
+/* Exynos5410 power management registers */
+#define EDCS_CORE_CONFIGURATION(_nr)   (S5P_ARM_CORE0_CONFIGURATION\
+   + ((_nr) * 0x80))
+#define EDCS_CORE_STATUS(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x4)
+#define EDCS_CORE_OPTION(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x8)
+
+#define REG_CPU_STATE_ADDR0(S5P_VA_SYSRAM_NS + 0x28)
+#define REG_CPU_STATE_ADDR(_nr)(REG_CPU_STATE_ADDR0 +  \
+(_nr) * EDCS_CPUS_PER_CLUSTER)
+
+#define SECONDARY_RESET(1 << 1)
+#define REG_ENTRY_ADDR (S5P_VA_SYSRAM_NS + 0x1c)
+
+static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
+static int core_count[EDCS_CLUSTERS];
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+   bool enable)
+{
+   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
+   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+   if ((readl_relaxed(EDCS_CORE_STATUS(offset)) & 0x3) != value) {

I wonder if there is a race here.

If there is a pending powerdown which has reached the __mcpm_cpu_down()
stage, then the kernel has no way to know what is still pending.  This
means that when calling exynos_power_up(cpu, cluster) after a successful
call to exynos_power_down(same cpu, cluster), there is a chance that
the CPU still gets powered down, because of the pending
exynos_core_power_control() on the outbound side.

This isn't an issue for TC2, because TC2's power controller queues
requests and services them in order, so a new powerup request cannot
race with a powerdown request in that way.

For exynos5410, it looks like the kernel needs to do that sequencing,
based on my guess about what the EDCS_CORE_STATUS() bits tell us.


I think that for correct behaviour we would need to wait for the race to
be resolved here, but only if a powerdown might be pending.

This implies that something like a call to the power_down_finish()
method (which you would need to write -- see my comments below) is
needed in exynos_core_power_up().


It might make sense to have a per-cpu flag that tracks whether a
powerdown is pending.  The flag could be set after
__mcpm_cpu_going_down() is called, and cleared in the powered_up()
method (which you would need to add).


Maybe we should always just poll and wait, though.  exynos_power_up()
should never be called for a CPU that the kernel thinks is already up,
so it should either be down already (in which case we will poll the
status once and then continue), or a power down is pending (in which
case we must wait, but we know the wait will terminate).  This would
be simpler than tracking a "power down pending" flag for each CPU.


+   wmb();
+   writel_relaxed(value, EDCS_CORE_CONFIGURATION(offset));
+   }
+}
+
+static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+   exynos_core_power_control(cpu, cluster, true);
+}
+
+static void exynos_core_power_down(unsigned int cpu, unsigned int cluster)
+{
+   

Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

Hi,

On 08.11.2013 23:21, Nicolas Pitre wrote:

On Fri, 8 Nov 2013, Dave Martin wrote:


On Thu, Nov 07, 2013 at 11:51:49AM -0500, Nicolas Pitre wrote:

On Thu, 7 Nov 2013, Dave Martin wrote:


If there is a pending powerdown which has reached the __mcpm_cpu_down()
stage, then the kernel has no way to know what is still pending.  This
means that when calling exynos_power_up(cpu, cluster) after a successful
call to exynos_power_down(same cpu, cluster), there is a chance that
the CPU still gets powered down, because of the pending
exynos_core_power_control() on the outbound side.
This isn't an issue for TC2, because TC2's power controller queues
requests and services them in order, so a new powerup request cannot
race with a powerdown request in that way.

We still rely on the power controller doing some serialisation.

Come to think of it, I should go and take a look at how cpu_kill()
should be implemented for DSCSB too.


The reason why this isn't an issue for TC2 is because the request to
power down request is sent from within the spinlock protected area which
serializes all requests.  Here exynos_core_power_down() is invoked where
there is no such protection.

We don't wait for the requests to complete before dropping the lock, so
we still rely on the SPC doing some serialisation.

Yes.  But the request order is still preserved in that case.

In this case here, the exynos_core_power_up call is performed with a
lock held, but exynos_core_power_down is not.  This means that, by the
time exynos_core_power_down is about to be called, some other CPU might
have decided that the current CPU should not power down after all and
call exynos_core_power_up.  Which one will win the race and execute
before the other is up in the air.

It is important that the actual power control be tightly related to the
management of the usage count currently and properly done within the
lock protected area.  If the use count is zero in the power_up request
then the power has to be turned on.

However here there is still a chance that the power will be turned off
right away afterwards based on the skip_wfi variable which is wrong.


The simple fix would be to simply move this call up, assuming that the
power is actually turned off only when the WFI signal is asserted.

Can you explain?  I'm not sure I get this -- once the outbound CPU has
gone into blackout there's no way to know when it's finished except
to wait.

The issue here is not about whether or not the outbound has finished
killing itself.  It is about making sure that the actual power knob is
on or off according to the use count.  Therefore the power knob has to
be toggled from within the same lock protected area as the use count for
coherency to be preserved.  If exynos_core_power_down is called outside
of the lock protected area, it is well possible that the use count might
have gone back to 1 in the mean time.


Maybe we should always just poll and wait, though.  exynos_power_up()
should never be called for a CPU that the kernel thinks is already up,
so it should either be down already (in which case we will poll the
status once and then continue), or a power down is pending (in which
case we must wait, but we know the wait will terminate).  This would
be simpler than tracking a "power down pending" flag for each CPU.

That is also a good way to avoid the race.

I guess it will depend on exactly what the power controller does.

Right.

Samsung people: could you give us more info on the behavior of the power
controller please?


Nicolas


This is how the power controller works on exynos5410. For example for CORE0.

ARM_CORE0_STATUS register indicates the power state of Core 0 part of 
processor core.
0x3 indicates that power to Core 0 is turned-on. 0x0 indicates that 
power to Core 0 is turned-off.
All other values indicate that the power On/Off sequence of Core 0 in 
progress.


To turn Off the power of Core 0 power domain:

1. Set the LOCAL_POWER_CFG field of ARM_CORE0_CONFIGURATION register to 0x3.
2. After PMU detects a change in the LOCAL_POWER_CFG field, it waits for 
the execution of WFI.
3. After Core 0 executes the WFI instruction, PMU starts the power-down 
sequence.
4. The Status field of ARM_CORE0_STATUS register indicates the 
completion of the sequence.


That's why in the v1 of this patch exynos_core_power_control function 
was implemented as:


static int exynos_core_power_control(unsigned int cpu, unsigned int 
cluster,  int enable)

{
   unsigned long timeout = jiffies + msecs_to_jiffies(10);
   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;

   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset)) & 0x3) == value)
   return 0;

   __raw_writel(value, EXYNOS5410_CORE_CONFIGURATION(offset));
   do {
   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset)) & 0x3)
   == value)
   return 0;
   } while 

Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

Hi,

On 08.11.2013 23:21, Nicolas Pitre wrote:

On Fri, 8 Nov 2013, Dave Martin wrote:


On Thu, Nov 07, 2013 at 11:51:49AM -0500, Nicolas Pitre wrote:

On Thu, 7 Nov 2013, Dave Martin wrote:


If there is a pending powerdown which has reached the __mcpm_cpu_down()
stage, then the kernel has no way to know what is still pending.  This
means that when calling exynos_power_up(cpu, cluster) after a successful
call to exynos_power_down(same cpu, cluster), there is a chance that
the CPU still gets powered down, because of the pending
exynos_core_power_control() on the outbound side.
This isn't an issue for TC2, because TC2's power controller queues
requests and services them in order, so a new powerup request cannot
race with a powerdown request in that way.

We still rely on the power controller doing some serialisation.

Come to think of it, I should go and take a look at how cpu_kill()
should be implemented for DSCSB too.


The reason why this isn't an issue for TC2 is because the request to
power down request is sent from within the spinlock protected area which
serializes all requests.  Here exynos_core_power_down() is invoked where
there is no such protection.

We don't wait for the requests to complete before dropping the lock, so
we still rely on the SPC doing some serialisation.

Yes.  But the request order is still preserved in that case.

In this case here, the exynos_core_power_up call is performed with a
lock held, but exynos_core_power_down is not.  This means that, by the
time exynos_core_power_down is about to be called, some other CPU might
have decided that the current CPU should not power down after all and
call exynos_core_power_up.  Which one will win the race and execute
before the other is up in the air.

It is important that the actual power control be tightly related to the
management of the usage count currently and properly done within the
lock protected area.  If the use count is zero in the power_up request
then the power has to be turned on.

However here there is still a chance that the power will be turned off
right away afterwards based on the skip_wfi variable which is wrong.


The simple fix would be to simply move this call up, assuming that the
power is actually turned off only when the WFI signal is asserted.

Can you explain?  I'm not sure I get this -- once the outbound CPU has
gone into blackout there's no way to know when it's finished except
to wait.

The issue here is not about whether or not the outbound has finished
killing itself.  It is about making sure that the actual power knob is
on or off according to the use count.  Therefore the power knob has to
be toggled from within the same lock protected area as the use count for
coherency to be preserved.  If exynos_core_power_down is called outside
of the lock protected area, it is well possible that the use count might
have gone back to 1 in the mean time.


Maybe we should always just poll and wait, though.  exynos_power_up()
should never be called for a CPU that the kernel thinks is already up,
so it should either be down already (in which case we will poll the
status once and then continue), or a power down is pending (in which
case we must wait, but we know the wait will terminate).  This would
be simpler than tracking a power down pending flag for each CPU.

That is also a good way to avoid the race.

I guess it will depend on exactly what the power controller does.

Right.

Samsung people: could you give us more info on the behavior of the power
controller please?


Nicolas


This is how the power controller works on exynos5410. For example for CORE0.

ARM_CORE0_STATUS register indicates the power state of Core 0 part of 
processor core.
0x3 indicates that power to Core 0 is turned-on. 0x0 indicates that 
power to Core 0 is turned-off.
All other values indicate that the power On/Off sequence of Core 0 in 
progress.


To turn Off the power of Core 0 power domain:

1. Set the LOCAL_POWER_CFG field of ARM_CORE0_CONFIGURATION register to 0x3.
2. After PMU detects a change in the LOCAL_POWER_CFG field, it waits for 
the execution of WFI.
3. After Core 0 executes the WFI instruction, PMU starts the power-down 
sequence.
4. The Status field of ARM_CORE0_STATUS register indicates the 
completion of the sequence.


That's why in the v1 of this patch exynos_core_power_control function 
was implemented as:


static int exynos_core_power_control(unsigned int cpu, unsigned int 
cluster,  int enable)

{
   unsigned long timeout = jiffies + msecs_to_jiffies(10);
   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;

   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset))  0x3) == value)
   return 0;

   __raw_writel(value, EXYNOS5410_CORE_CONFIGURATION(offset));
   do {
   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset))  0x3)
   == value)
   return 0;
   } while 

Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

On 07.11.2013 17:01, Dave Martin wrote:

On Thu, Nov 07, 2013 at 08:12:48AM +, Vyacheslav Tyrtov wrote:

From: Tarek Dakhran t.dakh...@samsung.com

Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.

Signed-off-by: Tarek Dakhran t.dakh...@samsung.com
Signed-off-by: Vyacheslav Tyrtov v.tyr...@samsung.com
---
  arch/arm/mach-exynos/Makefile |   2 +
  arch/arm/mach-exynos/edcs.c   | 278 ++
  2 files changed, 280 insertions(+)
  create mode 100644 arch/arm/mach-exynos/edcs.c

diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index 5369615..ba6efdb 100644
--- a/arch/arm/mach-exynos/Makefile
+++ b/arch/arm/mach-exynos/Makefile
@@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o   :=-Wa,-march=armv7-a$(plus_sec)
  
  obj-$(CONFIG_MACH_EXYNOS4_DT)		+= mach-exynos4-dt.o

  obj-$(CONFIG_MACH_EXYNOS5_DT) += mach-exynos5-dt.o
+
+obj-$(CONFIG_SOC_EXYNOS5410)   += edcs.o
diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
new file mode 100644
index 000..980bfdd
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,278 @@
+/*
+ * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran t.dakh...@samsung.com
+ *
+ * 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.
+ *
+ * EDCS(exynos dual cluster support) for Exynos5410 SoC.
+ */
+
+#include linux/init.h
+#include linux/io.h
+#include linux/of_address.h
+#include linux/spinlock.h
+#include linux/errno.h
+#include linux/irqchip/arm-gic.h
+
+#include asm/mcpm.h
+#include asm/proc-fns.h
+#include asm/cacheflush.h
+#include asm/cputype.h
+#include asm/cp15.h
+
+#include linux/arm-cci.h
+#include mach/regs-pmu.h
+
+#define EDCS_CPUS_PER_CLUSTER  4
+#define EDCS_CLUSTERS  2
+
+/* Exynos5410 power management registers */
+#define EDCS_CORE_CONFIGURATION(_nr)   (S5P_ARM_CORE0_CONFIGURATION\
+   + ((_nr) * 0x80))
+#define EDCS_CORE_STATUS(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x4)
+#define EDCS_CORE_OPTION(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x8)
+
+#define REG_CPU_STATE_ADDR0(S5P_VA_SYSRAM_NS + 0x28)
+#define REG_CPU_STATE_ADDR(_nr)(REG_CPU_STATE_ADDR0 +  \
+(_nr) * EDCS_CPUS_PER_CLUSTER)
+
+#define SECONDARY_RESET(1  1)
+#define REG_ENTRY_ADDR (S5P_VA_SYSRAM_NS + 0x1c)
+
+static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
+static int core_count[EDCS_CLUSTERS];
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+   bool enable)
+{
+   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
+   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+   if ((readl_relaxed(EDCS_CORE_STATUS(offset))  0x3) != value) {

I wonder if there is a race here.

If there is a pending powerdown which has reached the __mcpm_cpu_down()
stage, then the kernel has no way to know what is still pending.  This
means that when calling exynos_power_up(cpu, cluster) after a successful
call to exynos_power_down(same cpu, cluster), there is a chance that
the CPU still gets powered down, because of the pending
exynos_core_power_control() on the outbound side.

This isn't an issue for TC2, because TC2's power controller queues
requests and services them in order, so a new powerup request cannot
race with a powerdown request in that way.

For exynos5410, it looks like the kernel needs to do that sequencing,
based on my guess about what the EDCS_CORE_STATUS() bits tell us.


I think that for correct behaviour we would need to wait for the race to
be resolved here, but only if a powerdown might be pending.

This implies that something like a call to the power_down_finish()
method (which you would need to write -- see my comments below) is
needed in exynos_core_power_up().


It might make sense to have a per-cpu flag that tracks whether a
powerdown is pending.  The flag could be set after
__mcpm_cpu_going_down() is called, and cleared in the powered_up()
method (which you would need to add).


Maybe we should always just poll and wait, though.  exynos_power_up()
should never be called for a CPU that the kernel thinks is already up,
so it should either be down already (in which case we will poll the
status once and then continue), or a power down is pending (in which
case we must wait, but we know the wait will terminate).  This would
be simpler than tracking a power down pending flag for each CPU.


+   wmb();
+   writel_relaxed(value, 

Re: [PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-11 Thread Tarek Dakhran

Hi,

On 11.11.2013 11:58, Tarek Dakhran wrote:



On Thu, Nov 07, 2013 at 11:51:49AM -0500, Nicolas Pitre wrote:
Samsung people: could you give us more info on the behavior of the power
controller please?


Nicolas

This is how the power controller works on exynos5410. For example for 
CORE0.


ARM_CORE0_STATUS register indicates the power state of Core 0 part of 
processor core.
0x3 indicates that power to Core 0 is turned-on. 0x0 indicates that 
power to Core 0 is turned-off.
All other values indicate that the power On/Off sequence of Core 0 in 
progress.


To turn Off the power of Core 0 power domain:

1. Set the LOCAL_POWER_CFG field of ARM_CORE0_CONFIGURATION register 
to 0x3.
2. After PMU detects a change in the LOCAL_POWER_CFG field, it waits 
for the execution of WFI.
3. After Core 0 executes the WFI instruction, PMU starts the 
power-down sequence.
4. The Status field of ARM_CORE0_STATUS register indicates the 
completion of the sequence.


That's why in the v1 of this patch exynos_core_power_control function 
was implemented as:


static int exynos_core_power_control(unsigned int cpu, unsigned int 
cluster,  int enable)

{
   unsigned long timeout = jiffies + msecs_to_jiffies(10);
   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;

   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset))  0x3) == value)
   return 0;

   __raw_writel(value, EXYNOS5410_CORE_CONFIGURATION(offset));
   do {
   if ((__raw_readl(EXYNOS5410_CORE_STATUS(offset))  0x3)
   == value)
   return 0;
   } while (time_before(jiffies, timeout));

   return -EDEADLK;
}

But, as i mentioned, this is no good using while here.
Now thinking about the problem.

Thank you,
Tarek Dakhran


What do you think about this way of solving the problem with races?

Add new edcs_power_controller_wait function.

static void edcs_power_controller_wait(unsigned int cpu, unsigned int 
cluster){


unsigned long timeout = jiffies + msecs_to_jiffies(10);
unsigned int offset = cluster * EDCS_CPUS_PER_CLUSTER + cpu;
void __iomem *status_reg = EDCS_CORE_STATUS(offset);

/* wait till core power controller finish the work */

do {
if ((readl_relaxed(status_reg)  3) == 
edcs_use_count[cpu][cluster] ? 3 : 0)

return;
} while (time_before(jiffies, timeout));

/* Should never get here */
BUG();
}

Use it in:

static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
{
exynos_core_power_control(cpu, cluster, true);
edcs_power_controller_wait(cpu, cluster);
}

and in:

static void exynos_power_down(void)
{
bool last_man = false, skip_wfi = false;
unsigned int mpidr = read_cpuid_mpidr();
unsigned int cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
unsigned int cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);


pr_debug(%s: CORE%d on CLUSTER %d\n, __func__, cpu, cluster);
BUG_ON(cpu = EDCS_CPUS_PER_CLUSTER  || cluster = EDCS_CLUSTERS);

__mcpm_cpu_going_down(cpu, cluster);

arch_spin_lock(edcs_lock);
BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
edcs_use_count[cpu][cluster]--;
if (edcs_use_count[cpu][cluster] == 0) {
exynos_core_power_down(cpu, cluster);
--core_count[cluster];
if (core_count[cluster] == 0)
last_man = true;
[snip]
__mcpm_cpu_down(cpu, cluster);

if (!skip_wfi){
wfi();
}
edcs_power_controller_wait(cpu, cluster);
}

Comments appreciated. Thanks.

Best regards,
Tarek Dakhran.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-07 Thread Vyacheslav Tyrtov
From: Tarek Dakhran 

Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.

Signed-off-by: Tarek Dakhran 
Signed-off-by: Vyacheslav Tyrtov 
---
 arch/arm/mach-exynos/Makefile |   2 +
 arch/arm/mach-exynos/edcs.c   | 278 ++
 2 files changed, 280 insertions(+)
 create mode 100644 arch/arm/mach-exynos/edcs.c

diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index 5369615..ba6efdb 100644
--- a/arch/arm/mach-exynos/Makefile
+++ b/arch/arm/mach-exynos/Makefile
@@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o   :=-Wa,-march=armv7-a$(plus_sec)
 
 obj-$(CONFIG_MACH_EXYNOS4_DT)  += mach-exynos4-dt.o
 obj-$(CONFIG_MACH_EXYNOS5_DT)  += mach-exynos5-dt.o
+
+obj-$(CONFIG_SOC_EXYNOS5410)   += edcs.o
diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
new file mode 100644
index 000..980bfdd
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,278 @@
+/*
+ * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran 
+ *
+ * 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.
+ *
+ * EDCS(exynos dual cluster support) for Exynos5410 SoC.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#define EDCS_CPUS_PER_CLUSTER  4
+#define EDCS_CLUSTERS  2
+
+/* Exynos5410 power management registers */
+#define EDCS_CORE_CONFIGURATION(_nr)   (S5P_ARM_CORE0_CONFIGURATION\
+   + ((_nr) * 0x80))
+#define EDCS_CORE_STATUS(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x4)
+#define EDCS_CORE_OPTION(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x8)
+
+#define REG_CPU_STATE_ADDR0(S5P_VA_SYSRAM_NS + 0x28)
+#define REG_CPU_STATE_ADDR(_nr)(REG_CPU_STATE_ADDR0 +  \
+(_nr) * EDCS_CPUS_PER_CLUSTER)
+
+#define SECONDARY_RESET(1 << 1)
+#define REG_ENTRY_ADDR (S5P_VA_SYSRAM_NS + 0x1c)
+
+static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
+static int core_count[EDCS_CLUSTERS];
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+   bool enable)
+{
+   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
+   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+   if ((readl_relaxed(EDCS_CORE_STATUS(offset)) & 0x3) != value) {
+   wmb();
+   writel_relaxed(value, EDCS_CORE_CONFIGURATION(offset));
+   }
+}
+
+static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+   exynos_core_power_control(cpu, cluster, true);
+}
+
+static void exynos_core_power_down(unsigned int cpu, unsigned int cluster)
+{
+   exynos_core_power_control(cpu, cluster, false);
+}
+
+void set_boot_flag(unsigned int cpu, unsigned int mode)
+{
+   writel_relaxed(mode, REG_CPU_STATE_ADDR(cpu));
+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+   pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+   BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER || cluster >= EDCS_CLUSTERS);
+
+   local_irq_disable();
+   arch_spin_lock(_lock);
+
+   edcs_use_count[cpu][cluster]++;
+   if (edcs_use_count[cpu][cluster] == 1) {
+   ++core_count[cluster];
+   set_boot_flag(cpu, SECONDARY_RESET);
+   exynos_core_power_up(cpu, cluster);
+   } else if (edcs_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(_lock);
+   local_irq_enable();
+
+   return 0;
+}
+static void exynos_power_down(void)
+{
+   unsigned int mpidr, cpu, cluster;
+   bool last_man = false, skip_wfi = false;
+
+   mpidr = read_cpuid_mpidr();
+   cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+   cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+   pr_debug("%s: CORE%d on CLUSTER %d\n", __func__, cpu, cluster);
+   BUG_ON(cpu >= EDCS_CPUS_PER_CLUSTER  || cluster >= EDCS_CLUSTERS);
+
+   __mcpm_cpu_going_down(cpu, cluster);
+
+   arch_spin_lock(_lock);
+   BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+   edcs_use_count[cpu][cluster]--;
+   if 

[PATCH v3 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support

2013-11-07 Thread Vyacheslav Tyrtov
From: Tarek Dakhran t.dakh...@samsung.com

Add EDCS(Exynos Dual Cluster Support) for Samsung Exynos5410 SoC.
This enables all 8 cores, 4 x A7 and 4 x A15 run at the same time.

Signed-off-by: Tarek Dakhran t.dakh...@samsung.com
Signed-off-by: Vyacheslav Tyrtov v.tyr...@samsung.com
---
 arch/arm/mach-exynos/Makefile |   2 +
 arch/arm/mach-exynos/edcs.c   | 278 ++
 2 files changed, 280 insertions(+)
 create mode 100644 arch/arm/mach-exynos/edcs.c

diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index 5369615..ba6efdb 100644
--- a/arch/arm/mach-exynos/Makefile
+++ b/arch/arm/mach-exynos/Makefile
@@ -34,3 +34,5 @@ AFLAGS_exynos-smc.o   :=-Wa,-march=armv7-a$(plus_sec)
 
 obj-$(CONFIG_MACH_EXYNOS4_DT)  += mach-exynos4-dt.o
 obj-$(CONFIG_MACH_EXYNOS5_DT)  += mach-exynos5-dt.o
+
+obj-$(CONFIG_SOC_EXYNOS5410)   += edcs.o
diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
new file mode 100644
index 000..980bfdd
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,278 @@
+/*
+ * arch/arm/mach-exynos/edcs.c - exynos dual cluster power management support
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran t.dakh...@samsung.com
+ *
+ * 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.
+ *
+ * EDCS(exynos dual cluster support) for Exynos5410 SoC.
+ */
+
+#include linux/init.h
+#include linux/io.h
+#include linux/of_address.h
+#include linux/spinlock.h
+#include linux/errno.h
+#include linux/irqchip/arm-gic.h
+
+#include asm/mcpm.h
+#include asm/proc-fns.h
+#include asm/cacheflush.h
+#include asm/cputype.h
+#include asm/cp15.h
+
+#include linux/arm-cci.h
+#include mach/regs-pmu.h
+
+#define EDCS_CPUS_PER_CLUSTER  4
+#define EDCS_CLUSTERS  2
+
+/* Exynos5410 power management registers */
+#define EDCS_CORE_CONFIGURATION(_nr)   (S5P_ARM_CORE0_CONFIGURATION\
+   + ((_nr) * 0x80))
+#define EDCS_CORE_STATUS(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x4)
+#define EDCS_CORE_OPTION(_nr)  (EDCS_CORE_CONFIGURATION(_nr) + 0x8)
+
+#define REG_CPU_STATE_ADDR0(S5P_VA_SYSRAM_NS + 0x28)
+#define REG_CPU_STATE_ADDR(_nr)(REG_CPU_STATE_ADDR0 +  \
+(_nr) * EDCS_CPUS_PER_CLUSTER)
+
+#define SECONDARY_RESET(1  1)
+#define REG_ENTRY_ADDR (S5P_VA_SYSRAM_NS + 0x1c)
+
+static arch_spinlock_t edcs_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+
+static int edcs_use_count[EDCS_CPUS_PER_CLUSTER][EDCS_CLUSTERS];
+static int core_count[EDCS_CLUSTERS];
+
+static void exynos_core_power_control(unsigned int cpu, unsigned int cluster,
+   bool enable)
+{
+   unsigned int offset = cluster * MAX_CPUS_PER_CLUSTER + cpu;
+   int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+   if ((readl_relaxed(EDCS_CORE_STATUS(offset))  0x3) != value) {
+   wmb();
+   writel_relaxed(value, EDCS_CORE_CONFIGURATION(offset));
+   }
+}
+
+static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+   exynos_core_power_control(cpu, cluster, true);
+}
+
+static void exynos_core_power_down(unsigned int cpu, unsigned int cluster)
+{
+   exynos_core_power_control(cpu, cluster, false);
+}
+
+void set_boot_flag(unsigned int cpu, unsigned int mode)
+{
+   writel_relaxed(mode, REG_CPU_STATE_ADDR(cpu));
+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+   pr_debug(%s: cpu %u cluster %u\n, __func__, cpu, cluster);
+   BUG_ON(cpu = EDCS_CPUS_PER_CLUSTER || cluster = EDCS_CLUSTERS);
+
+   local_irq_disable();
+   arch_spin_lock(edcs_lock);
+
+   edcs_use_count[cpu][cluster]++;
+   if (edcs_use_count[cpu][cluster] == 1) {
+   ++core_count[cluster];
+   set_boot_flag(cpu, SECONDARY_RESET);
+   exynos_core_power_up(cpu, cluster);
+   } else if (edcs_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(edcs_lock);
+   local_irq_enable();
+
+   return 0;
+}
+static void exynos_power_down(void)
+{
+   unsigned int mpidr, cpu, cluster;
+   bool last_man = false, skip_wfi = false;
+
+   mpidr = read_cpuid_mpidr();
+   cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+   cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+   pr_debug(%s: CORE%d on CLUSTER %d\n, __func__, cpu, cluster);
+