From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Catalin Marinas <Catalin.Marinas@arm.com>,
"robherring2@gmail.com" <robherring2@gmail.com>,
"arnd@arndb.de" <arnd@arndb.de>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"lina.iyer@linaro.org" <lina.iyer@linaro.org>,
"sboyd@codeaurora.org" <sboyd@codeaurora.org>
Subject: Re: [PATCH V3 7/8] ARM: cpuidle: Register per cpuidle device
Date: Sat, 21 Mar 2015 20:35:47 +0000 [thread overview]
Message-ID: <20150321203546.GE22354@red-moon> (raw)
In-Reply-To: <1426851841-2072-8-git-send-email-daniel.lezcano@linaro.org>
On Fri, Mar 20, 2015 at 11:44:00AM +0000, Daniel Lezcano wrote:
> Some architectures have some cpus which does not support idle states.
>
> Let the underlying low level code to return -ENXIO when it is not
> possible to set an idle state.
Well, this is getting interesting. We are parsing possible CPUs to
detect if they have common idle states in DT. If a CPU does not support
idle states, the cpu node for that CPU should not define any idle
state.
The approach above will work with my heterogenous system patch, since
the respective CPUidle driver mask will be created by parsing the DT
idle states.
http://www.spinics.net/lists/arm-kernel/msg403190.html
In current approach if a "possible " CPU does not have idle states, we do
not init CPUidle at all.
So, to cut a long story short, what does "a cpu does not support idle
states" mean ?
Does it mean that firmware defines idle states for that CPU in DT but
initializing them fail ?
I am fine with this patch, but we need to define -ENXIO return properly.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpuidle/cpuidle-arm.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 1c94b88..e4a6eba 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -17,6 +17,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/slab.h>
>
> #include <asm/cpuidle.h>
>
> @@ -94,6 +95,7 @@ static int __init arm_idle_init(void)
> {
> int cpu, ret;
> struct cpuidle_driver *drv = &arm_idle_driver;
> + struct cpuidle_device *dev;
>
> /*
> * Initialize idle states data, starting at index 1.
> @@ -105,18 +107,54 @@ static int __init arm_idle_init(void)
> if (ret <= 0)
> return ret ? : -ENODEV;
>
> + ret = cpuidle_register_driver(drv);
> + if (ret) {
> + pr_err("Failed to register cpuidle driver\n");
> + return ret;
> + }
> +
> /*
> * Call arch CPU operations in order to initialize
> * idle states suspend back-end specific data
> */
> for_each_possible_cpu(cpu) {
> ret = arm_cpuidle_init(cpu);
> +
> + /* This cpu does not support any idle states */
We need to define what this means. If it means a cpu with no idle
states in its cpu node the parsing would not even get here since
to init the driver all possible cpus have to have the *same* idle states to
function at present.
Lorenzo
> + if (ret == -ENXIO)
> + continue;
> +
> if (ret) {
> pr_err("CPU %d failed to init idle CPU ops\n", cpu);
> - return ret;
> + goto out_fail;
> + }
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + pr_err("Failed to allocate cpuidle device\n");
> + goto out_fail;
> + }
> + dev->cpu = cpu;
> +
> + ret = cpuidle_register_device(dev);
> + if (ret) {
> + pr_err("Failed to register cpuidle device for CPU %d\n",
> + cpu);
> + kfree(dev);
> + goto out_fail;
> }
> }
>
> - return cpuidle_register(drv, NULL);
> + return 0;
> +out_fail:
> + while (--cpu >= 0) {
> + dev = per_cpu(cpuidle_devices, cpu);
> + cpuidle_unregister_device(dev);
> + kfree(dev);
> + }
> +
> + cpuidle_unregister_driver(drv);
> +
> + return ret;
> }
> device_initcall(arm_idle_init);
> --
> 1.9.1
>
>
WARNING: multiple messages have this Message-ID (diff)
From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V3 7/8] ARM: cpuidle: Register per cpuidle device
Date: Sat, 21 Mar 2015 20:35:47 +0000 [thread overview]
Message-ID: <20150321203546.GE22354@red-moon> (raw)
In-Reply-To: <1426851841-2072-8-git-send-email-daniel.lezcano@linaro.org>
On Fri, Mar 20, 2015 at 11:44:00AM +0000, Daniel Lezcano wrote:
> Some architectures have some cpus which does not support idle states.
>
> Let the underlying low level code to return -ENXIO when it is not
> possible to set an idle state.
Well, this is getting interesting. We are parsing possible CPUs to
detect if they have common idle states in DT. If a CPU does not support
idle states, the cpu node for that CPU should not define any idle
state.
The approach above will work with my heterogenous system patch, since
the respective CPUidle driver mask will be created by parsing the DT
idle states.
http://www.spinics.net/lists/arm-kernel/msg403190.html
In current approach if a "possible " CPU does not have idle states, we do
not init CPUidle at all.
So, to cut a long story short, what does "a cpu does not support idle
states" mean ?
Does it mean that firmware defines idle states for that CPU in DT but
initializing them fail ?
I am fine with this patch, but we need to define -ENXIO return properly.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpuidle/cpuidle-arm.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 1c94b88..e4a6eba 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -17,6 +17,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/slab.h>
>
> #include <asm/cpuidle.h>
>
> @@ -94,6 +95,7 @@ static int __init arm_idle_init(void)
> {
> int cpu, ret;
> struct cpuidle_driver *drv = &arm_idle_driver;
> + struct cpuidle_device *dev;
>
> /*
> * Initialize idle states data, starting at index 1.
> @@ -105,18 +107,54 @@ static int __init arm_idle_init(void)
> if (ret <= 0)
> return ret ? : -ENODEV;
>
> + ret = cpuidle_register_driver(drv);
> + if (ret) {
> + pr_err("Failed to register cpuidle driver\n");
> + return ret;
> + }
> +
> /*
> * Call arch CPU operations in order to initialize
> * idle states suspend back-end specific data
> */
> for_each_possible_cpu(cpu) {
> ret = arm_cpuidle_init(cpu);
> +
> + /* This cpu does not support any idle states */
We need to define what this means. If it means a cpu with no idle
states in its cpu node the parsing would not even get here since
to init the driver all possible cpus have to have the *same* idle states to
function at present.
Lorenzo
> + if (ret == -ENXIO)
> + continue;
> +
> if (ret) {
> pr_err("CPU %d failed to init idle CPU ops\n", cpu);
> - return ret;
> + goto out_fail;
> + }
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + pr_err("Failed to allocate cpuidle device\n");
> + goto out_fail;
> + }
> + dev->cpu = cpu;
> +
> + ret = cpuidle_register_device(dev);
> + if (ret) {
> + pr_err("Failed to register cpuidle device for CPU %d\n",
> + cpu);
> + kfree(dev);
> + goto out_fail;
> }
> }
>
> - return cpuidle_register(drv, NULL);
> + return 0;
> +out_fail:
> + while (--cpu >= 0) {
> + dev = per_cpu(cpuidle_devices, cpu);
> + cpuidle_unregister_device(dev);
> + kfree(dev);
> + }
> +
> + cpuidle_unregister_driver(drv);
> +
> + return ret;
> }
> device_initcall(arm_idle_init);
> --
> 1.9.1
>
>
next prev parent reply other threads:[~2015-03-21 20:35 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-20 11:43 [PATCH V3 0/8] ARM: cpuidle: Unify the ARM64/ARM DT approach Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` [PATCH V3 1/8] ARM: cpuidle: Remove duplicate header inclusion Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` [PATCH V3 2/8] ARM: cpuidle: Add a cpuidle ops structure to be used for DT Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-25 21:34 ` Stephen Boyd
2015-03-25 21:34 ` Stephen Boyd
2015-03-20 11:43 ` [PATCH V3 3/8] ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` [PATCH V3 4/8] ARM64: cpuidle: Rename cpu_init_idle to a common function name Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-21 20:24 ` Lorenzo Pieralisi
2015-03-21 20:24 ` Lorenzo Pieralisi
2015-03-20 11:43 ` [PATCH V3 5/8] ARM64: cpuidle: Remove arm64 reference Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-21 20:21 ` Lorenzo Pieralisi
2015-03-21 20:21 ` Lorenzo Pieralisi
2015-03-20 11:43 ` [PATCH V3 6/8] ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64 Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-20 11:43 ` Daniel Lezcano
2015-03-21 20:20 ` Lorenzo Pieralisi
2015-03-21 20:20 ` Lorenzo Pieralisi
2015-03-20 11:44 ` [PATCH V3 7/8] ARM: cpuidle: Register per cpuidle device Daniel Lezcano
2015-03-20 11:44 ` Daniel Lezcano
2015-03-21 20:35 ` Lorenzo Pieralisi [this message]
2015-03-21 20:35 ` Lorenzo Pieralisi
2015-03-23 14:42 ` Daniel Lezcano
2015-03-23 14:42 ` Daniel Lezcano
2015-03-23 15:41 ` Daniel Lezcano
2015-03-23 15:41 ` Daniel Lezcano
2015-03-23 16:25 ` Lorenzo Pieralisi
2015-03-23 16:25 ` Lorenzo Pieralisi
2015-03-23 16:50 ` [PATCH V4] " Daniel Lezcano
2015-03-23 16:50 ` Daniel Lezcano
2015-03-23 22:58 ` Lorenzo Pieralisi
2015-03-23 22:58 ` Lorenzo Pieralisi
2015-03-24 9:54 ` [PATCH V5 1/2] " Daniel Lezcano
2015-03-24 9:54 ` Daniel Lezcano
2015-03-24 9:54 ` [PATCH 2/2] ARM: cpuidle: Document the code Daniel Lezcano
2015-03-24 9:54 ` Daniel Lezcano
2015-03-24 18:51 ` Lorenzo Pieralisi
2015-03-24 18:51 ` Lorenzo Pieralisi
2015-03-24 19:04 ` Lorenzo Pieralisi
2015-03-24 19:04 ` Lorenzo Pieralisi
2015-03-24 19:04 ` Lorenzo Pieralisi
2015-03-25 7:03 ` Daniel Lezcano
2015-03-25 7:03 ` Daniel Lezcano
2015-03-24 18:10 ` [PATCH V5 1/2] ARM: cpuidle: Register per cpuidle device Lorenzo Pieralisi
2015-03-24 18:10 ` Lorenzo Pieralisi
2015-03-20 11:44 ` [PATCH V3 8/8] ARM: cpuidle: Change function name to be consistent with x86 Daniel Lezcano
2015-03-20 11:44 ` Daniel Lezcano
2015-03-21 20:09 ` Lorenzo Pieralisi
2015-03-21 20:09 ` Lorenzo Pieralisi
2015-03-20 18:31 ` [PATCH V3 0/8] ARM: cpuidle: Unify the ARM64/ARM DT approach Lorenzo Pieralisi
2015-03-20 18:31 ` Lorenzo Pieralisi
2015-03-20 18:39 ` Daniel Lezcano
2015-03-20 18:39 ` Daniel Lezcano
2015-03-21 20:51 ` Lorenzo Pieralisi
2015-03-21 20:51 ` Lorenzo Pieralisi
2015-03-23 15:27 ` Lorenzo Pieralisi
2015-03-23 15:27 ` Lorenzo Pieralisi
2015-03-23 15:31 ` Daniel Lezcano
2015-03-23 15:31 ` Daniel Lezcano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150321203546.GE22354@red-moon \
--to=lorenzo.pieralisi@arm.com \
--cc=Catalin.Marinas@arm.com \
--cc=arnd@arndb.de \
--cc=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=lina.iyer@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
--cc=robherring2@gmail.com \
--cc=sboyd@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.