All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] PCI: IPQ6018 PCIe controller support
@ 2021-12-27  6:46 ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Selvam Sathappan Periakaruppan, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

This series adds support for the single PCIe lane on IPQ6018 SoCs. The code is 
ported from downstream Codeaurora v5.4 kernel. The main difference from 
downstream code is the split of PCIe registers configuration from .init to 
.post_init, since it requires phy_power_on().

Tested on IPQ6010 based hardware.

Changes in v4:

  * Drop applied DT bits

  * Add max-link-speed that was missing from the applied v2 patch

  * Rebase the driver on v5.16-rc3

Changes in v3:

  * Drop applied patches

  * Rely on generic code for speed setup

  * Drop unused macros

  * Formatting fixes

Changes in v2:

  * Add patch moving GEN3_RELATED macros to a common header

  * Drop ATU configuration from pcie-qcom

  * Remove local definition of common registers

  * Use bulk clk and reset APIs

  * Remove msi-parent from device-tree

Baruch Siach (2):
  arm64: dts: qcom: ipq6018: add pcie max-link-speed
  PCI: dwc: tegra: move GEN3_RELATED DBI register to common header

Selvam Sathappan Periakaruppan (1):
  PCI: qcom: add support for IPQ60xx PCIe controller

 arch/arm64/boot/dts/qcom/ipq6018.dtsi        |   1 +
 drivers/pci/controller/dwc/pcie-designware.h |   7 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   |   6 -
 4 files changed, 153 insertions(+), 6 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 0/3] PCI: IPQ6018 PCIe controller support
@ 2021-12-27  6:46 ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Selvam Sathappan Periakaruppan, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

This series adds support for the single PCIe lane on IPQ6018 SoCs. The code is 
ported from downstream Codeaurora v5.4 kernel. The main difference from 
downstream code is the split of PCIe registers configuration from .init to 
.post_init, since it requires phy_power_on().

Tested on IPQ6010 based hardware.

Changes in v4:

  * Drop applied DT bits

  * Add max-link-speed that was missing from the applied v2 patch

  * Rebase the driver on v5.16-rc3

Changes in v3:

  * Drop applied patches

  * Rely on generic code for speed setup

  * Drop unused macros

  * Formatting fixes

Changes in v2:

  * Add patch moving GEN3_RELATED macros to a common header

  * Drop ATU configuration from pcie-qcom

  * Remove local definition of common registers

  * Use bulk clk and reset APIs

  * Remove msi-parent from device-tree

Baruch Siach (2):
  arm64: dts: qcom: ipq6018: add pcie max-link-speed
  PCI: dwc: tegra: move GEN3_RELATED DBI register to common header

Selvam Sathappan Periakaruppan (1):
  PCI: qcom: add support for IPQ60xx PCIe controller

 arch/arm64/boot/dts/qcom/ipq6018.dtsi        |   1 +
 drivers/pci/controller/dwc/pcie-designware.h |   7 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   |   6 -
 4 files changed, 153 insertions(+), 6 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
  2021-12-27  6:46 ` Baruch Siach
@ 2021-12-27  6:46   ` Baruch Siach
  -1 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Selvam Sathappan Periakaruppan, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

From: Baruch Siach <baruch.siach@siklu.com>

Add the generic 'max-link-speed' property to describe the IPQ6018 PCIe
link generation limit. This allows the generic dwc code to configure the
link speed correctly.

Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index a717fc17523d..665ee301b85d 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -438,6 +438,7 @@ pcie0: pci@20000000 {
 			linux,pci-domain = <0>;
 			bus-range = <0x00 0xff>;
 			num-lanes = <1>;
+			max-link-speed = <3>;
 			#address-cells = <3>;
 			#size-cells = <2>;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
@ 2021-12-27  6:46   ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Selvam Sathappan Periakaruppan, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

From: Baruch Siach <baruch.siach@siklu.com>

Add the generic 'max-link-speed' property to describe the IPQ6018 PCIe
link generation limit. This allows the generic dwc code to configure the
link speed correctly.

Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index a717fc17523d..665ee301b85d 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -438,6 +438,7 @@ pcie0: pci@20000000 {
 			linux,pci-domain = <0>;
 			bus-range = <0x00 0xff>;
 			num-lanes = <1>;
+			max-link-speed = <3>;
 			#address-cells = <3>;
 			#size-cells = <2>;
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 2/3] PCI: dwc: tegra: move GEN3_RELATED DBI register to common header
  2021-12-27  6:46 ` Baruch Siach
@ 2021-12-27  6:46   ` Baruch Siach
  -1 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Rob Herring, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra

From: Baruch Siach <baruch.siach@siklu.com>

These are common dwc macros that will be used for other platforms.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
 drivers/pci/controller/dwc/pcie-designware.h | 6 ++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b7576be..ea87809ee298 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -74,6 +74,12 @@
 #define PCIE_MSI_INTR0_MASK		0x82C
 #define PCIE_MSI_INTR0_STATUS		0x830
 
+#define GEN3_RELATED_OFF			0x890
+#define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
+#define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
+#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
+#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
+
 #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
 #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
 
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 904976913081..846c9d154f49 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -193,12 +193,6 @@
 #define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
 #define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK	GENMASK(3, 0)
 
-#define GEN3_RELATED_OFF			0x890
-#define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
-#define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
-#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
-#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
-
 #define PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT	0x8D0
 #define AMBA_ERROR_RESPONSE_CRS_SHIFT		3
 #define AMBA_ERROR_RESPONSE_CRS_MASK		GENMASK(1, 0)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 2/3] PCI: dwc: tegra: move GEN3_RELATED DBI register to common header
@ 2021-12-27  6:46   ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Baruch Siach, Rob Herring, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra

From: Baruch Siach <baruch.siach@siklu.com>

These are common dwc macros that will be used for other platforms.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
 drivers/pci/controller/dwc/pcie-designware.h | 6 ++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b7576be..ea87809ee298 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -74,6 +74,12 @@
 #define PCIE_MSI_INTR0_MASK		0x82C
 #define PCIE_MSI_INTR0_STATUS		0x830
 
+#define GEN3_RELATED_OFF			0x890
+#define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
+#define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
+#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
+#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
+
 #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
 #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
 
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 904976913081..846c9d154f49 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -193,12 +193,6 @@
 #define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
 #define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK	GENMASK(3, 0)
 
-#define GEN3_RELATED_OFF			0x890
-#define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
-#define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
-#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
-#define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
-
 #define PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT	0x8D0
 #define AMBA_ERROR_RESPONSE_CRS_SHIFT		3
 #define AMBA_ERROR_RESPONSE_CRS_MASK		GENMASK(1, 0)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
  2021-12-27  6:46 ` Baruch Siach
@ 2021-12-27  6:46   ` Baruch Siach
  -1 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Selvam Sathappan Periakaruppan, Baruch Siach, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>

IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
platform.

The code is based on downstream[1] Codeaurora kernel v5.4 (branch
win.linuxopenwrt.2.0).

Split out the DBI registers access part from .init into .post_init. DBI
registers are only accessible after phy_power_on().

[1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/

Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
v4:

  * Rebase on v5.16-rc1

v3:
  * Drop speed setup; rely on generic code (Rob Herring)

  * Drop unused CLK_RATE macros (Bjorn Helgaas)

  * Minor formatting fixes (Bjorn Helgaas)

  * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)

v2:
  * Drop ATU configuration; rely on common code instead

  * Use more common register macros

  * Use bulk clk and reset APIs
---
 drivers/pci/controller/dwc/pcie-designware.h |   1 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
 2 files changed, 146 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ea87809ee298..279c3778a13b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -76,6 +76,7 @@
 
 #define GEN3_RELATED_OFF			0x890
 #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
+#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
 #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 1c3d1116bb60..14f86c45a8d9 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -52,6 +52,10 @@
 #define PCIE20_PARF_DBI_BASE_ADDR		0x168
 #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
 #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
+#define AHB_CLK_EN				BIT(0)
+#define MSTR_AXI_CLK_EN				BIT(1)
+#define BYPASS					BIT(4)
+
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
 #define PCIE20_PARF_LTSSM			0x1B0
@@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
 	struct clk *ref_clk_src;
 };
 
