Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1563239 > unrolled thread

[PATCH v2] PM / Domains: Keep the pd status during system PM phases

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] PM / Domains: Keep the pd status during system PM phases Elaine Zhang <zhangqing@rock-chips.com> - 2017-01-20 03:30 +0100
    Re: [PATCH v2] PM / Domains: Keep the pd status during system PM  phases Greg KH <gregkh@linuxfoundation.org> - 2017-01-20 09:50 +0100
    Re: [PATCH v2] PM / Domains: Keep the pd status during system PM phases Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-20 14:20 +0100
      Re: [PATCH v2] PM / Domains: Keep the pd status during system PM  phases Elaine Zhang <zhangqing@rock-chips.com> - 2017-01-22 04:40 +0100
        Re: [PATCH v2] PM / Domains: Keep the pd status during system PM phases Ulf Hansson <ulf.hansson@linaro.org> - 2017-01-25 22:40 +0100

#1563239 — [PATCH v2] PM / Domains: Keep the pd status during system PM phases

If a PM domain is powered off before system suspend,
we hope do nothing in system runtime suspend noirq phase
and system runtime resume noirq phase.

This modify is to slove system resume issue for RK3399.
RK3399 SOC pd_gpu have voltage domain vdd_gpu,
so we must follow open vdd_gpu and power on pd_gpu,
power off pd_gpu and disable vdd_gpu.
Fix up in runtime resume noirq phase power on all PDs.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/base/power/domain.c | 10 +++++++---
 include/linux/pm_domain.h   |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 5711708532db..81351eec570f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -842,8 +842,10 @@ static int pm_genpd_prepare(struct device *dev)
 
 	genpd_lock(genpd);
 
-	if (genpd->prepared_count++ == 0)
+	if (genpd->prepared_count++ == 0) {
 		genpd->suspended_count = 0;
+		genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
+	}
 
 	genpd_unlock(genpd);
 
