[PATCH 0/4] refactor the system PM logic for sdhci-esdhc-imx

[PATCH 0/4] refactor the system PM logic for sdhci-esdhc-imx

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Currently, if SD slot do not insert any SD card, system suspend/resume
will hung, because in system suspend, there is register access, but the
usdhc per clock is gate off since it is in runtime suspend state.

This patch set refactor the system PM logic, try to merge PM callback
sdhci_esdhc_suspend/sdhci_esdhc_resume into runtiem PM callback
sdhci_esdhc_runtime_suspend/sdhci_esdhc_runtime_resume, and use
pm_runtime_force_suspend/resume instead. To support SDIO wakeup,
add this pm_runtime_force_suspend/resume in no irq stage.

Haibo Chen (4):
  mmc: sdhci: export APIs for sdhci irq wakeup
  mmc: host: sdhci-esdhc-imx: refactor the system PM logic
  mmc: host: sdhci-esdhc-imx: save tuning value for the SDIO card as
    wakeup source
  mmc: sdhci-esdhc-imx: do not change to sleep pinctrl state in suspend
    if enable wakeup

 drivers/mmc/host/sdhci-esdhc-imx.c | 145 ++++++++++++++++++++++++-----
 drivers/mmc/host/sdhci.c           |   6 +-
 drivers/mmc/host/sdhci.h           |   2 +
 3 files changed, 127 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH 1/4] mmc: sdhci: export APIs for sdhci irq wakeup

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Export the sdhci_enable_irq_wakeups() and sdhci_disable_irq_wakeups,
so other driver can use them.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci.c | 6 ++++--
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4b91c9e96635..b3df3e7653af 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3698,7 +3698,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;
@@ -3730,8 +3730,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
@@ -3745,6 +3746,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 f531b617f28d..bc303fe7f87b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -802,6 +802,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);
-- 
2.34.1

[PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[haibo.chen]

From: Haibo Chen <haibo.chen@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 hung.

Now use pm_runtime_force_suspend/resume() in NOIRQ_SYSTEM_SLEEP_PM,
add in NOIRQ stage can cover SDIO wakeup feature, because in interrupt
handler, there is register access, need the per clock on.

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.

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>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 39 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index c7582ad45123..18febfeb60cf 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1871,11 +1871,13 @@ 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, so need to make sure gate on ipg clock.
+	 * 2, make sure the pm_runtime_force_suspend() in NOIRQ stage really
+	 *    invoke its ->runtime_suspend callback.
+	 */
+	pm_runtime_get_sync(dev);
 
 	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
 		(host->tuning_mode != SDHCI_TUNING_MODE_1)) {
@@ -1883,12 +1885,11 @@ 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);
-
-	ret = sdhci_suspend_host(host);
-	if (ret)
-		return ret;
+	if (device_may_wakeup(dev)) {
+		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)
@@ -1904,22 +1905,18 @@ static int sdhci_esdhc_resume(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = pinctrl_pm_select_default_state(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->mmc->caps2 & MMC_CAP2_CQE)
-		ret = cqhci_resume(host->mmc);
+	if (host->irq_wake_enabled)
+		sdhci_disable_irq_wakeups(host);
 
-	if (!ret)
-		ret = mmc_gpio_set_cd_wake(host->mmc, false);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return ret;
 }
@@ -2011,6 +2008,8 @@ static const struct dev_pm_ops sdhci_esdhc_pmops = {
 	SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume)
 	SET_RUNTIME_PM_OPS(sdhci_esdhc_runtime_suspend,
 				sdhci_esdhc_runtime_resume, NULL)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+					pm_runtime_force_resume)
 };
 
 static struct platform_driver sdhci_esdhc_imx_driver = {
-- 
2.34.1

Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Ulf Hansson]

On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@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 hung.
>
> Now use pm_runtime_force_suspend/resume() in NOIRQ_SYSTEM_SLEEP_PM,
> add in NOIRQ stage can cover SDIO wakeup feature, because in interrupt
> handler, there is register access, need the per clock on.
>
> 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.
>
> 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.

I had a look at the code - and yes, there are certainly several
problems with PM support in this driver.

>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 39 +++++++++++++++---------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index c7582ad45123..18febfeb60cf 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1871,11 +1871,13 @@ 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, so need to make sure gate on ipg clock.

You are right that we need to call pm_runtime_get_sync() for this reason.

However, the real question is rather; Under what circumstances do we
really need to make a register access beyond this point?

If the device is already runtime suspended, I am sure we could just
leave it in that state without having to touch any of its registers.

As I understand it, there are mainly two reasons why the device may be
runtime resumed at this point:
*) The runtime PM usage count has been bumped in
sdhci_enable_sdio_irq(), since the SDIO irqs are enabled and it's
likely that we will configure them for system wakeup too.
*) The device has been used, but nothing really prevents it from being
put into a low power state via the ->runtime_suspend() callback.

> +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ stage really
> +        *    invoke its ->runtime_suspend callback.
> +        */

Rather than using the *noirq-callbacks, we should be able to call
pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice versa
for sdhci_esdhc_resume().

Although, according to my earlier comment above, we also need to take
into account the SDIO irq. If it's being enabled for system wakeup, we
must not put the controller into low power mode by calling
pm_runtime_force_suspend(), otherwise we will not be able to deliver
the wakeup, right?

> +       pm_runtime_get_sync(dev);
>
>         if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
>                 (host->tuning_mode != SDHCI_TUNING_MODE_1)) {
> @@ -1883,12 +1885,11 @@ 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);
> -
> -       ret = sdhci_suspend_host(host);
> -       if (ret)
> -               return ret;
> +       if (device_may_wakeup(dev)) {
> +               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)
> @@ -1904,22 +1905,18 @@ static int sdhci_esdhc_resume(struct device *dev)
>         struct sdhci_host *host = dev_get_drvdata(dev);
>         int ret;
>
> -       ret = pinctrl_pm_select_default_state(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);

