| Message ID | 20250304000458.1826450-1-royluo@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] usb: dwc3: core: Avoid redundant system suspend/resume callbacks | expand |
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 > >
On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@synopsys.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@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 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 > > > >
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@synopsys.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@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 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 > > > >
On Tue, Mar 11, 2025, Roy Luo wrote: > On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > 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://urldefense.com/v3/__https://lore.kernel.org/all/9dd70870cfee40154a37186d4cf3ae0e9a452cbd.1441029572.git.nsekhar@ti.com/__;!!A4F2R9G_pg!chKbE4OVWBGIEQnHmj7n-VFIKaiyjgdJSmP7lqMx1N4pT1gpIz_qYMiI_vSwCRa6IzMHJ41eq9wkN3N8HE4$ > 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. If this pin drives the VBUS, changing this will also change how often we turning on/off VBUS. Unfortunately, I don't know enough about these platforms to know whether this change may impact its timing and stability. > > 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://urldefense.com/v3/__https://lore.kernel.org/all/4a8a072030c2a82867c6548627739146681b35a5.1441029572.git.nsekhar@ti.com/__;!!A4F2R9G_pg!chKbE4OVWBGIEQnHmj7n-VFIKaiyjgdJSmP7lqMx1N4pT1gpIz_qYMiI_vSwCRa6IzMHJ41eq9wkfd77zMg$ ) Hm... this link to the patch above seems never made it upstream. > 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. Thanks for the investigation. > > 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. > This still doesn't sit easy for me. I would prefer a change to the pinctrl logic be a separate commit. For the particular intention of your patch, can you just do a check if dev->pins exists and leave that alone. Create a separate patch should you think pinctrl logic be set somewhere else. Thanks, Thinh
On Tue, Mar 11, 2025 at 4:44 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Tue, Mar 11, 2025, Roy Luo wrote: > > On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > 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://urldefense.com/v3/__https://lore.kernel.org/all/9dd70870cfee40154a37186d4cf3ae0e9a452cbd.1441029572.git.nsekhar@ti.com/__;!!A4F2R9G_pg!chKbE4OVWBGIEQnHmj7n-VFIKaiyjgdJSmP7lqMx1N4pT1gpIz_qYMiI_vSwCRa6IzMHJ41eq9wkN3N8HE4$ > > 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. > > If this pin drives the VBUS, changing this will also change how often we > turning on/off VBUS. Unfortunately, I don't know enough about these > platforms to know whether this change may impact its timing and > stability. > > > > > 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://urldefense.com/v3/__https://lore.kernel.org/all/4a8a072030c2a82867c6548627739146681b35a5.1441029572.git.nsekhar@ti.com/__;!!A4F2R9G_pg!chKbE4OVWBGIEQnHmj7n-VFIKaiyjgdJSmP7lqMx1N4pT1gpIz_qYMiI_vSwCRa6IzMHJ41eq9wkfd77zMg$ ) > > Hm... this link to the patch above seems never made it upstream. > > > 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. > > Thanks for the investigation. > > > > > 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. > > > > This still doesn't sit easy for me. I would prefer a change to the > pinctrl logic be a separate commit. > > For the particular intention of your patch, can you just do a check if > dev->pins exists and leave that alone. Create a separate patch should > you think pinctrl logic be set somewhere else. > > Thanks, > Thinh SGTM, will do it in v2. Thanks a lot for the suggestion! Thanks, Roy
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; + + 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
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(-) base-commit: 99fa936e8e4f117d62f229003c9799686f74cebc