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 > > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev