Re: [PATCH 5/5] drivers: ufs: qcom: Initialize and enable clocks before hardware access

[Sumit Garg <sumit.garg@kernel.org>]

On Thu, Mar 19, 2026 at 03:07:42PM +0530, Balaji Selvanathan wrote:
> Move UFS clock initialization and enabling before hardware setup
> to ensure clocks are running when accessing UFS registers.
> 
> Previously, U-Boot depended on earlier bootloader stages to
> initialize UFS clocks. When these bootloaders failed to do so,
> UFS registers became inaccessible, causing initialization to fail.
> This change makes U-Boot initialize and enable UFS clocks early
> in the init sequence, removing the dependency on previous
> bootloaders.
> 
> Signed-off-by: Balaji Selvanathan <balaji.selvanathan@oss.qualcomm.com>
> ---
>  drivers/ufs/ufs-qcom.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ufs/ufs-qcom.c b/drivers/ufs/ufs-qcom.c
> index dc40ee62daf..c63e550c881 100644
> --- a/drivers/ufs/ufs-qcom.c
> +++ b/drivers/ufs/ufs-qcom.c
> @@ -30,6 +30,7 @@
>  #define UFS_CPU_MAX_BANDWIDTH	819200
>  
>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_hba *hba, bool enable);
> +static u32 ufs_qcom_get_core_clk_unipro_max_freq(struct ufs_hba *hba);
>  
>  static int ufs_qcom_enable_clks(struct ufs_qcom_priv *priv)
>  {
> @@ -49,12 +50,34 @@ static int ufs_qcom_enable_clks(struct ufs_qcom_priv *priv)
>  
>  static int ufs_qcom_init_clks(struct ufs_qcom_priv *priv)
>  {
> -	int err;
>  	struct udevice *dev = priv->hba->dev;
> +	struct clk clk;
> +	int err;
> +	long rate;
> +	u32 max_freq;
> +
> +	/* Get maximum frequency for core_clk_unipro from device tree */
> +	max_freq = ufs_qcom_get_core_clk_unipro_max_freq(priv->hba);
> +
> +	/* Get and configure core_clk_unipro */
> +	err = clk_get_by_name(dev, "core_clk_unipro", &clk);
> +	if (err) {
> +		dev_err(dev, "Failed to get core_clk_unipro: %d\n", err);
> +		return err;
> +	}
> +
> +	rate = clk_set_rate(&clk, max_freq);
> +
> +	if (rate < 0) {
> +		dev_err(dev, "Failed to set core_clk_unipro rate to %u Hz: %ld\n",
> +			max_freq, rate);
> +	}
>  
>  	err = clk_get_bulk(dev, &priv->clks);
> -	if (err)
> +	if (err) {
> +		dev_err(dev, "clk_get_bulk failed: %d\n", err);
>  		return err;
> +	}
>  
>  	return 0;
>  }
> @@ -561,6 +584,18 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  
>  	priv->hba = hba;
>  
> +	err = ufs_qcom_init_clks(priv);

I would just drop this wrapper and directly invoke clk_get_bulk() here.

> +	if (err) {
> +		dev_err(hba->dev, "failed to initialize clocks, err:%d\n", err);
> +		return err;
> +	}
> +
> +	err = ufs_qcom_enable_clks(priv);

There is already a comment related to this API:

/*
 * The PHY PLL output is the source of tx/rx lane symbol
 * clocks, hence, enable the lane clocks only after PHY
 * is initialized.
 */

Shouldn't PHY be initialized before clocks being enabled?

> +	if (err) {
> +		dev_err(hba->dev, "failed to enable clocks, err:%d\n", err);
> +		return err;
> +	}
> +
>  	/* setup clocks */
>  	ufs_qcom_setup_clocks(hba, true, PRE_CHANGE);
>  
> @@ -579,12 +614,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  		 priv->hw_ver.minor,
>  		 priv->hw_ver.step);
>  
> -	err = ufs_qcom_init_clks(priv);
> -	if (err) {
> -		dev_err(hba->dev, "failed to initialize clocks, err:%d\n", err);
> -		return err;
> -	}
> -
>  	ufs_qcom_advertise_quirks(hba);
>  	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);

While at it, I see this POST_CHANGE setup invoked twice. Can you check
as to why that is needed?

-Sumit