All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] mmc: agressive clocking framework v9
@ 2010-11-06  5:38 Philip Rakity
  2010-11-06 17:24 ` Ohad Ben-Cohen
  2010-11-08  9:23 ` Linus Walleij
  0 siblings, 2 replies; 25+ messages in thread
From: Philip Rakity @ 2010-11-06  5:38 UTC (permalink / raw)
  To: linus.Walleij; +Cc: linux-mmc


Linus,

One case that I do not see handled is support of hardware clock gating.

SD 3.0 has this as well as our v2.0 controller.

What I think makes sense is (from discussion with Nico on the mailing list)

a) add a quirk that the driver can set to indicate h/w support for clock gating is available
b) add a new field to ios that indicates if hardware clock gating to be enabled / disabled
c) clock gating is set to disabled until final speed negotiation during card detection after which it may be enabled.
d) keep sdio clock gating OFF -- need this from our experience.

I can try to add this to your patch if you like or perhaps you can do this in the next submission

regards,

Philip


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-06  5:38 [PATCH] mmc: agressive clocking framework v9 Philip Rakity
@ 2010-11-06 17:24 ` Ohad Ben-Cohen
  2010-11-06 17:38   ` Philip Rakity
  2010-11-08  9:23 ` Linus Walleij
  1 sibling, 1 reply; 25+ messages in thread
From: Ohad Ben-Cohen @ 2010-11-06 17:24 UTC (permalink / raw)
  To: Philip Rakity; +Cc: linus.Walleij, linux-mmc

Hi Phillip,

On Sat, Nov 6, 2010 at 1:38 AM, Philip Rakity <prakity@marvell.com> wrote:
> d) keep sdio clock gating OFF -- need this from our experience.

I'm really interested to hear more about that one - what issues did you see ?
Can you please elaborate ?

Thanks,
Ohad.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-06 17:24 ` Ohad Ben-Cohen
@ 2010-11-06 17:38   ` Philip Rakity
  2010-11-07  1:44     ` Nicolas Pitre
  2010-11-07  9:32     ` Ohad Ben-Cohen
  0 siblings, 2 replies; 25+ messages in thread
From: Philip Rakity @ 2010-11-06 17:38 UTC (permalink / raw)
  To: Ohad Ben-Cohen; +Cc: linus.Walleij, linux-mmc


We need a way to specify

a) Hardware can support clock gating
setting the clock to 0 in the call to set_ios (with hardware clock gating enabled) becomes a no-op so some care is needed which is why I suggested another field in the ios structure.

When clock gating is enabled with SDIO cards we have seen 
a) Missed Interrupts
b) Transactions not completed

We think this is because some SDIO cards need n clocks after the xfer completes from the host point of view to be in a state to handle clocks off.  n clocks should be 8 for SD 3.0 but in sometimes the cards need more.  We have seen SDIO cards work with no clocks needed and some needing more then 12 clocks.  This is completely understandable considering that clock gating is a new feature.  


b) hardware doing clock gating with SDIO is BAD -- we see the same issues that s/w clock gating patch see's ..  The sdio cards do not work correctly.

c) The quirk is needed to tell the core/ layer that h/w support is available.

On Nov 6, 2010, at 10:24 AM, Ohad Ben-Cohen wrote:

> Hi Phillip,
> 
> On Sat, Nov 6, 2010 at 1:38 AM, Philip Rakity <prakity@marvell.com> wrote:
>> d) keep sdio clock gating OFF -- need this from our experience.
> 
> I'm really interested to hear more about that one - what issues did you see ?
> Can you please elaborate ?
> 
> Thanks,
> Ohad.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-06 17:38   ` Philip Rakity
@ 2010-11-07  1:44     ` Nicolas Pitre
  2010-11-07 18:18       ` Philip Rakity
  2010-11-07  9:32     ` Ohad Ben-Cohen
  1 sibling, 1 reply; 25+ messages in thread
From: Nicolas Pitre @ 2010-11-07  1:44 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Ohad Ben-Cohen, linus.Walleij, linux-mmc

On Sat, 6 Nov 2010, Philip Rakity wrote:

> 
> We need a way to specify
> 
> a) Hardware can support clock gating setting the clock to 0 in the 
> call to set_ios (with hardware clock gating enabled) becomes a no-op 
> so some care is needed which is why I suggested another field in the 
> ios structure.

Why should the core code care?

What difference is there with the core telling the controller driver to 
set the clock to a frequency of 0 ?


Nicolas

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-06 17:38   ` Philip Rakity
  2010-11-07  1:44     ` Nicolas Pitre
@ 2010-11-07  9:32     ` Ohad Ben-Cohen
  2010-11-08  9:37       ` Linus Walleij
  1 sibling, 1 reply; 25+ messages in thread
From: Ohad Ben-Cohen @ 2010-11-07  9:32 UTC (permalink / raw)
  To: Philip Rakity; +Cc: linus.Walleij, linux-mmc, David Vrabel

On Sat, Nov 6, 2010 at 7:38 PM, Philip Rakity <prakity@marvell.com> wrote:
> When clock gating is enabled with SDIO cards we have seen
> a) Missed Interrupts
> b) Transactions not completed

As David suggested, these issues are probably card-specific.

The 128x wlan card, e.g., supports both synchronous and asynchronous
SDIO interrupts (and also external non-SDIO interrupts).

So the decision whether to enable clock gating for SDIO should not be
static (eventually. I'm not saying this is something Linus should take
care of in his patch, it's up to us to make this work). By default,
SDIO clock gating should be disabled, but we need to allow drivers to
explicitly enable it if supported.

I'm tempted to suggest that drivers should control this too using
runtime PM API, but for that we need PM core to support several
different levels of runtime suspended states (and not just
active/suspended). It might be a good use case to push runtime PM core
for that.

We would then have three power states:

* Active (powered on; clocks ungated)
* Gated (powered on; clocks gated)
* Suspended (powered off)

>
> We think this is because some SDIO cards need n clocks after the xfer completes from the host point of view to be in a state to handle clocks off.  n clocks should be 8 for SD 3.0 but in sometimes the cards need more.  We have seen SDIO cards work with no clocks needed and some needing more then 12 clocks.  This is completely understandable considering that clock gating is a new feature.
>
>
> b) hardware doing clock gating with SDIO is BAD -- we see the same issues that s/w clock gating patch see's ..  The sdio cards do not work correctly.
>
> c) The quirk is needed to tell the core/ layer that h/w support is available.
>
> On Nov 6, 2010, at 10:24 AM, Ohad Ben-Cohen wrote:
>
>> Hi Phillip,
>>
>> On Sat, Nov 6, 2010 at 1:38 AM, Philip Rakity <prakity@marvell.com> wrote:
>>> d) keep sdio clock gating OFF -- need this from our experience.
>>
>> I'm really interested to hear more about that one - what issues did you see ?
>> Can you please elaborate ?
>>
>> Thanks,
>> Ohad.
>
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-07  1:44     ` Nicolas Pitre
@ 2010-11-07 18:18       ` Philip Rakity
  2010-11-08  4:33         ` Nicolas Pitre
  2010-11-08  9:40         ` Linus Walleij
  0 siblings, 2 replies; 25+ messages in thread
From: Philip Rakity @ 2010-11-07 18:18 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Ohad Ben-Cohen, linus.Walleij, linux-mmc


On Nov 6, 2010, at 6:44 PM, Nicolas Pitre wrote:

> On Sat, 6 Nov 2010, Philip Rakity wrote:
> 
>> 
>> We need a way to specify
>> 
>> a) Hardware can support clock gating setting the clock to 0 in the 
>> call to set_ios (with hardware clock gating enabled) becomes a no-op 
>> so some care is needed which is why I suggested another field in the 
>> ios structure.
> 
> Why should the core code care?
> 
> What difference is there with the core telling the controller driver to 
> set the clock to a frequency of 0 ?
> 
> 
> Nicolas


When h/w clock gating is enabled then sd clocks are disabled/enabled by the sd controller; there is nothing for the core/ layer to do.

I am concerned that having the core/ layer ask for clocks off and on is

a) not needed when h/w clock gating is enabled
b) may invoke bad behavior in the controller
c) is not optimal since the h/w is already handling this. 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-07 18:18       ` Philip Rakity
@ 2010-11-08  4:33         ` Nicolas Pitre
  2010-11-08  9:40         ` Linus Walleij
  1 sibling, 0 replies; 25+ messages in thread
