Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci and ohci

Date

Wed, 01 Jun 2011 17:06:29 -0700

Keshava Munegowda <keshava_mgowda@ti.com> writes:

> From: Keshava Munegowda <Keshava_mgowda@ti.com>>> The global suspend and resume functions for usbhs core driver> are implemented.These routine are called when the global suspend> and resume occurs. Before calling these functions, the> bus suspend and resume of ehci and ohci drivers are called> from runtime pm.>> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com>

First, from what I can see, this is only a partial implementation ofruntime PM. What I mean is that the runtime PM methods are used onlyduring the suspend path. The rest of the time the USB host IP block isleft enabled, even when nothing is connected.

I tested this on my 3530/Overo board, and verified that indeed theusbhost powerdomain hits retention on suspend, but while idle, whennothing is connected, I would expect the driver could be clever enoughto use runtime PM (probably using autosuspend timeouts) to disable thehardware as well.

The current code is making the assumption that every call to get/put isgoing to result in an enable/disable of the hardware.

Instead, all of the code that needs to be run only upon actualenable/disable of the hardware should be done in the driver'sruntime_suspend/runtime_resume callbacks. These are only called whenthe hardware actually changes state.

Not knowing that much about the EHCI block, upon first glance, it lookslike mmuch of what is done in usbhs_enable() should actually be done inthe ->runtime_resume() callback, and similarily, much of what is done inusbhs_disable() should be done in the ->runtime_suspend() callback.

Another thing to be aware of is that runtime PM can be disabled fromuserspace. For example, try this:

echo on > /sys/devices/platform/omap/usbhs_omap/power/control

This disables runtime PM for the device. After doing this andsuspending, you'll notice that usbhost powerdomain no longer hitsretention on suspend. Setting it back to 'auto' allows it to workagain.

Because of this, you can not simply call pm_runtime_put() from thestatic suspend callback. You should check pm_runtime_is_suspended().If it is, there's nothing to do. If not, the runtime PM callbacks forthe subsystem need to manually be called. See drivers/i2c/i2c-omap.cfor an example (and check the version in my PM branch, which has a fixrequired starting with kernel v3.0.)

While I'm preaching on runtime PM here, some other things that should becleaned up because they duplicate what other frameworks are doing:

- drivers should not be touching their SYSCONFIG register. This is managed by omap_hwmod- current driver is doing usage counting, but runtime PM core is already handling usage counting.

My apologies for not reviewing the runtime PM work in this driverearlier. Some of the problems above come from code that's already inmainline (which I should've reviewed earlier), and some are added withthis series. All of them should be cleaned up before merging this.