[PATCH] cpuidle: add freescale e500 family porcessors idle support

[Dongsheng.Wang at freescale.com]

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano at linaro.org]
> Sent: Thursday, April 03, 2014 2:29 PM
> To: Wang Dongsheng-B40534; Wood Scott-B07421
> Cc: rjw at rjwysocki.net; Li Yang-Leo-R58472; Jin Zhengxiong-R64188; Zhao Chenhui-
> B35336; linux-pm at vger.kernel.org; linuxppc-dev at lists.ozlabs.org
> Subject: Re: [PATCH] cpuidle: add freescale e500 family porcessors idle support
> 
> On 04/03/2014 05:20 AM, Dongsheng.Wang at 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
> 
>