diff mbox series

[v3,2/3] mmc: sdhci-of-dwcmshc: Add runtime PM support

Message ID 1675298118-64243-3-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New
Headers show
Series Some features and fix for sdhci-of-dwcmshc | expand

Commit Message

Shawn Lin Feb. 2, 2023, 12:35 a.m. UTC
This patch adds runtime PM support.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2: None

 drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Ulf Hansson Feb. 13, 2023, 11:45 p.m. UTC | #1
On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> This patch adds runtime PM support.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2: None
>
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 46b1ce7..fc917ed 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>
>
> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>         if (err)
>                 goto err_setup_host;
>
> +       pm_runtime_get_noresume(&pdev->dev);
> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);
> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +       pm_runtime_use_autosuspend(&pdev->dev);
> +       pm_runtime_put_autosuspend(&pdev->dev);
> +
>         return 0;
>
>  err_setup_host:
> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>         if (rk_priv)
>                 clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>                                            rk_priv->rockchip_clks);
> +
> +       pm_runtime_get_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +
>         sdhci_pltfm_free(pdev);
>
>         return 0;
> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>  }
>  #endif
>
> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> +#ifdef CONFIG_PM
> +static int dwcmshc_runtime_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       u16 data;
> +       int ret;
> +
> +       ret = sdhci_runtime_suspend_host(host);
> +       if (ret)
> +               return ret;
> +
> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       data &= ~SDHCI_CLOCK_CARD_EN;
> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> +       return 0;
> +}
> +
> +static int dwcmshc_runtime_resume(struct device *dev)
> +{
> +       struct sdhci_host *host = dev_get_drvdata(dev);
> +       u16 data;
> +
> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       data |= SDHCI_CLOCK_CARD_EN;
> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> +
> +       return sdhci_runtime_resume_host(host, 0);
> +}
> +#endif
> +
> +static const struct dev_pm_ops dwcmshc_pmops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> +                               dwcmshc_resume)

I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
As sdhci_suspend_host() will write to internal registers of the IP
block, it's recommended to make sure the device's runtime resumed
before doing that call. So that needs to be added along with $subject
patch.

There is also another option that may better for you, which is to use
the pm_runtime_force_suspend|resume() helpers. There should be plenty
of references to look at, but don't hesitate to ask around that, if
you need some more help to get that working.

> +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
> +                          dwcmshc_runtime_resume, NULL)
> +};
>
>  static struct platform_driver sdhci_dwcmshc_driver = {
>         .driver = {

Kind regards
Uffe
Shawn Lin Feb. 14, 2023, 3:36 a.m. UTC | #2
Hi Ulf,

On 2023/2/14 7:45, Ulf Hansson wrote:
> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> This patch adds runtime PM support.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2: None
>>
>>   drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 46b1ce7..fc917ed 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>
>>
>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>          if (err)
>>                  goto err_setup_host;
>>
>> +       pm_runtime_get_noresume(&pdev->dev);
>> +       pm_runtime_set_active(&pdev->dev);
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>> +       pm_runtime_use_autosuspend(&pdev->dev);
>> +       pm_runtime_put_autosuspend(&pdev->dev);
>> +
>>          return 0;
>>
>>   err_setup_host:
>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>>          if (rk_priv)
>>                  clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>>                                             rk_priv->rockchip_clks);
>> +
>> +       pm_runtime_get_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +
>>          sdhci_pltfm_free(pdev);
>>
>>          return 0;
>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>>   }
>>   #endif
>>
>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>> +#ifdef CONFIG_PM
>> +static int dwcmshc_runtime_suspend(struct device *dev)
>> +{
>> +       struct sdhci_host *host = dev_get_drvdata(dev);
>> +       u16 data;
>> +       int ret;
>> +
>> +       ret = sdhci_runtime_suspend_host(host);
>> +       if (ret)
>> +               return ret;
>> +
>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> +       data &= ~SDHCI_CLOCK_CARD_EN;
>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> +       return 0;
>> +}
>> +
>> +static int dwcmshc_runtime_resume(struct device *dev)
>> +{
>> +       struct sdhci_host *host = dev_get_drvdata(dev);
>> +       u16 data;
>> +
>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> +       data |= SDHCI_CLOCK_CARD_EN;
>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>> +
>> +       return sdhci_runtime_resume_host(host, 0);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops dwcmshc_pmops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>> +                               dwcmshc_resume)
> 
> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> As sdhci_suspend_host() will write to internal registers of the IP
> block, it's recommended to make sure the device's runtime resumed
> before doing that call. So that needs to be added along with $subject
> patch.
> 

