[v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

  This commit implements the runtime PM operations to disable eMMC
card clock when idle.

Reviewed-by: David Thompson <davthompson@nvidia.com>
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v6->v7:
    - Address Ulf's comment;
v5->v6:
    - Address Adrian's more comments and add coordination between
      runtime PM and system PM;
v4->v5:
    - Address Adrian's comment to move the pm_enable to the end to
      avoid race;
v3->v4:
    - Fix compiling reported by 'kernel test robot';
v2->v3:
    - Revise the commit message;
v1->v2:
    Updates for comments from Ulf:
    - Make the runtime PM logic generic for sdhci-of-dwcmshc;
v1: Initial version.
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 2 deletions(-)
  

On 8/08/23 23:23, Liming Sun wrote:
> This commit implements the runtime PM operations to disable eMMC
> card clock when idle.
> 
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
> v6->v7:
>     - Address Ulf's comment;
> v5->v6:
>     - Address Adrian's more comments and add coordination between
>       runtime PM and system PM;
> v4->v5:
>     - Address Adrian's comment to move the pm_enable to the end to
>       avoid race;
> v3->v4:
>     - Fix compiling reported by 'kernel test robot';
> v2->v3:
>     - Revise the commit message;
> v1->v2:
>     Updates for comments from Ulf:
>     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
> v1: Initial version.
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..c8e145031429 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  
> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  
>  	host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>  
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	err = sdhci_setup_host(host);
>  	if (err)
> -		goto err_clk;
> +		goto err_rpm;
>  
>  	if (rk_priv)
>  		dwcmshc_rk35xx_postinit(host, priv);
> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_setup_host;
>  
> +	pm_runtime_put(dev);
> +
>  	return 0;
>  
>  err_setup_host:
>  	sdhci_cleanup_host(host);
> +err_rpm:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  err_clk:
>  	clk_disable_unprepare(pltfm_host->clk);
>  	clk_disable_unprepare(priv->bus_clk);
> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret) {
> +		sdhci_resume_host(host);
> +		return ret;
> +	}

Since you are only using the runtime PM callbacks to turn off the card
clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
pm_runtime_force_resume() are not needed at all.

sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
(And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
the result is no clock either way)

sdhci_resume_host() does not restore state unless
SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
or runtime resumed.

So it just needs some comments, for example:

+/*
+ * Note, runtime suspend changes only SDHCI_CLOCK_CARD_EN which has no effect on
+ * system suspend.
+ */
 static int dwcmshc_suspend(struct device *dev)
 
+/*
+ * Note, system resume leaves SDHCI_CLOCK_INT_EN off which is consistent with
+ * either runtime suspended or runtime resumed.
+ */
 static int dwcmshc_resume(struct device *dev)


