usb: dwc3: Don't reinitialize core during host bus-suspend/resume

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.

Signed-off-by Manu Gautam <[email protected]>

---
 drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

drivers/usb/dwc3/core.c +20 -23

index 03474d3..f75613f 100644
@@ -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;

Hi Felipe,

Let me know if patches in this series look fine to you.

Quoted 19 lines

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.

Signed-off-by Manu Gautam <[email protected]>

---
 drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

drivers/usb/dwc3/core.c +0 -0

index 03474d3..f75613f 100644
@@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 

drivers/usb/dwc3/core.c static int dwc3_core_init_mode(struct dwc3 *dwc) · line 930

Quoted 3 lines

drivers/usb/dwc3/core.c +1 -0

 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
+		dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE;

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.

Signed-off-by Manu Gautam <[email protected]>

---
 drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

drivers/usb/dwc3/core.c +20 -23

index 03474d3..f75613f 100644
@@ -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;

Hi Manu,

Quoted 8 lines · Manu Gautam

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.

Hi,

Quoted 22 lines · Roger Quadros

Hi Manu,

Quoted 8 lines · Manu Gautam

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?

Quoted 2 lines

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.

Hi Manu,

Quoted 8 lines · Manu Gautam

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.

Quoted 19 lines · Manu Gautam

Quoted 8 lines

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?

Quoted 3 lines

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.

Hi,

Quoted 19 lines · Manu Gautam

Quoted 19 lines · Manu Gautam

Quoted 8 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 30 lines

Quoted 19 lines

Quoted 19 lines · Manu Gautam

Quoted 8 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 1 line

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.

Hi,

Quoted 30 lines · Manu Gautam

Quoted 30 lines

Quoted 19 lines

Quoted 19 lines · Manu Gautam

Quoted 8 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 7 lines

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 29 lines

Quoted 4 lines

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

Quoted 5 lines

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.

Hi,

Quoted 17 lines · Roger Quadros

Hi Manu,

Quoted 8 lines · Manu Gautam

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.

Quoted 7 lines

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.

Hi Manu,

Quoted 10 lines

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.