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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 

>-----Original Message-----
>From: linux-usb-owner@xxxxxxxxxxxxxxx 
>[mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Kalliguddi, Hema
>Sent: Thursday, September 23, 2010 1:21 PM
>To: Balbi, Felipe
>Cc: linux-omap@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; 
>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@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; 
>>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@xxxxxx>
>>>Signed-off-by: Basak, Partha <p-basak2@xxxxxx>
>>>Cc: Felipe Balbi <balbi@xxxxxx>
>>>Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>>>Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx>
>>>Cc: Cousson, Benoit <b-cousson@xxxxxx>
>>>Cc: Paul Walmsley <paul@xxxxxxxxx>
>>
>>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@xxxxxxxxxxxxxxx
>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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux