[PATCH v3] ufs: core: Add WB buffer resize support

Huan Tang posted 1 patch 3 weeks, 6 days ago
There is a newer version of this series
Documentation/ABI/testing/sysfs-driver-ufs | 52 ++++++++++++++++++++++
drivers/ufs/core/ufs-sysfs.c               | 42 +++++++++++++++++
drivers/ufs/core/ufshcd.c                  | 15 +++++++
include/ufs/ufs.h                          |  5 ++-
include/ufs/ufshcd.h                       |  1 +
5 files changed, 114 insertions(+), 1 deletion(-)
[PATCH v3] ufs: core: Add WB buffer resize support
Posted by Huan Tang 3 weeks, 6 days ago
Support WB buffer resize function through sysfs, the host can obtain
resize hint and resize status and enable the resize operation. To
achieve this goals, three sysfs nodes have been added:
	1. wb_toggle_buf_resize
	2. wb_buf_resize_hint
	3. wb_buf_resize_status

The detailed definition of the three nodes can be found in the sysfs
documentation.

Changelog
===
v2 - > v3:
	remove needless code:
	drivers/ufs/core/ufs-sysfs.c:441:2:
	index = ufshcd_wb_get_query_index(hba)

v1 - > v2:
	remove unused variable "u8 index",
	drivers/ufs/core/ufs-sysfs.c:419:12: warning: variable 'index'
	set but not used.

v1
	https://lore.kernel.org/all/20241025085924.4855-1-tanghuan@vivo.com/
v2
	https://lore.kernel.org/all/20241026004423.135-1-tanghuan@vivo.com/

Signed-off-by: Huan Tang <tanghuan@vivo.com>
Signed-off-by: Lu Hongfei <luhongfei@vivo.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 52 ++++++++++++++++++++++
 drivers/ufs/core/ufs-sysfs.c               | 42 +++++++++++++++++
 drivers/ufs/core/ufshcd.c                  | 15 +++++++
 include/ufs/ufs.h                          |  5 ++-
 include/ufs/ufshcd.h                       |  1 +
 5 files changed, 114 insertions(+), 1 deletion(-)

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);
-- 
2.39.0
Re: [PATCH v3] ufs: core: Add WB buffer resize support
Posted by Bart Van Assche 3 weeks, 6 days ago
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.