[PATCH V2] scsi: ufs: ufs-qcom: Enable only lane clocks in lane clock APIs

[PATCH V2] scsi: ufs: ufs-qcom: Enable only lane clocks in lane clock APIs

[Nitin Rawat]

ufs_qcom_enable_lane_clks() and ufs_qcom_disable_lane_clks() currently
use clk_bulk_prepare_enable()/clk_bulk_disable_unprepare() on the
entire host->clks array obtained from devm_clk_bulk_get_all(). This
array contains all device clocks, not just lane symbol clocks.

Since the UFS core framework already manages the non-lane clocks via
the setup_clocks callback, the bulk enable/disable in the lane clock
APIs resulted in duplicate reference count increments on those shared
clocks. The extra enable counts were never balanced by a corresponding
disable from the framework's clock gating path, preventing the clock
reference counts from reaching zero and ultimately blocking CXO
shutdown during low-power states.

Fix this by restricting the lane clock APIs to only prepare/enable
and disable/unprepare the three lane symbol clocks (tx_lane0_sync_clk,
rx_lane0_sync_clk, rx_lane1_sync_clk), leaving the handling of all
other clocks to the UFS core framework. The lane clocks are now
acquired individually via devm_clk_get() instead of being looked up
in the bulk clock array.

Signed-off-by: Nitin Rawat <nitin.rawat@oss.qualcomm.com>
---
Changes from v1:
1. As per konrad's comment, used devm_clk_get instead of to get
   lane clock handle instead of using bulk call API.
---
 drivers/ufs/host/ufs-qcom.c | 47 ++++++++++++++++++++++++++++++-------
 drivers/ufs/host/ufs-qcom.h |  5 ++--
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index aa2ac2cd2b69..50f34d1c5fdb 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -348,7 +348,9 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
 	if (!host->is_lane_clks_enabled)
 		return;

-	clk_bulk_disable_unprepare(host->num_clks, host->clks);
+	clk_disable_unprepare(host->rx_lane1_sync_clk);
+	clk_disable_unprepare(host->rx_lane0_sync_clk);
+	clk_disable_unprepare(host->tx_lane0_sync_clk);

 	host->is_lane_clks_enabled = false;
 }
