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 <ccr...@google.com> >>>> Cc: Kukjin Kim <kgene....@samsung.com> >>>> Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com> >>>> Cc: Tomasz Figa <tomasz.f...@gmail.com> >>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com> >>>> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org> >>>> Acked-by: Kyungmin Park <kyungmin.p...@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 <ccr...@android.com> >>>> + * Daniel Lezcano <daniel.lezc...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
