Re: [PATCH v5 1/3] PCI: qcom: Add equalization settings for 8.0 GT/s and 32.0 GT/s

[Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>]

On 8/22/2025 4:06 PM, Manivannan Sadhasivam wrote:
> On Tue, Aug 19, 2025 at 03:16:46PM GMT, Ziyue Zhang wrote:
> > Add lane equalization setting for 8.0 GT/s and 32.0 GT/s to enhance link
> > stability and avoid AER Correctable Errors reported on some platforms
> > (eg. SA8775P).
>
> So this is fixing an issue, right? Then you should add relevant Fixes tag. I
> guess the tag here would be the commit that added SA8775p.
>
> > 8.0 GT/s, 16.0 GT/s and 32.0 GT/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 speeds by writing 00b,
> > 01b and 10b to `RATE_SHADOW_SEL`.
> >
> > Hence program equalization registers in a loop using link speed as index,
> > so that equalization setting can be programmed for 8.0 GT/s, 16.0 GT/s
> > and 32.0 GT/s.
> >
> > Co-developed-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ziyue Zhang <ziyue.zhang@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-designware.h  |  1 -
> >   drivers/pci/controller/dwc/pcie-qcom-common.c | 58 +++++++++++--------
> >   drivers/pci/controller/dwc/pcie-qcom-common.h |  2 +-
> >   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  6 +-
> >   drivers/pci/controller/dwc/pcie-qcom.c        |  6 +-
> >   5 files changed, 41 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index b5e7e18138a6..11de844428e5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -123,7 +123,6 @@
> >   #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
> >   #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> >   #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> > -#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_16_0GT	0x1
> >   
> >   #define GEN3_EQ_CONTROL_OFF			0x8A8
> >   #define GEN3_EQ_CONTROL_OFF_FB_MODE		GENMASK(3, 0)
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > index 3aad19b56da8..cb98e66d81d9 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > @@ -8,9 +8,11 @@
> >   #include "pcie-designware.h"
> >   #include "pcie-qcom-common.h"
> >   
> > -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;
> > +	struct device *dev = pci->dev;
>
> Reverse Xmas order please.
>
> >   
> >   	/*
> >   	 * GEN3_RELATED_OFF register is repurposed to apply equalization
> > @@ -19,32 +21,40 @@ void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci)
> >   	 * determines the data rate for which these equalization settings are
> >   	 * applied.
> >   	 */
> > -	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);
> >   
> > -	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 <= pcie_link_speed[pci->max_link_speed]; ++speed) {
> > +		if (speed > PCIE_SPEED_32_0GT) {
> > +			dev_warn(dev, "Skipped equalization settings for speeds higher than 32.0 GT/s\n");
> > +			break;
> > +		}
> >   
> > -	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_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_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);
> > +	}
> >   }
> > -EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_equalization);
> > +EXPORT_SYMBOL_GPL(qcom_pcie_common_set_equalization);
> >   
> >   void qcom_pcie_common_set_16gt_lane_margining(struct dw_pcie *pci)
> >   {
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > index 7d88d29e4766..7f5ca2fd9a72 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-common.h
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > @@ -8,7 +8,7 @@
> >   
> >   struct dw_pcie;
> >   
> > -void qcom_pcie_common_set_16gt_equalization(struct dw_pcie *pci);
> > +void qcom_pcie_common_set_equalization(struct dw_pcie *pci);
> >   void qcom_pcie_common_set_16gt_lane_margining(struct dw_pcie *pci);
> >   
> >   #endif
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index 60afb4d0134c..aeb166f68d55 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -511,10 +511,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >   		goto err_disable_resources;
> >   	}
> >   
> > -	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT) {
> > -		qcom_pcie_common_set_16gt_equalization(pci);
> > +	qcom_pcie_common_set_equalization(pci);
> > +
> > +	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
>
> This condition has existed even before this patch, but just noticing this
> possible issue. So if 'max_link_speed' is > 16 GT/s, we do not need to set lane
> margining? We used the same logic to set equalization setting earlier also.

Lane margining is supported for 16 GT/s and 32 GT/s. The settings are
dependent on phy design. For a specific phy, they have same settings
for 16 GT/s and 32 GT/s. Perhaps we can get the settings from devicetree
and program them in a loop.

But I'm not sure why we need to program it. It will no affect singal
quality and only required when user wants to collect margining info.

- Qiang Yu

> - Mani