[PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Can Guo]

Introduce a new generic function ufshcd_query_attr_qword() to handle
quad-word (64-bit) UFS attribute operations. This consolidates the
handling of 64-bit attributes which was previously scattered across
multiple specialized functions.

Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
---
 drivers/ufs/core/ufs-sysfs.c   |  30 ++++++--
 drivers/ufs/core/ufshcd-priv.h |   3 +-
 drivers/ufs/core/ufshcd.c      | 126 +++++++++++++++++----------------
 3 files changed, 94 insertions(+), 65 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 99af3c73f1af..61052df2915c 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -594,8 +594,13 @@ static ssize_t device_lvl_exception_id_show(struct device *dev,
 	u64 exception_id;
 	int err;
 
+	if (hba->dev_info.wspecversion < 0x410)
+		return -EOPNOTSUPP;
+
 	ufshcd_rpm_get_sync(hba);
-	err = ufshcd_read_device_lvl_exception_id(hba, &exception_id);
+	err = ufshcd_query_attr_qword(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				      QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID,
+				      0, 0, &exception_id);
 	ufshcd_rpm_put_sync(hba);
 
 	if (err)
@@ -1670,6 +1675,12 @@ static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
 		idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE;
 }
 
+static inline bool ufshcd_is_qword_attrs(enum attr_idn idn)
+{
+	return idn == QUERY_ATTR_IDN_TIMESTAMP ||
+	       idn == QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID;
+}
+
 static int wb_read_resize_attrs(struct ufs_hba *hba,
 			enum attr_idn idn, u32 *attr_val)
 {
@@ -1736,6 +1747,7 @@ static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
 {									\
 	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	u64 qword_value;							\
 	u32 value;							\
 	int ret;							\
 	u8 index = 0;							\
@@ -1748,14 +1760,24 @@ static ssize_t _name##_show(struct device *dev,				\
 	if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname))			\
 		index = ufshcd_wb_get_query_index(hba);			\
 	ufshcd_rpm_get_sync(hba);					\
-	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,	\
-		QUERY_ATTR_IDN##_uname, index, 0, &value);		\
+	if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))		\
+		ret = ufshcd_query_attr_qword(hba,			\
+			UPIU_QUERY_OPCODE_READ_ATTR,			\
+			QUERY_ATTR_IDN##_uname,				\
+			index, 0, &qword_value);				\
+	else								\
+		ret = ufshcd_query_attr(hba,				\
+			UPIU_QUERY_OPCODE_READ_ATTR,			\
+			QUERY_ATTR_IDN##_uname, index, 0, &value);	\
 	ufshcd_rpm_put_sync(hba);					\
 	if (ret) {							\
 		ret = -EINVAL;						\
 		goto out;						\
 	}								\
-	ret = sysfs_emit(buf, "0x%08X\n", value);			\
+	if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))               \
+		ret = sysfs_emit(buf, "0x%016llX\n", qword_value);		\
+	else								\
+		ret = sysfs_emit(buf, "0x%08X\n", value);		\
 out:									\
 	up(&hba->host_sem);						\
 	return ret;							\
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 0a72148cb053..ed1adeb22ec6 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -60,6 +60,8 @@ int ufshcd_query_attr_retry(struct ufs_hba *hba, enum query_opcode opcode,
 			    u32 *attr_val);
 int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 		      enum attr_idn idn, u8 index, u8 selector, u32 *attr_val);
+int ufshcd_query_attr_qword(struct ufs_hba *hba, enum query_opcode opcode,
+			    enum attr_idn idn, u8 index, u8 sel, u64 *attr_val);
 int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 	enum flag_idn idn, u8 index, bool *flag_res);
 void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
@@ -106,7 +108,6 @@ 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_read_device_lvl_exception_id(struct ufs_hba *hba, u64 *exception_id);
 
 int ufshcd_uic_tx_eqtr(struct ufs_hba *hba, int gear);
 void ufshcd_apply_valid_tx_eq_settings(struct ufs_hba *hba);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4805e40ed4d7..e5e22adcbbc3 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3611,6 +3611,67 @@ int ufshcd_query_attr_retry(struct ufs_hba *hba,
 	return ret;
 }
 
+/**
+ * ufshcd_query_attr_qword - API function of sending query requests for quad-word attributes
+ * @hba: per-adapter instance
+ * @opcode: attribute opcode
+ * @idn: attribute idn to access
+ * @index: index field
+ * @sel: selector field
+ * @attr_val: the attribute value after the query request completes
+ *
+ * Return: 0 for success, non-zero in case of failure.
+ */
+int ufshcd_query_attr_qword(struct ufs_hba *hba, enum query_opcode opcode,
+			    enum attr_idn idn, u8 index, u8 sel, u64 *attr_val)
+{
+	struct utp_upiu_query_v4_0 *upiu_req;
+	struct utp_upiu_query_v4_0 *upiu_resp;
+	struct ufs_query_req *request = NULL;
+	struct ufs_query_res *response = NULL;
+	int err;
+
+	if (!attr_val) {
+		dev_err(hba->dev, "%s: attribute value required for opcode 0x%x\n",
+			__func__, opcode);
+		return -EINVAL;
+	}
+
+	ufshcd_dev_man_lock(hba);
+
+	ufshcd_init_query(hba, &request, &response, opcode, idn, index, sel);
+
+	switch (opcode) {
+	case UPIU_QUERY_OPCODE_WRITE_ATTR:
+		request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
+		upiu_req = (struct utp_upiu_query_v4_0 *)&request->upiu_req;
+		put_unaligned_be64(*attr_val, &upiu_req->osf3);
+		break;
+	case UPIU_QUERY_OPCODE_READ_ATTR:
+		request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
+		break;
+	default:
+		dev_err(hba->dev, "%s: Expected query attr opcode but got = 0x%.2x\n",
+			__func__, opcode);
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
+	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
+	if (err) {
+		dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, index %d, selector %d, err = %d\n",
+			__func__, opcode, idn, index, sel, err);
+		goto out_unlock;
+	}
+
+	upiu_resp = (struct utp_upiu_query_v4_0 *)response;
+	*attr_val = get_unaligned_be64(&upiu_resp->osf3);
+
+out_unlock:
+	ufshcd_dev_man_unlock(hba);
+	return err;
+}
+
 /*
  * Return: 0 upon success; > 0 in case the UFS device reported an OCS error;
  * < 0 if another error occurred.
@@ -6224,46 +6285,6 @@ static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
-/*
- * Return: 0 upon success; > 0 in case the UFS device reported an OCS error;
- * < 0 if another error occurred.
- */
-int ufshcd_read_device_lvl_exception_id(struct ufs_hba *hba, u64 *exception_id)
-{
-	struct utp_upiu_query_v4_0 *upiu_resp;
-	struct ufs_query_req *request = NULL;
-	struct ufs_query_res *response = NULL;
-	int err;
-
-	if (hba->dev_info.wspecversion < 0x410)
-		return -EOPNOTSUPP;
-
-	ufshcd_hold(hba);
-	mutex_lock(&hba->dev_cmd.lock);
-
-	ufshcd_init_query(hba, &request, &response,
-			  UPIU_QUERY_OPCODE_READ_ATTR,
-			  QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID, 0, 0);
-
-	request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
-
-	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
-
-	if (err) {
-		dev_err(hba->dev, "%s: failed to read device level exception %d\n",
-			__func__, err);
-		goto out;
-	}
-
-	upiu_resp = (struct utp_upiu_query_v4_0 *)response;
-	*exception_id = get_unaligned_be64(&upiu_resp->osf3);
-out:
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
-
-	return err;
-}
-
 static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
 {
 	u8 index;
@@ -9113,35 +9134,20 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 
 static void ufshcd_set_timestamp_attr(struct ufs_hba *hba)
 {
-	int err;
-	struct ufs_query_req *request = NULL;
-	struct ufs_query_res *response = NULL;
 	struct ufs_dev_info *dev_info = &hba->dev_info;
-	struct utp_upiu_query_v4_0 *upiu_data;
+	u64 ts_ns;
+	int err;
 
 	if (dev_info->wspecversion < 0x400 ||
 	    hba->dev_quirks & UFS_DEVICE_QUIRK_NO_TIMESTAMP_SUPPORT)
 		return;
 
-	ufshcd_dev_man_lock(hba);
-
-	ufshcd_init_query(hba, &request, &response,
-			  UPIU_QUERY_OPCODE_WRITE_ATTR,
-			  QUERY_ATTR_IDN_TIMESTAMP, 0, 0);
-
-	request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
-
-	upiu_data = (struct utp_upiu_query_v4_0 *)&request->upiu_req;
-
-	put_unaligned_be64(ktime_get_real_ns(), &upiu_data->osf3);
-
-	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
-
+	ts_ns = ktime_get_real_ns();
+	err = ufshcd_query_attr_qword(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+				      QUERY_ATTR_IDN_TIMESTAMP, 0, 0, &ts_ns);
 	if (err)
 		dev_err(hba->dev, "%s: failed to set timestamp %d\n",
 			__func__, err);
-
-	ufshcd_dev_man_unlock(hba);
 }
 
 /**
-- 
2.34.1

Re: [PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Peter Wang (王信友)]

On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
> Introduce a new generic function ufshcd_query_attr_qword() to handle
> quad-word (64-bit) UFS attribute operations. This consolidates the
> handling of 64-bit attributes which was previously scattered across
> multiple specialized functions.
> 
> Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Bart Van Assche]

On 4/19/26 6:52 AM, Can Guo wrote:
> +static inline bool ufshcd_is_qword_attrs(enum attr_idn idn)
> +{
> +	return idn == QUERY_ATTR_IDN_TIMESTAMP ||
> +	       idn == QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID;
> +}

Please change "ufshcd_is_qword_attrs()" into "ufshcd_is_qword_attr()".

> +/**
> + * ufshcd_query_attr_qword - API function of sending query requests for quad-word attributes
> + * @hba: per-adapter instance
> + * @opcode: attribute opcode
> + * @idn: attribute idn to access
> + * @index: index field
> + * @sel: selector field
> + * @attr_val: the attribute value after the query request completes
> + *
> + * Return: 0 for success, non-zero in case of failure.
> + */

The word "API" is uncommon in the first line of kernel-doc headers.
Please remove it.

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Can Guo]

On 4/21/2026 12:58 AM, Bart Van Assche wrote:
> On 4/19/26 6:52 AM, Can Guo wrote:
>> +static inline bool ufshcd_is_qword_attrs(enum attr_idn idn)
>> +{
>> +    return idn == QUERY_ATTR_IDN_TIMESTAMP ||
>> +           idn == QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID;
>> +}
>
> Please change "ufshcd_is_qword_attrs()" into "ufshcd_is_qword_attr()".
Done.
>
>> +/**
>> + * ufshcd_query_attr_qword - API function of sending query requests 
>> for quad-word attributes
>> + * @hba: per-adapter instance
>> + * @opcode: attribute opcode
>> + * @idn: attribute idn to access
>> + * @index: index field
>> + * @sel: selector field
>> + * @attr_val: the attribute value after the query request completes
>> + *
>> + * Return: 0 for success, non-zero in case of failure.
>> + */
>
> The word "API" is uncommon in the first line of kernel-doc headers.
> Please remove it.
Done.

Thanks,
Can Guo.
>
> Otherwise this patch looks good to me.
>
> Thanks,
>
> Bart.

Re: [PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Bean Huo]

Can,


On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
>  static int wb_read_resize_attrs(struct ufs_hba *hba,
>                         enum attr_idn idn, u32 *attr_val)
>  {
> @@ -1736,6 +1747,7 @@ static ssize_t _name##_show(struct device
> *dev,                           \
>         struct device_attribute *attr, char *buf)                       \
>  {                                                                      \
>         struct ufs_hba *hba = dev_get_drvdata(dev);                     \
> +       u64
> qword_value;                                                        \

u64 qword_value __maybe_unused;


>         u32 value;                                                      \
>         int ret;                                                        \
>         u8 index = 0;                                                   \
> @@ -1748,14 +1760,24 @@ static ssize_t _name##_show(struct device
> *dev,                         \
>         if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname))                 \
>                 index = ufshcd_wb_get_query_index(hba);                 \
>         ufshcd_rpm_get_sync(hba);                                       \
> -       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,       \
> -               QUERY_ATTR_IDN##_uname, index, 0, &value);              \
> +       if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))              \
> +               ret = ufshcd_query_attr_qword(hba,                      \
> +                       UPIU_QUERY_OPCODE_READ_ATTR,                    \
> +                       QUERY_ATTR_IDN##_uname,                         \
> +                       index, 0,
> &qword_value);                                \
> +       else                                                            \
> +               ret = ufshcd_query_attr(hba,                            \
> +                       UPIU_QUERY_OPCODE_READ_ATTR,                    \
> +                       QUERY_ATTR_IDN##_uname, index, 0, &value);      \
>         ufshcd_rpm_put_sync(hba);                                       \
>         if (ret) {                                                      \
>                 ret = -EINVAL;                                          \
>                 goto out;                                               \
>         }                                                               \
> -       ret = sysfs_emit(buf, "0x%08X\n", value);                       \
> +       if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))               \
> +               ret = sysfs_emit(buf, "0x%016llX\n",
> qword_value);              \
> +       else                                                            \
> +               ret = sysfs_emit(buf, "0x%08X\n", value);               \
>  out:                                                                   \
>         up(&hba->host_sem);                                             \
>         return ret;                                                     \
> 

...


>  
> +/**
> + * ufshcd_query_attr_qword - API function of sending query requests for quad-
> word attributes
> + * @hba: per-adapter instance
> + * @opcode: attribute opcode
> + * @idn: attribute idn to access
> + * @index: index field
> + * @sel: selector field
> + * @attr_val: the attribute value after the query request completes
> + *
> + * Return: 0 for success, non-zero in case of failure.
> + */
> +int ufshcd_query_attr_qword(struct ufs_hba *hba, enum query_opcode opcode,
> +                           enum attr_idn idn, u8 index, u8 sel, u64
> *attr_val)
> +{
> +       struct utp_upiu_query_v4_0 *upiu_req;
> +       struct utp_upiu_query_v4_0 *upiu_resp;
> +       struct ufs_query_req *request = NULL;
> +       struct ufs_query_res *response = NULL;
> +       int err;
> +
> +       if (!attr_val) {
> +               dev_err(hba->dev, "%s: attribute value required for opcode
> 0x%x\n",
> +                       __func__, opcode);
> +               return -EINVAL;
> +       }
> +
> +       ufshcd_dev_man_lock(hba);
> +
> +       ufshcd_init_query(hba, &request, &response, opcode, idn, index, sel);
> +
> +       switch (opcode) {
> +       case UPIU_QUERY_OPCODE_WRITE_ATTR:
> +               request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
> +               upiu_req = (struct utp_upiu_query_v4_0 *)&request->upiu_req;
> +               put_unaligned_be64(*attr_val, &upiu_req->osf3);
> +               break;
> +       case UPIU_QUERY_OPCODE_READ_ATTR:
> +               request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> +               break;
> +       default:
> +               dev_err(hba->dev, "%s: Expected query attr opcode but got =
> 0x%.2x\n",
> +                       __func__, opcode);
> +               err = -EINVAL;
> +               goto out_unlock;
> +       }
> +
> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
> +       if (err) {
> +               dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, index
> %d, selector %d, err = %d\n",
> +                       __func__, opcode, idn, index, sel, err);
> +               goto out_unlock;
> +       }
> +
> +       upiu_resp = (struct utp_upiu_query_v4_0 *)response;
> +       *attr_val = get_unaligned_be64(&upiu_resp->osf3);
> +
> +out_unlock:
> +       ufshcd_dev_man_unlock(hba);
> +       return err;
> +}
> +

this needs a wrapper for retry?  In ufshcd_extract_tx_eq_settings_attrs(), the
32-bit dTxEQGnSettingsExt read uses ufshcd_query_attr_retry(), but the 64-bit
qTxEQGnSettings read uses bare ufshcd_query_attr_qword() with no retries.

Kind regards,
Bean

Re: [PATCH 1/2] scsi: ufs: core: Introduce function ufshcd_query_attr_qword()

[Can Guo]

