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

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

[Wesley Cheng]

The Qualcomm QMP combo PHY has a USB and Displayport block that is
sourced with different clocks.  In situations where the USB path is not
utilized, and only Displayport is active, unnecessary clocks are enabled
and not used.  Split the current clock list into USB specific clocks,
which will be enabled whenever the USB initialization routine is
executed.

In addition, move autonomous mode control specifically when USB is
active, as its primary purpose is to detect RX detection and LFPS
signals, which occurs only when USB is active.

Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 56 ++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index a4f130fc33e3..f14fb8ef9028 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2607,6 +2607,8 @@ struct qmp_combo {
 	struct clk_bulk_data *clks;
 	struct clk *p2rr2p_pipe_clk;
 	int num_clks;
+	struct clk_bulk_data *usb_clks;
+	int num_usb_clks;
 	struct reset_control_bulk_data *resets;
 	struct regulator_bulk_data *vregs;
 
@@ -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",
 };
 
 /* list of resets */
@@ -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;
 
+	ret = qmp_combo_com_init(qmp, false);
+	if (ret) {
+		clk_bulk_disable_unprepare(qmp->num_usb_clks, qmp->usb_clks);
+		return ret;
+	}
+
 	ret = qmp_combo_usb_power_on(phy);
 	if (ret) {
 		qmp_combo_com_exit(qmp, false);
+		clk_bulk_disable_unprepare(qmp->num_usb_clks, qmp->usb_clks);
 		return ret;
 	}
 
@@ -4440,6 +4453,8 @@ static int qmp_combo_usb_exit(struct phy *phy)
 	if (ret)
 		return ret;
 
+	clk_bulk_disable_unprepare(qmp->num_usb_clks, qmp->usb_clks);
+
 	qmp->usb_init_count--;
 
 	return 0;
@@ -4670,7 +4685,10 @@ static int __maybe_unused qmp_combo_runtime_suspend(struct device *dev)
 		return 0;
 	}
 
-	qmp_combo_enable_autonomous_mode(qmp);
+	if (qmp->usb_init_count) {
+		qmp_combo_enable_autonomous_mode(qmp);
+		clk_bulk_disable_unprepare(qmp->num_usb_clks, qmp->usb_clks);
+	}
 
 	clk_disable_unprepare(qmp->pipe_clk);
 	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
@@ -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;
+		qmp_combo_disable_autonomous_mode(qmp);
+	}
 
 	return 0;
 }
@@ -4736,19 +4759,34 @@ static int qmp_combo_reset_init(struct qmp_combo *qmp)
 static int qmp_combo_clk_init(struct qmp_combo *qmp)
 {
 	struct device *dev = qmp->dev;
-	int num = ARRAY_SIZE(qmp_combo_phy_clk_l);
+	int num_clks = ARRAY_SIZE(qmp_combo_phy_clk_l);
+	int num_usb_clks = ARRAY_SIZE(qmp_combo_usb_phy_clk_l);
 	int i;
+	int ret;
 
-	qmp->clks = devm_kcalloc(dev, num, sizeof(*qmp->clks), GFP_KERNEL);
+	qmp->clks = devm_kcalloc(dev, num_clks, sizeof(*qmp->clks), GFP_KERNEL);
 	if (!qmp->clks)
 		return -ENOMEM;
 
-	for (i = 0; i < num; i++)
+	for (i = 0; i < num_clks; i++)
 		qmp->clks[i].id = qmp_combo_phy_clk_l[i];
 
-	qmp->num_clks = num;
+	qmp->num_clks = num_clks;
+
+	qmp->usb_clks = devm_kcalloc(dev, num_usb_clks, sizeof(*qmp->usb_clks), GFP_KERNEL);
+	if (!qmp->usb_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < num_usb_clks; i++)
+		qmp->usb_clks[i].id = qmp_combo_usb_phy_clk_l[i];
+
+	qmp->num_usb_clks = num_usb_clks;
+
+	ret = devm_clk_bulk_get_optional(dev, num_clks, qmp->clks);
+	if (ret)
+		return ret;
 
-	return devm_clk_bulk_get_optional(dev, num, qmp->clks);
+	return devm_clk_bulk_get_optional(dev, num_usb_clks, qmp->usb_clks);
 }
 
 static void phy_clk_release_provider(void *res)

---
base-commit: c7f2322431cb6d108b18fb4154606b49e2bc50f7
change-id: 20260930-qmp_phy_usb_clks_v1-25b9374086b2

Best regards,
--  
Wesley Cheng <wesley.cheng@oss.qualcomm.com>


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

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

[sashiko-bot]

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