This looks like another special use-case. If I understand correctly,
on some platforms some additional re-initialization of the controller
may be needed at system resume.

If you want to move towards using pm_runtime_force_suspend|resume(), I
suggest moving the above call into the ->runtime_resume() callback. To
allow the ->runtime_resume() callback to know when this
re-initialization is needed, we can use a flag that we set here and
clear in the ->runtime_resume() callback.

>
> -       ret = sdhci_resume_host(host);
> -       if (ret)
> -               return ret;
> -
> -       if (host->mmc->caps2 & MMC_CAP2_CQE)
> -               ret = cqhci_resume(host->mmc);
> +       if (host->irq_wake_enabled)
> +               sdhci_disable_irq_wakeups(host);
>
> -       if (!ret)
> -               ret = mmc_gpio_set_cd_wake(host->mmc, false);
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
>
>         return ret;
>  }
> @@ -2011,6 +2008,8 @@ static const struct dev_pm_ops sdhci_esdhc_pmops = {
>         SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume)
>         SET_RUNTIME_PM_OPS(sdhci_esdhc_runtime_suspend,
>                                 sdhci_esdhc_runtime_resume, NULL)
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                       pm_runtime_force_resume)
>  };
>
>  static struct platform_driver sdhci_esdhc_imx_driver = {
> --
> 2.34.1
>

Kind regards
Uffe

RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Bough Chen]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: 2024年10月17日 21:07
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-S32 <S32@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM
> logic
> 
> On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
> >
> > From: Haibo Chen <haibo.chen@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 hung.
> >
> > Now use pm_runtime_force_suspend/resume() in NOIRQ_SYSTEM_SLEEP_PM,
> > add in NOIRQ stage can cover SDIO wakeup feature, because in interrupt
> > handler, there is register access, need the per clock on.
> >
> > 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.
> >
> > 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.
> 
> I had a look at the code - and yes, there are certainly several problems with PM
> support in this driver.
> 
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 39
> > +++++++++++++++---------------
> >  1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index c7582ad45123..18febfeb60cf 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -1871,11 +1871,13 @@ 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, so need to make sure gate on ipg clock.
> 
> You are right that we need to call pm_runtime_get_sync() for this reason.
> 
> However, the real question is rather; Under what circumstances do we really
> need to make a register access beyond this point?
> 
> If the device is already runtime suspended, I am sure we could just leave it in
> that state without having to touch any of its registers.
> 
> As I understand it, there are mainly two reasons why the device may be runtime
> resumed at this point:
> *) The runtime PM usage count has been bumped in sdhci_enable_sdio_irq(),
> since the SDIO irqs are enabled and it's likely that we will configure them for
> system wakeup too.
> *) The device has been used, but nothing really prevents it from being put into a
> low power state via the ->runtime_suspend() callback.
> 
> > +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ stage
> really
> > +        *    invoke its ->runtime_suspend callback.
> > +        */
> 
> Rather than using the *noirq-callbacks, we should be able to call
> pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice versa for
> sdhci_esdhc_resume().
> 
> Although, according to my earlier comment above, we also need to take into
> account the SDIO irq. If it's being enabled for system wakeup, we must not put
> the controller into low power mode by calling pm_runtime_force_suspend(),
> otherwise we will not be able to deliver the wakeup, right?

Thanks for your careful review! 
Yes, I agree.

> 
> > +       pm_runtime_get_sync(dev);
> >
> >         if ((imx_data->socdata->flags &
> ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
> >                 (host->tuning_mode != SDHCI_TUNING_MODE_1)) { @@
> > -1883,12 +1885,11 @@ 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);
> > -
> > -       ret = sdhci_suspend_host(host);
> > -       if (ret)
> > -               return ret;
> > +       if (device_may_wakeup(dev)) {
> > +               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)
> > @@ -1904,22 +1905,18 @@ static int sdhci_esdhc_resume(struct device
> *dev)
> >         struct sdhci_host *host = dev_get_drvdata(dev);
> >         int ret;
> >
> > -       ret = pinctrl_pm_select_default_state(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);
> 
> This looks like another special use-case. If I understand correctly, on some
> platforms some additional re-initialization of the controller may be needed at
> system resume.
> 
> If you want to move towards using pm_runtime_force_suspend|resume(), I
> suggest moving the above call into the ->runtime_resume() callback. To allow
> the ->runtime_resume() callback to know when this re-initialization is needed,
> we can use a flag that we set here and clear in the ->runtime_resume()
> callback.

Yes, I can do like that. Seems I can remove the NOIRQ in v2.

Thanks for your suggestion

Regards
Haibo Chen


> 
> >
> > -       ret = sdhci_resume_host(host);
> > -       if (ret)
> > -               return ret;
> > -
> > -       if (host->mmc->caps2 & MMC_CAP2_CQE)
> > -               ret = cqhci_resume(host->mmc);
> > +       if (host->irq_wake_enabled)
> > +               sdhci_disable_irq_wakeups(host);
> >
> > -       if (!ret)
> > -               ret = mmc_gpio_set_cd_wake(host->mmc, false);
> > +       pm_runtime_mark_last_busy(dev);
> > +       pm_runtime_put_autosuspend(dev);
> >
> >         return ret;
> >  }
> > @@ -2011,6 +2008,8 @@ static const struct dev_pm_ops sdhci_esdhc_pmops
> = {
> >         SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend,
> sdhci_esdhc_resume)
> >         SET_RUNTIME_PM_OPS(sdhci_esdhc_runtime_suspend,
> >                                 sdhci_esdhc_runtime_resume, NULL)
> > +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +                                       pm_runtime_force_resume)
> >  };
> >
> >  static struct platform_driver sdhci_esdhc_imx_driver = {
> > --
> > 2.34.1
> >
> 
> Kind regards
> Uffe

RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Bough Chen]

