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

[Balaji Selvanathan <balaji.selvanathan@oss.qualcomm.com>]

On 3/24/2026 12:19 PM, Sumit Garg wrote:
> 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.

Hi Sumit,

Implemented this in v2: 
https://lore.kernel.org/u-boot/20260427-ufs_clk-v2-6-36e10a7c0ef6@oss.qualcomm.com/

>
>> +	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?

We can enable CBCRs of the tx/rx clks before we initiatize the PHYs. 
Once the PHY is turned on, these clks are enabled automatically.

The same process is followed in XBL/UEFI codes.

>
>> +	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?

Implemented this in v2: 
https://lore.kernel.org/u-boot/20260427-ufs_clk-v2-7-36e10a7c0ef6@oss.qualcomm.com/

Thanks,

Balaji

>
> -Sumit