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

[Roger Quadros]

On 26/10/2023 01:21, Elson Serrao wrote:
> 
> 
> 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).

But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
Why do we need separate flags for?

-- 
cheers,
-roger