Skip to contentSkip to conversation

usb: dwc3: qcom: Keep USB 2.0 working in DisplayPort-only modes

PATCH In review Joonhoe Kim 3 participants 7 messages Nov 12 – Nov 13

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

v3 cover usb: dwc3: qcom: Keep USB 2.0 working in DisplayPort-only modes

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, and such monitors often have a USB 2.0 hub, but the dwc3 controller still runs from 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 registers the dwc3 glue as a Type-C mode switch next to the PHY, so that it gets the mode (it does not route any signals). In DP-only modes it moves the controller to the UTMI clock, as "qcom,select-utmi-as-pipe-clk" does statically, before the PHY switches.

On Kaanapali the PHY becomes a mode switch with Balaji Kalkonda's DisplayPort series [1]. The controller side for that SoC is a DTS patch sent separately to the Qualcomm tree. 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: "phy: qcom: qmp-combo: Leave the USB PCS alone in DP-only mode" (suspend in DP-only mode, sent separately) [2] and commit 89aaaa26f698 ("usb: typec: mux: fsa4480: Keep the USB 2.0 switch on in DP-only modes") for boards with an FSA4480 on the USB 2.0 pair.

Patch 2 is on top of Filippo Falezza's "usb: dwc3: qcom: Restore UTMI PIPE clock on resume" [3]. On SM7250 QSCRATCH loses its contents in system suspend, so patch 2 reads the selected clock back from QSCRATCH instead of caching it. On Kaanapali QSCRATCH keeps its contents. [3] is still under review and may change; patch 2 will be rebased if it does.

Vladyslav Tymoshchuk's VBUS override fix [4] also changed dwc3_qcom_pm_suspend() and dwc3_qcom_pm_resume(). Its next version moves the change to the dwc3 core, as Thinh suggested, so it should no longer conflict with patch 2.

Both patches are for the USB tree. The Kaanapali DTS patch, sent separately, must not be applied before patch 2: once the controller node has "mode-switch", pmic_glink_altmode defers its probe until dwc3-qcom registers the mux, which never happens without patch 2.

Patch 2 makes USB_DWC3_QCOM depend on TYPEC, so with the arm64 defconfig (TYPEC=m) dwc3-qcom is built as a module. With "mode-switch" on the controller, pmic_glink_altmode then probes later, and its first HPD event can arrive before the DisplayPort driver has enabled HPD on the aux-hpd bridge. aux-hpd drops that event, so a monitor attached at boot is not detected. Yongxing Mou's "drm/bridge: aux-hpd-bridge:
replay last HPD status on hpd_enable" [5] addresses this; it was seen on X1E as well.

Tested on a Lenovo Y700 Gen 5 (TB323FU, Kaanapali) with the series and
[3] backported to v7.3, plus [2] and the fsa4480 commit; the boot test also had a variant of [5]. On each DisplayPort entry the controller switched to the UTMI clock before the PHY switched, and back to the PIPE clock on the next USB state. The monitor's USB 2.0 hub enumerated after each of 5 DisplayPort entries, 3 of them on a replug less than 2 s after the unplug, and after each of 3 s2idle cycles with the monitor attached. Two other plugs stopped in USB PD negotiation, before DisplayPort was entered. With the monitor attached, rebooting worked (2 times) and DisplayPort came up at boot (2 boots). In these tests the mux state came after resume or needed no clock change, so the recorded change was not applied on resume.

Changes in v3:
- Patch 1: drop the mode-switch line and disallow orientation-switch and retimer-switch (Krzysztof)
- Patch 1: say that the controller only needs the mode and does not route any signals (Krzysztof)
- Patch 2: rebase on [3]; compare with the clock selected in QSCRATCH instead of a cached value
- Drop the Kaanapali DTS patch from the series. It is sent on its own for the Qualcomm tree (Krzysztof)
- Patch 2: check the safe state before the altmode, as the safe and USB states can come with the DisplayPort altmode (Sashiko)
- Patch 2: always select the UTMI clock for pin assignment C or E (Sashiko)
- Patch 2: do not resume the glue from the mux callback. From the start of a runtime or system suspend until the core has resumed, only record the state, and apply it on resume (Sashiko)
- Patch 2: leave the clock select alone on nodes without a mux
- Patch 2: only record the state once remove has started
- Link to v2: https://lore.kernel.org/all/[email protected]/

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/[email protected]/

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/
[5] https://lore.kernel.org/all/[email protected]/