From: Nicolas Pitre @ 2010-11-08  4:33 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Ohad Ben-Cohen, linus.Walleij, linux-mmc

On Sun, 7 Nov 2010, Philip Rakity wrote:

> 
> On Nov 6, 2010, at 6:44 PM, Nicolas Pitre wrote:
> 
> > On Sat, 6 Nov 2010, Philip Rakity wrote:
> > 
> >> 
> >> We need a way to specify
> >> 
> >> a) Hardware can support clock gating setting the clock to 0 in the 
> >> call to set_ios (with hardware clock gating enabled) becomes a no-op 
> >> so some care is needed which is why I suggested another field in the 
> >> ios structure.
> > 
> > Why should the core code care?
> > 
> > What difference is there with the core telling the controller driver to 
> > set the clock to a frequency of 0 ?
> 
> When h/w clock gating is enabled then sd clocks are disabled/enabled 
> by the sd controller; there is nothing for the core/ layer to do.

OK, then that could be an optimization.

> I am concerned that having the core/ layer ask for clocks off and on 
> is
> 
> a) not needed when h/w clock gating is enabled

Sure, but right now it shouldn't be enabled just yet.  Making sure that 
the software version works well should be done first.

> b) may invoke bad behavior in the controller

That would be a really bad controller.

> c) is not optimal since the h/w is already handling this. --

Agreed.  But again let's make sure that the software version works 
first.  Then adding support for hardware clock gating would be really 
simple.


Nicolas

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-06  5:38 [PATCH] mmc: agressive clocking framework v9 Philip Rakity
  2010-11-06 17:24 ` Ohad Ben-Cohen
@ 2010-11-08  9:23 ` Linus Walleij
  1 sibling, 0 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-08  9:23 UTC (permalink / raw)
  To: Philip Rakity; +Cc: linux-mmc

Philip Rakity wrote:

> One case that I do not see handled is support of hardware clock gating.
> SD 3.0 has this as well as our v2.0 controller.

> a) add a quirk that the driver can set to indicate h/w support for clock gating is available

Right now there is a Kconfig option, that compiles out all of the
software clock gating. This sounds more like we shouldn't do
it that way: instead always compile it in and then use a new
host flag to indicate whether to activate it or not.

Is this what you mean?

> b) add a new field to ios that indicates if hardware clock gating to be enabled / disabled
> c) clock gating is set to disabled until final speed negotiation during card detection after which it may be enabled.

This seems like orthogonal, but realated stuff that can be
added on top.

> d) keep sdio clock gating OFF -- need this from our experience.

This was added already in the v8 variant of this patchset, for
the SW-controlled gating.

> I can try to add this to your patch if you like or perhaps you
> can do this in the next submission

OK, seems like a good idea.

I can't test it with your hardware anyway.

But base your work on the v8 patch, this v9 using runtime PM is
not going to work. v10 will move back to the v8 approach.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-07  9:32     ` Ohad Ben-Cohen
@ 2010-11-08  9:37       ` Linus Walleij
  2010-11-08  9:47         ` Philip Rakity
  2010-11-08 18:47         ` Nicolas Pitre
  0 siblings, 2 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-08  9:37 UTC (permalink / raw)
  To: Ohad Ben-Cohen; +Cc: Philip Rakity, linux-mmc, David Vrabel

Ohad Ben-Cohen wrote:

> So the decision whether to enable clock gating for SDIO should not be
> static (eventually. I'm not saying this is something Linus should take
> care of in his patch, it's up to us to make this work). By default,
> SDIO clock gating should be disabled, but we need to allow drivers to
> explicitly enable it if supported.

I currently (patch v8) disable gating for all SDIO cards,
but I guess there are two approaches to cases where you'd
like to enable it:

- Card (including SDIO device) whitelisting.

- Host whitelisting - for the case where as gateable SDIO
  device is soldered to the bus.

The first looks like big-ish so would be a separate patch, but a
host flag for the latter like MMC_CAP_GATE_SDIO can easily be
folded into a v10 of this patch.

Is that desireable?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-07 18:18       ` Philip Rakity
  2010-11-08  4:33         ` Nicolas Pitre
@ 2010-11-08  9:40         ` Linus Walleij
  2010-11-08  9:45           ` Philip Rakity
  2010-11-09 10:29           ` Jaehoon Chung
  1 sibling, 2 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-08  9:40 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Nicolas Pitre, Ohad Ben-Cohen, linux-mmc

Philip Rakity wrote:

> I am concerned that having the core/ layer ask for clocks off and on is
> 
> a) not needed when h/w clock gating is enabled
> b) may invoke bad behavior in the controller
> c) is not optimal since the h/w is already handling this. 

So do not CONFIG_MMC_CLKGATE for this platform?

I don't quite get it I think...

Yours,
Linus Walleij


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08  9:40         ` Linus Walleij
@ 2010-11-08  9:45           ` Philip Rakity
  2010-11-09 10:29           ` Jaehoon Chung
  1 sibling, 0 replies; 25+ messages in thread
From: Philip Rakity @ 2010-11-08  9:45 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Nicolas Pitre, Ohad Ben-Cohen, linux-mmc


On Nov 8, 2010, at 1:40 AM, Linus Walleij wrote:

> Philip Rakity wrote:
> 
>> I am concerned that having the core/ layer ask for clocks off and on is
>> 
>> a) not needed when h/w clock gating is enabled
>> b) may invoke bad behavior in the controller
>> c) is not optimal since the h/w is already handling this. 
> 
> So do not CONFIG_MMC_CLKGATE for this platform?
> 
> I don't quite get it I think...

I want h/w clock gating for SD/eMMC/MMC cards and NOT sdio.

If h/w is doing clock gating do not know what why I can to turn off  clock since h/w is already doing this.  Need a way to distinguish.

> 
> Yours,
> Linus Walleij
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08  9:37       ` Linus Walleij
@ 2010-11-08  9:47         ` Philip Rakity
  2010-11-08 10:40           ` Linus Walleij
  2010-11-08 18:47         ` Nicolas Pitre
  1 sibling, 1 reply; 25+ messages in thread
From: Philip Rakity @ 2010-11-08  9:47 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Ohad Ben-Cohen, linux-mmc, David Vrabel


On Nov 8, 2010, at 1:37 AM, Linus Walleij wrote:

