diff mbox

usb: dwc3: Support the dwc3 host suspend/resume

Message ID 2086ce508be1c1d75d47c892a5be9593b12fa6f3.1500619870.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang July 21, 2017, 6:58 a.m. UTC 
For some mobile devices with strict power management, we also want to
suspend the host when the slave was detached for power saving. Thus
adding the host suspend/resume functions to support this requirement.

We will issue the pm_suspend_ignore_children() for the dwc3 device,
since we will resume the child device (xHCI device) in runtime resume
callback (dwc3_host_resume()) of dwc3 device, now the dwc3 device's
runtime state is not RPM_ACTIVE, which will block to resume its
child device (xHCI device). Add ignore children flag can avoid this
situation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
 drivers/usb/dwc3/core.c |   26 +++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h |    7 +++++++
 drivers/usb/dwc3/host.c |   15 +++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
1.7.9.5

Comments

Manu Gautam July 21, 2017, 8:45 a.m. UTC | #1 
Hi,


On 7/21/2017 12:28 PM, Baolin Wang wrote:
> For some mobile devices with strict power management, we also want to

> suspend the host when the slave was detached for power saving. Thus

> adding the host suspend/resume functions to support this requirement.



USB/PM core already takes care of suspending root-HUB/XHCI when
no device connected.

>

> We will issue the pm_suspend_ignore_children() for the dwc3 device,

> since we will resume the child device (xHCI device) in runtime resume

> callback (dwc3_host_resume()) of dwc3 device, now the dwc3 device's

> runtime state is not RPM_ACTIVE, which will block to resume its

> child device (xHCI device). Add ignore children flag can avoid this

> situation.


This defeats the basic purpose of runtime PM. Without ignore_children
once USB bus is suspended, dwc3 gets suspended and then dwc3 glue device.
Only requirement I see from the patch is to resume XHCI/root-hub on
dwc3 resume. I am sure there must be other way to deal with that e.g.
doing same from glue driver by using a wq or use pm_request_resume()

>

> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

> ---

>  drivers/usb/dwc3/core.c |   26 +++++++++++++++++++++++++-

>  drivers/usb/dwc3/core.h |    7 +++++++

>  drivers/usb/dwc3/host.c |   15 +++++++++++++++

>  3 files changed, 47 insertions(+), 1 deletion(-)

>

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

> index 326b302..2be7ddc 100644

> --- a/drivers/usb/dwc3/core.c

> +++ b/drivers/usb/dwc3/core.c

> @@ -1193,6 +1193,7 @@ static int dwc3_probe(struct platform_device *pdev)

>  	pm_runtime_use_autosuspend(dev);

>  	pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);

>  	pm_runtime_enable(dev);

> +	pm_suspend_ignore_children(dev, true);

>  	ret = pm_runtime_get_sync(dev);

>  	if (ret < 0)

>  		goto err1;

> @@ -1292,15 +1293,27 @@ static int dwc3_remove(struct platform_device *pdev)

>  static int dwc3_suspend_common(struct dwc3 *dwc)


What is the trigger for runtime suspend now that you have ignore_children set.
>  {

>  	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);

With DRD/OTG, if current mode is device and dwc3->xhci won't be valid.
You can refer to the patch that I pushed to address this.

> +		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;

> @@ -1322,12 +1335,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 ea910ac..9b5a4eb 100644

> --- a/drivers/usb/dwc3/core.h

> +++ b/drivers/usb/dwc3/core.h

> @@ -1193,11 +1193,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc)

>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

>  int dwc3_host_init(struct dwc3 *dwc);

>  void dwc3_host_exit(struct dwc3 *dwc);

> +int dwc3_host_suspend(struct dwc3 *dwc);

> +int dwc3_host_resume(struct dwc3 *dwc);

>  #else

>  static inline int dwc3_host_init(struct dwc3 *dwc)

>  { return 0; }

>  static inline void dwc3_host_exit(struct dwc3 *dwc)

>  { }

> +

> +static inline int dwc3_host_suspend(struct dwc3 *dwc)

> +{ return 0; }

> +static inline int dwc3_host_resume(struct dwc3 *dwc)

> +{ return 0; }

>  #endif

>  

>  #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c

