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

Commit Message

Munegowda, Keshava June 1, 2011, 1:27 p.m. UTC

From: Keshava Munegowda <[e-mail]>

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 <[e-mail]>
---
 drivers/mfd/omap-usb-host.c |  103 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 103 insertions(+), 0 deletions(-)

Comments

Felipe Balbi June 1, 2011, 1:31 p.m. UTC | #1

Hi,

On Wed, Jun 01, 2011 at 06:57:27PM +0530, Keshava Munegowda wrote:
> 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

#define OMAP_USBHS_INIT		BIT(0)
#define OMAP_USBHS_SUSPEND	BIT(1)

???

Munegowda, Keshava June 1, 2011, 1:38 p.m. UTC | #2

On Wed, Jun 1, 2011 at 7:01 PM, Felipe Balbi <[e-mail]> wrote:
> Hi,
>
> On Wed, Jun 01, 2011 at 06:57:27PM +0530, Keshava Munegowda wrote:
>> 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
>
> #define OMAP_USBHS_INIT         BIT(0)
> #define OMAP_USBHS_SUSPEND      BIT(1)
>
> ???

yes, I will do.. please comment on other patches too

keshava
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael Wysocki June 1, 2011, 1:54 p.m. UTC | #3

On Wednesday, June 01, 2011, Keshava Munegowda wrote:
> From: Keshava Munegowda <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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
> +
>  struct usbhs_hcd_omap {
>  	struct clk			*xclk60mhsp1_ck;
>  	struct clk			*xclk60mhsp2_ck;
> @@ -165,6 +169,7 @@ struct usbhs_hcd_omap {
>  	u32				usbhs_rev;
>  	spinlock_t			lock;
>  	int				count;
> +	unsigned long			state;
>  };
>  /*-------------------------------------------------------------------------*/
>  
> @@ -809,6 +814,8 @@ static int usbhs_enable(struct device *dev)
>  				(pdata->ehci_data->reset_gpio_port[1], 1);
>  	}
>  
> +	set_bit(OMAP_USBHS_INIT, &omap->state);
> +
>  end_count:
>  	omap->count++;
>  	spin_unlock_irqrestore(&omap->lock, flags);
> @@ -897,6 +904,7 @@ static void usbhs_disable(struct device *dev)
>  	}
>  
>  	pm_runtime_put_sync(dev);
> +	clear_bit(OMAP_USBHS_INIT, &omap->state);
>  
>  	/* The gpio_free migh sleep; so unlock the spinlock */
>  	spin_unlock_irqrestore(&omap->lock, flags);
> @@ -926,10 +934,105 @@ void omap_usbhs_disable(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(omap_usbhs_disable);
>  
> +#ifdef	CONFIG_PM
> +
> +static int usbhs_resume(struct device *dev)
> +{
> +	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
> +	unsigned long			flags = 0;
> +
> +	dev_dbg(dev, "Resuming TI HSUSB Controller\n");
> +
> +	if (!pdata) {
> +		dev_dbg(dev, "missing platform_data\n");
> +		return  -ENODEV;
> +	}
> +
> +	spin_lock_irqsave(&omap->lock, flags);
> +
> +	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
> +		!test_bit(OMAP_USBHS_SUSPEND, &omap->state))
> +			goto end_resume;
> +
> +	pm_runtime_get_sync(dev);
> +
> +	if (is_omap_usbhs_rev2(omap)) {
> +		if (is_ehci_tll_mode(pdata->port_mode[0])) {
> +			clk_enable(omap->usbhost_p1_fck);
> +			clk_enable(omap->usbtll_p1_fck);
> +		}
> +		if (is_ehci_tll_mode(pdata->port_mode[1])) {
> +			clk_enable(omap->usbhost_p2_fck);
> +			clk_enable(omap->usbtll_p2_fck);
> +		}
> +		clk_enable(omap->utmi_p1_fck);
> +		clk_enable(omap->utmi_p2_fck);
> +	}
> +	clear_bit(OMAP_USBHS_SUSPEND, &omap->state);
> +
> +end_resume:
> +	spin_unlock_irqrestore(&omap->lock, flags);
> +	return 0;
> +}
> +
> +
> +static int usbhs_suspend(struct device *dev)
> +{
> +	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
> +	unsigned long			flags = 0;
> +
> +	dev_dbg(dev, "Suspending TI HSUSB Controller\n");
> +
> +	if (!pdata) {
> +		dev_dbg(dev, "missing platform_data\n");
> +		return  -ENODEV;
> +	}
> +
> +	spin_lock_irqsave(&omap->lock, flags);
> +
> +	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
> +		test_bit(OMAP_USBHS_SUSPEND, &omap->state))
> +			goto end_suspend;
> +
> +	if (is_omap_usbhs_rev2(omap)) {
> +		if (is_ehci_tll_mode(pdata->port_mode[0])) {
> +			clk_disable(omap->usbhost_p1_fck);
> +			clk_disable(omap->usbtll_p1_fck);
> +		}
> +		if (is_ehci_tll_mode(pdata->port_mode[1])) {
> +			clk_disable(omap->usbhost_p2_fck);
> +			clk_disable(omap->usbtll_p2_fck);
> +		}
> +		clk_disable(omap->utmi_p2_fck);
> +		clk_disable(omap->utmi_p1_fck);
> +	}
> +
> +	set_bit(OMAP_USBHS_SUSPEND, &omap->state);
> +	pm_runtime_put_sync(dev);
> +
> +end_suspend:
> +	spin_unlock_irqrestore(&omap->lock, flags);
> +	return 0;
> +}
> +
> +
> +static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> +	.suspend	= usbhs_suspend,
> +	.resume		= usbhs_resume,
> +};

Please add .freeze()/.thaw() and .poweroff()/.restore() callbacks too.
They may point to the same routines, but must be present.

You can actually use the SIMPLE_DEV_PM_OPS() convenience macro for this
purpose.

> +
> +#define USBHS_OMAP_DEV_PM_OPS (&usbhsomap_dev_pm_ops)
> +#else
> +#define	USBHS_OMAP_DEV_PM_OPS	NULL
> +#endif
> +
>  static struct platform_driver usbhs_omap_driver = {
>  	.driver = {
>  		.name		= (char *)usbhs_driver_name,
>  		.owner		= THIS_MODULE,
> +		.pm		= USBHS_OMAP_DEV_PM_OPS,
>  	},
>  	.remove		= __exit_p(usbhs_omap_remove),
>  };
> 

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi June 1, 2011, 2:32 p.m. UTC | #4

Hi,

On Wed, Jun 01, 2011 at 03:54:38PM +0200, Rafael J. Wysocki wrote:
> > +static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> > +	.suspend	= usbhs_suspend,
> > +	.resume		= usbhs_resume,
> > +};
> 
> Please add .freeze()/.thaw() and .poweroff()/.restore() callbacks too.
> They may point to the same routines, but must be present.
> 
> You can actually use the SIMPLE_DEV_PM_OPS() convenience macro for this
> purpose.

good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
popping on most drivers recently ? To me it looks like driver.pm field
is always available even if PM is disabled, so what's the point ? Saving
a few bytes ?

Kevin Hilman June 2, 2011, 12:06 a.m. UTC | #5

Keshava Munegowda <[e-mail]> writes:

> From: Keshava Munegowda <[e-mail]>
>
> 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 <[e-mail]>

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.

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

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

This disables runtime PM for the device.  After doing this and
suspending, you'll notice that usbhost powerdomain no longer hits
retention on suspend.  Setting it back to 'auto' allows it to work
again.

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

While I'm preaching on runtime PM here, some other things that should be
cleaned 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 driver
earlier.  Some of the problems above come from code that's already in
mainline (which I should've reviewed earlier), and some are added with
this series.  All of them should be cleaned up before merging this.

Kevin

>  struct usbhs_hcd_omap {
>  	struct clk			*xclk60mhsp1_ck;
>  	struct clk			*xclk60mhsp2_ck;
> @@ -165,6 +169,7 @@ struct usbhs_hcd_omap {
>  	u32				usbhs_rev;
>  	spinlock_t			lock;
>  	int				count;
> +	unsigned long			state;
>  };
>  /*-------------------------------------------------------------------------*/
>  
> @@ -809,6 +814,8 @@ static int usbhs_enable(struct device *dev)
>  				(pdata->ehci_data->reset_gpio_port[1], 1);
>  	}
>  
> +	set_bit(OMAP_USBHS_INIT, &omap->state);
> +
>  end_count:
>  	omap->count++;
>  	spin_unlock_irqrestore(&omap->lock, flags);
> @@ -897,6 +904,7 @@ static void usbhs_disable(struct device *dev)
>  	}
>  
>  	pm_runtime_put_sync(dev);
> +	clear_bit(OMAP_USBHS_INIT, &omap->state);
>  
>  	/* The gpio_free migh sleep; so unlock the spinlock */
>  	spin_unlock_irqrestore(&omap->lock, flags);
> @@ -926,10 +934,105 @@ void omap_usbhs_disable(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(omap_usbhs_disable);
>  
> +#ifdef	CONFIG_PM
> +
> +static int usbhs_resume(struct device *dev)
> +{
> +	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
> +	unsigned long			flags = 0;
> +
> +	dev_dbg(dev, "Resuming TI HSUSB Controller\n");
> +
> +	if (!pdata) {
> +		dev_dbg(dev, "missing platform_data\n");
> +		return  -ENODEV;
> +	}
> +
> +	spin_lock_irqsave(&omap->lock, flags);
> +
> +	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
> +		!test_bit(OMAP_USBHS_SUSPEND, &omap->state))
> +			goto end_resume;
> +
> +	pm_runtime_get_sync(dev);
> +
> +	if (is_omap_usbhs_rev2(omap)) {
> +		if (is_ehci_tll_mode(pdata->port_mode[0])) {
> +			clk_enable(omap->usbhost_p1_fck);
> +			clk_enable(omap->usbtll_p1_fck);
> +		}
> +		if (is_ehci_tll_mode(pdata->port_mode[1])) {
> +			clk_enable(omap->usbhost_p2_fck);
> +			clk_enable(omap->usbtll_p2_fck);
> +		}
> +		clk_enable(omap->utmi_p1_fck);
> +		clk_enable(omap->utmi_p2_fck);
> +	}
> +	clear_bit(OMAP_USBHS_SUSPEND, &omap->state);
> +
> +end_resume:
> +	spin_unlock_irqrestore(&omap->lock, flags);
> +	return 0;
> +}
> +
> +
> +static int usbhs_suspend(struct device *dev)
> +{
> +	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
> +	unsigned long			flags = 0;
> +
> +	dev_dbg(dev, "Suspending TI HSUSB Controller\n");
> +
> +	if (!pdata) {
> +		dev_dbg(dev, "missing platform_data\n");
> +		return  -ENODEV;
> +	}
> +
> +	spin_lock_irqsave(&omap->lock, flags);
> +
> +	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
> +		test_bit(OMAP_USBHS_SUSPEND, &omap->state))
> +			goto end_suspend;
> +
> +	if (is_omap_usbhs_rev2(omap)) {
> +		if (is_ehci_tll_mode(pdata->port_mode[0])) {
> +			clk_disable(omap->usbhost_p1_fck);
> +			clk_disable(omap->usbtll_p1_fck);
> +		}
> +		if (is_ehci_tll_mode(pdata->port_mode[1])) {
> +			clk_disable(omap->usbhost_p2_fck);
> +			clk_disable(omap->usbtll_p2_fck);
> +		}
> +		clk_disable(omap->utmi_p2_fck);
> +		clk_disable(omap->utmi_p1_fck);
> +	}
> +
> +	set_bit(OMAP_USBHS_SUSPEND, &omap->state);
> +	pm_runtime_put_sync(dev);
> +
> +end_suspend:
> +	spin_unlock_irqrestore(&omap->lock, flags);
> +	return 0;
> +}
> +
> +
> +static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> +	.suspend	= usbhs_suspend,
> +	.resume		= usbhs_resume,
> +};
> +
> +#define USBHS_OMAP_DEV_PM_OPS (&usbhsomap_dev_pm_ops)
> +#else
> +#define	USBHS_OMAP_DEV_PM_OPS	NULL
> +#endif
> +
>  static struct platform_driver usbhs_omap_driver = {
>  	.driver = {
>  		.name		= (char *)usbhs_driver_name,
>  		.owner		= THIS_MODULE,
> +		.pm		= USBHS_OMAP_DEV_PM_OPS,
>  	},
>  	.remove		= __exit_p(usbhs_omap_remove),
>  };
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael Wysocki June 5, 2011, 5:19 p.m. UTC | #6