> Ohad Ben-Cohen wrote:
> 
>> So the decision whether to enable clock gating for SDIO should not be
>> static (eventually. I'm not saying this is something Linus should take
>> care of in his patch, it's up to us to make this work). By default,
>> SDIO clock gating should be disabled, but we need to allow drivers to
>> explicitly enable it if supported.
> 
> I currently (patch v8) disable gating for all SDIO cards,
> but I guess there are two approaches to cases where you'd
> like to enable it:
> 
> - Card (including SDIO device) whitelisting.

do not understand the term whitelisting ..  please explain.

> 
> - Host whitelisting - for the case where as gateable SDIO
>  device is soldered to the bus.
> 
> The first looks like big-ish so would be a separate patch, but a
> host flag for the latter like MMC_CAP_GATE_SDIO can easily be
> folded into a v10 of this patch.

Nico was not in favor of this.  He thought it best to have the core/ layer handle this.


> 
> Is that desireable?
> 
> Yours,
> Linus Walleij


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08  9:47         ` Philip Rakity
@ 2010-11-08 10:40           ` Linus Walleij
  2010-11-08 18:49             ` Nicolas Pitre
  0 siblings, 1 reply; 25+ messages in thread
From: Linus Walleij @ 2010-11-08 10:40 UTC (permalink / raw)
  To: Philip Rakity; +Cc: Ohad Ben-Cohen, linux-mmc, David Vrabel

Philip Rakity wrote:
> On Nov 8, 2010, at 1:37 AM, Linus Walleij wrote:
>> - Card (including SDIO device) whitelisting.
> 
> do not understand the term whitelisting ..  please explain.

The idea would be to have a database with ID:s for cards
that may be gated. Any SDIO card not it this list would
not be gated.

>> - Host whitelisting - for the case where as gateable SDIO
>>  device is soldered to the bus.
>>
>> The first looks like big-ish so would be a separate patch, but a
>> host flag for the latter like MMC_CAP_GATE_SDIO can easily be
>> folded into a v10 of this patch.
> 
> Nico was not in favor of this.  He thought it best to have
> the core/ layer handle this.

My patch is indeed to the core?

This was to be a flag for explicitly letting the core
gate some specific hosts on the MMC bus, in the case
where you (when defining platform data) already know that
a gateable SDIO card was soldered hardly onto a certain
host.

Anyway, if we have such needs they can be fixed later.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08  9:37       ` Linus Walleij
  2010-11-08  9:47         ` Philip Rakity
@ 2010-11-08 18:47         ` Nicolas Pitre
  1 sibling, 0 replies; 25+ messages in thread
From: Nicolas Pitre @ 2010-11-08 18:47 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Ohad Ben-Cohen, Philip Rakity, linux-mmc, David Vrabel

On Mon, 8 Nov 2010, Linus Walleij wrote:

> Ohad Ben-Cohen wrote:
> 
> > So the decision whether to enable clock gating for SDIO should not be
> > static (eventually. I'm not saying this is something Linus should take
> > care of in his patch, it's up to us to make this work). By default,
> > SDIO clock gating should be disabled, but we need to allow drivers to
> > explicitly enable it if supported.
> 
> I currently (patch v8) disable gating for all SDIO cards,
> but I guess there are two approaches to cases where you'd
> like to enable it:
> 
> - Card (including SDIO device) whitelisting.
> 
> - Host whitelisting - for the case where as gateable SDIO
>   device is soldered to the bus.
> 
> The first looks like big-ish so would be a separate patch, but a
> host flag for the latter like MMC_CAP_GATE_SDIO can easily be
> folded into a v10 of this patch.

No.  Please don't put such capability flag with the host.  This is still 
a card specific issue, whether or not it is soldered on the board.  So 
the first option would be the way to go.


Nicolas

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08 10:40           ` Linus Walleij
@ 2010-11-08 18:49             ` Nicolas Pitre
  2010-11-08 22:06               ` Linus Walleij
  0 siblings, 1 reply; 25+ messages in thread
From: Nicolas Pitre @ 2010-11-08 18:49 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Philip Rakity, Ohad Ben-Cohen, linux-mmc, David Vrabel

On Mon, 8 Nov 2010, Linus Walleij wrote:

> Philip Rakity wrote:
> > On Nov 8, 2010, at 1:37 AM, Linus Walleij wrote:
> >> - Card (including SDIO device) whitelisting.
> > 
> > do not understand the term whitelisting ..  please explain.
> 
> The idea would be to have a database with ID:s for cards
> that may be gated. Any SDIO card not it this list would
> not be gated.

Instead, let each SDIO card function driver indicate to the core if it 
is OK to gate the clock or not.


Nicolas

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08 18:49             ` Nicolas Pitre
@ 2010-11-08 22:06               ` Linus Walleij
  0 siblings, 0 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-08 22:06 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Philip Rakity, Ohad Ben-Cohen, linux-mmc, David Vrabel

2010/11/8 Nicolas Pitre <nico@fluxnic.net>:
> On Mon, 8 Nov 2010, Linus Walleij wrote:
>> The idea would be to have a database with ID:s for cards
>> that may be gated. Any SDIO card not it this list would
>> not be gated.
>
> Instead, let each SDIO card function driver indicate to the core if it
> is OK to gate the clock or not.

Ah yeah of course. I say let's stick with patch v8 and save this
SDIO gating for later. I think v8 does the basic job pretty well
anyhow.

Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-08  9:40         ` Linus Walleij
  2010-11-08  9:45           ` Philip Rakity
@ 2010-11-09 10:29           ` Jaehoon Chung
  2010-11-09 16:52             ` Linus Walleij
  1 sibling, 1 reply; 25+ messages in thread
From: Jaehoon Chung @ 2010-11-09 10:29 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Philip Rakity, Nicolas Pitre, Ohad Ben-Cohen, linux-mmc

Hi Linus..

i tested the clocking framework v8...at samsung SOC
just this patch worked S/W enable/disable..not H/W clock..

So i agreed the philip's opinion..
i also think that need some approach about H/W clock gating.. 

Linus Walleij wrote:
> Philip Rakity wrote:
> 
>> I am concerned that having the core/ layer ask for clocks off and on is
>>
>> a) not needed when h/w clock gating is enabled
>> b) may invoke bad behavior in the controller
>> c) is not optimal since the h/w is already handling this. 
> 
> So do not CONFIG_MMC_CLKGATE for this platform?
> 
> I don't quite get it I think...
> 
> Yours,
> Linus Walleij
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks,
Jaehoon Chung

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-09 10:29           ` Jaehoon Chung
@ 2010-11-09 16:52             ` Linus Walleij
  0 siblings, 0 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-09 16:52 UTC (permalink / raw)
  To: Jaehoon Chung; +Cc: Philip Rakity, Nicolas Pitre, Ohad Ben-Cohen, linux-mmc

2010/11/9 Jaehoon Chung <jh80.chung@samsung.com>:

> i tested the clocking framework v8...at samsung SOC
> just this patch worked S/W enable/disable..not H/W clock..

Yes that is the intention. It only handles soft clock gating.

> So i agreed the philip's opinion..
> i also think that need some approach about H/W clock gating..

Please create a patch on top of v8 adding the stuff
you need so we can review it.

As you understand I cannot do this since I don't
have a system with HW clock gating.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-05  8:35       ` Linus Walleij
@ 2010-11-05 10:03         ` Ohad Ben-Cohen
  0 siblings, 0 replies; 25+ messages in thread
From: Ohad Ben-Cohen @ 2010-11-05 10:03 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Nicolas Pitre, David Vrabel, linux-mmc, Ghorai Sukumar,
	Chris Ball, Adrian Hunter, Kyungmin Park, jh80.chung

On Fri, Nov 5, 2010 at 4:35 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> Nicolas Pitre wrote:
>
>> Maybe the runtime PM stuff is not the right abstraction for this?
>
> Hm, yeah the bad thing is that I can't reuse the delay code
> from runtime PM.

Why not ?

>  But all the hazzle of creating dummy devices
> nodes just to reuse runtime PM is even more disturbing.

Yeah creating dummy devices is not a good approach.

> Can I take this as your Acked-by: for the v7 patch using the old
> host-local approach?

FWIW, I don't mind helping out with the runtime PM migration, so if
you prefer to push v7 I could take it from there (when I get to
it...).

>
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-04 17:01     ` Nicolas Pitre
@ 2010-11-05  8:35       ` Linus Walleij
  2010-11-05 10:03         ` Ohad Ben-Cohen
  0 siblings, 1 reply; 25+ messages in thread
From: Linus Walleij @ 2010-11-05  8:35 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: David Vrabel, linux-mmc, Ghorai Sukumar, Chris Ball,
	Adrian Hunter, Kyungmin Park, jh80.chung

Nicolas Pitre wrote:

> Maybe the runtime PM stuff is not the right abstraction for this?

Hm, yeah the bad thing is that I can't reuse the delay code
from runtime PM. But all the hazzle of creating dummy devices
nodes just to reuse runtime PM is even more disturbing.

Can I take this as your Acked-by: for the v7 patch using the old
host-local approach?

Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-04 14:25   ` Linus Walleij
  2010-11-04 17:01     ` Nicolas Pitre
@ 2010-11-04 21:20     ` Ohad Ben-Cohen
  1 sibling, 0 replies; 25+ messages in thread
From: Ohad Ben-Cohen @ 2010-11-04 21:20 UTC (permalink / raw)
  To: Linus Walleij
  Cc: David Vrabel, linux-mmc, Ghorai Sukumar, Chris Ball,
	Nicolas Pitre, Adrian Hunter, Kyungmin Park, jh80.chung

On Thu, Nov 4, 2010 at 10:25 AM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> Since the runtime_pm_get/put calls are balanced for the host
> enable/disable case and the clock gating is orthogonal,
> I need to create an orthogonal instance of hooks, maybe it's
> easiest to just spawn a second dummy device like mmc0_clk
> for this to be able to use the runtime PM hooks?

What about:
- register the (generic ?) runtime PM handlers to mmc_host_class
- call the runtime pm get/put API on host->class_dev (in
mmc_start_request/mmc_request_done)
- use runtime PM autosuspend for taking care of the required delay

The host controller will get the idle notification directly to its
runtime PM handlers. It's not via set_ios, I know, but it looks so
simple and intuitive..

>
> Back to hacking!
>
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-04 14:25   ` Linus Walleij
@ 2010-11-04 17:01     ` Nicolas Pitre
  2010-11-05  8:35       ` Linus Walleij
  2010-11-04 21:20     ` Ohad Ben-Cohen
  1 sibling, 1 reply; 25+ messages in thread
From: Nicolas Pitre @ 2010-11-04 17:01 UTC (permalink / raw)
  To: Linus Walleij
  Cc: David Vrabel, linux-mmc, Ghorai Sukumar, Chris Ball,
	Adrian Hunter, Kyungmin Park, jh80.chung

On Thu, 4 Nov 2010, Linus Walleij wrote:

> David Vrabel wrote:
> > Linus Walleij wrote:
> >> @@ -148,14 +148,19 @@ static int mmc_runtime_suspend(struct device *dev)
> >>  {
> >>  	struct mmc_card *card = mmc_dev_to_card(dev);
> >>  
> >> +	mmc_gate_clock(card->host);
> >>  	return mmc_power_save_host(card->host);
> >>  }
> > 
> > Haven't you tied the clock gating to the power on/off state of the
> > card/host?  This looks like the wrong thing to me.  Surely we want to
> > gate the clock even if the card (or SDIO functions) are active?
> 
> You're probably right :-/
> 
> Since the runtime_pm_get/put calls are balanced for the host
> enable/disable case and the clock gating is orthogonal,
> I need to create an orthogonal instance of hooks, maybe it's
> easiest to just spawn a second dummy device like mmc0_clk
> for this to be able to use the runtime PM hooks?

Maybe the runtime PM stuff is not the right abstraction for this?  Like 
I said before, the host controller driver should do opportunistic power 
savings on its own and remain transparent to the core layer and the 
card.  But power saving on the card should be controlled by the core 
code or the function driver.  So there shouldn't be any coupling between 
those, even if the host controller driver is in the middle.


Nicolas

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-04 13:15 ` David Vrabel
@ 2010-11-04 14:25   ` Linus Walleij
  2010-11-04 17:01     ` Nicolas Pitre
  2010-11-04 21:20     ` Ohad Ben-Cohen
  0 siblings, 2 replies; 25+ messages in thread
From: Linus Walleij @ 2010-11-04 14:25 UTC (permalink / raw)
  To: David Vrabel
  Cc: linux-mmc, Ghorai Sukumar, Chris Ball, Nicolas Pitre,
	Adrian Hunter, Kyungmin Park, jh80.chung

David Vrabel wrote:
> Linus Walleij wrote:
>> @@ -148,14 +148,19 @@ static int mmc_runtime_suspend(struct device *dev)
>>  {
>>  	struct mmc_card *card = mmc_dev_to_card(dev);
>>  
>> +	mmc_gate_clock(card->host);
>>  	return mmc_power_save_host(card->host);
>>  }
> 
> Haven't you tied the clock gating to the power on/off state of the
> card/host?  This looks like the wrong thing to me.  Surely we want to
> gate the clock even if the card (or SDIO functions) are active?

You're probably right :-/

Since the runtime_pm_get/put calls are balanced for the host
enable/disable case and the clock gating is orthogonal,
I need to create an orthogonal instance of hooks, maybe it's
easiest to just spawn a second dummy device like mmc0_clk
for this to be able to use the runtime PM hooks?

Back to hacking!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] mmc: agressive clocking framework v9
  2010-11-04 11:16 Linus Walleij
@ 2010-11-04 13:15 ` David Vrabel
  2010-11-04 14:25   ` Linus Walleij
  0 siblings, 1 reply; 25+ messages in thread
From: David Vrabel @ 2010-11-04 13:15 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-mmc, Ghorai Sukumar, Chris Ball, Nicolas Pitre,
	Adrian Hunter, Kyungmin Park, jh80.chung

Linus Walleij wrote:
> 
> Switch to grouping this with the runtime PM stuff as Alan
> suggested. Is this looking better?
> [...]
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index af8dc6a..9064def 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -148,14 +148,19 @@ static int mmc_runtime_suspend(struct device *dev)
>  {
>  	struct mmc_card *card = mmc_dev_to_card(dev);
>  
> +	mmc_gate_clock(card->host);
>  	return mmc_power_save_host(card->host);
>  }

Haven't you tied the clock gating to the power on/off state of the
card/host?  This looks like the wrong thing to me.  Surely we want to
gate the clock even if the card (or SDIO functions) are active?

David--
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] mmc: agressive clocking framework v9
@ 2010-11-04 11:16 Linus Walleij
  2010-11-04 13:15 ` David Vrabel
  0 siblings, 1 reply; 25+ messages in thread
From: Linus Walleij @ 2010-11-04 11:16 UTC (permalink / raw)
  To: linux-mmc
  Cc: Ghorai Sukumar, Chris Ball, Nicolas Pitre, Adrian Hunter,
	David Vrabel, Kyungmin Park, jh80.chung, Linus Walleij

This patch modifies the MMC core code to optionally call the
set_ios() operation on the driver with the clock frequency set
to 0 (gate) after a grace period of at least 8 MCLK cycles, then
restore it (ungate) before any new request. This gives
the driver the option to shut down the MCI clock to the MMC/SD
card when the clock frequency is 0, i.e. the core has stated
that the MCI clock does not need to be generated.

It is inspired by existing clock gating code found in the OMAP
and Atmel drivers and brings this up to the host abstraction.
Gating is performed before and after any MMC request.

It exemplifies by implementing this for the MMCI/PL180 MMC/SD
host controller, but it should be simple to switch OMAP and
Atmel over to using this instead.

It is injected into the runtime PM support that is already
available for the host controller, so the runtime suspend/resume
flow will now be like this:

1. gate the clock with set_ios()
2. call the host core .disable() function
3. suspend complete
4. call the host core .enable() function
5. ungate the clock with set_ios()

Both gating and enable/disable are made optional.

The gate/disable delay is now one and the same. However
if clock gating is enabled, the delay is assured to be
at least 8 MCI bus cycles, as per the MMC spec. Biggest
delay takes precedence.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
Changes v7->v9:

Switch to grouping this with the runtime PM stuff as Alan
suggested. Is this looking better?

Disadvantage of this: platforms that don't want runtime PM can't
use this.

Code is untested: compiling in runtime PM hangs my platform,
can the Texas guys try this maybe?
---
 drivers/mmc/core/Kconfig |   11 ++++
 drivers/mmc/core/bus.c   |    7 ++-
 drivers/mmc/core/core.c  |  114 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/core/core.h  |   23 +++++++++
 include/linux/mmc/host.h |    5 ++
 5 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index bb22ffd..8915c03 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -16,3 +16,14 @@ config MMC_UNSAFE_RESUME
 
 	  This option sets a default which can be overridden by the
 	  module parameter "removable=0" or "removable=1".
+
+config MMC_CLKGATE
+	bool "MMC host clock gating (EXPERIMENTAL)"
+	depends on EXPERIMENTAL && PM_RUNTIME
+	help
+	  This will attempt to agressively gate the clock to the MMC card.
+	  This is done to save power due to gating off the logic and bus
+	  noise when the MMC card is not in use. Your host driver has to
+	  support handling this in order for it to be of any use.
+
+	  If unsure, say N.
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index af8dc6a..9064def 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -148,14 +148,19 @@ static int mmc_runtime_suspend(struct device *dev)
 {
 	struct mmc_card *card = mmc_dev_to_card(dev);
 
+	mmc_gate_clock(card->host);
 	return mmc_power_save_host(card->host);
 }
 
 static int mmc_runtime_resume(struct device *dev)
 {
 	struct mmc_card *card = mmc_dev_to_card(dev);
+	int ret;
 
-	return mmc_power_restore_host(card->host);
+	ret = mmc_power_restore_host(card->host);
+	if (!ret)
+		mmc_ungate_clock(card->host);
+	return ret;
 }
 
 static int mmc_runtime_idle(struct device *dev)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 8f86d70..b997d6c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -296,7 +296,7 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 
 		timeout_us = data->timeout_ns / 1000;
 		timeout_us += data->timeout_clks * 1000 /
-			(card->host->ios.clock / 1000);
+			(mmc_host_clk_rate(card->host) / 1000);
 
 		if (data->flags & MMC_DATA_WRITE)
 			/*
@@ -561,6 +561,8 @@ void mmc_host_deeper_disable(struct work_struct *work)
  */
 int mmc_host_lazy_disable(struct mmc_host *host)
 {
+	unsigned int disable_delay = mmc_host_disable_delay(host);
+
 	if (!(host->caps & MMC_CAP_DISABLE))
 		return 0;
 
@@ -573,9 +575,9 @@ int mmc_host_lazy_disable(struct mmc_host *host)
 	if (!host->enabled)
 		return 0;
 
-	if (host->disable_delay) {
+	if (disable_delay) {
 		mmc_schedule_delayed_work(&host->disable,
-				msecs_to_jiffies(host->disable_delay));
+				msecs_to_jiffies(disable_delay));
 		return 0;
 	} else
 		return mmc_host_do_disable(host, 1);
@@ -614,6 +616,12 @@ static inline void mmc_set_ios(struct mmc_host *host)
 		 ios->power_mode, ios->chip_select, ios->vdd,
 		 ios->bus_width, ios->timing);
 
+	/*
+	 * We get a new frequency while the clock is gated,
+	 * so make sure we regard this as ungating it.
+	 */
+	if (ios->clock > 0 && host->clk_gated)
+		host->clk_gated = false;
 	host->ops->set_ios(host, ios);
 }
 
@@ -641,6 +649,106 @@ void mmc_set_clock(struct mmc_host *host, unsigned int hz)
 	mmc_set_ios(host);
 }
 
