On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@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 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@xxxxxx/
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@xxxxxx/)
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
> >
> >
