| Message ID | 20220727070841epcms2p5e212d617dd0f985555fa052f099013f0@epcms2p5 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: wb: Add sysfs attribute and cleanup | expand |
On Wed, 2022-07-27 at 16:08 +0900, Jinyoung CHOI wrote: > There is the following quirk to bypass "WB Flush" in Write Booster. > > - UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL > > If this quirk is not set, there is no knob that can control "WB > Flush". > > There are three flags that control Write Booster Feature. > 1. WB ON/OFF > 2. WB Hibern Flush ON/OFF (implicitly) > 3. WB Flush ON/OFF (explicit) > > The sysfs attribute that controls the WB was implemented. (1) > > In the case of "Hibern Flush", it is always good to turn on. > Control may not be required. (2) > > Finally, "Flush" may be necessary because the Auto-Hibern8 is not > supported in a specific environment. > So the sysfs attribute that controls this is necessary. (3) > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> > --- ... > > +static ssize_t wb_buf_flush_en_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int wb_buf_flush_en; > + ssize_t res; > + > + if (ufshcd_is_wb_allowed(hba) && > + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) { > + dev_warn(dev, "It is not allowed to configure WB buf > flush!\n"); > + return -EOPNOTSUPP; > + } > + Hi J-young, I don't understand here, if UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL is not set (manual flush is not disable), so we cannot manually flush buffer? or should we check if Auto-Hibern8 is supported? Kind regards, Bean
>> There is the following quirk to bypass "WB Flush" in Write Booster. >> >> - UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL >> >> If this quirk is not set, there is no knob that can control "WB >> Flush". > >> >> There are three flags that control Write Booster Feature. >> 1. WB ON/OFF >> 2. WB Hibern Flush ON/OFF (implicitly) >> 3. WB Flush ON/OFF (explicit) >> >> The sysfs attribute that controls the WB was implemented. (1) >> >> In the case of "Hibern Flush", it is always good to turn on. >> Control may not be required. (2) >> >> Finally, "Flush" may be necessary because the Auto-Hibern8 is not >> supported in a specific environment. >> So the sysfs attribute that controls this is necessary. (3) >> >> Reviewed-by: Avri Altman <avri.altman@wdc.com> >> Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> >> --- >... >> >> +static ssize_t wb_buf_flush_en_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct ufs_hba *hba = dev_get_drvdata(dev); >> + unsigned int wb_buf_flush_en; >> + ssize_t res; >> + >> + if (ufshcd_is_wb_allowed(hba) && >> + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) { >> + dev_warn(dev, "It is not allowed to configure WB buf >> flush!\n"); >> + return -EOPNOTSUPP; >> + } >> + >Hi J-young, > >I don't understand here, if UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL is >not set (manual flush is not disable), so we cannot manually flush >buffer? or should we check if Auto-Hibern8 is supported? > >Kind regards, >Bean Hi Bean, As the patch was separated, the conditional sentence went wrong. Thank you for checking. I will modify it quickly. Thanks, Jinyoung.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index 0a088b47d557..e7800e49998a 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -254,6 +254,49 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr, return res < 0 ? res : count; } +static ssize_t wb_buf_flush_en_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%d\n", hba->dev_info.wb_buf_flush_enabled); +} + +static ssize_t wb_buf_flush_en_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + unsigned int wb_buf_flush_en; + ssize_t res; + + if (ufshcd_is_wb_allowed(hba) && + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) { + dev_warn(dev, "It is not allowed to configure WB buf flush!\n"); + return -EOPNOTSUPP; + } + + if (kstrtouint(buf, 0, &wb_buf_flush_en)) + return -EINVAL; + + if (wb_buf_flush_en != 0 && wb_buf_flush_en != 1) + return -EINVAL; + + down(&hba->host_sem); + if (!ufshcd_is_user_access_allowed(hba)) { + res = -EBUSY; + goto out; + } + + ufshcd_rpm_get_sync(hba); + res = ufshcd_wb_toggle_buf_flush(hba, wb_buf_flush_en); + ufshcd_rpm_put_sync(hba); +out: + up(&hba->host_sem); + return res < 0 ? res : count; +} + static DEVICE_ATTR_RW(rpm_lvl); static DEVICE_ATTR_RO(rpm_target_dev_state); static DEVICE_ATTR_RO(rpm_target_link_state); @@ -262,6 +305,7 @@ static DEVICE_ATTR_RO(spm_target_dev_state); static DEVICE_ATTR_RO(spm_target_link_state); static DEVICE_ATTR_RW(auto_hibern8); static DEVICE_ATTR_RW(wb_on); +static DEVICE_ATTR_RW(wb_buf_flush_en); static struct attribute *ufs_sysfs_ufshcd_attrs[] = { &dev_attr_rpm_lvl.attr, @@ -272,6 +316,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { &dev_attr_spm_target_link_state.attr, &dev_attr_auto_hibern8.attr, &dev_attr_wb_on.attr, + &dev_attr_wb_buf_flush_en.attr, NULL }; diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 52377fedae49..a8a797e0033d 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -267,7 +267,6 @@ static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag); static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, bool enable); -static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable); static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba); static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba); @@ -5767,24 +5766,26 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba, __func__, enable ? "enabled" : "disabled"); } -static void ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable) +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable) { int ret; if (hba->dev_info.wb_buf_flush_enabled == enable) - return; + return 0; ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN); if (ret) { dev_err(hba->dev, "%s WB-Buf Flush %s failed %d\n", __func__, enable ? "enable" : "disable", ret); - return; + return ret; } hba->dev_info.wb_buf_flush_enabled = enable; dev_dbg(hba->dev, "%s WB-Buf Flush %s\n", __func__, enable ? "enabled" : "disabled"); + + return ret; } static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba, diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 7fe1a926cd99..94bcfec98fb8 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -1211,6 +1211,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, enum query_opcode desc_op); int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); +int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable); int ufshcd_suspend_prepare(struct device *dev); int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); void ufshcd_resume_complete(struct device *dev);