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

[Ziyue Zhang <quic_ziyuzhan@quicinc.com>]

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@oss.qualcomm.com>
>> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
>> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
>> ---
> [...]
>
>> -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