On 4/21/2026 6:04 AM, Bean Huo wrote:
> Can,
>
>
> On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
>>   static int wb_read_resize_attrs(struct ufs_hba *hba,
>>                          enum attr_idn idn, u32 *attr_val)
>>   {
>> @@ -1736,6 +1747,7 @@ static ssize_t _name##_show(struct device
>> *dev,                           \
>>          struct device_attribute *attr, char *buf)                       \
>>   {                                                                      \
>>          struct ufs_hba *hba = dev_get_drvdata(dev);                     \
>> +       u64
>> qword_value;                                                        \
> u64 qword_value __maybe_unused;
>
>
>>          u32 value;                                                      \
>>          int ret;                                                        \
>>          u8 index = 0;                                                   \
>> @@ -1748,14 +1760,24 @@ static ssize_t _name##_show(struct device
>> *dev,                         \
>>          if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname))                 \
>>                  index = ufshcd_wb_get_query_index(hba);                 \
>>          ufshcd_rpm_get_sync(hba);                                       \
>> -       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,       \
>> -               QUERY_ATTR_IDN##_uname, index, 0, &value);              \
>> +       if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))              \
>> +               ret = ufshcd_query_attr_qword(hba,                      \
>> +                       UPIU_QUERY_OPCODE_READ_ATTR,                    \
>> +                       QUERY_ATTR_IDN##_uname,                         \
>> +                       index, 0,
>> &qword_value);                                \
>> +       else                                                            \
>> +               ret = ufshcd_query_attr(hba,                            \
>> +                       UPIU_QUERY_OPCODE_READ_ATTR,                    \
>> +                       QUERY_ATTR_IDN##_uname, index, 0, &value);      \
>>          ufshcd_rpm_put_sync(hba);                                       \
>>          if (ret) {                                                      \
>>                  ret = -EINVAL;                                          \
>>                  goto out;                                               \
>>          }                                                               \
>> -       ret = sysfs_emit(buf, "0x%08X\n", value);                       \
>> +       if (ufshcd_is_qword_attrs(QUERY_ATTR_IDN##_uname))               \
>> +               ret = sysfs_emit(buf, "0x%016llX\n",
>> qword_value);              \
>> +       else                                                            \
>> +               ret = sysfs_emit(buf, "0x%08X\n", value);               \
>>   out:                                                                   \
>>          up(&hba->host_sem);                                             \
>>          return ret;                                                     \
>>
> ...
>
>
>>   
>> +/**
>> + * ufshcd_query_attr_qword - API function of sending query requests for quad-
>> word attributes
>> + * @hba: per-adapter instance
>> + * @opcode: attribute opcode
>> + * @idn: attribute idn to access
>> + * @index: index field
>> + * @sel: selector field
>> + * @attr_val: the attribute value after the query request completes
>> + *
>> + * Return: 0 for success, non-zero in case of failure.
>> + */
>> +int ufshcd_query_attr_qword(struct ufs_hba *hba, enum query_opcode opcode,
>> +                           enum attr_idn idn, u8 index, u8 sel, u64
>> *attr_val)
>> +{
>> +       struct utp_upiu_query_v4_0 *upiu_req;
>> +       struct utp_upiu_query_v4_0 *upiu_resp;
>> +       struct ufs_query_req *request = NULL;
>> +       struct ufs_query_res *response = NULL;
>> +       int err;
>> +
>> +       if (!attr_val) {
>> +               dev_err(hba->dev, "%s: attribute value required for opcode
>> 0x%x\n",
>> +                       __func__, opcode);
>> +               return -EINVAL;
>> +       }
>> +
>> +       ufshcd_dev_man_lock(hba);
>> +
>> +       ufshcd_init_query(hba, &request, &response, opcode, idn, index, sel);
>> +
>> +       switch (opcode) {
>> +       case UPIU_QUERY_OPCODE_WRITE_ATTR:
>> +               request->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
>> +               upiu_req = (struct utp_upiu_query_v4_0 *)&request->upiu_req;
>> +               put_unaligned_be64(*attr_val, &upiu_req->osf3);
>> +               break;
>> +       case UPIU_QUERY_OPCODE_READ_ATTR:
>> +               request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
>> +               break;
>> +       default:
>> +               dev_err(hba->dev, "%s: Expected query attr opcode but got =
>> 0x%.2x\n",
>> +                       __func__, opcode);
>> +               err = -EINVAL;
>> +               goto out_unlock;
>> +       }
>> +
>> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, dev_cmd_timeout);
>> +       if (err) {
>> +               dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, index
>> %d, selector %d, err = %d\n",
>> +                       __func__, opcode, idn, index, sel, err);
>> +               goto out_unlock;
>> +       }
>> +
>> +       upiu_resp = (struct utp_upiu_query_v4_0 *)response;
>> +       *attr_val = get_unaligned_be64(&upiu_resp->osf3);
>> +
>> +out_unlock:
>> +       ufshcd_dev_man_unlock(hba);
>> +       return err;
>> +}
>> +
> this needs a wrapper for retry?  In ufshcd_extract_tx_eq_settings_attrs(), the
> 32-bit dTxEQGnSettingsExt read uses ufshcd_query_attr_retry(), but the 64-bit
> qTxEQGnSettings read uses bare ufshcd_query_attr_qword() with no retries.
I will use the non-retry version in ufs_txeq.c to be symmetrical, 
because I don't really need
retry - retry never helped in real cases.

Thanks,
Can Guo.
>
> Kind regards,
> Bean

[PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Can Guo]

Add support for UFS v5.0 JEDEC attributes qTxEQGnSettings and
wTxEQGnSettingsExt to enable persistent storage and retrieval of
optimal TX Equalization settings.

This provides a fast-path for TX Equalization by reusing previously
stored optimal settings, avoiding TX Equalization Training (EQTR)
procedures during subsequent Power Mode changes.

When no valid TX Equalization settings are found, fall back to full TX
EQTR procedures and optionally save the results for future use.

The validity of one set of TX Equalization settings is indicated by
Bit[15] in wTxEQGnSettingsExt.

Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
---
 drivers/ufs/core/ufs-txeq.c    | 241 +++++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd-priv.h |   2 +
 drivers/ufs/core/ufshcd.c      |   5 +
 include/ufs/ufs.h              |   2 +
 include/ufs/ufshcd.h           |   2 +
 5 files changed, 252 insertions(+)

diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-txeq.c
index b2dc89124353..aa9420b0d1c3 100644
--- a/drivers/ufs/core/ufs-txeq.c
+++ b/drivers/ufs/core/ufs-txeq.c
@@ -14,6 +14,83 @@
 #include <ufs/unipro.h>
 #include "ufshcd-priv.h"
 
+#define TX_EQ_SETTING_MASK		0x7
+#define TX_EQ_SETTINGS_VALID_BIT	BIT(15)
+
+/*
+ * Decode Device TX Equalization settings based on qTxEQGnSettings bit assignment:
+ * bit[3:0]: Device TX Logical LANE 0 PreShoot
+ * bit[7:4]: Device TX Logical LANE 1 PreShoot
+ * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
+ * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
+ */
+#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
+	(((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) & TX_EQ_SETTING_MASK)
+#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
+	(((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) & TX_EQ_SETTING_MASK)
+/*
+ * Decode Host TX Equalization settings based on qTxEQGnSettings bit assignment:
+ * bit[35:32]: Host TX Logical LANE 0 PreShoot
+ * bit[39:36]: Host TX Logical LANE 1 PreShoot
+ * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
+ * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
+ */
+#define TX_EQ_HOST_PRESHOOT_DECODE(eq, lane) \
+	(((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT + 32)) & TX_EQ_SETTING_MASK)
+#define TX_EQ_HOST_DEEMPHASIS_DECODE(eq, lane) \
+	(((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 48)) & TX_EQ_SETTING_MASK)
+
+/*
+ * Decode Device TX precode_en indication based on dTxEQGnSettingsExt bit assignment:
+ * bit[0]: PreCodeEn for Device TX Logical LANE 0
+ * bit[1]: PreCodeEn for Device TX Logical LANE 1
+ */
+#define TX_EQ_DEVICE_PRECODE_EN_DECODE(eq_ext, lane) \
+	(!!((eq_ext) & (1 << (lane))))
+/*
+ * Decode Host TX precode_en indication based on dTxEQGnSettingsExt bit assignment:
+ * bit[4]: PreCodeEn for Device RX Logical LANE 0
+ * bit[5]: PreCodeEn for Device RX Logical LANE 1
+ */
+#define TX_EQ_HOST_PRECODE_EN_DECODE(eq_ext, lane) \
+	(!!((eq_ext) & (1 << ((lane) + 4))))
+
+/*
+ * Encode qTxEQGnSettings based on bit assignment:
+ * bit[3:0]: Device TX Logical LANE 0 PreShoot
+ * bit[7:4]: Device TX Logical LANE 1 PreShoot
+ * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
+ * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
+ */
+#define TX_EQ_DEVICE_PRESHOOT_ENCODE(val, lane) \
+	(((val) & TX_EQ_SETTING_MASK) << ((lane) * TX_HS_PRESHOOT_SHIFT))
+#define TX_EQ_DEVICE_DEEMPHASIS_ENCODE(val, lane) \
+	(((val) & TX_EQ_SETTING_MASK) << ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16))
+/*
+ * Encode qTxEQGnSettings based on bit assignment:
+ * bit[35:32]: Host TX Logical LANE 0 PreShoot
+ * bit[39:36]: Host TX Logical LANE 1 PreShoot
+ * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
+ * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
+ */
+#define TX_EQ_HOST_PRESHOOT_ENCODE(val, lane) \
+	(((val) & TX_EQ_SETTING_MASK) << ((lane) * TX_HS_PRESHOOT_SHIFT + 32))
+#define TX_EQ_HOST_DEEMPHASIS_ENCODE(val, lane) \
+	(((val) & TX_EQ_SETTING_MASK) << ((lane) * TX_HS_DEEMPHASIS_SHIFT + 48))
+
+/*
+ * Encode dTxEQGnSettingsExt based on bit assignment:
+ * bit[0]: PreCodeEn for Device TX Logical LANE 0
+ * bit[1]: PreCodeEn for Device TX Logical LANE 1
+ */
+#define TX_EQ_DEVICE_PRECODE_EN_ENCODE(val, lane)	((val) << (lane))
+/*
+ * Encode dTxEQGnSettingsExt based on bit assignment:
+ * bit[4]: PreCodeEn for Device RX Logical LANE 0
+ * bit[5]: PreCodeEn for Device RX Logical LANE 1
+ */
+#define TX_EQ_HOST_PRECODE_EN_ENCODE(val, lane)		((val) << ((lane) + 4))
+
 static bool use_adaptive_txeq;
 module_param(use_adaptive_txeq, bool, 0644);
 MODULE_PARM_DESC(use_adaptive_txeq, "Find and apply optimal TX Equalization settings before changing Power Mode (default: false)");
@@ -40,6 +117,28 @@ static bool txeq_presets_selected[UFS_TX_EQ_PRESET_MAX] = {[0 ... (UFS_TX_EQ_PRE
 module_param_array(txeq_presets_selected, bool, NULL, 0644);
 MODULE_PARM_DESC(txeq_presets_selected, "Use only the selected Presets out of the 8 TX Equalization Presets for TX EQTR");
 
+static int txeq_setting_sel_set(const char *val, const struct kernel_param *kp)
+{
+	return param_set_uint_minmax(val, kp, 0, 1);
+}
+
+static const struct kernel_param_ops txeq_setting_sel_ops = {
+	.set = txeq_setting_sel_set,
+	.get = param_get_uint,
+};
+
+static unsigned int txeq_setting_sel;
+module_param_cb(txeq_setting_sel, &txeq_setting_sel_ops, &txeq_setting_sel, 0644);
+MODULE_PARM_DESC(txeq_setting_sel, "The qTxEQGnSettings and dTxEQGnSettingsExt Attributes selector used to retrieve and store TX Equalization settings");
+
+static bool retrieve_txeq_setting = true;
+module_param(retrieve_txeq_setting, bool, 0644);
+MODULE_PARM_DESC(retrieve_txeq_setting, "Retrieve TX Equalization settings from qTxEQGnSettings and dTxEQGnSettingsExt Attributes (default: true)");
+
+static bool store_txeq_setting = true;
+module_param(store_txeq_setting, bool, 0644);
+MODULE_PARM_DESC(store_txeq_setting, "Store the optimal TX Equalization settings to qTxEQGnSettings and dTxEQGnSettingsExt Attributes (default: true)");
+
 /*
  * ufs_tx_eq_preset - Table of minimum required list of presets.
  *
@@ -1164,6 +1263,7 @@ int ufshcd_config_tx_eq_settings(struct ufs_hba *hba,
 
 		/* Mark TX Equalization settings as valid */
 		params->is_valid = true;
+		params->is_trained = true;
 		params->is_applied = false;
 	}
 
@@ -1291,3 +1391,144 @@ int ufshcd_retrain_tx_eq(struct ufs_hba *hba, u32 gear)
 
 	return ret;
 }
+
+/**
+ * ufshcd_extract_tx_eq_settings_attrs - Extract TX Equalization settings from UFS attributes
+ * @hba: per adapter instance
+ * @gear: target gear
+ *
+ * This function extracts previously stored TX Equalization settings from UFS
+ * attributes qTxEQGnSettings and dTxEQGnSettingsExt. These attributes contain
+ * the optimal TX Equalization parameters (PreShoot, DeEmphasis, and PreCoding
+ * enable) that were determined during a previous EQTR procedure.
+ *
+ * The function reads:
+ * 1. qTxEQGnSettings (64-bit): Main attribute containing PreShoot and
+ *    DeEmphasis values for both host and device TX lanes
+ * 2. dTxEQGnSettingsExt (32-bit): Extended attribute containing PreCoding
+ *    enable flags and validity indicator
+ */
+static void ufshcd_extract_tx_eq_settings_attrs(struct ufs_hba *hba, u8 gear)
+{
+	struct ufshcd_tx_eq_params *params;
+	u32 lane, eq_ext;
+	int ret;
+	u64 eq;
+
+	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				      QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS_EXT,
+				      gear - 1, (u8)txeq_setting_sel, &eq_ext);
+	if (ret)
+		return;
+
+	dev_dbg(hba->dev, "%s: HS-G%u dTxEQGnSettingsExt (Selector %u) = 0x%08x\n",
+		__func__, gear, txeq_setting_sel, eq_ext);
+
+	if (!(eq_ext & TX_EQ_SETTINGS_VALID_BIT))
+		return;
+
+	ret = ufshcd_query_attr_qword(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				      QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS,
+				      gear - 1, (u8)txeq_setting_sel, &eq);
+	if (ret)
+		return;
+
+	dev_dbg(hba->dev, "%s: HS-G%u qTxEQGnSettings (Selector %u) = 0x%016llx\n",
+		__func__, gear, txeq_setting_sel, eq);
+
+	params = &hba->tx_eq_params[gear - 1];
+
+	for (lane = 0; lane < UFS_MAX_LANES; lane++) {
+		params->host[lane].preshoot = TX_EQ_HOST_PRESHOOT_DECODE(eq, lane);
+		params->host[lane].deemphasis = TX_EQ_HOST_DEEMPHASIS_DECODE(eq, lane);
+		params->host[lane].precode_en = TX_EQ_HOST_PRECODE_EN_DECODE(eq_ext, lane);
+
+		params->device[lane].preshoot = TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane);
+		params->device[lane].deemphasis = TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane);
+		params->device[lane].precode_en = TX_EQ_DEVICE_PRECODE_EN_DECODE(eq_ext, lane);
+	}
+
+	params->is_valid = true;
+}
+
+void ufshcd_retrieve_tx_eq_settings(struct ufs_hba *hba)
+{
+	u8 gear = (u8)adaptive_txeq_gear;
+
+	if (!hba->max_pwr_info.is_valid || !ufshcd_is_tx_eq_supported(hba) ||
+	    !use_adaptive_txeq || !retrieve_txeq_setting)
+		return;
+
+	for (; gear <= UFS_HS_GEAR_MAX; gear++)
+		ufshcd_extract_tx_eq_settings_attrs(hba, gear);
+}
+
+/**
+ * ufshcd_update_tx_eq_settings_attrs - Update TX EQ settings in UFS attributes
+ * @hba: per adapter instance
+ * @gear: target gear
+ *
+ * This function stores the optimal TX Equalization settings obtained from
+ * TX EQTR procedure into UFS device attributes for future fast-path retrieval.
+ * The settings are stored in two complementary attributes:
+ *
+ * 1. qTxEQGnSettings (64-bit): Main attribute containing PreShoot and
+ *    DeEmphasis values for both host and device TX lanes
+ * 2. dTxEQGnSettingsExt (32-bit): Extended attribute containing PreCoding
+ *    enable flags and validity indicator
+ */
+static void ufshcd_update_tx_eq_settings_attrs(struct ufs_hba *hba, u8 gear)
+{
+	struct ufshcd_tx_eq_params *params;
+	u32 lane, eq_ext = 0;
+	u64 eq = 0;
+	int ret;
+
+	params = &hba->tx_eq_params[gear - 1];
+	if (!params->is_valid || !params->is_trained)
+		return;
+
+	for (lane = 0; lane < UFS_MAX_LANES; lane++) {
+		eq |= TX_EQ_HOST_PRESHOOT_ENCODE((u64)params->host[lane].preshoot, lane);
+		eq |= TX_EQ_HOST_DEEMPHASIS_ENCODE((u64)params->host[lane].deemphasis, lane);
+		eq_ext |= TX_EQ_HOST_PRECODE_EN_ENCODE(params->host[lane].precode_en, lane);
+
+		eq |= TX_EQ_DEVICE_PRESHOOT_ENCODE((u64)params->device[lane].preshoot, lane);
+		eq |= TX_EQ_DEVICE_DEEMPHASIS_ENCODE((u64)params->device[lane].deemphasis, lane);
+		eq_ext |= TX_EQ_DEVICE_PRECODE_EN_ENCODE(params->device[lane].precode_en, lane);
+	}
+
+	/* Set validity flag to indicate valid settings are stored */
+	eq_ext |= TX_EQ_SETTINGS_VALID_BIT;
+
+	/* Write qTxEQGnSettings */
+	ret = ufshcd_query_attr_qword(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+				      QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS,
+				      gear - 1, (u8)txeq_setting_sel, &eq);
+	if (ret)
+		return;
+
+	/* Write dTxEQGnSettingsExt */
+	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+				      QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS_EXT,
+				      gear - 1, (u8)txeq_setting_sel, &eq_ext);
+	if (ret)
+		return;
+
+	dev_dbg(hba->dev, "%s: Saved HS-G%u qTxEQGnSettings (Selector %u) = 0x%016llx\n",
+		__func__, gear, txeq_setting_sel, eq);
+	dev_dbg(hba->dev, "%s: Saved HS-G%u dTxEQGnSettingsExt (Selector %u) = 0x%08x\n",
+		__func__, gear, txeq_setting_sel, eq_ext);
+}
+
+void ufshcd_store_tx_eq_settings(struct ufs_hba *hba)
+{
+	u8 gear = (u8)adaptive_txeq_gear;
+
+	if (!hba->max_pwr_info.is_valid || !ufshcd_is_tx_eq_supported(hba) ||
+	    !use_adaptive_txeq || !store_txeq_setting)
+		return;
+
+	for (; gear <= UFS_HS_GEAR_MAX; gear++)
+		ufshcd_update_tx_eq_settings_attrs(hba, gear);
+}
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index ed1adeb22ec6..70f90d97f217 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -118,6 +118,8 @@ void ufshcd_print_tx_eq_params(struct ufs_hba *hba);
 bool ufshcd_is_txeq_presets_used(struct ufs_hba *hba);
 bool ufshcd_is_txeq_preset_selected(u8 preshoot, u8 deemphasis);
 int ufshcd_retrain_tx_eq(struct ufs_hba *hba, u32 gear);
