On Tue, Mar 11, 2025, Roy Luo wrote: > On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxx/__;!!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@xxxxxx/__;!!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
