[PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[Stanley Chu]

Hi,
This series cleans up and refactors clk-scaling feature, and shall not change any functionality.

This series is based on Can's series "Three changes related with UFS clock scaling" in 5.10/scsi-fixes branch in Martin's tree.

However this series may not be required to be merged to 5.10. The choice of base branch is simply making these patches easy to be reviewed because this series is based on clk-scaling fixes by Can. If this series is decided not being merged to 5.10, then I would rebase it to 5.11/scsi-queue.

Changes since v1:
  - Refactor ufshcd_clk_scaling_suspend() in patch [3/4]
  - Change function name from ufshcd_clk_scaling_pm() to ufshcd_clk_scaling_suspend() in patch [3/4]
  - Refine patch titles

Stanley Chu (4):
  scsi: ufs: Refactor cancelling clkscaling works
  scsi: ufs: Remove redundant null checking of devfreq instance
  scsi: ufs: Cleanup and refactor clk-scaling feature
  scsi: ufs: Fix build warning by incorrect function description

 drivers/scsi/ufs/ufshcd.c | 90 +++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

-- 
2.18.0

[PATCH v2 1/4] scsi: ufs: Refactor cancelling clkscaling works

[Stanley Chu]

Cancelling suspend_work and resume_work is only required while
suspending clk-scaling. Thus moving these two invokes into
ufshcd_suspend_clkscaling() function.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 052479a56a6f..a91b73a1fc48 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1451,6 +1451,9 @@ static void ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return;
 
+	cancel_work_sync(&hba->clk_scaling.suspend_work);
+	cancel_work_sync(&hba->clk_scaling.resume_work);
+
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (!hba->clk_scaling.is_suspended) {
 		suspend = true;
@@ -1514,9 +1517,6 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	pm_runtime_get_sync(hba->dev);
 	ufshcd_hold(hba, false);
 
-	cancel_work_sync(&hba->clk_scaling.suspend_work);
-	cancel_work_sync(&hba->clk_scaling.resume_work);
-
 	if (value) {
 		ufshcd_resume_clkscaling(hba);
 	} else {
@@ -5663,11 +5663,8 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
 	} else {
 		ufshcd_hold(hba, false);
-		if (hba->clk_scaling.is_enabled) {
-			cancel_work_sync(&hba->clk_scaling.suspend_work);
-			cancel_work_sync(&hba->clk_scaling.resume_work);
+		if (hba->clk_scaling.is_enabled)
 			ufshcd_suspend_clkscaling(hba);
-		}
 	}
 	down_write(&hba->clk_scaling_lock);
 	hba->clk_scaling.is_allowed = false;
@@ -8512,11 +8509,9 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ufshcd_hold(hba, false);
 	hba->clk_gating.is_suspended = true;
 
-	if (hba->clk_scaling.is_enabled) {
-		cancel_work_sync(&hba->clk_scaling.suspend_work);
-		cancel_work_sync(&hba->clk_scaling.resume_work);
+	if (hba->clk_scaling.is_enabled)
 		ufshcd_suspend_clkscaling(hba);
-	}
+
 	down_write(&hba->clk_scaling_lock);
 	hba->clk_scaling.is_allowed = false;
 	up_write(&hba->clk_scaling_lock);
-- 
2.18.0

Re: [PATCH v2 1/4] scsi: ufs: Refactor cancelling clkscaling works

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> Cancelling suspend_work and resume_work is only required while
> suspending clk-scaling. Thus moving these two invokes into
> ufshcd_suspend_clkscaling() function.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 052479a56a6f..a91b73a1fc48 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1451,6 +1451,9 @@ static void ufshcd_suspend_clkscaling(struct 
> ufs_hba *hba)
>  	if (!ufshcd_is_clkscaling_supported(hba))
>  		return;
> 
> +	cancel_work_sync(&hba->clk_scaling.suspend_work);
> +	cancel_work_sync(&hba->clk_scaling.resume_work);
> +
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (!hba->clk_scaling.is_suspended) {
>  		suspend = true;
> @@ -1514,9 +1517,6 @@ static ssize_t
> ufshcd_clkscale_enable_store(struct device *dev,
>  	pm_runtime_get_sync(hba->dev);
>  	ufshcd_hold(hba, false);
> 
> -	cancel_work_sync(&hba->clk_scaling.suspend_work);
> -	cancel_work_sync(&hba->clk_scaling.resume_work);
> -
>  	if (value) {
>  		ufshcd_resume_clkscaling(hba);
>  	} else {
> @@ -5663,11 +5663,8 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>  		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
>  	} else {
>  		ufshcd_hold(hba, false);
> -		if (hba->clk_scaling.is_enabled) {
> -			cancel_work_sync(&hba->clk_scaling.suspend_work);
> -			cancel_work_sync(&hba->clk_scaling.resume_work);
> +		if (hba->clk_scaling.is_enabled)
>  			ufshcd_suspend_clkscaling(hba);
> -		}
>  	}
>  	down_write(&hba->clk_scaling_lock);
>  	hba->clk_scaling.is_allowed = false;
> @@ -8512,11 +8509,9 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	ufshcd_hold(hba, false);
>  	hba->clk_gating.is_suspended = true;
> 
> -	if (hba->clk_scaling.is_enabled) {
> -		cancel_work_sync(&hba->clk_scaling.suspend_work);
> -		cancel_work_sync(&hba->clk_scaling.resume_work);
> +	if (hba->clk_scaling.is_enabled)
>  		ufshcd_suspend_clkscaling(hba);
> -	}
> +
>  	down_write(&hba->clk_scaling_lock);
>  	hba->clk_scaling.is_allowed = false;
>  	up_write(&hba->clk_scaling_lock);

Reviewed-by: Can Guo <cang@codeaurora.org>

[PATCH v2 2/4] scsi: ufs: Remove redundant null checking of devfreq instance

[Stanley Chu]

hba->devfreq is zero-initialized thus it is not required
to check its existence in ufshcd_add_lus() function which
is invoked during initialization only.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a91b73a1fc48..9cc16598136d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7636,15 +7636,13 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 			&hba->pwr_info,
 			sizeof(struct ufs_pa_layer_attr));
 		hba->clk_scaling.saved_pwr_info.is_valid = true;
-		if (!hba->devfreq) {
-			hba->clk_scaling.is_allowed = true;
-			ret = ufshcd_devfreq_init(hba);
-			if (ret)
-				goto out;
+		hba->clk_scaling.is_allowed = true;
+		ret = ufshcd_devfreq_init(hba);
+		if (ret)
+			goto out;
 
-			hba->clk_scaling.is_enabled = true;
-			ufshcd_clkscaling_init_sysfs(hba);
-		}
+		hba->clk_scaling.is_enabled = true;
+		ufshcd_clkscaling_init_sysfs(hba);
 	}
 
 	ufs_bsg_probe(hba);
-- 
2.18.0

Re: [PATCH v2 2/4] scsi: ufs: Remove redundant null checking of devfreq instance

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> hba->devfreq is zero-initialized thus it is not required
> to check its existence in ufshcd_add_lus() function which
> is invoked during initialization only.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a91b73a1fc48..9cc16598136d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7636,15 +7636,13 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  			&hba->pwr_info,
>  			sizeof(struct ufs_pa_layer_attr));
>  		hba->clk_scaling.saved_pwr_info.is_valid = true;
> -		if (!hba->devfreq) {

Just FYI, checking this was due to consideration for supporting UFS 
cards
which can do hot-plug. But UFS cards seems out of the picture for a long 
time.
Anyways, if someday UFS cards come back in future, let's add these 
checks back again.

Thanks,

Can Guo.

> -			hba->clk_scaling.is_allowed = true;
> -			ret = ufshcd_devfreq_init(hba);
> -			if (ret)
> -				goto out;
> +		hba->clk_scaling.is_allowed = true;
> +		ret = ufshcd_devfreq_init(hba);
> +		if (ret)
> +			goto out;
> 
> -			hba->clk_scaling.is_enabled = true;
> -			ufshcd_clkscaling_init_sysfs(hba);
> -		}
> +		hba->clk_scaling.is_enabled = true;
> +		ufshcd_clkscaling_init_sysfs(hba);
>  	}
> 
>  	ufs_bsg_probe(hba);

