LinuxLists.cc - [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[permalink] [raw]

Subject: [RESEND PATCH 3/3] usb: dwc3: core: Notify current USB mode to USB3 PHY as well

Driver currently notifies only USB2 PHY on USB mode change.
Extend this to USB3 PHY so that PHY drivers based on the
mode can release system resources - clocks, regulators etc.
Additionally Qualcomm QMP and QUSB2 PHY drivers need to
override VBUS signal in PHY wrapper in device mode as USB
VBUS line is not connected to PHYs. Also, remove NULL checks
for PHY when calling phy_set_mode as PHY ops already check this.

Signed-off-by: Manu Gautam <[email protected]>
---
drivers/usb/dwc3/core.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 23bb192..dabfa16 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -156,9 +156,8 @@ static void __dwc3_set_mode(struct work_struct *work)
} else {
if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, true);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-
+ phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+ phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
}
break;
case DWC3_GCTL_PRTCAP_DEVICE:
@@ -166,8 +165,8 @@ static void __dwc3_set_mode(struct work_struct *work)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, false);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);

ret = dwc3_gadget_init(dwc);
if (ret)
@@ -932,8 +931,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, false);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);

ret = dwc3_gadget_init(dwc);
if (ret) {
@@ -948,8 +947,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, true);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+ phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+ phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);

ret = dwc3_host_init(dwc);
if (ret) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2017-09-27 11:20:26

[permalink] [raw]

Subject: [RESEND PATCH 2/3] usb: dwc3: pci: Runtime resume child device from wq

Driver currently resumes and increments pm usage_count
of its child device (dwc3 main) from its runtime_resume
handler. This requires dwc3 runtime_resume to perform
pm_runtime_put to decrement the pm usage_count. However
runtime_put from dwc3 happens for non pci drivers
(e.g. dwc3-if-simple.c) as well which results in dwc3
pm usage_count becoming negative after couple of
runtime suspend resume iterations. Fix this by
performing runtime_get/put from dwc3-pci driver only
using workqueue.

Signed-off-by: Manu Gautam <[email protected]>
---
drivers/usb/dwc3/core.c | 1 -
drivers/usb/dwc3/dwc3-pci.c | 29 +++++++++++++++++++++++++++--
2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f75613f..23bb192 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1391,7 +1391,6 @@ static int dwc3_runtime_resume(struct device *dev)
}

pm_runtime_mark_last_busy(dev);
- pm_runtime_put(dev);

return 0;
}
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 7e995df..74e4cd3 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/pci.h>
+#include <linux/workqueue.h>
#include <linux/pm_runtime.h>
#include <linux/platform_device.h>
#include <linux/gpio/consumer.h>
@@ -61,6 +62,7 @@ struct dwc3_pci {
guid_t guid;

unsigned int has_dsm_for_pm:1;
+ struct work_struct wakeup_work;
};

static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
@@ -174,6 +176,22 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
return 0;
}

