* [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
2013-10-01 16:17 [PATCH 0/6] Exynos 5410 Dual cluster support Vyacheslav Tyrtov
@ 2013-10-01 16:17 ` Vyacheslav Tyrtov
[not found] ` <1380644227-12244-4-git-send-email-v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-02 12:55 ` Dave Martin
0 siblings, 2 replies; 7+ messages in thread
From: Vyacheslav Tyrtov @ 2013-10-01 16:17 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, devicetree, Kukjin Kim, Russell King, Ben Dooks,
Pawel Moll, Ian Campbell, Stephen Warren, linux-doc, Rob Herring,
Tarek Dakhran, Daniel Lezcano, linux-samsung-soc,
Tyrtov Vyacheslav, Rob Landley, Mike Turquette, Thomas Gleixner,
Naour Romain, linux-arm-kernel, Heiko Stuebner
From: Tarek Dakhran <t.dakhran@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.dakhran@samsung.com>
Signed-off-by: Vyacheslav Tyrtov <v.tyrtov@samsung.com>
---
arch/arm/mach-exynos/Makefile | 2 +
arch/arm/mach-exynos/edcs.c | 217 +++++++++++++++++++++++++++++++++++++
arch/arm/mach-exynos/edcs.h | 41 +++++++
arch/arm/mach-exynos/edcs_status.c | 110 +++++++++++++++++++
4 files changed, 370 insertions(+)
create mode 100644 arch/arm/mach-exynos/edcs.c
create mode 100644 arch/arm/mach-exynos/edcs.h
create mode 100644 arch/arm/mach-exynos/edcs_status.c
diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index 5369615..18e6162 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_ARM_CCI) += edcs.o edcs_status.o
diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
new file mode 100644
index 0000000..34b4e4b
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.c
@@ -0,0 +1,217 @@
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran <t.dakhran@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/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/errno.h>
+#include <linux/vexpress.h>
+#include <asm/mcpm.h>
+#include <asm/proc-fns.h>
+#include <asm/cacheflush.h>
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+
+#include "edcs.h"
+
+static arch_spinlock_t exynos_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+static int kfs_use_count[MAX_CPUS_PER_CLUSTER][MAX_NR_CLUSTERS];
+static int core_count[MAX_NR_CLUSTERS];
+
+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;
+}
+static int exynos_cluster_power_control(unsigned int cluster, int enable)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(10);
+ int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
+
+ if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) & 0x3)
+ == value)
+ return 0;
+
+ __raw_writel(value, EXYNOS5410_COMMON_CONFIGURATION(cluster));
+ do {
+ if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) &
+ __raw_readl(EXYNOS5410_L2_STATUS(cluster)) & 0x3)
+ == value)
+ return 0;
+ } while (time_before(jiffies, timeout));
+
+ return -EDEADLK;
+}
+
+static int exynos_core_power_up(unsigned int cpu, unsigned int cluster)
+{
+ return exynos_core_power_control(cpu, cluster, 1);
+}
+
+static int exynos_core_power_down(unsigned int cpu, unsigned int cluster)
+{
+ return exynos_core_power_control(cpu, cluster, 0);
+}
+
+static int exynos_cluster_power_up(unsigned int cluster)
+{
+ return exynos_cluster_power_control(cluster, 1);
+}
+
+static int exynos_power_up(unsigned int cpu, unsigned int cluster)
+{
+ int ret;
+ local_irq_disable();
+ arch_spin_lock(&exynos_lock);
+
+ pr_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
+ kfs_use_count[cpu][cluster]++;
+ if (kfs_use_count[cpu][cluster] == 1) {
+ ++core_count[cluster];
+ if (core_count[cluster] == 1) {
+ ret = exynos_cluster_power_up(cluster);
+ if (ret) {
+ pr_err("%s: cluster %u power up error\n",
+ __func__, cluster);
+ return ret;
+ }
+ __cci_control_port_by_index(MAX_NR_CLUSTERS
+ - cluster, true);
+ }
+ ret = exynos_core_power_up(cpu, cluster);
+ if (ret) {
+ pr_err("%s: cpu %u cluster %u power up error\n",
+ __func__, cpu, cluster);
+ return ret;
+ }
+ } else if (kfs_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(&exynos_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_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
+ BUG_ON(cpu >= MAX_CPUS_PER_CLUSTER || cluster >= MAX_NR_CLUSTERS);
+ __mcpm_cpu_going_down(cpu, cluster);
+ arch_spin_lock(&exynos_lock);
+ BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+ kfs_use_count[cpu][cluster]--;
+
+ if (kfs_use_count[cpu][cluster] == 0) {
+ exynos_core_power_down(cpu, cluster);
+ --core_count[cluster];
+ if (core_count[cluster] == 0)
+ last_man = true;
+
+ } else if (kfs_use_count[cpu][cluster] == 1) {
+ skip_wfi = true;
+ } else
+ BUG();
+ if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
+ arch_spin_unlock(&exynos_lock);
+ flush_cache_all();
+ set_cr(get_cr() & ~CR_C);
+ flush_cache_all();
+ outer_flush_all();
+ set_auxcr(get_auxcr() & ~(1 << 6));
+ cci_disable_port_by_cpu(mpidr);
+ __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
+ } else {
+ arch_spin_unlock(&exynos_lock);
+ flush_cache_louis();
+ set_cr(get_cr() & ~CR_C);
+ flush_cache_louis();
+ set_auxcr(get_auxcr() & ~(1 << 6));
+ }
+ __mcpm_cpu_down(cpu, cluster);
+
+ dsb();
+ if (!skip_wfi)
+ wfi();
+}
+static const struct mcpm_platform_ops exynos_power_ops = {
+ .power_up = exynos_power_up,
+ .power_down = exynos_power_down,
+};
+
+static void __init edcs_data_init(void)
+{
+ unsigned int mpidr, cpu, cluster;
+
+ mpidr = read_cpuid_mpidr();
+ cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+ cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+ pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
+ BUG_ON(cpu >= 4 || cluster >= 2);
+ kfs_use_count[cpu][cluster] = 1;
+ ++core_count[cluster];
+ __cci_control_port_by_index(MAX_NR_CLUSTERS - cluster, true);
+}
+
+
+static int __init edcs_init(void)
+{
+ struct device_node *node;
+
+ if (!cci_probed())
+ return -ENODEV;
+
+ node = of_find_compatible_node(NULL, NULL, "samsung,edcs");
+ if (!node)
+ return -ENODEV;
+ edcs_data_init();
+ mcpm_smp_set_ops();
+ mcpm_platform_register(&exynos_power_ops);
+
+ /*
+ * Future entries into the kernel can now go
+ * through the cluster entry vectors.
+ */
+
+ __raw_writel(virt_to_phys(mcpm_entry_point),
+ S5P_VA_SYSRAM_NS + 0x1c);
+
+ pr_info("EDCS: EXYNOS5410 DUAL CLUSTER SUPPORT installed\n");
+
+ return 0;
+}
+
+early_initcall(edcs_init);
diff --git a/arch/arm/mach-exynos/edcs.h b/arch/arm/mach-exynos/edcs.h
new file mode 100644
index 0000000..dd3e204
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran <t.dakhran@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/arm-cci.h>
+#include <linux/of_address.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <mach/regs-pmu.h>
+
+#define EXYNOS5410_ARM_COMMON_CONFIGURATION S5P_PMUREG(0x2500)
+#define EXYNOS5410_COMMON_CONFIGURATION(_nr) \
+ (EXYNOS5410_ARM_COMMON_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS5410_COMMON_STATUS(_nr) \
+ (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x4)
+#define EXYNOS5410_COMMON_OPTION(_nr) \
+ (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x8)
+
+#define EXYNOS5410_ARM_L2_CONFIGURATION S5P_PMUREG(0x2600)
+#define EXYNOS5410_L2_CONFIGURATION(_nr) \
+ (EXYNOS5410_ARM_L2_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS5410_L2_STATUS(_nr) \
+ (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x4)
+#define EXYNOS5410_L2_OPTION(_nr) \
+ (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x8)
+
+#define EXYNOS5410_CORE_CONFIGURATION(_nr) \
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define EXYNOS5410_CORE_STATUS(_nr) \
+ (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x4)
+#define EXYNOS5410_CORE_OPTION(_nr) \
+ (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x8)
+
+#define EXYNOS5_PA_CCI 0x10D20000
diff --git a/arch/arm/mach-exynos/edcs_status.c b/arch/arm/mach-exynos/edcs_status.c
new file mode 100644
index 0000000..bff1e7f
--- /dev/null
+++ b/arch/arm/mach-exynos/edcs_status.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tarek Dakhran <t.dakhran@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/fs.h>
+#include <linux/miscdevice.h>
+#include <asm/uaccess.h>
+
+#include "edcs.h"
+
+int edcs_check_status(char *info)
+{
+ size_t it = 30;
+ int i;
+
+ void __iomem *cci_base;
+
+ cci_base = ioremap(EXYNOS5_PA_CCI, SZ_64K);
+ if (!cci_base)
+ return -ENOMEM;
+
+ if ((__raw_readl(EXYNOS5410_COMMON_STATUS(0)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+
+ for (i = 0; i < 4; i++) {
+ if ((__raw_readl(EXYNOS5410_CORE_STATUS(i)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+ }
+
+ if ((__raw_readl(EXYNOS5410_L2_STATUS(0)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+
+ if ((readl(cci_base + 0x4000 + 1 * 0x1000) & 0x3) == 3)
+ info[it] = '1';
+
+ it += 10;
+
+ if ((__raw_readl(EXYNOS5410_COMMON_STATUS(1)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+
+ for (i = 4; i < 8; i++) {
+ if ((__raw_readl(EXYNOS5410_CORE_STATUS(i)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+ }
+
+ if ((__raw_readl(EXYNOS5410_L2_STATUS(1)) & 0x3) == 3)
+ info[it] = '1';
+ it += 3;
+
+ if ((readl(cci_base + 0x4000 + 0 * 0x1000) & 0x3) == 3)
+ info[it] = '1';
+
+ iounmap(cci_base);
+ return 0;
+}
+
+static ssize_t edcs_status_read(struct file *file, char __user *buf,
+ size_t len, loff_t *pos)
+{
+
+ char info[] = "\tC 0 1 2 3 L2 CCI\n"
+ "[A15] 0 0 0 0 0 0 0\n"
+ " [A7] 0 0 0 0 0 0 0\n";
+ size_t count = sizeof(info);
+ edcs_check_status(info);
+ return simple_read_from_buffer(buf, len, pos, info, count);
+}
+
+static const struct file_operations edcs_status_fops = {
+ .read = edcs_status_read,
+ .owner = THIS_MODULE,
+};
+static struct miscdevice edcs_status_device = {
+ .name = "edcs_status",
+ .minor = 255,
+ .fops = &edcs_status_fops,
+};
+
+static int __init edcs_dev_init(void)
+{
+ struct device_node *node;
+ int ret;
+ if (!cci_probed())
+ return -ENODEV;
+
+ node = of_find_compatible_node(NULL, NULL, "samsung,edcs");
+ if (!node)
+ return -ENODEV;
+
+ ret = misc_register(&edcs_status_device);
+ if (ret) {
+ pr_err("EDCS: edcs_status device is not registered\n");
+ return ret;
+ }
+ pr_info("EDCS: edcs_status device registered\n");
+ return 0;
+}
+device_initcall(edcs_dev_init);
--
1.8.1.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
[not found] ` <1380644227-12244-4-git-send-email-v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2013-10-01 19:55 ` Nicolas Pitre
2013-10-02 13:05 ` Dave Martin
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Pitre @ 2013-10-01 19:55 UTC (permalink / raw)
To: Vyacheslav Tyrtov
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
Kukjin Kim, Russell King, Ben Dooks, Mike Turquette,
Daniel Lezcano, Thomas Gleixner, Heiko Stuebner, Naour Romain,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Tarek Dakhran
On Tue, 1 Oct 2013, Vyacheslav Tyrtov wrote:
> From: Tarek Dakhran <t.dakhran-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>
> 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.dakhran-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vyacheslav Tyrtov <v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> arch/arm/mach-exynos/Makefile | 2 +
> arch/arm/mach-exynos/edcs.c | 217 +++++++++++++++++++++++++++++++++++++
> arch/arm/mach-exynos/edcs.h | 41 +++++++
> arch/arm/mach-exynos/edcs_status.c | 110 +++++++++++++++++++
> 4 files changed, 370 insertions(+)
> create mode 100644 arch/arm/mach-exynos/edcs.c
> create mode 100644 arch/arm/mach-exynos/edcs.h
> create mode 100644 arch/arm/mach-exynos/edcs_status.c
>
> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> index 5369615..18e6162 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_ARM_CCI) += edcs.o edcs_status.o
> diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
> new file mode 100644
> index 0000000..34b4e4b
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs.c
> @@ -0,0 +1,217 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * 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/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +#include <linux/vexpress.h>
Why do you need a vexpress include?
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include "edcs.h"
> +
> +static arch_spinlock_t exynos_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +static int kfs_use_count[MAX_CPUS_PER_CLUSTER][MAX_NR_CLUSTERS];
The "kfs_" prefix looks like a remnant of ancient code which stood for
"Kingfisher" before it was renamed to "DCSCB". :-)
You might consider using "edcs_" instead. Same thing for exynos_lock
above which is rather generic a name. Maybe edcs_lock would be better.
Also please define your own constant such as EDCS_CPUS_PER_CLUSTER
instead of using MAX_CPUS_PER_CLUSTER as the later is related to the
code limit and not the actual hardware topology, and it could change in
the future.
> +static int core_count[MAX_NR_CLUSTERS]; > +
> +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;
Here's an example of where your code would break if MAX_CPUS_PER_CLUSTER
changes.
> + 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;
> +}
> +static int exynos_cluster_power_control(unsigned int cluster, int enable)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(10);
> + int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
> +
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) & 0x3)
> + == value)
You could put the above if () all on the same line.
> + return 0;
> +
> + __raw_writel(value, EXYNOS5410_COMMON_CONFIGURATION(cluster));
> + do {
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) &
> + __raw_readl(EXYNOS5410_L2_STATUS(cluster)) & 0x3)
> + == value)
> + return 0;
> + } while (time_before(jiffies, timeout));
> +
> + return -EDEADLK;
> +}
> +
> +static int exynos_core_power_up(unsigned int cpu, unsigned int cluster)
> +{
> + return exynos_core_power_control(cpu, cluster, 1);
> +}
> +
> +static int exynos_core_power_down(unsigned int cpu, unsigned int cluster)
> +{
> + return exynos_core_power_control(cpu, cluster, 0);
> +}
> +
> +static int exynos_cluster_power_up(unsigned int cluster)
> +{
> + return exynos_cluster_power_control(cluster, 1);
> +}
> +
> +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> +{
> + int ret;
> + local_irq_disable();
> + arch_spin_lock(&exynos_lock);
> +
> + pr_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
You should consider tuning this down to pr_debug.
> + kfs_use_count[cpu][cluster]++;
> + if (kfs_use_count[cpu][cluster] == 1) {
> + ++core_count[cluster];
> + if (core_count[cluster] == 1) {
> + ret = exynos_cluster_power_up(cluster);
> + if (ret) {
> + pr_err("%s: cluster %u power up error\n",
> + __func__, cluster);
> + return ret;
> + }
> + __cci_control_port_by_index(MAX_NR_CLUSTERS
> + - cluster, true);
This is wrong and very racy. The state machine implemented in
mcpm-head.S is there already to handle proper synchronization for you.
You need to provide a tiny bit of code to mcpm_sync_init() in order to
set things up properly.
Please have a look at arch/arm/mach-vexpress/tc2_pm.c in latest mainline
kernel tree, in particular tc2_pm_power_up_setup().
> + }
> + ret = exynos_core_power_up(cpu, cluster);
> + if (ret) {
> + pr_err("%s: cpu %u cluster %u power up error\n",
> + __func__, cpu, cluster);
> + return ret;
> + }
> + } else if (kfs_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(&exynos_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_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
> + BUG_ON(cpu >= MAX_CPUS_PER_CLUSTER || cluster >= MAX_NR_CLUSTERS);
> + __mcpm_cpu_going_down(cpu, cluster);
> + arch_spin_lock(&exynos_lock);
> + BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> + kfs_use_count[cpu][cluster]--;
> +
> + if (kfs_use_count[cpu][cluster] == 0) {
> + exynos_core_power_down(cpu, cluster);
> + --core_count[cluster];
> + if (core_count[cluster] == 0)
> + last_man = true;
> +
> + } else if (kfs_use_count[cpu][cluster] == 1) {
> + skip_wfi = true;
> + } else
> + BUG();
> + if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> + arch_spin_unlock(&exynos_lock);
> + flush_cache_all();
> + set_cr(get_cr() & ~CR_C);
> + flush_cache_all();
> + outer_flush_all();
> + set_auxcr(get_auxcr() & ~(1 << 6));
> + cci_disable_port_by_cpu(mpidr);
> + __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
> + } else {
> + arch_spin_unlock(&exynos_lock);
> + flush_cache_louis();
> + set_cr(get_cr() & ~CR_C);
> + flush_cache_louis();
> + set_auxcr(get_auxcr() & ~(1 << 6));
This cache handling sequence is also buggy. Please have a look at
arch/arm/mach-vexpress/tc2_pm.c for an example of how it should be done.
This code sequence should also be abstracted into a common
implementation eventually, but in the mean time the TC2 implementation
is a good model to follow.
> + } + __mcpm_cpu_down(cpu, cluster); + + dsb(); + if (!skip_wfi) +
> wfi(); +} +static const struct mcpm_platform_ops exynos_power_ops = {
> + .power_up = exynos_power_up, + .power_down = exynos_power_down, +};
> + +static void __init edcs_data_init(void) +{ + unsigned int mpidr,
> cpu, cluster; + + mpidr = read_cpuid_mpidr(); + cpu =
> MPIDR_AFFINITY_LEVEL(mpidr, 0); + cluster =
> MPIDR_AFFINITY_LEVEL(mpidr, 1); + pr_debug("%s: cpu %u cluster %u\n",
> __func__, cpu, cluster); + BUG_ON(cpu >= 4 || cluster >= 2); +
> kfs_use_count[cpu][cluster] = 1; + ++core_count[cluster]; +
> __cci_control_port_by_index(MAX_NR_CLUSTERS - cluster, true); +} + +
> +static int __init edcs_init(void) +{ + struct device_node *node; + +
> if (!cci_probed()) + return -ENODEV; + + node =
> of_find_compatible_node(NULL, NULL, "samsung,edcs"); + if (!node) +
> return -ENODEV; + edcs_data_init(); + mcpm_smp_set_ops(); +
> mcpm_platform_register(&exynos_power_ops);
You are lacking a call to mcpm_sync_init().
> +
> + /*
> + * Future entries into the kernel can now go
> + * through the cluster entry vectors.
> + */
> +
> + __raw_writel(virt_to_phys(mcpm_entry_point),
> + S5P_VA_SYSRAM_NS + 0x1c);
> +
> + pr_info("EDCS: EXYNOS5410 DUAL CLUSTER SUPPORT installed\n");
> +
> + return 0;
> +}
> +
> +early_initcall(edcs_init);
> diff --git a/arch/arm/mach-exynos/edcs.h b/arch/arm/mach-exynos/edcs.h
> new file mode 100644
> index 0000000..dd3e204
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * 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/arm-cci.h>
> +#include <linux/of_address.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <mach/regs-pmu.h>
> +
> +#define EXYNOS5410_ARM_COMMON_CONFIGURATION S5P_PMUREG(0x2500)
> +#define EXYNOS5410_COMMON_CONFIGURATION(_nr) \
> + (EXYNOS5410_ARM_COMMON_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_COMMON_STATUS(_nr) \
> + (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_COMMON_OPTION(_nr) \
> + (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5410_ARM_L2_CONFIGURATION S5P_PMUREG(0x2600)
> +#define EXYNOS5410_L2_CONFIGURATION(_nr) \
> + (EXYNOS5410_ARM_L2_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_L2_STATUS(_nr) \
> + (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_L2_OPTION(_nr) \
> + (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5410_CORE_CONFIGURATION(_nr) \
> + (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_CORE_STATUS(_nr) \
> + (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_CORE_OPTION(_nr) \
> + (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5_PA_CCI 0x10D20000
> diff --git a/arch/arm/mach-exynos/edcs_status.c b/arch/arm/mach-exynos/edcs_status.c
> new file mode 100644
> index 0000000..bff1e7f
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs_status.c
> @@ -0,0 +1,110 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * 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/fs.h>
> +#include <linux/miscdevice.h>
> +#include <asm/uaccess.h>
> +
> +#include "edcs.h"
> +
> +int edcs_check_status(char *info)
[...]
What is the purpose of this driver? What information does it provide?
In any case, creating a misc device for this is most likely not the best
way to export that kind of information.
> +{
> + size_t it = 30;
> + int i;
> +
> + void __iomem *cci_base;
> +
> + cci_base = ioremap(EXYNOS5_PA_CCI, SZ_64K);
> + if (!cci_base)
> + return -ENOMEM;
> +
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(0)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> +
> + for (i = 0; i < 4; i++) {
> + if ((__raw_readl(EXYNOS5410_CORE_STATUS(i)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> + }
> +
> + if ((__raw_readl(EXYNOS5410_L2_STATUS(0)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> +
> + if ((readl(cci_base + 0x4000 + 1 * 0x1000) & 0x3) == 3)
> + info[it] = '1';
> +
> + it += 10;
> +
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(1)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> +
> + for (i = 4; i < 8; i++) {
> + if ((__raw_readl(EXYNOS5410_CORE_STATUS(i)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> + }
> +
> + if ((__raw_readl(EXYNOS5410_L2_STATUS(1)) & 0x3) == 3)
> + info[it] = '1';
> + it += 3;
> +
> + if ((readl(cci_base + 0x4000 + 0 * 0x1000) & 0x3) == 3)
> + info[it] = '1';
> +
> + iounmap(cci_base);
> + return 0;
> +}
> +
> +static ssize_t edcs_status_read(struct file *file, char __user *buf,
> + size_t len, loff_t *pos)
> +{
> +
> + char info[] = "\tC 0 1 2 3 L2 CCI\n"
> + "[A15] 0 0 0 0 0 0 0\n"
> + " [A7] 0 0 0 0 0 0 0\n";
> + size_t count = sizeof(info);
> + edcs_check_status(info);
> + return simple_read_from_buffer(buf, len, pos, info, count);
> +}
> +
> +static const struct file_operations edcs_status_fops = {
> + .read = edcs_status_read,
> + .owner = THIS_MODULE,
> +};
> +static struct miscdevice edcs_status_device = {
> + .name = "edcs_status",
> + .minor = 255,
> + .fops = &edcs_status_fops,
> +};
> +
> +static int __init edcs_dev_init(void)
> +{
> + struct device_node *node;
> + int ret;
> + if (!cci_probed())
> + return -ENODEV;
> +
> + node = of_find_compatible_node(NULL, NULL, "samsung,edcs");
> + if (!node)
> + return -ENODEV;
> +
> + ret = misc_register(&edcs_status_device);
> + if (ret) {
> + pr_err("EDCS: edcs_status device is not registered\n");
> + return ret;
> + }
> + pr_info("EDCS: edcs_status device registered\n");
> + return 0;
> +}
> +device_initcall(edcs_dev_init);
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
2013-10-01 16:17 ` [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support Vyacheslav Tyrtov
[not found] ` <1380644227-12244-4-git-send-email-v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2013-10-02 12:55 ` Dave Martin
[not found] ` <20131002125458.GA3407-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
1 sibling, 1 reply; 7+ messages in thread
From: Dave Martin @ 2013-10-02 12:55 UTC (permalink / raw)
To: Vyacheslav Tyrtov
Cc: Mark Rutland, devicetree, Kukjin Kim, Russell King,
Daniel Lezcano, Pawel Moll, Ian Campbell, Stephen Warren,
linux-doc, linux-kernel, Rob Herring, linux-samsung-soc,
Rob Landley, Ben Dooks, Tarek Dakhran, Thomas Gleixner,
Naour Romain, Mike Turquette, linux-arm-kernel, Heiko Stuebner
On Tue, Oct 01, 2013 at 08:17:04PM +0400, Vyacheslav Tyrtov wrote:
> From: Tarek Dakhran <t.dakhran@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.dakhran@samsung.com>
> Signed-off-by: Vyacheslav Tyrtov <v.tyrtov@samsung.com>
> ---
> arch/arm/mach-exynos/Makefile | 2 +
> arch/arm/mach-exynos/edcs.c | 217 +++++++++++++++++++++++++++++++++++++
> arch/arm/mach-exynos/edcs.h | 41 +++++++
> arch/arm/mach-exynos/edcs_status.c | 110 +++++++++++++++++++
> 4 files changed, 370 insertions(+)
> create mode 100644 arch/arm/mach-exynos/edcs.c
> create mode 100644 arch/arm/mach-exynos/edcs.h
> create mode 100644 arch/arm/mach-exynos/edcs_status.c
>
> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> index 5369615..18e6162 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_ARM_CCI) += edcs.o edcs_status.o
> diff --git a/arch/arm/mach-exynos/edcs.c b/arch/arm/mach-exynos/edcs.c
> new file mode 100644
> index 0000000..34b4e4b
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs.c
> @@ -0,0 +1,217 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran@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/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/errno.h>
> +#include <linux/vexpress.h>
> +#include <asm/mcpm.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cputype.h>
> +#include <asm/cp15.h>
> +
> +#include "edcs.h"
> +
> +static arch_spinlock_t exynos_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +static int kfs_use_count[MAX_CPUS_PER_CLUSTER][MAX_NR_CLUSTERS];
> +static int core_count[MAX_NR_CLUSTERS];
> +
> +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;
Are you sure you need to wait here? In MCPM, the power_up() and
power_down() methods are expected to trigger the requested operation,
but they aren't required to wait for it to complete.
Waiting may just burn energy and add pointless latency -- particularly
bad for power_up() with IKS, because the calling CPU could go off and do
something useful in the meantime.
For CPU hotplug and kexec, it is neecssary to wait for a CPU really to
be halted or powered down, so a new power_down_wait() method will need
to be implemented.
See this series (which includes an implementation example for TC2):
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/201618.html
If Russell accepts the patches, they will likely be in 3.13.
Your wait loop may be relevant to this, but for power_down_finish() the
loop should be more relaxed to avoid thrashing the bus and power
controller unnecessarily.
Without a power_down_wait() method, the kernel will build OK, but will
BUG() if hotplug or kexec is attempted.
> +}
> +static int exynos_cluster_power_control(unsigned int cluster, int enable)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(10);
> + int value = enable ? S5P_CORE_LOCAL_PWR_EN : 0;
> +
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) & 0x3)
> + == value)
> + return 0;
> +
> + __raw_writel(value, EXYNOS5410_COMMON_CONFIGURATION(cluster));
> + do {
> + if ((__raw_readl(EXYNOS5410_COMMON_STATUS(cluster)) &
> + __raw_readl(EXYNOS5410_L2_STATUS(cluster)) & 0x3)
> + == value)
> + return 0;
> + } while (time_before(jiffies, timeout));
> +
> + return -EDEADLK;
> +}
The same comment about waiting applies here, unless we must really wait
for this to finish before exynos_core_power_up() will work. This might
be the case if the EXYNOS5410_CORE_CONFIGURATION() registers are implemented
in the cluster-local power domains, but it doesn't look that way.
For example, can you set the target core's PWR_EN bit first and then set
the PWR_UP bit for the cluster, so that the target CPU is brought up as
soon as the cluster comes online? The correct way to do this, and whether
it's possible at all, depend on the hardware...
> +
> +static int exynos_core_power_up(unsigned int cpu, unsigned int cluster)
> +{
> + return exynos_core_power_control(cpu, cluster, 1);
> +}
> +
> +static int exynos_core_power_down(unsigned int cpu, unsigned int cluster)
> +{
> + return exynos_core_power_control(cpu, cluster, 0);
> +}
> +
> +static int exynos_cluster_power_up(unsigned int cluster)
> +{
> + return exynos_cluster_power_control(cluster, 1);
> +}
No exynos_cluster_power_down()?
> +
> +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> +{
> + int ret;
> + local_irq_disable();
Should there be a local_fiq_disable() here also?
We actually don't have this for TC2 either. Linux on TC2 doesn't make
use of FIQs, but it would be better to have it for completeness, unless
FIQ is already masked for some reason.
Nico might have a view on this -- if local_fiq_disable() is needed here,
we should add it in the TC2 code too.
> + arch_spin_lock(&exynos_lock);
> +
> + pr_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
Minor nits: in addition to Nico's comment, please add some context (like
"%s:", ... __func__).
The rest of the MCPM stack uses the "cpu, cluster" convention in most
places for its messages, so it's best to follow that convention for
consistency.
Some boards allow the cluster numbers to be swapped via DIP switches or
other config. I don't know whether this applies to EXYNOS5410, but
unnecessary assumptions should be avoided. Nothing else in this file
cares which cluster is A15 and which is A7, if I understand correctly.
> + kfs_use_count[cpu][cluster]++;
> + if (kfs_use_count[cpu][cluster] == 1) {
> + ++core_count[cluster];
> + if (core_count[cluster] == 1) {
> + ret = exynos_cluster_power_up(cluster);
> + if (ret) {
> + pr_err("%s: cluster %u power up error\n",
> + __func__, cluster);
> + return ret;
> + }
> + __cci_control_port_by_index(MAX_NR_CLUSTERS
> + - cluster, true);
> + }
> + ret = exynos_core_power_up(cpu, cluster);
> + if (ret) {
> + pr_err("%s: cpu %u cluster %u power up error\n",
> + __func__, cpu, cluster);
> + return ret;
All these "return ret" in the middle of the function will leave local
IRQs masked, and exynos_lock will remain locked, leading to future
deadlocks.
> + }
> + } else if (kfs_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();
> + }
If this function might return prematurely on error, then I suggest you
put a label here, and goto it on errors, with "return ret" in place of
"return 0". ret should be initialised to zero if you do that.
> + arch_spin_unlock(&exynos_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_info("A%d CORE%d\n", 15 - cluster * 8, cpu);
> + BUG_ON(cpu >= MAX_CPUS_PER_CLUSTER || cluster >= MAX_NR_CLUSTERS);
> + __mcpm_cpu_going_down(cpu, cluster);
> + arch_spin_lock(&exynos_lock);
> + BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
> + kfs_use_count[cpu][cluster]--;
> +
> + if (kfs_use_count[cpu][cluster] == 0) {
> + exynos_core_power_down(cpu, cluster);
> + --core_count[cluster];
> + if (core_count[cluster] == 0)
> + last_man = true;
> +
> + } else if (kfs_use_count[cpu][cluster] == 1) {
> + skip_wfi = true;
> + } else
> + BUG();
> + if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
> + arch_spin_unlock(&exynos_lock);
> + flush_cache_all();
> + set_cr(get_cr() & ~CR_C);
> + flush_cache_all();
> + outer_flush_all();
> + set_auxcr(get_auxcr() & ~(1 << 6));
> + cci_disable_port_by_cpu(mpidr);
> + __mcpm_outbound_leave_critical(cluster, CLUSTER_DOWN);
How does the cluster get powered down? Does the power controller always
do that automatically when there are no powered-up CPUs left in a
cluster?
> + } else {
> + arch_spin_unlock(&exynos_lock);
> + flush_cache_louis();
> + set_cr(get_cr() & ~CR_C);
> + flush_cache_louis();
> + set_auxcr(get_auxcr() & ~(1 << 6));
> + }
> + __mcpm_cpu_down(cpu, cluster);
> +
> + dsb();
You don't need this dsb, because __mcpm_cpu_down() contains one already.
This is not just coincidence, because __mcpm_cpu_down() is a signalling
operation, which kicks CPUs waiting in __mcpm_outbound_enter_critical()
or in mcpm_head.S.
> + if (!skip_wfi)
> + wfi();
> +}
> +static const struct mcpm_platform_ops exynos_power_ops = {
> + .power_up = exynos_power_up,
> + .power_down = exynos_power_down,
What about .suspend ?
> +};
> +
> +static void __init edcs_data_init(void)
> +{
> + unsigned int mpidr, cpu, cluster;
> +
> + mpidr = read_cpuid_mpidr();
> + cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> + cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> + pr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
> + BUG_ON(cpu >= 4 || cluster >= 2);
> + kfs_use_count[cpu][cluster] = 1;
> + ++core_count[cluster];
> + __cci_control_port_by_index(MAX_NR_CLUSTERS - cluster, true);
> +}
> +
> +
> +static int __init edcs_init(void)
> +{
> + struct device_node *node;
> +
> + if (!cci_probed())
> + return -ENODEV;
> +
> + node = of_find_compatible_node(NULL, NULL, "samsung,edcs");
> + if (!node)
> + return -ENODEV;
> + edcs_data_init();
> + mcpm_smp_set_ops();
> + mcpm_platform_register(&exynos_power_ops);
> +
> + /*
> + * Future entries into the kernel can now go
> + * through the cluster entry vectors.
> + */
> +
> + __raw_writel(virt_to_phys(mcpm_entry_point),
> + S5P_VA_SYSRAM_NS + 0x1c);
> +
> + pr_info("EDCS: EXYNOS5410 DUAL CLUSTER SUPPORT installed\n");
> +
> + return 0;
> +}
> +
> +early_initcall(edcs_init);
> diff --git a/arch/arm/mach-exynos/edcs.h b/arch/arm/mach-exynos/edcs.h
> new file mode 100644
> index 0000000..dd3e204
> --- /dev/null
> +++ b/arch/arm/mach-exynos/edcs.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tarek Dakhran <t.dakhran@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/arm-cci.h>
> +#include <linux/of_address.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <mach/regs-pmu.h>
> +
> +#define EXYNOS5410_ARM_COMMON_CONFIGURATION S5P_PMUREG(0x2500)
> +#define EXYNOS5410_COMMON_CONFIGURATION(_nr) \
> + (EXYNOS5410_ARM_COMMON_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_COMMON_STATUS(_nr) \
> + (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_COMMON_OPTION(_nr) \
> + (EXYNOS5410_COMMON_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5410_ARM_L2_CONFIGURATION S5P_PMUREG(0x2600)
> +#define EXYNOS5410_L2_CONFIGURATION(_nr) \
> + (EXYNOS5410_ARM_L2_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_L2_STATUS(_nr) \
> + (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_L2_OPTION(_nr) \
> + (EXYNOS5410_L2_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5410_CORE_CONFIGURATION(_nr) \
> + (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
> +#define EXYNOS5410_CORE_STATUS(_nr) \
> + (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x4)
> +#define EXYNOS5410_CORE_OPTION(_nr) \
> + (EXYNOS5410_CORE_CONFIGURATION(_nr) + 0x8)
> +
> +#define EXYNOS5_PA_CCI 0x10D20000
This physical address should be discovered from the DT by the arm-cci
driver. If the arm-cci driver and arm,cci-400 binding are insufficient
for your needs, that needs discussion.
If querying the CCI status is necessary, interfaces should be added to
the arm-cci driver to allow that instead of poking it directly.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
2013-10-01 19:55 ` Nicolas Pitre
@ 2013-10-02 13:05 ` Dave Martin
0 siblings, 0 replies; 7+ messages in thread
From: Dave Martin @ 2013-10-02 13:05 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Mark Rutland, devicetree, Kukjin Kim, Russell King,
Daniel Lezcano, Pawel Moll, Ian Campbell, Stephen Warren,
linux-doc, linux-kernel, Rob Herring, linux-samsung-soc,
Vyacheslav Tyrtov, Ben Dooks, Tarek Dakhran, Thomas Gleixner,
Rob Landley, Heiko Stuebner, Mike Turquette, linux-arm-kernel,
Naour Romain
On Tue, Oct 01, 2013 at 03:55:24PM -0400, Nicolas Pitre wrote:
> On Tue, 1 Oct 2013, Vyacheslav Tyrtov wrote:
>
> > From: Tarek Dakhran <t.dakhran@samsung.com>
[...]
> > + kfs_use_count[cpu][cluster]++;
> > + if (kfs_use_count[cpu][cluster] == 1) {
> > + ++core_count[cluster];
> > + if (core_count[cluster] == 1) {
> > + ret = exynos_cluster_power_up(cluster);
> > + if (ret) {
> > + pr_err("%s: cluster %u power up error\n",
> > + __func__, cluster);
> > + return ret;
> > + }
> > + __cci_control_port_by_index(MAX_NR_CLUSTERS
> > + - cluster, true);
>
> This is wrong and very racy. The state machine implemented in
> mcpm-head.S is there already to handle proper synchronization for you.
Maybe this issue didn't make itself obvious yet due to the lack of
suspend support.
Moving the CCI maintenance to power_up_setup() is essential for suspend/
resume to work, because then CPUs can power up randomly in response to
interrupts -- exynos_lock is not sufficient protection in that case.
The TC2 code should provide a good example of what to do.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
[not found] ` <20131002125458.GA3407-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2013-10-04 19:51 ` Nicolas Pitre
0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Pitre @ 2013-10-04 19:51 UTC (permalink / raw)
To: Dave Martin
Cc: Vyacheslav Tyrtov, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Kukjin Kim, Russell King,
Daniel Lezcano, Pawel Moll, Ian Campbell, Stephen Warren,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Rob Landley, Ben Dooks,
Tarek Dakhran, Thomas Gleixner, Naour Romain, Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Heiko Stuebner
On Wed, 2 Oct 2013, Dave Martin wrote:
> On Tue, Oct 01, 2013 at 08:17:04PM +0400, Vyacheslav Tyrtov wrote:
> > +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> > +{
> > + int ret;
> > + local_irq_disable();
>
> Should there be a local_fiq_disable() here also?
No. In fact this is paired with
> > + arch_spin_lock(&exynos_lock);
to create the equivalent of a arch_spin_lock_irq(). And the reason is:
/*
* We can't use regular spinlocks. In the switcher case, it is possible
* for an outbound CPU to call power_down() after its inbound counterpart
* is already live using the same logical CPU number which trips lockdep
* debugging.
*/
Otherwise we simply would have used spin_lock_irq().
No FIQs are supposed to ever race with this code.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
@ 2013-10-07 14:03 Dave Martin
2013-10-07 22:06 ` Nicolas Pitre
0 siblings, 1 reply; 7+ messages in thread
From: Dave Martin @ 2013-10-07 14:03 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Mark Rutland, devicetree, Kukjin Kim, Russell King, Ben Dooks,
Pawel Moll, Ian Campbell, Daniel Lezcano, linux-doc, linux-kernel,
Rob Herring, linux-samsung-soc, Vyacheslav Tyrtov, Rob Landley,
Stephen Warren, Tarek Dakhran, Thomas Gleixner, Naour Romain,
Mike Turquette, linux-arm-kernel, Heiko Stuebner
On Fri, Oct 04, 2013 at 03:51:31PM -0400, Nicolas Pitre wrote:
> On Wed, 2 Oct 2013, Dave Martin wrote:
>
> > On Tue, Oct 01, 2013 at 08:17:04PM +0400, Vyacheslav Tyrtov wrote:
> > > +static int exynos_power_up(unsigned int cpu, unsigned int cluster)
> > > +{
> > > + int ret;
> > > + local_irq_disable();
> >
> > Should there be a local_fiq_disable() here also?
>
> No. In fact this is paired with
>
> > > + arch_spin_lock(&exynos_lock);
>
> to create the equivalent of a arch_spin_lock_irq(). And the reason is:
>
> /*
> * We can't use regular spinlocks. In the switcher case, it is possible
> * for an outbound CPU to call power_down() after its inbound counterpart
> * is already live using the same logical CPU number which trips lockdep
> * debugging.
> */
>
> Otherwise we simply would have used spin_lock_irq().
Duh, of course. Looks like I suffered temporary brain failure there.
> No FIQs are supposed to ever race with this code.
There is an anomaly though: FIQ and external abort don't seem to get
explicitly masked anywhere, either on the suspend or powerdown paths.
Sometimes either or both remains unmasked (I tried some trace in the
TC2 MCPM backend to confirm this.)
Looks like a possible omission in the arch/arm/ suspend and shutdown
code, rather than a problem specific to MCPM.
Shouldn't be an issue for this series, though.
Cheers
---Dave
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support
2013-10-07 14:03 [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support Dave Martin
@ 2013-10-07 22:06 ` Nicolas Pitre
0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Pitre @ 2013-10-07 22:06 UTC (permalink / raw)
To: Dave Martin
Cc: Mark Rutland, devicetree, Kukjin Kim, Russell King, Ben Dooks,
Pawel Moll, Ian Campbell, Daniel Lezcano, linux-doc, linux-kernel,
Rob Herring, linux-samsung-soc, Vyacheslav Tyrtov, Rob Landley,
Stephen Warren, Tarek Dakhran, Thomas Gleixner, Naour Romain,
Mike Turquette, linux-arm-kernel, Heiko Stuebner
On Mon, 7 Oct 2013, Dave Martin wrote:
> On Fri, Oct 04, 2013 at 03:51:31PM -0400, Nicolas Pitre wrote:
> > No FIQs are supposed to ever race with this code.
>
> There is an anomaly though: FIQ and external abort don't seem to get
> explicitly masked anywhere, either on the suspend or powerdown paths.
> Sometimes either or both remains unmasked (I tried some trace in the
> TC2 MCPM backend to confirm this.)
>
> Looks like a possible omission in the arch/arm/ suspend and shutdown
> code, rather than a problem specific to MCPM.
Possibly, yes.
Feel free to post a patch.
Nicolas
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-07 22:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-07 14:03 [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support Dave Martin
2013-10-07 22:06 ` Nicolas Pitre
-- strict thread matches above, loose matches on Subject: below --
2013-10-01 16:17 [PATCH 0/6] Exynos 5410 Dual cluster support Vyacheslav Tyrtov
2013-10-01 16:17 ` [PATCH 3/6] ARM: EXYNOS: add Exynos Dual Cluster Support Vyacheslav Tyrtov
[not found] ` <1380644227-12244-4-git-send-email-v.tyrtov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-01 19:55 ` Nicolas Pitre
2013-10-02 13:05 ` Dave Martin
2013-10-02 12:55 ` Dave Martin
[not found] ` <20131002125458.GA3407-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2013-10-04 19:51 ` Nicolas Pitre
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).