diff mbox series

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

Message ID 20250116091150.1167739-5-quic_ziqichen@quicinc.com (mailing list archive)
State New
Headers show
Series Support Multi-frequency scale for UFS | expand

Commit Message

Ziqi Chen Jan. 16, 2025, 9:11 a.m. UTC 
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(+)

Comments

Avri Altman Jan. 16, 2025, 9:40 p.m. UTC | #1 
> 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) {
Maybe you can use here the UNIPRO_CORE_CLK_FREQ_xx ?

Thanks,
Avri
> +       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
Manivannan Sadhasivam Jan. 19, 2025, 7:30 a.m. UTC | #2 
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
>
Ziqi Chen Jan. 20, 2025, 12:04 p.m. UTC | #3 
Hi Avri,

Thanks for your review~

On 1/17/2025 5:40 AM, Avri Altman 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;
>> +
>> +       switch (freq) {
> Maybe you can use here the UNIPRO_CORE_CLK_FREQ_xx
The UNIPRO_CORE_CLK_FREQ_xx is used for "cycles_in_1us" which be handled 
by ceil() function.  It is not an exact frequency number and is not 
appropriate for use here.

> 
> Thanks,
> Avri

-Ziqi

>> +       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
>
Ziqi Chen Jan. 20, 2025, 12:07 p.m. UTC | #4 
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
>>
>
Manivannan Sadhasivam Jan. 20, 2025, 3:41 p.m. UTC | #5 
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
diff mbox series

Patch

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,
 };
 
 /**