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@xxxxxxxxxx>
> ---
> 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
>
>