Yep, let me add a check here.

> There is also another option that may better for you, which is to use
> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> of references to look at, but don't hesitate to ask around that, if
> you need some more help to get that working.

If I understand correctly,  pm_runtime_force_suspend|resume() helpers
would use runtime PM ops for suspend/resume routine. In this case, the
main issue is the handling of clock is quite different. In suspend we
need to gate all clocks but in rpm case, it shouldn't.


> 
>> +       SET_RUNTIME_PM_OPS(dwcmshc_runtime_suspend,
>> +                          dwcmshc_runtime_resume, NULL)
>> +};
>>
>>   static struct platform_driver sdhci_dwcmshc_driver = {
>>          .driver = {
> 
> Kind regards
> Uffe
Ulf Hansson Feb. 14, 2023, 10:41 a.m. UTC | #3
On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf,
>
> On 2023/2/14 7:45, Ulf Hansson wrote:
> > On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> This patch adds runtime PM support.
> >>
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> ---
> >>
> >> Changes in v2: None
> >>
> >>   drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >> index 46b1ce7..fc917ed 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>
> >>
> >> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>          if (err)
> >>                  goto err_setup_host;
> >>
> >> +       pm_runtime_get_noresume(&pdev->dev);
> >> +       pm_runtime_set_active(&pdev->dev);
> >> +       pm_runtime_enable(&pdev->dev);
> >> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >> +       pm_runtime_use_autosuspend(&pdev->dev);
> >> +       pm_runtime_put_autosuspend(&pdev->dev);
> >> +
> >>          return 0;
> >>
> >>   err_setup_host:
> >> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >>          if (rk_priv)
> >>                  clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >>                                             rk_priv->rockchip_clks);
> >> +
> >> +       pm_runtime_get_sync(&pdev->dev);
> >> +       pm_runtime_disable(&pdev->dev);
> >> +       pm_runtime_put_noidle(&pdev->dev);
> >> +
> >>          sdhci_pltfm_free(pdev);
> >>
> >>          return 0;
> >> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >>   }
> >>   #endif
> >>
> >> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >> +#ifdef CONFIG_PM
> >> +static int dwcmshc_runtime_suspend(struct device *dev)
> >> +{
> >> +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> +       u16 data;
> >> +       int ret;
> >> +
> >> +       ret = sdhci_runtime_suspend_host(host);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> +       data &= ~SDHCI_CLOCK_CARD_EN;
> >> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static int dwcmshc_runtime_resume(struct device *dev)
> >> +{
> >> +       struct sdhci_host *host = dev_get_drvdata(dev);
> >> +       u16 data;
> >> +
> >> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >> +       data |= SDHCI_CLOCK_CARD_EN;
> >> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >> +
> >> +       return sdhci_runtime_resume_host(host, 0);
> >> +}
> >> +#endif
> >> +
> >> +static const struct dev_pm_ops dwcmshc_pmops = {
> >> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >> +                               dwcmshc_resume)
> >
> > I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> > As sdhci_suspend_host() will write to internal registers of the IP
> > block, it's recommended to make sure the device's runtime resumed
> > before doing that call. So that needs to be added along with $subject
> > patch.
> >
>
> Yep, let me add a check here.
>
> > There is also another option that may better for you, which is to use
> > the pm_runtime_force_suspend|resume() helpers. There should be plenty
> > of references to look at, but don't hesitate to ask around that, if
> > you need some more help to get that working.
>
> If I understand correctly,  pm_runtime_force_suspend|resume() helpers
> would use runtime PM ops for suspend/resume routine. In this case, the
> main issue is the handling of clock is quite different. In suspend we
> need to gate all clocks but in rpm case, it shouldn't.

I see, but let me then ask, what's the point of keeping the clocks
ungated at runtime suspend?

That sounds like wasting energy to me - but maybe there are good reasons for it?

[...]

Kind regards
Uffe
Shawn Lin Feb. 15, 2023, 12:50 a.m. UTC | #4
Hi Ulf

On 2023/2/14 18:41, Ulf Hansson wrote:
> On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> Hi Ulf,
>>
>> On 2023/2/14 7:45, Ulf Hansson wrote:
>>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>>>
>>>> This patch adds runtime PM support.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>>
>>>>    drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 50 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>>> index 46b1ce7..fc917ed 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>
>>>>
>>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>>>           if (err)
>>>>                   goto err_setup_host;
>>>>
>>>> +       pm_runtime_get_noresume(&pdev->dev);
>>>> +       pm_runtime_set_active(&pdev->dev);
>>>> +       pm_runtime_enable(&pdev->dev);
>>>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
>>>> +       pm_runtime_use_autosuspend(&pdev->dev);
>>>> +       pm_runtime_put_autosuspend(&pdev->dev);
>>>> +
>>>>           return 0;
>>>>
>>>>    err_setup_host:
>>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
>>>>           if (rk_priv)
>>>>                   clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>>>>                                              rk_priv->rockchip_clks);
>>>> +
>>>> +       pm_runtime_get_sync(&pdev->dev);
>>>> +       pm_runtime_disable(&pdev->dev);
>>>> +       pm_runtime_put_noidle(&pdev->dev);
>>>> +
>>>>           sdhci_pltfm_free(pdev);
>>>>
>>>>           return 0;
>>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
>>>>    }
>>>>    #endif
>>>>
>>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
>>>> +#ifdef CONFIG_PM
>>>> +static int dwcmshc_runtime_suspend(struct device *dev)
>>>> +{
>>>> +       struct sdhci_host *host = dev_get_drvdata(dev);
>>>> +       u16 data;
>>>> +       int ret;
>>>> +
>>>> +       ret = sdhci_runtime_suspend_host(host);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> +       data &= ~SDHCI_CLOCK_CARD_EN;
>>>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int dwcmshc_runtime_resume(struct device *dev)
>>>> +{
>>>> +       struct sdhci_host *host = dev_get_drvdata(dev);
>>>> +       u16 data;
>>>> +
>>>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> +       data |= SDHCI_CLOCK_CARD_EN;
>>>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +       return sdhci_runtime_resume_host(host, 0);
>>>> +}
>>>> +#endif
>>>> +
>>>> +static const struct dev_pm_ops dwcmshc_pmops = {
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
>>>> +                               dwcmshc_resume)
>>>
>>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
>>> As sdhci_suspend_host() will write to internal registers of the IP
>>> block, it's recommended to make sure the device's runtime resumed
>>> before doing that call. So that needs to be added along with $subject
>>> patch.
>>>
>>
>> Yep, let me add a check here.
>>
>>> There is also another option that may better for you, which is to use
>>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
>>> of references to look at, but don't hesitate to ask around that, if
>>> you need some more help to get that working.
>>
>> If I understand correctly,  pm_runtime_force_suspend|resume() helpers
>> would use runtime PM ops for suspend/resume routine. In this case, the
>> main issue is the handling of clock is quite different. In suspend we
>> need to gate all clocks but in rpm case, it shouldn't.
> 
> I see, but let me then ask, what's the point of keeping the clocks
> ungated at runtime suspend?
> 
> That sounds like wasting energy to me - but maybe there are good reasons for it?

The point to keep the clocks ungated at runtime suspend is that if they
are gated,  the DLL would lost its locked state which causes retuning
every time. It's quite painful for performance. However, if we just gate
output clock to the device, the devices(basically I refer to eMMC) will 
get into power-save status by itself. That helps us too keep balance
between power & performance during runtime. :)

