Skip to contentSkip to conversation

pcie: qcom: add missing ipq806x clocks in pcie driver

PATCH Superseded Ansuel Smith 6 participants 32 messages Mar 20, 2020 – Apr 1, 2020

Subsystems (5)
Device tree bindings Arm Qualcomm mailing list PCI PCI native host bridge and endpoint drivers +1 more subsystems

v1 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Ansuel Smith

author
#0 lore raw

PATCH 1/12 pcie: qcom: add missing ipq806x clocks in pcie driver

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

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +33 -5
index 5ea527a6bd9f..f958c535de6e 100644
@@ -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(dev, "aux");
+	if (IS_ERR(res->aux_clk))
+		return PTR_ERR(res->aux_clk);
+
+	res->ref_clk = devm_clk_get(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,28 @@ 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);
+	ret = clk_prepare_enable(res->aux_clk);
 	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_core;
+		dev_err(dev, "cannot prepare/enable aux clock\n");
+		goto err_clk_aux;
+	}
+
+	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 +396,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);
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 2/12 devicetree: bindings: pci: add missing clks to qcom,pcie

Document missing clks used in ipq806x soc

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +6 -2
index 981b4de12807..becdbdc0fffa 100644
@@ -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>,
Signature · 1 line
2.25.1
maintainer Acked-by on v1 2/12
#2 lore raw

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

Quoted 6 lines · Ansuel Smith

Document missing clks used in ipq806x soc

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

What a mess the clocks are for this binding...

Oh well,

Acked-by Rob Herring <[email protected]>
Quoted 30 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +6 -2
index 981b4de12807..becdbdc0fffa 100644
@@ -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>,
Signature · 1 line
2.25.1
author
#3 lore raw

PATCH 3/12 pcie: qcom: change duplicate pci reset to phy reset

In reply to Ansuel Smith

From: Abhishek Sahu <[email protected]>

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

Signed-off-by Abhishek Sahu <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-qcom.c +1 -1
index f958c535de6e..1fcc7fed8443 100644
@@ -284,7 +284,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->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);
Signature · 1 line
2.25.1
author
#4 lore raw

PATCH 4/12 pcie: qcom: Fixed pcie_phy_clk branch issue

In reply to Ansuel Smith

Following backtraces are observed in PCIe deinit operation.

 Hardware name: Qualcomm (Flattened Device Tree)
 (unwind_backtrace) from [] (show_stack+0x10/0x14)
 (show_stack) from [] (dump_stack+0x84/0x98)
 (dump_stack) from [] (warn_slowpath_common+0x9c/0xb8)
 (warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40)
 (warn_slowpath_fmt) from [] (clk_branch_wait+0x114/0x120)
 (clk_branch_wait) from [] (clk_core_disable+0xd0/0x1f4)
 (clk_core_disable) from [] (clk_disable+0x24/0x30)
 (clk_disable) from [] (qcom_pcie_deinit_v0+0x6c/0xb8)
 (qcom_pcie_deinit_v0) from [] (qcom_pcie_host_init+0xe0/0xe8)
 (qcom_pcie_host_init) from [] (dw_pcie_host_init+0x3b0/0x538)
 (dw_pcie_host_init) from [] (qcom_pcie_probe+0x20c/0x2e4)

pcie_phy_clk is generated for PCIe controller itself and the GCC controls its branch operation. This error is coming since the assert operations turn off the parent clock before branch clock. Now this patch moves clk_disable_unprepare before assert operations.

Similarly, during probe function, the clock branch operation should be done after dessert operation. Currently, it does not generate any error since bootloader enables the pcie_phy_clk but the same error is coming during probe, if bootloader disables pcie_phy_clk.

Signed-off-by Abhishek Sahu <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +7 -9
index 1fcc7fed8443..596731b54728 100644
@@ -280,6 +280,7 @@ 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);

@@ -287,7 +288,6 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
 	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;
-	}
-
 	ret = clk_prepare_enable(res->aux_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable aux clock\n");

@@ -383,6 +377,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);
 

@@ -400,8 +400,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);
Signature · 1 line
2.25.1
author
#5 lore raw

PATCH 5/12 pcie: qcom: add missing reset for ipq806x

In reply to Ansuel Smith

Add missing ext reset used by ipq806x soc in
pcie qcom driver

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +18 -6
index 596731b54728..ecc22fd27ea6 100644
@@ -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(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);

@@ -301,18 +307,18 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	u32 val;
 	int ret;
 
+	ret = reset_control_assert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert ahb reset\n");
+		return ret;
+	}
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
 	if (ret < 0) {
 		dev_err(dev, "cannot enable regulators\n");
 		return ret;
 	}
 
-	ret = reset_control_assert(res->ahb_reset);
-	if (ret) {
-		dev_err(dev, "cannot assert ahb reset\n");
-		goto err_assert_ahb;
-	}
-
 	ret = clk_prepare_enable(res->iface_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable iface clock\n");

@@ -343,6 +349,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);
Signature · 1 line
2.25.1
#6 lore raw

Re: [PATCH 05/12] pcie: qcom: add missing reset for ipq806x

Quoted 2 lines · Ansuel Smith

Add missing ext reset used by ipq806x soc in
pcie qcom driver

You say "missing" -- does that mean this is a *new* requirement for this ipq806x device, and previous devices work correctly without this patch?

Or does this fix an omission and previous devices actually didn't work correctly?

s/soc/SoC/
s/pcie/PCIe/

Period at end of sentence. Please wrap to fill 75 columns.

#7 lore raw

Re: [PATCH 05/12] pcie: qcom: add missing reset for ipq806x

In reply to Ansuel Smith

Hi Ansuel,

Quoted 22 lines · Ansuel Smith

