Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend
From: Roger Quadros
Date: Thu Jul 13 2023 - 08:56:27 EST
On 13/07/2023 01:57, Elson Serrao wrote:
>
>
> On 7/12/2023 6:46 AM, Roger Quadros wrote:
>>
>>
>> On 11/07/2023 20:43, Elson Roy Serrao wrote:
>>> The current implementation blocks the runtime pm operations when cable
>>> is connected. This would block platforms from entering system wide suspend
>>> 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 | 26 ++++++++++++++++++++++---
>>> drivers/usb/dwc3/core.h | 3 +++
>>> drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>> 3 files changed, 59 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index f6689b731718..898c0f68e190 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>> dwc->dis_split_quirk = device_property_read_bool(dev,
>>> "snps,dis-split-quirk");
>>> + dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>>> + "snps,allow-rtsusp-on-u3");
>>> +
>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>> dwc->tx_de_emphasis = tx_de_emphasis;
>>> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>> {
>>> unsigned long flags;
>>> u32 reg;
>>> + int link_state;
>>> switch (dwc->current_dr_role) {
>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>> if (pm_runtime_suspended(dwc->dev))
>>> break;
>>> +
>>> + if (dwc->connected) {
>>> + link_state = dwc3_gadget_get_link_state(dwc);
>>> + /* bus suspend case */
>>> + if (dwc->allow_rtsusp_on_u3 &&
>>> + link_state == DWC3_LINK_STATE_U3> + break;
>>
>> dwc3_suspend_common() is called both during runtime suspend and system sleep with
>> appropriate pm_msg_t argument.
>> Do you want the same behavior for both cases?
>>
> Thank you for your comments and feedback.
> The goal here is to trigger runtime suspend when bus suspend interrupt is received. If dwc3 runtime suspend is executed, then for system sleep dwc3_suspend_common() is a no-op as we have a pm_runtime_suspended(dwc->dev) check in place in the above code.
> The exception I can think of with these changes in place is below sequence
> USB bus suspended---->Runtime suspend fails OR is not invoked for some reason--->System sleep is triggered.
>
> We can extend this change to above case as well. I believe that was the intention of your patch (i.e avoid gadget disconnect if usb bus is suspended for system sleep case)
> https://lore.kernel.org/linux-usb/20230320093447.32105-3-rogerq@xxxxxxxxxx/
>
> Please let me know your opinion on extending this series to system sleep also.
As dwc3_suspend_common()/dwc3_resume_common() is used for both runtime and
system suspend it would be nice if you test that system suspend is not broken.
Doesn't have to be enhanced.
>
>
>> You are not checking if the gadget driver was put into suspend state or not.
>> So, instead of just checking line state, should we also check that we processed
>> the suspend interrupt and gadget driver has been suspended as well?
>> i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
>> and clear it at beginning of dwc3_resume_gadget().
>> Check for that flag in the above if condition.
>> i.e. if (dwc->connected && dwc->suspended)
>>
>
> We already have flags for these purposes (dwc->link_state and dwc->suspended) that we set when suspend interrupt is received and clear during wakeup. We can use these flags here
OK but bear in mind that link_state might not be updated once runtime suspended.
>
>>> + return -EBUSY;
>>> + }
>>> dwc3_gadget_suspend(dwc);
>>> synchronize_irq(dwc->irq_gadget);
>>> dwc3_core_exit(dwc);
>>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>> switch (dwc->current_dr_role) {
>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>> + /* bus resume case */
>>> + if (dwc->connected)
>>
>> dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
>> You might want to set another SW flag to track this special case of suspend
>> with controller kept active.
>>
>
> Are you referring to the DISCONNECT interrupt ? We dont handle gadget interrupts when in runtime suspended state and dwc3 driver has to be resumed first. The mechanism for this already exists in dwc3_check_event_buf() like below
>
> if (pm_runtime_suspended(dwc->dev)) {
> pm_runtime_get(dwc->dev);
> disable_irq_nosync(dwc->irq_gadget);
> dwc->pending_events = true;
> return IRQ_HANDLED;
> }
>
> I cant think of any scenario where dwc->connected is cleared before calling resume_common() for bus suspend case. Please let me know if I am missing any corner case.
>
>>> + break;
>>> ret = dwc3_core_init_for_resume(dwc);
>>> if (ret)
>>> return ret;
>>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>> struct dwc3 *dwc = dev_get_drvdata(dev);
>>> int ret;
>>> - if (dwc3_runtime_checks(dwc))
>>> - return -EBUSY;
>>> -
>> Instead of removing this how about modifying dwc3_runtime_checks(). see below.
>>
>
> Yes we can do that. But if we are planning to extend these changes to system sleep better to leave it here IMO.
>
>>> ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>> if (ret)
>>> return ret;
>>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>> static int dwc3_runtime_idle(struct device *dev)
>>> {
>>> struct dwc3 *dwc = dev_get_drvdata(dev);
>>> + int link_state;
>>> switch (dwc->current_dr_role) {
>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>> + link_state = dwc3_gadget_get_link_state(dwc);
>>> + /* for bus suspend case return success */
>>> + if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>>> + link_state == DWC3_LINK_STATE_U3)
>>> + goto autosuspend;
>>
>> why not just break?
> Done
>>
>>> if (dwc3_runtime_checks(dwc))
>>
>> what about this dwc3_runtime_checks()?
>> Looks like the if condition you added above can go in dwc3_runtime_checks() instead.
>>
>>> return -EBUSY;
>>> break;
>>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>> break;
>>> }
>>> +autosuspend:
>>> pm_runtime_mark_last_busy(dev);
>>> pm_runtime_autosuspend(dev);
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 8b1295e4dcdd..33b2ccbbd963 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>> * @num_ep_resized: carries the current number endpoints which have had its tx
>>> * fifo resized.
>>> * @debug_root: root debugfs directory for this device to put its files in.
>>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>>> + * suspend scenario.
>>
>> what about system sleep?
>>
> Currently this series is tested for runtime suspend during bus suspend scenario. We can extend to system sleep like explained above.
>
>>> */
>>> struct dwc3 {
>>> struct work_struct drd_work;
>>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>> int last_fifo_depth;
>>> int num_ep_resized;
>>> struct dentry *debug_root;
>>> + bool allow_rtsusp_on_u3;
>>> };
>>> #define INCRX_BURST_MODE 0
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 5fd067151fbf..0797cffa2d48 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>> return -EINVAL;
>>> }
>>> - spin_lock_irqsave(&dwc->lock, flags);
>>> if (!dwc->gadget->wakeup_armed) {
>>> dev_err(dwc->dev, "not armed for remote wakeup\n");
>>> - spin_unlock_irqrestore(&dwc->lock, flags);
>>> return -EINVAL;
>>> }
>>> - ret = __dwc3_gadget_wakeup(dwc, true);
>>> + ret = pm_runtime_resume_and_get(dwc->dev);
>>> + if (ret < 0) {
>>> + pm_runtime_set_suspended(dwc->dev);
>>> + return ret;
>>> + }
>>> +
>>> + spin_lock_irqsave(&dwc->lock, flags);
>>> + ret = __dwc3_gadget_wakeup(dwc, true);
>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>> + pm_runtime_put_noidle(dwc->dev);
>>> return ret;
>>> }
>>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>> return -EINVAL;
>>> }
>>> + ret = pm_runtime_resume_and_get(dwc->dev);
>>> + if (ret < 0) {
>>> + pm_runtime_set_suspended(dwc->dev);
>>> + return ret;
>>> + }
>>> +
>>> spin_lock_irqsave(&dwc->lock, flags);
>>> /*
>>> * If the link is in U3, signal for remote wakeup and wait for the
>>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>> ret = __dwc3_gadget_wakeup(dwc, false);
>>> if (ret) {
>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>> + pm_runtime_put_noidle(dwc->dev);
>>> return -EINVAL;
>>> }
>>> dwc3_resume_gadget(dwc);
>>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>> dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>> spin_unlock_irqrestore(&dwc->lock, flags);
>>> + pm_runtime_put_noidle(dwc->dev);
>>> return ret;
>>> }
>>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>> /*
>>> * Avoid issuing a runtime resume if the device is already in the
>>> * suspended state during gadget disconnect. DWC3 gadget was already
>>> - * halted/stopped during runtime suspend.
>>> + * halted/stopped during runtime suspend except for bus suspend case
>>> + * where we would have skipped the controller halt.
>>> */
>>> if (!is_on) {
>>> pm_runtime_barrier(dwc->dev);
>>> - if (pm_runtime_suspended(dwc->dev))
>>> + if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>
>> dwc->connected will to to false on disconnect. I'm not sure if we can rely
>> on this flag here.
>>
> This is for handling gadget_pullup() when we are in bus suspend state (and thus runtime suspended) for cases where a usb composition switch is triggered. Like explained above we dont clear dwc->connected flag without resuming the driver first.
>
>>> return 0;
>>> }
>>> /*
>>> * Check the return value for successful resume, or error. For a
>>> * successful resume, the DWC3 runtime PM resume routine will handle
>>> - * the run stop sequence, so avoid duplicate operations here.
>>> + * the run stop sequence except for bus resume case, so avoid
>>> + * duplicate operations here.
>>> */
>>> ret = pm_runtime_get_sync(dwc->dev);
>>> - if (!ret || ret < 0) {
>>> + if ((!ret && !dwc->connected) || ret < 0) {
>>
>> Why this check?
>>
> In the resume operation above we would have skipped setting the run stop bit and other operations for bus suspend scenario. Hence we are not bailing out here if dwc->connected is true (which indicates that the resume operation was for cable connected case)
I think we are over-complicating the simple pullup() function.
How about not skipping setting the necessary bits in bus suspend case?
>
>>> pm_runtime_put(dwc->dev);
>>> if (ret < 0)
>>> pm_runtime_set_suspended(dwc->dev);
>>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>> }
>>> dwc->link_state = next;
>>> + pm_runtime_mark_last_busy(dwc->dev);
>>> + pm_request_autosuspend(dwc->dev);
>>
>> Not here. You should do this in dwc3_suspend_gadget() after gadget driver
>> has completed suspend.
>>
>
> Since the goal was to trigger runtime suspend when bus suspend irq is received I have added it here. I see that dwc3_suspend_gadget is called in other instances as well (like U1,U2) and hence having it here specific to U3/L2 handler is better IMO.
OK.
>>> }
>>> static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>> {
>>> if (dwc->pending_events) {
>>> dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>> + pm_runtime_put(dwc->dev);
>>
>> Why the put here?
>>
>
> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>
> if (pm_runtime_suspended(dwc->dev)) {
> pm_runtime_get(dwc->dev);
> disable_irq_nosync(dwc->irq_gadget);
> dwc->pending_events = true;
> return IRQ_HANDLED;
> }
>
No this wrong. We want the device to be active from now on.
runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
Only on next USB suspend you want to do the pm_runtime_put like you are doing it
in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
>>> dwc->pending_events = false;
>>> enable_irq(dwc->irq_gadget);
>>> + /*
>>> + * We have only stored the pending events as part
>>> + * of dwc3_interrupt() above, but those events are
>>> + * not yet handled. So explicitly invoke the
>>> + * interrupt handler for handling those events.
>>> + */
>>> + dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>> }
>>> }
>>
--
cheers,
-roger