On Wednesday, June 01, 2011, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Jun 01, 2011 at 03:54:38PM +0200, Rafael J. Wysocki wrote:
> > > +static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> > > +	.suspend	= usbhs_suspend,
> > > +	.resume		= usbhs_resume,
> > > +};
> > 
> > Please add .freeze()/.thaw() and .poweroff()/.restore() callbacks too.
> > They may point to the same routines, but must be present.
> > 
> > You can actually use the SIMPLE_DEV_PM_OPS() convenience macro for this
> > purpose.
> 
> good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> popping on most drivers recently ? To me it looks like driver.pm field
> is always available even if PM is disabled, so what's the point ? Saving
> a few bytes ?

Basically, yes (you may want to avoid defining the object this points to if
CONFIG_PM is unset).

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi June 5, 2011, 6:50 p.m. UTC | #7

Hi,

On Sun, Jun 05, 2011 at 07:19:38PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 01, 2011 at 03:54:38PM +0200, Rafael J. Wysocki wrote:
> > > > +static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
> > > > +	.suspend	= usbhs_suspend,
> > > > +	.resume		= usbhs_resume,
> > > > +};
> > > 
> > > Please add .freeze()/.thaw() and .poweroff()/.restore() callbacks too.
> > > They may point to the same routines, but must be present.
> > > 
> > > You can actually use the SIMPLE_DEV_PM_OPS() convenience macro for this
> > > purpose.
> > 
> > good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> > popping on most drivers recently ? To me it looks like driver.pm field
> > is always available even if PM is disabled, so what's the point ? Saving
> > a few bytes ?
> 
> Basically, yes (you may want to avoid defining the object this points to if
> CONFIG_PM is unset).

wouldn't it look nicer to have a specific section for dev_pm_ops which
gets automatically freed if CONFIG_PM isn't set ? I mean, there are a
few drivers which don't have the ifdeferry around dev_pm_ops anyway.

So, something like:

#define __pm_ops	__section(.pm.ops)

static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
	.suspend	= my_driver_suspend,
	.resume		= my_driver_resume,
	[ blablabla ]
};

to simplify things, you could:

#define DEFINE_DEV_PM_OPS(_ops)		\
	const struct dev_pm_ops _ops __pm_ops

that would mean changes to all linker scripts, though and a utility call
that only does anything ifndef CONFIG_PM to free the .pm.ops section.

Alan Stern June 5, 2011, 7:30 p.m. UTC | #8

On Sun, 5 Jun 2011, Felipe Balbi wrote:

> > > good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> > > popping on most drivers recently ? To me it looks like driver.pm field
> > > is always available even if PM is disabled, so what's the point ? Saving
> > > a few bytes ?
> > 
> > Basically, yes (you may want to avoid defining the object this points to if
> > CONFIG_PM is unset).
> 
> wouldn't it look nicer to have a specific section for dev_pm_ops which
> gets automatically freed if CONFIG_PM isn't set ? I mean, there are a
> few drivers which don't have the ifdeferry around dev_pm_ops anyway.
> 
> So, something like:
> 
> #define __pm_ops	__section(.pm.ops)
> 
> static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
> 	.suspend	= my_driver_suspend,
> 	.resume		= my_driver_resume,
> 	[ blablabla ]
> };
> 
> to simplify things, you could:
> 
> #define DEFINE_DEV_PM_OPS(_ops)		\
> 	const struct dev_pm_ops _ops __pm_ops
> 
> that would mean changes to all linker scripts, though and a utility call
> that only does anything ifndef CONFIG_PM to free the .pm.ops section.

In my opinion this would make programming harder, not easier.  It's
very easy to understand "#ifdef" followed by "#endif"; people see them
all the time.  The new tags you propose would force people to go
searching through tons of source files to see what they mean, and then
readers would still have to figure out when these tags should be used
or what advantage they might bring.

It's a little like "typedef struct foo foo_t;" -- doing this forces
people to remember one extra piece of information that serves no real
purpose except perhaps a minimal reduction in the amount of typing.  
Since the limiting factor in kernel programming is human brainpower,
not source file length, this is a bad tradeoff.  (Not to mention that
it also obscures an important fact: A foo_t is an extended structure
rather than a single value.)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi June 5, 2011, 7:54 p.m. UTC | #9

Hi,

On Sun, Jun 05, 2011 at 03:30:55PM -0400, Alan Stern wrote:
> > > > good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> > > > popping on most drivers recently ? To me it looks like driver.pm field
> > > > is always available even if PM is disabled, so what's the point ? Saving
> > > > a few bytes ?
> > > 
> > > Basically, yes (you may want to avoid defining the object this points to if
> > > CONFIG_PM is unset).
> > 
> > wouldn't it look nicer to have a specific section for dev_pm_ops which
> > gets automatically freed if CONFIG_PM isn't set ? I mean, there are a
> > few drivers which don't have the ifdeferry around dev_pm_ops anyway.
> > 
> > So, something like:
> > 
> > #define __pm_ops	__section(.pm.ops)
> > 
> > static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
> > 	.suspend	= my_driver_suspend,
> > 	.resume		= my_driver_resume,
> > 	[ blablabla ]
> > };
> > 
> > to simplify things, you could:
> > 
> > #define DEFINE_DEV_PM_OPS(_ops)		\
> > 	const struct dev_pm_ops _ops __pm_ops
> > 
> > that would mean changes to all linker scripts, though and a utility call
> > that only does anything ifndef CONFIG_PM to free the .pm.ops section.
> 
> In my opinion this would make programming harder, not easier.  It's

I tend to disagree with this statement, see below.

> very easy to understand "#ifdef" followed by "#endif"; people see them

very true... Still everybody has to put them in place.

> all the time.  The new tags you propose would force people to go
> searching through tons of source files to see what they mean, and then

only those who want to see "how things work" would be forced to do that,
other people would be allowed to "assume it's doing the right thing".

> readers would still have to figure out when these tags should be used
> or what advantage they might bring.

not really, if you add a macro which adds that correctly and during
review we only accept drivers using that particular macro, things
wouldn't go bad at all.

> It's a little like "typedef struct foo foo_t;" -- doing this forces

hey c'mon. Then you're saying that all __initdata, __devinitdata,
__initconst and all of those are "typedef struct foo foo_t" ;-)

> people to remember one extra piece of information that serves no real
> purpose except perhaps a minimal reduction in the amount of typing.  

and a guarantee that the unused data will be freed when it's really not
needed ;-)

> Since the limiting factor in kernel programming is human brainpower,
> not source file length, this is a bad tradeoff.  (Not to mention that

OTOH we are going through a big re-factor of the ARM port to reduce the
amount of code. Not that these few characters would change much but my
point is that amount of code also matters. So does uniformity, coding
style, etc...

> it also obscures an important fact: A foo_t is an extended structure
> rather than a single value.)

then it would make sense to have dev_pm_ops only defined when CONFIG_PM
is set to force all drivers stick to a common way of handling this.

Besides, currently, everybody who wants to keep the ifdeferry, needs to
define a macro for &my_dev_pm_ops or have two #ifdef..#endif blocks.

Either you do:

#ifdef CONFIG_PM
static int my_driver_suspend(struct device *dev)
{
	...

	return 0;
}
....

static const struct dev_pm_ops my_driver_pm_ops = {
	.suspend	= my_driver_suspend,
	...
};

#define DEV_PM_OPS	(&my_driver_pm_ops)
#else
#define DEV_PM_OPS	NULL
#endif

static struct platform_driver my_driver = {
	...
	.driver	= {
		.pm = DEV_PM_OPS
	},
};

or you do:

#ifdef CONFIG_PM
static int my_driver_suspend(struct device *dev)
{
	...

	return 0;
}
....

static const struct dev_pm_ops my_driver_pm_ops = {
	.suspend	= my_driver_suspend,
	...
};

#endif

static struct platform_driver my_driver = {
	...
	.driver	= {
#ifdef CONFIG_PM
		.pm = &my_driver_pm_ops,
#endif
	},
};

So, while this is a small thing which is easy to understand, it's still
yet another thing that all drivers have to remember to add. And when
everybody needs to remember that, I'd rather have it done
"automatically" by other means.

I mean, we already free .init.* sections after __init anyway, so what's
the problem in freeing another section ? I don't see it as obfuscation
at all. I see it as if the kernel is smart enough to free all unused
data by itself, without myself having to add ifdefs or freeing it by my
own.

On top of all that, today, we have driver with both ways of ifdefs plus
drivers with no ifdeferry at all, leaving dev_pm_ops floating around for
nothing.

IMHO, if things aren't uniform, we will have small problems, such as
this, proliferate because new drivers are based on other drivers,
generally.

Mark Brown June 6, 2011, 9:45 a.m. UTC | #10

On Sun, Jun 05, 2011 at 03:30:55PM -0400, Alan Stern wrote:
> On Sun, 5 Jun 2011, Felipe Balbi wrote:

> > that would mean changes to all linker scripts, though and a utility call
> > that only does anything ifndef CONFIG_PM to free the .pm.ops section.

> In my opinion this would make programming harder, not easier.  It's
> very easy to understand "#ifdef" followed by "#endif"; people see them
> all the time.  The new tags you propose would force people to go
> searching through tons of source files to see what they mean, and then
> readers would still have to figure out when these tags should be used
> or what advantage they might bring.

The big advantage is that they make it much harder to introduce random
build breakage and they're an awful lot less fiddly to do - it used to
be not so bad when it was just the function pointers that needed to be
defined to NULL but now we need to faff around with both the functions
and the dev_pm_ops.

The annotation approach is already familiar from the init stuff so it
wouldn't be so hard for people to understand.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Stern June 6, 2011, 4:06 p.m. UTC | #11

On Sun, 5 Jun 2011, Felipe Balbi wrote:

> Hi,
> 
> On Sun, Jun 05, 2011 at 03:30:55PM -0400, Alan Stern wrote:
> > > > > good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> > > > > popping on most drivers recently ? To me it looks like driver.pm field
> > > > > is always available even if PM is disabled, so what's the point ? Saving
> > > > > a few bytes ?
> > > > 
> > > > Basically, yes (you may want to avoid defining the object this points to if
> > > > CONFIG_PM is unset).
> > > 
> > > wouldn't it look nicer to have a specific section for dev_pm_ops which
> > > gets automatically freed if CONFIG_PM isn't set ? I mean, there are a
> > > few drivers which don't have the ifdeferry around dev_pm_ops anyway.
> > > 
> > > So, something like:
> > > 
> > > #define __pm_ops	__section(.pm.ops)
> > > 
> > > static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
> > > 	.suspend	= my_driver_suspend,
> > > 	.resume		= my_driver_resume,
> > > 	[ blablabla ]
> > > };
> > > 
> > > to simplify things, you could:
> > > 
> > > #define DEFINE_DEV_PM_OPS(_ops)		\
> > > 	const struct dev_pm_ops _ops __pm_ops
> > > 
> > > that would mean changes to all linker scripts, though and a utility call
> > > that only does anything ifndef CONFIG_PM to free the .pm.ops section.
> > 
> > In my opinion this would make programming harder, not easier.  It's
> 
> I tend to disagree with this statement, see below.
> 
> > very easy to understand "#ifdef" followed by "#endif"; people see them
> 
> very true... Still everybody has to put them in place.

True.  But with your suggestion, people have to remember to use 
__pm_ops and DEFINE_DEV_PM_OPS.

> > all the time.  The new tags you propose would force people to go
> > searching through tons of source files to see what they mean, and then
> 
> only those who want to see "how things work" would be forced to do that,
> other people would be allowed to "assume it's doing the right thing".

But what is the "right thing"?  Suppose you want to have conditional 
support for dev_pm_ops whenever CONFIG_PM is enabled and you _also_ 
want to have conditional support for runtime PM whenever 
CONFIG_PM_RUNTIME is enabled?

> > readers would still have to figure out when these tags should be used
> > or what advantage they might bring.
> 
> not really, if you add a macro which adds that correctly and during
> review we only accept drivers using that particular macro, things
> wouldn't go bad at all.
> 
> > It's a little like "typedef struct foo foo_t;" -- doing this forces
> 
> hey c'mon. Then you're saying that all __initdata, __devinitdata,
> __initconst and all of those are "typedef struct foo foo_t" ;-)

No.  Unlike foo_t, they don't obscure important information and they do 
provide a significant gain in simplicity.  On the other hand, they also 
provide a certain degree of confusion.  Remember all the difficulty we 
had with intialization code sections in the gadget framework.

> > people to remember one extra piece of information that serves no real
> > purpose except perhaps a minimal reduction in the amount of typing.  
> 
> and a guarantee that the unused data will be freed when it's really not
> needed ;-)

