[v2,2/4] ARM: exynos: Ensure PM domains are powered at initialization

Message ID 1412174494-15346-3-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Oct. 1, 2014, 2:41 p.m.
At ->probe() it's common practice for drivers/subsystems to bring their
devices to full power and without depending on CONFIG_PM_RUNTIME.

We could also expect that drivers/subsystems requires their device's
corresponding PM domains to be powered, to successfully complete a
->probe() sequence.

Align to the behavior above, by ensuring all PM domains are powered
prior initialization of a generic PM domain.

Do note, since the generic PM domain will try to power off unused PM
domains at late_init, there should be no increased power consumption
over time, but potentially during boot.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 arch/arm/mach-exynos/pm_domains.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Sylwester Nawrocki Oct. 1, 2014, 4:18 p.m. | #1
On 01/10/14 16:41, Ulf Hansson wrote:
> At ->probe() it's common practice for drivers/subsystems to bring their
> devices to full power and without depending on CONFIG_PM_RUNTIME.
> 
> We could also expect that drivers/subsystems requires their device's
> corresponding PM domains to be powered, to successfully complete a
> ->probe() sequence.
> 
> Align to the behavior above, by ensuring all PM domains are powered
> prior initialization of a generic PM domain.
> 
> Do note, since the generic PM domain will try to power off unused PM
> domains at late_init, there should be no increased power consumption
> over time, but potentially during boot.

Wouldn't it be a better idea to power on the power domains which are
turned off only when CONFIG_PM_RUNTIME is not enabled ? I had a plan
to submit a patch doing that but unfortunately this has fallen through
the cracks. At the moment mach-exynos/pm_domains.c is not even built in
when CONFIG_PM_RUNTIME is disabled.

I don't like the behaviour introduced in this patch to be the default,
i.e. turning all possible power domains during boot sequence, even if
some are not used and not needed. While we're trying to decrease the
power consumption in any possible way this doesn't help at all.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  arch/arm/mach-exynos/pm_domains.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
> index 20f2671..58e18e9 100644
> --- a/arch/arm/mach-exynos/pm_domains.c
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -112,7 +112,7 @@ static __init int exynos4_pm_init_power_domain(void)
>  
>  	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
>  		struct exynos_pm_domain *pd;
> -		int on, i;
> +		int i;
>  		struct device *dev;
>  
>  		pdev = of_find_device_by_node(np);
> @@ -155,9 +155,10 @@ static __init int exynos4_pm_init_power_domain(void)
>  			clk_put(pd->oscclk);
>  
>  no_clk:
> -		on = __raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN;
> +		if (!(__raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN))
> +			exynos_pd_power_on(&pd->pd);
>  
> -		pm_genpd_init(&pd->pd, NULL, !on);
> +		pm_genpd_init(&pd->pd, NULL, false);
>  		of_genpd_add_provider_simple(np, &pd->pd);
>  	}

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Oct. 1, 2014, 7:50 p.m. | #2
On Wednesday, October 01, 2014 06:18:58 PM Sylwester Nawrocki wrote:
> On 01/10/14 16:41, Ulf Hansson wrote:
> > At ->probe() it's common practice for drivers/subsystems to bring their
> > devices to full power and without depending on CONFIG_PM_RUNTIME.
> > 
> > We could also expect that drivers/subsystems requires their device's
> > corresponding PM domains to be powered, to successfully complete a
> > ->probe() sequence.
> > 
> > Align to the behavior above, by ensuring all PM domains are powered
> > prior initialization of a generic PM domain.
> > 
> > Do note, since the generic PM domain will try to power off unused PM
> > domains at late_init, there should be no increased power consumption
> > over time, but potentially during boot.
> 
> Wouldn't it be a better idea to power on the power domains which are
> turned off only when CONFIG_PM_RUNTIME is not enabled ? I had a plan
> to submit a patch doing that but unfortunately this has fallen through
> the cracks. At the moment mach-exynos/pm_domains.c is not even built in
> when CONFIG_PM_RUNTIME is disabled.
> 
> I don't like the behaviour introduced in this patch to be the default,
> i.e. turning all possible power domains during boot sequence, even if
> some are not used and not needed. While we're trying to decrease the
> power consumption in any possible way this doesn't help at all.

Agreed (as stated before).

