On pon, 2015-01-26 at 12:33 +0100, Bartlomiej Zolnierkiewicz wrote:
Hi,
On Monday, January 26, 2015 09:16:48 AM Krzysztof Kozlowski wrote:
On pią, 2015-01-23 at 17:24 +0100, Bartlomiej Zolnierkiewicz wrote:
The following patch adds coupled cpuidle support for Exynos4210 to an existing cpuidle-exynos driver. As a result it enables AFTR mode to be used by default on Exynos4210 without the need to hot unplug CPU1 first.
The patch is heavily based on earlier cpuidle-exynos4210 driver from Daniel Lezcano:
http://www.spinics.net/lists/linux-samsung-soc/msg28134.html
Changes from Daniel's code include:
- porting code to current kernels
- fixing it to work on my setup (by using S5P_INFORM register instead of S5P_VA_SYSRAM one on Revison 1.1 and retrying poking CPU1 out of the BOOT ROM if necessary)
- fixing rare lockup caused by waiting for CPU1 to get stuck in the BOOT ROM (CPU hotplug code in arch/arm/mach-exynos/platsmp.c doesn't require this and works fine)
- moving Exynos specific code to arch/arm/mach-exynos/pm.c
- using cpu_boot_reg_base() helper instead of BOOT_VECTOR macro
- using exynos_cpu_*() helpers instead of accessing registers directly
- using arch_send_wakeup_ipi_mask() instead of dsb_sev() (this matches CPU hotplug code in arch/arm/mach-exynos/platsmp.c)
- integrating separate exynos4210-cpuidle driver into existing exynos-cpuidle one
Cc: Colin Cross ccross@google.com Cc: Kukjin Kim kgene.kim@samsung.com Cc: Krzysztof Kozlowski k.kozlowski@samsung.com Cc: Tomasz Figa tomasz.figa@gmail.com Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Kyungmin Park kyungmin.park@samsung.com
I've seen the patch during internal review and now looks good... except minor issues below.
arch/arm/mach-exynos/common.h | 4 + arch/arm/mach-exynos/exynos.c | 4 + arch/arm/mach-exynos/platsmp.c | 2 +- arch/arm/mach-exynos/pm.c | 122 +++++++++++++++++++++++++++ drivers/cpuidle/Kconfig.arm | 1 + drivers/cpuidle/cpuidle-exynos.c | 76 +++++++++++++++-- include/linux/platform_data/cpuidle-exynos.h | 20 +++++ 7 files changed, 223 insertions(+), 6 deletions(-) create mode 100644 include/linux/platform_data/cpuidle-exynos.h
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h index 865f878..f70eca7 100644 --- a/arch/arm/mach-exynos/common.h +++ b/arch/arm/mach-exynos/common.h @@ -13,6 +13,7 @@ #define __ARCH_ARM_MACH_EXYNOS_COMMON_H #include <linux/of.h> +#include <linux/platform_data/cpuidle-exynos.h> #define EXYNOS3250_SOC_ID 0xE3472000 #define EXYNOS3_SOC_MASK 0xFFFFF000 @@ -150,8 +151,11 @@ extern void exynos_pm_central_suspend(void); extern int exynos_pm_central_resume(void); extern void exynos_enter_aftr(void); +extern struct cpuidle_exynos_data cpuidle_coupled_exynos_data;
extern void s5p_init_cpu(void __iomem *cpuid_addr); extern unsigned int samsung_rev(void); +extern void __iomem *cpu_boot_reg_base(void); static inline void pmu_raw_writel(u32 val, u32 offset) { diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c index 78eca99b..2013f73 100644 --- a/arch/arm/mach-exynos/exynos.c +++ b/arch/arm/mach-exynos/exynos.c @@ -211,6 +211,10 @@ static void __init exynos_dt_machine_init(void) if (!IS_ENABLED(CONFIG_SMP)) exynos_sysram_init(); +#ifdef CONFIG_ARM_EXYNOS_CPUIDLE
- if (of_machine_is_compatible("samsung,exynos4210"))
exynos_cpuidle.dev.platform_data = &cpuidle_coupled_exynos_data;+#endif if (of_machine_is_compatible("samsung,exynos4210") || of_machine_is_compatible("samsung,exynos4212") || (of_machine_is_compatible("samsung,exynos4412") && diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index 7a1ebfe..3f32c47 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -194,7 +194,7 @@ int exynos_cluster_power_state(int cluster) S5P_CORE_LOCAL_PWR_EN); } -static inline void __iomem *cpu_boot_reg_base(void) +void __iomem *cpu_boot_reg_base(void) { if (soc_is_exynos4210() && samsung_rev() == EXYNOS4210_REV_1_1) return pmu_base_addr + S5P_INFORM5; diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c index 1a7454d..e6209da 100644 --- a/arch/arm/mach-exynos/pm.c +++ b/arch/arm/mach-exynos/pm.c @@ -180,3 +180,125 @@ void exynos_enter_aftr(void) cpu_pm_exit(); }
+static atomic_t cpu1_wakeup = ATOMIC_INIT(0);
+static int exynos_cpu0_enter_aftr(void) +{
- int ret = -1;
- /*
* If the other cpu is powered on, we have to power it off, because* the AFTR state won't work otherwise*/- if (cpu_online(1)) {
/** We reach a sync point with the coupled idle state, we know* the other cpu will power down itself or will abort the* sequence, let's wait for one of these to happen*/while (exynos_cpu_power_state(1)) {/** The other cpu may skip idle and boot back* up again*/if (atomic_read(&cpu1_wakeup))goto abort;/** The other cpu may bounce through idle and* boot back up again, getting stuck in the* boot rom code*/if (__raw_readl(cpu_boot_reg_base()) == 0)goto abort;cpu_relax();}- }
- exynos_enter_aftr();
- ret = 0;
+abort:
- if (cpu_online(1)) {
/** Set the boot vector to something non-zero*/__raw_writel(virt_to_phys(exynos_cpu_resume),cpu_boot_reg_base());dsb();/** Turn on cpu1 and wait for it to be on*/exynos_cpu_power_up(1);while (exynos_cpu_power_state(1) != S5P_CORE_LOCAL_PWR_EN)cpu_relax();while (!atomic_read(&cpu1_wakeup)) {/** Poke cpu1 out of the boot rom*/__raw_writel(virt_to_phys(exynos_cpu_resume),cpu_boot_reg_base());arch_send_wakeup_ipi_mask(cpumask_of(1));}- }
- return ret;
+}
+static int exynos_wfi_finisher(unsigned long flags) +{
- cpu_do_idle();
- return -1;
+}
+static int exynos_cpu1_powerdown(void) +{
- int ret = -1;
- /*
* Idle sequence for cpu1*/- if (cpu_pm_enter())
goto cpu1_aborted;- /*
* Turn off cpu 1*/- exynos_cpu_power_down(1);
- ret = cpu_suspend(0, exynos_wfi_finisher);
- cpu_pm_exit();
+cpu1_aborted:
- dsb();
- /*
* Notify cpu 0 that cpu 1 is awake*/- atomic_set(&cpu1_wakeup, 1);
- return ret;
+}
+static void exynos_pre_enter_aftr(void) +{
- __raw_writel(virt_to_phys(exynos_cpu_resume), cpu_boot_reg_base());
+}
+static void exynos_post_enter_aftr(void) +{
- atomic_set(&cpu1_wakeup, 0);
+}
+struct cpuidle_exynos_data cpuidle_coupled_exynos_data = {
- .cpu0_enter_aftr = exynos_cpu0_enter_aftr,
- .cpu1_powerdown = exynos_cpu1_powerdown,
- .pre_enter_aftr = exynos_pre_enter_aftr,
- .post_enter_aftr = exynos_post_enter_aftr,
+}; diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm index 8c16ab2..8e07c94 100644 --- a/drivers/cpuidle/Kconfig.arm +++ b/drivers/cpuidle/Kconfig.arm @@ -55,6 +55,7 @@ config ARM_AT91_CPUIDLE config ARM_EXYNOS_CPUIDLE bool "Cpu Idle Driver for the Exynos processors" depends on ARCH_EXYNOS
- select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP help Select this to enable cpuidle for Exynos processors
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c index 4003a31..26f5f29 100644 --- a/drivers/cpuidle/cpuidle-exynos.c +++ b/drivers/cpuidle/cpuidle-exynos.c @@ -1,8 +1,11 @@ -/* linux/arch/arm/mach-exynos/cpuidle.c
- Copyright (c) 2011 Samsung Electronics Co., Ltd.
+/*
- Copyright (c) 2011-2014 Samsung Electronics Co., Ltd.
http://www.samsung.com
- Coupled cpuidle support based on the work of:
- Colin Cross ccross@android.com
- Daniel Lezcano daniel.lezcano@linaro.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.
@@ -13,13 +16,49 @@ #include <linux/export.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/platform_data/cpuidle-exynos.h> #include <asm/proc-fns.h> #include <asm/suspend.h> #include <asm/cpuidle.h> +static atomic_t exynos_idle_barrier;
+static struct cpuidle_exynos_data *exynos_cpuidle_pdata; static void (*exynos_enter_aftr)(void); +static int exynos_enter_coupled_lowpower(struct cpuidle_device *dev,
struct cpuidle_driver *drv,int index)+{
- int ret;
- exynos_cpuidle_pdata->pre_enter_aftr();
- /*
* Waiting all cpus to reach this point at the same moment*/- cpuidle_coupled_parallel_barrier(dev, &exynos_idle_barrier);
- /*
* Both cpus will reach this point at the same time*/- ret = dev->cpu ? exynos_cpuidle_pdata->cpu1_powerdown()
: exynos_cpuidle_pdata->cpu0_enter_aftr();- if (ret)
index = ret;- /*
* Waiting all cpus to finish the power sequence before going further*/- cpuidle_coupled_parallel_barrier(dev, &exynos_idle_barrier);
- exynos_cpuidle_pdata->post_enter_aftr();
- return index;
+}
static int exynos_enter_lowpower(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) @@ -55,13 +94,40 @@ static struct cpuidle_driver exynos_idle_driver = { .safe_state_index = 0, }; +static struct cpuidle_driver exynos_coupled_idle_driver = {
- .name = "exynos_coupled_idle",
- .owner = THIS_MODULE,
- .states = {
[0] = ARM_CPUIDLE_WFI_STATE,[1] = {.enter = exynos_enter_coupled_lowpower,.exit_latency = 5000,.target_residency = 10000,Have you measured these numbers? Existing cpuidle has residency of 100000.
These numbers are from the original driver by Daniel. If needed they can be changed later.
.flags = CPUIDLE_FLAG_COUPLED |CPUIDLE_FLAG_TIMER_STOP,.name = "C1",.desc = "ARM power down",},- },
- .state_count = 2,
- .safe_state_index = 0,
+};
static int exynos_cpuidle_probe(struct platform_device *pdev) { int ret;
- exynos_enter_aftr = (void *)(pdev->dev.platform_data);
- if (of_machine_is_compatible("samsung,exynos4210")) {
exynos_cpuidle_pdata = pdev->dev.platform_data;ret = cpuidle_register(&exynos_coupled_idle_driver,cpu_possible_mask);- } else {
exynos_enter_aftr = (void *)(pdev->dev.platform_data);ret = cpuidle_register(&exynos_idle_driver, NULL);- }
- ret = cpuidle_register(&exynos_idle_driver, NULL); if (ret) { dev_err(&pdev->dev, "failed to register cpuidle driver\n"); return ret;
diff --git a/include/linux/platform_data/cpuidle-exynos.h b/include/linux/platform_data/cpuidle-exynos.h new file mode 100644 index 0000000..bfa40e4 --- /dev/null +++ b/include/linux/platform_data/cpuidle-exynos.h @@ -0,0 +1,20 @@ +/*
- Copyright (c) 2014 Samsung Electronics Co., Ltd.
http://www.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.
+*/
+#ifndef __CPUIDLE_EXYNOS_H +#define __CPUIDLE_EXYNOS_H
+struct cpuidle_exynos_data {
- int (*cpu0_enter_aftr)(void);
- int (*cpu1_powerdown)(void);
- void (*pre_enter_aftr)(void);
- void (*post_enter_aftr)(void);
I don't like the names of pre/post_enter_aftr. They are called also on CPU1 for powerdown. Probably they will be also called for CPU0 to enter
powerdown on CPU1 is a part of preparations for system to go into AFTR state.
LPA/LPD/W-AFTR.
Maybe "pre/post_enter_lowpower"?
I agree but it can be changed later when LPA/LPD/W-AFTR support is added.
Sure.
Additionally could you add short comment for them (like when they are called and on which CPU). It is not exynos internal header so one may not be sure that the raw code is actual documentation for this.
It is an Exynos internal header, no generic code uses it. It is a good idea to enhance documentation but lets leave the code as it is for v3.20 and update it in the future changes.
OK, I'm fine with that. I saw that Kukjin applied these patches so actually I don't oppose them. As I said it was rather nit-picking from my side.
Best regards, Krzysztof
PS I appreciate the review but it would greatly help to do review earlier. These patches are almost unchanged from the v2 version which was posted two months ago (+ the code you have commented on was already present in v1 which posted on 7th November).
Best regards,
Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics