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

[Manivannan Sadhasivam]

On Thu, May 28, 2026 at 04:40:59PM +0800, Can Guo wrote:
> 
> 
> On 5/28/2026 4:16 PM, Manivannan Sadhasivam wrote:
> > On Thu, May 28, 2026 at 03:24:37PM +0800, Can Guo wrote:
> > > 
> > > On 5/28/2026 2:13 PM, Manivannan Sadhasivam wrote:
> > > > On Wed, May 27, 2026 at 07:40:55AM -0700, Can Guo wrote:
> > > > > Static TX Equalization settings and TX Precode enable indication from DT
> > > > > properties txeq-preshoot-g[1-6], txeq-deemphasis-g[1-6], and
> > > > > tx-precode-enable-g6 are board-specific baseline values. Values are
> > > > > provided as per-lane tuples:
> > > > > 
> > > > > <Host_Lane0 Device_Lane0>, [<Host_Lane1 Device_Lane1>]
> > > > > 
> > > > > Parse DT u32 properties with explicit range checks by using
> > > > > of_property_count_u32_elems()/of_property_read_u32_array().
> > > > > 
> > > > > When adaptive TX Equalization is used, these static settings are not final:
> > > > > 
> > > > > - If valid settings are retrieved from qTxEQGnSettings/wTxEQGnSettingsExt,
> > > > >     those retrieved settings override static DT settings.
> > > > > - If retrieval is not available/valid, TX EQTR runs and trained settings
> > > > >     override static DT settings.
> > > > > 
> > > > > So static DT settings are a fallback and are intended for cases where
> > > > > adaptive TX Equalization is not enabled/used. Adaptive TX Equalization
> > > > > remains the primary path when enabled.
> > > > > 
> > > > > No behavior changes for platforms that do not provide these properties.
> > > > > 
> > > > > Signed-off-by: Can Guo <can.guo@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >    drivers/ufs/core/ufs-txeq.c      |   4 +-
> > > > >    drivers/ufs/host/ufshcd-pltfrm.c | 128 +++++++++++++++++++++++++++++++
> > > > >    include/ufs/ufshcd.h             |   2 +
> > > > >    3 files changed, 133 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/ufs/core/ufs-txeq.c b/drivers/ufs/core/ufs-txeq.c
> > > > > index 4b264adfdf49..634ec039e129 100644
> > > > > --- a/drivers/ufs/core/ufs-txeq.c
> > > > > +++ b/drivers/ufs/core/ufs-txeq.c
> > > > > @@ -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) {
> > > > >    		int ret;
> > > > >    		ret = ufshcd_tx_eqtr(hba, params, pwr_mode);
> > > > > @@ -1310,6 +1310,7 @@ int ufshcd_config_tx_eq_settings(struct ufs_hba *hba,
> > > > >    		/* Mark TX Equalization settings as valid */
> > > > >    		params->is_valid = true;
> > > > >    		params->is_trained = true;
> > > > > +		params->is_static = false;
> > > > >    		params->is_applied = false;
> > > > >    	}
> > > > > @@ -1495,6 +1496,7 @@ static void ufshcd_extract_tx_eq_settings_attrs(struct ufs_hba *hba, u8 gear)
> > > > >    	}
> > > > >    	params->is_valid = true;
> > > > > +	params->is_static = false;
> > > > Maybe it's me, but I'm not able to understand how you want to apply these static
> > > > EQ settings. In commit message you said, the static values should be used as a
> > > > fallback, but you just check for 'params->is_static' while triggering
> > > > ufshcd_tx_eqtr() which is supposed to perform adaptive TX EQ training. IMO, you
> > > > don't need any check at all for applying static setting. If '(!params->is_valid
> > > > || force_tx_eqtr)' condition is not satisfied, then the static setting should be
> > > > used.
> > > Thanks for the review.
> > > 
> > > The distinction is between two different sources that can pre-populate
> > > txeq_params with
> > > is_valid set to true before ufshcd_config_tx_eq_settings() is called:
> > > 
> > > 1. DT properties — parsed by ufshcd_pltfrm_parse_tx_eq_settings(),
> > >      sets is_valid = true, is_static = true.
> > > 2. UFS Attributes (qTxEQGnSettings/wTxEQGnSettingsExt) — retrieved by
> > >      ufshcd_retrieve_tx_eq_settings() (introduced in the 2nd series),
> > >      sets is_valid = true, is_static = false.
> > > 
> > > Since both sources set is_valid = true, the is_valid flag alone cannot tell
> > > them apart.
> > > The is_static flag is the discriminator:
> > > 
> > > - is_valid && is_static -> settings came from DT; they are a board-level
> > > baseline.
> > >    TX EQTR should still run to find optimal settings, which will then
> > > overwrite the static ones.
> > > - is_valid && !is_static -> settings came from UFS Attributes; they are
> > > previously trained
> > You use '&&' here, but '||' in the code. When you use '||', then I see no point
> > for 'is_static' check.
> The code is correct. My reply was explaining why the check is there, but not
> explaining the check itself.
> 
> Original check in the code is (!params->is_valid || force_tx_eqtr).
> 
> Static TX EQ settings are valid, so '!params->is_valid' is false, TX EQTR
> would be skipped.
> 
> Update the check as (!params->is_valid || params->is_static ||
> force_tx_eqtr) so TX EQTR
> must run when static settings are provided.
> 

Ok. I still see having 'is_static' is a bit of overkill, but I don't see a
sensible way to check whether the static EQ settings are available or not.

But please add a comment above the check to make it clear.

- Mani

-- 
மணிவண்ணன் சதாசிவம்