+void ufshcd_retrieve_tx_eq_settings(struct ufs_hba *hba);
+void ufshcd_store_tx_eq_settings(struct ufs_hba *hba);
 
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e5e22adcbbc3..bf6deb34a5fe 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9128,6 +9128,8 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
 		dev_err(hba->dev,
 			"%s: Failed getting max supported power mode\n",
 			__func__);
+
+	ufshcd_retrieve_tx_eq_settings(hba);
 out:
 	return ret;
 }
@@ -10748,6 +10750,9 @@ static void ufshcd_wl_shutdown(struct scsi_device *sdev)
 
 	/* Turn on everything while shutting down */
 	ufshcd_rpm_get_sync(hba);
+
+	ufshcd_store_tx_eq_settings(hba);
+
 	scsi_device_quiesce(sdev);
 	shost_for_each_device(sdev, hba->host) {
 		if (sdev == hba->ufs_device_wlun)
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 602aa34c9822..0d48e137d66d 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -191,6 +191,8 @@ enum attr_idn {
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_HINT	= 0x3C,
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_EN		= 0x3D,
 	QUERY_ATTR_IDN_WB_BUF_RESIZE_STATUS	= 0x3E,
+	QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS        = 0x47,
+	QUERY_ATTR_IDN_TX_EQ_GN_SETTINGS_EXT    = 0x48,
 };
 
 /* Descriptor idn for Query requests */
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index cfbc75d8df83..f48d6416e299 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -358,6 +358,7 @@ struct ufshcd_tx_eqtr_record {
  * @eqtr_record: Pointer to TX EQTR record
  * @is_valid: True if parameter contains valid TX Equalization settings
  * @is_applied: True if settings have been applied to UniPro of both sides
+ * @is_trained: True if parameters obtained from TX EQTR procedure
  */
 struct ufshcd_tx_eq_params {
 	struct ufshcd_tx_eq_settings host[UFS_MAX_LANES];
@@ -365,6 +366,7 @@ struct ufshcd_tx_eq_params {
 	struct ufshcd_tx_eqtr_record *eqtr_record;
 	bool is_valid;
 	bool is_applied;
+	bool is_trained;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Peter Wang (王信友)]

On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
> Add support for UFS v5.0 JEDEC attributes qTxEQGnSettings and
> wTxEQGnSettingsExt to enable persistent storage and retrieval of
> optimal TX Equalization settings.
> 
> This provides a fast-path for TX Equalization by reusing previously
> stored optimal settings, avoiding TX Equalization Training (EQTR)
> procedures during subsequent Power Mode changes.
> 
> When no valid TX Equalization settings are found, fall back to full
> TX
> EQTR procedures and optionally save the results for future use.
> 
> The validity of one set of TX Equalization settings is indicated by
> Bit[15] in wTxEQGnSettingsExt.
> 
> Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> ---
>  drivers/ufs/core/ufs-txeq.c    | 241
> +++++++++++++++++++++++++++++++++
>  drivers/ufs/core/ufshcd-priv.h |   2 +
>  drivers/ufs/core/ufshcd.c      |   5 +
>  include/ufs/ufs.h              |   2 +
>  include/ufs/ufshcd.h           |   2 +
>  5 files changed, 252 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-
> txeq.c
> index b2dc89124353..aa9420b0d1c3 100644
> --- a/drivers/ufs/core/ufs-txeq.c
> +++ b/drivers/ufs/core/ufs-txeq.c
> @@ -14,6 +14,83 @@
>  #include <ufs/unipro.h>
>  #include "ufshcd-priv.h"
> 
> +#define TX_EQ_SETTING_MASK             0x7
> +#define TX_EQ_SETTINGS_VALID_BIT       BIT(15)
> +
> +/*
> + * Decode Device TX Equalization settings based on qTxEQGnSettings
> bit assignment:
> + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> + */
> +#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
> +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) &
> TX_EQ_SETTING_MASK)
> +#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
> +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) &
> TX_EQ_SETTING_MASK)
> +/*
> + * Decode Host TX Equalization settings based on qTxEQGnSettings bit
> assignment:
> + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> + */
> +#define TX_EQ_HOST_PRESHOOT_DECODE(eq, lane) \
> +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT + 32)) &
> TX_EQ_SETTING_MASK)
> +#define TX_EQ_HOST_DEEMPHASIS_DECODE(eq, lane) \
> +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 48)) &
> TX_EQ_SETTING_MASK)
> +
> +/*
> + * Decode Device TX precode_en indication based on
> dTxEQGnSettingsExt bit assignment:
> + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> + */
> +#define TX_EQ_DEVICE_PRECODE_EN_DECODE(eq_ext, lane) \
> +       (!!((eq_ext) & (1 << (lane))))
> +/*
> + * Decode Host TX precode_en indication based on dTxEQGnSettingsExt
> bit assignment:
> + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> + */
> +#define TX_EQ_HOST_PRECODE_EN_DECODE(eq_ext, lane) \
> +       (!!((eq_ext) & (1 << ((lane) + 4))))
> +
> +/*
> + * Encode qTxEQGnSettings based on bit assignment:
> + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> + */
> +#define TX_EQ_DEVICE_PRESHOOT_ENCODE(val, lane) \
> +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> TX_HS_PRESHOOT_SHIFT))
> +#define TX_EQ_DEVICE_DEEMPHASIS_ENCODE(val, lane) \
> +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> TX_HS_DEEMPHASIS_SHIFT + 16))
> +/*
> + * Encode qTxEQGnSettings based on bit assignment:
> + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> + */
> +#define TX_EQ_HOST_PRESHOOT_ENCODE(val, lane) \
> +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> TX_HS_PRESHOOT_SHIFT + 32))
> +#define TX_EQ_HOST_DEEMPHASIS_ENCODE(val, lane) \
> +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> TX_HS_DEEMPHASIS_SHIFT + 48))
> +
> +/*
> + * Encode dTxEQGnSettingsExt based on bit assignment:
> + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> + */
> +#define TX_EQ_DEVICE_PRECODE_EN_ENCODE(val, lane)      ((val) <<
> (lane))
> +/*
> + * Encode dTxEQGnSettingsExt based on bit assignment:
> + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> + */
> +#define TX_EQ_HOST_PRECODE_EN_ENCODE(val, lane)               
> ((val) << ((lane) + 4))
> +
> 

