diff mbox series

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

Message ID 20241014060130.1162629-3-haibo.chen@nxp.com (mailing list archive)
State New
Headers show
Series refactor the system PM logic for sdhci-esdhc-imx | expand

Commit Message

Bough Chen Oct. 14, 2024, 6:01 a.m. UTC
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(-)

Comments

Ulf Hansson Oct. 17, 2024, 1:07 p.m. UTC | #1
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
Bough Chen Oct. 18, 2024, 1:22 a.m. UTC | #2
> -----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
Bough Chen Oct. 18, 2024, 3:20 a.m. UTC | #3
> -----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
diff mbox series

Patch

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 = {