[PATCH v6 0/4] PCI: qcom: Add 16.0 GT/s equalization and margining settings

[PATCH v6 0/4] PCI: qcom: Add 16.0 GT/s equalization and margining settings

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series adds 16.0 GT/s specific equalization and RX lane margining settings
to the Qcom RC and EP drivers. This series is mandatory for the stable operation
of the PCIe link at 16.0 GT/s on the Qcom platforms.

NOTE:
=====

I've taken over the series from Shashank based on the discussion [1]. In order
to apply the equalization/margining settings properly in the Qcom driver, I
added the first 2 patches to the series which inevitably touches other vendor
drivers also.

- Mani

Changes in v6:

- Dropped the code refactoring patch as suggested by Johan
- Added 2 patches to fix the caching of maximum supported link speed value that
  is needed to apply the equalization/margining settings
- Updated the commit message of patch 3 as per Bjorn's suggestion

For previous changelogs, please refer [2]

[1] https://lore.kernel.org/linux-pci/af65b744-7538-4929-9ab4-8ee42e17b8d1@quicinc.com/
[2] https://lore.kernel.org/linux-pci/20240821170917.21018-1-quic_schintav@quicinc.com/

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Manivannan Sadhasivam (2):
      PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'
      PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

Shashank Babu Chinta Venkata (2):
      PCI: qcom: Add equalization settings for 16.0 GT/s
      PCI: qcom: Add RX margining settings for 16.0 GT/s

 MAINTAINERS                                   |  4 +-
 drivers/pci/controller/dwc/Kconfig            |  5 ++
 drivers/pci/controller/dwc/Makefile           |  1 +
 drivers/pci/controller/dwc/pci-imx6.c         |  8 +--
 drivers/pci/controller/dwc/pcie-designware.c  | 22 +++++---
 drivers/pci/controller/dwc/pcie-designware.h  | 32 ++++++++++-
 drivers/pci/controller/dwc/pcie-intel-gw.c    |  4 +-
 drivers/pci/controller/dwc/pcie-qcom-common.c | 76 +++++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  9 ++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  6 +++
 drivers/pci/controller/dwc/pcie-qcom.c        |  6 +++
 drivers/pci/controller/dwc/pcie-rcar-gen4.c   |  6 +--
 12 files changed, 162 insertions(+), 17 deletions(-)
---
base-commit: 47ac09b91befbb6a235ab620c32af719f8208399
change-id: 20240904-pci-qcom-gen4-stability-02ec65a7e6e4

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

'link_gen' field is now holding the maximum supported link speed set either
by the controller driver or by DT through 'max-link-speed' property.

But the name 'link_gen' sounds like the negotiated link speed of the PCIe
link. So let's rename it to 'max_link_speed' to make it clear that it holds
the maximum supported link speed of the controller.

NOTE: For the sake of clarity, I've used 'max_link_speed' instead of
'max_link_gen'. Also the link speed and link generation values map 1:1.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pci-imx6.c        |  8 ++++----
 drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++------
 drivers/pci/controller/dwc/pcie-designware.h |  2 +-
 drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
 drivers/pci/controller/dwc/pcie-rcar-gen4.c  |  6 +++---
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 964d67756eb2..ef12a4f31740 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -847,12 +847,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
 	if (ret)
 		goto err_reset_phy;
 
-	if (pci->link_gen > 1) {
+	if (pci->max_link_speed > 1) {
 		/* Allow faster modes after the link is up */
 		dw_pcie_dbi_ro_wr_en(pci);
 		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
 		tmp &= ~PCI_EXP_LNKCAP_SLS;
-		tmp |= pci->link_gen;
+		tmp |= pci->max_link_speed;
 		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
 
 		/*
@@ -1386,8 +1386,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		imx6_pcie->tx_swing_low = 127;
 
 	/* Limit link speed */
-	pci->link_gen = 1;
-	of_property_read_u32(node, "fsl,max-link-speed", &pci->link_gen);
+	pci->max_link_speed = 1;
+	of_property_read_u32(node, "fsl,max-link-speed", &pci->max_link_speed);
 
 	imx6_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
 	if (IS_ERR(imx6_pcie->vpcie)) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 1b5aba1f0c92..86c49ba097c6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -166,8 +166,8 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
 			return ret;
 	}
 
-	if (pci->link_gen < 1)
-		pci->link_gen = of_pci_get_max_link_speed(np);
+	if (pci->max_link_speed < 1)
+		pci->max_link_speed = of_pci_get_max_link_speed(np);
 
 	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
 
@@ -687,7 +687,7 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
 
-static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
+static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
 {
 	u32 cap, ctrl2, link_speed;
 	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -696,7 +696,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
 	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
 	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
 
-	switch (pcie_link_speed[link_gen]) {
+	switch (pcie_link_speed[max_link_speed]) {
 	case PCIE_SPEED_2_5GT:
 		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
 		break;
@@ -1058,8 +1058,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
 {
 	u32 val;
 
-	if (pci->link_gen > 0)
-		dw_pcie_link_set_max_speed(pci, pci->link_gen);
+	if (pci->max_link_speed > 0)
+		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
 
 	/* Configure Gen1 N_FTS */
 	if (pci->n_fts[0]) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 53c4c8f399c8..22765564f301 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -421,7 +421,7 @@ struct dw_pcie {
 	u32			type;
 	unsigned long		caps;
 	int			num_lanes;
-	int			link_gen;
+	int			max_link_speed;
 	u8			n_fts[2];
 	struct dw_edma_chip	edma;
 	struct clk_bulk_data	app_clks[DW_PCIE_NUM_APP_CLKS];
diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
index acbe4f6d3291..676d2aba4fbd 100644
--- a/drivers/pci/controller/dwc/pcie-intel-gw.c
+++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
@@ -132,7 +132,7 @@ static void intel_pcie_link_setup(struct intel_pcie *pcie)
 
 static void intel_pcie_init_n_fts(struct dw_pcie *pci)
 {
-	switch (pci->link_gen) {
+	switch (pci->max_link_speed) {
 	case 3:
 		pci->n_fts[1] = PORT_AFR_N_FTS_GEN3;
 		break;
@@ -252,7 +252,7 @@ static int intel_pcie_wait_l2(struct intel_pcie *pcie)
 	int ret;
 	struct dw_pcie *pci = &pcie->pci;
 
-	if (pci->link_gen < 3)
+	if (pci->max_link_speed < 3)
 		return 0;
 
 	/* Send PME_TURN_OFF message */
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
index f0f3ebd1a033..00ad4832f2cf 100644
--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -141,10 +141,10 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
 	}
 
 	/*
-	 * Require direct speed change with retrying here if the link_gen is
-	 * PCIe Gen2 or higher.
+	 * Require direct speed change with retrying here if the max_link_speed
+	 * is PCIe Gen2 or higher.
 	 */
-	changes = min_not_zero(dw->link_gen, RCAR_MAX_LINK_SPEED) - 1;
+	changes = min_not_zero(dw->max_link_speed, RCAR_MAX_LINK_SPEED) - 1;
 
 	/*
 	 * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.

-- 
2.25.1

[PATCH v6 2/4] PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Currently, dw_pcie::max_link_speed has a valid value only if the controller
driver restricts the maximum link speed in the driver or if the platform
does so in the devicetree using the 'max-link-speed' property.

But having the maximum supported link speed of the platform would be
helpful for the vendor drivers to configure any link specific settings.
So in the case of non-valid value in dw_pcie::max_link_speed, just cache
the hardware default value from Link Capability register.

While at it, let's also remove the 'max_link_speed' argument to the
dw_pcie_link_set_max_speed() function since the value can be retrieved
within the function.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-designware.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 86c49ba097c6..0704853daa85 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -687,16 +687,27 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
 
-static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
+static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
 {
 	u32 cap, ctrl2, link_speed;
 	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 
 	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+
+	/*
+	 * Even if the platform doesn't want to limit the maximum link speed,
+	 * just cache the hardware default value so that the vendor drivers can
+	 * use it to do any link specific configuration.
+	 */
+	if (pci->max_link_speed < 0) {
+		pci->max_link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
+		return;
+	}
+
 	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
 	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
 
-	switch (pcie_link_speed[max_link_speed]) {
+	switch (pcie_link_speed[pci->max_link_speed]) {
 	case PCIE_SPEED_2_5GT:
 		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
 		break;
@@ -1058,8 +1069,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 {
 	u32 val;
 
-	if (pci->max_link_speed > 0)
-		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
+	dw_pcie_link_set_max_speed(pci);
 
 	/* Configure Gen1 N_FTS */
 	if (pci->n_fts[0]) {

-- 
2.25.1

[PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Manivannan Sadhasivam via B4 Relay]

From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>

During high data transmission rates such as 16.0 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>
[mani: dropped the code refactoring and minor changes]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 MAINTAINERS                                   |  4 ++-
 drivers/pci/controller/dwc/Kconfig            |  5 +++
 drivers/pci/controller/dwc/Makefile           |  1 +
 drivers/pci/controller/dwc/pcie-designware.h  | 12 +++++++
 drivers/pci/controller/dwc/pcie-qcom-common.c | 45 +++++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-common.h |  8 +++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  4 +++
 drivers/pci/controller/dwc/pcie-qcom.c        |  4 +++
 8 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..3cfb6068b9f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2728,7 +2728,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.c
+F:	drivers/pci/controller/dwc/pcie-qcom*
 F:	drivers/phy/qualcomm/
 F:	drivers/power/*/msm*
 F:	drivers/reset/reset-qcom-*
@@ -17757,6 +17757,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
@@ -17793,6 +17794,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..a8308d9ea986 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
+obj-$(CONFIG_PCIE_QCOM_COMMON) += pcie-qcom-common.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 22765564f301..51744ad25575 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
new file mode 100644
index 000000000000..dc7d93db9dc5
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/pci.h>
+
+#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);
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..259e04b7bdf9
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "pcie-designware.h"
+
+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 236229f66c80..af83470216e8 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
@@ -486,6 +487,9 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
+	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
+		qcom_pcie_common_set_16gt_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 0180edf3310e..2742e82fdcb3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -35,6 +35,7 @@
 
 #include "../../pci.h"
 #include "pcie-designware.h"
+#include "pcie-qcom-common.h"
 
 /* PARF registers */
 #define PARF_SYS_CTRL				0x00
@@ -283,6 +284,9 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
+	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
+		qcom_pcie_common_set_16gt_eq_settings(pci);
+
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
 		pcie->cfg->ops->ltssm_enable(pcie);

-- 
2.25.1

[PATCH v6 4/4] PCI: qcom: Add RX margining settings for 16.0 GT/s

[Manivannan Sadhasivam via B4 Relay]

From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>

Add RX lane margining settings for 16.0 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>
[mani: dropped the code refactoring and minor changes]
Signed-off-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 51744ad25575..f5be99731f7e 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.0 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 dc7d93db9dc5..99b75e7f085d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
@@ -43,3 +43,34 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
 	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
 }
 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);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
index 259e04b7bdf9..e9ddc901082e 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-common.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
@@ -6,3 +6,4 @@
 #include "pcie-designware.h"
 
 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 af83470216e8..5c220f2ecafe 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -487,8 +487,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
-	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
+	if (pcie_link_speed[pci->max_link_speed] == 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 2742e82fdcb3..b0b1d8d34279 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -284,8 +284,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
-	if (pcie_link_speed[pci->max_link_speed] == PCIE_SPEED_16_0GT)
+	if (pcie_link_speed[pci->max_link_speed] == 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.25.1

Re: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[Johan Hovold]

On Wed, Sep 04, 2024 at 12:41:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> 'link_gen' field is now holding the maximum supported link speed set either
> by the controller driver or by DT through 'max-link-speed' property.
> 
> But the name 'link_gen' sounds like the negotiated link speed of the PCIe
> link. So let's rename it to 'max_link_speed' to make it clear that it holds
> the maximum supported link speed of the controller.
> 
> NOTE: For the sake of clarity, I've used 'max_link_speed' instead of
> 'max_link_gen'. Also the link speed and link generation values map 1:1.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Re: [PATCH v6 2/4] PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

[Johan Hovold]

On Wed, Sep 04, 2024 at 12:41:58PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Currently, dw_pcie::max_link_speed has a valid value only if the controller
> driver restricts the maximum link speed in the driver or if the platform
> does so in the devicetree using the 'max-link-speed' property.
> 
> But having the maximum supported link speed of the platform would be
> helpful for the vendor drivers to configure any link specific settings.
> So in the case of non-valid value in dw_pcie::max_link_speed, just cache
> the hardware default value from Link Capability register.
> 
> While at it, let's also remove the 'max_link_speed' argument to the
> dw_pcie_link_set_max_speed() function since the value can be retrieved
> within the function.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 86c49ba097c6..0704853daa85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -687,16 +687,27 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>  
> -static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
> +static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  {
>  	u32 cap, ctrl2, link_speed;
>  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>  
>  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +
> +	/*
> +	 * Even if the platform doesn't want to limit the maximum link speed,
> +	 * just cache the hardware default value so that the vendor drivers can
> +	 * use it to do any link specific configuration.
> +	 */
> +	if (pci->max_link_speed < 0) {

This should be 

	if (pci->max_link_speed < 1) {

but the patch works as-is because of the default case in the switch
below which falls back to PCI_EXP_LNKCAP_SLS.

> +		pci->max_link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> +		return;
> +	}
> +
>  	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
>  	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
>  
> -	switch (pcie_link_speed[max_link_speed]) {
> +	switch (pcie_link_speed[pci->max_link_speed]) {
>  	case PCIE_SPEED_2_5GT:
>  		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
>  		break;

> @@ -1058,8 +1069,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  {
>  	u32 val;
>  
> -	if (pci->max_link_speed > 0)
> -		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
> +	dw_pcie_link_set_max_speed(pci);

With the above fixed:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Johan Hovold]

On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> 
> During high data transmission rates such as 16.0 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>
> [mani: dropped the code refactoring and minor changes]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
 
> +#define GEN3_EQ_CONTROL_OFF			0x8a8

Nit: uppercase hex since that's what is used for the other offsets

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

Nit: odd indentation uses spaces, uppercase

> +#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
> new file mode 100644
> index 000000000000..dc7d93db9dc5
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/pci.h>
> +
> +#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.

*The* RATE_SHADOW_SEL bit field

*the* data rate

s/this/these/

> +	 */
> +	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);

How does 0x1 map to gen4/16 GT?

> +	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);
> 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..259e04b7bdf9
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include "pcie-designware.h"

You only need a forward declaration:

	struct dw_pcie;

> +
> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);

Compile guard still missing.

Johan

Re: [PATCH v6 4/4] PCI: qcom: Add RX margining settings for 16.0 GT/s

[Johan Hovold]

On Wed, Sep 04, 2024 at 12:42:00PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> 
> Add RX lane margining settings for 16.0 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>
> [mani: dropped the code refactoring and minor changes]
> Signed-off-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 51744ad25575..f5be99731f7e 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.0 GT/s (GEN4) lane margining register definitions

nit: Gen 4?

> + */
> +#define GEN4_LANE_MARGINING_1_OFF		0xb80

nit: upper case hex

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

Same here

> +#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 dc7d93db9dc5..99b75e7f085d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> @@ -43,3 +43,34 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>  	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
>  }
>  EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
> +
> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)

I'd try to find a shorter symbol name here, "settings" seems redundant
after "set". Perhaps just

	qcom_pcie_common_enable_lane_margining()

or

	qcom_pcie_common_enable_16gt_lane_margining()?

if these settings are indeed specific to 16 GT/s. But perhaps it's
better to let the helper honour pci->max_link_speed if different
settings will later be needed for higher speeds:

	if (pcie_link_speed[pci->max_link_speed] >= PCIE_SPEED_16_0GT)
		qcom_pcie_common_enable_lane_margining(pci)

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

And maybe something similar for the eq settings for symmetry.

Johan

Re: [PATCH v6 0/4] PCI: qcom: Add 16.0 GT/s equalization and margining settings

[Johan Hovold]

On Wed, Sep 04, 2024 at 12:41:56PM +0530, Manivannan Sadhasivam via B4 Relay wrote:

> This series adds 16.0 GT/s specific equalization and RX lane margining settings
> to the Qcom RC and EP drivers. This series is mandatory for the stable operation
> of the PCIe link at 16.0 GT/s on the Qcom platforms.

> Changes in v6:
> 
> - Dropped the code refactoring patch as suggested by Johan
> - Added 2 patches to fix the caching of maximum supported link speed value that
>   is needed to apply the equalization/margining settings
> - Updated the commit message of patch 3 as per Bjorn's suggestion

Thanks for the update. This series fixes the NVMe link errors on the
x1e80100 CRD:

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH v6 2/4] PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

[Manivannan Sadhasivam]

On Wed, Sep 04, 2024 at 11:30:08AM +0200, Johan Hovold wrote:
> On Wed, Sep 04, 2024 at 12:41:58PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Currently, dw_pcie::max_link_speed has a valid value only if the controller
> > driver restricts the maximum link speed in the driver or if the platform
> > does so in the devicetree using the 'max-link-speed' property.
> > 
> > But having the maximum supported link speed of the platform would be
> > helpful for the vendor drivers to configure any link specific settings.
> > So in the case of non-valid value in dw_pcie::max_link_speed, just cache
> > the hardware default value from Link Capability register.
> > 
> > While at it, let's also remove the 'max_link_speed' argument to the
> > dw_pcie_link_set_max_speed() function since the value can be retrieved
> > within the function.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 86c49ba097c6..0704853daa85 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -687,16 +687,27 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
> >  
> > -static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
> > +static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
> >  {
> >  	u32 cap, ctrl2, link_speed;
> >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >  
> >  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> > +
> > +	/*
> > +	 * Even if the platform doesn't want to limit the maximum link speed,
> > +	 * just cache the hardware default value so that the vendor drivers can
> > +	 * use it to do any link specific configuration.
> > +	 */
> > +	if (pci->max_link_speed < 0) {
> 
> This should be 
> 
> 	if (pci->max_link_speed < 1) {
> 

Well I was trying to catch the error value here because if neither driver nor
platform limits the max link speed, this would have -EINVAL (returned by
of_pci_get_max_link_speed()).

But logically it makes sense to use 'pci->max_link_speed < 1' since anything
below value 1 is an invalid value.

Will change it.

- Mani

> but the patch works as-is because of the default case in the switch
> below which falls back to PCI_EXP_LNKCAP_SLS.
> 
> > +		pci->max_link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> > +		return;
> > +	}
> > +
> >  	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
> >  	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
> >  
> > -	switch (pcie_link_speed[max_link_speed]) {
> > +	switch (pcie_link_speed[pci->max_link_speed]) {
> >  	case PCIE_SPEED_2_5GT:
> >  		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
> >  		break;
> 
> > @@ -1058,8 +1069,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  {
> >  	u32 val;
> >  
> > -	if (pci->max_link_speed > 0)
> > -		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
> > +	dw_pcie_link_set_max_speed(pci);
> 
> With the above fixed:
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

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

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Manivannan Sadhasivam]

On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> > 
> > During high data transmission rates such as 16.0 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>
> > [mani: dropped the code refactoring and minor changes]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>  
> > +#define GEN3_EQ_CONTROL_OFF			0x8a8
> 
> Nit: uppercase hex since that's what is used for the other offsets
> 
> > +#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
> 
> Nit: odd indentation uses spaces, uppercase
> 
> > +#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
> > new file mode 100644
> > index 000000000000..dc7d93db9dc5
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > @@ -0,0 +1,45 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/pci.h>
> > +
> > +#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.
> 
> *The* RATE_SHADOW_SEL bit field
> 
> *the* data rate
> 
> s/this/these/
> 
> > +	 */
> > +	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);
> 
> How does 0x1 map to gen4/16 GT?
> 

I need inputs from Shashank here as I don't know the answer.

- Mani

> > +	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);
> > 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..259e04b7bdf9
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include "pcie-designware.h"
> 
> You only need a forward declaration:
> 
> 	struct dw_pcie;
> 
> > +
> > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
> 
> Compile guard still missing.
> 
> Johan

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

Re: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[Frank Li]

On Wed, Sep 04, 2024 at 12:41:57PM +0530, Manivannan Sadhasivam wrote:
> 'link_gen' field is now holding the maximum supported link speed set either
> by the controller driver or by DT through 'max-link-speed' property.
>
> But the name 'link_gen' sounds like the negotiated link speed of the PCIe
> link. So let's rename it to 'max_link_speed' to make it clear that it holds
> the maximum supported link speed of the controller.
>
> NOTE: For the sake of clarity, I've used 'max_link_speed' instead of
> 'max_link_gen'. Also the link speed and link generation values map 1:1.

Maybe a little confuse is about unit of max_link_speed. the word 'gen'
(1, 2, 3...), we know it PCIe[1,2,3...].  But word "speed" look like should
be some mHz.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c        |  8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++------
>  drivers/pci/controller/dwc/pcie-designware.h |  2 +-
>  drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c  |  6 +++---
>  5 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 964d67756eb2..ef12a4f31740 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -847,12 +847,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
>  	if (ret)
>  		goto err_reset_phy;
>
> -	if (pci->link_gen > 1) {
> +	if (pci->max_link_speed > 1) {
>  		/* Allow faster modes after the link is up */
>  		dw_pcie_dbi_ro_wr_en(pci);
>  		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>  		tmp &= ~PCI_EXP_LNKCAP_SLS;
> -		tmp |= pci->link_gen;
> +		tmp |= pci->max_link_speed;
>  		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>
>  		/*
> @@ -1386,8 +1386,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		imx6_pcie->tx_swing_low = 127;
>
>  	/* Limit link speed */
> -	pci->link_gen = 1;
> -	of_property_read_u32(node, "fsl,max-link-speed", &pci->link_gen);
> +	pci->max_link_speed = 1;
> +	of_property_read_u32(node, "fsl,max-link-speed", &pci->max_link_speed);
>
>  	imx6_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
>  	if (IS_ERR(imx6_pcie->vpcie)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 1b5aba1f0c92..86c49ba097c6 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -166,8 +166,8 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  			return ret;
>  	}
>
> -	if (pci->link_gen < 1)
> -		pci->link_gen = of_pci_get_max_link_speed(np);
> +	if (pci->max_link_speed < 1)
> +		pci->max_link_speed = of_pci_get_max_link_speed(np);
>
>  	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
>
> @@ -687,7 +687,7 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>
> -static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> +static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
>  {
>  	u32 cap, ctrl2, link_speed;
>  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -696,7 +696,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
>  	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
>
> -	switch (pcie_link_speed[link_gen]) {
> +	switch (pcie_link_speed[max_link_speed]) {
>  	case PCIE_SPEED_2_5GT:
>  		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
>  		break;
> @@ -1058,8 +1058,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  {
>  	u32 val;
>
> -	if (pci->link_gen > 0)
> -		dw_pcie_link_set_max_speed(pci, pci->link_gen);
> +	if (pci->max_link_speed > 0)
> +		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
>
>  	/* Configure Gen1 N_FTS */
>  	if (pci->n_fts[0]) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 53c4c8f399c8..22765564f301 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -421,7 +421,7 @@ struct dw_pcie {
>  	u32			type;
>  	unsigned long		caps;
>  	int			num_lanes;
> -	int			link_gen;
> +	int			max_link_speed;
>  	u8			n_fts[2];
>  	struct dw_edma_chip	edma;
>  	struct clk_bulk_data	app_clks[DW_PCIE_NUM_APP_CLKS];
> diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> index acbe4f6d3291..676d2aba4fbd 100644
> --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> @@ -132,7 +132,7 @@ static void intel_pcie_link_setup(struct intel_pcie *pcie)
>
>  static void intel_pcie_init_n_fts(struct dw_pcie *pci)
>  {
> -	switch (pci->link_gen) {
> +	switch (pci->max_link_speed) {
>  	case 3:
>  		pci->n_fts[1] = PORT_AFR_N_FTS_GEN3;
>  		break;
> @@ -252,7 +252,7 @@ static int intel_pcie_wait_l2(struct intel_pcie *pcie)
>  	int ret;
>  	struct dw_pcie *pci = &pcie->pci;
>
> -	if (pci->link_gen < 3)
> +	if (pci->max_link_speed < 3)
>  		return 0;
>
>  	/* Send PME_TURN_OFF message */
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index f0f3ebd1a033..00ad4832f2cf 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -141,10 +141,10 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
>  	}
>
>  	/*
> -	 * Require direct speed change with retrying here if the link_gen is
> -	 * PCIe Gen2 or higher.
> +	 * Require direct speed change with retrying here if the max_link_speed
> +	 * is PCIe Gen2 or higher.
>  	 */
> -	changes = min_not_zero(dw->link_gen, RCAR_MAX_LINK_SPEED) - 1;
> +	changes = min_not_zero(dw->max_link_speed, RCAR_MAX_LINK_SPEED) - 1;
>
>  	/*
>  	 * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.
>
> --
> 2.25.1
>

Re: [PATCH v6 2/4] PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

[Frank Li]

On Wed, Sep 04, 2024 at 12:41:58PM +0530, Manivannan Sadhasivam wrote:
> Currently, dw_pcie::max_link_speed has a valid value only if the controller
> driver restricts the maximum link speed in the driver or if the platform
> does so in the devicetree using the 'max-link-speed' property.
>
> But having the maximum supported link speed of the platform would be
> helpful for the vendor drivers to configure any link specific settings.
> So in the case of non-valid value in dw_pcie::max_link_speed, just cache
> the hardware default value from Link Capability register.
>
> While at it, let's also remove the 'max_link_speed' argument to the
> dw_pcie_link_set_max_speed() function since the value can be retrieved
> within the function.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 86c49ba097c6..0704853daa85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -687,16 +687,27 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
>
> -static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
> +static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  {
>  	u32 cap, ctrl2, link_speed;
>  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>
>  	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +
> +	/*
> +	 * Even if the platform doesn't want to limit the maximum link speed,
> +	 * just cache the hardware default value so that the vendor drivers can
> +	 * use it to do any link specific configuration.
> +	 */
> +	if (pci->max_link_speed < 0) {
> +		pci->max_link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> +		return;
> +	}
> +
>  	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
>  	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
>
> -	switch (pcie_link_speed[max_link_speed]) {
> +	switch (pcie_link_speed[pci->max_link_speed]) {
>  	case PCIE_SPEED_2_5GT:
>  		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
>  		break;
> @@ -1058,8 +1069,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  {
>  	u32 val;
>
> -	if (pci->max_link_speed > 0)
> -		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
> +	dw_pcie_link_set_max_speed(pci);
>
>  	/* Configure Gen1 N_FTS */
>  	if (pci->n_fts[0]) {
>
> --
> 2.25.1
>

Re: [PATCH v6 4/4] PCI: qcom: Add RX margining settings for 16.0 GT/s

[Manivannan Sadhasivam]

On Wed, Sep 04, 2024 at 11:53:42AM +0200, Johan Hovold wrote:
> On Wed, Sep 04, 2024 at 12:42:00PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> > 
> > Add RX lane margining settings for 16.0 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>
> > [mani: dropped the code refactoring and minor changes]
> > Signed-off-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 51744ad25575..f5be99731f7e 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.0 GT/s (GEN4) lane margining register definitions
> 
> nit: Gen 4?
> 
> > + */
> > +#define GEN4_LANE_MARGINING_1_OFF		0xb80
> 
> nit: upper case hex
> 
> > +#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
> 
> Same here
> 
> > +#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 dc7d93db9dc5..99b75e7f085d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > @@ -43,3 +43,34 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
> >  	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
> > +
> > +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
> 
> I'd try to find a shorter symbol name here, "settings" seems redundant
> after "set". Perhaps just
> 
> 	qcom_pcie_common_enable_lane_margining()
> 
> or
> 
> 	qcom_pcie_common_enable_16gt_lane_margining()?
> 

This one looks better. Since lane margining is implemented in the receiver, we
don't really need 'rx' in the function name.

> if these settings are indeed specific to 16 GT/s. But perhaps it's
> better to let the helper honour pci->max_link_speed if different
> settings will later be needed for higher speeds:
> 
> 	if (pcie_link_speed[pci->max_link_speed] >= PCIE_SPEED_16_0GT)
> 		qcom_pcie_common_enable_lane_margining(pci)
> 

I did thought about it during the review, but this setting claims to be for 16
GT/s only. So I wouldn't recommend applying it to other speeds without checking 
with Qcom.

Unfortunately, I'm on vacation for 2 weeks and have limited access to Qcom
internal docs/chat. So won't be able to check it soon. If Shashank could check
it, it is fine. But on the conservative side, let's stick to 16 GT/s only?

- Mani

> >  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);
> 
> And maybe something similar for the eq settings for symmetry.
> 
> Johan

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

Re: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[Manivannan Sadhasivam]

On Wed, Sep 04, 2024 at 11:58:58AM -0400, Frank Li wrote:
> On Wed, Sep 04, 2024 at 12:41:57PM +0530, Manivannan Sadhasivam wrote:
> > 'link_gen' field is now holding the maximum supported link speed set either
> > by the controller driver or by DT through 'max-link-speed' property.
> >
> > But the name 'link_gen' sounds like the negotiated link speed of the PCIe
> > link. So let's rename it to 'max_link_speed' to make it clear that it holds
> > the maximum supported link speed of the controller.
> >
> > NOTE: For the sake of clarity, I've used 'max_link_speed' instead of
> > 'max_link_gen'. Also the link speed and link generation values map 1:1.
> 
> Maybe a little confuse is about unit of max_link_speed. the word 'gen'
> (1, 2, 3...), we know it PCIe[1,2,3...].  But word "speed" look like should
> be some mHz.
> 

Ideally, the DT property should've used the definitions in pci.h, but it ended
up accepting the PCIe Gen version. Still, it is named as 'max-link-speed', so I
wanted to keept the same name for the variable.

- Mani

> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c        |  8 ++++----
> >  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++------
> >  drivers/pci/controller/dwc/pcie-designware.h |  2 +-
> >  drivers/pci/controller/dwc/pcie-intel-gw.c   |  4 ++--
> >  drivers/pci/controller/dwc/pcie-rcar-gen4.c  |  6 +++---
> >  5 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 964d67756eb2..ef12a4f31740 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -847,12 +847,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> >  	if (ret)
> >  		goto err_reset_phy;
> >
> > -	if (pci->link_gen > 1) {
> > +	if (pci->max_link_speed > 1) {
> >  		/* Allow faster modes after the link is up */
> >  		dw_pcie_dbi_ro_wr_en(pci);
> >  		tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> >  		tmp &= ~PCI_EXP_LNKCAP_SLS;
> > -		tmp |= pci->link_gen;
> > +		tmp |= pci->max_link_speed;
> >  		dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> >
> >  		/*
> > @@ -1386,8 +1386,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  		imx6_pcie->tx_swing_low = 127;
> >
> >  	/* Limit link speed */
> > -	pci->link_gen = 1;
> > -	of_property_read_u32(node, "fsl,max-link-speed", &pci->link_gen);
> > +	pci->max_link_speed = 1;
> > +	of_property_read_u32(node, "fsl,max-link-speed", &pci->max_link_speed);
> >
> >  	imx6_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
> >  	if (IS_ERR(imx6_pcie->vpcie)) {
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 1b5aba1f0c92..86c49ba097c6 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -166,8 +166,8 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> >  			return ret;
> >  	}
> >
> > -	if (pci->link_gen < 1)
> > -		pci->link_gen = of_pci_get_max_link_speed(np);
> > +	if (pci->max_link_speed < 1)
> > +		pci->max_link_speed = of_pci_get_max_link_speed(np);
> >
> >  	of_property_read_u32(np, "num-lanes", &pci->num_lanes);
> >
> > @@ -687,7 +687,7 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
> >
> > -static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> > +static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 max_link_speed)
> >  {
> >  	u32 cap, ctrl2, link_speed;
> >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > @@ -696,7 +696,7 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> >  	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
> >  	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
> >
> > -	switch (pcie_link_speed[link_gen]) {
> > +	switch (pcie_link_speed[max_link_speed]) {
> >  	case PCIE_SPEED_2_5GT:
> >  		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
> >  		break;
> > @@ -1058,8 +1058,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  {
> >  	u32 val;
> >
> > -	if (pci->link_gen > 0)
> > -		dw_pcie_link_set_max_speed(pci, pci->link_gen);
> > +	if (pci->max_link_speed > 0)
> > +		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
> >
> >  	/* Configure Gen1 N_FTS */
> >  	if (pci->n_fts[0]) {
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 53c4c8f399c8..22765564f301 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -421,7 +421,7 @@ struct dw_pcie {
> >  	u32			type;
> >  	unsigned long		caps;
> >  	int			num_lanes;
> > -	int			link_gen;
> > +	int			max_link_speed;
> >  	u8			n_fts[2];
> >  	struct dw_edma_chip	edma;
> >  	struct clk_bulk_data	app_clks[DW_PCIE_NUM_APP_CLKS];
> > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > index acbe4f6d3291..676d2aba4fbd 100644
> > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > @@ -132,7 +132,7 @@ static void intel_pcie_link_setup(struct intel_pcie *pcie)
> >
> >  static void intel_pcie_init_n_fts(struct dw_pcie *pci)
> >  {
> > -	switch (pci->link_gen) {
> > +	switch (pci->max_link_speed) {
> >  	case 3:
> >  		pci->n_fts[1] = PORT_AFR_N_FTS_GEN3;
> >  		break;
> > @@ -252,7 +252,7 @@ static int intel_pcie_wait_l2(struct intel_pcie *pcie)
> >  	int ret;
> >  	struct dw_pcie *pci = &pcie->pci;
> >
> > -	if (pci->link_gen < 3)
> > +	if (pci->max_link_speed < 3)
> >  		return 0;
> >
> >  	/* Send PME_TURN_OFF message */
> > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > index f0f3ebd1a033..00ad4832f2cf 100644
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -141,10 +141,10 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
> >  	}
> >
> >  	/*
> > -	 * Require direct speed change with retrying here if the link_gen is
> > -	 * PCIe Gen2 or higher.
> > +	 * Require direct speed change with retrying here if the max_link_speed
> > +	 * is PCIe Gen2 or higher.
> >  	 */
> > -	changes = min_not_zero(dw->link_gen, RCAR_MAX_LINK_SPEED) - 1;
> > +	changes = min_not_zero(dw->max_link_speed, RCAR_MAX_LINK_SPEED) - 1;
> >
> >  	/*
> >  	 * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained.
> >
> > --
> > 2.25.1
> >

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

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Shashank Babu Chinta Venkata]

On 9/4/24 08:52, Manivannan Sadhasivam wrote:
> On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
>> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
>>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>>>
>>> During high data transmission rates such as 16.0 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>
>>> [mani: dropped the code refactoring and minor changes]
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>  
>>> +#define GEN3_EQ_CONTROL_OFF			0x8a8
>>
>> Nit: uppercase hex since that's what is used for the other offsets
>>
>>> +#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
>>
>> Nit: odd indentation uses spaces, uppercase
>>
>>> +#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
>>> new file mode 100644
>>> index 000000000000..dc7d93db9dc5
>>> --- /dev/null
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>>> @@ -0,0 +1,45 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/pci.h>
>>> +
>>> +#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.
>>
>> *The* RATE_SHADOW_SEL bit field
>>
>> *the* data rate
>>
>> s/this/these/
>>
>>> +	 */
>>> +	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);
>>
>> How does 0x1 map to gen4/16 GT?
>>
> 
> I need inputs from Shashank here as I don't know the answer.
> 
> - Mani

GEN3_RELATED_OFF has been repurposed to use with multiple data rates. RATE_SHADOW_SEL_MASK on GEN3_RELATED_OFF value decides the data rate of shadow registers namely GEN3_EQ_* registers. Per documentation 0x0 maps to 8 GT/s, 0x1 maps to 16 GT/s and 0x2 maps to 32 GT/s. 
> 
>>> +	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);
>>> 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..259e04b7bdf9
>>> --- /dev/null
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
>>> @@ -0,0 +1,8 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include "pcie-designware.h"
>>
>> You only need a forward declaration:
>>
>> 	struct dw_pcie;
>>
>>> +
>>> +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
>>
>> Compile guard still missing.
>>
>> Johan
> 

Re: [PATCH v6 4/4] PCI: qcom: Add RX margining settings for 16.0 GT/s

[Shashank Babu Chinta Venkata]

On 9/4/24 09:04, Manivannan Sadhasivam wrote:
> On Wed, Sep 04, 2024 at 11:53:42AM +0200, Johan Hovold wrote:
>> On Wed, Sep 04, 2024 at 12:42:00PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
>>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
>>>
>>> Add RX lane margining settings for 16.0 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>
>>> [mani: dropped the code refactoring and minor changes]
>>> Signed-off-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 51744ad25575..f5be99731f7e 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.0 GT/s (GEN4) lane margining register definitions
>>
>> nit: Gen 4?
>>
>>> + */
>>> +#define GEN4_LANE_MARGINING_1_OFF		0xb80
>>
>> nit: upper case hex
>>
>>> +#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
>>
>> Same here
>>
>>> +#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 dc7d93db9dc5..99b75e7f085d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
>>> @@ -43,3 +43,34 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
>>>  	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
>>>  }
>>>  EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
>>> +
>>> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
>>
>> I'd try to find a shorter symbol name here, "settings" seems redundant
>> after "set". Perhaps just
>>
>> 	qcom_pcie_common_enable_lane_margining()
>>
>> or
>>
>> 	qcom_pcie_common_enable_16gt_lane_margining()?
>>
> 
> This one looks better. Since lane margining is implemented in the receiver, we
> don't really need 'rx' in the function name.
> 
>> if these settings are indeed specific to 16 GT/s. But perhaps it's
>> better to let the helper honour pci->max_link_speed if different
>> settings will later be needed for higher speeds:
>>
>> 	if (pcie_link_speed[pci->max_link_speed] >= PCIE_SPEED_16_0GT)
>> 		qcom_pcie_common_enable_lane_margining(pci)
>>
> 
> I did thought about it during the review, but this setting claims to be for 16
> GT/s only. So I wouldn't recommend applying it to other speeds without checking 
> with Qcom.
> 
> Unfortunately, I'm on vacation for 2 weeks and have limited access to Qcom
> internal docs/chat. So won't be able to check it soon. If Shashank could check
> it, it is fine. But on the conservative side, let's stick to 16 GT/s only?
> 
> - Mani

Yes Mani I think we have to stick to 16 GT/s only for now as we haven't characterized 32 GT/s yet.


> 
>>>  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);
>>
>> And maybe something similar for the eq settings for symmetry.
>>
>> Johan
> 

Re: [PATCH v6 2/4] PCI: dwc: Always cache the maximum link speed value in dw_pcie::max_link_speed

[Johan Hovold]

On Wed, Sep 04, 2024 at 09:19:44PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Sep 04, 2024 at 11:30:08AM +0200, Johan Hovold wrote:
> > On Wed, Sep 04, 2024 at 12:41:58PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> > > +	/*
> > > +	 * Even if the platform doesn't want to limit the maximum link speed,
> > > +	 * just cache the hardware default value so that the vendor drivers can
> > > +	 * use it to do any link specific configuration.
> > > +	 */
> > > +	if (pci->max_link_speed < 0) {
> > 
> > This should be 
> > 
> > 	if (pci->max_link_speed < 1) {
> > 
> 
> Well I was trying to catch the error value here because if neither driver nor
> platform limits the max link speed, this would have -EINVAL (returned by
> of_pci_get_max_link_speed()).

Indeed, I thought I'd traced it do be zero here, but I must have made a
mistake. The old code did check for 0 before calling this function,
though (e.g. in case max_link_speed was never initialised as intended).

> But logically it makes sense to use 'pci->max_link_speed < 1' since anything
> below value 1 is an invalid value.
> 
> Will change it.

Sounds good.

> > > @@ -1058,8 +1069,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > >  {
> > >  	u32 val;
> > >  
> > > -	if (pci->max_link_speed > 0)
> > > -		dw_pcie_link_set_max_speed(pci, pci->max_link_speed);
> > > +	dw_pcie_link_set_max_speed(pci);

Johan

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Johan Hovold]

On Wed, Sep 04, 2024 at 01:46:09PM -0700, Shashank Babu Chinta Venkata wrote:
> On 9/4/24 08:52, Manivannan Sadhasivam wrote:
> > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
> >> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> >>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>

> >>> +	/*
> >>> +	 * 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 |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK, 0x1);
> >>
> >> How does 0x1 map to gen4/16 GT?

> GEN3_RELATED_OFF has been repurposed to use with multiple data rates.
> RATE_SHADOW_SEL_MASK on GEN3_RELATED_OFF value decides the data rate
> of shadow registers namely GEN3_EQ_* registers. Per documentation 0x0
> maps to 8 GT/s, 0x1 maps to 16 GT/s and 0x2 maps to 32 GT/s. 

Thanks for clarifying. Perhaps these should become defines eventually
(or the comment could be extended). There are a lot of "magic" constants
in here.

Johan

Re: [PATCH v6 4/4] PCI: qcom: Add RX margining settings for 16.0 GT/s

[Johan Hovold]

On Wed, Sep 04, 2024 at 01:48:09PM -0700, Shashank Babu Chinta Venkata wrote:
> On 9/4/24 09:04, Manivannan Sadhasivam wrote:
> > On Wed, Sep 04, 2024 at 11:53:42AM +0200, Johan Hovold wrote:
> >> On Wed, Sep 04, 2024 at 12:42:00PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> >>> From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>

> >>> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci)
> >>
> >> I'd try to find a shorter symbol name here, "settings" seems redundant
> >> after "set". Perhaps just
> >>
> >> 	qcom_pcie_common_enable_lane_margining()
> >>
> >> or
> >>
> >> 	qcom_pcie_common_enable_16gt_lane_margining()?
> >>
> > 
> > This one looks better. Since lane margining is implemented in the receiver, we
> > don't really need 'rx' in the function name.
> > 
> >> if these settings are indeed specific to 16 GT/s. But perhaps it's
> >> better to let the helper honour pci->max_link_speed if different
> >> settings will later be needed for higher speeds:
> >>
> >> 	if (pcie_link_speed[pci->max_link_speed] >= PCIE_SPEED_16_0GT)
> >> 		qcom_pcie_common_enable_lane_margining(pci)
> >>
> > 
> > I did thought about it during the review, but this setting claims to be for 16
> > GT/s only. So I wouldn't recommend applying it to other speeds without checking 
> > with Qcom.

Yeah, this was more an example of what the code may look like eventually
since IIUC anything above Gen4 will need lane margining.

> > Unfortunately, I'm on vacation for 2 weeks and have limited access to Qcom
> > internal docs/chat. So won't be able to check it soon. If Shashank could check
> > it, it is fine. But on the conservative side, let's stick to 16 GT/s only?

> Yes Mani I think we have to stick to 16 GT/s only for now as we
> haven't characterized 32 GT/s yet.

Sounds good. We can always generalise or rename these functions later
(e.g. when adding support for higher speeds).

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

Perhaps just dropping "_settings" (and replacing "_rx") is enough for
now? So something like:

	qcom_pcie_common_set_16gt_equalization()
	qcom_pcie_common_set_16gt_lane_margining()

A bit shorter and pretty self-explaining.

Johan

Re: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[kernel test robot]

Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 47ac09b91befbb6a235ab620c32af719f8208399]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-dwc-Rename-dw_pcie-link_gen-to-dw_pcie-max_link_speed/20240904-151354
base:   47ac09b91befbb6a235ab620c32af719f8208399
patch link:    https://lore.kernel.org/r/20240904-pci-qcom-gen4-stability-v6-1-ec39f7ae3f62%40linaro.org
patch subject: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20240905/202409051650.UA8WoHFz-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240905/202409051650.UA8WoHFz-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/202409051650.UA8WoHFz-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/dwc/pcie-spear13xx.c: In function 'spear13xx_pcie_probe':
>> drivers/pci/controller/dwc/pcie-spear13xx.c:236:20: error: 'struct dw_pcie' has no member named 'link_gen'
     236 |                 pci->link_gen = 1;
         |                    ^~


vim +236 drivers/pci/controller/dwc/pcie-spear13xx.c

442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  190  
a43f32d64727302 drivers/pci/host/pcie-spear13xx.c           Matwey V. Kornilov     2015-02-19  191  static int spear13xx_pcie_probe(struct platform_device *pdev)
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  192  {
6a43a425a074afc drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  193  	struct device *dev = &pdev->dev;
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  194  	struct dw_pcie *pci;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  195  	struct spear13xx_pcie *spear13xx_pcie;
6a43a425a074afc drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  196  	struct device_node *np = dev->of_node;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  197  	int ret;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  198  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  199  	spear13xx_pcie = devm_kzalloc(dev, sizeof(*spear13xx_pcie), GFP_KERNEL);
20f9ece101d8793 drivers/pci/host/pcie-spear13xx.c           Jingoo Han             2014-11-12  200  	if (!spear13xx_pcie)
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  201  		return -ENOMEM;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  202  
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  203  	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  204  	if (!pci)
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  205  		return -ENOMEM;
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  206  
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  207  	pci->dev = dev;
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  208  	pci->ops = &dw_pcie_ops;
442ec4c04d1235f drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  209  
c0464062bfea9cd drivers/pci/dwc/pcie-spear13xx.c            Guenter Roeck          2017-02-25  210  	spear13xx_pcie->pci = pci;
c0464062bfea9cd drivers/pci/dwc/pcie-spear13xx.c            Guenter Roeck          2017-02-25  211  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  212  	spear13xx_pcie->phy = devm_phy_get(dev, "pcie-phy");
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  213  	if (IS_ERR(spear13xx_pcie->phy)) {
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  214  		ret = PTR_ERR(spear13xx_pcie->phy);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  215  		if (ret == -EPROBE_DEFER)
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  216  			dev_info(dev, "probe deferred\n");
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  217  		else
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  218  			dev_err(dev, "couldn't get pcie-phy\n");
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  219  		return ret;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  220  	}
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  221  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  222  	phy_init(spear13xx_pcie->phy);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  223  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  224  	spear13xx_pcie->clk = devm_clk_get(dev, NULL);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  225  	if (IS_ERR(spear13xx_pcie->clk)) {
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  226  		dev_err(dev, "couldn't get clk for pcie\n");
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  227  		return PTR_ERR(spear13xx_pcie->clk);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  228  	}
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  229  	ret = clk_prepare_enable(spear13xx_pcie->clk);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  230  	if (ret) {
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  231  		dev_err(dev, "couldn't enable clk for pcie\n");
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  232  		return ret;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  233  	}
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  234  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  235  	if (of_property_read_bool(np, "st,pcie-is-gen1"))
39bc5006501cc31 drivers/pci/controller/dwc/pcie-spear13xx.c Rob Herring            2020-08-20 @236  		pci->link_gen = 1;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  237  
9bcf0a6fdc5062e drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  238  	platform_set_drvdata(pdev, spear13xx_pcie);
9bcf0a6fdc5062e drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  239  
ffe82fa66afb19c drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  240  	ret = spear13xx_add_pcie_port(spear13xx_pcie, pdev);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  241  	if (ret < 0)
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  242  		goto fail_clk;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  243  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  244  	return 0;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  245  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  246  fail_clk:
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  247  	clk_disable_unprepare(spear13xx_pcie->clk);
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  248  
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  249  	return ret;
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  250  }
51b66a6ce12570e drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  251  

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

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Manivannan Sadhasivam]

On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
> On Wed, Sep 04, 2024 at 12:41:59PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> > 
> > During high data transmission rates such as 16.0 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>
> > [mani: dropped the code refactoring and minor changes]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[...]

> > 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..259e04b7bdf9
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include "pcie-designware.h"
> 
> You only need a forward declaration:
> 
> 	struct dw_pcie;
> 
> > +
> > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
> 
> Compile guard still missing.
> 

Perhaps we can just get rid of the Kconfig entry and build it by default for
both RC and EP drivers? I don't see a value in building it as a separate module.
And we may also move more common code in the future.

- Mani

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

Re: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'

[kernel test robot]

Hi Manivannan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 47ac09b91befbb6a235ab620c32af719f8208399]

url:    https://github.com/intel-lab-lkp/linux/commits/Manivannan-Sadhasivam-via-B4-Relay/PCI-dwc-Rename-dw_pcie-link_gen-to-dw_pcie-max_link_speed/20240904-151354
base:   47ac09b91befbb6a235ab620c32af719f8208399
patch link:    https://lore.kernel.org/r/20240904-pci-qcom-gen4-stability-v6-1-ec39f7ae3f62%40linaro.org
patch subject: [PATCH v6 1/4] PCI: dwc: Rename 'dw_pcie::link_gen' to 'dw_pcie::max_link_speed'
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240906/202409060041.zY2BEBq7-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/20240906/202409060041.zY2BEBq7-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/202409060041.zY2BEBq7-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-spear13xx.c:236:8: error: no member named 'link_gen' in 'struct dw_pcie'
     236 |                 pci->link_gen = 1;
         |                 ~~~  ^
   1 error generated.


vim +236 drivers/pci/controller/dwc/pcie-spear13xx.c

442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  190  
a43f32d6472730 drivers/pci/host/pcie-spear13xx.c           Matwey V. Kornilov     2015-02-19  191  static int spear13xx_pcie_probe(struct platform_device *pdev)
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  192  {
6a43a425a074af drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  193  	struct device *dev = &pdev->dev;
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  194  	struct dw_pcie *pci;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  195  	struct spear13xx_pcie *spear13xx_pcie;
6a43a425a074af drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  196  	struct device_node *np = dev->of_node;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  197  	int ret;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  198  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  199  	spear13xx_pcie = devm_kzalloc(dev, sizeof(*spear13xx_pcie), GFP_KERNEL);
20f9ece101d879 drivers/pci/host/pcie-spear13xx.c           Jingoo Han             2014-11-12  200  	if (!spear13xx_pcie)
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  201  		return -ENOMEM;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  202  
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  203  	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  204  	if (!pci)
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  205  		return -ENOMEM;
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  206  
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  207  	pci->dev = dev;
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  208  	pci->ops = &dw_pcie_ops;
442ec4c04d1235 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  209  
c0464062bfea9c drivers/pci/dwc/pcie-spear13xx.c            Guenter Roeck          2017-02-25  210  	spear13xx_pcie->pci = pci;
c0464062bfea9c drivers/pci/dwc/pcie-spear13xx.c            Guenter Roeck          2017-02-25  211  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  212  	spear13xx_pcie->phy = devm_phy_get(dev, "pcie-phy");
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  213  	if (IS_ERR(spear13xx_pcie->phy)) {
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  214  		ret = PTR_ERR(spear13xx_pcie->phy);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  215  		if (ret == -EPROBE_DEFER)
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  216  			dev_info(dev, "probe deferred\n");
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  217  		else
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  218  			dev_err(dev, "couldn't get pcie-phy\n");
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  219  		return ret;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  220  	}
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  221  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  222  	phy_init(spear13xx_pcie->phy);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  223  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  224  	spear13xx_pcie->clk = devm_clk_get(dev, NULL);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  225  	if (IS_ERR(spear13xx_pcie->clk)) {
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  226  		dev_err(dev, "couldn't get clk for pcie\n");
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  227  		return PTR_ERR(spear13xx_pcie->clk);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  228  	}
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  229  	ret = clk_prepare_enable(spear13xx_pcie->clk);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  230  	if (ret) {
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  231  		dev_err(dev, "couldn't enable clk for pcie\n");
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  232  		return ret;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  233  	}
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  234  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  235  	if (of_property_read_bool(np, "st,pcie-is-gen1"))
39bc5006501cc3 drivers/pci/controller/dwc/pcie-spear13xx.c Rob Herring            2020-08-20 @236  		pci->link_gen = 1;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  237  
9bcf0a6fdc5062 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  238  	platform_set_drvdata(pdev, spear13xx_pcie);
9bcf0a6fdc5062 drivers/pci/dwc/pcie-spear13xx.c            Kishon Vijay Abraham I 2017-02-15  239  
ffe82fa66afb19 drivers/pci/host/pcie-spear13xx.c           Bjorn Helgaas          2016-10-06  240  	ret = spear13xx_add_pcie_port(spear13xx_pcie, pdev);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  241  	if (ret < 0)
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  242  		goto fail_clk;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  243  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  244  	return 0;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  245  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  246  fail_clk:
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  247  	clk_disable_unprepare(spear13xx_pcie->clk);
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  248  
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  249  	return ret;
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  250  }
51b66a6ce12570 drivers/pci/host/pcie-spear13xx.c           Pratyush Anand         2014-02-11  251  

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

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Johan Hovold]

On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:

> > > 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..259e04b7bdf9
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > > @@ -0,0 +1,8 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > > + */
> > > +
> > > +#include "pcie-designware.h"
> > 
> > You only need a forward declaration:
> > 
> > 	struct dw_pcie;
> > 
> > > +
> > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
> > 
> > Compile guard still missing.

Sorry, I meant to say *include* guard here.
 
> Perhaps we can just get rid of the Kconfig entry and build it by default for
> both RC and EP drivers? I don't see a value in building it as a separate module.
> And we may also move more common code in the future.

It is already built by default for both drivers. I'm not sure what
you're suggesting here.

Johan

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Manivannan Sadhasivam]

On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote:
> On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Sep 04, 2024 at 11:39:09AM +0200, Johan Hovold wrote:
> 
> > > > 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..259e04b7bdf9
> > > > --- /dev/null
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > > > @@ -0,0 +1,8 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > > > + */
> > > > +
> > > > +#include "pcie-designware.h"
> > > 
> > > You only need a forward declaration:
> > > 
> > > 	struct dw_pcie;
> > > 
> > > > +
> > > > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
> > > 
> > > Compile guard still missing.
> 
> Sorry, I meant to say *include* guard here.
>

Okay. I got confused initially.
  
> > Perhaps we can just get rid of the Kconfig entry and build it by default for
> > both RC and EP drivers? I don't see a value in building it as a separate module.
> > And we may also move more common code in the future.
> 
> It is already built by default for both drivers. I'm not sure what
> you're suggesting here.
> 

Right now it is selected by both drivers using a Kconfig symbol. But I'm
thinking of building it by default as below:

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

A separate Kconfig symbol is not really needed here as this file contains common
code required by both the drivers.

- Mani

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

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Johan Hovold]

On Thu, Sep 05, 2024 at 11:04:37PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote:
> > On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
 
> > > Perhaps we can just get rid of the Kconfig entry and build it by default for
> > > both RC and EP drivers? I don't see a value in building it as a separate module.
> > > And we may also move more common code in the future.
> > 
> > It is already built by default for both drivers. I'm not sure what
> > you're suggesting here.
> 
> Right now it is selected by both drivers using a Kconfig symbol. But I'm
> thinking of building it by default as below:
> 
> -obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> -obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> +obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o pcie-qcom-common.o
> +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o pcie-qcom-common.o
> 
> A separate Kconfig symbol is not really needed here as this file contains common
> code required by both the drivers.

But the separate Kconfig symbol will only be enabled via either PCI
driver's option (e.g. can't be enabled on its own).

I'm also not sure if the above works if you build one driver as a module
and the other into the kernel (yes, I still intend to resubmit my patch
for making the rc driver modular).

Johan

Re: [PATCH v6 3/4] PCI: qcom: Add equalization settings for 16.0 GT/s

[Manivannan Sadhasivam]

On Fri, Sep 06, 2024 at 08:49:03AM +0200, Johan Hovold wrote:
> On Thu, Sep 05, 2024 at 11:04:37PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Sep 05, 2024 at 06:27:36PM +0200, Johan Hovold wrote:
> > > On Thu, Sep 05, 2024 at 08:57:42PM +0530, Manivannan Sadhasivam wrote:
>  
> > > > Perhaps we can just get rid of the Kconfig entry and build it by default for
> > > > both RC and EP drivers? I don't see a value in building it as a separate module.
> > > > And we may also move more common code in the future.
> > > 
> > > It is already built by default for both drivers. I'm not sure what
> > > you're suggesting here.
> > 
> > Right now it is selected by both drivers using a Kconfig symbol. But I'm
> > thinking of building it by default as below:
> > 
> > -obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > -obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> > +obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o pcie-qcom-common.o
> > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o pcie-qcom-common.o
> > 
> > A separate Kconfig symbol is not really needed here as this file contains common
> > code required by both the drivers.
> 
> But the separate Kconfig symbol will only be enabled via either PCI
> driver's option (e.g. can't be enabled on its own).
> 

True. But since the common file is required by both drivers, I thought of just
building it by default. But looking at your below reply, it won't be possible.

> I'm also not sure if the above works if you build one driver as a module
> and the other into the kernel (yes, I still intend to resubmit my patch
> for making the rc driver modular).
> 

Hmm, I thought you dropped that patch ;) Anyway, if that happens, it will be a
problem. I'll keep it as it is.

- Mani

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