[PATCH v2 0/3] usb: dwc3: qcom: Keep USB 2.0 working in DisplayPort-only modes

[PATCH v2 0/3] usb: dwc3: qcom: Keep USB 2.0 working in DisplayPort-only modes

[Joonhoe Kim]

With a USB-C monitor in DisplayPort pin assignment C or E, the QMP
USB3-DP combo PHY gives all four lanes to DisplayPort and keeps its USB
PCS powered down. The USB 2.0 pair still works. Such monitors often
have a USB 2.0 hub. But the dwc3 controller still depends on the USB3
PIPE clock. On Kaanapali the next xHCI reset in this state times out
("Host halt failed, -110"). USB stays lost until the cable is replugged.
Commit f546912bcac6 ("phy: qcom: qmp-combo: Move pipe_clk on/off to
common") keeps the pipe clock branch enabled. That is not enough here.

This series lets the controller be a Type-C mode switch next to the PHY
and moves it to the UTMI clock, the way "qcom,select-utmi-as-pipe-clk"
does statically, before the PHY switches to DP-only mode.

On Kaanapali the PHY becomes a mode switch with Balaji Kalkonda's
DisplayPort series [1]; patch 3 is the controller side for the same
SoC. Other SoCs whose PHY is already a mode switch (e.g. SM8650,
SM8750) may need the same property on their controller node (not
tested).

Related fixes sent separately: "phy: qcom: qmp-combo: Leave the USB PCS
alone in DP-only mode" (suspend in DP-only mode) [2] and "usb: typec:
mux: fsa4480: Keep the USB 2.0 switch on in DP-only modes" (boards with
an FSA4480 on the USB 2.0 pair) [3].

Patch 2 conflicts with Vladyslav Tymoshchuk's "usb: dwc3: qcom: Defer
VBUS override update during system suspend" [4] in
dwc3_qcom_pm_suspend() and dwc3_qcom_pm_resume(). Both add a lock
there. Like [4], the mux callback defers the QSCRATCH write while the
glue is suspended for system sleep. On top of [4] the two locks can
become one.

Patches 1-2 are for the USB tree. Patch 3 must not be applied before
patch 2: with "mode-switch" on the controller and no mux registered by
dwc3-qcom, pmic_glink_altmode defers its probe indefinitely.

Tested on a Lenovo Y700 Gen 5 (TB323FU, Kaanapali) with the series
backported to v7.3, plus [2] and [3]. A USB-C monitor ran over 4-lane
HBR3. Its USB 2.0 hub worked after 10 replugs and after a plug-in during
s2idle. In the s2idle case the mux state came after resume, so the
deferred clock switch was not exercised. With the monitor
unplugged, a USB 3 stick enumerated at SuperSpeed. Quick replugs (under
a second) worked 9 times out of 10. The other time no PD contract was
made, so no alternate mode was entered.

Changes in v2:
- Patch 1: use the common usb-switch schema
- Patch 2: switch back to the PIPE clock in the mux callback, when a
  USB state follows the safe state. v1 waited for the next role change,
  which can come before the mux state (Sashiko)
- Patch 2: keep the UTMI clock on a change of pin assignment while
  DisplayPort is active, as the PHY keeps its mode then
- Patch 2: while the glue is suspended for system sleep, only record
  the mux state and select the clock on resume
- Patch 2: depend on TYPEC || TYPEC=n. Skip the mux without
  CONFIG_TYPEC. The arm64 defconfig has USB_DWC3_QCOM=y and TYPEC=m,
  which failed to link (Sashiko). It now builds the glue as a module
- Patch 2: no change to the role notifier
- Patch 3: note the dependency on [1] for the PHY's "mode-switch"
- Patch 3: add Reviewed-by from Abel
- Link to v1: https://lore.kernel.org/all/20261102215121.738-1-26rote@gmail.com/

[1] https://lore.kernel.org/all/20261022-kaanapali_dp-v3-0-b5ae8e6083c3@oss.qualcomm.com/
[2] https://lore.kernel.org/all/20261023123512.1912-1-26rote@gmail.com/
[3] https://lore.kernel.org/all/20261022165143.1327-1-26rote@gmail.com/
[4] https://lore.kernel.org/all/20261018143126.3978-1-tvlad744@gmail.com/

