lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 30 Jan 2015 08:38:01 +0900
From:	Kukjin Kim <kgene@...nel.org>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
CC:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Kukjin Kim <kgene@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Colin Cross <ccross@...gle.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	linux-samsung-soc@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linaro-kernel@...ts.linaro.org
Subject: Re: [PATCH v3 2/2] cpuidle: exynos: add coupled cpuidle support for
 Exynos4210

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@...gle.com>
>>>> Cc: Kukjin Kim <kgene.kim@...sung.com>
>>>> Cc: Krzysztof Kozlowski <k.kozlowski@...sung.com>
>>>> Cc: Tomasz Figa <tomasz.figa@...il.com>
>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>>>> Acked-by: Kyungmin Park <kyungmin.park@...sung.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@...roid.com>
>>>> + *	Daniel Lezcano <daniel.lezcano@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists