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

[Can Guo <can.guo@oss.qualcomm.com>]

On 3/17/2026 3:22 PM, Can Guo wrote:
> 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.
The main data struct which is costing memory is the TX EQTR record 
arrays, I can optimize
in next very by dynamically allocating memory ONLY for the Gears which 
actually
need TX EQTR.

Thanks,
Can Guo.
>>
>>
>> > +#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!
>