You can obtain that same guarantee by using #ifdef ... #endif.  Even 
better, you can guarantee that the unused data won't be present at all, 
as opposed to loaded and then freed.

> > Since the limiting factor in kernel programming is human brainpower,
> > not source file length, this is a bad tradeoff.  (Not to mention that
> 
> OTOH we are going through a big re-factor of the ARM port to reduce the
> amount of code. Not that these few characters would change much but my
> point is that amount of code also matters. So does uniformity, coding
> style, etc...
> 
> > it also obscures an important fact: A foo_t is an extended structure
> > rather than a single value.)
> 
> then it would make sense to have dev_pm_ops only defined when CONFIG_PM
> is set to force all drivers stick to a common way of handling this.
> 
> Besides, currently, everybody who wants to keep the ifdeferry, needs to
> define a macro for &my_dev_pm_ops or have two #ifdef..#endif blocks.
> 
> Either you do:
> 
> #ifdef CONFIG_PM
> static int my_driver_suspend(struct device *dev)
> {
> 	...
> 
> 	return 0;
> }
> ....
> 
> static const struct dev_pm_ops my_driver_pm_ops = {
> 	.suspend	= my_driver_suspend,
> 	...
> };
> 
> #define DEV_PM_OPS	(&my_driver_pm_ops)
> #else
> #define DEV_PM_OPS	NULL
> #endif
> 
> static struct platform_driver my_driver = {
> 	...
> 	.driver	= {
> 		.pm = DEV_PM_OPS
> 	},
> };
> 
> or you do:
> 
> #ifdef CONFIG_PM
> static int my_driver_suspend(struct device *dev)
> {
> 	...
> 
> 	return 0;
> }
> ....
> 
> static const struct dev_pm_ops my_driver_pm_ops = {
> 	.suspend	= my_driver_suspend,
> 	...
> };
> 
> #endif
> 
> static struct platform_driver my_driver = {
> 	...
> 	.driver	= {
> #ifdef CONFIG_PM
> 		.pm = &my_driver_pm_ops,
> #endif
> 	},
> };

Whereas your way people write:

static int __pm_ops my_driver_suspend(struct device *dev)
{
	...

	return 0;
}
....

static DEFINE_DEV_PM_OPS(my_driver_pm_ops) = {
	.suspend	= my_driver_suspend,
	...
};

static struct platform_driver my_driver = {
	...
	.driver	= {
		.pm = &my_driver_pm_ops,
	},
};

It doesn't seem like a good idea to keep the invalid pointer to 
my_driver_pm_ops, even though it should never get used.

An approach that might work better would be for the PM core to define a 
suitable macro:

#ifdef CONFIG_PM
	#define DEV_PM_OPS_REF(my_pm_ops)	&(my_pm_ops)
#else
	#define DEV_PM_OPS_REF(my_pm_ops)	NULL
#endif

Then people could write

static struct platform_driver my_driver = {
	...
	.driver	= {
		.pm = DEV_PM_OPS_REF(my_driver_pm_ops),
	},
};

without worrying about whether or not my_driver_pm_ops was defined.  
And only one #ifdef block would be needed.

> So, while this is a small thing which is easy to understand, it's still
> yet another thing that all drivers have to remember to add. And when
> everybody needs to remember that, I'd rather have it done
> "automatically" by other means.
> 
> I mean, we already free .init.* sections after __init anyway, so what's
> the problem in freeing another section ? I don't see it as obfuscation
> at all. I see it as if the kernel is smart enough to free all unused
> data by itself, without myself having to add ifdefs or freeing it by my
> own.
> 
> On top of all that, today, we have driver with both ways of ifdefs plus
> drivers with no ifdeferry at all, leaving dev_pm_ops floating around for
> nothing.
> 
> IMHO, if things aren't uniform, we will have small problems, such as
> this, proliferate because new drivers are based on other drivers,
> generally.

I have to agree that uniformity is to be desired.  And it's probably 
already way too late, because we can't prevent new drivers from being 
based on the existing drivers -- even if all the existing drivers get 
changed over (which seems unlikely).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi June 6, 2011, 5:25 p.m. UTC | #12

Hi,

On Mon, Jun 06, 2011 at 12:06:44PM -0400, Alan Stern wrote:
> > > > So, something like:
> > > > 
> > > > #define __pm_ops	__section(.pm.ops)
> > > > 
> > > > static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
> > > > 	.suspend	= my_driver_suspend,
> > > > 	.resume		= my_driver_resume,
> > > > 	[ blablabla ]
> > > > };
> > > > 
> > > > to simplify things, you could:
> > > > 
> > > > #define DEFINE_DEV_PM_OPS(_ops)		\
> > > > 	const struct dev_pm_ops _ops __pm_ops
> > > > 
> > > > that would mean changes to all linker scripts, though and a utility call
> > > > that only does anything ifndef CONFIG_PM to free the .pm.ops section.
> > > 
> > > In my opinion this would make programming harder, not easier.  It's
> > 
> > I tend to disagree with this statement, see below.
> > 
> > > very easy to understand "#ifdef" followed by "#endif"; people see them
> > 
> > very true... Still everybody has to put them in place.
> 
> True.  But with your suggestion, people have to remember to use 
> __pm_ops and DEFINE_DEV_PM_OPS.

Ok, I see your point here.

> > > all the time.  The new tags you propose would force people to go
> > > searching through tons of source files to see what they mean, and then
> > 
> > only those who want to see "how things work" would be forced to do that,
> > other people would be allowed to "assume it's doing the right thing".
> 
> But what is the "right thing"?  Suppose you want to have conditional 
> support for dev_pm_ops whenever CONFIG_PM is enabled and you _also_ 
> want to have conditional support for runtime PM whenever 
> CONFIG_PM_RUNTIME is enabled?

we don't have this today either. Currently everybody does #ifdef
CONFIG_PM, so either all the data is available, or none is and adding
another #ifdef CONFIG_PM_RUNTIME for the runtime_* friends, would just
look even uglier :-)

> > > readers would still have to figure out when these tags should be used
> > > or what advantage they might bring.
> > 
> > not really, if you add a macro which adds that correctly and during
> > review we only accept drivers using that particular macro, things
> > wouldn't go bad at all.
> > 
> > > It's a little like "typedef struct foo foo_t;" -- doing this forces
> > 
> > hey c'mon. Then you're saying that all __initdata, __devinitdata,
> > __initconst and all of those are "typedef struct foo foo_t" ;-)
> 
> No.  Unlike foo_t, they don't obscure important information and they do 
> provide a significant gain in simplicity.  On the other hand, they also 
> provide a certain degree of confusion.  Remember all the difficulty we 
> had with intialization code sections in the gadget framework.

