On Tue, Mar 11, 2025 at 4:44 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote: > > 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 SGTM, will do it in v2. Thanks a lot for the suggestion! Thanks, Roy
