FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Kevin Liu]

> From: linux-mmc-owner at vger.kernel.org [mailto:linux-mmc-owner at vger.kernel.org] On Behalf Of Linus Walleij
> Sent: Wednesday, December 12, 2012 2:54 PM
> To: Russell King - ARM Linux; Ulf Hansson
> Cc: linux-mmc at vger.kernel.org; Chris Ball; linux-arm-kernel at lists.infradead.org; Ulf Hansson
> Subject: Re: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power
>
> On Tue, Dec 11, 2012 at 3:52 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Dec 11, 2012 at 03:17:54PM +0100, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> The amba bus is already performing same actions but for the apb_pclk.
>>> So here we just make sure the clock to card is gated as well to save
>>> more power. At runtime resume we will thus restore the clock again.
>>
>> And how exactly do you ensure the requirement that a certain number of
>> clocks is supplied to the card after the last command before you cut
>> the clock?
>
> Oh, that's gonna be a problem.
>
> So I can see a quite straight-forward way to do this actually:
>
> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig)
> which means that the ios will be called with f=0 whenever
> the card is unused, taking into account the required number
> of clocks to the card.
>
> 2) Implement handling of f=0 from the ios in the ios callback.
> Then clk_disable(host->clk). I did a hack at this some months
> back but never got around to finish it, sorry for not doing
> that even though this clock gating was invented for the MMCI
> in the first place :-(
>
> 3) In the suspend() callback, sleep until the clock indicates
> that the card is declocked with something like:
> while (clk_is_enabled(host->clk)) { sleep(1) }
> However the clk framework does not have clk_is_enabled()
> so you'd either have to add that or use a local atomic
> variable host_clk_enabled set in (2).
>
> Should work, I think?
>

Since we use pm runtime with delay setting to 50ms, so there is no such problem?

Thanks
Kevin

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Ulf Hansson]

On 12 December 2012 12:39, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> From: linux-mmc-owner at vger.kernel.org [mailto:linux-mmc-owner at vger.kernel.org] On Behalf Of Linus Walleij
>> Sent: Wednesday, December 12, 2012 2:54 PM
>> To: Russell King - ARM Linux; Ulf Hansson
>> Cc: linux-mmc at vger.kernel.org; Chris Ball; linux-arm-kernel at lists.infradead.org; Ulf Hansson
>> Subject: Re: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power
>>
>> On Tue, Dec 11, 2012 at 3:52 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Tue, Dec 11, 2012 at 03:17:54PM +0100, Ulf Hansson wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> The amba bus is already performing same actions but for the apb_pclk.
>>>> So here we just make sure the clock to card is gated as well to save
>>>> more power. At runtime resume we will thus restore the clock again.
>>>
>>> And how exactly do you ensure the requirement that a certain number of
>>> clocks is supplied to the card after the last command before you cut
>>> the clock?
>>
>> Oh, that's gonna be a problem.
>>
>> So I can see a quite straight-forward way to do this actually:
>>
>> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig)
>> which means that the ios will be called with f=0 whenever
>> the card is unused, taking into account the required number
>> of clocks to the card.
>>
>> 2) Implement handling of f=0 from the ios in the ios callback.
>> Then clk_disable(host->clk). I did a hack at this some months
>> back but never got around to finish it, sorry for not doing
>> that even though this clock gating was invented for the MMCI
>> in the first place :-(
>>
>> 3) In the suspend() callback, sleep until the clock indicates
>> that the card is declocked with something like:
>> while (clk_is_enabled(host->clk)) { sleep(1) }
>> However the clk framework does not have clk_is_enabled()
>> so you'd either have to add that or use a local atomic
>> variable host_clk_enabled set in (2).
>>
>> Should work, I think?
>>
>
> Since we use pm runtime with delay setting to 50ms, so there is no such problem?

Correct!

>
> Thanks
> Kevin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


Kind regards
Ulf Hansson

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Linus Walleij]

On Wed, Dec 12, 2012 at 12:39 PM, Kevin Liu <keyuan.liu@gmail.com> wrote:

