UFS driver general fixes bundle 2
v4 cover UFS driver general fixes bundle 2
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(-)
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 2/2] scsi: ufs: Do not rely on prefetched data
Hi Can,
Quoted 9 lines · Can Guo
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 <[email protected]>
Reviewed and Tested by: Stanley Chu <[email protected]>
Avri Altman
Reviewed-byRE: [PATCH 2/2] scsi: ufs: Do not rely on prefetched data
In reply to a message not in the archiveQuoted 10 lines
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 <[email protected]>
Thanks for fixing this.
Avri
PATCH v4 1/2 scsi: ufs: Clean up ufshcd_scale_clks() and clock scaling error out path
In reply to Can GuoFrom: Subhash Jadavani <[email protected]>
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.
drivers/scsi/ufs/ufshcd.c +49 -25
@@ -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)
Signature · 1 line
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
Hi,
Quoted 25 lines
From: Subhash Jadavani <[email protected]>
This change introduces a func
ufshcd_set_clk_freq()to explicitly set clock frequency so that it can be used inreset_and_resotrepath and inufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling error out path.Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Can Guo <[email protected]>drivers/scsi/ufs/ufshcd.c +4
-1@@ -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
Quoted 6 lines
frequency
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static intufshcd_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.
Quoted 65 lines
{ int ret = 0; struct ufs_clk_info *clki; struct list_head *head = &hba->clk_list_head;drivers/scsi/ufs/ufshcd.c +0
-7-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",
drivers/scsi/ufs/ufshcd.c +0
-1@@ -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",
drivers/scsi/ufs/ufshcd.c +24
-0@@ -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 );
drivers/scsi/ufs/ufshcd.c static int ufshcd_scale_clks(struct ufs_hba *hba, · line 924
Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +4
-5+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?
drivers/scsi/ufs/ufshcd.c static int ufshcd_devfreq_scale(struct ufs_hba · line 1133
Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +1
-1return 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.
Quoted 20 lines
drivers/scsi/ufs/ufshcd.c +3
-6/* 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) {
drivers/scsi/ufs/ufshcd.c static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) · line 1154
Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +2
-2ret =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
Quoted 24 lines
drivers/scsi/ufs/ufshcd.c +6
-2-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
Quoted 113 lines · Avri Altman
Hi,
Quoted 25 lines
From: Subhash Jadavani <[email protected]>
This change introduces a func
ufshcd_set_clk_freq()to explicitly set clock frequency so that it can be used inreset_and_resotrepath and inufshcd_scale_clks(). Meanwhile, this change cleans up the clock scaling error out path.Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Can Guo <[email protected]>drivers/scsi/ufs/ufshcd.c +4
-1@@ -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 Quoted 6 lines
frequency
+ *
+ * Returns 0 if successful
+ * Returns < 0 for any other errors
+ */
+static intufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)Personally I prefer using the convention of "
__scale_clks" to describe the privet core ofscale_clks,
But I know that many people are against it.Quoted 76 lines
{ int ret = 0; struct ufs_clk_info *clki; struct list_head *head = &hba->clk_list_head;drivers/scsi/ufs/ufshcd.c +0
-7-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 clkset rate(%dHz) failed,
%d\n",drivers/scsi/ufs/ufshcd.c +0
-1@@ -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 clkset rate(%dHz) failed,
%d\n",drivers/scsi/ufs/ufshcd.c +28
-0@@ -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; + }
Quoted 10 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +0
-5-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_scalingtrace?
Shall add it back, this is just a collection of Subhash's changes.
Quoted 6 lines
Quoted 6 lines
return ret;}
drivers/scsi/ufs/ufshcd.c +0
-0@@ -1106,35 +1129,36 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) · line 1132
Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +1
-1ret =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
Quoted 28 lines
Quoted 28 lines
drivers/scsi/ufs/ufshcd.c +4
-7/* 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; } }
Quoted 9 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_devfreq_scale(struct ufs_hba · line 1155
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1
-1- 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.
Quoted 25 lines
Quoted 25 lines
drivers/scsi/ufs/ufshcd.c +5
-1-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 */
drivers/scsi/ufs/ufshcd.c +1
-2-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.