diff mbox series

[2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

Message ID 20250116091150.1167739-3-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>

If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
two freqs. In the case of scaling up, the devfreq may decide to scale the
clock to an intermidiate freq base on load, but the clock scale up pre
change operation uses settings for the max clock freq unconditionally. Fix
it by passing the target_freq to clock scale up pre change so that the
correct settings for the target_freq can be used.

In the case of scaling down, the clock scale down post change operation
is doing fine, because it reads the actual clock rate to tell freq, but to
keep symmetry with clock scale up pre change operation, just use the
target_freq instead of reading clock rate.

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 | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Manivannan Sadhasivam Jan. 19, 2025, 7:22 a.m. UTC | #1 
On Thu, Jan 16, 2025 at 05:11:43PM +0800, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
> two freqs.

Same comment as previous patch.

> In the case of scaling up, the devfreq may decide to scale the
> clock to an intermidiate freq base on load, but the clock scale up pre
> change operation uses settings for the max clock freq unconditionally. Fix
> it by passing the target_freq to clock scale up pre change so that the
> correct settings for the target_freq can be used.
> 
> In the case of scaling down, the clock scale down post change operation
> is doing fine, because it reads the actual clock rate to tell freq, but to
> keep symmetry with clock scale up pre change operation, just use the
> target_freq instead of reading clock rate.
> 
> 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 | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index b6eef975dc46..1e8a23eb8c13 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -97,7 +97,7 @@ static const struct __ufs_qcom_bw_table {
>  };
>  
>  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, bool is_scale_up);
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
>  
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -524,7 +524,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  			return -EINVAL;
>  		}
>  
> -		err = ufs_qcom_set_core_clk_ctrl(hba, true);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
>  		if (err)
>  			dev_err(hba->dev, "cfg core clk ctrl failed\n");
>  		/*
> @@ -1231,7 +1231,7 @@ 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, bool is_scale_up)
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct list_head *head = &hba->clk_list_head;
> @@ -1245,10 +1245,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>  		    !strcmp(clki->name, "core_clk_unipro")) {
>  			if (!clki->max_freq)
>  				cycles_in_1us = 150; /* default for backwards compatibility */
> -			else if (is_scale_up)
> +			else if (freq == ULONG_MAX)
>  				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
>  			else
> -				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
> +				cycles_in_1us = ceil(freq, (1000 * 1000));

Consider switching to HZ_PER_MHZ in a separate patch later.

- Mani
Ziqi Chen Jan. 20, 2025, 12:02 p.m. UTC | #2 
Hi Mani,

Thanks for you review~

On 1/19/2025 3:22 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:43PM +0800, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
>> two freqs.
> 
> Same comment as previous patch.
> 

please see my response in previous patch.

>> In the case of scaling up, the devfreq may decide to scale the
>> clock to an intermidiate freq base on load, but the clock scale up pre
>> change operation uses settings for the max clock freq unconditionally. Fix
>> it by passing the target_freq to clock scale up pre change so that the
>> correct settings for the target_freq can be used.
>>
>> In the case of scaling down, the clock scale down post change operation
>> is doing fine, because it reads the actual clock rate to tell freq, but to
>> keep symmetry with clock scale up pre change operation, just use the
>> target_freq instead of reading clock rate.
>>
>> 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 | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index b6eef975dc46..1e8a23eb8c13 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -97,7 +97,7 @@ static const struct __ufs_qcom_bw_table {
>>   };
>>   
>>   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, bool is_scale_up);
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
>>   
>>   static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>>   {
>> @@ -524,7 +524,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   			return -EINVAL;
>>   		}
>>   
>> -		err = ufs_qcom_set_core_clk_ctrl(hba, true);
>> +		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
>>   		if (err)
>>   			dev_err(hba->dev, "cfg core clk ctrl failed\n");
>>   		/*
>> @@ -1231,7 +1231,7 @@ 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, bool is_scale_up)
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	struct list_head *head = &hba->clk_list_head;
>> @@ -1245,10 +1245,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>>   		    !strcmp(clki->name, "core_clk_unipro")) {
>>   			if (!clki->max_freq)
>>   				cycles_in_1us = 150; /* default for backwards compatibility */
>> -			else if (is_scale_up)
>> +			else if (freq == ULONG_MAX)
>>   				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
>>   			else
>> -				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
>> +				cycles_in_1us = ceil(freq, (1000 * 1000));
> 
> Consider switching to HZ_PER_MHZ in a separate patch later
Sure, Thanks for suggestion,  will update in next version.

> 
> - Mani

-Ziqi

>
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index b6eef975dc46..1e8a23eb8c13 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -97,7 +97,7 @@  static const struct __ufs_qcom_bw_table {
 };
 
 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, bool is_scale_up);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
 
 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -524,7 +524,7 @@  static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		err = ufs_qcom_set_core_clk_ctrl(hba, true);
+		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
 		if (err)
 			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
@@ -1231,7 +1231,7 @@  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, bool is_scale_up)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
@@ -1245,10 +1245,11 @@  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 		    !strcmp(clki->name, "core_clk_unipro")) {
 			if (!clki->max_freq)
 				cycles_in_1us = 150; /* default for backwards compatibility */
-			else if (is_scale_up)
+			else if (freq == ULONG_MAX)
 				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
 			else
-				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
+				cycles_in_1us = ceil(freq, (1000 * 1000));
+
 			break;
 		}
 	}
@@ -1285,7 +1286,7 @@  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 	return ufs_qcom_set_clk_40ns_cycles(hba, cycles_in_1us);
 }
 
-static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
+static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
@@ -1298,7 +1299,7 @@  static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 		return ret;
 	}
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, true);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1327,10 +1328,10 @@  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 	return err;
 }
 
-static int ufs_qcom_clk_scale_down_post_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, false);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
@@ -1349,7 +1350,7 @@  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (err)
 			return err;
 		if (scale_up)
-			err = ufs_qcom_clk_scale_up_pre_change(hba);
+			err = ufs_qcom_clk_scale_up_pre_change(hba, target_freq);
 		else
 			err = ufs_qcom_clk_scale_down_pre_change(hba);
 
@@ -1361,7 +1362,7 @@  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (scale_up)
 			err = ufs_qcom_clk_scale_up_post_change(hba);
 		else
-			err = ufs_qcom_clk_scale_down_post_change(hba);
+			err = ufs_qcom_clk_scale_down_post_change(hba, target_freq);
 
 
 		if (err) {