Messages in this thread | | | Date | Tue, 24 Oct 2023 11:41:51 -0700 | Subject | Re: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend | From | Elson Serrao <> |
| |
On 10/24/2023 3:14 AM, Roger Quadros wrote: > Hi Elson, > > On 14/08/2023 21:50, Elson Roy Serrao wrote: >> The current implementation blocks the runtime pm operations when cable >> is connected. This would block dwc3 to enter a low power state 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 | 28 ++++++++++++++++++++++++++-- >> drivers/usb/dwc3/core.h | 3 +++ >> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- >> 3 files changed, 54 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 9c6bf054f15d..9bfd9bb18caf 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >> dwc->dis_split_quirk = device_property_read_bool(dev, >> "snps,dis-split-quirk"); >> >> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, >> + "snps,runtime-suspend-on-usb-suspend"); >> + >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >> dwc->tx_de_emphasis = tx_de_emphasis; >> >> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> + /* runtime resume on bus resume scenario */ >> + if (PMSG_IS_AUTO(msg) && dwc->connected) >> + break; >> ret = dwc3_core_init_for_resume(dwc); >> if (ret) >> return ret; >> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) >> { >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> - if (dwc->connected) >> + if (dwc->connected) { >> + /* bus suspend scenario */ >> + if (dwc->runtime_suspend_on_usb_suspend && >> + dwc->suspended) > > If dwc is already suspended why do we return -EBUSY? > Should this be !dwc->suspended? >
Hi Roger
Thank you for reviewing. If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0.
>> + break; >> return -EBUSY; >> + } >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> default: >> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) >> struct dwc3 *dwc = dev_get_drvdata(dev); >> int ret; >> >> - if (dwc3_runtime_checks(dwc)) >> + ret = dwc3_runtime_checks(dwc); >> + if (ret) >> return -EBUSY; >> >> + switch (dwc->current_dr_role) { >> + case DWC3_GCTL_PRTCAP_DEVICE: >> + /* bus suspend case */ >> + if (!ret && dwc->connected) > > No need to check !ret again as it will never happen because > we are returning -EBUSY earlier if (ret); > Thanks for this catch. I will remove !ret check in v5.
>> + return 0; >> + break; >> + case DWC3_GCTL_PRTCAP_HOST: >> + default: >> + /* do nothing */ >> + break; >> + } >> + > > While this takes care of runtime suspend case, what about system_suspend? > Should this check be moved to dwc3_suspend_common() instead? >
Sure I can move these checks to dwc3_suspend_common to make it generic. Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback.
I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback . I will accommodate above feedback as well and upload v5.
Thanks Elson >> ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND); >> if (ret) >> return ret; >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index a69ac67d89fe..f2f788a6b4b5 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -1124,6 +1124,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. >> + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed >> + * during bus suspend scenario. >> */ >> struct dwc3 { >> struct work_struct drd_work; >> @@ -1340,6 +1342,7 @@ struct dwc3 { >> int last_fifo_depth; >> int num_ep_resized; >> struct dentry *debug_root; >> + bool runtime_suspend_on_usb_suspend; >> }; >> >> #define INCRX_BURST_MODE 0 >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 5fd067151fbf..978ce0e91164 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, >
| |