Message-ID: <CA+zupgzw0Fr-PHzj0PPRQGuvxB+py0EMseiToq5iPKe=iRNtgg@mail.gmail.com>
Date: Tue, 11 Mar 2025 12:55:29 -0700
From: Roy Luo <royluo@...gle.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, 
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] usb: dwc3: core: Avoid redundant system suspend/resume callbacks

Apologies, I noticed the formatting in my previous email was incorrect.
Please find the same content below, reformatted for improved readability.

On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
>
> 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@...gle.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 was looking at drivers/base/power/main.c device_suspend() to see how
direct complete works, and there are a bunch of checks in the function to
determine direct complete eligibility, including pm_runtime_status_suspended().
That's why I thought it's already taken care of by the PM framework.
However, looking at the documentation
https://docs.kernel.org/power/runtime_pm.html
again "Namely, if a system suspend .prepare() callback returns a positive
number for a device, that indicates to the PM core that the device appears to
be runtime-suspended and its state is fine.". You're right, we should also
do the check inside our .prepare callback. Will fix it in the next patch.

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

I couldn't find any device node that's actually utilizing the pinctrl "sleep"
state in upstream. I digged into the patch that introduced pinctrl to dwc3, i.e.
https://lore.kernel.org/all/9dd70870cfee40154a37186d4cf3ae0e9a452cbd.1441029572.git.nsekhar@ti.com/
The intention was to control DRVVBUS pins using the pinctrl API so
that VBUS can be turned off to conserve power when device is suspended
(which also implies this is only relevant in host mode, at least in the initial
patch). Since there was no runtime PM support in dwc3 at that time, the
code was only added in the system suspend/resume path. Yet I don't see
why this cannot be extended to the runtime suspend/resume path,
ultimately it should be safe to turn off VBUS once the controller is
completely torn down with dwc3_core_exit() regardless of which suspend
path it's taking.

Besides looking at how pinctrl in dwc3 is intended to be used, I did
an inventory on how it actually is used. There are 3 major players:
ti, qcom and socionext that has pinctrl property set in their dwc3 device node.
1. ti/omap
The pinctrl property is only set when dr_mode is host or otg.
Only the "default" state is defined, none of boards has "sleep" state
defined, not even the first user
arch/arm/boot/dts/omap/am437x-gp-evm.dts
that introduced the API to dwc3.
(https://lore.kernel.org/all/4a8a072030c2a82867c6548627739146681b35a5.1441029572.git.nsekhar@ti.com/)
Setting pinctrl "default" state seems to be pretty common in ti/omap,
and the usage is aligned with the original intention: control DRVVBUS.
It's unclear why "sleep" state is no longer used though.

2. qcom
The following 2 boards have pinctrl property defined, dr_mode are all
host and also only the "default" state is defined.
- sa8155p-adp.dts  &usb_1_dwc3 {
                               dr_mode = "host";
                               pinctrl-names = "default";
                               pinctrl-0 = <&usb2phy_ac_en1_default>;
                               };
                               &usb_2_dwc3 {
                               dr_mode = "host";
                               pinctrl-names = "default";
                               pinctrl-0 = <&usb2phy_ac_en2_default>;
                               };
- sm8350-hdk.dts  &usb_2_dwc3 {
                              dr_mode = "host";
                              pinctrl-names = "default";
                              pinctrl-0 = <&usb_hub_enabled_state>;
                              };
It seems the pinctrl is used to control phy and perhaps downstream usb hub.
Nothing is turned off explicitly during sleep as "sleep" state isn't defined.
It's more like setting the required pins for host mode to work.

3. socionext
The pinctrl property is set on controllers with dr_mode peripheral or host.
Still, only the "default" state is defined.
The pin is assigned according to its dr_mode, controllers with dr_mode
host will be assigned with pinctrl_usb* pin, while controllers with dr_mode
peripheral will get pinctrl_usb*_device pin.
        pinctrl_usb0: usb0 {
                groups = "usb0";
                function = "usb0";
        };
        pinctrl_usb0_device: usb0-device {
                groups = "usb0_device";
                function = "usb0";
        };
Again, these pins are not related to power management, it's tied to dr_mode.

To summarize the current pinctrl usage in dwc3:
1. No user of "sleep" state, meaning it's unlikely to cause any impact
on suspend flow.
2. Typically, the default pin state reflects the controller's dr_mode,
acting as a pre-configuration step to set the operational mode.

Based on the above observation, the code change on the pinctrl is
unlikely to introduce a regression as it aligns with the original intention
of the pinctrl property, and the pinctrl_pm_select_sleep_state() is
essentially an NOP in upstream as of now. Besides,
pinctrl_pm_select_default_state() is called whenever we try to
re-initialize the controller.
I hope this addresses your concern.

Best,
Roy

> > +
> > +     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
> >
> >

Powered by blists - more mailing lists