Add missing ext reset used by ipq806x soc in
pcie qcom driver

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +1 -0
index 596731b54728..ecc22fd27ea6 100644
@@ -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];
 };

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie) · line 276

Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +1 -0
@@ -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(dev, "ext");

Please use devm_reset_control_get_exclusive() instead.

Quoted 26 lines
drivers/pci/controller/dwc/pcie-qcom.c +10 -0
+	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);

@@ -301,18 +307,18 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	u32 val;
 	int ret;
 
+	ret = reset_control_assert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert ahb reset\n");
+		return ret;
+	}
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
 	if (ret < 0) {

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie) · line 315

Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +0 -6
 		dev_err(dev, "cannot enable regulators\n");
 		return ret;
 	}
 
-	ret = reset_control_assert(res->ahb_reset);
-	if (ret) {
-		dev_err(dev, "cannot assert ahb reset\n");
-		goto err_assert_ahb;
-	}
-

This change is not described in the commit message.

regards
Philipp

author
#8 lore raw

PATCH 6/12 devicetree: bindings: pci: add ext reset to qcom,pcie

In reply to Ansuel Smith

Document ext reset used in ipq806x soc by qcom pcie driver

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +4 -2
index becdbdc0fffa..6efcef040741 100644
@@ -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";
 	};
Signature · 1 line
2.25.1
maintainer Acked-by on v1 6/12
#9 lore raw

Re: [PATCH 06/12] devicetree: bindings: pci: add ext reset to qcom,pcie

Quoted 7 lines · Ansuel Smith

Document ext reset used in ipq806x soc by qcom pcie driver

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Acked-by Rob Herring <[email protected]>
author
#10 lore raw

PATCH 7/12 pcie: qcom: add tx term offset support

In reply to Ansuel Smith

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 9 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +52 -9
index ecc22fd27ea6..8009e3117765 100644
@@ -45,7 +45,13 @@
 #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
 
 #define PCIE20_PARF_PHY_CTRL			0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)
+
 #define PCIE20_PARF_PHY_REFCLK			0x4C
+#define REF_SSP_EN				BIT(16)
+#define REF_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 +83,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 +115,7 @@ 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];
+	uint8_t phy_tx0_term_offset;
 };
 
 struct qcom_pcie_resources_1_0_0 {

@@ -184,6 +203,16 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static inline void
+writel_masked(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);

@@ -277,6 +306,10 @@ 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_property_read_u8(dev->of_node, "phy-tx0-term-offset",
+				&res->phy_tx0_term_offset))
+		res->phy_tx0_term_offset = 0;
+
 	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }

@@ -304,7 +337,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 = reset_control_assert(res->ahb_reset);

@@ -355,15 +387,26 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_deassert_ahb;
 	}
 
-	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
+
+	/* Set Tx termination offset */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
+
+	/* PARF programming */
+	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22),
+	       pcie->parf + PCIE20_PARF_PCS_DEEMPH);
+	writel(PCS_SWING_TX_SWING_FULL(0x78) |
+	       PCS_SWING_TX_SWING_LOW(0x78),
+	       pcie->parf + PCIE20_PARF_PCS_SWING);
+	writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS);
 
-	/* enable external reference clock */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
-	val |= BIT(16);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+	/* Enable reference clock */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_REFCLK,
+		      REF_USE_PAD, REF_SSP_EN);
 
 	ret = reset_control_deassert(res->phy_reset);
 	if (ret) {
Signature · 1 line
2.25.1
#11 lore raw

Re: [PATCH 07/12] pcie: qcom: add tx term offset support

Quoted 4 lines · Ansuel Smith

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

The usual (s/pcie/PCIe/, s/soc/SoC/, wrap to fill 75 columns, add period at end). Apply to all patches.

drivers/pci/controller/dwc/pcie-qcom.c line 49

Quoted 2 lines
drivers/pci/controller/dwc/pcie-qcom.c +2 -0
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)

Since the rest of the file uses BIT(), I think it would make sense to use GENMASK() here. And below, of course.

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie { · line 207

Quoted 2 lines
drivers/pci/controller/dwc/pcie-qcom.c +2 -0
+static inline void
+writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)

Follow coding style of rest of file, i.e.,

static inline void writel_masked(...)

The name "writel_masked" suggests that we're writing "val & mask" to a register, but that's not what's happening. This is really a "clear some bits and set others" operation.

The names are wordy, but we do have pci_clear_and_set_dword(), pcie_capability_clear_and_set_word(), etc., functions that work similarly.

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie {

Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +8 -0
+{
+	u32 val = readl(addr);
+
+	val &= ~clear_mask;
+	val |= set_mask;
+	writel(val, addr);
+}
 
+	/* Set Tx termination offset */

Capitalize consistently with other comments in the file. Below also. Hmm, I see the file is already a bit of a mess in that regard. So just do what you can.

#12 lore raw

Re: [PATCH 07/12] pcie: qcom: add tx term offset support

In reply to Ansuel Smith
Quoted 74 lines · Ansuel Smith

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 61 ++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 9 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +29 -0
index ecc22fd27ea6..8009e3117765 100644
@@ -45,7 +45,13 @@
 #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
 
 #define PCIE20_PARF_PHY_CTRL			0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)
+
 #define PCIE20_PARF_PHY_REFCLK			0x4C
+#define REF_SSP_EN				BIT(16)
+#define REF_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 +83,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 +115,7 @@ 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];
+	uint8_t phy_tx0_term_offset;
 };
 
 struct qcom_pcie_resources_1_0_0 {

@@ -184,6 +203,16 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static inline void
+writel_masked(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);

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie) · line 311

