[PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Hema HK]

Calling runtime pm APIs pm_runtime_put_sync() and pm_runtime_get_sync()
for enabling/disabling the clocks,sysconfig settings.

Also need to put the USB in force standby and force idle mode when usb not used
and set it back to no idle and no stndby after wakeup.
For OMAP3 auto idle bit has to be disabled because of the errata.So using
HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.

Signed-off-by: Hema HK <hemahk@ti.com>
Signed-off-by: Basak, Partha <p-basak2@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Cousson, Benoit <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---

 drivers/usb/musb/musb_core.c |   26 ++++++++++++++++++++++++
 drivers/usb/musb/omap2430.c  |   45 ++++++-------------------------------------
 2 files changed, 33 insertions(+), 38 deletions(-)

Index: linux-omap-pm/drivers/usb/musb/musb_core.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/musb_core.c
+++ linux-omap-pm/drivers/usb/musb/musb_core.c
@@ -98,6 +98,7 @@
 #include <linux/kobject.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
 
 #ifdef	CONFIG_ARM
 #include <mach/hardware.h>
@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
 		 * they will even be wakeup-enabled.
 		 */
 	}
+	pm_runtime_put_sync(dev);
 
+#ifndef CONFIG_PM_RUNTIME
 	musb_save_context(musb);
 
 	if (musb->set_clock)
 		musb->set_clock(musb->clock, 0);
 	else
 		clk_disable(musb->clock);
+#endif
 	spin_unlock_irqrestore(&musb->lock, flags);
 	return 0;
 }
@@ -2443,12 +2447,16 @@ static int musb_resume_noirq(struct devi
 	if (!musb->clock)
 		return 0;
 
+	pm_runtime_get_sync(dev);
+
+#ifndef CONFIG_PM_RUNTIME
 	if (musb->set_clock)
 		musb->set_clock(musb->clock, 1);
 	else
 		clk_enable(musb->clock);
 
 	musb_restore_context(musb);
+#endif
 
 	/* for static cmos like DaVinci, register values were preserved
 	 * unless for some reason the whole soc powered down or the USB
@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
 	return 0;
 }
 
+static int musb_runtime_suspend(struct device *dev)
+{
+	struct musb	*musb = dev_to_musb(dev);
+
+	musb_save_context(musb);
+	return 0;
+}
+
+static int musb_runtime_resume(struct device *dev)
+{
+	struct musb	*musb = dev_to_musb(dev);
+
+	musb_restore_context(musb);
+	return 0;
+}
 static const struct dev_pm_ops musb_dev_pm_ops = {
 	.suspend	= musb_suspend,
 	.resume_noirq	= musb_resume_noirq,
+	.runtime_suspend = musb_runtime_suspend,
+	.runtime_resume	 = musb_runtime_resume,
 };
 
 #define MUSB_DEV_PM_OPS (&musb_dev_pm_ops)
Index: linux-omap-pm/drivers/usb/musb/omap2430.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/omap2430.c
+++ linux-omap-pm/drivers/usb/musb/omap2430.c
@@ -31,6 +31,8 @@
 #include <linux/list.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
 
 #include "musb_core.h"
 #include "omap2430.h"
@@ -206,21 +208,6 @@ int __init musb_platform_init(struct mus
 
 	musb_platform_resume(musb);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	l &= ~NOSTDBY;		/* remove possible nostdby */
-	l |= SMARTSTDBY;	/* enable smart standby */
-	l &= ~AUTOIDLE;		/* disable auto idle */
-	l &= ~NOIDLE;		/* remove possible noidle */
-	l |= SMARTIDLE;		/* enable smart idle */
-	/*
-	 * MUSB AUTOIDLE don't work in 3430.
-	 * Workaround by Richard Woodruff/TI
-	 */
-	if (!cpu_is_omap3430())
-		l |= AUTOIDLE;		/* enable auto idle */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	l = musb_readl(musb->mregs, OTG_INTERFSEL);
 
 	if (data->interface_type == MUSB_INTERFACE_UTMI) {
@@ -253,15 +240,13 @@ int __init musb_platform_init(struct mus
 void musb_platform_save_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	musb_context->otg_sysconfig = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	musb_context->otg_forcestandby = musb_readl(musb->mregs, OTG_FORCESTDBY);
+	musb_writel(musb->mregs, OTG_FORCESTDBY, ENABLEFORCE);
 }
 
 void musb_platform_restore_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	musb_writel(musb->mregs, OTG_SYSCONFIG, musb_context->otg_sysconfig);
-	musb_writel(musb->mregs, OTG_FORCESTDBY, musb_context->otg_forcestandby);
+	musb_writel(musb->mregs, OTG_FORCESTDBY, 0);
 }
 #endif
 
@@ -277,37 +262,23 @@ static int musb_platform_suspend(struct 
 	l |= ENABLEFORCE;	/* enable MSTANDBY */
 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l |= ENABLEWAKEUP;	/* enable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	otg_set_suspend(musb->xceiv, 1);
 
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 0);
-	else
-		clk_disable(musb->clock);
-
 	return 0;
 }
 
 static int musb_platform_resume(struct musb *musb)
 {
 	u32 l;
+	struct device *dev = musb->controller;
 
 	if (!musb->clock)
 		return 0;
 
 	otg_set_suspend(musb->xceiv, 0);
 
-	if (musb->set_clock)
-		musb->set_clock(musb->clock, 1);
-	else
-		clk_enable(musb->clock);
-
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
 	l &= ~ENABLEFORCE;	/* disable MSTANDBY */

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>Calling runtime pm APIs pm_runtime_put_sync() and pm_runtime_get_sync()
>for enabling/disabling the clocks,sysconfig settings.
>
>Also need to put the USB in force standby and force idle mode when usb not used
>and set it back to no idle and no stndby after wakeup.
>For OMAP3 auto idle bit has to be disabled because of the errata.So using
>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>
>Signed-off-by: Hema HK <hemahk@ti.com>
>Signed-off-by: Basak, Partha <p-basak2@ti.com>
>Cc: Felipe Balbi <balbi@ti.com>
>Cc: Tony Lindgren <tony@atomide.com>
>Cc: Kevin Hilman <khilman@deeprootsystems.com>
>Cc: Cousson, Benoit <b-cousson@ti.com>
>Cc: Paul Walmsley <paul@pwsan.com>

these two should a separate series, otherwise it's difficult for
different maintainers to decide what they need to pick up :-)

Anyways, let me continue;

>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
> 		 * they will even be wakeup-enabled.
> 		 */
> 	}
>+	pm_runtime_put_sync(dev);
>
>+#ifndef CONFIG_PM_RUNTIME
> 	musb_save_context(musb);
>
> 	if (musb->set_clock)
> 		musb->set_clock(musb->clock, 0);
> 	else
> 		clk_disable(musb->clock);
>+#endif

