[PATCH] usb: dwc3: keystone: switch to use runtime pm

On Friday 31 January 2014 11:45 AM, Felipe Balbi wrote:

On Fri, Jan 31, 2014 at 10:50:40AM -0500, Santosh Shilimkar wrote:

quoted

On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:

quoted

On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:

quoted

On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:

quoted

Hi,

On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:

quoted

The Keystone PM management layer has been implemented using PM bus for
power management clocks. As result, most of Keystone drivers don't need
to manage clocks directly. They just need to enable runtime PM and use it
to handle their PM state and clocks.

Hence, remove clock management code and switch to use runtime PM.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

quite a few weeks back I sent a series enabling runtime pm for all glue
layers. I'll use that version instead, sorry.

That should be fine but you need to drop clk_*() related code
from that patch. I assume you will send refresh version of it then.

why ? it makes no difference if you enable twice and disable twice.

Sure but why do you want to have the clock node handling code in drivers
if it is not needed. Isn't that better ?

It might, but then way that I wanted to see it is so that I can make
assumptions about the device state. From a driver perspective, what I
would love to see is the ability to assume that when my probe gets
called the device is already enabled. So if you can make sure that
clk_enable() happens before my probe and that you call
pm_runtime_set_active() before my probe too, then I can more than
hapilly remove clk_* calls from the driver ;-)

either that or maintain the driver like so:

probe()
{
	...

	clk_get(dev, "fck");
	clk_prepare(clk);
	clk_enable(clk);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	pm_runtime_get_sync(dev);
	...
}

runtime_suspend()
{
	clk_disable(dev);
}

runtime_resume()
{
	clk_enable(dev);
}

note that because of pm_runtime_set_active() that first
pm_runtime_get_sync() in probe() will simply increase the reference
counter without calling my ->runtime_resume() callback, which is exactly
what we want, as that would completely avoid situations of bad context
being restored because of that initial pm_runtime_get_sync().

Thanks for making your point bit clear. 

Then, we can even make pm_runtime completely async easily, because
clk_prepare() was called only on probe() (or before it, for that
matter).

Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
clk_enable() and pm_runtime_set_active() will be called properly before
my probe, i'll be more than happy to comply with your request above as
that will greatly simplify my driver.

Which is the case at least I see on Keystone. And hence the patch from
Grygorii works. I also noticed your proposal for wider platform to
enforce above behavior which seems to be a good idea.

Just make, also, that if this clock is shared between dwc3-keystone
wrapper and dwc3 core, you clk_get() on both driver's probe.

I understand. In summary, whichever patch you pick(yours) or Grygorii's,
its completely safe to remove the clock handling from Keystone USB driver.

Regards,
Santosh