| Message ID | 1396604925-18383-1-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Fri, Apr 04, 2014 at 10:48:45AM +0100, Daniel Lezcano wrote: > The following driver is for exynos4210. I did not yet finished the other boards, so > I created a specific driver for 4210 which could be merged later. > > The driver is based on Colin Cross's driver found at: > > https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > > This one was based on a 3.4 kernel and an old API. > > It has been refreshed, simplified and based on the recent code cleanup I sent > today. > > The AFTR could be entered when all the cpus (except cpu0) are down. In order to > reach this situation, the couple idle states are used. > > There is a sync barrier at the entry and the exit of the low power function. So > all cpus will enter and exit the function at the same time. > > At this point, CPU0 knows the other cpu will power down itself. CPU0 waits for > the CPU1 to be powered down and then initiate the AFTR power down sequence. > > No interrupts are handled by CPU1, this is why we switch to the timer broadcast > even if the local timer is not impacted by the idle state. Can you elaborate on this please ? Is that because the local timers are not wake-up capable (shutdown) ? Or because only the IRQs targeted at CPU with MPIDR[7:0] == 0x0 are wake-up capable ? > When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then they both > exit the idle function. Does this mean that once shutdown CPU1 is stuck solid till it is poked by CPU0, right ? Again I think this should be handled with MPIDRs, not with cpu logical indices. > This driver allows the exynos4210 to have the same power consumption at idle > time than the one when we have to unplug CPU1 in order to let CPU0 to reach > the AFTR state. Which is a good thing and that's the way the driver should have been implemented in the first place, thanks for posting it. [...] > +static void (*exynos_aftr)(void); > + > +static int cpu_suspend_finish(unsigned long flags) > +{ > + if (flags) > + exynos_aftr(); This function pointer obfuscates things, not very easy to follow. Is there a reason why you should pass flags and run that code in the finisher instead of running this code in the enter method for CPU0 ? Is that a timing issue ? I do not see why running those commands before saving the context in cpu_suspend() can make any difference (well, apart from timing as I said), the idle functions are already different for CPU0 and CPU1. > + > + cpu_do_idle(); > + > + return -1; > +} > + > +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)) { Again, this dependency on logical CPUs is not ideal, but I guess it is impossible on Exynos to swap the boot CPU (or put it differently boot on a different CPU), so we should live with that. I've got to say MCPM could help make this code a bit more generic (I know that already CPUs are coordinated through coupled C-states, but this primary-secondary shutdown coordination could reuse the MCPM state machine, I have to check). > + > + /* > + * 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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { > + > + /* > + * 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(BOOT_VECTOR) == 0) Who clears the boot vector value ? FW ? Does this mean that CPU1 can be reset if there is a pending IRQ for it ? That's "interesting". Since IRQs are always affine to CPU0 and the local timer entered broadcast I really would like to understand what IRQ can wake up CPU1 here (hardly an IPI, CPU0 has hit the coupled barried already). CPU1 is still in SMP mode though when it hits wfi (and it should not), I wonder whether this plays a role here. > + goto abort; > + > + cpu_relax(); > + } > + } > + > + cpu_pm_enter(); > + > + ret = cpu_suspend(1, cpu_suspend_finish); > + If we are executing here and the cluster has been shutdown, we must be sure that CPU1 is off and I guess that's the case. If both CPUs are released from reset and can run concurrently, there is a problem. It is related to the enabling of the SCU, which, given your last series is supposed to be executed in a CPU PM notifier. That code will work only if CPU1 is dead while CPU0 reboots and I still think that enabling scu so late is a recipe for mayhem and a bad example (not related to your clean-ups - this is true even in current code). On platforms where all CPUs are reset on wake-up this will never ever work, ie the SCU must be on by the time a CPU switches on its caches (because there might be other CPUs running concurrently and they _have_ to be coherent). > + cpu_pm_exit(); > + > +abort: > + if (cpu_online(1)) { > + /* > + * Set the boot vector to something non-zero > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); > + dsb(); > + > + /* > + * Turn on cpu1 and wait for it to be on > + */ > + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); > + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) > + cpu_relax(); > + > + /* > + * Wait for cpu1 to get stuck in the boot rom > + */ > + while ((__raw_readl(BOOT_VECTOR) != 0) && > + !atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + > + if (!atomic_read(&cpu1_wakeup)) { > + /* > + * Poke cpu1 out of the boot rom > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); > + dsb_sev(); > + } > + > + /* > + * Wait for cpu1 to finish booting > + */ > + while (!atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + } > + > + return ret; > +} > + > +static int exynos_powerdown_cpu1(void) > +{ > + int ret = -1; > + > + /* > + * Idle sequence for cpu1 > + */ > + if (cpu_pm_enter()) > + goto cpu1_aborted; > + > + /* > + * Turn off cpu 1 > + */ > + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); > + > + ret = cpu_suspend(0, cpu_suspend_finish); To reiterate my point, CPU1 should execute this code if and only if CPU0 has rebooted it (or idling it failed so it jumps back from ROM code straight away and CPU0 bails out of idle too - scu is not reset in that case). This should be commented in detail. More importantly, I do not see anywhere code that allows CPU1 to clean its caches properly. While running cpu_suspend() the CPU cleans its data cache using the LoUIS API, but it does that with the C bit in SCTLR set, which means that it still allocates and could fetch dirty lines from other CPUs, that's not 100% safe. So, as it is done in arch/arm/mach-vexpress/tc2_pm.c - tc2_pm_down, core must clean its L1 with C bit in SCTLR cleared to stop allocations so that by the time L1 cache clean is complete, the L1 data cache can't contain dirty lines. > + > + cpu_pm_exit(); > + > +cpu1_aborted: > + dsb(); > + /* > + * Notify cpu 0 that cpu 1 is awake > + */ > + atomic_set(&cpu1_wakeup, 1); > + > + return ret; > +} > + > +static int exynos_enter_aftr(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + int ret; > + > + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); > + > + /* > + * 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_powerdown_cpu1() : exynos_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); > + > + atomic_set(&cpu1_wakeup, 0); > + > + return index; > +} > + > +static struct cpuidle_driver exynos_idle_driver = { > + .name = "exynos4210_idle", > + .owner = THIS_MODULE, > + .states = { > + ARM_CPUIDLE_WFI_STATE, > + [1] = { > + .enter = exynos_enter_aftr, > + .exit_latency = 5000, > + .target_residency = 10000, > + .flags = CPUIDLE_FLAG_TIME_VALID | > + 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) > +{ > + exynos_aftr = (void *)(pdev->dev.platform_data); This function pointer passing serves what purpose ? Removing platform code dependency ? I do not see how you can avoid that given how tied this code is to a specific platform. BTW, you should check exynos_aftr != NULL, just in case. Thank you !! Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, > The following driver is for exynos4210. I did not yet finished the > other boards, so I created a specific driver for 4210 which could be > merged later. > If I may ask - do you plan to develop this code for Exynos4412 in a near future? I did some tests (with hotplug) and it turns out, that due to static leakage current one can save up to 12 % of power consumption when power domains for cores are disabled. Such notable power consumption reduction could drive (and justify) the further development of power aware scheduling code. If you don't have time, then I can offer myself to develop the code. I just want to avoid potential duplication of effort. > The driver is based on Colin Cross's driver found at: > > https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > > This one was based on a 3.4 kernel and an old API. > > It has been refreshed, simplified and based on the recent code > cleanup I sent today. > > The AFTR could be entered when all the cpus (except cpu0) are down. > In order to reach this situation, the couple idle states are used. > > There is a sync barrier at the entry and the exit of the low power > function. So all cpus will enter and exit the function at the same > time. > > At this point, CPU0 knows the other cpu will power down itself. CPU0 > waits for the CPU1 to be powered down and then initiate the AFTR > power down sequence. > > No interrupts are handled by CPU1, this is why we switch to the timer > broadcast even if the local timer is not impacted by the idle state. > > When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then > they both exit the idle function. > > This driver allows the exynos4210 to have the same power consumption > at idle time than the one when we have to unplug CPU1 in order to let > CPU0 to reach the AFTR state. > > This patch is a RFC because, we have to find a way to remove the > macros definitions and cpu powerdown function without pulling the > arch dependent headers. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm/mach-exynos/common.c | 11 +- > drivers/cpuidle/Kconfig.arm | 8 ++ > drivers/cpuidle/Makefile | 1 + > drivers/cpuidle/cpuidle-exynos4210.c | 226 > ++++++++++++++++++++++++++++++++++ 4 files changed, 245 > insertions(+), 1 deletion(-) create mode 100644 > drivers/cpuidle/cpuidle-exynos4210.c > > diff --git a/arch/arm/mach-exynos/common.c > b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 > --- a/arch/arm/mach-exynos/common.c > +++ b/arch/arm/mach-exynos/common.c > @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle = { > .id = -1, > }; > > +static struct platform_device exynos4210_cpuidle = { > + .name = "exynos4210-cpuidle", > + .dev.platform_data = exynos_sys_powerdown_aftr, > + .id = -1, > +}; > + > void __init exynos_cpuidle_init(void) > { > - platform_device_register(&exynos_cpuidle); > + if (soc_is_exynos4210()) > + platform_device_register(&exynos4210_cpuidle); > + else > + platform_device_register(&exynos_cpuidle); > } > > void __init exynos_cpufreq_init(void) > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm > index 92f0c12..2772130 100644 > --- a/drivers/cpuidle/Kconfig.arm > +++ b/drivers/cpuidle/Kconfig.arm > @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE > depends on ARCH_EXYNOS > help > Select this to enable cpuidle for Exynos processors > + > +config ARM_EXYNOS4210_CPUIDLE > + bool "Cpu Idle Driver for the Exynos 4210 processor" > + default y > + depends on ARCH_EXYNOS > + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP > + help > + Select this to enable cpuidle for the Exynos 4210 processors > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index 0d1540a..e0ec9bc 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += > cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += > cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += > cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += > cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += > cpuidle-exynos4210.o > > ############################################################################### > # POWERPC drivers > diff --git a/drivers/cpuidle/cpuidle-exynos4210.c > b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 > index 0000000..56f6d51 > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-exynos4210.c > @@ -0,0 +1,226 @@ > +/* > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * Copyright (c) 2014 Linaro : Daniel Lezcano > <daniel.lezcano@linaro.org> > + * http://www.linaro.org > + * > + * Based on the work of Colin Cross <ccross@android.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. > + */ > + > +#include <linux/cpuidle.h> > +#include <linux/cpu_pm.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > + > +#include <asm/proc-fns.h> > +#include <asm/suspend.h> > +#include <asm/cpuidle.h> > + > +#include <plat/pm.h> > +#include <plat/cpu.h> > +#include <plat/map-base.h> > +#include <plat/map-s5p.h> > + > +static atomic_t exynos_idle_barrier; > +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); > + > +#define BOOT_VECTOR S5P_VA_SYSRAM > +#define S5P_VA_PMU S3C_ADDR(0x02180000) > +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) > +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) > +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) > + > +static void (*exynos_aftr)(void); > + > +static int cpu_suspend_finish(unsigned long flags) > +{ > + if (flags) > + exynos_aftr(); > + > + cpu_do_idle(); > + > + return -1; > +} > + > +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { > + > + /* > + * 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(BOOT_VECTOR) == 0) > + goto abort; > + > + cpu_relax(); > + } > + } > + > + cpu_pm_enter(); > + > + ret = cpu_suspend(1, cpu_suspend_finish); > + > + cpu_pm_exit(); > + > +abort: > + if (cpu_online(1)) { > + /* > + * Set the boot vector to something non-zero > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); > + dsb(); > + > + /* > + * Turn on cpu1 and wait for it to be on > + */ > + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); > + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) > + cpu_relax(); > + > + /* > + * Wait for cpu1 to get stuck in the boot rom > + */ > + while ((__raw_readl(BOOT_VECTOR) != 0) && > + !atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + > + if (!atomic_read(&cpu1_wakeup)) { > + /* > + * Poke cpu1 out of the boot rom > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); > + dsb_sev(); > + } > + > + /* > + * Wait for cpu1 to finish booting > + */ > + while (!atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + } > + > + return ret; > +} > + > +static int exynos_powerdown_cpu1(void) > +{ > + int ret = -1; > + > + /* > + * Idle sequence for cpu1 > + */ > + if (cpu_pm_enter()) > + goto cpu1_aborted; > + > + /* > + * Turn off cpu 1 > + */ > + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); > + > + ret = cpu_suspend(0, cpu_suspend_finish); > + > + cpu_pm_exit(); > + > +cpu1_aborted: > + dsb(); > + /* > + * Notify cpu 0 that cpu 1 is awake > + */ > + atomic_set(&cpu1_wakeup, 1); > + > + return ret; > +} > + > +static int exynos_enter_aftr(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + int ret; > + > + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); > + > + /* > + * 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_powerdown_cpu1() : > exynos_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); > + > + atomic_set(&cpu1_wakeup, 0); > + > + return index; > +} > + > +static struct cpuidle_driver exynos_idle_driver = { > + .name = "exynos4210_idle", > + .owner = THIS_MODULE, > + .states = { > + ARM_CPUIDLE_WFI_STATE, > + [1] = { > + .enter = exynos_enter_aftr, > + .exit_latency = 5000, > + .target_residency = 10000, > + .flags = > CPUIDLE_FLAG_TIME_VALID | > + 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) > +{ > + exynos_aftr = (void *)(pdev->dev.platform_data); > + > + return cpuidle_register(&exynos_idle_driver, > cpu_possible_mask); +} > + > +static struct platform_driver exynos_cpuidle_driver = { > + .driver = { > + .name = "exynos4210-cpuidle", > + .owner = THIS_MODULE, > + }, > + .probe = exynos_cpuidle_probe, > +}; > + > +module_platform_driver(exynos_cpuidle_driver); > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 04/15/2014 08:37 AM, Lukasz Majewski wrote: > Hi Daniel, > >> The following driver is for exynos4210. I did not yet finished the >> other boards, so I created a specific driver for 4210 which could be >> merged later. >> > > If I may ask - do you plan to develop this code for Exynos4412 in a > near future? Yes it is in my plan. > I did some tests (with hotplug) and it turns out, that due to static > leakage current one can save up to 12 % of power consumption when power > domains for cores are disabled. > > > Such notable power consumption reduction could drive (and justify) the > further development of power aware scheduling code. > > If you don't have time, then I can offer myself to develop the code. I > just want to avoid potential duplication of effort. I would be very glad if we can cooperate. Thanks for proposing your help. I have put a branch containing the cleanups + driver moving + dual cpu support, so you can base your work in it. git://git.linaro.org/people/daniel.lezcano/linux.git cpuidle/samsung-next I am wondering if the 5250 board wouldn't make sense as a primary target before the 4412... >> The driver is based on Colin Cross's driver found at: >> >> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >> >> This one was based on a 3.4 kernel and an old API. >> >> It has been refreshed, simplified and based on the recent code >> cleanup I sent today. >> >> The AFTR could be entered when all the cpus (except cpu0) are down. >> In order to reach this situation, the couple idle states are used. >> >> There is a sync barrier at the entry and the exit of the low power >> function. So all cpus will enter and exit the function at the same >> time. >> >> At this point, CPU0 knows the other cpu will power down itself. CPU0 >> waits for the CPU1 to be powered down and then initiate the AFTR >> power down sequence. >> >> No interrupts are handled by CPU1, this is why we switch to the timer >> broadcast even if the local timer is not impacted by the idle state. >> >> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >> they both exit the idle function. >> >> This driver allows the exynos4210 to have the same power consumption >> at idle time than the one when we have to unplug CPU1 in order to let >> CPU0 to reach the AFTR state. >> >> This patch is a RFC because, we have to find a way to remove the >> macros definitions and cpu powerdown function without pulling the >> arch dependent headers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> arch/arm/mach-exynos/common.c | 11 +- >> drivers/cpuidle/Kconfig.arm | 8 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-exynos4210.c | 226 >> ++++++++++++++++++++++++++++++++++ 4 files changed, 245 >> insertions(+), 1 deletion(-) create mode 100644 >> drivers/cpuidle/cpuidle-exynos4210.c >> >> diff --git a/arch/arm/mach-exynos/common.c >> b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 >> --- a/arch/arm/mach-exynos/common.c >> +++ b/arch/arm/mach-exynos/common.c >> @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle = { >> .id = -1, >> }; >> >> +static struct platform_device exynos4210_cpuidle = { >> + .name = "exynos4210-cpuidle", >> + .dev.platform_data = exynos_sys_powerdown_aftr, >> + .id = -1, >> +}; >> + >> void __init exynos_cpuidle_init(void) >> { >> - platform_device_register(&exynos_cpuidle); >> + if (soc_is_exynos4210()) >> + platform_device_register(&exynos4210_cpuidle); >> + else >> + platform_device_register(&exynos_cpuidle); >> } >> >> void __init exynos_cpufreq_init(void) >> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm >> index 92f0c12..2772130 100644 >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm >> @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE >> depends on ARCH_EXYNOS >> help >> Select this to enable cpuidle for Exynos processors >> + >> +config ARM_EXYNOS4210_CPUIDLE >> + bool "Cpu Idle Driver for the Exynos 4210 processor" >> + default y >> + depends on ARCH_EXYNOS >> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP >> + help >> + Select this to enable cpuidle for the Exynos 4210 processors >> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile >> index 0d1540a..e0ec9bc 100644 >> --- a/drivers/cpuidle/Makefile >> +++ b/drivers/cpuidle/Makefile >> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += >> cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += >> cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += >> cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += >> cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += >> cpuidle-exynos4210.o >> >> ############################################################################### >> # POWERPC drivers >> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c >> b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 >> index 0000000..56f6d51 >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-exynos4210.c >> @@ -0,0 +1,226 @@ >> +/* >> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Copyright (c) 2014 Linaro : Daniel Lezcano >> <daniel.lezcano@linaro.org> >> + * http://www.linaro.org >> + * >> + * Based on the work of Colin Cross <ccross@android.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. >> + */ >> + >> +#include <linux/cpuidle.h> >> +#include <linux/cpu_pm.h> >> +#include <linux/io.h> >> +#include <linux/platform_device.h> >> + >> +#include <asm/proc-fns.h> >> +#include <asm/suspend.h> >> +#include <asm/cpuidle.h> >> + >> +#include <plat/pm.h> >> +#include <plat/cpu.h> >> +#include <plat/map-base.h> >> +#include <plat/map-s5p.h> >> + >> +static atomic_t exynos_idle_barrier; >> +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); >> + >> +#define BOOT_VECTOR S5P_VA_SYSRAM >> +#define S5P_VA_PMU S3C_ADDR(0x02180000) >> +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) >> +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) >> +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) >> + >> +static void (*exynos_aftr)(void); >> + >> +static int cpu_suspend_finish(unsigned long flags) >> +{ >> + if (flags) >> + exynos_aftr(); >> + >> + cpu_do_idle(); >> + >> + return -1; >> +} >> + >> +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { >> + >> + /* >> + * 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(BOOT_VECTOR) == 0) >> + goto abort; >> + >> + cpu_relax(); >> + } >> + } >> + >> + cpu_pm_enter(); >> + >> + ret = cpu_suspend(1, cpu_suspend_finish); >> + >> + cpu_pm_exit(); >> + >> +abort: >> + if (cpu_online(1)) { >> + /* >> + * Set the boot vector to something non-zero >> + */ >> + __raw_writel(virt_to_phys(s3c_cpu_resume), >> + BOOT_VECTOR); >> + dsb(); >> + >> + /* >> + * Turn on cpu1 and wait for it to be on >> + */ >> + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); >> + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) >> + cpu_relax(); >> + >> + /* >> + * Wait for cpu1 to get stuck in the boot rom >> + */ >> + while ((__raw_readl(BOOT_VECTOR) != 0) && >> + !atomic_read(&cpu1_wakeup)) >> + cpu_relax(); >> + >> + if (!atomic_read(&cpu1_wakeup)) { >> + /* >> + * Poke cpu1 out of the boot rom >> + */ >> + __raw_writel(virt_to_phys(s3c_cpu_resume), >> + BOOT_VECTOR); >> + dsb_sev(); >> + } >> + >> + /* >> + * Wait for cpu1 to finish booting >> + */ >> + while (!atomic_read(&cpu1_wakeup)) >> + cpu_relax(); >> + } >> + >> + return ret; >> +} >> + >> +static int exynos_powerdown_cpu1(void) >> +{ >> + int ret = -1; >> + >> + /* >> + * Idle sequence for cpu1 >> + */ >> + if (cpu_pm_enter()) >> + goto cpu1_aborted; >> + >> + /* >> + * Turn off cpu 1 >> + */ >> + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); >> + >> + ret = cpu_suspend(0, cpu_suspend_finish); >> + >> + cpu_pm_exit(); >> + >> +cpu1_aborted: >> + dsb(); >> + /* >> + * Notify cpu 0 that cpu 1 is awake >> + */ >> + atomic_set(&cpu1_wakeup, 1); >> + >> + return ret; >> +} >> + >> +static int exynos_enter_aftr(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + int ret; >> + >> + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); >> + >> + /* >> + * 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_powerdown_cpu1() : >> exynos_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); >> + >> + atomic_set(&cpu1_wakeup, 0); >> + >> + return index; >> +} >> + >> +static struct cpuidle_driver exynos_idle_driver = { >> + .name = "exynos4210_idle", >> + .owner = THIS_MODULE, >> + .states = { >> + ARM_CPUIDLE_WFI_STATE, >> + [1] = { >> + .enter = exynos_enter_aftr, >> + .exit_latency = 5000, >> + .target_residency = 10000, >> + .flags = >> CPUIDLE_FLAG_TIME_VALID | >> + 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) >> +{ >> + exynos_aftr = (void *)(pdev->dev.platform_data); >> + >> + return cpuidle_register(&exynos_idle_driver, >> cpu_possible_mask); +} >> + >> +static struct platform_driver exynos_cpuidle_driver = { >> + .driver = { >> + .name = "exynos4210-cpuidle", >> + .owner = THIS_MODULE, >> + }, >> + .probe = exynos_cpuidle_probe, >> +}; >> + >> +module_platform_driver(exynos_cpuidle_driver); >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > >
Hi Daniel, > On 04/15/2014 08:37 AM, Lukasz Majewski wrote: > > Hi Daniel, > > > >> The following driver is for exynos4210. I did not yet finished the > >> other boards, so I created a specific driver for 4210 which could > >> be merged later. > >> > > > > If I may ask - do you plan to develop this code for Exynos4412 in a > > near future? > > Yes it is in my plan. > > > I did some tests (with hotplug) and it turns out, that due to static > > leakage current one can save up to 12 % of power consumption when > > power domains for cores are disabled. > > > > > > Such notable power consumption reduction could drive (and justify) > > the further development of power aware scheduling code. > > > > If you don't have time, then I can offer myself to develop the > > code. I just want to avoid potential duplication of effort. > > I would be very glad if we can cooperate. Thanks for proposing your > help. You are welcome :-) > > I have put a branch containing the cleanups + driver moving + dual > cpu support, so you can base your work in it. > > git://git.linaro.org/people/daniel.lezcano/linux.git > cpuidle/samsung-next Thanks for sharing code. I will look into it. > > I am wondering if the 5250 board wouldn't make sense as a primary > target before the 4412... I'm working on a device based on 4412, not 5250. Therefore, I would prefer to have this concept implemented on 4412 as soon as possible to not hinder my scheduler related experiments. If you have other priorities, then we can split the work. What do you think? > > >> The driver is based on Colin Cross's driver found at: > >> > >> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > >> > >> This one was based on a 3.4 kernel and an old API. > >> > >> It has been refreshed, simplified and based on the recent code > >> cleanup I sent today. > >> > >> The AFTR could be entered when all the cpus (except cpu0) are down. > >> In order to reach this situation, the couple idle states are used. > >> > >> There is a sync barrier at the entry and the exit of the low power > >> function. So all cpus will enter and exit the function at the same > >> time. > >> > >> At this point, CPU0 knows the other cpu will power down itself. > >> CPU0 waits for the CPU1 to be powered down and then initiate the > >> AFTR power down sequence. > >> > >> No interrupts are handled by CPU1, this is why we switch to the > >> timer broadcast even if the local timer is not impacted by the > >> idle state. > >> > >> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. > >> Then they both exit the idle function. > >> > >> This driver allows the exynos4210 to have the same power > >> consumption at idle time than the one when we have to unplug CPU1 > >> in order to let CPU0 to reach the AFTR state. > >> > >> This patch is a RFC because, we have to find a way to remove the > >> macros definitions and cpu powerdown function without pulling the > >> arch dependent headers. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > >> arch/arm/mach-exynos/common.c | 11 +- > >> drivers/cpuidle/Kconfig.arm | 8 ++ > >> drivers/cpuidle/Makefile | 1 + > >> drivers/cpuidle/cpuidle-exynos4210.c | 226 > >> ++++++++++++++++++++++++++++++++++ 4 files changed, 245 > >> insertions(+), 1 deletion(-) create mode 100644 > >> drivers/cpuidle/cpuidle-exynos4210.c > >> > >> diff --git a/arch/arm/mach-exynos/common.c > >> b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 > >> --- a/arch/arm/mach-exynos/common.c > >> +++ b/arch/arm/mach-exynos/common.c > >> @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle > >> = { .id = -1, > >> }; > >> > >> +static struct platform_device exynos4210_cpuidle = { > >> + .name = "exynos4210-cpuidle", > >> + .dev.platform_data = exynos_sys_powerdown_aftr, > >> + .id = -1, > >> +}; > >> + > >> void __init exynos_cpuidle_init(void) > >> { > >> - platform_device_register(&exynos_cpuidle); > >> + if (soc_is_exynos4210()) > >> + platform_device_register(&exynos4210_cpuidle); > >> + else > >> + platform_device_register(&exynos_cpuidle); > >> } > >> > >> void __init exynos_cpufreq_init(void) > >> diff --git a/drivers/cpuidle/Kconfig.arm > >> b/drivers/cpuidle/Kconfig.arm index 92f0c12..2772130 100644 > >> --- a/drivers/cpuidle/Kconfig.arm > >> +++ b/drivers/cpuidle/Kconfig.arm > >> @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE > >> depends on ARCH_EXYNOS > >> help > >> Select this to enable cpuidle for Exynos processors > >> + > >> +config ARM_EXYNOS4210_CPUIDLE > >> + bool "Cpu Idle Driver for the Exynos 4210 processor" > >> + default y > >> + depends on ARCH_EXYNOS > >> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP > >> + help > >> + Select this to enable cpuidle for the Exynos 4210 > >> processors diff --git a/drivers/cpuidle/Makefile > >> b/drivers/cpuidle/Makefile index 0d1540a..e0ec9bc 100644 > >> --- a/drivers/cpuidle/Makefile > >> +++ b/drivers/cpuidle/Makefile > >> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += > >> cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += > >> cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += > >> cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += > >> cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += > >> cpuidle-exynos4210.o > >> > >> ############################################################################### > >> # POWERPC drivers > >> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c > >> b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 > >> index 0000000..56f6d51 > >> --- /dev/null > >> +++ b/drivers/cpuidle/cpuidle-exynos4210.c > >> @@ -0,0 +1,226 @@ > >> +/* > >> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > >> + * http://www.samsung.com > >> + * > >> + * Copyright (c) 2014 Linaro : Daniel Lezcano > >> <daniel.lezcano@linaro.org> > >> + * http://www.linaro.org > >> + * > >> + * Based on the work of Colin Cross <ccross@android.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. > >> + */ > >> + > >> +#include <linux/cpuidle.h> > >> +#include <linux/cpu_pm.h> > >> +#include <linux/io.h> > >> +#include <linux/platform_device.h> > >> + > >> +#include <asm/proc-fns.h> > >> +#include <asm/suspend.h> > >> +#include <asm/cpuidle.h> > >> + > >> +#include <plat/pm.h> > >> +#include <plat/cpu.h> > >> +#include <plat/map-base.h> > >> +#include <plat/map-s5p.h> > >> + > >> +static atomic_t exynos_idle_barrier; > >> +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); > >> + > >> +#define BOOT_VECTOR S5P_VA_SYSRAM > >> +#define S5P_VA_PMU S3C_ADDR(0x02180000) > >> +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) > >> +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) > >> +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) > >> + > >> +static void (*exynos_aftr)(void); > >> + > >> +static int cpu_suspend_finish(unsigned long flags) > >> +{ > >> + if (flags) > >> + exynos_aftr(); > >> + > >> + cpu_do_idle(); > >> + > >> + return -1; > >> +} > >> + > >> +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { > >> + > >> + /* > >> + * 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(BOOT_VECTOR) == 0) > >> + goto abort; > >> + > >> + cpu_relax(); > >> + } > >> + } > >> + > >> + cpu_pm_enter(); > >> + > >> + ret = cpu_suspend(1, cpu_suspend_finish); > >> + > >> + cpu_pm_exit(); > >> + > >> +abort: > >> + if (cpu_online(1)) { > >> + /* > >> + * Set the boot vector to something non-zero > >> + */ > >> + __raw_writel(virt_to_phys(s3c_cpu_resume), > >> + BOOT_VECTOR); > >> + dsb(); > >> + > >> + /* > >> + * Turn on cpu1 and wait for it to be on > >> + */ > >> + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); > >> + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != > >> 3) > >> + cpu_relax(); > >> + > >> + /* > >> + * Wait for cpu1 to get stuck in the boot rom > >> + */ > >> + while ((__raw_readl(BOOT_VECTOR) != 0) && > >> + !atomic_read(&cpu1_wakeup)) > >> + cpu_relax(); > >> + > >> + if (!atomic_read(&cpu1_wakeup)) { > >> + /* > >> + * Poke cpu1 out of the boot rom > >> + */ > >> + __raw_writel(virt_to_phys(s3c_cpu_resume), > >> + BOOT_VECTOR); > >> + dsb_sev(); > >> + } > >> + > >> + /* > >> + * Wait for cpu1 to finish booting > >> + */ > >> + while (!atomic_read(&cpu1_wakeup)) > >> + cpu_relax(); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static int exynos_powerdown_cpu1(void) > >> +{ > >> + int ret = -1; > >> + > >> + /* > >> + * Idle sequence for cpu1 > >> + */ > >> + if (cpu_pm_enter()) > >> + goto cpu1_aborted; > >> + > >> + /* > >> + * Turn off cpu 1 > >> + */ > >> + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); > >> + > >> + ret = cpu_suspend(0, cpu_suspend_finish); > >> + > >> + cpu_pm_exit(); > >> + > >> +cpu1_aborted: > >> + dsb(); > >> + /* > >> + * Notify cpu 0 that cpu 1 is awake > >> + */ > >> + atomic_set(&cpu1_wakeup, 1); > >> + > >> + return ret; > >> +} > >> + > >> +static int exynos_enter_aftr(struct cpuidle_device *dev, > >> + struct cpuidle_driver *drv, int index) > >> +{ > >> + int ret; > >> + > >> + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); > >> + > >> + /* > >> + * 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_powerdown_cpu1() : > >> exynos_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); + > >> + atomic_set(&cpu1_wakeup, 0); > >> + > >> + return index; > >> +} > >> + > >> +static struct cpuidle_driver exynos_idle_driver = { > >> + .name = "exynos4210_idle", > >> + .owner = THIS_MODULE, > >> + .states = { > >> + ARM_CPUIDLE_WFI_STATE, > >> + [1] = { > >> + .enter = > >> exynos_enter_aftr, > >> + .exit_latency = 5000, > >> + .target_residency = 10000, > >> + .flags = > >> CPUIDLE_FLAG_TIME_VALID | > >> + 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) > >> +{ > >> + exynos_aftr = (void *)(pdev->dev.platform_data); > >> + > >> + return cpuidle_register(&exynos_idle_driver, > >> cpu_possible_mask); +} > >> + > >> +static struct platform_driver exynos_cpuidle_driver = { > >> + .driver = { > >> + .name = "exynos4210-cpuidle", > >> + .owner = THIS_MODULE, > >> + }, > >> + .probe = exynos_cpuidle_probe, > >> +}; > >> + > >> +module_platform_driver(exynos_cpuidle_driver); > >> -- > >> 1.7.9.5 > >> > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > >
On 04/15/2014 05:54 PM, Lukasz Majewski wrote: > Hi Daniel, > >> On 04/15/2014 08:37 AM, Lukasz Majewski wrote: >>> Hi Daniel, >>> >>>> The following driver is for exynos4210. I did not yet finished the >>>> other boards, so I created a specific driver for 4210 which could >>>> be merged later. >>>> >>> >>> If I may ask - do you plan to develop this code for Exynos4412 in a >>> near future? >> >> Yes it is in my plan. >> >>> I did some tests (with hotplug) and it turns out, that due to static >>> leakage current one can save up to 12 % of power consumption when >>> power domains for cores are disabled. >>> >>> >>> Such notable power consumption reduction could drive (and justify) >>> the further development of power aware scheduling code. >>> >>> If you don't have time, then I can offer myself to develop the >>> code. I just want to avoid potential duplication of effort. >> >> I would be very glad if we can cooperate. Thanks for proposing your >> help. > > You are welcome :-) > >> >> I have put a branch containing the cleanups + driver moving + dual >> cpu support, so you can base your work in it. >> >> git://git.linaro.org/people/daniel.lezcano/linux.git >> cpuidle/samsung-next > > Thanks for sharing code. I will look into it. > >> >> I am wondering if the 5250 board wouldn't make sense as a primary >> target before the 4412... > > I'm working on a device based on 4412, not 5250. Therefore, I would > prefer to have this concept implemented on 4412 as soon as possible to > not hinder my scheduler related experiments. > > If you have other priorities, then we can split the work. What do you > think? It is ok for me if you want to handle the cpuidle driver 4412. Will you create a new driver or extend this dual cpu driver to support 4 cpus ? >>>> The driver is based on Colin Cross's driver found at: >>>> >>>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >>>> >>>> This one was based on a 3.4 kernel and an old API. >>>> >>>> It has been refreshed, simplified and based on the recent code >>>> cleanup I sent today. >>>> >>>> The AFTR could be entered when all the cpus (except cpu0) are down. >>>> In order to reach this situation, the couple idle states are used. >>>> >>>> There is a sync barrier at the entry and the exit of the low power >>>> function. So all cpus will enter and exit the function at the same >>>> time. >>>> >>>> At this point, CPU0 knows the other cpu will power down itself. >>>> CPU0 waits for the CPU1 to be powered down and then initiate the >>>> AFTR power down sequence. >>>> >>>> No interrupts are handled by CPU1, this is why we switch to the >>>> timer broadcast even if the local timer is not impacted by the >>>> idle state. >>>> >>>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. >>>> Then they both exit the idle function. >>>> >>>> This driver allows the exynos4210 to have the same power >>>> consumption at idle time than the one when we have to unplug CPU1 >>>> in order to let CPU0 to reach the AFTR state. >>>> >>>> This patch is a RFC because, we have to find a way to remove the >>>> macros definitions and cpu powerdown function without pulling the >>>> arch dependent headers. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> arch/arm/mach-exynos/common.c | 11 +- >>>> drivers/cpuidle/Kconfig.arm | 8 ++ >>>> drivers/cpuidle/Makefile | 1 + >>>> drivers/cpuidle/cpuidle-exynos4210.c | 226 >>>> ++++++++++++++++++++++++++++++++++ 4 files changed, 245 >>>> insertions(+), 1 deletion(-) create mode 100644 >>>> drivers/cpuidle/cpuidle-exynos4210.c >>>> >>>> diff --git a/arch/arm/mach-exynos/common.c >>>> b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 >>>> --- a/arch/arm/mach-exynos/common.c >>>> +++ b/arch/arm/mach-exynos/common.c >>>> @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle >>>> = { .id = -1, >>>> }; >>>> >>>> +static struct platform_device exynos4210_cpuidle = { >>>> + .name = "exynos4210-cpuidle", >>>> + .dev.platform_data = exynos_sys_powerdown_aftr, >>>> + .id = -1, >>>> +}; >>>> + >>>> void __init exynos_cpuidle_init(void) >>>> { >>>> - platform_device_register(&exynos_cpuidle); >>>> + if (soc_is_exynos4210()) >>>> + platform_device_register(&exynos4210_cpuidle); >>>> + else >>>> + platform_device_register(&exynos_cpuidle); >>>> } >>>> >>>> void __init exynos_cpufreq_init(void) >>>> diff --git a/drivers/cpuidle/Kconfig.arm >>>> b/drivers/cpuidle/Kconfig.arm index 92f0c12..2772130 100644 >>>> --- a/drivers/cpuidle/Kconfig.arm >>>> +++ b/drivers/cpuidle/Kconfig.arm >>>> @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE >>>> depends on ARCH_EXYNOS >>>> help >>>> Select this to enable cpuidle for Exynos processors >>>> + >>>> +config ARM_EXYNOS4210_CPUIDLE >>>> + bool "Cpu Idle Driver for the Exynos 4210 processor" >>>> + default y >>>> + depends on ARCH_EXYNOS >>>> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP >>>> + help >>>> + Select this to enable cpuidle for the Exynos 4210 >>>> processors diff --git a/drivers/cpuidle/Makefile >>>> b/drivers/cpuidle/Makefile index 0d1540a..e0ec9bc 100644 >>>> --- a/drivers/cpuidle/Makefile >>>> +++ b/drivers/cpuidle/Makefile >>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += >>>> cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += >>>> cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += >>>> cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += >>>> cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += >>>> cpuidle-exynos4210.o >>>> >>>> ############################################################################### >>>> # POWERPC drivers >>>> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c >>>> b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 >>>> index 0000000..56f6d51 >>>> --- /dev/null >>>> +++ b/drivers/cpuidle/cpuidle-exynos4210.c >>>> @@ -0,0 +1,226 @@ >>>> +/* >>>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. >>>> + * http://www.samsung.com >>>> + * >>>> + * Copyright (c) 2014 Linaro : Daniel Lezcano >>>> <daniel.lezcano@linaro.org> >>>> + * http://www.linaro.org >>>> + * >>>> + * Based on the work of Colin Cross <ccross@android.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. >>>> + */ >>>> + >>>> +#include <linux/cpuidle.h> >>>> +#include <linux/cpu_pm.h> >>>> +#include <linux/io.h> >>>> +#include <linux/platform_device.h> >>>> + >>>> +#include <asm/proc-fns.h> >>>> +#include <asm/suspend.h> >>>> +#include <asm/cpuidle.h> >>>> + >>>> +#include <plat/pm.h> >>>> +#include <plat/cpu.h> >>>> +#include <plat/map-base.h> >>>> +#include <plat/map-s5p.h> >>>> + >>>> +static atomic_t exynos_idle_barrier; >>>> +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); >>>> + >>>> +#define BOOT_VECTOR S5P_VA_SYSRAM >>>> +#define S5P_VA_PMU S3C_ADDR(0x02180000) >>>> +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) >>>> +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) >>>> +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) >>>> + >>>> +static void (*exynos_aftr)(void); >>>> + >>>> +static int cpu_suspend_finish(unsigned long flags) >>>> +{ >>>> + if (flags) >>>> + exynos_aftr(); >>>> + >>>> + cpu_do_idle(); >>>> + >>>> + return -1; >>>> +} >>>> + >>>> +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { >>>> + >>>> + /* >>>> + * 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(BOOT_VECTOR) == 0) >>>> + goto abort; >>>> + >>>> + cpu_relax(); >>>> + } >>>> + } >>>> + >>>> + cpu_pm_enter(); >>>> + >>>> + ret = cpu_suspend(1, cpu_suspend_finish); >>>> + >>>> + cpu_pm_exit(); >>>> + >>>> +abort: >>>> + if (cpu_online(1)) { >>>> + /* >>>> + * Set the boot vector to something non-zero >>>> + */ >>>> + __raw_writel(virt_to_phys(s3c_cpu_resume), >>>> + BOOT_VECTOR); >>>> + dsb(); >>>> + >>>> + /* >>>> + * Turn on cpu1 and wait for it to be on >>>> + */ >>>> + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); >>>> + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != >>>> 3) >>>> + cpu_relax(); >>>> + >>>> + /* >>>> + * Wait for cpu1 to get stuck in the boot rom >>>> + */ >>>> + while ((__raw_readl(BOOT_VECTOR) != 0) && >>>> + !atomic_read(&cpu1_wakeup)) >>>> + cpu_relax(); >>>> + >>>> + if (!atomic_read(&cpu1_wakeup)) { >>>> + /* >>>> + * Poke cpu1 out of the boot rom >>>> + */ >>>> + __raw_writel(virt_to_phys(s3c_cpu_resume), >>>> + BOOT_VECTOR); >>>> + dsb_sev(); >>>> + } >>>> + >>>> + /* >>>> + * Wait for cpu1 to finish booting >>>> + */ >>>> + while (!atomic_read(&cpu1_wakeup)) >>>> + cpu_relax(); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int exynos_powerdown_cpu1(void) >>>> +{ >>>> + int ret = -1; >>>> + >>>> + /* >>>> + * Idle sequence for cpu1 >>>> + */ >>>> + if (cpu_pm_enter()) >>>> + goto cpu1_aborted; >>>> + >>>> + /* >>>> + * Turn off cpu 1 >>>> + */ >>>> + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); >>>> + >>>> + ret = cpu_suspend(0, cpu_suspend_finish); >>>> + >>>> + cpu_pm_exit(); >>>> + >>>> +cpu1_aborted: >>>> + dsb(); >>>> + /* >>>> + * Notify cpu 0 that cpu 1 is awake >>>> + */ >>>> + atomic_set(&cpu1_wakeup, 1); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int exynos_enter_aftr(struct cpuidle_device *dev, >>>> + struct cpuidle_driver *drv, int index) >>>> +{ >>>> + int ret; >>>> + >>>> + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); >>>> + >>>> + /* >>>> + * 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_powerdown_cpu1() : >>>> exynos_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); + >>>> + atomic_set(&cpu1_wakeup, 0); >>>> + >>>> + return index; >>>> +} >>>> + >>>> +static struct cpuidle_driver exynos_idle_driver = { >>>> + .name = "exynos4210_idle", >>>> + .owner = THIS_MODULE, >>>> + .states = { >>>> + ARM_CPUIDLE_WFI_STATE, >>>> + [1] = { >>>> + .enter = >>>> exynos_enter_aftr, >>>> + .exit_latency = 5000, >>>> + .target_residency = 10000, >>>> + .flags = >>>> CPUIDLE_FLAG_TIME_VALID | >>>> + 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) >>>> +{ >>>> + exynos_aftr = (void *)(pdev->dev.platform_data); >>>> + >>>> + return cpuidle_register(&exynos_idle_driver, >>>> cpu_possible_mask); +} >>>> + >>>> +static struct platform_driver exynos_cpuidle_driver = { >>>> + .driver = { >>>> + .name = "exynos4210-cpuidle", >>>> + .owner = THIS_MODULE, >>>> + }, >>>> + .probe = exynos_cpuidle_probe, >>>> +}; >>>> + >>>> +module_platform_driver(exynos_cpuidle_driver); >>>> -- >>>> 1.7.9.5 >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> >>> >> >> > > >
On Tue, 15 Apr 2014 18:38:39 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 04/15/2014 05:54 PM, Lukasz Majewski wrote: > > Hi Daniel, > > > >> On 04/15/2014 08:37 AM, Lukasz Majewski wrote: > >>> Hi Daniel, > >>> > >>>> The following driver is for exynos4210. I did not yet finished > >>>> the other boards, so I created a specific driver for 4210 which > >>>> could be merged later. > >>>> > >>> > >>> If I may ask - do you plan to develop this code for Exynos4412 in > >>> a near future? > >> > >> Yes it is in my plan. > >> > >>> I did some tests (with hotplug) and it turns out, that due to > >>> static leakage current one can save up to 12 % of power > >>> consumption when power domains for cores are disabled. > >>> > >>> > >>> Such notable power consumption reduction could drive (and justify) > >>> the further development of power aware scheduling code. > >>> > >>> If you don't have time, then I can offer myself to develop the > >>> code. I just want to avoid potential duplication of effort. > >> > >> I would be very glad if we can cooperate. Thanks for proposing your > >> help. > > > > You are welcome :-) > > > >> > >> I have put a branch containing the cleanups + driver moving + dual > >> cpu support, so you can base your work in it. > >> > >> git://git.linaro.org/people/daniel.lezcano/linux.git > >> cpuidle/samsung-next > > > > Thanks for sharing code. I will look into it. > > > >> > >> I am wondering if the 5250 board wouldn't make sense as a primary > >> target before the 4412... > > > > I'm working on a device based on 4412, not 5250. Therefore, I would > > prefer to have this concept implemented on 4412 as soon as possible > > to not hinder my scheduler related experiments. > > > > If you have other priorities, then we can split the work. What do > > you think? > > It is ok for me if you want to handle the cpuidle driver 4412. Ok. Thanks :-) > Will > you create a new driver or extend this dual cpu driver to support 4 > cpus ? For the beginning, I will try to extend the solution proposed for Exynos4210. However, because of the Easter break, the code will be delivered at next week. > > > >>>> The driver is based on Colin Cross's driver found at: > >>>> > >>>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > >>>> > >>>> This one was based on a 3.4 kernel and an old API. > >>>> > >>>> It has been refreshed, simplified and based on the recent code > >>>> cleanup I sent today. > >>>> > >>>> The AFTR could be entered when all the cpus (except cpu0) are > >>>> down. In order to reach this situation, the couple idle states > >>>> are used. > >>>> > >>>> There is a sync barrier at the entry and the exit of the low > >>>> power function. So all cpus will enter and exit the function at > >>>> the same time. > >>>> > >>>> At this point, CPU0 knows the other cpu will power down itself. > >>>> CPU0 waits for the CPU1 to be powered down and then initiate the > >>>> AFTR power down sequence. > >>>> > >>>> No interrupts are handled by CPU1, this is why we switch to the > >>>> timer broadcast even if the local timer is not impacted by the > >>>> idle state. > >>>> > >>>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. > >>>> Then they both exit the idle function. > >>>> > >>>> This driver allows the exynos4210 to have the same power > >>>> consumption at idle time than the one when we have to unplug CPU1 > >>>> in order to let CPU0 to reach the AFTR state. > >>>> > >>>> This patch is a RFC because, we have to find a way to remove the > >>>> macros definitions and cpu powerdown function without pulling the > >>>> arch dependent headers. > >>>> > >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >>>> --- > >>>> arch/arm/mach-exynos/common.c | 11 +- > >>>> drivers/cpuidle/Kconfig.arm | 8 ++ > >>>> drivers/cpuidle/Makefile | 1 + > >>>> drivers/cpuidle/cpuidle-exynos4210.c | 226 > >>>> ++++++++++++++++++++++++++++++++++ 4 files changed, 245 > >>>> insertions(+), 1 deletion(-) create mode 100644 > >>>> drivers/cpuidle/cpuidle-exynos4210.c > >>>> > >>>> diff --git a/arch/arm/mach-exynos/common.c > >>>> b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 > >>>> --- a/arch/arm/mach-exynos/common.c > >>>> +++ b/arch/arm/mach-exynos/common.c > >>>> @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle > >>>> = { .id = -1, > >>>> }; > >>>> > >>>> +static struct platform_device exynos4210_cpuidle = { > >>>> + .name = "exynos4210-cpuidle", > >>>> + .dev.platform_data = exynos_sys_powerdown_aftr, > >>>> + .id = -1, > >>>> +}; > >>>> + > >>>> void __init exynos_cpuidle_init(void) > >>>> { > >>>> - platform_device_register(&exynos_cpuidle); > >>>> + if (soc_is_exynos4210()) > >>>> + platform_device_register(&exynos4210_cpuidle); > >>>> + else > >>>> + platform_device_register(&exynos_cpuidle); > >>>> } > >>>> > >>>> void __init exynos_cpufreq_init(void) > >>>> diff --git a/drivers/cpuidle/Kconfig.arm > >>>> b/drivers/cpuidle/Kconfig.arm index 92f0c12..2772130 100644 > >>>> --- a/drivers/cpuidle/Kconfig.arm > >>>> +++ b/drivers/cpuidle/Kconfig.arm > >>>> @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE > >>>> depends on ARCH_EXYNOS > >>>> help > >>>> Select this to enable cpuidle for Exynos processors > >>>> + > >>>> +config ARM_EXYNOS4210_CPUIDLE > >>>> + bool "Cpu Idle Driver for the Exynos 4210 processor" > >>>> + default y > >>>> + depends on ARCH_EXYNOS > >>>> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP > >>>> + help > >>>> + Select this to enable cpuidle for the Exynos 4210 > >>>> processors diff --git a/drivers/cpuidle/Makefile > >>>> b/drivers/cpuidle/Makefile index 0d1540a..e0ec9bc 100644 > >>>> --- a/drivers/cpuidle/Makefile > >>>> +++ b/drivers/cpuidle/Makefile > >>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) > >>>> += cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += > >>>> cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += > >>>> cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += > >>>> cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += > >>>> cpuidle-exynos4210.o > >>>> > >>>> ############################################################################### > >>>> # POWERPC drivers > >>>> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c > >>>> b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 > >>>> index 0000000..56f6d51 > >>>> --- /dev/null > >>>> +++ b/drivers/cpuidle/cpuidle-exynos4210.c > >>>> @@ -0,0 +1,226 @@ > >>>> +/* > >>>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > >>>> + * http://www.samsung.com > >>>> + * > >>>> + * Copyright (c) 2014 Linaro : Daniel Lezcano > >>>> <daniel.lezcano@linaro.org> > >>>> + * http://www.linaro.org > >>>> + * > >>>> + * Based on the work of Colin Cross <ccross@android.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. > >>>> + */ > >>>> + > >>>> +#include <linux/cpuidle.h> > >>>> +#include <linux/cpu_pm.h> > >>>> +#include <linux/io.h> > >>>> +#include <linux/platform_device.h> > >>>> + > >>>> +#include <asm/proc-fns.h> > >>>> +#include <asm/suspend.h> > >>>> +#include <asm/cpuidle.h> > >>>> + > >>>> +#include <plat/pm.h> > >>>> +#include <plat/cpu.h> > >>>> +#include <plat/map-base.h> > >>>> +#include <plat/map-s5p.h> > >>>> + > >>>> +static atomic_t exynos_idle_barrier; > >>>> +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); > >>>> + > >>>> +#define BOOT_VECTOR S5P_VA_SYSRAM > >>>> +#define S5P_VA_PMU S3C_ADDR(0x02180000) > >>>> +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) > >>>> +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) > >>>> +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) > >>>> + > >>>> +static void (*exynos_aftr)(void); > >>>> + > >>>> +static int cpu_suspend_finish(unsigned long flags) > >>>> +{ > >>>> + if (flags) > >>>> + exynos_aftr(); > >>>> + > >>>> + cpu_do_idle(); > >>>> + > >>>> + return -1; > >>>> +} > >>>> + > >>>> +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { > >>>> + > >>>> + /* > >>>> + * 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(BOOT_VECTOR) == 0) > >>>> + goto abort; > >>>> + > >>>> + cpu_relax(); > >>>> + } > >>>> + } > >>>> + > >>>> + cpu_pm_enter(); > >>>> + > >>>> + ret = cpu_suspend(1, cpu_suspend_finish); > >>>> + > >>>> + cpu_pm_exit(); > >>>> + > >>>> +abort: > >>>> + if (cpu_online(1)) { > >>>> + /* > >>>> + * Set the boot vector to something non-zero > >>>> + */ > >>>> + __raw_writel(virt_to_phys(s3c_cpu_resume), > >>>> + BOOT_VECTOR); > >>>> + dsb(); > >>>> + > >>>> + /* > >>>> + * Turn on cpu1 and wait for it to be on > >>>> + */ > >>>> + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); > >>>> + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != > >>>> 3) > >>>> + cpu_relax(); > >>>> + > >>>> + /* > >>>> + * Wait for cpu1 to get stuck in the boot rom > >>>> + */ > >>>> + while ((__raw_readl(BOOT_VECTOR) != 0) && > >>>> + !atomic_read(&cpu1_wakeup)) > >>>> + cpu_relax(); > >>>> + > >>>> + if (!atomic_read(&cpu1_wakeup)) { > >>>> + /* > >>>> + * Poke cpu1 out of the boot rom > >>>> + */ > >>>> + > >>>> __raw_writel(virt_to_phys(s3c_cpu_resume), > >>>> + BOOT_VECTOR); > >>>> + dsb_sev(); > >>>> + } > >>>> + > >>>> + /* > >>>> + * Wait for cpu1 to finish booting > >>>> + */ > >>>> + while (!atomic_read(&cpu1_wakeup)) > >>>> + cpu_relax(); > >>>> + } > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int exynos_powerdown_cpu1(void) > >>>> +{ > >>>> + int ret = -1; > >>>> + > >>>> + /* > >>>> + * Idle sequence for cpu1 > >>>> + */ > >>>> + if (cpu_pm_enter()) > >>>> + goto cpu1_aborted; > >>>> + > >>>> + /* > >>>> + * Turn off cpu 1 > >>>> + */ > >>>> + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); > >>>> + > >>>> + ret = cpu_suspend(0, cpu_suspend_finish); > >>>> + > >>>> + cpu_pm_exit(); > >>>> + > >>>> +cpu1_aborted: > >>>> + dsb(); > >>>> + /* > >>>> + * Notify cpu 0 that cpu 1 is awake > >>>> + */ > >>>> + atomic_set(&cpu1_wakeup, 1); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int exynos_enter_aftr(struct cpuidle_device *dev, > >>>> + struct cpuidle_driver *drv, int > >>>> index) +{ > >>>> + int ret; > >>>> + > >>>> + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); > >>>> + > >>>> + /* > >>>> + * 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_powerdown_cpu1() : > >>>> exynos_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); + > >>>> + atomic_set(&cpu1_wakeup, 0); > >>>> + > >>>> + return index; > >>>> +} > >>>> + > >>>> +static struct cpuidle_driver exynos_idle_driver = { > >>>> + .name = "exynos4210_idle", > >>>> + .owner = THIS_MODULE, > >>>> + .states = { > >>>> + ARM_CPUIDLE_WFI_STATE, > >>>> + [1] = { > >>>> + .enter = > >>>> exynos_enter_aftr, > >>>> + .exit_latency = 5000, > >>>> + .target_residency = 10000, > >>>> + .flags = > >>>> CPUIDLE_FLAG_TIME_VALID | > >>>> + 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) > >>>> +{ > >>>> + exynos_aftr = (void *)(pdev->dev.platform_data); > >>>> + > >>>> + return cpuidle_register(&exynos_idle_driver, > >>>> cpu_possible_mask); +} > >>>> + > >>>> +static struct platform_driver exynos_cpuidle_driver = { > >>>> + .driver = { > >>>> + .name = "exynos4210-cpuidle", > >>>> + .owner = THIS_MODULE, > >>>> + }, > >>>> + .probe = exynos_cpuidle_probe, > >>>> +}; > >>>> + > >>>> +module_platform_driver(exynos_cpuidle_driver); > >>>> -- > >>>> 1.7.9.5 > >>>> > >>>> > >>>> _______________________________________________ > >>>> linux-arm-kernel mailing list > >>>> linux-arm-kernel@lists.infradead.org > >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >>> > >>> > >>> > >> > >> > > > > > > > > Best regards, Lukasz Majewski
Hi Daniel, Please see my comments inline. Btw. Please fix your e-mail composer to properly wrap your messages around 7xth column, as otherwise they're hard to read. On 04.04.2014 11:48, Daniel Lezcano wrote: > The following driver is for exynos4210. I did not yet finished the other boards, so > I created a specific driver for 4210 which could be merged later. > > The driver is based on Colin Cross's driver found at: > > https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > > This one was based on a 3.4 kernel and an old API. > > It has been refreshed, simplified and based on the recent code cleanup I sent > today. > > The AFTR could be entered when all the cpus (except cpu0) are down. In order to > reach this situation, the couple idle states are used. > > There is a sync barrier at the entry and the exit of the low power function. So > all cpus will enter and exit the function at the same time. > > At this point, CPU0 knows the other cpu will power down itself. CPU0 waits for > the CPU1 to be powered down and then initiate the AFTR power down sequence. > > No interrupts are handled by CPU1, this is why we switch to the timer broadcast > even if the local timer is not impacted by the idle state. > > When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then they both > exit the idle function. > > This driver allows the exynos4210 to have the same power consumption at idle > time than the one when we have to unplug CPU1 in order to let CPU0 to reach > the AFTR state. > > This patch is a RFC because, we have to find a way to remove the macros > definitions and cpu powerdown function without pulling the arch dependent > headers. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm/mach-exynos/common.c | 11 +- > drivers/cpuidle/Kconfig.arm | 8 ++ > drivers/cpuidle/Makefile | 1 + > drivers/cpuidle/cpuidle-exynos4210.c | 226 ++++++++++++++++++++++++++++++++++ > 4 files changed, 245 insertions(+), 1 deletion(-) > create mode 100644 drivers/cpuidle/cpuidle-exynos4210.c > > diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c > index d5fa21e..1765a98 100644 > --- a/arch/arm/mach-exynos/common.c > +++ b/arch/arm/mach-exynos/common.c > @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle = { > .id = -1, > }; > > +static struct platform_device exynos4210_cpuidle = { > + .name = "exynos4210-cpuidle", > + .dev.platform_data = exynos_sys_powerdown_aftr, > + .id = -1, > +}; > + > void __init exynos_cpuidle_init(void) > { > - platform_device_register(&exynos_cpuidle); > + if (soc_is_exynos4210()) > + platform_device_register(&exynos4210_cpuidle); > + else > + platform_device_register(&exynos_cpuidle); > } > > void __init exynos_cpufreq_init(void) > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm > index 92f0c12..2772130 100644 > --- a/drivers/cpuidle/Kconfig.arm > +++ b/drivers/cpuidle/Kconfig.arm > @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE > depends on ARCH_EXYNOS > help > Select this to enable cpuidle for Exynos processors > + > +config ARM_EXYNOS4210_CPUIDLE > + bool "Cpu Idle Driver for the Exynos 4210 processor" > + default y > + depends on ARCH_EXYNOS > + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP > + help > + Select this to enable cpuidle for the Exynos 4210 processors > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index 0d1540a..e0ec9bc 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += cpuidle-zynq.o > obj-$(CONFIG_ARM_U8500_CPUIDLE) += cpuidle-ux500.o > obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o > obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += cpuidle-exynos.o > +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += cpuidle-exynos4210.o > > ############################################################################### > # POWERPC drivers > diff --git a/drivers/cpuidle/cpuidle-exynos4210.c b/drivers/cpuidle/cpuidle-exynos4210.c > new file mode 100644 > index 0000000..56f6d51 > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-exynos4210.c > @@ -0,0 +1,226 @@ > +/* > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * Copyright (c) 2014 Linaro : Daniel Lezcano <daniel.lezcano@linaro.org> > + * http://www.linaro.org > + * > + * Based on the work of Colin Cross <ccross@android.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. > + */ > + > +#include <linux/cpuidle.h> > +#include <linux/cpu_pm.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > + > +#include <asm/proc-fns.h> > +#include <asm/suspend.h> > +#include <asm/cpuidle.h> > + > +#include <plat/pm.h> > +#include <plat/cpu.h> > +#include <plat/map-base.h> > +#include <plat/map-s5p.h> This won't work with multiplatform. > + > +static atomic_t exynos_idle_barrier; > +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); > + > +#define BOOT_VECTOR S5P_VA_SYSRAM > +#define S5P_VA_PMU S3C_ADDR(0x02180000) > +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) > +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) > +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) > + > +static void (*exynos_aftr)(void); Could we get rid of those global variables? I know that they won't break anything, but they don't look good. A driver data struct would look much better. > + > +static int cpu_suspend_finish(unsigned long flags) Name of this function could be more meaningful. Something like exynos_cpu_enter_lpm() could be better. Same goes for the argument. Maybe it could be simply called enter_aftr? > +{ > + if (flags) > + exynos_aftr(); > + > + cpu_do_idle(); > + > + return -1; > +} > + > +static int exynos_cpu0_enter_aftr(void) > +{ > + int ret = -1; Hmm, wouldn't a real error code be better here? > + > + /* > + * 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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { nit: A macro could be used for 3. > + > + /* > + * 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(BOOT_VECTOR) == 0) Is this a reliable behavior? I mean, is this part of some specification or rather a feature specific to a particular Android bootloader this was used with in original kernel tree? > + goto abort; > + > + cpu_relax(); > + } > + } > + > + cpu_pm_enter(); > + > + ret = cpu_suspend(1, cpu_suspend_finish); > + > + cpu_pm_exit(); > + > +abort: > + if (cpu_online(1)) { > + /* > + * Set the boot vector to something non-zero > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); Resume address is quite a specific "something non-zero". :) > + dsb(); Checkpatch would probably complain about missing comment on why dsb() is needed here (or I'm confusing this with other barriers). > + > + /* > + * Turn on cpu1 and wait for it to be on > + */ > + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); > + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) > + cpu_relax(); nit: Here again 3 could be replaced with a macro. > + > + /* > + * Wait for cpu1 to get stuck in the boot rom > + */ > + while ((__raw_readl(BOOT_VECTOR) != 0) && Same comment about the assumption about BOOT_VECTOR changing to 0 as above. > + !atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + > + if (!atomic_read(&cpu1_wakeup)) { > + /* > + * Poke cpu1 out of the boot rom > + */ > + __raw_writel(virt_to_phys(s3c_cpu_resume), > + BOOT_VECTOR); > + dsb_sev(); Hmm, platsmp code seems to be using an IPI to do this. Again, I wonder if this isn't a behavior implemented only in some specific Android bootloader. > + } > + > + /* > + * Wait for cpu1 to finish booting > + */ > + while (!atomic_read(&cpu1_wakeup)) > + cpu_relax(); > + } > + > + return ret; > +} > + > +static int exynos_powerdown_cpu1(void) > +{ > + int ret = -1; Error code? > + > + /* > + * Idle sequence for cpu1 > + */ > + if (cpu_pm_enter()) > + goto cpu1_aborted; > + > + /* > + * Turn off cpu 1 > + */ > + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); > + > + ret = cpu_suspend(0, cpu_suspend_finish); > + > + cpu_pm_exit(); > + > +cpu1_aborted: > + dsb(); > + /* > + * Notify cpu 0 that cpu 1 is awake > + */ > + atomic_set(&cpu1_wakeup, 1); > + > + return ret; > +} > + > +static int exynos_enter_aftr(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + int ret; > + > + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); Why this is being written here, instead of some of the low level functions above? Otherwise, I quite like the whole idea. I need to play a bit with CPU hotplug and PMU to verify that things couldn't really be simplified a bit, but in general this looks reasonably. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24/2014 07:42 PM, Tomasz Figa wrote: > Hi Daniel, > > Please see my comments inline. Hi Tomasz, > Btw. Please fix your e-mail composer to properly wrap your messages > around 7xth column, as otherwise they're hard to read. Well it is already set to the 71th column. > On 04.04.2014 11:48, Daniel Lezcano wrote: >> The following driver is for exynos4210. I did not yet finished the >> other boards, so >> I created a specific driver for 4210 which could be merged later. >> >> The driver is based on Colin Cross's driver found at: >> >> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >> >> >> This one was based on a 3.4 kernel and an old API. >> >> It has been refreshed, simplified and based on the recent code cleanup >> I sent >> today. >> >> The AFTR could be entered when all the cpus (except cpu0) are down. In >> order to >> reach this situation, the couple idle states are used. >> >> There is a sync barrier at the entry and the exit of the low power >> function. So >> all cpus will enter and exit the function at the same time. >> >> At this point, CPU0 knows the other cpu will power down itself. CPU0 >> waits for >> the CPU1 to be powered down and then initiate the AFTR power down >> sequence. >> >> No interrupts are handled by CPU1, this is why we switch to the timer >> broadcast >> even if the local timer is not impacted by the idle state. >> >> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >> they both >> exit the idle function. >> >> This driver allows the exynos4210 to have the same power consumption >> at idle >> time than the one when we have to unplug CPU1 in order to let CPU0 to >> reach >> the AFTR state. >> >> This patch is a RFC because, we have to find a way to remove the macros >> definitions and cpu powerdown function without pulling the arch dependent >> headers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> arch/arm/mach-exynos/common.c | 11 +- >> drivers/cpuidle/Kconfig.arm | 8 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-exynos4210.c | 226 >> ++++++++++++++++++++++++++++++++++ >> 4 files changed, 245 insertions(+), 1 deletion(-) >> create mode 100644 drivers/cpuidle/cpuidle-exynos4210.c >> >> diff --git a/arch/arm/mach-exynos/common.c >> b/arch/arm/mach-exynos/common.c >> index d5fa21e..1765a98 100644 >> --- a/arch/arm/mach-exynos/common.c >> +++ b/arch/arm/mach-exynos/common.c >> @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle = { >> .id = -1, >> }; >> >> +static struct platform_device exynos4210_cpuidle = { >> + .name = "exynos4210-cpuidle", >> + .dev.platform_data = exynos_sys_powerdown_aftr, >> + .id = -1, >> +}; >> + >> void __init exynos_cpuidle_init(void) >> { >> - platform_device_register(&exynos_cpuidle); >> + if (soc_is_exynos4210()) >> + platform_device_register(&exynos4210_cpuidle); >> + else >> + platform_device_register(&exynos_cpuidle); >> } >> >> void __init exynos_cpufreq_init(void) >> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm >> index 92f0c12..2772130 100644 >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm >> @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE >> depends on ARCH_EXYNOS >> help >> Select this to enable cpuidle for Exynos processors >> + >> +config ARM_EXYNOS4210_CPUIDLE >> + bool "Cpu Idle Driver for the Exynos 4210 processor" >> + default y >> + depends on ARCH_EXYNOS >> + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP >> + help >> + Select this to enable cpuidle for the Exynos 4210 processors >> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile >> index 0d1540a..e0ec9bc 100644 >> --- a/drivers/cpuidle/Makefile >> +++ b/drivers/cpuidle/Makefile >> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += cpuidle-zynq.o >> obj-$(CONFIG_ARM_U8500_CPUIDLE) += cpuidle-ux500.o >> obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o >> obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += cpuidle-exynos.o >> +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += cpuidle-exynos4210.o >> >> >> ############################################################################### >> >> # POWERPC drivers >> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c >> b/drivers/cpuidle/cpuidle-exynos4210.c >> new file mode 100644 >> index 0000000..56f6d51 >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-exynos4210.c >> @@ -0,0 +1,226 @@ >> +/* >> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Copyright (c) 2014 Linaro : Daniel Lezcano >> <daniel.lezcano@linaro.org> >> + * http://www.linaro.org >> + * >> + * Based on the work of Colin Cross <ccross@android.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. >> + */ >> + >> +#include <linux/cpuidle.h> >> +#include <linux/cpu_pm.h> >> +#include <linux/io.h> >> +#include <linux/platform_device.h> >> + >> +#include <asm/proc-fns.h> >> +#include <asm/suspend.h> >> +#include <asm/cpuidle.h> >> + >> +#include <plat/pm.h> >> +#include <plat/cpu.h> >> +#include <plat/map-base.h> >> +#include <plat/map-s5p.h> > > This won't work with multiplatform. > >> + >> +static atomic_t exynos_idle_barrier; >> +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); >> + >> +#define BOOT_VECTOR S5P_VA_SYSRAM >> +#define S5P_VA_PMU S3C_ADDR(0x02180000) >> +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) >> +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) >> +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) >> + >> +static void (*exynos_aftr)(void); > > Could we get rid of those global variables? I know that they won't break > anything, but they don't look good. A driver data struct would look much > better. Yeah, that is part of the plan. As mentioned before I want to kill those variable/macro and define a cpuidle ops structure to be filled from pm.c. But I would like to have this structure to be shared across the different cpuidle drivers and defined in the common arm code. The structure definition will depend on the different existing drivers, so I would like to do it in a separate patchset and keep the exynos_aftr() callback for the moment. Does your comment 'those global variables' includes 'exynos_idle_barrier' and 'cpu1_wakeup' ? >> +static int cpu_suspend_finish(unsigned long flags) > > Name of this function could be more meaningful. Something like > exynos_cpu_enter_lpm() could be better. Same goes for the argument. > Maybe it could be simply called enter_aftr? Agree, I can find something more meaningful. >> +{ >> + if (flags) >> + exynos_aftr(); >> + >> + cpu_do_idle(); >> + >> + return -1; >> +} >> + >> +static int exynos_cpu0_enter_aftr(void) >> +{ >> + int ret = -1; > > Hmm, wouldn't a real error code be better here? Ok, what error ? -EAGAIN ? >> + >> + /* >> + * 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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { > > nit: A macro could be used for 3. > >> + >> + /* >> + * 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(BOOT_VECTOR) == 0) > > Is this a reliable behavior? I mean, is this part of some specification > or rather a feature specific to a particular Android bootloader this was > used with in original kernel tree? > >> + goto abort; >> + >> + cpu_relax(); >> + } >> + } >> + >> + cpu_pm_enter(); >> + >> + ret = cpu_suspend(1, cpu_suspend_finish); >> + >> + cpu_pm_exit(); >> + >> +abort: >> + if (cpu_online(1)) { >> + /* >> + * Set the boot vector to something non-zero >> + */ >> + __raw_writel(virt_to_phys(s3c_cpu_resume), >> + BOOT_VECTOR); > > Resume address is quite a specific "something non-zero". :) Ok, this is resulting from reverse engineering. I don't have the documentation. If you can briefly summarize how it works that may help to simplify/clarify the result. >> + dsb(); > > Checkpatch would probably complain about missing comment on why dsb() is > needed here (or I'm confusing this with other barriers). Same comment than above. I guess this barrier is needed to ensure the value is effectively written to BOOT_VECTOR before shutting down the core. >> + >> + /* >> + * Turn on cpu1 and wait for it to be on >> + */ >> + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); >> + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) >> + cpu_relax(); > > nit: Here again 3 could be replaced with a macro. > >> + >> + /* >> + * Wait for cpu1 to get stuck in the boot rom >> + */ >> + while ((__raw_readl(BOOT_VECTOR) != 0) && > > Same comment about the assumption about BOOT_VECTOR changing to 0 as above. > >> + !atomic_read(&cpu1_wakeup)) >> + cpu_relax(); >> + >> + if (!atomic_read(&cpu1_wakeup)) { >> + /* >> + * Poke cpu1 out of the boot rom >> + */ >> + __raw_writel(virt_to_phys(s3c_cpu_resume), >> + BOOT_VECTOR); >> + dsb_sev(); > > Hmm, platsmp code seems to be using an IPI to do this. Again, I wonder > if this isn't a behavior implemented only in some specific Android > bootloader. Yeah, definitively that needs clarification. >> + } >> + >> + /* >> + * Wait for cpu1 to finish booting >> + */ >> + while (!atomic_read(&cpu1_wakeup)) >> + cpu_relax(); >> + } >> + >> + return ret; >> +} >> + >> +static int exynos_powerdown_cpu1(void) >> +{ >> + int ret = -1; > > Error code? Yep, ret = cpu_pm_enter(); if (ret) goto cpu1_aborted; >> + >> + /* >> + * Idle sequence for cpu1 >> + */ >> + if (cpu_pm_enter()) >> + goto cpu1_aborted; >> + >> + /* >> + * Turn off cpu 1 >> + */ >> + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); >> + >> + ret = cpu_suspend(0, cpu_suspend_finish); >> + >> + cpu_pm_exit(); >> + >> +cpu1_aborted: >> + dsb(); >> + /* >> + * Notify cpu 0 that cpu 1 is awake >> + */ >> + atomic_set(&cpu1_wakeup, 1); >> + >> + return ret; >> +} >> + >> +static int exynos_enter_aftr(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + int ret; >> + >> + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); > > Why this is being written here, instead of some of the low level > functions above? > > Otherwise, I quite like the whole idea. I need to play a bit with CPU > hotplug and PMU to verify that things couldn't really be simplified a > bit, but in general this looks reasonably. Yes, there is a nice improvement in term of power saving for dual processor. If you can clarify the BOOT_VECTOR stuff that will be nice. I have a more recent version of the driver based on samsung-next and the cleanups I previously sent: https://git.linaro.org/people/daniel.lezcano/linux.git/ branch : cpuidle/samsung-next Thanks for the review. -- Daniel
On 04/24/2014 07:42 PM, Tomasz Figa wrote: > Hi Daniel, > > Please see my comments inline. > > Btw. Please fix your e-mail composer to properly wrap your messages > around 7xth column, as otherwise they're hard to read. > > On 04.04.2014 11:48, Daniel Lezcano wrote: >> The following driver is for exynos4210. I did not yet finished the >> other boards, so >> I created a specific driver for 4210 which could be merged later. >> >> The driver is based on Colin Cross's driver found at: >> >> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >> >> >> This one was based on a 3.4 kernel and an old API. >> >> It has been refreshed, simplified and based on the recent code cleanup >> I sent >> today. >> >> The AFTR could be entered when all the cpus (except cpu0) are down. In >> order to >> reach this situation, the couple idle states are used. >> >> There is a sync barrier at the entry and the exit of the low power >> function. So >> all cpus will enter and exit the function at the same time. >> >> At this point, CPU0 knows the other cpu will power down itself. CPU0 >> waits for >> the CPU1 to be powered down and then initiate the AFTR power down >> sequence. >> >> No interrupts are handled by CPU1, this is why we switch to the timer >> broadcast >> even if the local timer is not impacted by the idle state. >> >> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >> they both >> exit the idle function. >> >> This driver allows the exynos4210 to have the same power consumption >> at idle >> time than the one when we have to unplug CPU1 in order to let CPU0 to >> reach >> the AFTR state. >> >> This patch is a RFC because, we have to find a way to remove the macros >> definitions and cpu powerdown function without pulling the arch dependent >> headers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> arch/arm/mach-exynos/common.c | 11 +- >> drivers/cpuidle/Kconfig.arm | 8 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-exynos4210.c | 226 >> ++++++++++++++++++++++++++++++++++ [ ... ] > Otherwise, I quite like the whole idea. I need to play a bit with CPU > hotplug and PMU to verify that things couldn't really be simplified a > bit, but in general this looks reasonably. Hi Tomasz, did you have time to look at this simplification ? Thanks -- Daniel
Hi Daniel, On 30.05.2014 11:30, Daniel Lezcano wrote: > On 04/24/2014 07:42 PM, Tomasz Figa wrote: >> Hi Daniel, >> >> Please see my comments inline. >> >> Btw. Please fix your e-mail composer to properly wrap your messages >> around 7xth column, as otherwise they're hard to read. >> >> On 04.04.2014 11:48, Daniel Lezcano wrote: >>> The following driver is for exynos4210. I did not yet finished the >>> other boards, so >>> I created a specific driver for 4210 which could be merged later. >>> >>> The driver is based on Colin Cross's driver found at: >>> >>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >>> >>> >>> >>> This one was based on a 3.4 kernel and an old API. >>> >>> It has been refreshed, simplified and based on the recent code cleanup >>> I sent >>> today. >>> >>> The AFTR could be entered when all the cpus (except cpu0) are down. In >>> order to >>> reach this situation, the couple idle states are used. >>> >>> There is a sync barrier at the entry and the exit of the low power >>> function. So >>> all cpus will enter and exit the function at the same time. >>> >>> At this point, CPU0 knows the other cpu will power down itself. CPU0 >>> waits for >>> the CPU1 to be powered down and then initiate the AFTR power down >>> sequence. >>> >>> No interrupts are handled by CPU1, this is why we switch to the timer >>> broadcast >>> even if the local timer is not impacted by the idle state. >>> >>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >>> they both >>> exit the idle function. >>> >>> This driver allows the exynos4210 to have the same power consumption >>> at idle >>> time than the one when we have to unplug CPU1 in order to let CPU0 to >>> reach >>> the AFTR state. >>> >>> This patch is a RFC because, we have to find a way to remove the macros >>> definitions and cpu powerdown function without pulling the arch >>> dependent >>> headers. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >>> arch/arm/mach-exynos/common.c | 11 +- >>> drivers/cpuidle/Kconfig.arm | 8 ++ >>> drivers/cpuidle/Makefile | 1 + >>> drivers/cpuidle/cpuidle-exynos4210.c | 226 >>> ++++++++++++++++++++++++++++++++++ > > [ ... ] > > >> Otherwise, I quite like the whole idea. I need to play a bit with CPU >> hotplug and PMU to verify that things couldn't really be simplified a >> bit, but in general this looks reasonably. > > Hi Tomasz, > > did you have time to look at this simplification ? Unfortunately I got preempted with other things to do and now I'm on vacation. I worked a bit with Bart (added on CC) on this and generally the conclusion was that all the things are necessary. He was also working to extend the driver to support Exynos4x12. Bart, has anything interesting showed up since we talked about this last time? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Friday, May 30, 2014 01:34:45 PM Tomasz Figa wrote: > Hi Daniel, > > On 30.05.2014 11:30, Daniel Lezcano wrote: > > On 04/24/2014 07:42 PM, Tomasz Figa wrote: > >> Hi Daniel, > >> > >> Please see my comments inline. > >> > >> Btw. Please fix your e-mail composer to properly wrap your messages > >> around 7xth column, as otherwise they're hard to read. > >> > >> On 04.04.2014 11:48, Daniel Lezcano wrote: > >>> The following driver is for exynos4210. I did not yet finished the > >>> other boards, so > >>> I created a specific driver for 4210 which could be merged later. > >>> > >>> The driver is based on Colin Cross's driver found at: > >>> > >>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > >>> > >>> > >>> > >>> This one was based on a 3.4 kernel and an old API. > >>> > >>> It has been refreshed, simplified and based on the recent code cleanup > >>> I sent > >>> today. > >>> > >>> The AFTR could be entered when all the cpus (except cpu0) are down. In > >>> order to > >>> reach this situation, the couple idle states are used. > >>> > >>> There is a sync barrier at the entry and the exit of the low power > >>> function. So > >>> all cpus will enter and exit the function at the same time. > >>> > >>> At this point, CPU0 knows the other cpu will power down itself. CPU0 > >>> waits for > >>> the CPU1 to be powered down and then initiate the AFTR power down > >>> sequence. > >>> > >>> No interrupts are handled by CPU1, this is why we switch to the timer > >>> broadcast > >>> even if the local timer is not impacted by the idle state. > >>> > >>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then > >>> they both > >>> exit the idle function. > >>> > >>> This driver allows the exynos4210 to have the same power consumption > >>> at idle > >>> time than the one when we have to unplug CPU1 in order to let CPU0 to > >>> reach > >>> the AFTR state. > >>> > >>> This patch is a RFC because, we have to find a way to remove the macros > >>> definitions and cpu powerdown function without pulling the arch > >>> dependent > >>> headers. > >>> > >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >>> --- > >>> arch/arm/mach-exynos/common.c | 11 +- > >>> drivers/cpuidle/Kconfig.arm | 8 ++ > >>> drivers/cpuidle/Makefile | 1 + > >>> drivers/cpuidle/cpuidle-exynos4210.c | 226 > >>> ++++++++++++++++++++++++++++++++++ > > > > [ ... ] > > > > > >> Otherwise, I quite like the whole idea. I need to play a bit with CPU > >> hotplug and PMU to verify that things couldn't really be simplified a > >> bit, but in general this looks reasonably. > > > > Hi Tomasz, > > > > did you have time to look at this simplification ? > > Unfortunately I got preempted with other things to do and now I'm on > vacation. I worked a bit with Bart (added on CC) on this and generally > the conclusion was that all the things are necessary. He was also > working to extend the driver to support Exynos4x12. > > Bart, has anything interesting showed up since we talked about this last > time? Since we last looked into this I have fixed the "standard" AFTR support for Exynos3250 and started to work on adding Exynos3250 support to this driver (as Exynos3250 support has bigger priority than Exynos4x12 one). Unfortunately I also got preempted with other things so it is still unfinished and doesn't work yet. Moreover I had no possibility to test the new driver on Exynos4210 based Origen yet (I hope to do this next week). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On pi?, 2014-04-04 at 11:48 +0200, Daniel Lezcano wrote: > The following driver is for exynos4210. I did not yet finished the other boards, so > I created a specific driver for 4210 which could be merged later. > > The driver is based on Colin Cross's driver found at: > > https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > > This one was based on a 3.4 kernel and an old API. > > It has been refreshed, simplified and based on the recent code cleanup I sent > today. > > The AFTR could be entered when all the cpus (except cpu0) are down. In order to > reach this situation, the couple idle states are used. > > There is a sync barrier at the entry and the exit of the low power function. So > all cpus will enter and exit the function at the same time. > > At this point, CPU0 knows the other cpu will power down itself. CPU0 waits for > the CPU1 to be powered down and then initiate the AFTR power down sequence. > > No interrupts are handled by CPU1, this is why we switch to the timer broadcast > even if the local timer is not impacted by the idle state. > > When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then they both > exit the idle function. > > This driver allows the exynos4210 to have the same power consumption at idle > time than the one when we have to unplug CPU1 in order to let CPU0 to reach > the AFTR state. > > This patch is a RFC because, we have to find a way to remove the macros > definitions and cpu powerdown function without pulling the arch dependent > headers. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm/mach-exynos/common.c | 11 +- > drivers/cpuidle/Kconfig.arm | 8 ++ > drivers/cpuidle/Makefile | 1 + > drivers/cpuidle/cpuidle-exynos4210.c | 226 ++++++++++++++++++++++++++++++++++ > 4 files changed, 245 insertions(+), 1 deletion(-) > create mode 100644 drivers/cpuidle/cpuidle-exynos4210.c (...) > diff --git a/drivers/cpuidle/cpuidle-exynos4210.c b/drivers/cpuidle/cpuidle-exynos4210.c > new file mode 100644 > index 0000000..56f6d51 > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-exynos4210.c > @@ -0,0 +1,226 @@ > +/* > + * Copyright (c) 2014 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * Copyright (c) 2014 Linaro : Daniel Lezcano <daniel.lezcano@linaro.org> > + * http://www.linaro.org > + * > + * Based on the work of Colin Cross <ccross@android.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. > + */ > + > +#include <linux/cpuidle.h> > +#include <linux/cpu_pm.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > + > +#include <asm/proc-fns.h> > +#include <asm/suspend.h> > +#include <asm/cpuidle.h> > + > +#include <plat/pm.h> > +#include <plat/cpu.h> > +#include <plat/map-base.h> > +#include <plat/map-s5p.h> > + > +static atomic_t exynos_idle_barrier; Hi, Shouldn't the exynos_idle_barrier be initialized here? I know you sent the patch almost 2 months ago but I stomped on this while testing it on Exynos3250. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/11/2014 10:50 AM, Krzysztof Kozlowski wrote: > On pi?, 2014-04-04 at 11:48 +0200, Daniel Lezcano wrote: >> The following driver is for exynos4210. I did not yet finished the other boards, so >> I created a specific driver for 4210 which could be merged later. >> >> The driver is based on Colin Cross's driver found at: >> >> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >> >> This one was based on a 3.4 kernel and an old API. >> >> It has been refreshed, simplified and based on the recent code cleanup I sent >> today. >> >> The AFTR could be entered when all the cpus (except cpu0) are down. In order to >> reach this situation, the couple idle states are used. >> >> There is a sync barrier at the entry and the exit of the low power function. So >> all cpus will enter and exit the function at the same time. >> >> At this point, CPU0 knows the other cpu will power down itself. CPU0 waits for >> the CPU1 to be powered down and then initiate the AFTR power down sequence. >> >> No interrupts are handled by CPU1, this is why we switch to the timer broadcast >> even if the local timer is not impacted by the idle state. >> >> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then they both >> exit the idle function. >> >> This driver allows the exynos4210 to have the same power consumption at idle >> time than the one when we have to unplug CPU1 in order to let CPU0 to reach >> the AFTR state. >> >> This patch is a RFC because, we have to find a way to remove the macros >> definitions and cpu powerdown function without pulling the arch dependent >> headers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> arch/arm/mach-exynos/common.c | 11 +- >> drivers/cpuidle/Kconfig.arm | 8 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-exynos4210.c | 226 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 245 insertions(+), 1 deletion(-) >> create mode 100644 drivers/cpuidle/cpuidle-exynos4210.c > > (...) > >> diff --git a/drivers/cpuidle/cpuidle-exynos4210.c b/drivers/cpuidle/cpuidle-exynos4210.c >> new file mode 100644 >> index 0000000..56f6d51 >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-exynos4210.c >> @@ -0,0 +1,226 @@ >> +/* >> + * Copyright (c) 2014 Samsung Electronics Co., Ltd. >> + * http://www.samsung.com >> + * >> + * Copyright (c) 2014 Linaro : Daniel Lezcano <daniel.lezcano@linaro.org> >> + * http://www.linaro.org >> + * >> + * Based on the work of Colin Cross <ccross@android.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. >> + */ >> + >> +#include <linux/cpuidle.h> >> +#include <linux/cpu_pm.h> >> +#include <linux/io.h> >> +#include <linux/platform_device.h> >> + >> +#include <asm/proc-fns.h> >> +#include <asm/suspend.h> >> +#include <asm/cpuidle.h> >> + >> +#include <plat/pm.h> >> +#include <plat/cpu.h> >> +#include <plat/map-base.h> >> +#include <plat/map-s5p.h> >> + >> +static atomic_t exynos_idle_barrier; > > Hi, > > Shouldn't the exynos_idle_barrier be initialized here? As it is a static data it will be initialized to zero. > I know you sent the patch almost 2 months ago but I stomped on this > while testing it on Exynos3250. No problem. And what results on exynos3250 ? Thanks ! -- Daniel
On sob, 2014-06-14 at 00:43 +0200, Daniel Lezcano wrote: > On 06/11/2014 10:50 AM, Krzysztof Kozlowski wrote: (...) > > > > Hi, > > > > Shouldn't the exynos_idle_barrier be initialized here? > > As it is a static data it will be initialized to zero. > > > I know you sent the patch almost 2 months ago but I stomped on this > > while testing it on Exynos3250. > > No problem. And what results on exynos3250 ? I had to change some of the ways for synchronization two cores (boot vector behaves differently) so actually the driver is different except the general idea. As for the results (on dual-core Exynos3250) - the mode (AFTR+CPU1 off) consumes about 5% less energy than idle-WFI state. I expect there will be much more savings in deeper mode (called here W-AFTR). Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Friday, May 30, 2014 03:53:09 PM Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Friday, May 30, 2014 01:34:45 PM Tomasz Figa wrote: > > Hi Daniel, > > > > On 30.05.2014 11:30, Daniel Lezcano wrote: > > > On 04/24/2014 07:42 PM, Tomasz Figa wrote: > > >> Hi Daniel, > > >> > > >> Please see my comments inline. > > >> > > >> Btw. Please fix your e-mail composer to properly wrap your messages > > >> around 7xth column, as otherwise they're hard to read. > > >> > > >> On 04.04.2014 11:48, Daniel Lezcano wrote: > > >>> The following driver is for exynos4210. I did not yet finished the > > >>> other boards, so > > >>> I created a specific driver for 4210 which could be merged later. > > >>> > > >>> The driver is based on Colin Cross's driver found at: > > >>> > > >>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ > > >>> > > >>> > > >>> > > >>> This one was based on a 3.4 kernel and an old API. > > >>> > > >>> It has been refreshed, simplified and based on the recent code cleanup > > >>> I sent > > >>> today. > > >>> > > >>> The AFTR could be entered when all the cpus (except cpu0) are down. In > > >>> order to > > >>> reach this situation, the couple idle states are used. > > >>> > > >>> There is a sync barrier at the entry and the exit of the low power > > >>> function. So > > >>> all cpus will enter and exit the function at the same time. > > >>> > > >>> At this point, CPU0 knows the other cpu will power down itself. CPU0 > > >>> waits for > > >>> the CPU1 to be powered down and then initiate the AFTR power down > > >>> sequence. > > >>> > > >>> No interrupts are handled by CPU1, this is why we switch to the timer > > >>> broadcast > > >>> even if the local timer is not impacted by the idle state. > > >>> > > >>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then > > >>> they both > > >>> exit the idle function. > > >>> > > >>> This driver allows the exynos4210 to have the same power consumption > > >>> at idle > > >>> time than the one when we have to unplug CPU1 in order to let CPU0 to > > >>> reach > > >>> the AFTR state. > > >>> > > >>> This patch is a RFC because, we have to find a way to remove the macros > > >>> definitions and cpu powerdown function without pulling the arch > > >>> dependent > > >>> headers. > > >>> > > >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > >>> --- > > >>> arch/arm/mach-exynos/common.c | 11 +- > > >>> drivers/cpuidle/Kconfig.arm | 8 ++ > > >>> drivers/cpuidle/Makefile | 1 + > > >>> drivers/cpuidle/cpuidle-exynos4210.c | 226 > > >>> ++++++++++++++++++++++++++++++++++ > > > > > > [ ... ] > > > > > > > > >> Otherwise, I quite like the whole idea. I need to play a bit with CPU > > >> hotplug and PMU to verify that things couldn't really be simplified a > > >> bit, but in general this looks reasonably. > > > > > > Hi Tomasz, > > > > > > did you have time to look at this simplification ? > > > > Unfortunately I got preempted with other things to do and now I'm on > > vacation. I worked a bit with Bart (added on CC) on this and generally > > the conclusion was that all the things are necessary. He was also > > working to extend the driver to support Exynos4x12. > > > > Bart, has anything interesting showed up since we talked about this last > > time? > > Since we last looked into this I have fixed the "standard" AFTR support > for Exynos3250 and started to work on adding Exynos3250 support to this > driver (as Exynos3250 support has bigger priority than Exynos4x12 one). > Unfortunately I also got preempted with other things so it is still > unfinished and doesn't work yet. Moreover I had no possibility to test > the new driver on Exynos4210 based Origen yet (I hope to do this next > week). I have tested this patch on Origen board (Exynos4210 rev 1.1) and it causes system lockup (it seems that the code gets stuck on waiting for CPU1 to wake up). The kernels I have tried: * current -next + this patch + few fixes to bring it up to date * commit cd245f5 + this patch + some fixes * next-20140403 + your Exynos cpuidle patch series + this patch I have also tried with S5P_VA_SYSRAM replaced by S5P_INFORM5 to match Exynos 4210 rev 1.1 but it didn't help. U-Boot version used is: U-Boot 2012.07 (Sep 22 2012 - 05:12:41) for ORIGEN Could you please tell me which hardware/bootloader/kernel exactly have you used to test this patch? Also could you please port/retest your patch over the current -next? Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/16/2014 07:34 PM, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Friday, May 30, 2014 03:53:09 PM Bartlomiej Zolnierkiewicz wrote: >> >> Hi, >> >> On Friday, May 30, 2014 01:34:45 PM Tomasz Figa wrote: >>> Hi Daniel, >>> >>> On 30.05.2014 11:30, Daniel Lezcano wrote: >>>> On 04/24/2014 07:42 PM, Tomasz Figa wrote: >>>>> Hi Daniel, >>>>> >>>>> Please see my comments inline. >>>>> >>>>> Btw. Please fix your e-mail composer to properly wrap your messages >>>>> around 7xth column, as otherwise they're hard to read. >>>>> >>>>> On 04.04.2014 11:48, Daniel Lezcano wrote: >>>>>> The following driver is for exynos4210. I did not yet finished the >>>>>> other boards, so >>>>>> I created a specific driver for 4210 which could be merged later. >>>>>> >>>>>> The driver is based on Colin Cross's driver found at: >>>>>> >>>>>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >>>>>> >>>>>> >>>>>> >>>>>> This one was based on a 3.4 kernel and an old API. >>>>>> >>>>>> It has been refreshed, simplified and based on the recent code cleanup >>>>>> I sent >>>>>> today. >>>>>> >>>>>> The AFTR could be entered when all the cpus (except cpu0) are down. In >>>>>> order to >>>>>> reach this situation, the couple idle states are used. >>>>>> >>>>>> There is a sync barrier at the entry and the exit of the low power >>>>>> function. So >>>>>> all cpus will enter and exit the function at the same time. >>>>>> >>>>>> At this point, CPU0 knows the other cpu will power down itself. CPU0 >>>>>> waits for >>>>>> the CPU1 to be powered down and then initiate the AFTR power down >>>>>> sequence. >>>>>> >>>>>> No interrupts are handled by CPU1, this is why we switch to the timer >>>>>> broadcast >>>>>> even if the local timer is not impacted by the idle state. >>>>>> >>>>>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >>>>>> they both >>>>>> exit the idle function. >>>>>> >>>>>> This driver allows the exynos4210 to have the same power consumption >>>>>> at idle >>>>>> time than the one when we have to unplug CPU1 in order to let CPU0 to >>>>>> reach >>>>>> the AFTR state. >>>>>> >>>>>> This patch is a RFC because, we have to find a way to remove the macros >>>>>> definitions and cpu powerdown function without pulling the arch >>>>>> dependent >>>>>> headers. >>>>>> >>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>> --- >>>>>> arch/arm/mach-exynos/common.c | 11 +- >>>>>> drivers/cpuidle/Kconfig.arm | 8 ++ >>>>>> drivers/cpuidle/Makefile | 1 + >>>>>> drivers/cpuidle/cpuidle-exynos4210.c | 226 >>>>>> ++++++++++++++++++++++++++++++++++ >>>> >>>> [ ... ] >>>> >>>> >>>>> Otherwise, I quite like the whole idea. I need to play a bit with CPU >>>>> hotplug and PMU to verify that things couldn't really be simplified a >>>>> bit, but in general this looks reasonably. >>>> >>>> Hi Tomasz, >>>> >>>> did you have time to look at this simplification ? >>> >>> Unfortunately I got preempted with other things to do and now I'm on >>> vacation. I worked a bit with Bart (added on CC) on this and generally >>> the conclusion was that all the things are necessary. He was also >>> working to extend the driver to support Exynos4x12. >>> >>> Bart, has anything interesting showed up since we talked about this last >>> time? >> >> Since we last looked into this I have fixed the "standard" AFTR support >> for Exynos3250 and started to work on adding Exynos3250 support to this >> driver (as Exynos3250 support has bigger priority than Exynos4x12 one). >> Unfortunately I also got preempted with other things so it is still >> unfinished and doesn't work yet. Moreover I had no possibility to test >> the new driver on Exynos4210 based Origen yet (I hope to do this next >> week). > > I have tested this patch on Origen board (Exynos4210 rev 1.1) and it > causes system lockup (it seems that the code gets stuck on waiting for > CPU1 to wake up). > > The kernels I have tried: > * current -next + this patch + few fixes to bring it up to date > * commit cd245f5 + this patch + some fixes > * next-20140403 + your Exynos cpuidle patch series + this patch > > I have also tried with S5P_VA_SYSRAM replaced by S5P_INFORM5 to > match Exynos 4210 rev 1.1 but it didn't help. > > U-Boot version used is: > U-Boot 2012.07 (Sep 22 2012 - 05:12:41) for ORIGEN > > Could you please tell me which hardware/bootloader/kernel exactly > have you used to test this patch? When I used it, it was on top of 3.15-rc1: https://git.linaro.org/people/daniel.lezcano/linux.git/shortlog/refs/heads/cpuidle/samsung-next The hardware was a exynos-4210 origen board ver A. > Also could you please port/retest your patch over the current -next? Will do that in my free time after unstacking my emails after 2 weeks of vacations :)
diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c index d5fa21e..1765a98 100644 --- a/arch/arm/mach-exynos/common.c +++ b/arch/arm/mach-exynos/common.c @@ -299,9 +299,18 @@ static struct platform_device exynos_cpuidle = { .id = -1, }; +static struct platform_device exynos4210_cpuidle = { + .name = "exynos4210-cpuidle", + .dev.platform_data = exynos_sys_powerdown_aftr, + .id = -1, +}; + void __init exynos_cpuidle_init(void) { - platform_device_register(&exynos_cpuidle); + if (soc_is_exynos4210()) + platform_device_register(&exynos4210_cpuidle); + else + platform_device_register(&exynos_cpuidle); } void __init exynos_cpufreq_init(void) diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm index 92f0c12..2772130 100644 --- a/drivers/cpuidle/Kconfig.arm +++ b/drivers/cpuidle/Kconfig.arm @@ -51,3 +51,11 @@ config ARM_EXYNOS_CPUIDLE depends on ARCH_EXYNOS help Select this to enable cpuidle for Exynos processors + +config ARM_EXYNOS4210_CPUIDLE + bool "Cpu Idle Driver for the Exynos 4210 processor" + default y + depends on ARCH_EXYNOS + select ARCH_NEEDS_CPU_IDLE_COUPLED if SMP + help + Select this to enable cpuidle for the Exynos 4210 processors diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index 0d1540a..e0ec9bc 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE) += cpuidle-zynq.o obj-$(CONFIG_ARM_U8500_CPUIDLE) += cpuidle-ux500.o obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) += cpuidle-exynos.o +obj-$(CONFIG_ARM_EXYNOS4210_CPUIDLE) += cpuidle-exynos4210.o ############################################################################### # POWERPC drivers diff --git a/drivers/cpuidle/cpuidle-exynos4210.c b/drivers/cpuidle/cpuidle-exynos4210.c new file mode 100644 index 0000000..56f6d51 --- /dev/null +++ b/drivers/cpuidle/cpuidle-exynos4210.c @@ -0,0 +1,226 @@ +/* + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * + * Copyright (c) 2014 Linaro : Daniel Lezcano <daniel.lezcano@linaro.org> + * http://www.linaro.org + * + * Based on the work of Colin Cross <ccross@android.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. + */ + +#include <linux/cpuidle.h> +#include <linux/cpu_pm.h> +#include <linux/io.h> +#include <linux/platform_device.h> + +#include <asm/proc-fns.h> +#include <asm/suspend.h> +#include <asm/cpuidle.h> + +#include <plat/pm.h> +#include <plat/cpu.h> +#include <plat/map-base.h> +#include <plat/map-s5p.h> + +static atomic_t exynos_idle_barrier; +static atomic_t cpu1_wakeup = ATOMIC_INIT(0); + +#define BOOT_VECTOR S5P_VA_SYSRAM +#define S5P_VA_PMU S3C_ADDR(0x02180000) +#define S5P_PMUREG(x) (S5P_VA_PMU + (x)) +#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) +#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) + +static void (*exynos_aftr)(void); + +static int cpu_suspend_finish(unsigned long flags) +{ + if (flags) + exynos_aftr(); + + cpu_do_idle(); + + return -1; +} + +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 (__raw_readl(S5P_ARM_CORE1_STATUS) & 3) { + + /* + * 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(BOOT_VECTOR) == 0) + goto abort; + + cpu_relax(); + } + } + + cpu_pm_enter(); + + ret = cpu_suspend(1, cpu_suspend_finish); + + cpu_pm_exit(); + +abort: + if (cpu_online(1)) { + /* + * Set the boot vector to something non-zero + */ + __raw_writel(virt_to_phys(s3c_cpu_resume), + BOOT_VECTOR); + dsb(); + + /* + * Turn on cpu1 and wait for it to be on + */ + __raw_writel(0x3, S5P_ARM_CORE1_CONFIGURATION); + while ((__raw_readl(S5P_ARM_CORE1_STATUS) & 3) != 3) + cpu_relax(); + + /* + * Wait for cpu1 to get stuck in the boot rom + */ + while ((__raw_readl(BOOT_VECTOR) != 0) && + !atomic_read(&cpu1_wakeup)) + cpu_relax(); + + if (!atomic_read(&cpu1_wakeup)) { + /* + * Poke cpu1 out of the boot rom + */ + __raw_writel(virt_to_phys(s3c_cpu_resume), + BOOT_VECTOR); + dsb_sev(); + } + + /* + * Wait for cpu1 to finish booting + */ + while (!atomic_read(&cpu1_wakeup)) + cpu_relax(); + } + + return ret; +} + +static int exynos_powerdown_cpu1(void) +{ + int ret = -1; + + /* + * Idle sequence for cpu1 + */ + if (cpu_pm_enter()) + goto cpu1_aborted; + + /* + * Turn off cpu 1 + */ + __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION); + + ret = cpu_suspend(0, cpu_suspend_finish); + + cpu_pm_exit(); + +cpu1_aborted: + dsb(); + /* + * Notify cpu 0 that cpu 1 is awake + */ + atomic_set(&cpu1_wakeup, 1); + + return ret; +} + +static int exynos_enter_aftr(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + int ret; + + __raw_writel(virt_to_phys(s3c_cpu_resume), BOOT_VECTOR); + + /* + * 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_powerdown_cpu1() : exynos_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); + + atomic_set(&cpu1_wakeup, 0); + + return index; +} + +static struct cpuidle_driver exynos_idle_driver = { + .name = "exynos4210_idle", + .owner = THIS_MODULE, + .states = { + ARM_CPUIDLE_WFI_STATE, + [1] = { + .enter = exynos_enter_aftr, + .exit_latency = 5000, + .target_residency = 10000, + .flags = CPUIDLE_FLAG_TIME_VALID | + 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) +{ + exynos_aftr = (void *)(pdev->dev.platform_data); + + return cpuidle_register(&exynos_idle_driver, cpu_possible_mask); +} + +static struct platform_driver exynos_cpuidle_driver = { + .driver = { + .name = "exynos4210-cpuidle", + .owner = THIS_MODULE, + }, + .probe = exynos_cpuidle_probe, +}; + +module_platform_driver(exynos_cpuidle_driver);
The following driver is for exynos4210. I did not yet finished the other boards, so I created a specific driver for 4210 which could be merged later. The driver is based on Colin Cross's driver found at: https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ This one was based on a 3.4 kernel and an old API. It has been refreshed, simplified and based on the recent code cleanup I sent today. The AFTR could be entered when all the cpus (except cpu0) are down. In order to reach this situation, the couple idle states are used. There is a sync barrier at the entry and the exit of the low power function. So all cpus will enter and exit the function at the same time. At this point, CPU0 knows the other cpu will power down itself. CPU0 waits for the CPU1 to be powered down and then initiate the AFTR power down sequence. No interrupts are handled by CPU1, this is why we switch to the timer broadcast even if the local timer is not impacted by the idle state. When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then they both exit the idle function. This driver allows the exynos4210 to have the same power consumption at idle time than the one when we have to unplug CPU1 in order to let CPU0 to reach the AFTR state. This patch is a RFC because, we have to find a way to remove the macros definitions and cpu powerdown function without pulling the arch dependent headers. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- arch/arm/mach-exynos/common.c | 11 +- drivers/cpuidle/Kconfig.arm | 8 ++ drivers/cpuidle/Makefile | 1 + drivers/cpuidle/cpuidle-exynos4210.c | 226 ++++++++++++++++++++++++++++++++++ 4 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 drivers/cpuidle/cpuidle-exynos4210.c