[PATCH] ufs: Fix handling of active power mode in ufshcd_suspend()

[PATCH] ufs: Fix handling of active power mode in ufshcd_suspend()

[Bart Van Assche]

If the rpm_lvl and spm_lvl sysfs attributes indicate that ufshcd_suspend()
should keep the link active, re-enable clock gating instead of disabling
clocks.

Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Can Guo <cang@codeaurora.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f540b0cc253f..c96e36aab989 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8690,9 +8690,8 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ufshcd_clk_scaling_suspend(hba, true);
 
 	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
-			req_link_state == UIC_LINK_ACTIVE_STATE) {
-		goto disable_clks;
-	}
+			req_link_state == UIC_LINK_ACTIVE_STATE)
+		goto enable_gating;
 
 	if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
 	    (req_link_state == hba->uic_link_state))
@@ -8754,7 +8753,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (ret)
 		goto set_dev_active;
 
-disable_clks:
 	/*
 	 * Call vendor specific suspend callback. As these callbacks may access
 	 * vendor specific host controller register space call them before the

Re: [PATCH] ufs: Fix handling of active power mode in ufshcd_suspend()

[Can Guo]

On 2021-05-13 03:57, Bart Van Assche wrote:
> If the rpm_lvl and spm_lvl sysfs attributes indicate that 
> ufshcd_suspend()
> should keep the link active, re-enable clock gating instead of 
> disabling
> clocks.
> 
> Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f540b0cc253f..c96e36aab989 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8690,9 +8690,8 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	ufshcd_clk_scaling_suspend(hba, true);
> 
>  	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
> -			req_link_state == UIC_LINK_ACTIVE_STATE) {
> -		goto disable_clks;
> -	}
> +			req_link_state == UIC_LINK_ACTIVE_STATE)
> +		goto enable_gating;
> 
>  	if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
>  	    (req_link_state == hba->uic_link_state))
> @@ -8754,7 +8753,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
> enum ufs_pm_op pm_op)
>  	if (ret)
>  		goto set_dev_active;
> 
> -disable_clks:
>  	/*
>  	 * Call vendor specific suspend callback. As these callbacks may 
> access
>  	 * vendor specific host controller register space call them before 
> the

Hi Bart,

The change is unnecessary, ufshcd_suspend() is indeed keeping link alive 
even if
we are disabling clocks. In __ufshcd_setup_clocks(), we have checks on 
clock sources
so that we leave certain clock sources ON if the link is alive. And we 
have many
of our customers tested the case rpm/spm_lvl == 0, it is working well. 
Please check
below changes:

https://lore.kernel.org/patchwork/patch/1345336/
https://lore.kernel.org/patchwork/patch/1345337/

With this change, after suspend (rpm/spm_lvl == 0), leaving clock gating 
running is risky:
1. clock gating may run into concurrency with resume.
2. In ufshcd_resume(), we also have a ufshcd_release(), it will cause 
unbalanced usage of clock gating.

And it seems quite opposite from what you want - you want to keep link
alive but you are leaving clock gating enabled, then when clock gating
kicks start, it will put the link into hibern8, but not keep link alive.

Thanks,
Can Guo.

Re: [PATCH] ufs: Fix handling of active power mode in ufshcd_suspend()

[Bart Van Assche]

On 5/12/21 6:35 PM, Can Guo wrote:
> The change is unnecessary, ufshcd_suspend() is indeed keeping link alive
> even if
> we are disabling clocks. In __ufshcd_setup_clocks(), we have checks on
> clock sources
> so that we leave certain clock sources ON if the link is alive. And we
> have many
> of our customers tested the case rpm/spm_lvl == 0, it is working well.
> Please check
> below changes:
> 
> https://lore.kernel.org/patchwork/patch/1345336/
> https://lore.kernel.org/patchwork/patch/1345337/
> 
> With this change, after suspend (rpm/spm_lvl == 0), leaving clock gating
> running is risky:
> 1. clock gating may run into concurrency with resume.
> 2. In ufshcd_resume(), we also have a ufshcd_release(), it will cause
> unbalanced usage of clock gating.
> 
> And it seems quite opposite from what you want - you want to keep link
> alive but you are leaving clock gating enabled, then when clock gating
> kicks start, it will put the link into hibern8, but not keep link alive.

Hi Can,

Thank you for the detailed feedback. I will drop this patch.

Bart.