Quoted 6 lines
drivers/pci/controller/dwc/pcie-qcom.c +3 -0
@@ -277,6 +306,10 @@ 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_property_read_u8(dev->of_node, "phy-tx0-term-offset",
+				&res->phy_tx0_term_offset))
+		res->phy_tx0_term_offset = 0;

The appropriate way is to encode differences in hardware is to use different compatibles for the two different versions of the hardware.

Regards,
Bjorn

Quoted 50 lines
drivers/pci/controller/dwc/pcie-qcom.c +20 -9
+
 	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }

@@ -304,7 +337,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 = reset_control_assert(res->ahb_reset);

@@ -355,15 +387,26 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_deassert_ahb;
 	}
 
-	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
+
+	/* Set Tx termination offset */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET(res->phy_tx0_term_offset));
+
+	/* PARF programming */
+	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22),
+	       pcie->parf + PCIE20_PARF_PCS_DEEMPH);
+	writel(PCS_SWING_TX_SWING_FULL(0x78) |
+	       PCS_SWING_TX_SWING_LOW(0x78),
+	       pcie->parf + PCIE20_PARF_PCS_SWING);
+	writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS);
 
-	/* enable external reference clock */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
-	val |= BIT(16);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+	/* Enable reference clock */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_REFCLK,
+		      REF_USE_PAD, REF_SSP_EN);
 
 	ret = reset_control_deassert(res->phy_reset);
 	if (ret) {
Signature · 1 line
2.25.1
author
#13 lore raw

R: [PATCH 07/12] pcie: qcom: add tx term offset support

Quoted 90 lines · Ansuel Smith
Quoted 10 lines

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 61

++++++++++++++++++++++----

Review

Quoted 3 lines
 1 file changed, 52 insertions(+), 9 deletions(-)
(quoted diff) +0 -0

b/drivers/pci/controller/dwc/pcie-qcom.c

Quoted 37 lines

index ecc22fd27ea6..8009e3117765 100644

drivers/pci/controller/dwc/pcie-qcom.c +18 -0
@@ -45,7 +45,13 @@
 #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
 
 #define PCIE20_PARF_PHY_CTRL			0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)
+
 #define PCIE20_PARF_PHY_REFCLK			0x4C
+#define REF_SSP_EN				BIT(16)
+#define REF_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 +83,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

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie_resources_2_1_0 { · line 117

Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0 -0
@@ -97,6 +115,7 @@ 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];

drivers/pci/controller/dwc/pcie-qcom.c static int

Quoted 22 lines
drivers/pci/controller/dwc/pcie-qcom.c +11 -0
+	uint8_t phy_tx0_term_offset;
 };
 
 struct qcom_pcie_resources_1_0_0 {

@@ -184,6 +203,16 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static inline void
+writel_masked(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);

@@ -277,6 +306,10 @@ static int

qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie) · line 311

Quoted 6 lines
drivers/pci/controller/dwc/pcie-qcom.c +3 -0
 	if (IS_ERR(res->ext_reset))
 		return PTR_ERR(res->ext_reset);
 
+	if (of_property_read_u8(dev->of_node, "phy-tx0-term-offset",
+				&res->phy_tx0_term_offset))
+		res->phy_tx0_term_offset = 0;

The appropriate way is to encode differences in hardware is to use different compatibles for the two different versions of the hardware.

Quoted 3 lines

Regards,
Bjorn

So a better way to handle this would be to check the SoC compatible? AFAIK a different offset is only needed on ipq8064 revision 2 and ipq8065 but
it looks bad to add a special code just for that 2 SoC.
I would prefer to handle this with the offset definition but If you think this would be
the right way, I will follow that. Waiting for your response about this.

Quoted 53 lines

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_init_2_1_0(struct

Quoted 5 lines
drivers/pci/controller/dwc/pcie-qcom.c +1 -0
+
 	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }

@@ -304,7 +337,6 @@ static int qcom_pcie_init_2_1_0(struct

qcom_pcie *pcie)

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_init_2_1_0(struct

Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +0 -1
 	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 = reset_control_assert(res->ahb_reset);

@@ -355,15 +387,26 @@ static int qcom_pcie_init_2_1_0(struct

qcom_pcie *pcie)

Quoted 38 lines
drivers/pci/controller/dwc/pcie-qcom.c +19 -8
 		goto err_deassert_ahb;
 	}
 
-	/* enable PCIe clocks and resets */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
-	val &= ~BIT(0);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL, BIT(0), 0);
+
+	/* Set Tx termination offset */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_CTRL,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK,
+		      PHY_CTRL_PHY_TX0_TERM_OFFSET(res-
 phy_tx0_term_offset));
