[PATCH 2/2] ufs: Add bRefClkFreq attribute setting

[Jared McArthur]

On 10/9/25 10:57, Kumar, Udit wrote:
> 
> On 8/28/2025 12:20 AM, Jared McArthur wrote:
>> A UFS device needs its bRefClkFreq attribute set to the correct value
>> before switching to high speed. If bRefClkFreq is set to the wrong
>> value, all transactions after the power mode change will fail.
>>
>> The bRefClkFreq depends on the host controller and the device.
>> Query the device's current bRefClkFreq and compare with the ref_clk
>> specified in the device-tree. If the two differ, set the bRefClkFreq
>> to the device-tree's ref_clk frequency.
>>
>> Taken from Linux kernel v6.17 (drivers/ufs/core/ufshcd.c and
>> include/ufs/ufs.h) and ported to U-Boot.
> 
>> This patch depends on the previous patch: "ufs: Add support for
>> sending UFS attribute requests"
> 
> Since this is same series, you can drop above from commit message
> 

Will do.

> 
>> Signed-off-by: Jared McArthur <j-mcarthur at ti.com>
>> ---
>>   drivers/ufs/ufs.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/ufs/ufs.h | 10 ++++++
>>   2 files changed, 99 insertions(+)
>>
>> diff --git a/drivers/ufs/ufs.c b/drivers/ufs/ufs.c
>> index 9e67dd86232..4bffbf87749 100644
>> --- a/drivers/ufs/ufs.c
>> +++ b/drivers/ufs/ufs.c
>> @@ -10,6 +10,7 @@
>>     #include <bouncebuf.h>
>>   #include <charset.h>
>> +#include <clk.h>
>>   #include <dm.h>
>>   #include <log.h>
>>   #include <dm/device_compat.h>
>> @@ -1773,6 +1774,92 @@ out:
>>       return err;
>>   }
>>   +struct ufs_ref_clk {
>> +    unsigned long freq_hz;
>> +    enum ufs_ref_clk_freq val;
>> +};
>> +
>> +static const struct ufs_ref_clk ufs_ref_clk_freqs[] = {
>> +    {19200000, REF_CLK_FREQ_19_2_MHZ},
>> +    {26000000, REF_CLK_FREQ_26_MHZ},
>> +    {38400000, REF_CLK_FREQ_38_4_MHZ},
>> +    {52000000, REF_CLK_FREQ_52_MHZ},
>> +    {0, REF_CLK_FREQ_INVAL},
>> +};
>> +
>> +static enum ufs_ref_clk_freq
>> +ufs_get_bref_clk_from_hz(unsigned long freq)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; ufs_ref_clk_freqs[i].freq_hz; i++)
>> +        if (ufs_ref_clk_freqs[i].freq_hz == freq)
>> +            return ufs_ref_clk_freqs[i].val;
>> +
>> +    return REF_CLK_FREQ_INVAL;
>> +}
>> +
>> +void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk)
>> +{
>> +    unsigned long freq;
>> +
>> +    freq = clk_get_rate(refclk);
>> +
>> +    hba->dev_ref_clk_freq =
>> +        ufs_get_bref_clk_from_hz(freq);
> 
> I understand this is backport but  I don't see real use of dev_ref_clk_freq variable ,
> 
> I suggest to drop dev_ref_clk_freq varaible and return freq index from this function instead of void.
> 

Noted.

> 
>> +
>> +    if (hba->dev_ref_clk_freq == REF_CLK_FREQ_INVAL)
>> +        dev_err(hba->dev,
>> +            "invalid ref_clk setting = %ld\n", freq);
>> +}
>> +
>> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>> +{
>> +    int err;
>> +    struct clk *ref_clk;
>> +    u32 host_ref_clk_freq;
>> +    u32 dev_ref_clk_freq;
>> +
>> +    /* get ref_clk */
>> +    ref_clk = devm_clk_get(hba->dev, "ref_clk");
>> +    if (IS_ERR((const void *)ref_clk)) {
>> +        err = PTR_ERR(ref_clk);
>> +        goto out;
>> +    }
>> +
>> +    ufshcd_parse_dev_ref_clk_freq(hba, ref_clk);
>> +    host_ref_clk_freq = hba->dev_ref_clk_freq;
> 
> host_ref_clk_freq = ufshcd_parse_dev_ref_clk_freq(hba, ref_clk);
> 
> and check for error code here
> 
> If you agree with above change, then please use
> 
> Reviewed-by: Udit Kumar <u-kumar1 at ti.com>  in next version
> 
> 

I think the two edits (one change) are reasonable.

>> +
>> +    if (host_ref_clk_freq == REF_CLK_FREQ_INVAL)
>> +        goto out;
>> +
>> +    err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +                      QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &dev_ref_clk_freq);
>> +
>> +    if (err) {
>> +        dev_err(hba->dev, "failed reading bRefClkFreq. err = %d\n", err);
>> +        goto out;
>> +    }
>> +
>> +    if (dev_ref_clk_freq == host_ref_clk_freq)
>> +        goto out; /* nothing to update */
>> +
>> +    err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +                      QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &host_ref_clk_freq);
>> +
>> +    if (err) {
>> +        dev_err(hba->dev, "bRefClkFreq setting to %lu Hz failed\n",
>> +            ufs_ref_clk_freqs[host_ref_clk_freq].freq_hz);
>> +        goto out;
>> +    }
>> +
>> +    dev_dbg(hba->dev, "bRefClkFreq setting to %lu Hz succeeded\n",
>> +        ufs_ref_clk_freqs[host_ref_clk_freq].freq_hz);
>> +
>> +out:
>> +    return err;
>> +}
>> +
>>   /**
>>    * ufshcd_get_max_pwr_mode - reads the max power mode negotiated with device
>>    */
>> @@ -2000,6 +2087,8 @@ static int ufs_start(struct ufs_hba *hba)
>>           return ret;
>>       }
>>   +    ufshcd_set_dev_ref_clk(hba);
>> +
>>       if (ufshcd_get_max_pwr_mode(hba)) {
>>           dev_err(hba->dev,
>>               "%s: Failed getting max supported power mode\n",
>> diff --git a/drivers/ufs/ufs.h b/drivers/ufs/ufs.h
>> index 53137fae3a8..def39d4ad24 100644
>> --- a/drivers/ufs/ufs.h
>> +++ b/drivers/ufs/ufs.h
>> @@ -172,6 +172,15 @@ enum query_opcode {
>>       UPIU_QUERY_OPCODE_TOGGLE_FLAG    = 0x8,
>>   };
>>   +/* bRefClkFreq attribute values */
>> +enum ufs_ref_clk_freq {
>> +    REF_CLK_FREQ_19_2_MHZ    = 0,
>> +    REF_CLK_FREQ_26_MHZ    = 1,
>> +    REF_CLK_FREQ_38_4_MHZ    = 2,
>> +    REF_CLK_FREQ_52_MHZ    = 3,
>> +    REF_CLK_FREQ_INVAL    = -1,
>> +};
>> +
>>   /* Query response result code */
>>   enum {
>>       QUERY_RESULT_SUCCESS                    = 0x00,
>> @@ -684,6 +693,7 @@ struct ufs_hba {
>>       u32            capabilities;
>>       u32            version;
>>       u32            intr_mask;
>> +    enum ufs_ref_clk_freq dev_ref_clk_freq;
> 
>>       enum ufshcd_quirks    quirks;
>>         /* Virtual memory reference */

-- 
Best,
Jared McArthur