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