>> 3) In the suspend() callback, sleep until the clock indicates
>> that the card is declocked with something like:
>> while (clk_is_enabled(host->clk)) { sleep(1) }
>> However the clk framework does not have clk_is_enabled()
>> so you'd either have to add that or use a local atomic
>> variable host_clk_enabled set in (2).
>>
>> Should work, I think?
>
> Since we use pm runtime with delay setting to 50ms, so there is no such problem?

Depends on the clock to the card I guess?

The MMCI is default-clocked at 515633 Hz, so period
time T = 1/515633 and for 8 clock cycles on the card clock
it needs 8/515633 ~ 0.01 ms so it's pretty sufficient :-)

OK I'm in on using runtime PM then.

Yours,
Linus Walleij

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Kevin Liu]

> -----Original Message-----
> From: linux-mmc-owner at vger.kernel.org [mailto:linux-mmc-owner at vger.kernel.org] On Behalf Of Linus Walleij
> Sent: Thursday, December 13, 2012 2:54 AM
> To: Ulf Hansson
> Cc: Russell King - ARM Linux; Ulf Hansson; linux-mmc at vger.kernel.org; Chris Ball; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power
>
> On Wed, Dec 12, 2012 at 12:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 12 December 2012 07:53, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig)
>>> which means that the ios will be called with f=0 whenever
>>> the card is unused, taking into account the required number
>>> of clocks to the card.
>>
>> I think MMC_CLKGATE was a good initiative in the past. But that was
>> before runtime pm was there to use. Runtime pm suits much better for
>> handling clock gating and other runtime power save actions that could
>> be possible for a host driver to do.
>>
>> I would even suggest the MMC_CLKGATE should be removed from the
>> protocol layer, once we see that all host drivers that used it has
>> switch to runtime pm.
>
> Ah now I remember that you've actually even explained this to me ...
> memory is too short sometimes. But I follow this idea.
>
> Then we have only the coordination between runtime suspend
> and suspend proper to iron out.
>
> Russell's concern is valid if suspend() will not wait for runtime_suspend()
> to complete the last cycle before suspending.
>

pm_runtime_get_noresume is called before suspend and there are
sdhci_runtime_pm_get/sdhci_runtime_pm_put within sdhci suspend/resume
function. So the actual runtime_suspend won't be called after suspend
finished. So it's same as original code without pm_runtime during
suspend/resume.

> Hm this sounds scaringly familiar to what we've discussed with
> Kevin Hilman et al recently...
>

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Ulf Hansson]

On 13 December 2012 02:51, Kevin Liu <keyuan.liu@gmail.com> wrote:
>> -----Original Message-----
>> From: linux-mmc-owner at vger.kernel.org [mailto:linux-mmc-owner at vger.kernel.org] On Behalf Of Linus Walleij
>> Sent: Thursday, December 13, 2012 2:54 AM
>> To: Ulf Hansson
>> Cc: Russell King - ARM Linux; Ulf Hansson; linux-mmc at vger.kernel.org; Chris Ball; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power
>>
>> On Wed, Dec 12, 2012 at 12:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 12 December 2012 07:53, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>>>> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig)
>>>> which means that the ios will be called with f=0 whenever
>>>> the card is unused, taking into account the required number
>>>> of clocks to the card.
>>>
>>> I think MMC_CLKGATE was a good initiative in the past. But that was
>>> before runtime pm was there to use. Runtime pm suits much better for
>>> handling clock gating and other runtime power save actions that could
>>> be possible for a host driver to do.
>>>
>>> I would even suggest the MMC_CLKGATE should be removed from the
>>> protocol layer, once we see that all host drivers that used it has
>>> switch to runtime pm.
>>
>> Ah now I remember that you've actually even explained this to me ...
>> memory is too short sometimes. But I follow this idea.
>>
>> Then we have only the coordination between runtime suspend
>> and suspend proper to iron out.
>>
>> Russell's concern is valid if suspend() will not wait for runtime_suspend()
>> to complete the last cycle before suspending.

A valid point, thanks for pointing this out clearly Linus!

