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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 04/03/2014 05:20 AM, Dongsheng.Wang@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 ?


>>> +	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