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

Ziyue Zhang <quic_ziyuzhan@xxxxxxxxxxx> · Mon, 30 Jun 2025 15:19:12 +0800

On 6/25/2025 11:58 PM, Manivannan Sadhasivam wrote:
On Wed, Jun 25, 2025 at 04:57:59PM +0800, Ziyue Zhang wrote:
Add lane equalization setting for 8.0 GT/s to enhance link stability and
aviod AER Correctable Errors reported on some platforms (eg. SA8775P).

8.0 GT/s and 16.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 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>
---
  drivers/pci/controller/dwc/pcie-designware.h  |  1 -
  drivers/pci/controller/dwc/pcie-qcom-common.c | 55 +++++++++++--------
  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, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ce9e18554e42..388306991467 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -127,7 +127,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..ed466496f077 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, max_speed = PCIE_SPEED_16_0GT;
+	struct device *dev = pci->dev;
  
  	/*
  	 * GEN3_RELATED_OFF register is repurposed to apply equalization
@@ -19,32 +21,37 @@ 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);
+	if (pcie_link_speed[pci->max_link_speed] < PCIE_SPEED_32_0GT)
+		max_speed = pcie_link_speed[pci->max_link_speed];
So the logic here is that you want to limit the max_speed to < 32 GT/s because
you are not sure if 32 GT/s or more would require the same settings?

If so, why can't you just simply bail out early if the link speed > 16 GT/s and
just use pci->max_link_speed directly? Right now, 32 GT/s or more would be
skipped implicitly because you have initialized max_speed to PCIE_SPEED_16_0GT.

- Mani

Hi Mani

I'll update the code according to your feedback in my next submission.

BRs

Ziyue

  
-	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);
+	}
  }
-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 bf7c6ac0f3e3..aaf060bf39d4 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)
  		qcom_pcie_common_set_16gt_lane_margining(pci);
-	}
  
  	/*
  	 * The physical address of the MMIO region which is exposed as the BAR
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f85655..0fcb17ffd2e9 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -298,10 +298,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
  {
  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
  
-	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)
  		qcom_pcie_common_set_16gt_lane_margining(pci);
-	}
  
  	/* Enable Link Training state machine */
  	if (pcie->cfg->ops->ltssm_enable)
--
2.34.1








