| Message ID | 20230518082035.335112-1-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mmc: sdhci-esdhc-imx: enable IPG clock before register access during suspend/resume | expand |
On 18/05/23 11:20, 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 a card slot do not insert a card, so when > system suspend, 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> > --- > The first thing I do is try to merge the sdhci_esdhc_suspend() into > current sdhci_esdhc_runtime_suspend(), but find some obstacles: > 1, sdhci_esdhc_imx_hwinit() is called in original sdhci_esdhc_resume(), > it will clear the DLL config, which after remove to sdhci_esdhc_runtime_resume, > will finally impact the DDR50 timing. > 2, if merge, everytime after sdhci_esdhc_runtime_resume, will do re-tuning, > this is unnecessary for the normal runtime PM case without power lost. > 3, the CD gpio wakeup. Seems strange to enable the CD gpio wakeup in > sdhci_esdhc_runtime_suspend(). > > So, finally I drop this merge method, and directly add change in the original > sdhci_esdhc_suspend()/sdhci_esdhc_resume(). > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index eebf94604a7f..4cf42a028bb9 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1836,6 +1836,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > int ret; > > + pm_runtime_get_sync(dev); > + > if (host->mmc->caps2 & MMC_CAP2_CQE) { > ret = cqhci_suspend(host->mmc); > if (ret) > @@ -1855,6 +1857,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > if (ret) > return ret; > > + pm_runtime_force_suspend(dev); Won't this result both in sdhci_suspend_host() and sdhci_runtime_suspend_host() being called. Please ensure that just call one of them is called. And cqhci_suspend() will be called twice. Please avoid that. > + > ret = pinctrl_pm_select_sleep_state(dev); > if (ret) > return ret; > @@ -1873,6 +1877,8 @@ static int sdhci_esdhc_resume(struct device *dev) > if (ret) > return ret; > > + pm_runtime_force_resume(dev); > + > /* re-initialize hw state in case it's lost in low power mode */ > sdhci_esdhc_imx_hwinit(host); > > @@ -1886,6 +1892,8 @@ static int sdhci_esdhc_resume(struct device *dev) > if (!ret) > ret = mmc_gpio_set_cd_wake(host->mmc, false); > > + pm_runtime_put_autosuspend(dev); > + > return ret; > } > #endif
On Thu, 18 May 2023 at 10:17, <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 a card slot do not insert a card, so when > system suspend, 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> > --- > The first thing I do is try to merge the sdhci_esdhc_suspend() into > current sdhci_esdhc_runtime_suspend(), but find some obstacles: > 1, sdhci_esdhc_imx_hwinit() is called in original sdhci_esdhc_resume(), > it will clear the DLL config, which after remove to sdhci_esdhc_runtime_resume, > will finally impact the DDR50 timing. > 2, if merge, everytime after sdhci_esdhc_runtime_resume, will do re-tuning, > this is unnecessary for the normal runtime PM case without power lost. > 3, the CD gpio wakeup. Seems strange to enable the CD gpio wakeup in > sdhci_esdhc_runtime_suspend(). > > So, finally I drop this merge method, and directly add change in the original > sdhci_esdhc_suspend()/sdhci_esdhc_resume(). Looks like you need some differentiation between runtime suspend/resume and system suspend/resume. One way to deal with this is to set some driver flags during system suspend and then monitor those flags at runtime resume - and then act accordingly. There are a couple of sdhci drivers that do this, like sdhci-xenon.c and sdhci-of-at91.c. Those drivers don't seems to need the sdhci_suspend|resume_host(). Maybe that pattern can help to fix this too? Kind regards Uffe > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index eebf94604a7f..4cf42a028bb9 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1836,6 +1836,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > int ret; > > + pm_runtime_get_sync(dev); > + > if (host->mmc->caps2 & MMC_CAP2_CQE) { > ret = cqhci_suspend(host->mmc); > if (ret) > @@ -1855,6 +1857,8 @@ static int sdhci_esdhc_suspend(struct device *dev) > if (ret) > return ret; > > + pm_runtime_force_suspend(dev); > + > ret = pinctrl_pm_select_sleep_state(dev); > if (ret) > return ret; > @@ -1873,6 +1877,8 @@ static int sdhci_esdhc_resume(struct device *dev) > if (ret) > return ret; > > + pm_runtime_force_resume(dev); > + > /* re-initialize hw state in case it's lost in low power mode */ > sdhci_esdhc_imx_hwinit(host); > > @@ -1886,6 +1892,8 @@ static int sdhci_esdhc_resume(struct device *dev) > if (!ret) > ret = mmc_gpio_set_cd_wake(host->mmc, false); > > + pm_runtime_put_autosuspend(dev); > + > return ret; > } > #endif > -- > 2.34.1 >
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index eebf94604a7f..4cf42a028bb9 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -1836,6 +1836,8 @@ static int sdhci_esdhc_suspend(struct device *dev) struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); int ret; + pm_runtime_get_sync(dev); + if (host->mmc->caps2 & MMC_CAP2_CQE) { ret = cqhci_suspend(host->mmc); if (ret) @@ -1855,6 +1857,8 @@ static int sdhci_esdhc_suspend(struct device *dev) if (ret) return ret; + pm_runtime_force_suspend(dev); + ret = pinctrl_pm_select_sleep_state(dev); if (ret) return ret; @@ -1873,6 +1877,8 @@ static int sdhci_esdhc_resume(struct device *dev) if (ret) return ret; + pm_runtime_force_resume(dev); + /* re-initialize hw state in case it's lost in low power mode */ sdhci_esdhc_imx_hwinit(host); @@ -1886,6 +1892,8 @@ static int sdhci_esdhc_resume(struct device *dev) if (!ret) ret = mmc_gpio_set_cd_wake(host->mmc, false); + pm_runtime_put_autosuspend(dev); + return ret; } #endif