| Message ID | 20250507074415.2451940-3-quic_ziqichen@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bug fixes for UFS multi-frequency scaling on Qcom platform | expand |
Hi Ziqi, On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote: > From: Can Guo <quic_cang@quicinc.com> > > On some platforms, the devfreq OPP freq may be different than the unipro > core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to > find the unipro core clk freq. > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com> > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++----- > 1 file changed, 71 insertions(+), 10 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 7f10926100a5..804c8ccd8d03 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -122,7 +122,9 @@ static const struct { > }; > > static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); > -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq); > +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, unsigned long freq, > + char *name); > +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq); > > static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd) > { > @@ -656,7 +658,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba, > return -EINVAL; > } > > - err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX); > + err = ufs_qcom_set_core_clk_ctrl(hba, true, ULONG_MAX); > if (err) > dev_err(hba->dev, "cfg core clk ctrl failed\n"); > /* > @@ -1414,29 +1416,46 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba, > return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg); > } > > -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq) > +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq) > { > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > struct list_head *head = &hba->clk_list_head; > struct ufs_clk_info *clki; > u32 cycles_in_1us = 0; > u32 core_clk_ctrl_reg; > + unsigned long clk_freq; > int err; > > + if (hba->use_pm_opp) { > + clk_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro"); > + if (clk_freq) { > + cycles_in_1us = ceil(clk_freq, HZ_PER_MHZ); > + goto set_core_clk_ctrl; > + } > + } > + > list_for_each_entry(clki, head, list) { > if (!IS_ERR_OR_NULL(clki->clk) && > !strcmp(clki->name, "core_clk_unipro")) { > - if (!clki->max_freq) > + if (!clki->max_freq) { > cycles_in_1us = 150; /* default for backwards compatibility */ > - else if (freq == ULONG_MAX) > + break; > + } > + > + if (freq == ULONG_MAX) { > cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ); > - else > - cycles_in_1us = ceil(freq, HZ_PER_MHZ); > + break; > + } > > + if (is_scale_up) > + cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ); > + else > + cycles_in_1us = ceil(clk_get_rate(clki->clk), HZ_PER_MHZ); > break; > } > } > > +set_core_clk_ctrl: > err = ufshcd_dme_get(hba, > UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), > &core_clk_ctrl_reg); > @@ -1479,7 +1498,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f > return ret; > } > /* set unipro core clock attributes and clear clock divider */ > - return ufs_qcom_set_core_clk_ctrl(hba, freq); > + return ufs_qcom_set_core_clk_ctrl(hba, true, freq); > } > > static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba) > @@ -1511,7 +1530,7 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba) > static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba, unsigned long freq) > { > /* set unipro core clock attributes and clear clock divider */ > - return ufs_qcom_set_core_clk_ctrl(hba, freq); > + return ufs_qcom_set_core_clk_ctrl(hba, false, freq); > } > > static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up, > @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) > return ret; > } > > +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, > + unsigned long freq, char *name) > +{ > + struct ufs_clk_info *clki; > + struct dev_pm_opp *opp; > + unsigned long clk_freq; > + int idx = 0; > + bool found = false; > + > + opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true); > + if (IS_ERR(opp)) { > + dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq); I'm hitting this print on bootup: [ 0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615 [ 0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615 Doesn't look like it's intended? The number is (2^64 - 1) Regards Luca > + return 0; > + } > + > + list_for_each_entry(clki, &hba->clk_list_head, list) { > + if (!strcmp(clki->name, name)) { > + found = true; > + break; > + } > + > + idx++; > + } > + > + if (!found) { > + dev_err(hba->dev, "Failed to find clock '%s' in clk list\n", name); > + dev_pm_opp_put(opp); > + return 0; > + } > + > + clk_freq = dev_pm_opp_get_freq_indexed(opp, idx); > + > + dev_pm_opp_put(opp); > + > + return clk_freq; > +} > + > static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq) > { > u32 gear = 0; > + unsigned long unipro_freq; > + > + if (!hba->use_pm_opp) > + return gear; > > - switch (freq) { > + unipro_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro"); > + switch (unipro_freq) { > case 403000000: > gear = UFS_HS_G5; > break;
On Wed May 7, 2025 at 11:09 AM CEST, Ziqi Chen wrote: > Hi Luca, > > On 5/7/2025 4:19 PM, Luca Weiss wrote: >> Hi Ziqi, >> >> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote: >>> From: Can Guo <quic_cang@quicinc.com> >>> >>> On some platforms, the devfreq OPP freq may be different than the unipro >>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to >>> find the unipro core clk freq. >>> >>> Signed-off-by: Can Guo <quic_cang@quicinc.com> >>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com> >>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> >>> --- >>> drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++----- >>> 1 file changed, 71 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>> index 7f10926100a5..804c8ccd8d03 100644 >>> --- a/drivers/ufs/host/ufs-qcom.c >>> +++ b/drivers/ufs/host/ufs-qcom.c >>> >>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, >>> + unsigned long freq, char *name) >>> +{ >>> + struct ufs_clk_info *clki; >>> + struct dev_pm_opp *opp; >>> + unsigned long clk_freq; >>> + int idx = 0; >>> + bool found = false; >>> + >>> + opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true); >>> + if (IS_ERR(opp)) { >>> + dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq); >> >> I'm hitting this print on bootup: >> >> [ 0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615 >> [ 0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615 >> >> Doesn't look like it's intended? The number is (2^64 - 1) >> > Yes, this is expected. During link startup, the frequency > ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and > ufs_qcom_cfg_timer(). This frequency cannot be found through the API > dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the > frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl() > and ufs_qcom_cfg_timer(). > > This print only be print twice during link startup. If you think print > such print during bootup is not make sense, I can improve the code and > update a new vwesion. I'll let others comment on what should happen but certainly this large number looks more like a mistake, like an integer overflow, if you don't dig into what this number is supposed to represent. Perhaps an idea could be to just skip the print (or even more code) for ULONG_MAX since an opp for that is not supposed to exist anyways? I didn't check the code now but for other frequencies this would be an actual error I imagine where it should be visible. Regards Luca > > BRs. > Ziqi > >> Regards >> Luca >>
On Wed, 2025-05-07 at 15:44 +0800, Ziqi Chen wrote: > - return ufs_qcom_set_core_clk_ctrl(hba, freq); > + return ufs_qcom_set_core_clk_ctrl(hba, false, freq); > } > > static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up, > @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) > return ret; > } > > +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, > + unsigned long freq, char *name) This tab indentation is strange! > +{ > + struct ufs_clk_info *clki
On 5/7/2025 7:59 PM, neil.armstrong@linaro.org wrote: > On 07/05/2025 11:09, Ziqi Chen wrote: >> Hi Luca, >> >> On 5/7/2025 4:19 PM, Luca Weiss wrote: >>> Hi Ziqi, >>> >>> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote: >>>> From: Can Guo <quic_cang@quicinc.com> >>>> >>>> On some platforms, the devfreq OPP freq may be different than the >>>> unipro >>>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use >>>> it to >>>> find the unipro core clk freq. >>>> >>>> Signed-off-by: Can Guo <quic_cang@quicinc.com> >>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com> >>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> >>>> --- >>>> drivers/ufs/host/ufs-qcom.c | 81 +++++++++++++++++++++++++++++++ >>>> +----- >>>> 1 file changed, 71 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >>>> index 7f10926100a5..804c8ccd8d03 100644 >>>> --- a/drivers/ufs/host/ufs-qcom.c >>>> +++ b/drivers/ufs/host/ufs-qcom.c >>>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba >>>> *hba, >>>> + unsigned long >>>> freq, char *name) >>>> +{ >>>> + struct ufs_clk_info *clki; >>>> + struct dev_pm_opp *opp; >>>> + unsigned long clk_freq; >>>> + int idx = 0; >>>> + bool found = false; >>>> + >>>> + opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true); >>>> + if (IS_ERR(opp)) { >>>> + dev_err(hba->dev, "Failed to find OPP for exact frequency >>>> %lu\n", freq); >>> >>> I'm hitting this print on bootup: >>> >>> [ 0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for >>> exact frequency 18446744073709551615 >>> [ 0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for >>> exact frequency 18446744073709551615 >>> >>> Doesn't look like it's intended? The number is (2^64 - 1) >>> >> Yes, this is expected. During link startup, the frequency >> ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and >> ufs_qcom_cfg_timer(). This frequency cannot be found through the API >> dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the >> frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl() >> and ufs_qcom_cfg_timer(). >> >> This print only be print twice during link startup. If you think print >> such print during bootup is not make sense, I can improve the code and >> update a new vwesion. > > I think just don't call ufs_qcom_opp_freq_to_clk_freq() if freq==ULONG_MAX > Thanks Neil, yes, it is a way , let me discuss internally and update new version. BRs, Ziqi > Neil > >> >> BRs. >> Ziqi >> >>> Regards >>> Luca >>> >> >> >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 7f10926100a5..804c8ccd8d03 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -122,7 +122,9 @@ static const struct { }; static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq); +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, unsigned long freq, + char *name); +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq); static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd) { @@ -656,7 +658,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba, return -EINVAL; } - err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX); + err = ufs_qcom_set_core_clk_ctrl(hba, true, ULONG_MAX); if (err) dev_err(hba->dev, "cfg core clk ctrl failed\n"); /* @@ -1414,29 +1416,46 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba, return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg); } -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq) +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); struct list_head *head = &hba->clk_list_head; struct ufs_clk_info *clki; u32 cycles_in_1us = 0; u32 core_clk_ctrl_reg; + unsigned long clk_freq; int err; + if (hba->use_pm_opp) { + clk_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro"); + if (clk_freq) { + cycles_in_1us = ceil(clk_freq, HZ_PER_MHZ); + goto set_core_clk_ctrl; + } + } + list_for_each_entry(clki, head, list) { if (!IS_ERR_OR_NULL(clki->clk) && !strcmp(clki->name, "core_clk_unipro")) { - if (!clki->max_freq) + if (!clki->max_freq) { cycles_in_1us = 150; /* default for backwards compatibility */ - else if (freq == ULONG_MAX) + break; + } + + if (freq == ULONG_MAX) { cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ); - else - cycles_in_1us = ceil(freq, HZ_PER_MHZ); + break; + } + if (is_scale_up) + cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ); + else + cycles_in_1us = ceil(clk_get_rate(clki->clk), HZ_PER_MHZ); break; } } +set_core_clk_ctrl: err = ufshcd_dme_get(hba, UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), &core_clk_ctrl_reg); @@ -1479,7 +1498,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f return ret; } /* set unipro core clock attributes and clear clock divider */ - return ufs_qcom_set_core_clk_ctrl(hba, freq); + return ufs_qcom_set_core_clk_ctrl(hba, true, freq); } static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba) @@ -1511,7 +1530,7 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba) static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba, unsigned long freq) { /* set unipro core clock attributes and clear clock divider */ - return ufs_qcom_set_core_clk_ctrl(hba, freq); + return ufs_qcom_set_core_clk_ctrl(hba, false, freq); } static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up, @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) return ret; } +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, + unsigned long freq, char *name) +{ + struct ufs_clk_info *clki; + struct dev_pm_opp *opp; + unsigned long clk_freq; + int idx = 0; + bool found = false; + + opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true); + if (IS_ERR(opp)) { + dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq); + return 0; + } + + list_for_each_entry(clki, &hba->clk_list_head, list) { + if (!strcmp(clki->name, name)) { + found = true; + break; + } + + idx++; + } + + if (!found) { + dev_err(hba->dev, "Failed to find clock '%s' in clk list\n", name); + dev_pm_opp_put(opp); + return 0; + } + + clk_freq = dev_pm_opp_get_freq_indexed(opp, idx); + + dev_pm_opp_put(opp); + + return clk_freq; +} + static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq) { u32 gear = 0; + unsigned long unipro_freq; + + if (!hba->use_pm_opp) + return gear; - switch (freq) { + unipro_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro"); + switch (unipro_freq) { case 403000000: gear = UFS_HS_G5; break;