Kevin, >-----Original Message----- >From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >Sent: Tuesday, September 28, 2010 12:27 AM >To: Kalliguddi, Hema >Cc: linux-omap@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; >Mankad, Maulik Ojas; Balbi, Felipe; Tony Lindgren; Cousson, >Benoit; Paul Walmsley >Subject: Re: [PATCH 9/9 v3] usb : musb: Offmode fix for idle path > >"Kalliguddi, Hema" <hemahk@xxxxxx> writes: > >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Saturday, September 25, 2010 1:12 AM >>>To: Kalliguddi, Hema >>>Cc: linux-omap@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; >>>Mankad, Maulik Ojas; Balbi, Felipe; Tony Lindgren; Cousson, >>>Benoit; Paul Walmsley >>>Subject: Re: [PATCH 9/9 v3] 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. >>> >>>This should be a separate patch. >>> >> >> Let me give the description of the musb offmode support in >the idle path. > >The detail you provided below is very good, and this is the level of >detail that needs to go into the changelogs. > >> With the current mainline code, offmode in the idle path is >not supported with usb. >> When the core hits off and wakes up the musb will not be functional. >> This patch is to support the musb functionality with offmode >enabled in the idle path. > >OK, what about the PM branch. I was under the impression that offmode >was working fine in the PM branch. > In current PM branch, the core hits retention and off mode without usb drive loaded. Once after loading the musb driver it may not retention/off also as the driver is configuring musb in smart idle/standby. Even if it hits retention/off as it might work sometimes,MUSB is not functional.because there is no context save/restore done in the driver. >And, looking at the PM branch, the only thing done in the idle path is >the disable of autoidle upon wakeup. Everything else (context >save/restore etc.) is done in the driver. > No.There is no context save/restore done in the driver today. >> Below are the requirements to support retention and offmode >of OMAP in idle path with usb enabled >> During idle and when there is no activity on the bus: >> >> 1.Since there is no hardware context save/restore supported >in OMAP for musb, software >> has to save the context. >> 2.Configure the musb in force idle/standby mode during idle mode > >This needs more detailed description (TRM reference, etc.) > I have provided the link to the public TRM and I had refered to the exact section of the TRM sometime in the older version the patches, when Benoit and you had asked for the description. >> 3.May or may not disable the interface clock(as interface >clock is autogated) >> and the functional clock is from ULPI phy on triton chip. >> >> When OMAP awakes: >> >> 1.enable the clock if it is disabled.and >> 2.Configure it back to no idle/standby or smart idle/standby >after wakeup. > >In the PM branch, this part is done in idle. > I only see the disable autoidle bit function the idle path in pm branch. There is no code for setting the no idle/standby or smart idle/standby bits in the idle path. >> 3.Restore the context back. > >But this is done by the driver. > No. current driver is not doing any context save/restore. >> Idling of device can be done when there is no activity on >the bus by using the pm_runtime_put_sync >n> apis in when device disconnects or suspends, but resuming >has to done as soon as the omap is wokenup from >> retention or core off, as we have to put back the musb in >known state ie restore the conext atleast >> enabling D+/D- lines,enabling interrupts and configuring the >no idle/standby or smart idle/standby >> to even capture the irqs. Otherwise we will not be able to >capture the musb connect/reset or resume/remote >> wakeup events as D+/D- lines will disabled when the context >is lost duribg offmode. >> >> If I use the pm_runtime_put_sync in disconnect/suspend >handler when device suspends/disconnects >> and use pm_runtime_get_sync when OMAP wakesup, then there >will be mismatch in the usecount. >> >> We could have achieved the same by using the triton >connect/disconnect events to idle/resume musb, >> but some of the phys do not support the connect/disconnect events. >> >> So cameup with the design of idling musb device in idle loop >and resume once the OMAP wakes up. >> All this done onl when the musb is not active. > >Rather than seeing more work done in the idle path, I would rather be >moving code out of the idle path. > >Did you consider using a (deferrable) timer during no-activity times >which periodically checks to be sure the IP is in force idle/standby? > >> Since the IDLE REQ/ACK protocol is broken, the >recommendation from ip team is to >> configure the musb in force idle/standby during omap >retention and offmode. > >Yes, this is in the PM branch and there is an errata number for it >there. Please reference that errata when implementing this feature >(both in the changelog and in the code.) > >Kevin > >> Since we have to touch the sysconfig registers and context >save/restore everytime, >> I am using the runtime pm apis. >> >>>> And also musb was blocking the core-off after loading the gadget >>>> driver even with no cable connected sometimes. >>> >>>this too >>> >>>> Added the idle and wakeup APIs in the platform layer which will >>>> be called in the idle and wakeup path. >>> >>>And this errata fix should be a separate patch, with reference to the >>>errata etc. >>> >> This is not an errata, this is requirement from the ip. it >is mentioned in >> the functional spec that when device is not use put it in >force idle/standby mode. >> >> >>>> Used the pm_runtime_put_sysc API to configure the >>>> musb to force idle/standby modes, saving the context and >>>disable the clk in >>>> while idling if there is no activity on the usb bus. >>> >>>Why? This should not be part of the idle path. >>> >> >> >>>If there is no activity on the bus, why hasn't the musb >driver already >>>runtime suspended itself? >>> >>>If the driver want's to runtime_suspend itself based on >inactivity, it >>>should use an inactivity timer, not hook into the idle loop. The >>>runtime PM API has a function for a deferred suspend >>> >>> int pm_schedule_suspend(struct device *dev, unsigned int delay) >>> >>>The only thing in the idle loop in the current code is the >errata fix, >>>and even that I'm not sure I'm ready to merge. >>> >>>I'd really like to see even the errata fix out of the idle >>>path. I wonder if usb-musb.c could create a periodic, >>>deferrable timer >>>to fire that ensure that autoidle is disabled. >>> >>>> Used the pm_runtime_get_sync API to configure the musb to >>>> no idle/standby modes, enable the clock and restore the context >>>> after wakeup when there is no activity on the usb bus. >>> >>>Why would you wakeup MUSB in the idle path if there is no activity on >>>the bus? >>> >> This is because MUSB has to be in known good state to >capture any musb interrupts. >> >>>I don't understand the motiviation or these last two >changes, and they >>>are a departure from the behavior of the previous code. >>> >> Can you please let mw know which 2 changes you reering to? >> You mean assing function to check whether the device is active >> and adding 2 flags for conext save/restore? >> >>>At a minimum, they should be separated into a separate patch, and >>>thoroughly described, including an answer to "why?" >>> >>>Kevin >>> >>>> Signed-off-by: Hema HK <hemahk@xxxxxx> >>>> Signed-off-by: Maulik Mankad <x0082077@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> >>>> >>>> --- >>>> arch/arm/mach-omap2/cpuidle34xx.c | 1 >>>> arch/arm/mach-omap2/pm34xx.c | 3 >>>> arch/arm/mach-omap2/usb-musb.c | 107 >>>++++++++++++++++++++++++++++++++++ >>>> arch/arm/plat-omap/include/plat/usb.h | 2 >>>> drivers/usb/musb/musb_core.c | 15 ++++ >>>> drivers/usb/musb/omap2430.c | 14 ++++ >>>> include/linux/usb/musb.h | 9 ++ >>>> 7 files changed, 149 insertions(+), 2 deletions(-) >>>> >>>> Index: linux-omap-pm/arch/arm/mach-omap2/cpuidle34xx.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/arch/arm/mach-omap2/cpuidle34xx.c >>>> +++ linux-omap-pm/arch/arm/mach-omap2/cpuidle34xx.c >>>> @@ -31,6 +31,7 @@ >>>> #include <plat/clockdomain.h> >>>> #include <plat/control.h> >>>> #include <plat/serial.h> >>>> +#include <plat/usb.h> >>>> >>>> #include "pm.h" >>>> >>>> Index: linux-omap-pm/arch/arm/mach-omap2/pm34xx.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/arch/arm/mach-omap2/pm34xx.c >>>> +++ linux-omap-pm/arch/arm/mach-omap2/pm34xx.c >>>> @@ -38,6 +38,7 @@ >>>> #include <plat/prcm.h> >>>> #include <plat/gpmc.h> >>>> #include <plat/dma.h> >>>> +#include <plat/usb.h> >>>> >>>> #include <asm/tlbflush.h> >>>> >>>> @@ -324,11 +325,13 @@ static void restore_table_entry(void) >>>> void omap3_device_idle(void) >>>> { >>>> omap2_gpio_prepare_for_idle(); >>>> + musb_prepare_for_idle(); >>>> } >>>> >>>> void omap3_device_resume(void) >>>> { >>>> omap2_gpio_resume_after_idle(); >>>> + musb_wakeup_from_idle(); >>>> } >>>> >>>> void omap_sram_idle(void) >>>> Index: linux-omap-pm/arch/arm/mach-omap2/usb-musb.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/arch/arm/mach-omap2/usb-musb.c >>>> +++ linux-omap-pm/arch/arm/mach-omap2/usb-musb.c >>>> @@ -25,16 +25,21 @@ >>>> #include <linux/io.h> >>>> >>>> #include <linux/usb/musb.h> >>>> +#include <linux/pm_runtime.h> >>>> >>>> #include <mach/hardware.h> >>>> #include <mach/irqs.h> >>>> #include <plat/usb.h> >>>> #include <plat/omap_device.h> >>>> +#include <plat/powerdomain.h> >>>> >>>> #ifdef CONFIG_USB_MUSB_SOC >>>> static const char name[] = "musb_hdrc"; >>>> #define MAX_OMAP_MUSB_HWMOD_NAME_LEN 16 >>>> >>>> +struct omap_hwmod *oh_p; >>>> +static struct powerdomain *core_pwrdm; >>>> + >>>> static struct musb_hdrc_config musb_config = { >>>> .multipoint = 1, >>>> .dyn_fifo = 1, >>>> @@ -58,6 +63,10 @@ static struct musb_hdrc_platform_data mu >>>> * "mode", and should be passed to usb_musb_init(). >>>> */ >>>> .power = 50, /* up to 100 mA */ >>>> + >>>> + /* OMAP supports offmode */ >>>> + .save_context = 1, >>>> + .restore_context = 1, >>>> }; >>>> >>>> static u64 musb_dmamask = DMA_BIT_MASK(32); >>>> @@ -80,6 +89,7 @@ void __init usb_musb_init(struct omap_mu >>>> const char *oh_name = "usb_otg_hs"; >>>> struct musb_hdrc_platform_data *pdata; >>>> >>>> + core_pwrdm = pwrdm_lookup("per_pwrdm"); >>>> oh = omap_hwmod_lookup(oh_name); >>>> >>>> if (!oh) { >>>> @@ -97,6 +107,7 @@ void __init usb_musb_init(struct omap_mu >>>> 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), >>>> @@ -115,8 +126,101 @@ void __init usb_musb_init(struct omap_mu >>>> put_device(dev); >>>> } >>>> >>>> +void musb_prepare_for_idle() >>>> +{ >>>> + int core_next_state; >>>> + struct omap_hwmod *oh = oh_p; >>>> + struct omap_device *od; >>>> + struct platform_device *pdev; >>>> + struct musb_hdrc_platform_data *pdata; >>>> + struct device *dev; >>>> + >>>> + if (!core_pwrdm) >>>> + return; >>>> + >>>> + core_next_state = pwrdm_read_next_pwrst(core_pwrdm); >>>> + if (core_next_state >= PWRDM_POWER_INACTIVE) >>>> + return; >>>> + if (!oh) >>>> + return; >>>> + >>>> + od = oh->od; >>>> + pdev = &od->pdev; >>>> + >>>> + if (!pdev) >>>> + return; >>>> + dev = &pdev->dev; >>>> + >>>> + if (dev->driver) { >>>> + pdata = dev->platform_data; >>>> + >>>> + if (pdata->is_usb_active) >>>> + if (!pdata->is_usb_active(dev)) { >>>> + if (core_next_state == PWRDM_POWER_OFF) { >>>> + pdata->save_context = 1; >>>> + pm_runtime_put_sync(dev); >>>> + } else if (core_next_state == >>>PWRDM_POWER_RET) { >>>> + pdata->save_context = 0; >>>> + pm_runtime_put_sync(dev); >>>> + } >>>> + } >>>> + } >>>> +} >>>> + >>>> +void musb_wakeup_from_idle() >>>> +{ >>>> + int core_next_state; >>>> + int core_prev_state; >>>> + struct omap_hwmod *oh = oh_p; >>>> + struct omap_device *od; >>>> + struct platform_device *pdev; >>>> + struct device *dev; >>>> + struct musb_hdrc_platform_data *pdata; >>>> + >>>> + if (!core_pwrdm) >>>> + return; >>>> + >>>> + core_next_state = pwrdm_read_next_pwrst(core_pwrdm); >>>> + >>>> + if (core_next_state >= PWRDM_POWER_INACTIVE) >>>> + return; >>>> + core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm); >>>> + >>>> + if (!oh) >>>> + return; >>>> + od = oh->od; >>>> + pdev = &od->pdev; >>>> + >>>> + if (!pdev) >>>> + return; >>>> + >>>> + dev = &pdev->dev; >>>> + >>>> + if (dev->driver) { >>>> + pdata = dev->platform_data; >>>> + >>>> + if (pdata->is_usb_active) >>>> + if (!pdata->is_usb_active(dev)) { >>>> + if (core_prev_state == >>>PWRDM_POWER_OFF) { >>>> + pdata->restore_context = 1; >>>> + pm_runtime_get_sync(dev); >>>> + } else { >>>> + pdata->restore_context = 0; >>>> + pm_runtime_get_sync(dev); >>>> + } >>>> + } >>>> + } >>>> +} >>>> #else >>>> void __init usb_musb_init(struct omap_musb_board_data *board_data) >>>> { >>>> } >>>> + >>>> +void musb_prepare_for_idle() >>>> +{ >>>> +} >>>> + >>>> +void musb_wakeup_from_idle() >>>> +{ >>>> +} >>>> #endif /* CONFIG_USB_MUSB_SOC */ >>>> Index: linux-omap-pm/arch/arm/plat-omap/include/plat/usb.h >>>> =================================================================== >>>> --- linux-omap-pm.orig/arch/arm/plat-omap/include/plat/usb.h >>>> +++ linux-omap-pm/arch/arm/plat-omap/include/plat/usb.h >>>> @@ -79,6 +79,8 @@ extern void usb_ehci_init(const struct e >>>> >>>> extern void usb_ohci_init(const struct >>>ohci_hcd_omap_platform_data *pdata); >>>> >>>> +extern void musb_prepare_for_idle(void); >>>> +extern void musb_wakeup_from_idle(void); >>>> #endif >>>> >>>> >>>> Index: linux-omap-pm/drivers/usb/musb/musb_core.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/drivers/usb/musb/musb_core.c >>>> +++ linux-omap-pm/drivers/usb/musb/musb_core.c >>>> @@ -2410,6 +2410,7 @@ static int musb_suspend(struct device *d >>>> 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; >>>> @@ -2425,6 +2426,9 @@ static int musb_suspend(struct device *d >>>> * they will even be wakeup-enabled. >>>> */ >>>> } >>>> + >>>> + if (plat->save_context) >>>> + plat->save_context = 1; >>>> pm_runtime_put_sync(dev); >>>> >>>> #ifndef CONFIG_PM_RUNTIME >>>> @@ -2443,10 +2447,13 @@ static int musb_resume_noirq(struct devi >>>> { >>>> 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 (plat->restore_context) >>>> + plat->restore_context = 1; >>>> pm_runtime_get_sync(dev); >>>> >>>> #ifndef CONFIG_PM_RUNTIME >>>> @@ -2468,16 +2475,20 @@ static int musb_resume_noirq(struct devi >>>> static int musb_runtime_suspend(struct device *dev) >>>> { >>>> struct musb *musb = dev_to_musb(dev); >>>> + struct musb_hdrc_platform_data *plat = dev->platform_data; >>>> >>>> - musb_save_context(musb); >>>> + if (plat->save_context) >>>> + musb_save_context(musb); >>>> return 0; >>>> } >>>> >>>> static int musb_runtime_resume(struct device *dev) >>>> { >>>> struct musb *musb = dev_to_musb(dev); >>>> + struct musb_hdrc_platform_data *plat = dev->platform_data; >>>> >>>> - musb_restore_context(musb); >>>> + if (plat->restore_context) >>>> + musb_restore_context(musb); >>>> return 0; >>>> } >>>> static const struct dev_pm_ops musb_dev_pm_ops = { >>>> Index: linux-omap-pm/drivers/usb/musb/omap2430.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/drivers/usb/musb/omap2430.c >>>> +++ linux-omap-pm/drivers/usb/musb/omap2430.c >>>> @@ -189,6 +189,19 @@ int musb_platform_set_mode(struct musb * >>>> 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) >>>> { >>>> u32 l; >>>> @@ -232,6 +245,7 @@ int __init musb_platform_init(struct mus >>>> musb->board_set_vbus = omap_set_vbus; >>>> >>>> setup_timer(&musb_idle_timer, musb_do_idle, (unsigned >>>long) musb); >>>> + plat->is_usb_active = is_musb_active; >>>> >>>> return 0; >>>> } >>>> Index: linux-omap-pm/include/linux/usb/musb.h >>>> =================================================================== >>>> --- linux-omap-pm.orig/include/linux/usb/musb.h >>>> +++ linux-omap-pm/include/linux/usb/musb.h >>>> @@ -93,6 +93,8 @@ struct musb_hdrc_config { >>>> >>>> }; >>>> >>>> +struct device; >>>> + >>>> struct musb_hdrc_platform_data { >>>> /* MUSB_HOST, MUSB_PERIPHERAL, or MUSB_OTG */ >>>> u8 mode; >>>> @@ -126,6 +128,17 @@ struct musb_hdrc_platform_data { >>>> >>>> /* Architecture specific board data */ >>>> void *board_data; >>>> + >>>> + /* check usb device active state*/ >>>> + int (*is_usb_active)(struct device *dev); >>>> + >>>> + /* >>>> + * Used for saving and restoring the registers only when core >>>> + * next state is off and previous state was off. >>>> + * Otherwise avoid save restore. >>>> + */ >>>> + int save_context; >>>> + int restore_context; >>>> }; >>>> >>>> >>> > -- 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
