RE: [PATCH 6/8 v2] usb: musb: Offmode fix for idle path

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

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux