[v1] Support Multi-frequency scale for UFS

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

Posted by Ziqi Chen 2 weeks, 6 days ago

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;
+
+	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");
+		break;
+	}
+
+	return ret;
+}
+
 /*
  * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
  *
@@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
 	.op_runtime_config	= ufs_qcom_op_runtime_config,
 	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
 	.config_esi		= ufs_qcom_config_esi,
+	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
 };
 
 /**
-- 
2.34.1

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

Posted by Manivannan Sadhasivam 2 weeks, 3 days ago

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.

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

- Mani

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>   *
> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>  	.op_runtime_config	= ufs_qcom_op_runtime_config,
>  	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
>  	.config_esi		= ufs_qcom_config_esi,
> +	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
>  };
>  
>  /**
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

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

Posted by Ziqi Chen 2 weeks, 2 days ago

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.

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

> 
> - Mani >

-Ziqi

>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   /*
>>    * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>>    *
>> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>>   	.op_runtime_config	= ufs_qcom_op_runtime_config,
>>   	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
>>   	.config_esi		= ufs_qcom_config_esi,
>> +	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
>>   };
>>   
>>   /**
>> -- 
>> 2.34.1
>>
>

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

Posted by Manivannan Sadhasivam 2 weeks, 2 days ago

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.

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

-- 
மணிவண்ணன் சதாசிவம்

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

Posted by Ziqi Chen 2 weeks, 1 day ago


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?

>>>> +
>>>> +	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.
2. This prints will be print very frequent, I afraid it will increase 
the latency of clock scaling.


How do you think ?

-Ziqi

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

Posted by Manivannan Sadhasivam 1 week, 5 days ago

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

-- 
மணிவண்ணன் சதாசிவம்

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

Posted by Ziqi Chen 1 week, 5 days ago


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