[PATCH v4 0/2] UFS driver general fixes bundle 2

[PATCH v4 0/2] UFS driver general fixes bundle 2

[Can Guo]

This bundle includes 2 general fixes for UFS driver.

Changes since v3:
- Removed trivial spaces in comments

Changes since v2:
- Rebased on 5.7/scsi-queue and fixed minor conflicts

Changes since v1:
- Fixed minor typo

Can Guo (1):
  scsi: ufs: Do not rely on prefetched data

Subhash Jadavani (1):
  scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out
    path

 drivers/scsi/ufs/ufshcd.c | 100 +++++++++++++++++++++++++++++-----------------
 drivers/scsi/ufs/ufshcd.h |  11 -----
 2 files changed, 64 insertions(+), 47 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Can Guo]

From: Subhash Jadavani <subhashj@codeaurora.org>

This change introduces a func ufshcd_set_clk_freq() to explicitly
set clock frequency so that it can be used in reset_and_resotre path and
in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling
error out path.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2a2a63b..63aaa88f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -855,28 +855,29 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
 		return false;
 }
 
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+/**
+ * ufshcd_set_clk_freq - set UFS controller clock frequencies
+ * @hba: per adapter instance
+ * @scale_up: If True, set max possible frequency othewise set low frequency
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
 {
 	int ret = 0;
 	struct ufs_clk_info *clki;
 	struct list_head *head = &hba->clk_list_head;
-	ktime_t start = ktime_get();
-	bool clk_state_changed = false;
 
 	if (list_empty(head))
 		goto out;
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
-	if (ret)
-		return ret;
-
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
 			if (scale_up && clki->max_freq) {
 				if (clki->curr_freq == clki->max_freq)
 					continue;
 
-				clk_state_changed = true;
 				ret = clk_set_rate(clki->clk, clki->max_freq);
 				if (ret) {
 					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 				if (clki->curr_freq == clki->min_freq)
 					continue;
 
-				clk_state_changed = true;
 				ret = clk_set_rate(clki->clk, clki->min_freq);
 				if (ret) {
 					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 				clki->name, clk_get_rate(clki->clk));
 	}
 
+out:
+	return ret;
+}
+
+/**
+ * ufshcd_scale_clks - scale up or scale down UFS controller clocks
+ * @hba: per adapter instance
+ * @scale_up: True if scaling up and false if scaling down
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+{
+	int ret = 0;
+
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
+	if (ret)
+		return ret;
+
+	ret = ufshcd_set_clk_freq(hba, scale_up);
+	if (ret)
+		return ret;
+
 	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	if (ret) {
+		ufshcd_set_clk_freq(hba, !scale_up);
+		return ret;
+	}
 
-out:
-	if (clk_state_changed)
-		trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
-			(scale_up ? "up" : "down"),
-			ktime_to_us(ktime_sub(ktime_get(), start)), ret);
 	return ret;
 }
 
@@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 
 	ret = ufshcd_clock_scaling_prepare(hba);
 	if (ret)
-		return ret;
+		goto out;
 
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
 		ret = ufshcd_scale_gear(hba, false);
 		if (ret)
-			goto out;
+			goto clk_scaling_unprepare;
 	}
 
 	ret = ufshcd_scale_clks(hba, scale_up);
-	if (ret) {
-		if (!scale_up)
-			ufshcd_scale_gear(hba, true);
-		goto out;
-	}
+	if (ret)
+		goto scale_up_gear;
 
 	/* scale up the gear after scaling up clocks */
 	if (scale_up) {
 		ret = ufshcd_scale_gear(hba, true);
 		if (ret) {
 			ufshcd_scale_clks(hba, false);
-			goto out;
+			goto clk_scaling_unprepare;
 		}
 	}
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	goto clk_scaling_unprepare;
 
-out:
+scale_up_gear:
+	if (!scale_up)
+		ufshcd_scale_gear(hba, true);
+clk_scaling_unprepare:
 	ufshcd_clock_scaling_unprepare(hba);
+out:
 	ufshcd_release(hba);
 	return ret;
 }
