[PATCH v3 00/11] Multiple fixes in PCIe qcom driver

[PATCH v3 00/11] Multiple fixes in PCIe qcom driver

[Ansuel Smith]

This contains multiple fix for PCIe qcom driver.
Some optional reset and clocks were missing.
Fix a problem with no PARF programming that cause kernel lock on load.
Add support to force gen 1 speed if needed. (due to hardware limitation)
Add ipq8064 rev 2 support that use a different tx termination offset.

v3:
* Fix check reported by checkpatch --strict
* Rename force_gen1 to gen
* Fix spelling error
* Better describe qcom_clear_and_set_dword
* Make PARF deemph and equalization configurable

v2:
* Drop iATU programming (already done in pcie init)
* Use max-link-speed instead of force-gen1 custom definition
* Drop MRRS to 256B (Can't find a realy reason why this was suggested)
* Introduce a new variant for different revision of ipq8064

Abhishek Sahu (1):
  PCI: qcom: change duplicate PCI reset to phy reset

Ansuel Smith (8):
  PCI: qcom: add missing ipq806x clocks in PCIe driver
  devicetree: bindings: pci: add missing clks to qcom,pcie
  PCI: qcom: add missing reset for ipq806x
  devicetree: bindings: pci: add ext reset to qcom,pcie
  PCI: qcom: introduce qcom_clear_and_set_dword
  PCI: qcom: add support for defining some PARF params
  devicetree: bindings: pci: document PARF params bindings
  devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie

Sham Muthayyan (2):
  PCI: qcom: add ipq8064 rev2 variant and set tx term offset
  PCI: qcom: add Force GEN1 support

 .../devicetree/bindings/pci/qcom,pcie.txt     |  51 +++-
 drivers/pci/controller/dwc/pcie-qcom.c        | 241 ++++++++++++------
 2 files changed, 211 insertions(+), 81 deletions(-)

-- 
2.25.1

[PATCH v3 01/11] PCI: qcom: add missing ipq806x clocks in PCIe driver

[Ansuel Smith]

Aux and Ref clk are missing in PCIe qcom driver.
Add support in the driver to fix PCIe initialization in ipq806x.

Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v4.5+
---
 drivers/pci/controller/dwc/pcie-qcom.c | 44 ++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 5ea527a6bd9f..2a39dfdccfc8 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -88,6 +88,8 @@ struct qcom_pcie_resources_2_1_0 {
 	struct clk *iface_clk;
 	struct clk *core_clk;
 	struct clk *phy_clk;
+	struct clk *aux_clk;
+	struct clk *ref_clk;
 	struct reset_control *pci_reset;
 	struct reset_control *axi_reset;
 	struct reset_control *ahb_reset;
@@ -246,6 +248,14 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->phy_clk))
 		return PTR_ERR(res->phy_clk);
 
+	res->aux_clk = devm_clk_get_optional(dev, "aux");
+	if (IS_ERR(res->aux_clk))
+		return PTR_ERR(res->aux_clk);
+
+	res->ref_clk = devm_clk_get_optional(dev, "ref");
+	if (IS_ERR(res->ref_clk))
+		return PTR_ERR(res->ref_clk);
+
 	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
 	if (IS_ERR(res->pci_reset))
 		return PTR_ERR(res->pci_reset);
@@ -278,6 +288,8 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->iface_clk);
 	clk_disable_unprepare(res->core_clk);
 	clk_disable_unprepare(res->phy_clk);
+	clk_disable_unprepare(res->aux_clk);
+	clk_disable_unprepare(res->ref_clk);
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
@@ -307,16 +319,32 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_assert_ahb;
 	}
 
+	ret = clk_prepare_enable(res->core_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable core clock\n");
+		goto err_clk_core;
+	}
+
 	ret = clk_prepare_enable(res->phy_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable phy clock\n");
 		goto err_clk_phy;
 	}
 
-	ret = clk_prepare_enable(res->core_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_core;
+	if (res->aux_clk) {
+		ret = clk_prepare_enable(res->aux_clk);
+		if (ret) {
+			dev_err(dev, "cannot prepare/enable aux clock\n");
+			goto err_clk_aux;
+		}
+	}
+
+	if (res->ref_clk) {
+		ret = clk_prepare_enable(res->ref_clk);
+		if (ret) {
+			dev_err(dev, "cannot prepare/enable ref clock\n");
+			goto err_clk_ref;
+		}
 	}
 
 	ret = reset_control_deassert(res->ahb_reset);
@@ -372,10 +400,14 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	return 0;
 
 err_deassert_ahb:
-	clk_disable_unprepare(res->core_clk);
-err_clk_core:
+	clk_disable_unprepare(res->ref_clk);
+err_clk_ref:
+	clk_disable_unprepare(res->aux_clk);
+err_clk_aux:
 	clk_disable_unprepare(res->phy_clk);
 err_clk_phy:
+	clk_disable_unprepare(res->core_clk);
+err_clk_core:
 	clk_disable_unprepare(res->iface_clk);
 err_assert_ahb:
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-- 
2.25.1

[PATCH v3 02/11] devicetree: bindings: pci: add missing clks to qcom,pcie

[Ansuel Smith]

Document missing clks used in ipq8064 soc.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 981b4de12807..becdbdc0fffa 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -90,6 +90,8 @@
 	Definition: Should contain the following entries
 			- "core"	Clocks the pcie hw block
 			- "phy"		Clocks the pcie PHY block
+			- "aux" 	Clocks the pcie AUX block
+			- "ref" 	Clocks the pcie ref block
 - clock-names:
 	Usage: required for apq8084/ipq4019
 	Value type: <stringlist>
@@ -277,8 +279,10 @@
 				<0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
 		clocks = <&gcc PCIE_A_CLK>,
 			 <&gcc PCIE_H_CLK>,
-			 <&gcc PCIE_PHY_CLK>;
-		clock-names = "core", "iface", "phy";
+			 <&gcc PCIE_PHY_CLK>,
+			 <&gcc PCIE_AUX_CLK>,
+			 <&gcc PCIE_ALT_REF_CLK>;
+		clock-names = "core", "iface", "phy", "aux", "ref";
 		resets = <&gcc PCIE_ACLK_RESET>,
 			 <&gcc PCIE_HCLK_RESET>,
 			 <&gcc PCIE_POR_RESET>,
-- 
2.25.1

[PATCH v3 03/11] PCI: qcom: change duplicate PCI reset to phy reset

[Ansuel Smith]

From: Abhishek Sahu <absahu@codeaurora.org>

The deinit issues reset_control_assert for PCI twice and does not contain
phy reset.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 2a39dfdccfc8..7a8901efc031 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -280,14 +280,14 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
 
+	clk_disable_unprepare(res->phy_clk);
 	reset_control_assert(res->pci_reset);
 	reset_control_assert(res->axi_reset);
 	reset_control_assert(res->ahb_reset);
 	reset_control_assert(res->por_reset);
-	reset_control_assert(res->pci_reset);
+	reset_control_assert(res->phy_reset);
 	clk_disable_unprepare(res->iface_clk);
 	clk_disable_unprepare(res->core_clk);
-	clk_disable_unprepare(res->phy_clk);
 	clk_disable_unprepare(res->aux_clk);
 	clk_disable_unprepare(res->ref_clk);
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -325,12 +325,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_clk_core;
 	}
 