Joonhoe Kim (2):
dt-bindings: usb: qcom,snps-dwc3: Add mode-switch
usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only
modes

 .../bindings/usb/qcom,snps-dwc3.yaml          |   5 +
 drivers/usb/dwc3/Kconfig                      |   1 +
 drivers/usb/dwc3/dwc3-qcom.c                  | 154 ++++++++++++++++--
 3 files changed, 142 insertions(+), 18 deletions(-)

base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
prerequisite-patch-id: a793fca7329dac964c21459f8d7a8b003d36d84d --
2.43.0

#1 lore raw

PATCH v3 1/2 dt-bindings: usb: qcom,snps-dwc3: Add mode-switch

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 to know the Type-C mode. It does not route any signals itself.

The Type-C class passes the mode only to graph nodes that have the "mode-switch" property, so allow it on the controller node through usb-switch.yaml. Orientation and retimer switching do not apply to the controller.

---
v3:
- Drop the mode-switch line, the usb-switch.yaml reference allows it. Disallow orientation-switch and retimer-switch (Krzysztof)
- Say in the message that the controller does not route any signals v2:
- Use the common usb-switch schema instead of a local definition

 Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml +5 -0
index b77c02c64d45..fdc2bc8d3045 100644
@@ -158,6 +158,10 @@ properties:
       HS/FS/LS modes are supported.
     type: boolean
 
+  orientation-switch: false
+
+  retimer-switch: false
+
   wakeup-source: true
 
 required:
@@ -170,6 +174,7 @@ required:
 
 allOf:
   - $ref: snps,dwc3-common.yaml#
+  - $ref: /schemas/usb/usb-switch.yaml#
   - if:
       properties:
         compatible:
Signature · 1 line
2.43.0
#2 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 6 lines

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 to know the Type-C mode. It does not route any signals itself.

Quoted 6 lines

The Type-C class passes the mode only to graph nodes that have the "mode-switch" property, so allow it on the controller node through usb-switch.yaml. Orientation and retimer switching do not apply to the controller.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v3 1/2
#3 lore raw

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

In reply to Joonhoe Kim
Quoted 6 lines · 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 to know the Type-C mode. It does not route any signals itself.

Quoted 7 lines

The Type-C class passes the mode only to graph nodes that have the "mode-switch" property, so allow it on the controller node through usb-switch.yaml. Orientation and retimer switching do not apply to the controller.

---

Best regards,
Krzysztof

#4 lore raw

PATCH v3 2/2 usb: dwc3: qcom: Use the UTMI clock as PIPE clock in DisplayPort-only modes

In reply to 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.

The safe and USB states can come with the DisplayPort altmode, so only the modal states with the DisplayPort SVID count as DisplayPort. Pin assignment C or E always selects the UTMI clock. Go back to the PIPE clock when a USB state follows the safe state. A change from C/E to D/F while DisplayPort is active keeps the UTMI clock: the PHY may keep DP-only mode then, and the UTMI clock still runs USB 2.0 if it does not.

Read the selected clock back from QSCRATCH, as on some SoCs it loses its contents in system suspend.

The mux callback does not resume the glue. From the start of a runtime or system suspend until the core has resumed, the callback only records the state. The resume paths apply it before the core resumes, and again once the core has resumed or when the suspend fails.

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.

---
v3:
- Rebase on Filippo Falezza's "usb: dwc3: qcom: Restore UTMI PIPE clock on resume" and use its utmi_as_pipe_clk
- Compare with the clock selected in QSCRATCH, not a cached value
- Check the safe state before the altmode: the safe and USB states can come with the DisplayPort altmode (Sashiko)
- Always select the UTMI clock for pin assignment C or E (Sashiko)
- Do not resume the glue from the mux callback. From the start of a runtime or system suspend until the core has resumed, only record the state; apply it before and after the core resumes, and when the suspend fails (Sashiko)
- Leave the clock select alone on nodes without a mux
- Only record the state once remove has started
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 | 154 +++++++++++++++++++++++++++++++----
 2 files changed, 137 insertions(+), 18 deletions(-)
drivers/usb/dwc3/Kconfig +1 -0
index 18169727a413..282f366de9af 100644
@@ -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
drivers/usb/dwc3/dwc3-qcom.c +136 -18

268 lines not on this page · raw message

Signature · 1 line
2.43.0
#5 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 15 lines

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.

Quoted 6 lines

[ ... ]
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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#6 lore raw

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

