Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

["Luca Weiss" <luca.weiss@fairphone.com>]

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