Re: [PATCH v1] usb: dwc3: core: Avoid redundant system suspend/resume callbacks

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Tue, Mar 04, 2025, Roy Luo wrote:
> dwc3 device suspend/resume callbacks were being triggered during system
> suspend and resume even if the device was already runtime-suspended.
> This is redundant for device mode because the suspend and resume routines
> are essentially identical for system PM and runtime PM. The minor
> difference in pinctrl state changes has been moved to the common section
> in this patch.
> To prevent these unnecessary callbacks, indicate to the PM core that it
> can safely leave the device in runtime suspend if it's already
> runtime-suspended in device mode by returning a positive value in
> prepare() callback.
> 
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
>  drivers/usb/dwc3/core.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index dfa1b5fe48dc..b83f094ff1c5 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2398,10 +2398,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		dwc3_gadget_suspend(dwc);
>  		synchronize_irq(dwc->irq_gadget);
>  		dwc3_core_exit(dwc);
> +		pinctrl_pm_select_sleep_state(dwc->dev);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
>  			dwc3_core_exit(dwc);
> +			pinctrl_pm_select_sleep_state(dwc->dev);
>  			break;
>  		}
>  
> @@ -2436,6 +2438,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  		dwc3_otg_exit(dwc);
>  		dwc3_core_exit(dwc);
> +		pinctrl_pm_select_sleep_state(dwc->dev);
>  		break;
>  	default:
>  		/* do nothing */
> @@ -2453,6 +2456,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		pinctrl_pm_select_default_state(dwc->dev);
>  		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
> @@ -2462,6 +2466,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> +			pinctrl_pm_select_default_state(dwc->dev);
>  			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
> @@ -2490,6 +2495,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  		if (PMSG_IS_AUTO(msg))
>  			break;
>  
> +		pinctrl_pm_select_default_state(dwc->dev);
>  		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
> @@ -2608,8 +2614,6 @@ static int dwc3_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	pinctrl_pm_select_sleep_state(dev);
> -
>  	return 0;
>  }
>  
> @@ -2618,8 +2622,6 @@ static int dwc3_resume(struct device *dev)
>  	struct dwc3	*dwc = dev_get_drvdata(dev);
>  	int		ret = 0;
>  
> -	pinctrl_pm_select_default_state(dev);
> -
>  	pm_runtime_disable(dev);
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
> @@ -2647,14 +2649,29 @@ static void dwc3_complete(struct device *dev)
>  		dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
>  	}
>  }
> +
> +static int dwc3_prepare(struct device *dev)
> +{
> +	struct dwc3	*dwc = dev_get_drvdata(dev);
> +
> +	/*
> +	 * Indicate to the PM core that it may safely leave the device in
> +	 * runtime suspend if runtime-suspended already in device mode.
> +	 */
> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> +		return 1;

Why are you skipping suspend for all cases when in device mode? Don't we
need to check for current runtime suspend status?
(ie. check pm_runtime_status_suspended()).

I'm also a bit concernt about moving pinctrl_pm_select* to the
suspend/resume_common function. Is your device using pinctrl? If not,
how did you validate this?

Thanks,
Thinh

> +
> +	return 0;
> +}
>  #else
>  #define dwc3_complete NULL
> +#define dwc3_prepare NULL
>  #endif /* CONFIG_PM_SLEEP */
>  
>  static const struct dev_pm_ops dwc3_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
>  	.complete = dwc3_complete,
> -
> +	.prepare = dwc3_prepare,
>  	/*
>  	 * Runtime suspend halts the controller on disconnection. It relies on
>  	 * platforms with custom connection notification to start the controller
> 
> base-commit: 99fa936e8e4f117d62f229003c9799686f74cebc
> -- 
> 2.48.1.711.g2feabab25a-goog
> 
>