| Message ID | 20250514094903.1771642-1-ziniu.wang_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] mmc: sdhci: export APIs for sdhci irq wakeup | expand |
On 14/05/2025 12:49, ziniu.wang_1@nxp.com wrote: > From: Luke Wang <ziniu.wang_1@nxp.com> > > Current suspend/resume logic has one issue. In suspend, will config > register when call sdhci_suspend_host(), but at this time, can't > guarantee host in runtime resume state. If not, the per clock is gate > off, access register will hang. > > In sdhci_esdhc_suspend/sdhci_esdhc_resume, remove sdhci_suspend_host() > and sdhci_resume_host(), all are handled in runtime PM callbacks except > the wakeup irq setting. For wakeup irq setting, use pm_runtime_get_sync() > in sdhci_esdhc_suspend() to make sure clock gate on. > > Remove pinctrl_pm_select_default_state() in sdhci_esdhc_resume, because > pm_runtime_force_resume() already config the pinctrl state according to > ios timing, and here config the default pinctrl state again is wrong for > SDIO3.0 device if it keep power in suspend. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> Looks OK to me, at least, although Ulf may still have comments. Otherwise, for both patches: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 50 ++++++++++++++++++------------ > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index c0160c69a027..7611682f10c3 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -2009,11 +2009,14 @@ static int sdhci_esdhc_suspend(struct device *dev) > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > int ret; > > - if (host->mmc->caps2 & MMC_CAP2_CQE) { > - ret = cqhci_suspend(host->mmc); > - if (ret) > - return ret; > - } > + /* > + * Switch to runtime resume for two reasons: > + * 1, there is register access (e.g., wakeup control register), so > + * need to make sure gate on ipg clock. > + * 2, make sure the pm_runtime_force_resume() in sdhci_esdhc_resume() really > + * invoke its ->runtime_resume callback (needs_force_resume = 1). > + */ > + pm_runtime_get_sync(dev); > > if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) && > (host->tuning_mode != SDHCI_TUNING_MODE_1)) { > @@ -2021,9 +2024,6 @@ static int sdhci_esdhc_suspend(struct device *dev) > mmc_retune_needed(host->mmc); > } > > - if (host->tuning_mode != SDHCI_TUNING_MODE_3) > - mmc_retune_needed(host->mmc); > - > /* > * For the device need to keep power during system PM, need > * to save the tuning delay value just in case the usdhc > @@ -2033,9 +2033,13 @@ static int sdhci_esdhc_suspend(struct device *dev) > esdhc_is_usdhc(imx_data)) > sdhc_esdhc_tuning_save(host); > > - ret = sdhci_suspend_host(host); > - if (ret) > - return ret; > + if (device_may_wakeup(dev)) { > + /* The irqs of imx are not shared. It is safe to disable */ > + disable_irq(host->irq); > + ret = sdhci_enable_irq_wakeups(host); > + if (!ret) > + dev_warn(dev, "Failed to enable irq wakeup\n"); > + } > > ret = pinctrl_pm_select_sleep_state(dev); > if (ret) > @@ -2043,6 +2047,12 @@ static int sdhci_esdhc_suspend(struct device *dev) > > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > + /* > + * Make sure invoke runtime_suspend to gate off clock. > + * uSDHC IP supports in-band SDIO wakeup even without clock. > + */ > + pm_runtime_force_suspend(dev); > + > return ret; > } > > @@ -2053,16 +2063,19 @@ static int sdhci_esdhc_resume(struct device *dev) > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > int ret; > > - ret = pinctrl_pm_select_default_state(dev); > + pm_runtime_force_resume(dev); > + > + ret = mmc_gpio_set_cd_wake(host->mmc, false); > 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; > + if (host->irq_wake_enabled) { > + sdhci_disable_irq_wakeups(host); > + enable_irq(host->irq); > + } > > /* > * restore the saved tuning delay value for the device which keep > @@ -2072,11 +2085,8 @@ static int sdhci_esdhc_resume(struct device *dev) > esdhc_is_usdhc(imx_data)) > sdhc_esdhc_tuning_restore(host); > > - if (host->mmc->caps2 & MMC_CAP2_CQE) > - ret = cqhci_resume(host->mmc); > - > - if (!ret) > - ret = mmc_gpio_set_cd_wake(host->mmc, false); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > > return ret; > }
On Thu, 15 May 2025 at 14:53, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 14/05/2025 12:49, ziniu.wang_1@nxp.com wrote: > > From: Luke Wang <ziniu.wang_1@nxp.com> > > > > Current suspend/resume logic has one issue. In suspend, will config > > register when call sdhci_suspend_host(), but at this time, can't > > guarantee host in runtime resume state. If not, the per clock is gate > > off, access register will hang. > > > > In sdhci_esdhc_suspend/sdhci_esdhc_resume, remove sdhci_suspend_host() > > and sdhci_resume_host(), all are handled in runtime PM callbacks except > > the wakeup irq setting. For wakeup irq setting, use pm_runtime_get_sync() > > in sdhci_esdhc_suspend() to make sure clock gate on. > > > > Remove pinctrl_pm_select_default_state() in sdhci_esdhc_resume, because > > pm_runtime_force_resume() already config the pinctrl state according to > > ios timing, and here config the default pinctrl state again is wrong for > > SDIO3.0 device if it keep power in suspend. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > > Looks OK to me, at least, although Ulf may still have comments. > Otherwise, for both patches: Looks good to me too! > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> Both patches applied for next, thanks! Kind regards Uffe > > > --- > > drivers/mmc/host/sdhci-esdhc-imx.c | 50 ++++++++++++++++++------------ > > 1 file changed, 30 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > > index c0160c69a027..7611682f10c3 100644 > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > @@ -2009,11 +2009,14 @@ static int sdhci_esdhc_suspend(struct device *dev) > > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > > int ret; > > > > - if (host->mmc->caps2 & MMC_CAP2_CQE) { > > - ret = cqhci_suspend(host->mmc); > > - if (ret) > > - return ret; > > - } > > + /* > > + * Switch to runtime resume for two reasons: > > + * 1, there is register access (e.g., wakeup control register), so > > + * need to make sure gate on ipg clock. > > + * 2, make sure the pm_runtime_force_resume() in sdhci_esdhc_resume() really > > + * invoke its ->runtime_resume callback (needs_force_resume = 1). > > + */ > > + pm_runtime_get_sync(dev); > > > > if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) && > > (host->tuning_mode != SDHCI_TUNING_MODE_1)) { > > @@ -2021,9 +2024,6 @@ static int sdhci_esdhc_suspend(struct device *dev) > > mmc_retune_needed(host->mmc); > > } > > > > - if (host->tuning_mode != SDHCI_TUNING_MODE_3) > > - mmc_retune_needed(host->mmc); > > - > > /* > > * For the device need to keep power during system PM, need > > * to save the tuning delay value just in case the usdhc > > @@ -2033,9 +2033,13 @@ static int sdhci_esdhc_suspend(struct device *dev) > > esdhc_is_usdhc(imx_data)) > > sdhc_esdhc_tuning_save(host); > > > > - ret = sdhci_suspend_host(host); > > - if (ret) > > - return ret; > > + if (device_may_wakeup(dev)) { > > + /* The irqs of imx are not shared. It is safe to disable */ > > + disable_irq(host->irq); > > + ret = sdhci_enable_irq_wakeups(host); > > + if (!ret) > > + dev_warn(dev, "Failed to enable irq wakeup\n"); > > + } > > > > ret = pinctrl_pm_select_sleep_state(dev); > > if (ret) > > @@ -2043,6 +2047,12 @@ static int sdhci_esdhc_suspend(struct device *dev) > > > > ret = mmc_gpio_set_cd_wake(host->mmc, true); > > > > + /* > > + * Make sure invoke runtime_suspend to gate off clock. > > + * uSDHC IP supports in-band SDIO wakeup even without clock. > > + */ > > + pm_runtime_force_suspend(dev); > > + > > return ret; > > } > > > > @@ -2053,16 +2063,19 @@ static int sdhci_esdhc_resume(struct device *dev) > > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > > int ret; > > > > - ret = pinctrl_pm_select_default_state(dev); > > + pm_runtime_force_resume(dev); > > + > > + ret = mmc_gpio_set_cd_wake(host->mmc, false); > > 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; > > + if (host->irq_wake_enabled) { > > + sdhci_disable_irq_wakeups(host); > > + enable_irq(host->irq); > > + } > > > > /* > > * restore the saved tuning delay value for the device which keep > > @@ -2072,11 +2085,8 @@ static int sdhci_esdhc_resume(struct device *dev) > > esdhc_is_usdhc(imx_data)) > > sdhc_esdhc_tuning_restore(host); > > > > - if (host->mmc->caps2 & MMC_CAP2_CQE) > > - ret = cqhci_resume(host->mmc); > > - > > - if (!ret) > > - ret = mmc_gpio_set_cd_wake(host->mmc, false); > > + pm_runtime_mark_last_busy(dev); > > + pm_runtime_put_autosuspend(dev); > > > > return ret; > > } >
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index fd5681d1e31f..32fa0b2bb912 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -3755,7 +3755,7 @@ static bool sdhci_cd_irq_can_wakeup(struct sdhci_host *host) * sdhci_disable_irq_wakeups() since it will be set by * sdhci_enable_card_detection() or sdhci_init(). */ -static bool sdhci_enable_irq_wakeups(struct sdhci_host *host) +bool sdhci_enable_irq_wakeups(struct sdhci_host *host) { u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE | SDHCI_WAKE_ON_INT; @@ -3787,8 +3787,9 @@ static bool sdhci_enable_irq_wakeups(struct sdhci_host *host) return host->irq_wake_enabled; } +EXPORT_SYMBOL_GPL(sdhci_enable_irq_wakeups); -static void sdhci_disable_irq_wakeups(struct sdhci_host *host) +void sdhci_disable_irq_wakeups(struct sdhci_host *host) { u8 val; u8 mask = SDHCI_WAKE_ON_INSERT | SDHCI_WAKE_ON_REMOVE @@ -3802,6 +3803,7 @@ static void sdhci_disable_irq_wakeups(struct sdhci_host *host) host->irq_wake_enabled = false; } +EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups); int sdhci_suspend_host(struct sdhci_host *host) { diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index cd0e35a80542..f9d65dd0f2b2 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -875,6 +875,8 @@ void sdhci_adma_write_desc(struct sdhci_host *host, void **desc, dma_addr_t addr, int len, unsigned int cmd); #ifdef CONFIG_PM +bool sdhci_enable_irq_wakeups(struct sdhci_host *host); +void sdhci_disable_irq_wakeups(struct sdhci_host *host); int sdhci_suspend_host(struct sdhci_host *host); int sdhci_resume_host(struct sdhci_host *host); int sdhci_runtime_suspend_host(struct sdhci_host *host);