@@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* scale up clocks to max frequency before full reinitialization */
-	ufshcd_scale_clks(hba, true);
+	ufshcd_set_clk_freq(hba, true);
 
 	err = ufshcd_hba_enable(hba);
 	if (err)
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Avri Altman]

Hi,

> 
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> This change introduces a func ufshcd_set_clk_freq() to explicitly
> set clock frequency so that it can be used in reset_and_resotre path and
> in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling
> error out path.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2a2a63b..63aaa88f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -855,28 +855,29 @@ static bool
> ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>                 return false;
>  }
> 
> -static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
> +/**
> + * ufshcd_set_clk_freq - set UFS controller clock frequencies
> + * @hba: per adapter instance
> + * @scale_up: If True, set max possible frequency othewise set low
> frequency
> + *
> + * Returns 0 if successful
> + * Returns < 0 for any other errors
> + */
> +static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
Personally I prefer using the convention of "__scale_clks" to describe the privet core of scale_clks,
But I know that many people are against it.

>  {
>         int ret = 0;
>         struct ufs_clk_info *clki;
>         struct list_head *head = &hba->clk_list_head;
> -       ktime_t start = ktime_get();
> -       bool clk_state_changed = false;
> 
>         if (list_empty(head))
>                 goto out;
> 
> -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> -       if (ret)
> -               return ret;
> -
>         list_for_each_entry(clki, head, list) {
>                 if (!IS_ERR_OR_NULL(clki->clk)) {
>                         if (scale_up && clki->max_freq) {
>                                 if (clki->curr_freq == clki->max_freq)
>                                         continue;
> 
> -                               clk_state_changed = true;
>                                 ret = clk_set_rate(clki->clk, clki->max_freq);
>                                 if (ret) {
>                                         dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed,
> %d\n",
> @@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
> bool scale_up)
>                                 if (clki->curr_freq == clki->min_freq)
>                                         continue;
> 
> -                               clk_state_changed = true;
>                                 ret = clk_set_rate(clki->clk, clki->min_freq);
>                                 if (ret) {
>                                         dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed,
> %d\n",
> @@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
> bool scale_up)
>                                 clki->name, clk_get_rate(clki->clk));
>         }
> 
> +out:
> +       return ret;
> +}
> +
> +/**
> + * ufshcd_scale_clks - scale up or scale down UFS controller clocks
> + * @hba: per adapter instance
> + * @scale_up: True if scaling up and false if scaling down
> + *
> + * Returns 0 if successful
> + * Returns < 0 for any other errors
> + */
> +static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
> +{
> +       int ret = 0;
> +
> +       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> +       if (ret)
> +               return ret;
> +
> +       ret = ufshcd_set_clk_freq(hba, scale_up);
> +       if (ret)
> +               return ret;
> +
>         ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
> +       if (ret) {
> +               ufshcd_set_clk_freq(hba, !scale_up);
> +               return ret;
> +       }
> 
> -out:
> -       if (clk_state_changed)
> -               trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
> -                       (scale_up ? "up" : "down"),
> -                       ktime_to_us(ktime_sub(ktime_get(), start)), ret);

Why remove the ufshcd_profile_clk_scaling trace?


>         return ret;
>  }
> 
> @@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba
> *hba, bool scale_up)
> 
>         ret = ufshcd_clock_scaling_prepare(hba);
>         if (ret)
> -               return ret;
> +               goto out;
Are you fixing here a hold without release?
Should make note of that.

> 
>         /* scale down the gear before scaling down clocks */
>         if (!scale_up) {
>                 ret = ufshcd_scale_gear(hba, false);
>                 if (ret)
> -                       goto out;
> +                       goto clk_scaling_unprepare;
>         }
> 
>         ret = ufshcd_scale_clks(hba, scale_up);
> -       if (ret) {
> -               if (!scale_up)
> -                       ufshcd_scale_gear(hba, true);
> -               goto out;
> -       }
> +       if (ret)
> +               goto scale_up_gear;
> 
>         /* scale up the gear after scaling up clocks */
>         if (scale_up) {
>                 ret = ufshcd_scale_gear(hba, true);
>                 if (ret) {
>                         ufshcd_scale_clks(hba, false);
> -                       goto out;
> +                       goto clk_scaling_unprepare;
>                 }
>         }
> 
> -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
> +       goto clk_scaling_unprepare;
I think you should find a way to make this function more readable.
Adding all those "spaghetti" gotos making it even harder to read. 

