diff mbox

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

Message ID 1415383252-20118-3-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz Nov. 7, 2014, 6 p.m. UTC
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

Comments

Daniel Lezcano Nov. 9, 2014, 3:57 p.m. UTC | #1
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
>
Bartlomiej Zolnierkiewicz Nov. 12, 2014, 3:13 p.m. UTC | #2
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
Daniel Lezcano Nov. 12, 2014, 4:43 p.m. UTC | #3
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
Bartlomiej Zolnierkiewicz Nov. 12, 2014, 6:41 p.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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