| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51ACB06A.2050206@monstr.eu>
Date: Mon, 03 Jun 2013 17:04:10 +0200
From: Michal Simek <monstr@...str.eu>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
CC: Michal Simek <michal.simek@...inx.com>,
linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
linux-arm-kernel@...ts.infradead.org,
Josh Cartwright <josh.cartwright@...com>,
Steffen Trumtrar <s.trumtrar@...gutronix.de>,
Rob Herring <robherring2@...il.com>,
Peter Crosthwaite <peter.crosthwaite@...inx.com>,
"Rafael J. Wysocki" <rjw@...k.pl>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3] arm: zynq: Add cpuidle support
On 06/03/2013 04:03 PM, Daniel Lezcano wrote:
> On 06/03/2013 03:40 PM, Michal Simek wrote:
>> Add support for cpuidle.
>>
>> Signed-off-by: Michal Simek <michal.simek@...inx.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.
Will change.
>> + return 0;
>> +}
>> +module_init(zynq_init_cpuidle);
>
> Do you really want it as module ?
kirkwood is a module
but in Makefile depends directly on ARCH
obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
Calxeda uses module_init() which is in this configuration device_initcall.
Any advantage to write it as module?
Maybe for testing purpose will be helpful to have it as module too.
What do you think?
BTW: I will also add entry to MAINTAINERS file and list myself in this header.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Download attachment "signature.asc" of type "application/pgp-signature" (264 bytes)
Powered by blists - more mailing lists