Joonhoe Kim (3):
  dt-bindings: usb: qcom,snps-dwc3: Add mode-switch
  usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only
    modes
  arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode
    switch

 .../bindings/usb/qcom,snps-dwc3.yaml          |   3 +
 arch/arm64/boot/dts/qcom/kaanapali.dtsi       |   1 +
 drivers/usb/dwc3/Kconfig                      |   1 +
 drivers/usb/dwc3/dwc3-qcom.c                  | 103 +++++++++++++++---
 4 files changed, 95 insertions(+), 13 deletions(-)


base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
-- 
2.43.0

[PATCH v2 1/3] dt-bindings: usb: qcom,snps-dwc3: Add mode-switch

[Joonhoe Kim]

The QMP USB3-DP combo PHY stops the USB3 PIPE clock when a Type-C
DisplayPort pin assignment gives it all four lanes for DisplayPort,
while the controller can keep running its USB 2.0 port. The controller
has to move to the UTMI clock before that happens, so it needs the
Type-C mux state as well. fwnode_typec_mux_get() only matches graph
nodes that have the "mode-switch" property, so allow it on the
controller node.

Signed-off-by: Joonhoe Kim <26rote@gmail.com>
---
v2:
- Use the common usb-switch schema instead of a local definition

 Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
index b77c02c64d45..14fc7e4ebc80 100644
--- a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
@@ -158,6 +158,8 @@ properties:
       HS/FS/LS modes are supported.
     type: boolean
 
+  mode-switch: true
+
   wakeup-source: true
 
 required:
@@ -170,6 +172,7 @@ required:
 
 allOf:
   - $ref: snps,dwc3-common.yaml#
+  - $ref: /schemas/usb/usb-switch.yaml#
   - if:
       properties:
         compatible:
-- 
2.43.0

Re: [PATCH v2 1/3] dt-bindings: usb: qcom,snps-dwc3: Add mode-switch

[sashiko-bot]

> The QMP USB3-DP combo PHY stops the USB3 PIPE clock when a Type-C
> DisplayPort pin assignment gives it all four lanes for DisplayPort,
> while the controller can keep running its USB 2.0 port. The controller
> has to move to the UTMI clock before that happens, so it needs the
> Type-C mux state as well. fwnode_typec_mux_get() only matches graph
> nodes that have the "mode-switch" property, so allow it on the
> controller node.
> 
> Signed-off-by: Joonhoe Kim <26rote@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103135057.1682-1-26rote@gmail.com?part=1

[PATCH v2 2/3] usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only modes

[Joonhoe Kim]