Hi Can,

Could you remove redundant parentheses, such as
(val), (lane), (eq_ext), and (eq)?


> +static unsigned int txeq_setting_sel;
> +module_param_cb(txeq_setting_sel, &txeq_setting_sel_ops,
> &txeq_setting_sel, 0644);
> +MODULE_PARM_DESC(txeq_setting_sel, "The qTxEQGnSettings and
> dTxEQGnSettingsExt Attributes selector used to retrieve and store TX
> Equalization settings");

Why is this selection necessary? Shouldn't we follow 
the JEDEC specification? As Bart said, introducing new 
kernel module parameters is easy, but removing them is hard.

Thanks
Peter

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Can Guo]

On 4/20/2026 8:33 PM, Peter Wang (王信友) wrote:
>
> On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
> > Add support for UFS v5.0 JEDEC attributes qTxEQGnSettings and
> > wTxEQGnSettingsExt to enable persistent storage and retrieval of
> > optimal TX Equalization settings.
> > 
> > This provides a fast-path for TX Equalization by reusing previously
> > stored optimal settings, avoiding TX Equalization Training (EQTR)
> > procedures during subsequent Power Mode changes.
> > 
> > When no valid TX Equalization settings are found, fall back to full
> > TX
> > EQTR procedures and optionally save the results for future use.
> > 
> > The validity of one set of TX Equalization settings is indicated by
> > Bit[15] in wTxEQGnSettingsExt.
> > 
> > Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> > ---
> >  drivers/ufs/core/ufs-txeq.c    | 241
> > +++++++++++++++++++++++++++++++++
> >  drivers/ufs/core/ufshcd-priv.h |   2 +
> >  drivers/ufs/core/ufshcd.c      |   5 +
> >  include/ufs/ufs.h              |   2 +
> >  include/ufs/ufshcd.h           |   2 +
> >  5 files changed, 252 insertions(+)
> > 
> > diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-
> > txeq.c
> > index b2dc89124353..aa9420b0d1c3 100644
> > --- a/drivers/ufs/core/ufs-txeq.c
> > +++ b/drivers/ufs/core/ufs-txeq.c
> > @@ -14,6 +14,83 @@
> >  #include <ufs/unipro.h>
> >  #include "ufshcd-priv.h"
> > 
> > +#define TX_EQ_SETTING_MASK             0x7
> > +#define TX_EQ_SETTINGS_VALID_BIT       BIT(15)
> > +
> > +/*
> > + * Decode Device TX Equalization settings based on qTxEQGnSettings
> > bit assignment:
> > + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> > + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> > + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> > + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) &
> > TX_EQ_SETTING_MASK)
> > +#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) &
> > TX_EQ_SETTING_MASK)
> > +/*
> > + * Decode Host TX Equalization settings based on qTxEQGnSettings bit
> > assignment:
> > + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> > + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> > + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> > + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_HOST_PRESHOOT_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT + 32)) &
> > TX_EQ_SETTING_MASK)
> > +#define TX_EQ_HOST_DEEMPHASIS_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 48)) &
> > TX_EQ_SETTING_MASK)
> > +
> > +/*
> > + * Decode Device TX precode_en indication based on
> > dTxEQGnSettingsExt bit assignment:
> > + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> > + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> > + */
> > +#define TX_EQ_DEVICE_PRECODE_EN_DECODE(eq_ext, lane) \
> > +       (!!((eq_ext) & (1 << (lane))))
> > +/*
> > + * Decode Host TX precode_en indication based on dTxEQGnSettingsExt
> > bit assignment:
> > + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> > + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> > + */
> > +#define TX_EQ_HOST_PRECODE_EN_DECODE(eq_ext, lane) \
> > +       (!!((eq_ext) & (1 << ((lane) + 4))))
> > +
> > +/*
> > + * Encode qTxEQGnSettings based on bit assignment:
> > + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> > + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> > + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> > + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_DEVICE_PRESHOOT_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_PRESHOOT_SHIFT))
> > +#define TX_EQ_DEVICE_DEEMPHASIS_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_DEEMPHASIS_SHIFT + 16))
> > +/*
> > + * Encode qTxEQGnSettings based on bit assignment:
> > + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> > + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> > + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> > + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_HOST_PRESHOOT_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_PRESHOOT_SHIFT + 32))
> > +#define TX_EQ_HOST_DEEMPHASIS_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_DEEMPHASIS_SHIFT + 48))
> > +
> > +/*
> > + * Encode dTxEQGnSettingsExt based on bit assignment:
> > + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> > + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> > + */
> > +#define TX_EQ_DEVICE_PRECODE_EN_ENCODE(val, lane)      ((val) <<
> > (lane))
> > +/*
> > + * Encode dTxEQGnSettingsExt based on bit assignment:
> > + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> > + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> > + */
> > +#define TX_EQ_HOST_PRECODE_EN_ENCODE(val, lane)               
> > ((val) << ((lane) + 4))
> > +
> > 
>
> Hi Can,
>
> Could you remove redundant parentheses, such as
> (val), (lane), (eq_ext), and (eq)?
>
>
> > +static unsigned int txeq_setting_sel;
> > +module_param_cb(txeq_setting_sel, &txeq_setting_sel_ops,
> > &txeq_setting_sel, 0644);
> > +MODULE_PARM_DESC(txeq_setting_sel, "The qTxEQGnSettings and
> > dTxEQGnSettingsExt Attributes selector used to retrieve and store TX
> > Equalization settings");
>
> Why is this selection necessary? Shouldn't we follow
> the JEDEC specification? As Bart said, introducing new
> kernel module parameters is easy, but removing them is hard.
Hi Peter,

I proposed the two Attributes to JEDEC spec with two Selectors supported 
initially
to provide flexibility for use cases where we want to keep two different 
settings for
the same HS-Gear, for example:

1. 0 for Room temperature, 1 for high temperature.
2. 0 for Rate-A, 1 for Rate-B.
3. 0 for Linux, 1 for SBL.

The usage of the two Selectors is not limited to above examples. Yet it 
is hard to get
aligned on how to use the two Selectors across different companies. 
Hence I am adding
a module parameter.

Thanks,
Can Guo.
>
> Thanks
> Peter
>
> ************* MEDIATEK Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Bart Van Assche]

On 4/20/26 6:23 AM, Can Guo wrote:
> The usage of the two Selectors is not limited to above examples. Yet it 
> is hard to get
> aligned on how to use the two Selectors across different companies. 
> Hence I am adding
> a module parameter.

These kernel module parameters can be converted into sysfs attributes,
isn't it? Converting these kernel module parameters into sysfs
attributes has the advantage that different values can be configured per
host controller in systems with multiple UFS host controllers.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Can Guo]

Hi Bart,

On 4/21/2026 12:28 AM, Bart Van Assche wrote:
> On 4/20/26 6:23 AM, Can Guo wrote:
>> The usage of the two Selectors is not limited to above examples. Yet 
>> it is hard to get
>> aligned on how to use the two Selectors across different companies. 
>> Hence I am adding
>> a module parameter.
>
> These kernel module parameters can be converted into sysfs attributes,
> isn't it? Converting these kernel module parameters into sysfs
> attributes has the advantage that different values can be configured per
> host controller in systems with multiple UFS host controllers.
Thanks for the suggestion.

For this series, these knobs are needed before the first HS-G6 power 
mode change,
which happens during early UFS bring-up in probe. At that point, 
userspace-driven
sysfs attributes are not available yet, so relying only on sysfs would 
miss the first
power mode transition.

That is why this patch uses module parameters (boot-time configurable 
via kernel
command line).

I agree sysfs is better for per-host runtime tuning on multi-controller 
systems.
We can add per-host sysfs controls as a follow-up later on, while 
keeping boot-time
defaults through module parameters for the first-transition requirement.

Thanks,
Can Guo.
>
> Thanks,
>
> Bart.

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Bart Van Assche]

On 4/19/26 6:52 AM, Can Guo wrote:
> +#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
> +	(((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) & TX_EQ_SETTING_MASK)
> +#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
> +	(((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) & TX_EQ_SETTING_MASK)

Please convert these two macros and also all the other new macros in
this patch that accept arguments into inline functions.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Can Guo]

On 4/21/2026 1:01 AM, Bart Van Assche wrote:
> On 4/19/26 6:52 AM, Can Guo wrote:
>> +#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
>> +    (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) & TX_EQ_SETTING_MASK)
>> +#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
>> +    (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) & 
>> TX_EQ_SETTING_MASK)
>
> Please convert these two macros and also all the other new macros in
> this patch that accept arguments into inline functions.
Will do.

Thanks,
Can Guo.
>
> Thanks,
>
> Bart.

Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

[Bean Huo]

>  /* Wrapper functions for safely calling variant operations */
>  static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e5e22adcbbc3..bf6deb34a5fe 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9128,6 +9128,8 @@ static int ufshcd_device_params_init(struct ufs_hba
> *hba)
>                 dev_err(hba->dev,
>                         "%s: Failed getting max supported power mode\n",
>                         __func__);
> +
> +       ufshcd_retrieve_tx_eq_settings(hba);

needs to check wspecversion before calling it?



Kind regards,
Bean