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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Thu, Nov 12, 2026 at 08:38:38PM +0900, Joonhoe Kim wrote:
> @@ -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)
> +{
> +	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.

> +	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?

> +
> +	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);
> @@ -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?

> +		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