> 
> [...]
> 
> Kind regards
> Uffe
Ulf Hansson Feb. 28, 2023, 11:24 a.m. UTC | #5
+ Rafael

On Wed, 15 Feb 2023 at 01:50, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Ulf
>
> On 2023/2/14 18:41, Ulf Hansson wrote:
> > On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> Hi Ulf,
> >>
> >> On 2023/2/14 7:45, Ulf Hansson wrote:
> >>> On Thu, 2 Feb 2023 at 01:35, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>>>
> >>>> This patch adds runtime PM support.
> >>>>
> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>> ---
> >>>>
> >>>> Changes in v2: None
> >>>>
> >>>>    drivers/mmc/host/sdhci-of-dwcmshc.c | 51 ++++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 50 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> >>>> index 46b1ce7..fc917ed 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>
> >>>>
> >>>> @@ -551,6 +552,13 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >>>>           if (err)
> >>>>                   goto err_setup_host;
> >>>>
> >>>> +       pm_runtime_get_noresume(&pdev->dev);
> >>>> +       pm_runtime_set_active(&pdev->dev);
> >>>> +       pm_runtime_enable(&pdev->dev);
> >>>> +       pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> >>>> +       pm_runtime_use_autosuspend(&pdev->dev);
> >>>> +       pm_runtime_put_autosuspend(&pdev->dev);
> >>>> +
> >>>>           return 0;
> >>>>
> >>>>    err_setup_host:
> >>>> @@ -580,6 +588,11 @@ static int dwcmshc_remove(struct platform_device *pdev)
> >>>>           if (rk_priv)
> >>>>                   clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> >>>>                                              rk_priv->rockchip_clks);
> >>>> +
> >>>> +       pm_runtime_get_sync(&pdev->dev);
> >>>> +       pm_runtime_disable(&pdev->dev);
> >>>> +       pm_runtime_put_noidle(&pdev->dev);
> >>>> +
> >>>>           sdhci_pltfm_free(pdev);
> >>>>
> >>>>           return 0;
> >>>> @@ -638,7 +651,43 @@ static int dwcmshc_resume(struct device *dev)
> >>>>    }
> >>>>    #endif
> >>>>
> >>>> -static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
> >>>> +#ifdef CONFIG_PM
> >>>> +static int dwcmshc_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> +       struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> +       u16 data;
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = sdhci_runtime_suspend_host(host);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>> +
> >>>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> +       data &= ~SDHCI_CLOCK_CARD_EN;
> >>>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> +       return 0;
> >>>> +}
> >>>> +
> >>>> +static int dwcmshc_runtime_resume(struct device *dev)
> >>>> +{
> >>>> +       struct sdhci_host *host = dev_get_drvdata(dev);
> >>>> +       u16 data;
> >>>> +
> >>>> +       data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> >>>> +       data |= SDHCI_CLOCK_CARD_EN;
> >>>> +       sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
> >>>> +
> >>>> +       return sdhci_runtime_resume_host(host, 0);
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> +static const struct dev_pm_ops dwcmshc_pmops = {
> >>>> +       SET_SYSTEM_SLEEP_PM_OPS(dwcmshc_suspend,
> >>>> +                               dwcmshc_resume)
> >>>
> >>> I have looked at dwcmshc_suspend(), which calls sdhci_suspend_host().
> >>> As sdhci_suspend_host() will write to internal registers of the IP
> >>> block, it's recommended to make sure the device's runtime resumed
> >>> before doing that call. So that needs to be added along with $subject
> >>> patch.
> >>>
> >>
> >> Yep, let me add a check here.
> >>
> >>> There is also another option that may better for you, which is to use
> >>> the pm_runtime_force_suspend|resume() helpers. There should be plenty
> >>> of references to look at, but don't hesitate to ask around that, if
> >>> you need some more help to get that working.
> >>
> >> If I understand correctly,  pm_runtime_force_suspend|resume() helpers
> >> would use runtime PM ops for suspend/resume routine. In this case, the
> >> main issue is the handling of clock is quite different. In suspend we
> >> need to gate all clocks but in rpm case, it shouldn't.
> >
> > I see, but let me then ask, what's the point of keeping the clocks
> > ungated at runtime suspend?
> >
> > That sounds like wasting energy to me - but maybe there are good reasons for it?
>
> The point to keep the clocks ungated at runtime suspend is that if they
> are gated,  the DLL would lost its locked state which causes retuning
> every time. It's quite painful for performance. However, if we just gate
> output clock to the device, the devices(basically I refer to eMMC) will
> get into power-save status by itself. That helps us too keep balance
> between power & performance during runtime. :)

I see, thanks for clarifying! In principle your approach should work
fine, but it depends a bit on the platform/soc too.

I assume there could also be a PM domain attached to the mmc host
device, right? If such a PM domain gets powered off, wouldn't you need
to run a retuning sequence anyway?

FYI, I have heard about similar problems for devices before - and it's
been discussed too. In principle, it sounds to me that we have devices
that would benefit from using *multiple* idle states, rather than just
the one we have for runtime suspend and the one for system wide
suspend.

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 46b1ce7..fc917ed 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>
 
@@ -551,6 +552,13 @@  static int dwcmshc_probe(struct platform_device *pdev)
 	if (err)
 		goto err_setup_host;
 
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+
 	return 0;
 
 err_setup_host:
@@ -580,6 +588,11 @@  static int dwcmshc_remove(struct platform_device *pdev)
 	if (rk_priv)
 		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
 					   rk_priv->rockchip_clks);
+
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
 	sdhci_pltfm_free(pdev);
 
 	return 0;
@@ -638,7 +651,43 @@  static int dwcmshc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dwcmshc_pmops, dwcmshc_suspend, dwcmshc_resume);
+#ifdef CONFIG_PM
+static int dwcmshc_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	u16 data;
+	int ret;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (ret)
+		return ret;
+
+	data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	data &= ~SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+	return 0;
+}
+
+static int dwcmshc_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	u16 data;
+
+	data = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	data |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, data, SDHCI_CLOCK_CONTROL);
+
+	return sdhci_runtime_resume_host(host, 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	= {