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 <[email protected]> >> --- >> 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 <[email protected]> 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