@@ -877,7 +879,8 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
+	if (genpd->suspend_power_off ||
+	    (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
 		return 0;
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start) {
@@ -914,7 +917,8 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
+	if (genpd->suspend_power_off ||
+	    (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
 		return 0;
 
 	/*
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 81ece61075df..9c0dc364f089 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -62,6 +62,7 @@ struct generic_pm_domain {
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
 	unsigned int prepared_count;	/* Suspend counter of prepared devices */
+	bool suspend_power_off;	/* Power status before system suspend */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
 	struct gpd_dev_ops dev_ops;
-- 
1.9.1

[toc] | [next] | [standalone]

#1563387 — Re: [PATCH v2] PM / Domains: Keep the pd status during system PM phases

On Fri, Jan 20, 2017 at 10:21:33AM +0800, Elaine Zhang wrote:
> If a PM domain is powered off before system suspend,
> we hope do nothing in system runtime suspend noirq phase
> and system runtime resume noirq phase.
> 
> This modify is to slove system resume issue for RK3399.
> RK3399 SOC pd_gpu have voltage domain vdd_gpu,
> so we must follow open vdd_gpu and power on pd_gpu,
> power off pd_gpu and disable vdd_gpu.
> Fix up in runtime resume noirq phase power on all PDs.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  drivers/base/power/domain.c | 10 +++++++---
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)

What changed from v1?  You always have to list that below the --- line.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1563561

On 20 January 2017 at 03:21, Elaine Zhang <zhangqing@rock-chips.com> wrote:
> If a PM domain is powered off before system suspend,
> we hope do nothing in system runtime suspend noirq phase
> and system runtime resume noirq phase.

One can hope, but that isn't good enough. :-)


>
> This modify is to slove system resume issue for RK3399.
> RK3399 SOC pd_gpu have voltage domain vdd_gpu,
> so we must follow open vdd_gpu and power on pd_gpu,
> power off pd_gpu and disable vdd_gpu.
> Fix up in runtime resume noirq phase power on all PDs.

This doesn't make any sense to me. Can please try to explain this is
in great more detail, then I can try to help.

>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  drivers/base/power/domain.c | 10 +++++++---
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 5711708532db..81351eec570f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -842,8 +842,10 @@ static int pm_genpd_prepare(struct device *dev)
>
>         genpd_lock(genpd);
>
> -       if (genpd->prepared_count++ == 0)
> +       if (genpd->prepared_count++ == 0) {
>                 genpd->suspended_count = 0;
> +               genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
> +       }
>
>         genpd_unlock(genpd);
>
> @@ -877,7 +879,8 @@ static int pm_genpd_suspend_noirq(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> +       if (genpd->suspend_power_off ||
> +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
>                 return 0;

This isn't going to work, because the PM domain might have been powered on.

There is simply no guarantee to that the PM domain remains powered off
during the system PM suspend phases. For example, any device in the PM
domain that get runtime resumed after the ->prepare() callback is
invoked this, will of course also trigger the PM domain to be powered
on.

>
>         if (genpd->dev_ops.stop && genpd->dev_ops.start) {
> @@ -914,7 +917,8 @@ static int pm_genpd_resume_noirq(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> +       if (genpd->suspend_power_off ||
> +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
>                 return 0;

Ditto, for the same reasons as above.

>
>         /*
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 81ece61075df..9c0dc364f089 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -62,6 +62,7 @@ struct generic_pm_domain {
>         unsigned int device_count;      /* Number of devices */
>         unsigned int suspended_count;   /* System suspend device counter */
>         unsigned int prepared_count;    /* Suspend counter of prepared devices */
> +       bool suspend_power_off; /* Power status before system suspend */
>         int (*power_off)(struct generic_pm_domain *domain);
>         int (*power_on)(struct generic_pm_domain *domain);
>         struct gpd_dev_ops dev_ops;
> --
> 1.9.1
>
>

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1564296 — Re: [PATCH v2] PM / Domains: Keep the pd status during system PM phases

On 01/20/2017 09:16 PM, Ulf Hansson wrote:
> On 20 January 2017 at 03:21, Elaine Zhang <zhangqing@rock-chips.com> wrote:
>> If a PM domain is powered off before system suspend,
>> we hope do nothing in system runtime suspend noirq phase
>> and system runtime resume noirq phase.
>
> One can hope, but that isn't good enough. :-)
>
>
>>
>> This modify is to slove system resume issue for RK3399.
>> RK3399 SOC pd_gpu have voltage domain vdd_gpu,
>> so we must follow open vdd_gpu and power on pd_gpu,
>> power off pd_gpu and disable vdd_gpu.
>> Fix up in runtime resume noirq phase power on all PDs.
>
> This doesn't make any sense to me. Can please try to explain this is
> in great more detail, then I can try to help.
>
For example:
-->device suspend
    (mali gpu driver set pd_gpu off by pm_runtime_put_sync(),
     and then disabled the vdd_gpu by regulator_disable().)
	--> system suspend
		-->prepare
			-->suspend_noirq():
			(power off all pds)
	-->system resume
		-->resume_noirq():
		(power up all pds)
		(in this case the vdd_gpu is still disabled,
		 if power on the pd_gpu maybe make the system crash)
	-->complete : power off the not used pd
-->device resuem
    (mali gpu driver enable vdd_gpu by regulator_enable(),
     and then power up the pd_gpu by pm_runtime_get_sync())

So for RK3399 soc, if to set pd_gpu the vdd_gpu must be enabled, or else 
will can't get the ack back.
I hope the pd_gpu power up/off by the driver itself.

May be I solution is not the optimal solution,Do you have better suggestion?

>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> ---
>>   drivers/base/power/domain.c | 10 +++++++---
>>   include/linux/pm_domain.h   |  1 +
>>   2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 5711708532db..81351eec570f 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -842,8 +842,10 @@ static int pm_genpd_prepare(struct device *dev)
>>
>>          genpd_lock(genpd);
>>
>> -       if (genpd->prepared_count++ == 0)
>> +       if (genpd->prepared_count++ == 0) {
>>                  genpd->suspended_count = 0;
>> +               genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
>> +       }
>>
>>          genpd_unlock(genpd);
>>
>> @@ -877,7 +879,8 @@ static int pm_genpd_suspend_noirq(struct device *dev)
>>          if (IS_ERR(genpd))
>>                  return -EINVAL;
>>
>> -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
>> +       if (genpd->suspend_power_off ||
>> +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
>>                  return 0;
>
> This isn't going to work, because the PM domain might have been powered on.
>
> There is simply no guarantee to that the PM domain remains powered off
> during the system PM suspend phases. For example, any device in the PM
> domain that get runtime resumed after the ->prepare() callback is
> invoked this, will of course also trigger the PM domain to be powered
> on.
>
>>
>>          if (genpd->dev_ops.stop && genpd->dev_ops.start) {
>> @@ -914,7 +917,8 @@ static int pm_genpd_resume_noirq(struct device *dev)
>>          if (IS_ERR(genpd))
>>                  return -EINVAL;
>>
>> -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
>> +       if (genpd->suspend_power_off ||
>> +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
>>                  return 0;
>
> Ditto, for the same reasons as above.
>
>>
>>          /*
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 81ece61075df..9c0dc364f089 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -62,6 +62,7 @@ struct generic_pm_domain {
>>          unsigned int device_count;      /* Number of devices */
>>          unsigned int suspended_count;   /* System suspend device counter */
>>          unsigned int prepared_count;    /* Suspend counter of prepared devices */
>> +       bool suspend_power_off; /* Power status before system suspend */
>>          int (*power_off)(struct generic_pm_domain *domain);
>>          int (*power_on)(struct generic_pm_domain *domain);
>>          struct gpd_dev_ops dev_ops;
>> --
>> 1.9.1
>>
>>
>
> Kind regards
> Uffe
>
>
>

[toc] | [prev] | [next] | [standalone]

#1566976

On 22 January 2017 at 04:34, Elaine Zhang <zhangqing@rock-chips.com> wrote:
>
>
> On 01/20/2017 09:16 PM, Ulf Hansson wrote:
>>
>> On 20 January 2017 at 03:21, Elaine Zhang <zhangqing@rock-chips.com>
>> wrote:
>>>
>>> If a PM domain is powered off before system suspend,
>>> we hope do nothing in system runtime suspend noirq phase
>>> and system runtime resume noirq phase.
>>
>>
>> One can hope, but that isn't good enough. :-)
>>
>>
>>>
>>> This modify is to slove system resume issue for RK3399.
>>> RK3399 SOC pd_gpu have voltage domain vdd_gpu,
>>> so we must follow open vdd_gpu and power on pd_gpu,
>>> power off pd_gpu and disable vdd_gpu.
>>> Fix up in runtime resume noirq phase power on all PDs.
>>
>>
>> This doesn't make any sense to me. Can please try to explain this is
>> in great more detail, then I can try to help.
>>
> For example:
> -->device suspend
>    (mali gpu driver set pd_gpu off by pm_runtime_put_sync(),

This is the wrong approach, as runtime suspend is prevented by the PM
core in this phase. More precisely, it does a
pm_runtime_get_noresume() in the device prepare phase.

I think it seems like you would benefit from using the so called the
runtime PM centric approach, which gives you system PM support for
"free". Please have a look at the pm_runtime_force_suspend|resume()
helpers.

>     and then disabled the vdd_gpu by regulator_disable().)
>         --> system suspend
>                 -->prepare
>                         -->suspend_noirq():
>                         (power off all pds)
>         -->system resume
>                 -->resume_noirq():
>                 (power up all pds)
>                 (in this case the vdd_gpu is still disabled,
>                  if power on the pd_gpu maybe make the system crash)
>         -->complete : power off the not used pd
> -->device resuem
>    (mali gpu driver enable vdd_gpu by regulator_enable(),
>     and then power up the pd_gpu by pm_runtime_get_sync())

Seems like there is also a missing configuration of the relationship
between the PM domains. In the genpd terminology, you probably want to
set pd_gpu as a subdomain of the vdd_gpu.

In that way, the vdd_gpu is always powered on before pd_gpu is powered
on. And vice verse when powering off.

>
> So for RK3399 soc, if to set pd_gpu the vdd_gpu must be enabled, or else
> will can't get the ack back.
> I hope the pd_gpu power up/off by the driver itself.
>
> May be I solution is not the optimal solution,Do you have better suggestion?
>

Please see my comment above.

[...]

Kind regards
Uffe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web