Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 5/5] scsi: ufs: Fix possible VCC power drain during runtime suspend | From | "Asutosh Das (asd)" <> | Date | Tue, 19 May 2020 09:27:37 -0700 |
| |
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 */ >
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project
| |