mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties
       [not found] <20260610071516.3763916-1-can.guo@oss.qualcomm.com>
@ 2026-06-10  7:15 ` Can Guo
  2026-06-11 13:32   ` Krzysztof Kozlowski
  2026-06-10  7:15 ` [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings Can Guo
  1 sibling, 1 reply; 8+ messages in thread
From: Can Guo @ 2026-06-10  7:15 UTC (permalink / raw)
  To: krzk, bvanassche, beanhuo, peter.wang, martin.petersen, mani
  Cc: linux-scsi, Can Guo, Alim Akhtar, Avri Altman, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Zhaoming Luo,
	Ram Kumar Dwivedi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list

UFS v5.0/UFSHCI v5.0 add HS-G6 support via UniPro v3.0 and M-PHY v6.0.
These specs define TX Equalization for all High Speed Gears, and HS-G6 may
also require TX precode depending on channel characteristics.

Document vendor-neutral DT properties in ufs-common.yaml:

- patternProperties for txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6]
- tx-precode-g6-host-lanes
- tx-precode-g6-device-lanes

txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6] accept per-lane tuples:
<Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]

PreShoot and DeEmphasis values are 0..7 and accept 2 or 4 values for x1/x2
lane configurations.

tx-precode-g6-host-lanes and tx-precode-g6-device-lanes list lane indices
where precode is enabled on host and device sides.

Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
---
 .../devicetree/bindings/ufs/ufs-common.yaml   | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/Documentation/devicetree/bindings/ufs/ufs-common.yaml b/Documentation/devicetree/bindings/ufs/ufs-common.yaml
index ed97f5682509..2d53bbbe5865 100644
--- a/Documentation/devicetree/bindings/ufs/ufs-common.yaml
+++ b/Documentation/devicetree/bindings/ufs/ufs-common.yaml
@@ -105,6 +105,67 @@ properties:
       Restricts the UFS controller to rate-a or rate-b for both TX and
       RX directions.
 
+  tx-precode-g6-host-lanes:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    uniqueItems: true
+    items:
+      minimum: 0
+      maximum: 1
+    description: |
+      Lane indices for static Host-side TX precode enable settings for HS-G6
+      only. Listed lanes have precode enabled; unlisted lanes are disabled.
+
+  tx-precode-g6-device-lanes:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+    uniqueItems: true
+    items:
+      minimum: 0
+      maximum: 1
+    description: |
+      Lane indices for static Device-side TX precode enable settings for HS-G6
+      only. Listed lanes have precode enabled; unlisted lanes are disabled.
+
+patternProperties:
+  "^txeq-preshoot-g[1-6]$":
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    oneOf:
+      - minItems: 2
+        maxItems: 2
+      - minItems: 4
+        maxItems: 4
+    items:
+      minimum: 0
+      maximum: 7
+    description: |
+      Static TX Equalization PreShoot settings for High Speed Gears. These
+      values are programmed to the corresponding UniPro PA layer attribute
+      PA_TxEQG[1-6]Setting. Each value selects a Pre-Shoot level as defined
+      by the MIPI M-PHY specification (TX_HS_PreShoot_Setting).
+      Values are specified as per-lane tuples:
+      <Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]
+
+  "^txeq-deemphasis-g[1-6]$":
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    oneOf:
+      - minItems: 2
+        maxItems: 2
+      - minItems: 4
+        maxItems: 4
+    items:
+      minimum: 0
+      maximum: 7
+    description: |
+      Static TX Equalization DeEmphasis settings for High Speed Gears. These
+      values are programmed to the corresponding UniPro PA layer attribute
+      PA_TxEQG[1-6]Setting. Each value selects a De-Emphasis level as defined
+      by the MIPI M-PHY specification (TX_HS_DeEmphasis_Setting).
+      Values are specified as per-lane tuples:
+      <Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]
+
 dependencies:
   freq-table-hz: [ clocks ]
   operating-points-v2: [ clocks, clock-names ]
-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings
       [not found] <20260610071516.3763916-1-can.guo@oss.qualcomm.com>
  2026-06-10  7:15 ` [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties Can Guo
@ 2026-06-10  7:15 ` Can Guo
  2026-06-10 20:08   ` Bart Van Assche
  2026-06-11 10:11   ` Peter Wang (王信友)
  1 sibling, 2 replies; 8+ messages in thread
From: Can Guo @ 2026-06-10  7:15 UTC (permalink / raw)
  To: krzk, bvanassche, beanhuo, peter.wang, martin.petersen, mani
  Cc: linux-scsi, Can Guo, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Ram Kumar Dwivedi, Nitin Rawat, open list

Parse board-specific static TX Equalization settings from DT for each HS
gear and store them in hba->tx_eq_params.

Parse txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6] as per-lane tuples:
<Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]

For HS-G6, parse optional lane lists:
- tx-precode-g6-host-lanes
- tx-precode-g6-device-lanes

Introduce is_static in struct ufshcd_tx_eq_params to track whether TX EQ
values came from static DT data.

When adaptive TX Equalization is used, these static settings are not final:

- If valid settings are retrieved from qTxEQGnSettings/wTxEQGnSettingsExt,
  those retrieved settings override static DT settings.
- If retrieval is not available/valid, TX EQTR runs and trained settings
  override static DT settings.

So static DT settings are a fallback and are intended for cases where
adaptive TX Equalization is not enabled/used. Adaptive TX Equalization
remains the primary path when enabled.

No behavior changes for platforms that do not provide these properties.

Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
---
 drivers/ufs/core/ufs-txeq.c      |  10 +-
 drivers/ufs/host/ufshcd-pltfrm.c | 159 +++++++++++++++++++++++++++++++
 include/ufs/ufshcd.h             |   2 +
 3 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-txeq.c
index 4b264adfdf49..b645fe5f6d95 100644
--- a/drivers/ufs/core/ufs-txeq.c
+++ b/drivers/ufs/core/ufs-txeq.c
@@ -1297,7 +1297,13 @@ int ufshcd_config_tx_eq_settings(struct ufs_hba *hba,
 	}
 
 	params = &hba->tx_eq_params[gear - 1];
-	if (!params->is_valid || force_tx_eqtr) {
+	/*
+	 * TX EQTR must run for the following cases:
+	 * 1. TX EQ settings are invalid.
+	 * 2. TX EQ settings are valid but static, i.e., populated from DT.
+	 * 3. TX EQTR procedure is forced.
+	 */
+	if (!params->is_valid || params->is_static || force_tx_eqtr) {
 		int ret;
 
 		ret = ufshcd_tx_eqtr(hba, params, pwr_mode);
@@ -1310,6 +1316,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_static = false;
 		params->is_applied = false;
 	}
 
@@ -1495,6 +1502,7 @@ static void ufshcd_extract_tx_eq_settings_attrs(struct ufs_hba *hba, u8 gear)
 	}
 
 	params->is_valid = true;
+	params->is_static = false;
 }
 
 void ufshcd_retrieve_tx_eq_settings(struct ufs_hba *hba)
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index c2dafb583cf5..e8b352f5b68f 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -210,6 +210,163 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
 	}
 }
 
+static int ufshcd_parse_tx_precode_lane_list(struct ufs_hba *hba,
+					     const char *prop_name,
+					     bool precode_en[UFS_MAX_LANES])
+{
+	const u32 lpd = hba->lanes_per_direction;
+	struct device *dev = hba->dev;
+	u32 lane_ids[UFS_MAX_LANES];
+	int len, count, err, i;
+	struct property *prop;
+
+	prop = of_find_property(dev->of_node, prop_name, &len);
+	if (!prop || !len)
+		return 0;
+
+	count = of_property_count_u32_elems(dev->of_node, prop_name);
+	if (count < 0) {
+		dev_err(dev, "Property %s is malformed, %d\n", prop_name, count);
+		return count;
+	}
+
+	if (count > lpd) {
+		dev_err(dev, "Property %s has invalid count (%d), max %u\n",
+			prop_name, count, lpd);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32_array(dev->of_node, prop_name, lane_ids, count);
+	if (err) {
+		dev_err(dev, "Failed to read %s property, %d\n", prop_name, err);
+		return err;
+	}
+
+	for (i = 0; i < count; i++) {
+		if (lane_ids[i] >= lpd) {
+			dev_err(dev, "Invalid lane index %u provided in %s property\n",
+				lane_ids[i], prop_name);
+			return -EINVAL;
+		}
+
+		precode_en[lane_ids[i]] = true;
+	}
+
+	return 0;
+}
+
+static int ufshcd_parse_tx_eq_value_array(struct ufs_hba *hba,
+					  const char *prop_name,
+					  const u32 max_value,
+					  u32 values[UFS_MAX_LANES * 2])
+{
+	u32 num_elems = 2 * hba->lanes_per_direction;
+	struct device *dev = hba->dev;
+	int count, err, i;
+
+	count = of_property_count_u32_elems(dev->of_node, prop_name);
+	if (count <= 0)
+		return count ? count : -ENOENT;
+
+	if (count != num_elems) {
+		dev_err(dev, "Property %s has invalid count (%d), expecting %u\n",
+			prop_name, count, num_elems);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32_array(dev->of_node, prop_name, values, num_elems);
+	if (err) {
+		dev_err(dev, "Failed to read %s property, %d\n", prop_name, err);
+		return err;
+	}
+
+	for (i = 0; i < num_elems; i++) {
+		if (values[i] >= max_value) {
+			dev_err(dev, "Invalid TX EQ value (%u) in %s property\n",
+				values[i], prop_name);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * ufshcd_parse_tx_eq_settings_for_gear - Parse static TX EQ DT settings for one gear
+ * @hba: per adapter instance
+ * @gear: target HS gear
+ *
+ * Reads the txeq-preshoot-gN, txeq-deemphasis-gN, and (for G6)
+ * tx-precode-g6-host-lanes/tx-precode-g6-device-lanes device-tree properties.
+ * If all present values are valid, stores them as static TX Equalization
+ * settings for the given gear.
+ */
+static void ufshcd_parse_tx_eq_settings_for_gear(struct ufs_hba *hba, int gear)
+{
+	bool device_precode_en[UFS_MAX_LANES] = { false };
+	bool host_precode_en[UFS_MAX_LANES] = { false };
+	const u32 lpd = hba->lanes_per_direction;
+	struct ufshcd_tx_eq_params *params;
+	u32 deemphasis[UFS_MAX_LANES * 2];
+	u32 preshoot[UFS_MAX_LANES * 2];
+	char prop_name[MAX_PROP_SIZE];
+	int err, lane;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "txeq-preshoot-g%d", gear);
+	err = ufshcd_parse_tx_eq_value_array(hba, prop_name, TX_HS_NUM_PRESHOOT, preshoot);
+	if (err)
+		return;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "txeq-deemphasis-g%d", gear);
+	err = ufshcd_parse_tx_eq_value_array(hba, prop_name, TX_HS_NUM_DEEMPHASIS, deemphasis);
+	if (err)
+		return;
+
+	if (gear == UFS_HS_G6) {
+		err = ufshcd_parse_tx_precode_lane_list(hba, "tx-precode-g6-host-lanes",
+							host_precode_en);
+		if (err)
+			return;
+
+		err = ufshcd_parse_tx_precode_lane_list(hba, "tx-precode-g6-device-lanes",
+							device_precode_en);
+		if (err)
+			return;
+	}
+
+	params = &hba->tx_eq_params[gear - 1];
+	for (lane = 0; lane < lpd; lane++) {
+		params->host[lane].preshoot = preshoot[lane * 2];
+		params->host[lane].deemphasis = deemphasis[lane * 2];
+		params->host[lane].precode_en = host_precode_en[lane];
+
+		params->device[lane].preshoot = preshoot[lane * 2 + 1];
+		params->device[lane].deemphasis = deemphasis[lane * 2 + 1];
+		params->device[lane].precode_en = device_precode_en[lane];
+	}
+
+	params->is_valid = true;
+	params->is_static = true;
+}
+
+static void ufshcd_parse_static_tx_eq_settings(struct ufs_hba *hba)
+{
+	const u32 lpd = hba->lanes_per_direction;
+	int gear;
+
+	if (!lpd)
+		return;
+
+	if (lpd > UFS_MAX_LANES) {
+		dev_warn(hba->dev, "lanes_per_direction (%u) exceeds UFS_MAX_LANES (%u)\n",
+			 lpd, UFS_MAX_LANES);
+		return;
+	}
+
+	for (gear = UFS_HS_G1; gear <= UFS_HS_GEAR_MAX; gear++)
+		ufshcd_parse_tx_eq_settings_for_gear(hba, gear);
+}
+
 /**
  * ufshcd_parse_clock_min_max_freq  - Parse MIN and MAX clocks freq
  * @hba: per adapter instance
@@ -528,6 +685,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 
 	ufshcd_init_lanes_per_dir(hba);
 
+	ufshcd_parse_static_tx_eq_settings(hba);
+
 	err = ufshcd_parse_operating_points(hba);
 	if (err) {
 		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index f48d6416e299..c01824576472 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -359,6 +359,7 @@ struct ufshcd_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
+ * @is_static: True if settings are static
  */
 struct ufshcd_tx_eq_params {
 	struct ufshcd_tx_eq_settings host[UFS_MAX_LANES];
@@ -367,6 +368,7 @@ struct ufshcd_tx_eq_params {
 	bool is_valid;
 	bool is_applied;
 	bool is_trained;
+	bool is_static;
 };
 
 /**
-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings
  2026-06-10  7:15 ` [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings Can Guo
@ 2026-06-10 20:08   ` Bart Van Assche
  2026-06-11  1:26     ` Can Guo
  2026-06-11 10:11   ` Peter Wang (王信友)
  1 sibling, 1 reply; 8+ messages in thread
From: Bart Van Assche @ 2026-06-10 20:08 UTC (permalink / raw)
  To: Can Guo, krzk, beanhuo, peter.wang, martin.petersen, mani
  Cc: linux-scsi, Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Ram Kumar Dwivedi, Nitin Rawat, open list

On 6/10/26 12:15 AM, Can Guo wrote:
> Parse board-specific static TX Equalization settings from DT for each HS
> gear and store them in hba->tx_eq_params.

The word "static" means "showing little change". My understanding is
that the settings from the DT tree are used if equalization training is
not performed. If my understanding is correct, I think the use of the
word "static" is misleading. Maybe "default" or "from_dt" reflects the
purpose of these settings better?

> When adaptive TX Equalization is used, these static settings are not final:

What is the meaning of "adaptive" in the above sentence? I haven't found 
that word in the UFSHCI 5.0 standard nor in the UFS 5.0 standard in the
context of TX equalization. Should that word perhaps be left out?

> +	/*
> +	 * TX EQTR must run for the following cases:
> +	 * 1. TX EQ settings are invalid.
> +	 * 2. TX EQ settings are valid but static, i.e., populated from DT.
> +	 * 3. TX EQTR procedure is forced.
> +	 */

What is the difference between "TX EQ" and "TX EQTR"? If both refer to
TX equalization, please use the same acronym for all three bullets.

>   	params->is_valid = true;
> +	params->is_static = false;
>   }

Why is "is_static" changed into false here? A comment might be
appropriate.

> +	ufshcd_parse_static_tx_eq_settings(hba);

Please consider changing "static" into "default" or "dt" (device tree)
in the above function name. I think that will make the code easier to
follow.

Otherwise this patch looks good to me.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings
  2026-06-10 20:08   ` Bart Van Assche
@ 2026-06-11  1:26     ` Can Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Can Guo @ 2026-06-11  1:26 UTC (permalink / raw)
  To: Bart Van Assche, krzk, beanhuo, peter.wang, martin.petersen, mani
  Cc: linux-scsi, Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Ram Kumar Dwivedi, Nitin Rawat, open list



On 6/11/2026 4:08 AM, Bart Van Assche wrote:
> On 6/10/26 12:15 AM, Can Guo wrote:
>> Parse board-specific static TX Equalization settings from DT for each HS
>> gear and store them in hba->tx_eq_params.
>
> The word "static" means "showing little change". My understanding is
> that the settings from the DT tree are used if equalization training is
> not performed. If my understanding is correct, I think the use of the
> word "static" is misleading. Maybe "default" or "from_dt" reflects the
> purpose of these settings better?
Thanks for your review.

'from_dt' sounds better.
>
>> When adaptive TX Equalization is used, these static settings are not 
>> final:
>
> What is the meaning of "adaptive" in the above sentence? I haven't 
> found that word in the UFSHCI 5.0 standard nor in the UFS 5.0 standard 
> in the
> context of TX equalization. Should that word perhaps be left out?
'adaptive' is not standard term in spec, I used it to refer to on-demand TX
Equalization Training procedure I put up in the first patch series. If it is
confusing, I can remove it from the commit msg.
>
>> +    /*
>> +     * TX EQTR must run for the following cases:
>> +     * 1. TX EQ settings are invalid.
>> +     * 2. TX EQ settings are valid but static, i.e., populated from DT.
>> +     * 3. TX EQTR procedure is forced.
>> +     */
>
> What is the difference between "TX EQ" and "TX EQTR"? If both refer to
> TX equalization, please use the same acronym for all three bullets.
TX EQ is TX Equalization.
TX EQTR is TX Equalization Training.
>
>>       params->is_valid = true;
>> +    params->is_static = false;
>>   }
>
> Why is "is_static" changed into false here? A comment might be
> appropriate.
After TX EQTR procedure, the params is populated with training results,
which are no longer 'from_dt'. I will add some comments in next version.
>
>> + ufshcd_parse_static_tx_eq_settings(hba);
>
> Please consider changing "static" into "default" or "dt" (device tree)
> in the above function name. I think that will make the code easier to
> follow.
Sure, will use 'from_dt'.

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings
  2026-06-10  7:15 ` [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings Can Guo
  2026-06-10 20:08   ` Bart Van Assche
@ 2026-06-11 10:11   ` Peter Wang (王信友)
  2026-06-11 14:15     ` Can Guo
  1 sibling, 1 reply; 8+ messages in thread
From: Peter Wang (王信友) @ 2026-06-11 10:11 UTC (permalink / raw)
  To: beanhuo, mani, can.guo, bvanassche, krzk, martin.petersen
  Cc: linux-scsi, quic_nitirawa, linux-kernel, alim.akhtar,
	avri.altman, quic_rdwivedi, James.Bottomley

On Wed, 2026-06-10 at 00:15 -0700, Can Guo wrote:
> +static int ufshcd_parse_tx_eq_value_array(struct ufs_hba *hba,
> +                                         const char *prop_name,
> +                                         const u32 max_value,
> +                                         u32 values[UFS_MAX_LANES *
> 2])
> +{
> +       u32 num_elems = 2 * hba->lanes_per_direction;
> +       struct device *dev = hba->dev;
> +       int count, err, i;
> +
> +       count = of_property_count_u32_elems(dev->of_node, prop_name);
> +       if (count <= 0)
> +               return count ? count : -ENOENT;
> 

Hi Can,

Returning -ENOENT when count == 0 is not correct.
count == 0 means "empty property," whereas -ENOENT means "property 
not present."
I suggest only checking for < 0 as follows:
if (count < 0)
    return count;

and letting the count == 0 case be handled below:
if (count != num_elems) {
    ...
    return -EINVAL;
}

Thanks.
Peter


> +
> +       if (count != num_elems) {
> +               dev_err(dev, "Property %s has invalid count (%d),
> expecting %u\n",
> +                       prop_name, count, num_elems);
> +               return -EINVAL;
> +       }


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties
  2026-06-10  7:15 ` [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties Can Guo
@ 2026-06-11 13:32   ` Krzysztof Kozlowski
  2026-06-11 14:14     ` Can Guo
  0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2026-06-11 13:32 UTC (permalink / raw)
  To: Can Guo, bvanassche, beanhuo, peter.wang, martin.petersen, mani
  Cc: linux-scsi, Alim Akhtar, Avri Altman, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Zhaoming Luo,
	Ram Kumar Dwivedi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list

On 10/06/2026 09:15, Can Guo wrote:
> UFS v5.0/UFSHCI v5.0 add HS-G6 support via UniPro v3.0 and M-PHY v6.0.
> These specs define TX Equalization for all High Speed Gears, and HS-G6 may
> also require TX precode depending on channel characteristics.
> 
> Document vendor-neutral DT properties in ufs-common.yaml:
> 
> - patternProperties for txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6]
> - tx-precode-g6-host-lanes
> - tx-precode-g6-device-lanes
> 
> txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6] accept per-lane tuples:
> <Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]

Instead of repeating the diff, you should explain why these properties
are needed. Insufficient explanation was also pointed out at v1.

Why this cannot be deduced from the IP compatible? Does it depend on the
device memory? Who determines the values here and what do they depend
on? Also here you explain lack of auto tuning for example.

> 
> PreShoot and DeEmphasis values are 0..7 and accept 2 or 4 values for x1/x2
> lane configurations.
> 

...


> +      Lane indices for static Host-side TX precode enable settings for HS-G6
> +      only. Listed lanes have precode enabled; unlisted lanes are disabled.
> +
> +  tx-precode-g6-device-lanes:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 2
> +    uniqueItems: true
> +    items:
> +      minimum: 0
> +      maximum: 1
> +    description: |
> +      Lane indices for static Device-side TX precode enable settings for HS-G6
> +      only. Listed lanes have precode enabled; unlisted lanes are disabled.


I need to reverse my opinion and let's go to v6 implementation. These
properties look more consistent in v6 with respect to preshoot and
deepmhasis properties.

You want actually matrix, so:

  tx-precode-enable-g6:
    $ref: /schemas/types.yaml#/definitions/uint32-matrix
    oneOf:
      - items:
          - description: Host_Lane0 precode
          - description: Device_Lane0 precode
      - items:
          - description: Host_Lane0 precode
          - description: Device_Lane0 precode
          - description: Host_Lane1 precode
          - description: Device_Lane1 precode
    items:
      enum: [0, 1]
    description:
      Static TX Precode enable values for HS-G6 only.

And similar style with items also for preshoot and deepmhasis.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties
  2026-06-11 13:32   ` Krzysztof Kozlowski
@ 2026-06-11 14:14     ` Can Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Can Guo @ 2026-06-11 14:14 UTC (permalink / raw)
  To: Krzysztof Kozlowski, bvanassche, beanhuo, peter.wang,
	martin.petersen, mani
  Cc: linux-scsi, Alim Akhtar, Avri Altman, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Zhaoming Luo,
	Ram Kumar Dwivedi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list



On 6/11/2026 9:32 PM, Krzysztof Kozlowski wrote:
> On 10/06/2026 09:15, Can Guo wrote:
>> UFS v5.0/UFSHCI v5.0 add HS-G6 support via UniPro v3.0 and M-PHY v6.0.
>> These specs define TX Equalization for all High Speed Gears, and HS-G6 may
>> also require TX precode depending on channel characteristics.
>>
>> Document vendor-neutral DT properties in ufs-common.yaml:
>>
>> - patternProperties for txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6]
>> - tx-precode-g6-host-lanes
>> - tx-precode-g6-device-lanes
>>
>> txeq-preshoot-g[1-6] and txeq-deemphasis-g[1-6] accept per-lane tuples:
>> <Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]
> Instead of repeating the diff, you should explain why these properties
> are needed. Insufficient explanation was also pointed out at v1.
>
> Why this cannot be deduced from the IP compatible? Does it depend on the
> device memory? Who determines the values here and what do they depend
> on? Also here you explain lack of auto tuning for example.
I will give more explanations in next version.
>
>> PreShoot and DeEmphasis values are 0..7 and accept 2 or 4 values for x1/x2
>> lane configurations.
>>
> ...
>
>
>> +      Lane indices for static Host-side TX precode enable settings for HS-G6
>> +      only. Listed lanes have precode enabled; unlisted lanes are disabled.
>> +
>> +  tx-precode-g6-device-lanes:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 1
>> +    maxItems: 2
>> +    uniqueItems: true
>> +    items:
>> +      minimum: 0
>> +      maximum: 1
>> +    description: |
>> +      Lane indices for static Device-side TX precode enable settings for HS-G6
>> +      only. Listed lanes have precode enabled; unlisted lanes are disabled.
>
> I need to reverse my opinion and let's go to v6 implementation. These
> properties look more consistent in v6 with respect to preshoot and
> deepmhasis properties.
>
> You want actually matrix, so:
>
>    tx-precode-enable-g6:
>      $ref: /schemas/types.yaml#/definitions/uint32-matrix
>      oneOf:
>        - items:
>            - description: Host_Lane0 precode
>            - description: Device_Lane0 precode
>        - items:
>            - description: Host_Lane0 precode
>            - description: Device_Lane0 precode
>            - description: Host_Lane1 precode
>            - description: Device_Lane1 precode
>      items:
>        enum: [0, 1]
>      description:
>        Static TX Precode enable values for HS-G6 only.
>
> And similar style with items also for preshoot and deepmhasis.
Thanks for the suggestions. Will update in next version and come back.

Best Regards,
Can Guo.
>
>
> Best regards,
> Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings
  2026-06-11 10:11   ` Peter Wang (王信友)
@ 2026-06-11 14:15     ` Can Guo
  0 siblings, 0 replies; 8+ messages in thread
From: Can Guo @ 2026-06-11 14:15 UTC (permalink / raw)
  To: Peter Wang (王信友),
	beanhuo, mani, bvanassche, krzk, martin.petersen
  Cc: linux-scsi, quic_nitirawa, linux-kernel, alim.akhtar,
	avri.altman, quic_rdwivedi, James.Bottomley



On 6/11/2026 6:11 PM, Peter Wang (王信友) wrote:
>
> On Wed, 2026-06-10 at 00:15 -0700, Can Guo wrote:
> > +static int ufshcd_parse_tx_eq_value_array(struct ufs_hba *hba,
> > +                                         const char *prop_name,
> > +                                         const u32 max_value,
> > +                                         u32 values[UFS_MAX_LANES *
> > 2])
> > +{
> > +       u32 num_elems = 2 * hba->lanes_per_direction;
> > +       struct device *dev = hba->dev;
> > +       int count, err, i;
> > +
> > +       count = of_property_count_u32_elems(dev->of_node, prop_name);
> > +       if (count <= 0)
> > +               return count ? count : -ENOENT;
> > 
>
> Hi Can,
>
> Returning -ENOENT when count == 0 is not correct.
> count == 0 means "empty property," whereas -ENOENT means "property
> not present."
> I suggest only checking for < 0 as follows:
> if (count < 0)
>      return count;
>
> and letting the count == 0 case be handled below:
> if (count != num_elems) {
>      ...
>      return -EINVAL;
> }
Thanks for your review and suggestions, I will update accordingly in 
next version.

Best Regards,
Can Guo.
>
> Thanks.
> Peter
>
>
> > +
> > +       if (count != num_elems) {
> > +               dev_err(dev, "Property %s has invalid count (%d),
> > expecting %u\n",
> > +                       prop_name, count, num_elems);
> > +               return -EINVAL;
> > +       }
>
>
> ************* 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!


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-06-11 14:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20260610071516.3763916-1-can.guo@oss.qualcomm.com>
2026-06-10  7:15 ` [PATCH v7 1/2] dt-bindings: ufs: Document static TX Equalization settings properties Can Guo
2026-06-11 13:32   ` Krzysztof Kozlowski
2026-06-11 14:14     ` Can Guo
2026-06-10  7:15 ` [PATCH v7 2/2] scsi: ufs: core: Add support for static TX Equalization settings Can Guo
2026-06-10 20:08   ` Bart Van Assche
2026-06-11  1:26     ` Can Guo
2026-06-11 10:11   ` Peter Wang (王信友)
2026-06-11 14:15     ` Can Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®