With DisplayPort pin assignment C or E the QMP USB3-DP combo PHY gives
all four lanes to DisplayPort and powers its USB PCS down. The
controller can still run USB 2.0, e.g. for the hub of a USB-C monitor.
Keeping the pipe clock branch enabled, as commit f546912bcac6 ("phy:
qcom: qmp-combo: Move pipe_clk on/off to common") does, is not enough on
Kaanapali. The next xHCI reset in this mode times out ("Host halt
failed, -110"), and USB stops working until the cable is replugged.

Register the glue as a Type-C mode switch when the node has the
"mode-switch" property. Select the UTMI clock as PIPE clock in DP-only
modes, as "qcom,select-utmi-as-pipe-clk" does statically.
typec_mux_set() calls the switches in the order of the connector's
ports. Port 0 (HS, the controller) comes before port 1 (SS, the PHY).
So the controller switches before the PHY powers its USB side down.

Go back to the PIPE clock when a USB state follows the safe state. A
change of pin assignment while DisplayPort is active keeps the UTMI
clock, as the PHY keeps DP-only mode then. The PHY also delays its
switch while DisplayPort is still powered, which the controller does
not see. After a very quick replug the controller could thus use the
PIPE clock while the PHY is still in DP-only mode. This has not been
seen on hardware.

The mux can be called while the glue is suspended for system sleep,
e.g. from the UCSI connector change work. Then only record the state
and select the clock on resume, before the core resumes.

The mux needs the Type-C class, so depend on TYPEC || TYPEC=n as
PHY_QCOM_QMP_COMBO does, and do not register the mux without it. With
the arm64 defconfig (TYPEC=m) this builds the glue, including
dwc3-qcom-legacy, as a module.

Signed-off-by: Joonhoe Kim <26rote@gmail.com>
---
v2:
- Switch back to the PIPE clock from the mux callback, not on a role
  change (Sashiko)
- Keep dp_only while DisplayPort is active, as the PHY keeps its mode
- Defer the clock switch to resume while suspended for system sleep
- Depend on TYPEC || TYPEC=n, skip the mux without CONFIG_TYPEC (Sashiko)

 drivers/usb/dwc3/Kconfig     |   1 +
 drivers/usb/dwc3/dwc3-qcom.c | 103 ++++++++++++++++++++++++++++++-----
 2 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 18169727a413..282f366de9af 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -132,6 +132,7 @@ config USB_DWC3_QCOM
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on EXTCON || !EXTCON
 	depends on OF
+	depends on TYPEC || TYPEC=n
 	default USB_DWC3
 	help
 	  Some Qualcomm SoCs use DesignWare Core IP for USB2/3
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ac68b4218b56..71b0402f0ae6 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -19,6 +19,9 @@
 #include <linux/iopoll.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb.h>
+#include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
 #include "core.h"
 #include "glue.h"
 
@@ -81,6 +84,13 @@ struct dwc3_qcom {
 	enum usb_dr_mode	mode;
 	bool			is_suspended;
 	bool			pm_suspended;
+	bool			ignore_pipe_clk;
+	bool			dp_active;
+	bool			dp_only;
+	bool			utmi_pipe_clk;
+	/* protects dp_active, dp_only, utmi_pipe_clk and pm_suspended */
+	struct mutex		pipe_clk_lock;
+	struct typec_mux_dev	*mux;
 	struct icc_path		*icc_path_ddr;
 	struct icc_path		*icc_path_apps;
 
@@ -419,21 +429,72 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
+static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom, bool utmi)
 {
-	/* Configure dwc3 to use UTMI clock as PIPE clock not present */
 	dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
 			  PIPE_UTMI_CLK_DIS);
 
 	usleep_range(100, 1000);
 
-	dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
-			  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
+	if (utmi)
+		dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+				  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
+	else
+		dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+				  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
 
 	usleep_range(100, 1000);
 
 	dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
 			  PIPE_UTMI_CLK_DIS);
+
+	qcom->utmi_pipe_clk = utmi;
+}
+
+static int dwc3_qcom_mux_set(struct typec_mux_dev *mux,
+			     struct typec_mux_state *state)
+{
+	struct dwc3_qcom *qcom = typec_mux_get_drvdata(mux);
+
+	guard(mutex)(&qcom->pipe_clk_lock);
+
+	if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
+		if (!qcom->dp_active)
+			qcom->dp_only = state->mode == TYPEC_DP_STATE_C ||
+					state->mode == TYPEC_DP_STATE_E;
+		qcom->dp_active = true;
+	} else if (state->mode == TYPEC_STATE_SAFE) {
+		qcom->dp_active = false;
+	} else if (!qcom->dp_active) {
+		qcom->dp_only = false;
+	}
+
+	if (qcom->ignore_pipe_clk || qcom->pm_suspended ||
+	    qcom->dp_only == qcom->utmi_pipe_clk ||
+	    pm_runtime_resume_and_get(qcom->dev))
+		return 0;
+
+	dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
+	pm_runtime_put_autosuspend(qcom->dev);
+
+	return 0;
+}
+
+static int dwc3_qcom_mux_register(struct dwc3_qcom *qcom)
+{
+	struct typec_mux_desc desc = {};
+
+	if (!IS_ENABLED(CONFIG_TYPEC) ||
+	    !device_property_present(qcom->dev, "mode-switch"))
+		return 0;
+
+	desc.drvdata = qcom;
+	desc.fwnode = dev_fwnode(qcom->dev);
+	desc.set = dwc3_qcom_mux_set;
+
+	qcom->mux = typec_mux_register(qcom->dev, &desc);
+
+	return PTR_ERR_OR_ZERO(qcom->mux);
 }
 
 static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
@@ -615,7 +676,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct resource		res;
 	struct resource		*r;
 	int			ret;
-	bool			ignore_pipe_clk;
 	bool			wakeup_source;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
@@ -678,10 +738,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	 * Disable pipe_clk requirement if specified. Used when dwc3
 	 * operates without SSPHY and only HS/FS/LS modes are supported.
 	 */