+#ifdef CONFIG_MMC_CLKGATE
+/**
+ * mmc_host_may_gate_card - check if this card may be gated
+ * @card: card to check.
+ */
+static bool mmc_may_gate_card(struct mmc_card *card)
+{
+	/* If there is no card we may gate it */
+	if (!card)
+		return true;
+	/*
+	 * Don't gate SDIO cards! These need to be clocked at all times
+	 * since they may be independent systems generating interrupts
+	 * and other events. The clock requests counter from the core will
+	 * go down to zero since the core does not need it, but we will not
+	 * gate the clock, because there is somebody out there that may still
+	 * be using it.
+	 */
+	if (mmc_card_sdio(card))
+		return false;
+
+	return true;
+}
+
+/**
+ * mmc_host_clk_rate - get current clock frequency setting no matter
+ * whether it's gated or not.
+ * @host: host to get the clock frequency for.
+ */
+unsigned int mmc_host_clk_rate(struct mmc_host *host)
+{
+	unsigned long freq;
+
+	if (host->clk_gated)
+		freq = host->clk_old;
+	else
+		freq = host->ios.clock;
+	return freq;
+}
+
+/**
+ * mmc_gate_clock - gates the clock by setting it to 0 Hz.
+ * @host: the host to gate
+ */
+void mmc_gate_clock(struct mmc_host *host)
+{
+	if (!mmc_may_gate_card(host->card))
+		return;
+	host->clk_old = host->ios.clock;
+	host->ios.clock = 0;
+	host->clk_gated = true;
+	mmc_set_ios(host);
+}
+
+/**
+ * mmc_ungate_clock - restores the clock from gating by using the cached
+ * clock value
+ * @host: the host to ungate
+ */
+void mmc_ungate_clock(struct mmc_host *host)
+{
+	/*
+	 * We should previously have gated the clock, so the clock
+	 * shall be 0 here!
+	 * The clock may however be 0 during intialization,
+	 * when some request operations are performed before setting
+	 * the frequency. When ungate is requested in that situation
+	 * we just ignore the call.
+	 */
+	if (host->clk_old) {
+		BUG_ON(host->ios.clock);
+		mmc_set_clock(host, host->clk_old);
+	}
+	host->clk_gated = false;
+}
+
+/**
+ * mmc_host_disable_delay - adjust delay so it covers atleast 8 MCLK cycles
+ * @host_delay: delay suggested by the host controller
+ * All measures in milliseconds. This makes the runtime PM gate the
+ * clock and also call the host disable function (if any) after at
+ * least 8 MCI clock cycles.
+ */
+unsigned int mmc_host_disable_delay(struct mmc_host *host)
+{
+	unsigned long freq = host->ios.clock;
+	unsigned long tick_ms;
+	unsigned long host_delay = host->disable_delay;
+
+	if (!freq)
+		return host_delay;
+	/*
+	 * Delay n bus cycles (at least 8 from MMC spec) before attempting
+	 * to disable the MCI clock.
+	 */
+	tick_ms = 8 * DIV_ROUND_UP(1000000, freq);
+	return (tick_ms > host_delay) ? tick_ms : host_delay;
+}
+#endif
+
 /*
  * Change the bus mode (open drain/push-pull) of a host.
  */
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 77240cd..0eb3f3f 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -68,5 +68,28 @@ void mmc_remove_host_debugfs(struct mmc_host *host);
 void mmc_add_card_debugfs(struct mmc_card *card);
 void mmc_remove_card_debugfs(struct mmc_card *card);
 
