Messages in this thread | | | Date | Wed, 25 Oct 2023 15:21:15 -0700 | Subject | Re: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend | From | Elson Serrao <> |
| |
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@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. > > 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 >
| |