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

[Adrian Hunter]

On 10/08/23 13:21, Ulf Hansson wrote:
> On Thu, 10 Aug 2023 at 10:13, Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxxx>
>>> Signed-off-by: Liming Sun <limings@xxxxxxxxxx>
>>> ---
>>> 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.