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

[Can Guo]

Hi Peter,

On 3/17/2026 2:49 PM, Peter Wang (王信友) wrote:
> 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?
Here is the consideration:

1. Scanning all 64 PreShoot/DeEmphasis combinations cost (much) more time
  a. This could impact bootup KPI
  b. During TX EQTR, IOs are paused, when one conducts a re-training, 
the IOs could
       be paused for too long.
2. As per our study in the past few months, the optimal/best combination 
is most
    likely within the 8 presets, which is true for both Host TX lanes 
and Device TX lanes.
3. Even if sometime the optimal settings which fall out of the 8 
presets, they are very
    close to optimal one found within the 8 presets.

So, scanning the 8 presets only is more cost-efficient.
> > +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?
It can be valid if we are here (again) because one conducts a re-training.
> > 
> > +                       /* 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?
Sure.
> > +       ret = ufshcd_vops_tx_eqtr_notify(hba, POST_CHANGE, pwr_mode);
> > +       if (ret)
> > +               goto out;
> > +
> > +out:
> > 
>
> The if check can be removed.
Good catch.
> > + * @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.
I will move to 'is_updated'.
> > +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?
They are used by debugfs entries to print out the TX EQTR history.
> > +
> > +       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.
1. Even we use dynamic memory, it is still the same amount of memory.
2. G1/G2/G3 supports also TX Equalization settings, my next series will 
provide
    changes to allow one to give TX Equalization settings from DTS and/or
    persistent memory.
3. G4/G5 supports TX Equalization as well as TX Equalization training as per
    spec, we should support them like G6 equally to enable better link 
quality.
> > +#define PA_PEERRXHSG6ADAPTINITIALL0L3          0x15DF
> > +#define PA_PEERRXHSG6ADAPTREFRESHL0L1L2L3      0x15DE
> > 
>
> These two lines should be swapped to match the correct order.
Will do.

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!