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

[Roger Quadros]

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.
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?


> 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

-- 
cheers,
-roger