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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: qcom: missing CONFIG_TYPEC handling in dwc3_qcom_mux_register()
- [High] usb: dwc3: qcom: UTMI clock stuck after DP-only exit in dwc3_qcom_mux_set()
--

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

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

This commit registers dwc3-qcom as a Type-C mode switch when the
"mode-switch" property is present and selects the UTMI clock as the PIPE
clock during DisplayPort-only pin assignments (C and E). It defers
switching back to the PHY PIPE clock until a role change notification
occurs after leaving the safe state.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..4919deae682d9 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
[ ... ]
> @@ -419,21 +429,77 @@ 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 void dwc3_qcom_update_pipe_clk(struct dwc3_qcom *qcom)
> +{
> +	if (qcom->ignore_pipe_clk || qcom->dp_only == qcom->utmi_pipe_clk)
> +		return;
> +
> +	dwc3_qcom_select_utmi_clk(qcom, qcom->dp_only);
> +}
> +
> +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) {
> +		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;
> +
> +	dwc3_qcom_update_pipe_clk(qcom);
> +	pm_runtime_put_autosuspend(qcom->dev);
> +
> +	return 0;
> +}

[Severity: High]
Can the controller get stuck on the UTMI clock after leaving a
DisplayPort-only mode?

In dwc3_qcom_mux_set(), when qcom->dp_only transitions to false, the
function returns early at:

	if (!qcom->dp_only || pm_runtime_resume_and_get(qcom->dev))
		return 0;

Switching back to the PHY PIPE clock via dwc3_qcom_select_utmi_clk(qcom,
false) therefore relies entirely on a subsequent call to
dwc3_qcom_set_role_notifier() where next_role != qcom->current_role.

However, as noted in the commit message, role and mux notifications arrive
from different services in no fixed order. After unplugging a 4-lane DP
monitor (where TYPEC_STATE_SAFE sets qcom->dp_active = false while leaving
qcom->dp_only = true and qcom->utmi_pipe_clk = true), plugging in a USB 3.x
device or 2-lane DP + USB 3.x dock can deliver the role change first:

1. dwc3_qcom_set_role_notifier(USB_ROLE_HOST) runs while qcom->dp_only is
   still true, so dwc3_qcom_update_pipe_clk() does nothing and
   qcom->current_role becomes USB_ROLE_HOST.
2. dwc3_qcom_mux_set() runs next and sets qcom->dp_only = false (for
   TYPEC_STATE_USB or TYPEC_DP_STATE_D), then returns early because
   !qcom->dp_only is true.
3. Any subsequent role notification for USB_ROLE_HOST returns early at
   if (qcom->current_role == next_role) in dwc3_qcom_set_role_notifier().

Similarly, if a dock reconfigures its DisplayPort 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 role, or if
pmic_glink_altmode_sc8280xp_notify() coalesces MUX_CTRL_STATE_NO_CONN with
a subsequent USB3 state before its worker runs (leaving qcom->dp_active
true when TYPEC_STATE_USB arrives), qcom->utmi_pipe_clk will remain true
with PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW asserted in
QSCRATCH_GENERAL_CFG.

> +
> +static int dwc3_qcom_mux_register(struct dwc3_qcom *qcom)
> +{
> +	struct typec_mux_desc desc = {};
> +
> +	if (!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);
> +}

[Severity: High]
What happens here when CONFIG_USB_DWC3_QCOM=y and CONFIG_TYPEC=m, or when
CONFIG_TYPEC=n?

In drivers/usb/dwc3/Kconfig, USB_DWC3_QCOM does not depend on
TYPEC || !TYPEC, so built-in dwc3-qcom.o will fail to link against
typec_mux_register(), typec_mux_unregister(), and typec_mux_get_drvdata()
when CONFIG_TYPEC=m.

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

static inline struct typec_mux_dev *
typec_mux_register(struct device *parent, const struct typec_mux_desc *desc)
{
	return ERR_PTR(-EOPNOTSUPP);
}

On a board with the "mode-switch" property in its device tree,
dwc3_qcom_mux_register() will return -EOPNOTSUPP to dwc3_qcom_probe() and
abort probing of the USB controller.

Should USB_DWC3_QCOM add a Kconfig dependency on TYPEC || !TYPEC (or
TYPEC || TYPEC=n, like PHY_QCOM_QMP_COMBO) and guard the mux registration
when CONFIG_TYPEC is disabled?

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