-	ret = clk_prepare_enable(res->phy_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable phy clock\n");
-		goto err_clk_phy;
-	}
-
 	if (res->aux_clk) {
 		ret = clk_prepare_enable(res->aux_clk);
 		if (ret) {
@@ -387,6 +381,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
+	ret = clk_prepare_enable(res->phy_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable phy clock\n");
+		goto err_deassert_ahb;
+	}
+
 	/* wait for clock acquisition */
 	usleep_range(1000, 1500);
 
@@ -404,8 +404,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 err_clk_ref:
 	clk_disable_unprepare(res->aux_clk);
 err_clk_aux:
-	clk_disable_unprepare(res->phy_clk);
-err_clk_phy:
 	clk_disable_unprepare(res->core_clk);
 err_clk_core:
 	clk_disable_unprepare(res->iface_clk);
-- 
2.25.1

[PATCH v3 04/11] PCI: qcom: add missing reset for ipq806x

[Ansuel Smith]

Add missing ext reset used by ipq8064 SoC in PCIe qcom driver.

Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v4.5+
---
 drivers/pci/controller/dwc/pcie-qcom.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 7a8901efc031..921030a64bab 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -95,6 +95,7 @@ struct qcom_pcie_resources_2_1_0 {
 	struct reset_control *ahb_reset;
 	struct reset_control *por_reset;
 	struct reset_control *phy_reset;
+	struct reset_control *ext_reset;
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
 };
 
@@ -272,6 +273,10 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->por_reset))
 		return PTR_ERR(res->por_reset);
 
+	res->ext_reset = devm_reset_control_get_optional_exclusive(dev, "ext");
+	if (IS_ERR(res->ext_reset))
+		return PTR_ERR(res->ext_reset);
+
 	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }
@@ -285,6 +290,7 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
 	reset_control_assert(res->axi_reset);
 	reset_control_assert(res->ahb_reset);
 	reset_control_assert(res->por_reset);
+	reset_control_assert(res->ext_reset);
 	reset_control_assert(res->phy_reset);
 	clk_disable_unprepare(res->iface_clk);
 	clk_disable_unprepare(res->core_clk);
@@ -347,6 +353,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_deassert_ahb;
 	}
 
+	ret = reset_control_deassert(res->ext_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert ext reset\n");
+		goto err_deassert_ahb;
+	}
+
 	/* enable PCIe clocks and resets */
 	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
 	val &= ~BIT(0);
-- 
2.25.1

[PATCH v3 05/11] devicetree: bindings: pci: add ext reset to qcom,pcie

[Ansuel Smith]

Document ext reset used in ipq8064 SoC by qcom PCIe driver.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index becdbdc0fffa..6efcef040741 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -179,6 +179,7 @@
 			- "pwr"			PWR reset
 			- "ahb"			AHB reset
 			- "phy_ahb"		PHY AHB reset
+			- "ext"			EXT reset
 
 - reset-names:
 	Usage: required for ipq8074
@@ -287,8 +288,9 @@
 			 <&gcc PCIE_HCLK_RESET>,
 			 <&gcc PCIE_POR_RESET>,
 			 <&gcc PCIE_PCI_RESET>,
-			 <&gcc PCIE_PHY_RESET>;
-		reset-names = "axi", "ahb", "por", "pci", "phy";
+			 <&gcc PCIE_PHY_RESET>,
+			 <&gcc PCIE_EXT_RESET>;
+		reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
 		pinctrl-0 = <&pcie_pins_default>;
 		pinctrl-names = "default";
 	};
-- 
2.25.1

[PATCH v3 06/11] PCI: qcom: introduce qcom_clear_and_set_dword

[Ansuel Smith]

Use qcom_clear_and_set_dword instead of use the same code many times in
the entire driver.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 108 ++++++++++---------------
 1 file changed, 41 insertions(+), 67 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 921030a64bab..a4fd5baada34 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -184,6 +184,16 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static void qcom_clear_and_set_dword(void __iomem *addr, u32 clear_mask,
+				     u32 set_mask)
+{
+	u32 val = readl(addr);
+
+	val &= ~clear_mask;
+	val |= set_mask;
+	writel(val, addr);
+}
+
 static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
 {
 	gpiod_set_value_cansleep(pcie->reset, 1);
@@ -214,12 +224,9 @@ static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
 
 static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
 {
-	u32 val;
-
 	/* enable link training */
-	val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
-	val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
-	writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+	qcom_clear_and_set_dword(pcie->elbi + PCIE20_ELBI_SYS_CTRL, 0,
+				 PCIE20_ELBI_SYS_CTRL_LT_ENABLE);
 }
 
 static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
@@ -304,7 +311,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	u32 val;
 	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -360,14 +366,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	}
 
 	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
 	/* enable external reference clock */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
-	val |= BIT(16);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK, 0,
+				 BIT(16));
 
 	ret = reset_control_deassert(res->phy_reset);
 	if (ret) {
@@ -514,10 +517,9 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
 	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
-
-		val |= BIT(31);
-		writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+		qcom_clear_and_set_dword(
+			pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT, 0,
+			BIT(31));
 	}
 
 	return 0;
@@ -537,12 +539,8 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
 
 static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
 {
-	u32 val;
-
 	/* enable link training */
-	val = readl(pcie->parf + PCIE20_PARF_LTSSM);
-	val |= BIT(8);
-	writel(val, pcie->parf + PCIE20_PARF_LTSSM);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_LTSSM, 0, BIT(8));
 }
 
 static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
@@ -603,7 +601,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	u32 val;
 	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -637,25 +634,19 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	}
 
 	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
 	/* change DBI base address */
 	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
 
 	/* MAC PHY_POWERDOWN MUX DISABLE  */
-	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
-	val &= ~BIT(29);
-	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_SYS_CTRL, BIT(29), 0);
 
-	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
-	val |= BIT(4);
-	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL,
+				 0, BIT(4));
 
-	val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
-	val |= BIT(31);
-	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+	qcom_clear_and_set_dword(
+		pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2, 0, BIT(31));
 
 	return 0;
 
@@ -792,7 +783,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	u32 val;
 	int ret;
 
 	ret = reset_control_assert(res->axi_m_reset);
@@ -918,25 +908,19 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 		goto err_clks;
 
 	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
 	/* change DBI base address */
 	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
 
 	/* MAC PHY_POWERDOWN MUX DISABLE  */
-	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
-	val &= ~BIT(29);
-	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_SYS_CTRL, BIT(29), 0);
 
-	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
-	val |= BIT(4);
-	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL,
+				 0, BIT(4));
 
-	val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
-	val |= BIT(31);
-	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+	qcom_clear_and_set_dword(
+		pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2, 0, BIT(31));
 
 	return 0;
 
@@ -1017,7 +1001,6 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
 	int i, ret;
-	u32 val;
 
 	for (i = 0; i < ARRAY_SIZE(res->rst); i++) {
 		ret = reset_control_assert(res->rst[i]);
@@ -1077,9 +1060,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 	writel(SLV_ADDR_SPACE_SZ,
 		pcie->parf + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE);
 
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
 	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
 
@@ -1093,9 +1074,8 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 	writel(DBI_RO_WR_EN, pci->dbi_base + PCIE20_MISC_CONTROL_1_REG);
 	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + PCIE20_CAP_LINK_1);
 
-	val = readl(pci->dbi_base + PCIE20_CAP_LINK_CAPABILITIES);
-	val &= ~PCIE20_CAP_ACTIVE_STATE_LINK_PM_SUPPORT;
-	writel(val, pci->dbi_base + PCIE20_CAP_LINK_CAPABILITIES);
+	qcom_clear_and_set_dword(pcie->dbi_base + PCIE20_CAP_LINK_CAPABILITIES,
+				 PCIE20_CAP_ACTIVE_STATE_LINK_PM_SUPPORT, 0);
 
 	writel(PCIE_CAP_CPL_TIMEOUT_DISABLE, pci->dbi_base +
 		PCIE20_DEVICE_CONTROL2_STATUS2);
@@ -1159,7 +1139,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
-	u32 val;
 	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -1196,26 +1175,21 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
 
 	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
 	/* change DBI base address */
 	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
 
 	/* MAC PHY_POWERDOWN MUX DISABLE  */
-	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
-	val &= ~BIT(29);
-	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_SYS_CTRL, BIT(29), 0);
 
-	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
-	val |= BIT(4);
-	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL,
+				 0, BIT(4));
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
-		val |= BIT(31);
-		writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+		qcom_clear_and_set_dword(
+			pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL, 0,
+			BIT(31));
 	}
 
 	return 0;
