diff mbox series

[V6,5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

Message ID 20230901114336.31339-6-quic_nitirawa@quicinc.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: qcom: Align programming sequence as per HW spec | expand

Commit Message

Nitin Rawat Sept. 1, 2023, 11:43 a.m. UTC 
a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
b)Move ufs_qcom_cfg_timers from clk scaling post change ops
to clk scaling pre change ops to align with the HPG.
c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
to configure sys_1us timer for max freq as per HPG.

Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 63 +++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 17 deletions(-)

--
2.17.1

Comments

Bjorn Andersson Sept. 1, 2023, 4:06 p.m. UTC | #1 
On Fri, Sep 01, 2023 at 05:13:35PM +0530, Nitin Rawat wrote:
> a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
> b)Move ufs_qcom_cfg_timers from clk scaling post change ops
> to clk scaling pre change ops to align with the HPG.
> c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
> to configure sys_1us timer for max freq as per HPG.

You forgot to describe the problem you're trying to solve. This is just
a summary of the changes done in the code.

> 
> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 63 +++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index d670fcc27ffb..c251c98a74f0 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>   * Return: zero for success and non-zero in case of a failure.
>   */
>  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
> -			       u32 hs, u32 rate, bool update_link_startup_timer)

Again, it doesn't seem like this variable name change has any functional
impact, so please don't change it.

PS. You're very welcome to send separate cleanup patches just making the
code easier to read, but please do so separately.


You're changing the prototype of the function, but you're not updating
the kernel-doc above, please correct this.

> +				 u32 hs, u32 rate, bool link_startup,
> +				 bool is_pre_scale_up)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct ufs_clk_info *clki;
> @@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>  	/*
>  	 * The Qunipro controller does not use following registers:
>  	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
> -	 * UFS_REG_PA_LINK_STARTUP_TIMER
> -	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
> -	 * Aggregation logic.
> -	*/
> -	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
> +	 * UFS_REG_PA_LINK_STARTUP_TIMER.
> +	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
> +	 * Aggregation logic and Auto hibern8 logic.
> +	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
> +	 * controller V4.0.0 onwards.
> +	 */
> +	if (ufs_qcom_cap_qunipro(host) &&
> +	    !(ufshcd_is_intr_aggr_allowed(hba) ||
> +	    ufshcd_is_auto_hibern8_supported(hba) ||

This line is indented to the start of the condition, but it's actually
part of the !() expression starting on the line above. Please indent it
to align with the ufshcd_is_intr_aggr_allowed() to make this obvious.

> +	    host->hw_ver.major >= 4))

I think that this condition is added so that the return will only happen
on hw_ver < 4. But if it's "significant", why is it hidden in the inner
expression and not part of the outer expression.

  cap && !(aggr || h8 || ver >= 4)

vs

  ver < 4 && cap && !(aggr || h8)

The latter prunes the option space much faster than the latter.


Second, there are two changes to this condition in this one patch; one
affects hw_ver >= 4, and the other affects any previous target
supporting hibernation.

Please separate these changes, to facilitate debugging any effects of
the hibernation change.

Regards,
Bjorn
Nitin Rawat Sept. 4, 2023, 2:48 p.m. UTC | #2 
On 9/1/2023 9:36 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 05:13:35PM +0530, Nitin Rawat wrote:
>> a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
>> b)Move ufs_qcom_cfg_timers from clk scaling post change ops
>> to clk scaling pre change ops to align with the HPG.
>> c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
>> to configure sys_1us timer for max freq as per HPG.
> 
> You forgot to describe the problem you're trying to solve. This is just
> a summary of the changes done in the code.
Sure..Will update the commit text to describe the problem we are solving
in next patchset.

> 
>>
>> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
>> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 63 +++++++++++++++++++++++++++----------
>>   1 file changed, 46 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index d670fcc27ffb..c251c98a74f0 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>>    * Return: zero for success and non-zero in case of a failure.
>>    */
>>   static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>> -			       u32 hs, u32 rate, bool update_link_startup_timer)
> 
> Again, it doesn't seem like this variable name change has any functional
> impact, so please don't change it.
Sure..Will take care of this in next patchset.

> 
> PS. You're very welcome to send separate cleanup patches just making the
> code easier to read, but please do so separately.
Sure.

> 
> 
> You're changing the prototype of the function, but you're not updating
> the kernel-doc above, please correct this.
I'll update the kernel doc in next patchset.

> 
>> +				 u32 hs, u32 rate, bool link_startup,
>> +				 bool is_pre_scale_up)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	struct ufs_clk_info *clki;
>> @@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>>   	/*
>>   	 * The Qunipro controller does not use following registers:
>>   	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
>> -	 * UFS_REG_PA_LINK_STARTUP_TIMER
>> -	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
>> -	 * Aggregation logic.
>> -	*/
>> -	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
>> +	 * UFS_REG_PA_LINK_STARTUP_TIMER.
>> +	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
>> +	 * Aggregation logic and Auto hibern8 logic.
>> +	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
>> +	 * controller V4.0.0 onwards.
>> +	 */
>> +	if (ufs_qcom_cap_qunipro(host) &&
>> +	    !(ufshcd_is_intr_aggr_allowed(hba) ||
>> +	    ufshcd_is_auto_hibern8_supported(hba) ||
> 
> This line is indented to the start of the condition, but it's actually
> part of the !() expression starting on the line above. Please indent it
> to align with the ufshcd_is_intr_aggr_allowed() to make this obvious.
- Sure..Will correct the code identation.


> 
>> +	    host->hw_ver.major >= 4))
> 
> I think that this condition is added so that the return will only happen
> on hw_ver < 4. But if it's "significant", why is it hidden in the inner
> expression and not part of the outer expression.
> 
>    cap && !(aggr || h8 || ver >= 4)
> 
> vs
> 
>    ver < 4 && cap && !(aggr || h8)
> 
> The latter prunes the option space much faster than the latter.
Sure..Will take care of this in next patchset.

> 
> 
> Second, there are two changes to this condition in this one patch; one
> affects hw_ver >= 4, and the other affects any previous target
> supporting hibernation.
> 
> Please separate these changes, to facilitate debugging any effects of
> the hibernation change.
Sure...Would send separate patch to include hibernation change.


> 
> Regards,
> Bjorn

Thanks,
Nitin
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index d670fcc27ffb..c251c98a74f0 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -532,7 +532,8 @@  static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
  * Return: zero for success and non-zero in case of a failure.
  */
 static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
-			       u32 hs, u32 rate, bool update_link_startup_timer)
+				 u32 hs, u32 rate, bool link_startup,
+				 bool is_pre_scale_up)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_clk_info *clki;
@@ -563,11 +564,16 @@  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	/*
 	 * The Qunipro controller does not use following registers:
 	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
-	 * UFS_REG_PA_LINK_STARTUP_TIMER
-	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
-	 * Aggregation logic.
-	*/
-	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
+	 * UFS_REG_PA_LINK_STARTUP_TIMER.
+	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
+	 * Aggregation logic and Auto hibern8 logic.
+	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
+	 * controller V4.0.0 onwards.
+	 */
+	if (ufs_qcom_cap_qunipro(host) &&
+	    !(ufshcd_is_intr_aggr_allowed(hba) ||
+	    ufshcd_is_auto_hibern8_supported(hba) ||
+	    host->hw_ver.major >= 4))
 		return 0;

 	if (gear == 0) {
@@ -576,8 +582,14 @@  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	}

 	list_for_each_entry(clki, &hba->clk_list_head, list) {
-		if (!strcmp(clki->name, "core_clk"))
-			core_clk_rate = clk_get_rate(clki->clk);
+		if (!strcmp(clki->name, "core_clk")) {
+			if (is_pre_scale_up)
+				core_clk_rate = clki->max_freq;
+			else
+				core_clk_rate = clk_get_rate(clki->clk);
+			break;
+		}
+
 	}

 	/* If frequency is smaller than 1MHz, set to 1MHz */
@@ -657,7 +669,7 @@  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 		mb();
 	}

-	if (update_link_startup_timer && host->hw_ver.major != 0x5) {
+	if (link_startup && host->hw_ver.major != 0x5) {
 		ufshcd_writel(hba, ((core_clk_rate / MSEC_PER_SEC) * 100),
 			      REG_UFS_CFG0);
 		/*
@@ -679,7 +691,7 @@  static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 	switch (status) {
 	case PRE_CHANGE:
 		if (ufs_qcom_cfg_timers(hba, UFS_PWM_G1, SLOWAUTO_MODE,
-					0, true)) {
+					0, true, false)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			return -EINVAL;
@@ -927,7 +939,7 @@  static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 	case POST_CHANGE:
 		if (ufs_qcom_cfg_timers(hba, dev_req_params->gear_rx,
 					dev_req_params->pwr_rx,
-					dev_req_params->hs_rate, false)) {
+					dev_req_params->hs_rate, false, false)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			/*
@@ -1412,10 +1424,22 @@  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
+	int ret;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

+	if (attr) {
+		ret = ufs_qcom_cfg_timers(hba, attr->gear_rx,
+					attr->pwr_rx, attr->hs_rate,
+					false, true);
+		if (ret) {
+			dev_err(hba->dev, "%s ufs cfg timer failed\n",
+						__func__);
+			return ret;
+		}
+	}
 	return ufs_qcom_set_core_clk_ctrl(hba, true);
 }

@@ -1452,10 +1476,21 @@  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
+	int ret;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

+	if (attr) {
+		ret = ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
+					attr->hs_rate, false, false);
+		if (ret) {
+			dev_err(hba->dev, "%s ufs cfg timer failed\n",
+					__func__);
+			return ret;
+		}
+	}
 	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }

@@ -1463,7 +1498,6 @@  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 		bool scale_up, enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct ufs_pa_layer_attr *dev_req_params = &host->dev_req_params;
 	int err = 0;

 	/* check the host controller state before sending hibern8 cmd */
@@ -1493,11 +1527,6 @@  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 			return err;
 		}

-		ufs_qcom_cfg_timers(hba,
-				    dev_req_params->gear_rx,
-				    dev_req_params->pwr_rx,
-				    dev_req_params->hs_rate,
-				    false);
 		ufs_qcom_icc_update_bw(host);
 		ufshcd_uic_hibern8_exit(hba);
 	}