| Message ID | 1616403599-29650-1-git-send-email-haibo.chen@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-esdhc-imx: make sure ipg clock enabled during suspend/resume | expand |
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 --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; }