-- 
2.25.1

[PATCH v3 07/11] PCI: qcom: add support for defining some PARF params

[Ansuel Smith]

Add support for Tx De-Emphasis, Tx Swing and Rx equalization definition
needed on some ipq8064 based device (Netgear R7800 for example). This
cause a total lock of the system on kernel load.

Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v4.5+
---
 drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index a4fd5baada34..da8058fd1925 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -46,6 +46,9 @@
 
 #define PCIE20_PARF_PHY_CTRL			0x40
 #define PCIE20_PARF_PHY_REFCLK			0x4C
+#define PHY_REFCLK_SSP_EN			BIT(16)
+#define PHY_REFCLK_USE_PAD			BIT(12)
+
 #define PCIE20_PARF_DBI_BASE_ADDR		0x168
 #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
 #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
@@ -77,6 +80,18 @@
 #define DBI_RO_WR_EN				1
 
 #define PERST_DELAY_US				1000
+/* PARF registers */
+#define PCIE20_PARF_PCS_DEEMPH			0x34
+#define PCS_DEEMPH_TX_DEEMPH_GEN1(x)		((x) << 16)
+#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x)	((x) << 8)
+#define PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(x)	((x) << 0)
+
+#define PCIE20_PARF_PCS_SWING			0x38
+#define PCS_SWING_TX_SWING_FULL(x)		((x) << 8)
+#define PCS_SWING_TX_SWING_LOW(x)		((x) << 0)
+
+#define PCIE20_PARF_CONFIG_BITS		0x50
+#define PHY_RX0_EQ(x)				((x) << 24)
 
 #define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE	0x358
 #define SLV_ADDR_SPACE_SZ			0x10000000
@@ -97,6 +112,12 @@ struct qcom_pcie_resources_2_1_0 {
 	struct reset_control *phy_reset;
 	struct reset_control *ext_reset;
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_1_0_MAX_SUPPLY];
+	u32 tx_deemph_gen1;
+	u32 tx_deemph_gen2_3p5db;
+	u32 tx_deemph_gen2_6db;
+	u32 tx_swing_full;
+	u32 tx_swing_low;
+	u32 rx0_eq;
 };
 
 struct qcom_pcie_resources_1_0_0 {
@@ -234,8 +255,21 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	struct device_node *node = dev->of_node;
 	int ret;
 
+	of_property_read_u32(node, "qcom,tx-deemph-gen1",
+			     &res->tx_deemph_gen1);
+	of_property_read_u32(node, "qcom,tx-deemph-gen2-3p5db",
+			     &res->tx_deemph_gen2_3p5db);
+	of_property_read_u32(node, "qcom,tx-deemph-gen2-6db",
+			     &res->tx_deemph_gen2_6db);
+	of_property_read_u32(node, "qcom,tx-swing-full",
+			     &res->tx_swing_full);
+	of_property_read_u32(node, "qcom,tx-swing-low",
+			     &res->tx_swing_low);
+	of_property_read_u32(node, "qcom,rx0-eq", &res->rx0_eq);
+
 	res->supplies[0].supply = "vdda";
 	res->supplies[1].supply = "vdda_phy";
 	res->supplies[2].supply = "vdda_refclk";
@@ -368,9 +402,18 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	/* enable PCIe clocks and resets */
 	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
+	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(res->tx_deemph_gen1) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(res->tx_deemph_gen2_3p5db) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(res->tx_deemph_gen2_6db),
+	       pcie->parf + PCIE20_PARF_PCS_DEEMPH);
+	writel(PCS_SWING_TX_SWING_FULL(res->tx_swing_full) |
+	       PCS_SWING_TX_SWING_LOW(res->tx_swing_low),
+	       pcie->parf + PCIE20_PARF_PCS_SWING);
+	writel(PHY_RX0_EQ(res->rx0_eq), pcie->parf + PCIE20_PARF_CONFIG_BITS);
+
 	/* enable external reference clock */
-	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK, 0,
-				 BIT(16));
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_REFCLK,
+				 PHY_REFCLK_USE_PAD, PHY_REFCLK_SSP_EN;
 
 	ret = reset_control_deassert(res->phy_reset);
 	if (ret) {
-- 
2.25.1

[PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[Ansuel Smith]

It is now supported the editing of Tx De-Emphasis, Tx Swing and
Rx equalization params on ipq8064. Document this new optional params.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/pci/qcom,pcie.txt     | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 6efcef040741..8cc5aea8a1da 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -254,6 +254,42 @@
 			- "perst-gpios"	PCIe endpoint reset signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
 
+- qcom,tx-deemph-gen1:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: Gen1 De-emphasis value.
+		    For ipq806x should be set to 24.
+
+- qcom,tx-deemph-gen2-3p5db:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: Gen2 (3.5db) De-emphasis value.
+		    For ipq806x should be set to 24.
+
+- qcom,tx-deemph-gen2-6db:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: Gen2 (6db) De-emphasis value.
+		    For ipq806x should be set to 34.
+
+- qcom,tx-swing-full:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: TX launch amplitude swing_full value.
+		    For ipq806x should be set to 120.
+
+- qcom,tx-swing-low:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: TX launch amplitude swing_low value.
+		    For ipq806x should be set to 120.
+
+- qcom,rx0-eq:
+	Usage: optional (available for ipq/apq8064)
+	Value type: <u32>
+	Definition: RX0 equalization value.
+		    For ipq806x should be set to 4.
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
-- 
2.25.1

[PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[Ansuel Smith]

From: Sham Muthayyan <smuthayy@codeaurora.org>

Add tx term offset support to pcie qcom driver need in some revision of
the ipq806x SoC.
Ipq8064 have tx term offset set to 7.
Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.

Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index da8058fd1925..372d2c8508b5 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -45,6 +45,9 @@
 #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
 
 #define PCIE20_PARF_PHY_CTRL			0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12, 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
+
 #define PCIE20_PARF_PHY_REFCLK			0x4C
 #define PHY_REFCLK_SSP_EN			BIT(16)
 #define PHY_REFCLK_USE_PAD			BIT(12)
@@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
 	u32 tx_swing_full;
 	u32 tx_swing_low;
 	u32 rx0_eq;
+	u8 phy_tx0_term_offset;
 };
 
 struct qcom_pcie_resources_1_0_0 {
@@ -318,6 +322,11 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->ext_reset))
 		return PTR_ERR(res->ext_reset);
 
+	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
+		res->phy_tx0_term_offset = 7;
+	else
+		res->phy_tx0_term_offset = 0;
+
 	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }
@@ -402,6 +411,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	/* enable PCIe clocks and resets */
 	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
 
+	/* set TX termination offset */
+	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL,
+			PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
+			PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
+
 	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(res->tx_deemph_gen1) |
 	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(res->tx_deemph_gen2_3p5db) |
 	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(res->tx_deemph_gen2_6db),
@@ -1485,6 +1499,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
+	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
 	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
 	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
 	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
-- 
2.25.1

[PATCH v3 10/11] devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie

[Ansuel Smith]

Document qcom,pcie-ipq8064-v2 needed to use different phy_tx0_term_offset.
In ipq8064 phy_tx0_term_offset is 7.
In ipq8064 v2 other SoC it's set to 0 by default.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 8cc5aea8a1da..c7102863d855 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -5,6 +5,7 @@
 	Value type: <stringlist>
 	Definition: Value should contain
 			- "qcom,pcie-ipq8064" for ipq8064
+			- "qcom,pcie-ipq8064-v2" for ipq8064 rev 2 or ipq8065
 			- "qcom,pcie-apq8064" for apq8064
 			- "qcom,pcie-apq8084" for apq8084
 			- "qcom,pcie-msm8996" for msm8996 or apq8096
-- 
2.25.1

[PATCH v3 11/11] PCI: qcom: add Force GEN1 support

[Ansuel Smith]

From: Sham Muthayyan <smuthayy@codeaurora.org>

Add Force GEN1 support needed in some ipq8064 board that needs to limit
some PCIe line to gen1 for some hardware limitation.
This is set by the max-link-speed binding and needed by some soc based
on ipq8064. (for example Netgear R7800 router)

Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 372d2c8508b5..a568f28645e7 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#include "../../pci.h"
 #include "pcie-designware.h"
 
 #define PCIE20_PARF_SYS_CTRL			0x00
@@ -99,6 +100,8 @@
 #define PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE	0x358
 #define SLV_ADDR_SPACE_SZ			0x10000000
 
+#define PCIE20_LNK_CONTROL2_LINK_STATUS2	0xa0
+
 #define DEVICE_TYPE_RC				0x4
 
 #define QCOM_PCIE_2_1_0_MAX_SUPPLY	3
@@ -205,6 +208,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	int gen;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
@@ -462,6 +466,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	/* wait for clock acquisition */
 	usleep_range(1000, 1500);
 
+	if (pcie->gen == 1) {
+		qcom_clear_and_set_dword(pci->dbi_base +
+				PCIE20_LNK_CONTROL2_LINK_STATUS2, 0, 1);
+	}
+
 
 	/* Set the Max TLP size to 2K, instead of using default of 4K */
 	writel(CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K,
@@ -1431,6 +1440,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
+	pcie->gen = of_pci_get_max_link_speed(pdev->dev.of_node);
+	if (pcie->gen < 0)
+		pcie->gen = 2;
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
 	pcie->parf = devm_ioremap_resource(dev, res);
 	if (IS_ERR(pcie->parf)) {
-- 
2.25.1

Re: [PATCH v3 00/11] Multiple fixes in PCIe qcom driver

[Bjorn Helgaas]

On Fri, May 01, 2020 at 12:06:07AM +0200, Ansuel Smith wrote:
> This contains multiple fix for PCIe qcom driver.
> Some optional reset and clocks were missing.
> Fix a problem with no PARF programming that cause kernel lock on load.
> Add support to force gen 1 speed if needed. (due to hardware limitation)
> Add ipq8064 rev 2 support that use a different tx termination offset.
> 
> v3:
> * Fix check reported by checkpatch --strict
> * Rename force_gen1 to gen
> * Fix spelling error
> * Better describe qcom_clear_and_set_dword
> * Make PARF deemph and equalization configurable
> 
> v2:
> * Drop iATU programming (already done in pcie init)
> * Use max-link-speed instead of force-gen1 custom definition
> * Drop MRRS to 256B (Can't find a realy reason why this was suggested)
> * Introduce a new variant for different revision of ipq8064
> 
> Abhishek Sahu (1):
>   PCI: qcom: change duplicate PCI reset to phy reset
> 
> Ansuel Smith (8):
>   PCI: qcom: add missing ipq806x clocks in PCIe driver

s/in PCIe driver// (obvious from context)

>   devicetree: bindings: pci: add missing clks to qcom,pcie
>   PCI: qcom: add missing reset for ipq806x
>   devicetree: bindings: pci: add ext reset to qcom,pcie

s/to qcom,pcie// (obvious from context after updating as below)

>   PCI: qcom: introduce qcom_clear_and_set_dword
>   PCI: qcom: add support for defining some PARF params
>   devicetree: bindings: pci: document PARF params bindings
>   devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie

s/to qcom,pcie// (obvious from context after updating as below)

> Sham Muthayyan (2):
>   PCI: qcom: add ipq8064 rev2 variant and set tx term offset
>   PCI: qcom: add Force GEN1 support

Hi Ansuel, if you post this again, would you mind adjusting your
subject lines to match the history, e.g.,

  $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
  604f3956524a PCI: qcom: Fix the fixup of PCI_VENDOR_ID_QCOM
  ed8cc3b1fc84 PCI: qcom: Add support for SDM845 PCIe controller
  64adde31c8e9 PCI: qcom: Ensure that PERST is asserted for at least 100 ms
  ...

  $ git log --oneline Documentation/devicetree/bindings/pci/qcom,pcie.txt
  5d28bee7c91e dt-bindings: PCI: qcom: Add support for SDM845 PCIe
  29a50257a9d6 dt-bindings: PCI: qcom: Add QCS404 to the binding

(Capitalize first word, follow "dt-bindings: PCI: qcom" example).

Some of the commit logs also have random-length short lines.  Please
wrap them to use the entire ~75 column width and add blank lines
between paragraphs.

Add ("..") on the Fixes: lines.  See git log for common practice.  I
use this alias to make them:

  $ type gsr
  gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'
  $ gsr 82a823833f4e
  82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")

Some of the logs use "SoC", some use "soc".  I prefer "SoC" because
"soc" really isn't an English word.

You don't need to post a new version just for these tweaks, but maybe
make them in your local copy so if you do post a v4 for some other
reason, they'll be included.

>  .../devicetree/bindings/pci/qcom,pcie.txt     |  51 +++-
>  drivers/pci/controller/dwc/pcie-qcom.c        | 241 ++++++++++++------
>  2 files changed, 211 insertions(+), 81 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH v3 01/11] PCI: qcom: add missing ipq806x clocks in PCIe driver

[Sasha Levin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver").

The bot has tested the following trees: v5.6.8, v5.4.36, v4.19.119, v4.14.177, v4.9.220.

v5.6.8: Build OK!
v5.4.36: Build OK!
v4.19.119: Build failed! Errors:
    drivers/pci/controller/dwc/pcie-qcom.c:240:17: error: implicit declaration of function ‘devm_clk_get_optional’; did you mean ‘devm_gpiod_get_optional’? [-Werror=implicit-function-declaration]

v4.14.177: Failed to apply! Possible dependencies:
    68e7c15ceb8d ("PCI: qcom: Use regulator bulk api for apq8064 supplies")

v4.9.220: Failed to apply! Possible dependencies:
    11a61a860281 ("PCI: dwc: Use PTR_ERR_OR_ZERO to simplify code")
    19ce01cc8cbc ("PCI: dwc: all: Rename cfg_read/cfg_write to read/write")
    1d77040bde2d ("PCI: layerscape: Add LS1046a support")
    1f6c4501c667 ("PCI: dra7xx: Group PHY API invocations")
    244e00071fd8 ("PCI: qcom: Explicitly request exclusive reset control")
    40f67fb2c384 ("PCI: dwc: designware: Get device pointer at the start of dw_pcie_host_init()")
    442ec4c04d12 ("PCI: dwc: all: Split struct pcie_port into host-only and core structures")
    5d0f1b84c526 ("PCI: qcom: Reorder to put v0 functions together, v1 functions together, etc")
    9bcf0a6fdc50 ("PCI: dwc: all: Use platform_set_drvdata() to save private data")
    ab5fe4f4d31e ("PCI: dra7xx: Add support to force RC to work in GEN1 mode")
    d0491fc39bdd ("PCI: qcom: Add support for MSM8996 PCIe controller")
    e594233803aa ("PCI: layerscape: Remove redundant error message from ls_pcie_probe()")
    ebe85a44aad4 ("PCI: dra7xx: Enable MSI and legacy interrupts simultaneously")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v3 01/11] PCI: qcom: add missing ipq806x clocks in PCIe driver

[Rob Herring]

On Fri, May 01, 2020 at 12:06:08AM +0200, Ansuel Smith wrote:
> Aux and Ref clk are missing in PCIe qcom driver.
> Add support in the driver to fix PCIe initialization in ipq806x.
> 
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v4.5+

Doesn't strike me as stable material. Looks like new h/w enablement.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 44 ++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5ea527a6bd9f..2a39dfdccfc8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -88,6 +88,8 @@ struct qcom_pcie_resources_2_1_0 {
>  	struct clk *iface_clk;
>  	struct clk *core_clk;
>  	struct clk *phy_clk;
> +	struct clk *aux_clk;
> +	struct clk *ref_clk;
>  	struct reset_control *pci_reset;
>  	struct reset_control *axi_reset;
>  	struct reset_control *ahb_reset;
> @@ -246,6 +248,14 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>  	if (IS_ERR(res->phy_clk))
>  		return PTR_ERR(res->phy_clk);
>  
> +	res->aux_clk = devm_clk_get_optional(dev, "aux");
> +	if (IS_ERR(res->aux_clk))
> +		return PTR_ERR(res->aux_clk);
> +
> +	res->ref_clk = devm_clk_get_optional(dev, "ref");
> +	if (IS_ERR(res->ref_clk))
> +		return PTR_ERR(res->ref_clk);

Seems like you'd want to report an error for ipq608x? Based on the 
commit msg, they aren't optional.

> +
>  	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
>  	if (IS_ERR(res->pci_reset))
>  		return PTR_ERR(res->pci_reset);
> @@ -278,6 +288,8 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->iface_clk);
>  	clk_disable_unprepare(res->core_clk);
>  	clk_disable_unprepare(res->phy_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +	clk_disable_unprepare(res->ref_clk);
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> @@ -307,16 +319,32 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  		goto err_assert_ahb;
>  	}
>  
> +	ret = clk_prepare_enable(res->core_clk);

Perhaps use the bulk api.

> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable core clock\n");
> +		goto err_clk_core;
> +	}
> +
>  	ret = clk_prepare_enable(res->phy_clk);
>  	if (ret) {
>  		dev_err(dev, "cannot prepare/enable phy clock\n");
>  		goto err_clk_phy;
>  	}
>  
> -	ret = clk_prepare_enable(res->core_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable core clock\n");
> -		goto err_clk_core;
> +	if (res->aux_clk) {
> +		ret = clk_prepare_enable(res->aux_clk);
> +		if (ret) {
> +			dev_err(dev, "cannot prepare/enable aux clock\n");
> +			goto err_clk_aux;
> +		}
> +	}
> +
> +	if (res->ref_clk) {
> +		ret = clk_prepare_enable(res->ref_clk);
> +		if (ret) {
> +			dev_err(dev, "cannot prepare/enable ref clock\n");
> +			goto err_clk_ref;
> +		}
>  	}
>  
>  	ret = reset_control_deassert(res->ahb_reset);
> @@ -372,10 +400,14 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	return 0;
>  
>  err_deassert_ahb:
> -	clk_disable_unprepare(res->core_clk);
> -err_clk_core:
> +	clk_disable_unprepare(res->ref_clk);
> +err_clk_ref:
> +	clk_disable_unprepare(res->aux_clk);
> +err_clk_aux:
>  	clk_disable_unprepare(res->phy_clk);
>  err_clk_phy:
> +	clk_disable_unprepare(res->core_clk);
> +err_clk_core:
>  	clk_disable_unprepare(res->iface_clk);
>  err_assert_ahb:
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> -- 
> 2.25.1
> 

Re: [PATCH v3 03/11] PCI: qcom: change duplicate PCI reset to phy reset

[Rob Herring]

On Fri,  1 May 2020 00:06:10 +0200, Ansuel Smith wrote:
> From: Abhishek Sahu <absahu@codeaurora.org>
> 
> The deinit issues reset_control_assert for PCI twice and does not contain
> phy reset.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 04/11] PCI: qcom: add missing reset for ipq806x