-	ignore_pipe_clk = device_property_read_bool(dev,
-				"qcom,select-utmi-as-pipe-clk");
-	if (ignore_pipe_clk)
-		dwc3_qcom_select_utmi_clk(qcom);
+	qcom->ignore_pipe_clk =
+		device_property_read_bool(dev, "qcom,select-utmi-as-pipe-clk");
+	if (qcom->ignore_pipe_clk)
+		dwc3_qcom_select_utmi_clk(qcom, true);
 
 	qcom->mode = usb_get_dr_mode(dev);
 
@@ -698,6 +758,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 			qcom->current_role = USB_ROLE_DEVICE;
 	}
 
+	mutex_init(&qcom->pipe_clk_lock);
 	qcom->dwc.glue_ops = &dwc3_qcom_glue_ops;
 
 	qcom->dwc.dev = dev;
@@ -715,6 +776,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto remove_core;
 
+	ret = dwc3_qcom_mux_register(qcom);
+	if (ret)
+		goto interconnect_exit;
+
 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
 	device_init_wakeup(&pdev->dev, wakeup_source);
 
@@ -722,6 +787,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	return 0;
 
+interconnect_exit:
+	dwc3_qcom_interconnect_exit(qcom);
 remove_core:
 	dwc3_core_remove(&qcom->dwc);
 clk_disable:
@@ -738,6 +805,8 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	if (pm_runtime_resume_and_get(qcom->dev) < 0)
 		return;
 
+	if (qcom->mux)
+		typec_mux_unregister(qcom->mux);
 	dwc3_core_remove(&qcom->dwc);
 	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
 	dwc3_qcom_interconnect_exit(qcom);
@@ -756,6 +825,8 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	guard(mutex)(&qcom->pipe_clk_lock);
+
 	ret = dwc3_qcom_suspend(qcom, wakeup);
 	if (ret)
 		return ret;
@@ -772,11 +843,17 @@ static int dwc3_qcom_pm_resume(struct device *dev)
 	bool wakeup = device_may_wakeup(dev);
 	int ret;
 
-	ret = dwc3_qcom_resume(qcom, wakeup);
-	if (ret)
-		return ret;
+	scoped_guard(mutex, &qcom->pipe_clk_lock) {
+		ret = dwc3_qcom_resume(qcom, wakeup);
+		if (ret)
+			return ret;
 
-	qcom->pm_suspended = false;
+		if (!qcom->ignore_pipe_clk &&
+		    qcom->dp_only != qcom->utmi_pipe_clk)
+			dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
+
+		qcom->pm_suspended = false;
+	}
 
 	ret = dwc3_pm_resume(&qcom->dwc);
 	if (ret)
-- 
2.43.0

Re: [PATCH v2 2/3] usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only modes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: qcom: broken Type-C state handling in dwc3_qcom_mux_set()
- [High] usb: dwc3: qcom: PM races and lost clock switches with dwc3_qcom_mux_set()
--

commit 8f6090d15a3eafa1d84e143a83d04937bda97612
Author: Joonhoe Kim <26rote@gmail.com>

usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only modes

This commit registers the Qualcomm DWC3 glue driver as a Type-C mode switch
when the mode-switch property is present. It switches the controller to use
the UTMI clock in place of the PIPE clock during 4-lane DisplayPort-only
modes (pin assignments C and E) so that USB 2.0 continues to work when the
QMP combo PHY powers down its USB PCS.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..71b0402f0ae66 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> @@ -419,21 +429,72 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
> +static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom, bool utmi)
>  {
> -	/* Configure dwc3 to use UTMI clock as PIPE clock not present */
>  	dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
>  			  PIPE_UTMI_CLK_DIS);
>  
>  	usleep_range(100, 1000);
>  
> -	dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
> -			  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
> +	if (utmi)
> +		dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
> +				  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
> +	else
> +		dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
> +				  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
>  
>  	usleep_range(100, 1000);
>  
>  	dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
>  			  PIPE_UTMI_CLK_DIS);
> +
> +	qcom->utmi_pipe_clk = utmi;
> +}
> +
> +static int dwc3_qcom_mux_set(struct typec_mux_dev *mux,
> +			     struct typec_mux_state *state)
> +{
> +	struct dwc3_qcom *qcom = typec_mux_get_drvdata(mux);
> +
> +	guard(mutex)(&qcom->pipe_clk_lock);
> +
> +	if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
> +		if (!qcom->dp_active)
> +			qcom->dp_only = state->mode == TYPEC_DP_STATE_C ||
> +					state->mode == TYPEC_DP_STATE_E;
> +		qcom->dp_active = true;
> +	} else if (state->mode == TYPEC_STATE_SAFE) {
> +		qcom->dp_active = false;
> +	} else if (!qcom->dp_active) {
> +		qcom->dp_only = false;
> +	}

