Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1464973 > unrolled thread

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

Started byFelipe Balbi <balbi@kernel.org>
First post2016-08-18 09:40 +0200
Last post2016-08-18 15:50 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-08-18 09:40 +0200
    Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-08-18 11:10 +0200
      Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-08-18 14:30 +0200
        Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-08-18 15:00 +0200
          Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-08-18 15:50 +0200

#1464973 — Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

FromFelipe Balbi <balbi@kernel.org>
Date2016-08-18 09:40 +0200
SubjectRe: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
Message-ID<s7q5H-525-3@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> For some mobile devices with strict power management, we also want to suspend
> the host when the slave is detached for power saving.
>
> Thus we add the host suspend/resume functions to support this requirement, and
> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
> suspending the xhci.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/dwc3/Kconfig |    7 +++++++
>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index a64ce1c..725d2bd 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>  	  This is the default mode of working of DWC3 controller where
>  	  both host and gadget features are enabled.
>  
> +config USB_DWC3_HOST_SUSPEND
> +	bool "Choose if the host (xhci) can be suspend/resume"
> +	depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
> +	help
> +	  We can suspend the host when the slave is detached for power saving,
> +	  and resume the host when one slave is attached.
> +
>  endchoice
>  
>  comment "Platform Glue Driver Support"
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1485480..5140b4d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>  static int dwc3_suspend_common(struct dwc3 *dwc)
>  {
>  	unsigned long	flags;
> +	int		ret;
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc3_gadget_suspend(dwc);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
>  	case USB_DR_MODE_OTG:
> +		ret = dwc3_host_suspend(dwc);
> +		if (ret)
> +			return ret;
> +
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_suspend(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
>  		break;
>  	case USB_DR_MODE_HOST:
> +		ret = dwc3_host_suspend(dwc);
> +		if (ret)
> +			return ret;
>  	default:
>  		/* do nothing */
>  		break;
> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc3_gadget_resume(dwc);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
>  	case USB_DR_MODE_OTG:
> +		ret = dwc3_host_resume(dwc);
> +		if (ret)
> +			return ret;
> +
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_resume(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> -		/* FALLTHROUGH */
> +		break;
>  	case USB_DR_MODE_HOST:
> +		ret = dwc3_host_resume(dwc);
> +		if (ret)
> +			return ret;
>  	default:
>  		/* do nothing */
>  		break;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45d6de5..0ba203e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>  { }
>  #endif
>  
> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
> +int dwc3_host_suspend(struct dwc3 *dwc);
> +int dwc3_host_resume(struct dwc3 *dwc);
> +#else
> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> +	return 0;
> +}
> +
> +static inline int dwc3_host_resume(struct dwc3 *dwc)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 2e960ed..2ec3eff 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -17,8 +17,11 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/usb/xhci_pdriver.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
>  
>  #include "core.h"
> +#include "../host/xhci.h"
>  
>  int dwc3_host_init(struct dwc3 *dwc)
>  {
> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	memset(&pdata, 0, sizeof(pdata));
>  
>  	pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
> +	/* dwc3 controller need an extraordinary delay when suspending xhci. */
> +	pdata.usb3_slow_suspend = 1;
>  
>  	ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>  	if (ret) {
> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>  			  dev_name(&dwc->xhci->dev));
>  	platform_device_unregister(dwc->xhci);
>  }
> +
> +int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	int ret, cnt = 20;
> +
> +try_again:
> +	 /* We should wait for xhci bus has been into suspend mode firstly. */
> +	ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
> +	if (ret && --cnt > 0) {
> +		dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
> +			 ret);
> +		msleep(200);
> +		goto try_again;
> +	}
> +
> +	return ret;
> +}
> +
> +int dwc3_host_resume(struct dwc3 *dwc)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +	return xhci_resume(xhci, 0);
> +}

This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
xhci_supend() has already been called. Why isn't it called for you?

-- 
balbi

[toc] | [next] | [standalone]

#1465020

FromBaolin Wang <baolin.wang@linaro.org>
Date2016-08-18 11:10 +0200
Message-ID<s7ruO-66w-25@gated-at.bofh.it>
In reply to#1464973
Hi Felipe,

