Re: [PATCH 2/2] scsi: ufs: core: Add support for static TX Equalization settings

[Can Guo]

Hi Bean,

On 5/14/2026 9:57 PM, Bean Huo wrote:
> Can,
>
>
> Sorry for the late review of this patch. I have several questions:
Not at all sir.
> On Fri, 2026-05-01 at 06:44 -0700, Can Guo wrote:
> > @@ -1297,7 +1297,7 @@ int ufshcd_config_tx_eq_settings(struct ufs_hba *hba,
> >          }
> >   
> >          params = &hba->tx_eq_params[gear - 1];
> > -       if (!params->is_valid || force_tx_eqtr) {
> > +       if (!params->is_valid || params->is_static || force_tx_eqtr) {
> When use_adaptive_txeq is on and params->is_static is true, EQTR will overwrite
> the static values. That is reasonable since EQTR is more accurate. Is the
> is_static check really needed here?
The check of flag is_static is required because we need it to 
differentiate the static
(DTS) settings from the settings retrieved from persistent storage:

- If settings are retrieved from persistent storage, TX EQTR should be 
skipped.
- If settings are from DTS, TX EQTR should execute anyways to override 
static settings.
> >                  int ret;
> >   
> ...
>
> >   
> >   void ufshcd_retrieve_tx_eq_settings(struct ufs_hba *hba)
> > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-
> > pltfrm.c
> > index c2dafb583cf5..de6302e8c067 100644
> > --- a/drivers/ufs/host/ufshcd-pltfrm.c
> > +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> > @@ -210,6 +210,86 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba
> > *hba)
> >          }
> >   }
> >   
> > +static void ufshcd_parse_static_tx_eq_settings(struct ufs_hba *hba)
> > +{
> > +       size_t sz = hba->lanes_per_direction * 2 * TX_EQ_SETTINGS_TUPLE_SZ;
> > +       u32 settings[UFS_MAX_LANES * 2 * TX_EQ_SETTINGS_TUPLE_SZ];
> > +       u32 *host_settings, *device_settings;
> > +       u32 lpd = hba->lanes_per_direction;
> > +       struct ufshcd_tx_eq_params *params;
> ....
> > +
> > +               params = &hba->tx_eq_params[gear - 1];
> > +               host_settings = settings;
> > +               device_settings = settings + lpd * TX_EQ_SETTINGS_TUPLE_SZ;
> > +
> > +               for (lane = 0; lane < lpd; lane++) {
> > +                       params->host[lane].preshoot = host_settings[0];
> > +                       params->host[lane].deemphasis = host_settings[1];
> > +                       params->host[lane].precode_en = host_settings[2];
> > +                       host_settings += TX_EQ_SETTINGS_TUPLE_SZ;
> > +
> > +                       params->device[lane].preshoot = device_settings[0];
> > +                       params->device[lane].deemphasis = device_settings[1];
> > +                       params->device[lane].precode_en = device_settings[2];
> > +                       device_settings += TX_EQ_SETTINGS_TUPLE_SZ;
> > +               }
> > +
> > +               params->is_valid = true;
> > +               params->is_static = true;
> I want to confirm I understand the code correctly. Please tell me if I am wrong:
>
> 1, When use_adaptive_txeq = 0: static values are used directly as TX EQ for HS-
> G4 to G6. But ufshcd_config_tx_eq_settings() returns early when
> use_adaptive_txeq = 0. So which function applies the static values in this
> case?
ufshcd_post_device_init()->
    ufshcd_tune_unipro_params()->
        ufshcd_apply_valid_tx_eq_settings()
> 2, when use_adaptive_txeq = 1: static host values are used as the fixed host TX
> EQ during EQTR. This is because ufs_qcom_get_rx_fom() only sweeps the device
> side. It reads host values from hba->tx_eq_params[gear-1]->host[]. The static
> values also work as the per-lane fallback in ufshcd_update_tx_eq_params() when
> FOM is 0.
Correct.
> > +       }
> > +}
> > +
> >   /**
> >    * ufshcd_parse_clock_min_max_freq  - Parse MIN and MAX clocks freq
> >    * @hba: per adapter instance
> > @@ -528,6 +608,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
> >   
> >          ufshcd_init_lanes_per_dir(hba);
> >   
> > +       ufshcd_parse_static_tx_eq_settings(hba);
> > +
> >          err = ufshcd_parse_operating_points(hba);
> >          if (err) {
> >                  dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
> > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> > index f48d6416e299..2d385d42fcff 100644
> > --- a/include/ufs/ufshcd.h
> > +++ b/include/ufs/ufshcd.h
> > @@ -359,6 +359,7 @@ struct ufshcd_tx_eqtr_record {
> >    * @is_valid: True if parameter contains valid TX Equalization settings
> >    * @is_applied: True if settings have been applied to UniPro of both sides
> >    * @is_trained: True if parameters obtained from TX EQTR procedure
> > + * @is_static: True if settings are static
> >    */
> >   struct ufshcd_tx_eq_params {
> >          struct ufshcd_tx_eq_settings host[UFS_MAX_LANES];
> > @@ -367,8 +368,12 @@ struct ufshcd_tx_eq_params {
> >          bool is_valid;
> >          bool is_applied;
> >          bool is_trained;
> > +       bool is_static;
> is_static is added next to is_trained, which was added in your "Add persistent
> TX Equalization settings support" series, That series still has Brian's open
> question about wTxEQGnSettingsExt Bit[15] being RFU per JESD220H:
>
> https://patchwork.kernel.org/project/linux-scsi/cover/20260424151420.111675-1-can.guo@xxxxxxxxxxxxxxx
>
> Is this the reason why "Add persistent..." has not been merged?
>
> I'd prefer to wait until that discussion concludes before tagging this one. I
> hope this is ok for you.
The persistent patch series has been merged by Martin, we just need to 
wait for Martin
to push the branch, then I will upload Patch V2 to address the comment 
from Conor.

Thanks,
Can Guo.
> Kind regards,
> Bean