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

[Peter Wang (王信友)]

On Sun, 2026-03-08 at 08:14 -0700, Can Guo wrote:
> +static bool use_txeq_presets = true;

Hi Can,

The default should scan all, not only presets.
Or, how could make sure the best FOM is in the presets?

> +ufshcd_tx_eqtr_result_examine(struct ufshcd_tx_eq_params
> *old_params,
> +                             struct ufshcd_tx_eq_params *new_params)
> +{
> +       int lane;
> +
> +       if (!old_params->is_valid)
> +               return;

Is is_valid always false, causing a return here?


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

Can deemphasis and preshoot be printed as well?


> +       ret = ufshcd_vops_tx_eqtr_notify(hba, POST_CHANGE, pwr_mode);
> +       if (ret)
> +               goto out;
> +
> +out:
> 

The if check can be removed.



> + * @is_new: Flag to indicate whether re-newed since previous
> iteration

is_new is confusing to me. Please consider using "need_renew" or 
"update_required", which are clearer.


> +struct ufshcd_tx_eq_params {
> +       u32 tx_lanes;
> +       u32 rx_lanes;
> +
> +       struct ufshcd_tx_eq_settings host[PA_MAXDATALANES];
> +       struct ufshcd_tx_eq_settings device[PA_MAXDATALANES];
> +
> +       u32
> host_eqtr_record[PA_MAXDATALANES][TX_HS_NUM_PRESHOOT][TX_HS_NUM_DEEMP
> HASIS];
> +       u32
> device_eqtr_record[PA_MAXDATALANES][TX_HS_NUM_PRESHOOT][TX_HS_NUM_DEE
> MPHASIS];
> 

Do these two records only store the FOM and are not used otherwise?

> +
> +       ktime_t last_eqtr_ts;
> +       int num_eqtr_records;
> +
> +       u32 saved_adapt_eqtr;
> +
> +       bool is_valid;
> +       bool is_applied;
> +};

The size of the struct ufshcd_tx_eq_params is 2.2K.
It seems that some fields could use u8 instead of u32.


> +       struct ufshcd_tx_eq_params tx_eq_params[UFS_HS_GEAR_MAX - 1];

This uses up to 12KB of memory. Is it really necessary to occupy
so much memory? Can we use dynamic memory allocation instead?
Especially since G1/G2/G3 are not used, and G4/G5 are optional.
Only G6 is actually needed, so we shouldn't waste so much memory.
After all, memory is expensive nowadays.


> +#define PA_PEERRXHSG6ADAPTINITIALL0L3          0x15DF
> +#define PA_PEERRXHSG6ADAPTREFRESHL0L1L2L3      0x15DE
> 

These two lines should be swapped to match the correct order.

Thanks
Peter