Re: [PATCH 2/3] MMC: OMAP: HSMMC: add runtime pm support

[Kevin Hilman <khilman@ti.com>]

"T Krishnamoorthy, Balaji" <balajitk@ti.com> writes:

> On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Balaji T K <balajitk@ti.com> writes:
>>
>
>>> @@ -1880,18 +1873,12 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>>
>>>       mmc->caps |= MMC_CAP_DISABLE;
>>>
>>> -     if (clk_enable(host->iclk) != 0) {
>>> -             clk_put(host->iclk);
>>> -             clk_put(host->fclk);
>>> -             goto err1;
>>> -     }
>>> -
>>> -     if (mmc_host_enable(host->mmc) != 0) {
>>> -             clk_disable(host->iclk);
>>> -             clk_put(host->iclk);
>>> -             clk_put(host->fclk);
>>> -             goto err1;
>>> -     }
>>> +     pm_runtime_enable(host->dev);
>>> +     pm_runtime_allow(host->dev);
>>> +     pm_runtime_get_sync(host->dev);
>>> +     pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
>>> +     pm_runtime_use_autosuspend(host->dev);
>>> +     pm_suspend_ignore_children(host->dev, 1);
>>
>> Why is ignore_children needed for this device?   Is this device the
>> parent of other devices?   If it is, why should it ignore it's
>> children?
>>
>
> No, I will remove. Added it for testing only.
>
>>>       if (cpu_is_omap2430()) {
>>>               host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
>>> @@ -2018,6 +2005,8 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>>>       }
>>>
>>>       omap_hsmmc_debugfs(mmc);
>>> +     pm_runtime_mark_last_busy(host->dev);
>>> +     pm_runtime_put_autosuspend(host->dev);
>>>
>>>       return 0;
>>>
>>> @@ -2033,8 +2022,8 @@ err_reg:
>>>  err_irq_cd_init:
>>>       free_irq(host->irq, host);
>>>  err_irq:
>>> -     mmc_host_disable(host->mmc);
>>> -     clk_disable(host->iclk);
>>> +     pm_runtime_mark_last_busy(host->dev);
>>> +     pm_runtime_put_autosuspend(host->dev);
>>>       clk_put(host->fclk);
>>>       clk_put(host->iclk);
>>>       if (host->got_dbclk) {
>>> @@ -2058,7 +2047,7 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>>       struct resource *res;
>>>
>>>       if (host) {
>>> -             mmc_host_enable(host->mmc);
>>> +             pm_runtime_get_sync(host->dev);
>>>               mmc_remove_host(host->mmc);
>>>               if (host->use_reg)
>>>                       omap_hsmmc_reg_put(host);
>>> @@ -2069,8 +2058,9 @@ static int omap_hsmmc_remove(struct platform_device *pdev)
>>>                       free_irq(mmc_slot(host).card_detect_irq, host);
>>>               flush_work_sync(&host->mmc_carddetect_work);
>>>
>>> -             mmc_host_disable(host->mmc);
>>> -             clk_disable(host->iclk);
>>> +             pm_runtime_put_sync(host->dev);
>>> +             pm_runtime_forbid(host->dev);
>>
>> Why?
>>
>
> Added for balancing pm_runtime_allow added in  _probe.
> But forbid also resume the device on remove.
> Should this be removed, keeping _allow in _probe ?

Neither the _allow or _forbid are needed,   _enable and _disable are enough.

>>> +             pm_runtime_disable(host->dev);
>>>               clk_put(host->fclk);
>>>               clk_put(host->iclk);
>>>               if (host->got_dbclk) {
>>> @@ -2102,6 +2092,8 @@ static int omap_hsmmc_suspend(struct device *dev)
>>>               return 0;
>>>
>>>       if (host) {
>>> +             /* FIXME: TODO move get_sync to proper dev_pm_ops function */
>>
>> what does this mean?
>
> get_sync is needed to enable clock before accessing the registers but
> the discusssion @
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg50819.html
> suggested to move runtime get_sync calls to .prepare
> Haven't tried it yet.

The _get is fine here, it's the _put that may be the problem.

Based on that thread you mentioned, it is the using of _put and
_put_sync in the suspend path that is the problem.  Basically, use of
runtime PM calls in the suspend/resume path is not recommended and not
guaranteed to work.   It currently works on OMAP, but I may have to
change this.

For now, what is certain is that runtime PM calls in the suspend
callbacks must be the _sync versions.  I'm still working on how to
properly implement the PM domain part for OMAP to correctly implement
the restrictions that the linux-pm maintainers want to enforce.

Kevin