On 08/23/2013 08:46 AM, Wang Dongsheng-B40534 wrote: > >> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig >> index 0e2cd5c..e805dcd 100644 >> --- a/drivers/cpuidle/Kconfig >> +++ b/drivers/cpuidle/Kconfig > > Maybe drivers/cpuidle/Kconfig.powerpc is better? Like arm. > >> +obj-$(CONFIG_CPU_IDLE_IBM_POWER) += cpuidle-ibm-power.o >> diff --git a/drivers/cpuidle/cpuidle-ibm-power.c >> b/drivers/cpuidle/cpuidle-ibm-power.c >> new file mode 100644 >> index 0000000..4ee5a94 >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-ibm-power.c >> @@ -0,0 +1,304 @@ >> +/* >> + * cpuidle-ibm-power - idle state cpuidle driver. >> + * Adapted from drivers/idle/intel_idle.c and >> + * drivers/acpi/processor_idle.c >> + * >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/moduleparam.h> >> +#include <linux/cpuidle.h> >> +#include <linux/cpu.h> >> +#include <linux/notifier.h> >> + >> +#include <asm/paca.h> >> +#include <asm/reg.h> >> +#include <asm/machdep.h> >> +#include <asm/firmware.h> >> +#include <asm/runlatch.h> >> +#include <asm/plpar_wrappers.h> >> + >> +struct cpuidle_driver power_idle_driver = { >> + .name = "IBM-POWER-Idle", >> + .owner = THIS_MODULE, >> +}; >> + >> +#define MAX_IDLE_STATE_COUNT 2 >> + >> +static int max_idle_state = MAX_IDLE_STATE_COUNT - 1; > > Again, do not use the macro. Wanting to re-use CPUIDLE_STATE_MAX macro, defined in linux/cpuidle.h similar to what x86 uses. This is def scalable for various platforms , as demonstrated in drivers/idle/intel_idle.c > >> +static struct cpuidle_state *cpuidle_state_table; >> + >> +static inline void idle_loop_prolog(unsigned long *in_purr) >> +{ >> + *in_purr = mfspr(SPRN_PURR); >> + /* >> + * Indicate to the HV that we are idle. Now would be >> + * a good time to find other work to dispatch. >> + */ >> + get_lppaca()->idle = 1; >> +} >> + >> +static inline void idle_loop_epilog(unsigned long in_purr) >> +{ >> + get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr; >> + get_lppaca()->idle = 0; >> +} >> + >> +static int snooze_loop(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, >> + int index) >> +{ >> + unsigned long in_purr; >> + >> + idle_loop_prolog(&in_purr); >> + local_irq_enable(); > > snooze_loop has already registered in cpuidle framework to handle snooze state. > where disable the irq? Why do "enable" here? > >> +/* >> + * States for dedicated partition case. >> + */ >> +static struct cpuidle_state dedicated_states[MAX_IDLE_STATE_COUNT] = { >> + { /* Snooze */ >> + .name = "snooze", >> + .desc = "snooze", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 0, >> + .target_residency = 0, >> + .enter = &snooze_loop }, >> + { /* CEDE */ >> + .name = "CEDE", >> + .desc = "CEDE", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 10, >> + .target_residency = 100, >> + .enter = &dedicated_cede_loop }, >> +}; >> + >> +/* >> + * States for shared partition case. >> + */ >> +static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = { >> + { /* Shared Cede */ >> + .name = "Shared Cede", >> + .desc = "Shared Cede", >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .exit_latency = 0, >> + .target_residency = 0, >> + .enter = &shared_cede_loop }, >> +}; >> + >> +static void __exit power_processor_idle_exit(void) >> +{ >> + >> + unregister_cpu_notifier(&setup_hotplug_notifier); > > Remove a blank line. > >> + cpuidle_unregister(&power_idle_driver); >> + return; >> +} >> + >> +module_init(power_processor_idle_init); >> +module_exit(power_processor_idle_exit); >> + > > Did you have tested the module? If not tested, please don't use the module. > >> +MODULE_AUTHOR("Deepthi Dharwar <deepthi@xxxxxxxxxxxxxxxxxx>"); >> +MODULE_DESCRIPTION("Cpuidle driver for IBM POWER platforms"); >> +MODULE_LICENSE("GPL"); >> >