Message-ID: <20250311234436.dtpkfvnwuwqhw5jr@synopsys.com>
Date: Tue, 11 Mar 2025 23:44:50 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Roy Luo <royluo@...gle.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "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

On Tue, Mar 11, 2025, Roy Luo wrote:
> On Fri, Mar 7, 2025 at 5:04 PM Thinh Nguyen <Thinh.Nguyen@...opsys.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

Powered by blists - more mailing lists