+struct qcom_pcie_resources_2_9_0 {
+	struct clk_bulk_data clks[5];
+	struct reset_control *rst;
+};
+
 union qcom_pcie_resources {
 	struct qcom_pcie_resources_1_0_0 v1_0_0;
 	struct qcom_pcie_resources_2_1_0 v2_1_0;
@@ -178,6 +187,7 @@ union qcom_pcie_resources {
 	struct qcom_pcie_resources_2_3_3 v2_3_3;
 	struct qcom_pcie_resources_2_4_0 v2_4_0;
 	struct qcom_pcie_resources_2_7_0 v2_7_0;
+	struct qcom_pcie_resources_2_9_0 v2_9_0;
 };
 
 struct qcom_pcie;
@@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	res->clks[0].id = "iface";
+	res->clks[1].id = "axi_m";
+	res->clks[2].id = "axi_s";
+	res->clks[3].id = "axi_bridge";
+	res->clks[4].id = "rchng";
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0)
+		return ret;
+
+	res->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(res->rst))
+		return PTR_ERR(res->rst);
+
+	return 0;
+}
+
+static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
+}
+
+static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+	struct device *dev = pcie->pci->dev;
+	int ret;
+
+	ret = reset_control_assert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset assert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(2000, 2500);
+
+	ret = reset_control_deassert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset deassert failed (%d)\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Don't have a way to see if the reset has completed.
+	 * Wait for some time.
+	 */
+	usleep_range(2000, 2500);
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	if (ret)
+		goto err_reset;
+
+	return 0;
+
+	/*
+	 * Not checking for failure, will anyway return
+	 * the original failure in 'ret'.
+	 */
+err_reset:
+	reset_control_assert(res->rst);
+
+	return ret;
+}
+
+static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u32 val;
+	int i;
+
+	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);
+
+	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
+	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
+		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
+		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
+		pci->dbi_base + GEN3_RELATED_OFF);
+
+	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
+		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
+		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
+		pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+
+	/* Configure PCIe link capabilities for ASPM */
+	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
+	val &= ~PCI_EXP_LNKCAP_ASPMS;
+	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
+
+	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
+			PCI_EXP_DEVCTL2);
+
+	for (i = 0; i < 256; i++)
+		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
+				+ (4 * i));
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
+static const struct qcom_pcie_ops ops_2_9_0 = {
+	.get_resources = qcom_pcie_get_resources_2_9_0,
+	.init = qcom_pcie_init_2_9_0,
+	.post_init = qcom_pcie_post_init_2_9_0,
+	.deinit = qcom_pcie_deinit_2_9_0,
+	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
+};
+
 static const struct qcom_pcie_cfg apq8084_cfg = {
 	.ops = &ops_1_0_0,
 };
@@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.pipe_clk_need_muxing = true,
 };
 
+static const struct qcom_pcie_cfg ipq6018_cfg = {
+	.ops = &ops_2_9_0,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
@@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
 	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
 	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
+	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
 	{ }
 };
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
@ 2021-12-27  6:46   ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2021-12-27  6:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson
  Cc: Selvam Sathappan Periakaruppan, Baruch Siach, Kathiravan T,
	Bjorn Helgaas, Rob Herring, Thierry Reding, Jonathan Hunter,
	Jingoo Han, Gustavo Pimentel, Robert Marko, linux-pci,
	linux-arm-msm, linux-arm-kernel, linux-tegra

From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>

IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
platform.

The code is based on downstream[1] Codeaurora kernel v5.4 (branch
win.linuxopenwrt.2.0).

Split out the DBI registers access part from .init into .post_init. DBI
registers are only accessible after phy_power_on().

[1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/

Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
v4:

  * Rebase on v5.16-rc1

v3:
  * Drop speed setup; rely on generic code (Rob Herring)

  * Drop unused CLK_RATE macros (Bjorn Helgaas)

  * Minor formatting fixes (Bjorn Helgaas)

  * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)

v2:
  * Drop ATU configuration; rely on common code instead

  * Use more common register macros

  * Use bulk clk and reset APIs
---
 drivers/pci/controller/dwc/pcie-designware.h |   1 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
 2 files changed, 146 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ea87809ee298..279c3778a13b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -76,6 +76,7 @@
 
 #define GEN3_RELATED_OFF			0x890
 #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
+#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
 #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
 #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 1c3d1116bb60..14f86c45a8d9 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -52,6 +52,10 @@
 #define PCIE20_PARF_DBI_BASE_ADDR		0x168
 #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
 #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
+#define AHB_CLK_EN				BIT(0)
+#define MSTR_AXI_CLK_EN				BIT(1)
+#define BYPASS					BIT(4)
+
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
 #define PCIE20_PARF_LTSSM			0x1B0
@@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
 	struct clk *ref_clk_src;
 };
 
+struct qcom_pcie_resources_2_9_0 {
+	struct clk_bulk_data clks[5];
+	struct reset_control *rst;
+};
+
 union qcom_pcie_resources {
 	struct qcom_pcie_resources_1_0_0 v1_0_0;
 	struct qcom_pcie_resources_2_1_0 v2_1_0;
@@ -178,6 +187,7 @@ union qcom_pcie_resources {
 	struct qcom_pcie_resources_2_3_3 v2_3_3;
 	struct qcom_pcie_resources_2_4_0 v2_4_0;
 	struct qcom_pcie_resources_2_7_0 v2_7_0;
+	struct qcom_pcie_resources_2_9_0 v2_9_0;
 };
 
 struct qcom_pcie;
@@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->pipe_clk);
 }
 
+static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	res->clks[0].id = "iface";
+	res->clks[1].id = "axi_m";
+	res->clks[2].id = "axi_s";
+	res->clks[3].id = "axi_bridge";
+	res->clks[4].id = "rchng";
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0)
+		return ret;
+
+	res->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(res->rst))
+		return PTR_ERR(res->rst);
+
+	return 0;
+}
+
+static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
+}
+
+static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
+	struct device *dev = pcie->pci->dev;
+	int ret;
+
+	ret = reset_control_assert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset assert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(2000, 2500);
+
+	ret = reset_control_deassert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset deassert failed (%d)\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Don't have a way to see if the reset has completed.
+	 * Wait for some time.
+	 */
+	usleep_range(2000, 2500);
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	if (ret)
+		goto err_reset;
+
+	return 0;
+
+	/*
+	 * Not checking for failure, will anyway return
+	 * the original failure in 'ret'.
+	 */
+err_reset:
+	reset_control_assert(res->rst);
+
+	return ret;
+}
+
+static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u32 val;
+	int i;
+
+	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);
+
+	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
+	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
+		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
+		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
+		pci->dbi_base + GEN3_RELATED_OFF);
+
+	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
+		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
+		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
+		pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+
+	/* Configure PCIe link capabilities for ASPM */
+	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
+	val &= ~PCI_EXP_LNKCAP_ASPMS;
+	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
+
+	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
+			PCI_EXP_DEVCTL2);
+
+	for (i = 0; i < 256; i++)
+		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
+				+ (4 * i));
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct dw_pcie *pci)
 {
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
@@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
+/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
+static const struct qcom_pcie_ops ops_2_9_0 = {
+	.get_resources = qcom_pcie_get_resources_2_9_0,
+	.init = qcom_pcie_init_2_9_0,
+	.post_init = qcom_pcie_post_init_2_9_0,
+	.deinit = qcom_pcie_deinit_2_9_0,
+	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
+};
+
 static const struct qcom_pcie_cfg apq8084_cfg = {
 	.ops = &ops_1_0_0,
 };
@@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
 	.pipe_clk_need_muxing = true,
 };
 
+static const struct qcom_pcie_cfg ipq6018_cfg = {
+	.ops = &ops_2_9_0,
+};
+
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = qcom_pcie_link_up,
 	.start_link = qcom_pcie_start_link,
@@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
 	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
 	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
