| Message ID | 20210512195721.8157-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ufs: Fix handling of active power mode in ufshcd_suspend() | expand |
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.
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.
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
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(-)