And I'm wondering why that comment of mine was ignored?
Ulf Hansson Oct. 2, 2014, 9:42 a.m. | #3
On 1 October 2014 21:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, October 01, 2014 06:18:58 PM Sylwester Nawrocki wrote:
>> On 01/10/14 16:41, Ulf Hansson wrote:
>> > At ->probe() it's common practice for drivers/subsystems to bring their
>> > devices to full power and without depending on CONFIG_PM_RUNTIME.
>> >
>> > We could also expect that drivers/subsystems requires their device's
>> > corresponding PM domains to be powered, to successfully complete a
>> > ->probe() sequence.
>> >
>> > Align to the behavior above, by ensuring all PM domains are powered
>> > prior initialization of a generic PM domain.
>> >
>> > Do note, since the generic PM domain will try to power off unused PM
>> > domains at late_init, there should be no increased power consumption
>> > over time, but potentially during boot.
>>
>> Wouldn't it be a better idea to power on the power domains which are
>> turned off only when CONFIG_PM_RUNTIME is not enabled ? I had a plan
>> to submit a patch doing that but unfortunately this has fallen through
>> the cracks. At the moment mach-exynos/pm_domains.c is not even built in
>> when CONFIG_PM_RUNTIME is disabled.

Yes, that's the approach I also intend to take in the next step.

But, it's not that simple. Since this requires a mechanism for drivers
to bring their device's PM domains into power state prior doing probe.
We don't have such today. I do have some ideas about this, but I think
we need to keep that as a separate discussion.

>>
>> I don't like the behaviour introduced in this patch to be the default,
>> i.e. turning all possible power domains during boot sequence, even if
>> some are not used and not needed. While we're trying to decrease the
>> power consumption in any possible way this doesn't help at all.

This will hit only during boot, until late_init. Unless you have a
platform that keeps rebooting all the time, is this really a big
worry?

Still, I certainly agree that we should strive for a solution where
it's possible to leave PM domains powered off at init. It's should be
too hard to support this from genpd point of view, but
drivers/subsystems will need some adaptations.

>
> Agreed (as stated before).
>
> And I'm wondering why that comment of mine was ignored?

Sorry, if missed to comment of that. I guess I have at this point.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 2, 2014, 9:55 a.m. | #4
On 2 October 2014 11:42, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 1 October 2014 21:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, October 01, 2014 06:18:58 PM Sylwester Nawrocki wrote:
>>> On 01/10/14 16:41, Ulf Hansson wrote:
>>> > At ->probe() it's common practice for drivers/subsystems to bring their
>>> > devices to full power and without depending on CONFIG_PM_RUNTIME.
>>> >
>>> > We could also expect that drivers/subsystems requires their device's
>>> > corresponding PM domains to be powered, to successfully complete a
>>> > ->probe() sequence.
>>> >
>>> > Align to the behavior above, by ensuring all PM domains are powered
>>> > prior initialization of a generic PM domain.
>>> >
>>> > Do note, since the generic PM domain will try to power off unused PM
>>> > domains at late_init, there should be no increased power consumption
>>> > over time, but potentially during boot.
>>>
>>> Wouldn't it be a better idea to power on the power domains which are
>>> turned off only when CONFIG_PM_RUNTIME is not enabled ? I had a plan
>>> to submit a patch doing that but unfortunately this has fallen through
>>> the cracks. At the moment mach-exynos/pm_domains.c is not even built in
>>> when CONFIG_PM_RUNTIME is disabled.
>
> Yes, that's the approach I also intend to take in the next step.
>
> But, it's not that simple. Since this requires a mechanism for drivers
> to bring their device's PM domains into power state prior doing probe.
> We don't have such today. I do have some ideas about this, but I think
> we need to keep that as a separate discussion.
>
>>>
>>> I don't like the behaviour introduced in this patch to be the default,
>>> i.e. turning all possible power domains during boot sequence, even if
>>> some are not used and not needed. While we're trying to decrease the
>>> power consumption in any possible way this doesn't help at all.
>
> This will hit only during boot, until late_init. Unless you have a
> platform that keeps rebooting all the time, is this really a big
> worry?
>
> Still, I certainly agree that we should strive for a solution where
> it's possible to leave PM domains powered off at init. It's should be

/s/ It's should/ It shouldn't

> too hard to support this from genpd point of view, but
> drivers/subsystems will need some adaptations.
>
>>
>> Agreed (as stated before).
>>
>> And I'm wondering why that comment of mine was ignored?
>
> Sorry, if missed to comment of that. I guess I have at this point.
>
> Kind regards
> Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 20f2671..58e18e9 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -112,7 +112,7 @@  static __init int exynos4_pm_init_power_domain(void)
 
 	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
 		struct exynos_pm_domain *pd;
-		int on, i;
+		int i;
 		struct device *dev;
 
 		pdev = of_find_device_by_node(np);
@@ -155,9 +155,10 @@  static __init int exynos4_pm_init_power_domain(void)
 			clk_put(pd->oscclk);
 
 no_clk:
-		on = __raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN;
+		if (!(__raw_readl(pd->base + 0x4) & INT_LOCAL_PWR_EN))
+			exynos_pd_power_on(&pd->pd);
 
-		pm_genpd_init(&pd->pd, NULL, !on);
+		pm_genpd_init(&pd->pd, NULL, false);
 		of_genpd_add_provider_simple(np, &pd->pd);
 	}