Thanks,
Avri

> 
> -out:
> +scale_up_gear:
> +       if (!scale_up)
> +               ufshcd_scale_gear(hba, true);
> +clk_scaling_unprepare:
>         ufshcd_clock_scaling_unprepare(hba);
> +out:
>         ufshcd_release(hba);
>         return ret;
>  }
> @@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct
> ufs_hba *hba)
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
>         /* scale up clocks to max frequency before full reinitialization */
> -       ufshcd_scale_clks(hba, true);
> +       ufshcd_set_clk_freq(hba, true);
> 
>         err = ufshcd_hba_enable(hba);
>         if (err)
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project.

Re: [PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Can Guo]

On 2020-03-18 16:19, Avri Altman wrote:
> Hi,
> 
>> 
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>> 
>> This change introduces a func ufshcd_set_clk_freq() to explicitly
>> set clock frequency so that it can be used in reset_and_resotre path 
>> and
>> in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock 
>> scaling
>> error out path.
>> 
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 2a2a63b..63aaa88f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -855,28 +855,29 @@ static bool
>> ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>>                 return false;
>>  }
>> 
>> -static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
>> +/**
>> + * ufshcd_set_clk_freq - set UFS controller clock frequencies
>> + * @hba: per adapter instance
>> + * @scale_up: If True, set max possible frequency othewise set low
>> frequency
>> + *
>> + * Returns 0 if successful
>> + * Returns < 0 for any other errors
>> + */
>> +static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
> Personally I prefer using the convention of "__scale_clks" to describe
> the privet core of scale_clks,
> But I know that many people are against it.
> 
>>  {
>>         int ret = 0;
>>         struct ufs_clk_info *clki;
>>         struct list_head *head = &hba->clk_list_head;
>> -       ktime_t start = ktime_get();
>> -       bool clk_state_changed = false;
>> 
>>         if (list_empty(head))
>>                 goto out;
>> 
>> -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>> -       if (ret)
>> -               return ret;
>> -
>>         list_for_each_entry(clki, head, list) {
>>                 if (!IS_ERR_OR_NULL(clki->clk)) {
>>                         if (scale_up && clki->max_freq) {
>>                                 if (clki->curr_freq == clki->max_freq)
>>                                         continue;
>> 
>> -                               clk_state_changed = true;
>>                                 ret = clk_set_rate(clki->clk, 
>> clki->max_freq);
>>                                 if (ret) {
>>                                         dev_err(hba->dev, "%s: %s clk 
>> set rate(%dHz) failed,
>> %d\n",
>> @@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba,
>> bool scale_up)
>>                                 if (clki->curr_freq == clki->min_freq)
>>                                         continue;
>> 
>> -                               clk_state_changed = true;
>>                                 ret = clk_set_rate(clki->clk, 
>> clki->min_freq);
>>                                 if (ret) {
>>                                         dev_err(hba->dev, "%s: %s clk 
>> set rate(%dHz) failed,
>> %d\n",
>> @@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba 
>> *hba,
>> bool scale_up)
>>                                 clki->name, clk_get_rate(clki->clk));
>>         }
>> 
>> +out:
>> +       return ret;
>> +}
>> +
>> +/**
>> + * ufshcd_scale_clks - scale up or scale down UFS controller clocks
>> + * @hba: per adapter instance
>> + * @scale_up: True if scaling up and false if scaling down
>> + *
>> + * Returns 0 if successful
>> + * Returns < 0 for any other errors
>> + */
>> +static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = ufshcd_set_clk_freq(hba, scale_up);
>> +       if (ret)
>> +               return ret;
>> +
>>         ret = ufshcd_vops_clk_scale_notify(hba, scale_up, 
>> POST_CHANGE);
>> +       if (ret) {
>> +               ufshcd_set_clk_freq(hba, !scale_up);
>> +               return ret;
>> +       }
>> 
>> -out:
>> -       if (clk_state_changed)
>> -               trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
>> -                       (scale_up ? "up" : "down"),
>> -                       ktime_to_us(ktime_sub(ktime_get(), start)), 
>> ret);
> 
> Why remove the ufshcd_profile_clk_scaling trace?
> 
> 

Shall add it back, this is just a collection of Subhash's changes.

>>         return ret;
>>  }
>> 
>> @@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba
>> *hba, bool scale_up)
>> 
>>         ret = ufshcd_clock_scaling_prepare(hba);
>>         if (ret)
>> -               return ret;
>> +               goto out;
> Are you fixing here a hold without release?
> Should make note of that.
> 

Yes, true

>> 
>>         /* scale down the gear before scaling down clocks */
>>         if (!scale_up) {
>>                 ret = ufshcd_scale_gear(hba, false);
>>                 if (ret)
>> -                       goto out;
>> +                       goto clk_scaling_unprepare;
>>         }
>> 
>>         ret = ufshcd_scale_clks(hba, scale_up);
>> -       if (ret) {
>> -               if (!scale_up)
>> -                       ufshcd_scale_gear(hba, true);
>> -               goto out;
>> -       }
>> +       if (ret)
>> +               goto scale_up_gear;
>> 
>>         /* scale up the gear after scaling up clocks */
>>         if (scale_up) {
>>                 ret = ufshcd_scale_gear(hba, true);
>>                 if (ret) {
>>                         ufshcd_scale_clks(hba, false);
>> -                       goto out;
>> +                       goto clk_scaling_unprepare;
>>                 }
>>         }
>> 
>> -       ret = ufshcd_vops_clk_scale_notify(hba, scale_up, 
>> POST_CHANGE);
>> +       goto clk_scaling_unprepare;
> I think you should find a way to make this function more readable.
> Adding all those "spaghetti" gotos making it even harder to read.
> 
> Thanks,
> Avri
> 

This is just a collection of Subhash's changes. I think the 2
clk_scaling_unprepare and out gotos make sense, but the
scale_up_gear goto is a bit vague. I will remove the
scale_up_gear goto.

Thanks,

Can Guo.

>> 
>> -out:
>> +scale_up_gear:
>> +       if (!scale_up)
>> +               ufshcd_scale_gear(hba, true);
>> +clk_scaling_unprepare:
>>         ufshcd_clock_scaling_unprepare(hba);
>> +out:
>>         ufshcd_release(hba);
>>         return ret;
>>  }
>> @@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct
>> ufs_hba *hba)
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>> 
>>         /* scale up clocks to max frequency before full 
>> reinitialization */
>> -       ufshcd_scale_clks(hba, true);
>> +       ufshcd_set_clk_freq(hba, true);
>> 
>>         err = ufshcd_hba_enable(hba);
>>         if (err)
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>> Linux Foundation Collaborative Project.

Re: [PATCH 2/2] scsi: ufs: Do not rely on prefetched data

[Stanley Chu]

Hi Can,

On Mon, 2020-03-16 at 00:06 -0700, Can Guo wrote:
> We were setting bActiveICCLevel attribute for UFS device only once but
> type of this attribute has changed from persistent to volatile since UFS
> device specification v2.1. This attribute is set to the default value after
> power cycle or hardware reset event. It isn't safe to rely on prefetched
> data (only used for bActiveICCLevel attribute now). Hence this change
> removes the code related to data prefetching and set this parameter on
> every attempt to probe the UFS device.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>

Reviewed and Tested by: Stanley Chu <stanley.chu@mediatek.com>

RE: [PATCH 2/2] scsi: ufs: Do not rely on prefetched data

[Avri Altman]

> 
> We were setting bActiveICCLevel attribute for UFS device only once but
> type of this attribute has changed from persistent to volatile since UFS
> device specification v2.1. This attribute is set to the default value after
> power cycle or hardware reset event. It isn't safe to rely on prefetched
> data (only used for bActiveICCLevel attribute now). Hence this change
> removes the code related to data prefetching and set this parameter on
> every attempt to probe the UFS device.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Thanks for fixing this.
Avri

[PATCH v3 0/2] UFS driver general fixes bundle 2

[Can Guo]

This bundle includes 2 general fixes for UFS driver.