> index 76f0b0d..3fa4414 100644

> --- a/drivers/usb/dwc3/host.c

> +++ b/drivers/usb/dwc3/host.c

> @@ -16,6 +16,7 @@

>   */

>  

>  #include <linux/platform_device.h>

> +#include <linux/pm_runtime.h>

>  

>  #include "core.h"

>  

> @@ -151,3 +152,17 @@ void dwc3_host_exit(struct dwc3 *dwc)

>  			  dev_name(dwc->dev));

>  	platform_device_unregister(dwc->xhci);

>  }

> +

> +int dwc3_host_suspend(struct dwc3 *dwc)

> +{

> +	struct device *xhci = &dwc->xhci->dev;

> +

> +	return pm_runtime_suspend(xhci);

If ignore_children is not marked then xhci would indeed be suspended.

> +}

> +

> +int dwc3_host_resume(struct dwc3 *dwc)

> +{

> +	struct device *xhci = &dwc->xhci->dev;

> +

> +	return pm_runtime_resume(xhci);


Other than what I pushed in my patch -
("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")
Just performing pm_request_resume or handling same in dwc3 glue
driver should be sufficient.
Also, what is trigger for runtime_resume for your platform?
(Exiting) Baolin Wang July 21, 2017, 9:01 a.m. UTC | #2 
On 21 July 2017 at 16:45, Manu Gautam <mgautam@codeaurora.org> wrote:
> Hi,

>

>

> On 7/21/2017 12:28 PM, Baolin Wang wrote:

>> For some mobile devices with strict power management, we also want to

>> suspend the host when the slave was detached for power saving. Thus

>> adding the host suspend/resume functions to support this requirement.

>

>

> USB/PM core already takes care of suspending root-HUB/XHCI when

> no device connected.


Correct, but what I mean is we can power off the dwc3 controller when
there are no device connected.

>

>>

>> We will issue the pm_suspend_ignore_children() for the dwc3 device,

>> since we will resume the child device (xHCI device) in runtime resume

>> callback (dwc3_host_resume()) of dwc3 device, now the dwc3 device's

>> runtime state is not RPM_ACTIVE, which will block to resume its

>> child device (xHCI device). Add ignore children flag can avoid this

>> situation.

>

> This defeats the basic purpose of runtime PM. Without ignore_children

> once USB bus is suspended, dwc3 gets suspended and then dwc3 glue device.

> Only requirement I see from the patch is to resume XHCI/root-hub on

> dwc3 resume. I am sure there must be other way to deal with that e.g.

> doing same from glue driver by using a wq or use pm_request_resume()


Ah, I will try pm_request_resume().

>

>>

>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

>> ---

>>  drivers/usb/dwc3/core.c |   26 +++++++++++++++++++++++++-

>>  drivers/usb/dwc3/core.h |    7 +++++++

>>  drivers/usb/dwc3/host.c |   15 +++++++++++++++

>>  3 files changed, 47 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>> index 326b302..2be7ddc 100644

>> --- a/drivers/usb/dwc3/core.c

>> +++ b/drivers/usb/dwc3/core.c

>> @@ -1193,6 +1193,7 @@ static int dwc3_probe(struct platform_device *pdev)

>>       pm_runtime_use_autosuspend(dev);

>>       pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);

>>       pm_runtime_enable(dev);

>> +     pm_suspend_ignore_children(dev, true);

>>       ret = pm_runtime_get_sync(dev);

>>       if (ret < 0)

>>               goto err1;

>> @@ -1292,15 +1293,27 @@ static int dwc3_remove(struct platform_device *pdev)

>>  static int dwc3_suspend_common(struct dwc3 *dwc)

>

> What is the trigger for runtime suspend now that you have ignore_children set.

>>  {

>>       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);

> With DRD/OTG, if current mode is device and dwc3->xhci won't be valid.


If current mode is device, then xHCI device is always in suspend state.

> You can refer to the patch that I pushed to address this.

>

>> +             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;

>> @@ -1322,12 +1335,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 ea910ac..9b5a4eb 100644

>> --- a/drivers/usb/dwc3/core.h

>> +++ b/drivers/usb/dwc3/core.h

>> @@ -1193,11 +1193,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc)

>>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