this is fairly true, but only because the gadget framework isn't really
a framework. It's just an agreement that all UDCs will export a
particular function. It's a great infrastructure for the function
drivers, but not for UDCs, so I think this isn't a great example :-)

> > > people to remember one extra piece of information that serves no real
> > > purpose except perhaps a minimal reduction in the amount of typing.  
> > 
> > and a guarantee that the unused data will be freed when it's really not
> > needed ;-)
> 
> You can obtain that same guarantee by using #ifdef ... #endif.  Even 
> better, you can guarantee that the unused data won't be present at all, 
> as opposed to loaded and then freed.

I agree with you here, but I give you the same question as you gave me.
How will you have conditional on CONFIG_RUNTIME_PM and CONFIG_PM ? you'd
need two levels of ifdefs.

> > > Since the limiting factor in kernel programming is human brainpower,
> > > not source file length, this is a bad tradeoff.  (Not to mention that
> > 
> > OTOH we are going through a big re-factor of the ARM port to reduce the
> > amount of code. Not that these few characters would change much but my
> > point is that amount of code also matters. So does uniformity, coding
> > style, etc...
> > 
> > > it also obscures an important fact: A foo_t is an extended structure
> > > rather than a single value.)
> > 
> > then it would make sense to have dev_pm_ops only defined when CONFIG_PM
> > is set to force all drivers stick to a common way of handling this.
> > 
> > Besides, currently, everybody who wants to keep the ifdeferry, needs to
> > define a macro for &my_dev_pm_ops or have two #ifdef..#endif blocks.
> > 
> > Either you do:
> > 
> > #ifdef CONFIG_PM
> > static int my_driver_suspend(struct device *dev)
> > {
> > 	...
> > 
> > 	return 0;
> > }
> > ....
> > 
> > static const struct dev_pm_ops my_driver_pm_ops = {
> > 	.suspend	= my_driver_suspend,
> > 	...
> > };
> > 
> > #define DEV_PM_OPS	(&my_driver_pm_ops)
> > #else
> > #define DEV_PM_OPS	NULL
> > #endif
> > 
> > static struct platform_driver my_driver = {
> > 	...
> > 	.driver	= {
> > 		.pm = DEV_PM_OPS
> > 	},
> > };
> > 
> > or you do:
> > 
> > #ifdef CONFIG_PM
> > static int my_driver_suspend(struct device *dev)
> > {
> > 	...
> > 
> > 	return 0;
> > }
> > ....
> > 
> > static const struct dev_pm_ops my_driver_pm_ops = {
> > 	.suspend	= my_driver_suspend,
> > 	...
> > };
> > 
> > #endif
> > 
> > static struct platform_driver my_driver = {
> > 	...
> > 	.driver	= {
> > #ifdef CONFIG_PM
> > 		.pm = &my_driver_pm_ops,
> > #endif
> > 	},
> > };
> 
> Whereas your way people write:
> 
> static int __pm_ops my_driver_suspend(struct device *dev)
> {
> 	...
> 
> 	return 0;
> }
> ....
> 
> static DEFINE_DEV_PM_OPS(my_driver_pm_ops) = {
> 	.suspend	= my_driver_suspend,
> 	...
> };
> 
> static struct platform_driver my_driver = {
> 	...
> 	.driver	= {
> 		.pm = &my_driver_pm_ops,
> 	},
> };
> 
> It doesn't seem like a good idea to keep the invalid pointer to 
> my_driver_pm_ops, even though it should never get used.

true, I agree.

> An approach that might work better would be for the PM core to define a 
> suitable macro:
> 
> #ifdef CONFIG_PM
> 	#define DEV_PM_OPS_REF(my_pm_ops)	&(my_pm_ops)
> #else
> 	#define DEV_PM_OPS_REF(my_pm_ops)	NULL
> #endif
> 
> Then people could write
> 
> static struct platform_driver my_driver = {
> 	...
> 	.driver	= {
> 		.pm = DEV_PM_OPS_REF(my_driver_pm_ops),
> 	},
> };
> 
> without worrying about whether or not my_driver_pm_ops was defined.  
> And only one #ifdef block would be needed.

that'd be nice. Something similar to __exit_p() and __devexit_p()

> > So, while this is a small thing which is easy to understand, it's still
> > yet another thing that all drivers have to remember to add. And when
> > everybody needs to remember that, I'd rather have it done
> > "automatically" by other means.
> > 
> > I mean, we already free .init.* sections after __init anyway, so what's
> > the problem in freeing another section ? I don't see it as obfuscation
> > at all. I see it as if the kernel is smart enough to free all unused
> > data by itself, without myself having to add ifdefs or freeing it by my
> > own.
> > 
> > On top of all that, today, we have driver with both ways of ifdefs plus
> > drivers with no ifdeferry at all, leaving dev_pm_ops floating around for
> > nothing.
> > 
> > IMHO, if things aren't uniform, we will have small problems, such as
> > this, proliferate because new drivers are based on other drivers,
> > generally.
> 
> I have to agree that uniformity is to be desired.  And it's probably 
> already way too late, because we can't prevent new drivers from being 

I wouldn't call it late. Such small convertions can be done by simple
scripts, but when patches switching drivers over are rejected [1] then
we loose the opportunity to give good example to newcomers.

> based on the existing drivers -- even if all the existing drivers get 
> changed over (which seems unlikely).

Well, it might work out if pm core makes dev_pm_ops only available on
CONFIG_PM builds.

[1] http://marc.info/?l=linux-usb&m=129733927804315&w=2

Alan Stern June 6, 2011, 6:03 p.m. UTC | #13

On Mon, 6 Jun 2011, Felipe Balbi wrote:

> > But what is the "right thing"?  Suppose you want to have conditional 
> > support for dev_pm_ops whenever CONFIG_PM is enabled and you _also_ 
> > want to have conditional support for runtime PM whenever 
> > CONFIG_PM_RUNTIME is enabled?
> 
> we don't have this today either. Currently everybody does #ifdef
> CONFIG_PM, so either all the data is available, or none is and adding
> another #ifdef CONFIG_PM_RUNTIME for the runtime_* friends, would just
> look even uglier :-)

