Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 16:22:12 -0700 | Subject | Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend | From | Elson Serrao <> |
| |
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@quicinc.com> >>> --- >>> 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); } }
| ![\](/images/icornerr.gif) |