[PATCH v5 0/3] pci: qcom: Add 16GT/s equalization and margining settings

[PATCH v5 0/3] pci: qcom: 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.

v4 -> v5:
- Added additional parameter bandwidth to accommodate new icc path.
- Fixed typo.
- Picked up Reviewed-by tags.

v3 -> v4:
- Addressed review comments from Mani and Konrad.
- Preceded subject line with pci: qcom: tags

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 16 GT/s
  PCI: qcom: Add RX margining settings for 16 GT/s

 MAINTAINERS                                   |   3 +
 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 | 156 ++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  17 ++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  44 +----
 drivers/pci/controller/dwc/pcie-qcom.c        | 146 ++++++----------
 8 files changed, 271 insertions(+), 131 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.c
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-common.h

-- 
2.46.0

[PATCH v5 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>
---
 MAINTAINERS                                   |   3 +
 drivers/pci/controller/dwc/Kconfig            |   5 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-qcom-common.c |  88 +++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  15 ++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  39 +----
 drivers/pci/controller/dwc/pcie-qcom.c        | 141 ++++++------------
 7 files changed, 161 insertions(+), 131 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/MAINTAINERS b/MAINTAINERS
index c0a62489be56..ef3e62677e9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2785,6 +2785,7 @@ F:	drivers/iommu/msm*
 F:	drivers/mfd/ssbi.c
 F:	drivers/mmc/host/mmci_qcom*
 F:	drivers/mmc/host/sdhci-msm.c
+F:	drivers/pci/controller/dwc/pcie-qcom-common.c
 F:	drivers/pci/controller/dwc/pcie-qcom.c
 F:	drivers/phy/qualcomm/
 F:	drivers/power/*/msm*
@@ -17844,6 +17845,7 @@ M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 L:	linux-pci@vger.kernel.org
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
+F:	drivers/pci/controller/dwc/pcie-qcom-common.c
 F:	drivers/pci/controller/dwc/pcie-qcom.c
 
 PCIE DRIVER FOR ROCKCHIP
@@ -17880,6 +17882,7 @@ L:	linux-pci@vger.kernel.org
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
+F:	drivers/pci/controller/dwc/pcie-qcom-common.c
 F:	drivers/pci/controller/dwc/pcie-qcom-ep.c
 
 PCMCIA SUBSYSTEM
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 4c38181acffa..b6d6778b0698 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 ec215b3d6191..82e51ba11031 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) += 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..1d8992147bba
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -0,0 +1,88 @@
+// 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 <linux/pm_opp.h>
+#include <linux/units.h>
+
+#include "../../pci.h"
+#include "pcie-designware.h"
+#include "pcie-qcom-common.h"
+
+struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
+{
+	struct icc_path *icc_p;
+
+	icc_p = devm_of_icc_get(pci->dev, path);
+	return icc_p;
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
+
+int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc, u32 bandwidth)
+{
+	int ret;
+
+	ret = icc_set_bw(icc, 0, bandwidth);
+	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, width, speed;
+	unsigned long freq_kbps;
+	struct dev_pm_opp *opp;
+	int ret, freq_mbps;
+
+	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);
+
+	if (icc_mem) {
+		ret = icc_set_bw(icc_mem, 0,
+				 width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+		if (ret) {
+			dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
+				ret);
+		}
+	} else {
+		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
+		if (freq_mbps < 0)
+			return;
+
+		freq_kbps = freq_mbps * KILO;
+		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
+						 true);
+		if (!IS_ERR(opp)) {
+			ret = dev_pm_opp_set_opp(pci->dev, opp);
+			if (ret)
+				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
+					freq_kbps * width, ret);
+			dev_pm_opp_put(opp);
+		}
+
+	}
+
+}
+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..897fa18e618a
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -0,0 +1,15 @@
+/* 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"
+
+#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
+		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
+
+struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
+int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
+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 236229f66c80..e1860026e134 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
@@ -142,9 +143,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 {
@@ -295,28 +293,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;
@@ -342,14 +318,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);
@@ -633,7 +602,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");
+	pcie_ep->icc_mem = qcom_pcie_common_icc_get_resource(&pcie_ep->pci, "pcie-mem");
 	if (IS_ERR(pcie_ep->icc_mem))
 		ret = PTR_ERR(pcie_ep->icc_mem);
 
@@ -660,7 +629,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 Bus Master Enable event\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_bus_master_enable_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 0180edf3310e..ee32590f1506 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -31,9 +31,9 @@
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/types.h>
-#include <linux/units.h>
 
 #include "../../pci.h"
+#include "pcie-qcom-common.h"
 #include "pcie-designware.h"
 
 /* PARF registers */
@@ -158,9 +158,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]))
-
 struct qcom_pcie_resources_1_0_0 {
 	struct clk_bulk_data *clks;
 	int num_clks;
@@ -1365,92 +1362,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);
-
-	pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
-	if (IS_ERR(pcie->icc_cpu))
-		return PTR_ERR(pcie->icc_cpu);
-	/*
-	 * 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 bandwidth for PCIe-MEM interconnect path: %d\n",
-			ret);
-		return ret;
-	}
-
-	/*
-	 * Since the CPU-PCIe path is only used for activities like register
-	 * access of the host controller and endpoint Config/BAR space access,
-	 * HW team has recommended to use a minimal bandwidth of 1KBps just to
-	 * keep the path active.
-	 */
-	ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
-	if (ret) {
-		dev_err(pci->dev, "Failed to set bandwidth for CPU-PCIe interconnect path: %d\n",
-			ret);
-		icc_set_bw(pcie->icc_mem, 0, 0);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
-{
-	u32 offset, status, width, speed;
-	struct dw_pcie *pci = pcie->pci;
-	unsigned long freq_kbps;
-	struct dev_pm_opp *opp;
-	int ret, freq_mbps;
-
-	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);
-
-	if (pcie->icc_mem) {
-		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 bandwidth for PCIe-MEM interconnect path: %d\n",
-				ret);
-		}
-	} else {
-		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
-		if (freq_mbps < 0)
-			return;
-
-		freq_kbps = freq_mbps * KILO;
-		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
-						 true);
-		if (!IS_ERR(opp)) {
-			ret = dev_pm_opp_set_opp(pci->dev, opp);
-			if (ret)
-				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
-					freq_kbps * width, ret);
-			dev_pm_opp_put(opp);
-		}
-	}
-}
-
 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);
@@ -1561,6 +1472,18 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
+	pcie->icc_mem = qcom_pcie_common_icc_get_resource(pcie->pci, "pcie-mem");
+	if (IS_ERR_OR_NULL(pcie->icc_mem)) {
+		ret = PTR_ERR(pcie->icc_mem);
+		goto err_pm_runtime_put;
+	}
+
+	pcie->icc_cpu = qcom_pcie_common_icc_get_resource(pcie->pci, "cpu-pcie");
+	if (IS_ERR_OR_NULL(pcie->icc_cpu)) {
+		ret = PTR_ERR(pcie->icc_cpu);
+		goto err_pm_runtime_put;
+	}
+
 	/* OPP table is optional */
 	ret = devm_pm_opp_of_add_table(dev);
 	if (ret && ret != -ENODEV) {
@@ -1593,10 +1516,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 			goto err_pm_runtime_put;
 		}
 	} else {
-		/* Skip ICC init if OPP is supported as it is handled by OPP */
-		ret = qcom_pcie_icc_init(pcie);
-		if (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 = qcom_pcie_common_icc_init(pcie->pci, pcie->icc_mem,
+					QCOM_PCIE_LINK_SPEED_TO_BW(1));
+		if (ret) {
+			dev_err(pci->dev,
+			"Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
+			ret);
 			goto err_pm_runtime_put;
+		}
+
+		/*
+		 * Since the CPU-PCIe path is only used for activities
+		 * like register access of the host controller and
+		 * endpoint Config/BAR space access, HW team has
+		 * recommended to use a minimal bandwidth of 1KBps just to
+		 * keep the path active.
+		 */
+		ret = qcom_pcie_common_icc_init(pcie->pci, pcie->icc_cpu, kBps_to_icc(1));
+		if (ret) {
+			dev_err(pci->dev,
+			"Failed to set bandwidth for CPU-PCIe interconnect path: %d\n",
+			ret);
+			goto err_pm_runtime_put;
+		}
 	}
 
 	ret = pcie->cfg->ops->get_resources(pcie);
@@ -1617,7 +1565,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_phy_exit;
 	}
 
-	qcom_pcie_icc_opp_update(pcie);
+	qcom_pcie_common_icc_update(pcie->pci, pcie->icc_mem);
 
 	if (pcie->mhi)
 		qcom_pcie_init_debugfs(pcie);
@@ -1681,7 +1629,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
 		ret = icc_disable(pcie->icc_cpu);
 		if (ret)
-			dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n", ret);
+			dev_err(dev,
+			"Failed to disable CPU-PCIe interconnect path: %d\n", ret);
 
 		if (!pcie->icc_mem)
 			dev_pm_opp_set_opp(pcie->pci->dev, NULL);
@@ -1710,7 +1659,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
 		pcie->suspended = false;
 	}
 
-	qcom_pcie_icc_opp_update(pcie);
+	qcom_pcie_common_icc_update(pcie->pci, pcie->icc_mem);
 
 	return 0;
 }
-- 
2.46.0

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

[Bjorn Helgaas]

On Wed, Aug 21, 2024 at 10:08:42AM -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.

Much of this seems to be replacing qcom_pcie_icc_opp_update() and
qcom_pcie_ep_icc_update() with qcom_pcie_common_icc_update().

That seems worthwhile and it would be helpful if the commit log called
that out so we'd know what to look for in the patch.

I think the qcom_pcie_common_icc_init() rework would be more
understandable if it were in its own patch and not mixed in here.

> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -0,0 +1,88 @@
> +// 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.
> + *

Spurious blank line.

> + */

> +struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
> +{
> +	struct icc_path *icc_p;
> +
> +	icc_p = devm_of_icc_get(pci->dev, path);
> +	return icc_p;

  return devm_of_icc_get(pci->dev, path);

> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
> +
> +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc, u32 bandwidth)
> +{
> +	int ret;
> +
> +	ret = icc_set_bw(icc, 0, bandwidth);
> +	if (ret) {
> +		dev_err(pci->dev, "Failed to set interconnect bandwidth: %d\n",
> +			ret);
> +		return ret;
> +	}

The callers also check and log similar messages.  I don't see the
point.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_init);

These both seem of dubious value.

> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h

Do we need "-common" in the filename?  Seems like "pcie-qcom.h" would
be enough.  I *hope* we don't someday need both a "pcie-qcom.h and a
"pcie-qcom-common.h"; that seems like it would really be overkill.

Bjorn

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

[Manivannan Sadhasivam]

On Wed, Aug 21, 2024 at 12:33:18PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 21, 2024 at 10:08:42AM -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.
> 
> Much of this seems to be replacing qcom_pcie_icc_opp_update() and
> qcom_pcie_ep_icc_update() with qcom_pcie_common_icc_update().
> 
> That seems worthwhile and it would be helpful if the commit log called
> that out so we'd know what to look for in the patch.
> 
> I think the qcom_pcie_common_icc_init() rework would be more
> understandable if it were in its own patch and not mixed in here.
> 
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > @@ -0,0 +1,88 @@
> > +// 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.
> > + *
> 
> Spurious blank line.
> 
> > + */
> 
> > +struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
> > +{
> > +	struct icc_path *icc_p;
> > +
> > +	icc_p = devm_of_icc_get(pci->dev, path);
> > +	return icc_p;
> 
>   return devm_of_icc_get(pci->dev, path);
> 
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
> > +
> > +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc, u32 bandwidth)
> > +{
> > +	int ret;
> > +
> > +	ret = icc_set_bw(icc, 0, bandwidth);
> > +	if (ret) {
> > +		dev_err(pci->dev, "Failed to set interconnect bandwidth: %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> 
> The callers also check and log similar messages.  I don't see the
> point.
> 
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_init);
> 
> These both seem of dubious value.
> 
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> 
> Do we need "-common" in the filename?  Seems like "pcie-qcom.h" would
> be enough.  I *hope* we don't someday need both a "pcie-qcom.h and a
> "pcie-qcom-common.h"; that seems like it would really be overkill.
> 

I suggested the -common suffix since pcie-qcom is historically meant for RC
driver. So creating a common header with that name will create confusion since
we have a separate EP driver.

- Mani

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

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

[kernel test robot]

Hi Shashank,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on linus/master v6.11-rc4]
[cannot apply to pci/for-linus next-20240823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shashank-Babu-Chinta-Venkata/PCI-qcom-Refactor-common-code/20240822-011229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240821170917.21018-2-quic_schintav%40quicinc.com
patch subject: [PATCH v5 1/3] PCI: qcom: Refactor common code
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240824/202408242300.2ECtncwN-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240824/202408242300.2ECtncwN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408242300.2ECtncwN-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-qcom-ep.c:321:55: error: too few arguments to function call, expected 3, have 2
     321 |         ret = qcom_pcie_common_icc_init(pci, pcie_ep->icc_mem);
         |               ~~~~~~~~~~~~~~~~~~~~~~~~~                      ^
   drivers/pci/controller/dwc/pcie-qcom-common.h:14:5: note: 'qcom_pcie_common_icc_init' declared here
      14 | int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
         |     ^                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +321 drivers/pci/controller/dwc/pcie-qcom-ep.c

   295	
   296	static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
   297	{
   298		struct dw_pcie *pci = &pcie_ep->pci;
   299		int ret;
   300	
   301		ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
   302		if (ret)
   303			return ret;
   304	
   305		ret = qcom_pcie_ep_core_reset(pcie_ep);
   306		if (ret)
   307			goto err_disable_clk;
   308	
   309		ret = phy_init(pcie_ep->phy);
   310		if (ret)
   311			goto err_disable_clk;
   312	
   313		ret = phy_set_mode_ext(pcie_ep->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_EP);
   314		if (ret)
   315			goto err_phy_exit;
   316	
   317		ret = phy_power_on(pcie_ep->phy);
   318		if (ret)
   319			goto err_phy_exit;
   320	
 > 321		ret = qcom_pcie_common_icc_init(pci, pcie_ep->icc_mem);
   322		if (ret) {
   323			dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
   324				ret);
   325			goto err_phy_off;
   326		}
   327	
   328		return 0;
   329	
   330	err_phy_off:
   331		phy_power_off(pcie_ep->phy);
   332	err_phy_exit:
   333		phy_exit(pcie_ep->phy);
   334	err_disable_clk:
   335		clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
   336	
   337		return ret;
   338	}
   339	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Shashank,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on linus/master v6.11-rc4]
[cannot apply to pci/for-linus next-20240823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shashank-Babu-Chinta-Venkata/PCI-qcom-Refactor-common-code/20240822-011229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240821170917.21018-2-quic_schintav%40quicinc.com
patch subject: [PATCH v5 1/3] PCI: qcom: Refactor common code
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240824/202408242341.lywV11DL-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240824/202408242341.lywV11DL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408242341.lywV11DL-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/dwc/pcie-qcom-ep.c: In function 'qcom_pcie_enable_resources':
>> drivers/pci/controller/dwc/pcie-qcom-ep.c:321:15: error: too few arguments to function 'qcom_pcie_common_icc_init'
     321 |         ret = qcom_pcie_common_icc_init(pci, pcie_ep->icc_mem);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/pci/controller/dwc/pcie-qcom-ep.c:28:
   drivers/pci/controller/dwc/pcie-qcom-common.h:14:5: note: declared here
      14 | int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/qcom_pcie_common_icc_init +321 drivers/pci/controller/dwc/pcie-qcom-ep.c

   295	
   296	static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
   297	{
   298		struct dw_pcie *pci = &pcie_ep->pci;
   299		int ret;
   300	
   301		ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
   302		if (ret)
   303			return ret;
   304	
   305		ret = qcom_pcie_ep_core_reset(pcie_ep);
   306		if (ret)
   307			goto err_disable_clk;
   308	
   309		ret = phy_init(pcie_ep->phy);
   310		if (ret)
   311			goto err_disable_clk;
   312	
   313		ret = phy_set_mode_ext(pcie_ep->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_EP);
   314		if (ret)
   315			goto err_phy_exit;
   316	
   317		ret = phy_power_on(pcie_ep->phy);
   318		if (ret)
   319			goto err_phy_exit;
   320	
 > 321		ret = qcom_pcie_common_icc_init(pci, pcie_ep->icc_mem);
   322		if (ret) {
   323			dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
   324				ret);
   325			goto err_phy_off;
   326		}
   327	
   328		return 0;
   329	
   330	err_phy_off:
   331		phy_power_off(pcie_ep->phy);
   332	err_phy_exit:
   333		phy_exit(pcie_ep->phy);
   334	err_disable_clk:
   335		clk_bulk_disable_unprepare(pcie_ep->num_clks, pcie_ep->clks);
   336	
   337		return ret;
   338	}
   339	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Manivannan Sadhasivam]

On Wed, Aug 21, 2024 at 10:08:42AM -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>
> ---
>  MAINTAINERS                                   |   3 +
>  drivers/pci/controller/dwc/Kconfig            |   5 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-qcom-common.c |  88 +++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.h |  15 ++
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  39 +----
>  drivers/pci/controller/dwc/pcie-qcom.c        | 141 ++++++------------
>  7 files changed, 161 insertions(+), 131 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/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> new file mode 100644
> index 000000000000..1d8992147bba
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -0,0 +1,88 @@
> +// 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 <linux/pm_opp.h>
> +#include <linux/units.h>
> +
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +#include "pcie-qcom-common.h"
> +
> +struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)

On top of Bjorn's review:

I think you can just use qcom_pcie_common_icc_get() as _resource suffix is not
really needed.

> +{
> +	struct icc_path *icc_p;
> +
> +	icc_p = devm_of_icc_get(pci->dev, path);
> +	return icc_p;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
> +
> +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc, u32 bandwidth)
> +{
> +	int ret;
> +
> +	ret = icc_set_bw(icc, 0, bandwidth);
> +	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, width, speed;
> +	unsigned long freq_kbps;
> +	struct dev_pm_opp *opp;
> +	int ret, freq_mbps;
> +
> +	if (!icc_mem)
> +		return;

With this check in place, below OPP code will never get executed.

> +
> +	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);
> +
> +	if (icc_mem) {
> +		ret = icc_set_bw(icc_mem, 0,
> +				 width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> +		if (ret) {
> +			dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
> +				ret);
> +		}
> +	} else {
> +		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
> +		if (freq_mbps < 0)
> +			return;
> +
> +		freq_kbps = freq_mbps * KILO;
> +		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
> +						 true);
> +		if (!IS_ERR(opp)) {
> +			ret = dev_pm_opp_set_opp(pci->dev, opp);
> +			if (ret)
> +				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
> +					freq_kbps * width, ret);
> +			dev_pm_opp_put(opp);
> +		}
> +
> +	}
> +
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_update);

[...]

>  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);
> @@ -1561,6 +1472,18 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_pm_runtime_put;
>  	}
>  
> +	pcie->icc_mem = qcom_pcie_common_icc_get_resource(pcie->pci, "pcie-mem");
> +	if (IS_ERR_OR_NULL(pcie->icc_mem)) {

Why are you checking for NULL here and below? ICC core will return NULL if the
path is not found in DT or ICC is disabled and it is important to not treat it
as an error to maintain DT backwards compatibility.

> +		ret = PTR_ERR(pcie->icc_mem);
> +		goto err_pm_runtime_put;
> +	}
> +
> +	pcie->icc_cpu = qcom_pcie_common_icc_get_resource(pcie->pci, "cpu-pcie");
> +	if (IS_ERR_OR_NULL(pcie->icc_cpu)) {
> +		ret = PTR_ERR(pcie->icc_cpu);
> +		goto err_pm_runtime_put;
> +	}
> +
>  	/* OPP table is optional */
>  	ret = devm_pm_opp_of_add_table(dev);
>  	if (ret && ret != -ENODEV) {
> @@ -1593,10 +1516,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  			goto err_pm_runtime_put;
>  		}
>  	} else {
> -		/* Skip ICC init if OPP is supported as it is handled by OPP */
> -		ret = qcom_pcie_icc_init(pcie);
> -		if (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 = qcom_pcie_common_icc_init(pcie->pci, pcie->icc_mem,
> +					QCOM_PCIE_LINK_SPEED_TO_BW(1));
> +		if (ret) {
> +			dev_err(pci->dev,
> +			"Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
> +			ret);
>  			goto err_pm_runtime_put;
> +		}
> +
> +		/*
> +		 * Since the CPU-PCIe path is only used for activities
> +		 * like register access of the host controller and
> +		 * endpoint Config/BAR space access, HW team has
> +		 * recommended to use a minimal bandwidth of 1KBps just to
> +		 * keep the path active.
> +		 */

Please wrap the comments to 80 columns. I don't know what column length you are
using here.

> +		ret = qcom_pcie_common_icc_init(pcie->pci, pcie->icc_cpu, kBps_to_icc(1));
> +		if (ret) {
> +			dev_err(pci->dev,
> +			"Failed to set bandwidth for CPU-PCIe interconnect path: %d\n",

You can make use of 100 column extension here and below.

- Mani

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

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

[Johan Hovold]

On Wed, Aug 21, 2024 at 10:08:42AM -0700, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)

Space before open parentheses, please (again).

> 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>

> 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..1d8992147bba
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -0,0 +1,88 @@
> +// 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.

Again, you can't claim copyright for just moving code around.

> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/interconnect.h>
> +#include <linux/pm_opp.h>
> +#include <linux/units.h>
> +
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +#include "pcie-qcom-common.h"
> +
> +struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
> +{
> +	struct icc_path *icc_p;
> +
> +	icc_p = devm_of_icc_get(pci->dev, path);
> +	return icc_p;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_common_icc_get_resource);
> +
> +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc, u32 bandwidth)
> +{
> +	int ret;
> +
> +	ret = icc_set_bw(icc, 0, bandwidth);
> +	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);

As I already pointed out, these helpers seems to be of very little worth
and just hides what is really going on (e.g. that the resources are
device managed). Please consider dropping them.

> 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..897fa18e618a
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> @@ -0,0 +1,15 @@
> +/* 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.

Same copyright issue here.

> + */
> +
> +#include "pcie-designware.h"
> +
> +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> +		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
> +
> +struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
> +int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
> +void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);

Compile guards still missing, despite me pointing that out before.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 236229f66c80..e1860026e134 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
  
> -	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);

Does not even compile, as reported by the build bots.

> -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);
> -
> -	pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
> -	if (IS_ERR(pcie->icc_cpu))
> -		return PTR_ERR(pcie->icc_cpu);
> -	/*
> -	 * 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 bandwidth for PCIe-MEM interconnect path: %d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	/*
> -	 * Since the CPU-PCIe path is only used for activities like register
> -	 * access of the host controller and endpoint Config/BAR space access,
> -	 * HW team has recommended to use a minimal bandwidth of 1KBps just to
> -	 * keep the path active.
> -	 */
> -	ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
> -	if (ret) {
> -		dev_err(pci->dev, "Failed to set bandwidth for CPU-PCIe interconnect path: %d\n",
> -			ret);
> -		icc_set_bw(pcie->icc_mem, 0, 0);
> -		return ret;
> -	}
> -
> -	return 0;
> -}