> -----Original Message-----
> From: Bough Chen
> Sent: 2024年10月18日 9:23
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-S32 <S32@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM
> logic
> 
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: 2024年10月17日 21:07
> > To: Bough Chen <haibo.chen@nxp.com>
> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > system PM logic
> >
> > On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
> > >
> > > From: Haibo Chen <haibo.chen@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 hung.
> > >
> > > Now use pm_runtime_force_suspend/resume() in
> NOIRQ_SYSTEM_SLEEP_PM,
> > > add in NOIRQ stage can cover SDIO wakeup feature, because in
> > > interrupt handler, there is register access, need the per clock on.
> > >
> > > 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.
> > >
> > > 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.
> >
> > I had a look at the code - and yes, there are certainly several
> > problems with PM support in this driver.
> >
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 39
> > > +++++++++++++++---------------
> > >  1 file changed, 19 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index c7582ad45123..18febfeb60cf 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -1871,11 +1871,13 @@ 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, so need to make sure gate on ipg
> clock.
> >
> > You are right that we need to call pm_runtime_get_sync() for this reason.
> >
> > However, the real question is rather; Under what circumstances do we
> > really need to make a register access beyond this point?
> >
> > If the device is already runtime suspended, I am sure we could just
> > leave it in that state without having to touch any of its registers.
> >
> > As I understand it, there are mainly two reasons why the device may be
> > runtime resumed at this point:
> > *) The runtime PM usage count has been bumped in
> > sdhci_enable_sdio_irq(), since the SDIO irqs are enabled and it's
> > likely that we will configure them for system wakeup too.
> > *) The device has been used, but nothing really prevents it from being
> > put into a low power state via the ->runtime_suspend() callback.
> >
> > > +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ
> > > + stage
> > really
> > > +        *    invoke its ->runtime_suspend callback.
> > > +        */
> >
> > Rather than using the *noirq-callbacks, we should be able to call
> > pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice versa
> > for sdhci_esdhc_resume().
> >
> > Although, according to my earlier comment above, we also need to take
> > into account the SDIO irq. If it's being enabled for system wakeup, we
> > must not put the controller into low power mode by calling
> > pm_runtime_force_suspend(), otherwise we will not be able to deliver the
> wakeup, right?
> 
> Thanks for your careful review!
> Yes, I agree.

Hi Ulf,

Sorry, I maybe give the wrong answer.

I double check the IP, usdhc can support sdio irq wakeup even when usdhc clock gate off.
So to save power, need to call pm_runtime_force_suspend() to gate off the clock when enable sdio irq for system wakeup.
This is the main reason I involve pm_runtime_force_suspend in noirq stage, because in sdhci_irq, there is register access, need gate on clock.

Best Regards
Haibo Chen 


> 
> >
> > > +       pm_runtime_get_sync(dev);
> > >
> > >         if ((imx_data->socdata->flags &
> > ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
> > >                 (host->tuning_mode != SDHCI_TUNING_MODE_1))
> { @@
> > > -1883,12 +1885,11 @@ 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);
> > > -
> > > -       ret = sdhci_suspend_host(host);
> > > -       if (ret)
> > > -               return ret;
> > > +       if (device_may_wakeup(dev)) {
> > > +               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)
> > > @@ -1904,22 +1905,18 @@ static int sdhci_esdhc_resume(struct device
> > *dev)
> > >         struct sdhci_host *host = dev_get_drvdata(dev);
> > >         int ret;
> > >
> > > -       ret = pinctrl_pm_select_default_state(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);
> >
> > This looks like another special use-case. If I understand correctly,
> > on some platforms some additional re-initialization of the controller
> > may be needed at system resume.
> >
> > If you want to move towards using pm_runtime_force_suspend|resume(), I
> > suggest moving the above call into the ->runtime_resume() callback. To
> > allow the ->runtime_resume() callback to know when this
> > re-initialization is needed, we can use a flag that we set here and
> > clear in the ->runtime_resume() callback.
> 
> Yes, I can do like that. Seems I can remove the NOIRQ in v2.
> 
> Thanks for your suggestion
> 
> Regards
> Haibo Chen
> 
> 
> >
> > >
> > > -       ret = sdhci_resume_host(host);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > > -       if (host->mmc->caps2 & MMC_CAP2_CQE)
> > > -               ret = cqhci_resume(host->mmc);
> > > +       if (host->irq_wake_enabled)
> > > +               sdhci_disable_irq_wakeups(host);
> > >
> > > -       if (!ret)
> > > -               ret = mmc_gpio_set_cd_wake(host->mmc, false);
> > > +       pm_runtime_mark_last_busy(dev);
> > > +       pm_runtime_put_autosuspend(dev);
> > >
> > >         return ret;
> > >  }
> > > @@ -2011,6 +2008,8 @@ static const struct dev_pm_ops
> > > sdhci_esdhc_pmops
> > = {
> > >         SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend,
> > sdhci_esdhc_resume)
> > >         SET_RUNTIME_PM_OPS(sdhci_esdhc_runtime_suspend,
> > >                                 sdhci_esdhc_runtime_resume,
> NULL)
> > > +
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > > +
> pm_runtime_force_resume)
> > >  };
> > >
> > >  static struct platform_driver sdhci_esdhc_imx_driver = {
> > > --
> > > 2.34.1
> > >
> >
> > Kind regards
> > Uffe

Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Ulf Hansson]

