Allow DWC3 runtime suspend if UDC is unbinded

The DWC3 runtime suspend routine checks for the USB connected parameter to determine if the controller can enter into a low power state. The connected state is only set to false after receiving a disconnect event. However, in the case of a device initiated disconnect (i.e. UDC unbind), the controller is halted and a disconnect event is never generated. Set the connected flag to false if issuing a device initiated disconnect to allow the controller to be suspended.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/dwc3/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

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

index 5d879b7606d5..6364429b2122 100644

drivers/usb/dwc3/gadget.c +2 -0

+
 	is_on = !!is_on;
 
 	/*

@@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 			dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
 						dwc->ev_buf->length;
 		}
+		dwc->connected = false;
 	}
 
 	ret = dwc3_gadget_run_stop(dwc, is_on, false);

Signature · 2 lines

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

Quoted 18 lines · Wesley Cheng

The DWC3 runtime suspend routine checks for the USB connected parameter to determine if the controller can enter into a low power state. The connected state is only set to false after receiving a disconnect event. However, in the case of a device initiated disconnect (i.e. UDC unbind), the controller is halted and a disconnect event is never generated. Set the connected flag to false if issuing a device initiated disconnect to allow the controller to be suspended.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/dwc3/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

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

index 5d879b7606d5..6364429b2122 100644

drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2001

Quoted 7 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	unsigned long		flags;
 	int			ret;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		pm_request_resume(dwc->dev);
+		return 0;
+	}

Isn't this racy? What happens if the controller was active but a runtime suspend occurs right here?

Alan Stern

Quoted 15 lines

drivers/usb/dwc3/gadget.c +2 -0

+
 	is_on = !!is_on;
 
 	/*

@@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 			dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
 						dwc->ev_buf->length;
 		}
+		dwc->connected = false;
 	}
 
 	ret = dwc3_gadget_run_stop(dwc, is_on, false);

Signature · 2 lines

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

Quoted 14 lines · Alan Stern

drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2001

Quoted 10 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	unsigned long		flags;
 	int			ret;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		pm_request_resume(dwc->dev);
+		return 0;
+	}

Isn't this racy? What happens if the controller was active but a runtime suspend occurs right here?

Quoted 2 lines

Alan Stern

Hi Alan,

Ah, yes you're right. I was hoping that the PM runtime layer would be utilizing the spinlock when reading out the runtime status, but even then, we wouldn't be able to catch intermediate states with this API (i.e. RPM_RESUMING or RPM_SUSPENDING)

Tried a few different approaches, and came up with something like the following:

static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) {
...

	ret = pm_runtime_get_sync(dwc->dev);
	if (!ret) {
		pm_runtime_put(dwc->dev);
		return 0;
	}
	...
	pm_runtime_put(dwc->dev);

	return 0;
}

I think this should be good to address your concern. The only way we would be able to ensure that the runtime PM state doesn't enter idle/suspend is if we increment the usage count for the duration we're accessing the DWC3 registers. With the synchronous PM runtime resume call, we can also ensure that no pending runtime suspends are executing in parallel while running this code.

The check for the zero return value would be for avoiding running the DWC3 run stop sequence for the case where we executed the runtime PM resume, as the DWC3 runtime PM resume routine will set the run stop bit in there.

Quoted 44 lines · Wesley Cheng

Quoted 14 lines · Alan Stern

drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2001

Quoted 10 lines

drivers/usb/dwc3/gadget.c +4 -0

@@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	unsigned long		flags;
 	int			ret;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		pm_request_resume(dwc->dev);
+		return 0;
+	}

Isn't this racy? What happens if the controller was active but a runtime suspend occurs right here?

Quoted 2 lines

Alan Stern

Hi Alan,

Ah, yes you're right. I was hoping that the PM runtime layer would be utilizing the spinlock when reading out the runtime status, but even then, we wouldn't be able to catch intermediate states with this API (i.e. RPM_RESUMING or RPM_SUSPENDING)

Tried a few different approaches, and came up with something like the following:

static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) {
...

	ret = pm_runtime_get_sync(dwc->dev);
	if (!ret) {
		pm_runtime_put(dwc->dev);
		return 0;
	}
	...
	pm_runtime_put(dwc->dev);

	return 0;
}

Quoted 6 lines

I think this should be good to address your concern. The only way we would be able to ensure that the runtime PM state doesn't enter idle/suspend is if we increment the usage count for the duration we're accessing the DWC3 registers. With the synchronous PM runtime resume call, we can also ensure that no pending runtime suspends are executing in parallel while running this code.

That's correct.

Quoted 4 lines

The check for the zero return value would be for avoiding running the DWC3 run stop sequence for the case where we executed the runtime PM resume, as the DWC3 runtime PM resume routine will set the run stop bit in there.

If you need to add an explanation of this subtle point in your email message, then you should add a similar explanation as a comment in the code. And don't forget to check for ret < 0 (i.e., a resume error).

The USB gadget/UDC driver can restrict the DWC3 controller speed using dwc3_gadget_set_speed(). Store this setting into a variable, in order for this setting to persist across controller resets due to runtime PM.

Signed-off-by Wesley Cheng <[email protected]>

---
 drivers/usb/dwc3/core.h   |   1 +
 drivers/usb/dwc3/gadget.c | 108 ++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 51 deletions(-)

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

index 2f04b3e42bf1..390d3deef0ba 100644
@@ -1119,6 +1119,7 @@ struct dwc3 {
 	u32			nr_scratch;
 	u32			u1u2;
 	u32			maximum_speed;
+	u32			gadget_max_speed;
 
 	u32			ip;
 

drivers/usb/dwc3/gadget.c +57 -51

index 6364429b2122..1a93b92a5e6f 100644
@@ -1941,6 +1941,61 @@ static void dwc3_stop_active_transfers(struct dwc3 *dwc)
 	}
 }
 
+static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
+{
+	u32			reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
+	reg &= ~(DWC3_DCFG_SPEED_MASK);
+
+	/*
+	 * WORKAROUND: DWC3 revision < 2.20a have an issue
+	 * which would cause metastability state on Run/Stop
+	 * bit if we try to force the IP to USB2-only mode.
+	 *
+	 * Because of that, we cannot configure the IP to any
+	 * speed other than the SuperSpeed
+	 *
+	 * Refers to:
+	 *
+	 * STAR#9000525659: Clock Domain Crossing on DCTL in
+	 * USB 2.0 Mode
+	 */
+	if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
+	    !dwc->dis_metastability_quirk) {
+		reg |= DWC3_DCFG_SUPERSPEED;
+	} else {
+		switch (dwc->gadget_max_speed) {
+		case USB_SPEED_LOW:
+			reg |= DWC3_DCFG_LOWSPEED;
+			break;
+		case USB_SPEED_FULL:
+			reg |= DWC3_DCFG_FULLSPEED;
+			break;
+		case USB_SPEED_HIGH:
+			reg |= DWC3_DCFG_HIGHSPEED;
+			break;
+		case USB_SPEED_SUPER:
+			reg |= DWC3_DCFG_SUPERSPEED;
+			break;
+		case USB_SPEED_SUPER_PLUS:
+			if (DWC3_IP_IS(DWC3))
+				reg |= DWC3_DCFG_SUPERSPEED;
+			else
+				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
+			break;
+		default:
+			dev_err(dwc->dev, "invalid speed (%d)\n", dwc->gadget_max_speed);
+
+			if (DWC3_IP_IS(DWC3))
+				reg |= DWC3_DCFG_SUPERSPEED;
+			else
+				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
+		}
+	}
+	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+}
+
 static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 {
 	u32			reg;

@@ -1963,6 +2018,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 		if (dwc->has_hibernation)
 			reg |= DWC3_DCTL_KEEP_CONNECT;
 
+		__dwc3_gadget_set_speed(dwc);
 		dwc->pullups_connected = true;
 	} else {
 		reg &= ~DWC3_DCTL_RUN_STOP;

@@ -2318,59 +2374,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
-	u32			reg;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
-	reg &= ~(DWC3_DCFG_SPEED_MASK);
-
-	/*
-	 * WORKAROUND: DWC3 revision < 2.20a have an issue
-	 * which would cause metastability state on Run/Stop
-	 * bit if we try to force the IP to USB2-only mode.
-	 *
-	 * Because of that, we cannot configure the IP to any
-	 * speed other than the SuperSpeed
-	 *
-	 * Refers to:
-	 *
-	 * STAR#9000525659: Clock Domain Crossing on DCTL in
-	 * USB 2.0 Mode
-	 */
-	if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
-	    !dwc->dis_metastability_quirk) {
-		reg |= DWC3_DCFG_SUPERSPEED;
-	} else {
-		switch (speed) {
-		case USB_SPEED_LOW:
-			reg |= DWC3_DCFG_LOWSPEED;
-			break;
-		case USB_SPEED_FULL:
-			reg |= DWC3_DCFG_FULLSPEED;
-			break;
-		case USB_SPEED_HIGH:
-			reg |= DWC3_DCFG_HIGHSPEED;
-			break;
-		case USB_SPEED_SUPER:
-			reg |= DWC3_DCFG_SUPERSPEED;
-			break;
-		case USB_SPEED_SUPER_PLUS:
-			if (DWC3_IP_IS(DWC3))
-				reg |= DWC3_DCFG_SUPERSPEED;
-			else
-				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
-			break;
-		default:
-			dev_err(dwc->dev, "invalid speed (%d)\n", speed);
-
-			if (DWC3_IP_IS(DWC3))
-				reg |= DWC3_DCFG_SUPERSPEED;
-			else
-				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
-		}
-	}
-	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
-
+	dwc->gadget_max_speed = speed;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 