Re: [PATCH v2 2/4] scsi: ufs: Remove redundant null checking of devfreq instance

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> hba->devfreq is zero-initialized thus it is not required
> to check its existence in ufshcd_add_lus() function which
> is invoked during initialization only.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a91b73a1fc48..9cc16598136d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7636,15 +7636,13 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  			&hba->pwr_info,
>  			sizeof(struct ufs_pa_layer_attr));
>  		hba->clk_scaling.saved_pwr_info.is_valid = true;
> -		if (!hba->devfreq) {
> -			hba->clk_scaling.is_allowed = true;
> -			ret = ufshcd_devfreq_init(hba);
> -			if (ret)
> -				goto out;
> +		hba->clk_scaling.is_allowed = true;
> +		ret = ufshcd_devfreq_init(hba);
> +		if (ret)
> +			goto out;
> 
> -			hba->clk_scaling.is_enabled = true;
> -			ufshcd_clkscaling_init_sysfs(hba);
> -		}
> +		hba->clk_scaling.is_enabled = true;
> +		ufshcd_clkscaling_init_sysfs(hba);
>  	}
> 
>  	ufs_bsg_probe(hba);

Reviewed-by: Can Guo <cang@codeaurora.org>

[PATCH v2 3/4] scsi: ufs: Cleanup and refactor clk-scaling feature