Like in hcd-pci.c?  :-)

> > You can obtain that same guarantee by using #ifdef ... #endif.  Even 
> > better, you can guarantee that the unused data won't be present at all, 
> > as opposed to loaded and then freed.
> 
> I agree with you here, but I give you the same question as you gave me.
> How will you have conditional on CONFIG_RUNTIME_PM and CONFIG_PM ? you'd
> need two levels of ifdefs.

Well, you'd need more #ifdefs, no question about that.  Whether you 
need more _levels_ of #ifdefs is unclear.

> > #ifdef CONFIG_PM
> > 	#define DEV_PM_OPS_REF(my_pm_ops)	&(my_pm_ops)
> > #else
> > 	#define DEV_PM_OPS_REF(my_pm_ops)	NULL
> > #endif
> > 
> > Then people could write
> > 
> > static struct platform_driver my_driver = {
> > 	...
> > 	.driver	= {
> > 		.pm = DEV_PM_OPS_REF(my_driver_pm_ops),
> > 	},
> > };
> > 
> > without worrying about whether or not my_driver_pm_ops was defined.  
> > And only one #ifdef block would be needed.
> 
> that'd be nice. Something similar to __exit_p() and __devexit_p()

Right.  Maybe even call it __pm_ops_p().

In fact, rather than tying this specifically to dev_pm_ops, it would 
make sense to have a general-purpose memory section for code that won't 
be used, and an appropriate macro (such as "__unused") to specify that 
section attribute.  Then the PM core could do:

#ifdef CONFIG_PM
	#define __pm_ops
#else
	#define __pm_ops	__unused
#endif

and that would (I think) put less of a mental burden on people.

> Well, it might work out if pm core makes dev_pm_ops only available on
> CONFIG_PM builds.

Currently the .pm member is part of struct bus_type, struct
device_driver, and others whether CONFIG_PM is enabled or not.  I don't
know if removing it when CONFIG_PM is disabled would cause build
problems -- it might.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Munegowda, Keshava June 29, 2011, 3:22 p.m. UTC | #14

On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <[e-mail]> wrote:
> Keshava Munegowda <[e-mail]> writes:
>
>> From: Keshava Munegowda <[e-mail]>
>>
>> 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 <[e-mail]>
>
> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Munegowda, Keshava June 29, 2011, 4:37 p.m. UTC | #15

On Wed, Jun 29, 2011 at 8:52 PM, Munegowda, Keshava
<[e-mail]> wrote:
> On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <[e-mail]> wrote:
>> Keshava Munegowda <[e-mail]> writes:
>>
>>> From: Keshava Munegowda <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>
>> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Stern June 29, 2011, 5:33 p.m. UTC | #16

On Wed, 29 Jun 2011, Munegowda, Keshava wrote:

> 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?

Because the PM domain takes precedence over the subsystem for PM 
callbacks.  If the subsystem routine should be called then the PM 
domain code has to call it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Basak, Partha June 29, 2011, 6:17 p.m. UTC | #17

>-----Original Message-----
>From: Alan Stern [mailto:[e-mail]]
>Sent: Wednesday, June 29, 2011 11:03 PM
>To: Munegowda, Keshava
>Cc: Kevin Hilman; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]; [e-mail]; b-
>[e-mail]; [e-mail]
>Subject: Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci
>and ohci
>
>On Wed, 29 Jun 2011, Munegowda, Keshava wrote:
>
>> 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?
>
>Because the PM domain takes precedence over the subsystem for PM
>callbacks.  If the subsystem routine should be called then the PM
>domain code has to call it.

This is taken care of in the pm-domain code:
static int _od_runtime_resume(struct device *dev)
{
	struct platform_device *pdev = to_platform_device(dev);

	omap_device_enable(pdev);

	return pm_generic_runtime_resume(dev);
}
pm_generic_runtime_resume will in turn call the driver call back.

int pm_generic_runtime_resume(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
NULL;
	int ret;

	ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;

	return ret;
}
>
>Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Stern June 29, 2011, 6:47 p.m. UTC | #18

On Wed, 29 Jun 2011, Partha Basak wrote:

> >-----Original Message-----
> >From: Alan Stern [mailto:[e-mail]]
> >Sent: Wednesday, June 29, 2011 11:03 PM
> >To: Munegowda, Keshava
> >Cc: Kevin Hilman; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; [e-mail];
> >[e-mail]; [e-mail]; [e-mail]; b-
> >[e-mail]; [e-mail]
> >Subject: Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci
> >and ohci
> >
> >On Wed, 29 Jun 2011, Munegowda, Keshava wrote:
> >
> >> 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?
> >
> >Because the PM domain takes precedence over the subsystem for PM
> >callbacks.  If the subsystem routine should be called then the PM
> >domain code has to call it.
> 
> This is taken care of in the pm-domain code:
> static int _od_runtime_resume(struct device *dev)
> {
> 	struct platform_device *pdev = to_platform_device(dev);
> 
> 	omap_device_enable(pdev);
> 
> 	return pm_generic_runtime_resume(dev);
> }
> pm_generic_runtime_resume will in turn call the driver call back.
> 
> int pm_generic_runtime_resume(struct device *dev)
> {
> 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
> NULL;
> 	int ret;
> 
> 	ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
> 
> 	return ret;
> }

You appear to be contradicting what Keshava wrote: "due to this; the
driver->runtime_resume was not getting called."

You can't both be right.

Alan Stern


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman June 29, 2011, 7:20 p.m. UTC | #19

"Munegowda, Keshava" <[e-mail]> writes:

> On Wed, Jun 29, 2011 at 8:52 PM, Munegowda, Keshava
> <[e-mail]> wrote:
>> On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <[e-mail]> wrote:
>>> Keshava Munegowda <[e-mail]> writes:
>>>
>>>> From: Keshava Munegowda <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>
>>> 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?

Because that's how it was designed. :)

On OMAP, for on-chip devices (omap_devices) we use PM domains
(pwr_domain) and not the subsystem (bus) to implment runtime PM, and as
Alan pointed out, PM domains have precedence over subsystem callbacks.

I'm curious why you determined the driver's runtime resume was not
getting called?

The PM domain callback will call your driver's runtime_resume (assuming
it exists.)

rpm_resume()
   dev->pwr_domain->ops.runtime_resume()
       omap_device_enable()
       pm_generic_runtime_resume()
          dev->driver->pm->runtime_resume()