Changes since v2:
- Rebased on 5.7/scsi-queue and fixed minor conflicts

Changes since v1:
- Fixed minor typo

Can Guo (1):
  scsi: ufs: Do not rely on prefetched data

Subhash Jadavani (1):
  scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out
    path

 drivers/scsi/ufs/ufshcd.c | 100 +++++++++++++++++++++++++++++-----------------
 drivers/scsi/ufs/ufshcd.h |  11 -----
 2 files changed, 64 insertions(+), 47 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path

[Can Guo]

From: Subhash Jadavani <subhashj@codeaurora.org>

This change introduces a func ufshcd_set_clk_freq() to explicitly
set clock frequency so that it can be used in reset_and_resotre path and
in ufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling
error out path.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2a2a63b..63aaa88f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -855,28 +855,29 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
 		return false;
 }
 
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+/**
+ * ufshcd_set_clk_freq - set UFS controller clock frequencies
+ * @hba: per adapter instance
+ * @scale_up: If True, set max possible frequency othewise set low frequency
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
 {
 	int ret = 0;
 	struct ufs_clk_info *clki;
 	struct list_head *head = &hba->clk_list_head;
-	ktime_t start = ktime_get();
-	bool clk_state_changed = false;
 
 	if (list_empty(head))
 		goto out;
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
-	if (ret)
-		return ret;
-
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
 			if (scale_up && clki->max_freq) {
 				if (clki->curr_freq == clki->max_freq)
 					continue;
 
-				clk_state_changed = true;
 				ret = clk_set_rate(clki->clk, clki->max_freq);
 				if (ret) {
 					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -895,7 +896,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 				if (clki->curr_freq == clki->min_freq)
 					continue;
 
-				clk_state_changed = true;
 				ret = clk_set_rate(clki->clk, clki->min_freq);
 				if (ret) {
 					dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
@@ -914,13 +914,36 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 				clki->name, clk_get_rate(clki->clk));
 	}
 
+out:
+	return ret;
+}
+
+/**
+ * ufshcd_scale_clks - scale up or scale down UFS controller clocks
+ * @hba: per adapter instance
+ * @scale_up: True if scaling up and false if scaling down
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+{
+	int ret = 0;
+
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
+	if (ret)
+		return ret;
+
+	ret = ufshcd_set_clk_freq(hba, scale_up);
+	if (ret)
+		return ret;
+
 	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	if (ret) {
+		ufshcd_set_clk_freq(hba, !scale_up);
+		return ret;
+	}
 
-out:
-	if (clk_state_changed)
-		trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
-			(scale_up ? "up" : "down"),
-			ktime_to_us(ktime_sub(ktime_get(), start)), ret);
 	return ret;
 }
 
@@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 
 	ret = ufshcd_clock_scaling_prepare(hba);
 	if (ret)
-		return ret;
+		goto out;
 
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
 		ret = ufshcd_scale_gear(hba, false);
 		if (ret)
-			goto out;
+			goto clk_scaling_unprepare;
 	}
 
 	ret = ufshcd_scale_clks(hba, scale_up);
-	if (ret) {
-		if (!scale_up)
-			ufshcd_scale_gear(hba, true);
-		goto out;
-	}
+	if (ret)
+		goto scale_up_gear;
 
 	/* scale up the gear after scaling up clocks */
 	if (scale_up) {
 		ret = ufshcd_scale_gear(hba, true);
 		if (ret) {
 			ufshcd_scale_clks(hba, false);
-			goto out;
+			goto clk_scaling_unprepare;
 		}
 	}
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	goto clk_scaling_unprepare;
 
-out:
+scale_up_gear:
+	if (!scale_up)
+		ufshcd_scale_gear(hba, true);
+clk_scaling_unprepare:
 	ufshcd_clock_scaling_unprepare(hba);
+out:
 	ufshcd_release(hba);
 	return ret;
 }
@@ -6251,7 +6275,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* scale up clocks to max frequency before full reinitialization */
-	ufshcd_scale_clks(hba, true);
+	ufshcd_set_clk_freq(hba, true);
 
 	err = ufshcd_hba_enable(hba);
 	if (err)
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.