> +
>  	clk_disable_unprepare(pltfm_host->clk);
>  	if (!IS_ERR(priv->bus_clk))
>  		clk_disable_unprepare(priv->bus_clk);
> @@ -625,6 +641,10 @@ static int dwcmshc_resume(struct device *dev)
>  	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = clk_prepare_enable(pltfm_host->clk);
>  	if (ret)
>  		return ret;
> @@ -646,7 +666,55 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>  
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +		ctrl |= SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_disable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +
> +	dwcmshc_enable_card_clk(host);
> +
> +	return 0;
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend, dwcmshc_resume)
> +	SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +			   dwcmshc_runtime_resume, NULL)
> +};
>  
>  static struct platform_driver sdhci_dwcmshc_driver = {
>  	.driver	= {
  

On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/08/23 23:23, Liming Sun wrote:
> > This commit implements the runtime PM operations to disable eMMC
> > card clock when idle.
> >
> > Reviewed-by: David Thompson <davthompson@nvidia.com>
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> > v6->v7:
> >     - Address Ulf's comment;
> > v5->v6:
> >     - Address Adrian's more comments and add coordination between
> >       runtime PM and system PM;
> > v4->v5:
> >     - Address Adrian's comment to move the pm_enable to the end to
> >       avoid race;
> > v3->v4:
> >     - Fix compiling reported by 'kernel test robot';
> > v2->v3:
> >     - Revise the commit message;
> > v1->v2:
> >     Updates for comments from Ulf:
> >     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
> > v1: Initial version.
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index e68cd87998c8..c8e145031429 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
> >  #include <linux/sizes.h>
> >
> > @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >
> >       host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> >
> > +     pm_runtime_get_noresume(dev);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> >       err = sdhci_setup_host(host);
> >       if (err)
> > -             goto err_clk;
> > +             goto err_rpm;
> >
> >       if (rk_priv)
> >               dwcmshc_rk35xx_postinit(host, priv);
> > @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >       if (err)
> >               goto err_setup_host;
> >
> > +     pm_runtime_put(dev);
> > +
> >       return 0;
> >
> >  err_setup_host:
> >       sdhci_cleanup_host(host);
> > +err_rpm:
> > +     pm_runtime_disable(dev);
> > +     pm_runtime_put_noidle(dev);
> >  err_clk:
> >       clk_disable_unprepare(pltfm_host->clk);
> >       clk_disable_unprepare(priv->bus_clk);
> > @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
> >       if (ret)
> >               return ret;
> >
> > +     ret = pm_runtime_force_suspend(dev);
> > +     if (ret) {
> > +             sdhci_resume_host(host);
> > +             return ret;
> > +     }
>
> Since you are only using the runtime PM callbacks to turn off the card
> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> pm_runtime_force_resume() are not needed at all.

Right, it can be done without these too.

>
> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> the result is no clock either way)
>
> sdhci_resume_host() does not restore state unless
> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> or runtime resumed.

Even if this may work, to me, it doesn't look like good practice for
how to use runtime PM in combination with system wide suspend/resume.

The point is, sdhci_suspend|resume_host() may end up reading/writing
to sdhci registers - and we should *not* allow that (because it may
not always work), unless the sdhci controller has been runtime resumed
first, right?

[...]

Kind regards
Uffe
  

On 10/08/23 13:21, Ulf Hansson wrote:
> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 8/08/23 23:23, Liming Sun wrote:
>>> This commit implements the runtime PM operations to disable eMMC
>>> card clock when idle.
>>>
>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>>> Signed-off-by: Liming Sun <limings@nvidia.com>
>>> ---
>>> v6->v7:
>>>     - Address Ulf's comment;
>>> v5->v6:
>>>     - Address Adrian's more comments and add coordination between
>>>       runtime PM and system PM;
>>> v4->v5:
>>>     - Address Adrian's comment to move the pm_enable to the end to
>>>       avoid race;
>>> v3->v4:
>>>     - Fix compiling reported by 'kernel test robot';
>>> v2->v3:
>>>     - Revise the commit message;
>>> v1->v2:
>>>     Updates for comments from Ulf:
>>>     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
>>> v1: Initial version.
>>> ---
>>>  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
>>>  1 file changed, 70 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> index e68cd87998c8..c8e145031429 100644
>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/of_device.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include <linux/reset.h>
>>>  #include <linux/sizes.h>
>>>
>>> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>
>>>       host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>>>
>>> +     pm_runtime_get_noresume(dev);
>>> +     pm_runtime_set_active(dev);
>>> +     pm_runtime_enable(dev);
>>> +
>>>       err = sdhci_setup_host(host);
>>>       if (err)
>>> -             goto err_clk;
>>> +             goto err_rpm;
>>>
>>>       if (rk_priv)
>>>               dwcmshc_rk35xx_postinit(host, priv);
>>> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>       if (err)
>>>               goto err_setup_host;
>>>
>>> +     pm_runtime_put(dev);
>>> +
>>>       return 0;
>>>
>>>  err_setup_host:
>>>       sdhci_cleanup_host(host);
>>> +err_rpm:
>>> +     pm_runtime_disable(dev);
>>> +     pm_runtime_put_noidle(dev);
>>>  err_clk:
>>>       clk_disable_unprepare(pltfm_host->clk);
>>>       clk_disable_unprepare(priv->bus_clk);
>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
>>>       if (ret)
>>>               return ret;
>>>
>>> +     ret = pm_runtime_force_suspend(dev);
>>> +     if (ret) {
>>> +             sdhci_resume_host(host);
>>> +             return ret;
>>> +     }
>>
>> Since you are only using the runtime PM callbacks to turn off the card
>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
>> pm_runtime_force_resume() are not needed at all.
> 
> Right, it can be done without these too.
> 
>>
>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
>> the result is no clock either way)
>>
>> sdhci_resume_host() does not restore state unless
>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
>> or runtime resumed.
> 
> Even if this may work, to me, it doesn't look like good practice for
> how to use runtime PM in combination with system wide suspend/resume.
> 
> The point is, sdhci_suspend|resume_host() may end up reading/writing
> to sdhci registers - and we should *not* allow that (because it may
> not always work), unless the sdhci controller has been runtime resumed
> first, right?