>>  int dwc3_host_init(struct dwc3 *dwc);

>>  void dwc3_host_exit(struct dwc3 *dwc);

>> +int dwc3_host_suspend(struct dwc3 *dwc);

>> +int dwc3_host_resume(struct dwc3 *dwc);

>>  #else

>>  static inline int dwc3_host_init(struct dwc3 *dwc)

>>  { return 0; }

>>  static inline void dwc3_host_exit(struct dwc3 *dwc)

>>  { }

>> +

>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)

>> +{ return 0; }

>> +static inline int dwc3_host_resume(struct dwc3 *dwc)

>> +{ return 0; }

>>  #endif

>>

>>  #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c

>> index 76f0b0d..3fa4414 100644

>> --- a/drivers/usb/dwc3/host.c

>> +++ b/drivers/usb/dwc3/host.c

>> @@ -16,6 +16,7 @@

>>   */

>>

>>  #include <linux/platform_device.h>

>> +#include <linux/pm_runtime.h>

>>

>>  #include "core.h"

>>

>> @@ -151,3 +152,17 @@ void dwc3_host_exit(struct dwc3 *dwc)

>>                         dev_name(dwc->dev));

>>       platform_device_unregister(dwc->xhci);

>>  }

>> +

>> +int dwc3_host_suspend(struct dwc3 *dwc)

>> +{

>> +     struct device *xhci = &dwc->xhci->dev;

>> +

>> +     return pm_runtime_suspend(xhci);

> If ignore_children is not marked then xhci would indeed be suspended.

>

>> +}

>> +

>> +int dwc3_host_resume(struct dwc3 *dwc)

>> +{

>> +     struct device *xhci = &dwc->xhci->dev;

>> +

>> +     return pm_runtime_resume(xhci);

>

> Other than what I pushed in my patch -

> ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")

> Just performing pm_request_resume or handling same in dwc3 glue

> driver should be sufficient.


Yes.

> Also, what is trigger for runtime_resume for your platform?


In our platform glue layer, we have extcon events to notify there are
devices connected, then gule layer will resume dwc3 device.

-- 
Baolin.wang
Best Regards
Manu Gautam July 21, 2017, 9:13 a.m. UTC | #3 
Hi,

On 7/21/2017 2:31 PM, Baolin Wang wrote:
> On 21 July 2017 at 16:45, Manu Gautam <mgautam@codeaurora.org> wrote:

>> Hi,

>>

>>

>> On 7/21/2017 12:28 PM, Baolin Wang wrote:

>>> For some mobile devices with strict power management, we also want to

>>> suspend the host when the slave was detached for power saving. Thus

>>> adding the host suspend/resume functions to support this requirement.

>>

>> USB/PM core already takes care of suspending root-HUB/XHCI when

>> no device connected.

> Correct, but what I mean is we can power off the dwc3 controller when

> there are no device connected.

>

>>> We will issue the pm_suspend_ignore_children() for the dwc3 device,

>>> since we will resume the child device (xHCI device) in runtime resume

>>> callback (dwc3_host_resume()) of dwc3 device, now the dwc3 device's

>>> runtime state is not RPM_ACTIVE, which will block to resume its

>>> child device (xHCI device). Add ignore children flag can avoid this

>>> situation.

>> This defeats the basic purpose of runtime PM. Without ignore_children

>> once USB bus is suspended, dwc3 gets suspended and then dwc3 glue device.

>> Only requirement I see from the patch is to resume XHCI/root-hub on

>> dwc3 resume. I am sure there must be other way to deal with that e.g.

>> doing same from glue driver by using a wq or use pm_request_resume()

> Ah, I will try pm_request_resume().

>

>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

>>> ---

>>>  drivers/usb/dwc3/core.c |   26 +++++++++++++++++++++++++-

>>>  drivers/usb/dwc3/core.h |    7 +++++++

>>>  drivers/usb/dwc3/host.c |   15 +++++++++++++++

>>>  3 files changed, 47 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c

>>> index 326b302..2be7ddc 100644

>>> --- a/drivers/usb/dwc3/core.c

>>> +++ b/drivers/usb/dwc3/core.c

>>> @@ -1193,6 +1193,7 @@ static int dwc3_probe(struct platform_device *pdev)

>>>       pm_runtime_use_autosuspend(dev);

>>>       pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);

