| Message ID | 20200516174615.15445-6-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Fix WriteBooster and cleanup UFS driver | expand |
Hi Stanley, On 5/16/2020 10:46 AM, Stanley Chu wrote: > The commit "scsi: ufs: Fix WriteBooster flush during runtime > suspend" promises essential resource, i.e., for UFS devices doing > WriteBooster buffer flush and Auto BKOPs. However if device > finishes its job but not resumed for a very long time, system > will have unnecessary power drain because VCC is still supplied. > > To fix this, a method to recheck the threshold of keeping VCC > supply is required. However, the threshold recheck needs to > re-activate the link because the decision depends on the device > status. > > Introduce a delayed work to force runtime resume after a certain > delay during runtime suspend. This makes threshold recheck simpler > which will be done in the next runtime-suspend. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > --- Is there a reason to have this code as a separate patch? [1] Commit: "scsi: ufs: Fix WriteBooster flush during runtime suspend" introduces 'keep_curr_dev_pwr_mode' and the very next change (this one) removes it. Do you think this change and [1] should be merged? > drivers/scsi/ufs/ufs.h | 1 + > drivers/scsi/ufs/ufshcd.c | 43 ++++++++++++++++++++++++++++++++++----- > drivers/scsi/ufs/ufshcd.h | 1 + > 3 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h > index db07eedfed96..c70845d41449 100644 > --- a/drivers/scsi/ufs/ufs.h > +++ b/drivers/scsi/ufs/ufs.h > @@ -574,6 +574,7 @@ struct ufs_dev_info { > u32 d_ext_ufs_feature_sup; > u8 b_wb_buffer_type; > u32 d_wb_alloc_units; > + bool b_rpm_dev_flush_capable; > u8 b_presrv_uspc_en; > }; > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index f4f2c7b5ab0a..a137553f9b41 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -94,6 +94,9 @@ > /* default delay of autosuspend: 2000 ms */ > #define RPM_AUTOSUSPEND_DELAY_MS 2000 > > +/* Default delay of RPM device flush delayed work */ > +#define RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS 5000 > + > /* Default value of wait time before gating device ref clock */ > #define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */ > > @@ -5310,7 +5313,7 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, > return false; > } > > -static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) > +static bool ufshcd_wb_need_flush(struct ufs_hba *hba) > { > int ret; > u32 avail_buf; > @@ -5348,6 +5351,21 @@ static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) > return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf); > } > > +static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work) > +{ > + struct ufs_hba *hba = container_of(to_delayed_work(work), > + struct ufs_hba, > + rpm_dev_flush_recheck_work); > + /* > + * To prevent unnecessary VCC power drain after device finishes > + * WriteBooster buffer flush or Auto BKOPs, force runtime resume > + * after a certain delay to recheck the threshold by next runtime > + * supsend. > + */ > + pm_runtime_get_sync(hba->dev); > + pm_runtime_put_sync(hba->dev); > +} > + > /** > * ufshcd_exception_event_handler - handle exceptions raised by device > * @work: pointer to work data > @@ -8164,7 +8182,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > enum ufs_pm_level pm_lvl; > enum ufs_dev_pwr_mode req_dev_pwr_mode; > enum uic_link_state req_link_state; > - bool keep_curr_dev_pwr_mode = false; > > hba->pm_op_in_progress = 1; > if (!ufshcd_is_shutdown_pm(pm_op)) { > @@ -8224,11 +8241,12 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > * Hibern8, keep device power mode as "active power mode" > * and VCC supply. > */ > - keep_curr_dev_pwr_mode = hba->auto_bkops_enabled || > + hba->dev_info.b_rpm_dev_flush_capable = > + hba->auto_bkops_enabled || > (((req_link_state == UIC_LINK_HIBERN8_STATE) || > ((req_link_state == UIC_LINK_ACTIVE_STATE) && > ufshcd_is_auto_hibern8_enabled(hba))) && > - ufshcd_wb_keep_vcc_on(hba)); > + ufshcd_wb_need_flush(hba)); > } > > if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { > @@ -8238,7 +8256,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > ufshcd_disable_auto_bkops(hba); > } > > - if (!keep_curr_dev_pwr_mode) { > + if (!hba->dev_info.b_rpm_dev_flush_capable) { > ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); > if (ret) > goto enable_gating; > @@ -8295,9 +8313,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > if (hba->clk_scaling.is_allowed) > ufshcd_resume_clkscaling(hba); > hba->clk_gating.is_suspended = false; > + hba->dev_info.b_rpm_dev_flush_capable = false; > ufshcd_release(hba); > out: > + if (hba->dev_info.b_rpm_dev_flush_capable) { > + schedule_delayed_work(&hba->rpm_dev_flush_recheck_work, > + msecs_to_jiffies(RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS)); > + } > + > hba->pm_op_in_progress = 0; > + Nitpick; newline, perhaps? > if (ret) > ufshcd_update_reg_hist(&hba->ufs_stats.suspend_err, (u32)ret); > return ret; > @@ -8386,6 +8411,11 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > /* Enable Auto-Hibernate if configured */ > ufshcd_auto_hibern8_enable(hba); > > + if (hba->dev_info.b_rpm_dev_flush_capable) { > + hba->dev_info.b_rpm_dev_flush_capable = false; > + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work); > + } > + > /* Schedule clock gating in case of no access to UFS device yet */ > ufshcd_release(hba); > > @@ -8859,6 +8889,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) > UFS_SLEEP_PWR_MODE, > UIC_LINK_HIBERN8_STATE); > > + INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work, > + ufshcd_rpm_dev_flush_recheck_work); > + > /* Set the default auto-hiberate idle timer value to 150 ms */ > if (ufshcd_is_auto_hibern8_supported(hba) && !hba->ahit) { > hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) | > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 8db7a6101892..9acd437037e8 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -745,6 +745,7 @@ struct ufs_hba { > struct request_queue *bsg_queue; > bool wb_buf_flush_enabled; > bool wb_enabled; > + struct delayed_work rpm_dev_flush_recheck_work; > }; > > /* Returns true if clocks can be gated. Otherwise false */ >
Hi, Asutosh, Thanks for your review. On Tue, 2020-05-19 at 09:27 -0700, Asutosh Das (asd) wrote: > Hi Stanley, > > On 5/16/2020 10:46 AM, Stanley Chu wrote: > > The commit "scsi: ufs: Fix WriteBooster flush during runtime > > suspend" promises essential resource, i.e., for UFS devices doing > > WriteBooster buffer flush and Auto BKOPs. However if device > > finishes its job but not resumed for a very long time, system > > will have unnecessary power drain because VCC is still supplied. > > > > To fix this, a method to recheck the threshold of keeping VCC > > supply is required. However, the threshold recheck needs to > > re-activate the link because the decision depends on the device > > status. > > > > Introduce a delayed work to force runtime resume after a certain > > delay during runtime suspend. This makes threshold recheck simpler > > which will be done in the next runtime-suspend. > > > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > > --- > > Is there a reason to have this code as a separate patch? > [1] Commit: "scsi: ufs: Fix WriteBooster flush during runtime suspend" > introduces 'keep_curr_dev_pwr_mode' and the very next change (this one) > removes it. > Do you think this change and [1] should be merged? Yes, these 2 patches shall be merged. I will do it in next version. > > > drivers/scsi/ufs/ufs.h | 1 + > > drivers/scsi/ufs/ufshcd.c | 43 ++++++++++++++++++++++++++++++++++----- > > drivers/scsi/ufs/ufshcd.h | 1 + > > 3 files changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h > > index db07eedfed96..c70845d41449 100644 > > --- a/drivers/scsi/ufs/ufs.h > > +++ b/drivers/scsi/ufs/ufs.h > > @@ -574,6 +574,7 @@ struct ufs_dev_info { > > u32 d_ext_ufs_feature_sup; > > u8 b_wb_buffer_type; > > u32 d_wb_alloc_units; > > + bool b_rpm_dev_flush_capable; > > u8 b_presrv_uspc_en; > > }; > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index f4f2c7b5ab0a..a137553f9b41 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -94,6 +94,9 @@ > > /* default delay of autosuspend: 2000 ms */ > > #define RPM_AUTOSUSPEND_DELAY_MS 2000 > > > > +/* Default delay of RPM device flush delayed work */ > > +#define RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS 5000 > > + > > /* Default value of wait time before gating device ref clock */ > > #define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */ > > > > @@ -5310,7 +5313,7 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, > > return false; > > } > > > > -static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) > > +static bool ufshcd_wb_need_flush(struct ufs_hba *hba) > > { > > int ret; > > u32 avail_buf; > > @@ -5348,6 +5351,21 @@ static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) > > return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf); > > } > > > > +static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work) > > +{ > > + struct ufs_hba *hba = container_of(to_delayed_work(work), > > + struct ufs_hba, > > + rpm_dev_flush_recheck_work); > > + /* > > + * To prevent unnecessary VCC power drain after device finishes > > + * WriteBooster buffer flush or Auto BKOPs, force runtime resume > > + * after a certain delay to recheck the threshold by next runtime > > + * supsend. > > + */ > > + pm_runtime_get_sync(hba->dev); > > + pm_runtime_put_sync(hba->dev); > > +} > > + > > /** > > * ufshcd_exception_event_handler - handle exceptions raised by device > > * @work: pointer to work data > > @@ -8164,7 +8182,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > enum ufs_pm_level pm_lvl; > > enum ufs_dev_pwr_mode req_dev_pwr_mode; > > enum uic_link_state req_link_state; > > - bool keep_curr_dev_pwr_mode = false; > > > > hba->pm_op_in_progress = 1; > > if (!ufshcd_is_shutdown_pm(pm_op)) { > > @@ -8224,11 +8241,12 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > * Hibern8, keep device power mode as "active power mode" > > * and VCC supply. > > */ > > - keep_curr_dev_pwr_mode = hba->auto_bkops_enabled || > > + hba->dev_info.b_rpm_dev_flush_capable = > > + hba->auto_bkops_enabled || > > (((req_link_state == UIC_LINK_HIBERN8_STATE) || > > ((req_link_state == UIC_LINK_ACTIVE_STATE) && > > ufshcd_is_auto_hibern8_enabled(hba))) && > > - ufshcd_wb_keep_vcc_on(hba)); > > + ufshcd_wb_need_flush(hba)); > > } > > > > if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { > > @@ -8238,7 +8256,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > ufshcd_disable_auto_bkops(hba); > > } > > > > - if (!keep_curr_dev_pwr_mode) { > > + if (!hba->dev_info.b_rpm_dev_flush_capable) { > > ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); > > if (ret) > > goto enable_gating; > > @@ -8295,9 +8313,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > if (hba->clk_scaling.is_allowed) > > ufshcd_resume_clkscaling(hba); > > hba->clk_gating.is_suspended = false; > > + hba->dev_info.b_rpm_dev_flush_capable = false; > > ufshcd_release(hba); > > out: > > + if (hba->dev_info.b_rpm_dev_flush_capable) { > > + schedule_delayed_work(&hba->rpm_dev_flush_recheck_work, > > + msecs_to_jiffies(RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS)); > > + } > > + > > hba->pm_op_in_progress = 0; > > + > Nitpick; newline, perhaps? Thanks, I Will remove it. > > > if (ret) > > ufshcd_update_reg_hist(&hba->ufs_stats.suspend_err, (u32)ret); > > return ret; > > @@ -8386,6 +8411,11 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > /* Enable Auto-Hibernate if configured */ > > ufshcd_auto_hibern8_enable(hba); > > > > + if (hba->dev_info.b_rpm_dev_flush_capable) { > > + hba->dev_info.b_rpm_dev_flush_capable = false; > > + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work); > > + } > > + > > /* Schedule clock gating in case of no access to UFS device yet */ > > ufshcd_release(hba); > > > > @@ -8859,6 +8889,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) > > UFS_SLEEP_PWR_MODE, > > UIC_LINK_HIBERN8_STATE); > > > > + INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work, > > + ufshcd_rpm_dev_flush_recheck_work); > > + > > /* Set the default auto-hiberate idle timer value to 150 ms */ > > if (ufshcd_is_auto_hibern8_supported(hba) && !hba->ahit) { > > hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) | > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > > index 8db7a6101892..9acd437037e8 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -745,6 +745,7 @@ struct ufs_hba { > > struct request_queue *bsg_queue; > > bool wb_buf_flush_enabled; > > bool wb_enabled; > > + struct delayed_work rpm_dev_flush_recheck_work; > > }; > > > > /* Returns true if clocks can be gated. Otherwise false */ > > > > Thanks, Stanley Chu
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index db07eedfed96..c70845d41449 100644 --- a/drivers/scsi/ufs/ufs.h +++ b/drivers/scsi/ufs/ufs.h @@ -574,6 +574,7 @@ struct ufs_dev_info { u32 d_ext_ufs_feature_sup; u8 b_wb_buffer_type; u32 d_wb_alloc_units; + bool b_rpm_dev_flush_capable; u8 b_presrv_uspc_en; }; diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index f4f2c7b5ab0a..a137553f9b41 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -94,6 +94,9 @@ /* default delay of autosuspend: 2000 ms */ #define RPM_AUTOSUSPEND_DELAY_MS 2000 +/* Default delay of RPM device flush delayed work */ +#define RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS 5000 + /* Default value of wait time before gating device ref clock */ #define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */ @@ -5310,7 +5313,7 @@ static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, return false; } -static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) +static bool ufshcd_wb_need_flush(struct ufs_hba *hba) { int ret; u32 avail_buf; @@ -5348,6 +5351,21 @@ static bool ufshcd_wb_keep_vcc_on(struct ufs_hba *hba) return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf); } +static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work) +{ + struct ufs_hba *hba = container_of(to_delayed_work(work), + struct ufs_hba, + rpm_dev_flush_recheck_work); + /* + * To prevent unnecessary VCC power drain after device finishes + * WriteBooster buffer flush or Auto BKOPs, force runtime resume + * after a certain delay to recheck the threshold by next runtime + * supsend. + */ + pm_runtime_get_sync(hba->dev); + pm_runtime_put_sync(hba->dev); +} + /** * ufshcd_exception_event_handler - handle exceptions raised by device * @work: pointer to work data @@ -8164,7 +8182,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) enum ufs_pm_level pm_lvl; enum ufs_dev_pwr_mode req_dev_pwr_mode; enum uic_link_state req_link_state; - bool keep_curr_dev_pwr_mode = false; hba->pm_op_in_progress = 1; if (!ufshcd_is_shutdown_pm(pm_op)) { @@ -8224,11 +8241,12 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) * Hibern8, keep device power mode as "active power mode" * and VCC supply. */ - keep_curr_dev_pwr_mode = hba->auto_bkops_enabled || + hba->dev_info.b_rpm_dev_flush_capable = + hba->auto_bkops_enabled || (((req_link_state == UIC_LINK_HIBERN8_STATE) || ((req_link_state == UIC_LINK_ACTIVE_STATE) && ufshcd_is_auto_hibern8_enabled(hba))) && - ufshcd_wb_keep_vcc_on(hba)); + ufshcd_wb_need_flush(hba)); } if (req_dev_pwr_mode != hba->curr_dev_pwr_mode) { @@ -8238,7 +8256,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) ufshcd_disable_auto_bkops(hba); } - if (!keep_curr_dev_pwr_mode) { + if (!hba->dev_info.b_rpm_dev_flush_capable) { ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); if (ret) goto enable_gating; @@ -8295,9 +8313,16 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (hba->clk_scaling.is_allowed) ufshcd_resume_clkscaling(hba); hba->clk_gating.is_suspended = false; + hba->dev_info.b_rpm_dev_flush_capable = false; ufshcd_release(hba); out: + if (hba->dev_info.b_rpm_dev_flush_capable) { + schedule_delayed_work(&hba->rpm_dev_flush_recheck_work, + msecs_to_jiffies(RPM_DEV_FLUSH_RECHECK_WORK_DELAY_MS)); + } + hba->pm_op_in_progress = 0; + if (ret) ufshcd_update_reg_hist(&hba->ufs_stats.suspend_err, (u32)ret); return ret; @@ -8386,6 +8411,11 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) /* Enable Auto-Hibernate if configured */ ufshcd_auto_hibern8_enable(hba); + if (hba->dev_info.b_rpm_dev_flush_capable) { + hba->dev_info.b_rpm_dev_flush_capable = false; + cancel_delayed_work(&hba->rpm_dev_flush_recheck_work); + } + /* Schedule clock gating in case of no access to UFS device yet */ ufshcd_release(hba); @@ -8859,6 +8889,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) UFS_SLEEP_PWR_MODE, UIC_LINK_HIBERN8_STATE); + INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work, + ufshcd_rpm_dev_flush_recheck_work); + /* Set the default auto-hiberate idle timer value to 150 ms */ if (ufshcd_is_auto_hibern8_supported(hba) && !hba->ahit) { hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) | diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 8db7a6101892..9acd437037e8 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -745,6 +745,7 @@ struct ufs_hba { struct request_queue *bsg_queue; bool wb_buf_flush_enabled; bool wb_enabled; + struct delayed_work rpm_dev_flush_recheck_work; }; /* Returns true if clocks can be gated. Otherwise false */
The commit "scsi: ufs: Fix WriteBooster flush during runtime suspend" promises essential resource, i.e., for UFS devices doing WriteBooster buffer flush and Auto BKOPs. However if device finishes its job but not resumed for a very long time, system will have unnecessary power drain because VCC is still supplied. To fix this, a method to recheck the threshold of keeping VCC supply is required. However, the threshold recheck needs to re-activate the link because the decision depends on the device status. Introduce a delayed work to force runtime resume after a certain delay during runtime suspend. This makes threshold recheck simpler which will be done in the next runtime-suspend. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufs.h | 1 + drivers/scsi/ufs/ufshcd.c | 43 ++++++++++++++++++++++++++++++++++----- drivers/scsi/ufs/ufshcd.h | 1 + 3 files changed, 40 insertions(+), 5 deletions(-)