Re: [PATCH v4 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao <quic_eserrao@quicinc.com>]

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
>