Re: [PATCH v3] arm: zynq: Add cpuidle support

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

On 06/03/2013 03:40 PM, Michal Simek wrote:
> Add support for cpuidle.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> Changes in v3:
> - Move driver to drivers/cpuidle/
> - Check zynq compatible string suggested by Arnd
> - Use zynq_ function prefix because of multiplatform kernel
> - Incorporate comments from Daniel Lezcano
> - Rebase on the top of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> 
> Changes in v2:
> - Fix file header
> 
> Changes in v1:
> - It was the part of Zynq core changes
>   https://patchwork.kernel.org/patch/2342511/
> 
>  drivers/cpuidle/Kconfig        |   6 +++
>  drivers/cpuidle/Makefile       |   1 +
>  drivers/cpuidle/cpuidle-zynq.c | 100 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 drivers/cpuidle/cpuidle-zynq.c
> 
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index c4cc27e..8272a08 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -39,4 +39,10 @@ config CPU_IDLE_CALXEDA
>  	help
>  	  Select this to enable cpuidle on Calxeda processors.
> 
> +config CPU_IDLE_ZYNQ
> +	bool "CPU Idle Driver for Xilinx Zynq processors"
> +	depends on ARCH_ZYNQ
> +	help
> +	  Select this to enable cpuidle on Xilinx Zynq processors.
> +
>  endif
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 0d8bd55..8767a7b 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
> 
>  obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
>  obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
> +obj-$(CONFIG_CPU_IDLE_ZYNQ) += cpuidle-zynq.o
> diff --git a/drivers/cpuidle/cpuidle-zynq.c b/drivers/cpuidle/cpuidle-zynq.c
> new file mode 100644
> index 0000000..afe6af3
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-zynq.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (C) 2012-2013 Xilinx
> + *
> + * CPU idle support for Xilinx Zynq
> + *
> + * based on arch/arm/mach-at91/cpuidle.c
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + *
> + * The cpu idle uses wait-for-interrupt and RAM self refresh in order
> + * to implement two idle states -
> + * #1 wait-for-interrupt
> + * #2 wait-for-interrupt and RAM self refresh
> + */

Please check you headers, it seems you are including unused headers.

> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/cpuidle.h>
> +#include <linux/io.h>

^^^^^

> +#include <linux/export.h>

^^^^^

> +#include <linux/clockchips.h>

^^^^^

> +#include <linux/cpu_pm.h>
> +#include <linux/clk.h>

^^^^

> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <asm/proc-fns.h>
> +#include <asm/cpuidle.h>
> +
> +#define ZYNQ_MAX_STATES		2
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, zynq_cpuidle_device);

see below.

> +
> +/* Actual code that puts the SoC in different idle states */
> +static int zynq_enter_idle(struct cpuidle_device *dev,
> +		struct cpuidle_driver *drv, int index)

Indentation.

> +{
> +	/* Devices must be stopped here */
> +	cpu_pm_enter();
> +
> +	/* Add code for DDR self refresh start */
> +	cpu_do_idle();
> +
> +	/* Add code for DDR self refresh stop */
> +	cpu_pm_exit();
> +
> +	return index;
> +}
> +
> +static struct cpuidle_driver zynq_idle_driver = {
> +	.name = "zynq_idle",
> +	.owner = THIS_MODULE,
> +	.states = {
> +		ARM_CPUIDLE_WFI_STATE,
> +		{
> +			.enter            = zynq_enter_idle,
> +			.exit_latency     = 10,
> +			.target_residency = 10000,
> +			.flags            = CPUIDLE_FLAG_TIME_VALID,
> +					    CPUIDLE_FLAG_TIMER_STOP,
> +			.name             = "RAM_SR",
> +			.desc             = "WFI and RAM Self Refresh",
> +		},
> +	},
> +	.safe_state_index = 0,
> +	.state_count = ZYNQ_MAX_STATES,
> +};
> +
> +/* Initialize CPU idle by registering the idle states */
> +static int zynq_init_cpuidle(void)
> +{
> +	unsigned int cpu;
> +	struct cpuidle_device *device;
> +	int ret;
> +
> +	if (!of_machine_is_compatible("xlnx,zynq-7000"))
> +		return -ENODEV;
> +
> +	ret = cpuidle_register_driver(&zynq_idle_driver);
> +	if (ret) {
> +		pr_err("%s: Driver registration failed\n", __func__);
> +		return ret;
> +	}
> +
> +	for_each_possible_cpu(cpu) {
> +		device = &per_cpu(zynq_cpuidle_device, cpu);
> +		device->cpu = cpu;
> +		ret = cpuidle_register_device(device);
> +		if (ret) {
> +			pr_err("%s: Device registration failed\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	pr_info("Xilinx Zynq CpuIdle Driver started\n");

Hi Michal,

actually you can replace this code by

cpuidle_register(&zynq_idle_driver, NULL);

and remove the zynq_cpuidle_device.

The framework will take care of registering the driver and the devices.


> +	return 0;
> +}
> +module_init(zynq_init_cpuidle);

Do you really want it as module ?

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