I am OK with drivers that just want to use runtime PM to turn off a
functional clock.  sdhci-tegra.c is also doing that although using the
clock framework.

Certainly that approach assumes that the host controller's power state
is not changed due to runtime PM.

To ensure that the host controller is runtime resumed before calling
sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
  

On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 10/08/23 13:21, Ulf Hansson wrote:
> > On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 8/08/23 23:23, Liming Sun wrote:
> >>> This commit implements the runtime PM operations to disable eMMC
> >>> card clock when idle.
> >>>
> >>> Reviewed-by: David Thompson <davthompson@nvidia.com>
> >>> Signed-off-by: Liming Sun <limings@nvidia.com>
> >>> ---
> >>> v6->v7:
> >>>     - Address Ulf's comment;
> >>> v5->v6:
> >>>     - Address Adrian's more comments and add coordination between
> >>>       runtime PM and system PM;
> >>> v4->v5:
> >>>     - Address Adrian's comment to move the pm_enable to the end to
> >>>       avoid race;
> >>> v3->v4:
> >>>     - Fix compiling reported by 'kernel test robot';
> >>> v2->v3:
> >>>     - Revise the commit message;
> >>> v1->v2:
> >>>     Updates for comments from Ulf:
> >>>     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
> >>> v1: Initial version.
> >>> ---
> >>>  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
> >>>  1 file changed, 70 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>> index e68cd87998c8..c8e145031429 100644
> >>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>> @@ -15,6 +15,7 @@
> >>>  #include <linux/module.h>
> >>>  #include <linux/of.h>
> >>>  #include <linux/of_device.h>
> >>> +#include <linux/pm_runtime.h>
> >>>  #include <linux/reset.h>
> >>>  #include <linux/sizes.h>
> >>>
> >>> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>
> >>>       host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> >>>
> >>> +     pm_runtime_get_noresume(dev);
> >>> +     pm_runtime_set_active(dev);
> >>> +     pm_runtime_enable(dev);
> >>> +
> >>>       err = sdhci_setup_host(host);
> >>>       if (err)
> >>> -             goto err_clk;
> >>> +             goto err_rpm;
> >>>
> >>>       if (rk_priv)
> >>>               dwcmshc_rk35xx_postinit(host, priv);
> >>> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>       if (err)
> >>>               goto err_setup_host;
> >>>
> >>> +     pm_runtime_put(dev);
> >>> +
> >>>       return 0;
> >>>
> >>>  err_setup_host:
> >>>       sdhci_cleanup_host(host);
> >>> +err_rpm:
> >>> +     pm_runtime_disable(dev);
> >>> +     pm_runtime_put_noidle(dev);
> >>>  err_clk:
> >>>       clk_disable_unprepare(pltfm_host->clk);
> >>>       clk_disable_unprepare(priv->bus_clk);
> >>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> +     ret = pm_runtime_force_suspend(dev);
> >>> +     if (ret) {
> >>> +             sdhci_resume_host(host);
> >>> +             return ret;
> >>> +     }
> >>
> >> Since you are only using the runtime PM callbacks to turn off the card
> >> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> >> pm_runtime_force_resume() are not needed at all.
> >
> > Right, it can be done without these too.
> >
> >>
> >> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> >> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> >> the result is no clock either way)
> >>
> >> sdhci_resume_host() does not restore state unless
> >> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> >> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> >> or runtime resumed.
> >
> > Even if this may work, to me, it doesn't look like good practice for
> > how to use runtime PM in combination with system wide suspend/resume.
> >
> > The point is, sdhci_suspend|resume_host() may end up reading/writing
> > to sdhci registers - and we should *not* allow that (because it may
> > not always work), unless the sdhci controller has been runtime resumed
> > first, right?
>
> I am OK with drivers that just want to use runtime PM to turn off a
> functional clock.  sdhci-tegra.c is also doing that although using the
> clock framework.

