Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend
From: Elson Serrao
Date: Tue Jul 11 2023 - 19:22:32 EST
On 7/11/2023 3:51 PM, Elson Serrao wrote:
On 7/11/2023 3:07 PM, Thinh Nguyen wrote:
On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
The current implementation blocks the runtime pm operations when cable
is connected. This would block platforms from entering system wide
suspend
during bus suspend scenario. Modify the runtime pm ops to handle bus
suspend case for such platforms where the controller low power mode
entry/exit is handled by the glue driver. This enablement is controlled
through a dt property and platforms capable of detecting bus resume can
benefit from this feature. Also modify the remote wakeup operations to
trigger runtime resume before sending wakeup signal.
Signed-off-by: Elson Roy Serrao <quic_eserrao@xxxxxxxxxxx>
---
drivers/usb/dwc3/core.c | 26 ++++++++++++++++++++++---
drivers/usb/dwc3/core.h | 3 +++
drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
3 files changed, 59 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f6689b731718..898c0f68e190 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
dwc->dis_split_quirk = device_property_read_bool(dev,
"snps,dis-split-quirk");
+ dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
+ "snps,allow-rtsusp-on-u3");
+
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
dwc->tx_de_emphasis = tx_de_emphasis;
@@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3
*dwc, pm_message_t msg)
{
unsigned long flags;
u32 reg;
+ int link_state;
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
if (pm_runtime_suspended(dwc->dev))
break;
+
+ if (dwc->connected) {
+ link_state = dwc3_gadget_get_link_state(dwc);
+ /* bus suspend case */
+ if (dwc->allow_rtsusp_on_u3 &&
+ link_state == DWC3_LINK_STATE_U3)
+ break;
+ return -EBUSY;
+ }
dwc3_gadget_suspend(dwc);
synchronize_irq(dwc->irq_gadget);
dwc3_core_exit(dwc);
@@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc,
pm_message_t msg)
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
+ /* bus resume case */
+ if (dwc->connected)
+ break;
ret = dwc3_core_init_for_resume(dwc);
if (ret)
return ret;
@@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device
*dev)
struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
- if (dwc3_runtime_checks(dwc))
- return -EBUSY;
-
ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
if (ret)
return ret;
@@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device
*dev)
static int dwc3_runtime_idle(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
+ int link_state;
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
+ link_state = dwc3_gadget_get_link_state(dwc);
+ /* for bus suspend case return success */
+ if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
+ link_state == DWC3_LINK_STATE_U3)
+ goto autosuspend;
if (dwc3_runtime_checks(dwc))
return -EBUSY;
break;
@@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
break;
}
+autosuspend:
pm_runtime_mark_last_busy(dev);
pm_runtime_autosuspend(dev);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8b1295e4dcdd..33b2ccbbd963 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
* @num_ep_resized: carries the current number endpoints which have
had its tx
* fifo resized.
* @debug_root: root debugfs directory for this device to put its
files in.
+ * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed
during bus
+ * suspend scenario.
*/
struct dwc3 {
struct work_struct drd_work;
@@ -1343,6 +1345,7 @@ struct dwc3 {
int last_fifo_depth;
int num_ep_resized;
struct dentry *debug_root;
+ bool allow_rtsusp_on_u3;
};
#define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5fd067151fbf..0797cffa2d48 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct
usb_gadget *g)
return -EINVAL;
}
- spin_lock_irqsave(&dwc->lock, flags);
if (!dwc->gadget->wakeup_armed) {
dev_err(dwc->dev, "not armed for remote wakeup\n");
- spin_unlock_irqrestore(&dwc->lock, flags);
return -EINVAL;
}
- ret = __dwc3_gadget_wakeup(dwc, true);
+ ret = pm_runtime_resume_and_get(dwc->dev);
+ if (ret < 0) {
+ pm_runtime_set_suspended(dwc->dev);
+ return ret;
+ }
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ ret = __dwc3_gadget_wakeup(dwc, true);
spin_unlock_irqrestore(&dwc->lock, flags);
+ pm_runtime_put_noidle(dwc->dev);
return ret;
}
@@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct
usb_gadget *g, int intf_id)
return -EINVAL;
}
+ ret = pm_runtime_resume_and_get(dwc->dev);
+ if (ret < 0) {
+ pm_runtime_set_suspended(dwc->dev);
+ return ret;
+ }
+
spin_lock_irqsave(&dwc->lock, flags);
/*
* If the link is in U3, signal for remote wakeup and wait for the
@@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct
usb_gadget *g, int intf_id)
ret = __dwc3_gadget_wakeup(dwc, false);
if (ret) {
spin_unlock_irqrestore(&dwc->lock, flags);
+ pm_runtime_put_noidle(dwc->dev);
return -EINVAL;
}
dwc3_resume_gadget(dwc);
@@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct
usb_gadget *g, int intf_id)
dev_err(dwc->dev, "function remote wakeup failed,
ret:%d\n", ret);
spin_unlock_irqrestore(&dwc->lock, flags);
+ pm_runtime_put_noidle(dwc->dev);
return ret;
}
@@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct
usb_gadget *g, int is_on)
/*
* Avoid issuing a runtime resume if the device is already in the
* suspended state during gadget disconnect. DWC3 gadget was
already
- * halted/stopped during runtime suspend.
+ * halted/stopped during runtime suspend except for bus suspend
case
+ * where we would have skipped the controller halt.
*/
if (!is_on) {
pm_runtime_barrier(dwc->dev);
- if (pm_runtime_suspended(dwc->dev))
+ if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
return 0;
}
/*
* Check the return value for successful resume, or error. For a
* successful resume, the DWC3 runtime PM resume routine will
handle
- * the run stop sequence, so avoid duplicate operations here.
+ * the run stop sequence except for bus resume case, so avoid
+ * duplicate operations here.
*/
ret = pm_runtime_get_sync(dwc->dev);
- if (!ret || ret < 0) {
+ if ((!ret && !dwc->connected) || ret < 0) {
pm_runtime_put(dwc->dev);
if (ret < 0)
pm_runtime_set_suspended(dwc->dev);
@@ -4331,6 +4347,8 @@ static void
dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
}
dwc->link_state = next;
+ pm_runtime_mark_last_busy(dwc->dev);
+ pm_request_autosuspend(dwc->dev);
}
static void dwc3_gadget_interrupt(struct dwc3 *dwc,
@@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct
dwc3 *dwc)
{
if (dwc->pending_events) {
dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
+ pm_runtime_put(dwc->dev);
dwc->pending_events = false;
enable_irq(dwc->irq_gadget);
+ /*
+ * We have only stored the pending events as part
+ * of dwc3_interrupt() above, but those events are
+ * not yet handled. So explicitly invoke the
+ * interrupt handler for handling those events.
+ */
+ dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
Why do we have to do this? If there are events, the threaded interrupt
should be woken up.
dwc3_thread_interrupt will be woken up only if dwc3_interrupt() handler
is invoked by the interrupt framework when the return value of
IRQ_WAKE_THREAD is handled. But while processing the pending events the
interrupt framework is not involved. We explicitly invoke the
dwc3_interrupt() above within the dwc3 driver. So the
dwc3_thread_interrupt() has to be explicitly invoked as well for
processing those pending events.
Perhaps we can make it more optimal by checking the return value like below?
void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
{
irqreturn_t ret;
if (dwc->pending_events) {
ret = dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
pm_runtime_put(dwc->dev);
dwc->pending_events = false;
enable_irq(dwc->irq_gadget);
/*
* We have only stored the pending events as part
* of dwc3_interrupt() above, but those events are
* not yet handled. So explicitly invoke the
* interrupt handler for handling those events.
*/
if (ret == IRQ_WAKE_THREAD)
dwc3_thread_interrupt(dwc->irq_gadget,
dwc->ev_buf);
}
}