On Fri, 18 Oct 2024 at 05:20, Bough Chen <haibo.chen@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2024年10月18日 9:23
> > To: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM
> > logic
> >
> > > -----Original Message-----
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > Sent: 2024年10月17日 21:07
> > > To: Bough Chen <haibo.chen@nxp.com>
> > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > > system PM logic
> > >
> > > On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
> > > >
> > > > From: Haibo Chen <haibo.chen@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 hung.
> > > >
> > > > Now use pm_runtime_force_suspend/resume() in
> > NOIRQ_SYSTEM_SLEEP_PM,
> > > > add in NOIRQ stage can cover SDIO wakeup feature, because in
> > > > interrupt handler, there is register access, need the per clock on.
> > > >
> > > > 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.
> > > >
> > > > 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.
> > >
> > > I had a look at the code - and yes, there are certainly several
> > > problems with PM support in this driver.
> > >
> > > >
> > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > ---
> > > >  drivers/mmc/host/sdhci-esdhc-imx.c | 39
> > > > +++++++++++++++---------------
> > > >  1 file changed, 19 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > index c7582ad45123..18febfeb60cf 100644
> > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > @@ -1871,11 +1871,13 @@ 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, so need to make sure gate on ipg
> > clock.
> > >
> > > You are right that we need to call pm_runtime_get_sync() for this reason.
> > >
> > > However, the real question is rather; Under what circumstances do we
> > > really need to make a register access beyond this point?
> > >
> > > If the device is already runtime suspended, I am sure we could just
> > > leave it in that state without having to touch any of its registers.
> > >
> > > As I understand it, there are mainly two reasons why the device may be
> > > runtime resumed at this point:
> > > *) The runtime PM usage count has been bumped in
> > > sdhci_enable_sdio_irq(), since the SDIO irqs are enabled and it's
> > > likely that we will configure them for system wakeup too.
> > > *) The device has been used, but nothing really prevents it from being
> > > put into a low power state via the ->runtime_suspend() callback.
> > >
> > > > +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ
> > > > + stage
> > > really
> > > > +        *    invoke its ->runtime_suspend callback.
> > > > +        */
> > >
> > > Rather than using the *noirq-callbacks, we should be able to call
> > > pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice versa
> > > for sdhci_esdhc_resume().
> > >
> > > Although, according to my earlier comment above, we also need to take
> > > into account the SDIO irq. If it's being enabled for system wakeup, we
> > > must not put the controller into low power mode by calling
> > > pm_runtime_force_suspend(), otherwise we will not be able to deliver the
> > wakeup, right?
> >
> > Thanks for your careful review!
> > Yes, I agree.
>
> Hi Ulf,
>
> Sorry, I maybe give the wrong answer.
>
> I double check the IP, usdhc can support sdio irq wakeup even when usdhc clock gate off.

Okay, so there is some out-band logic that can manage the SDIO irq,
even when the controller has been runtime suspended?

In these cases, we use dev_pm_set_dedicated_wake_irq* to manage that
wake-irq. There are other mmc host drivers that implement support for
this too.

If you are referring to solely clock gating, that is not going to
work. A runtime suspended controller is not supposed to deliver
in-band irqs.

> So to save power, need to call pm_runtime_force_suspend() to gate off the clock when enable sdio irq for system wakeup.
> This is the main reason I involve pm_runtime_force_suspend in noirq stage, because in sdhci_irq, there is register access, need gate on clock.

In summary, to support the out-band irq as a wakeup for runtime and
system suspend, dev_pm_set_dedicated_wake_irq* should be used.

To move things forward, I suggest you start simple and add support for
the out-band irq as a step on top.

[...]

Kind regards
Uffe

RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Bough Chen]

> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: 2024年10月22日 16:29
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-S32 <S32@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM
> logic
> 
> On Fri, 18 Oct 2024 at 05:20, Bough Chen <haibo.chen@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Bough Chen
> > > Sent: 2024年10月18日 9:23
> > > To: Ulf Hansson <ulf.hansson@linaro.org>
> > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > > system PM logic
> > >
> > > > -----Original Message-----
> > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Sent: 2024年10月17日 21:07
> > > > To: Bough Chen <haibo.chen@nxp.com>
> > > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > > > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > > > system PM logic
> > > >
> > > > On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
> > > > >
> > > > > From: Haibo Chen <haibo.chen@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 hung.
> > > > >
> > > > > Now use pm_runtime_force_suspend/resume() in
> > > NOIRQ_SYSTEM_SLEEP_PM,
> > > > > add in NOIRQ stage can cover SDIO wakeup feature, because in
> > > > > interrupt handler, there is register access, need the per clock on.
> > > > >
> > > > > 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.
> > > > >
> > > > > 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.
> > > >
> > > > I had a look at the code - and yes, there are certainly several
> > > > problems with PM support in this driver.
> > > >
> > > > >
> > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > > ---
> > > > >  drivers/mmc/host/sdhci-esdhc-imx.c | 39
> > > > > +++++++++++++++---------------
> > > > >  1 file changed, 19 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > index c7582ad45123..18febfeb60cf 100644
> > > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > @@ -1871,11 +1871,13 @@ 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, so need to make sure
> > > > > + gate on ipg
> > > clock.
> > > >
> > > > You are right that we need to call pm_runtime_get_sync() for this reason.
> > > >
> > > > However, the real question is rather; Under what circumstances do
> > > > we really need to make a register access beyond this point?
> > > >
> > > > If the device is already runtime suspended, I am sure we could
> > > > just leave it in that state without having to touch any of its registers.
> > > >
> > > > As I understand it, there are mainly two reasons why the device
> > > > may be runtime resumed at this point:
> > > > *) The runtime PM usage count has been bumped in
> > > > sdhci_enable_sdio_irq(), since the SDIO irqs are enabled and it's
> > > > likely that we will configure them for system wakeup too.
> > > > *) The device has been used, but nothing really prevents it from
> > > > being put into a low power state via the ->runtime_suspend() callback.
> > > >
> > > > > +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ
> > > > > + stage
> > > > really
> > > > > +        *    invoke its ->runtime_suspend callback.
> > > > > +        */
> > > >
> > > > Rather than using the *noirq-callbacks, we should be able to call
> > > > pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice
> > > > versa for sdhci_esdhc_resume().
> > > >
> > > > Although, according to my earlier comment above, we also need to
> > > > take into account the SDIO irq. If it's being enabled for system
> > > > wakeup, we must not put the controller into low power mode by
> > > > calling pm_runtime_force_suspend(), otherwise we will not be able
> > > > to deliver the
> > > wakeup, right?
> > >
> > > Thanks for your careful review!
> > > Yes, I agree.
> >
> > Hi Ulf,
> >
> > Sorry, I maybe give the wrong answer.
> >
> > I double check the IP, usdhc can support sdio irq wakeup even when usdhc
> clock gate off.
> 
> Okay, so there is some out-band logic that can manage the SDIO irq, even when
> the controller has been runtime suspended?

