Re: [PATCH v2 1/2] PCI: qcom: Add equalization settings for 8.0 GT/s

[Ziyue Zhang]

On 6/11/2025 11:31 PM, Konrad Dybcio wrote:
> On 6/11/25 12:03 PM, Ziyue Zhang wrote:
> > Adding lane equalization setting for 8.0 GT/s to enhance link stability
> > and fix AER correctable errors reported on some platforms (eg. SA8775P).
> >
> > 8.0 GT/s and 16.0GT/s require the same equalization setting. This setting
> > is programmed into a group of shadow registers, which can be switched to
> > configure equalization for different GEN speeds by writing 00b, 01b
> > to `RATE_SHADOW_SEL`.
> >
> > Hence program equalization registers in a loop using link speed as index,
> > so that equalization setting can be programmed for both 8.0 GT/s and
> > 16.0 GT/s.
> >
> > Co-developed-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ziyue Zhang <quic_ziyuzhan@xxxxxxxxxxx>
> > ---
> [...]
>
> > -void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci)
> > +void qcom_pcie_common_set_equalization(struct dw_pcie *pci)
> >   {
> >   	u32 reg;
> > +	u16 speed, max_speed = PCIE_SPEED_16_0GT;
> > +	struct device *dev = pci->dev;
> >   
> >   	/*
> >   	 * GEN3_RELATED_OFF register is repurposed to apply equalization
> > @@ -18,33 +20,43 @@ void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci)
> >   	 * GEN3_EQ_*. The RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF
> >   	 * determines the data rate for which these equalization settings are
> >   	 * applied.
> > +	 *
> > +	 * TODO:
> > +	 * EQ settings need to be added for 32.0 T/s in future
> >   	 */
> > -	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> > -	reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> > -	reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
> > -	reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK,
> > -			  GEN3_RELATED_OFF_RATE_SHADOW_SEL_16_0GT);
> > -	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
> > +	if (pcie_link_speed[pci->max_link_speed] < PCIE_SPEED_32_0GT)
> > +		max_speed = pcie_link_speed[pci->max_link_speed];
> > +	else
> > +		dev_warn(dev, "The target supports 32.0 GT/s, but the EQ setting for 32.0 GT/s is not configured.\n");
> >   
> > -	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
> > -	reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
> > -		GEN3_EQ_FMDC_N_EVALS |
> > -		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
> > -		GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
> > -	reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
> > -		FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
> > -		FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
> > -		FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
> > -	dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
> > +	for (speed = PCIE_SPEED_8_0GT; speed <= max_speed; ++speed) {
> > +		reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> > +		reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> > +		reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
> > +		reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK,
> > +			  speed - PCIE_SPEED_8_0GT);
> > +		dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
> >   
> > -	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> > -	reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
> > -		GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
> > -		GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
> > -		GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
> > -	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> > +		reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
> > +		reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
> > +			GEN3_EQ_FMDC_N_EVALS |
> > +			GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
> > +			GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
> > +		reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
> > +			FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
> > +			FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
> > +			FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
> > +		dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
> > +
> > +		reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> > +		reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
> > +			GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
> > +			GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
> > +			GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
> > +		dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> > +	}
> this function could receive `speed` as a parameter instead, so that
> it's easier to parse
>
> Konrad

Hi Konrad,

On the current platform, the register write configurations for both
8.0 GT/s and 16.0 GT/s are identical, so we believe it's unnecessary to
pass ‘speed’ as a parameter at this stage.

However, I agree that if future platforms or speed modes introduce
configuration differences, it would make sense to revisit this and
consider adding speed as a parameter for better flexibility.

BRs
Ziyue