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

[Peter Wang (王信友)]

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@xxxxxxxxxxxxxxxx>
> ---
>  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.

Thanks
Peter