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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: qcom: link and probe failures in dwc3_qcom_mux_register()
- [High] usb: dwc3: qcom: failure to restore PIPE clock in dwc3_qcom_mux_set()
- [Medium] arm64: dts: qcom: kaanapali: missing mode-switch on usb_dp_qmpphy node
--

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

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

This commit adds the mode-switch property to the Kaanapali USB controller
node so that it switches to the UTMI clock when DisplayPort pin assignments
C and E stop 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: High]
Does adding mode-switch here expose build and probe regressions in
drivers/usb/dwc3/dwc3-qcom.c depending on CONFIG_TYPEC?

When mode-switch is present on the usb node, dwc3_qcom_probe() calls
dwc3_qcom_mux_register(), which invokes typec_mux_register():

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_mux_register() {
	if (!device_property_present(qcom->dev, "mode-switch"))
		return 0;
	...
	qcom->mux = typec_mux_register(qcom->dev, &desc);

	return PTR_ERR_OR_ZERO(qcom->mux);
}

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_probe() {
	...
	ret = dwc3_qcom_mux_register(qcom);
	if (ret)
		goto interconnect_exit;
}

In drivers/usb/dwc3/Kconfig, USB_DWC3_QCOM does not have a dependency on
TYPEC || !TYPEC (unlike PHY_QCOM_QMP_COMBO in drivers/phy/qualcomm/Kconfig).
With the default arm64 defconfig (CONFIG_USB_DWC3_QCOM=y and CONFIG_TYPEC=m),
built-in dwc3-qcom.o references typec_mux_register(), typec_mux_unregister(),
and typec_mux_get_drvdata() from the loadable module typec.ko, causing a
vmlinux link failure.

Furthermore, when CONFIG_TYPEC=n, the stub in include/linux/usb/typec_mux.h
returns ERR_PTR(-EOPNOTSUPP):

include/linux/usb/typec_mux.h:typec_mux_register() {
	return ERR_PTR(-EOPNOTSUPP);
}

Because dwc3_qcom_mux_register() propagates that error directly, won't
dwc3_qcom_probe() abort and disable USB on Kaanapali whenever CONFIG_TYPEC is
disabled?

[Severity: High]
Can enabling the mode switch on Kaanapali leave the controller stuck on the
19.2 MHz UTMI clock after exiting DisplayPort-only mode (Pin Assignment C or
E), breaking SuperSpeed USB 3.0?

In drivers/usb/dwc3/dwc3-qcom.c, dwc3_qcom_mux_set() updates qcom->dp_active
and qcom->dp_only, and then returns early without calling
dwc3_qcom_update_pipe_clk() whenever qcom->dp_only is false:

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_mux_set() {
	if (state->alt && state->alt->svid == USB_TYPEC_DP_SID) {
		qcom->dp_active = true;
		qcom->dp_only = state->mode == TYPEC_DP_STATE_C ||
				state->mode == TYPEC_DP_STATE_E;
	} else if (state->mode == TYPEC_STATE_SAFE) {
		qcom->dp_active = false;
	} else if (!qcom->dp_active) {
		qcom->dp_only = false;
	}

	/* The PHY stops the PIPE clock right after this in DP-only mode */
	if (!qcom->dp_only || pm_runtime_resume_and_get(qcom->dev))
		return 0;

	mutex_lock(&qcom->pipe_clk_lock);
	dwc3_qcom_update_pipe_clk(qcom);
	mutex_unlock(&qcom->pipe_clk_lock);
	...
}

Because dwc3_qcom_mux_set() never calls dwc3_qcom_update_pipe_clk() when
qcom->dp_only becomes false, clearing PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW
in QSCRATCH_GENERAL_CFG relies entirely on a subsequent call to
dwc3_qcom_set_role_notifier() where next_role != qcom->current_role while
qcom->dp_only is already false:

drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_set_role_notifier() {
	...
	if (qcom->current_role == next_role)
		return;
	...
	mutex_lock(&qcom->pipe_clk_lock);
	dwc3_qcom_update_pipe_clk(qcom);
	mutex_unlock(&qcom->pipe_clk_lock);
}

Doesn't this leave qcom->utmi_pipe_clk stuck at true in several cases:

1. If a connected partner switches pin assignment in place from 4-lane DP
(TYPEC_DP_STATE_C or TYPEC_DP_STATE_E) to 2-lane DP + USB3 (TYPEC_DP_STATE_D
or TYPEC_DP_STATE_F) without changing the USB data role, dwc3_qcom_mux_set()
sets qcom->dp_only = false and returns early, while
dwc3_qcom_set_role_notifier() either does not run or returns early because
qcom->current_role == next_role.

2. In drivers/soc/qcom/pmic_glink_altmode.c, pmic_glink_altmode_worker() can
transition directly to MUX_CTRL_STATE_USB3_ONLY (TYPEC_STATE_USB) or
MUX_CTRL_STATE_TUNNELING (TYPEC_MODE_USB4) without sending TYPEC_STATE_SAFE
first. Since qcom->dp_active is only cleared on TYPEC_STATE_SAFE, the
else if (!qcom->dp_active) branch is not taken and both qcom->dp_active and
qcom->dp_only remain true.

3. After unplugging a DP-only partner (which sends TYPEC_STATE_SAFE and sets
qcom->dp_active = false while leaving qcom->dp_only = true), on the next
plug-in of a USB 3.0 device, if the UCSI role notification arrives before the
altmode mux notification (or if ucsi_partner_change() in
drivers/usb/typec/ucsi/ucsi.c skips typec_set_mode(con->port, TYPEC_STATE_USB)
when UCSI_CONSTAT_PARTNER_FLAG_USB is not yet set),
dwc3_qcom_set_role_notifier() runs while qcom->dp_only is still true and
updates qcom->current_role = next_role. When dwc3_qcom_mux_set() subsequently
clears qcom->dp_only = false, it returns early without updating the clock, and
any later role notification for the same role is ignored.

[Severity: Medium]
The commit message notes that this change is needed when the USB3-DP combo PHY
is a mode switch. However, in arch/arm64/boot/dts/qcom/kaanapali.dtsi, the
companion usb_dp_qmpphy node only has orientation-switch and does not have
mode-switch (unlike sm8750.dtsi and eliza.dtsi):

arch/arm64/boot/dts/qcom/kaanapali.dtsi:usb_dp_qmpphy {
	...
	#clock-cells = <1>;
	#phy-cells = <1>;

	orientation-switch;

	status = "disabled";
}

Without mode-switch on usb_dp_qmpphy, fwnode_typec_mux_get() in
drivers/usb/typec/mux.c will not match the combo PHY, so the USB controller
will switch to the UTMI clock on DisplayPort pin assignments C and E while
usb_dp_qmpphy is never notified of the mode change. Should usb_dp_qmpphy also
have mode-switch here?

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