Re: [PATCH 04/11] scsi: ufs: core: Add support for TX Equalization
From: Can Guo
Date: Sun Mar 01 2026 - 22:47:07 EST
Hi Bart,
On 2/28/2026 5:39 AM, Bart Van Assche wrote:
The three module parameters are necessary -
On 2/27/26 8:08 AM, Can Guo wrote:
+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 power mode change (default: false)");
+
+static int txeq_gear_set(const char *val, const struct kernel_param *kp)
+{
+ return param_set_uint_minmax(val, kp, UFS_HS_G1, UFS_HS_G6);
+}
+
+static const struct kernel_param_ops txeq_gear_ops = {
+ .set = txeq_gear_set,
+ .get = param_get_uint,
+};
+
+static unsigned int adaptive_txeq_gear = UFS_HS_G6;
+module_param_cb(adaptive_txeq_gear, &txeq_gear_ops, &adaptive_txeq_gear, 0644);
+MODULE_PARM_DESC(adaptive_txeq_gear, "For the HS-Gear[n] and above, adaptive txeq shall be used");
+
+static bool use_txeq_presets = true;
+module_param(use_txeq_presets, bool, 0644);
+MODULE_PARM_DESC(use_txeq_presets, "Use only the 8 TX Equalization Presets (pre-defined Pre-Shoot & De-Emphasis combinations) for TX EQTR (default: true)");
+
+static bool txeq_presets_selected[UFS_TX_EQ_PRESET_MAX] = {[0 ... (UFS_TX_EQ_PRESET_MAX - 1)] = 1};
+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");
Please minimize the kernel module parameters. Introducing new kernel module parameters is easy but removing these is hard. Can all the above
parameters be removed? If not, please only keep what is absolutely
necessary.
1. use_adaptive_txeq parameter is needed as an overall disable/enable
switch.
2. use_txeq_presets parameter is needed because a full TX EQTR procedure
with all 64 (8 x 8) PreShoot&DeEmphasis combinations give more
accurate TX EQTR result, but it takes long time. use_txeq_presets
parameter selects only the 8 combinations (or presets defined by M-PHY
spec) as a tradeoff.
3. txeq_presets_selected parameter provides a finer resolution of the 8
presets to further reduce TX EQTR loops. Qualcomm platforms would use
this module parameter.
Will do.
+/* A HS-G6 capable M-TX shall support the presets. */
+static const struct __ufs_tx_eq_preset {
+ unsigned int preshoot;
+ unsigned int deemphasis;
+} ufs_tx_eq_preset[UFS_TX_EQ_PRESET_MAX] = {
+ [UFS_TX_EQ_PRESET_P0] = {UFS_TX_HS_PRESHOOT_DB_0P0, UFS_TX_HS_DEEMPHASIS_DB_0P0},
+ [UFS_TX_EQ_PRESET_P1] = {UFS_TX_HS_PRESHOOT_DB_0P0, UFS_TX_HS_DEEMPHASIS_DB_0P8},
+ [UFS_TX_EQ_PRESET_P2] = {UFS_TX_HS_PRESHOOT_DB_0P0, UFS_TX_HS_DEEMPHASIS_DB_1P6},
+ [UFS_TX_EQ_PRESET_P3] = {UFS_TX_HS_PRESHOOT_DB_0P8, UFS_TX_HS_DEEMPHASIS_DB_0P0},
+ [UFS_TX_EQ_PRESET_P4] = {UFS_TX_HS_PRESHOOT_DB_1P6, UFS_TX_HS_DEEMPHASIS_DB_0P0},
+ [UFS_TX_EQ_PRESET_P5] = {UFS_TX_HS_PRESHOOT_DB_0P8, UFS_TX_HS_DEEMPHASIS_DB_0P8},
+ [UFS_TX_EQ_PRESET_P6] = {UFS_TX_HS_PRESHOOT_DB_0P8, UFS_TX_HS_DEEMPHASIS_DB_1P6},
+ [UFS_TX_EQ_PRESET_P7] = {UFS_TX_HS_PRESHOOT_DB_1P6, UFS_TX_HS_DEEMPHASIS_DB_0P8},
+};
Please mention in the comment above this array from what standard the above table comes.
Sure.
+static const u32 pa_peer_rx_adapt_initial[UFS_HS_GEAR_MAX] = {
+ 0,
+ 0,
+ 0,
+ 0,
+ PA_PEERRXHSG4ADAPTINITIAL,
+ PA_PEERRXHSG5ADAPTINITIAL,
+ PA_PEERRXHSG6ADAPTINITIALL0L3
+};
+
+static const u32 rx_adapt_initial_cap[UFS_HS_GEAR_MAX] = {
+ 0,
+ 0,
+ 0,
+ 0,
+ RX_HS_G4_ADAPT_INITIAL_CAP,
+ RX_HS_G5_ADAPT_INITIAL_CAP,
+ RX_HS_G6_ADAPT_INITIAL_CAP
+};
+
+static const u32 pa_tx_eq_setting[UFS_HS_GEAR_MAX] = {
+ 0,
+ PA_TXEQG1SETTING,
+ PA_TXEQG2SETTING,
+ PA_TXEQG3SETTING,
+ PA_TXEQG4SETTING,
+ PA_TXEQG5SETTING,
+ PA_TXEQG6SETTING
+};
Same comment for the above three arrays. Please add a comment that explains what standard these arrays come from.
Sure.
+ if (!local_precodeen && !peer_precodeen) {
+ dev_dbg(hba->dev, "Pre-Coding is not required for either side\n");
+ return ret;
+ }
Here and elsewhere in this patch, please change "precodeen" into "precode_en". That will make it easier for readers to guess that
"precode_en" refers to enabling precoding.
Will remove !!.
+/**
+ * ufshcd_evaluate_fom - Update TX EQ params based on FOM results
+ * @hba: per adapter instance
+ * @params: TX EQ parameters data structure
+ * @h_iter: host TX EQTR iterator data structure
+ * @d_iter: device TX EQTR iterator data structure
+ *
+ * Evaluate FOM results, update host and device TX EQ params if FOM results are
+ * improved, and record TX EQTR results.
+ */
+static void ufshcd_evaluate_fom(struct ufs_hba *hba,
+ struct ufshcd_tx_eq_params *params,
+ struct tx_eqtr_iter *h_iter,
+ struct tx_eqtr_iter *d_iter)
+{
+ u32 preshoot, deemphasis, fom_value;
+ bool precode_en;
+ int lane;
+
+ for (lane = 0; h_iter->is_new && lane < h_iter->num_lanes; lane++) {
+ preshoot = h_iter->preshoot;
+ deemphasis = h_iter->deemphasis;
+ fom_value = h_iter->fom[lane] & RX_FOM_VALUE_MASK;
+ precode_en = !!(h_iter->fom[lane] & RX_FOM_PRECODING_EN_MASK);
!! is superfluous when assigning to a boolean (precode_en).
Thanks for the suggestion.
+static int __ufshcd_tx_eqtr(struct ufs_hba *hba,
+ struct ufshcd_tx_eq_params *params,
+ struct ufs_pa_layer_attr *pwr_mode)
+{
+ struct ufshcd_tx_eq_params *new_params;
+ struct tx_eqtr_iter h_iter, d_iter;
+ unsigned int preshoot, deemphasis;
+ u32 gear = pwr_mode->gear_tx;
+ ktime_t start;
+ int ret;
+
+ new_params = kzalloc(sizeof(struct ufshcd_tx_eq_params), GFP_KERNEL);
+ if (!new_params)
+ return -ENOMEM;
Please combine the declaration of 'new_params' with its assignment and
use __free to simplify error handling, e.g. as follows:
struct ufshcd_tx_eq_params *new_params __free(kfree) = kzalloc(sizeof(struct ufshcd_tx_eq_params), GFP_KERNEL);
Will do.
+ /* TX EQTR main loop */
+ for (preshoot = 0; preshoot < TX_HS_NUM_PRESHOOT; preshoot++)
Please surround the body of this loop with braces ({}).
Sorry, my bad.
+ for (deemphasis = 0; deemphasis < TX_HS_NUM_DEEMPHASIS; deemphasis++) {
+ if (!tx_eqtr_iter_update(preshoot, deemphasis, &h_iter, &d_iter))
+ continue;
+
+ /* Step 3 - Apply TX EQTR settings */
+ ret = ufshcd_apply_tx_eqtr_settings(hba, pwr_mode, &h_iter, &d_iter);
+ if (ret) {
+ dev_err(hba->dev, "Failed to apply TX EQTR settings: %d\n",
+ ret);
+ goto out;
+ }
+
+ /* Step 4 - Trigger TX EQTR procedure start */
+ ret = ufshcd_trigger_tx_eqtr(hba, gear);
+ if (ret) {
+ dev_err(hba->dev, "Failed to start TX EQTR procedure for target gear %u: %d\n",
+ gear, ret);
+ goto out;
+ }
+
+ /* Step 5 - Get FOM */
+ ret = ufshcd_get_rx_fom(hba, pwr_mode, &h_iter, &d_iter);
+ if (ret) {
+ dev_err(hba->dev, "Failed to get RX_FOM: %d\n",
+ ret);
+ goto out;
+ }
+
+ ufshcd_evaluate_fom(hba, new_params, &h_iter, &d_iter);
+ };
Please remove the superfluous semicolon past }.
Thanks.
+ * It ensures that EQTR starts from the most reiliable link state (HS-G1) with
reiliable -> reliable
I made ufshcd_tx_eqtr_unprepare() void because I don't want power mode
+static void ufshcd_tx_eqtr_unprepare(struct ufs_hba *hba,
+ struct ufs_pa_layer_attr *pwr_mode)
+{
+ int err;
+
+ if (pwr_mode->pwr_rx == SLOWAUTO_MODE || pwr_mode->hs_rate == 0)
+ return;
+
+ err = ufshcd_change_power_mode(hba, pwr_mode, /*force_pmc=*/false);
+ if (err)
+ dev_err(hba->dev, "%s: Failed to restore Power Mode: %d\n",
+ __func__, err);
+}
What should happen if restoring the power mode fails?
restoring (if successful) to override the actual err which leads to power
mode restoring in the first place.
ufshcd_tx_eqtr(), caller of ufshcd_tx_eqtr_unprepare(), would anyways
return the actual err to its caller regardless of power mode restoring
succeeds or not. Eventually, the ufshcd_config_pwr_mode() would return
the same err to its caller.
Currently the two paths which call into ufshcd_config_pwr_mode() are
probe() and clock scaling.
For probe(), if ufshcd_config_pwr_mode() returns any err, probe() would
fail anyways regardless of if power mode is restored to HS-G1.
For clock scaling, if ufshcd_config_pwr_mode() returns err and
ufshcd_tx_eqtr_unprepare() couldn't restore to original power mode,
the worst case is that power mode is left as HS-G1 (the most reliable gear).
Do you see a problem with this approach?
OK
+int ufshcd_config_tx_eq_settings(struct ufs_hba *hba,
+ struct ufs_pa_layer_attr *pwr_mode)
+{
+ struct ufshcd_tx_eq_params *params;
+ u32 gear, rate;
+ int ret = 0;
Please remove the local variable 'ret' and move it into the two scopes
in which it is assigned a value.
Will do.
+ if (!ufshcd_is_tx_eq_supported(hba) || !use_adaptive_txeq)
+ return ret;
To improve code readability, please change "return ret" into "return 0".
Will do.
+ } else if (gear < adaptive_txeq_gear) {
+ return ret;
+ }
To improve code readability, please change "return ret" into "return 0".
Will do.
+ params->is_applied = true;
+ }
+
+ return ret;
To improve code readability, please change "return ret" into "return 0".
OK.
/*
* PHY Adapter attributes
*/
-#define PA_PHY_TYPE 0x1500
Please refrain from making whitespace changes in existing code in a patch that is already too big.
Thanks.
/* Adpat type for PA_TXHSADAPTTYPE attribute */
Adpat -> adapt
Best Regards,
Can Guo.
Thanks,
Bart.