* [PATCH 1/9] PM / Domains: Allocate memory outside domain locks
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-12 19:47 ` Kevin Hilman
2015-09-01 12:40 ` Ulf Hansson
2015-08-04 23:35 ` [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM Lina Iyer
` (7 subsequent siblings)
8 siblings, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
In preparation for supporting IRQ-safe domains, allocate domain data
outside the domain locks. These functions are not called in an atomic
context, so we can always allocate memory using GFP_KERNEL. By
allocating memory before the locks, we can safely lock the domain using
spinlocks instead of mutexes.
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/base/power/domain.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7666a1c..5fd1306 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1382,13 +1382,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *subdomain)
{
- struct gpd_link *link;
+ struct gpd_link *link, *itr;
int ret = 0;
if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
|| genpd == subdomain)
return -EINVAL;
+ link = kzalloc(sizeof(*link), GFP_KERNEL);
+ if (!link)
+ return -ENOMEM;
+
mutex_lock(&genpd->lock);
mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
@@ -1398,18 +1402,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
goto out;
}
- list_for_each_entry(link, &genpd->master_links, master_node) {
- if (link->slave == subdomain && link->master == genpd) {
+ list_for_each_entry(itr, &genpd->master_links, master_node) {
+ if (itr->slave == subdomain && itr->master == genpd) {
ret = -EINVAL;
goto out;
}
}
- link = kzalloc(sizeof(*link), GFP_KERNEL);
- if (!link) {
- ret = -ENOMEM;
- goto out;
- }
link->master = genpd;
list_add_tail(&link->master_node, &genpd->master_links);
link->slave = subdomain;
@@ -1420,7 +1419,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
out:
mutex_unlock(&subdomain->lock);
mutex_unlock(&genpd->lock);
-
+ if (ret)
+ kfree(link);
return ret;
}
@@ -1511,17 +1511,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
if (IS_ERR_OR_NULL(genpd) || state < 0)
return -EINVAL;
+ cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
+ if (!cpuidle_data)
+ return -ENOMEM;
+
mutex_lock(&genpd->lock);
if (genpd->cpuidle_data) {
ret = -EEXIST;
- goto out;
- }
- cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
- if (!cpuidle_data) {
- ret = -ENOMEM;
- goto out;
+ goto err_drv;
}
+
cpuidle_drv = cpuidle_driver_ref();
if (!cpuidle_drv) {
ret = -ENODEV;
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 1/9] PM / Domains: Allocate memory outside domain locks
2015-08-04 23:35 ` [PATCH 1/9] PM / Domains: Allocate memory outside domain locks Lina Iyer
@ 2015-08-12 19:47 ` Kevin Hilman
2015-09-01 12:40 ` Ulf Hansson
1 sibling, 0 replies; 60+ messages in thread
From: Kevin Hilman @ 2015-08-12 19:47 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> In preparation for supporting IRQ-safe domains, allocate domain data
> outside the domain locks. These functions are not called in an atomic
> context, so we can always allocate memory using GFP_KERNEL. By
> allocating memory before the locks, we can safely lock the domain using
> spinlocks instead of mutexes.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
> ---
> drivers/base/power/domain.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7666a1c..5fd1306 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1382,13 +1382,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> struct generic_pm_domain *subdomain)
> {
> - struct gpd_link *link;
> + struct gpd_link *link, *itr;
> int ret = 0;
>
> if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
> || genpd == subdomain)
> return -EINVAL;
>
> + link = kzalloc(sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> mutex_lock(&genpd->lock);
> mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>
> @@ -1398,18 +1402,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> goto out;
> }
>
> - list_for_each_entry(link, &genpd->master_links, master_node) {
> - if (link->slave == subdomain && link->master == genpd) {
> + list_for_each_entry(itr, &genpd->master_links, master_node) {
> + if (itr->slave == subdomain && itr->master == genpd) {
> ret = -EINVAL;
> goto out;
> }
> }
>
> - link = kzalloc(sizeof(*link), GFP_KERNEL);
> - if (!link) {
> - ret = -ENOMEM;
> - goto out;
> - }
> link->master = genpd;
> list_add_tail(&link->master_node, &genpd->master_links);
> link->slave = subdomain;
> @@ -1420,7 +1419,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> out:
> mutex_unlock(&subdomain->lock);
> mutex_unlock(&genpd->lock);
> -
> + if (ret)
> + kfree(link);
> return ret;
> }
>
> @@ -1511,17 +1511,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
> if (IS_ERR_OR_NULL(genpd) || state < 0)
> return -EINVAL;
>
> + cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> + if (!cpuidle_data)
> + return -ENOMEM;
> +
> mutex_lock(&genpd->lock);
>
> if (genpd->cpuidle_data) {
> ret = -EEXIST;
> - goto out;
> - }
> - cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> - if (!cpuidle_data) {
> - ret = -ENOMEM;
> - goto out;
> + goto err_drv;
> }
> +
> cpuidle_drv = cpuidle_driver_ref();
> if (!cpuidle_drv) {
> ret = -ENODEV;
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 1/9] PM / Domains: Allocate memory outside domain locks
2015-08-04 23:35 ` [PATCH 1/9] PM / Domains: Allocate memory outside domain locks Lina Iyer
2015-08-12 19:47 ` Kevin Hilman
@ 2015-09-01 12:40 ` Ulf Hansson
1 sibling, 0 replies; 60+ messages in thread
From: Ulf Hansson @ 2015-09-01 12:40 UTC (permalink / raw)
To: linux-arm-kernel
On 5 August 2015 at 01:35, Lina Iyer <lina.iyer@linaro.org> wrote:
> In preparation for supporting IRQ-safe domains, allocate domain data
> outside the domain locks. These functions are not called in an atomic
> context, so we can always allocate memory using GFP_KERNEL. By
> allocating memory before the locks, we can safely lock the domain using
> spinlocks instead of mutexes.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7666a1c..5fd1306 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1382,13 +1382,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> struct generic_pm_domain *subdomain)
> {
> - struct gpd_link *link;
> + struct gpd_link *link, *itr;
> int ret = 0;
>
> if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
> || genpd == subdomain)
> return -EINVAL;
>
> + link = kzalloc(sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> mutex_lock(&genpd->lock);
> mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>
> @@ -1398,18 +1402,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> goto out;
> }
>
> - list_for_each_entry(link, &genpd->master_links, master_node) {
> - if (link->slave == subdomain && link->master == genpd) {
> + list_for_each_entry(itr, &genpd->master_links, master_node) {
> + if (itr->slave == subdomain && itr->master == genpd) {
> ret = -EINVAL;
> goto out;
> }
> }
>
> - link = kzalloc(sizeof(*link), GFP_KERNEL);
> - if (!link) {
> - ret = -ENOMEM;
> - goto out;
> - }
> link->master = genpd;
> list_add_tail(&link->master_node, &genpd->master_links);
> link->slave = subdomain;
> @@ -1420,7 +1419,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> out:
> mutex_unlock(&subdomain->lock);
> mutex_unlock(&genpd->lock);
> -
> + if (ret)
> + kfree(link);
> return ret;
> }
>
> @@ -1511,17 +1511,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
> if (IS_ERR_OR_NULL(genpd) || state < 0)
> return -EINVAL;
>
> + cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> + if (!cpuidle_data)
> + return -ENOMEM;
> +
> mutex_lock(&genpd->lock);
>
> if (genpd->cpuidle_data) {
> ret = -EEXIST;
> - goto out;
> - }
> - cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
> - if (!cpuidle_data) {
> - ret = -ENOMEM;
> - goto out;
> + goto err_drv;
> }
> +
> cpuidle_drv = cpuidle_driver_ref();
> if (!cpuidle_drv) {
> ret = -ENODEV;
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
2015-08-04 23:35 ` [PATCH 1/9] PM / Domains: Allocate memory outside domain locks Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-12 19:50 ` Kevin Hilman
2015-09-01 13:28 ` Ulf Hansson
2015-08-04 23:35 ` [PATCH 3/9] PM / Domains: Support IRQ safe PM domains Lina Iyer
` (6 subsequent siblings)
8 siblings, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Remove check for driver of a device, for runtime PM. Device may be
suspended without an explicit driver. This check seems to be vestigial
and incorrect in the current context.
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/base/power/domain.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 5fd1306..ef8d19f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
if (stat > PM_QOS_FLAGS_NONE)
return -EBUSY;
- if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
- || pdd->dev->power.irq_safe))
+ if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
not_suspended++;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-04 23:35 ` [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM Lina Iyer
@ 2015-08-12 19:50 ` Kevin Hilman
2015-08-13 8:57 ` Geert Uytterhoeven
2015-09-01 13:28 ` Ulf Hansson
1 sibling, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-12 19:50 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> Remove check for driver of a device, for runtime PM. Device may be
> suspended without an explicit driver. This check seems to be vestigial
> and incorrect in the current context.
This one should probably have been RFC.
For a little more context here, this was uncovered when experimenting
with using runtime PM for CPU devices which don't have a dev->driver.
This check might have made sense before PM domains, but with PM domains,
it's entirely possible to have a simple device without a driver and the
PM domain handles all the necesary PM, so I think this check
could/should be removed.
Thoughts?
Kevin
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> drivers/base/power/domain.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5fd1306..ef8d19f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
> if (stat > PM_QOS_FLAGS_NONE)
> return -EBUSY;
>
> - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
> - || pdd->dev->power.irq_safe))
> + if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> not_suspended++;
> }
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-12 19:50 ` Kevin Hilman
@ 2015-08-13 8:57 ` Geert Uytterhoeven
2015-08-14 3:40 ` Kevin Hilman
0 siblings, 1 reply; 60+ messages in thread
From: Geert Uytterhoeven @ 2015-08-13 8:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Kevin,
On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
> Lina Iyer <lina.iyer@linaro.org> writes:
>
>> Remove check for driver of a device, for runtime PM. Device may be
>> suspended without an explicit driver. This check seems to be vestigial
>> and incorrect in the current context.
>
> This one should probably have been RFC.
>
> For a little more context here, this was uncovered when experimenting
> with using runtime PM for CPU devices which don't have a dev->driver.
>
> This check might have made sense before PM domains, but with PM domains,
> it's entirely possible to have a simple device without a driver and the
> PM domain handles all the necesary PM, so I think this check
> could/should be removed.
>
> Thoughts?
Simple devices without a driver aren't handled automatically.
At minimum, the driver should call pm_runtime_enable(), cfr.
drivers/bus/simple-pm-bus.c.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-13 8:57 ` Geert Uytterhoeven
@ 2015-08-14 3:40 ` Kevin Hilman
2015-08-14 7:24 ` Geert Uytterhoeven
0 siblings, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-14 3:40 UTC (permalink / raw)
To: linux-arm-kernel
Geert Uytterhoeven <geert@linux-m68k.org> writes:
> Hi Kevin,
>
> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>> Lina Iyer <lina.iyer@linaro.org> writes:
>>
>>> Remove check for driver of a device, for runtime PM. Device may be
>>> suspended without an explicit driver. This check seems to be vestigial
>>> and incorrect in the current context.
>>
>> This one should probably have been RFC.
>>
>> For a little more context here, this was uncovered when experimenting
>> with using runtime PM for CPU devices which don't have a dev->driver.
>>
>> This check might have made sense before PM domains, but with PM domains,
>> it's entirely possible to have a simple device without a driver and the
>> PM domain handles all the necesary PM, so I think this check
>> could/should be removed.
>>
>> Thoughts?
>
> Simple devices without a driver aren't handled automatically.
> At minimum, the driver should call pm_runtime_enable(), cfr.
> drivers/bus/simple-pm-bus.c.
That's correct, and in the proof-of-concept stuff I hacked up and in
Lina's series, the CPU "devices" do indeed to this. Without that, they
wouldn't end up ever taking this codepath through genpd's
runtime_suspend and power_off hooks.
Also, I'm not sure if your comment was meant to be an objection to the
patch? or if you're OK with it.
Thanks for the feedback,
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-14 3:40 ` Kevin Hilman
@ 2015-08-14 7:24 ` Geert Uytterhoeven
2015-08-14 17:19 ` Kevin Hilman
0 siblings, 1 reply; 60+ messages in thread
From: Geert Uytterhoeven @ 2015-08-14 7:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Kevin,
On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>> This check might have made sense before PM domains, but with PM domains,
>>> it's entirely possible to have a simple device without a driver and the
>>> PM domain handles all the necesary PM, so I think this check
>>> could/should be removed.
>>>
>>> Thoughts?
>>
>> Simple devices without a driver aren't handled automatically.
>> At minimum, the driver should call pm_runtime_enable(), cfr.
>> drivers/bus/simple-pm-bus.c.
>
> That's correct, and in the proof-of-concept stuff I hacked up and in
> Lina's series, the CPU "devices" do indeed to this. Without that, they
> wouldn't end up ever taking this codepath through genpd's
> runtime_suspend and power_off hooks.
>
> Also, I'm not sure if your comment was meant to be an objection to the
> patch? or if you're OK with it.
My comment was purely meant as a response to "it's entirely possible to have a
simple device without a driver and the PM domain handles all the necesary PM".
I have no objections to the patch (read: I'm not sufficiently familiar with
the code to make educated guesses about the side effects of this change ;-).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-14 7:24 ` Geert Uytterhoeven
@ 2015-08-14 17:19 ` Kevin Hilman
2015-08-16 9:24 ` Geert Uytterhoeven
0 siblings, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-14 17:19 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Kevin,
>
> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>>> This check might have made sense before PM domains, but with PM domains,
>>>> it's entirely possible to have a simple device without a driver and the
>>>> PM domain handles all the necesary PM, so I think this check
>>>> could/should be removed.
>>>>
>>>> Thoughts?
>>>
>>> Simple devices without a driver aren't handled automatically.
>>> At minimum, the driver should call pm_runtime_enable(), cfr.
>>> drivers/bus/simple-pm-bus.c.
>>
>> That's correct, and in the proof-of-concept stuff I hacked up and in
>> Lina's series, the CPU "devices" do indeed to this. Without that, they
>> wouldn't end up ever taking this codepath through genpd's
>> runtime_suspend and power_off hooks.
>>
>> Also, I'm not sure if your comment was meant to be an objection to the
>> patch? or if you're OK with it.
>
> My comment was purely meant as a response to "it's entirely possible to have a
> simple device without a driver and the PM domain handles all the necesary PM".
Right, so if the PM domain does the pm_runtime_enable() for these
"simple" devices without drivers, they can still exist without a
driver, and the PM domain doing all the magic.
> I have no objections to the patch (read: I'm not sufficiently familiar with
> the code to make educated guesses about the side effects of this change ;-).
OK, thanks for clarifiying.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-14 17:19 ` Kevin Hilman
@ 2015-08-16 9:24 ` Geert Uytterhoeven
2015-08-21 21:04 ` Kevin Hilman
0 siblings, 1 reply; 60+ messages in thread
From: Geert Uytterhoeven @ 2015-08-16 9:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Kevin,
On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote:
> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote:
>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>>>> This check might have made sense before PM domains, but with PM domains,
>>>>> it's entirely possible to have a simple device without a driver and the
>>>>> PM domain handles all the necesary PM, so I think this check
>>>>> could/should be removed.
>>>>>
>>>>> Thoughts?
>>>>
>>>> Simple devices without a driver aren't handled automatically.
>>>> At minimum, the driver should call pm_runtime_enable(), cfr.
>>>> drivers/bus/simple-pm-bus.c.
>>>
>>> That's correct, and in the proof-of-concept stuff I hacked up and in
>>> Lina's series, the CPU "devices" do indeed to this. Without that, they
>>> wouldn't end up ever taking this codepath through genpd's
>>> runtime_suspend and power_off hooks.
>>>
>>> Also, I'm not sure if your comment was meant to be an objection to the
>>> patch? or if you're OK with it.
>>
>> My comment was purely meant as a response to "it's entirely possible to have a
>> simple device without a driver and the PM domain handles all the necesary PM".
>
> Right, so if the PM domain does the pm_runtime_enable() for these
> "simple" devices without drivers, they can still exist without a
> driver, and the PM domain doing all the magic.
Is it possible to let the PM Domain do the pm_runtime_enable() itself in
the absence of a driver? If yes, I wouldn't have needed simple-pm-bus.c.
What if a driver is bound later?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-16 9:24 ` Geert Uytterhoeven
@ 2015-08-21 21:04 ` Kevin Hilman
2015-08-24 19:50 ` Lina Iyer
0 siblings, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-21 21:04 UTC (permalink / raw)
To: linux-arm-kernel
Geert Uytterhoeven <geert@linux-m68k.org> writes:
> Hi Kevin,
>
> On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote:
>> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote:
>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>>>>> This check might have made sense before PM domains, but with PM domains,
>>>>>> it's entirely possible to have a simple device without a driver and the
>>>>>> PM domain handles all the necesary PM, so I think this check
>>>>>> could/should be removed.
>>>>>>
>>>>>> Thoughts?
>>>>>
>>>>> Simple devices without a driver aren't handled automatically.
>>>>> At minimum, the driver should call pm_runtime_enable(), cfr.
>>>>> drivers/bus/simple-pm-bus.c.
>>>>
>>>> That's correct, and in the proof-of-concept stuff I hacked up and in
>>>> Lina's series, the CPU "devices" do indeed to this. Without that, they
>>>> wouldn't end up ever taking this codepath through genpd's
>>>> runtime_suspend and power_off hooks.
>>>>
>>>> Also, I'm not sure if your comment was meant to be an objection to the
>>>> patch? or if you're OK with it.
>>>
>>> My comment was purely meant as a response to "it's entirely possible to have a
>>> simple device without a driver and the PM domain handles all the necesary PM".
>>
>> Right, so if the PM domain does the pm_runtime_enable() for these
>> "simple" devices without drivers, they can still exist without a
>> driver, and the PM domain doing all the magic.
>
> Is it possible to let the PM Domain do the pm_runtime_enable() itself in
> the absence of a driver?
Well, I suppose it's possible, not sure it's recommended. :)
> If yes, I wouldn't have needed simple-pm-bus.c.
> What if a driver is bound later?
Yeah, you're approach is better.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-21 21:04 ` Kevin Hilman
@ 2015-08-24 19:50 ` Lina Iyer
2015-08-25 9:24 ` Geert Uytterhoeven
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-24 19:50 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 21 2015 at 15:04 -0600, Kevin Hilman wrote:
>Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> Hi Kevin,
>>
>> On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote:
>>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>>>>>> This check might have made sense before PM domains, but with PM domains,
>>>>>>> it's entirely possible to have a simple device without a driver and the
>>>>>>> PM domain handles all the necesary PM, so I think this check
>>>>>>> could/should be removed.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>
>>>>>> Simple devices without a driver aren't handled automatically.
>>>>>> At minimum, the driver should call pm_runtime_enable(), cfr.
>>>>>> drivers/bus/simple-pm-bus.c.
>>>>>
>>>>> That's correct, and in the proof-of-concept stuff I hacked up and in
>>>>> Lina's series, the CPU "devices" do indeed to this. Without that, they
>>>>> wouldn't end up ever taking this codepath through genpd's
>>>>> runtime_suspend and power_off hooks.
>>>>>
>>>>> Also, I'm not sure if your comment was meant to be an objection to the
>>>>> patch? or if you're OK with it.
>>>>
>>>> My comment was purely meant as a response to "it's entirely possible to have a
>>>> simple device without a driver and the PM domain handles all the necesary PM".
>>>
>>> Right, so if the PM domain does the pm_runtime_enable() for these
>>> "simple" devices without drivers, they can still exist without a
>>> driver, and the PM domain doing all the magic.
>>
>> Is it possible to let the PM Domain do the pm_runtime_enable() itself in
>> the absence of a driver?
>
>Well, I suppose it's possible, not sure it's recommended. :)
>
>> If yes, I wouldn't have needed simple-pm-bus.c.
>> What if a driver is bound later?
>
>Yeah, you're approach is better.
>
I am not sure I understand the approach? Initialize the CPU devices as
"simple-pm-bus" compatible?
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-24 19:50 ` Lina Iyer
@ 2015-08-25 9:24 ` Geert Uytterhoeven
0 siblings, 0 replies; 60+ messages in thread
From: Geert Uytterhoeven @ 2015-08-25 9:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Lina,
On Mon, Aug 24, 2015 at 9:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Fri, Aug 21 2015 at 15:04 -0600, Kevin Hilman wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>> On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote:
>>>> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven
>>>> <geert@linux-m68k.org> wrote:
>>>>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org>
>>>>> wrote:
>>>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org>
>>>>>>> wrote:
>>>>>>>> This check might have made sense before PM domains, but with PM
>>>>>>>> domains,
>>>>>>>> it's entirely possible to have a simple device without a driver and
>>>>>>>> the
>>>>>>>> PM domain handles all the necesary PM, so I think this check
>>>>>>>> could/should be removed.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Simple devices without a driver aren't handled automatically.
>>>>>>> At minimum, the driver should call pm_runtime_enable(), cfr.
>>>>>>> drivers/bus/simple-pm-bus.c.
>>>>>>
>>>>>> That's correct, and in the proof-of-concept stuff I hacked up and in
>>>>>> Lina's series, the CPU "devices" do indeed to this. Without that,
>>>>>> they
>>>>>> wouldn't end up ever taking this codepath through genpd's
>>>>>> runtime_suspend and power_off hooks.
>>>>>>
>>>>>> Also, I'm not sure if your comment was meant to be an objection to the
>>>>>> patch? or if you're OK with it.
>>>>>
>>>>> My comment was purely meant as a response to "it's entirely possible to
>>>>> have a
>>>>> simple device without a driver and the PM domain handles all the
>>>>> necesary PM".
>>>>
>>>> Right, so if the PM domain does the pm_runtime_enable() for these
>>>> "simple" devices without drivers, they can still exist without a
>>>> driver, and the PM domain doing all the magic.
>>>
>>> Is it possible to let the PM Domain do the pm_runtime_enable() itself in
>>> the absence of a driver?
>>
>> Well, I suppose it's possible, not sure it's recommended. :)
>>
>>> If yes, I wouldn't have needed simple-pm-bus.c.
>>> What if a driver is bound later?
>>
>> Yeah, you're approach is better.
>>
> I am not sure I understand the approach? Initialize the CPU devices as
> "simple-pm-bus" compatible?
No, Kevin and I are discussing about transparent buses that need power
management, not about CPU devices.
Cfr. commit 89d463ea106dba53 ("drivers: bus: Add Simple Power-Managed Bus
Driver").
Sorry for the confusion...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM
2015-08-04 23:35 ` [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM Lina Iyer
2015-08-12 19:50 ` Kevin Hilman
@ 2015-09-01 13:28 ` Ulf Hansson
1 sibling, 0 replies; 60+ messages in thread
From: Ulf Hansson @ 2015-09-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel
On 5 August 2015 at 01:35, Lina Iyer <lina.iyer@linaro.org> wrote:
> Remove check for driver of a device, for runtime PM. Device may be
> suspended without an explicit driver. This check seems to be vestigial
> and incorrect in the current context.
>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
I think this makes perfect sense!
Moreover in *theory*, I have considered both cases for how a device
currently gets attached/detached to its genpd and I can't find any
issues with $subject patch. With *both* cases I mean attaching the
device using *pm_genpd_[name]add_device() API or via the ->probe()
sequence using the dev_pm_domain_attach() API.
Now, of course that statement also relies on that drivers behaves
properly from a runtime PM point of view, for example make sure to do
pm_runtime_disable() at ->remove().
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/base/power/domain.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5fd1306..ef8d19f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
> if (stat > PM_QOS_FLAGS_NONE)
> return -EBUSY;
>
> - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
> - || pdd->dev->power.irq_safe))
> + if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> not_suspended++;
> }
>
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 3/9] PM / Domains: Support IRQ safe PM domains
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
2015-08-04 23:35 ` [PATCH 1/9] PM / Domains: Allocate memory outside domain locks Lina Iyer
2015-08-04 23:35 ` [PATCH 2/9] PM / Domains: Remove dev->driver check for runtime PM Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-12 20:12 ` Kevin Hilman
2015-08-12 23:03 ` Stephen Boyd
2015-08-04 23:35 ` [PATCH 4/9] kernel/cpu_pm: fix cpu_cluster_pm_exit comment Lina Iyer
` (5 subsequent siblings)
8 siblings, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Generic Power Domains currently support turning on/off only in process
context. This prevents the usage of PM domains for domains that could be
powered on/off in a context where IRQs are disabled. Many such domains
exist today and do not get powered off, when the IRQ safe devices in
that domain are powered off, because of this limitation.
However, not all domains can operate in IRQ safe contexts. Genpd
therefore, has to support both cases where the domain may or may not
operate in IRQ safe contexts. Configuring genpd to use an appropriate
lock for that domain, would allow domains that have IRQ safe devices to
runtime suspend and resume, in atomic context.
To achieve domain specific locking, set the domain's ->flag to
GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
should use a spinlock instead of a mutex for locking the domain. Locking
is abstracted through genpd_lock() and genpd_unlock() functions that use
the flag to determine the appropriate lock to be used for that domain.
Domains that have lower latency to suspend and resume and can operate
with IRQs disabled may now be able to save power, when the component
devices and sub-domains are idle at runtime.
The restriction this imposes on the domain hierarchy is that sub-domains
and all devices in the IRQ safe domain's hierarchy also have to be IRQ
safe, so that we dont try to lock a mutex, while holding a spinlock.
Non-IRQ safe domains may continue to have devices and sub-domains that
may or may not be IRQ safe.
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Documentation/power/devices.txt | 11 ++-
drivers/base/power/domain.c | 201 +++++++++++++++++++++++++++++++---------
include/linux/pm_domain.h | 11 ++-
3 files changed, 178 insertions(+), 45 deletions(-)
diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index 8ba6625..6d8318c 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -607,7 +607,16 @@ individually. Instead, a set of devices sharing a power resource can be put
into a low-power state together at the same time by turning off the shared
power resource. Of course, they also need to be put into the full-power state
together, by turning the shared power resource on. A set of devices with this
-property is often referred to as a power domain.
+property is often referred to as a power domain. A power domain may also be
+nested inside another power domain.
+
+Devices, by default, operate in process context and if a device can operate in
+IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
+default, operate in process context but could have devices that are IRQ safe.
+Such power domains cannot be powered on/off during runtime PM. On the other
+hand, an IRQ safe PM domain that can be powered on/off and suspend or resumed
+in an atomic context, may contain IRQ safe devices. Such domains may only
+contain IRQ safe devices or IRQ safe sub-domains.
Support for power domains is provided through the pm_domain field of struct
device. This field is a pointer to an object of type struct dev_pm_domain,
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index ef8d19f..221feb0 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -53,6 +53,74 @@
static LIST_HEAD(gpd_list);
static DEFINE_MUTEX(gpd_list_lock);
+static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
+ unsigned int subclass)
+ __acquires(&genpd->slock)
+{
+ unsigned long flags;
+
+ if (subclass > 0)
+ spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+ else
+ spin_lock_irqsave(&genpd->slock, flags);
+
+ genpd->lock_flags = flags;
+ return 0;
+}
+
+static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
+ __releases(&genpd->slock)
+{
+ spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+}
+
+static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
+ unsigned int subclass)
+ __acquires(&genpd->mlock)
+{
+ if (subclass > 0)
+ mutex_lock_nested(&genpd->mlock, subclass);
+ else
+ mutex_lock(&genpd->mlock);
+ return 0;
+}
+
+static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
+ __acquires(&genpd->mlock)
+{
+ return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
+ __releases(&genpd->mlock)
+{
+ mutex_unlock(&genpd->mlock);
+}
+
+static inline int genpd_lock(struct generic_pm_domain *genpd)
+{
+ return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+ : genpd_lock_irq(genpd, 0);
+}
+
+static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
+{
+ return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
+ : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
+}
+
+static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
+{
+ return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
+ : genpd_lock_interruptible_irq(genpd);
+}
+
+static inline void genpd_unlock(struct generic_pm_domain *genpd)
+{
+ return genpd->irq_safe ? genpd_unlock_noirq(genpd)
+ : genpd_unlock_irq(genpd);
+}
+
static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
{
struct generic_pm_domain *genpd = NULL, *gpd;
@@ -273,9 +341,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
{
int ret;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
ret = __pm_genpd_poweron(genpd);
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return ret;
}
@@ -335,9 +403,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
spin_unlock_irq(&dev->power.lock);
if (!IS_ERR(genpd)) {
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
genpd->max_off_time_changed = true;
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
}
dev = dev->parent;
@@ -394,7 +462,13 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
if (stat > PM_QOS_FLAGS_NONE)
return -EBUSY;
- if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
+ /*
+ * We do not want to power off the domain if the device is
+ * not suspended or an IRQ safe device is part of this
+ * non-IRQ safe domain.
+ */
+ if (!pm_runtime_suspended(pdd->dev) ||
+ (pdd->dev->power.irq_safe && !genpd->irq_safe))
not_suspended++;
}
@@ -460,9 +534,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
genpd = container_of(work, struct generic_pm_domain, power_off_work);
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
pm_genpd_poweroff(genpd);
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
}
/**
@@ -500,17 +574,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
}
/*
- * If power.irq_safe is set, this routine will be run with interrupts
- * off, so it can't use mutexes.
+ * If power.irq_safe is set, this routine may be run with
+ * IRQ disabled, so suspend only if the power domain is
+ * irq_safe.
*/
- if (dev->power.irq_safe)
+ if (dev->power.irq_safe && !genpd->irq_safe)
return 0;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
+
genpd->in_progress++;
pm_genpd_poweroff(genpd);
genpd->in_progress--;
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return 0;
}
@@ -535,15 +611,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
if (IS_ERR(genpd))
return -EINVAL;
- /* If power.irq_safe, the PM domain is never powered off. */
- if (dev->power.irq_safe) {
+ /*
+ * As we dont power off a non IRQ safe domain, which holds
+ * an IRQ safe device, we dont need to restore power to it.
+ */
+ if (dev->power.irq_safe && !genpd->irq_safe) {
timed = false;
goto out;
}
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
ret = __pm_genpd_poweron(genpd);
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
if (ret)
return ret;
@@ -743,14 +822,14 @@ static int pm_genpd_prepare(struct device *dev)
if (resume_needed(dev, genpd))
pm_runtime_resume(dev);
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
if (genpd->prepared_count++ == 0) {
genpd->suspended_count = 0;
genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
}
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
if (genpd->suspend_power_off) {
pm_runtime_put_noidle(dev);
@@ -768,12 +847,12 @@ static int pm_genpd_prepare(struct device *dev)
ret = pm_generic_prepare(dev);
if (ret) {
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
if (--genpd->prepared_count == 0)
genpd->suspend_power_off = false;
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
pm_runtime_enable(dev);
}
@@ -1130,13 +1209,13 @@ static void pm_genpd_complete(struct device *dev)
if (IS_ERR(genpd))
return;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
run_complete = !genpd->suspend_power_off;
if (--genpd->prepared_count == 0)
genpd->suspend_power_off = false;
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
if (run_complete) {
pm_generic_complete(dev);
@@ -1280,11 +1359,17 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
return -EINVAL;
+ if (genpd->irq_safe && !dev->power.irq_safe) {
+ dev_err(dev,
+ "Devices in an IRQ safe domain have to be IRQ safe.\n");
+ return -EINVAL;
+ }
+
gpd_data = genpd_alloc_dev_data(dev, genpd, td);
if (IS_ERR(gpd_data))
return PTR_ERR(gpd_data);
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
if (genpd->prepared_count > 0) {
ret = -EAGAIN;
@@ -1301,7 +1386,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
out:
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
if (ret)
genpd_free_dev_data(dev, gpd_data);
@@ -1345,7 +1430,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
gpd_data = to_gpd_data(pdd);
dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
if (genpd->prepared_count > 0) {
ret = -EAGAIN;
@@ -1360,14 +1445,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
list_del_init(&pdd->list_node);
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
genpd_free_dev_data(dev, gpd_data);
return 0;
out:
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
dev_pm_qos_add_notifier(dev, &gpd_data->nb);
return ret;
@@ -1388,12 +1473,24 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
|| genpd == subdomain)
return -EINVAL;
+ /*
+ * If the domain can be powered on/off in an IRQ safe
+ * context, ensure that the subdomain can also be
+ * powered on/off in that context.
+ */
+ if (genpd->irq_safe && !subdomain->irq_safe) {
+ WARN("Sub-domain (%s) in an IRQ-safe domain (%s)"
+ "have to be IRQ safe\n",
+ subdomain->name, genpd->name);
+ return -EINVAL;
+ }
+
link = kzalloc(sizeof(*link), GFP_KERNEL);
if (!link)
return -ENOMEM;
- mutex_lock(&genpd->lock);
- mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+ genpd_lock(genpd);
+ genpd_lock_nested(subdomain);
if (genpd->status == GPD_STATE_POWER_OFF
&& subdomain->status != GPD_STATE_POWER_OFF) {
@@ -1416,8 +1513,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
genpd_sd_counter_inc(genpd);
out:
- mutex_unlock(&subdomain->lock);
- mutex_unlock(&genpd->lock);
+ genpd_unlock(subdomain);
+ genpd_unlock(genpd);
if (ret)
kfree(link);
return ret;
@@ -1466,13 +1563,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
return -EINVAL;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
list_for_each_entry(link, &genpd->master_links, master_node) {
if (link->slave != subdomain)
continue;
- mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+ genpd_lock_nested(subdomain);
list_del(&link->master_node);
list_del(&link->slave_node);
@@ -1480,13 +1577,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
if (subdomain->status != GPD_STATE_POWER_OFF)
genpd_sd_counter_dec(genpd);
- mutex_unlock(&subdomain->lock);
+ genpd_unlock(subdomain);
ret = 0;
break;
}
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return ret;
}
@@ -1510,11 +1607,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
if (IS_ERR_OR_NULL(genpd) || state < 0)
return -EINVAL;
+ if (genpd->irq_safe) {
+ WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
+ genpd->name);
+ return -EINVAL;
+ }
+
cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
if (!cpuidle_data)
return -ENOMEM;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
if (genpd->cpuidle_data) {
ret = -EEXIST;
@@ -1541,7 +1644,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
genpd_recalc_cpu_exit_latency(genpd);
out:
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return ret;
err:
@@ -1578,7 +1681,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
if (IS_ERR_OR_NULL(genpd))
return -EINVAL;
- mutex_lock(&genpd->lock);
+ genpd_lock(genpd);
cpuidle_data = genpd->cpuidle_data;
if (!cpuidle_data) {
@@ -1596,7 +1699,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
kfree(cpuidle_data);
out:
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return ret;
}
@@ -1657,6 +1760,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
return cb ? cb(dev) : 0;
}
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+ if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+ spin_lock_init(&genpd->slock);
+ genpd->irq_safe = true;
+ } else {
+ mutex_init(&genpd->mlock);
+ genpd->irq_safe = false;
+ }
+}
+
/**
* pm_genpd_init - Initialize a generic I/O PM domain object.
* @genpd: PM domain object to initialize.
@@ -1672,7 +1786,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
INIT_LIST_HEAD(&genpd->master_links);
INIT_LIST_HEAD(&genpd->slave_links);
INIT_LIST_HEAD(&genpd->dev_list);
- mutex_init(&genpd->lock);
+ genpd_lock_init(genpd);
genpd->gov = gov;
INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
genpd->in_progress = 0;
@@ -2067,7 +2181,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
struct gpd_link *link;
int ret;
- ret = mutex_lock_interruptible(&genpd->lock);
+ ret = genpd_lock_interruptible(genpd);
if (ret)
return -ERESTARTSYS;
@@ -2087,7 +2201,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
}
list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
- kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+ kobj_path = kobject_get_path(&pm_data->dev->kobj,
+ genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
if (kobj_path == NULL)
continue;
@@ -2098,7 +2213,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
seq_puts(s, "\n");
exit:
- mutex_unlock(&genpd->lock);
+ genpd_unlock(genpd);
return 0;
}
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b2725e6..dc7cb53 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -16,9 +16,11 @@
#include <linux/of.h>
#include <linux/notifier.h>
#include <linux/cpuidle.h>
+#include <linux/spinlock.h>
/* Defines used for the flags field in the struct generic_pm_domain */
#define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */
+#define GENPD_FLAG_IRQ_SAFE (1U << 1) /* PM domain operates in atomic */
enum gpd_status {
GPD_STATE_ACTIVE = 0, /* PM domain is active */
@@ -49,7 +51,6 @@ struct generic_pm_domain {
struct list_head master_links; /* Links with PM domain as a master */
struct list_head slave_links; /* Links with PM domain as a slave */
struct list_head dev_list; /* List of devices */
- struct mutex lock;
struct dev_power_governor *gov;
struct work_struct power_off_work;
const char *name;
@@ -74,6 +75,14 @@ struct generic_pm_domain {
void (*detach_dev)(struct generic_pm_domain *domain,
struct device *dev);
unsigned int flags; /* Bit field of configs for genpd */
+ bool irq_safe;
+ union {
+ struct mutex mlock;
+ struct {
+ spinlock_t slock;
+ unsigned long lock_flags;
+ };
+ };
};
static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 3/9] PM / Domains: Support IRQ safe PM domains
2015-08-04 23:35 ` [PATCH 3/9] PM / Domains: Support IRQ safe PM domains Lina Iyer
@ 2015-08-12 20:12 ` Kevin Hilman
2015-08-12 20:47 ` Lina Iyer
2015-08-12 23:03 ` Stephen Boyd
1 sibling, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-12 20:12 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.
>
> However, not all domains can operate in IRQ safe contexts. Genpd
> therefore, has to support both cases where the domain may or may not
> operate in IRQ safe contexts. Configuring genpd to use an appropriate
> lock for that domain, would allow domains that have IRQ safe devices to
> runtime suspend and resume, in atomic context.
>
> To achieve domain specific locking, set the domain's ->flag to
> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
> should use a spinlock instead of a mutex for locking the domain. Locking
> is abstracted through genpd_lock() and genpd_unlock() functions that use
> the flag to determine the appropriate lock to be used for that domain.
> Domains that have lower latency to suspend and resume and can operate
> with IRQs disabled may now be able to save power, when the component
> devices and sub-domains are idle at runtime.
>
> The restriction this imposes on the domain hierarchy is that sub-domains
> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
> safe, so that we dont try to lock a mutex, while holding a spinlock.
> Non-IRQ safe domains may continue to have devices and sub-domains that
> may or may not be IRQ safe.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Approach seems good to me, some cosmetic issues below...
> ---
> Documentation/power/devices.txt | 11 ++-
> drivers/base/power/domain.c | 201 +++++++++++++++++++++++++++++++---------
> include/linux/pm_domain.h | 11 ++-
> 3 files changed, 178 insertions(+), 45 deletions(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..6d8318c 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,16 @@ individually. Instead, a set of devices sharing a power resource can be put
> into a low-power state together at the same time by turning off the shared
> power resource. Of course, they also need to be put into the full-power state
> together, by turning the shared power resource on. A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another power domain.
> +
> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
> +default, operate in process context but could have devices that are IRQ safe.
> +Such power domains cannot be powered on/off during runtime PM. On the other
> +hand, an IRQ safe PM domain that can be powered on/off and suspend or resumed
> +in an atomic context, may contain IRQ safe devices. Such domains may only
> +contain IRQ safe devices or IRQ safe sub-domains.
>
> Support for power domains is provided through the pm_domain field of struct
> device. This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ef8d19f..221feb0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -53,6 +53,74 @@
> static LIST_HEAD(gpd_list);
> static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
> + unsigned int subclass)
I don't like the _noirq naming, as in the context of suspend/resume,
that means no *device* IRQs, not no IRQs at all. Maybe _atomic is
better, or _nosleep? or ...
> + __acquires(&genpd->slock)
> +{
> + unsigned long flags;
> +
> + if (subclass > 0)
> + spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
> + else
> + spin_lock_irqsave(&genpd->slock, flags);
> +
> + genpd->lock_flags = flags;
> + return 0;
> +}
> +
> +static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
> + __releases(&genpd->slock)
> +{
> + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
> +}
> +
> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> + unsigned int subclass)
> + __acquires(&genpd->mlock)
> +{
> + if (subclass > 0)
> + mutex_lock_nested(&genpd->mlock, subclass);
> + else
> + mutex_lock(&genpd->mlock);
> + return 0;
> +}
> +
> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
> + __acquires(&genpd->mlock)
> +{
> + return mutex_lock_interruptible(&genpd->mlock);
> +}
> +
> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
> + __releases(&genpd->mlock)
> +{
> + mutex_unlock(&genpd->mlock);
> +}
> +
> +static inline int genpd_lock(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
> + : genpd_lock_irq(genpd, 0);
> +}
> +
> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
> + : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
> +}
> +
> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
> + : genpd_lock_interruptible_irq(genpd);
> +}
> +
> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_unlock_noirq(genpd)
> + : genpd_unlock_irq(genpd);
> +}
> +
> static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> {
> struct generic_pm_domain *genpd = NULL, *gpd;
> @@ -273,9 +341,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
> {
> int ret;
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
> ret = __pm_genpd_poweron(genpd);
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
> return ret;
> }
>
> @@ -335,9 +403,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> spin_unlock_irq(&dev->power.lock);
>
> if (!IS_ERR(genpd)) {
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
> genpd->max_off_time_changed = true;
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
> }
>
> dev = dev->parent;
> @@ -394,7 +462,13 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
> if (stat > PM_QOS_FLAGS_NONE)
> return -EBUSY;
>
> - if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> + /*
> + * We do not want to power off the domain if the device is
> + * not suspended or an IRQ safe device is part of this
> + * non-IRQ safe domain.
> + */
> + if (!pm_runtime_suspended(pdd->dev) ||
> + (pdd->dev->power.irq_safe && !genpd->irq_safe))
> not_suspended++;
The !irq_safe check should maybe spit a warning via WARN_ONCE(), as this
would be useful in debugging domains that are not shutting down.
> }
>
> @@ -460,9 +534,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>
> genpd = container_of(work, struct generic_pm_domain, power_off_work);
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
> pm_genpd_poweroff(genpd);
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
> }
>
> /**
> @@ -500,17 +574,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
> }
>
> /*
> - * If power.irq_safe is set, this routine will be run with interrupts
> - * off, so it can't use mutexes.
> + * If power.irq_safe is set, this routine may be run with
> + * IRQ disabled, so suspend only if the power domain is
> + * irq_safe.
> */
> - if (dev->power.irq_safe)
> + if (dev->power.irq_safe && !genpd->irq_safe)
> return 0;
Similarily, a WARN_ONCE() will probably be helpful here.
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
> +
> genpd->in_progress++;
> pm_genpd_poweroff(genpd);
> genpd->in_progress--;
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> return 0;
> }
> @@ -535,15 +611,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
> if (IS_ERR(genpd))
> return -EINVAL;
>
> - /* If power.irq_safe, the PM domain is never powered off. */
> - if (dev->power.irq_safe) {
> + /*
> + * As we dont power off a non IRQ safe domain, which holds
> + * an IRQ safe device, we dont need to restore power to it.
> + */
> + if (dev->power.irq_safe && !genpd->irq_safe) {
> timed = false;
> goto out;
> }
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
> ret = __pm_genpd_poweron(genpd);
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> if (ret)
> return ret;
> @@ -743,14 +822,14 @@ static int pm_genpd_prepare(struct device *dev)
> if (resume_needed(dev, genpd))
> pm_runtime_resume(dev);
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
>
> if (genpd->prepared_count++ == 0) {
> genpd->suspended_count = 0;
> genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
> }
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> if (genpd->suspend_power_off) {
> pm_runtime_put_noidle(dev);
> @@ -768,12 +847,12 @@ static int pm_genpd_prepare(struct device *dev)
>
> ret = pm_generic_prepare(dev);
> if (ret) {
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
>
> if (--genpd->prepared_count == 0)
> genpd->suspend_power_off = false;
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
> pm_runtime_enable(dev);
> }
>
> @@ -1130,13 +1209,13 @@ static void pm_genpd_complete(struct device *dev)
> if (IS_ERR(genpd))
> return;
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
>
> run_complete = !genpd->suspend_power_off;
> if (--genpd->prepared_count == 0)
> genpd->suspend_power_off = false;
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> if (run_complete) {
> pm_generic_complete(dev);
> @@ -1280,11 +1359,17 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
> return -EINVAL;
>
> + if (genpd->irq_safe && !dev->power.irq_safe) {
> + dev_err(dev,
> + "Devices in an IRQ safe domain have to be IRQ safe.\n");
This should also dump the PM domain, something like:
dev_err(dev, "PM domain %s is IRQ safe; device has to be IRQ safe.\n")
> + return -EINVAL;
> + }
> +
> gpd_data = genpd_alloc_dev_data(dev, genpd, td);
> if (IS_ERR(gpd_data))
> return PTR_ERR(gpd_data);
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
>
> if (genpd->prepared_count > 0) {
> ret = -EAGAIN;
> @@ -1301,7 +1386,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
> list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>
> out:
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> if (ret)
> genpd_free_dev_data(dev, gpd_data);
> @@ -1345,7 +1430,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
> gpd_data = to_gpd_data(pdd);
> dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>
> - mutex_lock(&genpd->lock);
> + genpd_lock(genpd);
>
> if (genpd->prepared_count > 0) {
> ret = -EAGAIN;
> @@ -1360,14 +1445,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
> list_del_init(&pdd->list_node);
>
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
>
> genpd_free_dev_data(dev, gpd_data);
>
> return 0;
>
> out:
> - mutex_unlock(&genpd->lock);
> + genpd_unlock(genpd);
> dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>
> return ret;
> @@ -1388,12 +1473,24 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> || genpd == subdomain)
> return -EINVAL;
>
> + /*
> + * If the domain can be powered on/off in an IRQ safe
> + * context, ensure that the subdomain can also be
> + * powered on/off in that context.
> + */
> + if (genpd->irq_safe && !subdomain->irq_safe) {
> + WARN("Sub-domain (%s) in an IRQ-safe domain (%s)"
> + "have to be IRQ safe\n",
s/have/has/
> + subdomain->name, genpd->name);
> + return -EINVAL;
> + }
> +
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 3/9] PM / Domains: Support IRQ safe PM domains
2015-08-12 20:12 ` Kevin Hilman
@ 2015-08-12 20:47 ` Lina Iyer
0 siblings, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-12 20:47 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 12 2015 at 14:12 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> Generic Power Domains currently support turning on/off only in process
>> context. This prevents the usage of PM domains for domains that could be
>> powered on/off in a context where IRQs are disabled. Many such domains
>> exist today and do not get powered off, when the IRQ safe devices in
>> that domain are powered off, because of this limitation.
>>
>> However, not all domains can operate in IRQ safe contexts. Genpd
>> therefore, has to support both cases where the domain may or may not
>> operate in IRQ safe contexts. Configuring genpd to use an appropriate
>> lock for that domain, would allow domains that have IRQ safe devices to
>> runtime suspend and resume, in atomic context.
>>
>> To achieve domain specific locking, set the domain's ->flag to
>> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
>> should use a spinlock instead of a mutex for locking the domain. Locking
>> is abstracted through genpd_lock() and genpd_unlock() functions that use
>> the flag to determine the appropriate lock to be used for that domain.
>> Domains that have lower latency to suspend and resume and can operate
>> with IRQs disabled may now be able to save power, when the component
>> devices and sub-domains are idle at runtime.
>>
>> The restriction this imposes on the domain hierarchy is that sub-domains
>> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
>> safe, so that we dont try to lock a mutex, while holding a spinlock.
>> Non-IRQ safe domains may continue to have devices and sub-domains that
>> may or may not be IRQ safe.
>>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
>Approach seems good to me, some cosmetic issues below...
>
Thanks for your review.
>> ---
>> Documentation/power/devices.txt | 11 ++-
>> drivers/base/power/domain.c | 201 +++++++++++++++++++++++++++++++---------
>> include/linux/pm_domain.h | 11 ++-
>> 3 files changed, 178 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
>> index 8ba6625..6d8318c 100644
>> --- a/Documentation/power/devices.txt
>> +++ b/Documentation/power/devices.txt
>> @@ -607,7 +607,16 @@ individually. Instead, a set of devices sharing a power resource can be put
>> into a low-power state together at the same time by turning off the shared
>> power resource. Of course, they also need to be put into the full-power state
>> together, by turning the shared power resource on. A set of devices with this
>> -property is often referred to as a power domain.
>> +property is often referred to as a power domain. A power domain may also be
>> +nested inside another power domain.
>> +
>> +Devices, by default, operate in process context and if a device can operate in
>> +IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
>> +default, operate in process context but could have devices that are IRQ safe.
>> +Such power domains cannot be powered on/off during runtime PM. On the other
>> +hand, an IRQ safe PM domain that can be powered on/off and suspend or resumed
>> +in an atomic context, may contain IRQ safe devices. Such domains may only
>> +contain IRQ safe devices or IRQ safe sub-domains.
>>
>> Support for power domains is provided through the pm_domain field of struct
>> device. This field is a pointer to an object of type struct dev_pm_domain,
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index ef8d19f..221feb0 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -53,6 +53,74 @@
>> static LIST_HEAD(gpd_list);
>> static DEFINE_MUTEX(gpd_list_lock);
>>
>> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
>> + unsigned int subclass)
>
>I don't like the _noirq naming, as in the context of suspend/resume,
>that means no *device* IRQs, not no IRQs at all. Maybe _atomic is
>better, or _nosleep? or ...
>
I like _nosleep, will change it.
>> + __acquires(&genpd->slock)
>> +{
>> + unsigned long flags;
>> +
>> + if (subclass > 0)
>> + spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
>> + else
>> + spin_lock_irqsave(&genpd->slock, flags);
>> +
>> + genpd->lock_flags = flags;
>> + return 0;
>> +}
>> +
>> +static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
>> + __releases(&genpd->slock)
>> +{
>> + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
>> +}
>> +
>> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
>> + unsigned int subclass)
>> + __acquires(&genpd->mlock)
>> +{
>> + if (subclass > 0)
>> + mutex_lock_nested(&genpd->mlock, subclass);
>> + else
>> + mutex_lock(&genpd->mlock);
>> + return 0;
>> +}
>> +
>> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
>> + __acquires(&genpd->mlock)
>> +{
>> + return mutex_lock_interruptible(&genpd->mlock);
>> +}
>> +
>> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
>> + __releases(&genpd->mlock)
>> +{
>> + mutex_unlock(&genpd->mlock);
>> +}
>> +
>> +static inline int genpd_lock(struct generic_pm_domain *genpd)
>> +{
>> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
>> + : genpd_lock_irq(genpd, 0);
>> +}
>> +
>> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
>> +{
>> + return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
>> + : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
>> +}
>> +
>> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
>> +{
>> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
>> + : genpd_lock_interruptible_irq(genpd);
>> +}
>> +
>> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
>> +{
>> + return genpd->irq_safe ? genpd_unlock_noirq(genpd)
>> + : genpd_unlock_irq(genpd);
>> +}
>> +
>> static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>> {
>> struct generic_pm_domain *genpd = NULL, *gpd;
>> @@ -273,9 +341,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
>> {
>> int ret;
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>> ret = __pm_genpd_poweron(genpd);
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>> return ret;
>> }
>>
>> @@ -335,9 +403,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>> spin_unlock_irq(&dev->power.lock);
>>
>> if (!IS_ERR(genpd)) {
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>> genpd->max_off_time_changed = true;
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>> }
>>
>> dev = dev->parent;
>> @@ -394,7 +462,13 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>> if (stat > PM_QOS_FLAGS_NONE)
>> return -EBUSY;
>>
>> - if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
>> + /*
>> + * We do not want to power off the domain if the device is
>> + * not suspended or an IRQ safe device is part of this
>> + * non-IRQ safe domain.
>> + */
>> + if (!pm_runtime_suspended(pdd->dev) ||
>> + (pdd->dev->power.irq_safe && !genpd->irq_safe))
>> not_suspended++;
>
>The !irq_safe check should maybe spit a warning via WARN_ONCE(), as this
>would be useful in debugging domains that are not shutting down.
>
>> }
I thought about putting it before, but I figured it might print only
once for all the genpd/devices. So there might be many cases that may
not be reported. Is my assumption of the __section usage for this macro,
correct?
>>
>> @@ -460,9 +534,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>>
>> genpd = container_of(work, struct generic_pm_domain, power_off_work);
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>> pm_genpd_poweroff(genpd);
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>> }
>>
>> /**
>> @@ -500,17 +574,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>> }
>>
>> /*
>> - * If power.irq_safe is set, this routine will be run with interrupts
>> - * off, so it can't use mutexes.
>> + * If power.irq_safe is set, this routine may be run with
>> + * IRQ disabled, so suspend only if the power domain is
>> + * irq_safe.
>> */
>> - if (dev->power.irq_safe)
>> + if (dev->power.irq_safe && !genpd->irq_safe)
>> return 0;
>
>Similarily, a WARN_ONCE() will probably be helpful here.
>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>> +
>> genpd->in_progress++;
>> pm_genpd_poweroff(genpd);
>> genpd->in_progress--;
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> return 0;
>> }
>> @@ -535,15 +611,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>> if (IS_ERR(genpd))
>> return -EINVAL;
>>
>> - /* If power.irq_safe, the PM domain is never powered off. */
>> - if (dev->power.irq_safe) {
>> + /*
>> + * As we dont power off a non IRQ safe domain, which holds
>> + * an IRQ safe device, we dont need to restore power to it.
>> + */
>> + if (dev->power.irq_safe && !genpd->irq_safe) {
>> timed = false;
>> goto out;
>> }
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>> ret = __pm_genpd_poweron(genpd);
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> if (ret)
>> return ret;
>> @@ -743,14 +822,14 @@ static int pm_genpd_prepare(struct device *dev)
>> if (resume_needed(dev, genpd))
>> pm_runtime_resume(dev);
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>>
>> if (genpd->prepared_count++ == 0) {
>> genpd->suspended_count = 0;
>> genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
>> }
>>
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> if (genpd->suspend_power_off) {
>> pm_runtime_put_noidle(dev);
>> @@ -768,12 +847,12 @@ static int pm_genpd_prepare(struct device *dev)
>>
>> ret = pm_generic_prepare(dev);
>> if (ret) {
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>>
>> if (--genpd->prepared_count == 0)
>> genpd->suspend_power_off = false;
>>
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>> pm_runtime_enable(dev);
>> }
>>
>> @@ -1130,13 +1209,13 @@ static void pm_genpd_complete(struct device *dev)
>> if (IS_ERR(genpd))
>> return;
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>>
>> run_complete = !genpd->suspend_power_off;
>> if (--genpd->prepared_count == 0)
>> genpd->suspend_power_off = false;
>>
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> if (run_complete) {
>> pm_generic_complete(dev);
>> @@ -1280,11 +1359,17 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>> if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>> return -EINVAL;
>>
>> + if (genpd->irq_safe && !dev->power.irq_safe) {
>> + dev_err(dev,
>> + "Devices in an IRQ safe domain have to be IRQ safe.\n");
>
>This should also dump the PM domain, something like:
>
> dev_err(dev, "PM domain %s is IRQ safe; device has to be IRQ safe.\n")
>
Sure.
>> + return -EINVAL;
>> + }
>> +
>> gpd_data = genpd_alloc_dev_data(dev, genpd, td);
>> if (IS_ERR(gpd_data))
>> return PTR_ERR(gpd_data);
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>>
>> if (genpd->prepared_count > 0) {
>> ret = -EAGAIN;
>> @@ -1301,7 +1386,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>> list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>>
>> out:
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> if (ret)
>> genpd_free_dev_data(dev, gpd_data);
>> @@ -1345,7 +1430,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>> gpd_data = to_gpd_data(pdd);
>> dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>>
>> - mutex_lock(&genpd->lock);
>> + genpd_lock(genpd);
>>
>> if (genpd->prepared_count > 0) {
>> ret = -EAGAIN;
>> @@ -1360,14 +1445,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>
>> list_del_init(&pdd->list_node);
>>
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>>
>> genpd_free_dev_data(dev, gpd_data);
>>
>> return 0;
>>
>> out:
>> - mutex_unlock(&genpd->lock);
>> + genpd_unlock(genpd);
>> dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>>
>> return ret;
>> @@ -1388,12 +1473,24 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>> || genpd == subdomain)
>> return -EINVAL;
>>
>> + /*
>> + * If the domain can be powered on/off in an IRQ safe
>> + * context, ensure that the subdomain can also be
>> + * powered on/off in that context.
>> + */
>> + if (genpd->irq_safe && !subdomain->irq_safe) {
>> + WARN("Sub-domain (%s) in an IRQ-safe domain (%s)"
>> + "have to be IRQ safe\n",
>
>s/have/has/
>
Argh. Fixed.
>> + subdomain->name, genpd->name);
>> + return -EINVAL;
>> + }
>> +
>
>Kevin
Thank you again for looking through this big patch.
-- Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 3/9] PM / Domains: Support IRQ safe PM domains
2015-08-04 23:35 ` [PATCH 3/9] PM / Domains: Support IRQ safe PM domains Lina Iyer
2015-08-12 20:12 ` Kevin Hilman
@ 2015-08-12 23:03 ` Stephen Boyd
1 sibling, 0 replies; 60+ messages in thread
From: Stephen Boyd @ 2015-08-12 23:03 UTC (permalink / raw)
To: linux-arm-kernel
On 08/04, Lina Iyer wrote:
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..6d8318c 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,16 @@ individually. Instead, a set of devices sharing a power resource can be put
> into a low-power state together at the same time by turning off the shared
> power resource. Of course, they also need to be put into the full-power state
> together, by turning the shared power resource on. A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another power domain.
> +
> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
> +default, operate in process context but could have devices that are IRQ safe.
> +Such power domains cannot be powered on/off during runtime PM. On the other
> +hand, an IRQ safe PM domain that can be powered on/off and suspend or resumed
s/suspend/suspended/?
> +in an atomic context, may contain IRQ safe devices. Such domains may only
> +contain IRQ safe devices or IRQ safe sub-domains.
>
> Support for power domains is provided through the pm_domain field of struct
> device. This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ef8d19f..221feb0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -53,6 +53,74 @@
> static LIST_HEAD(gpd_list);
> static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_noirq(struct generic_pm_domain *genpd,
> + unsigned int subclass)
> + __acquires(&genpd->slock)
> +{
> + unsigned long flags;
> +
> + if (subclass > 0)
> + spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
> + else
> + spin_lock_irqsave(&genpd->slock, flags);
> +
> + genpd->lock_flags = flags;
> + return 0;
Why return anything at all?
> +}
> +
> +static inline void genpd_unlock_noirq(struct generic_pm_domain *genpd)
> + __releases(&genpd->slock)
> +{
> + spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
> +}
> +
> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> + unsigned int subclass)
> + __acquires(&genpd->mlock)
> +{
> + if (subclass > 0)
> + mutex_lock_nested(&genpd->mlock, subclass);
> + else
> + mutex_lock(&genpd->mlock);
> + return 0;
Same here.
> +}
> +
> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
> + __acquires(&genpd->mlock)
> +{
> + return mutex_lock_interruptible(&genpd->mlock);
> +}
> +
> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
> + __releases(&genpd->mlock)
> +{
> + mutex_unlock(&genpd->mlock);
> +}
> +
> +static inline int genpd_lock(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
> + : genpd_lock_irq(genpd, 0);
> +}
> +
> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, SINGLE_DEPTH_NESTING)
> + : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
> +}
And for all these functions. The return is always 0.
> +
> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_lock_noirq(genpd, 0)
> + : genpd_lock_interruptible_irq(genpd);
> +}
I guess this is the only one that matters.
> +
> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
> +{
> + return genpd->irq_safe ? genpd_unlock_noirq(genpd)
> + : genpd_unlock_irq(genpd);
> +}
And this one again always returns 0?
> +
> static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
> {
> struct generic_pm_domain *genpd = NULL, *gpd;
> @@ -535,15 +611,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
> if (IS_ERR(genpd))
> return -EINVAL;
>
> - /* If power.irq_safe, the PM domain is never powered off. */
> - if (dev->power.irq_safe) {
> + /*
> + * As we dont power off a non IRQ safe domain, which holds
> + * an IRQ safe device, we dont need to restore power to it.
> + */
> + if (dev->power.irq_safe && !genpd->irq_safe) {
This same statement where we check dev for irq_safe and genpd for
!irq_safe has happened three times now. Maybe it should be some
sort of helper function?
if (irq_safe_device_in_no_sleep_domain(dev, genpd))
or something?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 4/9] kernel/cpu_pm: fix cpu_cluster_pm_exit comment
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (2 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 3/9] PM / Domains: Support IRQ safe PM domains Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-12 20:13 ` Kevin Hilman
2015-08-04 23:35 ` [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters Lina Iyer
` (4 subsequent siblings)
8 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
cpu_cluster_pm_exit() must be sent after cpu_cluster_pm_enter() has been
sent for the cluster and before any cpu_pm_exit() notifications are sent
for any CPU.
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
kernel/cpu_pm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 9656a3c..009cc9a 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -180,7 +180,7 @@ EXPORT_SYMBOL_GPL(cpu_cluster_pm_enter);
* low power state that may have caused some blocks in the same power domain
* to reset.
*
- * Must be called after cpu_pm_exit has been called on all cpus in the power
+ * Must be called after cpu_cluster_pm_enter has been called for the power
* domain, and before cpu_pm_exit has been called on any cpu in the power
* domain. Notified drivers can include VFP co-processor, interrupt controller
* and its PM extensions, local CPU timers context save/restore which
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 4/9] kernel/cpu_pm: fix cpu_cluster_pm_exit comment
2015-08-04 23:35 ` [PATCH 4/9] kernel/cpu_pm: fix cpu_cluster_pm_exit comment Lina Iyer
@ 2015-08-12 20:13 ` Kevin Hilman
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Hilman @ 2015-08-12 20:13 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> cpu_cluster_pm_exit() must be sent after cpu_cluster_pm_enter() has been
> sent for the cluster and before any cpu_pm_exit() notifications are sent
> for any CPU.
>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Acked-by: Kevin Hilman <khilman@linaro.org>
> ---
> kernel/cpu_pm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index 9656a3c..009cc9a 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -180,7 +180,7 @@ EXPORT_SYMBOL_GPL(cpu_cluster_pm_enter);
> * low power state that may have caused some blocks in the same power domain
> * to reset.
> *
> - * Must be called after cpu_pm_exit has been called on all cpus in the power
> + * Must be called after cpu_cluster_pm_enter has been called for the power
> * domain, and before cpu_pm_exit has been called on any cpu in the power
> * domain. Notified drivers can include VFP co-processor, interrupt controller
> * and its PM extensions, local CPU timers context save/restore which
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (3 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 4/9] kernel/cpu_pm: fix cpu_cluster_pm_exit comment Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-06 3:14 ` Rob Herring
2015-08-04 23:35 ` [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains Lina Iyer
` (3 subsequent siblings)
8 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
SoCs group CPUs as clusters. Clusters share common resources like GIC,
power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
idle, these shared resources may also be put in their idle state.
The idle time between the last CPU entering idle and a CPU resuming
execution is an opportunity for these shared resources to be powered
down. Generic PM domain provides a framework for defining such power
domains and attach devices to the domain. When the devices in the domain
are idle at runtime, the domain would also be suspended and resumed
before the first of the devices resume execution.
We define a generic PM domain for each cluster and attach CPU devices in
the cluster to that PM domain. The DT definitions for the SoC describe
this relationship. Genpd callbacks for power_on and power_off can then
be used to power up/down the shared resources for the domain.
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Documentation/arm/cpu-domains.txt | 49 ++++++
.../devicetree/bindings/arm/cpudomains.txt | 23 +++
arch/arm/common/Makefile | 1 +
arch/arm/common/domains.c | 166 +++++++++++++++++++++
4 files changed, 239 insertions(+)
create mode 100644 Documentation/arm/cpu-domains.txt
create mode 100644 Documentation/devicetree/bindings/arm/cpudomains.txt
create mode 100644 arch/arm/common/domains.c
diff --git a/Documentation/arm/cpu-domains.txt b/Documentation/arm/cpu-domains.txt
new file mode 100644
index 0000000..3e535b7
--- /dev/null
+++ b/Documentation/arm/cpu-domains.txt
@@ -0,0 +1,49 @@
+CPU Clusters and PM domain
+
+Newer ARM CPUs are grouped in a SoC as clusters. A cluster in addition to the
+CPUs may have caches, GIC, VFP and architecture specific power controller to
+power the cluster. A cluster may also be nested in another cluster, the
+hierarchy of which is depicted in the device tree. CPUIdle frameworks enables
+the CPUs to determine the sleep time and enter low power state to save power
+during periods of idle. CPUs in a cluster may enter and exit idle state
+independently. During the time when all the CPUs are in idle state, the
+cluster can safely be in idle state as well. When the last of the CPUs is
+powered off as a result of idle, the cluster may also be powered down, but the
+domain must be powered on before the first of the CPUs in the cluster resumes
+execution.
+
+ARM SoCs can power down the CPU and resume execution in a few uSecs and the
+domain that powers the CPU cluster also have comparable idle latencies. The
+ARM CPU WFI signal is used as a hardware trigger for the cluster hardware to
+enter their idle state. The hardware can be programmed in advance to put the
+cluster in the desired idle state befitting the wakeup latency requested by
+the CPUs. When all the CPUs in a cluster have executed their WFI instruction,
+the state machine for the power controller may put the cluster components in
+their power down or idle state. Generally, the domains would power on with the
+hardware sensing the CPU's interrupts. The domains may however, need to be
+reconfigured by the CPU to remain active, until the last CPU is ready to enter
+idle again. To power down a cluster, it is generally required to power down
+all the CPUs. The caches would also need to be flushed. The hardware state of
+some of the components may need to be saved and restored when powered back on.
+SoC vendors may also have hardware specific configuration that must be done
+before the cluster can be powered off. When the cluster is powered off,
+notifications may be sent out to other SoC components to scale down or even
+power off their resources.
+
+Power management domains represent relationship of devices and their power
+controllers. They are represented in the DT as domain consumers and providers.
+A device may have a domain provider and a domain provider may support multiple
+domain consumers. Domains like clusters, may also be nested inside one
+another. A domain that has no active consumer, may be powered off and any
+resuming consumer would trigger the domain back to active. Parent domains may
+be powered off when the child domains are powered off. ARM CPU cluster can be
+fashioned as a PM domain. When the CPU devices are powered off, the PM domain
+may be powered off.
+
+The code in Generic PM domains handles the hierarchy of devices, domains and
+the reference counting of objects leading to last man down and first man up.
+The ARM CPU domains common code defines PM domains for each CPU cluster and
+attaches the domains' CPU devices to as specified in the DT. This happens
+automatically at kernel init, when the domain is specified as compatible with
+"arm,pd". Powering on/off the common cluster hardware would also be done when
+the PM domain is runtime suspended or resumed.
diff --git a/Documentation/devicetree/bindings/arm/cpudomains.txt b/Documentation/devicetree/bindings/arm/cpudomains.txt
new file mode 100644
index 0000000..d945861
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cpudomains.txt
@@ -0,0 +1,23 @@
+ARM CPU Power domains
+
+The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
+CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
+caches, VFP and power controller and other peripheral hardware. Generally,
+when the CPUs in the cluster are idle/suspended, the shared resources may also
+be suspended and resumed before any of the CPUs resume execution.
+
+CPUs are the defined as the PM domain consumers and there is a PM domain
+provider for the CPUs. Bindings for generic PM domains (genpd) is described in
+[1].
+
+The ARM CPU PM domain follows the same binding convention as any generic PM
+domain. Additional binding properties are -
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: Must also have
+ "arm,pd"
+ inorder to initialize the genpd provider as ARM CPU PM domain.
+
+[1]. Documentation/devicetree/bindings/power/power_domain.txt
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index 6ee5959..e2e2c63 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -18,3 +18,4 @@ AFLAGS_vlock.o := -march=armv7-a
obj-$(CONFIG_TI_PRIV_EDMA) += edma.o
obj-$(CONFIG_BL_SWITCHER) += bL_switcher.o
obj-$(CONFIG_BL_SWITCHER_DUMMY_IF) += bL_switcher_dummy_if.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += domains.o
diff --git a/arch/arm/common/domains.c b/arch/arm/common/domains.c
new file mode 100644
index 0000000..15981e9
--- /dev/null
+++ b/arch/arm/common/domains.c
@@ -0,0 +1,166 @@
+/*
+ * ARM CPU Generic PM Domain.
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#define NAME_MAX 36
+
+struct arm_pm_domain {
+ struct generic_pm_domain genpd;
+};
+
+static inline
+struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
+{
+ return container_of(d, struct arm_pm_domain, genpd);
+}
+
+static int arm_pd_power_down(struct generic_pm_domain *genpd)
+{
+ /*
+ * Notify CPU PM domain power down
+ * TODO: Call the notificated directly from here.
+ */
+ cpu_cluster_pm_enter();
+
+ return 0;
+}
+
+static int arm_pd_power_up(struct generic_pm_domain *genpd)
+{
+ /* Notify CPU PM domain power up */
+ cpu_cluster_pm_exit();
+
+ return 0;
+}
+
+static void __init run_cpu(void *unused)
+{
+ struct device *cpu_dev = get_cpu_device(smp_processor_id());
+
+ /* We are running, increment the usage count */
+ pm_runtime_get_noresume(cpu_dev);
+}
+
+static int __init arm_domain_cpu_init(void)
+{
+ int cpuid, ret;
+
+ /* Find any CPU nodes with a phandle to this power domain */
+ for_each_possible_cpu(cpuid) {
+ struct device *cpu_dev;
+ struct of_phandle_args pd_args;
+
+ cpu_dev = get_cpu_device(cpuid);
+ if (!cpu_dev) {
+ pr_warn("%s: Unable to get device for CPU%d\n",
+ __func__, cpuid);
+ return -ENODEV;
+ }
+
+ /*
+ * We are only interested in CPUs that can be attached to
+ * PM domains that are arm,pd compatible.
+ */
+ ret = of_parse_phandle_with_args(cpu_dev->of_node,
+ "power-domains", "#power-domain-cells",
+ 0, &pd_args);
+ if (ret) {
+ dev_dbg(cpu_dev,
+ "%s: Did not find a valid PM domain\n",
+ __func__);
+ continue;
+ }
+
+ if (!of_device_is_compatible(pd_args.np, "arm,pd")) {
+ dev_dbg(cpu_dev, "%s: does not have an ARM PD\n",
+ __func__);
+ continue;
+ }
+
+ if (cpu_online(cpuid)) {
+ pm_runtime_set_active(cpu_dev);
+ /*
+ * Execute the below on that 'cpu' to ensure that the
+ * reference counting is correct. Its possible that
+ * while this code is executing, the 'cpu' may be
+ * powered down, but we may incorrectly increment the
+ * usage. By executing the get_cpu on the 'cpu',
+ * we can ensure that the 'cpu' and its usage count are
+ * matched.
+ */
+ smp_call_function_single(cpuid, run_cpu, NULL, true);
+ } else {
+ pm_runtime_set_suspended(cpu_dev);
+ }
+ pm_runtime_irq_safe(cpu_dev);
+ pm_runtime_enable(cpu_dev);
+
+ /*
+ * We attempt to attach the device to genpd again. We would
+ * have failed in our earlier attempt to attach to the domain
+ * provider as the CPU device would not have been IRQ safe,
+ * while the domain is defined as IRQ safe. IRQ safe domains
+ * can only have IRQ safe devices.
+ */
+ ret = genpd_dev_pm_attach(cpu_dev);
+ if (ret) {
+ dev_warn(cpu_dev,
+ "%s: Unable to attach to power-domain: %d\n",
+ __func__, ret);
+ pm_runtime_disable(cpu_dev);
+ }
+ }
+
+ return 0;
+}
+
+static int __init arm_domain_init(void)
+{
+ struct device_node *np;
+ int count = 0;
+
+ for_each_compatible_node(np, NULL, "arm,pd") {
+ struct arm_pm_domain *pd;
+
+ if (!of_device_is_available(np))
+ continue;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+
+ pd->genpd.name = kstrndup(np->name, NAME_MAX, GFP_KERNEL);
+ pd->genpd.power_off = arm_pd_power_down;
+ pd->genpd.power_on = arm_pd_power_up;
+ pd->genpd.flags |= GENPD_FLAG_IRQ_SAFE;
+
+ pr_debug("adding %s as generic power domain.\n", np->full_name);
+ pm_genpd_init(&pd->genpd, &simple_qos_governor, false);
+ of_genpd_add_provider_simple(np, &pd->genpd);
+
+ count++;
+ }
+
+ /* We have ARM PD(s), attach CPUs to their domain */
+ if (count)
+ return arm_domain_cpu_init();
+
+ return 0;
+}
+device_initcall(arm_domain_init);
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-04 23:35 ` [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters Lina Iyer
@ 2015-08-06 3:14 ` Rob Herring
2015-08-07 23:45 ` Kevin Hilman
2015-08-13 15:01 ` [PATCH 5/9] " Lorenzo Pieralisi
0 siblings, 2 replies; 60+ messages in thread
From: Rob Herring @ 2015-08-06 3:14 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
> SoCs group CPUs as clusters. Clusters share common resources like GIC,
> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
> idle, these shared resources may also be put in their idle state.
>
> The idle time between the last CPU entering idle and a CPU resuming
> execution is an opportunity for these shared resources to be powered
> down. Generic PM domain provides a framework for defining such power
> domains and attach devices to the domain. When the devices in the domain
> are idle at runtime, the domain would also be suspended and resumed
> before the first of the devices resume execution.
>
> We define a generic PM domain for each cluster and attach CPU devices in
> the cluster to that PM domain. The DT definitions for the SoC describe
> this relationship. Genpd callbacks for power_on and power_off can then
> be used to power up/down the shared resources for the domain.
[...]
> +ARM CPU Power domains
> +
> +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
> +caches, VFP and power controller and other peripheral hardware. Generally,
> +when the CPUs in the cluster are idle/suspended, the shared resources may also
> +be suspended and resumed before any of the CPUs resume execution.
> +
> +CPUs are the defined as the PM domain consumers and there is a PM domain
> +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
> +[1].
> +
> +The ARM CPU PM domain follows the same binding convention as any generic PM
> +domain. Additional binding properties are -
> +
> +- compatible:
> + Usage: required
> + Value type: <string>
> + Definition: Must also have
> + "arm,pd"
> + inorder to initialize the genpd provider as ARM CPU PM domain.
A compatible string should represent a particular h/w block. If it is
generic, it should represent some sort of standard programming
interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
is rather just a mapping of what "driver" you want to use.
I would expect that identifying a cpu's or cluster's power domain
would be done by a phandle between the cpu/cluster node and power
domain node. But I've not really looked at the power domain bindings
so who knows.
Rob
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-06 3:14 ` Rob Herring
@ 2015-08-07 23:45 ` Kevin Hilman
2015-08-11 13:07 ` Geert Uytterhoeven
2015-08-13 15:01 ` [PATCH 5/9] " Lorenzo Pieralisi
1 sibling, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-07 23:45 UTC (permalink / raw)
To: linux-arm-kernel
Rob Herring <robherring2@gmail.com> writes:
> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Define and add Generic PM domains (genpd) for ARM CPU clusters. Many
>> new
@Lina: I know you inherited this from some proof-of-concept code frome me, so
I'm partially to blame, but...
There's really nothing ARM specific about this driver.
>> SoCs group CPUs as clusters. Clusters share common resources like GIC,
>> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>> idle, these shared resources may also be put in their idle state.
>>
>> The idle time between the last CPU entering idle and a CPU resuming
>> execution is an opportunity for these shared resources to be powered
>> down. Generic PM domain provides a framework for defining such power
>> domains and attach devices to the domain. When the devices in the domain
>> are idle at runtime, the domain would also be suspended and resumed
>> before the first of the devices resume execution.
>>
>> We define a generic PM domain for each cluster and attach CPU devices in
>> the cluster to that PM domain. The DT definitions for the SoC describe
>> this relationship. Genpd callbacks for power_on and power_off can then
>> be used to power up/down the shared resources for the domain.
>
> [...]
>
>> +ARM CPU Power domains
>> +
>> +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>> +caches, VFP and power controller and other peripheral hardware. Generally,
>> +when the CPUs in the cluster are idle/suspended, the shared resources may also
>> +be suspended and resumed before any of the CPUs resume execution.
>> +
>> +CPUs are the defined as the PM domain consumers and there is a PM domain
>> +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>> +[1].
>> +
>> +The ARM CPU PM domain follows the same binding convention as any generic PM
>> +domain. Additional binding properties are -
>> +
>> +- compatible:
>> + Usage: required
>> + Value type: <string>
>> + Definition: Must also have
>> + "arm,pd"
>> + inorder to initialize the genpd provider as ARM CPU PM domain.
>
> A compatible string should represent a particular h/w block. If it is
> generic, it should represent some sort of standard programming
> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
> is rather just a mapping of what "driver" you want to use.
>
> I would expect that identifying a cpu's or cluster's power domain
> would be done by a phandle between the cpu/cluster node and power
> domain node.
That's correct, the CPU nodes (and other nodes in the cluster like GIC,
Coresight, etc.) would have phandles to the cluster power domain node.
But this series is meant to create the driver & binding for those cluster
power domain(s), so the question is how exactly describe it.
What we're trying to get to is binding to describe a powerdomain of a
generic CPU cluster, but of course the actual programming interface for
powering down the cluster will be platform specific. In earlier RFC
versions, Lina had proposed ways for platforms to register some
low-level hooks with this generic driver for the platform-specific bits,
but if you have some other suggests, we'd be all ears.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-07 23:45 ` Kevin Hilman
@ 2015-08-11 13:07 ` Geert Uytterhoeven
2015-08-11 15:58 ` Lina Iyer
0 siblings, 1 reply; 60+ messages in thread
From: Geert Uytterhoeven @ 2015-08-11 13:07 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Aug 8, 2015 at 1:45 AM, Kevin Hilman <khilman@kernel.org> wrote:
> Rob Herring <robherring2@gmail.com> writes:
>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> +ARM CPU Power domains
>>> +
>>> +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>>> +caches, VFP and power controller and other peripheral hardware. Generally,
>>> +when the CPUs in the cluster are idle/suspended, the shared resources may also
>>> +be suspended and resumed before any of the CPUs resume execution.
>>> +
>>> +CPUs are the defined as the PM domain consumers and there is a PM domain
>>> +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>>> +[1].
>>> +
>>> +The ARM CPU PM domain follows the same binding convention as any generic PM
>>> +domain. Additional binding properties are -
>>> +
>>> +- compatible:
>>> + Usage: required
>>> + Value type: <string>
>>> + Definition: Must also have
>>> + "arm,pd"
>>> + inorder to initialize the genpd provider as ARM CPU PM domain.
>>
>> A compatible string should represent a particular h/w block. If it is
>> generic, it should represent some sort of standard programming
>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>> is rather just a mapping of what "driver" you want to use.
>>
>> I would expect that identifying a cpu's or cluster's power domain
>> would be done by a phandle between the cpu/cluster node and power
>> domain node.
>
> That's correct, the CPU nodes (and other nodes in the cluster like GIC,
> Coresight, etc.) would have phandles to the cluster power domain node.
Indeed.
> But this series is meant to create the driver & binding for those cluster
> power domain(s), so the question is how exactly describe it.
I don't think I can add an "arm,pd" compatible property to e.g. a2sl
(for CA15-CPUx) and a3sm (for CA15-SCU) in arch/arm/boot/dts/r8a73a4.dtsi,
as these are just subdomains in a power domain hierarchy, all driven by a
single hardware block.
I can call e.g. a special registration method, or set up some ops pointer,
for the a2sl and a3sm subdomains from within the "renesas,sysc-rmobile"
driver.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-11 13:07 ` Geert Uytterhoeven
@ 2015-08-11 15:58 ` Lina Iyer
2015-08-11 20:12 ` Rob Herring
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-11 15:58 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 11 2015 at 07:07 -0600, Geert Uytterhoeven wrote:
>On Sat, Aug 8, 2015 at 1:45 AM, Kevin Hilman <khilman@kernel.org> wrote:
>> Rob Herring <robherring2@gmail.com> writes:
>>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>> +ARM CPU Power domains
>>>> +
>>>> +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>>>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>>>> +caches, VFP and power controller and other peripheral hardware. Generally,
>>>> +when the CPUs in the cluster are idle/suspended, the shared resources may also
>>>> +be suspended and resumed before any of the CPUs resume execution.
>>>> +
>>>> +CPUs are the defined as the PM domain consumers and there is a PM domain
>>>> +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>>>> +[1].
>>>> +
>>>> +The ARM CPU PM domain follows the same binding convention as any generic PM
>>>> +domain. Additional binding properties are -
>>>> +
>>>> +- compatible:
>>>> + Usage: required
>>>> + Value type: <string>
>>>> + Definition: Must also have
>>>> + "arm,pd"
>>>> + inorder to initialize the genpd provider as ARM CPU PM domain.
>>>
>>> A compatible string should represent a particular h/w block. If it is
>>> generic, it should represent some sort of standard programming
>>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>> is rather just a mapping of what "driver" you want to use.
>>>
>>> I would expect that identifying a cpu's or cluster's power domain
>>> would be done by a phandle between the cpu/cluster node and power
>>> domain node.
>>
>> That's correct, the CPU nodes (and other nodes in the cluster like GIC,
>> Coresight, etc.) would have phandles to the cluster power domain node.
>
>Indeed.
>
>> But this series is meant to create the driver & binding for those cluster
>> power domain(s), so the question is how exactly describe it.
>
>I don't think I can add an "arm,pd" compatible property to e.g. a2sl
>(for CA15-CPUx) and a3sm (for CA15-SCU) in arch/arm/boot/dts/r8a73a4.dtsi,
>as these are just subdomains in a power domain hierarchy, all driven by a
>single hardware block.
>
>I can call e.g. a special registration method, or set up some ops pointer,
>for the a2sl and a3sm subdomains from within the "renesas,sysc-rmobile"
>driver.
>
I was hoping the macro would help in such a case. But since your domain
cannot be defined as arm,pd (could you explain why, I seem to missing
the obvious) would it help if I export a function like that the
renesas,sysc-rmobile driver could call and setup the CPU PM domain?
There is catch to it though.
The problem is -
To be generic and not have every driver write code to do this generic
functionality, the common code would want to instantiate the
arm_pm_domain and therefore the embedded genpd. A pointer instead of
actual object, would mean maintaining a list and iterating through it,
everytime the domain is suspended/resumed. With an object, I could just
do container_of and get the arm_pd. But, we also want to give the
platform an option to define certain aspects of the CPU's generic PM
domain like the flags, genpd callbacks etc, before the genpd is
registered with the framework.
Would such a function work for you? What does everyone think about the
@template?
struct generic_pm_domain *of_arm_cpu_domain(struct device_node *dn,
struct of_arm_pd_ops *ops, struct generic_pm_domain *template)
{
struct arm_pm_domain *pd;
if (!of_device_is_available(dn))
return NULL;
/* This creates the memory for pd and setup basic stuff */
pd = setup_arm_pd(dn);
/* copy the platform's template genpd over to the pd's genpd */
memcpy(&pd.genpd, template, sizeof(*template));
/*
* Now set up the additional ops and flags and register with
* genpd framework
*/
register_arm_pd(dn, pd);
/*
* Returning the genpd back to the platform so it can be added
* as subdomains to other domains etc.
*/
return &pd.genpd;
}
EXPORT_SYMBOL(of_arm_cpu_domain);
The catch is that platform drivers have to provide a template for the
genpd. The @template would never be registered, but would just be used
to create the common code's genpd.
Any other ideas?
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-11 15:58 ` Lina Iyer
@ 2015-08-11 20:12 ` Rob Herring
2015-08-11 22:29 ` Lina Iyer
2015-08-12 19:00 ` [PATCH v2 1/2] " Lina Iyer
0 siblings, 2 replies; 60+ messages in thread
From: Rob Herring @ 2015-08-11 20:12 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 11, 2015 at 10:58 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Tue, Aug 11 2015 at 07:07 -0600, Geert Uytterhoeven wrote:
>>
>> On Sat, Aug 8, 2015 at 1:45 AM, Kevin Hilman <khilman@kernel.org> wrote:
>>>
>>> Rob Herring <robherring2@gmail.com> writes:
>>>>
>>>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>>
>>>>> +ARM CPU Power domains
>>>>> +
>>>>> +The device tree allows describing of CPU power domains in a SoC. In
>>>>> ARM SoC,
>>>>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC,
>>>>> Coresight,
>>>>> +caches, VFP and power controller and other peripheral hardware.
>>>>> Generally,
>>>>> +when the CPUs in the cluster are idle/suspended, the shared resources
>>>>> may also
>>>>> +be suspended and resumed before any of the CPUs resume execution.
>>>>> +
>>>>> +CPUs are the defined as the PM domain consumers and there is a PM
>>>>> domain
>>>>> +provider for the CPUs. Bindings for generic PM domains (genpd) is
>>>>> described in
>>>>> +[1].
>>>>> +
>>>>> +The ARM CPU PM domain follows the same binding convention as any
>>>>> generic PM
>>>>> +domain. Additional binding properties are -
>>>>> +
>>>>> +- compatible:
>>>>> + Usage: required
>>>>> + Value type: <string>
>>>>> + Definition: Must also have
>>>>> + "arm,pd"
>>>>> + inorder to initialize the genpd provider as ARM CPU PM
>>>>> domain.
>>>>
>>>>
>>>> A compatible string should represent a particular h/w block. If it is
>>>> generic, it should represent some sort of standard programming
>>>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>>> is rather just a mapping of what "driver" you want to use.
>>>>
>>>> I would expect that identifying a cpu's or cluster's power domain
>>>> would be done by a phandle between the cpu/cluster node and power
>>>> domain node.
>>>
>>>
>>> That's correct, the CPU nodes (and other nodes in the cluster like GIC,
>>> Coresight, etc.) would have phandles to the cluster power domain node.
>>
>>
>> Indeed.
>>
>>> But this series is meant to create the driver & binding for those cluster
>>> power domain(s), so the question is how exactly describe it.
>>
>>
>> I don't think I can add an "arm,pd" compatible property to e.g. a2sl
>> (for CA15-CPUx) and a3sm (for CA15-SCU) in arch/arm/boot/dts/r8a73a4.dtsi,
>> as these are just subdomains in a power domain hierarchy, all driven by a
>> single hardware block.
>>
>> I can call e.g. a special registration method, or set up some ops pointer,
>> for the a2sl and a3sm subdomains from within the "renesas,sysc-rmobile"
>> driver.
>>
> I was hoping the macro would help in such a case. But since your domain
> cannot be defined as arm,pd (could you explain why, I seem to missing
> the obvious) would it help if I export a function like that the
> renesas,sysc-rmobile driver could call and setup the CPU PM domain?
> There is catch to it though.
>
> The problem is -
>
> To be generic and not have every driver write code to do this generic
> functionality, the common code would want to instantiate the
> arm_pm_domain and therefore the embedded genpd. A pointer instead of
> actual object, would mean maintaining a list and iterating through it,
> everytime the domain is suspended/resumed. With an object, I could just
> do container_of and get the arm_pd. But, we also want to give the
> platform an option to define certain aspects of the CPU's generic PM
> domain like the flags, genpd callbacks etc, before the genpd is
> registered with the framework.
The problem here is what part of the hardware is generic? It is
generic, but yet ARM specific (presumably not as Kevin pointed out)?
I'm not exactly clear what the problem is, but it seems you are after
a common API/subsystem for managing power domains of CPU/clusters. I
fail to see how the problem is different than any other subsystem
where you have a core providing common API with hardware specific
drivers registering their ops with the core.
>
> Would such a function work for you? What does everyone think about the
> @template?
>
> struct generic_pm_domain *of_arm_cpu_domain(struct device_node *dn,
This is missing a verb. What does it do?
I don't think I really get what you are trying to solve to comment
whether this looks good or not.
Rob
> struct of_arm_pd_ops *ops, struct generic_pm_domain
> *template)
> {
> struct arm_pm_domain *pd;
>
> if (!of_device_is_available(dn))
> return NULL;
>
> /* This creates the memory for pd and setup basic stuff */
> pd = setup_arm_pd(dn);
>
> /* copy the platform's template genpd over to the pd's genpd */
> memcpy(&pd.genpd, template, sizeof(*template));
>
> /* * Now set up the additional ops and flags and register with
> * genpd framework
> */
> register_arm_pd(dn, pd);
>
> /* * Returning the genpd back to the platform so it can be
> added
> * as subdomains to other domains etc.
> */
> return &pd.genpd;
> }
> EXPORT_SYMBOL(of_arm_cpu_domain);
>
> The catch is that platform drivers have to provide a template for the
> genpd. The @template would never be registered, but would just be used
> to create the common code's genpd.
>
> Any other ideas?
>
> Thanks,
> Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-11 20:12 ` Rob Herring
@ 2015-08-11 22:29 ` Lina Iyer
2015-08-12 19:00 ` [PATCH v2 1/2] " Lina Iyer
1 sibling, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-11 22:29 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 11 2015 at 14:13 -0600, Rob Herring wrote:
>On Tue, Aug 11, 2015 at 10:58 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Tue, Aug 11 2015 at 07:07 -0600, Geert Uytterhoeven wrote:
>>> On Sat, Aug 8, 2015 at 1:45 AM, Kevin Hilman <khilman@kernel.org> wrote:
>>>> Rob Herring <robherring2@gmail.com> writes:
>>>>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>>>
>>>>>> +ARM CPU Power domains
>>>>>> +
>>>>>> +The device tree allows describing of CPU power domains in a SoC. In
>>>>>> ARM SoC,
>>>>>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC,
>>>>>> Coresight,
>>>>>> +caches, VFP and power controller and other peripheral hardware.
>>>>>> Generally,
>>>>>> +when the CPUs in the cluster are idle/suspended, the shared resources
>>>>>> may also
>>>>>> +be suspended and resumed before any of the CPUs resume execution.
>>>>>> +
>>>>>> +CPUs are the defined as the PM domain consumers and there is a PM
>>>>>> domain
>>>>>> +provider for the CPUs. Bindings for generic PM domains (genpd) is
>>>>>> described in
>>>>>> +[1].
>>>>>> +
>>>>>> +The ARM CPU PM domain follows the same binding convention as any
>>>>>> generic PM
>>>>>> +domain. Additional binding properties are -
>>>>>> +
>>>>>> +- compatible:
>>>>>> + Usage: required
>>>>>> + Value type: <string>
>>>>>> + Definition: Must also have
>>>>>> + "arm,pd"
>>>>>> + inorder to initialize the genpd provider as ARM CPU PM
>>>>>> domain.
>>>>>
>>>>>
>>>>> A compatible string should represent a particular h/w block. If it is
>>>>> generic, it should represent some sort of standard programming
>>>>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>>>> is rather just a mapping of what "driver" you want to use.
>>>>>
>>>>> I would expect that identifying a cpu's or cluster's power domain
>>>>> would be done by a phandle between the cpu/cluster node and power
>>>>> domain node.
>>>>
>>>>
>>>> That's correct, the CPU nodes (and other nodes in the cluster like GIC,
>>>> Coresight, etc.) would have phandles to the cluster power domain node.
>>>
>>>
>>> Indeed.
>>>
>>>> But this series is meant to create the driver & binding for those cluster
>>>> power domain(s), so the question is how exactly describe it.
>>>
>>>
>>> I don't think I can add an "arm,pd" compatible property to e.g. a2sl
>>> (for CA15-CPUx) and a3sm (for CA15-SCU) in arch/arm/boot/dts/r8a73a4.dtsi,
>>> as these are just subdomains in a power domain hierarchy, all driven by a
>>> single hardware block.
>>>
>>> I can call e.g. a special registration method, or set up some ops pointer,
>>> for the a2sl and a3sm subdomains from within the "renesas,sysc-rmobile"
>>> driver.
>>>
>> I was hoping the macro would help in such a case. But since your domain
>> cannot be defined as arm,pd (could you explain why, I seem to missing
>> the obvious) would it help if I export a function like that the
>> renesas,sysc-rmobile driver could call and setup the CPU PM domain?
>> There is catch to it though.
>>
>> The problem is -
>>
>> To be generic and not have every driver write code to do this generic
>> functionality, the common code would want to instantiate the
>> arm_pm_domain and therefore the embedded genpd. A pointer instead of
>> actual object, would mean maintaining a list and iterating through it,
>> everytime the domain is suspended/resumed. With an object, I could just
>> do container_of and get the arm_pd. But, we also want to give the
>> platform an option to define certain aspects of the CPU's generic PM
>> domain like the flags, genpd callbacks etc, before the genpd is
>> registered with the framework.
>
>The problem here is what part of the hardware is generic? It is
>generic, but yet ARM specific (presumably not as Kevin pointed out)?
>
CPU is the generic device that we are currently interested in. It is not
ARM specific and I am open to any better compatible description for such
domain providers.
>I'm not exactly clear what the problem is, but it seems you are after
>a common API/subsystem for managing power domains of CPU/clusters.
>
Partly yes. The common code is just managing common activities during
on/off of the power domain.
Platform driver needs to be involved to power on/off the power domain
hardware.
>I fail to see how the problem is different than any other subsystem
>where you have a core providing common API with hardware specific
>drivers registering their ops with the core.
>
Platform drivers today, directly use PM domains. They setup genpd
properties and callbacks (.power_on, .power_off) before registering with
the PM domain framework. For ex, renesas driver does this -
struct generic_pm_domain *genpd;
genpd->flags = GENPD_FLAG_PM_CLK;
pm_genpd_init(genpd, gov ? : &simple_qos_governor, false);
genpd->dev_ops.active_wakeup = rmobile_pd_active_wakeup;
genpd->power_off = rmobile_pd_power_down;
genpd->power_on = rmobile_pd_power_up;
genpd->attach_dev = rmobile_pd_attach_dev;
genpd->detach_dev = rmobile_pd_detach_dev;
The common code also uses genpd and has some additional properties. It
would also like to receive callbacks for .power_on and .power_off. On
the *same* genpd object -
genpd->flags |= GENPD_FLAG_IRQ_SAFE;
genpd->power_off = arm_pd_power_down;
genpd->power_on = arm_pd_power_up;
Most of the times the platform driver just would set the power_on,
power_off callbacks. In which case they could just register platform ops
with the core , during the OF_DECLARE_1() callback. The core would set
up the genpd->power_xxx to arm_pd_power_xxx and would relay the callback
to the platform using the callbacks registered the core.
But in the case like Renesas, where the genpd object created by the
core, needs to be modified by the platform driver before registering
with PM domain framework, the complexity arises. This where OF_DECLARE_1
lacks argument. The core code would like to pass the genpd object for the
platform code to amend and when the callback returns, use the genpd to
register with PM domain framework.
>>
>> Would such a function work for you? What does everyone think about the
>> @template?
>>
>> struct generic_pm_domain *of_arm_cpu_domain(struct device_node *dn,
>
>This is missing a verb. What does it do?
>
Sorry. /s/of_arm_cpu_domain/of_arm_init_cpu_domain/
>I don't think I really get what you are trying to solve to comment
>whether this looks good or not.
>
Let me know if this helps clarify.
Thanks,
Lina
>Rob
>
>> struct of_arm_pd_ops *ops, struct generic_pm_domain
>> *template)
>> {
>> struct arm_pm_domain *pd;
>>
>> if (!of_device_is_available(dn))
>> return NULL;
>>
>> /* This creates the memory for pd and setup basic stuff */
>> pd = setup_arm_pd(dn);
>>
>> /* copy the platform's template genpd over to the pd's genpd */
>> memcpy(&pd.genpd, template, sizeof(*template));
>>
>> /* * Now set up the additional ops and flags and register with
>> * genpd framework
>> */
>> register_arm_pd(dn, pd);
>>
>> /* * Returning the genpd back to the platform so it can be
>> added
>> * as subdomains to other domains etc.
>> */
>> return &pd.genpd;
>> }
>> EXPORT_SYMBOL(of_arm_cpu_domain);
>>
>> The catch is that platform drivers have to provide a template for the
>> genpd. The @template would never be registered, but would just be used
>> to create the common code's genpd.
>>
>> Any other ideas?
>>
>> Thanks,
>> Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-11 20:12 ` Rob Herring
2015-08-11 22:29 ` Lina Iyer
@ 2015-08-12 19:00 ` Lina Iyer
2015-08-12 19:00 ` [PATCH v2 2/2] ARM: domain: Add platform handlers for CPU PM domains Lina Iyer
2015-08-13 17:29 ` [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters Rob Herring
1 sibling, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-12 19:00 UTC (permalink / raw)
To: linux-arm-kernel
Define and add Generic PM domains (genpd) for CPU clusters. Many new
SoCs group CPUs as clusters. Clusters share common resources like GIC,
power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
idle, these shared resources may also be put in their idle state.
The idle time between the last CPU entering idle and a CPU resuming
execution is an opportunity for these shared resources to be powered
down. Generic PM domain provides a framework for defining such power
domains and attach devices to the domain. When the devices in the domain
are idle at runtime, the domain would also be suspended and resumed
before the first of the devices resume execution.
We define a generic PM domain for each cluster and attach CPU devices in
the cluster to that PM domain. The DT definitions for the SoC describe
this relationship. Genpd callbacks for power_on and power_off can then
be used to power up/down the shared resources for the domain.
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes since v1:
- Function name changes and split out common code
- Use cpu,pd for now. Removed references to ARM. Open to recommendations.
- Still located in arch/arm/common/. May move to a more appropriate location.
- Platform drivers can directly call of_init_cpu_domain() without using
compatibles.
- Now maintains a list of CPU PM domains.
---
Documentation/arm/cpu-domains.txt | 49 +++++
.../devicetree/bindings/arm/cpudomains.txt | 23 +++
arch/arm/common/Makefile | 1 +
arch/arm/common/domains.c | 225 +++++++++++++++++++++
arch/arm/include/asm/cpu-pd.h | 27 +++
5 files changed, 325 insertions(+)
create mode 100644 Documentation/arm/cpu-domains.txt
create mode 100644 Documentation/devicetree/bindings/arm/cpudomains.txt
create mode 100644 arch/arm/common/domains.c
create mode 100644 arch/arm/include/asm/cpu-pd.h
diff --git a/Documentation/arm/cpu-domains.txt b/Documentation/arm/cpu-domains.txt
new file mode 100644
index 0000000..49bd0d7
--- /dev/null
+++ b/Documentation/arm/cpu-domains.txt
@@ -0,0 +1,49 @@
+CPU Clusters and PM domain
+
+Newer CPUs are grouped in a SoC as clusters. A cluster in addition to the
+CPUs may have caches, GIC, VFP and architecture specific power controller to
+power the cluster. A cluster may also be nested in another cluster, the
+hierarchy of which is depicted in the device tree. CPUIdle frameworks enables
+the CPUs to determine the sleep time and enter low power state to save power
+during periods of idle. CPUs in a cluster may enter and exit idle state
+independently. During the time when all the CPUs are in idle state, the
+cluster can safely be in idle state as well. When the last of the CPUs is
+powered off as a result of idle, the cluster may also be powered down, but the
+domain must be powered on before the first of the CPUs in the cluster resumes
+execution.
+
+SoCs can power down the CPU and resume execution in a few uSecs and the domain
+that powers the CPU cluster also have comparable idle latencies. The CPU WFI
+signal in ARM CPUs is used as a hardware trigger for the cluster hardware to
+enter their idle state. The hardware can be programmed in advance to put the
+cluster in the desired idle state befitting the wakeup latency requested by
+the CPUs. When all the CPUs in a cluster have executed their WFI instruction,
+the state machine for the power controller may put the cluster components in
+their power down or idle state. Generally, the domains would power on with the
+hardware sensing the CPU's interrupts. The domains may however, need to be
+reconfigured by the CPU to remain active, until the last CPU is ready to enter
+idle again. To power down a cluster, it is generally required to power down
+all the CPUs. The caches would also need to be flushed. The hardware state of
+some of the components may need to be saved and restored when powered back on.
+SoC vendors may also have hardware specific configuration that must be done
+before the cluster can be powered off. When the cluster is powered off,
+notifications may be sent out to other SoC components to scale down or even
+power off their resources.
+
+Power management domains represent relationship of devices and their power
+controllers. They are represented in the DT as domain consumers and providers.
+A device may have a domain provider and a domain provider may support multiple
+domain consumers. Domains like clusters, may also be nested inside one
+another. A domain that has no active consumer, may be powered off and any
+resuming consumer would trigger the domain back to active. Parent domains may
+be powered off when the child domains are powered off. The CPU cluster can be
+fashioned as a PM domain. When the CPU devices are powered off, the PM domain
+may be powered off.
+
+The code in Generic PM domains handles the hierarchy of devices, domains and
+the reference counting of objects leading to last man down and first man up.
+The CPU domains core code defines PM domains for each CPU cluster and attaches
+the domains' CPU devices to as specified in the DT. This happens automatically
+at kernel init, when the domain is specified as compatible with "cpu,pd".
+Powering on/off the common cluster hardware would also be done when the PM
+domain is runtime suspended or resumed.
diff --git a/Documentation/devicetree/bindings/arm/cpudomains.txt b/Documentation/devicetree/bindings/arm/cpudomains.txt
new file mode 100644
index 0000000..54be393
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cpudomains.txt
@@ -0,0 +1,23 @@
+CPU Power domains
+
+The device tree allows describing of CPU power domains in a SoC. In many SoCs,
+CPUs are be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
+caches, VFP and power controller and other peripheral hardware. Generally,
+when the CPUs in the cluster are idle/suspended, the shared resources may also
+be suspended and resumed before any of the CPUs resume execution.
+
+CPUs are the defined as the PM domain consumers and there is a PM domain
+provider for the CPUs. Bindings for generic PM domains (genpd) is described in
+[1].
+
+The CPU PM domain follows the same binding convention as any generic PM
+domain. Additional binding properties are -
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: Should also have
+ "cpu,pd"
+ inorder to initialize the genpd provider as CPU PM domain.
+
+[1]. Documentation/devicetree/bindings/power/power_domain.txt
diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile
index 6764ed0..af0cd04 100644
--- a/arch/arm/common/Makefile
+++ b/arch/arm/common/Makefile
@@ -19,3 +19,4 @@ AFLAGS_vlock.o := -march=armv7-a
obj-$(CONFIG_TI_PRIV_EDMA) += edma.o
obj-$(CONFIG_BL_SWITCHER) += bL_switcher.o
obj-$(CONFIG_BL_SWITCHER_DUMMY_IF) += bL_switcher_dummy_if.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += domains.o
diff --git a/arch/arm/common/domains.c b/arch/arm/common/domains.c
new file mode 100644
index 0000000..4bc32a5
--- /dev/null
+++ b/arch/arm/common/domains.c
@@ -0,0 +1,225 @@
+/*
+ * CPU Generic PM Domain.
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include <asm/cpu-pd.h>
+
+#define NAME_MAX 36
+
+/* List of CPU PM domains we care about */
+static LIST_HEAD(of_cpu_pd_list);
+
+static int cpu_pd_power_down(struct generic_pm_domain *genpd)
+{
+ /*
+ * Notify CPU PM domain power down
+ * TODO: Call the notificated directly from here.
+ */
+ cpu_cluster_pm_enter();
+
+ return 0;
+}
+
+static int cpu_pd_power_up(struct generic_pm_domain *genpd)
+{
+ /* Notify CPU PM domain power up */
+ cpu_cluster_pm_exit();
+
+ return 0;
+}
+
+static void __init run_cpu(void *unused)
+{
+ struct device *cpu_dev = get_cpu_device(smp_processor_id());
+
+ /* We are running, increment the usage count */
+ pm_runtime_get_noresume(cpu_dev);
+}
+
+static int __init of_pm_domain_attach_cpus(void)
+{
+ int cpuid, ret;
+
+ /* Find any CPU nodes with a phandle to this power domain */
+ for_each_possible_cpu(cpuid) {
+ struct device *cpu_dev;
+ struct of_phandle_args pd_args;
+
+ cpu_dev = get_cpu_device(cpuid);
+ if (!cpu_dev) {
+ pr_warn("%s: Unable to get device for CPU%d\n",
+ __func__, cpuid);
+ return -ENODEV;
+ }
+
+ /*
+ * We are only interested in CPUs that can be attached to
+ * PM domains that are cpu,pd compatible.
+ */
+ ret = of_parse_phandle_with_args(cpu_dev->of_node,
+ "power-domains", "#power-domain-cells",
+ 0, &pd_args);
+ if (ret) {
+ dev_dbg(cpu_dev,
+ "%s: Did not find a valid PM domain\n",
+ __func__);
+ continue;
+ }
+
+ if (!of_device_is_compatible(pd_args.np, "cpu,pd")) {
+ dev_dbg(cpu_dev, "%s: does not have an CPU PD\n",
+ __func__);
+ continue;
+ }
+
+ if (cpu_online(cpuid)) {
+ pm_runtime_set_active(cpu_dev);
+ /*
+ * Execute the below on that 'cpu' to ensure that the
+ * reference counting is correct. Its possible that
+ * while this code is executing, the 'cpu' may be
+ * powered down, but we may incorrectly increment the
+ * usage. By executing the get_cpu on the 'cpu',
+ * we can ensure that the 'cpu' and its usage count are
+ * matched.
+ */
+ smp_call_function_single(cpuid, run_cpu, NULL, true);
+ } else {
+ pm_runtime_set_suspended(cpu_dev);
+ }
+ pm_runtime_irq_safe(cpu_dev);
+ pm_runtime_enable(cpu_dev);
+
+ /*
+ * We attempt to attach the device to genpd again. We would
+ * have failed in our earlier attempt to attach to the domain
+ * provider as the CPU device would not have been IRQ safe,
+ * while the domain is defined as IRQ safe. IRQ safe domains
+ * can only have IRQ safe devices.
+ */
+ ret = genpd_dev_pm_attach(cpu_dev);
+ if (ret) {
+ dev_warn(cpu_dev,
+ "%s: Unable to attach to power-domain: %d\n",
+ __func__, ret);
+ pm_runtime_disable(cpu_dev);
+ }
+ }
+
+ return 0;
+}
+
+static struct cpu_pm_domain __init *setup_cpu_pd(struct device_node *dn)
+{
+ struct cpu_pm_domain *pd;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return NULL;
+
+ pd->genpd = kzalloc(sizeof(*(pd->genpd)), GFP_KERNEL);
+ if (!pd->genpd) {
+ kfree(pd);
+ return NULL;
+ }
+
+ INIT_LIST_HEAD(&pd->link);
+ list_add(&pd->link, &of_cpu_pd_list);
+ pd->dn = dn;
+ pd->genpd->name = kstrndup(dn->full_name, NAME_MAX, GFP_KERNEL);
+
+ return pd;
+}
+
+static void __init of_register_cpu_pd(struct device_node *dn,
+ struct cpu_pm_domain *pd)
+{
+ pd->genpd->power_off = cpu_pd_power_down;
+ pd->genpd->power_on = cpu_pd_power_up;
+ pd->genpd->flags |= GENPD_FLAG_IRQ_SAFE;
+
+ /* Register the CPU genpd */
+ pr_debug("adding %s as generic power domain.\n", pd->genpd->name);
+ pm_genpd_init(pd->genpd, &simple_qos_governor, false);
+ of_genpd_add_provider_simple(dn, pd->genpd);
+}
+
+/**
+ * of_init_cpu_pm_domain() - Initialize a CPU PM domain using the CPU pd
+ * provided
+ * @dn: PM domain provider device node
+ * @pd: CPU PM domain to be initialized with genpd framework.
+ */
+int __init of_init_cpu_domain(struct device_node *dn, struct cpu_pm_domain *pd)
+{
+ if (!of_device_is_available(dn))
+ return -ENODEV;
+
+ if (!pd)
+ return -EINVAL;
+
+ of_register_cpu_pd(dn, pd);
+ of_pm_domain_attach_cpus();
+
+ return 0;
+}
+EXPORT_SYMBOL(of_init_cpu_domain);
+
+/**
+ * of_get_cpu_pm_domain() - Returns the CPU PD associated with the device node
+ * @dn: PM domain provider device node
+ */
+struct cpu_pm_domain __init *of_get_cpu_domain(struct device_node *dn)
+{
+ struct cpu_pm_domain *pd;
+
+ list_for_each_entry(pd, &of_cpu_pd_list, link) {
+ if (pd->dn == dn)
+ return pd;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(of_get_cpu_domain);
+
+static int __init of_cpu_pd_init(void)
+{
+ struct device_node *dn;
+ struct cpu_pm_domain *pd;
+
+ for_each_compatible_node(dn, NULL, "cpu,pd") {
+
+ if (!of_device_is_available(dn))
+ continue;
+
+ pd = setup_cpu_pd(dn);
+ if (!pd)
+ return -ENOMEM;
+
+ of_register_cpu_pd(dn, pd);
+ }
+
+ /* We have CPU PD(s), attach CPUs to their domain */
+ if (!list_empty(&of_cpu_pd_list))
+ return of_pm_domain_attach_cpus();
+
+ return 0;
+}
+device_initcall(of_cpu_pd_init);
diff --git a/arch/arm/include/asm/cpu-pd.h b/arch/arm/include/asm/cpu-pd.h
new file mode 100644
index 0000000..4785260
--- /dev/null
+++ b/arch/arm/include/asm/cpu-pd.h
@@ -0,0 +1,27 @@
+/*
+ * linux/arch/arm/include/asm/cpu-pd.h
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __CPU_PD_H__
+#define __CPU_PD_H__
+
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/pm_domain.h>
+
+struct cpu_pm_domain {
+ struct list_head link;
+ struct generic_pm_domain *genpd;
+ struct device_node *dn;
+};
+
+extern int of_init_cpu_domain(struct device_node *dn, struct cpu_pm_domain *pd);
+extern struct cpu_pm_domain *of_get_cpu_domain(struct device_node *dn);
+
+#endif /* __CPU_PD_H__ */
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v2 2/2] ARM: domain: Add platform handlers for CPU PM domains
2015-08-12 19:00 ` [PATCH v2 1/2] " Lina Iyer
@ 2015-08-12 19:00 ` Lina Iyer
2015-08-13 17:29 ` [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters Rob Herring
1 sibling, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-12 19:00 UTC (permalink / raw)
To: linux-arm-kernel
In addition to the common power up/down actions of CPU PM domain core,
platforms may have additional configuration before the CPU domain can b
powered off or considered active. Allow platform drivers to register
handlers for CPU PM domains.
Platform drivers may register their callbacks against a compatible
string defined by their PM domain provider device node in the DT. At
domain init, the platform driver can initialize the platform specific
genpd attributes. The init callback would need to return successfully,
for the platform power_on/off handlers to be registered with the CPU PM
domain.
The code uses __init section to reduce memory needed for platform
handlers and therefore can be freed after the driver is initialized, a
desirable outcome for single kernel image.
Cc: Rob Herring <robh@kernel.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Changes since v1:
- Removed references to ARM
- Use OF_DECLARE_1 for __init section tables
---
arch/arm/common/domains.c | 38 ++++++++++++++++++++++++++++++++++++++
arch/arm/include/asm/cpu-pd.h | 11 +++++++++++
include/asm-generic/vmlinux.lds.h | 2 ++
3 files changed, 51 insertions(+)
diff --git a/arch/arm/common/domains.c b/arch/arm/common/domains.c
index 4bc32a5..b1d9cc0 100644
--- a/arch/arm/common/domains.c
+++ b/arch/arm/common/domains.c
@@ -26,8 +26,28 @@
/* List of CPU PM domains we care about */
static LIST_HEAD(of_cpu_pd_list);
+static const struct of_device_id __cpu_pd_of_table_sentinel
+ __used __section(__cpu_pd_of_table_end);
+
+static inline
+struct cpu_pm_domain *to_cpu_pd(struct generic_pm_domain *d)
+{
+ struct cpu_pm_domain *pd;
+
+ list_for_each_entry(pd, &of_cpu_pd_list, link) {
+ if (pd->genpd == d)
+ return pd;
+ }
+
+ return NULL;
+}
+
static int cpu_pd_power_down(struct generic_pm_domain *genpd)
{
+ struct cpu_pm_domain *pd = to_cpu_pd(genpd);
+
+ if (pd->platform_ops.power_off)
+ pd->platform_ops.power_off(genpd);
/*
* Notify CPU PM domain power down
* TODO: Call the notificated directly from here.
@@ -39,6 +59,11 @@ static int cpu_pd_power_down(struct generic_pm_domain *genpd)
static int cpu_pd_power_up(struct generic_pm_domain *genpd)
{
+ struct cpu_pm_domain *pd = to_cpu_pd(genpd);
+
+ if (pd->platform_ops.power_on)
+ pd->platform_ops.power_on(genpd);
+
/* Notify CPU PM domain power up */
cpu_cluster_pm_exit();
@@ -203,6 +228,8 @@ static int __init of_cpu_pd_init(void)
{
struct device_node *dn;
struct cpu_pm_domain *pd;
+ const struct of_device_id *m = &__cpu_pd_of_table;
+ void (*pd_init)(struct device_node *) = NULL;
for_each_compatible_node(dn, NULL, "cpu,pd") {
@@ -213,6 +240,17 @@ static int __init of_cpu_pd_init(void)
if (!pd)
return -ENOMEM;
+ /* Find a compatible platform driver */
+ for (; m->compatible; m++) {
+ if (of_device_is_compatible(dn, m->compatible)) {
+ pd_init = m->data;
+ break;
+ }
+ }
+
+ if (pd_init)
+ pd_init(dn);
+
of_register_cpu_pd(dn, pd);
}
diff --git a/arch/arm/include/asm/cpu-pd.h b/arch/arm/include/asm/cpu-pd.h
index 4785260..65db517 100644
--- a/arch/arm/include/asm/cpu-pd.h
+++ b/arch/arm/include/asm/cpu-pd.h
@@ -15,13 +15,24 @@
#include <linux/of.h>
#include <linux/pm_domain.h>
+struct cpu_pd_ops {
+ int (*power_on)(struct generic_pm_domain *d);
+ int (*power_off)(struct generic_pm_domain *d);
+};
+
struct cpu_pm_domain {
struct list_head link;
struct generic_pm_domain *genpd;
+ struct cpu_pd_ops platform_ops;
struct device_node *dn;
};
extern int of_init_cpu_domain(struct device_node *dn, struct cpu_pm_domain *pd);
extern struct cpu_pm_domain *of_get_cpu_domain(struct device_node *dn);
+extern const struct of_device_id __cpu_pd_of_table;
+
+#define CPU_PD_METHOD_OF_DECLARE(name, compat, fn) \
+ OF_DECLARE_1(cpu_pd, name, compat, fn)
+
#endif /* __CPU_PD_H__ */
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 8bd374d..f952933 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,6 +179,7 @@
#define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
+#define CPU_PD_OF_TABLES() OF_TABLE(CONFIG_PM_GENERIC_DOMAINS, cpu_pd)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
#define KERNEL_DTB() \
@@ -514,6 +515,7 @@
IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
CPUIDLE_METHOD_OF_TABLES() \
+ CPU_PD_OF_TABLES() \
KERNEL_DTB() \
IRQCHIP_OF_MATCH_TABLE() \
EARLYCON_TABLE() \
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-12 19:00 ` [PATCH v2 1/2] " Lina Iyer
2015-08-12 19:00 ` [PATCH v2 2/2] ARM: domain: Add platform handlers for CPU PM domains Lina Iyer
@ 2015-08-13 17:29 ` Rob Herring
2015-08-13 20:12 ` Lina Iyer
1 sibling, 1 reply; 60+ messages in thread
From: Rob Herring @ 2015-08-13 17:29 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 12, 2015 at 2:00 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Define and add Generic PM domains (genpd) for CPU clusters. Many new
> SoCs group CPUs as clusters. Clusters share common resources like GIC,
> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
> idle, these shared resources may also be put in their idle state.
>
> The idle time between the last CPU entering idle and a CPU resuming
> execution is an opportunity for these shared resources to be powered
> down. Generic PM domain provides a framework for defining such power
> domains and attach devices to the domain. When the devices in the domain
> are idle at runtime, the domain would also be suspended and resumed
> before the first of the devices resume execution.
>
> We define a generic PM domain for each cluster and attach CPU devices in
> the cluster to that PM domain. The DT definitions for the SoC describe
> this relationship. Genpd callbacks for power_on and power_off can then
> be used to power up/down the shared resources for the domain.
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> Changes since v1:
>
> - Function name changes and split out common code
> - Use cpu,pd for now. Removed references to ARM. Open to recommendations.
> - Still located in arch/arm/common/. May move to a more appropriate location.
> - Platform drivers can directly call of_init_cpu_domain() without using
> compatibles.
> - Now maintains a list of CPU PM domains.
[...]
> +static int __init of_pm_domain_attach_cpus(void)
> +{
> + int cpuid, ret;
> +
> + /* Find any CPU nodes with a phandle to this power domain */
> + for_each_possible_cpu(cpuid) {
> + struct device *cpu_dev;
> + struct of_phandle_args pd_args;
> +
> + cpu_dev = get_cpu_device(cpuid);
> + if (!cpu_dev) {
> + pr_warn("%s: Unable to get device for CPU%d\n",
> + __func__, cpuid);
> + return -ENODEV;
> + }
> +
> + /*
> + * We are only interested in CPUs that can be attached to
> + * PM domains that are cpu,pd compatible.
> + */
Under what conditions would the power domain for a cpu not be cpu,pd compatible?
Why can't the driver handling the power domain register with gen_pd
and the cpu_pd as the driver is going to be aware of which domains are
for cpus. While there could be h/w such that all power domains within
a chip have a nice uniform programming model, I'd guess that is the
exception, not the rule. First, chips I have worked on were not that
way. CPU related and peripheral related domains are handled quite
differently. Second, often the actions on the CPU power domains don't
take effect until a WFI, so you end up with a different programming
sequence.
Rob
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 17:29 ` [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters Rob Herring
@ 2015-08-13 20:12 ` Lina Iyer
2015-08-13 22:01 ` Rob Herring
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-13 20:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 11:29 -0600, Rob Herring wrote:
>On Wed, Aug 12, 2015 at 2:00 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Define and add Generic PM domains (genpd) for CPU clusters. Many new
>> SoCs group CPUs as clusters. Clusters share common resources like GIC,
>> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>> idle, these shared resources may also be put in their idle state.
>>
>> The idle time between the last CPU entering idle and a CPU resuming
>> execution is an opportunity for these shared resources to be powered
>> down. Generic PM domain provides a framework for defining such power
>> domains and attach devices to the domain. When the devices in the domain
>> are idle at runtime, the domain would also be suspended and resumed
>> before the first of the devices resume execution.
>>
>> We define a generic PM domain for each cluster and attach CPU devices in
>> the cluster to that PM domain. The DT definitions for the SoC describe
>> this relationship. Genpd callbacks for power_on and power_off can then
>> be used to power up/down the shared resources for the domain.
>>
>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Kevin Hilman <khilman@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> Changes since v1:
>>
>> - Function name changes and split out common code
>> - Use cpu,pd for now. Removed references to ARM. Open to recommendations.
>> - Still located in arch/arm/common/. May move to a more appropriate location.
>> - Platform drivers can directly call of_init_cpu_domain() without using
>> compatibles.
>> - Now maintains a list of CPU PM domains.
>
>[...]
>
>> +static int __init of_pm_domain_attach_cpus(void)
>> +{
>> + int cpuid, ret;
>> +
>> + /* Find any CPU nodes with a phandle to this power domain */
>> + for_each_possible_cpu(cpuid) {
>> + struct device *cpu_dev;
>> + struct of_phandle_args pd_args;
>> +
>> + cpu_dev = get_cpu_device(cpuid);
>> + if (!cpu_dev) {
>> + pr_warn("%s: Unable to get device for CPU%d\n",
>> + __func__, cpuid);
>> + return -ENODEV;
>> + }
>> +
>> + /*
>> + * We are only interested in CPUs that can be attached to
>> + * PM domains that are cpu,pd compatible.
>> + */
>
>Under what conditions would the power domain for a cpu not be cpu,pd compatible?
>
Mostly never. But I dont want to assume and attach a CPU to its domain
that I am not concerned with.
>Why can't the driver handling the power domain register with gen_pd
>and the cpu_pd as the driver is going to be aware of which domains are
>for cpus.
They could and like Renesas they would. They could have an intricate
hierarchy of domains that they may want to deal with in their platform
drivers. Platforms could define the CPU devices as IRQ-safe and attach
it to their domains. Ensure the reference count of the online and
running CPUs are correct and they are good to go. They also would attach
the CPU devices to the domain and everything would work as they would
here. Its just repeated code across platforms that we are trying to
avoid.
>While there could be h/w such that all power domains within
>a chip have a nice uniform programming model, I'd guess that is the
>exception, not the rule. First, chips I have worked on were not that
>way. CPU related and peripheral related domains are handled quite
>differently.
>
Agreed. These are very SoC specific components and would require
platform specific programming. But most SoCs, would need to do many
other things like suspending debuggers, reducing clocks, GIC save and
restore, cluster state determination etc that are only getting more
generalized. We have generalized ARM CPU idle, I see this as the
platform for the next set of power saving in an SoC.
>Second, often the actions on the CPU power domains don't
>take effect until a WFI, so you end up with a different programming
>sequence.
>
How so? The last core going down (in this case, when the domain is
suspended the last core is the last device that does a _put() on the
domain) would determine and perform the power domain's programming
sequence, in the context of the last CPU. A sequence that would only be
effected in hardware when the the CPU executes WFI. I dont see why it
would be any different than how it is today.
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 20:12 ` Lina Iyer
@ 2015-08-13 22:01 ` Rob Herring
2015-08-14 14:38 ` Lina Iyer
0 siblings, 1 reply; 60+ messages in thread
From: Rob Herring @ 2015-08-13 22:01 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13, 2015 at 3:12 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Thu, Aug 13 2015 at 11:29 -0600, Rob Herring wrote:
>>
>> On Wed, Aug 12, 2015 at 2:00 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> Define and add Generic PM domains (genpd) for CPU clusters. Many new
>>> SoCs group CPUs as clusters. Clusters share common resources like GIC,
>>> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>>> idle, these shared resources may also be put in their idle state.
>>>
>>> The idle time between the last CPU entering idle and a CPU resuming
>>> execution is an opportunity for these shared resources to be powered
>>> down. Generic PM domain provides a framework for defining such power
>>> domains and attach devices to the domain. When the devices in the domain
>>> are idle at runtime, the domain would also be suspended and resumed
>>> before the first of the devices resume execution.
>>>
>>> We define a generic PM domain for each cluster and attach CPU devices in
>>> the cluster to that PM domain. The DT definitions for the SoC describe
>>> this relationship. Genpd callbacks for power_on and power_off can then
>>> be used to power up/down the shared resources for the domain.
>>>
>>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>>> Cc: Kevin Hilman <khilman@linaro.org>
>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Kevin Hilman <khilman@linaro.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>> Changes since v1:
>>>
>>> - Function name changes and split out common code
>>> - Use cpu,pd for now. Removed references to ARM. Open to recommendations.
>>> - Still located in arch/arm/common/. May move to a more appropriate
>>> location.
>>> - Platform drivers can directly call of_init_cpu_domain() without using
>>> compatibles.
>>> - Now maintains a list of CPU PM domains.
>>
>>
>> [...]
>>
>>> +static int __init of_pm_domain_attach_cpus(void)
>>> +{
>>> + int cpuid, ret;
>>> +
>>> + /* Find any CPU nodes with a phandle to this power domain */
>>> + for_each_possible_cpu(cpuid) {
>>> + struct device *cpu_dev;
>>> + struct of_phandle_args pd_args;
>>> +
>>> + cpu_dev = get_cpu_device(cpuid);
>>> + if (!cpu_dev) {
>>> + pr_warn("%s: Unable to get device for CPU%d\n",
>>> + __func__, cpuid);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + /*
>>> + * We are only interested in CPUs that can be attached to
>>> + * PM domains that are cpu,pd compatible.
>>> + */
>>
>>
>> Under what conditions would the power domain for a cpu not be cpu,pd
>> compatible?
>>
> Mostly never. But I dont want to assume and attach a CPU to its domain
> that I am not concerned with.
Which is why the power controller driver should tell you.
>> Why can't the driver handling the power domain register with gen_pd
>> and the cpu_pd as the driver is going to be aware of which domains are
>> for cpus.
>
> They could and like Renesas they would. They could have an intricate
> hierarchy of domains that they may want to deal with in their platform
> drivers. Platforms could define the CPU devices as IRQ-safe and attach
> it to their domains. Ensure the reference count of the online and
> running CPUs are correct and they are good to go. They also would attach
> the CPU devices to the domain and everything would work as they would
> here. Its just repeated code across platforms that we are trying to
> avoid.
I agree that we want to have core code doing all that setup, But that
has nothing to do with needing to have a DT property. The driver just
needs to tell you the list of cpu power domains and the associated
cpus they want the core to manage. Then it is up to you to do the rest
of the setup.
So I really don't think we need a DT binding here.
>> While there could be h/w such that all power domains within
>> a chip have a nice uniform programming model, I'd guess that is the
>> exception, not the rule. First, chips I have worked on were not that
>> way. CPU related and peripheral related domains are handled quite
>> differently.
>
> Agreed. These are very SoC specific components and would require
> platform specific programming. But most SoCs, would need to do many
> other things like suspending debuggers, reducing clocks, GIC save and
> restore, cluster state determination etc that are only getting more
> generalized. We have generalized ARM CPU idle, I see this as the
> platform for the next set of power saving in an SoC.
>
>> Second, often the actions on the CPU power domains don't
>> take effect until a WFI, so you end up with a different programming
>> sequence.
>>
> How so? The last core going down (in this case, when the domain is
> suspended the last core is the last device that does a _put() on the
> domain) would determine and perform the power domain's programming
> sequence, in the context of the last CPU. A sequence that would only be
> effected in hardware when the the CPU executes WFI. I dont see why it
> would be any different than how it is today.
I just mean that for a peripheral (e.g a SATA controller), you simply
quiesce the device and driver and shut off power. With a cpu or cpu
related component, you can't really shut it off until you stop
running. So cpu domains are special.
Rob
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH v2 1/2] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 22:01 ` Rob Herring
@ 2015-08-14 14:38 ` Lina Iyer
0 siblings, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-14 14:38 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 16:02 -0600, Rob Herring wrote:
>On Thu, Aug 13, 2015 at 3:12 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Thu, Aug 13 2015 at 11:29 -0600, Rob Herring wrote:
>>>
>>> On Wed, Aug 12, 2015 at 2:00 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>
>>>> Define and add Generic PM domains (genpd) for CPU clusters. Many new
>>>> SoCs group CPUs as clusters. Clusters share common resources like GIC,
>>>> power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>>>> idle, these shared resources may also be put in their idle state.
>>>>
>>>> The idle time between the last CPU entering idle and a CPU resuming
>>>> execution is an opportunity for these shared resources to be powered
>>>> down. Generic PM domain provides a framework for defining such power
>>>> domains and attach devices to the domain. When the devices in the domain
>>>> are idle at runtime, the domain would also be suspended and resumed
>>>> before the first of the devices resume execution.
>>>>
>>>> We define a generic PM domain for each cluster and attach CPU devices in
>>>> the cluster to that PM domain. The DT definitions for the SoC describe
>>>> this relationship. Genpd callbacks for power_on and power_off can then
>>>> be used to power up/down the shared resources for the domain.
>>>>
>>>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>>>> Cc: Kevin Hilman <khilman@linaro.org>
>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Signed-off-by: Kevin Hilman <khilman@linaro.org>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>> ---
>>>> Changes since v1:
>>>>
>>>> - Function name changes and split out common code
>>>> - Use cpu,pd for now. Removed references to ARM. Open to recommendations.
>>>> - Still located in arch/arm/common/. May move to a more appropriate
>>>> location.
>>>> - Platform drivers can directly call of_init_cpu_domain() without using
>>>> compatibles.
>>>> - Now maintains a list of CPU PM domains.
>>>
>>>
>>> [...]
>>>
>>>> +static int __init of_pm_domain_attach_cpus(void)
>>>> +{
>>>> + int cpuid, ret;
>>>> +
>>>> + /* Find any CPU nodes with a phandle to this power domain */
>>>> + for_each_possible_cpu(cpuid) {
>>>> + struct device *cpu_dev;
>>>> + struct of_phandle_args pd_args;
>>>> +
>>>> + cpu_dev = get_cpu_device(cpuid);
>>>> + if (!cpu_dev) {
>>>> + pr_warn("%s: Unable to get device for CPU%d\n",
>>>> + __func__, cpuid);
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + /*
>>>> + * We are only interested in CPUs that can be attached to
>>>> + * PM domains that are cpu,pd compatible.
>>>> + */
>>>
>>>
>>> Under what conditions would the power domain for a cpu not be cpu,pd
>>> compatible?
>>>
>> Mostly never. But I dont want to assume and attach a CPU to its domain
>> that I am not concerned with.
>
>Which is why the power controller driver should tell you.
>
>>> Why can't the driver handling the power domain register with gen_pd
>>> and the cpu_pd as the driver is going to be aware of which domains are
>>> for cpus.
>
I probably dont need that detail, the CPUs have phandle the domain, so
we know which CPUs are in the domain. I just need to know which domains
we should concern ourself with.
That would mean -
Platform drivers would explictly need to register the domains their CPU
domains. I was hoping to reduce that effort to register domains and
attach CPUs to their respective domains. The platform drivers need not
do anything unless they have SoC specific configuration to power down
the domain, in which case they would use the CPU_PD_METHOD_OF_DECLARE()
macro to register their platform callbacks. I thought it would be nice,
the platform specify the hardware relation in DT and drivers dont have
to explictly call out the domains.
>>
>> They could and like Renesas they would. They could have an intricate
>> hierarchy of domains that they may want to deal with in their platform
>> drivers. Platforms could define the CPU devices as IRQ-safe and attach
>> it to their domains. Ensure the reference count of the online and
>> running CPUs are correct and they are good to go. They also would attach
>> the CPU devices to the domain and everything would work as they would
>> here. Its just repeated code across platforms that we are trying to
>> avoid.
>
>I agree that we want to have core code doing all that setup, But that
>has nothing to do with needing to have a DT property. The driver just
>needs to tell you the list of cpu power domains and the associated
>cpus they want the core to manage. Then it is up to you to do the rest
>of the setup.
>
>So I really don't think we need a DT binding here.
>
Okay. I agree, that it is a far fetched idea. I will remove it for now.
May be when there are more common functionality, we can think about it.
>>> While there could be h/w such that all power domains within
>>> a chip have a nice uniform programming model, I'd guess that is the
>>> exception, not the rule. First, chips I have worked on were not that
>>> way. CPU related and peripheral related domains are handled quite
>>> differently.
>>
>>> Second, often the actions on the CPU power domains don't
>>> take effect until a WFI, so you end up with a different programming
>>> sequence.
>>>
>> How so? The last core going down (in this case, when the domain is
>> suspended the last core is the last device that does a _put() on the
>> domain) would determine and perform the power domain's programming
>> sequence, in the context of the last CPU. A sequence that would only be
>> effected in hardware when the the CPU executes WFI. I dont see why it
>> would be any different than how it is today.
>
>I just mean that for a peripheral (e.g a SATA controller), you simply
>quiesce the device and driver and shut off power. With a cpu or cpu
>related component, you can't really shut it off until you stop
>running. So cpu domains are special.
>
Makes sense.
Thanks Rob.
-- Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-06 3:14 ` Rob Herring
2015-08-07 23:45 ` Kevin Hilman
@ 2015-08-13 15:01 ` Lorenzo Pieralisi
2015-08-13 15:45 ` Lina Iyer
1 sibling, 1 reply; 60+ messages in thread
From: Lorenzo Pieralisi @ 2015-08-13 15:01 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
> > idle, these shared resources may also be put in their idle state.
> >
> > The idle time between the last CPU entering idle and a CPU resuming
> > execution is an opportunity for these shared resources to be powered
> > down. Generic PM domain provides a framework for defining such power
> > domains and attach devices to the domain. When the devices in the domain
> > are idle at runtime, the domain would also be suspended and resumed
> > before the first of the devices resume execution.
> >
> > We define a generic PM domain for each cluster and attach CPU devices in
> > the cluster to that PM domain. The DT definitions for the SoC describe
> > this relationship. Genpd callbacks for power_on and power_off can then
> > be used to power up/down the shared resources for the domain.
>
> [...]
>
> > +ARM CPU Power domains
> > +
> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
> > +caches, VFP and power controller and other peripheral hardware. Generally,
> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
> > +be suspended and resumed before any of the CPUs resume execution.
> > +
> > +CPUs are the defined as the PM domain consumers and there is a PM domain
> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
> > +[1].
> > +
> > +The ARM CPU PM domain follows the same binding convention as any generic PM
> > +domain. Additional binding properties are -
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <string>
> > + Definition: Must also have
> > + "arm,pd"
> > + inorder to initialize the genpd provider as ARM CPU PM domain.
>
> A compatible string should represent a particular h/w block. If it is
> generic, it should represent some sort of standard programming
> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
> is rather just a mapping of what "driver" you want to use.
>
> I would expect that identifying a cpu's or cluster's power domain
> would be done by a phandle between the cpu/cluster node and power
> domain node. But I've not really looked at the power domain bindings
> so who knows.
I would expect the same, meaning that a cpu node, like any other device
node would have a phandle pointing at the respective HW power domain.
I do not really understand why we want a "generic" CPU power domain, what
purpose does it serve ? Creating a collection of cpu devices that we
can call "cluster" ?
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 15:01 ` [PATCH 5/9] " Lorenzo Pieralisi
@ 2015-08-13 15:45 ` Lina Iyer
2015-08-13 15:52 ` Lorenzo Pieralisi
2015-08-13 17:26 ` Sudeep Holla
0 siblings, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-13 15:45 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
>> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
>> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>> > idle, these shared resources may also be put in their idle state.
>> >
>> > The idle time between the last CPU entering idle and a CPU resuming
>> > execution is an opportunity for these shared resources to be powered
>> > down. Generic PM domain provides a framework for defining such power
>> > domains and attach devices to the domain. When the devices in the domain
>> > are idle at runtime, the domain would also be suspended and resumed
>> > before the first of the devices resume execution.
>> >
>> > We define a generic PM domain for each cluster and attach CPU devices in
>> > the cluster to that PM domain. The DT definitions for the SoC describe
>> > this relationship. Genpd callbacks for power_on and power_off can then
>> > be used to power up/down the shared resources for the domain.
>>
>> [...]
>>
>> > +ARM CPU Power domains
>> > +
>> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>> > +caches, VFP and power controller and other peripheral hardware. Generally,
>> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
>> > +be suspended and resumed before any of the CPUs resume execution.
>> > +
>> > +CPUs are the defined as the PM domain consumers and there is a PM domain
>> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>> > +[1].
>> > +
>> > +The ARM CPU PM domain follows the same binding convention as any generic PM
>> > +domain. Additional binding properties are -
>> > +
>> > +- compatible:
>> > + Usage: required
>> > + Value type: <string>
>> > + Definition: Must also have
>> > + "arm,pd"
>> > + inorder to initialize the genpd provider as ARM CPU PM domain.
>>
>> A compatible string should represent a particular h/w block. If it is
>> generic, it should represent some sort of standard programming
>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>> is rather just a mapping of what "driver" you want to use.
>>
>> I would expect that identifying a cpu's or cluster's power domain
>> would be done by a phandle between the cpu/cluster node and power
>> domain node. But I've not really looked at the power domain bindings
>> so who knows.
>
>I would expect the same, meaning that a cpu node, like any other device
>node would have a phandle pointing at the respective HW power domain.
>
CPUs have phandles to their domains. That is how the relationship
between the domain provider (power-controller) and the consumer (CPU) is
established.
>I do not really understand why we want a "generic" CPU power domain, what
>purpose does it serve ? Creating a collection of cpu devices that we
>can call "cluster" ?
>
Nope, not for calling a cluster, a cluster :)
This compatible is used to define a generic behavior of the CPU domain
controller (in addition to the platform specific behavior of the domain
power controller). The kernel activities for such power controller are
generally the same which otherwise would be repeated across platforms.
An analogy to this would be the "arm,idle-state" that defines the DT
node as something that also depicts a generic cpuidle C state.
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 15:45 ` Lina Iyer
@ 2015-08-13 15:52 ` Lorenzo Pieralisi
2015-08-13 16:22 ` Lina Iyer
2015-08-14 3:51 ` Kevin Hilman
2015-08-13 17:26 ` Sudeep Holla
1 sibling, 2 replies; 60+ messages in thread
From: Lorenzo Pieralisi @ 2015-08-13 15:52 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13, 2015 at 04:45:03PM +0100, Lina Iyer wrote:
> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
> >On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> >> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
> >> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
> >> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
> >> > idle, these shared resources may also be put in their idle state.
> >> >
> >> > The idle time between the last CPU entering idle and a CPU resuming
> >> > execution is an opportunity for these shared resources to be powered
> >> > down. Generic PM domain provides a framework for defining such power
> >> > domains and attach devices to the domain. When the devices in the domain
> >> > are idle at runtime, the domain would also be suspended and resumed
> >> > before the first of the devices resume execution.
> >> >
> >> > We define a generic PM domain for each cluster and attach CPU devices in
> >> > the cluster to that PM domain. The DT definitions for the SoC describe
> >> > this relationship. Genpd callbacks for power_on and power_off can then
> >> > be used to power up/down the shared resources for the domain.
> >>
> >> [...]
> >>
> >> > +ARM CPU Power domains
> >> > +
> >> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
> >> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
> >> > +caches, VFP and power controller and other peripheral hardware. Generally,
> >> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
> >> > +be suspended and resumed before any of the CPUs resume execution.
> >> > +
> >> > +CPUs are the defined as the PM domain consumers and there is a PM domain
> >> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
> >> > +[1].
> >> > +
> >> > +The ARM CPU PM domain follows the same binding convention as any generic PM
> >> > +domain. Additional binding properties are -
> >> > +
> >> > +- compatible:
> >> > + Usage: required
> >> > + Value type: <string>
> >> > + Definition: Must also have
> >> > + "arm,pd"
> >> > + inorder to initialize the genpd provider as ARM CPU PM domain.
> >>
> >> A compatible string should represent a particular h/w block. If it is
> >> generic, it should represent some sort of standard programming
> >> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
> >> is rather just a mapping of what "driver" you want to use.
> >>
> >> I would expect that identifying a cpu's or cluster's power domain
> >> would be done by a phandle between the cpu/cluster node and power
> >> domain node. But I've not really looked at the power domain bindings
> >> so who knows.
> >
> >I would expect the same, meaning that a cpu node, like any other device
> >node would have a phandle pointing at the respective HW power domain.
> >
> CPUs have phandles to their domains. That is how the relationship
> between the domain provider (power-controller) and the consumer (CPU) is
> established.
>
> >I do not really understand why we want a "generic" CPU power domain, what
> >purpose does it serve ? Creating a collection of cpu devices that we
> >can call "cluster" ?
> >
> Nope, not for calling a cluster, a cluster :)
>
> This compatible is used to define a generic behavior of the CPU domain
> controller (in addition to the platform specific behavior of the domain
> power controller). The kernel activities for such power controller are
> generally the same which otherwise would be repeated across platforms.
What activities ? CPU PM notifiers ?
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 15:52 ` Lorenzo Pieralisi
@ 2015-08-13 16:22 ` Lina Iyer
2015-08-14 3:51 ` Kevin Hilman
1 sibling, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-13 16:22 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 09:52 -0600, Lorenzo Pieralisi wrote:
>On Thu, Aug 13, 2015 at 04:45:03PM +0100, Lina Iyer wrote:
>> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>> >On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> >> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
>> >> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
>> >> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>> >> > idle, these shared resources may also be put in their idle state.
>> >> >
>> >> > The idle time between the last CPU entering idle and a CPU resuming
>> >> > execution is an opportunity for these shared resources to be powered
>> >> > down. Generic PM domain provides a framework for defining such power
>> >> > domains and attach devices to the domain. When the devices in the domain
>> >> > are idle at runtime, the domain would also be suspended and resumed
>> >> > before the first of the devices resume execution.
>> >> >
>> >> > We define a generic PM domain for each cluster and attach CPU devices in
>> >> > the cluster to that PM domain. The DT definitions for the SoC describe
>> >> > this relationship. Genpd callbacks for power_on and power_off can then
>> >> > be used to power up/down the shared resources for the domain.
>> >>
>> >> [...]
>> >>
>> >> > +ARM CPU Power domains
>> >> > +
>> >> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>> >> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>> >> > +caches, VFP and power controller and other peripheral hardware. Generally,
>> >> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
>> >> > +be suspended and resumed before any of the CPUs resume execution.
>> >> > +
>> >> > +CPUs are the defined as the PM domain consumers and there is a PM domain
>> >> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>> >> > +[1].
>> >> > +
>> >> > +The ARM CPU PM domain follows the same binding convention as any generic PM
>> >> > +domain. Additional binding properties are -
>> >> > +
>> >> > +- compatible:
>> >> > + Usage: required
>> >> > + Value type: <string>
>> >> > + Definition: Must also have
>> >> > + "arm,pd"
>> >> > + inorder to initialize the genpd provider as ARM CPU PM domain.
>> >>
>> >> A compatible string should represent a particular h/w block. If it is
>> >> generic, it should represent some sort of standard programming
>> >> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>> >> is rather just a mapping of what "driver" you want to use.
>> >>
>> >> I would expect that identifying a cpu's or cluster's power domain
>> >> would be done by a phandle between the cpu/cluster node and power
>> >> domain node. But I've not really looked at the power domain bindings
>> >> so who knows.
>> >
>> >I would expect the same, meaning that a cpu node, like any other device
>> >node would have a phandle pointing at the respective HW power domain.
>> >
>> CPUs have phandles to their domains. That is how the relationship
>> between the domain provider (power-controller) and the consumer (CPU) is
>> established.
>>
>> >I do not really understand why we want a "generic" CPU power domain, what
>> >purpose does it serve ? Creating a collection of cpu devices that we
>> >can call "cluster" ?
>> >
>> Nope, not for calling a cluster, a cluster :)
>>
>> This compatible is used to define a generic behavior of the CPU domain
>> controller (in addition to the platform specific behavior of the domain
>> power controller). The kernel activities for such power controller are
>> generally the same which otherwise would be repeated across platforms.
>
>What activities ? CPU PM notifiers ?
>
Yes, for now. May be someday we can get rid of these notifiers and
directly invoke subsystems from these callbacks directly. Kevin proposed
this idea. With little exploration that I have done, I dont have a good
way to do that yet.
I am imagining here (only imagining at this time) that I could tie this
with last man down for cluster idle state determination and call into
cpuidle-PSCI to help compose the composite state id.
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 15:52 ` Lorenzo Pieralisi
2015-08-13 16:22 ` Lina Iyer
@ 2015-08-14 3:51 ` Kevin Hilman
2015-08-14 4:02 ` Lina Iyer
2015-08-14 15:49 ` Lorenzo Pieralisi
1 sibling, 2 replies; 60+ messages in thread
From: Kevin Hilman @ 2015-08-14 3:51 UTC (permalink / raw)
To: linux-arm-kernel
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> On Thu, Aug 13, 2015 at 04:45:03PM +0100, Lina Iyer wrote:
>> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>> >On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> >> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
>> >> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
>> >> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>> >> > idle, these shared resources may also be put in their idle state.
>> >> >
>> >> > The idle time between the last CPU entering idle and a CPU resuming
>> >> > execution is an opportunity for these shared resources to be powered
>> >> > down. Generic PM domain provides a framework for defining such power
>> >> > domains and attach devices to the domain. When the devices in the domain
>> >> > are idle at runtime, the domain would also be suspended and resumed
>> >> > before the first of the devices resume execution.
>> >> >
>> >> > We define a generic PM domain for each cluster and attach CPU devices in
>> >> > the cluster to that PM domain. The DT definitions for the SoC describe
>> >> > this relationship. Genpd callbacks for power_on and power_off can then
>> >> > be used to power up/down the shared resources for the domain.
>> >>
>> >> [...]
>> >>
>> >> > +ARM CPU Power domains
>> >> > +
>> >> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>> >> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>> >> > +caches, VFP and power controller and other peripheral hardware. Generally,
>> >> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
>> >> > +be suspended and resumed before any of the CPUs resume execution.
>> >> > +
>> >> > +CPUs are the defined as the PM domain consumers and there is a PM domain
>> >> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>> >> > +[1].
>> >> > +
>> >> > +The ARM CPU PM domain follows the same binding convention as any generic PM
>> >> > +domain. Additional binding properties are -
>> >> > +
>> >> > +- compatible:
>> >> > + Usage: required
>> >> > + Value type: <string>
>> >> > + Definition: Must also have
>> >> > + "arm,pd"
>> >> > + inorder to initialize the genpd provider as ARM CPU PM domain.
>> >>
>> >> A compatible string should represent a particular h/w block. If it is
>> >> generic, it should represent some sort of standard programming
>> >> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>> >> is rather just a mapping of what "driver" you want to use.
>> >>
>> >> I would expect that identifying a cpu's or cluster's power domain
>> >> would be done by a phandle between the cpu/cluster node and power
>> >> domain node. But I've not really looked at the power domain bindings
>> >> so who knows.
>> >
>> >I would expect the same, meaning that a cpu node, like any other device
>> >node would have a phandle pointing at the respective HW power domain.
>> >
>> CPUs have phandles to their domains. That is how the relationship
>> between the domain provider (power-controller) and the consumer (CPU) is
>> established.
>>
>> >I do not really understand why we want a "generic" CPU power domain, what
>> >purpose does it serve ? Creating a collection of cpu devices that we
>> >can call "cluster" ?
>> >
>> Nope, not for calling a cluster, a cluster :)
>>
>> This compatible is used to define a generic behavior of the CPU domain
>> controller (in addition to the platform specific behavior of the domain
>> power controller). The kernel activities for such power controller are
>> generally the same which otherwise would be repeated across platforms.
>
> What activities ? CPU PM notifiers ?
For today, yes.
However, you can think of CPU PM notifiers as the equivalent of runtime
PM hooks. They're called when the "devices" are about to be powered off
(runtime suspended) or powered on (runtime resumed.)
However the CPU PM framework and notifiers are rather dumb compared to
runtime PM. For example, runtime PM gives you usecounting, autosuspend,
control from userspace, statistics, etc. etc. Also, IMO, CPU PM will
not scale well for multiple clusters.
What if instead, we used runtime PM for the things that the CPU PM
notifiers manager (GIC, VFP, Coresight, etc.), and those drivers used
runtime PM callbacks to replace their CPU PM notifiers? We'd then be in
a beautiful land where CPU "devices" (and the other connected logic) can
be modeled as devices using runtime PM just like every other device in
the system.
Then take it up a level... what if we then could use genpd to model the
"cluster", made of of the CPUs and "connected" devices (GIC, VFP, etc.)
but also modeled the shared L2$ as a device which was using runtime PM.
Now we're in a place where we can use all the benefits of runtime PM,
plus the governor features of genpd to start doing a real, multi-CPU,
multi-cluster CPUidle that's flexible enough to model the various
dependencies in an SoC independent way, but generic enough to be able to
use common governors for last-man standing, cache flushing, etc. etc.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-14 3:51 ` Kevin Hilman
@ 2015-08-14 4:02 ` Lina Iyer
2015-08-14 15:49 ` Lorenzo Pieralisi
1 sibling, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-14 4:02 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 21:51 -0600, Kevin Hilman wrote:
>Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>
>> On Thu, Aug 13, 2015 at 04:45:03PM +0100, Lina Iyer wrote:
>>> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>>> >On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>>> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> >> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
>>> >> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
>>> >> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
>>> >> > idle, these shared resources may also be put in their idle state.
>>> >> >
>>> >> > The idle time between the last CPU entering idle and a CPU resuming
>>> >> > execution is an opportunity for these shared resources to be powered
>>> >> > down. Generic PM domain provides a framework for defining such power
>>> >> > domains and attach devices to the domain. When the devices in the domain
>>> >> > are idle at runtime, the domain would also be suspended and resumed
>>> >> > before the first of the devices resume execution.
>>> >> >
>>> >> > We define a generic PM domain for each cluster and attach CPU devices in
>>> >> > the cluster to that PM domain. The DT definitions for the SoC describe
>>> >> > this relationship. Genpd callbacks for power_on and power_off can then
>>> >> > be used to power up/down the shared resources for the domain.
>>> >>
>>> >> [...]
>>> >>
>>> >> > +ARM CPU Power domains
>>> >> > +
>>> >> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>>> >> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>>> >> > +caches, VFP and power controller and other peripheral hardware. Generally,
>>> >> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
>>> >> > +be suspended and resumed before any of the CPUs resume execution.
>>> >> > +
>>> >> > +CPUs are the defined as the PM domain consumers and there is a PM domain
>>> >> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>>> >> > +[1].
>>> >> > +
>>> >> > +The ARM CPU PM domain follows the same binding convention as any generic PM
>>> >> > +domain. Additional binding properties are -
>>> >> > +
>>> >> > +- compatible:
>>> >> > + Usage: required
>>> >> > + Value type: <string>
>>> >> > + Definition: Must also have
>>> >> > + "arm,pd"
>>> >> > + inorder to initialize the genpd provider as ARM CPU PM domain.
>>> >>
>>> >> A compatible string should represent a particular h/w block. If it is
>>> >> generic, it should represent some sort of standard programming
>>> >> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>> >> is rather just a mapping of what "driver" you want to use.
>>> >>
>>> >> I would expect that identifying a cpu's or cluster's power domain
>>> >> would be done by a phandle between the cpu/cluster node and power
>>> >> domain node. But I've not really looked at the power domain bindings
>>> >> so who knows.
>>> >
>>> >I would expect the same, meaning that a cpu node, like any other device
>>> >node would have a phandle pointing at the respective HW power domain.
>>> >
>>> CPUs have phandles to their domains. That is how the relationship
>>> between the domain provider (power-controller) and the consumer (CPU) is
>>> established.
>>>
>>> >I do not really understand why we want a "generic" CPU power domain, what
>>> >purpose does it serve ? Creating a collection of cpu devices that we
>>> >can call "cluster" ?
>>> >
>>> Nope, not for calling a cluster, a cluster :)
>>>
>>> This compatible is used to define a generic behavior of the CPU domain
>>> controller (in addition to the platform specific behavior of the domain
>>> power controller). The kernel activities for such power controller are
>>> generally the same which otherwise would be repeated across platforms.
>>
>> What activities ? CPU PM notifiers ?
>
>For today, yes.
>
>However, you can think of CPU PM notifiers as the equivalent of runtime
>PM hooks. They're called when the "devices" are about to be powered off
>(runtime suspended) or powered on (runtime resumed.)
>
>However the CPU PM framework and notifiers are rather dumb compared to
>runtime PM. For example, runtime PM gives you usecounting, autosuspend,
>control from userspace, statistics, etc. etc. Also, IMO, CPU PM will
>not scale well for multiple clusters.
>
>What if instead, we used runtime PM for the things that the CPU PM
>notifiers manager (GIC, VFP, Coresight, etc.), and those drivers used
>runtime PM callbacks to replace their CPU PM notifiers? We'd then be in
>a beautiful land where CPU "devices" (and the other connected logic) can
>be modeled as devices using runtime PM just like every other device in
>the system.
>
>Then take it up a level... what if we then could use genpd to model the
>"cluster", made of of the CPUs and "connected" devices (GIC, VFP, etc.)
>but also modeled the shared L2$ as a device which was using runtime PM.
>
>Now we're in a place where we can use all the benefits of runtime PM,
>plus the governor features of genpd to start doing a real, multi-CPU,
>multi-cluster CPUidle that's flexible enough to model the various
>dependencies in an SoC independent way, but generic enough to be able to
>use common governors for last-man standing, cache flushing, etc. etc.
>
- Off list
Nicely written response Kevin.
Thank you.
--Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-14 3:51 ` Kevin Hilman
2015-08-14 4:02 ` Lina Iyer
@ 2015-08-14 15:49 ` Lorenzo Pieralisi
2015-08-14 19:11 ` Kevin Hilman
1 sibling, 1 reply; 60+ messages in thread
From: Lorenzo Pieralisi @ 2015-08-14 15:49 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 14, 2015 at 04:51:15AM +0100, Kevin Hilman wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>
> > On Thu, Aug 13, 2015 at 04:45:03PM +0100, Lina Iyer wrote:
> >> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
> >> >On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
> >> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> >> >> > Define and add Generic PM domains (genpd) for ARM CPU clusters. Many new
> >> >> > SoCs group CPUs as clusters. Clusters share common resources like GIC,
> >> >> > power rail, caches, VFP, Coresight etc. When all CPUs in the cluster are
> >> >> > idle, these shared resources may also be put in their idle state.
> >> >> >
> >> >> > The idle time between the last CPU entering idle and a CPU resuming
> >> >> > execution is an opportunity for these shared resources to be powered
> >> >> > down. Generic PM domain provides a framework for defining such power
> >> >> > domains and attach devices to the domain. When the devices in the domain
> >> >> > are idle at runtime, the domain would also be suspended and resumed
> >> >> > before the first of the devices resume execution.
> >> >> >
> >> >> > We define a generic PM domain for each cluster and attach CPU devices in
> >> >> > the cluster to that PM domain. The DT definitions for the SoC describe
> >> >> > this relationship. Genpd callbacks for power_on and power_off can then
> >> >> > be used to power up/down the shared resources for the domain.
> >> >>
> >> >> [...]
> >> >>
> >> >> > +ARM CPU Power domains
> >> >> > +
> >> >> > +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
> >> >> > +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
> >> >> > +caches, VFP and power controller and other peripheral hardware. Generally,
> >> >> > +when the CPUs in the cluster are idle/suspended, the shared resources may also
> >> >> > +be suspended and resumed before any of the CPUs resume execution.
> >> >> > +
> >> >> > +CPUs are the defined as the PM domain consumers and there is a PM domain
> >> >> > +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
> >> >> > +[1].
> >> >> > +
> >> >> > +The ARM CPU PM domain follows the same binding convention as any generic PM
> >> >> > +domain. Additional binding properties are -
> >> >> > +
> >> >> > +- compatible:
> >> >> > + Usage: required
> >> >> > + Value type: <string>
> >> >> > + Definition: Must also have
> >> >> > + "arm,pd"
> >> >> > + inorder to initialize the genpd provider as ARM CPU PM domain.
> >> >>
> >> >> A compatible string should represent a particular h/w block. If it is
> >> >> generic, it should represent some sort of standard programming
> >> >> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
> >> >> is rather just a mapping of what "driver" you want to use.
> >> >>
> >> >> I would expect that identifying a cpu's or cluster's power domain
> >> >> would be done by a phandle between the cpu/cluster node and power
> >> >> domain node. But I've not really looked at the power domain bindings
> >> >> so who knows.
> >> >
> >> >I would expect the same, meaning that a cpu node, like any other device
> >> >node would have a phandle pointing at the respective HW power domain.
> >> >
> >> CPUs have phandles to their domains. That is how the relationship
> >> between the domain provider (power-controller) and the consumer (CPU) is
> >> established.
> >>
> >> >I do not really understand why we want a "generic" CPU power domain, what
> >> >purpose does it serve ? Creating a collection of cpu devices that we
> >> >can call "cluster" ?
> >> >
> >> Nope, not for calling a cluster, a cluster :)
> >>
> >> This compatible is used to define a generic behavior of the CPU domain
> >> controller (in addition to the platform specific behavior of the domain
> >> power controller). The kernel activities for such power controller are
> >> generally the same which otherwise would be repeated across platforms.
> >
> > What activities ? CPU PM notifiers ?
>
> For today, yes.
>
> However, you can think of CPU PM notifiers as the equivalent of runtime
> PM hooks. They're called when the "devices" are about to be powered off
> (runtime suspended) or powered on (runtime resumed.)
>
> However the CPU PM framework and notifiers are rather dumb compared to
> runtime PM. For example, runtime PM gives you usecounting, autosuspend,
> control from userspace, statistics, etc. etc. Also, IMO, CPU PM will
> not scale well for multiple clusters.
>
> What if instead, we used runtime PM for the things that the CPU PM
> notifiers manager (GIC, VFP, Coresight, etc.), and those drivers used
> runtime PM callbacks to replace their CPU PM notifiers? We'd then be in
> a beautiful land where CPU "devices" (and the other connected logic) can
> be modeled as devices using runtime PM just like every other device in
> the system.
I would agree with that (even though I do not see how we can make
eg GIC, VFP and arch timers behave like devices from a runtime PM
standpoint), still I do not see why we need a virtual power domain for
that, the CPU "devices" should be attached to the HW CPU power domain.
More below for systems relying on FW interfaces to handle CPU power
management.
> Then take it up a level... what if we then could use genpd to model the
> "cluster", made of of the CPUs and "connected" devices (GIC, VFP, etc.)
> but also modeled the shared L2$ as a device which was using runtime PM.
I have to understand what "modeled" means (do we create a struct device
on purpose for that ? Same goes for GIC and VFP).
But overall I get the gist of what you are saying, we just have to see
how this can be implemented within the genPD framework.
I suspect the "virtual" power domain you are introducing is there for
systems where the power controller is hidden from the kernel (ie PSCI),
where basically the CPU "devices" can't be attached to a power domain
simply because that power domain is not managed in the kernel but
by firmware.
> Now we're in a place where we can use all the benefits of runtime PM,
> plus the governor features of genpd to start doing a real, multi-CPU,
> multi-cluster CPUidle that's flexible enough to model the various
> dependencies in an SoC independent way, but generic enough to be able to
> use common governors for last-man standing, cache flushing, etc. etc.
I do not disagree (even though I think that last man standing is pushing
this concept a bit over the top), I am just concerned about the points
raised above, most of them should be reasonably simple to solve.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-14 15:49 ` Lorenzo Pieralisi
@ 2015-08-14 19:11 ` Kevin Hilman
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Hilman @ 2015-08-14 19:11 UTC (permalink / raw)
To: linux-arm-kernel
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> On Fri, Aug 14, 2015 at 04:51:15AM +0100, Kevin Hilman wrote:
[...]
>> However, you can think of CPU PM notifiers as the equivalent of runtime
>> PM hooks. They're called when the "devices" are about to be powered off
>> (runtime suspended) or powered on (runtime resumed.)
>>
>> However the CPU PM framework and notifiers are rather dumb compared to
>> runtime PM. For example, runtime PM gives you usecounting, autosuspend,
>> control from userspace, statistics, etc. etc. Also, IMO, CPU PM will
>> not scale well for multiple clusters.
>>
>> What if instead, we used runtime PM for the things that the CPU PM
>> notifiers manager (GIC, VFP, Coresight, etc.), and those drivers used
>> runtime PM callbacks to replace their CPU PM notifiers? We'd then be in
>> a beautiful land where CPU "devices" (and the other connected logic) can
>> be modeled as devices using runtime PM just like every other device in
>> the system.
>
> I would agree with that (even though I do not see how we can make
> eg GIC, VFP and arch timers behave like devices from a runtime PM
> standpoint),
Sure, that might be a stretch due the implementation details, but
conceptully it models the hardware well and I'd like to explore runtime
PM for all of these "devices", though it's not the highest priority.
> still I do not see why we need a virtual power domain for
> that, the CPU "devices" should be attached to the HW CPU power domain.
>
> More below for systems relying on FW interfaces to handle CPU power
> management.
>
>> Then take it up a level... what if we then could use genpd to model the
>> "cluster", made of of the CPUs and "connected" devices (GIC, VFP, etc.)
>> but also modeled the shared L2$ as a device which was using runtime PM.
>
> I have to understand what "modeled" means (do we create a struct device
> on purpose for that ? Same goes for GIC and VFP).
Not necessarily a struct device for the cluster, but for the CPUs (which
already have one) and and possibly GIC, VFP, timers. etc. With that in
place, cluster would just be modleled by a genpd (which is what Lina's
series is doing.)
> But overall I get the gist of what you are saying, we just have to see
> how this can be implemented within the genPD framework.
>
> I suspect the "virtual" power domain you are introducing is there for
> systems where the power controller is hidden from the kernel (ie PSCI),
> where basically the CPU "devices" can't be attached to a power domain
> simply because that power domain is not managed in the kernel but
> by firmware.
The main idea behind a "virtual" power domain was to collect the common
parts of cluster management, possibly governors etc. However, maybe
it's better just have a set of functions that the "real" hw power domain
drivers could use for the common parts. That might get rid of the need
to describe this in DT, which I think is what Rob is suggesting also.
>> Now we're in a place where we can use all the benefits of runtime PM,
>> plus the governor features of genpd to start doing a real, multi-CPU,
>> multi-cluster CPUidle that's flexible enough to model the various
>> dependencies in an SoC independent way, but generic enough to be able to
>> use common governors for last-man standing, cache flushing, etc. etc.
>
> I do not disagree (even though I think that last man standing is pushing
> this concept a bit over the top), I am just concerned about the points
> raised above, most of them should be reasonably simple to solve.
Good, hopefully we can have a good discussion about this at Plumbers
next week as the issues above and proposed in Lina's series are the main
issues I want to raise in my part of the EAS/PM track[1].
See you there!
Kevin
[1] https://linuxplumbersconf.org/2015/ocw/events/LPC2015/tracks/501
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 15:45 ` Lina Iyer
2015-08-13 15:52 ` Lorenzo Pieralisi
@ 2015-08-13 17:26 ` Sudeep Holla
2015-08-13 19:27 ` Lina Iyer
1 sibling, 1 reply; 60+ messages in thread
From: Sudeep Holla @ 2015-08-13 17:26 UTC (permalink / raw)
To: linux-arm-kernel
On 13/08/15 16:45, Lina Iyer wrote:
> On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>> On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
[..]
>>>
>>>> +ARM CPU Power domains
>>>> +
>>>> +The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>>>> +CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>>>> +caches, VFP and power controller and other peripheral hardware. Generally,
>>>> +when the CPUs in the cluster are idle/suspended, the shared resources may also
>>>> +be suspended and resumed before any of the CPUs resume execution.
>>>> +
>>>> +CPUs are the defined as the PM domain consumers and there is a PM domain
>>>> +provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>>>> +[1].
>>>> +
>>>> +The ARM CPU PM domain follows the same binding convention as any generic PM
>>>> +domain. Additional binding properties are -
>>>> +
>>>> +- compatible:
>>>> + Usage: required
>>>> + Value type: <string>
>>>> + Definition: Must also have
>>>> + "arm,pd"
>>>> + inorder to initialize the genpd provider as ARM CPU PM domain.
>>>
>>> A compatible string should represent a particular h/w block. If it is
>>> generic, it should represent some sort of standard programming
>>> interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>> is rather just a mapping of what "driver" you want to use.
>>>
>>> I would expect that identifying a cpu's or cluster's power domain
>>> would be done by a phandle between the cpu/cluster node and power
>>> domain node. But I've not really looked at the power domain bindings
>>> so who knows.
>>
>> I would expect the same, meaning that a cpu node, like any other device
>> node would have a phandle pointing at the respective HW power domain.
>>
> CPUs have phandles to their domains. That is how the relationship
> between the domain provider (power-controller) and the consumer (CPU) is
> established.
>
>> I do not really understand why we want a "generic" CPU power domain, what
>> purpose does it serve ? Creating a collection of cpu devices that we
>> can call "cluster" ?
>>
> Nope, not for calling a cluster, a cluster :)
>
> This compatible is used to define a generic behavior of the CPU domain
> controller (in addition to the platform specific behavior of the domain
> power controller). The kernel activities for such power controller are
> generally the same which otherwise would be repeated across platforms.
Having gone through this series and the one using it[1], the only common
activity is just cluster pm notifiers. Other than that it's just
creating indirection for now. The scenario might change in future, but
for now it seems unnecessary.
Also if you look at the shmobile power controller driver, it covers all
the devices including CPUs unlike QCOM power controller which handles
only CPU. Yes we can skip CPU genpd creation there only for CPU, IMO
creating the power domains should be part of power controller driver.
You can add helper functions for all the ARM specific code that can be
reused by multiple power controller drivers handling CPU/Cluster power
domain.
> An analogy to this would be the "arm,idle-state" that defines the DT
> node as something that also depicts a generic cpuidle C state.
>
I tend to disagree. In idle-states, the nodes define that generic
properties and they can be parsed in a generic way. That's not the case
here. Each power controller binding differs.
Yes the generic compatible might be useful to identify that this power
domain handles CPU/Cluster, but there will be more power controller
specific things compared to generic code.
Regards,
Sudeep
[1] http://www.spinics.net/lists/arm-kernel/msg437304.html
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 17:26 ` Sudeep Holla
@ 2015-08-13 19:27 ` Lina Iyer
2015-08-14 9:52 ` Sudeep Holla
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-13 19:27 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Aug 13 2015 at 11:26 -0600, Sudeep Holla wrote:
>
>
>On 13/08/15 16:45, Lina Iyer wrote:
>>On Thu, Aug 13 2015 at 09:01 -0600, Lorenzo Pieralisi wrote:
>>>On Thu, Aug 06, 2015 at 04:14:51AM +0100, Rob Herring wrote:
>>>>On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>
>[..]
>
>>>>
>>>>>+ARM CPU Power domains
>>>>>+
>>>>>+The device tree allows describing of CPU power domains in a SoC. In ARM SoC,
>>>>>+CPUs may be grouped as clusters. A cluster may have CPUs, GIC, Coresight,
>>>>>+caches, VFP and power controller and other peripheral hardware. Generally,
>>>>>+when the CPUs in the cluster are idle/suspended, the shared resources may also
>>>>>+be suspended and resumed before any of the CPUs resume execution.
>>>>>+
>>>>>+CPUs are the defined as the PM domain consumers and there is a PM domain
>>>>>+provider for the CPUs. Bindings for generic PM domains (genpd) is described in
>>>>>+[1].
>>>>>+
>>>>>+The ARM CPU PM domain follows the same binding convention as any generic PM
>>>>>+domain. Additional binding properties are -
>>>>>+
>>>>>+- compatible:
>>>>>+ Usage: required
>>>>>+ Value type: <string>
>>>>>+ Definition: Must also have
>>>>>+ "arm,pd"
>>>>>+ inorder to initialize the genpd provider as ARM CPU PM domain.
>>>>
>>>>A compatible string should represent a particular h/w block. If it is
>>>>generic, it should represent some sort of standard programming
>>>>interface (e.g, AHCI, EHCI, etc.). This doesn't seem to be either and
>>>>is rather just a mapping of what "driver" you want to use.
>>>>
>>>>I would expect that identifying a cpu's or cluster's power domain
>>>>would be done by a phandle between the cpu/cluster node and power
>>>>domain node. But I've not really looked at the power domain bindings
>>>>so who knows.
>>>
>>>I would expect the same, meaning that a cpu node, like any other device
>>>node would have a phandle pointing at the respective HW power domain.
>>>
>>CPUs have phandles to their domains. That is how the relationship
>>between the domain provider (power-controller) and the consumer (CPU) is
>>established.
>>
>>>I do not really understand why we want a "generic" CPU power domain, what
>>>purpose does it serve ? Creating a collection of cpu devices that we
>>>can call "cluster" ?
>>>
>>Nope, not for calling a cluster, a cluster :)
>>
>>This compatible is used to define a generic behavior of the CPU domain
>>controller (in addition to the platform specific behavior of the domain
>>power controller). The kernel activities for such power controller are
>>generally the same which otherwise would be repeated across platforms.
>
>Having gone through this series and the one using it[1], the only common
>activity is just cluster pm notifiers. Other than that it's just
>creating indirection for now. The scenario might change in future, but
>for now it seems unnecessary.
>
Not sure, what seems unnecessary to you. Platforms do have to send
cluster PM notifications, and they have to duplicate reference counting.
Also PM domain framework allows hierarchy which is quite desirable to
power down parts of the SoC that are powered on or have to clocked high,
until the CPU is running.
Cluster PM notifications are just one aspect of this that we currently
handle in the first submission. The patchset as a whole provides a way
to determine in Linux the last man down and the first man up and carry
out activities. There are a bunch of things that are done to power save
when the last man goes down - Turn off debuggers, switch off PLLs,
reduce bus clocks, flush caches amongst a few that I know of. Some of it
are platform specific and some of it arent. This patches provide the way
for both of them to be done easily. The CPU runtime PM and PM domains as
a framework, closely track what the hardware does.
Mentioned in an other mail in this thread, is also an option to
determine the cluster flush state and use it in conjunction with PSCI to
do OS-Initiated cluster power down.
>Also if you look at the shmobile power controller driver, it covers all
>the devices including CPUs unlike QCOM power controller which handles
>only CPU. Yes we can skip CPU genpd creation there only for CPU, IMO
>creating the power domains should be part of power controller driver.
>
>You can add helper functions for all the ARM specific code that can be
>reused by multiple power controller drivers handling CPU/Cluster power
>domain.
>
Sure, some architectures may desire that. I have them addressed in [2].
>>An analogy to this would be the "arm,idle-state" that defines the DT
>>node as something that also depicts a generic cpuidle C state.
>>
>
>I tend to disagree. In idle-states, the nodes define that generic
>properties and they can be parsed in a generic way. That's not the case
>here. Each power controller binding differs.
>
Yes, may be we will have common elements like latency, residency of
powering on/off a domain that a genpd governor can utilize in
determining if its worth powering off the domain or not.
>Yes the generic compatible might be useful to identify that this power
>domain handles CPU/Cluster, but there will be more power controller
>specific things compared to generic code.
>
Agreed, not debating that. Power controller is very SoC specific, but
not debuggers, GIC, caches, buses etc. Many SoCs have almost similiar
needs for many of these supplemental hardware to the CPUs and trend
seems to be generalizing on many of these components.
Thanks,
Lina
>[1] http://www.spinics.net/lists/arm-kernel/msg437304.html
[2] http://www.spinics.net/lists/arm-kernel/msg438971.html
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters
2015-08-13 19:27 ` Lina Iyer
@ 2015-08-14 9:52 ` Sudeep Holla
0 siblings, 0 replies; 60+ messages in thread
From: Sudeep Holla @ 2015-08-14 9:52 UTC (permalink / raw)
To: linux-arm-kernel
On 13/08/15 20:27, Lina Iyer wrote:
> On Thu, Aug 13 2015 at 11:26 -0600, Sudeep Holla wrote:
>>
[...]
>>
>> Having gone through this series and the one using it[1], the only common
>> activity is just cluster pm notifiers. Other than that it's just
>> creating indirection for now. The scenario might change in future, but
>> for now it seems unnecessary.
>>
> Not sure, what seems unnecessary to you. Platforms do have to send
> cluster PM notifications, and they have to duplicate reference counting.
> Also PM domain framework allows hierarchy which is quite desirable to
> power down parts of the SoC that are powered on or have to clocked high,
> until the CPU is running.
>
Agreed, no argument on using genpd for CPU PM for all the goodies genpd
provides, but the way this patch was creating the genpd domains. It
needs to be part of your power controller.
> Cluster PM notifications are just one aspect of this that we currently
> handle in the first submission. The patchset as a whole provides a way
> to determine in Linux the last man down and the first man up and carry
> out activities. There are a bunch of things that are done to power save
> when the last man goes down - Turn off debuggers, switch off PLLs,
> reduce bus clocks, flush caches amongst a few that I know of. Some of it
> are platform specific and some of it arent. This patches provide the way
> for both of them to be done easily. The CPU runtime PM and PM domains as
> a framework, closely track what the hardware does.
>
Again no argument, I just favour common interface functions. Since each
power controller/platform will have specific sequence, it might be hard
to generalize that, but I may be wrong. OTH common interface functions
to handle those components might give some flexibility to the power
controllers.
> Mentioned in an other mail in this thread, is also an option to
> determine the cluster flush state and use it in conjunction with PSCI to
> do OS-Initiated cluster power down.
>
I haven't yet explored down that route yet, with platform co-ordination
we don't need much complexity in kernel :). OS co-ordination is a
different story as we need to consider the secure/non-secure world
dimensions there. We will have to consider the privileges/restrictions
Linux has.
>> Also if you look at the shmobile power controller driver, it covers all
>> the devices including CPUs unlike QCOM power controller which handles
>> only CPU. Yes we can skip CPU genpd creation there only for CPU, IMO
>> creating the power domains should be part of power controller driver.
>>
>> You can add helper functions for all the ARM specific code that can be
>> reused by multiple power controller drivers handling CPU/Cluster power
>> domain.
>>
> Sure, some architectures may desire that. I have them addressed in [2].
>
I haven't looked at that yet, but will do soon.
>>> An analogy to this would be the "arm,idle-state" that defines the DT
>>> node as something that also depicts a generic cpuidle C state.
>>>
>>
>> I tend to disagree. In idle-states, the nodes define that generic
>> properties and they can be parsed in a generic way. That's not the case
>> here. Each power controller binding differs.
>>
> Yes, may be we will have common elements like latency, residency of
> powering on/off a domain that a genpd governor can utilize in
> determining if its worth powering off the domain or not.
>
Make sense, but as Rob pointed how generic are those on various
platforms is something we need to check considering few platforms before
we build the generic infrastructure.
>> Yes the generic compatible might be useful to identify that this power
>> domain handles CPU/Cluster, but there will be more power controller
>> specific things compared to generic code.
>>
> Agreed, not debating that. Power controller is very SoC specific, but
> not debuggers, GIC, caches, buses etc. Many SoCs have almost similiar
> needs for many of these supplemental hardware to the CPUs and trend
> seems to be generalizing on many of these components.
>
Generalizing those components and using genpd is absolutely fine, just
the mechanics is what I am debating on.
Regards,
Sudeep
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (4 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 5/9] ARM: common: Introduce PM domains for CPUs/clusters Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-05 14:45 ` Rob Herring
2015-08-04 23:35 ` [PATCH 7/9] ARM: cpuidle: Add runtime PM support for CPU idle Lina Iyer
` (2 subsequent siblings)
8 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
In addition to the common power up/down actions of CPU PM domain core,
platforms may have additional configuration before the CPU domain can be
powered off or considered active. Allow platform drivers to register
handlers for CPU PM domains.
Platform drivers may register their callbacks against a compatible
string defined by their PM domain provider device node in the DT. At
domain init, the platform driver can initialize the platform specific
genpd attributes. The init callback would need to return successfully,
for the platform power_on/off handlers to be registered with the CPU PM
domain.
The code uses __init section to reduce memory needed for platform
handlers and therefore can be freed after the driver is initialized, a
desirable outcome for single kernel image.
Cc: Rob Herring <robh@kernel.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Documentation/arm/cpu-domains.txt | 26 ++++++++++++++++++++++++++
arch/arm/common/domains.c | 37 +++++++++++++++++++++++++++++++++++++
arch/arm/include/asm/arm-pd.h | 30 ++++++++++++++++++++++++++++++
include/asm-generic/vmlinux.lds.h | 2 ++
4 files changed, 95 insertions(+)
create mode 100644 arch/arm/include/asm/arm-pd.h
diff --git a/Documentation/arm/cpu-domains.txt b/Documentation/arm/cpu-domains.txt
index 3e535b7..a0b98f3 100644
--- a/Documentation/arm/cpu-domains.txt
+++ b/Documentation/arm/cpu-domains.txt
@@ -47,3 +47,29 @@ attaches the domains' CPU devices to as specified in the DT. This happens
automatically at kernel init, when the domain is specified as compatible with
"arm,pd". Powering on/off the common cluster hardware would also be done when
the PM domain is runtime suspended or resumed.
+
+SoCs may have additional configuration for the CPU PM domain. The ARM code
+provides a way for the platform driver to add those properties to the genpd
+before the genpd object is initialized. Additionally, platform driver may also
+register for CPU domain power_on/power_off callbacks.
+
+Platform drivers may register the callbacks using the __init section macro
+ARM_PD_METHOD_OF_DECLARE. The callbacks in of_arm_pd_ops, can be specified
+against a compatible flag for the domain provider.
+
+Callback for platform drivers -
+
+int (*init)(struct device_node *dn, struct generic_pm_domain *d)
+The init() callback is called before the generic PM domain is registered with
+the GenPD framework. The device node is provided to identify the domain that
+is being initialized. The init function must return 0, in order for the
+power_on and power_off callbacks to be registered with the CPU PD framework.
+
+int (*power_on)(struct generic_pm_domain *d);
+The power_on() callback is called when the first CPU in the cluster is ready
+to resume execution. The domain may be considered active at this point.
+
+int (*power_off)(struct generic_pm_domain *d);
+The power_off() callback is called when the last CPU in the cluster enters
+idle. The domain may be configured to power off and wait for a CPU's wakeup
+interrupt.
diff --git a/arch/arm/common/domains.c b/arch/arm/common/domains.c
index 15981e9..bbffeed 100644
--- a/arch/arm/common/domains.c
+++ b/arch/arm/common/domains.c
@@ -18,12 +18,19 @@
#include <linux/pm_runtime.h>
#include <linux/slab.h>
+#include <asm/arm-pd.h>
+
#define NAME_MAX 36
struct arm_pm_domain {
struct generic_pm_domain genpd;
+ struct of_arm_pd_ops platform_ops;
};
+extern struct of_arm_pd_method __arm_pd_method_of_table[];
+static const struct of_arm_pd_method __arm_pd_method_of_table_sentinel
+ __used __section(__arm_pd_method_of_table_end);
+
static inline
struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
{
@@ -32,20 +39,30 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d)
static int arm_pd_power_down(struct generic_pm_domain *genpd)
{
+ struct arm_pm_domain *pd = to_arm_pd(genpd);
+
/*
* Notify CPU PM domain power down
* TODO: Call the notificated directly from here.
*/
cpu_cluster_pm_enter();
+ if (pd->platform_ops.power_off)
+ return pd->platform_ops.power_off(genpd);
+
return 0;
}
static int arm_pd_power_up(struct generic_pm_domain *genpd)
{
+ struct arm_pm_domain *pd = to_arm_pd(genpd);
+
/* Notify CPU PM domain power up */
cpu_cluster_pm_exit();
+ if (pd->platform_ops.power_on)
+ return pd->platform_ops.power_on(genpd);
+
return 0;
}
@@ -134,6 +151,7 @@ static int __init arm_domain_init(void)
{
struct device_node *np;
int count = 0;
+ struct of_arm_pd_method *m = __arm_pd_method_of_table;
for_each_compatible_node(np, NULL, "arm,pd") {
struct arm_pm_domain *pd;
@@ -145,6 +163,25 @@ static int __init arm_domain_init(void)
if (!pd)
return -ENOMEM;
+ /* Invoke platform initialization for the PM domain */
+ for (; m->handle; m++) {
+ int ret;
+
+ if (of_device_is_compatible(np, m->handle)) {
+ ret = m->ops->init(np, &pd->genpd);
+ if (!ret) {
+ pr_debug("CPU PD ops found for %s\n",
+ m->handle);
+ pd->platform_ops.power_on =
+ m->ops->power_on;
+ pd->platform_ops.power_off =
+ m->ops->power_off;
+ }
+ break;
+ }
+ }
+
+ /* Initialize rest of CPU PM domain specifics */
pd->genpd.name = kstrndup(np->name, NAME_MAX, GFP_KERNEL);
pd->genpd.power_off = arm_pd_power_down;
pd->genpd.power_on = arm_pd_power_up;
diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h
new file mode 100644
index 0000000..fc44abf
--- /dev/null
+++ b/arch/arm/include/asm/arm-pd.h
@@ -0,0 +1,30 @@
+/*
+ * linux/arch/arm/include/asm/arm-pd.h
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ARM_PD_H__
+#define __ARM_PD_H__
+
+struct of_arm_pd_ops {
+ int (*init)(struct device_node *dn, struct generic_pm_domain *d);
+ int (*power_on)(struct generic_pm_domain *d);
+ int (*power_off)(struct generic_pm_domain *d);
+};
+
+struct of_arm_pd_method {
+ const char *handle;
+ struct of_arm_pd_ops *ops;
+};
+
+#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
+ static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \
+ __used __section(__arm_pd_method_of_table) \
+ = { .handle = _handle, .ops = _ops }
+
+#endif /* __ARM_PD_H__ */
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 8bd374d..bd97a69 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,6 +179,7 @@
#define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
#define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
+#define ARM_PD_METHOD_OF_TABLES() OF_TABLE(CONFIG_PM_GENERIC_DOMAINS, arm_pd_method)
#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
#define KERNEL_DTB() \
@@ -514,6 +515,7 @@
IOMMU_OF_TABLES() \
CPU_METHOD_OF_TABLES() \
CPUIDLE_METHOD_OF_TABLES() \
+ ARM_PD_METHOD_OF_TABLES() \
KERNEL_DTB() \
IRQCHIP_OF_MATCH_TABLE() \
EARLYCON_TABLE() \
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-04 23:35 ` [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains Lina Iyer
@ 2015-08-05 14:45 ` Rob Herring
2015-08-05 16:38 ` Lina Iyer
2015-08-05 19:23 ` Lina Iyer
0 siblings, 2 replies; 60+ messages in thread
From: Rob Herring @ 2015-08-05 14:45 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> In addition to the common power up/down actions of CPU PM domain core,
> platforms may have additional configuration before the CPU domain can be
> powered off or considered active. Allow platform drivers to register
> handlers for CPU PM domains.
>
> Platform drivers may register their callbacks against a compatible
> string defined by their PM domain provider device node in the DT. At
> domain init, the platform driver can initialize the platform specific
> genpd attributes. The init callback would need to return successfully,
> for the platform power_on/off handlers to be registered with the CPU PM
> domain.
>
> The code uses __init section to reduce memory needed for platform
> handlers and therefore can be freed after the driver is initialized, a
> desirable outcome for single kernel image.
[...]
> diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h
> new file mode 100644
> index 0000000..fc44abf
> --- /dev/null
> +++ b/arch/arm/include/asm/arm-pd.h
> @@ -0,0 +1,30 @@
> +/*
> + * linux/arch/arm/include/asm/arm-pd.h
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ARM_PD_H__
> +#define __ARM_PD_H__
> +
> +struct of_arm_pd_ops {
> + int (*init)(struct device_node *dn, struct generic_pm_domain *d);
> + int (*power_on)(struct generic_pm_domain *d);
> + int (*power_off)(struct generic_pm_domain *d);
> +};
> +
> +struct of_arm_pd_method {
> + const char *handle;
> + struct of_arm_pd_ops *ops;
> +};
> +
> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
> + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \
> + __used __section(__arm_pd_method_of_table) \
> + = { .handle = _handle, .ops = _ops }
AFAICT, you are not using this in this series. You should add it when
you have a user.
Ideally, we keep some amount of uniformity across various *_OF_DECLARE
which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the
sections just arrays of struct of_device_id. Not all users follow
this, but most do. So instead of putting the ops in here, platforms
can provide a function callback which can then set the ops. Then you
also don't need the .init() ops function as the callback function can
do any initialization too.
Rob
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-05 14:45 ` Rob Herring
@ 2015-08-05 16:38 ` Lina Iyer
2015-08-05 19:23 ` Lina Iyer
1 sibling, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-05 16:38 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote:
>On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> In addition to the common power up/down actions of CPU PM domain core,
>> platforms may have additional configuration before the CPU domain can be
>> powered off or considered active. Allow platform drivers to register
>> handlers for CPU PM domains.
>>
>> Platform drivers may register their callbacks against a compatible
>> string defined by their PM domain provider device node in the DT. At
>> domain init, the platform driver can initialize the platform specific
>> genpd attributes. The init callback would need to return successfully,
>> for the platform power_on/off handlers to be registered with the CPU PM
>> domain.
>>
>> The code uses __init section to reduce memory needed for platform
>> handlers and therefore can be freed after the driver is initialized, a
>> desirable outcome for single kernel image.
>
>[...]
>
>> diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h
>> new file mode 100644
>> index 0000000..fc44abf
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arm-pd.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * linux/arch/arm/include/asm/arm-pd.h
>> + *
>> + * Copyright (C) 2015 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __ARM_PD_H__
>> +#define __ARM_PD_H__
>> +
>> +struct of_arm_pd_ops {
>> + int (*init)(struct device_node *dn, struct generic_pm_domain *d);
>> + int (*power_on)(struct generic_pm_domain *d);
>> + int (*power_off)(struct generic_pm_domain *d);
>> +};
>> +
>> +struct of_arm_pd_method {
>> + const char *handle;
>> + struct of_arm_pd_ops *ops;
>> +};
>> +
>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
>> + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \
>> + __used __section(__arm_pd_method_of_table) \
>> + = { .handle = _handle, .ops = _ops }
>
>AFAICT, you are not using this in this series. You should add it when
>you have a user.
>
Sorry, I had a last minute change of heart about some commit text and
could not get to send the complete series yesterday. I tried to send the
rest of the patches, the one that uses this macro, as in-reply-to but
that did not work for some reason.
Anyways, you are Cc'd in the driver series.
>Ideally, we keep some amount of uniformity across various *_OF_DECLARE
>which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the
>sections just arrays of struct of_device_id. Not all users follow
>this, but most do. So instead of putting the ops in here, platforms
>can provide a function callback which can then set the ops. Then you
>also don't need the .init() ops function as the callback function can
>do any initialization too.
>
Okay, will look into that.
>Rob
Thanks for your time Rob.
-- Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-05 14:45 ` Rob Herring
2015-08-05 16:38 ` Lina Iyer
@ 2015-08-05 19:23 ` Lina Iyer
2015-08-06 3:01 ` Rob Herring
1 sibling, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-05 19:23 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote:
>On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> In addition to the common power up/down actions of CPU PM domain core,
>> platforms may have additional configuration before the CPU domain can be
>> powered off or considered active. Allow platform drivers to register
>> handlers for CPU PM domains.
>>
>> Platform drivers may register their callbacks against a compatible
>> string defined by their PM domain provider device node in the DT. At
>> domain init, the platform driver can initialize the platform specific
>> genpd attributes. The init callback would need to return successfully,
>> for the platform power_on/off handlers to be registered with the CPU PM
>> domain.
>>
>> The code uses __init section to reduce memory needed for platform
>> handlers and therefore can be freed after the driver is initialized, a
>> desirable outcome for single kernel image.
>
>[...]
>
>> diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h
>> new file mode 100644
>> index 0000000..fc44abf
[...]
>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
>> + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \
>> + __used __section(__arm_pd_method_of_table) \
>> + = { .handle = _handle, .ops = _ops }
>
>AFAICT, you are not using this in this series. You should add it when
>you have a user.
>
>Ideally, we keep some amount of uniformity across various *_OF_DECLARE
>which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the
>sections just arrays of struct of_device_id. Not all users follow
>this, but most do. So instead of putting the ops in here, platforms
>can provide a function callback which can then set the ops. Then you
>also don't need the .init() ops function as the callback function can
>do any initialization too.
>
I looked through these and I can switch over to _OF_DECLARE() without any
issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting.
If I could set up my own function type for the .data member, then its a
lot more easier on the code. Like this -
typedef int (*arm_pd_init)(struct device_node *dn, struct
generic_pm_domain *genpd, struct of_arm_pd_ops *ops);
#define ARM_PD_METHOD_OF_DECLARE(name, compat, fn) \
_OF_DECLARE(arm_pd, name, compat, fn, arm_pd_init)
Its still in the of_device_id array.
Is that acceptable?
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-05 19:23 ` Lina Iyer
@ 2015-08-06 3:01 ` Rob Herring
2015-08-10 15:36 ` Lina Iyer
0 siblings, 1 reply; 60+ messages in thread
From: Rob Herring @ 2015-08-06 3:01 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 5, 2015 at 2:23 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote:
>>
>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> In addition to the common power up/down actions of CPU PM domain core,
>>> platforms may have additional configuration before the CPU domain can be
>>> powered off or considered active. Allow platform drivers to register
>>> handlers for CPU PM domains.
>>>
>>> Platform drivers may register their callbacks against a compatible
>>> string defined by their PM domain provider device node in the DT. At
>>> domain init, the platform driver can initialize the platform specific
>>> genpd attributes. The init callback would need to return successfully,
>>> for the platform power_on/off handlers to be registered with the CPU PM
>>> domain.
>>>
>>> The code uses __init section to reduce memory needed for platform
>>> handlers and therefore can be freed after the driver is initialized, a
>>> desirable outcome for single kernel image.
>>
>>
>> [...]
>>
>>> diff --git a/arch/arm/include/asm/arm-pd.h
>>> b/arch/arm/include/asm/arm-pd.h
>>> new file mode 100644
>>> index 0000000..fc44abf
>
>
> [...]
>
>>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
>>> + static const struct of_arm_pd_method
>>> __arm_pd_method_of_table_##_name \
>>> + __used __section(__arm_pd_method_of_table) \
>>> + = { .handle = _handle, .ops = _ops }
>>
>>
>> AFAICT, you are not using this in this series. You should add it when
>> you have a user.
>>
>> Ideally, we keep some amount of uniformity across various *_OF_DECLARE
>> which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the
>> sections just arrays of struct of_device_id. Not all users follow
>> this, but most do. So instead of putting the ops in here, platforms
>> can provide a function callback which can then set the ops. Then you
>> also don't need the .init() ops function as the callback function can
>> do any initialization too.
>>
> I looked through these and I can switch over to _OF_DECLARE() without any
> issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting.
Well yes, but then we only want to use it for things that will never
use the driver model.
> If I could set up my own function type for the .data member, then its a
> lot more easier on the code. Like this -
>
> typedef int (*arm_pd_init)(struct device_node *dn, struct
> generic_pm_domain *genpd, struct of_arm_pd_ops *ops);
You alloc the genpd struct just before calling init. You can just as
easily have the platform specific code call an allocation function and
a function to set the ops functions. This is more inline with how a
driver would work when registering with a subsystem. I also don't
think "arm,pd" is a good compatible string to be matching on. I'll
comment on that more in patch 5.
Rob
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains
2015-08-06 3:01 ` Rob Herring
@ 2015-08-10 15:36 ` Lina Iyer
0 siblings, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-10 15:36 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 05 2015 at 21:01 -0600, Rob Herring wrote:
>On Wed, Aug 5, 2015 at 2:23 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote:
>>>
>>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>
>>>> In addition to the common power up/down actions of CPU PM domain core,
>>>> platforms may have additional configuration before the CPU domain can be
>>>> powered off or considered active. Allow platform drivers to register
>>>> handlers for CPU PM domains.
>>>>
>>>> Platform drivers may register their callbacks against a compatible
>>>> string defined by their PM domain provider device node in the DT. At
>>>> domain init, the platform driver can initialize the platform specific
>>>> genpd attributes. The init callback would need to return successfully,
>>>> for the platform power_on/off handlers to be registered with the CPU PM
>>>> domain.
>>>>
>>>> The code uses __init section to reduce memory needed for platform
>>>> handlers and therefore can be freed after the driver is initialized, a
>>>> desirable outcome for single kernel image.
>>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm/include/asm/arm-pd.h
>>>> b/arch/arm/include/asm/arm-pd.h
>>>> new file mode 100644
>>>> index 0000000..fc44abf
>>
>>
>> [...]
>>
>>>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \
>>>> + static const struct of_arm_pd_method
>>>> __arm_pd_method_of_table_##_name \
>>>> + __used __section(__arm_pd_method_of_table) \
>>>> + = { .handle = _handle, .ops = _ops }
>>>
>>>
>>> AFAICT, you are not using this in this series. You should add it when
>>> you have a user.
>>>
>>> Ideally, we keep some amount of uniformity across various *_OF_DECLARE
>>> which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the
>>> sections just arrays of struct of_device_id. Not all users follow
>>> this, but most do. So instead of putting the ops in here, platforms
>>> can provide a function callback which can then set the ops. Then you
>>> also don't need the .init() ops function as the callback function can
>>> do any initialization too.
>>>
>> I looked through these and I can switch over to _OF_DECLARE() without any
>> issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting.
>
>Well yes, but then we only want to use it for things that will never
>use the driver model.
>
>> If I could set up my own function type for the .data member, then its a
>> lot more easier on the code. Like this -
>>
>> typedef int (*arm_pd_init)(struct device_node *dn, struct
>> generic_pm_domain *genpd, struct of_arm_pd_ops *ops);
>
>You alloc the genpd struct just before calling init. You can just as
>easily have the platform specific code call an allocation function and
>a function to set the ops functions. This is more inline with how a
>driver would work when registering with a subsystem.
>
OK. That would mean I have a way of mapping the device_node to the
of_arm_pd_ops, which I currently dont. Was trying to avoid a list. But,
I was able to use list and use OF_DECLARE_1. The platform driver, shall
call up separate API to register the ops or ge the Genpd and modify it.
Will submit it in the next spin, following the discussion on arm,pd.
Thanks,
Lina
>I also don't
>think "arm,pd" is a good compatible string to be matching on. I'll
>comment on that more in patch 5.
>
>Rob
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 7/9] ARM: cpuidle: Add runtime PM support for CPU idle
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (5 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 6/9] ARM: domain: Add platform handlers for CPU PM domains Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-04 23:35 ` [PATCH 8/9] ARM64: smp: Add runtime PM support for CPU hotplug Lina Iyer
2015-08-04 23:35 ` [PATCH 9/9] ARM: " Lina Iyer
8 siblings, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Notify runtime PM when the CPU is going to be powered off in the idle
state. This allows for runtime PM suspend/resume of the CPU as well as
its PM domain.
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/cpuidle/cpuidle-arm.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..ca118ed 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -11,12 +11,14 @@
#define pr_fmt(fmt) "CPUidle arm: " fmt
+#include <linux/cpu.h>
#include <linux/cpuidle.h>
#include <linux/cpumask.h>
#include <linux/cpu_pm.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <asm/cpuidle.h>
@@ -37,6 +39,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int idx)
{
int ret;
+ struct device *cpu_dev = get_cpu_device(dev->cpu);
if (!idx) {
cpu_do_idle();
@@ -46,12 +49,19 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
ret = cpu_pm_enter();
if (!ret) {
/*
+ * Notify runtime PM as well of this cpu powering down
+ * TODO: Merge CPU_PM and runtime PM.
+ */
+ pm_runtime_put_sync(cpu_dev);
+
+ /*
* Pass idle state index to cpu_suspend which in turn will
* call the CPU ops suspend protocol with idle index as a
* parameter.
*/
arm_cpuidle_suspend(idx);
+ pm_runtime_get_sync(cpu_dev);
cpu_pm_exit();
}
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 8/9] ARM64: smp: Add runtime PM support for CPU hotplug
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (6 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 7/9] ARM: cpuidle: Add runtime PM support for CPU idle Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-04 23:35 ` [PATCH 9/9] ARM: " Lina Iyer
8 siblings, 0 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Enable runtime PM for CPU devices. Do a runtime get of the CPU device
when the CPU is hotplugged in and runtime put of the CPU device when the
CPU is hotplugged off. When all the CPUs in a domain are hotplugged off,
the domain may also be powered off and cluster_pm_enter/exit()
notifications are be sent out.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
arch/arm64/kernel/smp.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..fbeeffa 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -135,6 +135,7 @@ asmlinkage void secondary_start_kernel(void)
{
struct mm_struct *mm = &init_mm;
unsigned int cpu = smp_processor_id();
+ struct device *cpu_dev;
/*
* All kernel threads share the same mm context; grab a
@@ -185,6 +186,11 @@ asmlinkage void secondary_start_kernel(void)
local_irq_enable();
local_async_enable();
+ /* We are running, enable runtime PM for the CPU. */
+ cpu_dev = get_cpu_device(cpu);
+ if (cpu_dev)
+ pm_runtime_get_sync(cpu_dev);
+
/*
* OK, it's off to the idle thread for us
*/
@@ -219,6 +225,9 @@ int __cpu_disable(void)
unsigned int cpu = smp_processor_id();
int ret;
+ /* We dont need the CPU device anymore. */
+ pm_runtime_put_sync(get_cpu_device(cpu));
+
ret = op_cpu_disable(cpu);
if (ret)
return ret;
@@ -293,6 +302,13 @@ void cpu_die(void)
{
unsigned int cpu = smp_processor_id();
+ /*
+ * We dont need the CPU device anymore.
+ * Lets do this before IRQs are disabled to allow
+ * runtime PM to suspend the domain as well.
+ */
+ pm_runtime_put_sync(get_cpu_device(cpu));
+
idle_task_exit();
local_irq_disable();
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-04 23:35 [PATCH 0/9] ARM: PM / Domains: Generic PM domains for CPUs/Clusters Lina Iyer
` (7 preceding siblings ...)
2015-08-04 23:35 ` [PATCH 8/9] ARM64: smp: Add runtime PM support for CPU hotplug Lina Iyer
@ 2015-08-04 23:35 ` Lina Iyer
2015-08-12 20:28 ` Kevin Hilman
2015-08-12 23:47 ` Stephen Boyd
8 siblings, 2 replies; 60+ messages in thread
From: Lina Iyer @ 2015-08-04 23:35 UTC (permalink / raw)
To: linux-arm-kernel
Enable runtime PM for CPU devices. Do a runtime get of the CPU device
when the CPU is hotplugged in and a runtime put of the CPU device when
the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
off, the domain may also be powered off and cluster_pm_enter/exit()
notifications are be sent out.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
arch/arm/kernel/smp.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 0496b48..1d614b8 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -27,6 +27,7 @@
#include <linux/completion.h>
#include <linux/cpufreq.h>
#include <linux/irq_work.h>
+#include <linux/pm_runtime.h>
#include <linux/atomic.h>
#include <asm/smp.h>
@@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
}
-
memset(&secondary_data, 0, sizeof(secondary_data));
return ret;
}
@@ -205,6 +205,9 @@ int __cpu_disable(void)
unsigned int cpu = smp_processor_id();
int ret;
+ /* We dont need the CPU device anymore. */
+ pm_runtime_put_sync(get_cpu_device(cpu));
+
ret = platform_cpu_disable(cpu);
if (ret)
return ret;
@@ -272,6 +275,13 @@ void __ref cpu_die(void)
{
unsigned int cpu = smp_processor_id();
+ /*
+ * We dont need the CPU device anymore.
+ * Lets do this before IRQs are disabled to allow
+ * runtime PM to suspend the domain as well.
+ */
+ pm_runtime_put_sync(get_cpu_device(cpu));
+
idle_task_exit();
local_irq_disable();
@@ -352,6 +362,7 @@ asmlinkage void secondary_start_kernel(void)
{
struct mm_struct *mm = &init_mm;
unsigned int cpu;
+ struct device *cpu_dev;
/*
* The identity mapping is uncached (strongly ordered), so
@@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
local_irq_enable();
local_fiq_enable();
+ /* We are running, enable runtime PM for the CPU. */
+ cpu_dev = get_cpu_device(cpu);
+ if (cpu_dev)
+ pm_runtime_get_sync(cpu_dev);
+
/*
* OK, it's off to the idle thread for us
*/
--
2.1.4
^ permalink raw reply related [flat|nested] 60+ messages in thread* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-04 23:35 ` [PATCH 9/9] ARM: " Lina Iyer
@ 2015-08-12 20:28 ` Kevin Hilman
2015-08-12 20:43 ` Lina Iyer
2015-08-12 23:47 ` Stephen Boyd
1 sibling, 1 reply; 60+ messages in thread
From: Kevin Hilman @ 2015-08-12 20:28 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> Enable runtime PM for CPU devices. Do a runtime get of the CPU device
> when the CPU is hotplugged in and a runtime put of the CPU device when
> the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
> off, the domain may also be powered off and cluster_pm_enter/exit()
> notifications are be sent out.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
How does the runtiem PM usage with hotplug work with the runtime PM
usage in idle?
IIUC, when a CPU is hotplugged in, it will always have a usecount of at
least 1, right? and if it's not idle, it will have done a _get() so it
will have a usecount of at least 2. So I'm not quite seeing how the
usecount will ever go to zero in idle and allow the domain to power_off.
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-12 20:28 ` Kevin Hilman
@ 2015-08-12 20:43 ` Lina Iyer
2015-08-14 18:59 ` Kevin Hilman
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-12 20:43 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 12 2015 at 14:28 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> Enable runtime PM for CPU devices. Do a runtime get of the CPU device
>> when the CPU is hotplugged in and a runtime put of the CPU device when
>> the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
>> off, the domain may also be powered off and cluster_pm_enter/exit()
>> notifications are be sent out.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
>How does the runtiem PM usage with hotplug work with the runtime PM
>usage in idle?
>
>IIUC, when a CPU is hotplugged in, it will always have a usecount of at
>least 1, right? and if it's not idle, it will have done a _get() so it
>will have a usecount of at least 2. So I'm not quite seeing how the
>usecount will ever go to zero in idle and allow the domain to power_off.
>
When the CPU is online and running, it would have a ref count of 1. Then
cpuidle would do a _put and the ref count would go to 0 and when coming
out of idle, cpuidle would get a _get and the ref count will be back at
1. Ref count is not incremented when cpuidle is initialized on the CPU.
So whenever the CPU is running, it would have a ref count of 1.
As of today, atleast my understanding is hotplug does not go back into
cpuidle, so the ref count would go to 0 when the core is hotplugged off.
This may change, if the CPU hotplug is routed to cpuidle.
Am I wrong in my understanding?
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-12 20:43 ` Lina Iyer
@ 2015-08-14 18:59 ` Kevin Hilman
0 siblings, 0 replies; 60+ messages in thread
From: Kevin Hilman @ 2015-08-14 18:59 UTC (permalink / raw)
To: linux-arm-kernel
Lina Iyer <lina.iyer@linaro.org> writes:
> On Wed, Aug 12 2015 at 14:28 -0600, Kevin Hilman wrote:
>>Lina Iyer <lina.iyer@linaro.org> writes:
>>
>>> Enable runtime PM for CPU devices. Do a runtime get of the CPU device
>>> when the CPU is hotplugged in and a runtime put of the CPU device when
>>> the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
>>> off, the domain may also be powered off and cluster_pm_enter/exit()
>>> notifications are be sent out.
>>>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>>How does the runtiem PM usage with hotplug work with the runtime PM
>>usage in idle?
>>
>>IIUC, when a CPU is hotplugged in, it will always have a usecount of at
>>least 1, right? and if it's not idle, it will have done a _get() so it
>>will have a usecount of at least 2. So I'm not quite seeing how the
>>usecount will ever go to zero in idle and allow the domain to power_off.
>>
> When the CPU is online and running, it would have a ref count of 1. Then
> cpuidle would do a _put and the ref count would go to 0 and when coming
> out of idle, cpuidle would get a _get and the ref count will be back at
> 1. Ref count is not incremented when cpuidle is initialized on the CPU.
> So whenever the CPU is running, it would have a ref count of 1.
Ah, OK, that was the part I was missing.
Thanks,
Kevin
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-04 23:35 ` [PATCH 9/9] ARM: " Lina Iyer
2015-08-12 20:28 ` Kevin Hilman
@ 2015-08-12 23:47 ` Stephen Boyd
2015-08-13 16:00 ` Lina Iyer
1 sibling, 1 reply; 60+ messages in thread
From: Stephen Boyd @ 2015-08-12 23:47 UTC (permalink / raw)
To: linux-arm-kernel
On 08/04, Lina Iyer wrote:
> @@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
> pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
> }
>
> -
Remove noise please.
> memset(&secondary_data, 0, sizeof(secondary_data));
> return ret;
> }
> @@ -205,6 +205,9 @@ int __cpu_disable(void)
> unsigned int cpu = smp_processor_id();
> int ret;
>
> + /* We dont need the CPU device anymore. */
> + pm_runtime_put_sync(get_cpu_device(cpu));
This is all very generic. Any reason it can't be done at a higher
level for all architectures? It certainly seems like
cpu_startup_entry() could be modifed to do the
pm_runtime_get_sync().
> +
> ret = platform_cpu_disable(cpu);
> if (ret)
> return ret;
> @@ -272,6 +275,13 @@ void __ref cpu_die(void)
> {
> unsigned int cpu = smp_processor_id();
>
> + /*
> + * We dont need the CPU device anymore.
> + * Lets do this before IRQs are disabled to allow
> + * runtime PM to suspend the domain as well.
> + */
> + pm_runtime_put_sync(get_cpu_device(cpu));
The two put calls is confusing. __cpu_disable() is called on the
CPU that's dying, and cpu_die() is called on the CPU that's doing
the takedown. That would be two decrements but only one increment
in secondary_start_kernel()? How is this properly balanced?
> +
> idle_task_exit();
>
> local_irq_disable();
> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
> local_irq_enable();
> local_fiq_enable();
>
> + /* We are running, enable runtime PM for the CPU. */
> + cpu_dev = get_cpu_device(cpu);
> + if (cpu_dev)
> + pm_runtime_get_sync(cpu_dev);
Also, where would the dev->power.irq_safe flag be set if we
aren't using the genpd DT stuff? It looks like we're going to
start causing warnings on devices that don't have the DT magic.
Please add a hotplug test with some device that isn't using this
genpd code to catch problems. Also please turn on lockdep and RCU
lockdep, touching the idle code like this
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-12 23:47 ` Stephen Boyd
@ 2015-08-13 16:00 ` Lina Iyer
2015-08-13 19:18 ` Stephen Boyd
0 siblings, 1 reply; 60+ messages in thread
From: Lina Iyer @ 2015-08-13 16:00 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 12 2015 at 17:47 -0600, Stephen Boyd wrote:
>On 08/04, Lina Iyer wrote:
>> @@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>> pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
>> }
>>
>> -
>
>Remove noise please.
>
OK
>> memset(&secondary_data, 0, sizeof(secondary_data));
>> return ret;
>> }
>> @@ -205,6 +205,9 @@ int __cpu_disable(void)
>> unsigned int cpu = smp_processor_id();
>> int ret;
>>
>> + /* We dont need the CPU device anymore. */
>> + pm_runtime_put_sync(get_cpu_device(cpu));
>
>This is all very generic. Any reason it can't be done at a higher
>level for all architectures? It certainly seems like
>cpu_startup_entry() could be modifed to do the
>pm_runtime_get_sync().
>
I am suspecting, when the concept of CPU PM domains are finalized, they
would probably move out of the ARM domain and into generic. Will keep
that in mind.
>> +
>> ret = platform_cpu_disable(cpu);
>> if (ret)
>> return ret;
>> @@ -272,6 +275,13 @@ void __ref cpu_die(void)
>> {
>> unsigned int cpu = smp_processor_id();
>>
>> + /*
>> + * We dont need the CPU device anymore.
>> + * Lets do this before IRQs are disabled to allow
>> + * runtime PM to suspend the domain as well.
>> + */
>> + pm_runtime_put_sync(get_cpu_device(cpu));
>
>The two put calls is confusing. __cpu_disable() is called on the
>CPU that's dying, and cpu_die() is called on the CPU that's doing
>the takedown.
>
Is that right? Looking at the code and the comments, I can only imagine
that they must be called on the CPU going down. If thats not the case,
then I need to fix this.
>That would be two decrements but only one increment
>in secondary_start_kernel()? How is this properly balanced?
>
I dont see __cpu_disable() ending up at cpu_die(). These seem two
different exit points. I will check again.
>> +
>> idle_task_exit();
>>
>> local_irq_disable();
>> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
>> local_irq_enable();
>> local_fiq_enable();
>>
>> + /* We are running, enable runtime PM for the CPU. */
>> + cpu_dev = get_cpu_device(cpu);
>> + if (cpu_dev)
>> + pm_runtime_get_sync(cpu_dev);
>
>Also, where would the dev->power.irq_safe flag be set if we
>aren't using the genpd DT stuff? It looks like we're going to
>start causing warnings on devices that don't have the DT magic.
>
Not necessarily. I have added _get and _put at points, when the
interrupts are still enabled. So there should not be a need for the CPU
devices to be IRQ safe. They will operate as regular devices. If they
are attached to a non-IRQ safe domain, they would effect power savings
on the domain.
>Please add a hotplug test with some device that isn't using this
>genpd code to catch problems. Also please turn on lockdep and RCU
>lockdep, touching the idle code like this
>
Good idea. Will add and test.
Thanks Stephen for the review.
Thanks,
Lina
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 9/9] ARM: smp: Add runtime PM support for CPU hotplug
2015-08-13 16:00 ` Lina Iyer
@ 2015-08-13 19:18 ` Stephen Boyd
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Boyd @ 2015-08-13 19:18 UTC (permalink / raw)
To: linux-arm-kernel
On 08/13, Lina Iyer wrote:
> On Wed, Aug 12 2015 at 17:47 -0600, Stephen Boyd wrote:
> >On 08/04, Lina Iyer wrote:
>
> >>+
> >> ret = platform_cpu_disable(cpu);
> >> if (ret)
> >> return ret;
> >>@@ -272,6 +275,13 @@ void __ref cpu_die(void)
> >> {
> >> unsigned int cpu = smp_processor_id();
> >>
> >>+ /*
> >>+ * We dont need the CPU device anymore.
> >>+ * Lets do this before IRQs are disabled to allow
> >>+ * runtime PM to suspend the domain as well.
> >>+ */
> >>+ pm_runtime_put_sync(get_cpu_device(cpu));
> >
> >The two put calls is confusing. __cpu_disable() is called on the
> >CPU that's dying, and cpu_die() is called on the CPU that's doing
> >the takedown.
> >
> Is that right? Looking at the code and the comments, I can only imagine
> that they must be called on the CPU going down. If thats not the case,
> then I need to fix this.
>
> >That would be two decrements but only one increment
> >in secondary_start_kernel()? How is this properly balanced?
> >
> I dont see __cpu_disable() ending up at cpu_die(). These seem two
> different exit points. I will check again.
Yeah I suspect we want a single call in __cpu_disable() so that
it runs on the CPU that's being hotplugged out.
>
> >>+
> >> idle_task_exit();
> >>
> >> local_irq_disable();
> >>@@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
> >> local_irq_enable();
> >> local_fiq_enable();
> >>
> >>+ /* We are running, enable runtime PM for the CPU. */
> >>+ cpu_dev = get_cpu_device(cpu);
> >>+ if (cpu_dev)
> >>+ pm_runtime_get_sync(cpu_dev);
> >
> >Also, where would the dev->power.irq_safe flag be set if we
> >aren't using the genpd DT stuff? It looks like we're going to
> >start causing warnings on devices that don't have the DT magic.
> >
> Not necessarily. I have added _get and _put at points, when the
> interrupts are still enabled. So there should not be a need for the CPU
> devices to be IRQ safe. They will operate as regular devices. If they
> are attached to a non-IRQ safe domain, they would effect power savings
> on the domain.
What about preemption? Preemption is disabled in __cpu_disable()
and secondary_start_kernel() where this patch is calling
pm_runtime_{get,put}_sync(). That should still trigger a warning
with the might_sleep() inside the runtime PM functions if we
haven't set the irq_safe flag.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 60+ messages in thread