On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> For some mobile devices with strict power management, we also want to suspend
>> the host when the slave is detached for power saving.
>>
>> Thus we add the host suspend/resume functions to support this requirement, and
>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>> suspending the xhci.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index a64ce1c..725d2bd 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>         This is the default mode of working of DWC3 controller where
>>         both host and gadget features are enabled.
>>
>> +config USB_DWC3_HOST_SUSPEND
>> +     bool "Choose if the host (xhci) can be suspend/resume"
>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>> +     help
>> +       We can suspend the host when the slave is detached for power saving,
>> +       and resume the host when one slave is attached.
>> +
>>  endchoice
>>
>>  comment "Platform Glue Driver Support"
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 1485480..5140b4d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>  {
>>       unsigned long   flags;
>> +     int             ret;
>>
>>       switch (dwc->dr_mode) {
>>       case USB_DR_MODE_PERIPHERAL:
>> +             spin_lock_irqsave(&dwc->lock, flags);
>> +             dwc3_gadget_suspend(dwc);
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +             break;
>>       case USB_DR_MODE_OTG:
>> +             ret = dwc3_host_suspend(dwc);
>> +             if (ret)
>> +                     return ret;
>> +
>>               spin_lock_irqsave(&dwc->lock, flags);
>>               dwc3_gadget_suspend(dwc);
>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>               break;
>>       case USB_DR_MODE_HOST:
>> +             ret = dwc3_host_suspend(dwc);
>> +             if (ret)
>> +                     return ret;
>>       default:
>>               /* do nothing */
>>               break;
>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>
>>       switch (dwc->dr_mode) {
>>       case USB_DR_MODE_PERIPHERAL:
>> +             spin_lock_irqsave(&dwc->lock, flags);
>> +             dwc3_gadget_resume(dwc);
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +             break;
>>       case USB_DR_MODE_OTG:
>> +             ret = dwc3_host_resume(dwc);
>> +             if (ret)
>> +                     return ret;
>> +
>>               spin_lock_irqsave(&dwc->lock, flags);
>>               dwc3_gadget_resume(dwc);
>>               spin_unlock_irqrestore(&dwc->lock, flags);
>> -             /* FALLTHROUGH */
>> +             break;
>>       case USB_DR_MODE_HOST:
>> +             ret = dwc3_host_resume(dwc);
>> +             if (ret)
>> +                     return ret;
>>       default:
>>               /* do nothing */
>>               break;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 45d6de5..0ba203e 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>  { }
>>  #endif
>>
>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>> +int dwc3_host_suspend(struct dwc3 *dwc);
>> +int dwc3_host_resume(struct dwc3 *dwc);
>> +#else
>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index 2e960ed..2ec3eff 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -17,8 +17,11 @@
>>
>>  #include <linux/platform_device.h>
>>  #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>>
>>  #include "core.h"
>> +#include "../host/xhci.h"
>>
>>  int dwc3_host_init(struct dwc3 *dwc)
>>  {
>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>       memset(&pdata, 0, sizeof(pdata));
>>
>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>> +     pdata.usb3_slow_suspend = 1;
>>
>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>       if (ret) {
>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>                         dev_name(&dwc->xhci->dev));
>>       platform_device_unregister(dwc->xhci);
>>  }
>> +
>> +int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +     int ret, cnt = 20;
>> +
>> +try_again:
>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>> +     if (ret && --cnt > 0) {
>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>> +                      ret);
>> +             msleep(200);
>> +             goto try_again;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +
>> +     return xhci_resume(xhci, 0);
>> +}
>
> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
> xhci_supend() has already been called. Why isn't it called for you?

xhci-plat.c did not set the runtime PM callbacks, how can we issued
xhci_supend() by runtime PM from dwc3?

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1465221

FromFelipe Balbi <balbi@kernel.org>
Date2016-08-18 14:30 +0200
Message-ID<s7uCm-8aC-21@gated-at.bofh.it>
In reply to#1465020

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Felipe,
>
> On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> For some mobile devices with strict power management, we also want to suspend
>>> the host when the slave is detached for power saving.
>>>
>>> Thus we add the host suspend/resume functions to support this requirement, and
>>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>>> suspending the xhci.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index a64ce1c..725d2bd 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>>         This is the default mode of working of DWC3 controller where
>>>         both host and gadget features are enabled.
>>>
>>> +config USB_DWC3_HOST_SUSPEND
>>> +     bool "Choose if the host (xhci) can be suspend/resume"
>>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>> +     help
>>> +       We can suspend the host when the slave is detached for power saving,
>>> +       and resume the host when one slave is attached.
>>> +
>>>  endchoice
>>>
>>>  comment "Platform Glue Driver Support"
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 1485480..5140b4d 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>>  {
>>>       unsigned long   flags;
>>> +     int             ret;
>>>
>>>       switch (dwc->dr_mode) {
>>>       case USB_DR_MODE_PERIPHERAL:
>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>> +             dwc3_gadget_suspend(dwc);
>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>> +             break;
>>>       case USB_DR_MODE_OTG:
>>> +             ret = dwc3_host_suspend(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>               dwc3_gadget_suspend(dwc);
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>               break;
>>>       case USB_DR_MODE_HOST:
>>> +             ret = dwc3_host_suspend(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>>       default:
>>>               /* do nothing */
>>>               break;
>>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>
>>>       switch (dwc->dr_mode) {
>>>       case USB_DR_MODE_PERIPHERAL:
>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>> +             dwc3_gadget_resume(dwc);
>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>> +             break;
>>>       case USB_DR_MODE_OTG:
>>> +             ret = dwc3_host_resume(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>               dwc3_gadget_resume(dwc);
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>> -             /* FALLTHROUGH */
>>> +             break;
>>>       case USB_DR_MODE_HOST:
>>> +             ret = dwc3_host_resume(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>>       default:
>>>               /* do nothing */
>>>               break;
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 45d6de5..0ba203e 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>  { }
>>>  #endif
>>>
>>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>> +#else
>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +     return 0;
>>> +}
>>> +#endif
>>> +
>>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>> index 2e960ed..2ec3eff 100644
>>> --- a/drivers/usb/dwc3/host.c
>>> +++ b/drivers/usb/dwc3/host.c
>>> @@ -17,8 +17,11 @@
>>>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/usb/xhci_pdriver.h>
>>> +#include <linux/usb.h>
>>> +#include <linux/usb/hcd.h>
>>>
>>>  #include "core.h"
>>> +#include "../host/xhci.h"
>>>
>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>  {
>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>       memset(&pdata, 0, sizeof(pdata));
>>>
>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>> +     pdata.usb3_slow_suspend = 1;
>>>
>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>       if (ret) {
>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>                         dev_name(&dwc->xhci->dev));
>>>       platform_device_unregister(dwc->xhci);
>>>  }
>>> +
>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +     int ret, cnt = 20;
>>> +
>>> +try_again:
>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>> +     if (ret && --cnt > 0) {
>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>> +                      ret);
>>> +             msleep(200);
>>> +             goto try_again;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +
>>> +     return xhci_resume(xhci, 0);
>>> +}
>>
>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>> xhci_supend() has already been called. Why isn't it called for you?
>
> xhci-plat.c did not set the runtime PM callbacks, how can we issued
> xhci_supend() by runtime PM from dwc3?

wouldn't it be nicer to just initialize PM runtime callbacks from
xhci-plat? It just needs to be added and verified, right?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1465236

FromBaolin Wang <baolin.wang@linaro.org>
Date2016-08-18 15:00 +0200
Message-ID<s7v5o-8lC-13@gated-at.bofh.it>
In reply to#1465221
Hi Felipe,

On 18 August 2016 at 20:25, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> For some mobile devices with strict power management, we also want to suspend
>>>> the host when the slave is detached for power saving.
>>>>
>>>> Thus we add the host suspend/resume functions to support this requirement, and
>>>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>>>> suspending the xhci.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index a64ce1c..725d2bd 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>>>         This is the default mode of working of DWC3 controller where
>>>>         both host and gadget features are enabled.
>>>>
>>>> +config USB_DWC3_HOST_SUSPEND
>>>> +     bool "Choose if the host (xhci) can be suspend/resume"
>>>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>> +     help
>>>> +       We can suspend the host when the slave is detached for power saving,
>>>> +       and resume the host when one slave is attached.
>>>> +
>>>>  endchoice
>>>>
>>>>  comment "Platform Glue Driver Support"
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 1485480..5140b4d 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>>>  {
>>>>       unsigned long   flags;
>>>> +     int             ret;
>>>>
>>>>       switch (dwc->dr_mode) {
>>>>       case USB_DR_MODE_PERIPHERAL:
>>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>>> +             dwc3_gadget_suspend(dwc);
>>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +             break;
>>>>       case USB_DR_MODE_OTG:
>>>> +             ret = dwc3_host_suspend(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>>               dwc3_gadget_suspend(dwc);
>>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>>               break;
>>>>       case USB_DR_MODE_HOST:
>>>> +             ret = dwc3_host_suspend(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>>       default:
>>>>               /* do nothing */
>>>>               break;
>>>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>>
>>>>       switch (dwc->dr_mode) {
>>>>       case USB_DR_MODE_PERIPHERAL:
>>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>>> +             dwc3_gadget_resume(dwc);
>>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +             break;
>>>>       case USB_DR_MODE_OTG:
>>>> +             ret = dwc3_host_resume(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>>               dwc3_gadget_resume(dwc);
>>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>> -             /* FALLTHROUGH */
>>>> +             break;
>>>>       case USB_DR_MODE_HOST:
>>>> +             ret = dwc3_host_resume(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>>       default:
>>>>               /* do nothing */
>>>>               break;
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 45d6de5..0ba203e 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>>  { }
>>>>  #endif
>>>>
>>>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>>> +#else
>>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>> index 2e960ed..2ec3eff 100644
>>>> --- a/drivers/usb/dwc3/host.c
>>>> +++ b/drivers/usb/dwc3/host.c
>>>> @@ -17,8 +17,11 @@
>>>>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/usb/xhci_pdriver.h>
>>>> +#include <linux/usb.h>
>>>> +#include <linux/usb/hcd.h>
>>>>
>>>>  #include "core.h"
>>>> +#include "../host/xhci.h"
>>>>
>>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>>  {
>>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>       memset(&pdata, 0, sizeof(pdata));
>>>>
>>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>>> +     pdata.usb3_slow_suspend = 1;
>>>>
>>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>>       if (ret) {
>>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>>                         dev_name(&dwc->xhci->dev));
>>>>       platform_device_unregister(dwc->xhci);
>>>>  }
>>>> +
>>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +     int ret, cnt = 20;
>>>> +
>>>> +try_again:
>>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>>> +     if (ret && --cnt > 0) {
>>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>>> +                      ret);
>>>> +             msleep(200);
>>>> +             goto try_again;
>>>> +     }
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +
>>>> +     return xhci_resume(xhci, 0);
>>>> +}
>>>
>>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>>> xhci_supend() has already been called. Why isn't it called for you?
>>
>> xhci-plat.c did not set the runtime PM callbacks, how can we issued
>> xhci_supend() by runtime PM from dwc3?
>
> wouldn't it be nicer to just initialize PM runtime callbacks from
> xhci-plat? It just needs to be added and verified, right?

Yes, you are right. Alan and Peter also suggested me to implement the
runtime PM callbacks for xhci-plat. I am also testing this and I will
send out the patch if it works well. So please ignore this patch.
Thanks for your comments.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1465368

FromFelipe Balbi <balbi@kernel.org>
Date2016-08-18 15:50 +0200
Message-ID<s7vRM-sf-53@gated-at.bofh.it>
In reply to#1465236

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:

[...]

>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>>> index 2e960ed..2ec3eff 100644
>>>>> --- a/drivers/usb/dwc3/host.c
>>>>> +++ b/drivers/usb/dwc3/host.c
>>>>> @@ -17,8 +17,11 @@
>>>>>
>>>>>  #include <linux/platform_device.h>
>>>>>  #include <linux/usb/xhci_pdriver.h>
>>>>> +#include <linux/usb.h>
>>>>> +#include <linux/usb/hcd.h>
>>>>>
>>>>>  #include "core.h"
>>>>> +#include "../host/xhci.h"
>>>>>
>>>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>>>  {
>>>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>>       memset(&pdata, 0, sizeof(pdata));
>>>>>
>>>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>>>> +     pdata.usb3_slow_suspend = 1;
>>>>>
>>>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>>>       if (ret) {
>>>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>>>                         dev_name(&dwc->xhci->dev));
>>>>>       platform_device_unregister(dwc->xhci);
>>>>>  }
>>>>> +
>>>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>>>> +{
>>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> +     int ret, cnt = 20;
>>>>> +
>>>>> +try_again:
>>>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>>>> +     if (ret && --cnt > 0) {
>>>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>>>> +                      ret);
>>>>> +             msleep(200);
>>>>> +             goto try_again;
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>>>> +{
>>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> +
>>>>> +     return xhci_resume(xhci, 0);
>>>>> +}
>>>>
>>>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>>>> xhci_supend() has already been called. Why isn't it called for you?
>>>
>>> xhci-plat.c did not set the runtime PM callbacks, how can we issued
>>> xhci_supend() by runtime PM from dwc3?
>>
>> wouldn't it be nicer to just initialize PM runtime callbacks from
>> xhci-plat? It just needs to be added and verified, right?
>
> Yes, you are right. Alan and Peter also suggested me to implement the
> runtime PM callbacks for xhci-plat. I am also testing this and I will
> send out the patch if it works well. So please ignore this patch.
> Thanks for your comments.

cool, thanks a lot :-)

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web