Skip to contentSkip to conversation

PCI: IPQ6018 platform support

PATCH Superseded Baruch Siach 4 participants 11 messages Jun 12, 2022 – Jun 21, 2022

Subsystems (6)

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v7 cover PCI: IPQ6018 platform 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 v7:

* Use FIELD_PREP for power limit and scale fields

* Add Stanimir Varbanov to Cc

* Rebase on v5.19-rc1

Changes in v6:

* Drop DT patch applied to the qcom tree

* Normalize driver changes subject line

* Add a preparatory patch to rename PCIE_CAP_LINK1_VAL to PCIE_CAP_SLOT_VAL, and define it using PCI_EXP_SLTCAP_* macros

* Drop a vague comment about ASPM configuration

* Add a comment about the source of delay periods

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):
PCI: dwc: tegra: move GEN3_RELATED DBI register to common header
PCI: qcom: Define slot capabilities using PCI_EXP_SLTCAP_*

Selvam Sathappan Periakaruppan (1):
PCI: qcom: Add IPQ60xx support

 drivers/pci/controller/dwc/pcie-designware.h |   7 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 157 ++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-tegra194.c   |   6 -
 3 files changed, 162 insertions(+), 8 deletions(-)
Signature · 1 line
2.35.1
#1 lore raw

PATCH v7 1/3 PCI: dwc: tegra: move GEN3_RELATED DBI register to common header

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 cc2678490162..0190786a24b8 100644
@@ -191,12 +191,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.35.1
#2 lore raw

PATCH v7 2/3 PCI: qcom: Define slot capabilities using PCI_EXP_SLTCAP_*

In reply to Baruch Siach

From: Baruch Siach <[email protected]>

The PCIE_CAP_LINK1_VAL macro actually defines slot capabilities. Use PCI_EXP_SLTCAP_* macros to spell its value, and rename it to better describe its meaning.

---
v7:
Use FIELD_PREP for power limit and stale (Pali Rohár)

---
 drivers/pci/controller/dwc/pcie-qcom.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pcie-qcom.c +15 -2
index 2ea13750b492..5ad9be6372f4 100644
@@ -69,7 +69,20 @@
 #define PCIE20_AXI_MSTR_RESP_COMP_CTRL1		0x81c
 #define CFG_BRIDGE_SB_INIT			BIT(0)
 
-#define PCIE_CAP_LINK1_VAL			0x2FD7F
+#define PCIE_CAP_SLOT_POWER_LIMIT_VAL		FIELD_PREP(PCI_EXP_SLTCAP_SPLV, \
+						250)
+#define PCIE_CAP_SLOT_POWER_LIMIT_SCALE		FIELD_PREP(PCI_EXP_SLTCAP_SPLS, \
+						1)
+#define PCIE_CAP_SLOT_VAL			(PCI_EXP_SLTCAP_ABP | \
+						PCI_EXP_SLTCAP_PCP | \
+						PCI_EXP_SLTCAP_MRLSP | \
+						PCI_EXP_SLTCAP_AIP | \
+						PCI_EXP_SLTCAP_PIP | \
+						PCI_EXP_SLTCAP_HPS | \
+						PCI_EXP_SLTCAP_HPC | \
+						PCI_EXP_SLTCAP_EIP | \
+						PCIE_CAP_SLOT_POWER_LIMIT_VAL | \
+						PCIE_CAP_SLOT_POWER_LIMIT_SCALE)
 
 #define PCIE20_PARF_Q2A_FLUSH			0x1AC
 
@@ -1114,7 +1127,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 
 	writel(PCI_COMMAND_MASTER, pci->dbi_base + PCI_COMMAND);
 	writel(DBI_RO_WR_EN, pci->dbi_base + PCIE20_MISC_CONTROL_1_REG);