[Severity: High]
When the Type-C DisplayPort altmode driver transitions through the safe
state or exits altmode, typec_altmode_set_mux() in drivers/usb/typec/bus.c
populates state.alt with the DP altmode device (where state.alt->svid is
USB_TYPEC_DP_SID):

drivers/usb/typec/bus.c:typec_altmode_set_mux()
	state.alt = &alt->adev;
	state.mode = conf;
	state.data = data;

	return typec_mux_set(alt->mux, &state);

Before configuring pin assignment C or E, typec_altmode_enter() and
dp_altmode_configure_vdm() in drivers/usb/typec/altmodes/displayport.c
first place the connector into TYPEC_STATE_SAFE via typec_altmode_notify().
Because dwc3_qcom_mux_set() checks state->alt && state->alt->svid ==
USB_TYPEC_DP_SID before checking state->mode == TYPEC_STATE_SAFE, that
initial TYPEC_STATE_SAFE notification enters the first branch, sets
qcom->dp_only = false, and sets qcom->dp_active = true. When
dp_altmode_configured() then notifies TYPEC_DP_STATE_C or TYPEC_DP_STATE_E,
qcom->dp_active is already true, so qcom->dp_only is not updated and
dwc3_qcom_select_utmi_clk() is never called.

Similarly, on disconnect or mode exit, typec_altmode_exit(), typec_remove(),
and dp_altmode_notify() pass state->alt->svid == USB_TYPEC_DP_SID with
TYPEC_STATE_SAFE or TYPEC_STATE_USB, which also hit the first branch and
leave qcom->dp_active and qcom->dp_only stuck.

In addition, qmp_combo_typec_mux_set() in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c only delays a PHY mode switch
when qmp->dp_powered_on is true:

	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
		dev_dbg(qmp->dev, "typec_mux_set: DP PHY is still in use, delaying switch\n");
		return 0;
	}

If a port driver such as pmic_glink_altmode enters TYPEC_DP_STATE_D while
qmp->dp_powered_on is false and then reconfigures to TYPEC_DP_STATE_C or
TYPEC_DP_STATE_E without an intervening TYPEC_STATE_SAFE, the PHY switches
to QMPPHY_MODE_DP_ONLY while dwc3_qcom_mux_set() skips updating
qcom->dp_only because qcom->dp_active is already true.

Should state->mode == TYPEC_STATE_SAFE and TYPEC_STATE_USB be checked
before inspecting state->alt, and can gating qcom->dp_only on
!qcom->dp_active miss valid transitions to DP-only mode?

> +
> +	if (qcom->ignore_pipe_clk || qcom->pm_suspended ||
> +	    qcom->dp_only == qcom->utmi_pipe_clk ||
> +	    pm_runtime_resume_and_get(qcom->dev))
> +		return 0;
> +
> +	dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
> +	pm_runtime_put_autosuspend(qcom->dev);
> +
> +	return 0;
> +}
[ ... ]
> @@ -756,6 +825,8 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	guard(mutex)(&qcom->pipe_clk_lock);
> +
>  	ret = dwc3_qcom_suspend(qcom, wakeup);
>  	if (ret)
>  		return ret;
> @@ -772,11 +843,17 @@ static int dwc3_qcom_pm_resume(struct device *dev)
>  	bool wakeup = device_may_wakeup(dev);
>  	int ret;
>  
> -	ret = dwc3_qcom_resume(qcom, wakeup);
> -	if (ret)
> -		return ret;
> +	scoped_guard(mutex, &qcom->pipe_clk_lock) {
> +		ret = dwc3_qcom_resume(qcom, wakeup);
> +		if (ret)
> +			return ret;
>  
> -	qcom->pm_suspended = false;
> +		if (!qcom->ignore_pipe_clk &&
> +		    qcom->dp_only != qcom->utmi_pipe_clk)
> +			dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
> +
> +		qcom->pm_suspended = false;
> +	}
>  
>  	ret = dwc3_pm_resume(&qcom->dwc);
>  	if (ret)

[Severity: High]
How does the synchronization between dwc3_qcom_mux_set(), system sleep, and
runtime PM handle the following cases?