Hi Ulf,

Sorry for the late reply.
 
Seems not out-band logic, refer to SD Host Controller Standard Specification Version 3.00, 2.2.13 Wakeup Control Register (Offset 02Bh), P45
Or refer to static bool sdhci_enable_irq_wakeups(struct sdhci_host *host) in sdhci.c

> 
> In these cases, we use dev_pm_set_dedicated_wake_irq* to manage that
> wake-irq. There are other mmc host drivers that implement support for this too.
> 
> If you are referring to solely clock gating, that is not going to work. A runtime
> suspended controller is not supposed to deliver in-band irqs.

In sdhci_irq(), it will check host->runtime_suspended, if in runtime suspend state, directly return. But for SDIO wakeup irq, here will meet irq storm, because no place to clear the interrupt status register.
This is why I involve noirq pm, force runtime resume before system handle sdio wakeup irq.

Or to support in-band SDIO wakeup, add some changes in common sdhic.c, just like following, which do you prefer? Or any thought?

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0b46b50aa28b..8928af3e62d3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3532,7 +3532,10 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
        spin_lock(&host->lock);

        if (host->runtime_suspended) {
+               disable_irq_nosync(host->irq);
+               host->wakeup_pending = true;
                spin_unlock(&host->lock);
                return IRQ_NONE;
        }

@@ -3878,6 +3881,10 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
        spin_lock_irqsave(&host->lock, flags);

        host->runtime_suspended = false;
