| Message ID | 1396341234-40515-1-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On 04/01/2014 10:33 AM, Dongsheng Wang wrote: > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > Add cpuidle support for e500 family, using cpuidle framework to > manage various low power modes. The new implementation will remain > compatible with original idle method. > > I have done test about power consumption and latency. Cpuidle framework > will make CPU response time faster than original method, but power > consumption is higher than original method. > > Power consumption: > The original method, power consumption is 10.51202 (W). > The cpuidle framework, power consumption is 10.5311 (W). > > Latency: > The original method, avg latency is 6782 (us). > The cpuidle framework, avg latency is 6482 (us). > > Initially, this supports PW10, PW20 and subsequent patches will support > DOZE/NAP and PH10, PH20. > > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> > > diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h > index 5b6c03f..9301420 100644 > --- a/arch/powerpc/include/asm/machdep.h > +++ b/arch/powerpc/include/asm/machdep.h > @@ -294,6 +294,15 @@ extern void power7_idle(void); > extern void ppc6xx_idle(void); > extern void book3e_idle(void); > > +static inline void cpuidle_wait(void) > +{ > +#ifdef CONFIG_PPC64 > + book3e_idle(); > +#else > + e500_idle(); > +#endif > +} > + > /* > * ppc_md contains a copy of the machine description structure for the > * current platform. machine_id contains the initial address where the > diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c > index 97e1dc9..edd193f 100644 > --- a/arch/powerpc/kernel/sysfs.c > +++ b/arch/powerpc/kernel/sysfs.c > @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device *dev, > return sprintf(buf, "%llu\n", time > 0 ? time : 0); > } > > +#ifdef CONFIG_CPU_IDLE_E500 > +u32 cpuidle_entry_bit; > +#endif > static void set_pw20_wait_entry_bit(void *val) > { > u32 *value = val; > @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val) > /* set count */ > pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT); > > +#ifdef CONFIG_CPU_IDLE_E500 > + cpuidle_entry_bit = *value; > +#else > mtspr(SPRN_PWRMGTCR0, pw20_idle); > +#endif > } > > static ssize_t store_pw20_wait_time(struct device *dev, > diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc > index 66c3a09..0949dbf 100644 > --- a/drivers/cpuidle/Kconfig.powerpc > +++ b/drivers/cpuidle/Kconfig.powerpc > @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE > help > Select this option to enable processor idle state management > through cpuidle subsystem. > + > +config CPU_IDLE_E500 > + bool "CPU Idle Driver for E500 family processors" > + depends on CPU_IDLE > + depends on FSL_SOC_BOOKE > + help > + Select this to enable cpuidle on e500 family processors. > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index f71ae1b..7e6adea 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o > # POWERPC drivers > obj-$(CONFIG_PSERIES_CPUIDLE) += cpuidle-pseries.o > obj-$(CONFIG_POWERNV_CPUIDLE) += cpuidle-powernv.o > +obj-$(CONFIG_CPU_IDLE_E500) += cpuidle-e500.o > diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c > new file mode 100644 > index 0000000..ddc0def > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-e500.c > @@ -0,0 +1,194 @@ > +/* > + * CPU Idle driver for Freescale PowerPC e500 family processors. > + * > + * Copyright 2014 Freescale Semiconductor, Inc. > + * > + * Author: Dongsheng Wang <dongsheng.wang@freescale.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/cpu.h> > +#include <linux/cpuidle.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/notifier.h> > + > +#include <asm/cputable.h> > +#include <asm/machdep.h> > +#include <asm/mpc85xx.h> > + > +static unsigned int max_idle_state; > +static struct cpuidle_state *cpuidle_state_table; > + > +struct cpuidle_driver e500_idle_driver = { > + .name = "e500_idle", > + .owner = THIS_MODULE, > +}; > + > +static void e500_cpuidle(void) > +{ > + if (cpuidle_idle_call()) > + cpuidle_wait(); > +} Nope, that has been changed. No more call to cpuidle_idle_call in a driver. > + > +static int pw10_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + cpuidle_wait(); > + return index; > +} > + > +#define MAX_BIT 63 > +#define MIN_BIT 1 > +extern u32 cpuidle_entry_bit; > +static int pw20_enter(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + u32 pw20_idle; > + u32 entry_bit; > + pw20_idle = mfspr(SPRN_PWRMGTCR0); > + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { > + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > + entry_bit = MAX_BIT - cpuidle_entry_bit; > + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); > + mtspr(SPRN_PWRMGTCR0, pw20_idle); > + } > + > + cpuidle_wait(); > + > + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); > + mtspr(SPRN_PWRMGTCR0, pw20_idle); > + > + return index; > +} Is it possible to give some comments and encapsulate the code with explicit function names to be implemented in an arch specific directory file (eg. pm.c) and export these functions in a linux/ header ? We try to prevent to include asm if possible. > + > +static struct cpuidle_state pw_idle_states[] = { > + { > + .name = "pw10", > + .desc = "pw10", > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .exit_latency = 0, > + .target_residency = 0, > + .enter = &pw10_enter > + }, > + > + { > + .name = "pw20", > + .desc = "pw20-core-idle", > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .exit_latency = 1, > + .target_residency = 50, > + .enter = &pw20_enter > + }, > +}; No need to define this intermediate structure here, you can directly initialize the cpuidle_driver: struct cpuidle_driver e500_idle_driver = { .name = "e500_idle", .owner = THIS_MODULE, .states = { .name = "pw10", .desc = "pw10", .flags = CPUIDLE_FLAG_TIME_VALID, .target_residency = 0, .enter = &pw10_enter, }, .... .state_count = 2, }; Then in the init function you initialize the state_count consequently: if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500) drv->state_count = 1; Then you can kill: max_idle_state, cpuidle_state_table, e500_idle_state_probe and pw_idle_states. > + > +static int cpu_hotplug_notify(struct notifier_block *n, > + unsigned long action, void *hcpu) > +{ > + unsigned long hotcpu = (unsigned long)hcpu; > + struct cpuidle_device *dev = > + per_cpu_ptr(cpuidle_devices, hotcpu); > + > + if (dev && cpuidle_get_driver()) { > + switch (action) { > + case CPU_ONLINE: > + case CPU_ONLINE_FROZEN: > + cpuidle_pause_and_lock(); > + cpuidle_enable_device(dev); > + cpuidle_resume_and_unlock(); > + break; > + > + case CPU_DEAD: > + case CPU_DEAD_FROZEN: > + cpuidle_pause_and_lock(); > + cpuidle_disable_device(dev); > + cpuidle_resume_and_unlock(); > + break; > + > + default: > + return NOTIFY_DONE; > + } > + } > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block cpu_hotplug_notifier = { > + .notifier_call = cpu_hotplug_notify, > +}; Can you explain why this is needed ? > +static void e500_cpuidle_driver_init(void) > +{ > + int idle_state; > + struct cpuidle_driver *drv = &e500_idle_driver; Pass the cpuidle_driver as parameter to the function. > + > + drv->state_count = 0; > + > + for (idle_state = 0; idle_state < max_idle_state; ++idle_state) { > + if (!cpuidle_state_table[idle_state].enter) > + break; > + > + drv->states[drv->state_count] = cpuidle_state_table[idle_state]; > + drv->state_count++; > + } This code should disappear. As this function will just initialize state_count, you can move it in caller and kill this function. > +} > + > +static int e500_idle_state_probe(void) > +{ > + if (cpuidle_disable != IDLE_NO_OVERRIDE) > + return -ENODEV; > + > + cpuidle_state_table = pw_idle_states; > + max_idle_state = ARRAY_SIZE(pw_idle_states); > + > + /* Disable PW20 feature for e500mc, e5500 */ > + if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500) > + cpuidle_state_table[1].enter = NULL; > + > + if (!cpuidle_state_table || !max_idle_state) > + return -ENODEV; > + > + return 0; > +} This code should disappear. > +static void replace_orig_idle(void *dummy) > +{ > + return; > +} > + > +static int __init e500_idle_init(void) > +{ > + struct cpuidle_driver *drv = &e500_idle_driver; > + int err; > + > + if (e500_idle_state_probe()) > + return -ENODEV; > + > + e500_cpuidle_driver_init(); > + if (!drv->state_count) > + return -ENODEV; No need of this check, because: 1. you know how you initialized the driver (1 or 2 states) 2. this is already by the cpuidle framework > + > + err = cpuidle_register(drv, NULL); > + if (err) { > + pr_err("Register e500 family cpuidle driver failed.\n"); extra carriage return. > + > + return err; > + } > + > + err = register_cpu_notifier(&cpu_hotplug_notifier); > + if (err) > + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); > + > + /* Replace the original way of idle after cpuidle registered. */ > + ppc_md.power_save = e500_cpuidle; > + on_each_cpu(replace_orig_idle, NULL, 1); Why ? > + pr_info("e500_idle_driver registered.\n"); > + > + return 0; > +} > +late_initcall(e500_idle_init); > Thanks -- Daniel
On 04/02/2014 11:36 AM, Daniel Lezcano wrote: > On 04/01/2014 10:33 AM, Dongsheng Wang wrote: >> From: Wang Dongsheng <dongsheng.wang@freescale.com> >> >> Add cpuidle support for e500 family, using cpuidle framework to >> manage various low power modes. The new implementation will remain >> compatible with original idle method. >> >> I have done test about power consumption and latency. Cpuidle framework >> will make CPU response time faster than original method, but power >> consumption is higher than original method. >> >> Power consumption: >> The original method, power consumption is 10.51202 (W). >> The cpuidle framework, power consumption is 10.5311 (W). >> >> Latency: >> The original method, avg latency is 6782 (us). >> The cpuidle framework, avg latency is 6482 (us). >> >> Initially, this supports PW10, PW20 and subsequent patches will support >> DOZE/NAP and PH10, PH20. >> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com> Please fix Rafael's email when resending/answering. Thanks -- Daniel >> diff --git a/arch/powerpc/include/asm/machdep.h >> b/arch/powerpc/include/asm/machdep.h >> index 5b6c03f..9301420 100644 >> --- a/arch/powerpc/include/asm/machdep.h >> +++ b/arch/powerpc/include/asm/machdep.h >> @@ -294,6 +294,15 @@ extern void power7_idle(void); >> extern void ppc6xx_idle(void); >> extern void book3e_idle(void); >> >> +static inline void cpuidle_wait(void) >> +{ >> +#ifdef CONFIG_PPC64 >> + book3e_idle(); >> +#else >> + e500_idle(); >> +#endif >> +} >> + >> /* >> * ppc_md contains a copy of the machine description structure for the >> * current platform. machine_id contains the initial address where the >> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c >> index 97e1dc9..edd193f 100644 >> --- a/arch/powerpc/kernel/sysfs.c >> +++ b/arch/powerpc/kernel/sysfs.c >> @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device >> *dev, >> return sprintf(buf, "%llu\n", time > 0 ? time : 0); >> } >> >> +#ifdef CONFIG_CPU_IDLE_E500 >> +u32 cpuidle_entry_bit; >> +#endif >> static void set_pw20_wait_entry_bit(void *val) >> { >> u32 *value = val; >> @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val) >> /* set count */ >> pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT); >> >> +#ifdef CONFIG_CPU_IDLE_E500 >> + cpuidle_entry_bit = *value; >> +#else >> mtspr(SPRN_PWRMGTCR0, pw20_idle); >> +#endif >> } >> >> static ssize_t store_pw20_wait_time(struct device *dev, >> diff --git a/drivers/cpuidle/Kconfig.powerpc >> b/drivers/cpuidle/Kconfig.powerpc >> index 66c3a09..0949dbf 100644 >> --- a/drivers/cpuidle/Kconfig.powerpc >> +++ b/drivers/cpuidle/Kconfig.powerpc >> @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE >> help >> Select this option to enable processor idle state management >> through cpuidle subsystem. >> + >> +config CPU_IDLE_E500 >> + bool "CPU Idle Driver for E500 family processors" >> + depends on CPU_IDLE >> + depends on FSL_SOC_BOOKE >> + help >> + Select this to enable cpuidle on e500 family processors. >> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile >> index f71ae1b..7e6adea 100644 >> --- a/drivers/cpuidle/Makefile >> +++ b/drivers/cpuidle/Makefile >> @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE) += >> cpuidle-at91.o >> # POWERPC drivers >> obj-$(CONFIG_PSERIES_CPUIDLE) += cpuidle-pseries.o >> obj-$(CONFIG_POWERNV_CPUIDLE) += cpuidle-powernv.o >> +obj-$(CONFIG_CPU_IDLE_E500) += cpuidle-e500.o >> diff --git a/drivers/cpuidle/cpuidle-e500.c >> b/drivers/cpuidle/cpuidle-e500.c >> new file mode 100644 >> index 0000000..ddc0def >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-e500.c >> @@ -0,0 +1,194 @@ >> +/* >> + * CPU Idle driver for Freescale PowerPC e500 family processors. >> + * >> + * Copyright 2014 Freescale Semiconductor, Inc. >> + * >> + * Author: Dongsheng Wang <dongsheng.wang@freescale.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/cpu.h> >> +#include <linux/cpuidle.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/notifier.h> >> + >> +#include <asm/cputable.h> >> +#include <asm/machdep.h> >> +#include <asm/mpc85xx.h> >> + >> +static unsigned int max_idle_state; >> +static struct cpuidle_state *cpuidle_state_table; >> + >> +struct cpuidle_driver e500_idle_driver = { >> + .name = "e500_idle", >> + .owner = THIS_MODULE, >> +}; >> + >> +static void e500_cpuidle(void) >> +{ >> + if (cpuidle_idle_call()) >> + cpuidle_wait(); >> +} > > Nope, that has been changed. No more call to cpuidle_idle_call in a driver. > >> + >> +static int pw10_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + cpuidle_wait(); >> + return index; >> +} >> + >> +#define MAX_BIT 63 >> +#define MIN_BIT 1 >> +extern u32 cpuidle_entry_bit; >> +static int pw20_enter(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + u32 pw20_idle; >> + u32 entry_bit; >> + pw20_idle = mfspr(SPRN_PWRMGTCR0); >> + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { >> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >> + entry_bit = MAX_BIT - cpuidle_entry_bit; >> + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); >> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >> + } >> + >> + cpuidle_wait(); >> + >> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >> + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); >> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >> + >> + return index; >> +} > > Is it possible to give some comments and encapsulate the code with > explicit function names to be implemented in an arch specific directory > file (eg. pm.c) and export these functions in a linux/ header ? We try > to prevent to include asm if possible. > >> + >> +static struct cpuidle_state pw_idle_states[] = { >> + { >> + .name = "pw10", >> + .desc = "pw10", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 0, >> + .target_residency = 0, >> + .enter = &pw10_enter >> + }, >> + >> + { >> + .name = "pw20", >> + .desc = "pw20-core-idle", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 1, >> + .target_residency = 50, >> + .enter = &pw20_enter >> + }, >> +}; > > No need to define this intermediate structure here, you can directly > initialize the cpuidle_driver: > > > struct cpuidle_driver e500_idle_driver = { > .name = "e500_idle", > .owner = THIS_MODULE, > .states = { > .name = "pw10", > .desc = "pw10", > .flags = CPUIDLE_FLAG_TIME_VALID, > .target_residency = 0, > .enter = &pw10_enter, > }, > > .... > > .state_count = 2, > }; > > Then in the init function you initialize the state_count consequently: > > if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500) > drv->state_count = 1; > > Then you can kill: > > max_idle_state, cpuidle_state_table, e500_idle_state_probe and > pw_idle_states. > >> + >> +static int cpu_hotplug_notify(struct notifier_block *n, >> + unsigned long action, void *hcpu) >> +{ >> + unsigned long hotcpu = (unsigned long)hcpu; >> + struct cpuidle_device *dev = >> + per_cpu_ptr(cpuidle_devices, hotcpu); >> + >> + if (dev && cpuidle_get_driver()) { >> + switch (action) { >> + case CPU_ONLINE: >> + case CPU_ONLINE_FROZEN: >> + cpuidle_pause_and_lock(); >> + cpuidle_enable_device(dev); >> + cpuidle_resume_and_unlock(); >> + break; >> + >> + case CPU_DEAD: >> + case CPU_DEAD_FROZEN: >> + cpuidle_pause_and_lock(); >> + cpuidle_disable_device(dev); >> + cpuidle_resume_and_unlock(); >> + break; >> + >> + default: >> + return NOTIFY_DONE; >> + } >> + } >> + >> + return NOTIFY_OK; >> +} >> + >> +static struct notifier_block cpu_hotplug_notifier = { >> + .notifier_call = cpu_hotplug_notify, >> +}; > > Can you explain why this is needed ? > >> +static void e500_cpuidle_driver_init(void) >> +{ >> + int idle_state; >> + struct cpuidle_driver *drv = &e500_idle_driver; > > Pass the cpuidle_driver as parameter to the function. > >> + >> + drv->state_count = 0; >> + >> + for (idle_state = 0; idle_state < max_idle_state; ++idle_state) { >> + if (!cpuidle_state_table[idle_state].enter) >> + break; >> + >> + drv->states[drv->state_count] = cpuidle_state_table[idle_state]; >> + drv->state_count++; >> + } > > This code should disappear. > > As this function will just initialize state_count, you can move it in > caller and kill this function. > >> +} >> + >> +static int e500_idle_state_probe(void) >> +{ >> + if (cpuidle_disable != IDLE_NO_OVERRIDE) >> + return -ENODEV; >> + >> + cpuidle_state_table = pw_idle_states; >> + max_idle_state = ARRAY_SIZE(pw_idle_states); >> + >> + /* Disable PW20 feature for e500mc, e5500 */ >> + if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500) >> + cpuidle_state_table[1].enter = NULL; >> + >> + if (!cpuidle_state_table || !max_idle_state) >> + return -ENODEV; >> + >> + return 0; >> +} > > This code should disappear. > >> +static void replace_orig_idle(void *dummy) >> +{ >> + return; >> +} >> + >> +static int __init e500_idle_init(void) >> +{ >> + struct cpuidle_driver *drv = &e500_idle_driver; >> + int err; >> + >> + if (e500_idle_state_probe()) >> + return -ENODEV; >> + >> + e500_cpuidle_driver_init(); >> + if (!drv->state_count) >> + return -ENODEV; > > No need of this check, because: > > 1. you know how you initialized the driver (1 or 2 states) > 2. this is already by the cpuidle framework > >> + >> + err = cpuidle_register(drv, NULL); >> + if (err) { >> + pr_err("Register e500 family cpuidle driver failed.\n"); > > extra carriage return. >> + >> + return err; >> + } >> + >> + err = register_cpu_notifier(&cpu_hotplug_notifier); >> + if (err) >> + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); >> + >> + /* Replace the original way of idle after cpuidle registered. */ >> + ppc_md.power_save = e500_cpuidle; >> + on_each_cpu(replace_orig_idle, NULL, 1); > > Why ? > >> + pr_info("e500_idle_driver registered.\n"); >> + >> + return 0; >> +} >> +late_initcall(e500_idle_init); >> > > Thanks > > -- Daniel > >
Hi Daniel, Thanks for your review. I will fix your comments. BTW, fix Rafael's email. :) > > +#include <linux/cpu.h> > > +#include <linux/cpuidle.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/notifier.h> > > + > > +#include <asm/cputable.h> > > +#include <asm/machdep.h> > > +#include <asm/mpc85xx.h> > > + > > +static unsigned int max_idle_state; > > +static struct cpuidle_state *cpuidle_state_table; > > + > > +struct cpuidle_driver e500_idle_driver = { > > + .name = "e500_idle", > > + .owner = THIS_MODULE, > > +}; > > + > > +static void e500_cpuidle(void) > > +{ > > + if (cpuidle_idle_call()) > > + cpuidle_wait(); > > +} > > Nope, that has been changed. No more call to cpuidle_idle_call in a driver. > Thanks. > > + > > +static int pw10_enter(struct cpuidle_device *dev, > > + struct cpuidle_driver *drv, int index) > > +{ > > + cpuidle_wait(); > > + return index; > > +} > > + > > +#define MAX_BIT 63 > > +#define MIN_BIT 1 > > +extern u32 cpuidle_entry_bit; > > +static int pw20_enter(struct cpuidle_device *dev, > > + struct cpuidle_driver *drv, int index) > > +{ > > + u32 pw20_idle; > > + u32 entry_bit; > > + pw20_idle = mfspr(SPRN_PWRMGTCR0); > > + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { > > + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > > + entry_bit = MAX_BIT - cpuidle_entry_bit; > > + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); > > + mtspr(SPRN_PWRMGTCR0, pw20_idle); > > + } > > + > > + cpuidle_wait(); > > + > > + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > > + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); > > + mtspr(SPRN_PWRMGTCR0, pw20_idle); > > + > > + return index; > > +} > > Is it possible to give some comments and encapsulate the code with > explicit function names to be implemented in an arch specific directory > file (eg. pm.c) and export these functions in a linux/ header ? We try > to prevent to include asm if possible. > Yep, Looks better. Thanks. > > + > > +static struct cpuidle_state pw_idle_states[] = { > > + { > > + .name = "pw10", > > + .desc = "pw10", > > + .flags = CPUIDLE_FLAG_TIME_VALID, > > + .exit_latency = 0, > > + .target_residency = 0, > > + .enter = &pw10_enter > > + }, > > + > > + { > > + .name = "pw20", > > + .desc = "pw20-core-idle", > > + .flags = CPUIDLE_FLAG_TIME_VALID, > > + .exit_latency = 1, > > + .target_residency = 50, > > + .enter = &pw20_enter > > + }, > > +}; > > No need to define this intermediate structure here, you can directly > initialize the cpuidle_driver: > Thanks. :) > > +static int cpu_hotplug_notify(struct notifier_block *n, > > + unsigned long action, void *hcpu) > > +{ > > + unsigned long hotcpu = (unsigned long)hcpu; > > + struct cpuidle_device *dev = > > + per_cpu_ptr(cpuidle_devices, hotcpu); > > + > > + if (dev && cpuidle_get_driver()) { > > + switch (action) { > > + case CPU_ONLINE: > > + case CPU_ONLINE_FROZEN: > > + cpuidle_pause_and_lock(); > > + cpuidle_enable_device(dev); > > + cpuidle_resume_and_unlock(); > > + break; > > + > > + case CPU_DEAD: > > + case CPU_DEAD_FROZEN: > > + cpuidle_pause_and_lock(); > > + cpuidle_disable_device(dev); > > + cpuidle_resume_and_unlock(); > > + break; > > + > > + default: > > + return NOTIFY_DONE; > > + } > > + } > > + > > + return NOTIFY_OK; > > +} > > + > > +static struct notifier_block cpu_hotplug_notifier = { > > + .notifier_call = cpu_hotplug_notify, > > +}; > > Can you explain why this is needed ? > If a cpu will be plugged out/in, We should be let Cpuidle know to remove corresponding sys interface and disable/enable cpudile-governor for current cpu. > > + err = register_cpu_notifier(&cpu_hotplug_notifier); > > + if (err) > > + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); > > + > > + /* Replace the original way of idle after cpuidle registered. */ > > + ppc_md.power_save = e500_cpuidle; > > + on_each_cpu(replace_orig_idle, NULL, 1); > > Why ? > I checked again, if we put cpuidle_idle_call in asm, this is not need. Regards, -Dongsheng > > + pr_info("e500_idle_driver registered.\n"); > > + > > + return 0; > > +} > > +late_initcall(e500_idle_init); > > > > Thanks > > -- Daniel > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > >
On 04/03/2014 05:20 AM, Dongsheng.Wang@freescale.com wrote: > Hi Daniel, > > Thanks for your review. I will fix your comments. > > BTW, fix Rafael's email. :) > >>> +#include <linux/cpu.h> >>> +#include <linux/cpuidle.h> >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/notifier.h> >>> + >>> +#include <asm/cputable.h> >>> +#include <asm/machdep.h> >>> +#include <asm/mpc85xx.h> >>> + >>> +static unsigned int max_idle_state; >>> +static struct cpuidle_state *cpuidle_state_table; >>> + >>> +struct cpuidle_driver e500_idle_driver = { >>> + .name = "e500_idle", >>> + .owner = THIS_MODULE, >>> +}; >>> + >>> +static void e500_cpuidle(void) >>> +{ >>> + if (cpuidle_idle_call()) >>> + cpuidle_wait(); >>> +} >> >> Nope, that has been changed. No more call to cpuidle_idle_call in a driver. >> > > Thanks. > >>> + >>> +static int pw10_enter(struct cpuidle_device *dev, >>> + struct cpuidle_driver *drv, int index) >>> +{ >>> + cpuidle_wait(); >>> + return index; >>> +} >>> + >>> +#define MAX_BIT 63 >>> +#define MIN_BIT 1 >>> +extern u32 cpuidle_entry_bit; >>> +static int pw20_enter(struct cpuidle_device *dev, >>> + struct cpuidle_driver *drv, int index) >>> +{ >>> + u32 pw20_idle; >>> + u32 entry_bit; >>> + pw20_idle = mfspr(SPRN_PWRMGTCR0); >>> + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { >>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >>> + entry_bit = MAX_BIT - cpuidle_entry_bit; >>> + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); >>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >>> + } >>> + >>> + cpuidle_wait(); >>> + >>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >>> + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); >>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >>> + >>> + return index; >>> +} >> >> Is it possible to give some comments and encapsulate the code with >> explicit function names to be implemented in an arch specific directory >> file (eg. pm.c) and export these functions in a linux/ header ? We try >> to prevent to include asm if possible. >> > > Yep, Looks better. Thanks. > >>> + >>> +static struct cpuidle_state pw_idle_states[] = { >>> + { >>> + .name = "pw10", >>> + .desc = "pw10", >>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>> + .exit_latency = 0, >>> + .target_residency = 0, >>> + .enter = &pw10_enter >>> + }, >>> + >>> + { >>> + .name = "pw20", >>> + .desc = "pw20-core-idle", >>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>> + .exit_latency = 1, >>> + .target_residency = 50, >>> + .enter = &pw20_enter >>> + }, >>> +}; >> >> No need to define this intermediate structure here, you can directly >> initialize the cpuidle_driver: >> > > Thanks. :) > >>> +static int cpu_hotplug_notify(struct notifier_block *n, >>> + unsigned long action, void *hcpu) >>> +{ >>> + unsigned long hotcpu = (unsigned long)hcpu; >>> + struct cpuidle_device *dev = >>> + per_cpu_ptr(cpuidle_devices, hotcpu); >>> + >>> + if (dev && cpuidle_get_driver()) { >>> + switch (action) { >>> + case CPU_ONLINE: >>> + case CPU_ONLINE_FROZEN: >>> + cpuidle_pause_and_lock(); >>> + cpuidle_enable_device(dev); >>> + cpuidle_resume_and_unlock(); >>> + break; >>> + >>> + case CPU_DEAD: >>> + case CPU_DEAD_FROZEN: >>> + cpuidle_pause_and_lock(); >>> + cpuidle_disable_device(dev); >>> + cpuidle_resume_and_unlock(); >>> + break; >>> + >>> + default: >>> + return NOTIFY_DONE; >>> + } >>> + } >>> + >>> + return NOTIFY_OK; >>> +} >>> + >>> +static struct notifier_block cpu_hotplug_notifier = { >>> + .notifier_call = cpu_hotplug_notify, >>> +}; >> >> Can you explain why this is needed ? >> > > If a cpu will be plugged out/in, We should be let Cpuidle know to remove > corresponding sys interface and disable/enable cpudile-governor for current cpu. Ok, this code is a copy-paste of the powers' cpuidle driver. IIRC, I posted a patchset to move this portion of code in the cpuidle common framework some time ago. Could you please get rid of this part of code ? >>> + err = register_cpu_notifier(&cpu_hotplug_notifier); >>> + if (err) >>> + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); >>> + >>> + /* Replace the original way of idle after cpuidle registered. */ >>> + ppc_md.power_save = e500_cpuidle; >>> + on_each_cpu(replace_orig_idle, NULL, 1); >> >> Why ? >> > > I checked again, if we put cpuidle_idle_call in asm, this is not need. > > Regards, > -Dongsheng > >>> + pr_info("e500_idle_driver registered.\n"); >>> + >>> + return 0; >>> +} >>> +late_initcall(e500_idle_init); >>> >> >> Thanks >> >> -- Daniel >> >> >> -- >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | >> <http://www.linaro.org/linaro-blog/> Blog >> >> >
> -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] > Sent: Thursday, April 03, 2014 2:29 PM > To: Wang Dongsheng-B40534; Wood Scott-B07421 > Cc: rjw@rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao Chenhui- > B35336; linux-pm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support > > On 04/03/2014 05:20 AM, Dongsheng.Wang@freescale.com wrote: > > Hi Daniel, > > > > Thanks for your review. I will fix your comments. > > > > BTW, fix Rafael's email. :) > > > >>> +#include <linux/cpu.h> > >>> +#include <linux/cpuidle.h> > >>> +#include <linux/init.h> > >>> +#include <linux/kernel.h> > >>> +#include <linux/notifier.h> > >>> + > >>> +#include <asm/cputable.h> > >>> +#include <asm/machdep.h> > >>> +#include <asm/mpc85xx.h> > >>> + > >>> +static unsigned int max_idle_state; static struct cpuidle_state > >>> +*cpuidle_state_table; > >>> + > >>> +struct cpuidle_driver e500_idle_driver = { > >>> + .name = "e500_idle", > >>> + .owner = THIS_MODULE, > >>> +}; > >>> + > >>> +static void e500_cpuidle(void) > >>> +{ > >>> + if (cpuidle_idle_call()) > >>> + cpuidle_wait(); > >>> +} > >> > >> Nope, that has been changed. No more call to cpuidle_idle_call in a driver. > >> > > > > Thanks. > > > >>> + > >>> +static int pw10_enter(struct cpuidle_device *dev, > >>> + struct cpuidle_driver *drv, int index) { > >>> + cpuidle_wait(); > >>> + return index; > >>> +} > >>> + > >>> +#define MAX_BIT 63 > >>> +#define MIN_BIT 1 > >>> +extern u32 cpuidle_entry_bit; > >>> +static int pw20_enter(struct cpuidle_device *dev, > >>> + struct cpuidle_driver *drv, int index) { > >>> + u32 pw20_idle; > >>> + u32 entry_bit; > >>> + pw20_idle = mfspr(SPRN_PWRMGTCR0); > >>> + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { > >>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > >>> + entry_bit = MAX_BIT - cpuidle_entry_bit; > >>> + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); > >>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); > >>> + } > >>> + > >>> + cpuidle_wait(); > >>> + > >>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; > >>> + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); > >>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); > >>> + > >>> + return index; > >>> +} > >> > >> Is it possible to give some comments and encapsulate the code with > >> explicit function names to be implemented in an arch specific > >> directory file (eg. pm.c) and export these functions in a linux/ > >> header ? We try to prevent to include asm if possible. > >> > > > > Yep, Looks better. Thanks. > > > >>> + > >>> +static struct cpuidle_state pw_idle_states[] = { > >>> + { > >>> + .name = "pw10", > >>> + .desc = "pw10", > >>> + .flags = CPUIDLE_FLAG_TIME_VALID, > >>> + .exit_latency = 0, > >>> + .target_residency = 0, > >>> + .enter = &pw10_enter > >>> + }, > >>> + > >>> + { > >>> + .name = "pw20", > >>> + .desc = "pw20-core-idle", > >>> + .flags = CPUIDLE_FLAG_TIME_VALID, > >>> + .exit_latency = 1, > >>> + .target_residency = 50, > >>> + .enter = &pw20_enter > >>> + }, > >>> +}; > >> > >> No need to define this intermediate structure here, you can directly > >> initialize the cpuidle_driver: > >> > > > > Thanks. :) > > > >>> +static int cpu_hotplug_notify(struct notifier_block *n, > >>> + unsigned long action, void *hcpu) { > >>> + unsigned long hotcpu = (unsigned long)hcpu; > >>> + struct cpuidle_device *dev = > >>> + per_cpu_ptr(cpuidle_devices, hotcpu); > >>> + > >>> + if (dev && cpuidle_get_driver()) { > >>> + switch (action) { > >>> + case CPU_ONLINE: > >>> + case CPU_ONLINE_FROZEN: > >>> + cpuidle_pause_and_lock(); > >>> + cpuidle_enable_device(dev); > >>> + cpuidle_resume_and_unlock(); > >>> + break; > >>> + > >>> + case CPU_DEAD: > >>> + case CPU_DEAD_FROZEN: > >>> + cpuidle_pause_and_lock(); > >>> + cpuidle_disable_device(dev); > >>> + cpuidle_resume_and_unlock(); > >>> + break; > >>> + > >>> + default: > >>> + return NOTIFY_DONE; > >>> + } > >>> + } > >>> + > >>> + return NOTIFY_OK; > >>> +} > >>> + > >>> +static struct notifier_block cpu_hotplug_notifier = { > >>> + .notifier_call = cpu_hotplug_notify, }; > >> > >> Can you explain why this is needed ? > >> > > > > If a cpu will be plugged out/in, We should be let Cpuidle know to > > remove corresponding sys interface and disable/enable cpudile-governor for > current cpu. > > Ok, this code is a copy-paste of the powers' cpuidle driver. > > IIRC, I posted a patchset to move this portion of code in the cpuidle common > framework some time ago. > > Could you please get rid of this part of code ? > Yes, I can. :) Could you share me your patchset link? I can't found them on your tree. Regards, -Dongsheng > > >>> + err = register_cpu_notifier(&cpu_hotplug_notifier); > >>> + if (err) > >>> + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); > >>> + > >>> + /* Replace the original way of idle after cpuidle registered. */ > >>> + ppc_md.power_save = e500_cpuidle; > >>> + on_each_cpu(replace_orig_idle, NULL, 1); > >> > >> Why ? > >> > > > > I checked again, if we put cpuidle_idle_call in asm, this is not need. > > > > Regards, > > -Dongsheng > > > >>> + pr_info("e500_idle_driver registered.\n"); > >>> + > >>> + return 0; > >>> +} > >>> +late_initcall(e500_idle_init); > >>> > >> > >> Thanks > >> > >> -- Daniel > >> > >> > >> -- > >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM > >> SoCs > >> > >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > >> <http://twitter.com/#!/linaroorg> Twitter | > >> <http://www.linaro.org/linaro-blog/> Blog > >> > >> > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> > Blog > >
On 04/03/2014 10:03 AM, Dongsheng.Wang@freescale.com wrote: > > >> -----Original Message----- >> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] >> Sent: Thursday, April 03, 2014 2:29 PM >> To: Wang Dongsheng-B40534; Wood Scott-B07421 >> Cc: rjw@rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao Chenhui- >> B35336; linux-pm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org >> Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support >> >> On 04/03/2014 05:20 AM, Dongsheng.Wang@freescale.com wrote: >>> Hi Daniel, >>> >>> Thanks for your review. I will fix your comments. >>> >>> BTW, fix Rafael's email. :) >>> >>>>> +#include <linux/cpu.h> >>>>> +#include <linux/cpuidle.h> >>>>> +#include <linux/init.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/notifier.h> >>>>> + >>>>> +#include <asm/cputable.h> >>>>> +#include <asm/machdep.h> >>>>> +#include <asm/mpc85xx.h> >>>>> + >>>>> +static unsigned int max_idle_state; static struct cpuidle_state >>>>> +*cpuidle_state_table; >>>>> + >>>>> +struct cpuidle_driver e500_idle_driver = { >>>>> + .name = "e500_idle", >>>>> + .owner = THIS_MODULE, >>>>> +}; >>>>> + >>>>> +static void e500_cpuidle(void) >>>>> +{ >>>>> + if (cpuidle_idle_call()) >>>>> + cpuidle_wait(); >>>>> +} >>>> >>>> Nope, that has been changed. No more call to cpuidle_idle_call in a driver. >>>> >>> >>> Thanks. >>> >>>>> + >>>>> +static int pw10_enter(struct cpuidle_device *dev, >>>>> + struct cpuidle_driver *drv, int index) { >>>>> + cpuidle_wait(); >>>>> + return index; >>>>> +} >>>>> + >>>>> +#define MAX_BIT 63 >>>>> +#define MIN_BIT 1 >>>>> +extern u32 cpuidle_entry_bit; >>>>> +static int pw20_enter(struct cpuidle_device *dev, >>>>> + struct cpuidle_driver *drv, int index) { >>>>> + u32 pw20_idle; >>>>> + u32 entry_bit; >>>>> + pw20_idle = mfspr(SPRN_PWRMGTCR0); >>>>> + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { >>>>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >>>>> + entry_bit = MAX_BIT - cpuidle_entry_bit; >>>>> + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); >>>>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >>>>> + } >>>>> + >>>>> + cpuidle_wait(); >>>>> + >>>>> + pw20_idle &= ~PWRMGTCR0_PW20_ENT; >>>>> + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); >>>>> + mtspr(SPRN_PWRMGTCR0, pw20_idle); >>>>> + >>>>> + return index; >>>>> +} >>>> >>>> Is it possible to give some comments and encapsulate the code with >>>> explicit function names to be implemented in an arch specific >>>> directory file (eg. pm.c) and export these functions in a linux/ >>>> header ? We try to prevent to include asm if possible. >>>> >>> >>> Yep, Looks better. Thanks. >>> >>>>> + >>>>> +static struct cpuidle_state pw_idle_states[] = { >>>>> + { >>>>> + .name = "pw10", >>>>> + .desc = "pw10", >>>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>>> + .exit_latency = 0, >>>>> + .target_residency = 0, >>>>> + .enter = &pw10_enter >>>>> + }, >>>>> + >>>>> + { >>>>> + .name = "pw20", >>>>> + .desc = "pw20-core-idle", >>>>> + .flags = CPUIDLE_FLAG_TIME_VALID, >>>>> + .exit_latency = 1, >>>>> + .target_residency = 50, >>>>> + .enter = &pw20_enter >>>>> + }, >>>>> +}; >>>> >>>> No need to define this intermediate structure here, you can directly >>>> initialize the cpuidle_driver: >>>> >>> >>> Thanks. :) >>> >>>>> +static int cpu_hotplug_notify(struct notifier_block *n, >>>>> + unsigned long action, void *hcpu) { >>>>> + unsigned long hotcpu = (unsigned long)hcpu; >>>>> + struct cpuidle_device *dev = >>>>> + per_cpu_ptr(cpuidle_devices, hotcpu); >>>>> + >>>>> + if (dev && cpuidle_get_driver()) { >>>>> + switch (action) { >>>>> + case CPU_ONLINE: >>>>> + case CPU_ONLINE_FROZEN: >>>>> + cpuidle_pause_and_lock(); >>>>> + cpuidle_enable_device(dev); >>>>> + cpuidle_resume_and_unlock(); >>>>> + break; >>>>> + >>>>> + case CPU_DEAD: >>>>> + case CPU_DEAD_FROZEN: >>>>> + cpuidle_pause_and_lock(); >>>>> + cpuidle_disable_device(dev); >>>>> + cpuidle_resume_and_unlock(); >>>>> + break; >>>>> + >>>>> + default: >>>>> + return NOTIFY_DONE; >>>>> + } >>>>> + } >>>>> + >>>>> + return NOTIFY_OK; >>>>> +} >>>>> + >>>>> +static struct notifier_block cpu_hotplug_notifier = { >>>>> + .notifier_call = cpu_hotplug_notify, }; >>>> >>>> Can you explain why this is needed ? >>>> >>> >>> If a cpu will be plugged out/in, We should be let Cpuidle know to >>> remove corresponding sys interface and disable/enable cpudile-governor for >> current cpu. >> >> Ok, this code is a copy-paste of the powers' cpuidle driver. >> >> IIRC, I posted a patchset to move this portion of code in the cpuidle common >> framework some time ago. >> >> Could you please get rid of this part of code ? >> > > Yes, I can. :) Could you share me your patchset link? I can't found them on your tree. > It was a while ago. I should have it somewhere locally. I will find it out and resend the patch next week when finishing my current task. -- Daniel >> -- >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> >> Blog >> >> >
On Tue, 2014-04-01 at 16:33 +0800, Dongsheng Wang wrote: > From: Wang Dongsheng <dongsheng.wang@freescale.com> > > Add cpuidle support for e500 family, using cpuidle framework to > manage various low power modes. The new implementation will remain > compatible with original idle method. > > I have done test about power consumption and latency. Cpuidle framework > will make CPU response time faster than original method, but power > consumption is higher than original method. > > Power consumption: > The original method, power consumption is 10.51202 (W). > The cpuidle framework, power consumption is 10.5311 (W). > > Latency: > The original method, avg latency is 6782 (us). > The cpuidle framework, avg latency is 6482 (us). > > Initially, this supports PW10, PW20 and subsequent patches will support > DOZE/NAP and PH10, PH20. Have you tried tuning the timebase bit for the original method, to match what you're doing in cpuidle and/or for general optimization? Do you get similar results for a variety of workloads? > +static struct cpuidle_state pw_idle_states[] = { > + { > + .name = "pw10", > + .desc = "pw10", > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .exit_latency = 0, > + .target_residency = 0, > + .enter = &pw10_enter > + }, > + > + { > + .name = "pw20", > + .desc = "pw20-core-idle", > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .exit_latency = 1, > + .target_residency = 50, > + .enter = &pw20_enter > + }, > +}; Where did exit_latency and target_residency come from? It looks rather different from the ~1ms delay before entering PW20 with the original method. Though, I'd have expected a shorter PW20 delay to have longer wake latency, due to entering the lower power state more often... > +static void replace_orig_idle(void *dummy) > +{ > + return; > +} Why? If you're using this as some sort of barrier to ensure that all CPUs have done something before continuing, explain that, along with why it matters. -Scott
diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h index 5b6c03f..9301420 100644 --- a/arch/powerpc/include/asm/machdep.h +++ b/arch/powerpc/include/asm/machdep.h @@ -294,6 +294,15 @@ extern void power7_idle(void); extern void ppc6xx_idle(void); extern void book3e_idle(void); +static inline void cpuidle_wait(void) +{ +#ifdef CONFIG_PPC64 + book3e_idle(); +#else + e500_idle(); +#endif +} + /* * ppc_md contains a copy of the machine description structure for the * current platform. machine_id contains the initial address where the diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c index 97e1dc9..edd193f 100644 --- a/arch/powerpc/kernel/sysfs.c +++ b/arch/powerpc/kernel/sysfs.c @@ -190,6 +190,9 @@ static ssize_t show_pw20_wait_time(struct device *dev, return sprintf(buf, "%llu\n", time > 0 ? time : 0); } +#ifdef CONFIG_CPU_IDLE_E500 +u32 cpuidle_entry_bit; +#endif static void set_pw20_wait_entry_bit(void *val) { u32 *value = val; @@ -204,7 +207,11 @@ static void set_pw20_wait_entry_bit(void *val) /* set count */ pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT); +#ifdef CONFIG_CPU_IDLE_E500 + cpuidle_entry_bit = *value; +#else mtspr(SPRN_PWRMGTCR0, pw20_idle); +#endif } static ssize_t store_pw20_wait_time(struct device *dev, diff --git a/drivers/cpuidle/Kconfig.powerpc b/drivers/cpuidle/Kconfig.powerpc index 66c3a09..0949dbf 100644 --- a/drivers/cpuidle/Kconfig.powerpc +++ b/drivers/cpuidle/Kconfig.powerpc @@ -18,3 +18,10 @@ config POWERNV_CPUIDLE help Select this option to enable processor idle state management through cpuidle subsystem. + +config CPU_IDLE_E500 + bool "CPU Idle Driver for E500 family processors" + depends on CPU_IDLE + depends on FSL_SOC_BOOKE + help + Select this to enable cpuidle on e500 family processors. diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index f71ae1b..7e6adea 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -18,3 +18,4 @@ obj-$(CONFIG_ARM_AT91_CPUIDLE) += cpuidle-at91.o # POWERPC drivers obj-$(CONFIG_PSERIES_CPUIDLE) += cpuidle-pseries.o obj-$(CONFIG_POWERNV_CPUIDLE) += cpuidle-powernv.o +obj-$(CONFIG_CPU_IDLE_E500) += cpuidle-e500.o diff --git a/drivers/cpuidle/cpuidle-e500.c b/drivers/cpuidle/cpuidle-e500.c new file mode 100644 index 0000000..ddc0def --- /dev/null +++ b/drivers/cpuidle/cpuidle-e500.c @@ -0,0 +1,194 @@ +/* + * CPU Idle driver for Freescale PowerPC e500 family processors. + * + * Copyright 2014 Freescale Semiconductor, Inc. + * + * Author: Dongsheng Wang <dongsheng.wang@freescale.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/cpu.h> +#include <linux/cpuidle.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/notifier.h> + +#include <asm/cputable.h> +#include <asm/machdep.h> +#include <asm/mpc85xx.h> + +static unsigned int max_idle_state; +static struct cpuidle_state *cpuidle_state_table; + +struct cpuidle_driver e500_idle_driver = { + .name = "e500_idle", + .owner = THIS_MODULE, +}; + +static void e500_cpuidle(void) +{ + if (cpuidle_idle_call()) + cpuidle_wait(); +} + +static int pw10_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + cpuidle_wait(); + return index; +} + +#define MAX_BIT 63 +#define MIN_BIT 1 +extern u32 cpuidle_entry_bit; +static int pw20_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + u32 pw20_idle; + u32 entry_bit; + pw20_idle = mfspr(SPRN_PWRMGTCR0); + if ((pw20_idle & PWRMGTCR0_PW20_ENT) != PWRMGTCR0_PW20_ENT) { + pw20_idle &= ~PWRMGTCR0_PW20_ENT; + entry_bit = MAX_BIT - cpuidle_entry_bit; + pw20_idle |= (entry_bit << PWRMGTCR0_PW20_ENT_SHIFT); + mtspr(SPRN_PWRMGTCR0, pw20_idle); + } + + cpuidle_wait(); + + pw20_idle &= ~PWRMGTCR0_PW20_ENT; + pw20_idle |= (MIN_BIT << PWRMGTCR0_PW20_ENT_SHIFT); + mtspr(SPRN_PWRMGTCR0, pw20_idle); + + return index; +} + +static struct cpuidle_state pw_idle_states[] = { + { + .name = "pw10", + .desc = "pw10", + .flags = CPUIDLE_FLAG_TIME_VALID, + .exit_latency = 0, + .target_residency = 0, + .enter = &pw10_enter + }, + + { + .name = "pw20", + .desc = "pw20-core-idle", + .flags = CPUIDLE_FLAG_TIME_VALID, + .exit_latency = 1, + .target_residency = 50, + .enter = &pw20_enter + }, +}; + +static int cpu_hotplug_notify(struct notifier_block *n, + unsigned long action, void *hcpu) +{ + unsigned long hotcpu = (unsigned long)hcpu; + struct cpuidle_device *dev = + per_cpu_ptr(cpuidle_devices, hotcpu); + + if (dev && cpuidle_get_driver()) { + switch (action) { + case CPU_ONLINE: + case CPU_ONLINE_FROZEN: + cpuidle_pause_and_lock(); + cpuidle_enable_device(dev); + cpuidle_resume_and_unlock(); + break; + + case CPU_DEAD: + case CPU_DEAD_FROZEN: + cpuidle_pause_and_lock(); + cpuidle_disable_device(dev); + cpuidle_resume_and_unlock(); + break; + + default: + return NOTIFY_DONE; + } + } + + return NOTIFY_OK; +} + +static struct notifier_block cpu_hotplug_notifier = { + .notifier_call = cpu_hotplug_notify, +}; + +static void e500_cpuidle_driver_init(void) +{ + int idle_state; + struct cpuidle_driver *drv = &e500_idle_driver; + + drv->state_count = 0; + + for (idle_state = 0; idle_state < max_idle_state; ++idle_state) { + if (!cpuidle_state_table[idle_state].enter) + break; + + drv->states[drv->state_count] = cpuidle_state_table[idle_state]; + drv->state_count++; + } +} + +static int e500_idle_state_probe(void) +{ + if (cpuidle_disable != IDLE_NO_OVERRIDE) + return -ENODEV; + + cpuidle_state_table = pw_idle_states; + max_idle_state = ARRAY_SIZE(pw_idle_states); + + /* Disable PW20 feature for e500mc, e5500 */ + if (PVR_VER(cur_cpu_spec->pvr_value) != PVR_VER_E6500) + cpuidle_state_table[1].enter = NULL; + + if (!cpuidle_state_table || !max_idle_state) + return -ENODEV; + + return 0; +} + +static void replace_orig_idle(void *dummy) +{ + return; +} + +static int __init e500_idle_init(void) +{ + struct cpuidle_driver *drv = &e500_idle_driver; + int err; + + if (e500_idle_state_probe()) + return -ENODEV; + + e500_cpuidle_driver_init(); + if (!drv->state_count) + return -ENODEV; + + err = cpuidle_register(drv, NULL); + if (err) { + pr_err("Register e500 family cpuidle driver failed.\n"); + + return err; + } + + err = register_cpu_notifier(&cpu_hotplug_notifier); + if (err) + pr_warn("Cpuidle driver: register cpu notifier failed.\n"); + + /* Replace the original way of idle after cpuidle registered. */ + ppc_md.power_save = e500_cpuidle; + on_each_cpu(replace_orig_idle, NULL, 1); + + pr_info("e500_idle_driver registered.\n"); + + return 0; +} +late_initcall(e500_idle_init);