Anyway, this need to be thought of as future step, since this patch is
not changing anything to that sequence as Kevin also is pointing out.

So, the next step is to figure how we can properly do similar actions
as in the runtime_suspend callback but as part of the suspend sequence
instead and at the same time cope with 8 clock cycles. I will push a
patch for this later, so then we can discuss more about that.

>>
>
> pm_runtime_get_noresume is called before suspend and there are
> sdhci_runtime_pm_get/sdhci_runtime_pm_put within sdhci suspend/resume
> function. So the actual runtime_suspend won't be called after suspend
> finished. So it's same as original code without pm_runtime during
> suspend/resume.
>
>> Hm this sounds scaringly familiar to what we've discussed with
>> Kevin Hilman et al recently...
>>

May I have your Acks on this patch?

Kind regards
Ulf Hansson

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Kevin Liu]

2012/12/13 Ulf Hansson <ulf.hansson@linaro.org>:
> On 13 December 2012 02:51, Kevin Liu <keyuan.liu@gmail.com> wrote:
>>> -----Original Message-----
>>> From: linux-mmc-owner at vger.kernel.org [mailto:linux-mmc-owner at vger.kernel.org] On Behalf Of Linus Walleij
>>> Sent: Thursday, December 13, 2012 2:54 AM
>>> To: Ulf Hansson
>>> Cc: Russell King - ARM Linux; Ulf Hansson; linux-mmc at vger.kernel.org; Chris Ball; linux-arm-kernel at lists.infradead.org
>>> Subject: Re: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power
>>>
>>> On Wed, Dec 12, 2012 at 12:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 12 December 2012 07:53, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>
>>>>> 1) Turn on MMC_CLKGATE for this driver (select from Kconfig)
>>>>> which means that the ios will be called with f=0 whenever
>>>>> the card is unused, taking into account the required number
>>>>> of clocks to the card.
>>>>
>>>> I think MMC_CLKGATE was a good initiative in the past. But that was
>>>> before runtime pm was there to use. Runtime pm suits much better for
>>>> handling clock gating and other runtime power save actions that could
>>>> be possible for a host driver to do.
>>>>
>>>> I would even suggest the MMC_CLKGATE should be removed from the
>>>> protocol layer, once we see that all host drivers that used it has
>>>> switch to runtime pm.
>>>
>>> Ah now I remember that you've actually even explained this to me ...
>>> memory is too short sometimes. But I follow this idea.
>>>
>>> Then we have only the coordination between runtime suspend
>>> and suspend proper to iron out.
>>>
>>> Russell's concern is valid if suspend() will not wait for runtime_suspend()
>>> to complete the last cycle before suspending.
>
> A valid point, thanks for pointing this out clearly Linus!
>
> Anyway, this need to be thought of as future step, since this patch is
> not changing anything to that sequence as Kevin also is pointing out.
>
> So, the next step is to figure how we can properly do similar actions
> as in the runtime_suspend callback but as part of the suspend sequence
> instead and at the same time cope with 8 clock cycles. I will push a
> patch for this later, so then we can discuss more about that.
>
>>>
>>
>> pm_runtime_get_noresume is called before suspend and there are
>> sdhci_runtime_pm_get/sdhci_runtime_pm_put within sdhci suspend/resume
>> function. So the actual runtime_suspend won't be called after suspend
>> finished. So it's same as original code without pm_runtime during
>> suspend/resume.
>>
>>> Hm this sounds scaringly familiar to what we've discussed with
>>> Kevin Hilman et al recently...
>>>
>
> May I have your Acks on this patch?
>
Sure, my pleasure.

Acked-by: Kevin Liu <kliu5@marvell.com>

> Kind regards
> Ulf Hansson

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Russell King - ARM Linux]

On Thu, Dec 13, 2012 at 09:51:38AM +0800, Kevin Liu wrote:
> > Russell's concern is valid if suspend() will not wait for runtime_suspend()
> > to complete the last cycle before suspending.
> 
> pm_runtime_get_noresume is called before suspend and there are
> sdhci_runtime_pm_get/sdhci_runtime_pm_put within sdhci suspend/resume
> function.