+/* Clock gating functions */
+#ifdef CONFIG_MMC_CLKGATE
+unsigned int mmc_host_clk_rate(struct mmc_host *host);
+void mmc_gate_clock(struct mmc_host *host);
+void mmc_ungate_clock(struct mmc_host *host);
+unsigned int mmc_host_disable_delay(struct mmc_host *host);
+#else
+static inline unsigned int mmc_host_clk_rate(struct mmc_host *host)
+{
+	return host->ios.clock;
+}
+static inline void mmc_gate_clock(struct mmc_host *host)
+{
+}
+static inline void mmc_ungate_clock(struct mmc_host *host)
+{
+}
+static inline unsigned int mmc_host_disable_delay(struct mmc_host *host)
+{
+	return host->disable_delay;
+}
+#endif
+
 #endif
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 6d87f68..6f83d71 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -171,6 +171,11 @@ struct mmc_host {
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
+#ifdef CONFIG_MMC_CLKGATE
+	bool			clk_gated;	/* clock gated */
+	unsigned int		clk_old;	/* old clock value cache */
+#endif
+
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
 	unsigned short		max_segs;	/* see blk_queue_max_segments */
-- 
1.7.2.3


^ permalink raw reply related	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2010-11-09 16:52 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-06  5:38 [PATCH] mmc: agressive clocking framework v9 Philip Rakity
2010-11-06 17:24 ` Ohad Ben-Cohen
2010-11-06 17:38   ` Philip Rakity
2010-11-07  1:44     ` Nicolas Pitre
2010-11-07 18:18       ` Philip Rakity
2010-11-08  4:33         ` Nicolas Pitre
2010-11-08  9:40         ` Linus Walleij
2010-11-08  9:45           ` Philip Rakity
2010-11-09 10:29           ` Jaehoon Chung
2010-11-09 16:52             ` Linus Walleij
2010-11-07  9:32     ` Ohad Ben-Cohen
2010-11-08  9:37       ` Linus Walleij
2010-11-08  9:47         ` Philip Rakity
2010-11-08 10:40           ` Linus Walleij
2010-11-08 18:49             ` Nicolas Pitre
2010-11-08 22:06               ` Linus Walleij
2010-11-08 18:47         ` Nicolas Pitre
2010-11-08  9:23 ` Linus Walleij
  -- strict thread matches above, loose matches on Subject: below --
2010-11-04 11:16 Linus Walleij
2010-11-04 13:15 ` David Vrabel
2010-11-04 14:25   ` Linus Walleij
2010-11-04 17:01     ` Nicolas Pitre
2010-11-05  8:35       ` Linus Walleij
2010-11-05 10:03         ` Ohad Ben-Cohen
2010-11-04 21:20     ` Ohad Ben-Cohen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.