diff mbox series

mmc: sdhci-esdhc-imx: make sure ipg clock enabled during suspend/resume

Message ID 1616403599-29650-1-git-send-email-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-esdhc-imx: make sure ipg clock enabled during suspend/resume | expand

Commit Message

Bough Chen March 22, 2021, 8:59 a.m. UTC
From: Haibo Chen <haibo.chen@nxp.com>

During suspend/resume, need to access usdhc register, so need to enable
IPG clock to avoid bus error.

Find this issue when both enable CONFIG_PM and CONFIG_PM_SLEEP, which
means both support PM and Runtime PM. If the card slot do not insert
a card, then after system boot up, will do sdhci_esdhc_runtime_suspend(),
disable all clocks, include the ipg clock. In this case, when suspend the
system, due to no card present, sdhci_esdhc_runtime_resume() will not be
called before sdhci_esdhc_suspend(), so will meet system hung or bus err
once access usdhc register.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

Comments

Ulf Hansson March 24, 2021, 9:40 a.m. UTC | #1
On Mon, 22 Mar 2021 at 10:13, <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> During suspend/resume, need to access usdhc register, so need to enable
> IPG clock to avoid bus error.
>
> Find this issue when both enable CONFIG_PM and CONFIG_PM_SLEEP, which
> means both support PM and Runtime PM. If the card slot do not insert
> a card, then after system boot up, will do sdhci_esdhc_runtime_suspend(),
> disable all clocks, include the ipg clock. In this case, when suspend the
> system, due to no card present, sdhci_esdhc_runtime_resume() will not be
> called before sdhci_esdhc_suspend(), so will meet system hung or bus err
> once access usdhc register.
>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 94327988da91..a48b977ca13f 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1695,10 +1695,14 @@ static int sdhci_esdhc_suspend(struct device *dev)
>         struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>         int ret;
>
> +       ret = clk_prepare_enable(imx_data->clk_ipg);
> +       if (ret)
> +               return ret;
> +

This isn't sufficient.

If the device is attached to a PM domain, that needs to be powered on
too. In other words, you need to call pm_runtime_get_sync() instead of
clk_prepare_enable().

That said, it's kind of surprising to me, that the clocks may remain
ungated during system suspend, but are gated in runtime suspend.
Perhaps what you really should look into, is to re-organize the code
and then can call pm_runtime_force_suspend() from
sdhci_esdhc_suspend() (and pm_runtime_force_resume() from
sdhci_esdhc_suspend()). I think that will be both easier and likely
helps to avoid wasting energy as well.

>         if (host->mmc->caps2 & MMC_CAP2_CQE) {
>                 ret = cqhci_suspend(host->mmc);
>                 if (ret)
> -                       return ret;
> +                       goto disable_ipg_clk;
>         }
>
>         if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
> @@ -1712,38 +1716,52 @@ static int sdhci_esdhc_suspend(struct device *dev)
>
>         ret = sdhci_suspend_host(host);
>         if (ret)
> -               return ret;
> +               goto disable_ipg_clk;
>
>         ret = pinctrl_pm_select_sleep_state(dev);
>         if (ret)
> -               return ret;
> +               goto disable_ipg_clk;
>
>         ret = mmc_gpio_set_cd_wake(host->mmc, true);
>
> +disable_ipg_clk:
> +       clk_disable_unprepare(imx_data->clk_ipg);
> +
>         return ret;
>  }

[...]

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 94327988da91..a48b977ca13f 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1695,10 +1695,14 @@  static int sdhci_esdhc_suspend(struct device *dev)
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
+	ret = clk_prepare_enable(imx_data->clk_ipg);
+	if (ret)
+		return ret;
+
 	if (host->mmc->caps2 & MMC_CAP2_CQE) {
 		ret = cqhci_suspend(host->mmc);
 		if (ret)
-			return ret;
+			goto disable_ipg_clk;
 	}
 
 	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
@@ -1712,38 +1716,52 @@  static int sdhci_esdhc_suspend(struct device *dev)
 
 	ret = sdhci_suspend_host(host);
 	if (ret)
-		return ret;
+		goto disable_ipg_clk;
 
 	ret = pinctrl_pm_select_sleep_state(dev);
 	if (ret)
-		return ret;
+		goto disable_ipg_clk;
 
 	ret = mmc_gpio_set_cd_wake(host->mmc, true);
 
+disable_ipg_clk:
+	clk_disable_unprepare(imx_data->clk_ipg);
+
 	return ret;
 }
 
 static int sdhci_esdhc_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
 	ret = pinctrl_pm_select_default_state(dev);
 	if (ret)
 		return ret;
 
+	ret = clk_prepare_enable(imx_data->clk_ipg);
+	if (ret)
+		return ret;
+
 	/* re-initialize hw state in case it's lost in low power mode */
 	sdhci_esdhc_imx_hwinit(host);
 
 	ret = sdhci_resume_host(host);
 	if (ret)
-		return ret;
+		goto disable_ipg_clk;
 
 	if (host->mmc->caps2 & MMC_CAP2_CQE)
 		ret = cqhci_resume(host->mmc);
 
-	if (!ret)
-		ret = mmc_gpio_set_cd_wake(host->mmc, false);
+	if (ret)
+		goto disable_ipg_clk;
+
+	ret = mmc_gpio_set_cd_wake(host->mmc, false);
+
+disable_ipg_clk:
+	clk_disable_unprepare(imx_data->clk_ipg);
 
 	return ret;
 }