Hi,
>-----Original Message-----
>From: Kevin Hilman [mailto:[email protected]]
>Sent: Thursday, August 26, 2010 5:44 AM
>To: Kalliguddi, Hema
>Cc: [email protected]; [email protected];
>Basak, Partha; Felipe Balbi; Tony Lindgren; Cousson, Benoit;
>Paul Walmsley
>Subject: Re: [PATCH 8/8 v2] usb : musb: Using runtime pm apis for musb.
>
>Hema HK <[email protected]> writes:
>
>> Calling runtime pm APIs pm_runtime_put_sync() and
>pm_runtime_get_sync()
>> for enabling/disabling the clocks,sysconfig settings.
>>
>> used omap_hwmod_enable_wakeup & omap_hwmod_disable_wakeup
>apis to set/clear
>> the wakeup enable bit.
>> Also need to put the USB in force standby and force idle
>mode when usb not used
>> and set it back to smart idle and smart stndby after wakeup.
>> these cases are handled using the oh flags.
>> For omap3430 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]>
>> ---
>> arch/arm/mach-omap2/pm34xx.c | 8 ++-
>> arch/arm/mach-omap2/usb-musb.c | 86
>++++++++++++++++++++++++++++++--
>> arch/arm/plat-omap/include/plat/usb.h | 9 +++-
>> drivers/usb/musb/musb_core.c | 12 +++++
>> drivers/usb/musb/omap2430.c | 65
>+++++--------------------
>> include/linux/usb/musb.h | 8 +++
>> 6 files changed, 127 insertions(+), 61 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c
>b/arch/arm/mach-omap2/pm34xx.c
>> index 7b34201..0eb39b3 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -418,7 +418,9 @@ void omap_sram_idle(void)
>> omap3_core_save_context();
>> omap3_prcm_save_context();
>> /* Save MUSB context */
>> - musb_context_save_restore(1);
>> + musb_context_save_restore(save_context);
>> + } else {
>> + musb_context_save_restore(disable_clk);
>
>Presumably the 'disable_clk' is meant to mean "no need to save context,
>just disable clock", in which case the function name is not really
>accurate anymore.
>
>What is needed is just a general function that takes the next power
>state and let the function internals make the decision. The idle loop
>should not have IP-specific logic in it.
>
>I don't really want any IP specific logic in this part of the
>idle loop.
>What I really would like to see is all of this driver specific stuff
>moved out of the core idle path and done before interrupts are
>disabled.
>
>If we can do this before interrups are disabled (a bit earlier in the
>CPUidle path), then we can just use the normal runtime PM API and not
>have to handle the special cases of doing all this black magic inside
>the core idle path.
>
This can be done. I will have a generic usb idle and wakeup functions defined
and will be called with next/previous core state as parameter and call
Before/after the interrupts are disabled/enabled as you suggested, and handle
the required
cases in the musb module.
I will post the patch with changes after testing.
>> }
>> }
>>
>> @@ -462,7 +464,9 @@ void omap_sram_idle(void)
>> omap3_sram_restore_context();
>> omap2_sms_restore_context();
>> /* restore MUSB context */
>> - musb_context_save_restore(0);
>> + musb_context_save_restore(restore_context);
>> + } else {
>> + musb_context_save_restore(enable_clk);
>> }
>> omap_uart_resume_idle(0);
>> omap_uart_resume_idle(1);
>> diff --git a/arch/arm/mach-omap2/usb-musb.c
>b/arch/arm/mach-omap2/usb-musb.c
>> index 9d10440..b7736d2 100644
>> --- a/arch/arm/mach-omap2/usb-musb.c
>> +++ b/arch/arm/mach-omap2/usb-musb.c
>> @@ -23,6 +23,7 @@
>> #include <linux/clk.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/io.h>
>> +#include <linux/pm_runtime.h>
>>
>> #include <linux/usb/musb.h>
>>
>> @@ -64,13 +65,32 @@ static struct musb_hdrc_platform_data
>musb_plat = {
>> /* supports clock autogating */
>> .clk_autogated = 1,
>> };
>> +static int usb_idle_hwmod(struct omap_device *od)
>> +{
>> + struct omap_hwmod *oh = *od->hwmods;
>> + if (irqs_disabled())
>> + _omap_hwmod_idle(oh);
>> + else
>> + omap_device_idle_hwmods(od);
>> + return 0;
>> +}
>> +
>> +static int usb_enable_hwmod(struct omap_device *od)
>> +{
>> + struct omap_hwmod *oh = *od->hwmods;
>> + if (irqs_disabled())
>> + _omap_hwmod_enable(oh);
>> + else
>> + omap_device_enable_hwmods(od);
>> + return 0;
>> +}
>
>see above. Please move the musb pre-idle outside of the interrupts
>disabled part of the idle loop and you can get rid of this special
>handling.
Agreed.
>
>> static u64 musb_dmamask = DMA_BIT_MASK(32);
>>
>> static struct omap_device_pm_latency omap_musb_latency[] = {
>> {
>> - .deactivate_func = omap_device_idle_hwmods,
>> - .activate_func = omap_device_enable_hwmods,
>> + .deactivate_func = usb_idle_hwmod,
>> + .activate_func = usb_enable_hwmod,
>> .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> },
>> };
>> @@ -100,6 +120,8 @@ void __init usb_musb_init(struct
>omap_musb_board_data *board_data)
>> musb_plat.power = board_data->power >> 1;
>> musb_plat.mode = board_data->mode;
>> musb_plat.extvbus = board_data->extvbus;
>> + musb_plat.enable_wakeup = omap_device_enable_wakeup;
>> + musb_plat.disable_wakeup = omap_device_disable_wakeup;
>> pdata = &musb_plat;
>> oh_p = oh;
>> od = omap_device_build(name, bus_id, oh, pdata,
>> @@ -120,7 +142,7 @@ void __init usb_musb_init(struct
>omap_musb_board_data *board_data)
>>
>> }
>>
>> -void musb_context_save_restore(int save)
>> +void musb_context_save_restore(enum musb_state state)
>> {
>> struct omap_hwmod *oh = oh_p;
>> struct omap_device *od;
>> @@ -145,10 +167,62 @@ void musb_context_save_restore(int save)
>>
>> if (!pdata->is_usb_active(dev)) {
>>
>> - if (save)
>> + switch (state) {
>> + case save_context:
>> + /* As per the OMAP USBOTG
>specicifcation,
>> + * if USB device is not active
>and attempting
>> + * to core off then save the context,
>> + * set the sysconfig reg to
>force standby
>> + * force idle and disable the clock.
>> + */
>> + oh->flags |= HWMOD_SWSUP_SIDLE
>> + | HWMOD_SWSUP_MSTANDBY;
>> pm->suspend(dev);
>> - else
>> + pm_runtime_put_sync(dev);
>> +
>> + break;
>> +
>> + case disable_clk:
>> + /* If the USB device not active then
>> + * set the sysconfig setting
>> + * to force standby force idle and
>> + * disable the clock.
>> + */
>> + oh->flags |= HWMOD_SWSUP_SIDLE
>> + | HWMOD_SWSUP_MSTANDBY;
>> + pm_runtime_put_sync(dev);
>> +
>> + break;
>> +
>> + case restore_context:
>> + /* As per the OMAP USBOTG
>specicifcation,
>> + * configure the USB into smart
>idle and smart
>> + * standbyduring active. Enable the
>> + * clock, set the sysconfig
>back to smart idle
>> + * and smart stndby and restore
>the context
>> + * after wakeup from core-off.
>> + */
>> + oh->flags &= ~(HWMOD_SWSUP_SIDLE
>> + | HWMOD_SWSUP_MSTANDBY);
>> + pm_runtime_get_sync(dev);
>> pm->resume_noirq(dev);
>> +
>> + break;
>> +
>> + case enable_clk:
>> + /* Set the sysconfig back to smart
>> + * idle and smart stndby after
>wakeup and
>> + * enable the clock.
>> + */
>> + oh->flags &= ~(HWMOD_SWSUP_SIDLE
>> + | HWMOD_SWSUP_MSTANDBY);
>> + pm_runtime_get_sync(dev);
>> +
>> + break;
>> +
>> + default:
>> + break;
>> + }
>> }
>> }
>> }
>> @@ -157,7 +231,7 @@ void musb_context_save_restore(int save)
>> void __init usb_musb_init(struct omap_musb_board_data *board_data)
>> {
>> }
>> -void musb_context_save_restore(int save)
>> +void musb_context_save_restore(enum musb_state state)
>> {
>> }
>> #endif /* CONFIG_USB_MUSB_SOC */
>> diff --git a/arch/arm/plat-omap/include/plat/usb.h
>b/arch/arm/plat-omap/include/plat/usb.h
>> index ed2b41a..82b8618 100644
>> --- a/arch/arm/plat-omap/include/plat/usb.h
>> +++ b/arch/arm/plat-omap/include/plat/usb.h
>> @@ -71,6 +71,13 @@ struct omap_musb_board_data {
>> unsigned extvbus:1;
>> };
>>
>> +enum musb_state {
>> + save_context = 1,
>> + disable_clk,
>> + restore_context,
>> + enable_clk,
>> + };
>> +
>> enum musb_interface {MUSB_INTERFACE_ULPI, MUSB_INTERFACE_UTMI};
>>
>> extern void usb_musb_init(struct omap_musb_board_data *board_data);
>> @@ -80,7 +87,7 @@ extern void usb_ehci_init(const struct
>ehci_hcd_omap_platform_data *pdata);
>> extern void usb_ohci_init(const struct
>ohci_hcd_omap_platform_data *pdata);
>>
>> /* For saving and restoring the musb context during off/wakeup*/
>> -extern void musb_context_save_restore(int save);
>> +extern void musb_context_save_restore(enum musb_state state);
>> #endif
>>
>>
>> diff --git a/drivers/usb/musb/musb_core.c
>b/drivers/usb/musb/musb_core.c
>> index 8510e55..e875c83 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -2456,9 +2456,21 @@ static int musb_resume_noirq(struct
>device *dev)
>> 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)
>> diff --git a/drivers/usb/musb/omap2430.c
>b/drivers/usb/musb/omap2430.c
>> index dcba935..d83b644 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/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 <plat/mux.h>
>>
>> @@ -219,22 +221,6 @@ int __init musb_platform_init(struct musb *musb)
>> }
>>
>> 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) {
>> @@ -274,16 +260,6 @@ void musb_platform_save_context(struct
>musb *musb,
>> */
>> void __iomem *musb_base = musb->mregs;
>>
>> - musb_writel(musb_base, OTG_FORCESTDBY, 0);
>> -
>> - musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
>> - OTG_SYSCONFIG) & ~(NOSTDBY |
>SMARTSTDBY));
>> -
>> - musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
>> - OTG_SYSCONFIG) & ~AUTOIDLE);
>> -
>> - musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
>> - OTG_SYSCONFIG) & ~(NOIDLE | SMARTIDLE));
>>
>> musb_writel(musb_base, OTG_FORCESTDBY, 1);
>> }
>> @@ -298,18 +274,15 @@ void
>musb_platform_restore_context(struct musb *musb,
>> void __iomem *musb_base = musb->mregs;
>>
>> musb_writel(musb_base, OTG_FORCESTDBY, 0);
>> -
>> - musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
>> - OTG_SYSCONFIG) | SMARTSTDBY);
>> -
>> - musb_writel(musb_base, OTG_SYSCONFIG, musb_readl(musb_base,
>> - OTG_SYSCONFIG) | SMARTIDLE |
>ENABLEWAKEUP);
>> }
>> #endif
>>
>> static int musb_platform_suspend(struct musb *musb)
>> {
>> u32 l;
>> + struct device *dev = musb->controller;
>> + struct musb_hdrc_platform_data *pdata = dev->platform_data;
>> + struct platform_device *pdev = to_platform_device(dev);
>>
>> if (!musb->clock)
>> return 0;
>> @@ -318,17 +291,9 @@ static int musb_platform_suspend(struct
>musb *musb)
>> l = musb_readl(musb->mregs, OTG_FORCESTDBY);
>> 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);
>> -
>> + pdata->enable_wakeup(pdev);
>> otg_set_suspend(musb->xceiv, 1);
>> -
>> - if (musb->set_clock)
>> - musb->set_clock(musb->clock, 0);
>> - else
>> - clk_disable(musb->clock);
>> + pm_runtime_put_sync(dev);
>>
>> return 0;
>> }
>> @@ -336,21 +301,17 @@ static int
>musb_platform_suspend(struct musb *musb)
>> static int musb_platform_resume(struct musb *musb)
>> {
>> u32 l;
>> + struct device *dev = musb->controller;
>> + struct musb_hdrc_platform_data *pdata = dev->platform_data;
>> + struct platform_device *pdev = to_platform_device(dev);
>>
>> 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);
>> + pdata->enable_wakeup(pdev);
>
>I think you mean ->disable_wakeup() here, right?
>
No I meant enable_wakeup only here. When smart idle/standby is enabled,
wakeup bit has to be set to generate the s-wakeup when the devie is in idle
and system is in ret.
>Also, remember that usage of the runtime PM API is usecount based, and
>the low level functions are only called when the usecount transitions
>to/from zero.
>
>Therefore, anything you want to happen when the actual HW transition
>happens should be done in the drivers .runtime_suspend &
>.runtime_resume
>callbacks instead of done when you call the runtime PM API.
>
It is a good idea to call the enable_wakeup in runtime_suspend and
runtime_resume APIs.
>Alternatively, don't do this from the driver at all. Your device layer
>already has customized hooks for the idle transitions. Just do it
>there, and avoid the need to add another pdata hook.
If this is called in the driver's runtime_suspend resume then it will be taken
care.
>
>> l = musb_readl(musb->mregs, OTG_FORCESTDBY);
>> l &= ~ENABLEFORCE; /* disable MSTANDBY */
>> musb_writel(musb->mregs, OTG_FORCESTDBY, l);
>
>Shouldn't this part be removed too?
>
It does not make sense when smart ilde and smart standby is enabled. I can
remove this.
>> diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
>> index da134ab..20b2cc8 100644
>> --- a/include/linux/usb/musb.h
>> +++ b/include/linux/usb/musb.h
>> @@ -93,6 +93,8 @@ struct musb_hdrc_config {
>>
>> };
>>
>> +struct platform_device;
>> +
>> struct musb_hdrc_platform_data {
>> /* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */
>> u8 mode;
>> @@ -132,6 +134,12 @@ struct musb_hdrc_platform_data {
>>
>> /* indiates whether clock is autogated */
>> int clk_autogated;
>> +
>> + /* set the enable wakeup bit of sysconfig register */
>> + int (*enable_wakeup)(struct platform_device *pdev);
>> +
>> + /* Clear the enable wakeup bit of sysconfig register */
>> + int (*disable_wakeup)(struct platform_device *pdev);
>> };
>
--
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
