[RFC patch 02/11] cpuidle / arm : a single cpuidle driver

Santosh Shilimkar santosh.shilimkar at ti.com
Tue Mar 26 00:31:25 EDT 2013


On Friday 15 March 2013 07:57 PM, Daniel Lezcano wrote:
> The cpuidle drivers are duplicating a lot of code and in most
> of the case there is a common pattern we can factor out:
> 
> 	* setup the broadcast timers
> 	* register the driver
> 	* register the devices
> 
> This arm driver is the common part between all the ARM cpuidle drivers,
> with the code factored out.
> 
> It does not handle the coupled idle state for now but it is the first
> step to have everyone to converge to the same code pattern.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano at linaro.org>
> ---
While I appreciate the effort behind code consolidation, $subject is
bit confusing. You are just abstracting the registration code to one
common place and I don't know why it has to be limited to arm-idle
since it is very generic code. That is true even for the broad-cast
notifier setup which is same across all arch's including ARM, X86.


>  MAINTAINERS                    |    6 +++
>  arch/arm/include/asm/cpuidle.h |    3 ++
>  drivers/cpuidle/Makefile       |    1 +
>  drivers/cpuidle/arm-idle.c     |  112 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 122 insertions(+)
>  create mode 100644 drivers/cpuidle/arm-idle.c
> 


[..]

> diff --git a/drivers/cpuidle/arm-idle.c b/drivers/cpuidle/arm-idle.c
> new file mode 100644
> index 0000000..397ff4c
> --- /dev/null
> +++ b/drivers/cpuidle/arm-idle.c
> @@ -0,0 +1,112 @@
> +/*
> + * Copyright 2012 Linaro Ltd: : Daniel Lezcano <daniel.lezcano at linaro.org>
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/clockchips.h>
> +#include <linux/module.h>
> +#include <linux/cpuidle.h>
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
> +static bool use_broadcast_timer = false;
> +
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> +static void setup_broadcast_timer(void *arg)
> +{
> +        int cpu = smp_processor_id();
> +        clockevents_notify((int)(arg), &cpu);
> +}
> +
> +static bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv)
> +{
> +	int i;
> +
> +	for (i = 0; i < drv->state_count; i++)
> +		if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)
> +			return true;
> +	return false;
> +}
> +
> +static inline void arm_idle_timer_broadcast(bool enable)
> +{
> +	on_each_cpu(setup_broadcast_timer, enable ?
> +		    (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON:
> +		    (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);
> +}
> +#else
> +static inline bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv)
> +{
> +	return false;
> +}
> +
> +static inline void arm_idle_timer_broadcast(bool enable)
> +{
> +	;
> +}
> +#endif
> +
> +int __init arm_idle_init(struct cpuidle_driver *drv)
> +{
> +	int ret, cpu;
> +	struct cpuidle_device *device;
> +
> +	use_broadcast_timer = arm_idle_use_broadcast(drv);
> +
> +	if (use_broadcast_timer)
> +		arm_idle_timer_broadcast(true);
> +
> +	ret = cpuidle_register_driver(drv);
> +	if (ret) {
> +		printk(KERN_ERR "failed to register idle driver '%s'\n",
> +			drv->name);
> +		return ret;
> +	}
> +
> +	for_each_online_cpu(cpu) {
> +
> +		device = &per_cpu(cpuidle_device, cpu);
> +		device->cpu = cpu;
> +		ret = cpuidle_register_device(device);
> +		if (ret) {
> +			printk(KERN_ERR "Failed to register cpuidle "
> +			       "device for cpu%d\n", cpu);
> +			goto out_unregister;
> +		}
> +	}
> +
> +out:
> +	return ret;
> +
> +out_unregister:
> +	for_each_online_cpu(cpu) {
> +		device = &per_cpu(cpuidle_device, cpu);
> +		cpuidle_unregister_device(device);
> +	}
> +
> +	cpuidle_unregister_driver(drv);
> +	goto out;
> +}
> +EXPORT_SYMBOL_GPL(arm_idle_init);
> +
> +void __exit arm_idle_exit(struct cpuidle_driver *drv)
> +{
> +	int cpu;
> +	struct cpuidle_device *device;
> +
> +	for_each_online_cpu(cpu) {
> +		device = &per_cpu(cpuidle_device, cpu);
> +		cpuidle_unregister_device(device);
> +	}
> +
> +	cpuidle_unregister_driver(drv);
> +
> +	if (use_broadcast_timer)
> +		arm_idle_timer_broadcast(false);
> +}
> +EXPORT_SYMBOL_GPL(arm_idle_exit);
> 
All above code is completly generic and I would rather create
some thing like "drivers/cpuidle/generic-idle.c" where it can
handle all the registration stuff for all arch's rather than
just ARM. There is nothing ARM specific in above code IMHO.

Regards,
Santosh




More information about the linux-arm-kernel mailing list