[PATCH v1 0/3] Add Gen4 equalization and margining settings

[PATCH v1 0/3] Add Gen4 equalization and margining settings

[Shashank Babu Chinta Venkata]

Add Gen4 specific equalization and rx margining settings. These
settings are inline with respective PHY settings for Gen4
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.

Shashank Babu Chinta Venkata (3):
  PCI: dwc: refactor common code
  PCI: dwc: add equalization settings for gen4
  PCI: dwc: add rx margining settings for gen4

 drivers/pci/controller/dwc/Kconfig         |   5 +
 drivers/pci/controller/dwc/Makefile        |   1 +
 drivers/pci/controller/dwc/pcie-qcom-cmn.c | 152 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-cmn.h |  87 ++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c  |  45 ++----
 drivers/pci/controller/dwc/pcie-qcom.c     |  73 ++--------
 6 files changed, 269 insertions(+), 94 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.c
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.h

-- 
2.43.2

[PATCH v1 1/3] PCI: dwc: 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 repository. This acts as placeholder
for common source code for both drivers 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-cmn.c | 85 ++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-cmn.h | 30 ++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c  | 39 +---------
 drivers/pci/controller/dwc/pcie-qcom.c     | 67 ++---------------
 6 files changed, 133 insertions(+), 94 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.c
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.h

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 8afacc90c63b..41d2746edc5f 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_CMN
+	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_CMN
 	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_CMN
 	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..022dc73c38a5 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
+obj-$(CONFIG_PCIE_QCOM_CMN) += pcie-qcom-cmn.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
new file mode 100644
index 000000000000..0f8d004fbc79
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
+ * Copyright 2015, 2021 Linaro Limited.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ */
+
+#include <linux/debugfs.h>
+#include <linux/pci.h>
+#include <linux/interconnect.h>
+
+#include "../../pci.h"
+#include "pcie-designware.h"
+#include "pcie-qcom-cmn.h"
+
+
+#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
+		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
+
+
+int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	int ret = 0;
+
+	if (IS_ERR(pci))
+		return PTR_ERR(pci);
+
+	icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
+	if (IS_ERR(icc_mem))
+		return PTR_ERR(icc_mem);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_get_resource);
+
+int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	int ret = 0;
+
+	if (IS_ERR(pci))
+		return PTR_ERR(pci);
+
+	if (IS_ERR(icc_mem))
+		return PTR_ERR(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(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_cmn_icc_init);
+
+void qcom_pcie_cmn_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);
+
+	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_cmn_icc_update);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
new file mode 100644
index 000000000000..8794dbd4775c
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
+ * Copyright 2015, 2021 Linaro Limited.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/pci.h>
+#include "../../pci.h"
+#include "pcie-designware.h"
+
+#ifdef CONFIG_PCIE_QCOM_CMN
+int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
+int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
+void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
+#else
+static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	return 0;
+}
+
+static inline int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+	return 0;
+}
+
+static inline void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
+{
+}
+#endif
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 36e5e80cd22f..ce6343426de8 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-cmn.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_cmn_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_cmn_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_cmn_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..57a08294c561 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-cmn.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_cmn_icc_get_resource(pcie->pci, pcie->icc_mem);
+	if (ret)
+		goto err_pm_runtime_put;
+
+	ret = qcom_pcie_cmn_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_cmn_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_cmn_icc_update(pcie->pci, pcie->icc_mem);
 
 	return 0;
 }
-- 
2.43.2

Re: [PATCH v1 1/3] PCI: dwc: refactor common code

[Frank Li]

tag should be  PCI: qcom:

Frank

On Thu, Feb 29, 2024 at 09:11:34PM -0800, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)
> drivers and move them to a common repository. This acts as placeholder
> for common source code for both drivers 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-cmn.c | 85 ++++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-cmn.h | 30 ++++++++
>  drivers/pci/controller/dwc/pcie-qcom-ep.c  | 39 +---------
>  drivers/pci/controller/dwc/pcie-qcom.c     | 67 ++---------------
>  6 files changed, 133 insertions(+), 94 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.h
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 8afacc90c63b..41d2746edc5f 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_CMN
> +	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_CMN
>  	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_CMN
>  	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..022dc73c38a5 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> +obj-$(CONFIG_PCIE_QCOM_CMN) += pcie-qcom-cmn.o
>  
>  # The following drivers are for devices that use the generic ACPI
>  # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> new file mode 100644
> index 000000000000..0f8d004fbc79
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + *
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/pci.h>
> +#include <linux/interconnect.h>
> +
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +#include "pcie-qcom-cmn.h"
> +
> +
> +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> +		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
> +
> +
> +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	int ret = 0;
> +
> +	if (IS_ERR(pci))
> +		return PTR_ERR(pci);
> +
> +	icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR(icc_mem))
> +		return PTR_ERR(icc_mem);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_get_resource);
> +
> +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	int ret = 0;
> +
> +	if (IS_ERR(pci))
> +		return PTR_ERR(pci);
> +
> +	if (IS_ERR(icc_mem))
> +		return PTR_ERR(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(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_cmn_icc_init);
> +
> +void qcom_pcie_cmn_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);
> +
> +	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_cmn_icc_update);
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> new file mode 100644
> index 000000000000..8794dbd4775c
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/pci.h>
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +
> +#ifdef CONFIG_PCIE_QCOM_CMN
> +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
> +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
> +void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
> +#else
> +static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	return 0;
> +}
> +
> +static inline int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	return 0;
> +}
> +
> +static inline void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +}
> +#endif
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 36e5e80cd22f..ce6343426de8 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-cmn.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_cmn_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_cmn_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_cmn_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..57a08294c561 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-cmn.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_cmn_icc_get_resource(pcie->pci, pcie->icc_mem);
> +	if (ret)
> +		goto err_pm_runtime_put;
> +
> +	ret = qcom_pcie_cmn_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_cmn_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_cmn_icc_update(pcie->pci, pcie->icc_mem);
>  
>  	return 0;
>  }
> -- 
> 2.43.2
> 

Re: [PATCH v1 1/3] PCI: dwc: refactor common code

[Bjorn Helgaas]

On Thu, Feb 29, 2024 at 09:11:34PM -0800, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)
> drivers and move them to a common repository. This acts as placeholder
> for common source code for both drivers 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-cmn.c | 85 ++++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom-cmn.h | 30 ++++++++
>  drivers/pci/controller/dwc/pcie-qcom-ep.c  | 39 +---------
>  drivers/pci/controller/dwc/pcie-qcom.c     | 67 ++---------------
>  6 files changed, 133 insertions(+), 94 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.h

Hmm.  I'm a little ambivalent about adding two new files.  Overall I
think I prefer the drivers that include both RC and EP mode in a
single source file because one file is easier to browse than four and
more things can be static.

A single file would also reduce quite a bit more duplication between
pcie-qcom.c and pcie-qcom-ep.c, e.g., register names and fields with
needlessly different names:

  #define AUX_PWR_DET                     BIT(4)  # pcie-qcom.c
  #define PARF_SYS_CTRL_AUX_PWR_DET       BIT(4)  # pcie-qcom-ep.c

I do see PCIE_QCOM is bool and PCIE_QCOM_EP is tristate, so that and
other considerations might make a single source file impractical.

> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> +obj-$(CONFIG_PCIE_QCOM_CMN) += pcie-qcom-cmn.o

If we have to have pcie-qcom-cmn.o, at least move this next to the
existing lines:

  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
  obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o

> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + *

Spurious blank line.

> +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)

I don't see the value of adding "cmn" in the middle of the names.

> +{
> +	int ret = 0;
> +
> +	if (IS_ERR(pci))
> +		return PTR_ERR(pci);
> +
> +	icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR(icc_mem))
> +		return PTR_ERR(icc_mem);
> +
> +	return ret;

No need for the "ret" variable since it's never assigned.  "return 0"
here would be easier to read.

> +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	int ret = 0;

Unnecessary initialization.

> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/pci.h>
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +
> +#ifdef CONFIG_PCIE_QCOM_CMN

Why the #ifdef wrapper?  And why do we need the stubs when
CONFIG_PCIE_QCOM_CMN isn't defined?

> +#else
> +static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	return 0;
> +}

Re: [PATCH v1 1/3] PCI: dwc: refactor common code

[Manivannan Sadhasivam]

On Fri, Mar 01, 2024 at 01:44:56PM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:11:34PM -0800, Shashank Babu Chinta Venkata wrote:
> > Refactor common code from RC(Root Complex) and EP(End Point)
> > drivers and move them to a common repository. This acts as placeholder
> > for common source code for both drivers 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-cmn.c | 85 ++++++++++++++++++++++
> >  drivers/pci/controller/dwc/pcie-qcom-cmn.h | 30 ++++++++
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c  | 39 +---------
> >  drivers/pci/controller/dwc/pcie-qcom.c     | 67 ++---------------
> >  6 files changed, 133 insertions(+), 94 deletions(-)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.c
> >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-cmn.h
> 
> Hmm.  I'm a little ambivalent about adding two new files.  Overall I
> think I prefer the drivers that include both RC and EP mode in a
> single source file because one file is easier to browse than four and
> more things can be static.
> 
> A single file would also reduce quite a bit more duplication between
> pcie-qcom.c and pcie-qcom-ep.c, e.g., register names and fields with
> needlessly different names:
> 
>   #define AUX_PWR_DET                     BIT(4)  # pcie-qcom.c
>   #define PARF_SYS_CTRL_AUX_PWR_DET       BIT(4)  # pcie-qcom-ep.c
> 
> I do see PCIE_QCOM is bool and PCIE_QCOM_EP is tristate, so that and
> other considerations might make a single source file impractical.
> 

Yeah, we explored that option while adding the EP driver and it didn't look
feasible.

- Mani

> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> >  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> >  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
> >  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> > +obj-$(CONFIG_PCIE_QCOM_CMN) += pcie-qcom-cmn.o
> 
> If we have to have pcie-qcom-cmn.o, at least move this next to the
> existing lines:
> 
>   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>   obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> 
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> > @@ -0,0 +1,85 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> > + * Copyright 2015, 2021 Linaro Limited.
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + *
> 
> Spurious blank line.
> 
> > +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> 
> I don't see the value of adding "cmn" in the middle of the names.
> 
> > +{
> > +	int ret = 0;
> > +
> > +	if (IS_ERR(pci))
> > +		return PTR_ERR(pci);
> > +
> > +	icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> > +	if (IS_ERR(icc_mem))
> > +		return PTR_ERR(icc_mem);
> > +
> > +	return ret;
> 
> No need for the "ret" variable since it's never assigned.  "return 0"
> here would be easier to read.
> 
> > +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> > +{
> > +	int ret = 0;
> 
> Unnecessary initialization.
> 
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> > + * Copyright 2015, 2021 Linaro Limited.
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include "../../pci.h"
> > +#include "pcie-designware.h"
> > +
> > +#ifdef CONFIG_PCIE_QCOM_CMN
> 
> Why the #ifdef wrapper?  And why do we need the stubs when
> CONFIG_PCIE_QCOM_CMN isn't defined?
> 
> > +#else
> > +static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> > +{
> > +	return 0;
> > +}

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

Re: [PATCH v1 1/3] PCI: dwc: refactor common code

[Konrad Dybcio]

On 1.03.2024 06:11, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)
> drivers and move them to a common repository. This acts as placeholder
> for common source code for both drivers avoiding duplication.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> ---

This "conveniently" conflicts with your colleague's patches..

https://lore.kernel.org/linux-arm-msm/20240223-opp_support-v7-3-10b4363d7e71@quicinc.com/

Konrad

[PATCH v1 2/3] PCI: dwc: add equalization settings for gen4

[Shashank Babu Chinta Venkata]

GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
generation5 data rates based on rate select mask settings on this register.
Select relevant mask and equalization settings for generation4 operation.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom-cmn.c | 31 ++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-cmn.h | 23 ++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c  |  4 +++
 drivers/pci/controller/dwc/pcie-qcom.c     |  4 +++
 4 files changed, 62 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
index 0f8d004fbc79..cfdc04eef78c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
@@ -18,6 +18,37 @@
 #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
 		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
 
+void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
+{
+	u32 reg;
+
+	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
+	reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
+	reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
+	reg |= (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_MASK;
+	reg &= ~GEN3_EQ_FMDC_N_EVALS_MASK;
+	reg |= (GEN3_EQ_FMDC_N_EVALS_VAL <<
+		GEN3_EQ_FMDC_N_EVALS_SHIFT);
+	reg &= ~GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK;
+	reg |= (GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL <<
+		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT);
+	reg &= ~GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK;
+	reg |= (GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL <<
+		GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT);
+	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_MASK;
+	reg &= ~GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE;
+	reg &= ~GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL;
+	reg &= ~GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK;
+	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_cmn_set_gen4_eq_settings);
 
 int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
index 8794dbd4775c..08e1bd179207 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
@@ -9,10 +9,29 @@
 #include "../../pci.h"
 #include "pcie-designware.h"
 
+#define GEN3_EQ_CONTROL_OFF			0x8a8
+#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK        GENMASK(3, 0)
+#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE   BIT(4)
+#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	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_MAX_PRE_CUSROR_DELTA_VAL   0x5
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
+#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
+#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK         GENMASK(4, 0)
+#define GEN3_EQ_FMDC_N_EVALS_MASK               GENMASK(9, 5)
+#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK  GENMASK(13, 10)
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK	GENMASK(17, 14)
+#define GEN3_EQ_FMDC_N_EVALS_SHIFT			5
+#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14
+
 #ifdef CONFIG_PCIE_QCOM_CMN
 int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
 int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
+void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci);
 #else
 static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
@@ -27,4 +46,8 @@ static inline int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *i
 static inline void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
 }
+
+static inline void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
+{
+}
 #endif
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index ce6343426de8..0b169bcd081d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
+	/* set Gen4 equalization settings */
+	if (pci->link_gen == 4)
+		qcom_pcie_cmn_set_gen4_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 57a08294c561..ad0cd55da777 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
+	/* set Gen4 equalization settings */
+	if (pci->link_gen == 4)
+		qcom_pcie_cmn_set_gen4_eq_settings(pci);
+
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
 		pcie->cfg->ops->ltssm_enable(pcie);
-- 
2.43.2

Re: [PATCH v1 2/3] PCI: dwc: add equalization settings for gen4

[Bjorn Helgaas]

On Thu, Feb 29, 2024 at 09:11:35PM -0800, Shashank Babu Chinta Venkata wrote:
> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
> generation5 data rates based on rate select mask settings on this register.
> Select relevant mask and equalization settings for generation4 operation.

Please capitalize subject lines to match history ("PCI: qcom: Add ...")

s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)

I wish these "GEN3_RELATED" things were named with the data rate
instead of "GEN3".  The PCIe spec defines these things based on the
data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
appeared in (gen3/gen4/etc).

Using "GEN3" means we have to first look up the "gen -> rate" mapping
before finding the relevant spec info.

Applies to the subject line, commit log, #defines, function names,
etc.

> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);

Warrants a one-line comment about using "GEN3_..." in a function named
"..._gen4_..."  (But ideally both would be renamed based on the data
rate instead.)

> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,10 +9,29 @@
>  #include "../../pci.h"
>  #include "pcie-designware.h"
>  
> +#define GEN3_EQ_CONTROL_OFF			0x8a8
> +#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK        GENMASK(3, 0)
> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE   BIT(4)
> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)

Are these qcom-specific registers, or should they be added alongside
GEN3_RELATED_OFF in pcie-designware.h?

> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
> +#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK         GENMASK(4, 0)
> +#define GEN3_EQ_FMDC_N_EVALS_MASK               GENMASK(9, 5)
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK  GENMASK(13, 10)
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK	GENMASK(17, 14)
> +#define GEN3_EQ_FMDC_N_EVALS_SHIFT			5
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14

> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  		goto err_disable_resources;
>  	}
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);

> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);

Re: [PATCH v1 2/3] PCI: dwc: add equalization settings for gen4

[Shashank Babu Chinta Venkata]

On 3/1/24 12:00, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:11:35PM -0800, Shashank Babu Chinta Venkata wrote:
>> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
>> generation5 data rates based on rate select mask settings on this register.
>> Select relevant mask and equalization settings for generation4 operation.
> 
> Please capitalize subject lines to match history ("PCI: qcom: Add ...")
> 
> s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)
> 
> I wish these "GEN3_RELATED" things were named with the data rate
> instead of "GEN3".  The PCIe spec defines these things based on the
> data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
> appeared in (gen3/gen4/etc).
I have kept it consistent with nomenclature followed in pcie designware
documentation for these registers. For function names and constraint to 
apply these, I will fall back to data rates rather than generation.

> Using "GEN3" means we have to first look up the "gen -> rate" mapping
> before finding the relevant spec info.
> 
> Applies to the subject line, commit log, #defines, function names,
> etc.
> 
>> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
>> +{
>> +	u32 reg;
>> +
>> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> 
> Warrants a one-line comment about using "GEN3_..." in a function named
> "..._gen4_..."  (But ideally both would be renamed based on the data
> rate instead.)
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
>> @@ -9,10 +9,29 @@
>>   #include "../../pci.h"
>>   #include "pcie-designware.h"
>>   
>> +#define GEN3_EQ_CONTROL_OFF			0x8a8
>> +#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK        GENMASK(3, 0)
>> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE   BIT(4)
>> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
>> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
> 
> Are these qcom-specific registers, or should they be added alongside
> GEN3_RELATED_OFF in pcie-designware.h?
yes, these are designware register offsets. Will move them to designware 
header file. However, the settings are vendor specific.I will park
settings for these in QCOM specific files.
> 
>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
>> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
>> +#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK         GENMASK(4, 0)
>> +#define GEN3_EQ_FMDC_N_EVALS_MASK               GENMASK(9, 5)
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK  GENMASK(13, 10)
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK	GENMASK(17, 14)
>> +#define GEN3_EQ_FMDC_N_EVALS_SHIFT			5
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>>   		goto err_disable_resources;
>>   	}
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);

[PATCH v1 3/3] PCI: dwc: add rx margining settings for gen4

[Shashank Babu Chinta Venkata]

Add rx margining settings for gen4 operation.

Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom-cmn.c | 36 ++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-cmn.h | 34 ++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c  |  4 ++-
 drivers/pci/controller/dwc/pcie-qcom.c     |  4 ++-
 4 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
index cfdc04eef78c..abba4de32005 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
@@ -50,6 +50,42 @@ void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
 }
 EXPORT_SYMBOL_GPL(qcom_pcie_cmn_set_gen4_eq_settings);
 
+void qcom_pcie_cmn_set_gen4_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_MASK;
+	reg |= (MARGINING_MAX_VOLTAGE_OFFSET_VAL <<
+		MARGINING_MAX_VOLTAGE_OFFSET_SHIFT);
+	reg &= ~MARGINING_NUM_VOLTAGE_STEPS_MASK;
+	reg |= (MARGINING_NUM_VOLTAGE_STEPS_VAL <<
+		MARGINING_NUM_VOLTAGE_STEPS_SHIFT);
+	reg &= ~MARGINING_MAX_TIMING_OFFSET_MASK;
+	reg |= (MARGINING_MAX_TIMING_OFFSET_VAL <<
+		MARGINING_MAX_TIMING_OFFSET_SHIFT);
+	reg &= ~MARGINING_NUM_TIMING_STEPS_MASK;
+	reg |= MARGINING_NUM_TIMING_STEPS_VAL;
+	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;
+	reg |= MARGINING_SAMPLE_REPORTING_METHOD;
+	reg |= MARGINING_IND_LEFT_RIGHT_TIMING;
+	reg |= MARGINING_VOLTAGE_SUPPORTED;
+	reg &= ~MARGINING_IND_UP_DOWN_VOLTAGE;
+	reg &= ~MARGINING_MAXLANES_MASK;
+	reg |= (pci->num_lanes <<
+		MARGINING_MAXLANES_SHIFT);
+	reg &= ~MARGINING_SAMPLE_RATE_TIMING_MASK;
+	reg |= (MARGINING_SAMPLE_RATE_TIMING_VAL <<
+		MARGINING_SAMPLE_RATE_TIMING_SHIFT);
+	reg |= MARGINING_SAMPLE_RATE_VOLTAGE_VAL;
+	dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_2_OFF, reg);
+}
+EXPORT_SYMBOL_GPL(qcom_pcie_cmn_set_gen4_rx_margining_settings);
+
 int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
 	int ret = 0;
diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
index 08e1bd179207..b145743a7558 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
@@ -27,11 +27,40 @@
 #define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
 #define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14
 
+#define GEN4_LANE_MARGINING_1_OFF		0xb80
+#define GEN4_LANE_MARGINING_2_OFF		0xb84
+
+#define MARGINING_MAX_VOLTAGE_OFFSET_MASK	GENMASK(29, 24)
+#define MARGINING_NUM_VOLTAGE_STEPS_MASK	GENMASK(22, 16)
+#define MARGINING_MAX_TIMING_OFFSET_MASK	GENMASK(13, 8)
+#define MARGINING_NUM_TIMING_STEPS_MASK		GENMASK(5, 0)
+#define MARGINING_MAX_VOLTAGE_OFFSET_SHIFT	24
+#define MARGINING_NUM_VOLTAGE_STEPS_SHIFT	16
+#define MARGINING_MAX_TIMING_OFFSET_SHIFT	8
+#define MARGINING_MAX_VOLTAGE_OFFSET_VAL	0x24
+#define MARGINING_NUM_VOLTAGE_STEPS_VAL		0x78
+#define MARGINING_MAX_TIMING_OFFSET_VAL		0x32
+#define MARGINING_NUM_TIMING_STEPS_VAL		0x10
+
+#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_MASK			GENMASK(20, 16)
+#define MARGINING_SAMPLE_RATE_TIMING_MASK	GENMASK(13, 8)
+#define MARGINING_SAMPLE_RATE_VOLTAGE_MASK	GENMASK(5, 0)
+#define MARGINING_MAXLANES_SHIFT		16
+#define MARGINING_SAMPLE_RATE_TIMING_SHIFT	8
+#define MARGINING_SAMPLE_RATE_TIMING_VAL	0x3f
+#define MARGINING_SAMPLE_RATE_VOLTAGE_VAL	0x3f
+
 #ifdef CONFIG_PCIE_QCOM_CMN
 int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
 int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
 void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci);
+void qcom_pcie_cmn_set_gen4_rx_margining_settings(struct dw_pcie *pci);
 #else
 static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
@@ -50,4 +79,9 @@ static inline void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path
 static inline void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
 {
 }
+
+static inline void qcom_pcie_cmn_set_gen4_rx_margining_settings(struct dw_pcie *pci)
+{
+}
+
 #endif
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 0b169bcd081d..5422fa970d9d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -439,8 +439,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 	}
 
 	/* set Gen4 equalization settings */
-	if (pci->link_gen == 4)
+	if (pci->link_gen == 4) {
 		qcom_pcie_cmn_set_gen4_eq_settings(pci);
+		qcom_pcie_cmn_set_gen4_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 ad0cd55da777..3ada1e9fdd11 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -264,8 +264,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
 	/* set Gen4 equalization settings */
-	if (pci->link_gen == 4)
+	if (pci->link_gen == 4) {
 		qcom_pcie_cmn_set_gen4_eq_settings(pci);
+		qcom_pcie_cmn_set_gen4_rx_margining_settings(pci);
+	}
 
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
-- 
2.43.2