Yes, I agree. At least this works for SoC specific drivers.

>
> Certainly that approach assumes that the host controller's power state
> is not changed due to runtime PM.
>
> To ensure that the host controller is runtime resumed before calling
> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.

Yes, that was kind of what I proposed in the other thread as option 1)
(except for the replacement of pm_runtime_force_suspend|resume).

Although, to be clear I would probably use pm_runtime_get_sync()
instead, to make sure the usage count is incremented too.

I don't have a strong opinion here, but from an optimization point of
view I would at least consider what I proposed in option 2) (in the
other thread). The benefit is that it can allow us to potentially
avoid runtime resuming the device, during system suspend.

Kind regards
Uffe
  

On 10/08/23 19:34, Ulf Hansson wrote:
> On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 10/08/23 13:21, Ulf Hansson wrote:
>>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 8/08/23 23:23, Liming Sun wrote:
>>>>> This commit implements the runtime PM operations to disable eMMC
>>>>> card clock when idle.
>>>>>
>>>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
>>>>> Signed-off-by: Liming Sun <limings@nvidia.com>
>>>>> ---
>>>>> v6->v7:
>>>>>     - Address Ulf's comment;
>>>>> v5->v6:
>>>>>     - Address Adrian's more comments and add coordination between
>>>>>       runtime PM and system PM;
>>>>> v4->v5:
>>>>>     - Address Adrian's comment to move the pm_enable to the end to
>>>>>       avoid race;
>>>>> v3->v4:
>>>>>     - Fix compiling reported by 'kernel test robot';
>>>>> v2->v3:
>>>>>     - Revise the commit message;
>>>>> v1->v2:
>>>>>     Updates for comments from Ulf:
>>>>>     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
>>>>> v1: Initial version.
>>>>> ---
>>>>>  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
>>>>>  1 file changed, 70 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>>> index e68cd87998c8..c8e145031429 100644
>>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>>> @@ -15,6 +15,7 @@
>>>>>  #include <linux/module.h>
>>>>>  #include <linux/of.h>
>>>>>  #include <linux/of_device.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>>  #include <linux/reset.h>
>>>>>  #include <linux/sizes.h>
>>>>>
>>>>> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>>>
>>>>>       host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>>>>>
>>>>> +     pm_runtime_get_noresume(dev);
>>>>> +     pm_runtime_set_active(dev);
>>>>> +     pm_runtime_enable(dev);
>>>>> +
>>>>>       err = sdhci_setup_host(host);
>>>>>       if (err)
>>>>> -             goto err_clk;
>>>>> +             goto err_rpm;
>>>>>
>>>>>       if (rk_priv)
>>>>>               dwcmshc_rk35xx_postinit(host, priv);
>>>>> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>>>       if (err)
>>>>>               goto err_setup_host;
>>>>>
>>>>> +     pm_runtime_put(dev);
>>>>> +
>>>>>       return 0;
>>>>>
>>>>>  err_setup_host:
>>>>>       sdhci_cleanup_host(host);
>>>>> +err_rpm:
>>>>> +     pm_runtime_disable(dev);
>>>>> +     pm_runtime_put_noidle(dev);
>>>>>  err_clk:
>>>>>       clk_disable_unprepare(pltfm_host->clk);
>>>>>       clk_disable_unprepare(priv->bus_clk);
>>>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
>>>>>       if (ret)
>>>>>               return ret;
>>>>>
>>>>> +     ret = pm_runtime_force_suspend(dev);
>>>>> +     if (ret) {
>>>>> +             sdhci_resume_host(host);
>>>>> +             return ret;
>>>>> +     }
>>>>
>>>> Since you are only using the runtime PM callbacks to turn off the card
>>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
>>>> pm_runtime_force_resume() are not needed at all.
>>>
>>> Right, it can be done without these too.
>>>
>>>>
>>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
>>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
>>>> the result is no clock either way)
>>>>
>>>> sdhci_resume_host() does not restore state unless
>>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
>>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
>>>> or runtime resumed.
>>>
>>> Even if this may work, to me, it doesn't look like good practice for
>>> how to use runtime PM in combination with system wide suspend/resume.
>>>
>>> The point is, sdhci_suspend|resume_host() may end up reading/writing
>>> to sdhci registers - and we should *not* allow that (because it may
>>> not always work), unless the sdhci controller has been runtime resumed
>>> first, right?
>>
>> I am OK with drivers that just want to use runtime PM to turn off a
>> functional clock.  sdhci-tegra.c is also doing that although using the
>> clock framework.
> 
> Yes, I agree. At least this works for SoC specific drivers.
> 
>>
>> Certainly that approach assumes that the host controller's power state
>> is not changed due to runtime PM.
>>
>> To ensure that the host controller is runtime resumed before calling
>> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> 
> Yes, that was kind of what I proposed in the other thread as option 1)
> (except for the replacement of pm_runtime_force_suspend|resume).
> 
> Although, to be clear I would probably use pm_runtime_get_sync()
> instead, to make sure the usage count is incremented too.