>>>       pm_runtime_enable(dev);

>>> +     pm_suspend_ignore_children(dev, true);

>>>       ret = pm_runtime_get_sync(dev);

>>>       if (ret < 0)

>>>               goto err1;

>>> @@ -1292,15 +1293,27 @@ static int dwc3_remove(struct platform_device *pdev)

>>>  static int dwc3_suspend_common(struct dwc3 *dwc)

>> What is the trigger for runtime suspend now that you have ignore_children set.

>>>  {

>>>       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);

>> With DRD/OTG, if current mode is device and dwc3->xhci won't be valid.

> If current mode is device, then xHCI device is always in suspend state.

dwc->xhci is allocated in dwc3_host_init. And dwc->xhci device is unregistred
from dwc3_host_exit that is called from __dwc3_set_mode.
>

>> You can refer to the patch that I pushed to address this.

>>

>>> +             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;

>>> @@ -1322,12 +1335,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 ea910ac..9b5a4eb 100644

>>> --- a/drivers/usb/dwc3/core.h

>>> +++ b/drivers/usb/dwc3/core.h

>>> @@ -1193,11 +1193,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc)

>>>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

>>>  int dwc3_host_init(struct dwc3 *dwc);

>>>  void dwc3_host_exit(struct dwc3 *dwc);

>>> +int dwc3_host_suspend(struct dwc3 *dwc);

>>> +int dwc3_host_resume(struct dwc3 *dwc);

>>>  #else

>>>  static inline int dwc3_host_init(struct dwc3 *dwc)

>>>  { return 0; }

>>>  static inline void dwc3_host_exit(struct dwc3 *dwc)

>>>  { }

>>> +

>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)

>>> +{ return 0; }

>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)

>>> +{ return 0; }

>>>  #endif

>>>

>>>  #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)

>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c

>>> index 76f0b0d..3fa4414 100644

>>> --- a/drivers/usb/dwc3/host.c

>>> +++ b/drivers/usb/dwc3/host.c

>>> @@ -16,6 +16,7 @@

>>>   */

>>>

>>>  #include <linux/platform_device.h>

>>> +#include <linux/pm_runtime.h>

>>>

>>>  #include "core.h"

>>>

>>> @@ -151,3 +152,17 @@ void dwc3_host_exit(struct dwc3 *dwc)

>>>                         dev_name(dwc->dev));

>>>       platform_device_unregister(dwc->xhci);

>>>  }

>>> +

>>> +int dwc3_host_suspend(struct dwc3 *dwc)

>>> +{

>>> +     struct device *xhci = &dwc->xhci->dev;

>>> +

>>> +     return pm_runtime_suspend(xhci);

>> If ignore_children is not marked then xhci would indeed be suspended.

>>

>>> +}

>>> +

>>> +int dwc3_host_resume(struct dwc3 *dwc)