-	writel(PCIE_CAP_LINK1_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+	writel(PCIE_CAP_SLOT_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
 
 	val = readl(pci->dbi_base + offset + PCI_EXP_LNKCAP);
 	val &= ~PCI_EXP_LNKCAP_ASPMS;
Signature · 1 line
2.35.1
reviewer Reviewed-by on v7 2/3
#3 lore raw

Re: [PATCH v7 2/3] PCI: qcom: Define slot capabilities using PCI_EXP_SLTCAP_*

Quoted 6 lines · Baruch Siach

From: Baruch Siach <[email protected]>

The PCIE_CAP_LINK1_VAL macro actually defines slot capabilities. Use PCI_EXP_SLTCAP_* macros to spell its value, and rename it to better describe its meaning.

Quoted 7 lines

---
v7:
Use FIELD_PREP for power limit and stale (Pali Rohár)

---
 drivers/pci/controller/dwc/pcie-qcom.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

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

Acked-by on v7 2/3
#4 lore raw

Re: [PATCH v7 2/3] PCI: qcom: Define slot capabilities using PCI_EXP_SLTCAP_*

In reply to Baruch Siach
Quoted 6 lines · Baruch Siach

From: Baruch Siach <[email protected]>

The PCIE_CAP_LINK1_VAL macro actually defines slot capabilities. Use PCI_EXP_SLTCAP_* macros to spell its value, and rename it to better describe its meaning.

Quoted 7 lines

---
v7:
Use FIELD_PREP for power limit and stale (Pali Rohár)

---
 drivers/pci/controller/dwc/pcie-qcom.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
Signature · 2 lines
regards,
Stan
#5 lore raw

PATCH v7 3/3 PCI: qcom: Add IPQ60xx support

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/

---
v7:

* Rebase on v5.19-rc1 (Bjorn Helgaas)

v6:

Address Bjorn Helgaas comments:

* Rename PCIE_CAP_LINK1_VAL to PCIE_CAP_SLOT_VAL

* Drop a vague comment about ASPM configuration

* Add a comment about the source of delay periods

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       | 140 +++++++++++++++++++
 2 files changed, 141 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 +140 -0

182 lines not on this page · raw message

Signature · 1 line
2.35.1
reviewer Reviewed-by on v7 3/3
#6 lore raw

Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

Quoted 50 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/

---
v7:

* Rebase on v5.19-rc1 (Bjorn Helgaas)

v6:

Address Bjorn Helgaas comments:

* Rename PCIE_CAP_LINK1_VAL to PCIE_CAP_SLOT_VAL

* Drop a vague comment about ASPM configuration

* Add a comment about the source of delay periods

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

Quoted 7 lines

* 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       | 140 +++++++++++++++++++
 2 files changed, 141 insertions(+)

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

Acked-by on v7 3/3
#7 lore raw

Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

In reply to Baruch Siach

Hi,

Thanks for the patch!

Quoted 8 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).

Quoted 7 lines

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/

Looks good:

Signature · 7 lines
regards,
Stan

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

Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

In reply to Baruch Siach
Quoted 17 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/

---

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

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

Assert reset as you do in the init error path?

Quoted 51 lines
drivers/pci/controller/dwc/pcie-qcom.c +51 -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);
+		return ret;
+	}
+
+	/*
+	 * Delay periods before and after reset deassert are working values
+	 * from downstream Codeaurora kernel
+	 */
+	usleep_range(2000, 2500);
+
+	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);

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

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

I noticed that some of this probably has been copied from from qcom_pcie_init_2_3_3(), but please move the | operator to the previous line.

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

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

Same here.

Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +8 -0
+		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_SLOT_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+

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

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

And here for +, but you should probably just remove the line break (you can go up to 100 chars if it makes the code more readable).

Please drop the 0x prefix too.

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

Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +3 -0
+
+	return 0;
+}

Johan

#9 lore raw

Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

Hi Johan,

Thanks for your review comments.

Quoted 18 lines · Johan Hovold
Quoted 16 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/

---

Quoted 7 lines

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

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

Assert reset as you do in the init error path?

Not sure about that. As I understand, the reset assert/deassert sequence on init is meant to ensure clean startup state. Deinit most likely does not need that. So maybe I should remove reset assert from init error path instead?

As always, this code sequence is from downstream kernel. I have no access to documentation.

baruch

Quoted 102 lines
Quoted 51 lines
drivers/pci/controller/dwc/pcie-qcom.c +51 -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);
+		return ret;
+	}
+
+	/*
+	 * Delay periods before and after reset deassert are working values
+	 * from downstream Codeaurora kernel
+	 */
+	usleep_range(2000, 2500);
+
+	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);

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

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

I noticed that some of this probably has been copied from from qcom_pcie_init_2_3_3(), but please move the | operator to the previous line.

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

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

Same here.

Quoted 8 lines
drivers/pci/controller/dwc/pcie-qcom.c +8 -0
+		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_SLOT_VAL, pci->dbi_base + offset + PCI_EXP_SLTCAP);
+

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

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

And here for +, but you should probably just remove the line break (you can go up to 100 chars if it makes the code more readable).

Please drop the 0x prefix too.

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

Quoted 3 lines
drivers/pci/controller/dwc/pcie-qcom.c +3 -0
+
+	return 0;
+}

Johan

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
#10 lore raw

Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

Quoted 31 lines · Baruch Siach

Hi Johan,

Thanks for your review comments.

Quoted 18 lines · Johan Hovold
Quoted 16 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/

---

Quoted 7 lines

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

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

Assert reset as you do in the init error path?

Quoted 4 lines

Not sure about that. As I understand, the reset assert/deassert sequence on init is meant to ensure clean startup state. Deinit most likely does not need that. So maybe I should remove reset assert from init error path instead?

Yeah, I think the init error path and deinit should at least be consistent.

Quoted 2 lines

As always, this code sequence is from downstream kernel. I have no access to documentation.

I feel your pain. ;)

Johan