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

["Munegowda, Keshava" <keshava_mgowda@ti.com>]

On Wed, Jun 29, 2011 at 8:52 PM, Munegowda, Keshava
<keshava_mgowda@ti.com> wrote:
> On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <khilman@ti.com> wrote:
>> 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 of
>> runtime PM.  What I mean is that the runtime PM methods are used only
>> during the suspend path.  The rest of the time the USB host IP block is
>> left enabled, even when nothing is connected.
>>
>> I tested this on my 3530/Overo board, and verified that indeed the
>> usbhost powerdomain hits retention on suspend, but while idle, when
>> nothing is connected, I would expect the driver could be clever enough
>> to use runtime PM (probably using autosuspend timeouts) to disable the
>> hardware as well.
>>
>>> ---
>>>  drivers/mfd/omap-usb-host.c |  103 +++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 103 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>>> index 43de12a..32d19e2 100644
>>> --- a/drivers/mfd/omap-usb-host.c
>>> +++ b/drivers/mfd/omap-usb-host.c
>>> @@ -146,6 +146,10 @@
>>>  #define is_ehci_hsic_mode(x) (x == OMAP_EHCI_PORT_MODE_HSIC)
>>>
>>>
>>> +/* USBHS state bits */
>>> +#define OMAP_USBHS_INIT              0
>>> +#define OMAP_USBHS_SUSPEND   4
>>
>> These additional state bits don't seem to be necessary.
>>
>> For suspend, just check 'pm_runtime_is_suspended()'
>>
>> The init flag is only used in the suspend/resume hooks, but the need for
>> it is a side effect of not correctly using the runtime PM callbacks.
>>
>> Remember that the runtime PM get/put hooks have usage counting.  Only
>> when the usage count transitions to/from zero is the actual
>> hardware-level enable/disable (via omap_hwmod) being done.
>>
>> The current code is making the assumption that every call to get/put is
>> going to result in an enable/disable of the hardware.
>>
>> Instead, all of the code that needs to be run only upon actual
>> enable/disable of the hardware should be done in the driver's
>> runtime_suspend/runtime_resume callbacks.  These are only called when
>> the hardware actually changes state.
>>
>> Not knowing that much about the EHCI block, upon first glance, it looks
>> like mmuch of what is done in usbhs_enable() should actually be done in
>> the ->runtime_resume() callback, and similarily, much of what is done in
>> usbhs_disable() should be done in the ->runtime_suspend() callback.
>
> Kevin,
>   do you mean driver->runtime_resume and driver->runtime_resume call backs.
> are these call backs from pm_runtime_get_sync and pm_runtime_put_sync?

for usb host case , I am seeing that the pm_runtime_get_sync


static int rpm_resume(struct device *dev, int rpmflags)
{
  ............
 ..........
	if (dev->pwr_domain) {
		callback = dev->pwr_domain->ops.runtime_resume;
		if(!strcmp(dev_name(dev),"usbhs_omap"))
			 pr_err("dev->pwr_domain->ops.runtime_resume");
	}	
	else if (dev->type && dev->type->pm) {
		callback = dev->type->pm->runtime_resume;
		if(!strcmp(dev_name(dev),"usbhs_omap"))
			 pr_err("dev->type->pm->runtime_resume");
	}	
	else if (dev->class && dev->class->pm) {
		callback = dev->class->pm->runtime_resume;
		if(!strcmp(dev_name(dev),"usbhs_omap"))
			 pr_err("ev->class->pm->runtime_resume");
	}	
	else if (dev->bus && dev->bus->pm) {
		callback = dev->bus->pm->runtime_resume;
	if(!strcmp(dev_name(dev),"usbhs_omap"))
		 pr_err("dev->bus->pm->runtime_resume");
	}	
	else
		callback = NULL;
}


I am seeing that below if statement was hitting true:

	if (dev->pwr_domain) {
		callback = dev->pwr_domain->ops.runtime_resume;
		if(!strcmp(dev_name(dev),"usbhs_omap"))
			 pr_err("dev->pwr_domain->ops.runtime_resume");


due to this; the driver->runtime_resume was not getting called.

Any idea on why I am seeing only the dev->pwr_domain is set not
dev->bus && dev->bus->pm is hitting here?