[PATCH 0/2] Allow DWC3 runtime suspend if UDC is unbinded

[PATCH 0/2] Allow DWC3 runtime suspend if UDC is unbinded

[Wesley Cheng]

During the following scenario, the DWC3 runtime suspend routine is blocked as
the connected flag is still true:

1.  Enumerate device w/ host.
2.  Gadget is unbinded
	- echo "" > /sys/kernel/config/usb_gadget/g1/UDC
3.  Disconnect the USB cable (VBUS low)
4.  No dwc3_gadget_disconnect_interrupt() seen (since controller is
   halted from step#1)
5.  Runtime PM autosuspend fails due to "dwc->connected" being true
(cleared in dwc3_gadget_disconnect_interrupt())
6.  Gadget binded
	- echo udc_name > /sys/kernel/config/usb_gadget/g1/UDC
7.  No runtime suspend until cable is plugged in and out

Technically, for device initiated disconnects, there is no active session/link
with the host, so the DWC3 controller should be allowed to go into a low power
state.  Also, we need to now consider when re-binding the UDC,
dwc3_gadget_set_speed() is executed before dwc3_gadget_pullup(), so if the DWC3
controller is suspended/disabled, while accessing the DCFG, that could result in
bus timeouts, etc...  Change the dwc3_gadget_set_speed() to save the speed
being requested, and program it during dwc3_gadget_run_stop(), which is executed
during PM runtime resume.  If not, previous setting will be overridden as we
execute a DWC3 controller reset during PM runtime resume. 

Wesley Cheng (2):
  usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
  usb: dwc3: gadget: Preserve UDC max speed setting

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

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

[PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

[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 <wcheng@codeaurora.org>
---
 drivers/usb/dwc3/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d879b7606d5..6364429b2122 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -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;
+	}
+
 	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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

[Alan Stern]

On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote:
> 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 <wcheng@codeaurora.org>
> ---
>  drivers/usb/dwc3/gadget.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5d879b7606d5..6364429b2122 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -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

> +
>  	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);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

[Wesley Cheng]

On 10/28/2020 6:07 PM, Alan Stern wrote:
> On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote:
>> 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 <wcheng@codeaurora.org>
>> ---
>>  drivers/usb/dwc3/gadget.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5d879b7606d5..6364429b2122 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -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
> 

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.

Thanks

Regards,
Wesley Cheng
>> +
>>  	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);
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project

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

Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

[Alan Stern]

On Tue, Nov 03, 2020 at 11:02:25AM -0800, Wesley Cheng wrote:
> 
> 
> On 10/28/2020 6:07 PM, Alan Stern wrote:
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -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
> > 
> 
> 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.

That's correct.

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

Alan Stern

Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

[Wesley Cheng]

On 11/3/2020 12:07 PM, Alan Stern wrote:
> On Tue, Nov 03, 2020 at 11:02:25AM -0800, Wesley Cheng wrote:
>>
>>
>> On 10/28/2020 6:07 PM, Alan Stern wrote:
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -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
>>>
>>
>> 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.
> 
> That's correct.
> 
>> 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).
> 

Hi Alan,

Got it, will do.  Yes, I'll include the error conditions as well in the
actual change.  Thanks again!

Thanks

Regards,
Wesley Cheng

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

[PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting

[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.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/dwc3/core.h   |   1 +
 drivers/usb/dwc3/gadget.c | 108 ++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2f04b3e42bf1..390d3deef0ba 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1119,6 +1119,7 @@ struct dwc3 {
 	u32			nr_scratch;
 	u32			u1u2;
 	u32			maximum_speed;
+	u32			gadget_max_speed;
 
 	u32			ip;
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 6364429b2122..1a93b92a5e6f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -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);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting

[Thinh Nguyen]

Hi,

Wesley Cheng wrote:
> 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

> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
>  drivers/usb/dwc3/core.h   |   1 +
>  drivers/usb/dwc3/gadget.c | 108 ++++++++++++++++++++------------------
>  2 files changed, 58 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f04b3e42bf1..390d3deef0ba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1119,6 +1119,7 @@ struct dwc3 {
>  	u32			nr_scratch;
>  	u32			u1u2;
>  	u32			maximum_speed;
> +	u32			gadget_max_speed;
>  
>  	u32			ip;
>  
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6364429b2122..1a93b92a5e6f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -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);
>  }
>  

Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting

[Wesley Cheng]

On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
> Hi,
> 
> Wesley Cheng wrote:
>> 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
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting

[Thinh Nguyen]

Wesley Cheng wrote:
>
> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> 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.

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

>
> 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()?

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

Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting

[Thinh Nguyen]

Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> Wesley Cheng wrote:
>>>> 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.
> I see. There's a hard reset on some platforms.
>
>> 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()?

Actually, ignore this comment. This is fine, and it may save some
register read/write operations. I was thinking of save/restore from
suspend and resume similar to hibernation.

Thanks,
Thinh

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