On 01/20/2017 09:16 PM, Ulf Hansson wrote:
> On 20 January 2017 at 03:21, Elaine Zhang <zhangqing at 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 at 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
>
>
>