+#ifdef CONFIG_PM
+static void dwc3_pci_resume_work(struct work_struct *work)
+{
+ struct dwc3_pci *dwc = container_of(work, struct dwc3_pci, wakeup_work);
+ struct platform_device *dwc3 = dwc->dwc3;
+ int ret;
+
+ ret = pm_runtime_get_sync(&dwc3->dev);
+ if (ret)
+ return;
+
+ pm_runtime_mark_last_busy(&dwc3->dev);
+ pm_runtime_put_sync_autosuspend(&dwc3->dev);
+}
+#endif
+
static int dwc3_pci_probe(struct pci_dev *pci,
const struct pci_device_id *id)
{
@@ -232,6 +250,9 @@ static int dwc3_pci_probe(struct pci_dev *pci,
device_init_wakeup(dev, true);
pci_set_drvdata(pci, dwc);
pm_runtime_put(dev);
+#ifdef CONFIG_PM
+ INIT_WORK(&dwc->wakeup_work, dwc3_pci_resume_work);
+#endif

return 0;
err:
@@ -243,6 +264,9 @@ static void dwc3_pci_remove(struct pci_dev *pci)
{
struct dwc3_pci *dwc = pci_get_drvdata(pci);

+#ifdef CONFIG_PM
+ cancel_work_sync(&dwc->wakeup_work);
+#endif
device_init_wakeup(&pci->dev, false);
pm_runtime_get(&pci->dev);
platform_device_unregister(dwc->dwc3);
@@ -318,14 +342,15 @@ static int dwc3_pci_runtime_suspend(struct device *dev)
static int dwc3_pci_runtime_resume(struct device *dev)
{
struct dwc3_pci *dwc = dev_get_drvdata(dev);
- struct platform_device *dwc3 = dwc->dwc3;
int ret;

ret = dwc3_pci_dsm(dwc, PCI_INTEL_BXT_STATE_D0);
if (ret)
return ret;

- return pm_runtime_get(&dwc3->dev);
+ queue_work(pm_wq, &dwc->wakeup_work);
+
+ return 0;
}
#endif /* CONFIG_PM */

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-10 12:48:53

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi Manu,

On 27/09/17 14:19, Manu Gautam wrote:
> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
> resume. While this works fine for gadget mode but in host
> mode there is not re-initialization of host stack. Also, resetting
> bus as part of bus_suspend/resume is not correct which could affect
> (or disconnect) connected devices.
> Fix this by not reinitializing core on suspend/resume in host mode
> for HOST only and OTG/drd configurations.
>

All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
during dwc3_suspend() to have the lowest power state for our platforms.

After this patch, DWC3 controller and PHYs won't be turned off thus
preventing our platform from reaching low power levels.

So this is a regression for us (TI) in v4.15-rc.

Felipe, do you agree?

If yes I can send a patch which fixes the regression
and also makes USB host work after suspend/resume.

> Signed-off-by: Manu Gautam <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++-----------------------
> 1 file changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 03474d3..f75613f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>
> switch (dwc->dr_mode) {
> case USB_DR_MODE_PERIPHERAL:
> + dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE;
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>
> if (dwc->usb2_phy)
> @@ -942,6 +943,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
> }
> break;
> case USB_DR_MODE_HOST:
> + dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST;
> dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>
> if (dwc->usb2_phy)
> @@ -1293,21 +1295,19 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
> {
> unsigned long flags;
>
> - switch (dwc->dr_mode) {
> - case USB_DR_MODE_PERIPHERAL:
> - case USB_DR_MODE_OTG:
> + switch (dwc->current_dr_role) {
> + case DWC3_GCTL_PRTCAP_DEVICE:
> spin_lock_irqsave(&dwc->lock, flags);
> dwc3_gadget_suspend(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
> + dwc3_core_exit(dwc);
> break;
> - case USB_DR_MODE_HOST:
> + case DWC3_GCTL_PRTCAP_HOST:
> default:
> /* do nothing */
> break;
> }
>
> - dwc3_core_exit(dwc);
> -
> return 0;
> }
>
> @@ -1316,18 +1316,17 @@ static int dwc3_resume_common(struct dwc3 *dwc)
> unsigned long flags;
> int ret;
>
> - ret = dwc3_core_init(dwc);
> - if (ret)
> - return ret;
> + switch (dwc->current_dr_role) {
> + case DWC3_GCTL_PRTCAP_DEVICE:
> + ret = dwc3_core_init(dwc);
> + if (ret)
> + return ret;
>
> - switch (dwc->dr_mode) {
> - case USB_DR_MODE_PERIPHERAL:
> - case USB_DR_MODE_OTG:
> spin_lock_irqsave(&dwc->lock, flags);
> dwc3_gadget_resume(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
> - /* FALLTHROUGH */
> - case USB_DR_MODE_HOST:
> + break;
> + case DWC3_GCTL_PRTCAP_HOST:
> default:
> /* do nothing */
> break;
> @@ -1338,7 +1337,7 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>
> static int dwc3_runtime_checks(struct dwc3 *dwc)
> {
> - switch (dwc->dr_mode) {
> + switch (dwc->current_dr_role) {
> case USB_DR_MODE_PERIPHERAL:
> case USB_DR_MODE_OTG:
> if (dwc->connected)
> @@ -1381,12 +1380,11 @@ static int dwc3_runtime_resume(struct device *dev)
> if (ret)
> return ret;
>
> - switch (dwc->dr_mode) {
> - case USB_DR_MODE_PERIPHERAL:
> - case USB_DR_MODE_OTG:
> + switch (dwc->current_dr_role) {
> + case DWC3_GCTL_PRTCAP_DEVICE:
> dwc3_gadget_process_pending_events(dwc);
> break;
> - case USB_DR_MODE_HOST:
> + case DWC3_GCTL_PRTCAP_HOST:
> default:
> /* do nothing */
> break;
> @@ -1402,13 +1400,12 @@ static int dwc3_runtime_idle(struct device *dev)
> {
> struct dwc3 *dwc = dev_get_drvdata(dev);
>
> - switch (dwc->dr_mode) {
> - case USB_DR_MODE_PERIPHERAL:
> - case USB_DR_MODE_OTG:
> + switch (dwc->current_dr_role) {
> + case DWC3_GCTL_PRTCAP_DEVICE:
> if (dwc3_runtime_checks(dwc))
> return -EBUSY;
> break;
> - case USB_DR_MODE_HOST:
> + case DWC3_GCTL_PRTCAP_HOST:
> default:
> /* do nothing */
> break;
>

--
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2018-01-10 12:57:35

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi,

Roger Quadros <[email protected]> writes:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>>
>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.

A power consumption regression is still a regression. But it's super
late in the -rc cycle, I think we need to get this merged after 4.16-rc1
and make sure to Cc stable.

Should be more than enough time to review and test patches.

--
balbi

Attachments:

signature.asc (832.00 B)

2018-01-10 12:59:44

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

On 10/01/18 14:57, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <[email protected]> writes:
>> Hi Manu,
>>
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>
> A power consumption regression is still a regression. But it's super
> late in the -rc cycle, I think we need to get this merged after 4.16-rc1
> and make sure to Cc stable.
>
> Should be more than enough time to review and test patches.
>

Fine with me.

--
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2018-01-11 01:41:35

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi,

On 1/10/2018 6:18 PM, Roger Quadros wrote:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.
>

I think it will be better to separate runtime_suspend and pm_suspend handling for
host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
suspend-resume should be ok but doing that for runtime suspend-resume is not
correct.
Let me know if that sounds ok, I can provide a patch for same instead of
reverting this which affects runtime PM with dwc3 host.
Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to
dr_mode as HOST.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-11 08:15:14

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi,

Manu Gautam <[email protected]> writes:
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
>
> I think it will be better to separate runtime_suspend and pm_suspend handling for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.

it sure is. It's part of hibernation-while-disconnected programming sequence

> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.

nope, that would break platforms using hibernation

--
balbi

Attachments:

signature.asc (832.00 B)

2018-01-11 08:55:32

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi Felipe,

On 1/11/2018 1:44 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam <[email protected]> writes:
>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>> resume. While this works fine for gadget mode but in host
>>>> mode there is not re-initialization of host stack. Also, resetting
>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>> (or disconnect) connected devices.
>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>> for HOST only and OTG/drd configurations.
>>>>
>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>
>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>> preventing our platform from reaching low power levels.
>>>
>>> So this is a regression for us (TI) in v4.15-rc.
>>>
>>> Felipe, do you agree?
>>>
>>> If yes I can send a patch which fixes the regression
>>> and also makes USB host work after suspend/resume.
>>>
>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>> correct.
> it sure is. It's part of hibernation-while-disconnected programming sequence
>
>> Let me know if that sounds ok, I can provide a patch for same instead of
>> reverting this which affects runtime PM with dwc3 host.
> nope, that would break platforms using hibernation

Please don't mind me asking this if it is very basic, I am probably missing something there
We should be able to distinguish between runtime_pm vs system_suspend/hibernation
and then process accordingly.
In host mode runtime suspend/resume could happen very often with device connected,
and resetting h/w on every runtime_resume might not be desired. And PHYs drivers
can also support runtime_suspend which would be preferred instead of shutting down
phy.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2018-01-11 09:04:46

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi,

Manu Gautam <[email protected]> writes:
>>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>>> resume. While this works fine for gadget mode but in host
>>>>> mode there is not re-initialization of host stack. Also, resetting
>>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>>> (or disconnect) connected devices.
>>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>>> for HOST only and OTG/drd configurations.
>>>>>
>>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>>
>>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>>> preventing our platform from reaching low power levels.
>>>>
>>>> So this is a regression for us (TI) in v4.15-rc.
>>>>
>>>> Felipe, do you agree?
>>>>
>>>> If yes I can send a patch which fixes the regression
>>>> and also makes USB host work after suspend/resume.
>>>>
>>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>> correct.
>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>
>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>> reverting this which affects runtime PM with dwc3 host.
>> nope, that would break platforms using hibernation
>
> Please don't mind me asking this if it is very basic, I am probably
> missing something there
>
> We should be able to distinguish between runtime_pm vs
> system_suspend/hibernation and then process accordingly.

I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
Hibernation feature (open up your databook and have a read ;-)

> In host mode runtime suspend/resume could happen very often with
> device connected, and resetting h/w on every runtime_resume might not
> be desired. And PHYs drivers can also support runtime_suspend which
> would be preferred instead of shutting down phy.

We don't do anything when dwc3 is working as a host, we simply assume if
we reach dwc3.ko, xhci has done its part. Here's what our
suspend_common looks like:

static int dwc3_suspend_common(struct dwc3 *dwc)
{
unsigned long flags;

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
spin_lock_irqsave(&dwc->lock, flags);
dwc3_gadget_suspend(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
dwc3_core_exit(dwc);
break;
case DWC3_GCTL_PRTCAP_HOST:
default:
/* do nothing */
break;
}

return 0;
}

We're not resetting anything, not tearing down anything. No idea why
you're saying that in host mode we're breaking things apart. If you have
out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
mainline is at fault.

--
balbi

Attachments:

signature.asc (832.00 B)

2018-01-11 09:15:16

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Felipe,

On 11/01/18 11:04, Felipe Balbi wrote:
>
> Hi,
>
> Manu Gautam <[email protected]> writes:
>>>>> On 27/09/17 14:19, Manu Gautam wrote:
>>>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>>>>> resume. While this works fine for gadget mode but in host
>>>>>> mode there is not re-initialization of host stack. Also, resetting
>>>>>> bus as part of bus_suspend/resume is not correct which could affect
>>>>>> (or disconnect) connected devices.
>>>>>> Fix this by not reinitializing core on suspend/resume in host mode
>>>>>> for HOST only and OTG/drd configurations.
>>>>>>
>>>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>>>>> during dwc3_suspend() to have the lowest power state for our platforms.
>>>>>
>>>>> After this patch, DWC3 controller and PHYs won't be turned off thus
>>>>> preventing our platform from reaching low power levels.
>>>>>
>>>>> So this is a regression for us (TI) in v4.15-rc.
>>>>>
>>>>> Felipe, do you agree?
>>>>>
>>>>> If yes I can send a patch which fixes the regression
>>>>> and also makes USB host work after suspend/resume.
>>>>>
>>>> I think it will be better to separate runtime_suspend and pm_suspend handling for
>>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
>>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>>> correct.
>>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>>
>>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>>> reverting this which affects runtime PM with dwc3 host.
>>> nope, that would break platforms using hibernation
>>
>> Please don't mind me asking this if it is very basic, I am probably
>> missing something there
>>
>> We should be able to distinguish between runtime_pm vs
>> system_suspend/hibernation and then process accordingly.
>
> I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
> Hibernation feature (open up your databook and have a read ;-)
>
>> In host mode runtime suspend/resume could happen very often with
>> device connected, and resetting h/w on every runtime_resume might not
>> be desired. And PHYs drivers can also support runtime_suspend which
>> would be preferred instead of shutting down phy.
>
> We don't do anything when dwc3 is working as a host, we simply assume if
> we reach dwc3.ko, xhci has done its part. Here's what our
> suspend_common looks like:
>
> static int dwc3_suspend_common(struct dwc3 *dwc)
> {
> unsigned long flags;
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> spin_lock_irqsave(&dwc->lock, flags);
> dwc3_gadget_suspend(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> default:
> /* do nothing */
> break;
> }
>
> return 0;
> }
>
> We're not resetting anything, not tearing down anything. No idea why
> you're saying that in host mode we're breaking things apart. If you have
> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
> mainline is at fault.
>

This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume")
which is breaking low power for TI platforms in Host mode.

If we revert that commit we will be doing dwc3_core_exit() for host mode as well. Which is what we want for system suspend
but probably not for runtime suspend in host case.

This is why Manu wants to differentiate runtime vs system suspend.

--
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2018-01-11 09:23:21

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi,

Roger Quadros <[email protected]> writes:
>>> In host mode runtime suspend/resume could happen very often with
>>> device connected, and resetting h/w on every runtime_resume might not
>>> be desired. And PHYs drivers can also support runtime_suspend which
>>> would be preferred instead of shutting down phy.
>>
>> We don't do anything when dwc3 is working as a host, we simply assume if
>> we reach dwc3.ko, xhci has done its part. Here's what our
>> suspend_common looks like:
>>
>> static int dwc3_suspend_common(struct dwc3 *dwc)
>> {
>> unsigned long flags;
>>
>> switch (dwc->current_dr_role) {
>> case DWC3_GCTL_PRTCAP_DEVICE:
>> spin_lock_irqsave(&dwc->lock, flags);
>> dwc3_gadget_suspend(dwc);
>> spin_unlock_irqrestore(&dwc->lock, flags);
>> dwc3_core_exit(dwc);
>> break;
>> case DWC3_GCTL_PRTCAP_HOST:
>> default:
>> /* do nothing */
>> break;
>> }
>>
>> return 0;
>> }
>>
>> We're not resetting anything, not tearing down anything. No idea why
>> you're saying that in host mode we're breaking things apart. If you have
>> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
>> mainline is at fault.
>>
>
> This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't
> reinitialize core during host bus-suspend/resume") which is breaking
> low power for TI platforms in Host mode.
>
> If we revert that commit we will be doing dwc3_core_exit() for host
> mode as well. Which is what we want for system suspend but probably
> not for runtime suspend in host case.
>
> This is why Manu wants to differentiate runtime vs system suspend.

We already differentiate them. Maybe the only thing we need is to *not*
call core_exit() in host-mode during runtime_suspend, but call it if
mode is device or during system sleep.

--
balbi

Attachments:

signature.asc (832.00 B)

2018-01-15 15:40:23

[permalink] [raw]

Subject: Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

Hi Manu,

On 11/01/18 03:41, Manu Gautam wrote:
> Hi,
>
>
> On 1/10/2018 6:18 PM, Roger Quadros wrote:
>> Hi Manu,
>>
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
>
> I think it will be better to separate runtime_suspend and pm_suspend handling for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.
> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.
> Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to
> dr_mode as HOST.
>
>

Are you going to provide a patch for this any time soon?

FYI, suspend/resume is broken on DRA7x with Dual-role while in host mode.

--
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2018-01-16 06:36:35