> -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org] > Sent: Thursday, April 03, 2014 2:29 PM > To: Wang Dongsheng-B40534; Wood Scott-B07421 > Cc: r...@rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao > Chenhui- > B35336; linux...@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.w...@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 > > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev