Skip to contentSkip to conversation

arm64: IPQ6018 PCIe support

PATCH In review Baruch Siach 2 participants 7 messages Jan 24, 2022 – Jan 27, 2022

Subsystems (8)
lore
Share & tools
lore
#0 lore raw

v5 cover arm64: IPQ6018 PCIe support

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 v5:

* Remove comments from qcom_pcie_init_2_9_0() (Bjorn Andersson)

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       | 137 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   |   6 -
 4 files changed, 145 insertions(+), 6 deletions(-)
Signature · 1 line
2.34.1
#1 lore raw

PATCH v5 1/3 arm64: dts: qcom: ipq6018: add pcie max-link-speed

From: Baruch Siach <[email protected]>

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

---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/boot/dts/qcom/ipq6018.dtsi +1 -0
index a717fc17523d..665ee301b85d 100644
@@ -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>;
 
Signature · 1 line
2.34.1
#2 lore raw

PATCH v5 2/3 PCI: dwc: tegra: move GEN3_RELATED DBI register to common header

In reply to Baruch Siach

From: Baruch Siach <[email protected]>

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

---
 drivers/pci/controller/dwc/pcie-designware.h | 6 ++++++
 drivers/pci/controller/dwc/pcie-tegra194.c   | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/pci/controller/dwc/pcie-designware.h +6 -0
index 7d6e9b7576be..ea87809ee298 100644
@@ -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)
 
drivers/pci/controller/dwc/pcie-tegra194.c +0 -6
index b1b5f836a806..74bf66d9afeb 100644
@@ -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)
Signature · 1 line
2.34.1
#3 lore raw

PATCH v5 3/3 PCI: qcom: add support for IPQ60xx PCIe controller

In reply to Baruch Siach

From: Selvam Sathappan Periakaruppan <[email protected]>

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/

---
v5:

* Remove comments from qcom_pcie_init_2_9_0() (Bjorn Andersson)

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       | 137 +++++++++++++++++++
 2 files changed, 138 insertions(+)
drivers/pci/controller/dwc/pcie-designware.h +1 -0
index ea87809ee298..279c3778a13b 100644
@@ -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)
drivers/pci/controller/dwc/pcie-qcom.c +137 -0

179 lines not on this page · raw message

Signature · 1 line
2.34.1
#4 lore raw

Re: [PATCH v5 3/3] PCI: qcom: add support for IPQ60xx PCIe controller

Capitalize subject line to match history, i.e.,

  $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
  3f13d611aa6b PCI: qcom: Use __be16 type to store return value from cpu_to_be16()
  4e0e90539bb0 PCI: qcom: Fix an error handling path in 'qcom_pcie_probe()'
  45a3ec891370 PCI: qcom: Add sc8180x compatible
  aa9c0df98c29 PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
  b89ff410253d PCI: qcom: Replace ops with struct pcie_cfg in pcie match data
  2cfef1971aea PCI: qcom: Use PHY_REFCLK_USE_PAD only for ipq8064
  7081556f81f7 PCI: qcom: Add support for ddrss_sf_tbu clock
  4c9398822106 PCI: qcom: Add support for configuring BDF to SID mapping for SM8250
  e1dd639e374a PCI: qcom: Add SM8250 SoC support

A good one would be:

PCI: qcom: Add IPQ60xx support

We already know this is a PCIe controller, and it's nice to have the important words like "IPQ60xx" early in the message so they don't get truncated or wrapped.

Quoted 129 lines · Baruch Siach

From: Selvam Sathappan Periakaruppan <[email protected]>

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/

---
v5:

* Remove comments from qcom_pcie_init_2_9_0() (Bjorn Andersson)

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       | 137 +++++++++++++++++++
 2 files changed, 138 insertions(+)
drivers/pci/controller/dwc/pcie-designware.h +1 -0
index ea87809ee298..279c3778a13b 100644
@@ -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)
drivers/pci/controller/dwc/pcie-qcom.c +44 -0
index c19cd506ed3f..5d47e572da44 100644
@@ -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,119 @@ 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;

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1353

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

Where do these sleep durations come from? If they're specified somewhere by PCIe, can you include a citation, e.g., a section number in the spec.

Quoted 42 lines
drivers/pci/controller/dwc/pcie-qcom.c +42 -0
+	ret = reset_control_deassert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset deassert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(2000, 2500);
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	if (ret)
+		goto err_reset;
+
+	return 0;
+
+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);

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1406

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

Is "PCIE_CAP_LINK1_VAL" misnamed? If it's being written to PCI_EXP_SLTCAP, seems like it should be named something about a *slot*, not a *link*.

Or is this an error and it should this be written to PCI_EXP_LNKCAP instead?

Could PCIE_CAP_LINK1_VAL be defined in terms of PCI_EXP_SLTCAP_* or PCI_EXP_LNKCAP_*?

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1411

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

Thanks for using the standard PCI_EXP_LNKCAP and similar #defines. Does this mean you don't advertise any ASPM support on this platform? If so, that's fine, but the comment could be more specific. "Configure" doesn't give any hint about *how* they are configured.

Quoted 56 lines
drivers/pci/controller/dwc/pcie-qcom.c +24 -0
+	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 +1610,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 +1652,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 +1765,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 },
 	{ }
 };
 
Signature · 7 lines
2.34.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#5 lore raw

Re: [PATCH v5 3/3] PCI: qcom: add support for IPQ60xx PCIe controller

Hi Bjorn,

Thanks for your review comments.

Quoted 157 lines · Bjorn Helgaas

Capitalize subject line to match history, i.e.,

  $ git log --oneline drivers/pci/controller/dwc/pcie-qcom.c
  3f13d611aa6b PCI: qcom: Use __be16 type to store return value from cpu_to_be16()
  4e0e90539bb0 PCI: qcom: Fix an error handling path in 'qcom_pcie_probe()'
  45a3ec891370 PCI: qcom: Add sc8180x compatible
  aa9c0df98c29 PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280
  b89ff410253d PCI: qcom: Replace ops with struct pcie_cfg in pcie match data
  2cfef1971aea PCI: qcom: Use PHY_REFCLK_USE_PAD only for ipq8064
  7081556f81f7 PCI: qcom: Add support for ddrss_sf_tbu clock
  4c9398822106 PCI: qcom: Add support for configuring BDF to SID mapping for SM8250
  e1dd639e374a PCI: qcom: Add SM8250 SoC support

A good one would be:

PCI: qcom: Add IPQ60xx support

We already know this is a PCIe controller, and it's nice to have the important words like "IPQ60xx" early in the message so they don't get truncated or wrapped.

Quoted 135 lines · Baruch Siach

From: Selvam Sathappan Periakaruppan <[email protected]>

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/

---
v5:

* Remove comments from qcom_pcie_init_2_9_0() (Bjorn Andersson)

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       | 137 +++++++++++++++++++
 2 files changed, 138 insertions(+)
drivers/pci/controller/dwc/pcie-designware.h +1 -0
index ea87809ee298..279c3778a13b 100644
@@ -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)
drivers/pci/controller/dwc/pcie-qcom.c +50 -0
index c19cd506ed3f..5d47e572da44 100644
@@ -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,119 @@ 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);
Quoted 8 lines

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1353

Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +4 -0
+		return ret;
+	}
+
+	usleep_range(2000, 2500);

Where do these sleep durations come from? If they're specified somewhere by PCIe, can you include a citation, e.g., a section number in the spec.

As I mentioned before, I have no access to hardware documentation. I'm only porting working code from downstream kernel.

In a comment on v4 Bjorn Andersson mentioned "datasheet stating the minimum timing of the operations to be performed to get the PCIe controller into a known (clean) state".

https://lore.kernel.org/all/Ydd5Wh0KeADBQ%2Fh1@ripper/

I have no further details.

Quoted 57 lines
Quoted 42 lines
drivers/pci/controller/dwc/pcie-qcom.c +42 -0
+	ret = reset_control_deassert(res->rst);
+	if (ret) {
+		dev_err(dev, "reset deassert failed (%d)\n", ret);
+		return ret;
+	}
+
+	usleep_range(2000, 2500);
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	if (ret)
+		goto err_reset;
+
+	return 0;
+
+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);

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1406

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

Is "PCIE_CAP_LINK1_VAL" misnamed? If it's being written to PCI_EXP_SLTCAP, seems like it should be named something about a *slot*, not a *link*.

Quoted 2 lines

Or is this an error and it should this be written to PCI_EXP_LNKCAP instead?

As above, I don't know. I can only mention that the exact same assignment is currently also in qcom_pcie_init_2_3_3().

Quoted 2 lines

Could PCIE_CAP_LINK1_VAL be defined in terms of PCI_EXP_SLTCAP_* or PCI_EXP_LNKCAP_*?

It surely can. Whether it makes sense I can't tell.

Quoted 9 lines

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1411

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

Thanks for using the standard PCI_EXP_LNKCAP and similar #defines. Does this mean you don't advertise any ASPM support on this platform? If so, that's fine, but the comment could be more specific. "Configure" doesn't give any hint about *how* they are configured.

Again, the same code is in qcom_pcie_init_2_3_3(). I'll just drop the comment in the next submission.

Thanks,
baruch

Quoted 50 lines
Quoted 50 lines
drivers/pci/controller/dwc/pcie-qcom.c +24 -0
+	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 +1610,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 +1652,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 +1765,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 },
 	{ }
 };
 
Signature · 1 line
2.34.1
Signature · 8 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#6 lore raw

Re: [PATCH v5 3/3] PCI: qcom: add support for IPQ60xx PCIe controller

Quoted 21 lines · Baruch Siach
Quoted 11 lines · Bjorn Helgaas
Quoted 9 lines · Baruch Siach
drivers/pci/controller/dwc/pcie-qcom.c +9 -0
+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);
Quoted 8 lines

drivers/pci/controller/dwc/pcie-qcom.c static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie) · line 1353

Quoted 4 lines
drivers/pci/controller/dwc/pcie-qcom.c +4 -0
+		return ret;
+	}
+
+	usleep_range(2000, 2500);

Where do these sleep durations come from? If they're specified somewhere by PCIe, can you include a citation, e.g., a section number in the spec.

Quoted 6 lines

As I mentioned before, I have no access to hardware documentation. I'm only porting working code from downstream kernel.

In a comment on v4 Bjorn Andersson mentioned "datasheet stating the minimum timing of the operations to be performed to get the PCIe controller into a known (clean) state".

Sorry if I'm repeating questions. We can help avoid that by adding a brief comment here like "black magic copied from working code at X" or whatever you *do* know. Repeated questions are wasted effort for both the author and reviewers.

Sleeps with unsourced durations are always suspect because nobody wants to wait longer than necessary, and if all we have is a bare number and nobody knows how it was derived, we can't be confident about changing it.

Quoted 3 lines

https://lore.kernel.org/all/Ydd5Wh0KeADBQ%2Fh1@ripper/

I have no further details.