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

Kevin Hilman
2010-02-25T00:24:03Z

"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.

>> 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