Note that the PM domain implementation is done at the omap_device
level.  Specifically, see plat-omap/omap_device.c:_od_runtime_resume()

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Munegowda, Keshava June 30, 2011, 12:40 p.m. UTC | #20

On Thu, Jun 30, 2011 at 12:50 AM, Kevin Hilman <[e-mail]> wrote:
> "Munegowda, Keshava" <[e-mail]> writes:
>
>> On Wed, Jun 29, 2011 at 8:52 PM, Munegowda, Keshava
>> <[e-mail]> wrote:
>>> On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <[e-mail]> wrote:
>>>> Keshava Munegowda <[e-mail]> writes:
>>>>
>>>>> From: Keshava Munegowda <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>
>>>> 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?
>
> Because that's how it was designed. :)
>
> On OMAP, for on-chip devices (omap_devices) we use PM domains
> (pwr_domain) and not the subsystem (bus) to implment runtime PM, and as
> Alan pointed out, PM domains have precedence over subsystem callbacks.
>
> I'm curious why you determined the driver's runtime resume was not
> getting called?
>
> The PM domain callback will call your driver's runtime_resume (assuming
> it exists.)
>
> rpm_resume()
>   dev->pwr_domain->ops.runtime_resume()
>       omap_device_enable()
>       pm_generic_runtime_resume()
>          dev->driver->pm->runtime_resume()
>
> Note that the PM domain implementation is done at the omap_device
> level.  Specifically, see plat-omap/omap_device.c:_od_runtime_resume()
>
> Kevin

Thanks to partha and others

I was an rc issue; I migrated to latest kernel ; its working now.
driver runtime call backs are getting
called now. :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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
+
 struct usbhs_hcd_omap {
 	struct clk			*xclk60mhsp1_ck;
 	struct clk			*xclk60mhsp2_ck;
@@ -165,6 +169,7 @@  struct usbhs_hcd_omap {
 	u32				usbhs_rev;
 	spinlock_t			lock;
 	int				count;
+	unsigned long			state;
 };
 /*-------------------------------------------------------------------------*/
 
@@ -809,6 +814,8 @@  static int usbhs_enable(struct device *dev)
 				(pdata->ehci_data->reset_gpio_port[1], 1);
 	}
 
+	set_bit(OMAP_USBHS_INIT, &omap->state);
+
 end_count:
 	omap->count++;
 	spin_unlock_irqrestore(&omap->lock, flags);
@@ -897,6 +904,7 @@  static void usbhs_disable(struct device *dev)
 	}
 
 	pm_runtime_put_sync(dev);
+	clear_bit(OMAP_USBHS_INIT, &omap->state);
 
 	/* The gpio_free migh sleep; so unlock the spinlock */
 	spin_unlock_irqrestore(&omap->lock, flags);
@@ -926,10 +934,105 @@  void omap_usbhs_disable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(omap_usbhs_disable);
 
+#ifdef	CONFIG_PM
+
+static int usbhs_resume(struct device *dev)
+{
+	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
+	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
+	unsigned long			flags = 0;
+
+	dev_dbg(dev, "Resuming TI HSUSB Controller\n");
+
+	if (!pdata) {
+		dev_dbg(dev, "missing platform_data\n");
+		return  -ENODEV;
+	}
+
+	spin_lock_irqsave(&omap->lock, flags);
+
+	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
+		!test_bit(OMAP_USBHS_SUSPEND, &omap->state))
+			goto end_resume;
+
+	pm_runtime_get_sync(dev);
+
+	if (is_omap_usbhs_rev2(omap)) {
+		if (is_ehci_tll_mode(pdata->port_mode[0])) {
+			clk_enable(omap->usbhost_p1_fck);
+			clk_enable(omap->usbtll_p1_fck);
+		}
+		if (is_ehci_tll_mode(pdata->port_mode[1])) {
+			clk_enable(omap->usbhost_p2_fck);
+			clk_enable(omap->usbtll_p2_fck);
+		}
+		clk_enable(omap->utmi_p1_fck);
+		clk_enable(omap->utmi_p2_fck);
+	}
+	clear_bit(OMAP_USBHS_SUSPEND, &omap->state);
+
+end_resume:
+	spin_unlock_irqrestore(&omap->lock, flags);
+	return 0;
+}
+
+
+static int usbhs_suspend(struct device *dev)
+{
+	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
+	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
+	unsigned long			flags = 0;
+
+	dev_dbg(dev, "Suspending TI HSUSB Controller\n");
+
+	if (!pdata) {
+		dev_dbg(dev, "missing platform_data\n");
+		return  -ENODEV;
+	}
+
+	spin_lock_irqsave(&omap->lock, flags);
+
+	if (!test_bit(OMAP_USBHS_INIT, &omap->state) ||
+		test_bit(OMAP_USBHS_SUSPEND, &omap->state))
+			goto end_suspend;
+
+	if (is_omap_usbhs_rev2(omap)) {
+		if (is_ehci_tll_mode(pdata->port_mode[0])) {
+			clk_disable(omap->usbhost_p1_fck);
+			clk_disable(omap->usbtll_p1_fck);
+		}
+		if (is_ehci_tll_mode(pdata->port_mode[1])) {
+			clk_disable(omap->usbhost_p2_fck);
+			clk_disable(omap->usbtll_p2_fck);
+		}
+		clk_disable(omap->utmi_p2_fck);
+		clk_disable(omap->utmi_p1_fck);
+	}
+
+	set_bit(OMAP_USBHS_SUSPEND, &omap->state);
+	pm_runtime_put_sync(dev);
+
+end_suspend:
+	spin_unlock_irqrestore(&omap->lock, flags);
+	return 0;
+}
+
+
+static const struct dev_pm_ops usbhsomap_dev_pm_ops = {
+	.suspend	= usbhs_suspend,
+	.resume		= usbhs_resume,
+};
+
+#define USBHS_OMAP_DEV_PM_OPS (&usbhsomap_dev_pm_ops)
+#else
+#define	USBHS_OMAP_DEV_PM_OPS	NULL
+#endif
+
 static struct platform_driver usbhs_omap_driver = {
 	.driver = {
 		.name		= (char *)usbhs_driver_name,
 		.owner		= THIS_MODULE,
+		.pm		= USBHS_OMAP_DEV_PM_OPS,
 	},
 	.remove		= __exit_p(usbhs_omap_remove),
 };