+	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
 	{ }
 };
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
  2021-12-27  6:46   ` Baruch Siach
@ 2022-01-06 14:45     ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 18+ messages in thread
From: Lorenzo Pieralisi @ 2022-01-06 14:45 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Andy Gross, Bjorn Andersson, Selvam Sathappan Periakaruppan,
	Baruch Siach, Kathiravan T, Bjorn Helgaas, Rob Herring,
	Thierry Reding, Jonathan Hunter, Jingoo Han, Gustavo Pimentel,
	Robert Marko, linux-pci, linux-arm-msm, linux-arm-kernel,
	linux-tegra, pali

[+Pali - query on reset delay]

On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> 
> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> platform.
> 
> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> win.linuxopenwrt.2.0).
> 
> Split out the DBI registers access part from .init into .post_init. DBI
> registers are only accessible after phy_power_on().
> 
> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> 
> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> ---
> v4:
> 
>   * Rebase on v5.16-rc1
> 
> v3:
>   * Drop speed setup; rely on generic code (Rob Herring)
> 
>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> 
>   * Minor formatting fixes (Bjorn Helgaas)
> 
>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> 
> v2:
>   * Drop ATU configuration; rely on common code instead
> 
>   * Use more common register macros
> 
>   * Use bulk clk and reset APIs
> ---
>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ea87809ee298..279c3778a13b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -76,6 +76,7 @@
>  
>  #define GEN3_RELATED_OFF			0x890
>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 1c3d1116bb60..14f86c45a8d9 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -52,6 +52,10 @@
>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> +#define AHB_CLK_EN				BIT(0)
> +#define MSTR_AXI_CLK_EN				BIT(1)
> +#define BYPASS					BIT(4)
> +
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
>  #define PCIE20_PARF_LTSSM			0x1B0
> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct clk *ref_clk_src;
>  };
>  
> +struct qcom_pcie_resources_2_9_0 {
> +	struct clk_bulk_data clks[5];
> +	struct reset_control *rst;
> +};
> +
>  union qcom_pcie_resources {
>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
>  };
>  
>  struct qcom_pcie;
> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->pipe_clk);
>  }
>  
> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	res->clks[0].id = "iface";
> +	res->clks[1].id = "axi_m";
> +	res->clks[2].id = "axi_s";
> +	res->clks[3].id = "axi_bridge";
> +	res->clks[4].id = "rchng";
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +	if (ret < 0)
> +		return ret;
> +
> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(res->rst))
> +		return PTR_ERR(res->rst);
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +}
> +
> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +	struct device *dev = pcie->pci->dev;
> +	int ret;
> +
> +	ret = reset_control_assert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(2000, 2500);
> +
> +	ret = reset_control_deassert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Don't have a way to see if the reset has completed.
> +	 * Wait for some time.

Is this arbitrary ? What does this reset represent ?

Thanks,
Lorenzo

> +	 */
> +	usleep_range(2000, 2500);
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +	if (ret)
> +		goto err_reset;
> +
> +	return 0;
> +
> +	/*
> +	 * Not checking for failure, will anyway return
> +	 * the original failure in 'ret'.
> +	 */
> +err_reset:
> +	reset_control_assert(res->rst);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u32 val;
> +	int i;
> +
> +	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);
> +
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> +		pci->dbi_base + GEN3_RELATED_OFF);
> +
> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +
> +	/* Configure PCIe link capabilities for ASPM */
> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> +
> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> +			PCI_EXP_DEVCTL2);
> +
> +	for (i = 0; i < 256; i++)
> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> +				+ (4 * i));
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.config_sid = qcom_pcie_config_sid_sm8250,
>  };
>  
> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> +static const struct qcom_pcie_ops ops_2_9_0 = {
> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> +	.init = qcom_pcie_init_2_9_0,
> +	.post_init = qcom_pcie_post_init_2_9_0,
> +	.deinit = qcom_pcie_deinit_2_9_0,
> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> +};
> +
>  static const struct qcom_pcie_cfg apq8084_cfg = {
>  	.ops = &ops_1_0_0,
>  };
> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.pipe_clk_need_muxing = true,
>  };
>  
> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> +	.ops = &ops_2_9_0,
> +};
> +
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
>  	{ }
>  };
>  
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
@ 2022-01-06 14:45     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 18+ messages in thread
From: Lorenzo Pieralisi @ 2022-01-06 14:45 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Andy Gross, Bjorn Andersson, Selvam Sathappan Periakaruppan,
	Baruch Siach, Kathiravan T, Bjorn Helgaas, Rob Herring,
	Thierry Reding, Jonathan Hunter, Jingoo Han, Gustavo Pimentel,
	Robert Marko, linux-pci, linux-arm-msm, linux-arm-kernel,
	linux-tegra, pali

[+Pali - query on reset delay]

On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> 
> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> platform.
> 
> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> win.linuxopenwrt.2.0).
> 
> Split out the DBI registers access part from .init into .post_init. DBI
> registers are only accessible after phy_power_on().
> 
> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> 
> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> ---
> v4:
> 
>   * Rebase on v5.16-rc1
> 
> v3:
>   * Drop speed setup; rely on generic code (Rob Herring)
> 
>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> 
>   * Minor formatting fixes (Bjorn Helgaas)
> 
>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> 
> v2:
>   * Drop ATU configuration; rely on common code instead
> 
>   * Use more common register macros
> 
>   * Use bulk clk and reset APIs
> ---
>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index ea87809ee298..279c3778a13b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -76,6 +76,7 @@
>  
>  #define GEN3_RELATED_OFF			0x890
>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 1c3d1116bb60..14f86c45a8d9 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -52,6 +52,10 @@
>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> +#define AHB_CLK_EN				BIT(0)
> +#define MSTR_AXI_CLK_EN				BIT(1)
> +#define BYPASS					BIT(4)
> +
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
>  #define PCIE20_PARF_LTSSM			0x1B0
> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
>  	struct clk *ref_clk_src;
>  };
>  
> +struct qcom_pcie_resources_2_9_0 {
> +	struct clk_bulk_data clks[5];
> +	struct reset_control *rst;
> +};
> +
>  union qcom_pcie_resources {
>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
>  };
>  
>  struct qcom_pcie;
> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->pipe_clk);
>  }
>  
> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	res->clks[0].id = "iface";
> +	res->clks[1].id = "axi_m";
> +	res->clks[2].id = "axi_s";
> +	res->clks[3].id = "axi_bridge";
> +	res->clks[4].id = "rchng";
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +	if (ret < 0)
> +		return ret;
> +
> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(res->rst))
> +		return PTR_ERR(res->rst);
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> +}
> +
> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> +	struct device *dev = pcie->pci->dev;
> +	int ret;
> +
> +	ret = reset_control_assert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	usleep_range(2000, 2500);
> +
> +	ret = reset_control_deassert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Don't have a way to see if the reset has completed.
> +	 * Wait for some time.

Is this arbitrary ? What does this reset represent ?

Thanks,
Lorenzo

> +	 */
> +	usleep_range(2000, 2500);
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +	if (ret)
> +		goto err_reset;
> +
> +	return 0;
> +
> +	/*
> +	 * Not checking for failure, will anyway return
> +	 * the original failure in 'ret'.
> +	 */
> +err_reset:
> +	reset_control_assert(res->rst);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u32 val;
> +	int i;
> +
> +	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);
> +
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> +		pci->dbi_base + GEN3_RELATED_OFF);
> +
> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> +
> +	/* Configure PCIe link capabilities for ASPM */
> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> +
> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> +			PCI_EXP_DEVCTL2);
> +
> +	for (i = 0; i < 256; i++)
> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> +				+ (4 * i));
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.config_sid = qcom_pcie_config_sid_sm8250,
>  };
>  
> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> +static const struct qcom_pcie_ops ops_2_9_0 = {
> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> +	.init = qcom_pcie_init_2_9_0,
> +	.post_init = qcom_pcie_post_init_2_9_0,
> +	.deinit = qcom_pcie_deinit_2_9_0,
> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> +};
> +
>  static const struct qcom_pcie_cfg apq8084_cfg = {
>  	.ops = &ops_1_0_0,
>  };
> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.pipe_clk_need_muxing = true,
>  };
>  
> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> +	.ops = &ops_2_9_0,
> +};
> +
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
>  	{ }
>  };
>  
> -- 
> 2.34.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
  2022-01-06 14:45     ` Lorenzo Pieralisi
@ 2022-01-06 18:05       ` Baruch Siach
  -1 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2022-01-06 18:05 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Andy Gross, Bjorn Andersson, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra, pali

Hi Lorenzo,

On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> [+Pali - query on reset delay]
>
> On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
>> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
>> 
>> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
>> platform.
>> 
>> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
>> win.linuxopenwrt.2.0).
>> 
>> Split out the DBI registers access part from .init into .post_init. DBI
>> registers are only accessible after phy_power_on().
>> 
>> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
>> 
>> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
>> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
>> ---
>> v4:
>> 
>>   * Rebase on v5.16-rc1
>> 
>> v3:
>>   * Drop speed setup; rely on generic code (Rob Herring)
>> 
>>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
>> 
>>   * Minor formatting fixes (Bjorn Helgaas)
>> 
>>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
>> 
>> v2:
>>   * Drop ATU configuration; rely on common code instead
>> 
>>   * Use more common register macros
>> 
>>   * Use bulk clk and reset APIs
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
>>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
>>  2 files changed, 146 insertions(+)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index ea87809ee298..279c3778a13b 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -76,6 +76,7 @@
>>  
>>  #define GEN3_RELATED_OFF			0x890
>>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
>> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
>>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
>>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
>>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 1c3d1116bb60..14f86c45a8d9 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -52,6 +52,10 @@
>>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
>>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
>>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
>> +#define AHB_CLK_EN				BIT(0)
>> +#define MSTR_AXI_CLK_EN				BIT(1)
>> +#define BYPASS					BIT(4)
>> +
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
>>  #define PCIE20_PARF_LTSSM			0x1B0
>> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
>>  	struct clk *ref_clk_src;
>>  };
>>  
>> +struct qcom_pcie_resources_2_9_0 {
>> +	struct clk_bulk_data clks[5];
>> +	struct reset_control *rst;
>> +};
>> +
>>  union qcom_pcie_resources {
>>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
>>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
>> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
>>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
>>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
>>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
>> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
>>  };
>>  
>>  struct qcom_pcie;
>> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>>  	clk_disable_unprepare(res->pipe_clk);
>>  }
>>  
>> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +	struct dw_pcie *pci = pcie->pci;
>> +	struct device *dev = pci->dev;
>> +	int ret;
>> +
>> +	res->clks[0].id = "iface";
>> +	res->clks[1].id = "axi_m";
>> +	res->clks[2].id = "axi_s";
>> +	res->clks[3].id = "axi_bridge";
>> +	res->clks[4].id = "rchng";
>> +
>> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	res->rst = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(res->rst))
>> +		return PTR_ERR(res->rst);
>> +
>> +	return 0;
>> +}
>> +
>> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
>> +}
>> +
>> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +	struct device *dev = pcie->pci->dev;
>> +	int ret;
>> +
>> +	ret = reset_control_assert(res->rst);
>> +	if (ret) {
>> +		dev_err(dev, "reset assert failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usleep_range(2000, 2500);
>> +
>> +	ret = reset_control_deassert(res->rst);
>> +	if (ret) {
>> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Don't have a way to see if the reset has completed.
>> +	 * Wait for some time.
>
> Is this arbitrary ? What does this reset represent ?

I have no idea. I'm just porting working downstream kernel code, and I
have no access to hardware documentation.

Note that some other variants also add delays before or after reset
deassert:

  qcom_pcie_init_2_4_0()

  qcom_pcie_init_2_3_3()

  qcom_pcie_init_2_7_0()

baruch

>> +	 */
>> +	usleep_range(2000, 2500);
>> +
>> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>> +	if (ret)
>> +		goto err_reset;
>> +
>> +	return 0;
>> +
>> +	/*
>> +	 * Not checking for failure, will anyway return
>> +	 * the original failure in 'ret'.
>> +	 */
>> +err_reset:
>> +	reset_control_assert(res->rst);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct dw_pcie *pci = pcie->pci;
>> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> +	u32 val;
>> +	int i;
>> +
>> +	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);
>> +
>> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
>> +
>> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
>> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
>> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
>> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
>> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
>> +		pci->dbi_base + GEN3_RELATED_OFF);
>> +
>> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
>> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
>> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
>> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
>> +
>> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
>> +
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
>> +
>> +	/* Configure PCIe link capabilities for ASPM */
>> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
>> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
>> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
>> +
>> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
>> +			PCI_EXP_DEVCTL2);
>> +
>> +	for (i = 0; i < 256; i++)
>> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
>> +				+ (4 * i));
>> +
>> +	return 0;
>> +}
>> +
>>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>>  {
>>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>  	.config_sid = qcom_pcie_config_sid_sm8250,
>>  };
>>  
>> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
>> +static const struct qcom_pcie_ops ops_2_9_0 = {
>> +	.get_resources = qcom_pcie_get_resources_2_9_0,
>> +	.init = qcom_pcie_init_2_9_0,
>> +	.post_init = qcom_pcie_post_init_2_9_0,
>> +	.deinit = qcom_pcie_deinit_2_9_0,
>> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>> +};
>> +
>>  static const struct qcom_pcie_cfg apq8084_cfg = {
>>  	.ops = &ops_1_0_0,
>>  };
>> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>>  	.pipe_clk_need_muxing = true,
>>  };
>>  
>> +static const struct qcom_pcie_cfg ipq6018_cfg = {
>> +	.ops = &ops_2_9_0,
>> +};
>> +
>>  static const struct dw_pcie_ops dw_pcie_ops = {
>>  	.link_up = qcom_pcie_link_up,
>>  	.start_link = qcom_pcie_start_link,
>> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
>>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
>>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
>>  	{ }
>>  };
>>  
>> -- 
>> 2.34.1
>> 


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
@ 2022-01-06 18:05       ` Baruch Siach
  0 siblings, 0 replies; 18+ messages in thread
From: Baruch Siach @ 2022-01-06 18:05 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Andy Gross, Bjorn Andersson, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra, pali

Hi Lorenzo,

On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> [+Pali - query on reset delay]
>
> On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
>> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
>> 
>> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
>> platform.
>> 
>> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
>> win.linuxopenwrt.2.0).
>> 
>> Split out the DBI registers access part from .init into .post_init. DBI
>> registers are only accessible after phy_power_on().
>> 
>> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
>> 
>> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
>> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
>> ---
>> v4:
>> 
>>   * Rebase on v5.16-rc1
>> 
>> v3:
>>   * Drop speed setup; rely on generic code (Rob Herring)
>> 
>>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
>> 
>>   * Minor formatting fixes (Bjorn Helgaas)
>> 
>>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
>> 
>> v2:
>>   * Drop ATU configuration; rely on common code instead
>> 
>>   * Use more common register macros
>> 
>>   * Use bulk clk and reset APIs
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
>>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
>>  2 files changed, 146 insertions(+)
>> 
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index ea87809ee298..279c3778a13b 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -76,6 +76,7 @@
>>  
>>  #define GEN3_RELATED_OFF			0x890
>>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
>> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
>>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
>>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
>>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 1c3d1116bb60..14f86c45a8d9 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -52,6 +52,10 @@
>>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
>>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
>>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
>> +#define AHB_CLK_EN				BIT(0)
>> +#define MSTR_AXI_CLK_EN				BIT(1)
>> +#define BYPASS					BIT(4)
>> +
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
>>  #define PCIE20_PARF_LTSSM			0x1B0
>> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
>>  	struct clk *ref_clk_src;
>>  };
>>  
>> +struct qcom_pcie_resources_2_9_0 {
>> +	struct clk_bulk_data clks[5];
>> +	struct reset_control *rst;
>> +};
>> +
>>  union qcom_pcie_resources {
>>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
>>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
>> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
>>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
>>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
>>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
>> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
>>  };
>>  
>>  struct qcom_pcie;
>> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
>>  	clk_disable_unprepare(res->pipe_clk);
>>  }
>>  
>> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +	struct dw_pcie *pci = pcie->pci;
>> +	struct device *dev = pci->dev;
>> +	int ret;
>> +
>> +	res->clks[0].id = "iface";
>> +	res->clks[1].id = "axi_m";
>> +	res->clks[2].id = "axi_s";
>> +	res->clks[3].id = "axi_bridge";
>> +	res->clks[4].id = "rchng";
>> +
>> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	res->rst = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(res->rst))
>> +		return PTR_ERR(res->rst);
>> +
>> +	return 0;
>> +}
>> +
>> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
>> +}
>> +
>> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> +	struct device *dev = pcie->pci->dev;
>> +	int ret;
>> +
>> +	ret = reset_control_assert(res->rst);
>> +	if (ret) {
>> +		dev_err(dev, "reset assert failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usleep_range(2000, 2500);
>> +
>> +	ret = reset_control_deassert(res->rst);
>> +	if (ret) {
>> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Don't have a way to see if the reset has completed.
>> +	 * Wait for some time.
>
> Is this arbitrary ? What does this reset represent ?

I have no idea. I'm just porting working downstream kernel code, and I
have no access to hardware documentation.

Note that some other variants also add delays before or after reset
deassert:

  qcom_pcie_init_2_4_0()

  qcom_pcie_init_2_3_3()

  qcom_pcie_init_2_7_0()

baruch

>> +	 */
>> +	usleep_range(2000, 2500);
>> +
>> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>> +	if (ret)
>> +		goto err_reset;
>> +
>> +	return 0;
>> +
>> +	/*
>> +	 * Not checking for failure, will anyway return
>> +	 * the original failure in 'ret'.
>> +	 */
>> +err_reset:
>> +	reset_control_assert(res->rst);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
>> +{
>> +	struct dw_pcie *pci = pcie->pci;
>> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> +	u32 val;
>> +	int i;
>> +
>> +	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);
>> +
>> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
>> +
>> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
>> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
>> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
>> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
>> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
>> +		pci->dbi_base + GEN3_RELATED_OFF);
>> +
>> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
>> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
>> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
>> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
>> +
>> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
>> +
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
>> +
>> +	/* Configure PCIe link capabilities for ASPM */
>> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
>> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
>> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
>> +
>> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
>> +			PCI_EXP_DEVCTL2);
>> +
>> +	for (i = 0; i < 256; i++)
>> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
>> +				+ (4 * i));
>> +
>> +	return 0;
>> +}
>> +
>>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>>  {
>>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>  	.config_sid = qcom_pcie_config_sid_sm8250,
>>  };
>>  
>> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
>> +static const struct qcom_pcie_ops ops_2_9_0 = {
>> +	.get_resources = qcom_pcie_get_resources_2_9_0,
>> +	.init = qcom_pcie_init_2_9_0,
>> +	.post_init = qcom_pcie_post_init_2_9_0,
>> +	.deinit = qcom_pcie_deinit_2_9_0,
>> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>> +};
>> +
>>  static const struct qcom_pcie_cfg apq8084_cfg = {
>>  	.ops = &ops_1_0_0,
>>  };
>> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>>  	.pipe_clk_need_muxing = true,
>>  };
>>  
>> +static const struct qcom_pcie_cfg ipq6018_cfg = {
>> +	.ops = &ops_2_9_0,
>> +};
>> +
>>  static const struct dw_pcie_ops dw_pcie_ops = {
>>  	.link_up = qcom_pcie_link_up,
>>  	.start_link = qcom_pcie_start_link,
>> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
>>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
>>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
>>  	{ }
>>  };
>>  
>> -- 
>> 2.34.1
>> 


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
  2022-01-06 18:05       ` Baruch Siach
@ 2022-01-06 23:20         ` Bjorn Andersson
  -1 siblings, 0 replies; 18+ messages in thread
From: Bjorn Andersson @ 2022-01-06 23:20 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Lorenzo Pieralisi, Andy Gross, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra, pali

On Thu 06 Jan 10:05 PST 2022, Baruch Siach wrote:

> Hi Lorenzo,
> 
> On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> > [+Pali - query on reset delay]
> >
> > On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> >> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> 
> >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> >> platform.
> >> 
> >> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> >> win.linuxopenwrt.2.0).
> >> 
> >> Split out the DBI registers access part from .init into .post_init. DBI
> >> registers are only accessible after phy_power_on().
> >> 
> >> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> >> 
> >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> >> ---
> >> v4:
> >> 
> >>   * Rebase on v5.16-rc1
> >> 
> >> v3:
> >>   * Drop speed setup; rely on generic code (Rob Herring)
> >> 
> >>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> >> 
> >>   * Minor formatting fixes (Bjorn Helgaas)
> >> 
> >>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> >> 
> >> v2:
> >>   * Drop ATU configuration; rely on common code instead
> >> 
> >>   * Use more common register macros
> >> 
> >>   * Use bulk clk and reset APIs
> >> ---
> >>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
> >>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
> >>  2 files changed, 146 insertions(+)
> >> 
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >> index ea87809ee298..279c3778a13b 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >> @@ -76,6 +76,7 @@
> >>  
> >>  #define GEN3_RELATED_OFF			0x890
> >>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> >> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
> >>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
> >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 1c3d1116bb60..14f86c45a8d9 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -52,6 +52,10 @@
> >>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
> >>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
> >>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> >> +#define AHB_CLK_EN				BIT(0)
> >> +#define MSTR_AXI_CLK_EN				BIT(1)
> >> +#define BYPASS					BIT(4)
> >> +
> >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
> >>  #define PCIE20_PARF_LTSSM			0x1B0
> >> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
> >>  	struct clk *ref_clk_src;
> >>  };
> >>  
> >> +struct qcom_pcie_resources_2_9_0 {
> >> +	struct clk_bulk_data clks[5];
> >> +	struct reset_control *rst;
> >> +};
> >> +
> >>  union qcom_pcie_resources {
> >>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
> >>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> >> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
> >>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
> >>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
> >>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> >> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
> >>  };
> >>  
> >>  struct qcom_pcie;
> >> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> >>  	clk_disable_unprepare(res->pipe_clk);
> >>  }
> >>  
> >> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +	struct device *dev = pci->dev;
> >> +	int ret;
> >> +
> >> +	res->clks[0].id = "iface";
> >> +	res->clks[1].id = "axi_m";
> >> +	res->clks[2].id = "axi_s";
> >> +	res->clks[3].id = "axi_bridge";
> >> +	res->clks[4].id = "rchng";
> >> +
> >> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(res->rst))
> >> +		return PTR_ERR(res->rst);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +
> >> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> >> +}
> >> +
> >> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +	struct device *dev = pcie->pci->dev;
> >> +	int ret;
> >> +
> >> +	ret = reset_control_assert(res->rst);
> >> +	if (ret) {
> >> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	usleep_range(2000, 2500);
> >> +
> >> +	ret = reset_control_deassert(res->rst);
> >> +	if (ret) {
> >> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Don't have a way to see if the reset has completed.
> >> +	 * Wait for some time.
> >
> > Is this arbitrary ? What does this reset represent ?
> 
> I have no idea. I'm just porting working downstream kernel code, and I
> have no access to hardware documentation.
> 

The reset here doesn't literally reset the device, it will assert (hold)
the reset line, then sleep 2ms, then deassert (release) it and the sleep
below will ensure that we don't enable the clocks etc until the hardware
has been given 2ms to "recover".

So it's not a matter of us waiting because we don't know how to check,
it's a matter of following the datasheet stating the minimum timing of
the operations to be performed to get the PCIe controller into a known
(clean) state..


I'm slightly puzzled to why this matters if the clocks are off, but if
you're porting downstream code, my suggestion is that you should just
omit the comment.

> Note that some other variants also add delays before or after reset
> deassert:
> 
>   qcom_pcie_init_2_4_0()
> 
>   qcom_pcie_init_2_3_3()
> 
>   qcom_pcie_init_2_7_0()
> 
> baruch
> 
> >> +	 */
> >> +	usleep_range(2000, 2500);
> >> +
> >> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> >> +	if (ret)
> >> +		goto err_reset;
> >> +
> >> +	return 0;
> >> +
> >> +	/*
> >> +	 * Not checking for failure, will anyway return
> >> +	 * the original failure in 'ret'.
> >> +	 */

I think you can omit this comment as well. You failed to enable the
clocks and you want to inform the caller about that error.

Also, you're asserting the reset line to put the hardware in reset
again, if that fails there's not much to do.

Regards,
Bjorn

> >> +err_reset:
> >> +	reset_control_assert(res->rst);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> +	u32 val;
> >> +	int i;
> >> +
> >> +	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);
> >> +
> >> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> >> +
> >> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> >> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> >> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> >> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> >> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> >> +		pci->dbi_base + GEN3_RELATED_OFF);
> >> +
> >> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> >> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> >> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> >> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> >> +
> >> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> >> +
> >> +	dw_pcie_dbi_ro_wr_en(pci);
> >> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> >> +
> >> +	/* Configure PCIe link capabilities for ASPM */
> >> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> >> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +
> >> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> >> +			PCI_EXP_DEVCTL2);
> >> +
> >> +	for (i = 0; i < 256; i++)
> >> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> >> +				+ (4 * i));
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int qcom_pcie_link_up(struct dw_pcie *pci)
> >>  {
> >>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> >>  	.config_sid = qcom_pcie_config_sid_sm8250,
> >>  };
> >>  
> >> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> >> +static const struct qcom_pcie_ops ops_2_9_0 = {
> >> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> >> +	.init = qcom_pcie_init_2_9_0,
> >> +	.post_init = qcom_pcie_post_init_2_9_0,
> >> +	.deinit = qcom_pcie_deinit_2_9_0,
> >> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> >> +};
> >> +
> >>  static const struct qcom_pcie_cfg apq8084_cfg = {
> >>  	.ops = &ops_1_0_0,
> >>  };
> >> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
> >>  	.pipe_clk_need_muxing = true,
> >>  };
> >>  
> >> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> >> +	.ops = &ops_2_9_0,
> >> +};
> >> +
> >>  static const struct dw_pcie_ops dw_pcie_ops = {
> >>  	.link_up = qcom_pcie_link_up,
> >>  	.start_link = qcom_pcie_start_link,
> >> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> >>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
> >>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> >>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> >> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
> >>  	{ }
> >>  };
> >>  
> >> -- 
> >> 2.34.1
> >> 
> 
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
@ 2022-01-06 23:20         ` Bjorn Andersson
  0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Andersson @ 2022-01-06 23:20 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Lorenzo Pieralisi, Andy Gross, Selvam Sathappan Periakaruppan,
	Kathiravan T, Bjorn Helgaas, Rob Herring, Thierry Reding,
	Jonathan Hunter, Jingoo Han, Gustavo Pimentel, Robert Marko,
	linux-pci, linux-arm-msm, linux-arm-kernel, linux-tegra, pali

On Thu 06 Jan 10:05 PST 2022, Baruch Siach wrote:

> Hi Lorenzo,
> 
> On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> > [+Pali - query on reset delay]
> >
> > On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> >> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> 
> >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> >> platform.
> >> 
> >> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> >> win.linuxopenwrt.2.0).
> >> 
> >> Split out the DBI registers access part from .init into .post_init. DBI
> >> registers are only accessible after phy_power_on().
> >> 
> >> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> >> 
> >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> >> ---
> >> v4:
> >> 
> >>   * Rebase on v5.16-rc1
> >> 
> >> v3:
> >>   * Drop speed setup; rely on generic code (Rob Herring)
> >> 
> >>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> >> 
> >>   * Minor formatting fixes (Bjorn Helgaas)
> >> 
> >>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> >> 
> >> v2:
> >>   * Drop ATU configuration; rely on common code instead
> >> 
> >>   * Use more common register macros
> >> 
> >>   * Use bulk clk and reset APIs
> >> ---
> >>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
> >>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
> >>  2 files changed, 146 insertions(+)
> >> 
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >> index ea87809ee298..279c3778a13b 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >> @@ -76,6 +76,7 @@
> >>  
> >>  #define GEN3_RELATED_OFF			0x890
> >>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> >> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
> >>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
> >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 1c3d1116bb60..14f86c45a8d9 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -52,6 +52,10 @@
> >>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
> >>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
> >>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> >> +#define AHB_CLK_EN				BIT(0)
> >> +#define MSTR_AXI_CLK_EN				BIT(1)
> >> +#define BYPASS					BIT(4)
> >> +
> >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
> >>  #define PCIE20_PARF_LTSSM			0x1B0
> >> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
> >>  	struct clk *ref_clk_src;
> >>  };
> >>  
> >> +struct qcom_pcie_resources_2_9_0 {
> >> +	struct clk_bulk_data clks[5];
> >> +	struct reset_control *rst;
> >> +};
> >> +
> >>  union qcom_pcie_resources {
> >>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
> >>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> >> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
> >>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
> >>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
> >>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> >> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
> >>  };
> >>  
> >>  struct qcom_pcie;
> >> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> >>  	clk_disable_unprepare(res->pipe_clk);
> >>  }
> >>  
> >> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +	struct device *dev = pci->dev;
> >> +	int ret;
> >> +
> >> +	res->clks[0].id = "iface";
> >> +	res->clks[1].id = "axi_m";
> >> +	res->clks[2].id = "axi_s";
> >> +	res->clks[3].id = "axi_bridge";
> >> +	res->clks[4].id = "rchng";
> >> +
> >> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(res->rst))
> >> +		return PTR_ERR(res->rst);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +
> >> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> >> +}
> >> +
> >> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +	struct device *dev = pcie->pci->dev;
> >> +	int ret;
> >> +
> >> +	ret = reset_control_assert(res->rst);
> >> +	if (ret) {
> >> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	usleep_range(2000, 2500);
> >> +
> >> +	ret = reset_control_deassert(res->rst);
> >> +	if (ret) {
> >> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Don't have a way to see if the reset has completed.
> >> +	 * Wait for some time.
> >
> > Is this arbitrary ? What does this reset represent ?
> 
> I have no idea. I'm just porting working downstream kernel code, and I
> have no access to hardware documentation.
> 

The reset here doesn't literally reset the device, it will assert (hold)
the reset line, then sleep 2ms, then deassert (release) it and the sleep
below will ensure that we don't enable the clocks etc until the hardware
has been given 2ms to "recover".

So it's not a matter of us waiting because we don't know how to check,
it's a matter of following the datasheet stating the minimum timing of
the operations to be performed to get the PCIe controller into a known
(clean) state..


I'm slightly puzzled to why this matters if the clocks are off, but if
you're porting downstream code, my suggestion is that you should just
omit the comment.

> Note that some other variants also add delays before or after reset
> deassert:
> 
>   qcom_pcie_init_2_4_0()
> 
>   qcom_pcie_init_2_3_3()
> 
>   qcom_pcie_init_2_7_0()
> 
> baruch
> 
> >> +	 */
> >> +	usleep_range(2000, 2500);
> >> +
> >> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> >> +	if (ret)
> >> +		goto err_reset;
> >> +
> >> +	return 0;
> >> +
> >> +	/*
> >> +	 * Not checking for failure, will anyway return
> >> +	 * the original failure in 'ret'.
> >> +	 */

I think you can omit this comment as well. You failed to enable the
clocks and you want to inform the caller about that error.

Also, you're asserting the reset line to put the hardware in reset
again, if that fails there's not much to do.

Regards,
Bjorn

> >> +err_reset:
> >> +	reset_control_assert(res->rst);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> +	u32 val;
> >> +	int i;
> >> +
> >> +	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);
> >> +
> >> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> >> +
> >> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> >> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> >> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> >> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> >> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> >> +		pci->dbi_base + GEN3_RELATED_OFF);
> >> +
> >> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> >> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> >> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> >> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> >> +
> >> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> >> +
> >> +	dw_pcie_dbi_ro_wr_en(pci);
> >> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> >> +
> >> +	/* Configure PCIe link capabilities for ASPM */
> >> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> >> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> >> +
> >> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> >> +			PCI_EXP_DEVCTL2);
> >> +
> >> +	for (i = 0; i < 256; i++)
> >> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> >> +				+ (4 * i));
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int qcom_pcie_link_up(struct dw_pcie *pci)
> >>  {
> >>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> >> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> >>  	.config_sid = qcom_pcie_config_sid_sm8250,
> >>  };
> >>  
> >> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> >> +static const struct qcom_pcie_ops ops_2_9_0 = {
> >> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> >> +	.init = qcom_pcie_init_2_9_0,
> >> +	.post_init = qcom_pcie_post_init_2_9_0,
> >> +	.deinit = qcom_pcie_deinit_2_9_0,
> >> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> >> +};
> >> +
> >>  static const struct qcom_pcie_cfg apq8084_cfg = {
> >>  	.ops = &ops_1_0_0,
> >>  };
> >> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
> >>  	.pipe_clk_need_muxing = true,
> >>  };
> >>  
> >> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> >> +	.ops = &ops_2_9_0,
> >> +};
> >> +
> >>  static const struct dw_pcie_ops dw_pcie_ops = {
> >>  	.link_up = qcom_pcie_link_up,
> >>  	.start_link = qcom_pcie_start_link,
> >> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> >>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
> >>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> >>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> >> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
> >>  	{ }
> >>  };
> >>  
> >> -- 
> >> 2.34.1
> >> 
> 
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
  2022-01-06 23:20         ` Bjorn Andersson
