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

[Shawn Lin]

Hi Ulf

On 2023/2/14 18:41, Ulf Hansson wrote:
> On Tue, 14 Feb 2023 at 04:36, Shawn Lin <shawn.lin at 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 at rock-chips.com> wrote:
>>>>
>>>> This patch adds runtime PM support.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin at 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