| Message ID | 20241028135205.188-1-tanghuan@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] ufs: core: Add WB buffer resize support | expand |
On 10/28/24 6:52 AM, Huan Tang wrote: > +What: /sys/bus/platform/drivers/ufshcd/*/wb_toggle_buf_resize > +What: /sys/bus/platform/devices/*.ufs/wb_toggle_buf_resize > +Date: Qct 2024 > +Contact: Huan Tang <tanghuan@vivo.com> > +Description: > + The host can decrease or increase the WriteBooster Buffer size by setting > + this file. > + > + ====== ====================================== > + 00h Idle (There is no resize operation) > + 01h Decrease WriteBooster Buffer Size > + 02h Increase WriteBooster Buffer Size > + Others Reserved > + ====== ====================================== > + > + The file is write only. The name "wb_toggle_buf_resize" is not clear and will make users guess what the purpose of this sysfs attribute is. Please choose a name that is more clear, e.g. "wb_resize_action". Additionally, please change the word "file" into "attribute" to maintain consistency with the rest of the sysfs documentation. Please also make sure that the documentation is consistent with the code. The above documentation mentions the value 0 while the code doesn't allow writing the value zero. > + > +What: /sys/bus/platform/drivers/ufshcd/*/attributes/wb_buf_resize_status > +What: /sys/bus/platform/devices/*.ufs/attributes/wb_buf_resize_status > +Date: Qct 2024 > +Contact: Huan Tang <tanghuan@vivo.com> > +Description: > + The host can check the Resize operation status of the WriteBooster Buffer > + by reading this file. > + > + ====== ======================================== > + 00h Idle (resize operation is not issued) > + 01h Resize operation in progress > + 02h Resize operation completed successfully > + 03h Resize operation general failure > + Others Reserved > + ====== ======================================== For the three new attributes: please use words in sysfs instead of numbers. Using numbers is not user-friendly. > +static ssize_t wb_toggle_buf_resize_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_resize_op; Please introduce an enumeration type instead of using 'unsigned int'. Please also choose a more descriptive variable name than 'op'. > +int ufshcd_wb_toggle_buf_resize(struct ufs_hba *hba, u32 op) > +{ > + int ret; > + u8 index; > + > + index = ufshcd_wb_get_query_index(hba); > + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, > + QUERY_ATTR_IDN_WB_BUF_RESIZE_EN, index, 0, &op); > + if (ret) > + dev_err(hba->dev, "%s: Enable WB buf resize operation failed %d\n", > + __func__, ret); > + > + return ret; > +} This function doesn't toggle anything - it sets the value of the bWriteBoosterBufferResizeEn attribute. Please reflect this in the function name, e.g. by renaming this function into ufshcd_wb_set_resize_en(). Additionally, the name of the 'op' argument doesn't reflect the purpose of that argument. How about renaming it into 'enum wb_resize_en' where the wb_resize_en type support the three values idle = 0, decrease = 1 and increase = 2? Thanks, Bart.
> For the three new attributes: please use words in sysfs instead of > numbers. Using numbers is not user-friendly. Hi Bart Thank you for your reply! Thanks for your suggestions I am confused about the above sentence and I don't understand it. Can you give me more detailed guidance? v4 https://lore.kernel.org/all/20241101093318.825-1-tanghuan@vivo.com/ Thanks Huan
On 11/1/24 2:37 AM, Huan Tang wrote: >> For the three new attributes: please use words in sysfs instead of >> numbers. Using numbers is not user-friendly. > > > Hi Bart > > Thank you for your reply! > Thanks for your suggestions > > I am confused about the above sentence and I don't understand it. Can you give me more detailed guidance? > > v4 > https://lore.kernel.org/all/20241101093318.825-1-tanghuan@vivo.com/ Hi Huan, Please modify the implementation of sysfs attributes like wb_toggle_buf_resize such that a string is accepted (e.g. increase, decrease) instead of numbers (1, 2). See also how sysfs_match_string() is used in e.g. drivers/scsi/sd.c. Several sd sysfs attributes support writing a string into sysfs attributes and that string is translated into a number. Thanks, Bart.
> Hi Huan, > Please modify the implementation of sysfs attributes like > wb_toggle_buf_resize such that a string is accepted (e.g. increase, > decrease) instead of numbers (1, 2). > > See also how sysfs_match_string() is used in e.g. drivers/scsi/sd.c. > Several sd sysfs attributes support writing a string into sysfs > attributes and that string is translated into a number. > > Thanks, > > Bart. Hi Bart, Thank you for your reply and guidance. I got it. I have submitted the v6 patch according to your suggestions.Could you please review it again? v6 patch: https://lore.kernel.org/all/20241104142437.234-1-tanghuan@vivo.com/ Thanks Huan
On 11/4/24 6:27 AM, Huan Tang wrote: > I have submitted the v6 patch according to your suggestions.Could you > please review it again? The WB buffer resize functionality will be included in the JEDEC UFS 4.1 standard and that standard has not yet been published. Do the JEDEC rules allow inclusion in the upstream kernel of functionality that has not yet been published? Thanks, Bart.
> The WB buffer resize functionality will be included in the JEDEC UFS 4.1 > standard and that standard has not yet been published. Do the JEDEC > rules allow inclusion in the upstream kernel of functionality that has > not yet been published? Hi Bart, Thank you for your reply . Sorry, my mistake; let's wait for JEDEC to officially release it before continuing the discussion Thanks Huan
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs index 5fa6655aee84..dbaa84277801 100644 --- a/Documentation/ABI/testing/sysfs-driver-ufs +++ b/Documentation/ABI/testing/sysfs-driver-ufs @@ -1559,3 +1559,55 @@ Description: Symbol - HCMID. This file shows the UFSHCD manufacturer id. The Manufacturer ID is defined by JEDEC in JEDEC-JEP106. The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/wb_toggle_buf_resize +What: /sys/bus/platform/devices/*.ufs/wb_toggle_buf_resize +Date: Qct 2024 +Contact: Huan Tang <tanghuan@vivo.com> +Description: + The host can decrease or increase the WriteBooster Buffer size by setting + this file. + + ====== ====================================== + 00h Idle (There is no resize operation) + 01h Decrease WriteBooster Buffer Size + 02h Increase WriteBooster Buffer Size + Others Reserved + ====== ====================================== + + The file is write only. + +What: /sys/bus/platform/drivers/ufshcd/*/attributes/wb_buf_resize_hint +What: /sys/bus/platform/devices/*.ufs/attributes/wb_buf_resize_hint +Date: Qct 2024 +Contact: Huan Tang <tanghuan@vivo.com> +Description: + wb_buf_resize_hint indicates hint information about which type of resize for + WriteBooster Buffer is recommended by the device. + + ====== ====================================== + 00h Recommend keep the buffer size + 01h Recommend to decrease the buffer size + 02h Recommend to increase the buffer size + Others: Reserved + ====== ====================================== + + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/attributes/wb_buf_resize_status +What: /sys/bus/platform/devices/*.ufs/attributes/wb_buf_resize_status +Date: Qct 2024 +Contact: Huan Tang <tanghuan@vivo.com> +Description: + The host can check the Resize operation status of the WriteBooster Buffer + by reading this file. + + ====== ======================================== + 00h Idle (resize operation is not issued) + 01h Resize operation in progress + 02h Resize operation completed successfully + 03h Resize operation general failure + Others Reserved + ====== ======================================== + + The file is read only. diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index 265f21133b63..683f916e558b 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -411,6 +411,42 @@ static ssize_t wb_flush_threshold_store(struct device *dev, return count; } +static ssize_t wb_toggle_buf_resize_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_resize_op; + ssize_t res; + + if (!ufshcd_is_wb_allowed(hba) || !hba->dev_info.wb_enabled || + !hba->dev_info.b_presrv_uspc_en) { + dev_err(dev, "The WB buf resize is not allowed!\n"); + return -EOPNOTSUPP; + } + + if (kstrtouint(buf, 0, &wb_buf_resize_op)) + return -EINVAL; + + if (wb_buf_resize_op != 0x01 && wb_buf_resize_op != 0x02) { + dev_err(dev, "The operation %u is invalid!\n", wb_buf_resize_op); + 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_resize(hba, wb_buf_resize_op); + ufshcd_rpm_put_sync(hba); + +out: + up(&hba->host_sem); + return res < 0 ? res : count; +} + /** * pm_qos_enable_show - sysfs handler to show pm qos enable value * @dev: device associated with the UFS controller @@ -468,6 +504,7 @@ static DEVICE_ATTR_RW(auto_hibern8); static DEVICE_ATTR_RW(wb_on); static DEVICE_ATTR_RW(enable_wb_buf_flush); static DEVICE_ATTR_RW(wb_flush_threshold); +static DEVICE_ATTR_WO(wb_toggle_buf_resize); static DEVICE_ATTR_RW(rtc_update_ms); static DEVICE_ATTR_RW(pm_qos_enable); @@ -482,6 +519,7 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = { &dev_attr_wb_on.attr, &dev_attr_enable_wb_buf_flush.attr, &dev_attr_wb_flush_threshold.attr, + &dev_attr_wb_toggle_buf_resize.attr, &dev_attr_rtc_update_ms.attr, &dev_attr_pm_qos_enable.attr, NULL @@ -1526,6 +1564,8 @@ UFS_ATTRIBUTE(wb_flush_status, _WB_FLUSH_STATUS); UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE); UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST); UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE); +UFS_ATTRIBUTE(wb_buf_resize_hint, _WB_BUF_RESIZE_HINT); +UFS_ATTRIBUTE(wb_buf_resize_status, _WB_BUF_RESIZE_STATUS); static struct attribute *ufs_sysfs_attributes[] = { @@ -1549,6 +1589,8 @@ static struct attribute *ufs_sysfs_attributes[] = { &dev_attr_wb_avail_buf.attr, &dev_attr_wb_life_time_est.attr, &dev_attr_wb_cur_buf.attr, + &dev_attr_wb_buf_resize_hint.attr, + &dev_attr_wb_buf_resize_status.attr, NULL, }; diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 630409187c10..c28915debab6 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -6167,6 +6167,21 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba) return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf); } +int ufshcd_wb_toggle_buf_resize(struct ufs_hba *hba, u32 op) +{ + int ret; + u8 index; + + index = ufshcd_wb_get_query_index(hba); + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, + QUERY_ATTR_IDN_WB_BUF_RESIZE_EN, index, 0, &op); + if (ret) + dev_err(hba->dev, "%s: Enable WB buf resize operation failed %d\n", + __func__, ret); + + return ret; +} + static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work) { struct ufs_hba *hba = container_of(to_delayed_work(work), diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index e594abe5d05f..f737d98044ac 100644 --- a/include/ufs/ufs.h +++ b/include/ufs/ufs.h @@ -181,7 +181,10 @@ enum attr_idn { QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST = 0x1E, QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE = 0x1F, QUERY_ATTR_IDN_EXT_IID_EN = 0x2A, - QUERY_ATTR_IDN_TIMESTAMP = 0x30 + QUERY_ATTR_IDN_TIMESTAMP = 0x30, + QUERY_ATTR_IDN_WB_BUF_RESIZE_HINT = 0x3C, + QUERY_ATTR_IDN_WB_BUF_RESIZE_EN = 0x3D, + QUERY_ATTR_IDN_WB_BUF_RESIZE_STATUS = 0x3E, }; /* Descriptor idn for Query requests */ diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index a95282b9f743..cbe208ce9293 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -1454,6 +1454,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r struct scatterlist *sg_list, enum dma_data_direction dir); int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable); +int ufshcd_wb_toggle_buf_resize(struct ufs_hba *hba, u32 op); 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);