Re: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend
From: Elson Serrao
Date: Wed Oct 25 2023 - 18:21:29 EST
On 10/25/2023 1:02 AM, Roger Quadros wrote:
On 24/10/2023 21:41, Elson Serrao wrote:
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@xxxxxxxxxxx>
---
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.
Of course. I missed the break :)
+ 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.
Before you do that let's first decide how we want the gadget driver to behave
in system_suspend case.
Current behavior is to Disconnect from the Host.
Earlier I was thinking on the lines that we prevent system suspend if
we are not already in USB suspend. But I'm not sure if that is the right
thing to do anymore. Mainly because, system suspend is a result of user
request and it may not be nice to not to meet his/her request.
Agree. Irrespective of whether USB is suspended or not it is better to
honor the system suspend request from user.
Maybe best to leave this policy handling to user space?
i.e. if user wants USB gadget operation to be alive, he will not issue
system suspend?
Sure. So below two cases
Case1: User doesn't care if gadget operation is alive and triggers
system suspend irrespective of USB suspend. Like you mentioned, current
behavior already takes care of this and initiates a DISCONNECT
Case2: User wants gadget to stay alive and hence can trigger system
suspend only when USB is suspended (there are already user space hooks
that read cdev->suspended bit to tell whether USB is suspended or not
for user to decide). Attempts to request system suspend when USB is not
suspended, would result in a DISCONNECT.
For supporting Case2 from gadget driver point of view, we need to extend
this series by having relevant checks in suspend_common()
Also, is it better to provide separate flags to control the gadget
driver behavior for runtime suspend Vs system suspend when USB is
suspended ? For example, what if we want to enable bus suspend handling
for runtime suspend only and not for system suspend (Case1).
Thanks
Elson
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