What has the sdhci host driver code got to do with the MMCI host driver?

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Russell King - ARM Linux]

On Thu, Dec 13, 2012 at 10:04:46AM +0100, Ulf Hansson wrote:
> Anyway, this need to be thought of as future step, since this patch is
> not changing anything to that sequence as Kevin also is pointing out.

Given that Kevin is talking about stuff in sdhci as if it affects MMCI,
and that neither of you have picked that up, you're *still* not gaining
any confidence from me that you're actually thinking about the changes
you're making.  So I'm not really caring about how many acks your patches
get, I'm still going to be reluctant to apply them without thinking about
them for a bit.

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Ulf Hansson]

On 13 December 2012 11:11, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Dec 13, 2012 at 10:04:46AM +0100, Ulf Hansson wrote:
>> Anyway, this need to be thought of as future step, since this patch is
>> not changing anything to that sequence as Kevin also is pointing out.
>
> Given that Kevin is talking about stuff in sdhci as if it affects MMCI,
> and that neither of you have picked that up, you're *still* not gaining
> any confidence from me that you're actually thinking about the changes
> you're making.  So I'm not really caring about how many acks your patches
> get, I'm still going to be reluctant to apply them without thinking about
> them for a bit.

MMCI is doing "pm_runtime_get_sync" in it suspend. So the same
principles as Kevin is describing for sdhci goes for MMCI. Sorry if
this was not clearly pointed out.

Kind regards
Ulf Hansson

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Kevin Liu]

2012/12/13 Ulf Hansson <ulf.hansson@linaro.org>:
> On 13 December 2012 11:11, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Dec 13, 2012 at 10:04:46AM +0100, Ulf Hansson wrote:
>>> Anyway, this need to be thought of as future step, since this patch is
>>> not changing anything to that sequence as Kevin also is pointing out.
>>
>> Given that Kevin is talking about stuff in sdhci as if it affects MMCI,
>> and that neither of you have picked that up, you're *still* not gaining
>> any confidence from me that you're actually thinking about the changes
>> you're making.  So I'm not really caring about how many acks your patches
>> get, I'm still going to be reluctant to apply them without thinking about
>> them for a bit.
>
> MMCI is doing "pm_runtime_get_sync" in it suspend. So the same
> principles as Kevin is describing for sdhci goes for MMCI. Sorry if
> this was not clearly pointed out.
>
sorry for my unclear words. I just use sdhci directly to explain this.
It's similar for sdhci and mmci on this point. pm_runtime is
temporarily disabled during suspend/resume.

Thanks
Kevin

FW: [PATCH] mmc: mmci: Gate the clock in runtime suspend to save power

[Ulf Hansson]

On 13 December 2012 12:45, Kevin Liu <keyuan.liu@gmail.com> wrote:
> 2012/12/13 Ulf Hansson <ulf.hansson@linaro.org>:
>> On 13 December 2012 11:11, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Dec 13, 2012 at 10:04:46AM +0100, Ulf Hansson wrote:
>>>> Anyway, this need to be thought of as future step, since this patch is
>>>> not changing anything to that sequence as Kevin also is pointing out.
>>>
>>> Given that Kevin is talking about stuff in sdhci as if it affects MMCI,
>>> and that neither of you have picked that up, you're *still* not gaining
>>> any confidence from me that you're actually thinking about the changes
>>> you're making.  So I'm not really caring about how many acks your patches
>>> get, I'm still going to be reluctant to apply them without thinking about
>>> them for a bit.
>>
>> MMCI is doing "pm_runtime_get_sync" in it suspend. So the same
>> principles as Kevin is describing for sdhci goes for MMCI. Sorry if
>> this was not clearly pointed out.
>>
> sorry for my unclear words. I just use sdhci directly to explain this.
> It's similar for sdhci and mmci on this point. pm_runtime is
> temporarily disabled during suspend/resume.
>
> Thanks
> Kevin

Russell, is this enough for convincing you that everything has been
thought of in this patch? Can we merge this?

Kind regards
Ulf Hansson