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

[Nitin Rawat]

On 7/14/2026 12:41 PM, Konrad Dybcio wrote:
> On 7/13/26 7:34 PM, 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 framewore.

Apologies for the delayed response. Reviving this thread.


> > Signed-off-by: Nitin Rawat <nitin.rawat@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/ufs/host/ufs-qcom.c | 41 ++++++++++++++++++++++++++++++++-----
> >   drivers/ufs/host/ufs-qcom.h |  3 +++
> >   2 files changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index 291c43448764..7ec1626704c6 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -347,7 +347,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);
>
> I don't know if this matters (probably not since IIUC the controller
> would be suspended already), but downstream disables them in the
> following order:
>
> TX_L1
> TX_L0
> RX_L1
> RX_L0

Yes, order shouldn't matter as controller is in suspend state.

> >   	host->is_lane_clks_enabled = false;
> >   }
> > @@ -356,18 +358,35 @@ 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;
> >
> > -	host->is_lane_clks_enabled = true;
> > +	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;
>
> Let's keep the \n above return
Sure, will take care in next patchset.
> > +
> > +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;
> > +	int err, i;
> >   	struct device *dev = host->hba->dev;
> >
> >   	if (has_acpi_companion(dev))
> > @@ -379,6 +398,18 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
> >
> >   	host->num_clks = err;
> >
> > +	for (i = 0; i < host->num_clks; i++) {
> > +		if (!host->clks[i].id)
> > +			continue;
> > +		if (!strcmp(host->clks[i].id, "tx_lane0_sync_clk"))
> > +			host->tx_lane0_sync_clk = host->clks[i].clk;
> > +		else if (!strcmp(host->clks[i].id, "rx_lane0_sync_clk"))
> > +			host->rx_lane0_sync_clk = host->clks[i].clk;
> > +		else if (!strcmp(host->clks[i].id, "rx_lane1_sync_clk"))
> > +			if (host->hba->lanes_per_direction > 1)
> > +				host->rx_lane1_sync_clk = host->clks[i].clk;
> > +	}
>
> This is technically probably a little faster than calling
> clk_get() again, but please do that instead

Sure, will take care in next patchset.

> Konrad