>-----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-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