[Rob Herring]

On Fri,  1 May 2020 00:06:11 +0200, Ansuel Smith wrote:
> Add missing ext reset used by ipq8064 SoC in PCIe qcom driver.
> 
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v4.5+
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 06/11] PCI: qcom: introduce qcom_clear_and_set_dword

[Rob Herring]

On Fri, May 01, 2020 at 12:06:13AM +0200, Ansuel Smith wrote:
> Use qcom_clear_and_set_dword instead of use the same code many times in
> the entire driver.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 108 ++++++++++---------------
>  1 file changed, 41 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 921030a64bab..a4fd5baada34 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -184,6 +184,16 @@ struct qcom_pcie {
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>  
> +static void qcom_clear_and_set_dword(void __iomem *addr, u32 clear_mask,
> +				     u32 set_mask)
> +{
> +	u32 val = readl(addr);
> +
> +	val &= ~clear_mask;
> +	val |= set_mask;
> +	writel(val, addr);
> +}

If we wanted this kind of register accessor in the kernel, then we'd 
have common ones. We don't because it hides the possible need for 
locking on a RMW sequence.

Also, not a fix. Don't mix refactoring with fixes.

Rob

Re: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[Rob Herring]

On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
> It is now supported the editing of Tx De-Emphasis, Tx Swing and
> Rx equalization params on ipq8064. Document this new optional params.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 6efcef040741..8cc5aea8a1da 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -254,6 +254,42 @@
>  			- "perst-gpios"	PCIe endpoint reset signal line
>  			- "wake-gpios"	PCIe endpoint wake signal line
>  
> +- qcom,tx-deemph-gen1:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: Gen1 De-emphasis value.
> +		    For ipq806x should be set to 24.

Unless these need to be tuned per board, then the compatible string for 
ipq806x should imply all these settings.

> +
> +- qcom,tx-deemph-gen2-3p5db:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: Gen2 (3.5db) De-emphasis value.
> +		    For ipq806x should be set to 24.
> +
> +- qcom,tx-deemph-gen2-6db:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: Gen2 (6db) De-emphasis value.
> +		    For ipq806x should be set to 34.
> +
> +- qcom,tx-swing-full:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: TX launch amplitude swing_full value.
> +		    For ipq806x should be set to 120.
> +
> +- qcom,tx-swing-low:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: TX launch amplitude swing_low value.
> +		    For ipq806x should be set to 120.
> +
> +- qcom,rx0-eq:
> +	Usage: optional (available for ipq/apq8064)
> +	Value type: <u32>
> +	Definition: RX0 equalization value.
> +		    For ipq806x should be set to 4.
> +
>  * Example for ipq/apq8064
>  	pcie@1b500000 {
>  		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
> -- 
> 2.25.1
> 

Re: [PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[Rob Herring]

On Fri, May 01, 2020 at 12:06:16AM +0200, Ansuel Smith wrote:
> From: Sham Muthayyan <smuthayy@codeaurora.org>
> 
> Add tx term offset support to pcie qcom driver need in some revision of
> the ipq806x SoC.
> Ipq8064 have tx term offset set to 7.
> Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.
> 
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index da8058fd1925..372d2c8508b5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,6 +45,9 @@
>  #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
>  
>  #define PCIE20_PARF_PHY_CTRL			0x40
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12, 16)
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> +
>  #define PCIE20_PARF_PHY_REFCLK			0x4C
>  #define PHY_REFCLK_SSP_EN			BIT(16)
>  #define PHY_REFCLK_USE_PAD			BIT(12)
> @@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
>  	u32 tx_swing_full;
>  	u32 tx_swing_low;
>  	u32 rx0_eq;
> +	u8 phy_tx0_term_offset;
>  };
>  
>  struct qcom_pcie_resources_1_0_0 {
> @@ -318,6 +322,11 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>  	if (IS_ERR(res->ext_reset))
>  		return PTR_ERR(res->ext_reset);
>  
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
> +		res->phy_tx0_term_offset = 7;

Based on my other comments, you'll want to turn this into match data.

> +	else
> +		res->phy_tx0_term_offset = 0;
> +

Re: [PATCH v3 10/11] devicetree: bindings: pci: add ipq8064 rev 2 variant to qcom,pcie

[Rob Herring]

On Fri,  1 May 2020 00:06:17 +0200, Ansuel Smith wrote:
> Document qcom,pcie-ipq8064-v2 needed to use different phy_tx0_term_offset.
> In ipq8064 phy_tx0_term_offset is 7.
> In ipq8064 v2 other SoC it's set to 0 by default.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

R: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[ansuelsmth]

> On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
> > It is now supported the editing of Tx De-Emphasis, Tx Swing and
> > Rx equalization params on ipq8064. Document this new optional params.
> >
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/pci/qcom,pcie.txt     | 36 +++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > index 6efcef040741..8cc5aea8a1da 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > @@ -254,6 +254,42 @@
> >  			- "perst-gpios"	PCIe endpoint reset signal line
> >  			- "wake-gpios"	PCIe endpoint wake signal line
> >
> > +- qcom,tx-deemph-gen1:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: Gen1 De-emphasis value.
> > +		    For ipq806x should be set to 24.
> 
> Unless these need to be tuned per board, then the compatible string for
> ipq806x should imply all these settings.
> 

It was requested by v2 to make this settings tunable. These don't change are
all the same for every ipq806x SoC. The original implementation had this 
value hardcoded for ipq806x. Should I restore this and drop this patch? 
Anyway thanks for the review. 

> > +
> > +- qcom,tx-deemph-gen2-3p5db:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: Gen2 (3.5db) De-emphasis value.
> > +		    For ipq806x should be set to 24.
> > +
> > +- qcom,tx-deemph-gen2-6db:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: Gen2 (6db) De-emphasis value.
> > +		    For ipq806x should be set to 34.
> > +
> > +- qcom,tx-swing-full:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: TX launch amplitude swing_full value.
> > +		    For ipq806x should be set to 120.
> > +
> > +- qcom,tx-swing-low:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: TX launch amplitude swing_low value.
> > +		    For ipq806x should be set to 120.
> > +
> > +- qcom,rx0-eq:
> > +	Usage: optional (available for ipq/apq8064)
> > +	Value type: <u32>
> > +	Definition: RX0 equalization value.
> > +		    For ipq806x should be set to 4.
> > +
> >  * Example for ipq/apq8064
> >  	pcie@1b500000 {
> >  		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064",
> "snps,dw-pcie";
> > --
> > 2.25.1
> >

Re: [PATCH v3 04/11] PCI: qcom: add missing reset for ipq806x

[Philipp Zabel]

Hi Ansuel,

On Fri, May 01, 2020 at 12:06:11AM +0200, Ansuel Smith wrote:
> Add missing ext reset used by ipq8064 SoC in PCIe qcom driver.
> 
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v4.5+
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7a8901efc031..921030a64bab 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[...]
> @@ -347,6 +353,12 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  		goto err_deassert_ahb;
>  	}
>  
> +	ret = reset_control_deassert(res->ext_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert ext reset\n");
                                     ^
This probably should say "cannot deassert ext reset". Apart from this,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v3 01/11] PCI: qcom: add missing ipq806x clocks in PCIe driver

[Stanimir Varbanov]

Hi Ansuel,

On 5/1/20 1:06 AM, Ansuel Smith wrote:
> Aux and Ref clk are missing in PCIe qcom driver.
> Add support in the driver to fix PCIe initialization in ipq806x.
> 
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v4.5+
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 44 ++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5ea527a6bd9f..2a39dfdccfc8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -88,6 +88,8 @@ struct qcom_pcie_resources_2_1_0 {
>  	struct clk *iface_clk;
>  	struct clk *core_clk;
>  	struct clk *phy_clk;
> +	struct clk *aux_clk;
> +	struct clk *ref_clk;
>  	struct reset_control *pci_reset;
>  	struct reset_control *axi_reset;
>  	struct reset_control *ahb_reset;
> @@ -246,6 +248,14 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>  	if (IS_ERR(res->phy_clk))
>  		return PTR_ERR(res->phy_clk);
>  
> +	res->aux_clk = devm_clk_get_optional(dev, "aux");
> +	if (IS_ERR(res->aux_clk))
> +		return PTR_ERR(res->aux_clk);
> +
> +	res->ref_clk = devm_clk_get_optional(dev, "ref");
> +	if (IS_ERR(res->ref_clk))
> +		return PTR_ERR(res->ref_clk);
> +
>  	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
>  	if (IS_ERR(res->pci_reset))
>  		return PTR_ERR(res->pci_reset);
> @@ -278,6 +288,8 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->iface_clk);
>  	clk_disable_unprepare(res->core_clk);
>  	clk_disable_unprepare(res->phy_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +	clk_disable_unprepare(res->ref_clk);
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> @@ -307,16 +319,32 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  		goto err_assert_ahb;
>  	}
>  
> +	ret = clk_prepare_enable(res->core_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable core clock\n");
> +		goto err_clk_core;
> +	}
> +
>  	ret = clk_prepare_enable(res->phy_clk);
>  	if (ret) {
>  		dev_err(dev, "cannot prepare/enable phy clock\n");
>  		goto err_clk_phy;
>  	}
>  
> -	ret = clk_prepare_enable(res->core_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable core clock\n");
> -		goto err_clk_core;
> +	if (res->aux_clk) {

you don't need this check, clk_prepare_enable handles NULL

> +		ret = clk_prepare_enable(res->aux_clk);
> +		if (ret) {
> +			dev_err(dev, "cannot prepare/enable aux clock\n");
> +			goto err_clk_aux;
> +		}
> +	}
> +
> +	if (res->ref_clk) {

here too

> +		ret = clk_prepare_enable(res->ref_clk);
> +		if (ret) {
> +			dev_err(dev, "cannot prepare/enable ref clock\n");
> +			goto err_clk_ref;
> +		}
>  	}
>  
>  	ret = reset_control_deassert(res->ahb_reset);
> @@ -372,10 +400,14 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	return 0;
>  
>  err_deassert_ahb:
> -	clk_disable_unprepare(res->core_clk);
> -err_clk_core:
> +	clk_disable_unprepare(res->ref_clk);
> +err_clk_ref:
> +	clk_disable_unprepare(res->aux_clk);
> +err_clk_aux:
>  	clk_disable_unprepare(res->phy_clk);
>  err_clk_phy:
> +	clk_disable_unprepare(res->core_clk);
> +err_clk_core:
>  	clk_disable_unprepare(res->iface_clk);
>  err_assert_ahb:
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> 

-- 
regards,
Stan

R: [PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[ansuelsmth]

> On Fri, May 01, 2020 at 12:06:16AM +0200, Ansuel Smith wrote:
> > From: Sham Muthayyan <smuthayy@codeaurora.org>
> >
> > Add tx term offset support to pcie qcom driver need in some revision of
> > the ipq806x SoC.
> > Ipq8064 have tx term offset set to 7.
> > Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.
> >
> > Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> > index da8058fd1925..372d2c8508b5 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -45,6 +45,9 @@
> >  #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
> >
> >  #define PCIE20_PARF_PHY_CTRL			0x40
> > +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12,
> 16)
> > +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> > +
> >  #define PCIE20_PARF_PHY_REFCLK			0x4C
> >  #define PHY_REFCLK_SSP_EN			BIT(16)
> >  #define PHY_REFCLK_USE_PAD			BIT(12)
> > @@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
> >  	u32 tx_swing_full;
> >  	u32 tx_swing_low;
> >  	u32 rx0_eq;
> > +	u8 phy_tx0_term_offset;
> >  };
> >
> >  struct qcom_pcie_resources_1_0_0 {
> > @@ -318,6 +322,11 @@ static int
> qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
> >  	if (IS_ERR(res->ext_reset))
> >  		return PTR_ERR(res->ext_reset);
> >
> > +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
> > +		res->phy_tx0_term_offset = 7;
> 
> Based on my other comments, you'll want to turn this into match data.
> 

I don't understand what you mean here. I really can't think of another way
to set this only for qcom,pci-ipq8064 as ipq8064-v2 and apq8064 use the
same get resource function. Should I create a different get_resources for
the other 2 device?
 
> > +	else
> > +		res->phy_tx0_term_offset = 0;
> > +

Re: R: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[Rob Herring]

On Thu, May 07, 2020 at 09:34:35PM +0200, ansuelsmth@gmail.com wrote:
> > On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
> > > It is now supported the editing of Tx De-Emphasis, Tx Swing and
> > > Rx equalization params on ipq8064. Document this new optional params.
> > >
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > >  .../devicetree/bindings/pci/qcom,pcie.txt     | 36 +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > index 6efcef040741..8cc5aea8a1da 100644
> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > @@ -254,6 +254,42 @@
> > >  			- "perst-gpios"	PCIe endpoint reset signal line
> > >  			- "wake-gpios"	PCIe endpoint wake signal line
> > >
> > > +- qcom,tx-deemph-gen1:
> > > +	Usage: optional (available for ipq/apq8064)
> > > +	Value type: <u32>
> > > +	Definition: Gen1 De-emphasis value.
> > > +		    For ipq806x should be set to 24.
> > 
> > Unless these need to be tuned per board, then the compatible string for
> > ipq806x should imply all these settings.
> > 
> 
> It was requested by v2 to make this settings tunable. These don't change are
> all the same for every ipq806x SoC. The original implementation had this 
> value hardcoded for ipq806x. Should I restore this and drop this patch? 

Yes, please.

Rob

Re: [PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[Stanimir Varbanov]

Hi Ansuel,

On 5/1/20 1:06 AM, Ansuel Smith wrote:
> From: Sham Muthayyan <smuthayy@codeaurora.org>
> 
> Add tx term offset support to pcie qcom driver need in some revision of
> the ipq806x SoC.
> Ipq8064 have tx term offset set to 7.
> Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.
> 
> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index da8058fd1925..372d2c8508b5 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,6 +45,9 @@
>  #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
>  
>  #define PCIE20_PARF_PHY_CTRL			0x40
> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12, 16)

The mask definition is not correct. Should be GENMASK(20, 16)

> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> +
>  #define PCIE20_PARF_PHY_REFCLK			0x4C
>  #define PHY_REFCLK_SSP_EN			BIT(16)
>  #define PHY_REFCLK_USE_PAD			BIT(12)
> @@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
>  	u32 tx_swing_full;
>  	u32 tx_swing_low;
>  	u32 rx0_eq;
> +	u8 phy_tx0_term_offset;
>  };
>  
>  struct qcom_pcie_resources_1_0_0 {
> @@ -318,6 +322,11 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>  	if (IS_ERR(res->ext_reset))
>  		return PTR_ERR(res->ext_reset);
>  
> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
> +		res->phy_tx0_term_offset = 7;

Before your change the phy_tx0_term_offser was 0 for apq8064, but here
you change it to 7, why?

> +	else
> +		res->phy_tx0_term_offset = 0;
> +
>  	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
>  	return PTR_ERR_OR_ZERO(res->phy_reset);
>  }
> @@ -402,6 +411,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	/* enable PCIe clocks and resets */
>  	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
>  
> +	/* set TX termination offset */
> +	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL,
> +			PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,

As the mask definition is incorrect you actually clear 12 to 16 bit in
the register where is another PHY parameter. Is that was intentional?

> +			PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
> +
>  	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(res->tx_deemph_gen1) |
>  	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(res->tx_deemph_gen2_3p5db) |
>  	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(res->tx_deemph_gen2_6db),
> @@ -1485,6 +1499,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
> +	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
>  	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
>  	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
>  	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
> 

-- 
regards,
Stan

Re: R: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[Stanimir Varbanov]

On 5/12/20 6:45 PM, Rob Herring wrote:
> On Thu, May 07, 2020 at 09:34:35PM +0200, ansuelsmth@gmail.com wrote:
>>> On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
>>>> It is now supported the editing of Tx De-Emphasis, Tx Swing and
>>>> Rx equalization params on ipq8064. Document this new optional params.
>>>>
>>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>>> ---
>>>>  .../devicetree/bindings/pci/qcom,pcie.txt     | 36 +++++++++++++++++++
>>>>  1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>> index 6efcef040741..8cc5aea8a1da 100644
>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>> @@ -254,6 +254,42 @@
>>>>  			- "perst-gpios"	PCIe endpoint reset signal line
>>>>  			- "wake-gpios"	PCIe endpoint wake signal line
>>>>
>>>> +- qcom,tx-deemph-gen1:
>>>> +	Usage: optional (available for ipq/apq8064)
>>>> +	Value type: <u32>
>>>> +	Definition: Gen1 De-emphasis value.
>>>> +		    For ipq806x should be set to 24.
>>>
>>> Unless these need to be tuned per board, then the compatible string for
>>> ipq806x should imply all these settings.
>>>
>>
>> It was requested by v2 to make this settings tunable. These don't change are
>> all the same for every ipq806x SoC. The original implementation had this 
>> value hardcoded for ipq806x. Should I restore this and drop this patch?
> 
> Yes, please.

I still think that the values for tx deemph and tx swing should be
tunable. But I can live with them in the driver if they not break
support for apq8064.

The default values in the registers for apq8064 and ipq806x are:

			default		your change
TX_DEEMPH_GEN1		21		24
TX_DEEMPH_GEN2_3_5DB	21		24
TX_DEEMPH_GEN2_6DB	32		34

TX_SWING_FULL		121		120
TX_SWING_LOW		121		120

So until now (without your change) apq8064 worked with default values.

> 
> Rob
> 

-- 
regards,
Stan

R: [PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[ansuelsmth]

> Hi Ansuel,
> 
> On 5/1/20 1:06 AM, Ansuel Smith wrote:
> > From: Sham Muthayyan <smuthayy@codeaurora.org>
> >
> > Add tx term offset support to pcie qcom driver need in some revision of
> > the ipq806x SoC.
> > Ipq8064 have tx term offset set to 7.
> > Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.
> >
> > Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> > index da8058fd1925..372d2c8508b5 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -45,6 +45,9 @@
> >  #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
> >
> >  #define PCIE20_PARF_PHY_CTRL			0x40
> > +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12,
> 16)
> 
> The mask definition is not correct. Should be GENMASK(20, 16)
> 
> > +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> > +
> >  #define PCIE20_PARF_PHY_REFCLK			0x4C
> >  #define PHY_REFCLK_SSP_EN			BIT(16)
> >  #define PHY_REFCLK_USE_PAD			BIT(12)
> > @@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
> >  	u32 tx_swing_full;
> >  	u32 tx_swing_low;
> >  	u32 rx0_eq;
> > +	u8 phy_tx0_term_offset;
> >  };
> >
> >  struct qcom_pcie_resources_1_0_0 {
> > @@ -318,6 +322,11 @@ static int
> qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
> >  	if (IS_ERR(res->ext_reset))
> >  		return PTR_ERR(res->ext_reset);
> >
> > +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
> > +		res->phy_tx0_term_offset = 7;
> 
> Before your change the phy_tx0_term_offser was 0 for apq8064, but here
> you change it to 7, why?
> 

apq8064 board should use qcom,pcie-apq8064 right? This should be set to 0
only with pcie-ipq8064 compatible. Tell me if I'm wrong.

> > +	else
> > +		res->phy_tx0_term_offset = 0;
> > +
> >  	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
> >  	return PTR_ERR_OR_ZERO(res->phy_reset);
> >  }
> > @@ -402,6 +411,11 @@ static int qcom_pcie_init_2_1_0(struct
> qcom_pcie *pcie)
> >  	/* enable PCIe clocks and resets */
> >  	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL,
> BIT(0), 0);
> >
> > +	/* set TX termination offset */
> > +	qcom_clear_and_set_dword(pcie->parf + PCIE20_PARF_PHY_CTRL,
> > +			PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
> 
> As the mask definition is incorrect you actually clear 12 to 16 bit in
> the register where is another PHY parameter. Is that was intentional?
> 

Will check this and the wrong genmask.

> > +			PHY_CTRL_PHY_TX0_TERM_OFFSET(res-
> >phy_tx0_term_offset));
> > +
> >  	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(res->tx_deemph_gen1) |
> >  	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(res-
> >tx_deemph_gen2_3p5db) |
> >  	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(res-
> >tx_deemph_gen2_6db),
> > @@ -1485,6 +1499,7 @@ static int qcom_pcie_probe(struct
> platform_device *pdev)
> >  static const struct of_device_id qcom_pcie_match[] = {
> >  	{ .compatible = "qcom,pcie-apq8084", .data = &ops_1_0_0 },
> >  	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_2_1_0 },
> > +	{ .compatible = "qcom,pcie-ipq8064-v2", .data = &ops_2_1_0 },
> >  	{ .compatible = "qcom,pcie-apq8064", .data = &ops_2_1_0 },
> >  	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
> >  	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
> >
> 
> --
> regards,
> Stan

R: R: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[ansuelsmth]

> On 5/12/20 6:45 PM, Rob Herring wrote:
> > On Thu, May 07, 2020 at 09:34:35PM +0200, ansuelsmth@gmail.com
> wrote:
> >>> On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
> >>>> It is now supported the editing of Tx De-Emphasis, Tx Swing and
> >>>> Rx equalization params on ipq8064. Document this new optional
> params.
> >>>>
> >>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> >>>> ---
> >>>>  .../devicetree/bindings/pci/qcom,pcie.txt     | 36
> +++++++++++++++++++
> >>>>  1 file changed, 36 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >>>> index 6efcef040741..8cc5aea8a1da 100644
> >>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> >>>> @@ -254,6 +254,42 @@
> >>>>  			- "perst-gpios"	PCIe endpoint reset signal line
> >>>>  			- "wake-gpios"	PCIe endpoint wake signal line
> >>>>
> >>>> +- qcom,tx-deemph-gen1:
> >>>> +	Usage: optional (available for ipq/apq8064)
> >>>> +	Value type: <u32>
> >>>> +	Definition: Gen1 De-emphasis value.
> >>>> +		    For ipq806x should be set to 24.
> >>>
> >>> Unless these need to be tuned per board, then the compatible string
> for
> >>> ipq806x should imply all these settings.
> >>>
> >>
> >> It was requested by v2 to make this settings tunable. These don't change
> are
> >> all the same for every ipq806x SoC. The original implementation had this
> >> value hardcoded for ipq806x. Should I restore this and drop this patch?
> >
> > Yes, please.
> 
> I still think that the values for tx deemph and tx swing should be
> tunable. But I can live with them in the driver if they not break
> support for apq8064.
> 
> The default values in the registers for apq8064 and ipq806x are:
> 
> 			default		your change
> TX_DEEMPH_GEN1		21		24
> TX_DEEMPH_GEN2_3_5DB	21		24
> TX_DEEMPH_GEN2_6DB	32		34
> 
> TX_SWING_FULL		121		120
> TX_SWING_LOW		121		120
> 
> So until now (without your change) apq8064 worked with default values.
> 