1. In dwc3_qcom_pm_resume(), qcom->pm_suspended is cleared and
qcom->pipe_clk_lock is released before dwc3_pm_resume(&qcom->dwc) runs:

drivers/usb/dwc3/core.c:dwc3_pm_resume()
	pm_runtime_disable(dev);
	ret = pm_runtime_set_active(dev);
	if (ret)
		goto out;

	ret = dwc3_resume_common(dwc, PMSG_RESUME);

If dwc3_qcom_mux_set() runs on a workqueue right after pipe_clk_lock is
released:
- Before pm_runtime_disable(dev), if the device was runtime-suspended prior
  to system sleep, pm_runtime_resume_and_get(qcom->dev) can trigger
  dwc3_qcom_runtime_resume() -> dwc3_runtime_resume(&qcom->dwc)
  concurrently with dwc3_pm_resume(&qcom->dwc).
- Between pm_runtime_disable(dev) and pm_runtime_enable(dev), if last_status
  was RPM_SUSPENDED, pm_runtime_resume_and_get(qcom->dev) returns -EACCES
  and dwc3_qcom_mux_set() returns 0 after dwc3_qcom_pm_resume() has already
  checked qcom->dp_only != qcom->utmi_pipe_clk, losing the clock switch.
- If the device was runtime-active, pm_runtime_resume_and_get(qcom->dev)
  succeeds and dwc3_qcom_select_utmi_clk() gates PIPE_UTMI_CLK_DIS while
  dwc3_resume_common() is resetting and reinitializing the DWC3 core.

2. In dwc3_qcom_pm_suspend(), dwc3_pm_suspend(&qcom->dwc) runs before
acquiring qcom->pipe_clk_lock and setting qcom->pm_suspended = true. If the
device was runtime-suspended, a concurrent dwc3_qcom_mux_set() call can
invoke pm_runtime_resume_and_get(qcom->dev) -> dwc3_qcom_runtime_resume()
after dwc3_pm_suspend() has completed, and dwc3_qcom_suspend() will then
disable qcom->clks while the DWC3 core and PHYs are active.

3. When the device is runtime-suspended in device role without wakeup,
dwc3_qcom_prepare() -> dwc3_pm_prepare() returns 1, enabling the PM core
direct_complete path, which disables runtime PM and skips both
dwc3_qcom_pm_suspend() and dwc3_qcom_pm_resume(). If dwc3_qcom_mux_set() is
called during system sleep under direct_complete, qcom->pm_suspended is
false and pm_runtime_resume_and_get(qcom->dev) fails with -EACCES. Because
dwc3_qcom_runtime_resume() does not check qcom->dp_only !=
qcom->utmi_pipe_clk, the pending clock switch is never applied on a
subsequent runtime resume.

4. When the controller is runtime-suspended and dwc3_qcom_mux_set() calls
pm_runtime_resume_and_get(qcom->dev), dwc3_qcom_runtime_resume() calls
dwc3_runtime_resume(&qcom->dwc) before dwc3_qcom_mux_set() switches the
clock via dwc3_qcom_select_utmi_clk().

Could the clock selection check be performed inside dwc3_qcom_resume() (so
both system resume and runtime resume apply any pending change before the
core resumes), with qcom->pipe_clk_lock and qcom->pm_suspended covering the
full suspend and resume sequences?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103135057.1682-1-26rote@gmail.com?part=2

[PATCH v2 3/3] arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode switch

[Joonhoe Kim]

When the USB3-DP combo PHY is a mode switch, DisplayPort pin
assignments C and E put it in DP-only mode, where it stops the USB3
PIPE clock. Make the USB controller a mode switch as well, so that it
moves to the UTMI clock first and keeps its USB 2.0 port working.

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
Signed-off-by: Joonhoe Kim <26rote@gmail.com>
---
The "mode-switch" property of usb_dp_qmpphy comes with patch 3 of
Balaji Kalkonda's Kaanapali DisplayPort series:
https://lore.kernel.org/all/20261022-kaanapali_dp-v3-0-b5ae8e6083c3@oss.qualcomm.com/

 arch/arm64/boot/dts/qcom/kaanapali.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
index e48a179d0722..a06d5d867488 100644
--- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
+++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
@@ -3941,6 +3941,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			tx-fifo-resize;
 			dma-coherent;
 
+			mode-switch;
 			usb-role-switch;
 			wakeup-source;
 
