Hi, >-----Original Message----- >From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >Sent: Tuesday, August 31, 2010 8:49 PM >To: Kalliguddi, Hema >Cc: linux-omap@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; >Mankad, Maulik Ojas; Felipe Balbi; Tony Lindgren; Cousson, >Benoit; Paul Walmsley >Subject: Re: [PATCH 6/8 v2] usb: musb: Offmode fix for idle path > >"Kalliguddi, Hema" <hemahk@xxxxxx> writes: > >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Thursday, August 26, 2010 5:40 AM >>>To: Kalliguddi, Hema >>>Cc: linux-omap@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; >>>Mankad, Maulik Ojas; Felipe Balbi; Tony Lindgren; Cousson, >>>Benoit; Paul Walmsley >>>Subject: Re: [PATCH 6/8 v2] usb: musb: Offmode fix for idle path >>> >>>Hema HK <hemahk@xxxxxx> writes: >>> >>>> With OMAP core-off support musb was not functional as >>>context was getting >>>> lost after wakeup from core-off. And also musb was blocking >>>the core-off >>>> after loading the gadget driver even with no cable connected >>>sometimes. >>>> >>>> Added the conext save/restore api in the platform layer which will >>>> be called in the idle and wakeup path. >>>> >>>> Changed the usb sysconfig settings as per the usbotg >functional spec. >>>> When the device is not used, configure to force idle and >>>force standby mode. >>>> When it is being used, configure in smart standby and smart >>>idle mode. >>>> So while attempting to coreoff the usb is configured to >>>force standby and >>>> force idle mode, after wakeup configured in smart idle and >>>smart standby. >>> >>>You don't describe the new .clk_autogated field added here. >That part >>>should be a separate patch as it's not directly related to >>>$SUBJECT patch. >>> >> >> In OMAP for USB there is no need to disable the interface >clock. But for the other >> platforms like davinci which uses mentor USB IP clock is not >auto gated so >> There is a need to disbale the clock when .suspend API >defined in the driver is called. >> So introduced a flag in the platform to enable/disable the clock >> In .supend and .resume APIs appropriately in the driver code. > >Yes, I understand why it's there, and just suggested that it should be >documented and done as a separate patch. > >> Yes. It may not be directly related to $SUBJECT patch, but I >am reusing >> .suspend and .resume APIs for context save and restore >during idle path. >> Because of that this fix as part of the patch. >> >>>> Signed-off-by: Hema HK <hemahk@xxxxxx> >>>> Signed-off-by: Maulik Mankad <x0082077@xxxxxx> >>>> >>>> Cc: Felipe Balbi <felipe.balbi@xxxxxxxxx> >>>> Cc: Tony Lindgren <tony@xxxxxxxxxxx> >>>> Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> >>>> Cc: Cousson, Benoit <b-cousson@xxxxxx> >>>> Cc: Paul Walmsley <paul@xxxxxxxxx> >>>> --- >>>> arch/arm/mach-omap2/pm34xx.c | 5 +++ >>>> arch/arm/mach-omap2/usb-musb.c | 42 >>>++++++++++++++++++++++++++++- >>>> arch/arm/plat-omap/include/plat/usb.h | 2 + >>>> drivers/usb/musb/musb_core.c | 10 +++---- >>>> drivers/usb/musb/musb_core.h | 1 - >>>> drivers/usb/musb/omap2430.c | 48 >>>++++++++++++++++++++++++++++++--- >>>> include/linux/usb/musb.h | 6 ++++ >>>> 7 files changed, 102 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/pm34xx.c >>>b/arch/arm/mach-omap2/pm34xx.c >>>> index fb4994a..7b34201 100644 >>>> --- a/arch/arm/mach-omap2/pm34xx.c >>>> +++ b/arch/arm/mach-omap2/pm34xx.c >>>> @@ -39,6 +39,7 @@ >>>> #include <plat/gpmc.h> >>>> #include <plat/dma.h> >>>> #include <plat/dmtimer.h> >>>> +#include <plat/usb.h> >>>> >>>> #include <asm/tlbflush.h> >>>> >>>> @@ -416,6 +417,8 @@ void omap_sram_idle(void) >>>> if (core_next_state == PWRDM_POWER_OFF) { >>>> omap3_core_save_context(); >>>> omap3_prcm_save_context(); >>>> + /* Save MUSB context */ >>>> + musb_context_save_restore(1); >>>> } >>>> } >>>> >>>> @@ -458,6 +461,8 @@ void omap_sram_idle(void) >>>> omap3_prcm_restore_context(); >>>> omap3_sram_restore_context(); >>>> omap2_sms_restore_context(); >>>> + /* restore MUSB context */ >>>> + musb_context_save_restore(0); >>>> } >>>> 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 c228cc0..9d10440 100644 >>>> --- a/arch/arm/mach-omap2/usb-musb.c >>>> +++ b/arch/arm/mach-omap2/usb-musb.c >>>> @@ -35,6 +35,7 @@ >>>> >>>> static const char name[] = "musb_hdrc"; >>>> #define MAX_OMAP_MUSB_HWMOD_NAME_LEN 16 >>>> +struct omap_hwmod *oh_p =NULL; >>>> >>>> static struct musb_hdrc_config musb_config = { >>>> .multipoint = 1, >>>> @@ -59,6 +60,9 @@ static struct musb_hdrc_platform_data >musb_plat = { >>>> * "mode", and should be passed to usb_musb_init(). >>>> */ >>>> .power = 50, /* up to 100 mA */ >>>> + >>>> + /* supports clock autogating */ >>>> + .clk_autogated = 1, >>> >>>This appears unrelated, and should be a separate patch. >>> >>>> }; >>>> >>>> static u64 musb_dmamask = DMA_BIT_MASK(32); >>>> @@ -97,7 +101,7 @@ void __init usb_musb_init(struct >>>omap_musb_board_data *board_data) >>>> musb_plat.mode = board_data->mode; >>>> musb_plat.extvbus = board_data->extvbus; >>>> pdata = &musb_plat; >>>> - >>>> + oh_p = oh; >>>> od = omap_device_build(name, bus_id, oh, pdata, >>>> sizeof(struct musb_hdrc_platform_data), >>>> omap_musb_latency, >>>> @@ -116,8 +120,44 @@ void __init usb_musb_init(struct >>>omap_musb_board_data *board_data) >>>> >>>> } >>>> >>>> +void musb_context_save_restore(int save) >>>> +{ >>>> + struct omap_hwmod *oh = oh_p; >>>> + struct omap_device *od; >>>> + struct platform_device *pdev; >>>> + struct device *dev; >>>> + struct device_driver *drv; >>>> + struct musb_hdrc_platform_data *pdata; >>>> + const struct dev_pm_ops *pm; >>>> + >>>> + if (!oh) >>>> + return; >>>> + od = oh->od; >>>> + pdev = &od->pdev; >>>> + if (!pdev) >>>> + return; >>>> + dev = &pdev->dev; >>>> + drv = dev->driver; >>>> + >>>> + if (drv) { >>>> + pdata = dev->platform_data; >>>> + pm = drv->pm; >>>> + >>>> + if (!pdata->is_usb_active(dev)) { >>>> + >>>> + if (save) >>>> + pm->suspend(dev); >>>> + else >>>> + pm->resume_noirq(dev); >>> >>>-ECONFUSED >>> >>>First, this 'save_restore' function neither saves or >restores anything. >>> >> >> There are musb_save/restore APIs defined in the driver code >and called in a system suspend/resume APIs. >> I am just making use of these apis in the idle path aswell. > >Please don't. This is not and intended use of the driver model hooks. > I am using this hooks because the save/restore API is not a exported API And it needs a musb structure parameter which will not be available in the platform layer. Once I move o runtime PM api usage and use the runtime suspend/resume APIs need not use these hooks. But for this patch we will have live with these hooks. >Instead, we need to move the musb idle management out of the >interrupts-disabled section of the idle loop and use the >runtime PM APIs >as intended. As part of the runtime PM patch I will take care of this. > >Kevin > >>>Second, I'm thoroughly confused about why you are simulating a system >>>suspend/resume here? >> >> As said, instead of exporting the internal save/restore >APIs, I am making use of the pm_ops function pointers >> To do the same job. >> >>> >>>Kevin >>> >>>> + } >>>> + } >>>> +} >>>> + >>>> #else >>>> void __init usb_musb_init(struct omap_musb_board_data *board_data) >>>> { >>>> } >>>> +void musb_context_save_restore(int save) >>>> +{ >>>> +} >>>> #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 2a9427c..ed2b41a 100644 >>>> --- a/arch/arm/plat-omap/include/plat/usb.h >>>> +++ b/arch/arm/plat-omap/include/plat/usb.h >>>> @@ -79,6 +79,8 @@ 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); >>>> #endif >>>> >>>> >>>> diff --git a/drivers/usb/musb/musb_core.c >>>b/drivers/usb/musb/musb_core.c >>>> index 2a50d12..8510e55 100644 >>>> --- a/drivers/usb/musb/musb_core.c >>>> +++ b/drivers/usb/musb/musb_core.c >>>> @@ -2410,6 +2410,7 @@ static int musb_suspend(struct device *dev) >>>> struct platform_device *pdev = to_platform_device(dev); >>>> unsigned long flags; >>>> struct musb *musb = dev_to_musb(&pdev->dev); >>>> + struct musb_hdrc_platform_data *plat = dev->platform_data; >>>> >>>> if (!musb->clock) >>>> return 0; >>>> @@ -2428,9 +2429,7 @@ static int musb_suspend(struct device *dev) >>>> >>>> musb_save_context(musb); >>>> >>>> - if (musb->set_clock) >>>> - musb->set_clock(musb->clock, 0); >>>> - else >>>> + if (!plat->clk_autogated) >>>> clk_disable(musb->clock); >>>> spin_unlock_irqrestore(&musb->lock, flags); >>>> return 0; >>>> @@ -2440,13 +2439,12 @@ static int musb_resume_noirq(struct >>>device *dev) >>>> { >>>> struct platform_device *pdev = to_platform_device(dev); >>>> struct musb *musb = dev_to_musb(&pdev->dev); >>>> + struct musb_hdrc_platform_data *plat = dev->platform_data; >>>> >>>> if (!musb->clock) >>>> return 0; >>>> >>>> - if (musb->set_clock) >>>> - musb->set_clock(musb->clock, 1); >>>> - else >>>> + if (!plat->clk_autogated) >>>> clk_enable(musb->clock); >>>> >>>> musb_restore_context(musb); >>>> diff --git a/drivers/usb/musb/musb_core.h >>>b/drivers/usb/musb/musb_core.h >>>> index 85a92fd..bfdf54d 100644 >>>> --- a/drivers/usb/musb/musb_core.h >>>> +++ b/drivers/usb/musb/musb_core.h >>>> @@ -472,7 +472,6 @@ struct musb_context_registers { >>>> >>>> #if defined(CONFIG_ARCH_OMAP2430) || >defined(CONFIG_ARCH_OMAP3) || \ >>>> defined(CONFIG_ARCH_OMAP4) >>>> - u32 otg_sysconfig, otg_forcestandby; >>>> #endif >>>> u8 power; >>>> u16 intrtxe, intrrxe; >>>> diff --git a/drivers/usb/musb/omap2430.c >>>b/drivers/usb/musb/omap2430.c >>>> index 3bae428..dcba935 100644 >>>> --- a/drivers/usb/musb/omap2430.c >>>> +++ b/drivers/usb/musb/omap2430.c >>>> @@ -188,6 +188,18 @@ int musb_platform_set_mode(struct musb >>>*musb, u8 musb_mode) >>>> >>>> return 0; >>>> } >>>> +int is_musb_active(struct device *dev) >>>> +{ >>>> + struct musb *musb; >>>> + >>>> +#ifdef CONFIG_USB_MUSB_HDRC_HCD >>>> + /* usbcore insists dev->driver_data is a "struct hcd *" */ >>>> + musb = hcd_to_musb(dev_get_drvdata(dev)); >>>> +#else >>>> + musb = dev_get_drvdata(dev); >>>> +#endif >>>> + return musb->is_active; >>>> +} >>>> >>>> int __init musb_platform_init(struct musb *musb) >>>> { >>>> @@ -246,6 +258,7 @@ int __init musb_platform_init(struct >musb *musb) >>>> if (is_host_enabled(musb)) >>>> musb->board_set_vbus = omap_set_vbus; >>>> >>>> + plat->is_usb_active = is_musb_active; >>>> setup_timer(&musb_idle_timer, musb_do_idle, (unsigned >>>long) musb); >>>> >>>> return 0; >>>> @@ -255,15 +268,42 @@ int __init musb_platform_init(struct >>>musb *musb) >>>> 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. >>>> + */ >>>> + 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); >>>> } >>>> >>>> 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. >>>> + */ >>>> + 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 >>>> >>>> diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h >>>> index ee2dd1d..da134ab 100644 >>>> --- a/include/linux/usb/musb.h >>>> +++ b/include/linux/usb/musb.h >>>> @@ -126,6 +126,12 @@ struct musb_hdrc_platform_data { >>>> >>>> /* Architecture specific board data */ >>>> void *board_data; >>>> + >>>> + /* check usb device active state*/ >>>> + int (*is_usb_active)(struct device *dev); >>>> + >>>> + /* indiates whether clock is autogated */ >>>> + int clk_autogated; >>>> }; >>> >-- 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