I will limit this to ipq8064(-v2) if this could be a problem.

> >
> > Rob
> >
> 
> --
> regards,
> Stan

Re: R: [PATCH v3 09/11] PCI: qcom: add ipq8064 rev2 variant and set tx term offset

[Stanimir Varbanov]

On 5/13/20 3:54 PM, ansuelsmth@gmail.com wrote:
>> Hi Ansuel,
>>
>> On 5/1/20 1:06 AM, Ansuel Smith wrote:
>>> From: Sham Muthayyan <smuthayy@codeaurora.org>
>>>
>>> Add tx term offset support to pcie qcom driver need in some revision of
>>> the ipq806x SoC.
>>> Ipq8064 have tx term offset set to 7.
>>> Ipq8064-v2 revision and ipq8065 have the tx term offset set to 0.
>>>
>>> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>  drivers/pci/controller/dwc/pcie-qcom.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
>> b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index da8058fd1925..372d2c8508b5 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -45,6 +45,9 @@
>>>  #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
>>>
>>>  #define PCIE20_PARF_PHY_CTRL			0x40
>>> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(12,
>> 16)
>>
>> The mask definition is not correct. Should be GENMASK(20, 16)
>>
>>> +#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
>>> +
>>>  #define PCIE20_PARF_PHY_REFCLK			0x4C
>>>  #define PHY_REFCLK_SSP_EN			BIT(16)
>>>  #define PHY_REFCLK_USE_PAD			BIT(12)
>>> @@ -118,6 +121,7 @@ struct qcom_pcie_resources_2_1_0 {
>>>  	u32 tx_swing_full;
>>>  	u32 tx_swing_low;
>>>  	u32 rx0_eq;
>>> +	u8 phy_tx0_term_offset;
>>>  };
>>>
>>>  struct qcom_pcie_resources_1_0_0 {
>>> @@ -318,6 +322,11 @@ static int
>> qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>>>  	if (IS_ERR(res->ext_reset))
>>>  		return PTR_ERR(res->ext_reset);
>>>
>>> +	if (of_device_is_compatible(dev->of_node, "qcom,pcie-ipq8064"))
>>> +		res->phy_tx0_term_offset = 7;
>>
>> Before your change the phy_tx0_term_offser was 0 for apq8064, but here
>> you change it to 7, why?
>>
> 
> apq8064 board should use qcom,pcie-apq8064 right? This should be set to 0
> only with pcie-ipq8064 compatible. Tell me if I'm wrong.

Sorry, my fault. I read the compatible check above as apq8064 but it is ipq.

-- 
regards,
Stan

Re: R: R: [PATCH v3 08/11] devicetree: bindings: pci: document PARF params bindings

[Stanimir Varbanov]

Hi,

On 5/13/20 3:56 PM, ansuelsmth@gmail.com wrote:
>> On 5/12/20 6:45 PM, Rob Herring wrote:
>>> On Thu, May 07, 2020 at 09:34:35PM +0200, ansuelsmth@gmail.com
>> wrote:
>>>>> On Fri, May 01, 2020 at 12:06:15AM +0200, Ansuel Smith wrote:
>>>>>> It is now supported the editing of Tx De-Emphasis, Tx Swing and
>>>>>> Rx equalization params on ipq8064. Document this new optional
>> params.
>>>>>>
>>>>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/pci/qcom,pcie.txt     | 36
>> +++++++++++++++++++
>>>>>>  1 file changed, 36 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>>>> index 6efcef040741..8cc5aea8a1da 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>>>>> @@ -254,6 +254,42 @@
>>>>>>  			- "perst-gpios"	PCIe endpoint reset signal line
>>>>>>  			- "wake-gpios"	PCIe endpoint wake signal line
>>>>>>
>>>>>> +- qcom,tx-deemph-gen1:
>>>>>> +	Usage: optional (available for ipq/apq8064)
>>>>>> +	Value type: <u32>
>>>>>> +	Definition: Gen1 De-emphasis value.
>>>>>> +		    For ipq806x should be set to 24.
>>>>>
>>>>> Unless these need to be tuned per board, then the compatible string
>> for
>>>>> ipq806x should imply all these settings.
>>>>>
>>>>
>>>> It was requested by v2 to make this settings tunable. These don't change
>> are
>>>> all the same for every ipq806x SoC. The original implementation had this
>>>> value hardcoded for ipq806x. Should I restore this and drop this patch?
>>>
>>> Yes, please.
>>
>> I still think that the values for tx deemph and tx swing should be
>> tunable. But I can live with them in the driver if they not break
>> support for apq8064.
>>
>> The default values in the registers for apq8064 and ipq806x are:
>>
>> 			default		your change
>> TX_DEEMPH_GEN1		21		24
>> TX_DEEMPH_GEN2_3_5DB	21		24
>> TX_DEEMPH_GEN2_6DB	32		34
>>
>> TX_SWING_FULL		121		120
>> TX_SWING_LOW		121		120
>>
>> So until now (without your change) apq8064 worked with default values.
>>
> 
> I will limit this to ipq8064(-v2) if this could be a problem.

I guess you can do it that way, but if new board appear in the future
with slightly different parameters (for example deemph_gen1 = 23 and so
on) do we need to add another compatible for that? At the end we will
have compatibles per board but not per SoC. :(

-- 
regards,
Stan