+
+	/* PARF programming */
+	writel(PCS_DEEMPH_TX_DEEMPH_GEN1(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(0x18) |
+	       PCS_DEEMPH_TX_DEEMPH_GEN2_6DB(0x22),
+	       pcie->parf + PCIE20_PARF_PCS_DEEMPH);
+	writel(PCS_SWING_TX_SWING_FULL(0x78) |
+	       PCS_SWING_TX_SWING_LOW(0x78),
+	       pcie->parf + PCIE20_PARF_PCS_SWING);
+	writel(PHY_RX0_EQ(0x4), pcie->parf + PCIE20_PARF_CONFIG_BITS);
 
-	/* enable external reference clock */
-	val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
-	val |= BIT(16);
-	writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+	/* Enable reference clock */
+	writel_masked(pcie->parf + PCIE20_PARF_PHY_REFCLK,
+		      REF_USE_PAD, REF_SSP_EN);
 
 	ret = reset_control_deassert(res->phy_reset);

if (ret) {
--
2.25.1

#14 lore raw

Re: R: [PATCH 07/12] pcie: qcom: add tx term offset support

Quoted 94 lines · On Wed 01 Apr 14:55 PDT 2020, [email protected] wrote:
Quoted 90 lines · Ansuel Smith
Quoted 10 lines

From: Sham Muthayyan <[email protected]>

Add tx term offset support to pcie qcom driver
need in some revision of the ipq806x soc

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 61

++++++++++++++++++++++----

Review

Quoted 3 lines
 1 file changed, 52 insertions(+), 9 deletions(-)
(quoted diff) +0 -0

b/drivers/pci/controller/dwc/pcie-qcom.c

Quoted 37 lines

index ecc22fd27ea6..8009e3117765 100644

drivers/pci/controller/dwc/pcie-qcom.c +18 -0
@@ -45,7 +45,13 @@
 #define PCIE_CAP_CPL_TIMEOUT_DISABLE		0x10
 
 #define PCIE20_PARF_PHY_CTRL			0x40
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	(0x1f << 16)
+#define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		(x << 16)
+
 #define PCIE20_PARF_PHY_REFCLK			0x4C
+#define REF_SSP_EN				BIT(16)
+#define REF_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 +83,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

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie_resources_2_1_0 { · line 117

Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +0 -0
@@ -97,6 +115,7 @@ 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];

drivers/pci/controller/dwc/pcie-qcom.c static int

Quoted 22 lines
drivers/pci/controller/dwc/pcie-qcom.c +11 -0
+	uint8_t phy_tx0_term_offset;
 };
 
 struct qcom_pcie_resources_1_0_0 {

@@ -184,6 +203,16 @@ struct qcom_pcie {
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
 
+static inline void
+writel_masked(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);

@@ -277,6 +306,10 @@ static int

qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie) · line 311

Quoted 6 lines
drivers/pci/controller/dwc/pcie-qcom.c +3 -0
 	if (IS_ERR(res->ext_reset))
 		return PTR_ERR(res->ext_reset);
 
+	if (of_property_read_u8(dev->of_node, "phy-tx0-term-offset",
+				&res->phy_tx0_term_offset))
+		res->phy_tx0_term_offset = 0;

The appropriate way is to encode differences in hardware is to use different compatibles for the two different versions of the hardware.

Quoted 3 lines

Regards,
Bjorn

Quoted 8 lines

So a better way to handle this would be to check the SoC compatible? AFAIK a different offset is only needed on ipq8064 revision 2 and ipq8065 but
it looks bad to add a special code just for that 2 SoC.
I would prefer to handle this with the offset definition but If you think this would be
the right way, I will follow that. Waiting for your response about this.

Yes, please do this by having different compatibles for the different revisions of the hardware block.

You should be able to use the same implementation for the two compatibles, just make the phy_tx0_term_offset depends on which was used.

Regards,
Bjorn

author
#15 lore raw

PATCH 8/12 devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

In reply to Ansuel Smith

Document phy-tx0-term-offset propriety to qcom pcie driver

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
 1 file changed, 7 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +7 -0
index 6efcef040741..8c1d014f37b0 100644
@@ -254,6 +254,12 @@
 			- "perst-gpios"	PCIe endpoint reset signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
 
+- phy-tx0-term-offset:
+	Usage: optional
+	Value type: <u32>
+	Definition: If not defined is 0. In ipq806x is set to 7. In newer
+				revision (v2.0) the offset is zero.
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";

@@ -293,6 +299,7 @@
 		reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
 		pinctrl-0 = <&pcie_pins_default>;
 		pinctrl-names = "default";
+		phy-tx0-term-offset = <7>;
 	};
 
 * Example for apq8084
Signature · 1 line
2.25.1
maintainer
#16 lore raw

Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

Quoted 1 line · Ansuel Smith

Document phy-tx0-term-offset propriety to qcom pcie driver

propriety?

Quoted 11 lines
Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
 1 file changed, 7 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
index 6efcef040741..8c1d014f37b0 100644

Documentation/devicetree/bindings/pci/qcom,pcie.txt line 257

Quoted 4 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +1 -0
@@ -254,6 +254,12 @@
 			- "perst-gpios"	PCIe endpoint reset signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
 
+- phy-tx0-term-offset:

Needs a vendor prefix.

Quoted 19 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +6 -0
+	Usage: optional
+	Value type: <u32>
+	Definition: If not defined is 0. In ipq806x is set to 7. In newer
+				revision (v2.0) the offset is zero.
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";

@@ -293,6 +299,7 @@
 		reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
 		pinctrl-0 = <&pcie_pins_default>;
 		pinctrl-names = "default";
+		phy-tx0-term-offset = <7>;
 	};
 
 * Example for apq8084
Signature · 1 line
2.25.1
author
#17 lore raw

R: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

Quoted 37 lines

-----Messaggio originale-----
Da: Rob Herring <[email protected]>
Inviato: martedì 31 marzo 2020 19:33
A: Ansuel Smith <[email protected]>
Cc: Stanimir Varbanov <[email protected]>; Andy Gross <[email protected]>; Bjorn Andersson <[email protected]>; Bjorn Helgaas <[email protected]>; Mark Rutland <[email protected]>; Lorenzo Pieralisi <[email protected]>; Andrew Murray <[email protected]>; Philipp Zabel <[email protected]>; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]
Oggetto: Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term- offset to qcom,pcie

Quoted 1 line · Ansuel Smith

Document phy-tx0-term-offset propriety to qcom pcie driver

propriety?

Review

Quoted 7 lines
Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
 1 file changed, 7 insertions(+)
(quoted diff) +0 -0

b/Documentation/devicetree/bindings/pci/qcom,pcie.txt

Quoted 7 lines

index 6efcef040741..8c1d014f37b0 100644

Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
@@ -254,6 +254,12 @@
 			- "perst-gpios"	PCIe endpoint reset signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
Quoted 4 lines

Documentation/devicetree/bindings/pci/qcom,pcie.txt line 257

Quoted 1 line
Documentation/devicetree/bindings/pci/qcom,pcie.txt +1 -0
+- phy-tx0-term-offset:

Needs a vendor prefix.

So I should change to qcom,phy-tx0-term-offset right?

Quoted 20 lines

Documentation/devicetree/bindings/pci/qcom,pcie.txt

Quoted 8 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +5 -0
+	Usage: optional
+	Value type: <u32>
+	Definition: If not defined is 0. In ipq806x is set to 7. In newer
+				revision (v2.0) the offset is zero.
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064",

"snps,dw-pcie";

Quoted 11 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +1 -1
@@ -293,6 +299,7 @@
 		reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
 		pinctrl-0 = <&pcie_pins_default>;
 		pinctrl-names = "default";
+		phy-tx0-term-offset = <7>;
 	};
 
 * Example for apq8084
--

2.25.1

#18 lore raw

Re: [PATCH 08/12] devicetree: bindings: pci: add phy-tx0-term-offset to qcom,pcie

In reply to Ansuel Smith
Quoted 12 lines · Ansuel Smith

Document phy-tx0-term-offset propriety to qcom pcie driver

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 7 +++++++
 1 file changed, 7 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
index 6efcef040741..8c1d014f37b0 100644

Documentation/devicetree/bindings/pci/qcom,pcie.txt line 257

Quoted 4 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +1 -0
@@ -254,6 +254,12 @@
 			- "perst-gpios"	PCIe endpoint reset signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
 
+- phy-tx0-term-offset:

If I understand your implementation correctly this difference in hardware revision should be encoded in the compatible string.

Regards,
Bjorn

Quoted 19 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +6 -0
+	Usage: optional
+	Value type: <u32>
+	Definition: If not defined is 0. In ipq806x is set to 7. In newer
+				revision (v2.0) the offset is zero.
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";

@@ -293,6 +299,7 @@
 		reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
 		pinctrl-0 = <&pcie_pins_default>;
 		pinctrl-names = "default";
+		phy-tx0-term-offset = <7>;
 	};
 
 * Example for apq8084
Signature · 1 line
2.25.1
author
#19 lore raw

PATCH 9/12 pcie: qcom: Programming the PCIE iATU for IPQ806x

In reply to Ansuel Smith

From: Sham Muthayyan <[email protected]>

Resolved PCIE EP detection errors caused due to missing iATU programming.

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 78 ++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +78 -0
index 8009e3117765..e26ba8f63d4f 100644
@@ -77,6 +77,30 @@
 #define PCIE20_CAP_LINK_1			(PCIE20_CAP + 0x14)
 #define PCIE_CAP_LINK1_VAL			0x2FD7F
 
+#define PCIE20_CAP_LINKCTRLSTATUS		(PCIE20_CAP + 0x10)
+
+#define PCIE20_AXI_MSTR_RESP_COMP_CTRL0		0x818
+#define PCIE20_AXI_MSTR_RESP_COMP_CTRL1		0x81c
+
+#define PCIE20_PLR_IATU_VIEWPORT		0x900
+#define PCIE20_PLR_IATU_REGION_OUTBOUND		(0x0 << 31)
+#define PCIE20_PLR_IATU_REGION_INDEX(x)		(x << 0)
+
+#define PCIE20_PLR_IATU_CTRL1			0x904
+#define PCIE20_PLR_IATU_TYPE_CFG0		(0x4 << 0)
+#define PCIE20_PLR_IATU_TYPE_MEM		(0x0 << 0)
+
+#define PCIE20_PLR_IATU_CTRL2			0x908
+#define PCIE20_PLR_IATU_ENABLE			BIT(31)
+
+#define PCIE20_PLR_IATU_LBAR			0x90C
+#define PCIE20_PLR_IATU_UBAR			0x910
+#define PCIE20_PLR_IATU_LAR			0x914
+#define PCIE20_PLR_IATU_LTAR			0x918
+#define PCIE20_PLR_IATU_UTAR			0x91c
+
+#define MSM_PCIE_DEV_CFG_ADDR			0x01000000
+
 #define PCIE20_PARF_Q2A_FLUSH			0x1AC
 
 #define PCIE20_MISC_CONTROL_1_REG		0x8BC

@@ -251,6 +275,57 @@ static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
 	writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
 }
 
+static void qcom_pcie_prog_viewport_cfg0(struct qcom_pcie *pcie, u32 busdev)
+{
+	struct pcie_port *pp = &pcie->pci->pp;
+
+	/*
+	 * program and enable address translation region 0 (device config
+	 * address space); region type config;
+	 * axi config address range to device config address range
+	 */
+	writel(PCIE20_PLR_IATU_REGION_OUTBOUND |
+	       PCIE20_PLR_IATU_REGION_INDEX(0),
+	       pcie->pci->dbi_base + PCIE20_PLR_IATU_VIEWPORT);
+
+	writel(PCIE20_PLR_IATU_TYPE_CFG0, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL1);
+	writel(PCIE20_PLR_IATU_ENABLE, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL2);
+	writel(pp->cfg0_base, pcie->pci->dbi_base + PCIE20_PLR_IATU_LBAR);
+	writel((pp->cfg0_base >> 32), pcie->pci->dbi_base + PCIE20_PLR_IATU_UBAR);
+	writel((pp->cfg0_base + pp->cfg0_size - 1),
+	       pcie->pci->dbi_base + PCIE20_PLR_IATU_LAR);
+	writel(busdev, pcie->pci->dbi_base + PCIE20_PLR_IATU_LTAR);
+	writel(0, pcie->pci->dbi_base + PCIE20_PLR_IATU_UTAR);
+}
+
+static void qcom_pcie_prog_viewport_mem2_outbound(struct qcom_pcie *pcie)
+{
+	struct pcie_port *pp = &pcie->pci->pp;
+
+	/*
+	 * program and enable address translation region 2 (device resource
+	 * address space); region type memory;
+	 * axi device bar address range to device bar address range
+	 */
+	writel(PCIE20_PLR_IATU_REGION_OUTBOUND |
+	       PCIE20_PLR_IATU_REGION_INDEX(2),
+	       pcie->pci->dbi_base + PCIE20_PLR_IATU_VIEWPORT);
+
+	writel(PCIE20_PLR_IATU_TYPE_MEM, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL1);
+	writel(PCIE20_PLR_IATU_ENABLE, pcie->pci->dbi_base + PCIE20_PLR_IATU_CTRL2);
+	writel(pp->mem_base, pcie->pci->dbi_base + PCIE20_PLR_IATU_LBAR);
+	writel((pp->mem_base >> 32), pcie->pci->dbi_base + PCIE20_PLR_IATU_UBAR);
+	writel(pp->mem_base + pp->mem_size - 1,
+	       pcie->pci->dbi_base + PCIE20_PLR_IATU_LAR);
+	writel(pp->mem_bus_addr, pcie->pci->dbi_base + PCIE20_PLR_IATU_LTAR);
+	writel(upper_32_bits(pp->mem_bus_addr),
+	       pcie->pci->dbi_base + PCIE20_PLR_IATU_UTAR);
+
+	/* 256B PCIE buffer setting */
+	writel(0x1, pcie->pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL0);
+	writel(0x1, pcie->pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
+}
+
 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;

@@ -448,6 +523,9 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	writel(CFG_BRIDGE_SB_INIT,
 	       pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
 
+	qcom_pcie_prog_viewport_cfg0(pcie, MSM_PCIE_DEV_CFG_ADDR);
+	qcom_pcie_prog_viewport_mem2_outbound(pcie);
+
 	return 0;
 
 err_deassert_ahb:
Signature · 1 line
2.25.1
#20 lore raw

Re: [PATCH 09/12] pcie: qcom: Programming the PCIE iATU for IPQ806x

s/Programming/Program/

Capitalize IPQ806x consistently (other patches mention "ipq806x").

Quoted 3 lines · Ansuel Smith

From: Sham Muthayyan <[email protected]>

Resolved PCIE EP detection errors caused due to missing iATU programming.

s/Resolved/Resolve/
s/PCIE/PCIe/

Is this a bug fix? If so, can you cite the commit that introduced the bug? This is useful when backporting fixes. Same question applies to the other patches as well.

NAK
#21 lore raw

Re: [PATCH 09/12] pcie: qcom: Programming the PCIE iATU for IPQ806x

In reply to Ansuel Smith

Hi Ansuel,

Quoted 3 lines · Ansuel Smith

From: Sham Muthayyan <[email protected]>

Resolved PCIE EP detection errors caused due to missing iATU programming.

NACK, the iATU programing is not belonging here. Did you check what pcie-designware-*.c is doing with iATU?

If you want to support endpoint mode in pcie-qcom driver you have to see how the other drivers is doing that.

Quoted 7 lines
Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 78 ++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
Signature · 2 lines
regards,
Stan
author
#22 lore raw

PATCH 10/12 pcie: qcom: add Force GEN1 support

In reply to Ansuel Smith

From: Sham Muthayyan <[email protected]>

Add Force GEN1 support needed in some ipq806x board that needs to limit some pcie line to gen1 for some hardware limitation

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +13 -0
index e26ba8f63d4f..03130a3206b4 100644
@@ -123,6 +123,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

@@ -223,6 +225,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_ops *ops;
+	uint32_t force_gen1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)

@@ -515,6 +518,11 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 
 	/* wait for clock acquisition */
 	usleep_range(1000, 1500);
+	if (pcie->force_gen1) {
+		writel_relaxed((readl_relaxed(
+			pcie->pci->dbi_base + PCIE20_LNK_CONTROL2_LINK_STATUS2) | 1),
+			pcie->pci->dbi_base + PCIE20_LNK_CONTROL2_LINK_STATUS2);
+	}
 
 
 	/* Set the Max TLP size to 2K, instead of using default of 4K */

@@ -1487,6 +1495,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
 	int ret;
+	uint32_t force_gen1 = 0;
+	struct device_node *np = pdev->dev.of_node;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)

@@ -1517,6 +1527,9 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
+	of_property_read_u32(np, "force_gen1", &force_gen1);
+	pcie->force_gen1 = force_gen1;
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
 	pcie->parf = devm_ioremap_resource(dev, res);
 	if (IS_ERR(pcie->parf)) {
Signature · 1 line
2.25.1
#23 lore raw

Re: [PATCH 10/12] pcie: qcom: add Force GEN1 support

Quoted 5 lines · Ansuel Smith

From: Sham Muthayyan <[email protected]>

Add Force GEN1 support needed in some ipq806x board that needs to limit some pcie line to gen1 for some hardware limitation

Usual commit log comments.

drivers/pci/controller/dwc/pcie-qcom.c struct qcom_pcie { · line 228

Quoted 1 line
drivers/pci/controller/dwc/pcie-qcom.c +1 -0
+	uint32_t force_gen1;

unsigned int force_gen1 : 1

drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct platform_device *pdev) · line 1531

Quoted 2 lines
drivers/pci/controller/dwc/pcie-qcom.c +2 -0
+	of_property_read_u32(np, "force_gen1", &force_gen1);
+	pcie->force_gen1 = force_gen1;

I think there's a more or less standard property you can use for this instead of inventing a new one specific to this device.

Documentation/devicetree/bindings/pci/pci.txt:
"max-link-speed"

author
#24 lore raw

PATCH 11/12 devicetree: bindings: pci: add force_gen1 for qcom,pcie

In reply to Ansuel Smith

Document force_gen1 optional definition to limit pcie line to GEN1 speed

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +5 -0
index 8c1d014f37b0..766876465c42 100644
@@ -260,6 +260,11 @@
 	Definition: If not defined is 0. In ipq806x is set to 7. In newer
 				revision (v2.0) the offset is zero.
 
+- force_gen1:
+	Usage: optional
+	Value type: <u32>
+	Definition: Set 1 to force the pcie line to GEN1
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
Signature · 1 line
2.25.1
maintainer
#25 lore raw

Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

Quoted 13 lines · Ansuel Smith

Document force_gen1 optional definition to limit pcie line to GEN1 speed

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
index 8c1d014f37b0..766876465c42 100644

Documentation/devicetree/bindings/pci/qcom,pcie.txt line 267

Quoted 8 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +5 -0
@@ -260,6 +260,11 @@
 	Definition: If not defined is 0. In ipq806x is set to 7. In newer
 				revision (v2.0) the offset is zero.
 
+- force_gen1:
+	Usage: optional
+	Value type: <u32>
+	Definition: Set 1 to force the pcie line to GEN1
+

I believe we have a standard property 'link-speed' for this purpose.

Quoted 6 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
Signature · 1 line
2.25.1
author
#26 lore raw

R: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

Quoted 35 lines

-----Messaggio originale-----
Da: Rob Herring <[email protected]>
Inviato: martedì 31 marzo 2020 19:34
A: Ansuel Smith <[email protected]>
Cc: Stanimir Varbanov <[email protected]>; Andy Gross <[email protected]>; Bjorn Andersson <[email protected]>; Bjorn Helgaas <[email protected]>; Mark Rutland <[email protected]>; Lorenzo Pieralisi <[email protected]>; Andrew Murray <[email protected]>; Philipp Zabel <[email protected]>; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]
Oggetto: Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

Review

Quoted 9 lines · Ansuel Smith

Document force_gen1 optional definition to limit pcie line to GEN1 speed

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
 1 file changed, 5 insertions(+)
(quoted diff) +0 -0

b/Documentation/devicetree/bindings/pci/qcom,pcie.txt

Quoted 7 lines

index 8c1d014f37b0..766876465c42 100644

Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
@@ -260,6 +260,11 @@
 	Definition: If not defined is 0. In ipq806x is set to 7. In newer
 				revision (v2.0) the offset is zero.
Quoted 8 lines

Documentation/devicetree/bindings/pci/qcom,pcie.txt line 267

Quoted 5 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +5 -0
+- force_gen1:
+	Usage: optional
+	Value type: <u32>
+	Definition: Set 1 to force the pcie line to GEN1
+

I believe we have a standard property 'link-speed' for this purpose.

Yes this will be dropped in v2

Quoted 7 lines

Documentation/devicetree/bindings/pci/qcom,pcie.txt

Quoted 3 lines
Documentation/devicetree/bindings/pci/qcom,pcie.txt +0 -0
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064",

"snps,dw-pcie";

Quoted 3 lines

--
2.25.1

#27 lore raw

Re: [PATCH 11/12] devicetree: bindings: pci: add force_gen1 for qcom,pcie

In reply to Ansuel Smith

Hi Ansuel,

Before inventing new DT property I'd suggest you to consult with [1]. There is already property max-link-speed for that purpose.

Quoted 25 lines · Ansuel Smith

Document force_gen1 optional definition to limit pcie line to GEN1 speed

Signed-off-by Ansuel Smith <[email protected]>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/pci/qcom,pcie.txt +5 -0
index 8c1d014f37b0..766876465c42 100644
@@ -260,6 +260,11 @@
 	Definition: If not defined is 0. In ipq806x is set to 7. In newer
 				revision (v2.0) the offset is zero.
 
+- force_gen1:
+	Usage: optional
+	Value type: <u32>
+	Definition: Set 1 to force the pcie line to GEN1
+
 * Example for ipq/apq8064
 	pcie@1b500000 {
 		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
Signature · 4 lines
regards,
Stan

[1] Documentation/devicetree/bindings/pci/pci.txt
author
#28 lore raw

PATCH 12/12 pcie: qcom: Set PCIE MRRS and MPS to 256B

In reply to Ansuel Smith

From: Sriram Palanisamy <[email protected]>

Set Max Read Request Size and Max Payload Size to 256 bytes, per chip team recommendation.

Signed-off-by Gokul Sriram Palanisamy <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 37 ++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +37 -0
index 03130a3206b4..ad437c6f49a0 100644
@@ -125,6 +125,14 @@
 
 #define PCIE20_LNK_CONTROL2_LINK_STATUS2        0xA0
 
+#define __set(v, a, b)	(((v) << (b)) & GENMASK(a, b))
+#define __mask(a, b)	(((1 << ((a) + 1)) - 1) & ~((1 << (b)) - 1))
+#define PCIE20_DEV_CAS			0x78
+#define PCIE20_MRRS_MASK		__mask(14, 12)
+#define PCIE20_MRRS(x)			__set(x, 14, 12)
+#define PCIE20_MPS_MASK			__mask(7, 5)
+#define PCIE20_MPS(x)			__set(x, 7, 5)
+
 #define DEVICE_TYPE_RC				0x4
 
 #define QCOM_PCIE_2_1_0_MAX_SUPPLY	3

@@ -1595,6 +1603,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void qcom_pcie_fixup_final(struct pci_dev *pcidev)
+{
+	int cap, err;
+	u16 ctl, reg_val;
+
+	cap = pci_pcie_cap(pcidev);
+	if (!cap)
+		return;
+
+	err = pci_read_config_word(pcidev, cap + PCI_EXP_DEVCTL, &ctl);
+
+	if (err)
+		return;
+
+	reg_val = ctl;
+
+	if (((reg_val & PCIE20_MRRS_MASK) >> 12) > 1)
+		reg_val = (reg_val & ~(PCIE20_MRRS_MASK)) | PCIE20_MRRS(0x1);
+
+	if (((ctl & PCIE20_MPS_MASK) >> 5) > 1)
+		reg_val = (reg_val & ~(PCIE20_MPS_MASK)) | PCIE20_MPS(0x1);
+
+	err = pci_write_config_word(pcidev, cap + PCI_EXP_DEVCTL, reg_val);
+
+	if (err)
+		dev_err(&pcidev->dev, "pcie config write failed %d\n", err);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, qcom_pcie_fixup_final);
+
 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 },
Signature · 1 line
2.25.1
#29 lore raw

Re: [PATCH 12/12] pcie: qcom: Set PCIE MRRS and MPS to 256B

Quoted 4 lines · Ansuel Smith

From: Sriram Palanisamy <[email protected]>

Set Max Read Request Size and Max Payload Size to 256 bytes, per chip team recommendation.

Is this to avoid a device defect or to optimize performance?

This should not be done in an individual driver for performance reasons because these parameters need to be managed at the system level.

If this is to work around a device defect, we probably need to think about a quirk that changes the device capabilities advertised by this bridge and then changes to the PCI core code to take that into account.

Quoted 11 lines
Signed-off-by Gokul Sriram Palanisamy <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 37 ++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
drivers/pci/controller/dwc/pcie-qcom.c +0 -0
index 03130a3206b4..ad437c6f49a0 100644

drivers/pci/controller/dwc/pcie-qcom.c line 134

Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +7 -0
@@ -125,6 +125,14 @@
 
 #define PCIE20_LNK_CONTROL2_LINK_STATUS2        0xA0
 
+#define __set(v, a, b)	(((v) << (b)) & GENMASK(a, b))
+#define __mask(a, b)	(((1 << ((a) + 1)) - 1) & ~((1 << (b)) - 1))
+#define PCIE20_DEV_CAS			0x78
+#define PCIE20_MRRS_MASK		__mask(14, 12)
+#define PCIE20_MRRS(x)			__set(x, 14, 12)
+#define PCIE20_MPS_MASK			__mask(7, 5)
+#define PCIE20_MPS(x)			__set(x, 7, 5)

These should all be the generic PCI_EXP_DEVCTL_READRQ and similar #defines, since you use them on values from PCI_EXP_DEVCTL.

Quoted 42 lines
drivers/pci/controller/dwc/pcie-qcom.c +29 -0
 #define DEVICE_TYPE_RC				0x4
 
 #define QCOM_PCIE_2_1_0_MAX_SUPPLY	3

@@ -1595,6 +1603,35 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void qcom_pcie_fixup_final(struct pci_dev *pcidev)
+{
+	int cap, err;
+	u16 ctl, reg_val;
+
+	cap = pci_pcie_cap(pcidev);
+	if (!cap)
+		return;
+
+	err = pci_read_config_word(pcidev, cap + PCI_EXP_DEVCTL, &ctl);
+
+	if (err)
+		return;
+
+	reg_val = ctl;
+
+	if (((reg_val & PCIE20_MRRS_MASK) >> 12) > 1)
+		reg_val = (reg_val & ~(PCIE20_MRRS_MASK)) | PCIE20_MRRS(0x1);
+
+	if (((ctl & PCIE20_MPS_MASK) >> 5) > 1)
+		reg_val = (reg_val & ~(PCIE20_MPS_MASK)) | PCIE20_MPS(0x1);
+
+	err = pci_write_config_word(pcidev, cap + PCI_EXP_DEVCTL, reg_val);
+
+	if (err)
+		dev_err(&pcidev->dev, "pcie config write failed %d\n", err);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, qcom_pcie_fixup_final);
+
 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 },
Signature · 1 line
2.25.1
#30 lore raw
In reply to Ansuel Smith

Please add a cover letter with the patches as responses to it.

Make your subjects match in capitalization, verb tense, etc.:

  $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
  ed8cc3b1fc84 PCI: qcom: Add support for SDM845 PCIe controller
  64adde31c8e9 PCI: qcom: Ensure that PERST is asserted for at least 100 ms
  67021ae0bbe9 PCI: qcom: Add QCS404 PCIe controller support
  5aa180974e4d PCI: qcom: Use clk bulk API for 2.4.0 controllers
  322f03436692 PCI: qcom: Use default config space read function
  02b485e31d98 PCI: qcom: Don't deassert reset GPIO during probe
  6e5da6f7d824 PCI: qcom: Fix error handling in runtime PM support
  739cd35918b7 PCI: qcom: Drop unnecessary root_bus_nr setting
Quoted 3 lines · Ansuel Smith

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

s/pcie/PCIe/ in English text like commit logs and comments. s/inizialization/initialization/

It'd be useful to have enough context to know what "ipq806x" is.

Add period at end of sentence.

#31 lore raw
In reply to Ansuel Smith

Hi Ansuel,

As Bjorn already mentioned please make cover-letter in next version of the patchset so that we know what is the purpose of the patches.

I've the impression that you want to use pcie-qcom platform driver as an endpoint, am I wrong?

I'll review the patches these days.

Quoted 9 lines · Ansuel Smith

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

Signed-off-by Sham Muthayyan <[email protected]>
Signed-off-by Ansuel Smith <[email protected]>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)
Signature · 2 lines
regards,
Stan