Re: [PATCH/RFC 0/4] convert HS-MMC driver to hwmod + runtime PM

[Adrian Hunter <adrian.hunter@nokia.com>]

Kevin Hilman wrote:
> "Madhusudhan" <madhu.cr@ti.com> writes:
> 
>>> -----Original Message-----
>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>> owner@vger.kernel.org] On Behalf Of Kevin Hilman
>>> Sent: Tuesday, February 23, 2010 6:51 PM
>>> To: linux-omap@vger.kernel.org
>>> Subject: Re: [PATCH/RFC 0/4] convert HS-MMC driver to hwmod + runtime PM
>>>
>>> Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>
>>>> This series converts the OMAP HS-MMC driver to use omap_hwmod +
>>>> runtime PM API.
>>>>
>>>> Depends on MMC hwmods available in 'pm-wip/hwmods' branch of
>>>> my git tree[1] as well as previously posted runtime PM series:
>>>>
>>>>    [PATCH/RFC 0/2] initial runtime PM layer for OMAP
>>>>
>>>> The easies way to experiment/test is to use my 'pm-wip/mmc' branch
>>>> which has all the dependencies, and is based on omap/for-next'.
>>>> It has been tested by merging with current PM branch.
>>>>
>>>> [1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git
>>>>
>>> A question for those of you who actually understand the MMC driver...
>>> I'm having problems getting my head around the current PM stuff in the
>>> MMC driver.  My primary question is:
>>>
>>> Why does the suspend hook need to re-enable the device before
>>> suspending?
>>>
>> In the scenario where there is no activity on the bus the MMC clocks are
>> kept disabled. Now in the suspend path the MMC core will issue certain CMDs
>> like CMD7(to end the suspend path) to deselect the card(more of a protocol
>> stuff). Hence the host need to be in enabled state before letting the core
>> know that there is a suspend request.
> 
> I guess what I'm wondering is whether we can (should) have the exact
> same path for runtime PM (idle) and static PM (suspend/resume.)
> 
> My current approach is that the suspend is a NOP if the device
> is already runtime suspended.

One difference is that the card-detect irq is disabled during suspend.
That means that removeable cards must have their block devices removed
too (or not if "unsafe-resume" is configured). The advantage of OFF-mode
is that it will wake-up if someone opens the cover, whereas suspend
will not.

A similar issue may develop with SDIO which may want to wake-up the
device from OFF-mode (e.g. wireless card connected via SDIO)

So is the difference that OFF-mode has wakeups but suspend does not?

> 
>>> When using runtime PM, the MMC device is disabled including
>>> clocks off  and regulator off (if power_savings == true) when there
>>> is no activity.
>>>
>>> Then, in the static suspend hook, it's re-enabled (including taking it out
>>> of
>>> off, re-enabling regulators etc) only to be quickly disabled again.
>>> This seems horribly inefficient.
>>>
>> This is exactly for the reason I mentioned above.
>>
>>> I admit to not understanding the MMC layer terribly well, so can someone
>>> enlighten me as to what is going on here?
>>>
>>> What I am testing here is a patch on top of this series (below) that
>>> adds a check to the static suspend hook.  If the device is already
>>> runtime suspended, then the suspend and resume hooks should be noop.
>>>
>>> This appears to work just fine while testing on omap3evm just doing
>>> simple read/write tests before an after suspend resume.
>>>
>> I did some basic testing with your previously posted patches. But my testing
>> was incomplete because on Zoom2 because for some reason the OFF mode was not
>> working even without your patches.
> 
> OK, I will try on Zoom2.  I've only tested this so far on OMAP3EVM.
> 
>> My concern was more with respect to OFF mode in idle path since your patches
>> removed context restore calls if I recall correctly. 
> 
> No, I didn't remove restore.  I just moved it into the runtime PM
> resume callback.
> 
>> Are you able to hit CORE OFF and then come back and do the
>> read/write transfers in idle as well as suspend/resume path?
> 
> Yes.
> 
> Kevin
> 
>>  
>>> Note that if you want to test this patch, it also depends on this
>>> patch to runtime PM from the linux-pm list:
>>> https://lists.linux-foundation.org/pipermail/linux-pm/2010-
>>> February/024275.html
>>>
>>> These are all included in an updated version of my pm-wip/mmc branch
>>> for ease of testing.  Merge it with the current PM branch, enable
>>> CONFIG_PM_RUNTIME and test away.
>>>
>>> Kevin
>>>
>>> commit 166cba7679fa267ee6e6eb39fd1e871ede5ded16
>>> Author: Kevin Hilman <khilman@deeprootsystems.com>
>>> Date:   Tue Feb 23 16:21:56 2010 -0800
>>>
>>>     MMC: omap_hsmmc: check for runtime-suspend in static suspend
>>>
>>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>>> index 16d66b9..dd027bb 100644
>>> --- a/drivers/mmc/host/omap_hsmmc.c
>>> +++ b/drivers/mmc/host/omap_hsmmc.c
>>> @@ -2222,6 +2222,9 @@ static int omap_hsmmc_suspend(struct device *dev)
>>>  	if (host && host->suspended)
>>>  		return 0;
>>>
>>> +	if (pm_is_runtime_suspended(host->dev))
>>> +		return 0;
>>> +
>>>  	if (host) {
>>>  		host->suspended = 1;
>>>  		if (host->pdata->suspend) {
>>> @@ -2260,12 +2263,6 @@ static int omap_hsmmc_suspend(struct device *dev)
>>>  			}
>>>  			mmc_host_disable(host->mmc);
>>>  		}
>>> -
>>> -		/*
>>> -		 * HACK: "extra" put to compensate for DPM core keeping
>>> -		 *       runtime PM disabled.  -- khilman
>>> -		 */
>>> -		pm_runtime_put_sync(host->dev);
>>>  	}
>>>  	return ret;
>>>  }
>>> @@ -2280,13 +2277,10 @@ static int omap_hsmmc_resume(struct device *dev)
>>>  	if (host && !host->suspended)
>>>  		return 0;
>>>
>>> -	if (host) {
>>> -		/*
>>> -		 * HACK: "extra" get to compensate for DPM core keeping
>>> -		 *       runtime PM disabled.  -- khilman
>>> -		 */
>>> -		pm_runtime_get_sync(host->dev);
>>> +	if (pm_is_runtime_suspended(host->dev))
>>> +		return 0;
>>>
>>> +	if (host) {
>>>  		if (mmc_host_enable(host->mmc) != 0) {
>>>  			pm_runtime_suspend(host->dev);
>>>  			goto clk_en_err;
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>