Hi,
This series fixes up an issue that WB is not re-enabled after device reset.
Stanley Chu (2):
scsi: ufs: Re-enable WriteBooster after device reset
scsi: ufs: Uninline ufshcd_vops_device_reset function
drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
drivers/scsi/ufs/ufshcd.h | 14 +++++---------
2 files changed, 27 insertions(+), 14 deletions(-)
--
2.18.0
UFS 3.1 specification mentions that below WriteBooster flags will be
set to default value, say disabled, after power cycle or any type
of reset event. Thus we need to reset those flag variables kept
in struct hba to align the device status and ensure WriteBooster
related functions being configured properly after device reset.
Without this fix, WriteBooster will not be enabled successfully
after by ufshcd_wb_ctrl() after device reset because hba->wb_enabled
remains true.
Flags required to be reset to default values:
- fWriteBoosterEn: hba->wb_enabled
- fWriteBoosterBufferFlushEn: hba->wb_buf_flush_enabled
- fWriteBoosterBufferFlushDuringHibernate: No variable mapped
Fixes: 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support")
Signed-off-by: Stanley Chu <[email protected]>
---
drivers/scsi/ufs/ufshcd.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 08c8a591e6b0..36d367eb8139 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1221,8 +1221,13 @@ static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
if (hba->vops && hba->vops->device_reset) {
int err = hba->vops->device_reset(hba);
- if (!err)
+ if (!err) {
ufshcd_set_ufs_dev_active(hba);
+ if (ufshcd_is_wb_allowed(hba)) {
+ hba->wb_enabled = false;
+ hba->wb_buf_flush_enabled = false;
+ }
+ }
if (err != -EOPNOTSUPP)
ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
}
--
2.18.0
Since more and more statements showing up in ufshcd_vops_device_reset(),
uninline it to allow compiler making possibly better optimization.
Signed-off-by: Stanley Chu <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
drivers/scsi/ufs/ufshcd.h | 19 +++++--------------
2 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c1c401b2b69d..b2ca1a6ad426 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -580,6 +580,23 @@ static void ufshcd_print_pwr_info(struct ufs_hba *hba)
hba->pwr_info.hs_rate);
}
+static void ufshcd_device_reset(struct ufs_hba *hba)
+{
+ int err;
+
+ err = ufshcd_vops_device_reset(hba);
+
+ if (!err) {
+ ufshcd_set_ufs_dev_active(hba);
+ if (ufshcd_is_wb_allowed(hba)) {
+ hba->wb_enabled = false;
+ hba->wb_buf_flush_enabled = false;
+ }
+ }
+ if (err != -EOPNOTSUPP)
+ ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
+}
+
void ufshcd_delay_us(unsigned long us, unsigned long tolerance)
{
if (!us)
@@ -3932,7 +3949,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
spin_unlock_irqrestore(hba->host->host_lock, flags);
/* Reset the attached device */
- ufshcd_vops_device_reset(hba);
+ ufshcd_device_reset(hba);
ret = ufshcd_host_reset_and_restore(hba);
@@ -6933,7 +6950,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
do {
/* Reset the attached device */
- ufshcd_vops_device_reset(hba);
+ ufshcd_device_reset(hba);
err = ufshcd_host_reset_and_restore(hba);
} while (err && --retries);
@@ -8712,7 +8729,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
* further below.
*/
if (ufshcd_is_ufs_dev_deepsleep(hba)) {
- ufshcd_vops_device_reset(hba);
+ ufshcd_device_reset(hba);
WARN_ON(!ufshcd_is_link_off(hba));
}
if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
@@ -8722,7 +8739,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
set_dev_active:
/* Can also get here needing to exit DeepSleep */
if (ufshcd_is_ufs_dev_deepsleep(hba)) {
- ufshcd_vops_device_reset(hba);
+ ufshcd_device_reset(hba);
ufshcd_host_reset_and_restore(hba);
}
if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
@@ -9321,7 +9338,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
}
/* Reset the attached device */
- ufshcd_vops_device_reset(hba);
+ ufshcd_device_reset(hba);
ufshcd_init_crypto(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 36d367eb8139..9bb5f0ed4124 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1216,21 +1216,12 @@ static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
hba->vops->dbg_register_dump(hba);
}
-static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
+static inline int ufshcd_vops_device_reset(struct ufs_hba *hba)
{
- if (hba->vops && hba->vops->device_reset) {
- int err = hba->vops->device_reset(hba);
-
- if (!err) {
- ufshcd_set_ufs_dev_active(hba);
- if (ufshcd_is_wb_allowed(hba)) {
- hba->wb_enabled = false;
- hba->wb_buf_flush_enabled = false;
- }
- }
- if (err != -EOPNOTSUPP)
- ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
- }
+ if (hba->vops && hba->vops->device_reset)
+ return hba->vops->device_reset(hba);
+
+ return -EOPNOTSUPP;
}
static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
--
2.18.0
On Tue, 2020-12-08 at 21:56 +0800, Stanley Chu wrote:
> Since more and more statements showing up in
> ufshcd_vops_device_reset(),
> uninline it to allow compiler making possibly better optimization.
>
> Signed-off-by: Stanley Chu <[email protected]>
reviewed-by: Bean Huo <[email protected]>
On Tue, 2020-12-08 at 21:56 +0800, Stanley Chu wrote:
> index 08c8a591e6b0..36d367eb8139 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1221,8 +1221,13 @@ static inline void
> ufshcd_vops_device_reset(struct ufs_hba *hba)
> if (hba->vops && hba->vops->device_reset) {
> int err = hba->vops->device_reset(hba);
>
> - if (!err)
> + if (!err) {
> ufshcd_set_ufs_dev_active(hba);
> + if (ufshcd_is_wb_allowed(hba)) {
> + hba->wb_enabled = false;
> + hba->wb_buf_flush_enabled = false;
> + }
> + }
Stanley,
how do you think group wb_buf_flush_enabled and wb_enabled to the
dev_info, since they are UFS device attributes. means they are set only
when UFS device flags being set.
Reviewed-by: Bean Huo <[email protected]>
On Tue, 2020-12-08 at 15:13 +0100, Bean Huo wrote:
> On Tue, 2020-12-08 at 21:56 +0800, Stanley Chu wrote:
> > index 08c8a591e6b0..36d367eb8139 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -1221,8 +1221,13 @@ static inline void
> > ufshcd_vops_device_reset(struct ufs_hba *hba)
> > if (hba->vops && hba->vops->device_reset) {
> > int err = hba->vops->device_reset(hba);
> >
> > - if (!err)
> > + if (!err) {
> > ufshcd_set_ufs_dev_active(hba);
> > + if (ufshcd_is_wb_allowed(hba)) {
> > + hba->wb_enabled = false;
> > + hba->wb_buf_flush_enabled = false;
> > + }
> > + }
>
> Stanley,
> how do you think group wb_buf_flush_enabled and wb_enabled to the
> dev_info, since they are UFS device attributes. means they are set only
> when UFS device flags being set.
Hi Bean,
Thanks for your review.
Yes, I agreed that wb related variables is a mess currently. I would
like to clean them up once I have time. Feel free to post your patch if
you want to take it up : )
Thanks,
Stanley Chu
>
> Reviewed-by: Bean Huo <[email protected]>
>
Stanley,
> This series fixes up an issue that WB is not re-enabled after device
> reset.
Applied to 5.11/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
On Wed, 2020-12-09 at 09:09 +0800, Stanley Chu wrote:
> > >
> > > - if (!err)
> > > + if (!err) {
> > > ufshcd_set_ufs_dev_active(hba);
> > > + if (ufshcd_is_wb_allowed(hba)) {
> > > + hba->wb_enabled = false;
> > > + hba->wb_buf_flush_enabled =
> > > false;
> > > + }
> > > + }
> >
> > Stanley,
> > how do you think group wb_buf_flush_enabled and wb_enabled to the
> > dev_info, since they are UFS device attributes. means they are set
> > only
> > when UFS device flags being set.
>
> Hi Bean,
>
> Thanks for your review.
>
> Yes, I agreed that wb related variables is a mess currently. I would
> like to clean them up once I have time. Feel free to post your patch
> if
> you want to take it up : )
>
Hi Stanley
I updated this change in my new "Several changes for UFS WriteBooster"
series patch, are you interested in reviewing that? to help Martin
easier pick up the changes.
Thanks,
Bean
On 12/8/2020 5:56 AM, Stanley Chu wrote:
> Since more and more statements showing up in ufshcd_vops_device_reset(),
> uninline it to allow compiler making possibly better optimization.
>
> Signed-off-by: Stanley Chu <[email protected]>
> ---
Reviewed-by: Asutosh Das <[email protected]>
> drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++++++++++++-----
> drivers/scsi/ufs/ufshcd.h | 19 +++++--------------
> 2 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c1c401b2b69d..b2ca1a6ad426 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -580,6 +580,23 @@ static void ufshcd_print_pwr_info(struct ufs_hba *hba)
> hba->pwr_info.hs_rate);
> }
>
> +static void ufshcd_device_reset(struct ufs_hba *hba)
> +{
> + int err;
> +
> + err = ufshcd_vops_device_reset(hba);
> +
> + if (!err) {
> + ufshcd_set_ufs_dev_active(hba);
> + if (ufshcd_is_wb_allowed(hba)) {
> + hba->wb_enabled = false;
> + hba->wb_buf_flush_enabled = false;
> + }
> + }
> + if (err != -EOPNOTSUPP)
> + ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
> +}
> +
> void ufshcd_delay_us(unsigned long us, unsigned long tolerance)
> {
> if (!us)
> @@ -3932,7 +3949,7 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> /* Reset the attached device */
> - ufshcd_vops_device_reset(hba);
> + ufshcd_device_reset(hba);
>
> ret = ufshcd_host_reset_and_restore(hba);
>
> @@ -6933,7 +6950,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>
> do {
> /* Reset the attached device */
> - ufshcd_vops_device_reset(hba);
> + ufshcd_device_reset(hba);
>
> err = ufshcd_host_reset_and_restore(hba);
> } while (err && --retries);
> @@ -8712,7 +8729,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> * further below.
> */
> if (ufshcd_is_ufs_dev_deepsleep(hba)) {
> - ufshcd_vops_device_reset(hba);
> + ufshcd_device_reset(hba);
> WARN_ON(!ufshcd_is_link_off(hba));
> }
> if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
> @@ -8722,7 +8739,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> set_dev_active:
> /* Can also get here needing to exit DeepSleep */
> if (ufshcd_is_ufs_dev_deepsleep(hba)) {
> - ufshcd_vops_device_reset(hba);
> + ufshcd_device_reset(hba);
> ufshcd_host_reset_and_restore(hba);
> }
> if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
> @@ -9321,7 +9338,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> }
>
> /* Reset the attached device */
> - ufshcd_vops_device_reset(hba);
> + ufshcd_device_reset(hba);
>
> ufshcd_init_crypto(hba);
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 36d367eb8139..9bb5f0ed4124 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1216,21 +1216,12 @@ static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
> hba->vops->dbg_register_dump(hba);
> }
>
> -static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
> +static inline int ufshcd_vops_device_reset(struct ufs_hba *hba)
> {
> - if (hba->vops && hba->vops->device_reset) {
> - int err = hba->vops->device_reset(hba);
> -
> - if (!err) {
> - ufshcd_set_ufs_dev_active(hba);
> - if (ufshcd_is_wb_allowed(hba)) {
> - hba->wb_enabled = false;
> - hba->wb_buf_flush_enabled = false;
> - }
> - }
> - if (err != -EOPNOTSUPP)
> - ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
> - }
> + if (hba->vops && hba->vops->device_reset)
> + return hba->vops->device_reset(hba);
> +
> + return -EOPNOTSUPP;
> }
>
> static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project