[PATCH v3 0/3] Add 16GT/s equalization and margining settings

[PATCH v3 0/3] Add 16GT/s equalization and margining settings

[Shashank Babu Chinta Venkata]

Add 16GT/s specific equalization and rx lane margining settings. These
settings are inline with respective PHY settings for 16GT/s 
operation. 

In addition, current QCOM EP and RC drivers do not share common
codebase which would result in code duplication. Hence, adding
common files for code reusability among RC and EP drivers.

v2 -> v3:
- Replaced FIELD_GET/FIELD_PREP macros for bit operations.
- Renamed cmn to common.
- Avoided unnecessary argument validations.
- Addressed review comments from Konrad and Mani.

v1 -> v2:
- Capitilized commit message to be inline with history 
- Dropped stubs from header file.
- Moved Designware specific register offsets and masks to
  pcie-designware.h header file.
- Applied settings based on bus data rate rather than link generation.
- Addressed review comments from Bjorn and Frank.

Shashank Babu Chinta Venkata (3):
  PCI: qcom: Refactor common code
  PCI: qcom: Add equalization settings for 16GT/s
  PCI: qcom: Add rx margining settings for 16GT/s

 drivers/pci/controller/dwc/Kconfig            |   5 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-designware.h  |  30 ++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 129 ++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  14 ++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  44 ++----
 drivers/pci/controller/dwc/pcie-qcom.c        |  72 ++--------
 7 files changed, 201 insertions(+), 94 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.c
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.h

-- 
2.43.2

[PATCH v3 1/3] PCI: qcom: Refactor common code

[Shashank Babu Chinta Venkata]

Refactor common code from RC(Root Complex) and EP(End Point)
drivers and move them to a common driver. This acts as placeholder
for common source code for both drivers, thus avoiding duplication.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
---
 drivers/pci/controller/dwc/Kconfig            |  5 ++
 drivers/pci/controller/dwc/Makefile           |  1 +
 drivers/pci/controller/dwc/pcie-qcom-common.c | 75 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h | 12 +++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     | 39 +---------
 drivers/pci/controller/dwc/pcie-qcom.c        | 67 ++---------------
 6 files changed, 105 insertions(+), 94 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.c
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.h

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 8afacc90c63b..1599550cd628 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -265,12 +265,16 @@ config PCIE_DW_PLAT_EP
 	  order to enable device-specific features PCI_DW_PLAT_EP must be
 	  selected.
 
+config PCIE_QCOM_COMMON
+	bool
+
 config PCIE_QCOM
 	bool "Qualcomm PCIe controller (host mode)"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
 	depends on PCI_MSI
 	select PCIE_DW_HOST
 	select CRC8
+	select PCIE_QCOM_COMMON
 	help
 	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
 	  PCIe controller uses the DesignWare core plus Qualcomm-specific
@@ -281,6 +285,7 @@ config PCIE_QCOM_EP
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
 	depends on PCI_ENDPOINT
 	select PCIE_DW_EP
+	select PCIE_QCOM_COMMON
 	help
 	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
 	  to work in endpoint mode. The PCIe controller uses the DesignWare core
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index bac103faa523..3f557dd60c38 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
+obj-$(CONFIG_PCIE_QCOM_COMMON) += pcie-qcom-common.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
new file mode 100644
index 000000000000..dc2120ec5fef
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, 2021 Linaro Limited.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/interconnect.h>
+
+#include "../../pci.h"
+#include "pcie-designware.h"
+#include "pcie-qcom-common.h"
+
+#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
+		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
+
+int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p)
+{
+	*icc_mem_p = devm_of_icc_get(pci->dev, "pcie-mem");
+	if (IS_ERR_OR_NULL(icc_mem_p))
+		return PTR_ERR(icc_mem_p);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
+
+int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	int ret;
+
+	/*
+	 * Some Qualcomm platforms require interconnect bandwidth constraints
+	 * to be set before enabling interconnect clocks.
+	 *
+	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
+	 * for the pcie-mem path.
+	 */
+	ret = icc_set_bw(icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_init);
+
+void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	u32 offset, status;
+	int speed, width;
+	int ret;
+
+	if (!icc_mem)
+		return;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	/* Only update constraints if link is up. */
+	if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		return;
+
+	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
+	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
+
+	ret = icc_set_bw(icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+	if (ret)
+		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+			ret);
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_update);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
new file mode 100644
index 000000000000..f0520d7301da
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, 2021 Linaro Limited.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "pcie-designware.h"
+
+int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p);
+int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
+void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 36e5e80cd22f..11c99b358147 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -25,6 +25,7 @@
 
 #include "../../pci.h"
 #include "pcie-designware.h"
+#include "pcie-qcom-common.h"
 
 /* PARF registers */
 #define PARF_SYS_CTRL				0x00
@@ -137,9 +138,6 @@
 #define CORE_RESET_TIME_US_MAX			1005
 #define WAKE_DELAY_US				2000 /* 2 ms */
 
-#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
-		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
-
 #define to_pcie_ep(x)				dev_get_drvdata((x)->dev)
 
 enum qcom_pcie_ep_link_status {
@@ -278,28 +276,6 @@ static void qcom_pcie_dw_write_dbi2(struct dw_pcie *pci, void __iomem *base,
 	writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE);
 }
 
-static void qcom_pcie_ep_icc_update(struct qcom_pcie_ep *pcie_ep)
-{
-	struct dw_pcie *pci = &pcie_ep->pci;
-	u32 offset, status;
-	int speed, width;
-	int ret;
-
-	if (!pcie_ep->icc_mem)
-		return;
-
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
-
-	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
-	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
-
-	ret = icc_set_bw(pcie_ep->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
-	if (ret)
-		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
-			ret);
-}
-
 static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
 {
 	struct dw_pcie *pci = &pcie_ep->pci;
@@ -325,14 +301,7 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
 	if (ret)
 		goto err_phy_exit;
 
-	/*
-	 * Some Qualcomm platforms require interconnect bandwidth constraints
-	 * to be set before enabling interconnect clocks.
-	 *
-	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
-	 * for the pcie-mem path.
-	 */
-	ret = icc_set_bw(pcie_ep->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
+	ret = qcom_pcie_common_icc_init(pci, pcie_ep->icc_mem);
 	if (ret) {
 		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
 			ret);
@@ -616,7 +585,7 @@ static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
 	if (IS_ERR(pcie_ep->phy))
 		ret = PTR_ERR(pcie_ep->phy);
 
-	pcie_ep->icc_mem = devm_of_icc_get(dev, "pcie-mem");
+	ret = qcom_pcie_common_icc_get_resource(&pcie_ep->pci, &pcie_ep->icc_mem);
 	if (IS_ERR(pcie_ep->icc_mem))
 		ret = PTR_ERR(pcie_ep->icc_mem);
 
@@ -643,7 +612,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data)
 	} else if (FIELD_GET(PARF_INT_ALL_BME, status)) {
 		dev_dbg(dev, "Received BME event. Link is enabled!\n");
 		pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
-		qcom_pcie_ep_icc_update(pcie_ep);
+		qcom_pcie_common_icc_update(pci, pcie_ep->icc_mem);
 		pci_epc_bme_notify(pci->ep.epc);
 	} else if (FIELD_GET(PARF_INT_ALL_PM_TURNOFF, status)) {
 		dev_dbg(dev, "Received PM Turn-off event! Entering L23\n");
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 2ce2a3bd932b..71f011daad1d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -32,6 +32,7 @@
 #include <linux/types.h>
 
 #include "../../pci.h"
+#include "pcie-qcom-common.h"
 #include "pcie-designware.h"
 
 /* PARF registers */
@@ -147,9 +148,6 @@
 
 #define QCOM_PCIE_CRC8_POLYNOMIAL		(BIT(2) | BIT(1) | BIT(0))
 
-#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
-		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
-
 #define QCOM_PCIE_1_0_0_MAX_CLOCKS		4
 struct qcom_pcie_resources_1_0_0 {
 	struct clk_bulk_data clks[QCOM_PCIE_1_0_0_MAX_CLOCKS];
@@ -1363,59 +1361,6 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = qcom_pcie_start_link,
 };
 
-static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
-{
-	struct dw_pcie *pci = pcie->pci;
-	int ret;
-
-	pcie->icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
-	if (IS_ERR(pcie->icc_mem))
-		return PTR_ERR(pcie->icc_mem);
-
-	/*
-	 * Some Qualcomm platforms require interconnect bandwidth constraints
-	 * to be set before enabling interconnect clocks.
-	 *
-	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
-	 * for the pcie-mem path.
-	 */
-	ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
-	if (ret) {
-		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
-			ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
-{
-	struct dw_pcie *pci = pcie->pci;
-	u32 offset, status;
-	int speed, width;
-	int ret;
-
-	if (!pcie->icc_mem)
-		return;
-
-	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
-
-	/* Only update constraints if link is up. */
-	if (!(status & PCI_EXP_LNKSTA_DLLLA))
-		return;
-
-	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
-	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
-
-	ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
-	if (ret) {
-		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
-			ret);
-	}
-}
-
 static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
 {
 	struct qcom_pcie *pcie = (struct qcom_pcie *)dev_get_drvdata(s->private);
@@ -1524,7 +1469,11 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
-	ret = qcom_pcie_icc_init(pcie);
+	ret = qcom_pcie_common_icc_get_resource(pcie->pci, &pcie->icc_mem);
+	if (ret)
+		goto err_pm_runtime_put;
+
+	ret = qcom_pcie_common_icc_init(pcie->pci, pcie->icc_mem);
 	if (ret)
 		goto err_pm_runtime_put;
 
@@ -1546,7 +1495,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_phy_exit;
 	}
 
-	qcom_pcie_icc_update(pcie);
+	qcom_pcie_common_icc_update(pcie->pci, pcie->icc_mem);
 
 	if (pcie->mhi)
 		qcom_pcie_init_debugfs(pcie);
@@ -1613,7 +1562,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
 		pcie->suspended = false;
 	}
 
-	qcom_pcie_icc_update(pcie);
+	qcom_pcie_common_icc_update(pcie->pci, pcie->icc_mem);
 
 	return 0;
 }
