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
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(+)
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
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 <[email protected]>
> ---
> 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
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(-)
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
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 <[email protected]>
>> ---
>> 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
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
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