In that case, a matching pm_runtime_put() is needed also at the
end of the resume callback.

> 
> I don't have a strong opinion here, but from an optimization point of
> view I would at least consider what I proposed in option 2) (in the
> other thread). The benefit is that it can allow us to potentially
> avoid runtime resuming the device, during system suspend.
> 
> Kind regards
> Uffe
  

On Fri, 11 Aug 2023 at 07:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 10/08/23 19:34, Ulf Hansson wrote:
> > On Thu, 10 Aug 2023 at 14:44, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 10/08/23 13:21, Ulf Hansson wrote:
> >>> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>
> >>>> On 8/08/23 23:23, Liming Sun wrote:
> >>>>> This commit implements the runtime PM operations to disable eMMC
> >>>>> card clock when idle.
> >>>>>
> >>>>> Reviewed-by: David Thompson <davthompson@nvidia.com>
> >>>>> Signed-off-by: Liming Sun <limings@nvidia.com>
> >>>>> ---
> >>>>> v6->v7:
> >>>>>     - Address Ulf's comment;
> >>>>> v5->v6:
> >>>>>     - Address Adrian's more comments and add coordination between
> >>>>>       runtime PM and system PM;
> >>>>> v4->v5:
> >>>>>     - Address Adrian's comment to move the pm_enable to the end to
> >>>>>       avoid race;
> >>>>> v3->v4:
> >>>>>     - Fix compiling reported by 'kernel test robot';
> >>>>> v2->v3:
> >>>>>     - Revise the commit message;
> >>>>> v1->v2:
> >>>>>     Updates for comments from Ulf:
> >>>>>     - Make the runtime PM logic generic for sdhci-of-dwcmshc;
> >>>>> v1: Initial version.
> >>>>> ---
> >>>>>  drivers/mmc/host/sdhci-of-dwcmshc.c | 72 ++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 70 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>>> index e68cd87998c8..c8e145031429 100644
> >>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>>> @@ -15,6 +15,7 @@
> >>>>>  #include <linux/module.h>
> >>>>>  #include <linux/of.h>
> >>>>>  #include <linux/of_device.h>
> >>>>> +#include <linux/pm_runtime.h>
> >>>>>  #include <linux/reset.h>
> >>>>>  #include <linux/sizes.h>
> >>>>>
> >>>>> @@ -548,9 +549,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>>>
> >>>>>       host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> >>>>>
> >>>>> +     pm_runtime_get_noresume(dev);
> >>>>> +     pm_runtime_set_active(dev);
> >>>>> +     pm_runtime_enable(dev);
> >>>>> +
> >>>>>       err = sdhci_setup_host(host);
> >>>>>       if (err)
> >>>>> -             goto err_clk;
> >>>>> +             goto err_rpm;
> >>>>>
> >>>>>       if (rk_priv)
> >>>>>               dwcmshc_rk35xx_postinit(host, priv);
> >>>>> @@ -559,10 +564,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>>>       if (err)
> >>>>>               goto err_setup_host;
> >>>>>
> >>>>> +     pm_runtime_put(dev);
> >>>>> +
> >>>>>       return 0;
> >>>>>
> >>>>>  err_setup_host:
> >>>>>       sdhci_cleanup_host(host);
> >>>>> +err_rpm:
> >>>>> +     pm_runtime_disable(dev);
> >>>>> +     pm_runtime_put_noidle(dev);
> >>>>>  err_clk:
> >>>>>       clk_disable_unprepare(pltfm_host->clk);
> >>>>>       clk_disable_unprepare(priv->bus_clk);
> >>>>> @@ -606,6 +616,12 @@ static int dwcmshc_suspend(struct device *dev)
> >>>>>       if (ret)
> >>>>>               return ret;
> >>>>>
> >>>>> +     ret = pm_runtime_force_suspend(dev);
> >>>>> +     if (ret) {
> >>>>> +             sdhci_resume_host(host);
> >>>>> +             return ret;
> >>>>> +     }
> >>>>
> >>>> Since you are only using the runtime PM callbacks to turn off the card
> >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and
> >>>> pm_runtime_force_resume() are not needed at all.
> >>>
> >>> Right, it can be done without these too.
> >>>
> >>>>
> >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or off.
> >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so presumably
> >>>> the result is no clock either way)
> >>>>
> >>>> sdhci_resume_host() does not restore state unless
> >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the internal clock
> >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime suspended
> >>>> or runtime resumed.
> >>>
> >>> Even if this may work, to me, it doesn't look like good practice for
> >>> how to use runtime PM in combination with system wide suspend/resume.
> >>>
> >>> The point is, sdhci_suspend|resume_host() may end up reading/writing
> >>> to sdhci registers - and we should *not* allow that (because it may
> >>> not always work), unless the sdhci controller has been runtime resumed
> >>> first, right?
> >>
> >> I am OK with drivers that just want to use runtime PM to turn off a
> >> functional clock.  sdhci-tegra.c is also doing that although using the
> >> clock framework.
> >
> > Yes, I agree. At least this works for SoC specific drivers.
> >
> >>
> >> Certainly that approach assumes that the host controller's power state
> >> is not changed due to runtime PM.
> >>
> >> To ensure that the host controller is runtime resumed before calling
> >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think.
> >
> > Yes, that was kind of what I proposed in the other thread as option 1)
> > (except for the replacement of pm_runtime_force_suspend|resume).
> >
> > Although, to be clear I would probably use pm_runtime_get_sync()
> > instead, to make sure the usage count is incremented too.
>
> In that case, a matching pm_runtime_put() is needed also at the
> end of the resume callback.

Yes, of course. Or depending if we are using the force_suspend|resume
helper, a pm_runtime_put_noidle is sufficient after
pm_runtime_force_suspend() has been called.

[...]

Kind regards
Uffe