>-----Original Message-----
>From: [email protected]
>[mailto:[email protected]] On Behalf Of Kalliguddi, Hema
>Sent: Thursday, September 23, 2010 1:21 PM
>To: Balbi, Felipe
>Cc: [email protected]; [email protected];
>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: [email protected]; [email protected];
>>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 <[email protected]>
>>>Signed-off-by: Basak, Partha <[email protected]>
>>>Cc: Felipe Balbi <[email protected]>
>>>Cc: Tony Lindgren <[email protected]>
>>>Cc: Kevin Hilman <[email protected]>
>>>Cc: Cousson, Benoit <[email protected]>
>>>Cc: Paul Walmsley <[email protected]>
>>
>>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 [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