[Stanley Chu]

Manipulate clock scaling related stuff only if the host capability
supports clock scaling feature to avoid redundant code execution.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 64 ++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9cc16598136d..ce0528f2e2ed 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1448,9 +1448,6 @@ static void ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 	unsigned long flags;
 	bool suspend = false;
 
-	if (!ufshcd_is_clkscaling_supported(hba))
-		return;
-
 	cancel_work_sync(&hba->clk_scaling.suspend_work);
 	cancel_work_sync(&hba->clk_scaling.resume_work);
 
@@ -1470,9 +1467,6 @@ static void ufshcd_resume_clkscaling(struct ufs_hba *hba)
 	unsigned long flags;
 	bool resume = false;
 
-	if (!ufshcd_is_clkscaling_supported(hba))
-		return;
-
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (hba->clk_scaling.is_suspended) {
 		resume = true;
@@ -5642,6 +5636,26 @@ static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
 	}
 }
 
+static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
+{
+	down_write(&hba->clk_scaling_lock);
+	hba->clk_scaling.is_allowed = allow;
+	up_write(&hba->clk_scaling_lock);
+}
+
+static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend)
+{
+	if (suspend) {
+		if (hba->clk_scaling.is_enabled)
+			ufshcd_suspend_clkscaling(hba);
+		ufshcd_clk_scaling_allow(hba, false);
+	} else {
+		ufshcd_clk_scaling_allow(hba, true);
+		if (hba->clk_scaling.is_enabled)
+			ufshcd_resume_clkscaling(hba);
+	}
+}
+
 static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 {
 	pm_runtime_get_sync(hba->dev);
@@ -5663,23 +5677,20 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
 	} else {
 		ufshcd_hold(hba, false);
-		if (hba->clk_scaling.is_enabled)
+		if (ufshcd_is_clkscaling_supported(hba) &&
+		    hba->clk_scaling.is_enabled)
 			ufshcd_suspend_clkscaling(hba);
 	}
