Re: [PATCH 2/2] scsi: ufs: core: Add support to retrieve and store TX Equalization settings

Can Guo
2026-04-20T13:24:05Z

On 4/20/2026 8:33 PM, Peter Wang (王信友) wrote:
>
> On Sun, 2026-04-19 at 06:52 -0700, Can Guo wrote:
> > Add support for UFS v5.0 JEDEC attributes qTxEQGnSettings and
> > wTxEQGnSettingsExt to enable persistent storage and retrieval of
> > optimal TX Equalization settings.
> >
> > This provides a fast-path for TX Equalization by reusing previously
> > stored optimal settings, avoiding TX Equalization Training (EQTR)
> > procedures during subsequent Power Mode changes.
> >
> > When no valid TX Equalization settings are found, fall back to full
> > TX
> > EQTR procedures and optionally save the results for future use.
> >
> > The validity of one set of TX Equalization settings is indicated by
> > Bit[15] in wTxEQGnSettingsExt.
> >
> > Signed-off-by: Can Guo <can.guo@oss.qualcomm.com>
> > ---
> >  drivers/ufs/core/ufs-txeq.c    | 241
> > +++++++++++++++++++++++++++++++++
> >  drivers/ufs/core/ufshcd-priv.h |   2 +
> >  drivers/ufs/core/ufshcd.c      |   5 +
> >  include/ufs/ufs.h              |   2 +
> >  include/ufs/ufshcd.h           |   2 +
> >  5 files changed, 252 insertions(+)
> >
> > diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-
> > txeq.c
> > index b2dc89124353..aa9420b0d1c3 100644
> > --- a/drivers/ufs/core/ufs-txeq.c
> > +++ b/drivers/ufs/core/ufs-txeq.c
> > @@ -14,6 +14,83 @@
> >  #include <ufs/unipro.h>
> >  #include "ufshcd-priv.h"
> >
> > +#define TX_EQ_SETTING_MASK             0x7
> > +#define TX_EQ_SETTINGS_VALID_BIT       BIT(15)
> > +
> > +/*
> > + * Decode Device TX Equalization settings based on qTxEQGnSettings
> > bit assignment:
> > + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> > + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> > + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> > + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_DEVICE_PRESHOOT_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT)) &
> > TX_EQ_SETTING_MASK)
> > +#define TX_EQ_DEVICE_DEEMPHASIS_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 16)) &
> > TX_EQ_SETTING_MASK)
> > +/*
> > + * Decode Host TX Equalization settings based on qTxEQGnSettings bit
> > assignment:
> > + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> > + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> > + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> > + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_HOST_PRESHOOT_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_PRESHOOT_SHIFT + 32)) &
> > TX_EQ_SETTING_MASK)
> > +#define TX_EQ_HOST_DEEMPHASIS_DECODE(eq, lane) \
> > +       (((eq) >> ((lane) * TX_HS_DEEMPHASIS_SHIFT + 48)) &
> > TX_EQ_SETTING_MASK)
> > +
> > +/*
> > + * Decode Device TX precode_en indication based on
> > dTxEQGnSettingsExt bit assignment:
> > + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> > + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> > + */
> > +#define TX_EQ_DEVICE_PRECODE_EN_DECODE(eq_ext, lane) \
> > +       (!!((eq_ext) & (1 << (lane))))
> > +/*
> > + * Decode Host TX precode_en indication based on dTxEQGnSettingsExt
> > bit assignment:
> > + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> > + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> > + */
> > +#define TX_EQ_HOST_PRECODE_EN_DECODE(eq_ext, lane) \
> > +       (!!((eq_ext) & (1 << ((lane) + 4))))
> > +
> > +/*
> > + * Encode qTxEQGnSettings based on bit assignment:
> > + * bit[3:0]: Device TX Logical LANE 0 PreShoot
> > + * bit[7:4]: Device TX Logical LANE 1 PreShoot
> > + * bit[19:16]: Device TX Logical LANE 0 DeEmphasis
> > + * bit[23:20]: Device TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_DEVICE_PRESHOOT_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_PRESHOOT_SHIFT))
> > +#define TX_EQ_DEVICE_DEEMPHASIS_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_DEEMPHASIS_SHIFT + 16))
> > +/*
> > + * Encode qTxEQGnSettings based on bit assignment:
> > + * bit[35:32]: Host TX Logical LANE 0 PreShoot
> > + * bit[39:36]: Host TX Logical LANE 1 PreShoot
> > + * bit[51:48]: Host TX Logical LANE 0 DeEmphasis
> > + * bit[55:52]: Host TX Logical LANE 1 DeEmphasis
> > + */
> > +#define TX_EQ_HOST_PRESHOOT_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_PRESHOOT_SHIFT + 32))
> > +#define TX_EQ_HOST_DEEMPHASIS_ENCODE(val, lane) \
> > +       (((val) & TX_EQ_SETTING_MASK) << ((lane) *
> > TX_HS_DEEMPHASIS_SHIFT + 48))
> > +
> > +/*
> > + * Encode dTxEQGnSettingsExt based on bit assignment:
> > + * bit[0]: PreCodeEn for Device TX Logical LANE 0
> > + * bit[1]: PreCodeEn for Device TX Logical LANE 1
> > + */
> > +#define TX_EQ_DEVICE_PRECODE_EN_ENCODE(val, lane)      ((val) <<
> > (lane))
> > +/*
> > + * Encode dTxEQGnSettingsExt based on bit assignment:
> > + * bit[4]: PreCodeEn for Device RX Logical LANE 0
> > + * bit[5]: PreCodeEn for Device RX Logical LANE 1
> > + */
> > +#define TX_EQ_HOST_PRECODE_EN_ENCODE(val, lane)
> > ((val) << ((lane) + 4))
> > +
> >
>
> Hi Can,
>
> Could you remove redundant parentheses, such as
> (val), (lane), (eq_ext), and (eq)?
>
>
> > +static unsigned int txeq_setting_sel;
> > +module_param_cb(txeq_setting_sel, &txeq_setting_sel_ops,
> > &txeq_setting_sel, 0644);
> > +MODULE_PARM_DESC(txeq_setting_sel, "The qTxEQGnSettings and
> > dTxEQGnSettingsExt Attributes selector used to retrieve and store TX
> > Equalization settings");
>
> Why is this selection necessary? Shouldn't we follow
> the JEDEC specification? As Bart said, introducing new
> kernel module parameters is easy, but removing them is hard.
Hi Peter,

I proposed the two Attributes to JEDEC spec with two Selectors supported
initially
to provide flexibility for use cases where we want to keep two different
settings for
the same HS-Gear, for example:

1. 0 for Room temperature, 1 for high temperature.
2. 0 for Rate-A, 1 for Rate-B.
3. 0 for Linux, 1 for SBL.

The usage of the two Selectors is not limited to above examples. Yet it
is hard to get
aligned on how to use the two Selectors across different companies.
Hence I am adding
a module parameter.

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!