* [PATCH 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
@ 2014-11-07 18:00 Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
0 siblings, 2 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-11-07 18:00 UTC (permalink / raw)
To: Kukjin Kim
Cc: Daniel Lezcano, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, b.zolnierkie
Hi,
The following patchset 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.
This work 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
patch #1 is a fix for Exynos platform PM code preparing it for
coupled cpuidle support
patch #2 adds coupled cpuidle AFTR mode for Exynos4210
The patchset depends on:
- 'next-20141031' branch of linux-next kernel tree (it also applies
fine to for-next branch of linux-samsung.git tree from today)
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Bartlomiej Zolnierkiewicz (2):
ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary
cpuidle: exynos: add coupled cpuidle support for Exynos4210
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 | 133 ++++++++++++++++++++++++++-
arch/arm/mach-exynos/suspend.c | 4 +
drivers/cpuidle/Kconfig.arm | 1 +
drivers/cpuidle/cpuidle-exynos.c | 62 ++++++++++++-
include/linux/platform_data/cpuidle-exynos.h | 20 ++++
8 files changed, 220 insertions(+), 10 deletions(-)
create mode 100644 include/linux/platform_data/cpuidle-exynos.h
--
1.8.2.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary
2014-11-07 18:00 [PATCH 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
@ 2014-11-07 18:00 ` Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
1 sibling, 0 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-11-07 18:00 UTC (permalink / raw)
To: Kukjin Kim
Cc: Daniel Lezcano, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, b.zolnierkie
Commit c2dd114d2486 ("ARM: EXYNOS: fix register setup for AFTR mode
code") added S5P_CENTRAL_SEQ_OPTION register setup fix for all
Exynos SoCs to AFTR mode code-path. It turned out that for coupled
cpuidle AFTR mode on Exynos4210 (added by the next patch) applying
this fix causes lockup so enable it in the AFTR mode code-path only
on SoCs that require it (in the suspend code-path it can be always
applied like it was before commit c2dd114d2486).
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
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>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/mach-exynos/pm.c | 11 +++++++----
arch/arm/mach-exynos/suspend.c | 4 ++++
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 4f10fa6..4b36ab5 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -96,10 +96,6 @@ void exynos_pm_central_suspend(void)
tmp = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-
- /* Setting SEQ_OPTION register */
- pmu_raw_writel(S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0,
- S5P_CENTRAL_SEQ_OPTION);
}
int exynos_pm_central_resume(void)
@@ -163,6 +159,13 @@ void exynos_enter_aftr(void)
exynos_pm_central_suspend();
+ if (of_machine_is_compatible("samsung,exynos4212") ||
+ of_machine_is_compatible("samsung,exynos4412")) {
+ /* Setting SEQ_OPTION register */
+ pmu_raw_writel(S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0,
+ S5P_CENTRAL_SEQ_OPTION);
+ }
+
cpu_suspend(0, exynos_aftr_finisher);
if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index f5d9773..ec6b04d 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -178,6 +178,10 @@ static int exynos_pm_suspend(void)
{
exynos_pm_central_suspend();
+ /* Setting SEQ_OPTION register */
+ pmu_raw_writel(S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0,
+ S5P_CENTRAL_SEQ_OPTION);
+
if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9)
exynos_cpu_save_register();
--
1.8.2.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
2014-11-07 18:00 [PATCH 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary Bartlomiej Zolnierkiewicz
@ 2014-11-07 18:00 ` Bartlomiej Zolnierkiewicz
2014-11-09 15:57 ` Daniel Lezcano
1 sibling, 1 reply; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-11-07 18:00 UTC (permalink / raw)
To: Kukjin Kim
Cc: Daniel Lezcano, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, b.zolnierkie
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: Daniel Lezcano <daniel.lezcano@linaro.org>
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>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
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 | 62 ++++++++++++--
include/linux/platform_data/cpuidle-exynos.h | 20 +++++
7 files changed, 209 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 d4d09bc..f208a60 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -14,6 +14,7 @@
#include <linux/reboot.h>
#include <linux/of.h>
+#include <linux/platform_data/cpuidle-exynos.h>
#define EXYNOS3250_SOC_ID 0xE3472000
#define EXYNOS3_SOC_MASK 0xFFFFF000
@@ -168,8 +169,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 a487e59..4f4eb9e 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -317,6 +317,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 adb36a8..0e3ffc9 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -182,7 +182,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 4b36ab5..44cc08a 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -178,3 +178,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 ba9b34b..4700d5d 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)
@@ -58,11 +97,24 @@ static struct cpuidle_driver exynos_idle_driver = {
static int exynos_cpuidle_probe(struct platform_device *pdev)
{
+ const struct cpumask *coupled_cpus = NULL;
int ret;
- exynos_enter_aftr = (void *)(pdev->dev.platform_data);
+ if (of_machine_is_compatible("samsung,exynos4210")) {
+ exynos_cpuidle_pdata = pdev->dev.platform_data;
+
+ exynos_idle_driver.states[1].enter =
+ exynos_enter_coupled_lowpower;
+ exynos_idle_driver.states[1].exit_latency = 5000;
+ exynos_idle_driver.states[1].target_residency = 10000;
+ exynos_idle_driver.states[1].flags |= CPUIDLE_FLAG_COUPLED |
+ CPUIDLE_FLAG_TIMER_STOP;
+
+ coupled_cpus = 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, coupled_cpus);
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);
+};
+
+#endif
--
1.8.2.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
2014-11-07 18:00 ` [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
@ 2014-11-09 15:57 ` Daniel Lezcano
2014-11-12 15:13 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2014-11-09 15:57 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Kukjin Kim
Cc: Tomasz Figa, Colin Cross, Krzysztof Kozlowski, Kyungmin Park,
linux-samsung-soc, linux-pm, linux-kernel, linaro-kernel
On 11/07/2014 07:00 PM, 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)
I am curious. You experienced very rare hangs after running the tests a
few hours, right ? Is the SEV replaced by the IPI solving the issue ? If
yes, how did you catch it ?
>- integrating separate exynos4210-cpuidle driver into existing
> exynos-cpuidle one
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> 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>
A few comments below:
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> 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 | 62 ++++++++++++--
> include/linux/platform_data/cpuidle-exynos.h | 20 +++++
> 7 files changed, 209 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 d4d09bc..f208a60 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -14,6 +14,7 @@
>
> #include <linux/reboot.h>
> #include <linux/of.h>
> +#include <linux/platform_data/cpuidle-exynos.h>
>
> #define EXYNOS3250_SOC_ID 0xE3472000
> #define EXYNOS3_SOC_MASK 0xFFFFF000
> @@ -168,8 +169,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 a487e59..4f4eb9e 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -317,6 +317,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
You should not add those #ifdef.
> 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 adb36a8..0e3ffc9 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -182,7 +182,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 4b36ab5..44cc08a 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -178,3 +178,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 ba9b34b..4700d5d 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)
> @@ -58,11 +97,24 @@ static struct cpuidle_driver exynos_idle_driver = {
>
> static int exynos_cpuidle_probe(struct platform_device *pdev)
> {
> + const struct cpumask *coupled_cpus = NULL;
> int ret;
>
> - exynos_enter_aftr = (void *)(pdev->dev.platform_data);
> + if (of_machine_is_compatible("samsung,exynos4210")) {
> + exynos_cpuidle_pdata = pdev->dev.platform_data;
> +
> + exynos_idle_driver.states[1].enter =
> + exynos_enter_coupled_lowpower;
> + exynos_idle_driver.states[1].exit_latency = 5000;
> + exynos_idle_driver.states[1].target_residency = 10000;
> + exynos_idle_driver.states[1].flags |= CPUIDLE_FLAG_COUPLED |
> + CPUIDLE_FLAG_TIMER_STOP;
I tried to remove those dynamic state allocation everywhere in the
different drivers. I would prefer to have another cpuidle_driver to be
registered with its states instead of overwriting the existing idle state.
struct cpuidle_driver exynos4210_idle_driver = {
.name = "exynos4210_idle",
.owner = THIS_MODULE,
.states = {
[0] = ARM_CPUIDLE_WFI_STATE,
[1] = {
.enter = exynos_enter_coupled_lowpower,
.exit_latency = 5000,
.target_residency = 10000,
.flags = CPUIDLE_FLAG_TIME_VALID |
CPUIDLE_FLAG_COUPLED |
CPUIDLE_FLAG_TIMER_STOP,
.name = "C1",
.desc = "ARM power down",
},
}
};
and then:
if (of_machine_is_compatible("samsung,exynos4210")) {
...
ret = cpuidle_register(&exynos4210_idle_driver,
cpu_online_mask);
...
}
...
If we can reuse this mechanism, which I believe it is possible to, for
4420 and 5250. Then we will be able to refactor this out again.
> +
> + coupled_cpus = 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, coupled_cpus);
> 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);
> +};
> +
> +#endif
>
--
<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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
2014-11-09 15:57 ` Daniel Lezcano
@ 2014-11-12 15:13 ` Bartlomiej Zolnierkiewicz
2014-11-12 16:43 ` Daniel Lezcano
0 siblings, 1 reply; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-11-12 15:13 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Kukjin Kim, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, Lukasz Majewski
[-- Attachment #1: Type: text/plain, Size: 15526 bytes --]
Hi,
On Sunday, November 09, 2014 04:57:51 PM Daniel Lezcano wrote:
> On 11/07/2014 07:00 PM, 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)
>
> I am curious. You experienced very rare hangs after running the tests a
> few hours, right ? Is the SEV replaced by the IPI solving the issue ? If
> yes, how did you catch it ?
Rare hangs showed up after about 30-40 minutes of testing with the attached
app and script (running of "./cpuidle_state1_test.sh script 2 500" has never
completed on the umodified driver).
The problem turned out to be in the following loop waiting for CPU1 to get
stuck in the BOOT ROM:
/*
* Wait for cpu1 to get stuck in the boot rom
*/
while ((__raw_readl(BOOT_VECTOR) != 0) &&
!atomic_read(&cpu1_wakeup))
cpu_relax();
[ Removal of the loop fixed the problem. ]
Using the SEV instead of the IPI was not a issue but it was changed to
match the existing Exynos platform code (exynos_boot_secondary() in
arch/arm/mach-exynos/platsmp.c) and as preparation for Exynos4412 (quad
core) support.
> >- integrating separate exynos4210-cpuidle driver into existing
> > exynos-cpuidle one
> >
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 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>
Thanks!
> A few comments below:
>
> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 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 | 62 ++++++++++++--
> > include/linux/platform_data/cpuidle-exynos.h | 20 +++++
> > 7 files changed, 209 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 d4d09bc..f208a60 100644
> > --- a/arch/arm/mach-exynos/common.h
> > +++ b/arch/arm/mach-exynos/common.h
> > @@ -14,6 +14,7 @@
> >
> > #include <linux/reboot.h>
> > #include <linux/of.h>
> > +#include <linux/platform_data/cpuidle-exynos.h>
> >
> > #define EXYNOS3250_SOC_ID 0xE3472000
> > #define EXYNOS3_SOC_MASK 0xFFFFF000
> > @@ -168,8 +169,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 a487e59..4f4eb9e 100644
> > --- a/arch/arm/mach-exynos/exynos.c
> > +++ b/arch/arm/mach-exynos/exynos.c
> > @@ -317,6 +317,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
>
> You should not add those #ifdef.
Without those #ifdef I get:
LD init/built-in.o
arch/arm/mach-exynos/built-in.o: In function `exynos_dt_machine_init':
/home/bzolnier/sam/linux-sprc/arch/arm/mach-exynos/exynos.c:334: undefined reference to `cpuidle_coupled_exynos_data'
make: *** [vmlinux] Error 1
when CONFIG_EXYNOS_CPU_SUSPEND is disabled.
> > 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 adb36a8..0e3ffc9 100644
> > --- a/arch/arm/mach-exynos/platsmp.c
> > +++ b/arch/arm/mach-exynos/platsmp.c
> > @@ -182,7 +182,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 4b36ab5..44cc08a 100644
> > --- a/arch/arm/mach-exynos/pm.c
> > +++ b/arch/arm/mach-exynos/pm.c
> > @@ -178,3 +178,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 ba9b34b..4700d5d 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)
> > @@ -58,11 +97,24 @@ static struct cpuidle_driver exynos_idle_driver = {
> >
> > static int exynos_cpuidle_probe(struct platform_device *pdev)
> > {
> > + const struct cpumask *coupled_cpus = NULL;
> > int ret;
> >
> > - exynos_enter_aftr = (void *)(pdev->dev.platform_data);
> > + if (of_machine_is_compatible("samsung,exynos4210")) {
> > + exynos_cpuidle_pdata = pdev->dev.platform_data;
> > +
> > + exynos_idle_driver.states[1].enter =
> > + exynos_enter_coupled_lowpower;
> > + exynos_idle_driver.states[1].exit_latency = 5000;
> > + exynos_idle_driver.states[1].target_residency = 10000;
> > + exynos_idle_driver.states[1].flags |= CPUIDLE_FLAG_COUPLED |
> > + CPUIDLE_FLAG_TIMER_STOP;
>
> I tried to remove those dynamic state allocation everywhere in the
> different drivers. I would prefer to have another cpuidle_driver to be
> registered with its states instead of overwriting the existing idle state.
>
> struct cpuidle_driver exynos4210_idle_driver = {
> .name = "exynos4210_idle",
> .owner = THIS_MODULE,
> .states = {
> [0] = ARM_CPUIDLE_WFI_STATE,
> [1] = {
> .enter = exynos_enter_coupled_lowpower,
> .exit_latency = 5000,
> .target_residency = 10000,
> .flags = CPUIDLE_FLAG_TIME_VALID |
> CPUIDLE_FLAG_COUPLED |
> CPUIDLE_FLAG_TIMER_STOP,
> .name = "C1",
> .desc = "ARM power down",
> },
> }
> };
>
>
> and then:
>
> if (of_machine_is_compatible("samsung,exynos4210")) {
> ...
> ret = cpuidle_register(&exynos4210_idle_driver,
> cpu_online_mask);
> ...
> }
> ...
OK, I will fix it but (if you are OK with it) I will make the code use
"exynos_coupled" naming instead of "exynos4210" one to not have to change
it later.
> If we can reuse this mechanism, which I believe it is possible to, for
> 4420 and 5250. Then we will be able to refactor this out again.
I plan to add support for Exynos3250 next as it should be the simplest
(it is also dual core) and I need it for other reasons anyway. Exynos4412
(quad core) support requires more work but should also be doable.
When it comes to Exynos5250 I was thinking about disabling normal AFTR
mode support for it as according to my testing (on Arndale board) it has
never worked (at least in upstream kernels, I don't know about Linaro or
internal ones).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> > +
> > + coupled_cpus = 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, coupled_cpus);
> > 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);
> > +};
> > +
> > +#endif
[-- Attachment #2: cpuidle_state1_test.sh --]
[-- Type: application/x-shellscript, Size: 1343 bytes --]
[-- Attachment #3: Makefile --]
[-- Type: text/x-makefile, Size: 440 bytes --]
CROSS_COMPILE ?= arm-linux-gnueabi-
CC = $(CROSS_COMPILE)gcc
OBJS = sched_task
# -static needed for cross building for old EGLIBC on newer GLIBC
# Example: Building for Rinato (Eglibc-2.13) on Glibc-2.19
CFLAGS = -static-libgcc -static
# -lrt for clock_gettime when Glibc < 2.17
LDFLAGS = -lrt
all: $(OBJS)
%.o : %.c
$(CC) $(CFLAGS) -c $< -o $@ $(LDFLAGS)
% : %.c
$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
clean:
rm -f *.o
rm -f $(OBJS)
[-- Attachment #4: sched_task.c --]
[-- Type: text/x-csrc, Size: 6449 bytes --]
/*
sched_task - scheduler test task -> for power evaluation
Copyright (C) 2013 Lukasz Majewski <l.majewski@samsung.com>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA
*/
/* This is based on the pi_stress.c rt-test program by Clark Williams */
#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <sched.h>
#include <sys/mman.h>
#include <string.h>
#include <getopt.h>
#define NSEC_PER_SEC 1000000000
#define USEC_TO_NSEC 1000
#define TIME_MIN 100000 /* 100 us minimal time */
#define TEST_TAB_SIZE 512
/*
* TODO: add support for chosing schedule policy (other, idle, rt)
*/
int sched_policy = SCHED_FIFO;
int priority = 10;
int task_period = 0;
int task_count = 0;
int work_time = 0;
int work_count = 0;
int work_offset = 0;
int lockall_flag = 0;
int debug_flag = 0;
#define DBG(fmt, args...) do { \
if (debug_flag) \
printf(fmt, args); \
} while (0);
/* command line options */
struct option options[] = {
{"tperiod", required_argument, NULL, 't'},
{"work", required_argument, NULL, 'w'},
{"count", required_argument, NULL, 'c'},
{"offset", required_argument, NULL, 'o'},
{"prio", no_argument, NULL, 'p'},
{"debug", no_argument, &debug_flag, 'd'},
{"mlockall", no_argument, &lockall_flag, 'm'},
{"help", no_argument, NULL, 'h'},
{"sched_other", no_argument, NULL, 's'},
{NULL, 0, NULL, 0},
};
void busy_test(void)
{
volatile static unsigned char test[TEST_TAB_SIZE];
int i, j;
for(i = 0xFF; i; i--)
for(j = 0; j < sizeof(test); j++)
test[j] = i;
}
int adjust_wakeup_time(struct timespec *t, int time)
{
t->tv_nsec += time;
while (t->tv_nsec >= NSEC_PER_SEC) {
t->tv_nsec -= NSEC_PER_SEC;
t->tv_sec++;
}
return 0;
}
int time_passed(struct timespec *x, struct timespec *y, struct timespec *result)
{
result->tv_sec = x->tv_sec - y->tv_sec;
result->tv_nsec = x->tv_nsec - y->tv_nsec;
if (result->tv_nsec < 0) {
--result->tv_sec;
result->tv_nsec += NSEC_PER_SEC;
}
/* Return 1 if result is negative. */
return result->tv_sec < 0;
}
void usage(void)
{
printf("usage: sched_task <options>\n");
printf(" options:\n");
printf("\t--prio\t\t- specify priority [real time] of the task\n");
printf("\t--work\t\t- specify time the task is executed [us]\n");
printf("\t--count\t\t- specify how many times the task is executed\n");
printf("\t\t\t- if --count is specified but --work is not then\n");
printf("\t\t\t the task will be executed (count/10) times before\n");
printf("\t\t\t each sleep time\n");
printf("\t\t\t- if both --count and --work are specified then\n");
printf("\t\t\t the task will be executed for --work time before\n");
printf("\t\t\t going to sleep\n");
printf("\t--offset\t- specify time after which execution starts [us]\n");
printf("\t--tperiod\t- specify period of the task [us]\n");
printf("\t--mlockall\t- lock current and future memory\n");
printf("\t--sched_other\t- use standard SCHED_OTHER scheduling policy for normal\n");
printf("\t\t\t task; priority will be ignored\n");
printf("\t--debug\t\t- turn on debug prints\n");
printf("\t--help\t\t- print this message\n");
}
void process_command_line(int argc, char **argv)
{
int opt;
while ((opt = getopt_long(argc, argv, "+", options, NULL)) != -1) {
switch (opt) {
case '?':
case 'h':
usage();
exit(0);
case 'p':
priority = strtol(optarg, NULL, 10);
break;
case 's':
sched_policy = SCHED_OTHER;
break;
case 't':
task_period = strtoul(optarg, NULL, 10) * USEC_TO_NSEC;
break;
case 'w':
work_time = strtoul(optarg, NULL, 10) * USEC_TO_NSEC;
break;
case 'c':
task_count = strtoul(optarg, NULL, 10);
break;
case 'o':
work_offset = strtoul(optarg, NULL, 10) * USEC_TO_NSEC;
break;
}
}
if (!work_time)
work_count = task_count / 10;
}
int main(int argc, char* argv[])
{
struct timespec t, tt, ttt, result;
struct sched_param param;
int count = 0;
process_command_line(argc, argv);
if(work_time > task_period) {
perror("Wrong value of \'work\' time");
exit(-1);
}
if(task_period < TIME_MIN) {
error(0, 0, "Task period time (%d) must be grater than %d [us]\n",
task_period/1000, TIME_MIN/1000);
exit(-1);
}
if(work_time && (work_time < TIME_MIN)) {
error(0, 0, "Work time (%d) must be grater than %d [us]\n",
work_time/1000, TIME_MIN/1000);
exit(-1);
}
if (task_count < 0) {
perror("Wrong value for --count\n");
exit(-1);
}
if (!work_time && !task_count) {
perror("At least one of (--work,--count) must be provided\n");
exit(-1);
}
if ((sched_policy != SCHED_FIFO) && (sched_policy != SCHED_RR))
priority = 0;
param.sched_priority = priority;
if(sched_setscheduler(0, sched_policy, ¶m) == -1) {
perror("sched_setscheduler for policy %d failed\n");
}
if (lockall_flag)
if(mlockall(MCL_CURRENT|MCL_FUTURE) == -1) {
perror("mlockall failed");
exit(-2);
}
clock_gettime(CLOCK_MONOTONIC ,&t);
adjust_wakeup_time(&t, work_offset);
while(!task_count || (count < task_count)) {
/* wait until next shot */
clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &t, NULL);
clock_gettime(CLOCK_MONOTONIC ,&tt);
clock_gettime(CLOCK_MONOTONIC ,&ttt);
DBG("t:%d %d tt:%d %d ", t.tv_sec, t.tv_nsec,
tt.tv_sec, tt.tv_nsec);
adjust_wakeup_time(&tt, work_time);
if (work_time) {
do {
busy_test();
clock_gettime(CLOCK_MONOTONIC ,&ttt);
count++;
} while (!time_passed(&tt, &ttt, &result));
} else {
int i = 0;
do {
busy_test();
clock_gettime(CLOCK_MONOTONIC ,&ttt);
count++;
} while (++i < work_count);
}
DBG("tt:%d %d ttt:%d %d\n", tt.tv_sec, tt.tv_nsec,
ttt.tv_sec, ttt.tv_nsec);
/* calculate next shot */
adjust_wakeup_time(&t, task_period);
}
printf("Work executions: %d\n", count);
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
2014-11-12 15:13 ` Bartlomiej Zolnierkiewicz
@ 2014-11-12 16:43 ` Daniel Lezcano
2014-11-12 18:41 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2014-11-12 16:43 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Kukjin Kim, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, Lukasz Majewski
On 11/12/2014 04:13 PM, Bartlomiej Zolnierkiewicz wrote:
>
Hi Bartlomiej,
[ cut ]
>>> - using arch_send_wakeup_ipi_mask() instead of dsb_sev()
>>> (this matches CPU hotplug code in arch/arm/mach-exynos/platsmp.c)
>>
>> I am curious. You experienced very rare hangs after running the tests a
>> few hours, right ? Is the SEV replaced by the IPI solving the issue ? If
>> yes, how did you catch it ?
>
> Rare hangs showed up after about 30-40 minutes of testing with the attached
> app and script (running of "./cpuidle_state1_test.sh script 2 500" has never
> completed on the umodified driver).
>
> The problem turned out to be in the following loop waiting for CPU1 to get
> stuck in the BOOT ROM:
>
> /*
> * Wait for cpu1 to get stuck in the boot rom
> */
> while ((__raw_readl(BOOT_VECTOR) != 0) &&
> !atomic_read(&cpu1_wakeup))
> cpu_relax();
>
> [ Removal of the loop fixed the problem. ]
Just for my personal information, do you know why ?
> Using the SEV instead of the IPI was not a issue but it was changed to
> match the existing Exynos platform code (exynos_boot_secondary() in
> arch/arm/mach-exynos/platsmp.c) and as preparation for Exynos4412 (quad
> core) support.
Ah, ok. Thanks for the info.
[ cut ]
>>> +#ifdef CONFIG_ARM_EXYNOS_CPUIDLE
>>> + if (of_machine_is_compatible("samsung,exynos4210"))
>>> + exynos_cpuidle.dev.platform_data = &cpuidle_coupled_exynos_data;
>>> +#endif
>>
>> You should not add those #ifdef.
>
> Without those #ifdef I get:
>
> LD init/built-in.o
> arch/arm/mach-exynos/built-in.o: In function `exynos_dt_machine_init':
> /home/bzolnier/sam/linux-sprc/arch/arm/mach-exynos/exynos.c:334: undefined reference to `cpuidle_coupled_exynos_data'
> make: *** [vmlinux] Error 1
>
> when CONFIG_EXYNOS_CPU_SUSPEND is disabled.
Here, we are introducing some dependencies I tried to drop in the
different drivers.
I looked more closely at the code and especially the
'cpuidle_coupled_exynos_data'. I don't think it is worth to have it
because it adds more complexity and you have to define this structure to
be visible from the drivers/cpuidle files.
I suggest you create an simple function in "pm.c"
int exynos_coupled_aftr(int cpu)
{
pre_enter...
if (!cpu)
cpu0_enter_aftr()
else
cpu1_powerdown()
post_enter...
}
and in the cpuidle driver itself, you just use the already existing
anonymous callback 'exynos_enter_aftr' (and mutate it to conform the
parameters).
You won't have to share any structure between the arch code and the
cpuidle driver.
>>> 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
[ cut ]
>>> - exynos_enter_aftr = (void *)(pdev->dev.platform_data);
>>> + if (of_machine_is_compatible("samsung,exynos4210")) {
>>> + exynos_cpuidle_pdata = pdev->dev.platform_data;
>>> +
>>> + exynos_idle_driver.states[1].enter =
>>> + exynos_enter_coupled_lowpower;
>>> + exynos_idle_driver.states[1].exit_latency = 5000;
>>> + exynos_idle_driver.states[1].target_residency = 10000;
>>> + exynos_idle_driver.states[1].flags |= CPUIDLE_FLAG_COUPLED |
>>> + CPUIDLE_FLAG_TIMER_STOP;
>>
>> I tried to remove those dynamic state allocation everywhere in the
>> different drivers. I would prefer to have another cpuidle_driver to be
>> registered with its states instead of overwriting the existing idle state.
>>
>> struct cpuidle_driver exynos4210_idle_driver = {
>> .name = "exynos4210_idle",
>> .owner = THIS_MODULE,
>> .states = {
>> [0] = ARM_CPUIDLE_WFI_STATE,
>> [1] = {
>> .enter = exynos_enter_coupled_lowpower,
>> .exit_latency = 5000,
>> .target_residency = 10000,
>> .flags = CPUIDLE_FLAG_TIME_VALID |
>> CPUIDLE_FLAG_COUPLED |
>> CPUIDLE_FLAG_TIMER_STOP,
>> .name = "C1",
>> .desc = "ARM power down",
>> },
>> }
>> };
>>
>>
>> and then:
>>
>> if (of_machine_is_compatible("samsung,exynos4210")) {
>> ...
>> ret = cpuidle_register(&exynos4210_idle_driver,
>> cpu_online_mask);
>> ...
>> }
>> ...
>
> OK, I will fix it but (if you are OK with it) I will make the code use
> "exynos_coupled" naming instead of "exynos4210" one to not have to change
> it later.
>
>> If we can reuse this mechanism, which I believe it is possible to, for
>> 4420 and 5250. Then we will be able to refactor this out again.
Ok, sounds good.
> I plan to add support for Exynos3250 next as it should be the simplest
> (it is also dual core) and I need it for other reasons anyway. Exynos4412
> (quad core) support requires more work but should also be doable.
>
> When it comes to Exynos5250 I was thinking about disabling normal AFTR
> mode support for it as according to my testing (on Arndale board) it has
> never worked (at least in upstream kernels, I don't know about Linaro or
> internal ones).
The AFTR state worked on my 5250 very well. It is a Arndale board.
Thanks for resurrecting the patch and providing the multi core idle
support. I am too busy to refocus on that right now.
-- Daniel
--
<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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
2014-11-12 16:43 ` Daniel Lezcano
@ 2014-11-12 18:41 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-11-12 18:41 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Kukjin Kim, Tomasz Figa, Colin Cross, Krzysztof Kozlowski,
Kyungmin Park, linux-samsung-soc, linux-pm, linux-kernel,
linaro-kernel, Lukasz Majewski
Hi,
On Wednesday, November 12, 2014 05:43:20 PM Daniel Lezcano wrote:
> On 11/12/2014 04:13 PM, Bartlomiej Zolnierkiewicz wrote:
> >
>
> Hi Bartlomiej,
>
> [ cut ]
>
> >>> - using arch_send_wakeup_ipi_mask() instead of dsb_sev()
> >>> (this matches CPU hotplug code in arch/arm/mach-exynos/platsmp.c)
> >>
> >> I am curious. You experienced very rare hangs after running the tests a
> >> few hours, right ? Is the SEV replaced by the IPI solving the issue ? If
> >> yes, how did you catch it ?
> >
> > Rare hangs showed up after about 30-40 minutes of testing with the attached
> > app and script (running of "./cpuidle_state1_test.sh script 2 500" has never
> > completed on the umodified driver).
> >
> > The problem turned out to be in the following loop waiting for CPU1 to get
> > stuck in the BOOT ROM:
> >
> > /*
> > * Wait for cpu1 to get stuck in the boot rom
> > */
> > while ((__raw_readl(BOOT_VECTOR) != 0) &&
> > !atomic_read(&cpu1_wakeup))
> > cpu_relax();
> >
> > [ Removal of the loop fixed the problem. ]
>
> Just for my personal information, do you know why ?
Unfortunately no. I just suspect that sometimes the BOOT_VECTOR register
is not zeroed (or is zeroed and then overwritten) because of the bug in
the firmware.
> [ cut ]
>
> >>> +#ifdef CONFIG_ARM_EXYNOS_CPUIDLE
> >>> + if (of_machine_is_compatible("samsung,exynos4210"))
> >>> + exynos_cpuidle.dev.platform_data = &cpuidle_coupled_exynos_data;
> >>> +#endif
> >>
> >> You should not add those #ifdef.
> >
> > Without those #ifdef I get:
> >
> > LD init/built-in.o
> > arch/arm/mach-exynos/built-in.o: In function `exynos_dt_machine_init':
> > /home/bzolnier/sam/linux-sprc/arch/arm/mach-exynos/exynos.c:334: undefined reference to `cpuidle_coupled_exynos_data'
> > make: *** [vmlinux] Error 1
> >
> > when CONFIG_EXYNOS_CPU_SUSPEND is disabled.
>
> Here, we are introducing some dependencies I tried to drop in the
> different drivers.
>
> I looked more closely at the code and especially the
> 'cpuidle_coupled_exynos_data'. I don't think it is worth to have it
> because it adds more complexity and you have to define this structure to
> be visible from the drivers/cpuidle files.
>
> I suggest you create an simple function in "pm.c"
>
> int exynos_coupled_aftr(int cpu)
> {
> pre_enter...
>
> if (!cpu)
> cpu0_enter_aftr()
> else
> cpu1_powerdown()
>
> post_enter...
> }
>
> and in the cpuidle driver itself, you just use the already existing
> anonymous callback 'exynos_enter_aftr' (and mutate it to conform the
> parameters).
>
> You won't have to share any structure between the arch code and the
> cpuidle driver.
The reason why the separate callbacks are needed is that the cpuidle
driver code uses coupled cpuidle barriers (I cannot move them to pm.c):
+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;
+}
Could you please explain how the proposed pm.c::exynos_coupled_aftr()
would operate without these barriers?
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-11-12 18:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-07 18:00 [PATCH 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary Bartlomiej Zolnierkiewicz
2014-11-07 18:00 ` [PATCH 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
2014-11-09 15:57 ` Daniel Lezcano
2014-11-12 15:13 ` Bartlomiej Zolnierkiewicz
2014-11-12 16:43 ` Daniel Lezcano
2014-11-12 18:41 ` Bartlomiej Zolnierkiewicz
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).