"T Krishnamoorthy, Balaji" <balajitk@xxxxxx> writes:
> On Thu, Jun 23, 2011 at 12:08 AM, Kevin Hilman <khilman@xxxxxx> wrote:
>> Balaji T K <balajitk@xxxxxx> 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@xxxxxxxxxxxxxxx/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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