-- 
2.43.0

Re: [PATCH v2 3/3] arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode switch

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: dwc3: qcom: race between dwc3_qcom_mux_set() and system suspend/resume
--

commit fe7520845f2780f9c1a4d2eb56c40d6f95200b73
Author: Joonhoe Kim <26rote@gmail.com>

arm64: dts: qcom: kaanapali: Mark the USB controller as a Type-C mode switch

This commit marks the Kaanapali USB controller node with the mode-switch
property so that it switches to the UTMI clock when the USB3-DP combo PHY
enters DisplayPort-only mode and stops the USB3 PIPE clock.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index e48a179d07220..a06d5d8674885 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> @@ -3941,6 +3941,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  			tx-fifo-resize;
>  			dma-coherent;
>  
> +			mode-switch;

[Severity: Medium]
Can enabling mode-switch on the Kaanapali USB controller trigger a race
between dwc3_qcom_mux_set() and system suspend/resume in
drivers/usb/dwc3/dwc3-qcom.c?

On Kaanapali boards (kaanapali-mtp.dts and kaanapali-qrd.dts),
qcom,kaanapali-pmic-glink instantiates UCSI on connector@0. In
ucsi_register_port(), UCSI creates a non-freezable singlethread workqueue
(con->wq) and acquires the USB controller mode switch via typec_mux_get().
If a connector change happens during system sleep transitions,
ucsi_handle_connector_change() runs on con->wq and invokes
typec_set_mode() -> typec_mux_set() -> dwc3_qcom_mux_set():

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_mux_set() {
	...
	if (qcom->ignore_pipe_clk || qcom->pm_suspended ||
	    qcom->dp_only == qcom->utmi_pipe_clk ||
	    pm_runtime_resume_and_get(qcom->dev))
		return 0;

	dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
	...
}

In dwc3_qcom_pm_suspend(), dwc3_pm_suspend(&qcom->dwc) runs before acquiring
qcom->pipe_clk_lock and setting qcom->pm_suspended = true, while runtime PM
is still enabled until device_suspend_late():

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_pm_suspend() {
	...
	ret = dwc3_pm_suspend(&qcom->dwc);
	if (ret)
		return ret;

	guard(mutex)(&qcom->pipe_clk_lock);

	ret = dwc3_qcom_suspend(qcom, wakeup);
	if (ret)
		return ret;

	qcom->pm_suspended = true;
	...
}

If qcom->dev was runtime-suspended and dwc3_qcom_mux_set() runs after
dwc3_pm_suspend() finishes but before pipe_clk_lock is acquired,
dwc3_qcom_mux_set() sees qcom->pm_suspended == false and calls
pm_runtime_resume_and_get(qcom->dev). Wouldn't that run
dwc3_qcom_runtime_resume() -> dwc3_runtime_resume(&qcom->dwc) (reinitializing
the core and resuming the PHYs) right before dwc3_qcom_suspend() disables
the glue clocks and interconnects?

Similarly, in dwc3_qcom_pm_resume(), qcom->pm_suspended is cleared and
pipe_clk_lock is released before calling dwc3_pm_resume(&qcom->dwc):

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_pm_resume() {
	...
	scoped_guard(mutex, &qcom->pipe_clk_lock) {
		ret = dwc3_qcom_resume(qcom, wakeup);
		if (ret)
			return ret;

		if (!qcom->ignore_pipe_clk &&
		    qcom->dp_only != qcom->utmi_pipe_clk)
			dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);

		qcom->pm_suspended = false;
	}

	ret = dwc3_pm_resume(&qcom->dwc);
	...
}

Inside dwc3_pm_resume(), pm_runtime_disable(dev) is held across
pm_runtime_set_active(dev) and dwc3_resume_common(). If qcom->dev was
runtime-suspended prior to system sleep, __pm_runtime_disable() records
dev->power.last_status = RPM_SUSPENDED, so rpm_resume() returns -EACCES while
runtime PM is disabled. If dwc3_qcom_mux_set() runs during that window, it
sees qcom->pm_suspended == false, updates qcom->dp_only, and returns 0 when
pm_runtime_resume_and_get(qcom->dev) fails with -EACCES. Doesn't this skip
dwc3_qcom_select_utmi_clk() and leave qcom->utmi_pipe_clk out of sync with
qcom->dp_only?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103135057.1682-1-26rote@gmail.com?part=3