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

[Thinh Nguyen]

On Tue, Jul 11, 2023, 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;
> +			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)
> +			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;
> -
>  	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;
>  		if (dwc3_runtime_checks(dwc))
>  			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.
>   */
>  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)
>  			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) {
>  		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);
>  }
>  
>  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);
>  		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);

Why do we have to do this? If there are events, the threaded interrupt
should be woken up.

Thanks,
Thinh

>  	}
>  }
> -- 
> 2.17.1
>