Re: [PATCH] phy: qualcomm: qmp-combo: Differentiate clocks related to the USB block

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phy: qcom-qmp-combo: clock state imbalance for usb_clks with USB4
- [High] phy: qcom-qmp-combo: unclocked register access in USB4 init path
- [Medium] phy: qcom-qmp-combo: missing clock rollback on runtime resume error
--

commit e81a462153a1ae3712754378a06c88bd963d18ee
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Differentiate clocks related to the USB block

This patch splits the USB and DisplayPort clocks into a separate list that
is enabled only when the USB initialization routine is executed. It also
moves autonomous mode control to when USB is active.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index a4f130fc33e39..f14fb8ef90289 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -2678,7 +2680,11 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
>  
>  /* list of clocks required by phy */
>  static const char * const qmp_combo_phy_clk_l[] = {
> -	"aux", "cfg_ahb", "ref", "com_aux",
> +	"cfg_ahb", "ref",
> +};
> +
> +static const char * const qmp_combo_usb_phy_clk_l[] = {
> +	"aux", "com_aux",
>  };

[Severity: High]
When these clocks are separated into usb_clks, they are no longer enabled
during qmp_combo_usb4_init().

Since qmp_combo_usb4_init() calls qmp_combo_reconfigure_phy() and
qmp_combo_usb_power_on() to access PCS and SerDes registers, will this
cause unclocked register access faults (such as a synchronous external
abort) because usb_clks are not enabled in the USB4 path?

[ ... ]
> @@ -4403,13 +4409,20 @@ static int qmp_combo_usb_init(struct phy *phy)
>  		return 0;
>  	}
>  
> -	ret = qmp_combo_com_init(qmp, false);
> +	ret = clk_bulk_prepare_enable(qmp->num_usb_clks, qmp->usb_clks);
>  	if (ret)
>  		return ret;

[Severity: High]
Just above this in qmp_combo_usb_init(), there is a check for USB4:

	if (qmp->usb4_init_count) {
		qmp->init_count++;
		qmp->usb_init_count++;
		return 0;
	}

If USB4 is active, qmp->usb_init_count is incremented but usb_clks are
never enabled.

Later, qmp_combo_runtime_suspend() unconditionally disables usb_clks based
on usb_init_count:

	if (qmp->usb_init_count) {
		qmp_combo_enable_autonomous_mode(qmp);
		clk_bulk_disable_unprepare(qmp->num_usb_clks, qmp->usb_clks);
	}

Could this cause a clock disable underflow since the clocks were skipped
during initialization? Similarly, would qmp_combo_usb_exit() leak the
clocks if USB is initialized first and exited before USB4?

[ ... ]
> @@ -4701,7 +4719,12 @@ static int __maybe_unused qmp_combo_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	qmp_combo_disable_autonomous_mode(qmp);
> +	if (qmp->usb_init_count) {
> +		ret = clk_bulk_prepare_enable(qmp->num_usb_clks, qmp->usb_clks);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If clk_bulk_prepare_enable() fails here, the function returns early. Does
this error path need to clean up qmp->pipe_clk and qmp->clks that were
enabled earlier in qmp_combo_runtime_resume()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-qmp_phy_usb_clks_v1-v1-1-d4dccc9eb263@oss.qualcomm.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy