LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
To: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>, Kukjin Kim <kgene@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Tomasz Figa <tomasz.figa@gmail.com>,
	Colin Cross <ccross@google.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	linux-samsung-soc@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, linaro-kernel@lists.linaro.org
Subject: Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210
Date: Mon, 26 Jan 2015 12:53:08 +0100	[thread overview]
Message-ID: <1422273188.30150.2.camel@AMDC1943> (raw)
In-Reply-To: <2952015.VHEX8XYZy5@amdc1032>

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


  reply	other threads:[~2015-01-26 11:53 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-23 16:24 [PATCH v3 0/2] " Bartlomiej Zolnierkiewicz
2015-01-23 16:24 ` [PATCH v3 1/2] ARM: EXYNOS: apply S5P_CENTRAL_SEQ_OPTION fix only when necessary Bartlomiej Zolnierkiewicz
2015-01-26  7:49   ` Krzysztof Kozlowski
2015-01-23 16:24 ` [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210 Bartlomiej Zolnierkiewicz
2015-01-26  8:16   ` Krzysztof Kozlowski
2015-01-26 11:33     ` Bartlomiej Zolnierkiewicz
2015-01-26 11:53       ` Krzysztof Kozlowski [this message]
2015-01-29 23:38         ` Kukjin Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1422273188.30150.2.camel@AMDC1943 \
    --to=k.kozlowski@samsung.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=ccross@google.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=kgene.kim@samsung.com \
    --cc=kgene@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=tomasz.figa@gmail.com \
    --subject='Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for Exynos4210' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).