Just keep this function as is.

> -static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
> -{
> -	u32 offset, status, width, speed;
> -	struct dw_pcie *pci = pcie->pci;
> -	unsigned long freq_kbps;
> -	struct dev_pm_opp *opp;
> -	int ret, freq_mbps;
> -
> -	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);
> -
> -	if (pcie->icc_mem) {
> -		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 bandwidth for PCIe-MEM interconnect path: %d\n",
> -				ret);
> -		}
> -	} else {
> -		freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
> -		if (freq_mbps < 0)
> -			return;
> -
> -		freq_kbps = freq_mbps * KILO;
> -		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
> -						 true);
> -		if (!IS_ERR(opp)) {
> -			ret = dev_pm_opp_set_opp(pci->dev, opp);
> -			if (ret)
> -				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
> -					freq_kbps * width, ret);
> -			dev_pm_opp_put(opp);
> -		}
> -	}
> -}

Maybe it's worth trying to generalise this, but probably not. Either
way, I don't think the gen4 stability *fixes* should depend on this (the
gen4 nvme link on x1e80100 is currently broken and depends on the later
changes in this series).

Please consider dropping all this, mostly bogus, refactoring and just
get the gen4 fixes in first.

> -
>  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);
> @@ -1561,6 +1472,18 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_pm_runtime_put;
>  	}
>  
> +	pcie->icc_mem = qcom_pcie_common_icc_get_resource(pcie->pci, "pcie-mem");
> +	if (IS_ERR_OR_NULL(pcie->icc_mem)) {

This will break machines which don't have this path. NULL is valid here.

> +		ret = PTR_ERR(pcie->icc_mem);
> +		goto err_pm_runtime_put;
> +	}
> +
> +	pcie->icc_cpu = qcom_pcie_common_icc_get_resource(pcie->pci, "cpu-pcie");
> +	if (IS_ERR_OR_NULL(pcie->icc_cpu)) {

Same here.

> +		ret = PTR_ERR(pcie->icc_cpu);
> +		goto err_pm_runtime_put;
> +	}
> +
>  	/* OPP table is optional */
>  	ret = devm_pm_opp_of_add_table(dev);
>  	if (ret && ret != -ENODEV) {

> @@ -1681,7 +1629,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>  	if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>  		ret = icc_disable(pcie->icc_cpu);
>  		if (ret)
> -			dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n", ret);
> +			dev_err(dev,
> +			"Failed to disable CPU-PCIe interconnect path: %d\n", ret);

Unrelated, bogus change.

Johan

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

[Shashank Babu Chinta Venkata]

During high data transmission rates such as 16 GT/s , there is an
increased risk of signal loss due to poor channel quality and
interference. This can impact receiver's ability to capture signals
accurately. Hence, signal compensation is achieved through appropriate
lane equalization settings at both transmitter and receiver. This will
result in increased PCIe signal strength.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 37 +++++++++++++++++++
 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, 56 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 53c4c8f399c8..50265a2fbb9f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -126,6 +126,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		GENMASK(3, 0)
+#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE	BIT(4)
+#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC	GENMASK(23, 8)
+#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
+
+#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
+#define GEN3_EQ_FMDC_T_MIN_PHASE23		GENMASK(4, 0)
+#define GEN3_EQ_FMDC_N_EVALS			GENMASK(9, 5)
+#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA	GENMASK(13, 10)
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA	GENMASK(17, 14)
+
 #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 1d8992147bba..e085075557cd 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -15,6 +15,43 @@
 #include "pcie-designware.h"
 #include "pcie-qcom-common.h"
 
+void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
+{
+	u32 reg;
+
+	/*
+	 * GEN3_RELATED_OFF register is repurposed to apply equalization
+	 * settings at various data transmission rates through registers
+	 * namely GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF
+	 * determines data rate for which this 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, 0x1);
+	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_eq_settings);
+
 struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
 {
 	struct icc_path *icc_p;
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
index 897fa18e618a..c281582de12c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -13,3 +13,4 @@
 struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
 int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
 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 e1860026e134..823e33a4d745 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -455,6 +455,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 ee32590f1506..829b34391af1 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -280,6 +280,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.46.0

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

[Bjorn Helgaas]

On Wed, Aug 21, 2024 at 10:08:43AM -0700, Shashank Babu Chinta Venkata wrote:
> During high data transmission rates such as 16 GT/s , there is an

s/ ,/,/

> increased risk of signal loss due to poor channel quality and
> interference. This can impact receiver's ability to capture signals
> accurately. Hence, signal compensation is achieved through appropriate
> lane equalization settings at both transmitter and receiver. This will
> result in increased PCIe signal strength.

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

[Manivannan Sadhasivam]

On Wed, Aug 21, 2024 at 10:08:43AM -0700, Shashank Babu Chinta Venkata wrote:
> During high data transmission rates such as 16 GT/s , there is an
> increased risk of signal loss due to poor channel quality and
> interference. This can impact receiver's ability to capture signals
> accurately. Hence, signal compensation is achieved through appropriate
> lane equalization settings at both transmitter and receiver. This will
> result in increased PCIe signal strength.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++
>  drivers/pci/controller/dwc/pcie-qcom-common.c | 37 +++++++++++++++++++
>  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, 56 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 53c4c8f399c8..50265a2fbb9f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -126,6 +126,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		GENMASK(3, 0)
> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE	BIT(4)
> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC	GENMASK(23, 8)
> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
> +
> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> +#define GEN3_EQ_FMDC_T_MIN_PHASE23		GENMASK(4, 0)
> +#define GEN3_EQ_FMDC_N_EVALS			GENMASK(9, 5)
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA	GENMASK(13, 10)
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA	GENMASK(17, 14)
> +
>  #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 1d8992147bba..e085075557cd 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -15,6 +15,43 @@
>  #include "pcie-designware.h"
>  #include "pcie-qcom-common.h"
>  
> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	/*
> +	 * GEN3_RELATED_OFF register is repurposed to apply equalization
> +	 * settings at various data transmission rates through registers
> +	 * namely GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF
> +	 * determines data rate for which this 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, 0x1);
> +	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_eq_settings);
> +
>  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
>  {
>  	struct icc_path *icc_p;
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
> index 897fa18e618a..c281582de12c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.h
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> @@ -13,3 +13,4 @@
>  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
>  int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
>  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 e1860026e134..823e33a4d745 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -455,6 +455,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)

Abel reported that 'pci->link_gen' is not updated unless the 'max-link-speed'
property is set in DT on his platform. I fixed that issue locally and this
series will depend on those patches.

Provided that you are having issues with your build environment as discussed
offline, I'd like to take over the series to combine my patches and address the
review comments. Let me know if you are OK with this or not.

- Mani

> +		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 ee32590f1506..829b34391af1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -280,6 +280,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.46.0
> 

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

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

[Shashank Babu Chinta Venkata]

On 8/26/24 00:55, Manivannan Sadhasivam wrote:
> On Wed, Aug 21, 2024 at 10:08:43AM -0700, Shashank Babu Chinta Venkata wrote:
>> During high data transmission rates such as 16 GT/s , there is an
>> increased risk of signal loss due to poor channel quality and
>> interference. This can impact receiver's ability to capture signals
>> accurately. Hence, signal compensation is achieved through appropriate
>> lane equalization settings at both transmitter and receiver. This will
>> result in increased PCIe signal strength.
>>
>> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++
>>  drivers/pci/controller/dwc/pcie-qcom-common.c | 37 +++++++++++++++++++
>>  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, 56 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 53c4c8f399c8..50265a2fbb9f 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -126,6 +126,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		GENMASK(3, 0)
>> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE	BIT(4)
>> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC	GENMASK(23, 8)
>> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
>> +
>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
>> +#define GEN3_EQ_FMDC_T_MIN_PHASE23		GENMASK(4, 0)
>> +#define GEN3_EQ_FMDC_N_EVALS			GENMASK(9, 5)
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA	GENMASK(13, 10)
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA	GENMASK(17, 14)
>> +
>>  #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 1d8992147bba..e085075557cd 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>> @@ -15,6 +15,43 @@
>>  #include "pcie-designware.h"
>>  #include "pcie-qcom-common.h"
>>  
>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>> +{
>> +	u32 reg;
>> +
>> +	/*
>> +	 * GEN3_RELATED_OFF register is repurposed to apply equalization
>> +	 * settings at various data transmission rates through registers
>> +	 * namely GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF
>> +	 * determines data rate for which this 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, 0x1);
>> +	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_eq_settings);
>> +
>>  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
>>  {
>>  	struct icc_path *icc_p;
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
>> index 897fa18e618a..c281582de12c 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.h
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
>> @@ -13,3 +13,4 @@
>>  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
>>  int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
>>  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 e1860026e134..823e33a4d745 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -455,6 +455,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)
> 
> Abel reported that 'pci->link_gen' is not updated unless the 'max-link-speed'
> property is set in DT on his platform. I fixed that issue locally and this
> series will depend on those patches.
> 
> Provided that you are having issues with your build environment as discussed
> offline, I'd like to take over the series to combine my patches and address the
> review comments. Let me know if you are OK with this or not.
> 
> - Mani
> 

Sure Mani.You can include my patches in your series.

Thanks
Shashank
>> +		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 ee32590f1506..829b34391af1 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -280,6 +280,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.46.0
>>
> 

[PATCH v5 3/3] PCI: qcom: Add RX margining settings for 16 GT/s

[Shashank Babu Chinta Venkata]

Add RX lane margining settings for 16 GT/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>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware.h  | 18 +++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 31 +++++++++++++++++++
 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, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 50265a2fbb9f..3d55ddf351a8 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -209,6 +209,24 @@
 
 #define PCIE_PL_CHK_REG_ERR_ADDR			0xB28
 
+/*
+ * 16 GT/s (GEN4) lane margining register definitions
+ */
+#define GEN4_LANE_MARGINING_1_OFF		0xb80
+#define MARGINING_MAX_VOLTAGE_OFFSET		GENMASK(29, 24)
+#define MARGINING_NUM_VOLTAGE_STEPS		GENMASK(22, 16)
+#define MARGINING_MAX_TIMING_OFFSET		GENMASK(13, 8)
+#define MARGINING_NUM_TIMING_STEPS		GENMASK(5, 0)
+
+#define GEN4_LANE_MARGINING_2_OFF		0xb84
+#define MARGINING_IND_ERROR_SAMPLER		BIT(28)
+#define MARGINING_SAMPLE_REPORTING_METHOD	BIT(27)
+#define MARGINING_IND_LEFT_RIGHT_TIMING		BIT(26)
+#define MARGINING_IND_UP_DOWN_VOLTAGE		BIT(25)
+#define MARGINING_VOLTAGE_SUPPORTED		BIT(24)
+#define MARGINING_MAXLANES			GENMASK(20, 16)
+#define MARGINING_SAMPLE_RATE_TIMING		GENMASK(13, 8)
+#define MARGINING_SAMPLE_RATE_VOLTAGE		GENMASK(5, 0)
 /*
  * 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 e085075557cd..3fa91cb52f5b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -52,6 +52,37 @@ 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 |
+		MARGINING_NUM_VOLTAGE_STEPS |
+		MARGINING_MAX_TIMING_OFFSET |
+		MARGINING_NUM_TIMING_STEPS);
+	reg |= FIELD_PREP(MARGINING_MAX_VOLTAGE_OFFSET, 0x24) |
+		FIELD_PREP(MARGINING_NUM_VOLTAGE_STEPS, 0x78) |
+		FIELD_PREP(MARGINING_MAX_TIMING_OFFSET, 0x32) |
+		FIELD_PREP(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 |
+		MARGINING_SAMPLE_REPORTING_METHOD |
+		MARGINING_IND_LEFT_RIGHT_TIMING |
+		MARGINING_VOLTAGE_SUPPORTED;
+	reg &= ~(MARGINING_IND_UP_DOWN_VOLTAGE |
+		MARGINING_MAXLANES |
+		MARGINING_SAMPLE_RATE_TIMING |
+		MARGINING_SAMPLE_RATE_VOLTAGE);
+	reg |= FIELD_PREP(MARGINING_MAXLANES, pci->num_lanes) |
+		FIELD_PREP(MARGINING_SAMPLE_RATE_TIMING, 0x3f) |
+		FIELD_PREP(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);
+
 struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
 {
 	struct icc_path *icc_p;
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
index c281582de12c..71ff675d6af0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -14,3 +14,4 @@ struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const ch
 int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem, u32 bandwidth);
 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 823e33a4d745..aff9ae9778a1 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -455,8 +455,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 829b34391af1..0256fb5131d7 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -280,8 +280,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.46.0

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

[Johan Hovold]

On Wed, Aug 21, 2024 at 10:08:41AM -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.
> 
> v4 -> v5:
> - Added additional parameter bandwidth to accommodate new icc path.
> - Fixed typo.
> - Picked up Reviewed-by tags.

First, make sure to CC people that help reviewing your patches.

Second, you don't mention that your previous series were completely
broken as I pointed out here:

	https://lore.kernel.org/all/ZpDlf5xD035x2DqL@hovoldconsulting.com/

You apparently fixed that in v5 but conveniently forgot to mention it in
the change log. Don't do that. Own your mistakes and learn from them.

Third, don't send untested crap upstream. You clearly did not test your
previous series properly and now v5 does not even build.

Seriously, this is completely unacceptable and you're just wasting other
people's time.

> v3 -> v4:
> - Addressed review comments from Mani and Konrad.
> - Preceded subject line with pci: qcom: tags
> 
> 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.

Johan