I would rather remove these, adding ifdefs is bad :-( Unless other archs
(blackfin, davinci) would have problems if we remove those.

>@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
> 	return 0;
> }
>
>+static int musb_runtime_suspend(struct device *dev)
>+{
>+	struct musb	*musb = dev_to_musb(dev);

missing lock ??

>+	musb_save_context(musb);

shouldn't you disable the clock here ?

>+	return 0;
>+}
>+
>+static int musb_runtime_resume(struct device *dev)
>+{
>+	struct musb	*musb = dev_to_musb(dev);

re-enable clock here and missing lock ??

>@@ -253,15 +240,13 @@ int __init musb_platform_init(struct mus
> void musb_platform_save_context(struct musb *musb,
> 		struct musb_context_registers *musb_context)
> {
>-	musb_context->otg_sysconfig = musb_readl(musb->mregs, OTG_SYSCONFIG);
>-	musb_context->otg_forcestandby = musb_readl(musb->mregs, OTG_FORCESTDBY);
>+	musb_writel(musb->mregs, OTG_FORCESTDBY, ENABLEFORCE);

not really saving context anymore :-p but that's ok, we will need change
all that mess anyway.

>@@ -277,37 +262,23 @@ static int musb_platform_suspend(struct
> 	l |= ENABLEFORCE;	/* enable MSTANDBY */
> 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
>
>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>-	l |= ENABLEWAKEUP;	/* enable wakeup */
>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>-
> 	otg_set_suspend(musb->xceiv, 1);
>
>-	if (musb->set_clock)
>-		musb->set_clock(musb->clock, 0);
>-	else
>-		clk_disable(musb->clock);
>-

should you pm_runtime_put_sync(dev) here ??

> 	return 0;
> }
>
> static int musb_platform_resume(struct musb *musb)
> {
> 	u32 l;
>+	struct device *dev = musb->controller;
>
> 	if (!musb->clock)
> 		return 0;

you're not touching clock anymore, this can go too.

> 	otg_set_suspend(musb->xceiv, 0);
>
>-	if (musb->set_clock)
>-		musb->set_clock(musb->clock, 1);
>-	else
>-		clk_enable(musb->clock);
>-
>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>+	pm_runtime_enable(dev);
>+	pm_runtime_get_sync(dev);

seems like you're going to call pm_runtime_get_sync twice ? once here
and another time on musb_resume_noirq(). why ?

-- 
balbi

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

 

>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Thursday, September 23, 2010 12:06 PM
>To: Kalliguddi, Hema
>Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>Basak, Partha; Balbi, Felipe; Tony Lindgren; Kevin Hilman; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Hi,
>
>On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>Calling runtime pm APIs pm_runtime_put_sync() and 
>pm_runtime_get_sync()
>>for enabling/disabling the clocks,sysconfig settings.
>>
>>Also need to put the USB in force standby and force idle mode 
>when usb not used
>>and set it back to no idle and no stndby after wakeup.
>>For OMAP3 auto idle bit has to be disabled because of the 
>errata.So using
>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>>
>>Signed-off-by: Hema HK <hemahk-l0cyMroinI0@public.gmane.org>
>>Signed-off-by: Basak, Partha <p-basak2-l0cyMroinI0@public.gmane.org>
>>Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>>Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>>Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>>Cc: Cousson, Benoit <b-cousson-l0cyMroinI0@public.gmane.org>
>>Cc: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
>
>these two should a separate series, otherwise it's difficult for
>different maintainers to decide what they need to pick up :-)
>
I don't mind 
>Anyways, let me continue;
>
>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>> 		 * they will even be wakeup-enabled.
>> 		 */
>> 	}
>>+	pm_runtime_put_sync(dev);
>>
>>+#ifndef CONFIG_PM_RUNTIME
>> 	musb_save_context(musb);
>>
>> 	if (musb->set_clock)
>> 		musb->set_clock(musb->clock, 0);
>> 	else
>> 		clk_disable(musb->clock);
>>+#endif
>
>I would rather remove these, adding ifdefs is bad :-( Unless 
>other archs
>(blackfin, davinci) would have problems if we remove those.
>
If we remove this then how the clock will be enabled for the other platforms where runtime
pm is not used?

>>@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
>> 	return 0;
>> }
>>
>>+static int musb_runtime_suspend(struct device *dev)
>>+{
>>+	struct musb	*musb = dev_to_musb(dev);
>
>missing lock ??
I am wondering whether we need the lock here? As these functions are supposed to be
Called by runtime pm framework only. 
>
>>+	musb_save_context(musb);
>
>shouldn't you disable the clock here ?
>
This hook is called when we call pm_runtime_put_sync api which takes care of disabling
clocks and configuring the sysconfig register.

>>+	return 0;
>>+}
>>+
>>+static int musb_runtime_resume(struct device *dev)
>>+{
>>+	struct musb	*musb = dev_to_musb(dev);
>
>re-enable clock here and missing lock ??
>

Not required. This hook gets aclled when pm_runtime_get_sync is called by the driver
Which will take care of enabling the clock.

I am just wondering whether we need the lock here? As these functions are supposed to be
Called by runtime pm framework only. 

>>@@ -253,15 +240,13 @@ int __init musb_platform_init(struct mus
>> void musb_platform_save_context(struct musb *musb,
>> 		struct musb_context_registers *musb_context)
>> {
>>-	musb_context->otg_sysconfig = musb_readl(musb->mregs, 
>OTG_SYSCONFIG);
>>-	musb_context->otg_forcestandby = 
>musb_readl(musb->mregs, OTG_FORCESTDBY);
>>+	musb_writel(musb->mregs, OTG_FORCESTDBY, ENABLEFORCE);
>
>not really saving context anymore :-p but that's ok, we will 
>need change
>all that mess anyway.
>
Yehh :-)

>>@@ -277,37 +262,23 @@ static int musb_platform_suspend(struct
>> 	l |= ENABLEFORCE;	/* enable MSTANDBY */
>> 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
>>
>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>-	l |= ENABLEWAKEUP;	/* enable wakeup */
>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>-
>> 	otg_set_suspend(musb->xceiv, 1);
>>
>>-	if (musb->set_clock)
>>-		musb->set_clock(musb->clock, 0);
>>-	else
>>-		clk_disable(musb->clock);
>>-
>
>should you pm_runtime_put_sync(dev) here ??
>
Calling pm_runtime_put_sync leading to crash as
driver_detach calls __device_release_driver which intern calls 
pm_runtime_put_sync function, with this the musb clocks are already disabled
And usecount is 0.
 
>> 	return 0;
>> }
>>
>> static int musb_platform_resume(struct musb *musb)
>> {
>> 	u32 l;
>>+	struct device *dev = musb->controller;
>>
>> 	if (!musb->clock)
>> 		return 0;
>
>you're not touching clock anymore, this can go too.
>
Yes. This can be removed.

>> 	otg_set_suspend(musb->xceiv, 0);
>>
>>-	if (musb->set_clock)
>>-		musb->set_clock(musb->clock, 1);
>>-	else
>>-		clk_enable(musb->clock);
>>-
>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>+	pm_runtime_enable(dev);
>>+	pm_runtime_get_sync(dev);
>
>seems like you're going to call pm_runtime_get_sync twice ? once here
>and another time on musb_resume_noirq(). why ?

pm_runtime_get_sync is called in the musb_platform_resume function 
during the initialization of the driver. Where we need to enable the clocks
and put the sysconfig registers to known configurations.

pm_runtime_get_sync is called in musb_resume_noirq() to re-enable the cloks
and configure the sysconfig because the clocks are disabled in musb_suspend()
by calling pm_runtime_put_sync() during global suspend/resume.


>
>-- 
>balbi
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

 

>-----Original Message-----
>From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
>[mailto:linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Kalliguddi, Hema
>Sent: Thursday, September 23, 2010 1:21 PM
>To: Balbi, Felipe
>Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>Basak, Partha; Tony Lindgren; Kevin Hilman; Cousson, Benoit; 
>Paul Walmsley
>Subject: RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
> 
>
>>-----Original Message-----
>>From: Balbi, Felipe 
>>Sent: Thursday, September 23, 2010 12:06 PM
>>To: Kalliguddi, Hema
>>Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>>Basak, Partha; Balbi, Felipe; Tony Lindgren; Kevin Hilman; 
>>Cousson, Benoit; Paul Walmsley
>>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis 
>for musb.
>>
>>Hi,
>>
>>On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>>Calling runtime pm APIs pm_runtime_put_sync() and 
>>pm_runtime_get_sync()
>>>for enabling/disabling the clocks,sysconfig settings.
>>>
>>>Also need to put the USB in force standby and force idle mode 
>>when usb not used
>>>and set it back to no idle and no stndby after wakeup.
>>>For OMAP3 auto idle bit has to be disabled because of the 
>>errata.So using
>>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>>>
>>>Signed-off-by: Hema HK <hemahk-l0cyMroinI0@public.gmane.org>
>>>Signed-off-by: Basak, Partha <p-basak2-l0cyMroinI0@public.gmane.org>
>>>Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>>>Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>>>Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>>>Cc: Cousson, Benoit <b-cousson-l0cyMroinI0@public.gmane.org>
>>>Cc: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
>>
>>these two should a separate series, otherwise it's difficult for
>>different maintainers to decide what they need to pick up :-)
>>
>I don't mind 

I mean I don't mind posting them as separate series. But this will have still the driver
And architecture files changes.

>>Anyways, let me continue;
>>
>>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>>> 		 * they will even be wakeup-enabled.
>>> 		 */
>>> 	}
>>>+	pm_runtime_put_sync(dev);
>>>
>>>+#ifndef CONFIG_PM_RUNTIME
>>> 	musb_save_context(musb);
>>>
>>> 	if (musb->set_clock)
>>> 		musb->set_clock(musb->clock, 0);
>>> 	else
>>> 		clk_disable(musb->clock);
>>>+#endif
>>
>>I would rather remove these, adding ifdefs is bad :-( Unless 
>>other archs
>>(blackfin, davinci) would have problems if we remove those.
>>
>If we remove this then how the clock will be enabled for the 
>other platforms where runtime
>pm is not used?
>
>>>@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
>>> 	return 0;
>>> }
>>>
>>>+static int musb_runtime_suspend(struct device *dev)
>>>+{
>>>+	struct musb	*musb = dev_to_musb(dev);
>>
>>missing lock ??
>I am wondering whether we need the lock here? As these 
>functions are supposed to be
>Called by runtime pm framework only. 
>>
>>>+	musb_save_context(musb);
>>
>>shouldn't you disable the clock here ?
>>
>This hook is called when we call pm_runtime_put_sync api which 
>takes care of disabling
>clocks and configuring the sysconfig register.
>
>>>+	return 0;
>>>+}
>>>+
>>>+static int musb_runtime_resume(struct device *dev)
>>>+{
>>>+	struct musb	*musb = dev_to_musb(dev);
>>
>>re-enable clock here and missing lock ??
>>
>
>Not required. This hook gets aclled when pm_runtime_get_sync 
>is called by the driver
>Which will take care of enabling the clock.
>
>I am just wondering whether we need the lock here? As these 
>functions are supposed to be
>Called by runtime pm framework only. 
>
>>>@@ -253,15 +240,13 @@ int __init musb_platform_init(struct mus
>>> void musb_platform_save_context(struct musb *musb,
>>> 		struct musb_context_registers *musb_context)
>>> {
>>>-	musb_context->otg_sysconfig = musb_readl(musb->mregs, 
>>OTG_SYSCONFIG);
>>>-	musb_context->otg_forcestandby = 
>>musb_readl(musb->mregs, OTG_FORCESTDBY);
>>>+	musb_writel(musb->mregs, OTG_FORCESTDBY, ENABLEFORCE);
>>
>>not really saving context anymore :-p but that's ok, we will 
>>need change
>>all that mess anyway.
>>
>Yehh :-)
>
>>>@@ -277,37 +262,23 @@ static int musb_platform_suspend(struct
>>> 	l |= ENABLEFORCE;	/* enable MSTANDBY */
>>> 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
>>>
>>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>>-	l |= ENABLEWAKEUP;	/* enable wakeup */
>>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>>-
>>> 	otg_set_suspend(musb->xceiv, 1);
>>>
>>>-	if (musb->set_clock)
>>>-		musb->set_clock(musb->clock, 0);
>>>-	else
>>>-		clk_disable(musb->clock);
>>>-
>>
>>should you pm_runtime_put_sync(dev) here ??
>>
>Calling pm_runtime_put_sync leading to crash as
>driver_detach calls __device_release_driver which intern calls 
>pm_runtime_put_sync function, with this the musb clocks are 
>already disabled
>And usecount is 0.
> 
>>> 	return 0;
>>> }
>>>
>>> static int musb_platform_resume(struct musb *musb)
>>> {
>>> 	u32 l;
>>>+	struct device *dev = musb->controller;
>>>
>>> 	if (!musb->clock)
>>> 		return 0;
>>
>>you're not touching clock anymore, this can go too.
>>
>Yes. This can be removed.
>
>>> 	otg_set_suspend(musb->xceiv, 0);
>>>
>>>-	if (musb->set_clock)
>>>-		musb->set_clock(musb->clock, 1);
>>>-	else
>>>-		clk_enable(musb->clock);
>>>-
>>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>>-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
>>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>>+	pm_runtime_enable(dev);
>>>+	pm_runtime_get_sync(dev);
>>
>>seems like you're going to call pm_runtime_get_sync twice ? once here
>>and another time on musb_resume_noirq(). why ?
>
>pm_runtime_get_sync is called in the musb_platform_resume function 
>during the initialization of the driver. Where we need to 
>enable the clocks
>and put the sysconfig registers to known configurations.
>
>pm_runtime_get_sync is called in musb_resume_noirq() to 
>re-enable the cloks
>and configure the sysconfig because the clocks are disabled in 
>musb_suspend()
>by calling pm_runtime_put_sync() during global suspend/resume.
>
>
>>
>>-- 
>>balbi
>>--
>To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Thu, Sep 23, 2010 at 02:51:12AM -0500, Kalliguddi, Hema wrote:
>>these two should a separate series, otherwise it's difficult for
>>different maintainers to decide what they need to pick up :-)
>>
>I don't mind

if you're re-sending already, please do split :-)

>>I would rather remove these, adding ifdefs is bad :-( Unless
>>other archs
>>(blackfin, davinci) would have problems if we remove those.
>>
>If we remove this then how the clock will be enabled for the other
>platforms where runtime pm is not used?

yeah, let's see what other archs will say...

>>>@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
>>> 	return 0;
>>> }
>>>
>>>+static int musb_runtime_suspend(struct device *dev)
>>>+{
>>>+	struct musb	*musb = dev_to_musb(dev);
>>
>>missing lock ??
>I am wondering whether we need the lock here? As these functions are supposed to be
>Called by runtime pm framework only.

is that done with IRQs disabled ?

>>>+	musb_save_context(musb);
>>
>>shouldn't you disable the clock here ?
>>
>This hook is called when we call pm_runtime_put_sync api which takes
>care of disabling clocks and configuring the sysconfig register.

but those are methods you have provided, right ? I mean, in the end
it'll call musb_runtime_suspend() and musb_runtime_resume() ?? Or am I
missing something ?

>>should you pm_runtime_put_sync(dev) here ??
>>
>Calling pm_runtime_put_sync leading to crash as
>driver_detach calls __device_release_driver which intern calls
>pm_runtime_put_sync function, with this the musb clocks are already disabled
>And usecount is 0.

looks weird though, I'll have to read that code more carefully when you
send another version.

>>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>>-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
>>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>>+	pm_runtime_enable(dev);
>>>+	pm_runtime_get_sync(dev);
>>
>>seems like you're going to call pm_runtime_get_sync twice ? once here
>>and another time on musb_resume_noirq(). why ?
>
>pm_runtime_get_sync is called in the musb_platform_resume function
>during the initialization of the driver. Where we need to enable the clocks
>and put the sysconfig registers to known configurations.
>
>pm_runtime_get_sync is called in musb_resume_noirq() to re-enable the cloks
>and configure the sysconfig because the clocks are disabled in musb_suspend()
>by calling pm_runtime_put_sync() during global suspend/resume.

hmm, also weird logic. I'll wait for next version and read that code
with more care.

-- 
balbi

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

Hi, 

>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Thursday, September 23, 2010 2:22 PM
>To: Kalliguddi, Hema
>Cc: Balbi, Felipe; linux-omap@vger.kernel.org; 
>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; Kevin 
>Hilman; Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Hi,
>
>On Thu, Sep 23, 2010 at 02:51:12AM -0500, Kalliguddi, Hema wrote:
>>>these two should a separate series, otherwise it's difficult for
>>>different maintainers to decide what they need to pick up :-)
>>>
>>I don't mind
>
>if you're re-sending already, please do split :-)
>

Ok. I will do that.

>>>I would rather remove these, adding ifdefs is bad :-( Unless
>>>other archs
>>>(blackfin, davinci) would have problems if we remove those.
>>>
>>If we remove this then how the clock will be enabled for the other
>>platforms where runtime pm is not used?
>
>yeah, let's see what other archs will say...
>
>>>>@@ -2457,9 +2465,26 @@ static int musb_resume_noirq(struct devi
>>>> 	return 0;
>>>> }
>>>>
>>>>+static int musb_runtime_suspend(struct device *dev)
>>>>+{
>>>>+	struct musb	*musb = dev_to_musb(dev);
>>>
>>>missing lock ??
>>I am wondering whether we need the lock here? As these 
>functions are supposed to be
>>Called by runtime pm framework only.
>
>is that done with IRQs disabled ?

At high level it was looking like the irqs are disabled by looking below function.

int pm_runtime_resume(struct device *dev)
{
	int retval;

	spin_lock_irq(&dev->power.lock);
	retval = __pm_runtime_resume(dev, false);
	spin_unlock_irq(&dev->power.lock);

	return retval;
}

But after going through further details, before calling the driver's 
pm->runtime_suspend/resume functions the irqs are enabled back.
So we need to have lock.

>
>>>>+	musb_save_context(musb);
>>>
>>>shouldn't you disable the clock here ?
>>>
>>This hook is called when we call pm_runtime_put_sync api which takes
>>care of disabling clocks and configuring the sysconfig register.
>
>but those are methods you have provided, right ? I mean, in the end
>it'll call musb_runtime_suspend() and musb_runtime_resume() ?? Or am I
>missing something ?

The flow when you call pm_runtime_put_sync is as below.

1. Calls the driver's runtime_suspend hook
2. Configures the sysconfig
3. Disable the clocks.

The driver's runtime_suspend/resume hooks are provided to prepare the
Device for the idle incase if there is something to be done. For 
Example you can use it for storing the context and restoring the context.
Since framework is taking of diabling the clock when runtime pm funtions are
called so there is no need of disabling the clock here.

>
>>>should you pm_runtime_put_sync(dev) here ??
>>>
>>Calling pm_runtime_put_sync leading to crash as
>>driver_detach calls __device_release_driver which intern calls
>>pm_runtime_put_sync function, with this the musb clocks are 
>already disabled
>>And usecount is 0.
>
>looks weird though, I'll have to read that code more carefully when you
>send another version.
>
OK.

>>>>-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
>>>>-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
>>>>-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
>>>>+	pm_runtime_enable(dev);
>>>>+	pm_runtime_get_sync(dev);
>>>
>>>seems like you're going to call pm_runtime_get_sync twice ? once here
>>>and another time on musb_resume_noirq(). why ?
>>
>>pm_runtime_get_sync is called in the musb_platform_resume function
>>during the initialization of the driver. Where we need to 
>enable the clocks
>>and put the sysconfig registers to known configurations.
>>
>>pm_runtime_get_sync is called in musb_resume_noirq() to 
>re-enable the cloks
>>and configure the sysconfig because the clocks are disabled 
>in musb_suspend()
>>by calling pm_runtime_put_sync() during global suspend/resume.
>
>hmm, also weird logic. I'll wait for next version and read that code
>with more care.
>

musb_platform_resume functions gets executed when the driver is loaded.
Clock hs to be enabled before accessing the registers so calling the
pm_runtime_get_sync api to do the same.

When there is global suspend initiated, driver's pm_ops functions
Musb_suspend api is called and the clocks are disabled.
Now when the system resumes after wakeup,
Musb_resume_noirq is called, now need to re-enable the clocks so calling
pm_runtime_get_sync function.

Do you see wny problem with it?

>-- 
>balbi
>

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Thu, Sep 23, 2010 at 06:11:22AM -0500, Kalliguddi, Hema wrote:
>When there is global suspend initiated, driver's pm_ops functions
>Musb_suspend api is called and the clocks are disabled.
>Now when the system resumes after wakeup,
>Musb_resume_noirq is called, now need to re-enable the clocks so calling
>pm_runtime_get_sync function.
>
>Do you see wny problem with it?

ok, now I get it. It's just that platform_suspend/resume() are actually
only used on platform_init/exit() :-p

After we apply this, I'll queue patches to fix up that goofy method
name.

-- 
balbi

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kevin Hilman]

Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> writes:

> Hi,
>
> On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>Calling runtime pm APIs pm_runtime_put_sync() and pm_runtime_get_sync()
>>for enabling/disabling the clocks,sysconfig settings.
>>
>>Also need to put the USB in force standby and force idle mode when usb not used
>>and set it back to no idle and no stndby after wakeup.
>>For OMAP3 auto idle bit has to be disabled because of the errata.So using
>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.

[...]

>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>> 		 * they will even be wakeup-enabled.
>> 		 */
>> 	}
>>+	pm_runtime_put_sync(dev);
>>
>>+#ifndef CONFIG_PM_RUNTIME
>> 	musb_save_context(musb);
>>
>> 	if (musb->set_clock)
>> 		musb->set_clock(musb->clock, 0);
>> 	else
>> 		clk_disable(musb->clock);
>>+#endif
>
> I would rather remove these, adding ifdefs is bad :-( Unless other archs
> (blackfin, davinci) would have problems if we remove those.

I didn't like these #ifdefs either, but davinci doesn't have runtime PM,
and I don't think blackfin does either.

But, rather than the ifdef here, this could be done with different
pointers in struct dev_pm_ops based on the arch.

Also, this shouldn't be based on CONFIG_PM_RUNTIME, but rather on the
arch.  We can still enable runtime PM on davinci for other subsystems
(PCI, USB core, etc.) but not have it supported for on-chip devices.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

 

>-----Original Message-----
>From: Kevin Hilman [mailto:khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org] 
>Sent: Thursday, September 23, 2010 9:00 PM
>To: Balbi, Felipe
>Cc: Kalliguddi, Hema; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> writes:
>
>> Hi,
>>
>> On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>>Calling runtime pm APIs pm_runtime_put_sync() and 
>pm_runtime_get_sync()
>>>for enabling/disabling the clocks,sysconfig settings.
>>>
>>>Also need to put the USB in force standby and force idle 
>mode when usb not used
>>>and set it back to no idle and no stndby after wakeup.
>>>For OMAP3 auto idle bit has to be disabled because of the 
>errata.So using
>>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>
>[...]
>
>>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>>> 		 * they will even be wakeup-enabled.
>>> 		 */
>>> 	}
>>>+	pm_runtime_put_sync(dev);
>>>
>>>+#ifndef CONFIG_PM_RUNTIME
>>> 	musb_save_context(musb);
>>>
>>> 	if (musb->set_clock)
>>> 		musb->set_clock(musb->clock, 0);
>>> 	else
>>> 		clk_disable(musb->clock);
>>>+#endif
>>
>> I would rather remove these, adding ifdefs is bad :-( Unless 
>other archs
>> (blackfin, davinci) would have problems if we remove those.
>
>I didn't like these #ifdefs either, but davinci doesn't have 
>runtime PM,
>and I don't think blackfin does either.
>
>But, rather than the ifdef here, this could be done with different
>pointers in struct dev_pm_ops based on the arch.
>
>Also, this shouldn't be based on CONFIG_PM_RUNTIME, but rather on the
>arch.  We can still enable runtime PM on davinci for other subsystems
>(PCI, USB core, etc.) but not have it supported for on-chip devices.
>
Is it a good idea to use the musb->set_clock function pointer for OMAP architure and
call the runtime pm apis from this function defined in the usb platform driver(omap2430)

>Kevin
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kevin Hilman]

"Kalliguddi, Hema" <hemahk@ti.com> writes:

>  
>
>>-----Original Message-----
>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>Sent: Thursday, September 23, 2010 9:00 PM
>>To: Balbi, Felipe
>>Cc: Kalliguddi, Hema; linux-omap@vger.kernel.org; 
>>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; 
>>Cousson, Benoit; Paul Walmsley
>>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>>
>>Felipe Balbi <balbi@ti.com> writes:
>>
>>> Hi,
>>>
>>> On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>>>Calling runtime pm APIs pm_runtime_put_sync() and 
>>pm_runtime_get_sync()
>>>>for enabling/disabling the clocks,sysconfig settings.
>>>>
>>>>Also need to put the USB in force standby and force idle 
>>mode when usb not used
>>>>and set it back to no idle and no stndby after wakeup.
>>>>For OMAP3 auto idle bit has to be disabled because of the 
>>errata.So using
>>>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>>
>>[...]
>>
>>>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>>>> 		 * they will even be wakeup-enabled.
>>>> 		 */
>>>> 	}
>>>>+	pm_runtime_put_sync(dev);
>>>>
>>>>+#ifndef CONFIG_PM_RUNTIME
>>>> 	musb_save_context(musb);
>>>>
>>>> 	if (musb->set_clock)
>>>> 		musb->set_clock(musb->clock, 0);
>>>> 	else
>>>> 		clk_disable(musb->clock);
>>>>+#endif
>>>
>>> I would rather remove these, adding ifdefs is bad :-( Unless 
>>other archs
>>> (blackfin, davinci) would have problems if we remove those.
>>
>>I didn't like these #ifdefs either, but davinci doesn't have 
>>runtime PM,
>>and I don't think blackfin does either.
>>
>>But, rather than the ifdef here, this could be done with different
>>pointers in struct dev_pm_ops based on the arch.
>>
>>Also, this shouldn't be based on CONFIG_PM_RUNTIME, but rather on the
>>arch.  We can still enable runtime PM on davinci for other subsystems
>>(PCI, USB core, etc.) but not have it supported for on-chip devices.
>>
> Is it a good idea to use the musb->set_clock function pointer for OMAP architure and
> call the runtime pm apis from this function defined in the usb platform driver(omap2430)

I guess that's Felipe's call, but I don't like that option.

I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
the runtime hooks be noops on the other platforms.

Kevin

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

 

>-----Original Message-----
>From: Kevin Hilman [mailto:khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org] 
>Sent: Thursday, September 23, 2010 11:04 PM
>To: Kalliguddi, Hema
>Cc: Balbi, Felipe; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>"Kalliguddi, Hema" <hemahk-l0cyMroinI0@public.gmane.org> writes:
>
>>  
>>
>>>-----Original Message-----
>>>From: Kevin Hilman [mailto:khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org] 
>>>Sent: Thursday, September 23, 2010 9:00 PM
>>>To: Balbi, Felipe
>>>Cc: Kalliguddi, Hema; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>>>linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Tony Lindgren; 
>>>Cousson, Benoit; Paul Walmsley
>>>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm 
>apis for musb.
>>>
>>>Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> writes:
>>>
>>>> Hi,
>>>>
>>>> On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>>>>Calling runtime pm APIs pm_runtime_put_sync() and 
>>>pm_runtime_get_sync()
>>>>>for enabling/disabling the clocks,sysconfig settings.
>>>>>
>>>>>Also need to put the USB in force standby and force idle 
>>>mode when usb not used
>>>>>and set it back to no idle and no stndby after wakeup.
>>>>>For OMAP3 auto idle bit has to be disabled because of the 
>>>errata.So using
>>>>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>>>
>>>[...]
>>>
>>>>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>>>>> 		 * they will even be wakeup-enabled.
>>>>> 		 */
>>>>> 	}
>>>>>+	pm_runtime_put_sync(dev);
>>>>>
>>>>>+#ifndef CONFIG_PM_RUNTIME
>>>>> 	musb_save_context(musb);
>>>>>
>>>>> 	if (musb->set_clock)
>>>>> 		musb->set_clock(musb->clock, 0);
>>>>> 	else
>>>>> 		clk_disable(musb->clock);
>>>>>+#endif
>>>>
>>>> I would rather remove these, adding ifdefs is bad :-( Unless 
>>>other archs
>>>> (blackfin, davinci) would have problems if we remove those.
>>>
>>>I didn't like these #ifdefs either, but davinci doesn't have 
>>>runtime PM,
>>>and I don't think blackfin does either.
>>>
>>>But, rather than the ifdef here, this could be done with different
>>>pointers in struct dev_pm_ops based on the arch.
>>>
>>>Also, this shouldn't be based on CONFIG_PM_RUNTIME, but rather on the
>>>arch.  We can still enable runtime PM on davinci for other subsystems
>>>(PCI, USB core, etc.) but not have it supported for on-chip devices.
>>>
>> Is it a good idea to use the musb->set_clock function 
>pointer for OMAP architure and
>> call the runtime pm apis from this function defined in the 
>usb platform driver(omap2430)
>
>I guess that's Felipe's call, but I don't like that option.
>
>I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>the runtime hooks be noops on the other platforms.
>
I think the set_clock function was used for setting the platform specific clocks
for different architecture. Anyway we need this hook or some other hook
once we start handling the optional clocks and internal phy clocks for musb.
Since this is already exists I thought of making use of it instand of adding another hook.

Felipe,
What do you say?

~Hema
>Kevin
>
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Fri, Sep 24, 2010 at 03:16:29AM -0500, Kalliguddi, Hema wrote:
>>I guess that's Felipe's call, but I don't like that option.
>>
>>I think it's cleaner to have the ->set_clock hook be a noop on OMAP
>>and the runtime hooks be noops on the other platforms.
>>
>I think the set_clock function was used for setting the platform
>specific clocks for different architecture. Anyway we need this hook or
>some other hook once we start handling the optional clocks and internal
>phy clocks for musb.  Since this is already exists I thought of making
>use of it instand of adding another hook.
>
>Felipe, What do you say?

Ideally we will get rid of those. It's only a matter of getting DaVinci
to fully support clkdev. We should not have wrappers on top of clk
framework neither pass down clock names as platform_data to driver,
that's insane.

-- 
balbi

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

Felipe,
 

>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Friday, September 24, 2010 2:14 PM
>To: Kalliguddi, Hema
>Cc: Kevin Hilman; Balbi, Felipe; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Hi,
>
>On Fri, Sep 24, 2010 at 03:16:29AM -0500, Kalliguddi, Hema wrote:
>>>I guess that's Felipe's call, but I don't like that option.
>>>
>>>I think it's cleaner to have the ->set_clock hook be a noop on OMAP
>>>and the runtime hooks be noops on the other platforms.
>>>
>>I think the set_clock function was used for setting the platform
>>specific clocks for different architecture. Anyway we need 
>this hook or
>>some other hook once we start handling the optional clocks 
>and internal
>>phy clocks for musb.  Since this is already exists I thought of making
>>use of it instand of adding another hook.
>>
>>Felipe, What do you say?
>
>Ideally we will get rid of those. It's only a matter of getting DaVinci
>to fully support clkdev. We should not have wrappers on top of clk
>framework neither pass down clock names as platform_data to driver,
>that's insane.
>
I think we will need to have some hooks for handling the clocks for
different platform as each platforms will have different clocks.
The way we are using set_clock is not the wrapper on top clock frameowrk,
it is a platform hook to enabling the clocks which still uses the clock framework.

~Hema

>-- 
>balbi
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

 

>-----Original Message-----
>From: linux-usb-owner@vger.kernel.org 
>[mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Kalliguddi, Hema
>Sent: Friday, September 24, 2010 2:23 PM
>To: Balbi, Felipe
>Cc: Kevin Hilman; linux-omap@vger.kernel.org; 
>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Felipe,
> 
>
>>-----Original Message-----
>>From: Balbi, Felipe 
>>Sent: Friday, September 24, 2010 2:14 PM
>>To: Kalliguddi, Hema
>>Cc: Kevin Hilman; Balbi, Felipe; linux-omap@vger.kernel.org; 
>>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; 
>>Cousson, Benoit; Paul Walmsley
>>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis 
>for musb.
>>
>>Hi,
>>
>>On Fri, Sep 24, 2010 at 03:16:29AM -0500, Kalliguddi, Hema wrote:
>>>>I guess that's Felipe's call, but I don't like that option.
>>>>
>>>>I think it's cleaner to have the ->set_clock hook be a noop on OMAP
>>>>and the runtime hooks be noops on the other platforms.
>>>>
>>>I think the set_clock function was used for setting the platform
>>>specific clocks for different architecture. Anyway we need 
>>this hook or
>>>some other hook once we start handling the optional clocks 
>>and internal
>>>phy clocks for musb.  Since this is already exists I thought 
>of making
>>>use of it instand of adding another hook.
>>>
>>>Felipe, What do you say?
>>
>>Ideally we will get rid of those. It's only a matter of 
>getting DaVinci
>>to fully support clkdev. We should not have wrappers on top of clk
>>framework neither pass down clock names as platform_data to driver,
>>that's insane.
>>
>I think we will need to have some hooks for handling the clocks for
>different platform as each platforms will have different clocks.
>The way we are using set_clock is not the wrapper on top clock 
>frameowrk,
>it is a platform hook to enabling the clocks which still uses 
>the clock framework.
>
>~Hema
>
I just noticed one more think in the code.
there are suspend/resume function pointers in platform_driver structure.
Why can't we use these for platform specific operations like enable/disable clocks, context save/restore?

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

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Fri, Sep 24, 2010 at 04:28:35AM -0500, Kalliguddi, Hema wrote:
>I just noticed one more think in the code.  there are suspend/resume
>function pointers in platform_driver structure.  Why can't we use these
>for platform specific operations like enable/disable clocks, context
>save/restore?

For doing that properly, we first need to split the HW glue layer to
another parent platform_device/driver. Otherwise, we will have all sorts
of crazy stuff to change the function pointer depending on the ARCH.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

Hi, 

>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Friday, September 24, 2010 4:27 PM
>To: Kalliguddi, Hema
>Cc: Balbi, Felipe; Kevin Hilman; linux-omap@vger.kernel.org; 
>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Hi,
>
>On Fri, Sep 24, 2010 at 04:28:35AM -0500, Kalliguddi, Hema wrote:
>>I just noticed one more think in the code.  there are suspend/resume
>>function pointers in platform_driver structure.  Why can't we 
>use these
>>for platform specific operations like enable/disable clocks, context
>>save/restore?
>
>For doing that properly, we first need to split the HW glue layer to
>another parent platform_device/driver. Otherwise, we will have 
>all sorts
>of crazy stuff to change the function pointer depending on the ARCH.
>
Agreed,

then for now I can think of using set_clok platform function to call
the runtime pm apis, Is there any alternative you are thinking of?
 
>-- 
>balbi
>

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Fri, Sep 24, 2010 at 06:00:26AM -0500, Kalliguddi, Hema wrote:
>then for now I can think of using set_clok platform function to call
>the runtime pm apis, Is there any alternative you are thinking of?

how about not pushing a patch which isn't the best way to solve the
problem in the first place ? .38 isn't that far away. If we can get all
those cleanups by that time, converting to pm_runtime will be a lot
simpler and local change.

I mean, if we keep as is for another major release cycle, it won't cause
problems to anyone, right ? So it's better to get things right from the
beginning and avoid lots of re-work later.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

Hi,
>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Friday, September 24, 2010 4:35 PM
>To: Kalliguddi, Hema
>Cc: Balbi, Felipe; Kevin Hilman; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Basak, Partha; Tony Lindgren; 
>Cousson, Benoit; Paul Walmsley
>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
>Hi,
>
>On Fri, Sep 24, 2010 at 06:00:26AM -0500, Kalliguddi, Hema wrote:
>>then for now I can think of using set_clok platform function to call
>>the runtime pm apis, Is there any alternative you are thinking of?
>
>how about not pushing a patch which isn't the best way to solve the
>problem in the first place ? .38 isn't that far away. If we can get all
>those cleanups by that time, converting to pm_runtime will be a lot
>simpler and local change.
>
>I mean, if we keep as is for another major release cycle, it 
>won't cause
>problems to anyone, right ? So it's better to get things right from the
>beginning and avoid lots of re-work later.
>
Agreed that it is better to get the things done in the best way from the beginning.
This may not be the best solution for converting usb to use runtime pm apis, but given the current mainline
code, without the cleanup, do not have other alternatives.
Do you think the other patches are in good shape to merge after fixing the comments
I got from Kevin and Benoit and you? 

Kevin,

Do you have any other comments on the hwmod patches other than runtime pm patch and offmode patch?
Do you accept the hwmod patches without the runtime pm conversion?

>-- 
>balbi
>--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Thu, Sep 23, 2010 at 12:33:58PM -0500, Kevin Hilman wrote:
>I guess that's Felipe's call, but I don't like that option.
>
>I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>the runtime hooks be noops on the other platforms.

Agreed. We should focus on removing ->set_clock for .38 actually. Is
DaVinci already using clkdev, Kevin ?

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Sergei Shtylyov]

Hello.

Felipe Balbi wrote:

>> I guess that's Felipe's call, but I don't like that option.

>> I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>> the runtime hooks be noops on the other platforms.

> Agreed. We should focus on removing ->set_clock for .38 actually. Is
> DaVinci already using clkdev, Kevin ?

    Sure. But DaVinci doesn't use ->set_clock().

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

On Fri, Sep 24, 2010 at 09:49:34AM -0500, Sergei Shtylyov wrote:
>Hello.
>
>Felipe Balbi wrote:
>
>>> I guess that's Felipe's call, but I don't like that option.
>
>>> I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>>> the runtime hooks be noops on the other platforms.
>
>> Agreed. We should focus on removing ->set_clock for .38 actually. Is
>> DaVinci already using clkdev, Kevin ?
>
>    Sure. But DaVinci doesn't use ->set_clock().

Ok, so seems like no-one is actually using that. We can already start
patching to remove that thing, later on, we need to remove the clock
name via platform_data.

-- 
balbi

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Sergei Shtylyov]

Hello.

On 27-09-2010 10:07, Felipe Balbi wrote:

>>>> I guess that's Felipe's call, but I don't like that option.

>>>> I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>>>> the runtime hooks be noops on the other platforms.

>>> Agreed. We should focus on removing ->set_clock for .38 actually. Is
>>> DaVinci already using clkdev, Kevin ?

>> Sure. But DaVinci doesn't use ->set_clock().

> Ok, so seems like no-one is actually using that.

    I thought OMAP does... but I'm seeing now that it doesn't.

> We can already start
> patching to remove that thing, later on, we need to remove the clock
> name via platform_data.

    As I've said already, there are cases where two clocks is needed by MUSB 
(like AM3517) or where USB 2.0 clock is also needed by the OHCI driver 
(DA8xx), hence the name seems needed still...

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

On Mon, Sep 27, 2010 at 04:48:04AM -0500, Sergei Shtylyov wrote:
>Hello.
>
>On 27-09-2010 10:07, Felipe Balbi wrote:
>
>>>>> I guess that's Felipe's call, but I don't like that option.
>
>>>>> I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>>>>> the runtime hooks be noops on the other platforms.
>
>>>> Agreed. We should focus on removing ->set_clock for .38 actually. Is
>>>> DaVinci already using clkdev, Kevin ?
>
>>> Sure. But DaVinci doesn't use ->set_clock().
>
>> Ok, so seems like no-one is actually using that.
>
>    I thought OMAP does... but I'm seeing now that it doesn't.
>
>> We can already start
>> patching to remove that thing, later on, we need to remove the clock
>> name via platform_data.
>
>    As I've said already, there are cases where two clocks is needed by MUSB
>(like AM3517) or where USB 2.0 clock is also needed by the OHCI driver
>(DA8xx), hence the name seems needed still...

we can all go through all that discussion with Russell again and will
come down to the same conclusion: clock names should never be needed.

-- 
balbi

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Sep 23, 2010 at 12:33:58PM -0500, Kevin Hilman wrote:
>>I guess that's Felipe's call, but I don't like that option.
>>
>>I think it's cleaner to have the ->set_clock hook be a noop on OMAP and
>>the runtime hooks be noops on the other platforms.
>
> Agreed. We should focus on removing ->set_clock for .38 actually. Is
> DaVinci already using clkdev, Kevin ?

Yes, it was one of the first to convert.

Kevin

RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Kalliguddi, Hema]

Hi,
 
>-----Original Message-----
>From: linux-usb-owner@vger.kernel.org 
>[mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Kalliguddi, Hema
>Sent: Thursday, September 23, 2010 9:10 PM
>To: Kevin Hilman; Balbi, Felipe
>Cc: linux-omap@vger.kernel.org; linux-usb@vger.kernel.org; 
>Basak, Partha; Tony Lindgren; Cousson, Benoit; Paul Walmsley
>Subject: RE: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.
>
> 
>
>>-----Original Message-----
>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>Sent: Thursday, September 23, 2010 9:00 PM
>>To: Balbi, Felipe
>>Cc: Kalliguddi, Hema; linux-omap@vger.kernel.org; 
>>linux-usb@vger.kernel.org; Basak, Partha; Tony Lindgren; 
>>Cousson, Benoit; Paul Walmsley
>>Subject: Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis 
>for musb.
>>
>>Felipe Balbi <balbi@ti.com> writes:
>>
>>> Hi,
>>>
>>> On Wed, Sep 22, 2010 at 07:30:30PM -0500, Kalliguddi, Hema wrote:
>>>>Calling runtime pm APIs pm_runtime_put_sync() and 
>>pm_runtime_get_sync()
>>>>for enabling/disabling the clocks,sysconfig settings.
>>>>
>>>>Also need to put the USB in force standby and force idle 
>>mode when usb not used
>>>>and set it back to no idle and no stndby after wakeup.
>>>>For OMAP3 auto idle bit has to be disabled because of the 
>>errata.So using
>>>>HWMOD_NO_OCP_AUTOIDLE flag for OMAP3430.
>>
>>[...]
>>
>>>>@@ -2424,13 +2425,16 @@ static int musb_suspend(struct device *d
>>>> 		 * they will even be wakeup-enabled.
>>>> 		 */
>>>> 	}
>>>>+	pm_runtime_put_sync(dev);
>>>>
>>>>+#ifndef CONFIG_PM_RUNTIME
>>>> 	musb_save_context(musb);
>>>>
>>>> 	if (musb->set_clock)
>>>> 		musb->set_clock(musb->clock, 0);
>>>> 	else
>>>> 		clk_disable(musb->clock);
>>>>+#endif
>>>
>>> I would rather remove these, adding ifdefs is bad :-( Unless 
>>other archs
>>> (blackfin, davinci) would have problems if we remove those.
>>
>>I didn't like these #ifdefs either, but davinci doesn't have 
>>runtime PM,
>>and I don't think blackfin does either.
>>
>>But, rather than the ifdef here, this could be done with different
>>pointers in struct dev_pm_ops based on the arch.
>>
>>Also, this shouldn't be based on CONFIG_PM_RUNTIME, but rather on the
>>arch.  We can still enable runtime PM on davinci for other subsystems
>>(PCI, USB core, etc.) but not have it supported for on-chip devices.
>>
>Is it a good idea to use the musb->set_clock function pointer 
>for OMAP architure and
>call the runtime pm apis from this function defined in the usb 
>platform driver(omap2430)
>
>>Kevin

Here is the patch which is making use of already existing platform set_clock functions pointer.
With this we don't need to use #ifdefs.
If it looks good I will post it again along with series.

 arch/arm/mach-omap2/usb-musb.c |   18 +++++++++++++++++
 drivers/usb/musb/musb_core.c   |   12 +++++++++++
 drivers/usb/musb/omap2430.c    |   43 ++++++++++++++---------------------------
 3 files changed, 45 insertions(+), 28 deletions(-)

Index: linux-omap-pm/arch/arm/mach-omap2/usb-musb.c
===================================================================
--- linux-omap-pm.orig/arch/arm/mach-omap2/usb-musb.c
+++ linux-omap-pm/arch/arm/mach-omap2/usb-musb.c
@@ -25,6 +25,7 @@
 #include <linux/io.h>
 
 #include <linux/usb/musb.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/sizes.h>
 
@@ -46,6 +47,7 @@ static struct platform_device dummy_pdev
 
 static void __iomem *otg_base;
 static struct clk *otg_clk;
+static struct omap_hwmod *oh_p;
 
 static void __init usb_musb_pm_init(void)
 {
@@ -129,6 +131,20 @@ static struct omap_device_pm_latency oma
 	},
 };
 
+void omap_set_clock(struct clk *clock , int is_on)
+{
+
+	struct omap_hwmod *oh = oh_p;
+	struct omap_device *od = oh->od;
+	struct platform_device *pdev = &od->pdev;
+	struct device *dev = &pdev->dev;
+
+	if(is_on)
+		pm_runtime_get_sync(dev);
+	else
+		pm_runtime_put_sync(dev);
+}
+
 void __init usb_musb_init(struct omap_musb_board_data *board_data)
 {
 	struct omap_hwmod *oh;
@@ -154,8 +170,10 @@ void __init usb_musb_init(struct omap_mu
 	musb_plat.power = board_data->power >> 1;
 	musb_plat.mode = board_data->mode;
 	musb_plat.extvbus = board_data->extvbus;
+	musb_plat.set_clock = omap_set_clock;
 
 	pdata = &musb_plat;
+	oh_p = oh;
 
 	od = omap_device_build(name, bus_id, oh, pdata,
 			       sizeof(*pdata), omap_musb_latency,
Index: linux-omap-pm/drivers/usb/musb/musb_core.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/musb_core.c
+++ linux-omap-pm/drivers/usb/musb/musb_core.c
@@ -98,6 +98,7 @@
 #include <linux/kobject.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
 
 #ifdef	CONFIG_ARM
 #include <mach/hardware.h>
@@ -2457,9 +2458,20 @@ static int musb_resume_noirq(struct devi
 	return 0;
 }
 
+static int musb_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int musb_runtime_resume(struct device *dev)
+{
+	return 0;
+}
 static const struct dev_pm_ops musb_dev_pm_ops = {
 	.suspend	= musb_suspend,
 	.resume_noirq	= musb_resume_noirq,
+	.runtime_suspend = musb_runtime_suspend,
+	.runtime_resume	 = musb_runtime_resume,
 };
 
 #define MUSB_DEV_PM_OPS (&musb_dev_pm_ops)
Index: linux-omap-pm/drivers/usb/musb/omap2430.c
===================================================================
--- linux-omap-pm.orig/drivers/usb/musb/omap2430.c
+++ linux-omap-pm/drivers/usb/musb/omap2430.c
@@ -31,6 +31,8 @@
 #include <linux/list.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
 
 #include "musb_core.h"
 #include "omap2430.h"
@@ -206,21 +208,6 @@ int __init musb_platform_init(struct mus
 
 	musb_platform_resume(musb);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	l &= ~NOSTDBY;		/* remove possible nostdby */
-	l |= SMARTSTDBY;	/* enable smart standby */
-	l &= ~AUTOIDLE;		/* disable auto idle */
-	l &= ~NOIDLE;		/* remove possible noidle */
-	l |= SMARTIDLE;		/* enable smart idle */
-	/*
-	 * MUSB AUTOIDLE don't work in 3430.
-	 * Workaround by Richard Woodruff/TI
-	 */
-	if (!cpu_is_omap3430())
-		l |= AUTOIDLE;		/* enable auto idle */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	l = musb_readl(musb->mregs, OTG_INTERFSEL);
 
 	if (data->interface_type == MUSB_INTERFACE_UTMI) {
@@ -253,15 +240,21 @@ int __init musb_platform_init(struct mus
 void musb_platform_save_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	musb_context->otg_sysconfig = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	musb_context->otg_forcestandby = musb_readl(musb->mregs, OTG_FORCESTDBY);
+	/*
+	 * As per the omap-usbotg specification, configure it to forced standby
+	 * and  force idle mode when no activity on usb.
+	 */
+	musb_writel(musb->mregs, OTG_FORCESTDBY, ENABLEFORCE);
 }
 
 void musb_platform_restore_context(struct musb *musb,
 		struct musb_context_registers *musb_context)
 {
-	musb_writel(musb->mregs, OTG_SYSCONFIG, musb_context->otg_sysconfig);
-	musb_writel(musb->mregs, OTG_FORCESTDBY, musb_context->otg_forcestandby);
+	/*
+	 * As per the omap-usbotg specification, configure it smart standby
+	 * and smart idle during operation.
+	 */
+	musb_writel(musb->mregs, OTG_FORCESTDBY, 0);
 }
 #endif
 
@@ -277,10 +270,6 @@ static int musb_platform_suspend(struct 
 	l |= ENABLEFORCE;	/* enable MSTANDBY */
 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l |= ENABLEWAKEUP;	/* enable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	otg_set_suspend(musb->xceiv, 1);
 
 	if (musb->set_clock)
@@ -294,23 +283,21 @@ static int musb_platform_suspend(struct 
 static int musb_platform_resume(struct musb *musb)
 {
 	u32 l;
+	struct device *dev = musb->controller;
 
 	if (!musb->clock)
 		return 0;
 
 	otg_set_suspend(musb->xceiv, 0);
 
+	pm_runtime_enable(dev);
 	if (musb->set_clock)
 		musb->set_clock(musb->clock, 1);
 	else
 		clk_enable(musb->clock);
 
-	l = musb_readl(musb->mregs, OTG_SYSCONFIG);
-	l &= ~ENABLEWAKEUP;	/* disable wakeup */
-	musb_writel(musb->mregs, OTG_SYSCONFIG, l);
-
 	l = musb_readl(musb->mregs, OTG_FORCESTDBY);
-	l &= ~ENABLEFORCE;	/* disable MSTANDBY */
+	l |= ENABLEFORCE;	/* enable MSTANDBY */
 	musb_writel(musb->mregs, OTG_FORCESTDBY, l);
 
 	return 0;




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

Re: [PATCH 8/9 v3] usb : musb: Using runtime pm apis for musb.

[Felipe Balbi]

Hi,

On Fri, Sep 24, 2010 at 12:34:09AM -0500, Kalliguddi, Hema wrote:
>Here is the patch which is making use of already existing platform
>set_clock functions pointer.  With this we don't need to use #ifdefs.
>If it looks good I will post it again along with series.

those weren't even used anymore on omap, it's kinda of a step backwards
to get back to using those pointers. I can already see Russell's angry
face when he sees this patch. We should not need such artifacts.

Granted, musb is extremely complicated to make any change because
it's used by several different platforms, so we need to somehow abstract
the platform-specific part and one way is to split the HW glue layer to
another platform_device/driver of its own.

I'm still thinking what's the best way to do that, but there's an RFC on
linux-usb regarding that. Help is welcome :-D

-- 
balbi