diff mbox series

[v2,3/3] PM: domains: Power off[on] domain in hibernate .freeze[thaw]_noirq hook

Message ID 20221101024736.1509207-4-shawn.guo@linaro.org
State New
Headers
Series Manage domain power state for hibernate freeze |

Commit Message

Shawn Guo Nov. 1, 2022, 2:47 a.m. UTC 
  On platforms which use SHUTDOWN as hibernation mode, the genpd noirq
hooks will be called like below.

    genpd_freeze_noirq()         genpd_restore_noirq()
          ↓                            ↑
    Create snapshot image        Restore system
          ↓                            ↑
    genpd_thaw_noirq()           Read snapshot image
          ↓                            ↑
    Write snapshot image         Kernel boot
          ↓                            ↑
    power_down()                 Power on device

As of today suspend hooks genpd_suspend[resume]_noirq() manages domain
on/off state, but hibernate hooks genpd_freeze[thaw]_noirq() doesn't.
This results in a different behavior of domain power state between suspend
and hibernate freeze, i.e. domain is powered off for the former while on
for the later.  It causes a problem on platforms like i.MX where the
domain needs to be powered on/off by calling clock and regulator interface.
When the platform restores from hibernation, the domain is off in hardware
and genpd_restore_noirq() tries to power it on, but will never succeed
because software state of domain (clock and regulator) is left on from the
last hibernate freeze, so kernel thinks that clock and regulator are
enabled while they are actually not turned on in hardware.  The
consequence would be that devices in the power domain will access
registers without clock or power, and cause hardware lockup.

Power off[on] domain in hibernate .freeze[thaw]_noirq hook for reasons:

- Align the behavior between suspend and hibernate freeze.
- Have power state of domains stay in sync between hardware and software
  for hibernate freeze, and thus fix the lockup issue seen on i.MX
  platform.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/base/power/domain.c | 35 ++++-------------------------------
 1 file changed, 4 insertions(+), 31 deletions(-)

Comments

Ulf Hansson Nov. 1, 2022, 10:24 a.m. UTC | #1 
On Tue, 1 Nov 2022 at 03:47, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On platforms which use SHUTDOWN as hibernation mode, the genpd noirq
> hooks will be called like below.
>
>     genpd_freeze_noirq()         genpd_restore_noirq()
>           ↓                            ↑
>     Create snapshot image        Restore system
>           ↓                            ↑
>     genpd_thaw_noirq()           Read snapshot image
>           ↓                            ↑
>     Write snapshot image         Kernel boot
>           ↓                            ↑
>     power_down()                 Power on device
>
> As of today suspend hooks genpd_suspend[resume]_noirq() manages domain
> on/off state, but hibernate hooks genpd_freeze[thaw]_noirq() doesn't.
> This results in a different behavior of domain power state between suspend
> and hibernate freeze, i.e. domain is powered off for the former while on
> for the later.  It causes a problem on platforms like i.MX where the
> domain needs to be powered on/off by calling clock and regulator interface.
> When the platform restores from hibernation, the domain is off in hardware
> and genpd_restore_noirq() tries to power it on, but will never succeed
> because software state of domain (clock and regulator) is left on from the
> last hibernate freeze, so kernel thinks that clock and regulator are
> enabled while they are actually not turned on in hardware.  The
> consequence would be that devices in the power domain will access
> registers without clock or power, and cause hardware lockup.
>
> Power off[on] domain in hibernate .freeze[thaw]_noirq hook for reasons:
>
> - Align the behavior between suspend and hibernate freeze.
> - Have power state of domains stay in sync between hardware and software
>   for hibernate freeze, and thus fix the lockup issue seen on i.MX
>   platform.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 35 ++++-------------------------------
>  1 file changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b81baeb38d81..28c9e04e3488 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1323,24 +1323,11 @@ static int genpd_resume_noirq(struct device *dev)
>   */
>  static int genpd_freeze_noirq(struct device *dev)
>  {
> -       const struct generic_pm_domain *genpd;
> -       int ret = 0;
> -
>         dev_dbg(dev, "%s()\n", __func__);
>
> -       genpd = dev_to_genpd(dev);
> -       if (IS_ERR(genpd))
> -               return -EINVAL;
> -
> -       ret = pm_generic_freeze_noirq(dev);
> -       if (ret)
> -               return ret;
> -
> -       if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> -           !pm_runtime_status_suspended(dev))
> -               ret = genpd_stop_dev(genpd, dev);
> -
> -       return ret;
> +       return genpd_finish_suspend(dev,
> +                                   pm_generic_freeze_noirq,
> +                                   pm_generic_thaw_noirq);
>  }
>
>  /**
> @@ -1352,23 +1339,9 @@ static int genpd_freeze_noirq(struct device *dev)
>   */
>  static int genpd_thaw_noirq(struct device *dev)
>  {
> -       const struct generic_pm_domain *genpd;
> -       int ret = 0;
> -
>         dev_dbg(dev, "%s()\n", __func__);
>
> -       genpd = dev_to_genpd(dev);
> -       if (IS_ERR(genpd))
> -               return -EINVAL;
> -
> -       if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> -           !pm_runtime_status_suspended(dev)) {
> -               ret = genpd_start_dev(genpd, dev);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       return pm_generic_thaw_noirq(dev);
> +       return genpd_finish_resume(dev, pm_generic_thaw_noirq);
>  }
>
>  /**
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b81baeb38d81..28c9e04e3488 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1323,24 +1323,11 @@  static int genpd_resume_noirq(struct device *dev)
  */
 static int genpd_freeze_noirq(struct device *dev)
 {
-	const struct generic_pm_domain *genpd;
-	int ret = 0;
-
 	dev_dbg(dev, "%s()\n", __func__);
 
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	ret = pm_generic_freeze_noirq(dev);
-	if (ret)
-		return ret;
-
-	if (genpd->dev_ops.stop && genpd->dev_ops.start &&
-	    !pm_runtime_status_suspended(dev))
-		ret = genpd_stop_dev(genpd, dev);
-
-	return ret;
+	return genpd_finish_suspend(dev,
+				    pm_generic_freeze_noirq,
+				    pm_generic_thaw_noirq);
 }
 
 /**
@@ -1352,23 +1339,9 @@  static int genpd_freeze_noirq(struct device *dev)
  */
 static int genpd_thaw_noirq(struct device *dev)
 {
-	const struct generic_pm_domain *genpd;
-	int ret = 0;
-
 	dev_dbg(dev, "%s()\n", __func__);
 
-	genpd = dev_to_genpd(dev);
-	if (IS_ERR(genpd))
-		return -EINVAL;
-
-	if (genpd->dev_ops.stop && genpd->dev_ops.start &&
-	    !pm_runtime_status_suspended(dev)) {
-		ret = genpd_start_dev(genpd, dev);
-		if (ret)
-			return ret;
-	}
-
-	return pm_generic_thaw_noirq(dev);
+	return genpd_finish_resume(dev, pm_generic_thaw_noirq);
 }
 
 /**