Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
hi Vyacheslav;
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 | 270
++
2 files changed, 272 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..e304bd9
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,270 @@
+/*
+ * 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 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_CLUSTER4
+#define EDCS_CLUSTERS2
+
+/* 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)
+
+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 * EDCS_CPUS_PER_CLUSTER + cpu;
+ int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+ if ((__raw_readl(EDCS_CORE_STATUS(offset)) 0x3) != value)
+ __raw_writel(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)
+{
+ __raw_writel(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, 0x2);
+ 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: cpu %u cluster %u\n, __func__, cpu, cluster);
+ BUG_ON(cpu = EDCS_CPUS_PER_CLUSTER || cluster = EDCS_CLUSTERS);
+
+
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On 17.10.2013 19:46, Dave Martin wrote:
On Mon, Oct 14, 2013 at 07:08:24PM +0400, 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 | 270 ++
2 files changed, 272 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..e304bd9
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,270 @@
+/*
+ * 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.
+ */
+
[snip]
+#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)
Is there any reason why S5P_VA_SYSRAM_NS needs to be a static mapping?
What do you mean by static mapping?
+#define REG_CPU_STATE_ADDR(_nr)(REG_CPU_STATE_ADDR0 + \
+_nr * EDCS_CPUS_PER_CLUSTER)
+
+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 * EDCS_CPUS_PER_CLUSTER + cpu;
+ int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+ if ((__raw_readl(EDCS_CORE_STATUS(offset)) 0x3) != value)
+ __raw_writel(value, EDCS_CORE_CONFIGURATION(offset));
I think you need to replace all the __raw_readl() / __raw_writel() calls
in this file with readl_relaxed() / writel_relaxed().
This ensures little-endian byte order, so that BE8 kernels will work.
Will be done.
+}
+
+static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+ exynos_core_power_control(cpu, cluster, true);
+}
[snip]
+
+ edcs_use_count[cpu][cluster]++;
+ if (edcs_use_count[cpu][cluster] == 1) {
+ ++core_count[cluster];
+ set_boot_flag(cpu, 0x2);
0x2 looks like a magic number. Can we have a #define for that?
Will be done.
If the boot flag is read by the inbound CPU or by a peripheral then
there are memory ordering issues to take into account. Otherwise, can't
the inbound CPU come online and race with the write to the boot flag?
Inbound CPU doesn't write the boot flag.
What is the memory type of REG_STATE_ADDR()?
Same type as S5P_VA_SYSRAM_NS.
This 4K region is mapped as follows:
static struct map_desc exynos5410_iodesc[] __initdata = {
{
.virtual= (unsigned long)S5P_VA_SYSRAM_NS,
.pfn= __phys_to_pfn(EXYNOS5410_PA_SYSRAM_NS),
.length = SZ_4K,
.type = MT_DEVICE,
},
};
+ 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: cpu %u cluster %u\n, __func__, cpu, cluster);
+ BUG_ON(cpu = EDCS_CPUS_PER_CLUSTER || cluster = EDCS_CLUSTERS);
+
+
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On Fri, Oct 18, 2013 at 12:29:03PM +0400, Tarek Dakhran wrote:
On 17.10.2013 19:46, Dave Martin wrote:
On Mon, Oct 14, 2013 at 07:08:24PM +0400, 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 | 270
++
2 files changed, 272 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..e304bd9
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,270 @@
+/*
+ * 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.
+ */
+
[snip]
+#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)
Is there any reason why S5P_VA_SYSRAM_NS needs to be a static mapping?
What do you mean by static mapping?
See below
+#define REG_CPU_STATE_ADDR(_nr) (REG_CPU_STATE_ADDR0 + \
+ _nr * EDCS_CPUS_PER_CLUSTER)
+
+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 * EDCS_CPUS_PER_CLUSTER + cpu;
+ int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+ if ((__raw_readl(EDCS_CORE_STATUS(offset)) 0x3) != value)
+ __raw_writel(value, EDCS_CORE_CONFIGURATION(offset));
I think you need to replace all the __raw_readl() / __raw_writel() calls
in this file with readl_relaxed() / writel_relaxed().
This ensures little-endian byte order, so that BE8 kernels will work.
Will be done.
+}
+
+static void exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+ exynos_core_power_control(cpu, cluster, true);
+}
[snip]
+
+ edcs_use_count[cpu][cluster]++;
+ if (edcs_use_count[cpu][cluster] == 1) {
+ ++core_count[cluster];
+ set_boot_flag(cpu, 0x2);
0x2 looks like a magic number. Can we have a #define for that?
Will be done.
Thanks
If the boot flag is read by the inbound CPU or by a peripheral then
there are memory ordering issues to take into account. Otherwise, can't
the inbound CPU come online and race with the write to the boot flag?
Inbound CPU doesn't write the boot flag.
Sorry, I didn't explain this clearly. The scenario I'm thinking about is:
CPU0 writes 2 to the boot flag
The write is temporarily held in a write buffer somewhere in the
memory system.
CPU0 pokes the power controller to bring CPU1 online
CPU1 powers up and starts running its boot ROM
CPU1 reads the boot flag, but misses the value written by CPU0
because that is still held in a write buffer somewhere, and isn't
globally visible.
... and finally ...
The write to the boot flag drains and becomes globally visible.
But it's too late now.
There's also the problem that set_boot_flag() and the read by the
boot ROM probably don't use the same memory type (Device versus
Strongly-Ordered for example). The ARM Architecture doesn't guarantee
full coherency in situations like this.
I believe that a correct solution to this problem is to put a wmb()
between setting the boot flag and poking the power controller.
Putting the wmb() before the writel in exynos_core_power_control()
should do the job.
What is the memory type of REG_STATE_ADDR()?
Same type as
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On 10/14/2013 05:08 PM, 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.
IIUC, the 5410 has a CCI-400 bug preventing to use the two clusters at
the same time. Right ? Could you explain how you fixed it ?
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 | 270 ++
2 files changed, 272 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..e304bd9
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,270 @@
+/*
+ * 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 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)
+
+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 * EDCS_CPUS_PER_CLUSTER + cpu;
+ int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+ if ((__raw_readl(EDCS_CORE_STATUS(offset)) 0x3) != value)
+ __raw_writel(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)
+{
+ __raw_writel(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, 0x2);
+ 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: cpu %u cluster %u\n, __func__, cpu, cluster);
+ BUG_ON(cpu =
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On Thu, Oct 17, 2013 at 12:45:29PM +0200, Daniel Lezcano wrote:
On 10/14/2013 05:08 PM, 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.
[...]
+ __mcpm_cpu_down(cpu, cluster);
+
+ if (!skip_wfi) {
+ exynos_core_power_down(cpu, cluster);
+ wfi();
+ }
+}
I did not looked line by line but these functions looks very similar
than the tc2_pm.c's function. no ?
This is true.
May be some code consolidation could be considered here.
Added Nico and Lorenzo in Cc.
Thanks
-- Daniel
Nico can commnent further, but I think the main concern here was that
this code shouldn't be factored prematurely.
There are many low-level platform specifics involved here, so it's
hard to be certain that all platforms could fit into a more abstracted
framework until we have some evidence to look at.
This could be revisited when we have a few diverse MCPM ports to
compare.
The low-level A15/A7 cacheflush sequence is already being factored
by Nico [1].
Cheers
---Dave
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
[PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code
[...]
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On 10/17/2013 04:32 PM, Dave Martin wrote:
On Thu, Oct 17, 2013 at 12:45:29PM +0200, Daniel Lezcano wrote:
On 10/14/2013 05:08 PM, 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.
[...]
+ __mcpm_cpu_down(cpu, cluster);
+
+ if (!skip_wfi) {
+ exynos_core_power_down(cpu, cluster);
+ wfi();
+ }
+}
I did not looked line by line but these functions looks very similar
than the tc2_pm.c's function. no ?
This is true.
May be some code consolidation could be considered here.
Added Nico and Lorenzo in Cc.
Thanks
-- Daniel
Nico can commnent further, but I think the main concern here was that
this code shouldn't be factored prematurely.
I do not share this opinion.
There are many low-level platform specifics involved here, so it's
hard to be certain that all platforms could fit into a more abstracted
framework until we have some evidence to look at.
This could be revisited when we have a few diverse MCPM ports to
compare.
I am worried about seeing more and more duplicated code around the ARM
arch (eg. arm[64]/kernel/smp.c arm64/kernel/smp.c).
The cpuidle drivers have been duplicated and it took a while before
refactoring them, and it is not finished. The hotplug code have been
duplicated and now it is very difficult to factor it out.
A lot of work have been done to consolidate the code across the
different SoC since the last 2 years.
If we let duplicate code populate the different files, they will belong
to different maintainers, thus different trees. Each SoC contributors
will tend to add their small changes making the code to diverge more and
more and making difficult to re-factor it later.
I am in favor of preventing duplicate code entering in the kernel and
force the contributors to improve the kernel in general, not just the
small part they are supposed to work on. Otherwise, we are letting the
kernel to fork itself, internally...
The low-level A15/A7 cacheflush sequence is already being factored
by Nico [1].
Hopefully he did it :)
Thanks
-- Daniel
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/205085.html
[PATCH] ARM: cacheflush: consolidate single-CPU ARMv7 cache disabling code
[...]
--
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] ARM: EXYNOS: add Exynos Dual Cluster Support
On Thu, 17 Oct 2013, Daniel Lezcano wrote:
On 10/17/2013 04:32 PM, Dave Martin wrote:
On Thu, Oct 17, 2013 at 12:45:29PM +0200, Daniel Lezcano wrote:
On 10/14/2013 05:08 PM, 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.
[...]
+ __mcpm_cpu_down(cpu, cluster);
+
+ if (!skip_wfi) {
+ exynos_core_power_down(cpu, cluster);
+ wfi();
+ }
+}
I did not looked line by line but these functions looks very similar
than the tc2_pm.c's function. no ?
This is true.
May be some code consolidation could be considered here.
Added Nico and Lorenzo in Cc.
Thanks
-- Daniel
Nico can commnent further, but I think the main concern here was that
this code shouldn't be factored prematurely.
I do not share this opinion.
There are many low-level platform specifics involved here, so it's
hard to be certain that all platforms could fit into a more abstracted
framework until we have some evidence to look at.
This could be revisited when we have a few diverse MCPM ports to
compare.
I am worried about seeing more and more duplicated code around the ARM arch
(eg. arm[64]/kernel/smp.c arm64/kernel/smp.c).
I didn't look too closely at those files so can't comment much on them.
The cpuidle drivers have been duplicated and it took a while before
refactoring them, and it is not finished. The hotplug code have been
duplicated and now it is very difficult to factor it out.
A lot of work have been done to consolidate the code across the different SoC
since the last 2 years.
If we let duplicate code populate the different files, they will belong to
different maintainers, thus different trees. Each SoC contributors will tend
to add their small changes making the code to diverge more and more and making
difficult to re-factor it later.
I am in favor of preventing duplicate code entering in the kernel and force
the contributors to improve the kernel in general, not just the small part
they are supposed to work on. Otherwise, we are letting the kernel to fork
itself, internally...
Now I'm going to comment.
What you say above is perfectly right. As a general principle, we want
good consolidation. That's the theory.
However you need to know what needs to be consolidated in the first
place. In practice you cannot consolidate duplicated code if that code
doesn't exist. In the cpuidle and hotplug cases it is very easy now to
see what kind of consolidation should have been done after the facts.
I'm sure that 10 years ago that wasn't as obvious.
In the MCPM case we didn't know up front what exactly would end up being
duplicated in each machine specific backend. And to some extent we
still don't know as there is very few backends merged into mainline at
this point (I know more of them exist out of tree but I didn't see the
code yet). Right now we have only 2 such backends and some duplication
was already identified, hence the patch to abstract the v7 cache
flushing I posted recently.
Another possibility for consolidation in the MCPM backends is the last
man determination. However we've seen that the PSCI compatibility
backend doesn't need that and abstracting that just yet might be
premature, possibly making interaction with PSCI very awkward.
So it is very important to get the right balance between code
duplication and over-engineering. Premature abstractions do fall into
the over-engineering category and it is just as bad. Above all it is
most important to be vigilant and take care of duplicated code before it
gets too big.
In the past we've seen bad code duplication in some subsystems because
no one was looking at the big picture. In the MCPM case I'm watching.
And in this case we're far from being in a critical situation.
Nicolas
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