@@ -357,28 +359,57 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
 {
 	int err;

-	err = clk_bulk_prepare_enable(host->num_clks, host->clks);
+	if (host->is_lane_clks_enabled)
+		return 0;
+
+	err = clk_prepare_enable(host->tx_lane0_sync_clk);
 	if (err)
-		return err;
+		goto out;
+
+	err = clk_prepare_enable(host->rx_lane0_sync_clk);
+	if (err)
+		goto out_disable_tx_lane0;
+
+	err = clk_prepare_enable(host->rx_lane1_sync_clk);
+	if (err)
+		goto out_disable_rx_lane0;

 	host->is_lane_clks_enabled = true;

 	return 0;
+
+out_disable_rx_lane0:
+	clk_disable_unprepare(host->rx_lane0_sync_clk);
+out_disable_tx_lane0:
+	clk_disable_unprepare(host->tx_lane0_sync_clk);
+out:
+	return err;
 }

 static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
 {
-	int err;
 	struct device *dev = host->hba->dev;

 	if (has_acpi_companion(dev))
 		return 0;

-	err = devm_clk_bulk_get_all(dev, &host->clks);
-	if (err <= 0)
-		return err;
+	host->tx_lane0_sync_clk = devm_clk_get(dev, "tx_lane0_sync_clk");
+	if (IS_ERR(host->tx_lane0_sync_clk))
+		return dev_err_probe(dev, PTR_ERR(host->tx_lane0_sync_clk),
+				     "failed to get tx_lane0_sync_clk\n");
+
+	host->rx_lane0_sync_clk = devm_clk_get(dev, "rx_lane0_sync_clk");
+	if (IS_ERR(host->rx_lane0_sync_clk))
+		return dev_err_probe(dev, PTR_ERR(host->rx_lane0_sync_clk),
+				     "failed to get rx_lane0_sync_clk\n");

-	host->num_clks = err;
+	/* In case of single lane per direction, don't read lane1 clocks */
+	if (host->hba->lanes_per_direction > 1) {
+		host->rx_lane1_sync_clk = devm_clk_get(dev, "rx_lane1_sync_clk");
+		if (IS_ERR(host->rx_lane1_sync_clk))
+			return dev_err_probe(dev, PTR_ERR(host->rx_lane1_sync_clk),
+					     "failed to get rx_lane1_sync_clk\n");
+	}

 	return 0;
 }
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index e20b3ca50577..37f6e88b0adf 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -330,8 +330,9 @@ struct ufs_qcom_host {
 	struct phy *generic_phy;
 	struct ufs_hba *hba;
 	struct ufs_pa_layer_attr dev_req_params;
-	struct clk_bulk_data *clks;
-	u32 num_clks;
+	struct clk *tx_lane0_sync_clk;
+	struct clk *rx_lane0_sync_clk;
+	struct clk *rx_lane1_sync_clk;
 	bool is_lane_clks_enabled;

 	struct icc_path *icc_ddr;
--
2.34.1

Re: [PATCH V2] scsi: ufs: ufs-qcom: Enable only lane clocks in lane clock APIs

[Manivannan Sadhasivam]

On Wed, Sep 09, 2026 at 11:09:44AM +0530, Nitin Rawat wrote:
> ufs_qcom_enable_lane_clks() and ufs_qcom_disable_lane_clks() currently
> use clk_bulk_prepare_enable()/clk_bulk_disable_unprepare() on the
> entire host->clks array obtained from devm_clk_bulk_get_all(). This
> array contains all device clocks, not just lane symbol clocks.
> 
> Since the UFS core framework already manages the non-lane clocks via
> the setup_clocks callback, the bulk enable/disable in the lane clock
> APIs resulted in duplicate reference count increments on those shared
> clocks. The extra enable counts were never balanced by a corresponding
> disable from the framework's clock gating path, preventing the clock
> reference counts from reaching zero and ultimately blocking CXO
> shutdown during low-power states.
> 
> Fix this by restricting the lane clock APIs to only prepare/enable
> and disable/unprepare the three lane symbol clocks (tx_lane0_sync_clk,
> rx_lane0_sync_clk, rx_lane1_sync_clk), leaving the handling of all
> other clocks to the UFS core framework. The lane clocks are now
> acquired individually via devm_clk_get() instead of being looked up
> in the bulk clock array.
> 
> Signed-off-by: Nitin Rawat <nitin.rawat@oss.qualcomm.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

One comment below for future optimization.

> ---
> Changes from v1:
> 1. As per konrad's comment, used devm_clk_get instead of to get
>    lane clock handle instead of using bulk call API.
> ---
>  drivers/ufs/host/ufs-qcom.c | 47 ++++++++++++++++++++++++++++++-------
>  drivers/ufs/host/ufs-qcom.h |  5 ++--
>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index aa2ac2cd2b69..50f34d1c5fdb 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -348,7 +348,9 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
>  	if (!host->is_lane_clks_enabled)
>  		return;
> 
> -	clk_bulk_disable_unprepare(host->num_clks, host->clks);
> +	clk_disable_unprepare(host->rx_lane1_sync_clk);
> +	clk_disable_unprepare(host->rx_lane0_sync_clk);
> +	clk_disable_unprepare(host->tx_lane0_sync_clk);
> 
>  	host->is_lane_clks_enabled = false;
>  }
> @@ -357,28 +359,57 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
>  {
>  	int err;
> 
> -	err = clk_bulk_prepare_enable(host->num_clks, host->clks);
> +	if (host->is_lane_clks_enabled)
> +		return 0;

Presence of this check/flag gives an indication that lane_clocks are not
handled in a refcounted manner i.e., ufs_qcom_{enable/disabled}_lane_clks() are
called multiple times from different places and the code only ensures that these
are called only once. As a future optimization, I would recommend removing this
check and ensure that enable/disable are called in a refcounted manner. Since
the clk framework handles refcount, the UFS driver's job is simply to ensure
the enable/disable count matches to avoid under/over flows.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V2] scsi: ufs: ufs-qcom: Enable only lane clocks in lane clock APIs

[Martin K. Petersen (Oracle)]

Nitin,

> ufs_qcom_enable_lane_clks() and ufs_qcom_disable_lane_clks() currently
> use clk_bulk_prepare_enable()/clk_bulk_disable_unprepare() on the
> entire host->clks array obtained from devm_clk_bulk_get_all(). This
> array contains all device clocks, not just lane symbol clocks.

Applied to 7.4/scsi-staging, thanks!

-- 
Martin K. Petersen