diff mbox series

[1/2] mmc: sdhci-esdhc-imx: enable IPG clock before register access during suspend/resume

Message ID 20230518082035.335112-1-haibo.chen@nxp.com
State New
Headers show
Series [1/2] mmc: sdhci-esdhc-imx: enable IPG clock before register access during suspend/resume | expand

Commit Message

Bough Chen May 18, 2023, 8:20 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 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(+)

Comments

Adrian Hunter May 29, 2023, 10:47 a.m. UTC | #1 
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
diff mbox series

Patch

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