>>> +{

>>> +     struct device *xhci = &dwc->xhci->dev;

>>> +

>>> +     return pm_runtime_resume(xhci);

>> Other than what I pushed in my patch -

>> ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")

>> Just performing pm_request_resume or handling same in dwc3 glue

>> driver should be sufficient.

> Yes.

>

>> Also, what is trigger for runtime_resume for your platform?

> In our platform glue layer, we have extcon events to notify there are

> devices connected, then gule layer will resume dwc3 device.

>


IMO you can just perform resume of &dwc->xhci->dev instead of resuming dwc3.
Since, dwc3 is parent of xhci that will trigger resume of dwc3 as well.
(Exiting) Baolin Wang July 24, 2017, 11:42 a.m. UTC | #4 
Hi Manu,

On 24 July 2017 at 17:38, Manu Gautam <mgautam@codeaurora.org> wrote:
> Hi,

>

>

> On 7/24/2017 2:45 PM, Baolin Wang wrote:

>> Hi Manu,

>>

>> On 24 July 2017 at 16:42, Manu Gautam <mgautam@codeaurora.org> wrote:

>>> Hi Baolin,

>>>

>>>

>>> On 7/24/2017 11:26 AM, Baolin Wang wrote:

>>>

>>>>>>> Other than what I pushed in my patch -

>>>>>>> ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")

>>>>>>> Just performing pm_request_resume or handling same in dwc3 glue

>>>>>>> driver should be sufficient.

>>>>>> Yes.

>>>>>>

>>>>>>> Also, what is trigger for runtime_resume for your platform?

>>>>>> In our platform glue layer, we have extcon events to notify there are

>>>>>> devices connected, then gule layer will resume dwc3 device.

>>>>>>

>>>>> IMO you can just perform resume of &dwc->xhci->dev instead of resuming dwc3.

>>>>> Since, dwc3 is parent of xhci that will trigger resume of dwc3 as well.

>>>> I am not sure this is good enough but it seems can solve some

>>>> problems, then we do not need resume/suspend host in dwc3 core.

>>> In that case can we proceed with this patch:

>>> [1] https://www.spinics.net/lists/linux-usb/msg158682.html

>>>

>>> For your platform you can pick this patch and resume dwc->xhci->dev resume

>>> from your glue driver.

>>> Do you have any other concerns?

>> The same concern I explained in your patch. If we power off the

>> controller when suspend, how can controller work well as host if we do

>> not issue dwc3_core_init()? Suppose below cases:

>> device attached: resume glue layer (power on dwc3 controller) --->

>> resume dwc3 device ---> resume xHCI device

> Just resume won't do. XHCI also needs to be reinitialized.

>

>> no device attached: suspend xHCI device ---> suspend dwc3 device --->

>> suspend glue layer (we can power off the dwc3 controller now)

>>

>> Though now there are no upstreamed glue drivers will power off dwc3

>> controller, should we consider this situation?

>>

>

> Can we discuss on that patch thread itself?

>

> How will this platform handle runtime PM with only external HUB

> connected? Will runtime suspend take place?

> Would connecting device to that HUB's port be detected?


No, it should not suspend xHCI now. Maybe need get xHCI device usage
counter for this case in case xHCI can suspend automatically if there
are no device connected.

>

> Since, there is no upstreamed glue driver currently doing, we can

> probably ignore that. Also, above sequnence wont work without xhci init.

>

> Also, for this particular case glue driver can take care of re-initializing of

> dwc3 core as well.


Glue layer can not re-initialize of dwc3 core now since
dwc3_core_init() is not exported. Let's see what Felipe's suggestion
on your patch thread.

-- 
Baolin.wang
Best Regards
diff mbox

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 326b302..2be7ddc 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1193,6 +1193,7 @@  static int dwc3_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
 	pm_runtime_enable(dev);
+	pm_suspend_ignore_children(dev, true);
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err1;
@@ -1292,15 +1293,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;
@@ -1322,12 +1335,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 ea910ac..9b5a4eb 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1193,11 +1193,18 @@  static inline bool dwc3_is_usb31(struct dwc3 *dwc)
 #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
 int dwc3_host_init(struct dwc3 *dwc);
 void dwc3_host_exit(struct dwc3 *dwc);
+int dwc3_host_suspend(struct dwc3 *dwc);
+int dwc3_host_resume(struct dwc3 *dwc);
 #else
 static inline int dwc3_host_init(struct dwc3 *dwc)
 { return 0; }
 static inline void dwc3_host_exit(struct dwc3 *dwc)
 { }
+
+static inline int dwc3_host_suspend(struct dwc3 *dwc)
+{ return 0; }
+static inline int dwc3_host_resume(struct dwc3 *dwc)
+{ return 0; }
 #endif
 
 #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 76f0b0d..3fa4414 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -16,6 +16,7 @@ 
  */
 
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 
 #include "core.h"
 
@@ -151,3 +152,17 @@  void dwc3_host_exit(struct dwc3 *dwc)
 			  dev_name(dwc->dev));
 	platform_device_unregister(dwc->xhci);
 }
+
+int dwc3_host_suspend(struct dwc3 *dwc)
+{
+	struct device *xhci = &dwc->xhci->dev;
+
+	return pm_runtime_suspend(xhci);
+}
+
+int dwc3_host_resume(struct dwc3 *dwc)
+{
+	struct device *xhci = &dwc->xhci->dev;
+
+	return pm_runtime_resume(xhci);
+}