Hi,

Quoted 3 lines · Wesley Cheng

The USB gadget/UDC driver can restrict the DWC3 controller speed using dwc3_gadget_set_speed(). Store this setting into a variable, in order for this setting to persist across controller resets due to runtime PM.

Why do we need to do this? DCFG should persist unless we overwrite it. The current PM shouldn't update the current speed setting.

BR,
Thinh

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 3 lines · Wesley Cheng

The USB gadget/UDC driver can restrict the DWC3 controller speed using dwc3_gadget_set_speed(). Store this setting into a variable, in order for this setting to persist across controller resets due to runtime PM.

Why do we need to do this? DCFG should persist unless we overwrite it. The current PM shouldn't update the current speed setting.

BR,
Thinh

Hi Thinh,

During runtime PM suspend, the dwc3_suspend_common() will call dwc3_core_exit(). On some platforms they register the DWC3 reset control to the DWC3 core driver (otherwise could be handled in the DWC3 glue drivers), which will be asserted here:

static void dwc3_core_exit(struct dwc3 *dwc)
{
...

	reset_control_assert(dwc->reset);

From the SNPS databook (Table 2-2 Resets for Registers) it mentions that assertion of the reset signal will reset the DCFG register.

In addition to the above, with the change to allow runtime PM suspend during UDC unbind, we need a way to avoid writing to the DCFG during the UDC bind path. (if we entered suspend before re-binding the UDC) If we add an early exit based on the PM state (in dwc3_gadget_set_udc_speed()), then we basically ignore the max speed request from the UDC/gadget layer.

Since it looks like the DWC3 gadget driver doesn't like using synchronous PM runtime resumes, by going this route, we can allow the async runtime resume handler to do everything, such as writing the speed config and re-enabling the controller.

Thanks

Regards,
Wesley Cheng

Quoted 20 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 3 lines · Wesley Cheng

The USB gadget/UDC driver can restrict the DWC3 controller speed using dwc3_gadget_set_speed(). Store this setting into a variable, in order for this setting to persist across controller resets due to runtime PM.

Why do we need to do this? DCFG should persist unless we overwrite it. The current PM shouldn't update the current speed setting.

BR,
Thinh

Hi Thinh,

During runtime PM suspend, the dwc3_suspend_common() will call dwc3_core_exit(). On some platforms they register the DWC3 reset control to the DWC3 core driver (otherwise could be handled in the DWC3 glue drivers), which will be asserted here:

Quoted 7 lines

static void dwc3_core_exit(struct dwc3 *dwc)
{
...

	reset_control_assert(dwc->reset);

From the SNPS databook (Table 2-2 Resets for Registers) it mentions that assertion of the reset signal will reset the DCFG register.

I see. There's a hard reset on some platforms.

Quoted 6 lines

In addition to the above, with the change to allow runtime PM suspend during UDC unbind, we need a way to avoid writing to the DCFG during the UDC bind path. (if we entered suspend before re-binding the UDC) If we add an early exit based on the PM state (in dwc3_gadget_set_udc_speed()), then we basically ignore the max speed request from the UDC/gadget layer.

Then shouldn't we restore the speed setting when dwc3_gadget_resume() instead of in dwc3_gadget_run_stop()?

Quoted 10 lines

Since it looks like the DWC3 gadget driver doesn't like using synchronous PM runtime resumes, by going this route, we can allow the async runtime resume handler to do everything, such as writing the speed config and re-enabling the controller.

Thanks

Regards,
Wesley Cheng

Thanks,
Thinh