In reply to Joonhoe Kim
Quoted 36 lines · Joonhoe Kim
drivers/usb/dwc3/dwc3-qcom.c +15 -4
@@ -336,16 +346,19 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 		dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
 }
 
-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);
 
@@ -353,6 +366,38 @@ static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
 			  PIPE_UTMI_CLK_DIS);
 }
 
+static bool dwc3_qcom_utmi_clk_selected(struct dwc3_qcom *qcom)
+{
+	return readl(qcom->qscratch_base + QSCRATCH_GENERAL_CFG) &
+	       PIPE_UTMI_CLK_SEL;
+}
+
+static void dwc3_qcom_update_pipe_clk(struct dwc3_qcom *qcom)
+{

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom) · line 386

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +10 -0
+	lockdep_assert_held(&qcom->pipe_clk_lock);
+
+	if (qcom->mode_switch && !qcom->utmi_as_pipe_clk &&
+	    qcom->dp_only != dwc3_qcom_utmi_clk_selected(qcom))
+		dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
+}
+
+static void dwc3_qcom_defer_mux(struct dwc3_qcom *qcom)
+{
+	guard(mutex)(&qcom->pipe_clk_lock);

Why aren't you using simple and most readable mutex lock/unlock? guard() is not more readable.

drivers/usb/dwc3/dwc3-qcom.c static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom) · line 392

Quoted 6 lines
drivers/usb/dwc3/dwc3-qcom.c +6 -0
+	qcom->mux_deferred = true;
+}
+
+static void dwc3_qcom_undefer_mux(struct dwc3_qcom *qcom)
+{
+	guard(mutex)(&qcom->pipe_clk_lock);

Here it makes sense, but other places, why?

Quoted 85 lines
drivers/usb/dwc3/dwc3-qcom.c +58 -2
+
+	if (qcom->is_suspended)
+		return;
+
+	qcom->mux_deferred = false;
+	dwc3_qcom_update_pipe_clk(qcom);
+}
+
 static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
 	u32 val;
@@ -413,7 +458,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 	 * clock ("failed to enable ep0out").
 	 */
 	if (qcom->utmi_as_pipe_clk)
-		dwc3_qcom_select_utmi_clk(qcom);
+		dwc3_qcom_select_utmi_clk(qcom, true);
+
+	scoped_guard(mutex, &qcom->pipe_clk_lock)
+		dwc3_qcom_update_pipe_clk(qcom);
 
 	/* Clear existing events from PHY related to L2 in/out */
 	for (i = 0; i < qcom->num_ports; i++) {
@@ -446,6 +494,52 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+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);
+	bool dp = state->mode >= TYPEC_STATE_MODAL && state->alt &&
+		  state->alt->svid == USB_TYPEC_DP_SID;
+
+	guard(mutex)(&qcom->pipe_clk_lock);
+
+	if (state->mode == TYPEC_STATE_SAFE) {
+		qcom->dp_active = false;
+	} else if (dp) {
+		if (state->mode == TYPEC_DP_STATE_C ||
+		    state->mode == TYPEC_DP_STATE_E)
+			qcom->dp_only = true;
+		else if (!qcom->dp_active)
+			qcom->dp_only = false;
+		qcom->dp_active = true;
+	} else if (!qcom->dp_active) {
+		qcom->dp_only = false;
+	}
+
+	if (!qcom->mux_deferred)
+		dwc3_qcom_update_pipe_clk(qcom);
+
+	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->mode_switch = true;
+	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,
 				 const char *name)
 {
@@ -625,7 +719,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);

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 784

Quoted 9 lines
drivers/usb/dwc3/dwc3-qcom.c +1 -5
@@ -688,11 +781,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");
-	qcom->utmi_as_pipe_clk = ignore_pipe_clk;
-	if (ignore_pipe_clk)
-		dwc3_qcom_select_utmi_clk(qcom);
+	qcom->utmi_as_pipe_clk =

You use some odd formatting, making this unreadable. That's one line, eventually wrapped.

I don't get why you are changing these lines in the first place. Mixing style changes and actual code changes make it difficult to spot the differenec, but the code looks the same, no?

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 789

Quoted 5 lines
drivers/usb/dwc3/dwc3-qcom.c +3 -0
+		device_property_read_bool(dev, "qcom,select-utmi-as-pipe-clk");
+	if (qcom->utmi_as_pipe_clk)
+		dwc3_qcom_select_utmi_clk(qcom, true);
 
 	qcom->mode = usb_get_dr_mode(dev);

Best regards,
Krzysztof