lkml.org 
[lkml]   [2023]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend
From


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
>

\
 
 \ /
  Last update: 2023-10-26 00:21    [W:0.471 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site