RE: [PATCH 9/9 v3] usb : musb: Offmode fix for idle path

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

 



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


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

  Powered by Linux