-	down_write(&hba->clk_scaling_lock);
-	hba->clk_scaling.is_allowed = false;
-	up_write(&hba->clk_scaling_lock);
+	ufshcd_clk_scaling_allow(hba, false);
 }
 
 static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 {
 	ufshcd_release(hba);
 
-	down_write(&hba->clk_scaling_lock);
-	hba->clk_scaling.is_allowed = true;
-	up_write(&hba->clk_scaling_lock);
-	if (hba->clk_scaling.is_enabled)
-		ufshcd_resume_clkscaling(hba);
+	if (ufshcd_is_clkscaling_supported(hba))
+		ufshcd_clk_scaling_suspend(hba, false);
+
 	pm_runtime_put(hba->dev);
 }
 
@@ -8507,12 +8518,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ufshcd_hold(hba, false);
 	hba->clk_gating.is_suspended = true;
 
-	if (hba->clk_scaling.is_enabled)
-		ufshcd_suspend_clkscaling(hba);
-
-	down_write(&hba->clk_scaling_lock);
-	hba->clk_scaling.is_allowed = false;
-	up_write(&hba->clk_scaling_lock);
+	if (ufshcd_is_clkscaling_supported(hba))
+		ufshcd_clk_scaling_suspend(hba, true);
 
 	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
 			req_link_state == UIC_LINK_ACTIVE_STATE) {
@@ -8618,11 +8625,9 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
 		ufshcd_disable_auto_bkops(hba);
 enable_gating:
-	down_write(&hba->clk_scaling_lock);
-	hba->clk_scaling.is_allowed = true;
-	up_write(&hba->clk_scaling_lock);
-	if (hba->clk_scaling.is_enabled)
-		ufshcd_resume_clkscaling(hba);
+	if (ufshcd_is_clkscaling_supported(hba))
+		ufshcd_clk_scaling_suspend(hba, false);
+
 	hba->clk_gating.is_suspended = false;
 	hba->dev_info.b_rpm_dev_flush_capable = false;
 	ufshcd_release(hba);
@@ -8719,11 +8724,8 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 
 	hba->clk_gating.is_suspended = false;
 
-	down_write(&hba->clk_scaling_lock);
-	hba->clk_scaling.is_allowed = true;
-	up_write(&hba->clk_scaling_lock);
-	if (hba->clk_scaling.is_enabled)
-		ufshcd_resume_clkscaling(hba);
+	if (ufshcd_is_clkscaling_supported(hba))
+		ufshcd_clk_scaling_suspend(hba, false);
 
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
-- 
2.18.0

Re: [PATCH v2 3/4] scsi: ufs: Cleanup and refactor clk-scaling feature

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> Manipulate clock scaling related stuff only if the host capability
> supports clock scaling feature to avoid redundant code execution.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 64 ++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9cc16598136d..ce0528f2e2ed 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1448,9 +1448,6 @@ static void ufshcd_suspend_clkscaling(struct 
> ufs_hba *hba)
>  	unsigned long flags;
>  	bool suspend = false;
> 
> -	if (!ufshcd_is_clkscaling_supported(hba))
> -		return;
> -
>  	cancel_work_sync(&hba->clk_scaling.suspend_work);
>  	cancel_work_sync(&hba->clk_scaling.resume_work);
> 
> @@ -1470,9 +1467,6 @@ static void ufshcd_resume_clkscaling(struct 
> ufs_hba *hba)
>  	unsigned long flags;
>  	bool resume = false;
> 
> -	if (!ufshcd_is_clkscaling_supported(hba))
> -		return;
> -
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	if (hba->clk_scaling.is_suspended) {
>  		resume = true;
> @@ -5642,6 +5636,26 @@ static inline void
> ufshcd_schedule_eh_work(struct ufs_hba *hba)
>  	}
>  }
> 
> +static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
> +{
> +	down_write(&hba->clk_scaling_lock);
> +	hba->clk_scaling.is_allowed = allow;
> +	up_write(&hba->clk_scaling_lock);
> +}
> +
> +static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool 
> suspend)
> +{
> +	if (suspend) {
> +		if (hba->clk_scaling.is_enabled)
> +			ufshcd_suspend_clkscaling(hba);
> +		ufshcd_clk_scaling_allow(hba, false);
> +	} else {
> +		ufshcd_clk_scaling_allow(hba, true);
> +		if (hba->clk_scaling.is_enabled)
> +			ufshcd_resume_clkscaling(hba);
> +	}
> +}
> +
>  static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>  {
>  	pm_runtime_get_sync(hba->dev);
> @@ -5663,23 +5677,20 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>  		ufshcd_vops_resume(hba, UFS_RUNTIME_PM);
>  	} else {
>  		ufshcd_hold(hba, false);
> -		if (hba->clk_scaling.is_enabled)
> +		if (ufshcd_is_clkscaling_supported(hba) &&
> +		    hba->clk_scaling.is_enabled)
>  			ufshcd_suspend_clkscaling(hba);
>  	}
> -	down_write(&hba->clk_scaling_lock);
> -	hba->clk_scaling.is_allowed = false;
> -	up_write(&hba->clk_scaling_lock);
> +	ufshcd_clk_scaling_allow(hba, false);
>  }
> 
>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  {
>  	ufshcd_release(hba);
> 
> -	down_write(&hba->clk_scaling_lock);
> -	hba->clk_scaling.is_allowed = true;
> -	up_write(&hba->clk_scaling_lock);
> -	if (hba->clk_scaling.is_enabled)
> -		ufshcd_resume_clkscaling(hba);
> +	if (ufshcd_is_clkscaling_supported(hba))
> +		ufshcd_clk_scaling_suspend(hba, false);
> +
>  	pm_runtime_put(hba->dev);
>  }
> 
> @@ -8507,12 +8518,8 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	ufshcd_hold(hba, false);
>  	hba->clk_gating.is_suspended = true;
> 
> -	if (hba->clk_scaling.is_enabled)
> -		ufshcd_suspend_clkscaling(hba);
> -
> -	down_write(&hba->clk_scaling_lock);
> -	hba->clk_scaling.is_allowed = false;
> -	up_write(&hba->clk_scaling_lock);
> +	if (ufshcd_is_clkscaling_supported(hba))
> +		ufshcd_clk_scaling_suspend(hba, true);
> 
>  	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
>  			req_link_state == UIC_LINK_ACTIVE_STATE) {
> @@ -8618,11 +8625,9 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
>  		ufshcd_disable_auto_bkops(hba);
>  enable_gating:
> -	down_write(&hba->clk_scaling_lock);
> -	hba->clk_scaling.is_allowed = true;
> -	up_write(&hba->clk_scaling_lock);
> -	if (hba->clk_scaling.is_enabled)
> -		ufshcd_resume_clkscaling(hba);
> +	if (ufshcd_is_clkscaling_supported(hba))
> +		ufshcd_clk_scaling_suspend(hba, false);
> +
>  	hba->clk_gating.is_suspended = false;
>  	hba->dev_info.b_rpm_dev_flush_capable = false;
>  	ufshcd_release(hba);
> @@ -8719,11 +8724,8 @@ static int ufshcd_resume(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
> 
>  	hba->clk_gating.is_suspended = false;
> 
> -	down_write(&hba->clk_scaling_lock);
> -	hba->clk_scaling.is_allowed = true;
> -	up_write(&hba->clk_scaling_lock);
> -	if (hba->clk_scaling.is_enabled)
> -		ufshcd_resume_clkscaling(hba);
> +	if (ufshcd_is_clkscaling_supported(hba))
> +		ufshcd_clk_scaling_suspend(hba, false);
> 
>  	/* Enable Auto-Hibernate if configured */
>  	ufshcd_auto_hibern8_enable(hba);

Thanks for the cleanup

Reviewed-by: Can Guo <cang@codeaurora.org>

[PATCH v2 4/4] scsi: ufs: Fix build warning by incorrect function description

[Stanley Chu]

Fix build warnings as below due to incorrect function description
of ufshcd_try_to_abort_task().

ufshcd.c:6651: warning: Function parameter or member 'hba' not described in 'ufshcd_try_to_abort_task'
ufshcd.c:6651: warning: Function parameter or member 'tag' not described in 'ufshcd_try_to_abort_task'
ufshcd.c:6651: warning: Excess function parameter 'cmd' description in 'ufshcd_try_to_abort_task'

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ce0528f2e2ed..79287fdd049b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6652,7 +6652,8 @@ static void ufshcd_set_req_abort_skip(struct ufs_hba *hba, unsigned long bitmap)
 
 /**
  * ufshcd_try_to_abort_task - abort a specific task
- * @cmd: SCSI command pointer
+ * @hba: per adapter instance
+ * @tag: position of the bit to be aborted
  *
  * Abort the pending command in device by sending UFS_ABORT_TASK task management
  * command, and in host controller by clearing the door-bell register. There can
-- 
2.18.0

Re: [PATCH v2 4/4] scsi: ufs: Fix build warning by incorrect function description

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> Fix build warnings as below due to incorrect function description
> of ufshcd_try_to_abort_task().
> 
> ufshcd.c:6651: warning: Function parameter or member 'hba' not
> described in 'ufshcd_try_to_abort_task'
> ufshcd.c:6651: warning: Function parameter or member 'tag' not
> described in 'ufshcd_try_to_abort_task'
> ufshcd.c:6651: warning: Excess function parameter 'cmd' description in
> 'ufshcd_try_to_abort_task'
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ce0528f2e2ed..79287fdd049b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6652,7 +6652,8 @@ static void ufshcd_set_req_abort_skip(struct
> ufs_hba *hba, unsigned long bitmap)
> 
>  /**
>   * ufshcd_try_to_abort_task - abort a specific task
> - * @cmd: SCSI command pointer
> + * @hba: per adapter instance
> + * @tag: position of the bit to be aborted
>   *
>   * Abort the pending command in device by sending UFS_ABORT_TASK task
> management
>   * command, and in host controller by clearing the door-bell
> register. There can

Thanks for the fix.

Reviewed-by: Can Guo <cang@codeaurora.org>

Re: [PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[Can Guo]

On 2020-12-16 21:16, Stanley Chu wrote:
> Hi,
> This series cleans up and refactors clk-scaling feature, and shall not
> change any functionality.
> 
> This series is based on Can's series "Three changes related with UFS
> clock scaling" in 5.10/scsi-fixes branch in Martin's tree.
> 

Hi Stanley,

Thanks for noticing my changes, will you review them?
I see customers manipulte UFS scaling related sysfs
nodes more often than before, so we may want to fix it asap.

Regards,

Can Guo.

> However this series may not be required to be merged to 5.10. The
> choice of base branch is simply making these patches easy to be
> reviewed because this series is based on clk-scaling fixes by Can. If
> this series is decided not being merged to 5.10, then I would rebase
> it to 5.11/scsi-queue.
> 
> Changes since v1:
>   - Refactor ufshcd_clk_scaling_suspend() in patch [3/4]
>   - Change function name from ufshcd_clk_scaling_pm() to
> ufshcd_clk_scaling_suspend() in patch [3/4]
>   - Refine patch titles
> 
> Stanley Chu (4):
>   scsi: ufs: Refactor cancelling clkscaling works
>   scsi: ufs: Remove redundant null checking of devfreq instance
>   scsi: ufs: Cleanup and refactor clk-scaling feature
>   scsi: ufs: Fix build warning by incorrect function description
> 
>  drivers/scsi/ufs/ufshcd.c | 90 +++++++++++++++++++--------------------
>  1 file changed, 43 insertions(+), 47 deletions(-)

Re: [PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[Stanley Chu]

Hi Can,

On Fri, 2020-12-18 at 14:20 +0800, Can Guo wrote:
> On 2020-12-16 21:16, Stanley Chu wrote:
> > Hi,
> > This series cleans up and refactors clk-scaling feature, and shall not
> > change any functionality.
> > 
> > This series is based on Can's series "Three changes related with UFS
> > clock scaling" in 5.10/scsi-fixes branch in Martin's tree.
> > 
> 
> Hi Stanley,
> 
> Thanks for noticing my changes, will you review them?
> I see customers manipulte UFS scaling related sysfs
> nodes more often than before, so we may want to fix it asap.

I have gave my review tag in all patches in this series : )

Thanks,
Stanley Chu

> 
> Regards,
> 
> Can Guo.
> 
> > However this series may not be required to be merged to 5.10. The
> > choice of base branch is simply making these patches easy to be
> > reviewed because this series is based on clk-scaling fixes by Can. If
> > this series is decided not being merged to 5.10, then I would rebase
> > it to 5.11/scsi-queue.
> > 
> > Changes since v1:
> >   - Refactor ufshcd_clk_scaling_suspend() in patch [3/4]
> >   - Change function name from ufshcd_clk_scaling_pm() to
> > ufshcd_clk_scaling_suspend() in patch [3/4]
> >   - Refine patch titles
> > 
> > Stanley Chu (4):
> >   scsi: ufs: Refactor cancelling clkscaling works
> >   scsi: ufs: Remove redundant null checking of devfreq instance
> >   scsi: ufs: Cleanup and refactor clk-scaling feature
> >   scsi: ufs: Fix build warning by incorrect function description
> > 
> >  drivers/scsi/ufs/ufshcd.c | 90 +++++++++++++++++++--------------------
> >  1 file changed, 43 insertions(+), 47 deletions(-)

Re: [PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[Can Guo]

On 2020-12-18 14:24, Stanley Chu wrote:
> Hi Can,
> 
> On Fri, 2020-12-18 at 14:20 +0800, Can Guo wrote:
>> On 2020-12-16 21:16, Stanley Chu wrote:
>> > Hi,
>> > This series cleans up and refactors clk-scaling feature, and shall not
>> > change any functionality.
>> >
>> > This series is based on Can's series "Three changes related with UFS
>> > clock scaling" in 5.10/scsi-fixes branch in Martin's tree.
>> >
>> 
>> Hi Stanley,
>> 
>> Thanks for noticing my changes, will you review them?
>> I see customers manipulte UFS scaling related sysfs
>> nodes more often than before, so we may want to fix it asap.
> 
> I have gave my review tag in all patches in this series : )
> 
> Thanks,
> Stanley Chu
> 

Hi Stanley,

oops, just saw it - I opened the wrong series.
I will push a new version which incorporates your comments soon.
Since the new changes would only be some words in ufshcd.h, so you
won't need to rebase.

Thanks,

Can Guo.

>> 
>> Regards,
>> 
>> Can Guo.
>> 
>> > However this series may not be required to be merged to 5.10. The
>> > choice of base branch is simply making these patches easy to be
>> > reviewed because this series is based on clk-scaling fixes by Can. If
>> > this series is decided not being merged to 5.10, then I would rebase
>> > it to 5.11/scsi-queue.
>> >
>> > Changes since v1:
>> >   - Refactor ufshcd_clk_scaling_suspend() in patch [3/4]
>> >   - Change function name from ufshcd_clk_scaling_pm() to
>> > ufshcd_clk_scaling_suspend() in patch [3/4]
>> >   - Refine patch titles
>> >
>> > Stanley Chu (4):
>> >   scsi: ufs: Refactor cancelling clkscaling works
>> >   scsi: ufs: Remove redundant null checking of devfreq instance
>> >   scsi: ufs: Cleanup and refactor clk-scaling feature
>> >   scsi: ufs: Fix build warning by incorrect function description
>> >
>> >  drivers/scsi/ufs/ufshcd.c | 90 +++++++++++++++++++--------------------
>> >  1 file changed, 43 insertions(+), 47 deletions(-)

Re: [PATCH v2 0/4] scsi: ufs: Cleanup and refactor clock scaling

[Martin K. Petersen]

Stanley,

> However this series may not be required to be merged to 5.10. The
> choice of base branch is simply making these patches easy to be
> reviewed because this series is based on clk-scaling fixes by Can.

Please redo your series on top of 5.12/scsi-queue + Can's changes.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering