Re: [PATCH 04/11] scsi: ufs: core: Add support for TX Equalization

[Bart Van Assche]

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.

> +/* 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.

> +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.

> +	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.

> +/**
> + * 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).

> +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);

> +	/* TX EQTR main loop */
> +	for (preshoot = 0; preshoot < TX_HS_NUM_PRESHOOT; preshoot++)

Please surround the body of this loop with braces ({}).

> +		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 }.

> + * It ensures that EQTR starts from the most reiliable link state (HS-G1) with

reiliable -> reliable

> +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?

> +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.

> +	if (!ufshcd_is_tx_eq_supported(hba) || !use_adaptive_txeq)
> +		return ret;

To improve code readability, please change "return ret" into "return 0".

> +	} else if (gear < adaptive_txeq_gear) {
> +		return ret;
> +	}

To improve code readability, please change "return ret" into "return 0".

> +		params->is_applied = true;
> +	}
> +
> +	return ret;

To improve code readability, please change "return ret" into "return 0".

>   /*
>    * 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.

>   /* Adpat type for PA_TXHSADAPTTYPE attribute */

Adpat -> adapt

Thanks,

Bart.