| Message ID | 20220729045600epcms2p45c0f8a5a0a76c7fe85b0961570de89ce@epcms2p4 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: wb: Add sysfs attribute and cleanup | expand |
On 7/28/22 21:56, Jinyoung CHOI wrote: > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs > index 6b248abb1bd7..7e9e1db55d60 100644 > --- a/Documentation/ABI/testing/sysfs-driver-ufs > +++ b/Documentation/ABI/testing/sysfs-driver-ufs > @@ -1417,6 +1417,16 @@ Description: This node is used to set or display whether UFS WriteBooster is > platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can > disable/enable WriteBooster through this sysfs node. > > +What: /sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en > +What: /sys/bus/platform/devices/*.ufs/wb_buf_flush_en > +Date: July 2022 > +Contact: Jinyoung Choi <j-young.choi@samsung.com> > +Description: This node is used to set or display whether WriteBooster ^^^^ Please change "node" into "attribute" (here and below). Sysfs files are called attributes. > + buffer flusing is enabled. The data written in the WriteBooster ^^^^^^^ flushing? > + Buffer can be flushed by an explicit host command or > + implicitly while in hibernate (HIBERN8) state. The above sentence is misleading because it suggests that setting this attribute causes the WB buffer to be flushed in its entirety. That is not correct - what this attribute controls is whether or not the UFS device is allowed to start with flushing the WB buffer. > + 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"); flush -> flushing > + 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; > +} Please leave a blank line above goto labels as requested by the kernel coding style guide. Thanks, Bart.
>On 7/28/22 21:56, Jinyoung CHOI wrote: >> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs >> index 6b248abb1bd7..7e9e1db55d60 100644 >> --- a/Documentation/ABI/testing/sysfs-driver-ufs >> +++ b/Documentation/ABI/testing/sysfs-driver-ufs >> @@ -1417,6 +1417,16 @@ Description: This node is used to set or display whether UFS WriteBooster is >> platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can >> disable/enable WriteBooster through this sysfs node. >> >> +What: /sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en >> +What: /sys/bus/platform/devices/*.ufs/wb_buf_flush_en >> +Date: July 2022 >> +Contact: Jinyoung Choi <j-young.choi@samsung.com> >> +Description: This node is used to set or display whether WriteBooster > ^^^^ > >Please change "node" into "attribute" (here and below). Sysfs files are >called attributes. > The wb_on description is also written as node. This will also be changed to other commit. >> + buffer flusing is enabled. The data written in the WriteBooster > ^^^^^^^ > flushing? >> + Buffer can be flushed by an explicit host command or >> + implicitly while in hibernate (HIBERN8) state. > >The above sentence is misleading because it suggests that setting this >attribute causes the WB buffer to be flushed in its entirety. That is >not correct - what this attribute controls is whether or not the UFS >device is allowed to start with flushing the WB buffer. > It seems to have been written so briefly. Because each device manufacturer may have different flush policies, So, I did not describe the amount. This is the same as the ufs spec. I will add more explanation. And, I don't fully understand your comment (That is not correct ~) If you explain it more, I will consider it. >> + 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"); > >flush -> flushing > OK, I will fix it. >> + 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; >> +} > >Please leave a blank line above goto labels as requested by the kernel >coding style guide. > >Thanks, > >Bart. OK, I will fix it. wb_on_store() will also be modified with this. (other commit) Kind Regards, Jinyoung
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs index 6b248abb1bd7..7e9e1db55d60 100644 --- a/Documentation/ABI/testing/sysfs-driver-ufs +++ b/Documentation/ABI/testing/sysfs-driver-ufs @@ -1417,6 +1417,16 @@ Description: This node is used to set or display whether UFS WriteBooster is platform that doesn't support UFSHCD_CAP_CLK_SCALING, we can disable/enable WriteBooster through this sysfs node. +What: /sys/bus/platform/drivers/ufshcd/*/wb_buf_flush_en +What: /sys/bus/platform/devices/*.ufs/wb_buf_flush_en +Date: July 2022 +Contact: Jinyoung Choi <j-young.choi@samsung.com> +Description: This node is used to set or display whether WriteBooster + buffer flusing is enabled. The data written in the WriteBooster + Buffer can be flushed by an explicit host command or + implicitly while in hibernate (HIBERN8) state. We can control + the explicit flushing through this sysfs node. + What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/hpb_version What: /sys/bus/platform/devices/*.ufs/device_descriptor/hpb_version Date: June 2021 diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index 0a088b47d557..920094665f4d 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 a14d3fe39570..36d4708eaee0 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); @@ -5765,25 +5764,27 @@ 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 (!ufshcd_is_wb_allowed(hba) || 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);