+       if (host->wakeup_pending) {
+               host->wakeup_pending = false;
+               enable_irq(host->irq);
+       }

        /* Enable SDIO IRQ */
        if (sdio_irq_claimed(mmc))
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index e62f483ff3b8..ce6f750cc4dc 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -536,6 +536,7 @@ struct sdhci_host {
        bool reinit_uhs;        /* Force UHS-related re-initialization */

        bool runtime_suspended; /* Host is runtime suspended */
+       bool wakeup_pending;
        bool bus_on;            /* Bus power prevents runtime suspend */
        bool preset_enabled;    /* Preset is enabled */
        bool pending_reset;     /* Cmd/data reset is pending


Best Regards
Haibo Chen
> 
> > So to save power, need to call pm_runtime_force_suspend() to gate off the
> clock when enable sdio irq for system wakeup.
> > This is the main reason I involve pm_runtime_force_suspend in noirq stage,
> because in sdhci_irq, there is register access, need gate on clock.
> 
> In summary, to support the out-band irq as a wakeup for runtime and system
> suspend, dev_pm_set_dedicated_wake_irq* should be used.
> 
> To move things forward, I suggest you start simple and add support for the
> out-band irq as a step on top.
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM logic

[Ulf Hansson]

On Thu, 7 Nov 2024 at 10:20, Bough Chen <haibo.chen@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Ulf Hansson <ulf.hansson@linaro.org>
> > Sent: 2024年10月22日 16:29
> > To: Bough Chen <haibo.chen@nxp.com>
> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the system PM
> > logic
> >
> > On Fri, 18 Oct 2024 at 05:20, Bough Chen <haibo.chen@nxp.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Bough Chen
> > > > Sent: 2024年10月18日 9:23
> > > > To: Ulf Hansson <ulf.hansson@linaro.org>
> > > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > > > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > > Subject: RE: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > > > system PM logic
> > > >
> > > > > -----Original Message-----
> > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > Sent: 2024年10月17日 21:07
> > > > > To: Bough Chen <haibo.chen@nxp.com>
> > > > > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org;
> > > > > imx@lists.linux.dev; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > > > > kernel@pengutronix.de; festevam@gmail.com; dl-S32 <S32@nxp.com>;
> > > > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH 2/4] mmc: host: sdhci-esdhc-imx: refactor the
> > > > > system PM logic
> > > > >
> > > > > On Mon, 14 Oct 2024 at 08:00, <haibo.chen@nxp.com> wrote:
> > > > > >
> > > > > > From: Haibo Chen <haibo.chen@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 hung.
> > > > > >
> > > > > > Now use pm_runtime_force_suspend/resume() in
> > > > NOIRQ_SYSTEM_SLEEP_PM,
> > > > > > add in NOIRQ stage can cover SDIO wakeup feature, because in
> > > > > > interrupt handler, there is register access, need the per clock on.
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > I had a look at the code - and yes, there are certainly several
> > > > > problems with PM support in this driver.
> > > > >
> > > > > >
> > > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > > > ---
> > > > > >  drivers/mmc/host/sdhci-esdhc-imx.c | 39
> > > > > > +++++++++++++++---------------
> > > > > >  1 file changed, 19 insertions(+), 20 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > > index c7582ad45123..18febfeb60cf 100644
> > > > > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > > > > @@ -1871,11 +1871,13 @@ 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, so need to make sure
> > > > > > + gate on ipg
> > > > clock.
> > > > >
> > > > > You are right that we need to call pm_runtime_get_sync() for this reason.
> > > > >
> > > > > However, the real question is rather; Under what circumstances do
> > > > > we really need to make a register access beyond this point?
> > > > >
> > > > > If the device is already runtime suspended, I am sure we could
> > > > > just leave it in that state without having to touch any of its registers.
> > > > >
> > > > > As I understand it, there are mainly two reasons why the device
> > > > > may be runtime resumed at this point:
> > > > > *) The runtime PM usage count has been bumped in
> > > > > sdhci_enable_sdio_irq(), since the SDIO irqs are enabled and it's
> > > > > likely that we will configure them for system wakeup too.
> > > > > *) The device has been used, but nothing really prevents it from
> > > > > being put into a low power state via the ->runtime_suspend() callback.
> > > > >
> > > > > > +        * 2, make sure the pm_runtime_force_suspend() in NOIRQ
> > > > > > + stage
> > > > > really
> > > > > > +        *    invoke its ->runtime_suspend callback.
> > > > > > +        */
> > > > >
> > > > > Rather than using the *noirq-callbacks, we should be able to call
> > > > > pm_runtime_force_suspend() from sdhci_esdhc_suspend(). And vice
> > > > > versa for sdhci_esdhc_resume().
> > > > >
> > > > > Although, according to my earlier comment above, we also need to
> > > > > take into account the SDIO irq. If it's being enabled for system
> > > > > wakeup, we must not put the controller into low power mode by
> > > > > calling pm_runtime_force_suspend(), otherwise we will not be able
> > > > > to deliver the
> > > > wakeup, right?
> > > >
> > > > Thanks for your careful review!
> > > > Yes, I agree.
> > >
> > > Hi Ulf,
> > >
> > > Sorry, I maybe give the wrong answer.
> > >
> > > I double check the IP, usdhc can support sdio irq wakeup even when usdhc
> > clock gate off.
> >
> > Okay, so there is some out-band logic that can manage the SDIO irq, even when
> > the controller has been runtime suspended?
>
> Hi Ulf,
>
> Sorry for the late reply.
>
> Seems not out-band logic, refer to SD Host Controller Standard Specification Version 3.00, 2.2.13 Wakeup Control Register (Offset 02Bh), P45
> Or refer to static bool sdhci_enable_irq_wakeups(struct sdhci_host *host) in sdhci.c

Right, those handle the in-band wakups.

If the platform doesn't support some out-band logic to deliver wakups,
we simply need to keep the controller always powered on, when SDIO
irqs are enabled.

In other words, call a pm_runtime_get_noresume() when SDIO irq gets
enabled and pm_runtime_put_noidle() when they become disabled.

>
> >
> > In these cases, we use dev_pm_set_dedicated_wake_irq* to manage that
> > wake-irq. There are other mmc host drivers that implement support for this too.
> >
> > If you are referring to solely clock gating, that is not going to work. A runtime
> > suspended controller is not supposed to deliver in-band irqs.
>
> In sdhci_irq(), it will check host->runtime_suspended, if in runtime suspend state, directly return. But for SDIO wakeup irq, here will meet irq storm, because no place to clear the interrupt status register.
> This is why I involve noirq pm, force runtime resume before system handle sdio wakeup irq.

That check is indeed to prevent us from accessing the controller when
there are spurious irqs.

Again, if there is no out-band logic you must keep the controller
runtime resumed, when SDIO irqs are enabled.

>
> Or to support in-band SDIO wakeup, add some changes in common sdhic.c, just like following, which do you prefer? Or any thought?
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 0b46b50aa28b..8928af3e62d3 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3532,7 +3532,10 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>         spin_lock(&host->lock);
>
>         if (host->runtime_suspended) {
> +               disable_irq_nosync(host->irq);
> +               host->wakeup_pending = true;

No, this isn't the way to fix it. See above.

>                 spin_unlock(&host->lock);
>                 return IRQ_NONE;
>         }
>
> @@ -3878,6 +3881,10 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
>         spin_lock_irqsave(&host->lock, flags);
>
>         host->runtime_suspended = false;
> +       if (host->wakeup_pending) {
> +               host->wakeup_pending = false;
> +               enable_irq(host->irq);
> +       }
>
>         /* Enable SDIO IRQ */
>         if (sdio_irq_claimed(mmc))
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index e62f483ff3b8..ce6f750cc4dc 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -536,6 +536,7 @@ struct sdhci_host {
>         bool reinit_uhs;        /* Force UHS-related re-initialization */
>
>         bool runtime_suspended; /* Host is runtime suspended */
> +       bool wakeup_pending;
>         bool bus_on;            /* Bus power prevents runtime suspend */
>         bool preset_enabled;    /* Preset is enabled */
>         bool pending_reset;     /* Cmd/data reset is pending
>
>
> Best Regards
> Haibo Chen
> >
> > > So to save power, need to call pm_runtime_force_suspend() to gate off the
> > clock when enable sdio irq for system wakeup.
> > > This is the main reason I involve pm_runtime_force_suspend in noirq stage,
> > because in sdhci_irq, there is register access, need gate on clock.
> >
> > In summary, to support the out-band irq as a wakeup for runtime and system
> > suspend, dev_pm_set_dedicated_wake_irq* should be used.
> >
> > To move things forward, I suggest you start simple and add support for the
> > out-band irq as a step on top.
> >
> > [...]

Kind regards
Uffe

[PATCH 3/4] mmc: host: sdhci-esdhc-imx: save tuning value for the SDIO card as wakeup source

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

For some SoCs like imx6ul(l/z)/imx7d/imx93, during system PM, usdhc will
totally power off, so the internal tuning status will lost. Here add
save/restore the tuning value for any command after system resume back
when re-tuning hold.

The tipical case is for the SDIO which contain flag MMC_PM_KEEP_POWER,
and contain pm_flags MMC_PM_WAKE_SDIO_IRQ. in mmc_sdio_suspend(), SDIO
will switch to 1 bit mode, and switch back to 4 bit mode when resume back.
According to spec, tuning command do not support in 1 bit mode. So when
send cmd52 to switch back to 4 bit mode, need to hold re-tuning. But this
cmd52 still need a correct sample point, otherwise will meet command CRC
error, so need to keep the previous tuning value.

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

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 18febfeb60cf..4173967022d0 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -80,6 +80,9 @@
 #define  ESDHC_TUNE_CTRL_STEP		1
 #define  ESDHC_TUNE_CTRL_MIN		0
 #define  ESDHC_TUNE_CTRL_MAX		((1 << 7) - 1)
+#define ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_MASK		0x7f000000
+#define ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_SHIFT	24
+#define ESDHC_TUNE_CTRL_STATUS_DLY_CELL_SET_PRE_SHIFT	8
 
 /* strobe dll register */
 #define ESDHC_STROBE_DLL_CTRL		0x70
@@ -234,6 +237,7 @@ struct esdhc_platform_data {
 	unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
 	unsigned int tuning_start_tap;	/* The start delay cell point in tuning procedure */
 	unsigned int strobe_dll_delay_target;	/* The delay cell for strobe pad (read clock) */
+	unsigned int saved_tuning_delay_cell;	/* save the value of tuning delay cell */
 };
 
 struct esdhc_soc_data {
@@ -1055,7 +1059,7 @@ static void esdhc_reset_tuning(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
-	u32 ctrl;
+	u32 ctrl, tuning_ctrl;
 	int ret;
 
 	/* Reset the tuning circuit */
@@ -1069,6 +1073,17 @@ static void esdhc_reset_tuning(struct sdhci_host *host)
 			writel(0, host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
 		} else if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
 			writel(ctrl, host->ioaddr + ESDHC_MIX_CTRL);
+
+			/*
+			 * enable the std tuning just in case it cleared in
+			 * sdhc_esdhc_tuning_restore.
+			 */
+			tuning_ctrl = readl(host->ioaddr + ESDHC_TUNING_CTRL);
+			if (!(tuning_ctrl & ESDHC_STD_TUNING_EN)) {
+				tuning_ctrl |= ESDHC_STD_TUNING_EN;
+				writel(tuning_ctrl, host->ioaddr + ESDHC_TUNING_CTRL);
+			}
+
 			ctrl = readl(host->ioaddr + SDHCI_AUTO_CMD_STATUS);
 			ctrl &= ~ESDHC_MIX_CTRL_SMPCLK_SEL;
 			ctrl &= ~ESDHC_MIX_CTRL_EXE_TUNE;
@@ -1147,7 +1162,8 @@ static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
 	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
 			ESDHC_MIX_CTRL_FBCLK_SEL;
 	writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
-	writel(val << 8, host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
+	writel(val << ESDHC_TUNE_CTRL_STATUS_DLY_CELL_SET_PRE_SHIFT,
+				host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
 	dev_dbg(mmc_dev(host->mmc),
 		"tuning with delay 0x%x ESDHC_TUNE_CTRL_STATUS 0x%x\n",
 			val, readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS));
@@ -1555,6 +1571,58 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 	}
 }
 
+static void sdhc_esdhc_tuning_save(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	u32 reg;
+
+	/*
+	 * SD/eMMC do not need this tuning save because it will re-init
+	 * after system resume back.
+	 * Here save the tuning delay value for SDIO device since it may
+	 * keep power during system PM. And for usdhc, only SDR50 and
+	 * SDR104 mode for SDIO devide need to do tuning, and need to
+	 * save/restore.
+	 */
+	if ((host->timing == MMC_TIMING_UHS_SDR50) |
+			(host->timing == MMC_TIMING_UHS_SDR104)) {
+		reg = readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
+		reg = (reg & ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_MASK) >>
+				ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_SHIFT;
+		imx_data->boarddata.saved_tuning_delay_cell = reg;
+	}
+}
+
+static void sdhc_esdhc_tuning_restore(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	u32 reg;
+
+	if ((host->timing == MMC_TIMING_UHS_SDR50) |
+			(host->timing == MMC_TIMING_UHS_SDR104)) {
+		/*
+		 * restore the tuning delay value actually is a
+		 * manual tuning method, so clear the standard
+		 * tuning enable bit here. Will set back this
+		 * ESDHC_STD_TUNING_EN in esdhc_reset_tuning()
+		 * when trigger re-tuning.
+		 */
+		reg = readl(host->ioaddr + ESDHC_TUNING_CTRL);
+		reg &= ~ESDHC_STD_TUNING_EN;
+		writel(reg, host->ioaddr + ESDHC_TUNING_CTRL);
+
+		reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
+		reg |= ESDHC_MIX_CTRL_SMPCLK_SEL | ESDHC_MIX_CTRL_FBCLK_SEL;
+		writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
+
+		writel(imx_data->boarddata.saved_tuning_delay_cell <<
+				ESDHC_TUNE_CTRL_STATUS_DLY_CELL_SET_PRE_SHIFT,
+				host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
+	}
+}
+
 static void esdhc_cqe_enable(struct mmc_host *mmc)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -1883,7 +1951,17 @@ static int sdhci_esdhc_suspend(struct device *dev)
 		(host->tuning_mode != SDHCI_TUNING_MODE_1)) {
 		mmc_retune_timer_stop(host->mmc);
 		mmc_retune_needed(host->mmc);
-	}
+
+		/*
+		 * For the SDIO device need to keep power during system PM, and enable
+		 * wakeup, need to save the tuning delay value just in case the retuning
+		 * is hold when SDIO resume, but still need to switch to 4 bit bus width.
+		 */
+		if (host->mmc->sdio_irqs && mmc_card_keep_power(host->mmc) &&
+				(esdhc_is_usdhc(imx_data)))
+			sdhc_esdhc_tuning_save(host);
+
+		}
 
 	if (device_may_wakeup(dev)) {
 		ret = sdhci_enable_irq_wakeups(host);
@@ -1903,6 +1981,8 @@ static int sdhci_esdhc_suspend(struct device *dev)
 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 = mmc_gpio_set_cd_wake(host->mmc, false);
@@ -1915,6 +1995,14 @@ static int sdhci_esdhc_resume(struct device *dev)
 	if (host->irq_wake_enabled)
 		sdhci_disable_irq_wakeups(host);
 
+	/*
+	 * Restore the saved tuning delay value for the SDIO device
+	 * which enabled wakeup and keep power during system PM.
+	 */
+	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
+	    mmc_card_keep_power(host->mmc) && mmc_card_wake_sdio_irq(host->mmc))
+		sdhc_esdhc_tuning_restore(host);
+
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
-- 
2.34.1

Re: [PATCH 3/4] mmc: host: sdhci-esdhc-imx: save tuning value for the SDIO card as wakeup source

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on shawnguo/for-next]
[also build test WARNING on linus/master v6.12-rc3 next-20241014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/haibo-chen-nxp-com/mmc-sdhci-export-APIs-for-sdhci-irq-wakeup/20241014-140300
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
patch link:    https://lore.kernel.org/r/20241014060130.1162629-4-haibo.chen%40nxp.com
patch subject: [PATCH 3/4] mmc: host: sdhci-esdhc-imx: save tuning value for the SDIO card as wakeup source
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20241015/202410150906.OEI0jyKN-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241015/202410150906.OEI0jyKN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410150906.OEI0jyKN-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci-esdhc-imx.c:1592:13: warning: 'sdhc_esdhc_tuning_restore' defined but not used [-Wunused-function]
    1592 | static void sdhc_esdhc_tuning_restore(struct sdhci_host *host)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/mmc/host/sdhci-esdhc-imx.c:1569:13: warning: 'sdhc_esdhc_tuning_save' defined but not used [-Wunused-function]
    1569 | static void sdhc_esdhc_tuning_save(struct sdhci_host *host)
         |             ^~~~~~~~~~~~~~~~~~~~~~


vim +/sdhc_esdhc_tuning_restore +1592 drivers/mmc/host/sdhci-esdhc-imx.c

  1568	
> 1569	static void sdhc_esdhc_tuning_save(struct sdhci_host *host)
  1570	{
  1571		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
  1572		struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
  1573		u32 reg;
  1574	
  1575		/*
  1576		 * SD/eMMC do not need this tuning save because it will re-init
  1577		 * after system resume back.
  1578		 * Here save the tuning delay value for SDIO device since it may
  1579		 * keep power during system PM. And for usdhc, only SDR50 and
  1580		 * SDR104 mode for SDIO devide need to do tuning, and need to
  1581		 * save/restore.
  1582		 */
  1583		if ((host->timing == MMC_TIMING_UHS_SDR50) |
  1584				(host->timing == MMC_TIMING_UHS_SDR104)) {
  1585			reg = readl(host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
  1586			reg = (reg & ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_MASK) >>
  1587					ESDHC_TUNE_CTRL_STATUS_TAP_SEL_PRE_SHIFT;
  1588			imx_data->boarddata.saved_tuning_delay_cell = reg;
  1589		}
  1590	}
  1591	
> 1592	static void sdhc_esdhc_tuning_restore(struct sdhci_host *host)
  1593	{
  1594		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
  1595		struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
  1596		u32 reg;
  1597	
  1598		if ((host->timing == MMC_TIMING_UHS_SDR50) |
  1599				(host->timing == MMC_TIMING_UHS_SDR104)) {
  1600			/*
  1601			 * restore the tuning delay value actually is a
  1602			 * manual tuning method, so clear the standard
  1603			 * tuning enable bit here. Will set back this
  1604			 * ESDHC_STD_TUNING_EN in esdhc_reset_tuning()
  1605			 * when trigger re-tuning.
  1606			 */
  1607			reg = readl(host->ioaddr + ESDHC_TUNING_CTRL);
  1608			reg &= ~ESDHC_STD_TUNING_EN;
  1609			writel(reg, host->ioaddr + ESDHC_TUNING_CTRL);
  1610	
  1611			reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
  1612			reg |= ESDHC_MIX_CTRL_SMPCLK_SEL | ESDHC_MIX_CTRL_FBCLK_SEL;
  1613			writel(reg, host->ioaddr + ESDHC_MIX_CTRL);
  1614	
  1615			writel(imx_data->boarddata.saved_tuning_delay_cell <<
  1616					ESDHC_TUNE_CTRL_STATUS_DLY_CELL_SET_PRE_SHIFT,
  1617					host->ioaddr + ESDHC_TUNE_CTRL_STATUS);
  1618		}
  1619	}
  1620	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 4/4] mmc: sdhci-esdhc-imx: do not change to sleep pinctrl state in suspend if enable wakeup

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

pinctrl sleep state may config the pin mux to certain function to save
power in system PM. But if usdhc is setting as wakeup source, like
the card interrupt(SDIO) or card insert interrupt, it depends on the
related pin mux configured to usdhc function pad.
e.g. To support card interrupt(SDIO interrupt), it need the pin is
configured as usdhc DATA[1] function pin.

Find the issue on imx93-11x11-evk board, SDIO WiFi in band interrupt
can't wakeup system because the pinctrl sleep state config the DATA[1]
pin as GPIO function.

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

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 4173967022d0..d4bb23c9e866 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1969,9 +1969,19 @@ static int sdhci_esdhc_suspend(struct device *dev)
 			dev_warn(dev, "Failed to enable irq wakeup\n");
 	}
 
-	ret = pinctrl_pm_select_sleep_state(dev);
-	if (ret)
-		return ret;
+	/*
+	 * For the device which works as wakeup source, no need
+	 * to change the pinctrl to sleep state.
+	 * e.g. For SDIO device, the interrupt share with data pin,
+	 * but the pinctrl sleep state may config the data pin to
+	 * other function like GPIO function to save power in PM,
+	 * which finally block the SDIO wakeup function.
+	 */
+	if (!device_may_wakeup(dev) || !host->irq_wake_enabled) {
+		ret = pinctrl_pm_select_sleep_state(dev);
+		if (ret)
+			return ret;
+	}
 
 	ret = mmc_gpio_set_cd_wake(host->mmc, true);
 
-- 
2.34.1