[PATCH v3 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[PATCH v3 0/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Bartlomiej Zolnierkiewicz]

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:
- for-next branch (commit: 9663ad71912b) of linux-samsung.git
  kernel tree

Changes since v2:
- rebased on top of for-next branch (commit: 9663ad71912b) of
  linux-samsung.git kernel tree

Changes since v1:
- rebased on top of for-next branch (commit: 760a7d4763c8) of
  linux-samsung.git kernel tree (the patchset also applies fine
  to next-20141126 branch of linux-next kernel tree but needs
  CPUIDLE_FLAG_TIME_VALID flag usage removed to fix build)
- added Signed-off-by from Daniel Lezcano to patch #2
- added separate struct cpuidle_driver exynos_coupled_idle_driver

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             |  76 ++++++++++++++-
 include/linux/platform_data/cpuidle-exynos.h |  20 ++++
 8 files changed, 234 insertions(+), 10 deletions(-)
 create mode 100644 include/linux/platform_data/cpuidle-exynos.h

-- 
1.8.2.3

[PATCH v3 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary

[Bartlomiej Zolnierkiewicz]

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 dfc8594..1a7454d 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -98,10 +98,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)
@@ -165,6 +161,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 82e6b6f..666ec3e 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -319,6 +319,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

[PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Bartlomiej Zolnierkiewicz]

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>
---
 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,
+			.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);
+};
+
+#endif
-- 
1.8.2.3

Re: [PATCH v3 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary

[Krzysztof Kozlowski]

On pią, 2015-01-23 at 17:24 +0100, Bartlomiej Zolnierkiewicz wrote:
> 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(-)

Looks good.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

> 
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index dfc8594..1a7454d 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -98,10 +98,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)
> @@ -165,6 +161,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 82e6b6f..666ec3e 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -319,6 +319,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();
>  

Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Krzysztof Kozlowski]

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.

> +			.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
LPA/LPD/W-AFTR.

Maybe "pre/post_enter_lowpower"?

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.


Best regards,
Krzysztof

> +};
> +
> +#endif

Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Bartlomiej Zolnierkiewicz]

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.

> 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.

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

Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Krzysztof Kozlowski]

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

Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210

[Kukjin Kim]

On 01/26/15 20:53, Krzysztof Kozlowski wrote:
> 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.
> 
Thanks, Krzysztof. And we can enhance this stuff next time, soon.

I've applied and merged into for-next just now.

- Kukjin