-- 
2.43.2

[PATCH v3 2/3] PCI: qcom: Add equalization settings for 16GT/s

[Shashank Babu Chinta Venkata]

GEN3_RELATED_OFFSET is being used to determine data rate of shadow
registers. Select data rate as 16GT/s and set appropriate equilization
settings to improve link stability for 16GT/s data rate.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 30 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  3 ++
 drivers/pci/controller/dwc/pcie-qcom.c        |  3 ++
 5 files changed, 49 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 26dae4837462..ad771bb52d29 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -122,6 +122,18 @@
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
 
+#define GEN3_EQ_CONTROL_OFF			0x8a8
+#define GEN3_EQ_CONTROL_OFF_FB_MODE(n)		FIELD_PREP(GENMASK(3, 0), n)
+#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE(n)	FIELD_PREP(BIT(4), n)
+#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC(n)	FIELD_PREP(GENMASK(23, 8), n)
+#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL(n)	FIELD_PREP(BIT(24), n)
+
+#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
+#define GEN3_EQ_FMDC_T_MIN_PHASE23(n)	FIELD_PREP(GENMASK(4, 0), n)
+#define GEN3_EQ_FMDC_N_EVALS(n)		FIELD_PREP(GENMASK(9, 5), n)
+#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA(n)	FIELD_PREP(GENMASK(13, 10), n)
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA(n)	FIELD_PREP(GENMASK(17, 14), n)
+
 #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
 #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
 
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
index dc2120ec5fef..a6f3eb4c3ee6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -16,6 +16,36 @@
 #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
 		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
 
+void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
+{
+	u32 reg;
+
+	/*
+	 * GEN3_RELATED_OFF is repurposed to be used with GEN4(16GT/s) rate
+	 * as well based on RATE_SHADOW_SEL_MASK settings on this register.
+	 */
+	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 |= (0x1 << GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT);
+	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(0) |
+		GEN3_EQ_FMDC_N_EVALS(0xD) |
+		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA(0x5) |
+		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(0) |
+		GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE(0) |
+		GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL(0) |
+		GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC(0);
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
+
 int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p)
 {
 	*icc_mem_p = devm_of_icc_get(pci->dev, "pcie-mem");
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
index f0520d7301da..e72c651b0d28 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -10,3 +10,4 @@
 int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p);
 int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
+void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 11c99b358147..cb75a874f76c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -438,6 +438,9 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
+	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
+		qcom_pcie_common_set_16gt_eq_settings(pci);
+
 	/*
 	 * The physical address of the MMIO region which is exposed as the BAR
 	 * should be written to MHI BASE registers.
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 71f011daad1d..acf66f974edc 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -263,6 +263,9 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
+	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
+		qcom_pcie_common_set_16gt_eq_settings(pci);
+
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
 		pcie->cfg->ops->ltssm_enable(pcie);
-- 
2.43.2

[PATCH v3 3/3] PCI: qcom: Add rx margining settings for 16GT/s

[Shashank Babu Chinta Venkata]

Add rx lane margining settings for 16GT/s(GEN 4) data rate. These
settings improve link stability while operating at high date rates
and helps to improve signal quality.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware.h  | 18 ++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  4 +++-
 drivers/pci/controller/dwc/pcie-qcom.c        |  4 +++-
 5 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ad771bb52d29..e8c48855143f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -203,6 +203,24 @@
 
 #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
 
+/*
+ * GEN4 lane margining register definitions
+ */
+#define GEN4_LANE_MARGINING_1_OFF		0xb80
+#define MARGINING_MAX_VOLTAGE_OFFSET(n)		FIELD_PREP(GENMASK(29, 24), n)
+#define MARGINING_NUM_VOLTAGE_STEPS(n)		FIELD_PREP(GENMASK(22, 16), n)
+#define MARGINING_MAX_TIMING_OFFSET(n)		FIELD_PREP(GENMASK(13, 8), n)
+#define MARGINING_NUM_TIMING_STEPS(n)		FIELD_PREP(GENMASK(5, 0), n)
+
+#define GEN4_LANE_MARGINING_2_OFF		0xb84
+#define MARGINING_IND_ERROR_SAMPLER(n)		FIELD_PREP(BIT(28), n)
+#define MARGINING_SAMPLE_REPORTING_METHOD(n)	FIELD_PREP(BIT(27), n)
+#define MARGINING_IND_LEFT_RIGHT_TIMING(n)	FIELD_PREP(BIT(26), n)
+#define MARGINING_IND_UP_DOWN_VOLTAGE(n)	FIELD_PREP(BIT(25), n)
+#define MARGINING_VOLTAGE_SUPPORTED(n)		FIELD_PREP(BIT(24), n)
+#define MARGINING_MAXLANES(n)			FIELD_PREP(GENMASK(20, 16), n)
+#define MARGINING_SAMPLE_RATE_TIMING(n)		FIELD_PREP(GENMASK(13, 8), n)
+#define MARGINING_SAMPLE_RATE_VOLTAGE(n)	FIELD_PREP(GENMASK(5, 0), n)
 /*
  * iATU Unroll-specific register definitions
  * From 4.80 core version the address translation will be made by unroll
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
index a6f3eb4c3ee6..3279314ae78c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
 }
 EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
 
+void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
+{
+	u32 reg;
+
+	reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF);
+	reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) |
+		MARGINING_NUM_VOLTAGE_STEPS(0x78) |
+		MARGINING_MAX_TIMING_OFFSET(0x32) |
+		MARGINING_NUM_TIMING_STEPS(0x10);
+	dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg);
+
+	reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_2_OFF);
+	reg = MARGINING_IND_ERROR_SAMPLER(1) |
+		MARGINING_SAMPLE_REPORTING_METHOD(1) |
+		MARGINING_IND_LEFT_RIGHT_TIMING(1) |
+		MARGINING_VOLTAGE_SUPPORTED(1) |
+		MARGINING_IND_UP_DOWN_VOLTAGE(0) |
+		MARGINING_MAXLANES(pci->num_lanes) |
+		MARGINING_SAMPLE_RATE_TIMING(0x3f) |
+		MARGINING_SAMPLE_RATE_VOLTAGE(0x3f);
+	dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_2_OFF, reg);
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_rx_margining_settings);
+
 int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p)
 {
 	*icc_mem_p = devm_of_icc_get(pci->dev, "pcie-mem");
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
index e72c651b0d28..b9eb78fcc766 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -11,3 +11,4 @@ int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc
 int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
+void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index cb75a874f76c..3032dd91514c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -438,8 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
-	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
+	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT) {
 		qcom_pcie_common_set_16gt_eq_settings(pci);
+		qcom_pcie_common_set_16gt_rx_margining_settings(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 acf66f974edc..f69364ecf2de 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -263,8 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
-	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
+	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT) {
 		qcom_pcie_common_set_16gt_eq_settings(pci);
+		qcom_pcie_common_set_16gt_rx_margining_settings(pci);
+	}
 
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
-- 
2.43.2

Re: [PATCH v3 0/3] Add 16GT/s equalization and margining settings

[Niklas Cassel]

On Thu, Apr 18, 2024 at 05:09:33PM -0700, Shashank Babu Chinta Venkata wrote:
> Add 16GT/s specific equalization and rx lane margining settings. These
> settings are inline with respective PHY settings for 16GT/s 
> operation. 
> 
> In addition, current QCOM EP and RC drivers do not share common
> codebase which would result in code duplication. Hence, adding
> common files for code reusability among RC and EP drivers.
> 
> v2 -> v3:
> - Replaced FIELD_GET/FIELD_PREP macros for bit operations.
> - Renamed cmn to common.
> - Avoided unnecessary argument validations.
> - Addressed review comments from Konrad and Mani.
> 
> v1 -> v2:
> - Capitilized commit message to be inline with history 
> - Dropped stubs from header file.
> - Moved Designware specific register offsets and masks to
>   pcie-designware.h header file.
> - Applied settings based on bus data rate rather than link generation.
> - Addressed review comments from Bjorn and Frank.
> 
> Shashank Babu Chinta Venkata (3):
>   PCI: qcom: Refactor common code
>   PCI: qcom: Add equalization settings for 16GT/s
>   PCI: qcom: Add rx margining settings for 16GT/s
> 
>  drivers/pci/controller/dwc/Kconfig            |   5 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-designware.h  |  30 ++++
>  drivers/pci/controller/dwc/pcie-qcom-common.c | 129 ++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.h |  14 ++
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  44 ++----
>  drivers/pci/controller/dwc/pcie-qcom.c        |  72 ++--------
>  7 files changed, 201 insertions(+), 94 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.h
> 
> -- 
> 2.43.2
> 

FWIW: I think the subject of the cover-letter should have been prefixed with:
"PCI: qcom: "


Kind regards,
Niklas

Re: [PATCH v3 1/3] PCI: qcom: Refactor common code

[Manivannan Sadhasivam]

On Thu, Apr 18, 2024 at 05:09:34PM -0700, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)
> drivers and move them to a common driver. This acts as placeholder
> for common source code for both drivers, thus avoiding duplication.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |  5 ++
>  drivers/pci/controller/dwc/Makefile           |  1 +
>  drivers/pci/controller/dwc/pcie-qcom-common.c | 75 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.h | 12 +++
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     | 39 +---------
>  drivers/pci/controller/dwc/pcie-qcom.c        | 67 ++---------------
>  6 files changed, 105 insertions(+), 94 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.h
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 8afacc90c63b..1599550cd628 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -265,12 +265,16 @@ config PCIE_DW_PLAT_EP
>  	  order to enable device-specific features PCI_DW_PLAT_EP must be
>  	  selected.
>  
> +config PCIE_QCOM_COMMON
> +	bool
> +
>  config PCIE_QCOM
>  	bool "Qualcomm PCIe controller (host mode)"
>  	depends on OF && (ARCH_QCOM || COMPILE_TEST)
>  	depends on PCI_MSI
>  	select PCIE_DW_HOST
>  	select CRC8
> +	select PCIE_QCOM_COMMON
>  	help
>  	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
>  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> @@ -281,6 +285,7 @@ config PCIE_QCOM_EP
>  	depends on OF && (ARCH_QCOM || COMPILE_TEST)
>  	depends on PCI_ENDPOINT
>  	select PCIE_DW_EP
> +	select PCIE_QCOM_COMMON
>  	help
>  	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
>  	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index bac103faa523..3f557dd60c38 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> +obj-$(CONFIG_PCIE_QCOM_COMMON) += pcie-qcom-common.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> new file mode 100644
> index 000000000000..dc2120ec5fef
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/interconnect.h>

Sort these alphabetically.

> +
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +#include "pcie-qcom-common.h"
> +
> +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> +		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
> +
> +int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p)

This API can be used for other paths also in the future (like CPU-PCIe). So it
should accept the path name and directly return the 'struct icc_path' pointer.

> +{
> +	*icc_mem_p = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR_OR_NULL(icc_mem_p))
> +		return PTR_ERR(icc_mem_p);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
> +
> +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	int ret;
> +
> +	/*
> +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> +	 * to be set before enabling interconnect clocks.
> +	 *
> +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> +	 * for the pcie-mem path.
> +	 */
> +	ret = icc_set_bw(icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_init);
> +
> +void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	u32 offset, status;
> +	int speed, width;
> +	int ret;
> +
> +	if (!icc_mem)
> +		return;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +
> +	/* Only update constraints if link is up. */
> +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +		return;
> +
> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
> +
> +	ret = icc_set_bw(icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> +	if (ret)
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",

'Failed to set bandwidth for PCIe-MEM interconnect path: %d\n'

- Mani

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

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

[Manivannan Sadhasivam]

On Thu, Apr 18, 2024 at 05:09:35PM -0700, Shashank Babu Chinta Venkata wrote:

PCI: qcom: Add PCIe link equalization settings for 16 GT/s

> GEN3_RELATED_OFFSET is being used to determine data rate of shadow
> registers. Select data rate as 16GT/s and set appropriate equilization
> settings to improve link stability for 16GT/s data rate.
> 

How about:

'To improve the PCIe link stability while running at the rate of 16 GT/s, set
the appropriate link equalization settings for both RC and EP controllers.'

However, I don't understand the statement about 'GEN3_RELATED_OFFSET' and
'shadow registers'. Please reword it to make it understandable.

> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.c | 30 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  3 ++
>  drivers/pci/controller/dwc/pcie-qcom.c        |  3 ++
>  5 files changed, 49 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 26dae4837462..ad771bb52d29 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -122,6 +122,18 @@
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
>  
> +#define GEN3_EQ_CONTROL_OFF			0x8a8
> +#define GEN3_EQ_CONTROL_OFF_FB_MODE(n)		FIELD_PREP(GENMASK(3, 0), n)
> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE(n)	FIELD_PREP(BIT(4), n)
> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC(n)	FIELD_PREP(GENMASK(23, 8), n)
> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL(n)	FIELD_PREP(BIT(24), n)
> +
> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> +#define GEN3_EQ_FMDC_T_MIN_PHASE23(n)	FIELD_PREP(GENMASK(4, 0), n)
> +#define GEN3_EQ_FMDC_N_EVALS(n)		FIELD_PREP(GENMASK(9, 5), n)
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA(n)	FIELD_PREP(GENMASK(13, 10), n)
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA(n)	FIELD_PREP(GENMASK(17, 14), n)
> +
>  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
>  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
>  
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> index dc2120ec5fef..a6f3eb4c3ee6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -16,6 +16,36 @@
>  #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
>  		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
>  
> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	/*
> +	 * GEN3_RELATED_OFF is repurposed to be used with GEN4(16GT/s) rate
> +	 * as well based on RATE_SHADOW_SEL_MASK settings on this register.

Same comment as above.

> +	 */
> +	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 |= (0x1 << GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT);

Use FIELD_PREP()

> +	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(0) |

You are reading 'reg' above and then overwriting immediately.

> +		GEN3_EQ_FMDC_N_EVALS(0xD) |

'0xd'

> +		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA(0x5) |
> +		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(0) |

Same as above.

- Mani

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

Re: [PATCH v3 3/3] PCI: qcom: Add rx margining settings for 16GT/s

[Manivannan Sadhasivam]

On Thu, Apr 18, 2024 at 05:09:36PM -0700, Shashank Babu Chinta Venkata wrote:

PCI: qcom: Add RX lane margining settings for 16 GT/s data rate

> Add rx lane margining settings for 16GT/s(GEN 4) data rate. These

RX

16 GT/s

> settings improve link stability while operating at high date rates
> and helps to improve signal quality.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.h  | 18 ++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  4 +++-
>  drivers/pci/controller/dwc/pcie-qcom.c        |  4 +++-
>  5 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ad771bb52d29..e8c48855143f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -203,6 +203,24 @@
>  
>  #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
>  
> +/*
> + * GEN4 lane margining register definitions

16 GT/s (GEN 4)

> + */
> +#define GEN4_LANE_MARGINING_1_OFF		0xb80
> +#define MARGINING_MAX_VOLTAGE_OFFSET(n)		FIELD_PREP(GENMASK(29, 24), n)
> +#define MARGINING_NUM_VOLTAGE_STEPS(n)		FIELD_PREP(GENMASK(22, 16), n)
> +#define MARGINING_MAX_TIMING_OFFSET(n)		FIELD_PREP(GENMASK(13, 8), n)
> +#define MARGINING_NUM_TIMING_STEPS(n)		FIELD_PREP(GENMASK(5, 0), n)
> +
> +#define GEN4_LANE_MARGINING_2_OFF		0xb84
> +#define MARGINING_IND_ERROR_SAMPLER(n)		FIELD_PREP(BIT(28), n)
> +#define MARGINING_SAMPLE_REPORTING_METHOD(n)	FIELD_PREP(BIT(27), n)
> +#define MARGINING_IND_LEFT_RIGHT_TIMING(n)	FIELD_PREP(BIT(26), n)
> +#define MARGINING_IND_UP_DOWN_VOLTAGE(n)	FIELD_PREP(BIT(25), n)
> +#define MARGINING_VOLTAGE_SUPPORTED(n)		FIELD_PREP(BIT(24), n)
> +#define MARGINING_MAXLANES(n)			FIELD_PREP(GENMASK(20, 16), n)
> +#define MARGINING_SAMPLE_RATE_TIMING(n)		FIELD_PREP(GENMASK(13, 8), n)
> +#define MARGINING_SAMPLE_RATE_VOLTAGE(n)	FIELD_PREP(GENMASK(5, 0), n)
>  /*
>   * iATU Unroll-specific register definitions
>   * From 4.80 core version the address translation will be made by unroll
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> index a6f3eb4c3ee6..3279314ae78c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>  }
>  EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>  
> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF);
> +	reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) |

Same comment as previous patch. Are you doing it intentionally for some reason?

- Mani

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

Re: [PATCH v3 3/3] PCI: qcom: Add rx margining settings for 16GT/s

[Konrad Dybcio]

On 4/19/24 02:09, Shashank Babu Chinta Venkata wrote:
> Add rx lane margining settings for 16GT/s(GEN 4) data rate. These
> settings improve link stability while operating at high date rates
> and helps to improve signal quality.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware.h  | 18 ++++++++++++++
>   drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++
>   drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
>   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  4 +++-
>   drivers/pci/controller/dwc/pcie-qcom.c        |  4 +++-
>   5 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ad771bb52d29..e8c48855143f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -203,6 +203,24 @@
>   
>   #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
>   
> +/*
> + * GEN4 lane margining register definitions
> + */
> +#define GEN4_LANE_MARGINING_1_OFF		0xb80
> +#define MARGINING_MAX_VOLTAGE_OFFSET(n)		FIELD_PREP(GENMASK(29, 24), n)
> +#define MARGINING_NUM_VOLTAGE_STEPS(n)		FIELD_PREP(GENMASK(22, 16), n)
> +#define MARGINING_MAX_TIMING_OFFSET(n)		FIELD_PREP(GENMASK(13, 8), n)
> +#define MARGINING_NUM_TIMING_STEPS(n)		FIELD_PREP(GENMASK(5, 0), n)
> +
> +#define GEN4_LANE_MARGINING_2_OFF		0xb84
> +#define MARGINING_IND_ERROR_SAMPLER(n)		FIELD_PREP(BIT(28), n)
> +#define MARGINING_SAMPLE_REPORTING_METHOD(n)	FIELD_PREP(BIT(27), n)
> +#define MARGINING_IND_LEFT_RIGHT_TIMING(n)	FIELD_PREP(BIT(26), n)
> +#define MARGINING_IND_UP_DOWN_VOLTAGE(n)	FIELD_PREP(BIT(25), n)
> +#define MARGINING_VOLTAGE_SUPPORTED(n)		FIELD_PREP(BIT(24), n)
> +#define MARGINING_MAXLANES(n)			FIELD_PREP(GENMASK(20, 16), n)
> +#define MARGINING_SAMPLE_RATE_TIMING(n)		FIELD_PREP(GENMASK(13, 8), n)
> +#define MARGINING_SAMPLE_RATE_VOLTAGE(n)	FIELD_PREP(GENMASK(5, 0), n)

That's a.. rather unusual.. use of FIELD_/GENMASK.. Usually, the fields are
defined with GENMASK and then referenced through FIELD_xyz(BITFIELD_NAME, val)

That said, I'm not entirely against this if Mani is ok with it

>   /*
>    * iATU Unroll-specific register definitions
>    * From 4.80 core version the address translation will be made by unroll
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> index a6f3eb4c3ee6..3279314ae78c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>   }
>   EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>   
> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF);
> +	reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) |
> +		MARGINING_NUM_VOLTAGE_STEPS(0x78) |
> +		MARGINING_MAX_TIMING_OFFSET(0x32) |
> +		MARGINING_NUM_TIMING_STEPS(0x10);
> +	dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg);

Since this is DW-common, why is this inside the qcom driver?

Konrad

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

[Konrad Dybcio]

On 4/19/24 02:09, Shashank Babu Chinta Venkata wrote:
> GEN3_RELATED_OFFSET is being used to determine data rate of shadow
> registers. Select data rate as 16GT/s and set appropriate equilization
> settings to improve link stability for 16GT/s data rate.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---

[...]

> +	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
> +	reg = GEN3_EQ_FMDC_T_MIN_PHASE23(0) |
> +		GEN3_EQ_FMDC_N_EVALS(0xD) |
> +		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA(0x5) |
> +		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(0) |
> +		GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE(0) |
> +		GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL(0) |
> +		GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC(0);
> +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);

Also, any chance we could get some explanations as to what these magic values mean?

Preferably in the form of a #define for each one

Konrad

Re: [PATCH v3 3/3] PCI: qcom: Add rx margining settings for 16GT/s

[Shashank Babu Chinta Venkata]

On 4/22/24 15:58, Konrad Dybcio wrote:
> 
> 
> On 4/19/24 02:09, Shashank Babu Chinta Venkata wrote:
>> Add rx lane margining settings for 16GT/s(GEN 4) data rate. These
>> settings improve link stability while operating at high date rates
>> and helps to improve signal quality.
>>
>> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware.h  | 18 ++++++++++++++
>>   drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++
>>   drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
>>   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  4 +++-
>>   drivers/pci/controller/dwc/pcie-qcom.c        |  4 +++-
>>   5 files changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index ad771bb52d29..e8c48855143f 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -203,6 +203,24 @@
>>     #define PCIE_PL_CHK_REG_ERR_ADDR            0xB28
>>   +/*
>> + * GEN4 lane margining register definitions
>> + */
>> +#define GEN4_LANE_MARGINING_1_OFF        0xb80
>> +#define MARGINING_MAX_VOLTAGE_OFFSET(n)        FIELD_PREP(GENMASK(29, 24), n)
>> +#define MARGINING_NUM_VOLTAGE_STEPS(n)        FIELD_PREP(GENMASK(22, 16), n)
>> +#define MARGINING_MAX_TIMING_OFFSET(n)        FIELD_PREP(GENMASK(13, 8), n)
>> +#define MARGINING_NUM_TIMING_STEPS(n)        FIELD_PREP(GENMASK(5, 0), n)
>> +
>> +#define GEN4_LANE_MARGINING_2_OFF        0xb84
>> +#define MARGINING_IND_ERROR_SAMPLER(n)        FIELD_PREP(BIT(28), n)
>> +#define MARGINING_SAMPLE_REPORTING_METHOD(n)    FIELD_PREP(BIT(27), n)
>> +#define MARGINING_IND_LEFT_RIGHT_TIMING(n)    FIELD_PREP(BIT(26), n)
>> +#define MARGINING_IND_UP_DOWN_VOLTAGE(n)    FIELD_PREP(BIT(25), n)
>> +#define MARGINING_VOLTAGE_SUPPORTED(n)        FIELD_PREP(BIT(24), n)
>> +#define MARGINING_MAXLANES(n)            FIELD_PREP(GENMASK(20, 16), n)
>> +#define MARGINING_SAMPLE_RATE_TIMING(n)        FIELD_PREP(GENMASK(13, 8), n)
>> +#define MARGINING_SAMPLE_RATE_VOLTAGE(n)    FIELD_PREP(GENMASK(5, 0), n)
> 
> That's a.. rather unusual.. use of FIELD_/GENMASK.. Usually, the fields are
> defined with GENMASK and then referenced through FIELD_xyz(BITFIELD_NAME, val)
> 
> That said, I'm not entirely against this if Mani is ok with it
will fall back to conventional approach in my next series to avoid confusion.
> 
>>   /*
>>    * iATU Unroll-specific register definitions
>>    * From 4.80 core version the address translation will be made by unroll
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
>> index a6f3eb4c3ee6..3279314ae78c 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>> @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>>   +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
>> +{
>> +    u32 reg;
>> +
>> +    reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF);
>> +    reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) |
>> +        MARGINING_NUM_VOLTAGE_STEPS(0x78) |
>> +        MARGINING_MAX_TIMING_OFFSET(0x32) |
>> +        MARGINING_NUM_TIMING_STEPS(0x10);
>> +    dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg);
> 
> Since this is DW-common, why is this inside the qcom driver?
Though this register space is in dw-common specific, these settings are purely vendor specific . These settings are determined by systems team on vendor hardware, as these settings are used as margin to compensate signal variance due to various physical factors(like connection length, retimers etc).
> 
> Konrad

Re: [PATCH v3 3/3] PCI: qcom: Add rx margining settings for 16GT/s

[Konrad Dybcio]

[...]

>>>   EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>>>   +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
>>> +{
>>> +    u32 reg;
>>> +
>>> +    reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF);
>>> +    reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) |
>>> +        MARGINING_NUM_VOLTAGE_STEPS(0x78) |
>>> +        MARGINING_MAX_TIMING_OFFSET(0x32) |
>>> +        MARGINING_NUM_TIMING_STEPS(0x10);
>>> +    dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg);
>>
>> Since this is DW-common, why is this inside the qcom driver?
> Though this register space is in dw-common specific, these settings are purely vendor specific . These settings are determined by systems team on vendor hardware, as these settings are used as margin to compensate signal variance due to various physical factors(like connection length, retimers etc).

Okay, so:

1. is the register layout vendor-specific too? i.e. are the bitfields DW-common?

2. will these settings work on all Qualcomm devices, regardless of SoC/board/
retimers used etc.?

Konrad