Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen <quic_ziqichen@quicinc.com>]

On 1/24/2025 1:35 PM, Manivannan Sadhasivam wrote:
> On Tue, Jan 21, 2025 at 11:52:42AM +0800, Ziqi Chen wrote:
>>
>>
>> On 1/20/2025 11:41 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
>>>> Hi Mani，
>>>>
>>>> Thanks for your comments~
>>>>
>>>> On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
>>>>> On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
>>>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>>>
>>>>>> Implement the freq_to_gear_speed() vops to map the unipro core clock
>>>>>> frequency to the corresponding maximum supported gear speed.
>>>>>>
>>>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>>>> ---
>>>>>>     drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>>>> index 1e8a23eb8c13..64263fa884f5 100644
>>>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>>>> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>>>>>     	return ret;
>>>>>>     }
>>>>>> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
>>>>>> +{
>>>>>> +	int ret = 0 >
>>>>> Please do not initialize ret with 0. Return the actual value directly.
>>>>>
>>>>
>>>> If we don't initialize ret here, for the cases of freq matched in the table,
>>>> it will return an unknown ret value. It is not make sense, right?
>>>>
>>>> Or you may want to say we don't need “ret” , just need to return gear value?
>>>> But we need this "ret" to check whether the freq is invalid.
>>>>
>>>
>>> I meant to say that you can just return 0 directly in success case and -EINVAL
>>> in the case of error.
>>>
>> Hi Mani,
>>
>> If we don't print freq here , I think your suggestion is very good. If we
>> print freq in this function , using "ret" to indicate success case and
>> failure case and print freq an the end of this function is the way to avoid
>> code bloat.
>>
>> How do you think about it?
>>
> 
> I don't understand how code bloat comes into picture here. I'm just asking for
> this:
> 
> static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> {
> 	switch (freq) {
> 	case 403000000:
> 		*gear = UFS_HS_G5;
> 		break;
> 	...
> 
> 	default:
> 		dev_err(hba->dev, "Unsupported clock freq: %ld\n", freq);
> 		return -EINVAL;
> 	}
> 	
> 	return 0;
> }
> 
>>>>>> +
>>>>>> +	switch (freq) {
>>>>>> +	case 403000000:
>>>>>> +		*gear = UFS_HS_G5;
>>>>>> +		break;
>>>>>> +	case 300000000:
>>>>>> +		*gear = UFS_HS_G4;
>>>>>> +		break;
>>>>>> +	case 201500000:
>>>>>> +		*gear = UFS_HS_G3;
>>>>>> +		break;
>>>>>> +	case 150000000:
>>>>>> +	case 100000000:
>>>>>> +		*gear = UFS_HS_G2;
>>>>>> +		break;
>>>>>> +	case 75000000:
>>>>>> +	case 37500000:
>>>>>> +		*gear = UFS_HS_G1;
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		ret = -EINVAL;
>>>>>> +		dev_err(hba->dev, "Unsupported clock freq\n");
>>>>>
>>>>> Print the freq.
>>>>
>>>> Ok, thank for your suggestion, we can print freq with dev_dbg() in next
>>>> version.
>>>>
>>>
>>> No, use dev_err() with the freq. >
>>> - Mani
>>>
>> I think use dev_err() here does not make sense:
>>
>> 1. This print is not an error message , just an information print. Using
>> dev_err() reduces the readability of this code.
> 
> Then why it was dev_err() in the first place?
> 
>> 2. This prints will be print very frequent, I afraid it will increase the
>> latency of clock scaling.
>>
> 
> First you need to decide whether this print should warn user or not. It is
> telling users that the OPP table supplied a frequency that doesn't match the
> gear speed. This can happen if there is a discrepancy between DT and the driver.
> In that case, the users *should* be warned to fix the driver/DT. If you bury it
> with dev_dbg(), no one will notice it.
> 
> If your concern is with the frequency of logs, then use dev_err_ratelimited().
> 
> - Mani
> 
I misunderstand your point Mani, I thought you want me to print freq for 
all cases...  if you mean that print failure case, I already added it in 
patch V2.

-Ziqi