@ 2022-01-06 23:54           ` Pali Rohár
  -1 siblings, 0 replies; 18+ messages in thread
From: Pali Rohár @ 2022-01-06 23:54 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Baruch Siach, Lorenzo Pieralisi, Andy Gross,
	Selvam Sathappan Periakaruppan, Kathiravan T, Bjorn Helgaas,
	Rob Herring, Thierry Reding, Jonathan Hunter, Jingoo Han,
	Gustavo Pimentel, Robert Marko, linux-pci, linux-arm-msm,
	linux-arm-kernel, linux-tegra

On Thursday 06 January 2022 15:20:58 Bjorn Andersson wrote:
> On Thu 06 Jan 10:05 PST 2022, Baruch Siach wrote:
> 
> > Hi Lorenzo,
> > 
> > On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> > > [+Pali - query on reset delay]
> > >
> > > On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> > >> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> > >> 
> > >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> > >> platform.
> > >> 
> > >> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> > >> win.linuxopenwrt.2.0).
> > >> 
> > >> Split out the DBI registers access part from .init into .post_init. DBI
> > >> registers are only accessible after phy_power_on().
> > >> 
> > >> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> > >> 
> > >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> > >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> > >> ---
> > >> v4:
> > >> 
> > >>   * Rebase on v5.16-rc1
> > >> 
> > >> v3:
> > >>   * Drop speed setup; rely on generic code (Rob Herring)
> > >> 
> > >>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> > >> 
> > >>   * Minor formatting fixes (Bjorn Helgaas)
> > >> 
> > >>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> > >> 
> > >> v2:
> > >>   * Drop ATU configuration; rely on common code instead
> > >> 
> > >>   * Use more common register macros
> > >> 
> > >>   * Use bulk clk and reset APIs
> > >> ---
> > >>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
> > >>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
> > >>  2 files changed, 146 insertions(+)
> > >> 
> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > >> index ea87809ee298..279c3778a13b 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > >> @@ -76,6 +76,7 @@
> > >>  
> > >>  #define GEN3_RELATED_OFF			0x890
> > >>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> > >> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
> > >>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
> > >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> > >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > >> index 1c3d1116bb60..14f86c45a8d9 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > >> @@ -52,6 +52,10 @@
> > >>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
> > >>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
> > >>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> > >> +#define AHB_CLK_EN				BIT(0)
> > >> +#define MSTR_AXI_CLK_EN				BIT(1)
> > >> +#define BYPASS					BIT(4)
> > >> +
> > >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> > >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
> > >>  #define PCIE20_PARF_LTSSM			0x1B0
> > >> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
> > >>  	struct clk *ref_clk_src;
> > >>  };
> > >>  
> > >> +struct qcom_pcie_resources_2_9_0 {
> > >> +	struct clk_bulk_data clks[5];
> > >> +	struct reset_control *rst;
> > >> +};
> > >> +
> > >>  union qcom_pcie_resources {
> > >>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
> > >>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> > >> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
> > >>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
> > >>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
> > >>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> > >> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
> > >>  };
> > >>  
> > >>  struct qcom_pcie;
> > >> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> > >>  	clk_disable_unprepare(res->pipe_clk);
> > >>  }
> > >>  
> > >> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +	struct dw_pcie *pci = pcie->pci;
> > >> +	struct device *dev = pci->dev;
> > >> +	int ret;
> > >> +
> > >> +	res->clks[0].id = "iface";
> > >> +	res->clks[1].id = "axi_m";
> > >> +	res->clks[2].id = "axi_s";
> > >> +	res->clks[3].id = "axi_bridge";
> > >> +	res->clks[4].id = "rchng";
> > >> +
> > >> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> > >> +	if (ret < 0)
> > >> +		return ret;
> > >> +
> > >> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> > >> +	if (IS_ERR(res->rst))
> > >> +		return PTR_ERR(res->rst);
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +
> > >> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> > >> +}
> > >> +
> > >> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +	struct device *dev = pcie->pci->dev;
> > >> +	int ret;
> > >> +
> > >> +	ret = reset_control_assert(res->rst);
> > >> +	if (ret) {
> > >> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	usleep_range(2000, 2500);
> > >> +
> > >> +	ret = reset_control_deassert(res->rst);
> > >> +	if (ret) {
> > >> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	/*
> > >> +	 * Don't have a way to see if the reset has completed.
> > >> +	 * Wait for some time.
> > >
> > > Is this arbitrary ? What does this reset represent ?
> > 
> > I have no idea. I'm just porting working downstream kernel code, and I
> > have no access to hardware documentation.
> > 
> 
> The reset here doesn't literally reset the device, it will assert (hold)
> the reset line, then sleep 2ms, then deassert (release) it and the sleep
> below will ensure that we don't enable the clocks etc until the hardware
> has been given 2ms to "recover".

Hello! What kind of reset line it asserts? Some internal line to PCIe
controller IP? Or external PCIe Reset line from PCIe controller? Or some
other?

> So it's not a matter of us waiting because we don't know how to check,
> it's a matter of following the datasheet stating the minimum timing of
> the operations to be performed to get the PCIe controller into a known
> (clean) state..

It would be really useful to put comment into code to which datasheet
you are referring and also mention the information that this datasheet
states that timeout of YXZ ms is required.

Lorenzo, added me into this discussion as we found out that lot of PCIe
controller drivers are adding "random" delays into the "random" places
and lot of them just implement required delays defined in PCIe specs.
Something which is common for evert PCIe HW. This starting to be a big
mess as every driver has to reinvent wheel and so all "sleeping code"
should be properly documented. I have an idea of moving PCIe state
machine delays from drivers to PCI core code, but such thing would
require to correctly identify them. And probably there is no better way
than putting comments, why delay is required and why it was chosen to
specific value. If you want to contribute into this idea, look at email:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

> 
> I'm slightly puzzled to why this matters if the clocks are off, but if
> you're porting downstream code, my suggestion is that you should just
> omit the comment.
> 
> > Note that some other variants also add delays before or after reset
> > deassert:
> > 
> >   qcom_pcie_init_2_4_0()
> > 
> >   qcom_pcie_init_2_3_3()
> > 
> >   qcom_pcie_init_2_7_0()
> > 
> > baruch
> > 
> > >> +	 */
> > >> +	usleep_range(2000, 2500);
> > >> +
> > >> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> > >> +	if (ret)
> > >> +		goto err_reset;
> > >> +
> > >> +	return 0;
> > >> +
> > >> +	/*
> > >> +	 * Not checking for failure, will anyway return
> > >> +	 * the original failure in 'ret'.
> > >> +	 */
> 
> I think you can omit this comment as well. You failed to enable the
> clocks and you want to inform the caller about that error.
> 
> Also, you're asserting the reset line to put the hardware in reset
> again, if that fails there's not much to do.
> 
> Regards,
> Bjorn
> 
> > >> +err_reset:
> > >> +	reset_control_assert(res->rst);
> > >> +
> > >> +	return ret;
> > >> +}
> > >> +
> > >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct dw_pcie *pci = pcie->pci;
> > >> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >> +	u32 val;
> > >> +	int i;
> > >> +
> > >> +	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);
> > >> +
> > >> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > >> +
> > >> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> > >> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> > >> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > >> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> > >> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> > >> +		pci->dbi_base + GEN3_RELATED_OFF);
> > >> +
> > >> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> > >> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> > >> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> > >> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> > >> +
> > >> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> > >> +
> > >> +	dw_pcie_dbi_ro_wr_en(pci);
> > >> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> > >> +
> > >> +	/* Configure PCIe link capabilities for ASPM */
> > >> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> > >> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> > >> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> > >> +
> > >> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> > >> +			PCI_EXP_DEVCTL2);
> > >> +
> > >> +	for (i = 0; i < 256; i++)
> > >> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> > >> +				+ (4 * i));
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >>  static int qcom_pcie_link_up(struct dw_pcie *pci)
> > >>  {
> > >>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > >>  	.config_sid = qcom_pcie_config_sid_sm8250,
> > >>  };
> > >>  
> > >> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> > >> +static const struct qcom_pcie_ops ops_2_9_0 = {
> > >> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> > >> +	.init = qcom_pcie_init_2_9_0,
> > >> +	.post_init = qcom_pcie_post_init_2_9_0,
> > >> +	.deinit = qcom_pcie_deinit_2_9_0,
> > >> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > >> +};
> > >> +
> > >>  static const struct qcom_pcie_cfg apq8084_cfg = {
> > >>  	.ops = &ops_1_0_0,
> > >>  };
> > >> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
> > >>  	.pipe_clk_need_muxing = true,
> > >>  };
> > >>  
> > >> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> > >> +	.ops = &ops_2_9_0,
> > >> +};
> > >> +
> > >>  static const struct dw_pcie_ops dw_pcie_ops = {
> > >>  	.link_up = qcom_pcie_link_up,
> > >>  	.start_link = qcom_pcie_start_link,
> > >> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > >>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
> > >>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> > >>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > >> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
> > >>  	{ }
> > >>  };
> > >>  
> > >> -- 
> > >> 2.34.1
> > >> 
> > 
> > 
> > -- 
> >                                                      ~. .~   Tk Open Systems
> > =}------------------------------------------------ooO--U--Ooo------------{=
> >    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller
@ 2022-01-06 23:54           ` Pali Rohár
  0 siblings, 0 replies; 18+ messages in thread
From: Pali Rohár @ 2022-01-06 23:54 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Baruch Siach, Lorenzo Pieralisi, Andy Gross,
	Selvam Sathappan Periakaruppan, Kathiravan T, Bjorn Helgaas,
	Rob Herring, Thierry Reding, Jonathan Hunter, Jingoo Han,
	Gustavo Pimentel, Robert Marko, linux-pci, linux-arm-msm,
	linux-arm-kernel, linux-tegra

On Thursday 06 January 2022 15:20:58 Bjorn Andersson wrote:
> On Thu 06 Jan 10:05 PST 2022, Baruch Siach wrote:
> 
> > Hi Lorenzo,
> > 
> > On Thu, Jan 06 2022, Lorenzo Pieralisi wrote:
> > > [+Pali - query on reset delay]
> > >
> > > On Mon, Dec 27, 2021 at 08:46:05AM +0200, Baruch Siach wrote:
> > >> From: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> > >> 
> > >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> > >> platform.
> > >> 
> > >> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> > >> win.linuxopenwrt.2.0).
> > >> 
> > >> Split out the DBI registers access part from .init into .post_init. DBI
> > >> registers are only accessible after phy_power_on().
> > >> 
> > >> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> > >> 
> > >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> > >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> > >> ---
> > >> v4:
> > >> 
> > >>   * Rebase on v5.16-rc1
> > >> 
> > >> v3:
> > >>   * Drop speed setup; rely on generic code (Rob Herring)
> > >> 
> > >>   * Drop unused CLK_RATE macros (Bjorn Helgaas)
> > >> 
> > >>   * Minor formatting fixes (Bjorn Helgaas)
> > >> 
> > >>   * Add reference to downstream Codeaurora kernel tree (Bjorn Helgaas)
> > >> 
> > >> v2:
> > >>   * Drop ATU configuration; rely on common code instead
> > >> 
> > >>   * Use more common register macros
> > >> 
> > >>   * Use bulk clk and reset APIs
> > >> ---
> > >>  drivers/pci/controller/dwc/pcie-designware.h |   1 +
> > >>  drivers/pci/controller/dwc/pcie-qcom.c       | 145 +++++++++++++++++++
> > >>  2 files changed, 146 insertions(+)
> > >> 
> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > >> index ea87809ee298..279c3778a13b 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > >> @@ -76,6 +76,7 @@
> > >>  
> > >>  #define GEN3_RELATED_OFF			0x890
> > >>  #define GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL	BIT(0)
> > >> +#define GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS	BIT(13)
> > >>  #define GEN3_RELATED_OFF_GEN3_EQ_DISABLE	BIT(16)
> > >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> > >>  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> > >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > >> index 1c3d1116bb60..14f86c45a8d9 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > >> @@ -52,6 +52,10 @@
> > >>  #define PCIE20_PARF_DBI_BASE_ADDR		0x168
> > >>  #define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16C
> > >>  #define PCIE20_PARF_MHI_CLOCK_RESET_CTRL	0x174
> > >> +#define AHB_CLK_EN				BIT(0)
> > >> +#define MSTR_AXI_CLK_EN				BIT(1)
> > >> +#define BYPASS					BIT(4)
> > >> +
> > >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> > >>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2	0x1A8
> > >>  #define PCIE20_PARF_LTSSM			0x1B0
> > >> @@ -171,6 +175,11 @@ struct qcom_pcie_resources_2_7_0 {
> > >>  	struct clk *ref_clk_src;
> > >>  };
> > >>  
> > >> +struct qcom_pcie_resources_2_9_0 {
> > >> +	struct clk_bulk_data clks[5];
> > >> +	struct reset_control *rst;
> > >> +};
> > >> +
> > >>  union qcom_pcie_resources {
> > >>  	struct qcom_pcie_resources_1_0_0 v1_0_0;
> > >>  	struct qcom_pcie_resources_2_1_0 v2_1_0;
> > >> @@ -178,6 +187,7 @@ union qcom_pcie_resources {
> > >>  	struct qcom_pcie_resources_2_3_3 v2_3_3;
> > >>  	struct qcom_pcie_resources_2_4_0 v2_4_0;
> > >>  	struct qcom_pcie_resources_2_7_0 v2_7_0;
> > >> +	struct qcom_pcie_resources_2_9_0 v2_9_0;
> > >>  };
> > >>  
> > >>  struct qcom_pcie;
> > >> @@ -1297,6 +1307,127 @@ static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> > >>  	clk_disable_unprepare(res->pipe_clk);
> > >>  }
> > >>  
> > >> +static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +	struct dw_pcie *pci = pcie->pci;
> > >> +	struct device *dev = pci->dev;
> > >> +	int ret;
> > >> +
> > >> +	res->clks[0].id = "iface";
> > >> +	res->clks[1].id = "axi_m";
> > >> +	res->clks[2].id = "axi_s";
> > >> +	res->clks[3].id = "axi_bridge";
> > >> +	res->clks[4].id = "rchng";
> > >> +
> > >> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> > >> +	if (ret < 0)
> > >> +		return ret;
> > >> +
> > >> +	res->rst = devm_reset_control_array_get_exclusive(dev);
> > >> +	if (IS_ERR(res->rst))
> > >> +		return PTR_ERR(res->rst);
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +
> > >> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> > >> +}
> > >> +
> > >> +static int qcom_pcie_init_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > >> +	struct device *dev = pcie->pci->dev;
> > >> +	int ret;
> > >> +
> > >> +	ret = reset_control_assert(res->rst);
> > >> +	if (ret) {
> > >> +		dev_err(dev, "reset assert failed (%d)\n", ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	usleep_range(2000, 2500);
> > >> +
> > >> +	ret = reset_control_deassert(res->rst);
> > >> +	if (ret) {
> > >> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	/*
> > >> +	 * Don't have a way to see if the reset has completed.
> > >> +	 * Wait for some time.
> > >
> > > Is this arbitrary ? What does this reset represent ?
> > 
> > I have no idea. I'm just porting working downstream kernel code, and I
> > have no access to hardware documentation.
> > 
> 
> The reset here doesn't literally reset the device, it will assert (hold)
> the reset line, then sleep 2ms, then deassert (release) it and the sleep
> below will ensure that we don't enable the clocks etc until the hardware
> has been given 2ms to "recover".

Hello! What kind of reset line it asserts? Some internal line to PCIe
controller IP? Or external PCIe Reset line from PCIe controller? Or some
other?

> So it's not a matter of us waiting because we don't know how to check,
> it's a matter of following the datasheet stating the minimum timing of
> the operations to be performed to get the PCIe controller into a known
> (clean) state..

It would be really useful to put comment into code to which datasheet
you are referring and also mention the information that this datasheet
states that timeout of YXZ ms is required.

Lorenzo, added me into this discussion as we found out that lot of PCIe
controller drivers are adding "random" delays into the "random" places
and lot of them just implement required delays defined in PCIe specs.
Something which is common for evert PCIe HW. This starting to be a big
mess as every driver has to reinvent wheel and so all "sleeping code"
should be properly documented. I have an idea of moving PCIe state
machine delays from drivers to PCI core code, but such thing would
require to correctly identify them. And probably there is no better way
than putting comments, why delay is required and why it was chosen to
specific value. If you want to contribute into this idea, look at email:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

> 
> I'm slightly puzzled to why this matters if the clocks are off, but if
> you're porting downstream code, my suggestion is that you should just
> omit the comment.
> 
> > Note that some other variants also add delays before or after reset
> > deassert:
> > 
> >   qcom_pcie_init_2_4_0()
> > 
> >   qcom_pcie_init_2_3_3()
> > 
> >   qcom_pcie_init_2_7_0()
> > 
> > baruch
> > 
> > >> +	 */
> > >> +	usleep_range(2000, 2500);
> > >> +
> > >> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> > >> +	if (ret)
> > >> +		goto err_reset;
> > >> +
> > >> +	return 0;
> > >> +
> > >> +	/*
> > >> +	 * Not checking for failure, will anyway return
> > >> +	 * the original failure in 'ret'.
> > >> +	 */
> 
> I think you can omit this comment as well. You failed to enable the
> clocks and you want to inform the caller about that error.
> 
> Also, you're asserting the reset line to put the hardware in reset
> again, if that fails there's not much to do.
> 
> Regards,
> Bjorn
> 
> > >> +err_reset:
> > >> +	reset_control_assert(res->rst);
> > >> +
> > >> +	return ret;
> > >> +}
> > >> +
> > >> +static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
> > >> +{
> > >> +	struct dw_pcie *pci = pcie->pci;
> > >> +	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >> +	u32 val;
> > >> +	int i;
> > >> +
> > >> +	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);
> > >> +
> > >> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > >> +
> > >> +	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> > >> +	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
> > >> +		pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > >> +	writel(GEN3_RELATED_OFF_RXEQ_RGRDLESS_RXTS
> > >> +		| GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL,
> > >> +		pci->dbi_base + GEN3_RELATED_OFF);
> > >> +
> > >> +	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
> > >> +		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
> > >> +		AUX_PWR_DET | L23_CLK_RMV_DIS | L1_CLK_RMV_DIS,
> > >> +		pcie->parf + PCIE20_PARF_SYS_CTRL);
> > >> +
> > >> +	writel(0, pcie->parf + PCIE20_PARF_Q2A_FLUSH);
> > >> +
> > >> +	dw_pcie_dbi_ro_wr_en(pci);
> > >> +	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
> > >> +
> > >> +	/* Configure PCIe link capabilities for ASPM */
> > >> +	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
> > >> +	val &= ~PCI_EXP_LNKCAP_ASPMS;
> > >> +	writel(val, pci->dbi_base + offset + PCI_EXP_LNKCAP);
> > >> +
> > >> +	writel(PCI_EXP_DEVCTL2_COMP_TMOUT_DIS, pci->dbi_base + offset +
> > >> +			PCI_EXP_DEVCTL2);
> > >> +
> > >> +	for (i = 0; i < 256; i++)
> > >> +		writel(0x0, pcie->parf + PCIE20_PARF_BDF_TO_SID_TABLE_N
> > >> +				+ (4 * i));
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >>  static int qcom_pcie_link_up(struct dw_pcie *pci)
> > >>  {
> > >>  	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >> @@ -1487,6 +1618,15 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > >>  	.config_sid = qcom_pcie_config_sid_sm8250,
> > >>  };
> > >>  
> > >> +/* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> > >> +static const struct qcom_pcie_ops ops_2_9_0 = {
> > >> +	.get_resources = qcom_pcie_get_resources_2_9_0,
> > >> +	.init = qcom_pcie_init_2_9_0,
> > >> +	.post_init = qcom_pcie_post_init_2_9_0,
> > >> +	.deinit = qcom_pcie_deinit_2_9_0,
> > >> +	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > >> +};
> > >> +
> > >>  static const struct qcom_pcie_cfg apq8084_cfg = {
> > >>  	.ops = &ops_1_0_0,
> > >>  };
> > >> @@ -1520,6 +1660,10 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
> > >>  	.pipe_clk_need_muxing = true,
> > >>  };
> > >>  
> > >> +static const struct qcom_pcie_cfg ipq6018_cfg = {
> > >> +	.ops = &ops_2_9_0,
> > >> +};
> > >> +
> > >>  static const struct dw_pcie_ops dw_pcie_ops = {
> > >>  	.link_up = qcom_pcie_link_up,
> > >>  	.start_link = qcom_pcie_start_link,
> > >> @@ -1629,6 +1773,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > >>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
> > >>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> > >>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > >> +	{ .compatible = "qcom,pcie-ipq6018", .data = &ipq6018_cfg },
> > >>  	{ }
> > >>  };
> > >>  
> > >> -- 
> > >> 2.34.1
> > >> 
> > 
> > 
> > -- 
> >                                                      ~. .~   Tk Open Systems
> > =}------------------------------------------------ooO--U--Ooo------------{=
> >    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: (subset) [PATCH v4 1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
  2021-12-27  6:46   ` Baruch Siach
@ 2022-02-01  5:19     ` Bjorn Andersson
  -1 siblings, 0 replies; 18+ messages in thread
From: Bjorn Andersson @ 2022-02-01  5:19 UTC (permalink / raw)
  To: Andy Gross, Baruch Siach
  Cc: Rob Herring, linux-arm-kernel, Selvam Sathappan Periakaruppan,
	Kathiravan T, linux-tegra, Jonathan Hunter, Jingoo Han,
	Bjorn Helgaas, Robert Marko, linux-arm-msm, linux-pci,
	Thierry Reding, Gustavo Pimentel, Baruch Siach

On Mon, 27 Dec 2021 08:46:03 +0200, Baruch Siach wrote:
> From: Baruch Siach <baruch.siach@siklu.com>
> 
> Add the generic 'max-link-speed' property to describe the IPQ6018 PCIe
> link generation limit. This allows the generic dwc code to configure the
> link speed correctly.
> 
> 
> [...]

Applied, thanks!

[1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
      commit: e3e8a472429923d1c430bf388e9e3df1d9cc63a7

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: (subset) [PATCH v4 1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
@ 2022-02-01  5:19     ` Bjorn Andersson
  0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Andersson @ 2022-02-01  5:19 UTC (permalink / raw)
  To: Andy Gross, Baruch Siach
  Cc: Rob Herring, linux-arm-kernel, Selvam Sathappan Periakaruppan,
	Kathiravan T, linux-tegra, Jonathan Hunter, Jingoo Han,
	Bjorn Helgaas, Robert Marko, linux-arm-msm, linux-pci,
	Thierry Reding, Gustavo Pimentel, Baruch Siach

On Mon, 27 Dec 2021 08:46:03 +0200, Baruch Siach wrote:
> From: Baruch Siach <baruch.siach@siklu.com>
> 
> Add the generic 'max-link-speed' property to describe the IPQ6018 PCIe
> link generation limit. This allows the generic dwc code to configure the
> link speed correctly.
> 
> 
> [...]

Applied, thanks!

[1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed
      commit: e3e8a472429923d1c430bf388e9e3df1d9cc63a7

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-02-01  5:21 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-27  6:46 [PATCH v4 0/3] PCI: IPQ6018 PCIe controller support Baruch Siach
2021-12-27  6:46 ` Baruch Siach
2021-12-27  6:46 ` [PATCH v4 1/3] arm64: dts: qcom: ipq6018: add pcie max-link-speed Baruch Siach
2021-12-27  6:46   ` Baruch Siach
2022-02-01  5:19   ` (subset) " Bjorn Andersson
2022-02-01  5:19     ` Bjorn Andersson
2021-12-27  6:46 ` [PATCH v4 2/3] PCI: dwc: tegra: move GEN3_RELATED DBI register to common header Baruch Siach
2021-12-27  6:46   ` Baruch Siach
2021-12-27  6:46 ` [PATCH v4 3/3] PCI: qcom: add support for IPQ60xx PCIe controller Baruch Siach
2021-12-27  6:46   ` Baruch Siach
2022-01-06 14:45   ` Lorenzo Pieralisi
2022-01-06 14:45     ` Lorenzo Pieralisi
2022-01-06 18:05     ` Baruch Siach
2022-01-06 18:05       ` Baruch Siach
2022-01-06 23:20       ` Bjorn Andersson
2022-01-06 23:20         ` Bjorn Andersson
2022-01-06 23:54         ` Pali Rohár
2022-01-06 23:54           ` Pali Rohár

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.