linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Add Qualcomm PCIe Endpoint driver support
@ 2021-06-03 10:38 Manivannan Sadhasivam
  2021-06-03 10:38 ` [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-03 10:38 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh, bhelgaas
  Cc: bjorn.andersson, linux-arm-msm, linux-pci, devicetree,
	linux-kernel, Manivannan Sadhasivam

Hello,

This series adds support for Qualcomm PCIe Endpoint controller found
in platforms like SDX55. The Endpoint controller is based on the designware
core with additional Qualcomm wrappers around the core.

The driver is added separately unlike other Designware based drivers that
combine RC and EP in a single driver. This is done to avoid complexity and
to maintain this driver autonomously.

The driver has been validated with an out of tree MHI function driver on
SDX55 based Telit FN980 EVB connected to x86 host machine over PCIe.

Thanks,
Mani

Changes in v2:

* Addressed the comments from Rob on bindings patch
* Modified the driver as per binding change
* Fixed the warnings reported by Kbuild bot
* Removed the PERST# "enable_irq" call from probe()

Manivannan Sadhasivam (3):
  dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP
    controller
  PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding

 .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++
 MAINTAINERS                                   |  10 +-
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-qcom-ep.c     | 780 ++++++++++++++++++
 5 files changed, 944 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c

-- 
2.25.1


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

* [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller
  2021-06-03 10:38 [PATCH v2 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
@ 2021-06-03 10:38 ` Manivannan Sadhasivam
  2021-06-06  3:13   ` Bjorn Andersson
  2021-06-03 10:38 ` [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
  2021-06-03 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding Manivannan Sadhasivam
  2 siblings, 1 reply; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-03 10:38 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh, bhelgaas
  Cc: bjorn.andersson, linux-arm-msm, linux-pci, devicetree,
	linux-kernel, Manivannan Sadhasivam

Add devicetree binding for Qualcomm PCIe EP controller used in platforms
like SDX55. The EP controller is based on the Designware core with
Qualcomm specific wrappers.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
new file mode 100644
index 000000000000..3e357cb03a5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
@@ -0,0 +1,144 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PCIe Endpoint Controller binding
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+allOf:
+  - $ref: "pci-ep.yaml#"
+
+properties:
+  compatible:
+    const: qcom,sdx55-pcie-ep
+
+  reg:
+    items:
+      - description: Qualcomm specific PARF configuration registers
+      - description: Designware PCIe registers
+      - description: External local bus interface registers
+      - description: Address Translation Unit (ATU) registers
+      - description: Memory region used to map remote RC address space
+      - description: Qualcomm specific TCSR registers
+
+  reg-names:
+    items:
+      - const: parf
+      - const: dbi
+      - const: elbi
+      - const: atu
+      - const: addr_space
+      - const: tcsr
+
+  clocks:
+    items:
+      - description: PCIe Auxiliary clock
+      - description: PCIe CFG AHB clock
+      - description: PCIe Master AXI clock
+      - description: PCIe Slave AXI clock
+      - description: PCIe Slave Q2A AXI clock
+      - description: PCIe Sleep clock
+      - description: PCIe Reference clock
+
+  clock-names:
+    items:
+      - const: aux
+      - const: cfg
+      - const: bus_master
+      - const: bus_slave
+      - const: slave_q2a
+      - const: sleep
+      - const: ref
+
+  interrupts:
+    maxItems: 1
+    description: PCIe Global interrupt
+
+  interrupt-names:
+    const: global
+
+  reset-gpios:
+    description: GPIO that is being used as PERST# input signal
+    maxItems: 1
+
+  wake-gpios:
+    description: GPIO that is being used as WAKE# output signal
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: core
+
+  power-domains:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pciephy
+
+  num-lanes:
+    default: 2
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - reset-gpios
+  - resets
+  - reset-names
+  - power-domains
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sdx55.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    pcie_ep: pcie-ep@40000000 {
+        compatible = "qcom,sdx55-pcie-ep";
+        reg = <0x01c00000 0x3000>,
+              <0x40000000 0xf1d>,
+              <0x40000f20 0xc8>,
+              <0x40001000 0x1000>,
+              <0x42000000 0x1000>,
+              <0x01fcb000 0x1000>;
+        reg-names = "parf", "dbi", "elbi", "atu", "addr_space",
+                    "tcsr";
+
+        clocks = <&gcc GCC_PCIE_AUX_CLK>,
+             <&gcc GCC_PCIE_CFG_AHB_CLK>,
+             <&gcc GCC_PCIE_MSTR_AXI_CLK>,
+             <&gcc GCC_PCIE_SLV_AXI_CLK>,
+             <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,
+             <&gcc GCC_PCIE_SLEEP_CLK>,
+             <&gcc GCC_PCIE_0_CLKREF_CLK>;
+        clock-names = "aux", "cfg", "bus_master", "bus_slave",
+                      "slave_q2a", "sleep", "ref";
+
+        interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "global";
+        reset-gpios = <&tlmm 57 GPIO_ACTIVE_HIGH>;
+        wake-gpios = <&tlmm 53 GPIO_ACTIVE_LOW>;
+        resets = <&gcc GCC_PCIE_BCR>;
+        reset-names = "core";
+        power-domains = <&gcc PCIE_GDSC>;
+        phys = <&pcie0_lane>;
+        phy-names = "pciephy";
+        max-link-speed = <3>;
+        num-lanes = <2>;
+
+        status = "disabled";
+    };
-- 
2.25.1


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

* [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-03 10:38 [PATCH v2 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
  2021-06-03 10:38 ` [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
@ 2021-06-03 10:38 ` Manivannan Sadhasivam
  2021-06-05 22:02   ` Stanimir Varbanov
  2021-06-06  3:07   ` Bjorn Andersson
  2021-06-03 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding Manivannan Sadhasivam
  2 siblings, 2 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-03 10:38 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh, bhelgaas
  Cc: bjorn.andersson, linux-arm-msm, linux-pci, devicetree,
	linux-kernel, Manivannan Sadhasivam, Siddartha Mohanadoss

Add driver support for Qualcomm PCIe Endpoint controller driver based on
the Designware core with added Qualcomm specific wrapper around the
core. The driver support is very basic such that it supports only
enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
for now but these will be added later.

The driver is capable of using the PERST# and WAKE# side-band GPIOs for
operation and written on top of the DWC PCI framework.

Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
[mani: restructured the driver and fixed several bugs for upstream]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/Kconfig        |  10 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
 3 files changed, 791 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 423d35872ce4..32e735b1fd85 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -180,6 +180,16 @@ config PCIE_QCOM
 	  PCIe controller uses the DesignWare core plus Qualcomm-specific
 	  hardware wrappers.
 
+config PCIE_QCOM_EP
+	bool "Qualcomm PCIe controller - Endpoint mode"
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	help
+	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
+	  to work in endpoint mode. The PCIe controller uses the DesignWare core
+	  plus Qualcomm-specific hardware wrappers.
+
 config PCIE_ARMADA_8K
 	bool "Marvell Armada-8K PCIe controller"
 	depends on ARCH_MVEBU || COMPILE_TEST
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index eca805c1a023..abb27642d46b 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
+obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
new file mode 100644
index 000000000000..b68511bacc2a
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -0,0 +1,780 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm PCIe Endpoint controller driver
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ * Author: Siddartha Mohanadoss <smohanad@codeaurora.org
+ *
+ * Copyright (c) 2021, Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
+ */
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/reset.h>
+#include <linux/delay.h>
+#include <linux/phy/phy.h>
+#include <linux/pm_domain.h>
+
+#include "pcie-designware.h"
+
+/* PARF registers */
+#define PARF_SYS_CTRL				0x00
+#define PARF_DB_CTRL				0x10
+#define PARF_PM_CTRL				0x20
+#define PARF_DEBUG_INT_EN			0x190
+#define PARF_AXI_MSTR_RD_HALT_NO_WRITES		0x1a4
+#define PARF_AXI_MSTR_WR_ADDR_HALT		0x1a8
+#define PARF_Q2A_FLUSH				0x1aC
+#define PARF_LTSSM				0x1b0
+#define PARF_CFG_BITS				0x210
+#define PARF_INT_ALL_STATUS			0x224
+#define PARF_INT_ALL_CLEAR			0x228
+#define PARF_INT_ALL_MASK			0x22c
+#define PARF_SLV_ADDR_MSB_CTRL			0x2c0
+#define PARF_DBI_BASE_ADDR			0x350
+#define PARF_DBI_BASE_ADDR_HI			0x354
+#define PARF_SLV_ADDR_SPACE_SIZE		0x358
+#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
+#define PARF_ATU_BASE_ADDR			0x634
+#define PARF_ATU_BASE_ADDR_HI			0x638
+#define PARF_SRIS_MODE				0x644
+#define PARF_DEVICE_TYPE			0x1000
+#define PARF_BDF_TO_SID_CFG			0x2c00
+
+/* ELBI registers */
+#define ELBI_SYS_STTS				0x08
+
+/* DBI registers */
+#define DBI_CAP_ID_NXT_PTR			0x40
+#define DBI_CON_STATUS				0x44
+#define DBI_DEVICE_CAPABILITIES			0x74
+#define DBI_LINK_CAPABILITIES			0x7c
+#define DBI_LINK_CONTROL2_LINK_STATUS2		0xa0
+#define DBI_L1SUB_CAPABILITY			0x234
+#define DBI_ACK_F_ASPM_CTRL			0x70c
+#define DBI_GEN3_RELATED_OFF			0x890
+#define DBI_AUX_CLK_FREQ			0xb40
+
+#define DBI_L0S_ACCPT_LATENCY_MASK		GENMASK(8, 6)
+#define DBI_L1_ACCPT_LATENCY_MASK		GENMASK(11, 9)
+#define DBI_L0S_EXIT_LATENCY_MASK		GENMASK(14, 12)
+#define DBI_L1_EXIT_LATENCY_MASK		GENMASK(17, 15)
+#define DBI_ACK_N_FTS_MASK			GENMASK(15, 8)
+
+/* TCSR registers */
+#define TCSR_PCIE_PERST_EN			0x258
+#define TCSR_PERST_SEPARATION_ENABLE		0x270
+
+#define XMLH_LINK_UP				0x400
+#define CORE_RESET_TIME_US_MIN			1000
+#define CORE_RESET_TIME_US_MAX			1005
+#define WAKE_DELAY_US				2000 /* 2 ms */
+
+#define to_pcie_ep(x)				dev_get_drvdata((x)->dev)
+
+enum qcom_pcie_ep_link_status {
+	QCOM_PCIE_EP_LINK_DISABLED,
+	QCOM_PCIE_EP_LINK_ENABLED,
+	QCOM_PCIE_EP_LINK_UP,
+	QCOM_PCIE_EP_LINK_DOWN,
+};
+
+enum qcom_pcie_ep_irq {
+	QCOM_PCIE_EP_INT_RESERVED,
+	QCOM_PCIE_EP_INT_LINK_DOWN,
+	QCOM_PCIE_EP_INT_BME,
+	QCOM_PCIE_EP_INT_PM_TURNOFF,
+	QCOM_PCIE_EP_INT_DEBUG,
+	QCOM_PCIE_EP_INT_LTR,
+	QCOM_PCIE_EP_INT_MHI_Q6,
+	QCOM_PCIE_EP_INT_MHI_A7,
+	QCOM_PCIE_EP_INT_DSTATE_CHANGE,
+	QCOM_PCIE_EP_INT_L1SUB_TIMEOUT,
+	QCOM_PCIE_EP_INT_MMIO_WRITE,
+	QCOM_PCIE_EP_INT_CFG_WRITE,
+	QCOM_PCIE_EP_INT_BRIDGE_FLUSH_N,
+	QCOM_PCIE_EP_INT_LINK_UP,
+	QCOM_PCIE_EP_INT_AER_LEGACY,
+	QCOM_PCIE_EP_INT_PLS_ERR,
+	QCOM_PCIE_EP_INT_PME_LEGACY,
+	QCOM_PCIE_EP_INT_PLS_PME,
+	QCOM_PCIE_EP_INT_MAX,
+};
+
+static struct clk_bulk_data qcom_pcie_ep_clks[] = {
+	{ .id = "cfg" },
+	{ .id = "aux" },
+	{ .id = "bus_master" },
+	{ .id = "bus_slave" },
+	{ .id = "ref" },
+	{ .id = "sleep" },
+	{ .id = "slave_q2a" },
+};
+
+struct qcom_pcie_ep {
+	struct dw_pcie pci;
+
+	void __iomem *parf;
+	void __iomem *elbi;
+	void __iomem *tcsr;
+
+	struct reset_control *core_reset;
+	struct gpio_desc *reset;
+	struct gpio_desc *wake;
+	struct phy *phy;
+
+	resource_size_t dbi_phys;
+	resource_size_t atu_phys;
+
+	enum qcom_pcie_ep_link_status link_status;
+	int global_irq;
+	int perst_irq;
+};
+
+static void qcom_pcie_ep_enable_ltssm(struct qcom_pcie_ep *pcie_ep)
+{
+	u32 reg;
+
+	reg = readl(pcie_ep->parf + PARF_LTSSM);
+	reg |= BIT(8);
+	writel_relaxed(reg, pcie_ep->parf + PARF_LTSSM);
+}
+
+static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
+{
+	struct dw_pcie *pci = &pcie_ep->pci;
+	struct device *dev = pci->dev;
+	int ret = 0;
+
+	ret = reset_control_assert(pcie_ep->core_reset);
+	if (ret) {
+		dev_err(dev, "Cannot assert core reset\n");
+		return ret;
+	}
+
+	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
+
+	ret = reset_control_deassert(pcie_ep->core_reset);
+	if (ret) {
+		dev_err(dev, "Cannot de-assert core reset\n");
+		return ret;
+	}
+
+	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
+
+	return 0;
+}
+
+/*
+ * Delatch PERST_EN and PERST_SEPARATION_ENABLE with TCSR to avoid
+ * device reset during host reboot and hibernation. The driver is
+ * expected to handle this situation.
+ */
+static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
+{
+	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
+	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
+}
+
+static int qcom_pcie_ep_enable_resources(struct qcom_pcie_ep *pcie_ep)
+{
+	struct dw_pcie *pci = &pcie_ep->pci;
+	struct device *dev = pci->dev;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qcom_pcie_ep_clks),
+				      qcom_pcie_ep_clks);
+	if (ret) {
+		dev_err(dev, "Failed to enable clocks: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void qcom_pcie_ep_disable_resources(struct qcom_pcie_ep *pcie_ep)
+{
+	clk_bulk_disable_unprepare(ARRAY_SIZE(qcom_pcie_ep_clks),
+				   qcom_pcie_ep_clks);
+}
+
+static void qcom_pcie_ep_configure_irq(struct qcom_pcie_ep *pcie_ep)
+{
+	u32 val;
+
+	writel_relaxed(0, pcie_ep->parf + PARF_INT_ALL_MASK);
+	val = BIT(QCOM_PCIE_EP_INT_LINK_DOWN) |
+		BIT(QCOM_PCIE_EP_INT_BME) |
+		BIT(QCOM_PCIE_EP_INT_PM_TURNOFF) |
+		BIT(QCOM_PCIE_EP_INT_DSTATE_CHANGE) |
+		BIT(QCOM_PCIE_EP_INT_LINK_UP);
+	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);
+}
+
+static int qcom_pcie_ep_core_init(struct qcom_pcie_ep *pcie_ep)
+{
+	struct dw_pcie *pci = &pcie_ep->pci;
+	u32 val;
+
+	/* Disable BDF to SID mapping */
+	val = readl_relaxed(pcie_ep->parf + PARF_BDF_TO_SID_CFG);
+	val |= BIT(0);
+	writel_relaxed(val, pcie_ep->parf + PARF_BDF_TO_SID_CFG);
+
+	/* enable debug IRQ */
+	writel_relaxed((BIT(3) | BIT(2) | BIT(1)),
+		       pcie_ep->parf + PARF_DEBUG_INT_EN);
+
+	/* Configure PCIe to endpoint mode */
+	writel_relaxed(0x0, pcie_ep->parf + PARF_DEVICE_TYPE);
+
+	/* Configure PCIe core to support 1GB aperture */
+	writel_relaxed(0x40000000, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
+
+	/* Allow entering L1 state */
+	val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
+	val &= ~BIT(5);
+	writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
+
+	/* Configure Slave, DBI and iATU base addresses */
+	writel_relaxed(BIT(0), pcie_ep->parf + PARF_SLV_ADDR_MSB_CTRL);
+	writel_relaxed(0x200, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
+	writel_relaxed(0x0, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
+	writel_relaxed(0x100, pcie_ep->parf + PARF_DBI_BASE_ADDR_HI);
+	writel_relaxed(pcie_ep->dbi_phys, pcie_ep->parf + PARF_DBI_BASE_ADDR);
+	writel_relaxed(0x100, pcie_ep->parf + PARF_ATU_BASE_ADDR_HI);
+	writel_relaxed(pcie_ep->atu_phys, pcie_ep->parf + PARF_ATU_BASE_ADDR);
+
+	/* Read halts write */
+	writel_relaxed(0x0, pcie_ep->parf + PARF_AXI_MSTR_RD_HALT_NO_WRITES);
+	/* Write after write halt */
+	writel_relaxed(BIT(31), pcie_ep->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
+	/* Q2A flush disable */
+	writel_relaxed(0, pcie_ep->parf + PARF_Q2A_FLUSH);
+
+	/* Disable the DBI Wakeup */
+	writel_relaxed(BIT(11), pcie_ep->parf + PARF_SYS_CTRL);
+	/* Disable the debouncers */
+	writel_relaxed(0x73, pcie_ep->parf + PARF_DB_CTRL);
+	/* Disable core clock CGC */
+	writel_relaxed(BIT(6), pcie_ep->parf + PARF_SYS_CTRL);
+	/* Set AUX power to be on */
+	writel_relaxed(BIT(4), pcie_ep->parf + PARF_SYS_CTRL);
+	/* Request to exit from L1SS for MSI and LTR MSG */
+	writel_relaxed(BIT(1), pcie_ep->parf + PARF_CFG_BITS);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	/* Set the PMC Register - to support PME in D0/D3hot/D3cold */
+	val = dw_pcie_readl_dbi(pci, DBI_CAP_ID_NXT_PTR);
+	val |= BIT(31) | BIT(30) | BIT(27);
+	dw_pcie_writel_dbi(pci, DBI_CAP_ID_NXT_PTR, val);
+
+	/* Set the Endpoint L0s Acceptable Latency to 1us (max) */
+	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
+	val |= FIELD_PREP(DBI_L0S_ACCPT_LATENCY_MASK, 0x7);
+	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
+
+	/* Set the Endpoint L1 Acceptable Latency to 1us (max) */
+	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
+	val |= FIELD_PREP(DBI_L1_ACCPT_LATENCY_MASK, 0x7);
+	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
+
+	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
+	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
+	val |= FIELD_PREP(DBI_L0S_EXIT_LATENCY_MASK, 0x6);
+	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
+
+	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
+	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
+	val |= FIELD_PREP(DBI_L1_EXIT_LATENCY_MASK, 0x6);
+	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
+
+	/* L1ss is supported */
+	val = dw_pcie_readl_dbi(pci, DBI_L1SUB_CAPABILITY);
+	val |= 0x1f;
+	dw_pcie_writel_dbi(pci, DBI_L1SUB_CAPABILITY, val);
+
+	/* Enable Clock Power Management */
+	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
+	val |= BIT(18);
+	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/* Set FTS value to match the PHY setting */
+	val = dw_pcie_readl_dbi(pci, DBI_ACK_F_ASPM_CTRL);
+	val |= FIELD_PREP(DBI_ACK_N_FTS_MASK, 0x80);
+	dw_pcie_writel_dbi(pci, DBI_ACK_F_ASPM_CTRL, val);
+
+	dw_pcie_writel_dbi(pci, DBI_AUX_CLK_FREQ, 0x14);
+
+	/* Prevent L1ss wakeup after 100ms */
+	val = dw_pcie_readl_dbi(pci, DBI_GEN3_RELATED_OFF);
+	val &= ~BIT(0);
+	dw_pcie_writel_dbi(pci, DBI_GEN3_RELATED_OFF, val);
+
+	/* Disable SRIS_MODE */
+	val = readl_relaxed(pcie_ep->parf + PARF_SRIS_MODE);
+	val &= ~BIT(0);
+	writel_relaxed(val, pcie_ep->parf + PARF_SRIS_MODE);
+
+	qcom_pcie_ep_configure_irq(pcie_ep);
+
+	return 0;
+}
+
+static int qcom_pcie_confirm_linkup(struct dw_pcie *pci)
+{
+	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+	u32 reg;
+
+	reg = readl_relaxed(pcie_ep->elbi + ELBI_SYS_STTS);
+
+	return reg & XMLH_LINK_UP;
+}
+
+static int qcom_pcie_start_link(struct dw_pcie *pci)
+{
+	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+
+	enable_irq(pcie_ep->perst_irq);
+
+	return 0;
+}
+
+static int qcom_pcie_establish_link(struct dw_pcie *pci)
+{
+	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+	struct device *dev = pci->dev;
+	int ret;
+
+	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_ENABLED) {
+		dev_info(dev, "Link is already enabled\n");
+		return 0;
+	}
+
+	/* Enable power and clocks */
+	ret = qcom_pcie_ep_enable_resources(pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to enable resources: %d\n", ret);
+		return ret;
+	}
+
+	/* Perform controller reset */
+	ret = qcom_pcie_ep_core_reset(pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to reset the core: %d\n", ret);
+		goto err_disable_resources;
+	}
+
+	/* Initialize PHY */
+	ret = phy_init(pcie_ep->phy);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto err_disable_resources;
+	}
+
+	/* Power ON PHY */
+	ret = phy_power_on(pcie_ep->phy);
+	if (ret) {
+		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
+		goto err_phy_exit;
+	}
+
+	/* Assert WAKE# to RC to indicate device is ready */
+	gpiod_set_value_cansleep(pcie_ep->wake, 0);
+	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
+	gpiod_set_value_cansleep(pcie_ep->wake, 1);
+
+	/* Configure TCSR */
+	qcom_pcie_ep_configure_tcsr(pcie_ep);
+
+	/* Initialize the controller */
+	ret = qcom_pcie_ep_core_init(pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to init controller: %d\n", ret);
+		goto err_phy_power_off;
+	}
+
+	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
+	if (ret) {
+		dev_err(dev, "Failed to complete initialization: %d\n", ret);
+		goto err_phy_power_off;
+	}
+
+	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
+
+	/* Enable LTSSM */
+	qcom_pcie_ep_enable_ltssm(pcie_ep);
+
+	return 0;
+/* TODO* assert reset? */
+err_phy_power_off:
+	phy_power_off(pcie_ep->phy);
+err_phy_exit:
+	phy_exit(pcie_ep->phy);
+err_disable_resources:
+	qcom_pcie_ep_disable_resources(pcie_ep);
+
+	return ret;
+}
+
+static void qcom_pcie_disable_link(struct dw_pcie *pci)
+{
+	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+	struct device *dev = pci->dev;
+
+	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
+		dev_info(dev, "Link is already disabled\n");
+		return;
+	}
+
+	phy_power_off(pcie_ep->phy);
+	phy_exit(pcie_ep->phy);
+	qcom_pcie_ep_disable_resources(pcie_ep);
+	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
+}
+
+/* Common DWC controller ops */
+static const struct dw_pcie_ops pci_ops = {
+	.link_up = qcom_pcie_confirm_linkup,
+	.start_link = qcom_pcie_start_link,
+	.stop_link = qcom_pcie_disable_link,
+};
+
+static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
+					 struct qcom_pcie_ep *pcie_ep)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = &pcie_ep->pci;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
+	pcie_ep->parf = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pcie_ep->parf))
+		return PTR_ERR(pcie_ep->parf);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pci->dbi_base))
+		return PTR_ERR(pci->dbi_base);
+	pci->dbi_base2 = pci->dbi_base;
+	pcie_ep->dbi_phys = res->start;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
+	pcie_ep->elbi = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pcie_ep->elbi))
+		return PTR_ERR(pcie_ep->elbi);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
+	pci->atu_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pci->atu_base))
+		return PTR_ERR(pci->atu_base);
+	pcie_ep->atu_phys = res->start;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
+	pcie_ep->tcsr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pcie_ep->tcsr))
+		return PTR_ERR(pcie_ep->tcsr);
+
+	return 0;
+}
+
+static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
+				      struct qcom_pcie_ep *pcie_ep)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = qcom_pcie_ep_get_io_resources(pdev, pcie_ep);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get io resources %d\n", ret);
+		return ret;
+	}
+
+	/* Enable GDSC power domain */
+	ret = dev_pm_domain_attach(dev, true);
+	if (ret)
+		return ret;
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(qcom_pcie_ep_clks),
+				qcom_pcie_ep_clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clocks: %d\n", ret);
+		return ret;
+	}
+
+	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
+	if (IS_ERR(pcie_ep->core_reset))
+		return PTR_ERR(pcie_ep->core_reset);
+
+	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
+	if (IS_ERR(pcie_ep->reset))
+		return PTR_ERR(pcie_ep->reset);
+
+	pcie_ep->wake = devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_HIGH);
+	if (IS_ERR(pcie_ep->wake))
+		return PTR_ERR(pcie_ep->wake);
+
+	pcie_ep->phy = devm_phy_optional_get(&pdev->dev, "pciephy");
+	if (IS_ERR(pcie_ep->phy))
+		ret = PTR_ERR(pcie_ep->phy);
+
+	return ret;
+}
+
+/* TODO: Notify clients about PCIe state change */
+static irqreturn_t qcom_pcie_ep_global_threaded_irq(int irq, void *data)
+{
+	struct qcom_pcie_ep *pcie_ep = data;
+	struct dw_pcie *pci = &pcie_ep->pci;
+	struct device *dev = pci->dev;
+	u32 status = readl(pcie_ep->parf + PARF_INT_ALL_STATUS);
+	u32 mask = readl(pcie_ep->parf + PARF_INT_ALL_MASK);
+	u32 dstate, event, val;
+
+	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);
+	status &= mask;
+
+	for (event = 1; event < QCOM_PCIE_EP_INT_MAX; event++) {
+		if (status & BIT(event)) {
+			switch (event) {
+			case QCOM_PCIE_EP_INT_LINK_DOWN:
+				dev_info(dev, "Received Linkdown event\n");
+				pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN;
+				break;
+			case QCOM_PCIE_EP_INT_BME:
+				dev_info(dev, "Received BME event. Link is enabled!\n");
+				pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
+				break;
+			case QCOM_PCIE_EP_INT_PM_TURNOFF:
+				dev_info(dev, "Received PM Turn-off event! Entering L23\n");
+				val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
+				val |= BIT(2);
+				writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
+				break;
+			case QCOM_PCIE_EP_INT_DSTATE_CHANGE:
+				dstate = dw_pcie_readl_dbi(pci, DBI_CON_STATUS) & 0x3;
+				dev_info(dev, "Received D%d state event\n", dstate);
+				if (dstate == 3) {
+					val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
+					val |= BIT(1);
+					writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
+				}
+				/* Handle D0 state change */
+				break;
+			case QCOM_PCIE_EP_INT_LINK_UP:
+				dev_info(dev, "Received Linkup event. Enumeration complete!\n");
+				dw_pcie_ep_linkup(&pci->ep);
+				pcie_ep->link_status = QCOM_PCIE_EP_LINK_UP;
+				break;
+			default:
+				dev_err(dev, "Received unknown event: %d\n", event);
+				break;
+			}
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
+{
+	struct qcom_pcie_ep *pcie_ep = data;
+	struct dw_pcie *pci = &pcie_ep->pci;
+	struct device *dev = pci->dev;
+	u32 perst;
+
+	perst = gpiod_get_value(pcie_ep->reset);
+
+	if (perst) {
+		/* Start link training */
+		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
+		qcom_pcie_establish_link(pci);
+	} else {
+		/* Shutdown the link if the link is already on */
+		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
+		qcom_pcie_disable_link(pci);
+	}
+
+	/* Set trigger type based on the next expected value of perst gpio */
+	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
+			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
+
+	return IRQ_HANDLED;
+}
+
+static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
+					     struct qcom_pcie_ep *pcie_ep)
+{
+	int irq, ret;
+
+	irq = platform_get_irq_byname(pdev, "global");
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
+		return irq;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					qcom_pcie_ep_global_threaded_irq,
+					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					"global_irq", pcie_ep);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
+		return ret;
+	}
+
+	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
+	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
+					qcom_pcie_ep_perst_threaded_irq,
+					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					"perst_irq", pcie_ep);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request PERST IRQ\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int qcom_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				  enum pci_epc_irq_type type, u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
+	case PCI_EPC_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	default:
+		dev_err(pci->dev, "Unknown IRQ type\n");
+		return -EINVAL;
+	}
+}
+
+static const struct pci_epc_features qcom_pcie_epc_features = {
+	.linkup_notifier = true,
+	.core_init_notifier = true,
+	.msi_capable = true,
+	.msix_capable = false,
+};
+
+static const struct pci_epc_features *
+qcom_pcie_epc_get_features(struct dw_pcie_ep *pci_ep)
+{
+	return &qcom_pcie_epc_features;
+}
+
+static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = BAR_0; bar <= BAR_5; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static struct dw_pcie_ep_ops pci_ep_ops = {
+	.ep_init = qcom_pcie_ep_init,
+	.raise_irq = qcom_pcie_ep_raise_irq,
+	.get_features = qcom_pcie_epc_get_features,
+};
+
+static const struct of_device_id qcom_pcie_ep_match[] = {
+	{ .compatible = "qcom,sdx55-pcie-ep", },
+	{ }
+};
+
+static int qcom_pcie_ep_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_pcie_ep *pcie_ep;
+	int ret;
+
+	pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL);
+	if (!pcie_ep)
+		return -ENOMEM;
+
+	pcie_ep->pci.dev = dev;
+	pcie_ep->pci.ops = &pci_ops;
+	pcie_ep->pci.ep.ops = &pci_ep_ops;
+
+	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to get resources:%d\n", ret);
+		return ret;
+	}
+
+	/* Enable power and clocks */
+	ret = qcom_pcie_ep_enable_resources(pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to enable resources: %d\n", ret);
+		return ret;
+	}
+
+	/* Perform controller reset */
+	ret = qcom_pcie_ep_core_reset(pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to reset the core: %d\n", ret);
+		goto err_disable_resources;
+	}
+
+	/* Initialize PHY */
+	ret = phy_init(pcie_ep->phy);
+	if (ret) {
+		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
+		goto err_disable_resources;
+	}
+
+	/* PHY needs to be powered on for dw_pcie_ep_init() */
+	ret = phy_power_on(pcie_ep->phy);
+	if (ret) {
+		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
+		goto err_phy_exit;
+	}
+
+	platform_set_drvdata(pdev, pcie_ep);
+
+	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
+	if (ret) {
+		dev_err(dev, "Failed to get IRQ resources %d\n", ret);
+		goto err_phy_power_off;
+	}
+
+	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
+	if (ret) {
+		dev_err(dev, "Failed to initialize endpoint:%d\n", ret);
+		goto err_phy_power_off;
+	}
+
+	return 0;
+
+err_phy_power_off:
+	phy_power_off(pcie_ep->phy);
+err_phy_exit:
+	phy_exit(pcie_ep->phy);
+err_disable_resources:
+	qcom_pcie_ep_disable_resources(pcie_ep);
+
+	return ret;
+}
+
+static struct platform_driver qcom_pcie_ep_driver = {
+	.probe	= qcom_pcie_ep_probe,
+	.driver	= {
+		.name		= "qcom-pcie-ep",
+		.suppress_bind_attrs = true,
+		.of_match_table	= qcom_pcie_ep_match,
+	},
+};
+builtin_platform_driver(qcom_pcie_ep_driver);
+
+MODULE_AUTHOR("Siddartha Mohanadoss <smohanad@codeaurora.org>");
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("Qualcomm PCIe Endpoint controller driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1


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

* [PATCH v2 3/3] MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding
  2021-06-03 10:38 [PATCH v2 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
  2021-06-03 10:38 ` [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
  2021-06-03 10:38 ` [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
@ 2021-06-03 10:38 ` Manivannan Sadhasivam
  2 siblings, 0 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-03 10:38 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh, bhelgaas
  Cc: bjorn.andersson, linux-arm-msm, linux-pci, devicetree,
	linux-kernel, Manivannan Sadhasivam

Add MAINTAINERS entry for Qualcomm PCIe Endpoint driver and its
devicetree binding. While at it, let's also fix the PCIE RC entry to
cover only the RC driver.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 MAINTAINERS | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index bd7aff0c120f..cdd370138b9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14254,7 +14254,15 @@ M:	Stanimir Varbanov <svarbanov@mm-sol.com>
 L:	linux-pci@vger.kernel.org
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
-F:	drivers/pci/controller/dwc/*qcom*
+F:	drivers/pci/controller/dwc/pcie-qcom.c
+
+PCIE ENDPOINT DRIVER FOR QUALCOMM
+M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+L:	linux-pci@vger.kernel.org
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
+F:	drivers/pci/controller/dwc/pcie-qcom-ep.c
 
 PCIE DRIVER FOR ROCKCHIP
 M:	Shawn Lin <shawn.lin@rock-chips.com>
-- 
2.25.1


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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-03 10:38 ` [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
@ 2021-06-05 22:02   ` Stanimir Varbanov
  2021-06-09  5:20     ` Manivannan Sadhasivam
  2021-06-06  3:07   ` Bjorn Andersson
  1 sibling, 1 reply; 17+ messages in thread
From: Stanimir Varbanov @ 2021-06-05 22:02 UTC (permalink / raw)
  To: Manivannan Sadhasivam, lorenzo.pieralisi, robh, bhelgaas
  Cc: bjorn.andersson, linux-arm-msm, linux-pci, devicetree,
	linux-kernel, Siddartha Mohanadoss

Hi Mani,

On 6/3/21 1:38 PM, Manivannan Sadhasivam wrote:
> Add driver support for Qualcomm PCIe Endpoint controller driver based on
> the Designware core with added Qualcomm specific wrapper around the
> core. The driver support is very basic such that it supports only
> enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> for now but these will be added later.
> 
> The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> operation and written on top of the DWC PCI framework.
> 
> Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> [mani: restructured the driver and fixed several bugs for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/Kconfig        |  10 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
>  3 files changed, 791 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 423d35872ce4..32e735b1fd85 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -180,6 +180,16 @@ config PCIE_QCOM
>  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
>  	  hardware wrappers.
>  
> +config PCIE_QCOM_EP
> +	bool "Qualcomm PCIe controller - Endpoint mode"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP

Do you see a problem if root-complex driver and EP drive are enabled on
the same time? I think the PCIe IP can work only on one of both modes.

> +	help
> +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> +	  plus Qualcomm-specific hardware wrappers.
> +
>  config PCIE_ARMADA_8K
>  	bool "Marvell Armada-8K PCIe controller"
>  	depends on ARCH_MVEBU || COMPILE_TEST
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..abb27642d46b 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> new file mode 100644
> index 000000000000..b68511bacc2a
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -0,0 +1,780 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm PCIe Endpoint controller driver
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + * Author: Siddartha Mohanadoss <smohanad@codeaurora.org
> + *
> + * Copyright (c) 2021, Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/delay.h>
> +#include <linux/phy/phy.h>
> +#include <linux/pm_domain.h>
> +
> +#include "pcie-designware.h"
> +
> +/* PARF registers */
> +#define PARF_SYS_CTRL				0x00
> +#define PARF_DB_CTRL				0x10
> +#define PARF_PM_CTRL				0x20
> +#define PARF_DEBUG_INT_EN			0x190
> +#define PARF_AXI_MSTR_RD_HALT_NO_WRITES		0x1a4
> +#define PARF_AXI_MSTR_WR_ADDR_HALT		0x1a8
> +#define PARF_Q2A_FLUSH				0x1aC
> +#define PARF_LTSSM				0x1b0
> +#define PARF_CFG_BITS				0x210
> +#define PARF_INT_ALL_STATUS			0x224
> +#define PARF_INT_ALL_CLEAR			0x228
> +#define PARF_INT_ALL_MASK			0x22c
> +#define PARF_SLV_ADDR_MSB_CTRL			0x2c0
> +#define PARF_DBI_BASE_ADDR			0x350
> +#define PARF_DBI_BASE_ADDR_HI			0x354
> +#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> +#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
> +#define PARF_ATU_BASE_ADDR			0x634
> +#define PARF_ATU_BASE_ADDR_HI			0x638
> +#define PARF_SRIS_MODE				0x644
> +#define PARF_DEVICE_TYPE			0x1000
> +#define PARF_BDF_TO_SID_CFG			0x2c00
> +
> +/* ELBI registers */
> +#define ELBI_SYS_STTS				0x08
> +
> +/* DBI registers */
> +#define DBI_CAP_ID_NXT_PTR			0x40
> +#define DBI_CON_STATUS				0x44
> +#define DBI_DEVICE_CAPABILITIES			0x74
> +#define DBI_LINK_CAPABILITIES			0x7c
> +#define DBI_LINK_CONTROL2_LINK_STATUS2		0xa0
> +#define DBI_L1SUB_CAPABILITY			0x234
> +#define DBI_ACK_F_ASPM_CTRL			0x70c
> +#define DBI_GEN3_RELATED_OFF			0x890
> +#define DBI_AUX_CLK_FREQ			0xb40
> +
> +#define DBI_L0S_ACCPT_LATENCY_MASK		GENMASK(8, 6)
> +#define DBI_L1_ACCPT_LATENCY_MASK		GENMASK(11, 9)
> +#define DBI_L0S_EXIT_LATENCY_MASK		GENMASK(14, 12)
> +#define DBI_L1_EXIT_LATENCY_MASK		GENMASK(17, 15)
> +#define DBI_ACK_N_FTS_MASK			GENMASK(15, 8)
> +
> +/* TCSR registers */
> +#define TCSR_PCIE_PERST_EN			0x258
> +#define TCSR_PERST_SEPARATION_ENABLE		0x270
> +
> +#define XMLH_LINK_UP				0x400
> +#define CORE_RESET_TIME_US_MIN			1000
> +#define CORE_RESET_TIME_US_MAX			1005
> +#define WAKE_DELAY_US				2000 /* 2 ms */
> +
> +#define to_pcie_ep(x)				dev_get_drvdata((x)->dev)

I don't think that this define adds some more readability or something else.

> +
> +enum qcom_pcie_ep_link_status {
> +	QCOM_PCIE_EP_LINK_DISABLED,
> +	QCOM_PCIE_EP_LINK_ENABLED,
> +	QCOM_PCIE_EP_LINK_UP,
> +	QCOM_PCIE_EP_LINK_DOWN,
> +};
> +
> +enum qcom_pcie_ep_irq {
> +	QCOM_PCIE_EP_INT_RESERVED,
> +	QCOM_PCIE_EP_INT_LINK_DOWN,
> +	QCOM_PCIE_EP_INT_BME,
> +	QCOM_PCIE_EP_INT_PM_TURNOFF,
> +	QCOM_PCIE_EP_INT_DEBUG,
> +	QCOM_PCIE_EP_INT_LTR,
> +	QCOM_PCIE_EP_INT_MHI_Q6,
> +	QCOM_PCIE_EP_INT_MHI_A7,
> +	QCOM_PCIE_EP_INT_DSTATE_CHANGE,
> +	QCOM_PCIE_EP_INT_L1SUB_TIMEOUT,
> +	QCOM_PCIE_EP_INT_MMIO_WRITE,
> +	QCOM_PCIE_EP_INT_CFG_WRITE,
> +	QCOM_PCIE_EP_INT_BRIDGE_FLUSH_N,
> +	QCOM_PCIE_EP_INT_LINK_UP,
> +	QCOM_PCIE_EP_INT_AER_LEGACY,
> +	QCOM_PCIE_EP_INT_PLS_ERR,
> +	QCOM_PCIE_EP_INT_PME_LEGACY,
> +	QCOM_PCIE_EP_INT_PLS_PME,
> +	QCOM_PCIE_EP_INT_MAX,
> +};
> +
> +static struct clk_bulk_data qcom_pcie_ep_clks[] = {
> +	{ .id = "cfg" },
> +	{ .id = "aux" },
> +	{ .id = "bus_master" },
> +	{ .id = "bus_slave" },
> +	{ .id = "ref" },
> +	{ .id = "sleep" },
> +	{ .id = "slave_q2a" },
> +};
> +
> +struct qcom_pcie_ep {
> +	struct dw_pcie pci;
> +
> +	void __iomem *parf;
> +	void __iomem *elbi;
> +	void __iomem *tcsr;
> +
> +	struct reset_control *core_reset;
> +	struct gpio_desc *reset;
> +	struct gpio_desc *wake;
> +	struct phy *phy;
> +
> +	resource_size_t dbi_phys;
> +	resource_size_t atu_phys;
> +
> +	enum qcom_pcie_ep_link_status link_status;
> +	int global_irq;
> +	int perst_irq;
> +};
> +
> +static void qcom_pcie_ep_enable_ltssm(struct qcom_pcie_ep *pcie_ep)
> +{
> +	u32 reg;
> +
> +	reg = readl(pcie_ep->parf + PARF_LTSSM);
> +	reg |= BIT(8);
> +	writel_relaxed(reg, pcie_ep->parf + PARF_LTSSM);
> +}
> +
> +static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	int ret = 0;
> +
> +	ret = reset_control_assert(pcie_ep->core_reset);
> +	if (ret) {
> +		dev_err(dev, "Cannot assert core reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> +
> +	ret = reset_control_deassert(pcie_ep->core_reset);
> +	if (ret) {
> +		dev_err(dev, "Cannot de-assert core reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> +
> +	return 0;
> +}
> +
> +/*
> + * Delatch PERST_EN and PERST_SEPARATION_ENABLE with TCSR to avoid
> + * device reset during host reboot and hibernation. The driver is
> + * expected to handle this situation.
> + */
> +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> +{
> +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);

Are you sure that _relaxed() variants (here and on the other places in
the driver) is enough? I'd use writel() for all places which are not
performance sesnsitive.

> +}
> +
> +static int qcom_pcie_ep_enable_resources(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qcom_pcie_ep_clks),
> +				      qcom_pcie_ep_clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clocks: %d\n", ret);

clk_bulk_prepare_enable already print errors so please drop this dev_err().

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_ep_disable_resources(struct qcom_pcie_ep *pcie_ep)
> +{
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(qcom_pcie_ep_clks),
> +				   qcom_pcie_ep_clks);
> +}
> +
> +static void qcom_pcie_ep_configure_irq(struct qcom_pcie_ep *pcie_ep)
> +{
> +	u32 val;
> +
> +	writel_relaxed(0, pcie_ep->parf + PARF_INT_ALL_MASK);
> +	val = BIT(QCOM_PCIE_EP_INT_LINK_DOWN) |
> +		BIT(QCOM_PCIE_EP_INT_BME) |
> +		BIT(QCOM_PCIE_EP_INT_PM_TURNOFF) |
> +		BIT(QCOM_PCIE_EP_INT_DSTATE_CHANGE) |
> +		BIT(QCOM_PCIE_EP_INT_LINK_UP);
> +	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);
> +}
> +
> +static int qcom_pcie_ep_core_init(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	u32 val;
> +
> +	/* Disable BDF to SID mapping */
> +	val = readl_relaxed(pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> +	val |= BIT(0);
> +	writel_relaxed(val, pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> +
> +	/* enable debug IRQ */
> +	writel_relaxed((BIT(3) | BIT(2) | BIT(1)),
> +		       pcie_ep->parf + PARF_DEBUG_INT_EN);
> +
> +	/* Configure PCIe to endpoint mode */
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_DEVICE_TYPE);
> +
> +	/* Configure PCIe core to support 1GB aperture */
> +	writel_relaxed(0x40000000, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +	/* Allow entering L1 state */
> +	val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +	val &= ~BIT(5);
> +	writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +
> +	/* Configure Slave, DBI and iATU base addresses */
> +	writel_relaxed(BIT(0), pcie_ep->parf + PARF_SLV_ADDR_MSB_CTRL);
> +	writel_relaxed(0x200, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +	writel_relaxed(0x100, pcie_ep->parf + PARF_DBI_BASE_ADDR_HI);
> +	writel_relaxed(pcie_ep->dbi_phys, pcie_ep->parf + PARF_DBI_BASE_ADDR);
> +	writel_relaxed(0x100, pcie_ep->parf + PARF_ATU_BASE_ADDR_HI);
> +	writel_relaxed(pcie_ep->atu_phys, pcie_ep->parf + PARF_ATU_BASE_ADDR);
> +
> +	/* Read halts write */
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_AXI_MSTR_RD_HALT_NO_WRITES);
> +	/* Write after write halt */
> +	writel_relaxed(BIT(31), pcie_ep->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
> +	/* Q2A flush disable */
> +	writel_relaxed(0, pcie_ep->parf + PARF_Q2A_FLUSH);
> +
> +	/* Disable the DBI Wakeup */
> +	writel_relaxed(BIT(11), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Disable the debouncers */
> +	writel_relaxed(0x73, pcie_ep->parf + PARF_DB_CTRL);
> +	/* Disable core clock CGC */
> +	writel_relaxed(BIT(6), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Set AUX power to be on */
> +	writel_relaxed(BIT(4), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Request to exit from L1SS for MSI and LTR MSG */
> +	writel_relaxed(BIT(1), pcie_ep->parf + PARF_CFG_BITS);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	/* Set the PMC Register - to support PME in D0/D3hot/D3cold */
> +	val = dw_pcie_readl_dbi(pci, DBI_CAP_ID_NXT_PTR);
> +	val |= BIT(31) | BIT(30) | BIT(27);
> +	dw_pcie_writel_dbi(pci, DBI_CAP_ID_NXT_PTR, val);
> +
> +	/* Set the Endpoint L0s Acceptable Latency to 1us (max) */
> +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L0S_ACCPT_LATENCY_MASK, 0x7);
> +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> +
> +	/* Set the Endpoint L1 Acceptable Latency to 1us (max) */
> +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L1_ACCPT_LATENCY_MASK, 0x7);
> +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> +
> +	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L0S_EXIT_LATENCY_MASK, 0x6);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L1_EXIT_LATENCY_MASK, 0x6);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	/* L1ss is supported */
> +	val = dw_pcie_readl_dbi(pci, DBI_L1SUB_CAPABILITY);
> +	val |= 0x1f;
> +	dw_pcie_writel_dbi(pci, DBI_L1SUB_CAPABILITY, val);
> +
> +	/* Enable Clock Power Management */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= BIT(18);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	/* Set FTS value to match the PHY setting */
> +	val = dw_pcie_readl_dbi(pci, DBI_ACK_F_ASPM_CTRL);
> +	val |= FIELD_PREP(DBI_ACK_N_FTS_MASK, 0x80);
> +	dw_pcie_writel_dbi(pci, DBI_ACK_F_ASPM_CTRL, val);
> +
> +	dw_pcie_writel_dbi(pci, DBI_AUX_CLK_FREQ, 0x14);
> +
> +	/* Prevent L1ss wakeup after 100ms */
> +	val = dw_pcie_readl_dbi(pci, DBI_GEN3_RELATED_OFF);
> +	val &= ~BIT(0);
> +	dw_pcie_writel_dbi(pci, DBI_GEN3_RELATED_OFF, val);
> +
> +	/* Disable SRIS_MODE */
> +	val = readl_relaxed(pcie_ep->parf + PARF_SRIS_MODE);
> +	val &= ~BIT(0);
> +	writel_relaxed(val, pcie_ep->parf + PARF_SRIS_MODE);
> +
> +	qcom_pcie_ep_configure_irq(pcie_ep);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_confirm_linkup(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	u32 reg;
> +
> +	reg = readl_relaxed(pcie_ep->elbi + ELBI_SYS_STTS);
> +
> +	return reg & XMLH_LINK_UP;
> +}
> +
> +static int qcom_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +
> +	enable_irq(pcie_ep->perst_irq);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_establish_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_ENABLED) {
> +		dev_info(dev, "Link is already enabled\n");
> +		return 0;
> +	}
> +
> +	/* Enable power and clocks */
> +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Perform controller reset */
> +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* Initialize PHY */
> +	ret = phy_init(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* Power ON PHY */
> +	ret = phy_power_on(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> +		goto err_phy_exit;
> +	}
> +
> +	/* Assert WAKE# to RC to indicate device is ready */
> +	gpiod_set_value_cansleep(pcie_ep->wake, 0);
> +	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
> +	gpiod_set_value_cansleep(pcie_ep->wake, 1);
> +
> +	/* Configure TCSR */
> +	qcom_pcie_ep_configure_tcsr(pcie_ep);
> +
> +	/* Initialize the controller */
> +	ret = qcom_pcie_ep_core_init(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to init controller: %d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
> +
> +	/* Enable LTSSM */
> +	qcom_pcie_ep_enable_ltssm(pcie_ep);
> +
> +	return 0;
> +/* TODO* assert reset? */
> +err_phy_power_off:
> +	phy_power_off(pcie_ep->phy);
> +err_phy_exit:
> +	phy_exit(pcie_ep->phy);
> +err_disable_resources:
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +
> +	return ret;
> +}
> +
> +static void qcom_pcie_disable_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	struct device *dev = pci->dev;
> +
> +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> +		dev_info(dev, "Link is already disabled\n");
> +		return;
> +	}
> +
> +	phy_power_off(pcie_ep->phy);
> +	phy_exit(pcie_ep->phy);
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> +}
> +
> +/* Common DWC controller ops */
> +static const struct dw_pcie_ops pci_ops = {
> +	.link_up = qcom_pcie_confirm_linkup,
> +	.start_link = qcom_pcie_start_link,
> +	.stop_link = qcom_pcie_disable_link,
> +};
> +
> +static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> +					 struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
> +	pcie_ep->parf = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pcie_ep->parf))
> +		return PTR_ERR(pcie_ep->parf);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pci->dbi_base))
> +		return PTR_ERR(pci->dbi_base);
> +	pci->dbi_base2 = pci->dbi_base;
> +	pcie_ep->dbi_phys = res->start;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
> +	pcie_ep->elbi = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pcie_ep->elbi))
> +		return PTR_ERR(pcie_ep->elbi);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> +	pci->atu_base = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pci->atu_base))
> +		return PTR_ERR(pci->atu_base);
> +	pcie_ep->atu_phys = res->start;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> +	pcie_ep->tcsr = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pcie_ep->tcsr))
> +		return PTR_ERR(pcie_ep->tcsr);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> +				      struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = qcom_pcie_ep_get_io_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get io resources %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Enable GDSC power domain */
> +	ret = dev_pm_domain_attach(dev, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(qcom_pcie_ep_clks),
> +				qcom_pcie_ep_clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks: %d\n", ret);
> +		return ret;
> +	}
> +
> +	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> +	if (IS_ERR(pcie_ep->core_reset))
> +		return PTR_ERR(pcie_ep->core_reset);
> +
> +	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> +	if (IS_ERR(pcie_ep->reset))
> +		return PTR_ERR(pcie_ep->reset);
> +
> +	pcie_ep->wake = devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pcie_ep->wake))
> +		return PTR_ERR(pcie_ep->wake);
> +
> +	pcie_ep->phy = devm_phy_optional_get(&pdev->dev, "pciephy");
> +	if (IS_ERR(pcie_ep->phy))
> +		ret = PTR_ERR(pcie_ep->phy);
> +
> +	return ret;
> +}
> +
> +/* TODO: Notify clients about PCIe state change */
> +static irqreturn_t qcom_pcie_ep_global_threaded_irq(int irq, void *data)
> +{
> +	struct qcom_pcie_ep *pcie_ep = data;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	u32 status = readl(pcie_ep->parf + PARF_INT_ALL_STATUS);
> +	u32 mask = readl(pcie_ep->parf + PARF_INT_ALL_MASK);
> +	u32 dstate, event, val;
> +
> +	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);
> +	status &= mask;
> +
> +	for (event = 1; event < QCOM_PCIE_EP_INT_MAX; event++) {
> +		if (status & BIT(event)) {
> +			switch (event) {
> +			case QCOM_PCIE_EP_INT_LINK_DOWN:
> +				dev_info(dev, "Received Linkdown event\n");
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN;
> +				break;
> +			case QCOM_PCIE_EP_INT_BME:
> +				dev_info(dev, "Received BME event. Link is enabled!\n");
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
> +				break;
> +			case QCOM_PCIE_EP_INT_PM_TURNOFF:
> +				dev_info(dev, "Received PM Turn-off event! Entering L23\n");
> +				val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +				val |= BIT(2);
> +				writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +				break;
> +			case QCOM_PCIE_EP_INT_DSTATE_CHANGE:
> +				dstate = dw_pcie_readl_dbi(pci, DBI_CON_STATUS) & 0x3;
> +				dev_info(dev, "Received D%d state event\n", dstate);
> +				if (dstate == 3) {
> +					val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +					val |= BIT(1);
> +					writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +				}
> +				/* Handle D0 state change */

I don't understand this comment.

> +				break;
> +			case QCOM_PCIE_EP_INT_LINK_UP:
> +				dev_info(dev, "Received Linkup event. Enumeration complete!\n");
> +				dw_pcie_ep_linkup(&pci->ep);
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_UP;
> +				break;
> +			default:
> +				dev_err(dev, "Received unknown event: %d\n", event);

Are you sure that you want those dev_info() and dev_err() in irq
hanldler even when it is threaded? I'd drop all dev_info|err() or make
them dev_dbg() if they can help.

> +				break;
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> +{
> +	struct qcom_pcie_ep *pcie_ep = data;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	u32 perst;
> +
> +	perst = gpiod_get_value(pcie_ep->reset);
> +
> +	if (perst) {
> +		/* Start link training */
> +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> +		qcom_pcie_establish_link(pci);
> +	} else {
> +		/* Shutdown the link if the link is already on */
> +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");

Those prints sounds like a candidate for dev_dbg or even you can drop them.

> +		qcom_pcie_disable_link(pci);
> +	}
> +
> +	/* Set trigger type based on the next expected value of perst gpio */

IMO commenting obvious code is not good practice.

> +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> +					     struct qcom_pcie_ep *pcie_ep)
> +{
> +	int irq, ret;
> +
> +	irq = platform_get_irq_byname(pdev, "global");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					qcom_pcie_ep_global_threaded_irq,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					"global_irq", pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> +		return ret;
> +	}
> +
> +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> +	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> +					qcom_pcie_ep_perst_threaded_irq,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					"perst_irq", pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request PERST IRQ\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				  enum pci_epc_irq_type type, u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "Unknown IRQ type\n");
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct pci_epc_features qcom_pcie_epc_features = {
> +	.linkup_notifier = true,
> +	.core_init_notifier = true,
> +	.msi_capable = true,
> +	.msix_capable = false,
> +};
> +
> +static const struct pci_epc_features *
> +qcom_pcie_epc_get_features(struct dw_pcie_ep *pci_ep)
> +{
> +	return &qcom_pcie_epc_features;
> +}
> +
> +static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	enum pci_barno bar;
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static struct dw_pcie_ep_ops pci_ep_ops = {
> +	.ep_init = qcom_pcie_ep_init,
> +	.raise_irq = qcom_pcie_ep_raise_irq,
> +	.get_features = qcom_pcie_epc_get_features,
> +};
> +
> +static const struct of_device_id qcom_pcie_ep_match[] = {
> +	{ .compatible = "qcom,sdx55-pcie-ep", },
> +	{ }
> +};
> +
> +static int qcom_pcie_ep_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_pcie_ep *pcie_ep;
> +	int ret;
> +
> +	pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL);
> +	if (!pcie_ep)
> +		return -ENOMEM;
> +
> +	pcie_ep->pci.dev = dev;
> +	pcie_ep->pci.ops = &pci_ops;
> +	pcie_ep->pci.ep.ops = &pci_ep_ops;
> +
> +	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to get resources:%d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Enable power and clocks */

Useless comment.

> +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Perform controller reset */

ditto

> +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset the core: %d\n", ret);

qcom_pcie_ep_core_reset() already print errors

> +		goto err_disable_resources;
> +	}
> +
> +	/* Initialize PHY */

ditto

> +	ret = phy_init(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);

phy_init() already print an error.

> +		goto err_disable_resources;
> +	}
> +
> +	/* PHY needs to be powered on for dw_pcie_ep_init() */

useless comment.

> +	ret = phy_power_on(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);

phy_power_on already has prints in case of an error

> +		goto err_phy_exit;
> +	}
> +
> +	platform_set_drvdata(pdev, pcie_ep);
> +
> +	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to get IRQ resources %d\n", ret);

errors are printed already form qcom_pcie_ep_enable_irq_resources()

> +		goto err_phy_power_off;
> +	}
> +
> +	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize endpoint:%d\n", ret);

I think this dev_err() should be dropped.

Please revisit all those dev_err and dev_info in the driver and if
possible reduce thei

> +		goto err_phy_power_off;
> +	}
> +
> +	return 0;
> +
> +err_phy_power_off:
> +	phy_power_off(pcie_ep->phy);
> +err_phy_exit:
> +	phy_exit(pcie_ep->phy);
> +err_disable_resources:
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver qcom_pcie_ep_driver = {
> +	.probe	= qcom_pcie_ep_probe,
> +	.driver	= {
> +		.name		= "qcom-pcie-ep",
> +		.suppress_bind_attrs = true,
> +		.of_match_table	= qcom_pcie_ep_match,
> +	},
> +};
> +builtin_platform_driver(qcom_pcie_ep_driver);
> +
> +MODULE_AUTHOR("Siddartha Mohanadoss <smohanad@codeaurora.org>");
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm PCIe Endpoint controller driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 
regards,
Stan

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-03 10:38 ` [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
  2021-06-05 22:02   ` Stanimir Varbanov
@ 2021-06-06  3:07   ` Bjorn Andersson
  2021-06-09  8:51     ` Manivannan Sadhasivam
  2021-06-15 21:40     ` Rob Herring
  1 sibling, 2 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-06  3:07 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: lorenzo.pieralisi, robh, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel, Siddartha Mohanadoss

On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:

> Add driver support for Qualcomm PCIe Endpoint controller driver based on
> the Designware core with added Qualcomm specific wrapper around the
> core. The driver support is very basic such that it supports only
> enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> for now but these will be added later.
> 
> The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> operation and written on top of the DWC PCI framework.
> 
> Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> [mani: restructured the driver and fixed several bugs for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Really nice to see this working!

> ---
>  drivers/pci/controller/dwc/Kconfig        |  10 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
>  3 files changed, 791 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 423d35872ce4..32e735b1fd85 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -180,6 +180,16 @@ config PCIE_QCOM
>  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
>  	  hardware wrappers.
>  
> +config PCIE_QCOM_EP
> +	bool "Qualcomm PCIe controller - Endpoint mode"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> +	  plus Qualcomm-specific hardware wrappers.
> +
>  config PCIE_ARMADA_8K
>  	bool "Marvell Armada-8K PCIe controller"
>  	depends on ARCH_MVEBU || COMPILE_TEST
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..abb27642d46b 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> new file mode 100644
> index 000000000000..b68511bacc2a
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -0,0 +1,780 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm PCIe Endpoint controller driver
> + *
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + * Author: Siddartha Mohanadoss <smohanad@codeaurora.org
> + *
> + * Copyright (c) 2021, Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/delay.h>
> +#include <linux/phy/phy.h>
> +#include <linux/pm_domain.h>
> +
> +#include "pcie-designware.h"
> +
> +/* PARF registers */
> +#define PARF_SYS_CTRL				0x00
> +#define PARF_DB_CTRL				0x10
> +#define PARF_PM_CTRL				0x20
> +#define PARF_DEBUG_INT_EN			0x190
> +#define PARF_AXI_MSTR_RD_HALT_NO_WRITES		0x1a4
> +#define PARF_AXI_MSTR_WR_ADDR_HALT		0x1a8
> +#define PARF_Q2A_FLUSH				0x1aC
> +#define PARF_LTSSM				0x1b0
> +#define PARF_CFG_BITS				0x210
> +#define PARF_INT_ALL_STATUS			0x224
> +#define PARF_INT_ALL_CLEAR			0x228
> +#define PARF_INT_ALL_MASK			0x22c
> +#define PARF_SLV_ADDR_MSB_CTRL			0x2c0
> +#define PARF_DBI_BASE_ADDR			0x350
> +#define PARF_DBI_BASE_ADDR_HI			0x354
> +#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> +#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
> +#define PARF_ATU_BASE_ADDR			0x634
> +#define PARF_ATU_BASE_ADDR_HI			0x638
> +#define PARF_SRIS_MODE				0x644
> +#define PARF_DEVICE_TYPE			0x1000
> +#define PARF_BDF_TO_SID_CFG			0x2c00
> +
> +/* ELBI registers */
> +#define ELBI_SYS_STTS				0x08
> +
> +/* DBI registers */
> +#define DBI_CAP_ID_NXT_PTR			0x40
> +#define DBI_CON_STATUS				0x44
> +#define DBI_DEVICE_CAPABILITIES			0x74
> +#define DBI_LINK_CAPABILITIES			0x7c
> +#define DBI_LINK_CONTROL2_LINK_STATUS2		0xa0
> +#define DBI_L1SUB_CAPABILITY			0x234
> +#define DBI_ACK_F_ASPM_CTRL			0x70c
> +#define DBI_GEN3_RELATED_OFF			0x890
> +#define DBI_AUX_CLK_FREQ			0xb40
> +
> +#define DBI_L0S_ACCPT_LATENCY_MASK		GENMASK(8, 6)
> +#define DBI_L1_ACCPT_LATENCY_MASK		GENMASK(11, 9)
> +#define DBI_L0S_EXIT_LATENCY_MASK		GENMASK(14, 12)
> +#define DBI_L1_EXIT_LATENCY_MASK		GENMASK(17, 15)
> +#define DBI_ACK_N_FTS_MASK			GENMASK(15, 8)
> +
> +/* TCSR registers */
> +#define TCSR_PCIE_PERST_EN			0x258
> +#define TCSR_PERST_SEPARATION_ENABLE		0x270
> +
> +#define XMLH_LINK_UP				0x400
> +#define CORE_RESET_TIME_US_MIN			1000
> +#define CORE_RESET_TIME_US_MAX			1005
> +#define WAKE_DELAY_US				2000 /* 2 ms */
> +
> +#define to_pcie_ep(x)				dev_get_drvdata((x)->dev)

Isn't this also container_of(x, struct qcom_pcie_ep, pci), but without
the need to assigning and use drvdata?

> +
> +enum qcom_pcie_ep_link_status {
> +	QCOM_PCIE_EP_LINK_DISABLED,
> +	QCOM_PCIE_EP_LINK_ENABLED,
> +	QCOM_PCIE_EP_LINK_UP,
> +	QCOM_PCIE_EP_LINK_DOWN,
> +};
> +
> +enum qcom_pcie_ep_irq {
> +	QCOM_PCIE_EP_INT_RESERVED,
> +	QCOM_PCIE_EP_INT_LINK_DOWN,
> +	QCOM_PCIE_EP_INT_BME,
> +	QCOM_PCIE_EP_INT_PM_TURNOFF,
> +	QCOM_PCIE_EP_INT_DEBUG,
> +	QCOM_PCIE_EP_INT_LTR,
> +	QCOM_PCIE_EP_INT_MHI_Q6,
> +	QCOM_PCIE_EP_INT_MHI_A7,
> +	QCOM_PCIE_EP_INT_DSTATE_CHANGE,
> +	QCOM_PCIE_EP_INT_L1SUB_TIMEOUT,
> +	QCOM_PCIE_EP_INT_MMIO_WRITE,
> +	QCOM_PCIE_EP_INT_CFG_WRITE,
> +	QCOM_PCIE_EP_INT_BRIDGE_FLUSH_N,
> +	QCOM_PCIE_EP_INT_LINK_UP,
> +	QCOM_PCIE_EP_INT_AER_LEGACY,
> +	QCOM_PCIE_EP_INT_PLS_ERR,
> +	QCOM_PCIE_EP_INT_PME_LEGACY,
> +	QCOM_PCIE_EP_INT_PLS_PME,
> +	QCOM_PCIE_EP_INT_MAX,

Afaict these aren't some logical sequence of numbers, they are bit
offsets in the interrupt registers. Using #define would make this
obvious.

> +};
> +
> +static struct clk_bulk_data qcom_pcie_ep_clks[] = {
> +	{ .id = "cfg" },
> +	{ .id = "aux" },
> +	{ .id = "bus_master" },
> +	{ .id = "bus_slave" },
> +	{ .id = "ref" },
> +	{ .id = "sleep" },
> +	{ .id = "slave_q2a" },
> +};
> +
> +struct qcom_pcie_ep {
> +	struct dw_pcie pci;
> +
> +	void __iomem *parf;
> +	void __iomem *elbi;
> +	void __iomem *tcsr;
> +
> +	struct reset_control *core_reset;
> +	struct gpio_desc *reset;
> +	struct gpio_desc *wake;
> +	struct phy *phy;
> +
> +	resource_size_t dbi_phys;
> +	resource_size_t atu_phys;
> +
> +	enum qcom_pcie_ep_link_status link_status;
> +	int global_irq;
> +	int perst_irq;
> +};
> +
> +static void qcom_pcie_ep_enable_ltssm(struct qcom_pcie_ep *pcie_ep)
> +{
> +	u32 reg;
> +
> +	reg = readl(pcie_ep->parf + PARF_LTSSM);
> +	reg |= BIT(8);
> +	writel_relaxed(reg, pcie_ep->parf + PARF_LTSSM);
> +}
> +
> +static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	int ret = 0;

No need to initialize ret, when the first access is an assignment.

> +
> +	ret = reset_control_assert(pcie_ep->core_reset);
> +	if (ret) {
> +		dev_err(dev, "Cannot assert core reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> +
> +	ret = reset_control_deassert(pcie_ep->core_reset);
> +	if (ret) {
> +		dev_err(dev, "Cannot de-assert core reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> +
> +	return 0;
> +}
> +
> +/*
> + * Delatch PERST_EN and PERST_SEPARATION_ENABLE with TCSR to avoid
> + * device reset during host reboot and hibernation. The driver is
> + * expected to handle this situation.
> + */
> +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> +{
> +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);

Please avoid _relaxed accessor unless there's a strong reason, and if so
document it.

> +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> +}
> +
> +static int qcom_pcie_ep_enable_resources(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qcom_pcie_ep_clks),
> +				      qcom_pcie_ep_clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clocks: %d\n", ret);

clk_bulk_prepare_enable() will print an error indicating which of the
clocks it failed to prepare/enable, so it's better to skip this.

As such, you can simply do:

	return clk_bulk_prepare_enable();

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void qcom_pcie_ep_disable_resources(struct qcom_pcie_ep *pcie_ep)
> +{
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(qcom_pcie_ep_clks),
> +				   qcom_pcie_ep_clks);
> +}
> +
> +static void qcom_pcie_ep_configure_irq(struct qcom_pcie_ep *pcie_ep)
> +{
> +	u32 val;
> +
> +	writel_relaxed(0, pcie_ep->parf + PARF_INT_ALL_MASK);
> +	val = BIT(QCOM_PCIE_EP_INT_LINK_DOWN) |
> +		BIT(QCOM_PCIE_EP_INT_BME) |
> +		BIT(QCOM_PCIE_EP_INT_PM_TURNOFF) |
> +		BIT(QCOM_PCIE_EP_INT_DSTATE_CHANGE) |
> +		BIT(QCOM_PCIE_EP_INT_LINK_UP);
> +	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);

Again, please avoid the _relaxed versions.

> +}
> +
> +static int qcom_pcie_ep_core_init(struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	u32 val;
> +
> +	/* Disable BDF to SID mapping */
> +	val = readl_relaxed(pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> +	val |= BIT(0);
> +	writel_relaxed(val, pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> +
> +	/* enable debug IRQ */
> +	writel_relaxed((BIT(3) | BIT(2) | BIT(1)),
> +		       pcie_ep->parf + PARF_DEBUG_INT_EN);
> +
> +	/* Configure PCIe to endpoint mode */
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_DEVICE_TYPE);
> +
> +	/* Configure PCIe core to support 1GB aperture */
> +	writel_relaxed(0x40000000, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +
> +	/* Allow entering L1 state */
> +	val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +	val &= ~BIT(5);
> +	writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +
> +	/* Configure Slave, DBI and iATU base addresses */
> +	writel_relaxed(BIT(0), pcie_ep->parf + PARF_SLV_ADDR_MSB_CTRL);
> +	writel_relaxed(0x200, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +	writel_relaxed(0x100, pcie_ep->parf + PARF_DBI_BASE_ADDR_HI);
> +	writel_relaxed(pcie_ep->dbi_phys, pcie_ep->parf + PARF_DBI_BASE_ADDR);
> +	writel_relaxed(0x100, pcie_ep->parf + PARF_ATU_BASE_ADDR_HI);
> +	writel_relaxed(pcie_ep->atu_phys, pcie_ep->parf + PARF_ATU_BASE_ADDR);
> +
> +	/* Read halts write */
> +	writel_relaxed(0x0, pcie_ep->parf + PARF_AXI_MSTR_RD_HALT_NO_WRITES);
> +	/* Write after write halt */
> +	writel_relaxed(BIT(31), pcie_ep->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
> +	/* Q2A flush disable */
> +	writel_relaxed(0, pcie_ep->parf + PARF_Q2A_FLUSH);
> +
> +	/* Disable the DBI Wakeup */
> +	writel_relaxed(BIT(11), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Disable the debouncers */
> +	writel_relaxed(0x73, pcie_ep->parf + PARF_DB_CTRL);
> +	/* Disable core clock CGC */
> +	writel_relaxed(BIT(6), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Set AUX power to be on */
> +	writel_relaxed(BIT(4), pcie_ep->parf + PARF_SYS_CTRL);
> +	/* Request to exit from L1SS for MSI and LTR MSG */
> +	writel_relaxed(BIT(1), pcie_ep->parf + PARF_CFG_BITS);
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	/* Set the PMC Register - to support PME in D0/D3hot/D3cold */
> +	val = dw_pcie_readl_dbi(pci, DBI_CAP_ID_NXT_PTR);
> +	val |= BIT(31) | BIT(30) | BIT(27);
> +	dw_pcie_writel_dbi(pci, DBI_CAP_ID_NXT_PTR, val);
> +
> +	/* Set the Endpoint L0s Acceptable Latency to 1us (max) */
> +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L0S_ACCPT_LATENCY_MASK, 0x7);
> +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> +
> +	/* Set the Endpoint L1 Acceptable Latency to 1us (max) */
> +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L1_ACCPT_LATENCY_MASK, 0x7);
> +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> +
> +	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L0S_EXIT_LATENCY_MASK, 0x6);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= FIELD_PREP(DBI_L1_EXIT_LATENCY_MASK, 0x6);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	/* L1ss is supported */
> +	val = dw_pcie_readl_dbi(pci, DBI_L1SUB_CAPABILITY);
> +	val |= 0x1f;
> +	dw_pcie_writel_dbi(pci, DBI_L1SUB_CAPABILITY, val);
> +
> +	/* Enable Clock Power Management */
> +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> +	val |= BIT(18);
> +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	/* Set FTS value to match the PHY setting */
> +	val = dw_pcie_readl_dbi(pci, DBI_ACK_F_ASPM_CTRL);
> +	val |= FIELD_PREP(DBI_ACK_N_FTS_MASK, 0x80);
> +	dw_pcie_writel_dbi(pci, DBI_ACK_F_ASPM_CTRL, val);
> +
> +	dw_pcie_writel_dbi(pci, DBI_AUX_CLK_FREQ, 0x14);
> +
> +	/* Prevent L1ss wakeup after 100ms */
> +	val = dw_pcie_readl_dbi(pci, DBI_GEN3_RELATED_OFF);
> +	val &= ~BIT(0);
> +	dw_pcie_writel_dbi(pci, DBI_GEN3_RELATED_OFF, val);
> +
> +	/* Disable SRIS_MODE */
> +	val = readl_relaxed(pcie_ep->parf + PARF_SRIS_MODE);
> +	val &= ~BIT(0);
> +	writel_relaxed(val, pcie_ep->parf + PARF_SRIS_MODE);
> +
> +	qcom_pcie_ep_configure_irq(pcie_ep);

Why is the irq part broken out to its own function?

> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_confirm_linkup(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	u32 reg;
> +
> +	reg = readl_relaxed(pcie_ep->elbi + ELBI_SYS_STTS);
> +
> +	return reg & XMLH_LINK_UP;
> +}
> +
> +static int qcom_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +
> +	enable_irq(pcie_ep->perst_irq);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_establish_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_ENABLED) {
> +		dev_info(dev, "Link is already enabled\n");

Per qcom_pcie_ep_global_threaded_irq() it seems possible that the link
is enabled but link_status != QCOM_PCIE_EP_LINK_ENABLED.

I'm also not sure how you can end up here with link_status == ENABLED;
it starts off DISABLED and expect the first thing to happen is "perst"
asserting, which would end up here and after that the only way you get
here again is if perst is asserted again - without first being
deasserted. So perhaps this is part of your manual edge detector?

Perhaps it's not needed if you actually use edge detection on your
interrupt?

> +		return 0;
> +	}
> +
> +	/* Enable power and clocks */
> +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Perform controller reset */
> +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* Initialize PHY */
> +	ret = phy_init(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* Power ON PHY */
> +	ret = phy_power_on(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> +		goto err_phy_exit;
> +	}
> +
> +	/* Assert WAKE# to RC to indicate device is ready */
> +	gpiod_set_value_cansleep(pcie_ep->wake, 0);

Assert sounds like "activate" or "turn on", so I would prefer that you
make this a logical 1 here and if the signal is inverted specify this
when you're requesting the gpio.

> +	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
> +	gpiod_set_value_cansleep(pcie_ep->wake, 1);
> +
> +	/* Configure TCSR */
> +	qcom_pcie_ep_configure_tcsr(pcie_ep);
> +
> +	/* Initialize the controller */
> +	ret = qcom_pcie_ep_core_init(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to init controller: %d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
> +
> +	/* Enable LTSSM */
> +	qcom_pcie_ep_enable_ltssm(pcie_ep);
> +
> +	return 0;
> +/* TODO* assert reset? */
> +err_phy_power_off:
> +	phy_power_off(pcie_ep->phy);
> +err_phy_exit:
> +	phy_exit(pcie_ep->phy);
> +err_disable_resources:
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +
> +	return ret;
> +}
> +
> +static void qcom_pcie_disable_link(struct dw_pcie *pci)
> +{
> +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> +	struct device *dev = pci->dev;
> +
> +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> +		dev_info(dev, "Link is already disabled\n");

Why can disable_link happen both from the interrupt handler directly and
from the dw_pcie_ops?

> +		return;
> +	}
> +
> +	phy_power_off(pcie_ep->phy);
> +	phy_exit(pcie_ep->phy);
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> +}
> +
> +/* Common DWC controller ops */
> +static const struct dw_pcie_ops pci_ops = {
> +	.link_up = qcom_pcie_confirm_linkup,
> +	.start_link = qcom_pcie_start_link,
> +	.stop_link = qcom_pcie_disable_link,
> +};
> +
> +static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> +					 struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
> +	pcie_ep->parf = devm_ioremap_resource(dev, res);

Better make this devm_platform_ioremap_resource_byname(), or someone
will send a patch the minute this code lands.

> +	if (IS_ERR(pcie_ep->parf))
> +		return PTR_ERR(pcie_ep->parf);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pci->dbi_base))
> +		return PTR_ERR(pci->dbi_base);
> +	pci->dbi_base2 = pci->dbi_base;
> +	pcie_ep->dbi_phys = res->start;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
> +	pcie_ep->elbi = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pcie_ep->elbi))
> +		return PTR_ERR(pcie_ep->elbi);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> +	pci->atu_base = devm_pci_remap_cfg_resource(dev, res);
> +	if (IS_ERR(pci->atu_base))
> +		return PTR_ERR(pci->atu_base);
> +	pcie_ep->atu_phys = res->start;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> +	pcie_ep->tcsr = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(pcie_ep->tcsr))
> +		return PTR_ERR(pcie_ep->tcsr);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> +				      struct qcom_pcie_ep *pcie_ep)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = qcom_pcie_ep_get_io_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get io resources %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Enable GDSC power domain */
> +	ret = dev_pm_domain_attach(dev, true);

If you only have a single power-domain specified, then it would already
be attached to "dev".

> +	if (ret)
> +		return ret;
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(qcom_pcie_ep_clks),
> +				qcom_pcie_ep_clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks: %d\n", ret);

Don't you already have a print in devm_clk_bulk_get()?

> +		return ret;
> +	}
> +
> +	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> +	if (IS_ERR(pcie_ep->core_reset))
> +		return PTR_ERR(pcie_ep->core_reset);
> +
> +	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> +	if (IS_ERR(pcie_ep->reset))
> +		return PTR_ERR(pcie_ep->reset);
> +
> +	pcie_ep->wake = devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pcie_ep->wake))
> +		return PTR_ERR(pcie_ep->wake);
> +
> +	pcie_ep->phy = devm_phy_optional_get(&pdev->dev, "pciephy");
> +	if (IS_ERR(pcie_ep->phy))
> +		ret = PTR_ERR(pcie_ep->phy);
> +
> +	return ret;
> +}
> +
> +/* TODO: Notify clients about PCIe state change */
> +static irqreturn_t qcom_pcie_ep_global_threaded_irq(int irq, void *data)
> +{
> +	struct qcom_pcie_ep *pcie_ep = data;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	u32 status = readl(pcie_ep->parf + PARF_INT_ALL_STATUS);
> +	u32 mask = readl(pcie_ep->parf + PARF_INT_ALL_MASK);
> +	u32 dstate, event, val;
> +
> +	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);
> +	status &= mask;
> +
> +	for (event = 1; event < QCOM_PCIE_EP_INT_MAX; event++) {
> +		if (status & BIT(event)) {

for_each_set_bit()

> +			switch (event) {
> +			case QCOM_PCIE_EP_INT_LINK_DOWN:
> +				dev_info(dev, "Received Linkdown event\n");
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN;
> +				break;
> +			case QCOM_PCIE_EP_INT_BME:
> +				dev_info(dev, "Received BME event. Link is enabled!\n");
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
> +				break;
> +			case QCOM_PCIE_EP_INT_PM_TURNOFF:
> +				dev_info(dev, "Received PM Turn-off event! Entering L23\n");
> +				val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +				val |= BIT(2);
> +				writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +				break;
> +			case QCOM_PCIE_EP_INT_DSTATE_CHANGE:
> +				dstate = dw_pcie_readl_dbi(pci, DBI_CON_STATUS) & 0x3;
> +				dev_info(dev, "Received D%d state event\n", dstate);
> +				if (dstate == 3) {
> +					val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> +					val |= BIT(1);
> +					writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> +				}
> +				/* Handle D0 state change */
> +				break;
> +			case QCOM_PCIE_EP_INT_LINK_UP:
> +				dev_info(dev, "Received Linkup event. Enumeration complete!\n");
> +				dw_pcie_ep_linkup(&pci->ep);
> +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_UP;
> +				break;
> +			default:
> +				dev_err(dev, "Received unknown event: %d\n", event);
> +				break;
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> +{
> +	struct qcom_pcie_ep *pcie_ep = data;
> +	struct dw_pcie *pci = &pcie_ep->pci;
> +	struct device *dev = pci->dev;
> +	u32 perst;
> +
> +	perst = gpiod_get_value(pcie_ep->reset);
> +
> +	if (perst) {
> +		/* Start link training */
> +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> +		qcom_pcie_establish_link(pci);
> +	} else {
> +		/* Shutdown the link if the link is already on */
> +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> +		qcom_pcie_disable_link(pci);
> +	}
> +
> +	/* Set trigger type based on the next expected value of perst gpio */
> +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));

Looks like you're manually implementing edge triggering, is there any
reason for that? EDGE_BOTH seems to do the same thing...

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> +					     struct qcom_pcie_ep *pcie_ep)
> +{
> +	int irq, ret;
> +
> +	irq = platform_get_irq_byname(pdev, "global");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					qcom_pcie_ep_global_threaded_irq,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,

Leave out the trigger and rely on DT.

> +					"global_irq", pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> +		return ret;
> +	}
> +
> +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);

Is the global interrupt needed for dw_pcie_ep_init()? Or could this
simply be done when things are ready to handle the interrupts?

> +	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> +					qcom_pcie_ep_perst_threaded_irq,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					"perst_irq", pcie_ep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request PERST IRQ\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				  enum pci_epc_irq_type type, u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "Unknown IRQ type\n");
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct pci_epc_features qcom_pcie_epc_features = {
> +	.linkup_notifier = true,
> +	.core_init_notifier = true,
> +	.msi_capable = true,
> +	.msix_capable = false,
> +};
> +
> +static const struct pci_epc_features *
> +qcom_pcie_epc_get_features(struct dw_pcie_ep *pci_ep)
> +{
> +	return &qcom_pcie_epc_features;
> +}
> +
> +static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	enum pci_barno bar;
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static struct dw_pcie_ep_ops pci_ep_ops = {
> +	.ep_init = qcom_pcie_ep_init,
> +	.raise_irq = qcom_pcie_ep_raise_irq,
> +	.get_features = qcom_pcie_epc_get_features,
> +};
> +
> +static const struct of_device_id qcom_pcie_ep_match[] = {
> +	{ .compatible = "qcom,sdx55-pcie-ep", },
> +	{ }
> +};

Move this below the probe function.

> +
> +static int qcom_pcie_ep_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_pcie_ep *pcie_ep;
> +	int ret;
> +
> +	pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL);
> +	if (!pcie_ep)
> +		return -ENOMEM;
> +
> +	pcie_ep->pci.dev = dev;
> +	pcie_ep->pci.ops = &pci_ops;
> +	pcie_ep->pci.ep.ops = &pci_ep_ops;
> +
> +	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to get resources:%d\n", ret);

You printed an error in about half the error cases in
qcom_pcie_ep_get_resources(), it would be better to ensure you print a
specific error for each of the remaining cases there and "silently"
return here.

Same comment for below cases.

> +		return ret;
> +	}
> +
> +	/* Enable power and clocks */
> +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Perform controller reset */
> +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* Initialize PHY */
> +	ret = phy_init(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> +		goto err_disable_resources;
> +	}
> +
> +	/* PHY needs to be powered on for dw_pcie_ep_init() */
> +	ret = phy_power_on(pcie_ep->phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> +		goto err_phy_exit;
> +	}
> +
> +	platform_set_drvdata(pdev, pcie_ep);
> +
> +	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to get IRQ resources %d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize endpoint:%d\n", ret);
> +		goto err_phy_power_off;
> +	}
> +
> +	return 0;
> +
> +err_phy_power_off:

Your interrupts remains enabled until devres takes them down after
returning, so I suspect you need to disable them here as well - at least
before shutting down the clocks.

> +	phy_power_off(pcie_ep->phy);
> +err_phy_exit:
> +	phy_exit(pcie_ep->phy);
> +err_disable_resources:
> +	qcom_pcie_ep_disable_resources(pcie_ep);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver qcom_pcie_ep_driver = {
> +	.probe	= qcom_pcie_ep_probe,
> +	.driver	= {
> +		.name		= "qcom-pcie-ep",

Skip the indentation of the '='.

> +		.suppress_bind_attrs = true,

Why do we suppress_bind_attrs?

Regards,
Bjorn

> +		.of_match_table	= qcom_pcie_ep_match,
> +	},
> +};
> +builtin_platform_driver(qcom_pcie_ep_driver);
> +
> +MODULE_AUTHOR("Siddartha Mohanadoss <smohanad@codeaurora.org>");
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm PCIe Endpoint controller driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.25.1
> 

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

* Re: [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller
  2021-06-03 10:38 ` [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
@ 2021-06-06  3:13   ` Bjorn Andersson
  2021-06-10 21:46     ` Rob Herring
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-06  3:13 UTC (permalink / raw)
  To: Manivannan Sadhasivam, robh
  Cc: lorenzo.pieralisi, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel

On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:

> Add devicetree binding for Qualcomm PCIe EP controller used in platforms
> like SDX55. The EP controller is based on the Designware core with
> Qualcomm specific wrappers.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++
>  1 file changed, 144 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> new file mode 100644
> index 000000000000..3e357cb03a5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PCIe Endpoint Controller binding
> +
> +maintainers:
> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> +
> +allOf:
> +  - $ref: "pci-ep.yaml#"
> +
> +properties:
> +  compatible:
> +    const: qcom,sdx55-pcie-ep

The binding looks good, but this is going to cause us an inevitable
warning as we'd have to describe the controller twice (rc + ep) in the
sdx55.dtsi.

@Rob, what do you suggest we do about this, because it's the same
problem currently responsible for hundreds of warnings in the case of
GENI (where each node is duplicated for different functions).

> +
> +  reg:
> +    items:
> +      - description: Qualcomm specific PARF configuration registers
> +      - description: Designware PCIe registers
> +      - description: External local bus interface registers
> +      - description: Address Translation Unit (ATU) registers
> +      - description: Memory region used to map remote RC address space
> +      - description: Qualcomm specific TCSR registers

tcsr is separate hardware block with "misc" registers, I think it's
better if we describe that as qcom,sdx55-tcsr, syscon and use the syscon
API to access those registers...

Regards,
Bjorn

> +
> +  reg-names:
> +    items:
> +      - const: parf
> +      - const: dbi
> +      - const: elbi
> +      - const: atu
> +      - const: addr_space
> +      - const: tcsr
> +
> +  clocks:
> +    items:
> +      - description: PCIe Auxiliary clock
> +      - description: PCIe CFG AHB clock
> +      - description: PCIe Master AXI clock
> +      - description: PCIe Slave AXI clock
> +      - description: PCIe Slave Q2A AXI clock
> +      - description: PCIe Sleep clock
> +      - description: PCIe Reference clock
> +
> +  clock-names:
> +    items:
> +      - const: aux
> +      - const: cfg
> +      - const: bus_master
> +      - const: bus_slave
> +      - const: slave_q2a
> +      - const: sleep
> +      - const: ref
> +
> +  interrupts:
> +    maxItems: 1
> +    description: PCIe Global interrupt
> +
> +  interrupt-names:
> +    const: global
> +
> +  reset-gpios:
> +    description: GPIO that is being used as PERST# input signal
> +    maxItems: 1
> +
> +  wake-gpios:
> +    description: GPIO that is being used as WAKE# output signal
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: core
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pciephy
> +
> +  num-lanes:
> +    default: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - reset-gpios
> +  - resets
> +  - reset-names
> +  - power-domains
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sdx55.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    pcie_ep: pcie-ep@40000000 {
> +        compatible = "qcom,sdx55-pcie-ep";
> +        reg = <0x01c00000 0x3000>,
> +              <0x40000000 0xf1d>,
> +              <0x40000f20 0xc8>,
> +              <0x40001000 0x1000>,
> +              <0x42000000 0x1000>,
> +              <0x01fcb000 0x1000>;
> +        reg-names = "parf", "dbi", "elbi", "atu", "addr_space",
> +                    "tcsr";
> +
> +        clocks = <&gcc GCC_PCIE_AUX_CLK>,
> +             <&gcc GCC_PCIE_CFG_AHB_CLK>,
> +             <&gcc GCC_PCIE_MSTR_AXI_CLK>,
> +             <&gcc GCC_PCIE_SLV_AXI_CLK>,
> +             <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,
> +             <&gcc GCC_PCIE_SLEEP_CLK>,
> +             <&gcc GCC_PCIE_0_CLKREF_CLK>;
> +        clock-names = "aux", "cfg", "bus_master", "bus_slave",
> +                      "slave_q2a", "sleep", "ref";
> +
> +        interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "global";
> +        reset-gpios = <&tlmm 57 GPIO_ACTIVE_HIGH>;
> +        wake-gpios = <&tlmm 53 GPIO_ACTIVE_LOW>;
> +        resets = <&gcc GCC_PCIE_BCR>;
> +        reset-names = "core";
> +        power-domains = <&gcc PCIE_GDSC>;
> +        phys = <&pcie0_lane>;
> +        phy-names = "pciephy";
> +        max-link-speed = <3>;
> +        num-lanes = <2>;
> +
> +        status = "disabled";
> +    };
> -- 
> 2.25.1
> 

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-05 22:02   ` Stanimir Varbanov
@ 2021-06-09  5:20     ` Manivannan Sadhasivam
  0 siblings, 0 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-09  5:20 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: lorenzo.pieralisi, robh, bhelgaas, bjorn.andersson,
	linux-arm-msm, linux-pci, devicetree, linux-kernel,
	Siddartha Mohanadoss

Hi Stan,

On Sun, Jun 06, 2021 at 01:02:26AM +0300, Stanimir Varbanov wrote:
> Hi Mani,
> 
> On 6/3/21 1:38 PM, Manivannan Sadhasivam wrote:
> > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > the Designware core with added Qualcomm specific wrapper around the
> > core. The driver support is very basic such that it supports only
> > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > for now but these will be added later.
> > 
> > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > operation and written on top of the DWC PCI framework.
> > 
> > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > [mani: restructured the driver and fixed several bugs for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/Kconfig        |  10 +
> >  drivers/pci/controller/dwc/Makefile       |   1 +
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
> >  3 files changed, 791 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 423d35872ce4..32e735b1fd85 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -180,6 +180,16 @@ config PCIE_QCOM
> >  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> >  	  hardware wrappers.
> >  
> > +config PCIE_QCOM_EP
> > +	bool "Qualcomm PCIe controller - Endpoint mode"
> > +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> > +	depends on PCI_ENDPOINT
> > +	select PCIE_DW_EP
> 
> Do you see a problem if root-complex driver and EP drive are enabled on
> the same time? I think the PCIe IP can work only on one of both modes.
>

Well if there is a single IP then it can only work in single mode but what if
there is more that 1 in an SoC? That's why I didn't make it mutually exclusive.
 
> > +	help
> > +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> > +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> > +	  plus Qualcomm-specific hardware wrappers.
> > +
> >  config PCIE_ARMADA_8K
> >  	bool "Marvell Armada-8K PCIe controller"
> >  	depends on ARCH_MVEBU || COMPILE_TEST
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index eca805c1a023..abb27642d46b 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> >  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> >  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> >  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> >  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> >  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> >  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > new file mode 100644
> > index 000000000000..b68511bacc2a
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -0,0 +1,780 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Qualcomm PCIe Endpoint controller driver
> > + *
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + * Author: Siddartha Mohanadoss <smohanad@codeaurora.org
> > + *
> > + * Copyright (c) 2021, Linaro Ltd.
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/reset.h>
> > +#include <linux/delay.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/pm_domain.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +/* PARF registers */
> > +#define PARF_SYS_CTRL				0x00
> > +#define PARF_DB_CTRL				0x10
> > +#define PARF_PM_CTRL				0x20
> > +#define PARF_DEBUG_INT_EN			0x190
> > +#define PARF_AXI_MSTR_RD_HALT_NO_WRITES		0x1a4
> > +#define PARF_AXI_MSTR_WR_ADDR_HALT		0x1a8
> > +#define PARF_Q2A_FLUSH				0x1aC
> > +#define PARF_LTSSM				0x1b0
> > +#define PARF_CFG_BITS				0x210
> > +#define PARF_INT_ALL_STATUS			0x224
> > +#define PARF_INT_ALL_CLEAR			0x228
> > +#define PARF_INT_ALL_MASK			0x22c
> > +#define PARF_SLV_ADDR_MSB_CTRL			0x2c0
> > +#define PARF_DBI_BASE_ADDR			0x350
> > +#define PARF_DBI_BASE_ADDR_HI			0x354
> > +#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> > +#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
> > +#define PARF_ATU_BASE_ADDR			0x634
> > +#define PARF_ATU_BASE_ADDR_HI			0x638
> > +#define PARF_SRIS_MODE				0x644
> > +#define PARF_DEVICE_TYPE			0x1000
> > +#define PARF_BDF_TO_SID_CFG			0x2c00
> > +
> > +/* ELBI registers */
> > +#define ELBI_SYS_STTS				0x08
> > +
> > +/* DBI registers */
> > +#define DBI_CAP_ID_NXT_PTR			0x40
> > +#define DBI_CON_STATUS				0x44
> > +#define DBI_DEVICE_CAPABILITIES			0x74
> > +#define DBI_LINK_CAPABILITIES			0x7c
> > +#define DBI_LINK_CONTROL2_LINK_STATUS2		0xa0
> > +#define DBI_L1SUB_CAPABILITY			0x234
> > +#define DBI_ACK_F_ASPM_CTRL			0x70c
> > +#define DBI_GEN3_RELATED_OFF			0x890
> > +#define DBI_AUX_CLK_FREQ			0xb40
> > +
> > +#define DBI_L0S_ACCPT_LATENCY_MASK		GENMASK(8, 6)
> > +#define DBI_L1_ACCPT_LATENCY_MASK		GENMASK(11, 9)
> > +#define DBI_L0S_EXIT_LATENCY_MASK		GENMASK(14, 12)
> > +#define DBI_L1_EXIT_LATENCY_MASK		GENMASK(17, 15)
> > +#define DBI_ACK_N_FTS_MASK			GENMASK(15, 8)
> > +
> > +/* TCSR registers */
> > +#define TCSR_PCIE_PERST_EN			0x258
> > +#define TCSR_PERST_SEPARATION_ENABLE		0x270
> > +
> > +#define XMLH_LINK_UP				0x400
> > +#define CORE_RESET_TIME_US_MIN			1000
> > +#define CORE_RESET_TIME_US_MAX			1005
> > +#define WAKE_DELAY_US				2000 /* 2 ms */
> > +
> > +#define to_pcie_ep(x)				dev_get_drvdata((x)->dev)
> 
> I don't think that this define adds some more readability or something else.
> 

I usually wrap the get_drvdata in a define to make it explicit on what data we
are getting. In this case it is pcie_ep but yeah it is not very useful here.
I'll remove it.

> > +
> > +enum qcom_pcie_ep_link_status {
> > +	QCOM_PCIE_EP_LINK_DISABLED,
> > +	QCOM_PCIE_EP_LINK_ENABLED,
> > +	QCOM_PCIE_EP_LINK_UP,
> > +	QCOM_PCIE_EP_LINK_DOWN,
> > +};
> > +
> > +enum qcom_pcie_ep_irq {
> > +	QCOM_PCIE_EP_INT_RESERVED,
> > +	QCOM_PCIE_EP_INT_LINK_DOWN,
> > +	QCOM_PCIE_EP_INT_BME,
> > +	QCOM_PCIE_EP_INT_PM_TURNOFF,
> > +	QCOM_PCIE_EP_INT_DEBUG,
> > +	QCOM_PCIE_EP_INT_LTR,
> > +	QCOM_PCIE_EP_INT_MHI_Q6,
> > +	QCOM_PCIE_EP_INT_MHI_A7,
> > +	QCOM_PCIE_EP_INT_DSTATE_CHANGE,
> > +	QCOM_PCIE_EP_INT_L1SUB_TIMEOUT,
> > +	QCOM_PCIE_EP_INT_MMIO_WRITE,
> > +	QCOM_PCIE_EP_INT_CFG_WRITE,
> > +	QCOM_PCIE_EP_INT_BRIDGE_FLUSH_N,
> > +	QCOM_PCIE_EP_INT_LINK_UP,
> > +	QCOM_PCIE_EP_INT_AER_LEGACY,
> > +	QCOM_PCIE_EP_INT_PLS_ERR,
> > +	QCOM_PCIE_EP_INT_PME_LEGACY,
> > +	QCOM_PCIE_EP_INT_PLS_PME,
> > +	QCOM_PCIE_EP_INT_MAX,
> > +};
> > +
> > +static struct clk_bulk_data qcom_pcie_ep_clks[] = {
> > +	{ .id = "cfg" },
> > +	{ .id = "aux" },
> > +	{ .id = "bus_master" },
> > +	{ .id = "bus_slave" },
> > +	{ .id = "ref" },
> > +	{ .id = "sleep" },
> > +	{ .id = "slave_q2a" },
> > +};
> > +
> > +struct qcom_pcie_ep {
> > +	struct dw_pcie pci;
> > +
> > +	void __iomem *parf;
> > +	void __iomem *elbi;
> > +	void __iomem *tcsr;
> > +
> > +	struct reset_control *core_reset;
> > +	struct gpio_desc *reset;
> > +	struct gpio_desc *wake;
> > +	struct phy *phy;
> > +
> > +	resource_size_t dbi_phys;
> > +	resource_size_t atu_phys;
> > +
> > +	enum qcom_pcie_ep_link_status link_status;
> > +	int global_irq;
> > +	int perst_irq;
> > +};
> > +
> > +static void qcom_pcie_ep_enable_ltssm(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	u32 reg;
> > +
> > +	reg = readl(pcie_ep->parf + PARF_LTSSM);
> > +	reg |= BIT(8);
> > +	writel_relaxed(reg, pcie_ep->parf + PARF_LTSSM);
> > +}
> > +
> > +static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	int ret = 0;
> > +
> > +	ret = reset_control_assert(pcie_ep->core_reset);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot assert core reset\n");
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> > +
> > +	ret = reset_control_deassert(pcie_ep->core_reset);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot de-assert core reset\n");
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Delatch PERST_EN and PERST_SEPARATION_ENABLE with TCSR to avoid
> > + * device reset during host reboot and hibernation. The driver is
> > + * expected to handle this situation.
> > + */
> > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> > +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> 
> Are you sure that _relaxed() variants (here and on the other places in
> the driver) is enough? I'd use writel() for all places which are not
> performance sesnsitive.
> 

Okay, it makes sense. This is not a "hot path", so writel should be fine.
I'll revisit all the instances.

> > +}
> > +
> > +static int qcom_pcie_ep_enable_resources(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	int ret;
> > +
> > +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				      qcom_pcie_ep_clks);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clocks: %d\n", ret);
> 
> clk_bulk_prepare_enable already print errors so please drop this dev_err().
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void qcom_pcie_ep_disable_resources(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	clk_bulk_disable_unprepare(ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				   qcom_pcie_ep_clks);
> > +}
> > +
> > +static void qcom_pcie_ep_configure_irq(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	u32 val;
> > +
> > +	writel_relaxed(0, pcie_ep->parf + PARF_INT_ALL_MASK);
> > +	val = BIT(QCOM_PCIE_EP_INT_LINK_DOWN) |
> > +		BIT(QCOM_PCIE_EP_INT_BME) |
> > +		BIT(QCOM_PCIE_EP_INT_PM_TURNOFF) |
> > +		BIT(QCOM_PCIE_EP_INT_DSTATE_CHANGE) |
> > +		BIT(QCOM_PCIE_EP_INT_LINK_UP);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);
> > +}
> > +
> > +static int qcom_pcie_ep_core_init(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	u32 val;
> > +
> > +	/* Disable BDF to SID mapping */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> > +	val |= BIT(0);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> > +
> > +	/* enable debug IRQ */
> > +	writel_relaxed((BIT(3) | BIT(2) | BIT(1)),
> > +		       pcie_ep->parf + PARF_DEBUG_INT_EN);
> > +
> > +	/* Configure PCIe to endpoint mode */
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_DEVICE_TYPE);
> > +
> > +	/* Configure PCIe core to support 1GB aperture */
> > +	writel_relaxed(0x40000000, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +
> > +	/* Allow entering L1 state */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +	val &= ~BIT(5);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +
> > +	/* Configure Slave, DBI and iATU base addresses */
> > +	writel_relaxed(BIT(0), pcie_ep->parf + PARF_SLV_ADDR_MSB_CTRL);
> > +	writel_relaxed(0x200, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +	writel_relaxed(0x100, pcie_ep->parf + PARF_DBI_BASE_ADDR_HI);
> > +	writel_relaxed(pcie_ep->dbi_phys, pcie_ep->parf + PARF_DBI_BASE_ADDR);
> > +	writel_relaxed(0x100, pcie_ep->parf + PARF_ATU_BASE_ADDR_HI);
> > +	writel_relaxed(pcie_ep->atu_phys, pcie_ep->parf + PARF_ATU_BASE_ADDR);
> > +
> > +	/* Read halts write */
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_AXI_MSTR_RD_HALT_NO_WRITES);
> > +	/* Write after write halt */
> > +	writel_relaxed(BIT(31), pcie_ep->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
> > +	/* Q2A flush disable */
> > +	writel_relaxed(0, pcie_ep->parf + PARF_Q2A_FLUSH);
> > +
> > +	/* Disable the DBI Wakeup */
> > +	writel_relaxed(BIT(11), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Disable the debouncers */
> > +	writel_relaxed(0x73, pcie_ep->parf + PARF_DB_CTRL);
> > +	/* Disable core clock CGC */
> > +	writel_relaxed(BIT(6), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Set AUX power to be on */
> > +	writel_relaxed(BIT(4), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Request to exit from L1SS for MSI and LTR MSG */
> > +	writel_relaxed(BIT(1), pcie_ep->parf + PARF_CFG_BITS);
> > +
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +
> > +	/* Set the PMC Register - to support PME in D0/D3hot/D3cold */
> > +	val = dw_pcie_readl_dbi(pci, DBI_CAP_ID_NXT_PTR);
> > +	val |= BIT(31) | BIT(30) | BIT(27);
> > +	dw_pcie_writel_dbi(pci, DBI_CAP_ID_NXT_PTR, val);
> > +
> > +	/* Set the Endpoint L0s Acceptable Latency to 1us (max) */
> > +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L0S_ACCPT_LATENCY_MASK, 0x7);
> > +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> > +
> > +	/* Set the Endpoint L1 Acceptable Latency to 1us (max) */
> > +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L1_ACCPT_LATENCY_MASK, 0x7);
> > +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> > +
> > +	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L0S_EXIT_LATENCY_MASK, 0x6);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L1_EXIT_LATENCY_MASK, 0x6);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	/* L1ss is supported */
> > +	val = dw_pcie_readl_dbi(pci, DBI_L1SUB_CAPABILITY);
> > +	val |= 0x1f;
> > +	dw_pcie_writel_dbi(pci, DBI_L1SUB_CAPABILITY, val);
> > +
> > +	/* Enable Clock Power Management */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= BIT(18);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> > +
> > +	/* Set FTS value to match the PHY setting */
> > +	val = dw_pcie_readl_dbi(pci, DBI_ACK_F_ASPM_CTRL);
> > +	val |= FIELD_PREP(DBI_ACK_N_FTS_MASK, 0x80);
> > +	dw_pcie_writel_dbi(pci, DBI_ACK_F_ASPM_CTRL, val);
> > +
> > +	dw_pcie_writel_dbi(pci, DBI_AUX_CLK_FREQ, 0x14);
> > +
> > +	/* Prevent L1ss wakeup after 100ms */
> > +	val = dw_pcie_readl_dbi(pci, DBI_GEN3_RELATED_OFF);
> > +	val &= ~BIT(0);
> > +	dw_pcie_writel_dbi(pci, DBI_GEN3_RELATED_OFF, val);
> > +
> > +	/* Disable SRIS_MODE */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_SRIS_MODE);
> > +	val &= ~BIT(0);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_SRIS_MODE);
> > +
> > +	qcom_pcie_ep_configure_irq(pcie_ep);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_confirm_linkup(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	u32 reg;
> > +
> > +	reg = readl_relaxed(pcie_ep->elbi + ELBI_SYS_STTS);
> > +
> > +	return reg & XMLH_LINK_UP;
> > +}
> > +
> > +static int qcom_pcie_start_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +
> > +	enable_irq(pcie_ep->perst_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_establish_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	struct device *dev = pci->dev;
> > +	int ret;
> > +
> > +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_ENABLED) {
> > +		dev_info(dev, "Link is already enabled\n");
> > +		return 0;
> > +	}
> > +
> > +	/* Enable power and clocks */
> > +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Perform controller reset */
> > +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Initialize PHY */
> > +	ret = phy_init(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Power ON PHY */
> > +	ret = phy_power_on(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> > +		goto err_phy_exit;
> > +	}
> > +
> > +	/* Assert WAKE# to RC to indicate device is ready */
> > +	gpiod_set_value_cansleep(pcie_ep->wake, 0);
> > +	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
> > +	gpiod_set_value_cansleep(pcie_ep->wake, 1);
> > +
> > +	/* Configure TCSR */
> > +	qcom_pcie_ep_configure_tcsr(pcie_ep);
> > +
> > +	/* Initialize the controller */
> > +	ret = qcom_pcie_ep_core_init(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to init controller: %d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
> > +
> > +	/* Enable LTSSM */
> > +	qcom_pcie_ep_enable_ltssm(pcie_ep);
> > +
> > +	return 0;
> > +/* TODO* assert reset? */
> > +err_phy_power_off:
> > +	phy_power_off(pcie_ep->phy);
> > +err_phy_exit:
> > +	phy_exit(pcie_ep->phy);
> > +err_disable_resources:
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_pcie_disable_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	struct device *dev = pci->dev;
> > +
> > +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > +		dev_info(dev, "Link is already disabled\n");
> > +		return;
> > +	}
> > +
> > +	phy_power_off(pcie_ep->phy);
> > +	phy_exit(pcie_ep->phy);
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> > +}
> > +
> > +/* Common DWC controller ops */
> > +static const struct dw_pcie_ops pci_ops = {
> > +	.link_up = qcom_pcie_confirm_linkup,
> > +	.start_link = qcom_pcie_start_link,
> > +	.stop_link = qcom_pcie_disable_link,
> > +};
> > +
> > +static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> > +					 struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct resource *res;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
> > +	pcie_ep->parf = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(pcie_ep->parf))
> > +		return PTR_ERR(pcie_ep->parf);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pci->dbi_base))
> > +		return PTR_ERR(pci->dbi_base);
> > +	pci->dbi_base2 = pci->dbi_base;
> > +	pcie_ep->dbi_phys = res->start;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
> > +	pcie_ep->elbi = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pcie_ep->elbi))
> > +		return PTR_ERR(pcie_ep->elbi);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > +	pci->atu_base = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pci->atu_base))
> > +		return PTR_ERR(pci->atu_base);
> > +	pcie_ep->atu_phys = res->start;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> > +	pcie_ep->tcsr = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(pcie_ep->tcsr))
> > +		return PTR_ERR(pcie_ep->tcsr);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> > +				      struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	ret = qcom_pcie_ep_get_io_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to get io resources %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Enable GDSC power domain */
> > +	ret = dev_pm_domain_attach(dev, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				qcom_pcie_ep_clks);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get clocks: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > +	if (IS_ERR(pcie_ep->core_reset))
> > +		return PTR_ERR(pcie_ep->core_reset);
> > +
> > +	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> > +	if (IS_ERR(pcie_ep->reset))
> > +		return PTR_ERR(pcie_ep->reset);
> > +
> > +	pcie_ep->wake = devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(pcie_ep->wake))
> > +		return PTR_ERR(pcie_ep->wake);
> > +
> > +	pcie_ep->phy = devm_phy_optional_get(&pdev->dev, "pciephy");
> > +	if (IS_ERR(pcie_ep->phy))
> > +		ret = PTR_ERR(pcie_ep->phy);
> > +
> > +	return ret;
> > +}
> > +
> > +/* TODO: Notify clients about PCIe state change */
> > +static irqreturn_t qcom_pcie_ep_global_threaded_irq(int irq, void *data)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = data;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 status = readl(pcie_ep->parf + PARF_INT_ALL_STATUS);
> > +	u32 mask = readl(pcie_ep->parf + PARF_INT_ALL_MASK);
> > +	u32 dstate, event, val;
> > +
> > +	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);
> > +	status &= mask;
> > +
> > +	for (event = 1; event < QCOM_PCIE_EP_INT_MAX; event++) {
> > +		if (status & BIT(event)) {
> > +			switch (event) {
> > +			case QCOM_PCIE_EP_INT_LINK_DOWN:
> > +				dev_info(dev, "Received Linkdown event\n");
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN;
> > +				break;
> > +			case QCOM_PCIE_EP_INT_BME:
> > +				dev_info(dev, "Received BME event. Link is enabled!\n");
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
> > +				break;
> > +			case QCOM_PCIE_EP_INT_PM_TURNOFF:
> > +				dev_info(dev, "Received PM Turn-off event! Entering L23\n");
> > +				val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +				val |= BIT(2);
> > +				writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +				break;
> > +			case QCOM_PCIE_EP_INT_DSTATE_CHANGE:
> > +				dstate = dw_pcie_readl_dbi(pci, DBI_CON_STATUS) & 0x3;
> > +				dev_info(dev, "Received D%d state event\n", dstate);
> > +				if (dstate == 3) {
> > +					val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +					val |= BIT(1);
> > +					writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +				}
> > +				/* Handle D0 state change */
> 
> I don't understand this comment.
> 

So this comment is intended as a TODO one. Currently only the D3 state is
handled by this driver and the endpoint function driver needs to handle the D0
state change accordingly (like wakeup etc...)

I have an idea in mind which I'll post for creating one more notifier between
endpoint controller and function driver for these kind of usecases.

> > +				break;
> > +			case QCOM_PCIE_EP_INT_LINK_UP:
> > +				dev_info(dev, "Received Linkup event. Enumeration complete!\n");
> > +				dw_pcie_ep_linkup(&pci->ep);
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_UP;
> > +				break;
> > +			default:
> > +				dev_err(dev, "Received unknown event: %d\n", event);
> 
> Are you sure that you want those dev_info() and dev_err() in irq
> hanldler even when it is threaded? I'd drop all dev_info|err() or make
> them dev_dbg() if they can help.
> 

Sorry, dev_info() calls are meant to be replaced by dev_dbg(). I'll do that.
Also the dev_err() here.

> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = data;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 perst;
> > +
> > +	perst = gpiod_get_value(pcie_ep->reset);
> > +
> > +	if (perst) {
> > +		/* Start link training */
> > +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> > +		qcom_pcie_establish_link(pci);
> > +	} else {
> > +		/* Shutdown the link if the link is already on */
> > +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> 
> Those prints sounds like a candidate for dev_dbg or even you can drop them.
> 
> > +		qcom_pcie_disable_link(pci);
> > +	}
> > +
> > +	/* Set trigger type based on the next expected value of perst gpio */
> 
> IMO commenting obvious code is not good practice.
> 

Okay, I'll remove it.

> > +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> > +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> > +					     struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	int irq, ret;
> > +
> > +	irq = platform_get_irq_byname(pdev, "global");
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> > +		return irq;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +					qcom_pcie_ep_global_threaded_irq,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					"global_irq", pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> > +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> > +	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> > +					qcom_pcie_ep_perst_threaded_irq,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					"perst_irq", pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request PERST IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > +				  enum pci_epc_irq_type type, u16 interrupt_num)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	switch (type) {
> > +	case PCI_EPC_IRQ_LEGACY:
> > +		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
> > +	case PCI_EPC_IRQ_MSI:
> > +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> > +	default:
> > +		dev_err(pci->dev, "Unknown IRQ type\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct pci_epc_features qcom_pcie_epc_features = {
> > +	.linkup_notifier = true,
> > +	.core_init_notifier = true,
> > +	.msi_capable = true,
> > +	.msix_capable = false,
> > +};
> > +
> > +static const struct pci_epc_features *
> > +qcom_pcie_epc_get_features(struct dw_pcie_ep *pci_ep)
> > +{
> > +	return &qcom_pcie_epc_features;
> > +}
> > +
> > +static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	enum pci_barno bar;
> > +
> > +	for (bar = BAR_0; bar <= BAR_5; bar++)
> > +		dw_pcie_ep_reset_bar(pci, bar);
> > +}
> > +
> > +static struct dw_pcie_ep_ops pci_ep_ops = {
> > +	.ep_init = qcom_pcie_ep_init,
> > +	.raise_irq = qcom_pcie_ep_raise_irq,
> > +	.get_features = qcom_pcie_epc_get_features,
> > +};
> > +
> > +static const struct of_device_id qcom_pcie_ep_match[] = {
> > +	{ .compatible = "qcom,sdx55-pcie-ep", },
> > +	{ }
> > +};
> > +
> > +static int qcom_pcie_ep_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct qcom_pcie_ep *pcie_ep;
> > +	int ret;
> > +
> > +	pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL);
> > +	if (!pcie_ep)
> > +		return -ENOMEM;
> > +
> > +	pcie_ep->pci.dev = dev;
> > +	pcie_ep->pci.ops = &pci_ops;
> > +	pcie_ep->pci.ep.ops = &pci_ep_ops;
> > +
> > +	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get resources:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Enable power and clocks */
> 
> Useless comment.
> 
> > +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Perform controller reset */
> 
> ditto
> 
> > +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> 
> qcom_pcie_ep_core_reset() already print errors
> 
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Initialize PHY */
> 
> ditto
> 
> > +	ret = phy_init(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> 
> phy_init() already print an error.
> 
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* PHY needs to be powered on for dw_pcie_ep_init() */
> 
> useless comment.
> 
> > +	ret = phy_power_on(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> 
> phy_power_on already has prints in case of an error
> 
> > +		goto err_phy_exit;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, pcie_ep);
> > +
> > +	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get IRQ resources %d\n", ret);
> 
> errors are printed already form qcom_pcie_ep_enable_irq_resources()
> 
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize endpoint:%d\n", ret);
> 
> I think this dev_err() should be dropped.
> 
> Please revisit all those dev_err and dev_info in the driver and if
> possible reduce thei
> 

Okay

Thanks,
Mani

> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_phy_power_off:
> > +	phy_power_off(pcie_ep->phy);
> > +err_phy_exit:
> > +	phy_exit(pcie_ep->phy);
> > +err_disable_resources:
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver qcom_pcie_ep_driver = {
> > +	.probe	= qcom_pcie_ep_probe,
> > +	.driver	= {
> > +		.name		= "qcom-pcie-ep",
> > +		.suppress_bind_attrs = true,
> > +		.of_match_table	= qcom_pcie_ep_match,
> > +	},
> > +};
> > +builtin_platform_driver(qcom_pcie_ep_driver);
> > +
> > +MODULE_AUTHOR("Siddartha Mohanadoss <smohanad@codeaurora.org>");
> > +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> > +MODULE_DESCRIPTION("Qualcomm PCIe Endpoint controller driver");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> -- 
> regards,
> Stan

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-06  3:07   ` Bjorn Andersson
@ 2021-06-09  8:51     ` Manivannan Sadhasivam
  2021-06-11  4:28       ` Bjorn Andersson
  2021-06-16 18:23       ` Manivannan Sadhasivam
  2021-06-15 21:40     ` Rob Herring
  1 sibling, 2 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-09  8:51 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: lorenzo.pieralisi, robh, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel, Siddartha Mohanadoss

On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> 
> > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > the Designware core with added Qualcomm specific wrapper around the
> > core. The driver support is very basic such that it supports only
> > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > for now but these will be added later.
> > 
> > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > operation and written on top of the DWC PCI framework.
> > 
> > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > [mani: restructured the driver and fixed several bugs for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Really nice to see this working!
> 
> > ---
> >  drivers/pci/controller/dwc/Kconfig        |  10 +
> >  drivers/pci/controller/dwc/Makefile       |   1 +
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
> >  3 files changed, 791 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 423d35872ce4..32e735b1fd85 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -180,6 +180,16 @@ config PCIE_QCOM
> >  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> >  	  hardware wrappers.
> >  
> > +config PCIE_QCOM_EP
> > +	bool "Qualcomm PCIe controller - Endpoint mode"
> > +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> > +	depends on PCI_ENDPOINT
> > +	select PCIE_DW_EP
> > +	help
> > +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> > +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> > +	  plus Qualcomm-specific hardware wrappers.
> > +
> >  config PCIE_ARMADA_8K
> >  	bool "Marvell Armada-8K PCIe controller"
> >  	depends on ARCH_MVEBU || COMPILE_TEST
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index eca805c1a023..abb27642d46b 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> >  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> >  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> >  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> >  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> >  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> >  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > new file mode 100644
> > index 000000000000..b68511bacc2a
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -0,0 +1,780 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Qualcomm PCIe Endpoint controller driver
> > + *
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + * Author: Siddartha Mohanadoss <smohanad@codeaurora.org
> > + *
> > + * Copyright (c) 2021, Linaro Ltd.
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/reset.h>
> > +#include <linux/delay.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/pm_domain.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +/* PARF registers */
> > +#define PARF_SYS_CTRL				0x00
> > +#define PARF_DB_CTRL				0x10
> > +#define PARF_PM_CTRL				0x20
> > +#define PARF_DEBUG_INT_EN			0x190
> > +#define PARF_AXI_MSTR_RD_HALT_NO_WRITES		0x1a4
> > +#define PARF_AXI_MSTR_WR_ADDR_HALT		0x1a8
> > +#define PARF_Q2A_FLUSH				0x1aC
> > +#define PARF_LTSSM				0x1b0
> > +#define PARF_CFG_BITS				0x210
> > +#define PARF_INT_ALL_STATUS			0x224
> > +#define PARF_INT_ALL_CLEAR			0x228
> > +#define PARF_INT_ALL_MASK			0x22c
> > +#define PARF_SLV_ADDR_MSB_CTRL			0x2c0
> > +#define PARF_DBI_BASE_ADDR			0x350
> > +#define PARF_DBI_BASE_ADDR_HI			0x354
> > +#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> > +#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35c
> > +#define PARF_ATU_BASE_ADDR			0x634
> > +#define PARF_ATU_BASE_ADDR_HI			0x638
> > +#define PARF_SRIS_MODE				0x644
> > +#define PARF_DEVICE_TYPE			0x1000
> > +#define PARF_BDF_TO_SID_CFG			0x2c00
> > +
> > +/* ELBI registers */
> > +#define ELBI_SYS_STTS				0x08
> > +
> > +/* DBI registers */
> > +#define DBI_CAP_ID_NXT_PTR			0x40
> > +#define DBI_CON_STATUS				0x44
> > +#define DBI_DEVICE_CAPABILITIES			0x74
> > +#define DBI_LINK_CAPABILITIES			0x7c
> > +#define DBI_LINK_CONTROL2_LINK_STATUS2		0xa0
> > +#define DBI_L1SUB_CAPABILITY			0x234
> > +#define DBI_ACK_F_ASPM_CTRL			0x70c
> > +#define DBI_GEN3_RELATED_OFF			0x890
> > +#define DBI_AUX_CLK_FREQ			0xb40
> > +
> > +#define DBI_L0S_ACCPT_LATENCY_MASK		GENMASK(8, 6)
> > +#define DBI_L1_ACCPT_LATENCY_MASK		GENMASK(11, 9)
> > +#define DBI_L0S_EXIT_LATENCY_MASK		GENMASK(14, 12)
> > +#define DBI_L1_EXIT_LATENCY_MASK		GENMASK(17, 15)
> > +#define DBI_ACK_N_FTS_MASK			GENMASK(15, 8)
> > +
> > +/* TCSR registers */
> > +#define TCSR_PCIE_PERST_EN			0x258
> > +#define TCSR_PERST_SEPARATION_ENABLE		0x270
> > +
> > +#define XMLH_LINK_UP				0x400
> > +#define CORE_RESET_TIME_US_MIN			1000
> > +#define CORE_RESET_TIME_US_MAX			1005
> > +#define WAKE_DELAY_US				2000 /* 2 ms */
> > +
> > +#define to_pcie_ep(x)				dev_get_drvdata((x)->dev)
> 
> Isn't this also container_of(x, struct qcom_pcie_ep, pci), but without
> the need to assigning and use drvdata?
> 

Okay

> > +
> > +enum qcom_pcie_ep_link_status {
> > +	QCOM_PCIE_EP_LINK_DISABLED,
> > +	QCOM_PCIE_EP_LINK_ENABLED,
> > +	QCOM_PCIE_EP_LINK_UP,
> > +	QCOM_PCIE_EP_LINK_DOWN,
> > +};
> > +
> > +enum qcom_pcie_ep_irq {
> > +	QCOM_PCIE_EP_INT_RESERVED,
> > +	QCOM_PCIE_EP_INT_LINK_DOWN,
> > +	QCOM_PCIE_EP_INT_BME,
> > +	QCOM_PCIE_EP_INT_PM_TURNOFF,
> > +	QCOM_PCIE_EP_INT_DEBUG,
> > +	QCOM_PCIE_EP_INT_LTR,
> > +	QCOM_PCIE_EP_INT_MHI_Q6,
> > +	QCOM_PCIE_EP_INT_MHI_A7,
> > +	QCOM_PCIE_EP_INT_DSTATE_CHANGE,
> > +	QCOM_PCIE_EP_INT_L1SUB_TIMEOUT,
> > +	QCOM_PCIE_EP_INT_MMIO_WRITE,
> > +	QCOM_PCIE_EP_INT_CFG_WRITE,
> > +	QCOM_PCIE_EP_INT_BRIDGE_FLUSH_N,
> > +	QCOM_PCIE_EP_INT_LINK_UP,
> > +	QCOM_PCIE_EP_INT_AER_LEGACY,
> > +	QCOM_PCIE_EP_INT_PLS_ERR,
> > +	QCOM_PCIE_EP_INT_PME_LEGACY,
> > +	QCOM_PCIE_EP_INT_PLS_PME,
> > +	QCOM_PCIE_EP_INT_MAX,
> 
> Afaict these aren't some logical sequence of numbers, they are bit
> offsets in the interrupt registers. Using #define would make this
> obvious.
> 

Yes but these are reused in the irqhandler for differentiating between the
events, so I thought it makes sense to use enum.

> > +};
> > +
> > +static struct clk_bulk_data qcom_pcie_ep_clks[] = {
> > +	{ .id = "cfg" },
> > +	{ .id = "aux" },
> > +	{ .id = "bus_master" },
> > +	{ .id = "bus_slave" },
> > +	{ .id = "ref" },
> > +	{ .id = "sleep" },
> > +	{ .id = "slave_q2a" },
> > +};
> > +
> > +struct qcom_pcie_ep {
> > +	struct dw_pcie pci;
> > +
> > +	void __iomem *parf;
> > +	void __iomem *elbi;
> > +	void __iomem *tcsr;
> > +
> > +	struct reset_control *core_reset;
> > +	struct gpio_desc *reset;
> > +	struct gpio_desc *wake;
> > +	struct phy *phy;
> > +
> > +	resource_size_t dbi_phys;
> > +	resource_size_t atu_phys;
> > +
> > +	enum qcom_pcie_ep_link_status link_status;
> > +	int global_irq;
> > +	int perst_irq;
> > +};
> > +
> > +static void qcom_pcie_ep_enable_ltssm(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	u32 reg;
> > +
> > +	reg = readl(pcie_ep->parf + PARF_LTSSM);
> > +	reg |= BIT(8);
> > +	writel_relaxed(reg, pcie_ep->parf + PARF_LTSSM);
> > +}
> > +
> > +static int qcom_pcie_ep_core_reset(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	int ret = 0;
> 
> No need to initialize ret, when the first access is an assignment.
> 
> > +
> > +	ret = reset_control_assert(pcie_ep->core_reset);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot assert core reset\n");
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> > +
> > +	ret = reset_control_deassert(pcie_ep->core_reset);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot de-assert core reset\n");
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(CORE_RESET_TIME_US_MIN, CORE_RESET_TIME_US_MAX);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Delatch PERST_EN and PERST_SEPARATION_ENABLE with TCSR to avoid
> > + * device reset during host reboot and hibernation. The driver is
> > + * expected to handle this situation.
> > + */
> > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> 
> Please avoid _relaxed accessor unless there's a strong reason, and if so
> document it.
> 

Okay

> > +	writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> > +}
> > +
> > +static int qcom_pcie_ep_enable_resources(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	int ret;
> > +
> > +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				      qcom_pcie_ep_clks);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clocks: %d\n", ret);
> 
> clk_bulk_prepare_enable() will print an error indicating which of the
> clocks it failed to prepare/enable, so it's better to skip this.
> 
> As such, you can simply do:
> 
> 	return clk_bulk_prepare_enable();
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void qcom_pcie_ep_disable_resources(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	clk_bulk_disable_unprepare(ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				   qcom_pcie_ep_clks);
> > +}
> > +
> > +static void qcom_pcie_ep_configure_irq(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	u32 val;
> > +
> > +	writel_relaxed(0, pcie_ep->parf + PARF_INT_ALL_MASK);
> > +	val = BIT(QCOM_PCIE_EP_INT_LINK_DOWN) |
> > +		BIT(QCOM_PCIE_EP_INT_BME) |
> > +		BIT(QCOM_PCIE_EP_INT_PM_TURNOFF) |
> > +		BIT(QCOM_PCIE_EP_INT_DSTATE_CHANGE) |
> > +		BIT(QCOM_PCIE_EP_INT_LINK_UP);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);
> 
> Again, please avoid the _relaxed versions.
> 
> > +}
> > +
> > +static int qcom_pcie_ep_core_init(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	u32 val;
> > +
> > +	/* Disable BDF to SID mapping */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> > +	val |= BIT(0);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_BDF_TO_SID_CFG);
> > +
> > +	/* enable debug IRQ */
> > +	writel_relaxed((BIT(3) | BIT(2) | BIT(1)),
> > +		       pcie_ep->parf + PARF_DEBUG_INT_EN);
> > +
> > +	/* Configure PCIe to endpoint mode */
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_DEVICE_TYPE);
> > +
> > +	/* Configure PCIe core to support 1GB aperture */
> > +	writel_relaxed(0x40000000, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +
> > +	/* Allow entering L1 state */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +	val &= ~BIT(5);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +
> > +	/* Configure Slave, DBI and iATU base addresses */
> > +	writel_relaxed(BIT(0), pcie_ep->parf + PARF_SLV_ADDR_MSB_CTRL);
> > +	writel_relaxed(0x200, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_SLV_ADDR_SPACE_SIZE);
> > +	writel_relaxed(0x100, pcie_ep->parf + PARF_DBI_BASE_ADDR_HI);
> > +	writel_relaxed(pcie_ep->dbi_phys, pcie_ep->parf + PARF_DBI_BASE_ADDR);
> > +	writel_relaxed(0x100, pcie_ep->parf + PARF_ATU_BASE_ADDR_HI);
> > +	writel_relaxed(pcie_ep->atu_phys, pcie_ep->parf + PARF_ATU_BASE_ADDR);
> > +
> > +	/* Read halts write */
> > +	writel_relaxed(0x0, pcie_ep->parf + PARF_AXI_MSTR_RD_HALT_NO_WRITES);
> > +	/* Write after write halt */
> > +	writel_relaxed(BIT(31), pcie_ep->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
> > +	/* Q2A flush disable */
> > +	writel_relaxed(0, pcie_ep->parf + PARF_Q2A_FLUSH);
> > +
> > +	/* Disable the DBI Wakeup */
> > +	writel_relaxed(BIT(11), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Disable the debouncers */
> > +	writel_relaxed(0x73, pcie_ep->parf + PARF_DB_CTRL);
> > +	/* Disable core clock CGC */
> > +	writel_relaxed(BIT(6), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Set AUX power to be on */
> > +	writel_relaxed(BIT(4), pcie_ep->parf + PARF_SYS_CTRL);
> > +	/* Request to exit from L1SS for MSI and LTR MSG */
> > +	writel_relaxed(BIT(1), pcie_ep->parf + PARF_CFG_BITS);
> > +
> > +	dw_pcie_dbi_ro_wr_en(pci);
> > +
> > +	/* Set the PMC Register - to support PME in D0/D3hot/D3cold */
> > +	val = dw_pcie_readl_dbi(pci, DBI_CAP_ID_NXT_PTR);
> > +	val |= BIT(31) | BIT(30) | BIT(27);
> > +	dw_pcie_writel_dbi(pci, DBI_CAP_ID_NXT_PTR, val);
> > +
> > +	/* Set the Endpoint L0s Acceptable Latency to 1us (max) */
> > +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L0S_ACCPT_LATENCY_MASK, 0x7);
> > +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> > +
> > +	/* Set the Endpoint L1 Acceptable Latency to 1us (max) */
> > +	val = dw_pcie_readl_dbi(pci, DBI_DEVICE_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L1_ACCPT_LATENCY_MASK, 0x7);
> > +	dw_pcie_writel_dbi(pci, DBI_DEVICE_CAPABILITIES, val);
> > +
> > +	/* Set the L0s Exit Latency to 2us-4us = 0x6 */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L0S_EXIT_LATENCY_MASK, 0x6);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	/* Set the L1 Exit Latency to be 32us-64 us = 0x6 */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= FIELD_PREP(DBI_L1_EXIT_LATENCY_MASK, 0x6);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	/* L1ss is supported */
> > +	val = dw_pcie_readl_dbi(pci, DBI_L1SUB_CAPABILITY);
> > +	val |= 0x1f;
> > +	dw_pcie_writel_dbi(pci, DBI_L1SUB_CAPABILITY, val);
> > +
> > +	/* Enable Clock Power Management */
> > +	val = dw_pcie_readl_dbi(pci, DBI_LINK_CAPABILITIES);
> > +	val |= BIT(18);
> > +	dw_pcie_writel_dbi(pci, DBI_LINK_CAPABILITIES, val);
> > +
> > +	dw_pcie_dbi_ro_wr_dis(pci);
> > +
> > +	/* Set FTS value to match the PHY setting */
> > +	val = dw_pcie_readl_dbi(pci, DBI_ACK_F_ASPM_CTRL);
> > +	val |= FIELD_PREP(DBI_ACK_N_FTS_MASK, 0x80);
> > +	dw_pcie_writel_dbi(pci, DBI_ACK_F_ASPM_CTRL, val);
> > +
> > +	dw_pcie_writel_dbi(pci, DBI_AUX_CLK_FREQ, 0x14);
> > +
> > +	/* Prevent L1ss wakeup after 100ms */
> > +	val = dw_pcie_readl_dbi(pci, DBI_GEN3_RELATED_OFF);
> > +	val &= ~BIT(0);
> > +	dw_pcie_writel_dbi(pci, DBI_GEN3_RELATED_OFF, val);
> > +
> > +	/* Disable SRIS_MODE */
> > +	val = readl_relaxed(pcie_ep->parf + PARF_SRIS_MODE);
> > +	val &= ~BIT(0);
> > +	writel_relaxed(val, pcie_ep->parf + PARF_SRIS_MODE);
> > +
> > +	qcom_pcie_ep_configure_irq(pcie_ep);
> 
> Why is the irq part broken out to its own function?
> 

Hmm, I removed one other use of this function. So will just use the code
directly.

> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_confirm_linkup(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	u32 reg;
> > +
> > +	reg = readl_relaxed(pcie_ep->elbi + ELBI_SYS_STTS);
> > +
> > +	return reg & XMLH_LINK_UP;
> > +}
> > +
> > +static int qcom_pcie_start_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +
> > +	enable_irq(pcie_ep->perst_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_establish_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	struct device *dev = pci->dev;
> > +	int ret;
> > +
> > +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_ENABLED) {
> > +		dev_info(dev, "Link is already enabled\n");
> 
> Per qcom_pcie_ep_global_threaded_irq() it seems possible that the link
> is enabled but link_status != QCOM_PCIE_EP_LINK_ENABLED.
> 
> I'm also not sure how you can end up here with link_status == ENABLED;
> it starts off DISABLED and expect the first thing to happen is "perst"
> asserting, which would end up here and after that the only way you get
> here again is if perst is asserted again - without first being
> deasserted. So perhaps this is part of your manual edge detector?
> 

Well, in the downstream this function is called from multiple places like SYSERR
but looking further reveals that this check is not relevant here. Will remove.

> Perhaps it's not needed if you actually use edge detection on your
> interrupt?
> 
> > +		return 0;
> > +	}
> > +
> > +	/* Enable power and clocks */
> > +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Perform controller reset */
> > +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Initialize PHY */
> > +	ret = phy_init(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Power ON PHY */
> > +	ret = phy_power_on(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> > +		goto err_phy_exit;
> > +	}
> > +
> > +	/* Assert WAKE# to RC to indicate device is ready */
> > +	gpiod_set_value_cansleep(pcie_ep->wake, 0);
> 
> Assert sounds like "activate" or "turn on", so I would prefer that you
> make this a logical 1 here and if the signal is inverted specify this
> when you're requesting the gpio.
> 

Okay

> > +	usleep_range(WAKE_DELAY_US, WAKE_DELAY_US + 500);
> > +	gpiod_set_value_cansleep(pcie_ep->wake, 1);
> > +
> > +	/* Configure TCSR */
> > +	qcom_pcie_ep_configure_tcsr(pcie_ep);
> > +
> > +	/* Initialize the controller */
> > +	ret = qcom_pcie_ep_core_init(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to init controller: %d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
> > +
> > +	/* Enable LTSSM */
> > +	qcom_pcie_ep_enable_ltssm(pcie_ep);
> > +
> > +	return 0;
> > +/* TODO* assert reset? */
> > +err_phy_power_off:
> > +	phy_power_off(pcie_ep->phy);
> > +err_phy_exit:
> > +	phy_exit(pcie_ep->phy);
> > +err_disable_resources:
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_pcie_disable_link(struct dw_pcie *pci)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
> > +	struct device *dev = pci->dev;
> > +
> > +	if (pcie_ep->link_status == QCOM_PCIE_EP_LINK_DISABLED) {
> > +		dev_info(dev, "Link is already disabled\n");
> 
> Why can disable_link happen both from the interrupt handler directly and
> from the dw_pcie_ops?
> 

Right, I think the PERST IRQ should be disabled here.

> > +		return;
> > +	}
> > +
> > +	phy_power_off(pcie_ep->phy);
> > +	phy_exit(pcie_ep->phy);
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> > +}
> > +
> > +/* Common DWC controller ops */
> > +static const struct dw_pcie_ops pci_ops = {
> > +	.link_up = qcom_pcie_confirm_linkup,
> > +	.start_link = qcom_pcie_start_link,
> > +	.stop_link = qcom_pcie_disable_link,
> > +};
> > +
> > +static int qcom_pcie_ep_get_io_resources(struct platform_device *pdev,
> > +					 struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct resource *res;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
> > +	pcie_ep->parf = devm_ioremap_resource(dev, res);
> 
> Better make this devm_platform_ioremap_resource_byname(), or someone
> will send a patch the minute this code lands.
> 
> > +	if (IS_ERR(pcie_ep->parf))
> > +		return PTR_ERR(pcie_ep->parf);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pci->dbi_base))
> > +		return PTR_ERR(pci->dbi_base);
> > +	pci->dbi_base2 = pci->dbi_base;
> > +	pcie_ep->dbi_phys = res->start;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
> > +	pcie_ep->elbi = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pcie_ep->elbi))
> > +		return PTR_ERR(pcie_ep->elbi);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> > +	pci->atu_base = devm_pci_remap_cfg_resource(dev, res);
> > +	if (IS_ERR(pci->atu_base))
> > +		return PTR_ERR(pci->atu_base);
> > +	pcie_ep->atu_phys = res->start;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr");
> > +	pcie_ep->tcsr = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(pcie_ep->tcsr))
> > +		return PTR_ERR(pcie_ep->tcsr);
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_ep_get_resources(struct platform_device *pdev,
> > +				      struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	ret = qcom_pcie_ep_get_io_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to get io resources %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Enable GDSC power domain */
> > +	ret = dev_pm_domain_attach(dev, true);
> 
> If you only have a single power-domain specified, then it would already
> be attached to "dev".
> 

Ah, right. Will remove this.

> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(qcom_pcie_ep_clks),
> > +				qcom_pcie_ep_clks);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get clocks: %d\n", ret);
> 
> Don't you already have a print in devm_clk_bulk_get()?
> 
> > +		return ret;
> > +	}
> > +
> > +	pcie_ep->core_reset = devm_reset_control_get_exclusive(dev, "core");
> > +	if (IS_ERR(pcie_ep->core_reset))
> > +		return PTR_ERR(pcie_ep->core_reset);
> > +
> > +	pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN);
> > +	if (IS_ERR(pcie_ep->reset))
> > +		return PTR_ERR(pcie_ep->reset);
> > +
> > +	pcie_ep->wake = devm_gpiod_get_optional(dev, "wake", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(pcie_ep->wake))
> > +		return PTR_ERR(pcie_ep->wake);
> > +
> > +	pcie_ep->phy = devm_phy_optional_get(&pdev->dev, "pciephy");
> > +	if (IS_ERR(pcie_ep->phy))
> > +		ret = PTR_ERR(pcie_ep->phy);
> > +
> > +	return ret;
> > +}
> > +
> > +/* TODO: Notify clients about PCIe state change */
> > +static irqreturn_t qcom_pcie_ep_global_threaded_irq(int irq, void *data)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = data;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 status = readl(pcie_ep->parf + PARF_INT_ALL_STATUS);
> > +	u32 mask = readl(pcie_ep->parf + PARF_INT_ALL_MASK);
> > +	u32 dstate, event, val;
> > +
> > +	writel_relaxed(status, pcie_ep->parf + PARF_INT_ALL_CLEAR);
> > +	status &= mask;
> > +
> > +	for (event = 1; event < QCOM_PCIE_EP_INT_MAX; event++) {
> > +		if (status & BIT(event)) {
> 
> for_each_set_bit()
> 

ack

> > +			switch (event) {
> > +			case QCOM_PCIE_EP_INT_LINK_DOWN:
> > +				dev_info(dev, "Received Linkdown event\n");
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN;
> > +				break;
> > +			case QCOM_PCIE_EP_INT_BME:
> > +				dev_info(dev, "Received BME event. Link is enabled!\n");
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED;
> > +				break;
> > +			case QCOM_PCIE_EP_INT_PM_TURNOFF:
> > +				dev_info(dev, "Received PM Turn-off event! Entering L23\n");
> > +				val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +				val |= BIT(2);
> > +				writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +				break;
> > +			case QCOM_PCIE_EP_INT_DSTATE_CHANGE:
> > +				dstate = dw_pcie_readl_dbi(pci, DBI_CON_STATUS) & 0x3;
> > +				dev_info(dev, "Received D%d state event\n", dstate);
> > +				if (dstate == 3) {
> > +					val = readl_relaxed(pcie_ep->parf + PARF_PM_CTRL);
> > +					val |= BIT(1);
> > +					writel_relaxed(val, pcie_ep->parf + PARF_PM_CTRL);
> > +				}
> > +				/* Handle D0 state change */
> > +				break;
> > +			case QCOM_PCIE_EP_INT_LINK_UP:
> > +				dev_info(dev, "Received Linkup event. Enumeration complete!\n");
> > +				dw_pcie_ep_linkup(&pci->ep);
> > +				pcie_ep->link_status = QCOM_PCIE_EP_LINK_UP;
> > +				break;
> > +			default:
> > +				dev_err(dev, "Received unknown event: %d\n", event);
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> > +{
> > +	struct qcom_pcie_ep *pcie_ep = data;
> > +	struct dw_pcie *pci = &pcie_ep->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 perst;
> > +
> > +	perst = gpiod_get_value(pcie_ep->reset);
> > +
> > +	if (perst) {
> > +		/* Start link training */
> > +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> > +		qcom_pcie_establish_link(pci);
> > +	} else {
> > +		/* Shutdown the link if the link is already on */
> > +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> > +		qcom_pcie_disable_link(pci);
> > +	}
> > +
> > +	/* Set trigger type based on the next expected value of perst gpio */
> > +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> > +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> 
> Looks like you're manually implementing edge triggering, is there any
> reason for that? EDGE_BOTH seems to do the same thing...
> 

PERST is a level based signal, so I don't think we can use EDGE_BOTH here.

> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> > +					     struct qcom_pcie_ep *pcie_ep)
> > +{
> > +	int irq, ret;
> > +
> > +	irq = platform_get_irq_byname(pdev, "global");
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> > +		return irq;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +					qcom_pcie_ep_global_threaded_irq,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> 
> Leave out the trigger and rely on DT.
> 

Okay

> > +					"global_irq", pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> > +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> 
> Is the global interrupt needed for dw_pcie_ep_init()? Or could this
> simply be done when things are ready to handle the interrupts?
> 

No it is not needed. I can move this after dw_pcie_ep_init().

> > +	ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL,
> > +					qcom_pcie_ep_perst_threaded_irq,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					"perst_irq", pcie_ep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request PERST IRQ\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > +				  enum pci_epc_irq_type type, u16 interrupt_num)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > +	switch (type) {
> > +	case PCI_EPC_IRQ_LEGACY:
> > +		return dw_pcie_ep_raise_legacy_irq(ep, func_no);
> > +	case PCI_EPC_IRQ_MSI:
> > +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> > +	default:
> > +		dev_err(pci->dev, "Unknown IRQ type\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct pci_epc_features qcom_pcie_epc_features = {
> > +	.linkup_notifier = true,
> > +	.core_init_notifier = true,
> > +	.msi_capable = true,
> > +	.msix_capable = false,
> > +};
> > +
> > +static const struct pci_epc_features *
> > +qcom_pcie_epc_get_features(struct dw_pcie_ep *pci_ep)
> > +{
> > +	return &qcom_pcie_epc_features;
> > +}
> > +
> > +static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	enum pci_barno bar;
> > +
> > +	for (bar = BAR_0; bar <= BAR_5; bar++)
> > +		dw_pcie_ep_reset_bar(pci, bar);
> > +}
> > +
> > +static struct dw_pcie_ep_ops pci_ep_ops = {
> > +	.ep_init = qcom_pcie_ep_init,
> > +	.raise_irq = qcom_pcie_ep_raise_irq,
> > +	.get_features = qcom_pcie_epc_get_features,
> > +};
> > +
> > +static const struct of_device_id qcom_pcie_ep_match[] = {
> > +	{ .compatible = "qcom,sdx55-pcie-ep", },
> > +	{ }
> > +};
> 
> Move this below the probe function.
> 
> > +
> > +static int qcom_pcie_ep_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct qcom_pcie_ep *pcie_ep;
> > +	int ret;
> > +
> > +	pcie_ep = devm_kzalloc(dev, sizeof(*pcie_ep), GFP_KERNEL);
> > +	if (!pcie_ep)
> > +		return -ENOMEM;
> > +
> > +	pcie_ep->pci.dev = dev;
> > +	pcie_ep->pci.ops = &pci_ops;
> > +	pcie_ep->pci.ep.ops = &pci_ep_ops;
> > +
> > +	ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get resources:%d\n", ret);
> 
> You printed an error in about half the error cases in
> qcom_pcie_ep_get_resources(), it would be better to ensure you print a
> specific error for each of the remaining cases there and "silently"
> return here.
> 
> Same comment for below cases.
> 

Okay

> > +		return ret;
> > +	}
> > +
> > +	/* Enable power and clocks */
> > +	ret = qcom_pcie_ep_enable_resources(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Perform controller reset */
> > +	ret = qcom_pcie_ep_core_reset(pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to reset the core: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* Initialize PHY */
> > +	ret = phy_init(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize PHY: %d\n", ret);
> > +		goto err_disable_resources;
> > +	}
> > +
> > +	/* PHY needs to be powered on for dw_pcie_ep_init() */
> > +	ret = phy_power_on(pcie_ep->phy);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to Power ON PHY: %d\n", ret);
> > +		goto err_phy_exit;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, pcie_ep);
> > +
> > +	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get IRQ resources %d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize endpoint:%d\n", ret);
> > +		goto err_phy_power_off;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_phy_power_off:
> 
> Your interrupts remains enabled until devres takes them down after
> returning, so I suspect you need to disable them here as well - at least
> before shutting down the clocks.
> 

Okay

> > +	phy_power_off(pcie_ep->phy);
> > +err_phy_exit:
> > +	phy_exit(pcie_ep->phy);
> > +err_disable_resources:
> > +	qcom_pcie_ep_disable_resources(pcie_ep);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver qcom_pcie_ep_driver = {
> > +	.probe	= qcom_pcie_ep_probe,
> > +	.driver	= {
> > +		.name		= "qcom-pcie-ep",
> 
> Skip the indentation of the '='.
> 
> > +		.suppress_bind_attrs = true,
> 
> Why do we suppress_bind_attrs?
> 

This driver doesn't support remove() callback and I don't think it is necessary
for this platform driver. So this flag is here to prevent unbind from sysfs.

Thanks,
Mani

> Regards,
> Bjorn
> 
> > +		.of_match_table	= qcom_pcie_ep_match,
> > +	},
> > +};
> > +builtin_platform_driver(qcom_pcie_ep_driver);
> > +
> > +MODULE_AUTHOR("Siddartha Mohanadoss <smohanad@codeaurora.org>");
> > +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
> > +MODULE_DESCRIPTION("Qualcomm PCIe Endpoint controller driver");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.25.1
> > 

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

* Re: [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller
  2021-06-06  3:13   ` Bjorn Andersson
@ 2021-06-10 21:46     ` Rob Herring
  0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2021-06-10 21:46 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Manivannan Sadhasivam, lorenzo.pieralisi, bhelgaas,
	linux-arm-msm, linux-pci, devicetree, linux-kernel

On Sat, Jun 05, 2021 at 10:13:57PM -0500, Bjorn Andersson wrote:
> On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> 
> > Add devicetree binding for Qualcomm PCIe EP controller used in platforms
> > like SDX55. The EP controller is based on the Designware core with
> > Qualcomm specific wrappers.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++
> >  1 file changed, 144 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > new file mode 100644
> > index 000000000000..3e357cb03a5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > @@ -0,0 +1,144 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm PCIe Endpoint Controller binding
> > +
> > +maintainers:
> > +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +
> > +allOf:
> > +  - $ref: "pci-ep.yaml#"
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,sdx55-pcie-ep
> 
> The binding looks good, but this is going to cause us an inevitable
> warning as we'd have to describe the controller twice (rc + ep) in the
> sdx55.dtsi.
> 
> @Rob, what do you suggest we do about this, because it's the same
> problem currently responsible for hundreds of warnings in the case of
> GENI (where each node is duplicated for different functions).

What determines the mode? Assuming it is fixed for a platform, can't you 
just have 2 .dtsi files and include the right one. The SoC file could 
have the common h/w specific parts (clks, resets, etc.) as those 
shouldn't really be different depending on the mode. IIRC, some PCI 
bindings do this by design (meaning there's only one definition).

Rob

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-09  8:51     ` Manivannan Sadhasivam
@ 2021-06-11  4:28       ` Bjorn Andersson
  2021-06-11  4:44         ` Manivannan Sadhasivam
  2021-06-16 18:23       ` Manivannan Sadhasivam
  1 sibling, 1 reply; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-11  4:28 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: lorenzo.pieralisi, robh, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel, Siddartha Mohanadoss

On Wed 09 Jun 03:51 CDT 2021, Manivannan Sadhasivam wrote:
> On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
[..]
> > > +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> > > +{
> > > +	struct qcom_pcie_ep *pcie_ep = data;
> > > +	struct dw_pcie *pci = &pcie_ep->pci;
> > > +	struct device *dev = pci->dev;
> > > +	u32 perst;
> > > +
> > > +	perst = gpiod_get_value(pcie_ep->reset);
> > > +
> > > +	if (perst) {
> > > +		/* Start link training */
> > > +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> > > +		qcom_pcie_establish_link(pci);
> > > +	} else {
> > > +		/* Shutdown the link if the link is already on */
> > > +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> > > +		qcom_pcie_disable_link(pci);
> > > +	}
> > > +
> > > +	/* Set trigger type based on the next expected value of perst gpio */
> > > +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> > > +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> > 
> > Looks like you're manually implementing edge triggering, is there any
> > reason for that? EDGE_BOTH seems to do the same thing...
> > 
> 
> PERST is a level based signal, so I don't think we can use EDGE_BOTH here.
> 

Afaict it's just a gpio and you define if the hardware should fire of
interrupts given its level or if it should detect transitions.

That said, if the gpio is already high when registering the irq handler
there's no transition.

> > > +
> > > +	return IRQ_HANDLED;
> > > +}
[..]
> > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > +	.probe	= qcom_pcie_ep_probe,
> > > +	.driver	= {
> > > +		.name		= "qcom-pcie-ep",
> > 
> > Skip the indentation of the '='.
> > 
> > > +		.suppress_bind_attrs = true,
> > 
> > Why do we suppress_bind_attrs?
> > 
> 
> This driver doesn't support remove() callback and I don't think it is necessary
> for this platform driver. So this flag is here to prevent unbind from sysfs.
> 

Right, that part makes sense. But do you know why this is, why it's not
possible to have the PCI controller built as a module? (GKI should
want this).

Regards,
Bjorn

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-11  4:28       ` Bjorn Andersson
@ 2021-06-11  4:44         ` Manivannan Sadhasivam
  0 siblings, 0 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-11  4:44 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: lorenzo.pieralisi, robh, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel, Siddartha Mohanadoss

On Thu, Jun 10, 2021 at 11:28:49PM -0500, Bjorn Andersson wrote:
> On Wed 09 Jun 03:51 CDT 2021, Manivannan Sadhasivam wrote:
> > On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> > > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> [..]
> > > > +static irqreturn_t qcom_pcie_ep_perst_threaded_irq(int irq, void *data)
> > > > +{
> > > > +	struct qcom_pcie_ep *pcie_ep = data;
> > > > +	struct dw_pcie *pci = &pcie_ep->pci;
> > > > +	struct device *dev = pci->dev;
> > > > +	u32 perst;
> > > > +
> > > > +	perst = gpiod_get_value(pcie_ep->reset);
> > > > +
> > > > +	if (perst) {
> > > > +		/* Start link training */
> > > > +		dev_info(dev, "PERST de-asserted by host. Starting link training!\n");
> > > > +		qcom_pcie_establish_link(pci);
> > > > +	} else {
> > > > +		/* Shutdown the link if the link is already on */
> > > > +		dev_info(dev, "PERST asserted by host. Shutting down the PCIe link!\n");
> > > > +		qcom_pcie_disable_link(pci);
> > > > +	}
> > > > +
> > > > +	/* Set trigger type based on the next expected value of perst gpio */
> > > > +	irq_set_irq_type(gpiod_to_irq(pcie_ep->reset),
> > > > +			 (perst ? IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH));
> > > 
> > > Looks like you're manually implementing edge triggering, is there any
> > > reason for that? EDGE_BOTH seems to do the same thing...
> > > 
> > 
> > PERST is a level based signal, so I don't think we can use EDGE_BOTH here.
> > 
> 
> Afaict it's just a gpio and you define if the hardware should fire of
> interrupts given its level or if it should detect transitions.
> 
> That said, if the gpio is already high when registering the irq handler
> there's no transition.
> 

Right, that's one of the issue with edge triggering. PERST# can be deasserted by
the host before EP driver probes. So if we wait for edge transition then we'll
be stuck.

> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> [..]
> > > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > > +	.probe	= qcom_pcie_ep_probe,
> > > > +	.driver	= {
> > > > +		.name		= "qcom-pcie-ep",
> > > 
> > > Skip the indentation of the '='.
> > > 
> > > > +		.suppress_bind_attrs = true,
> > > 
> > > Why do we suppress_bind_attrs?
> > > 
> > 
> > This driver doesn't support remove() callback and I don't think it is necessary
> > for this platform driver. So this flag is here to prevent unbind from sysfs.
> > 
> 
> Right, that part makes sense. But do you know why this is, why it's not
> possible to have the PCI controller built as a module? (GKI should
> want this).
> 

For an endpoint, making this driver built-in makes sense since this forms the
basic functionality of the device and we do want it to probe asap (without
initramfs dance). But looking at other drivers, most of them (including Qcom RC)
doesn't support tristate.

But for the GKI requirement, I can add it.

Thanks,
Mani

> Regards,
> Bjorn

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-06  3:07   ` Bjorn Andersson
  2021-06-09  8:51     ` Manivannan Sadhasivam
@ 2021-06-15 21:40     ` Rob Herring
  2021-06-15 21:57       ` Bjorn Andersson
  1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2021-06-15 21:40 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Manivannan Sadhasivam, Lorenzo Pieralisi, Bjorn Helgaas,
	linux-arm-msm, PCI, devicetree, linux-kernel,
	Siddartha Mohanadoss

On Sat, Jun 5, 2021 at 9:07 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
>
> > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > the Designware core with added Qualcomm specific wrapper around the
> > core. The driver support is very basic such that it supports only
> > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > for now but these will be added later.
> >
> > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > operation and written on top of the DWC PCI framework.
> >
> > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > [mani: restructured the driver and fixed several bugs for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Really nice to see this working!

[...]

> > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > +{
> > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
>
> Please avoid _relaxed accessor unless there's a strong reason, and if so
> document it.

Uhhh, what!? That's the wrong way around from what I've ever seen
anyone say. Have you ever looked at the resulting code on arm32 with
OMAP enabled? It's just a memory barrier and an indirect function call
on every access.

Use readl/writel if you have an ordering requirement WRT DMA,
otherwise use relaxed variants.

> > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> > +}
> > +

[...]

> > +static struct platform_driver qcom_pcie_ep_driver = {
> > +     .probe  = qcom_pcie_ep_probe,
> > +     .driver = {
> > +             .name           = "qcom-pcie-ep",
>
> Skip the indentation of the '='.
>
> > +             .suppress_bind_attrs = true,
>
> Why do we suppress_bind_attrs?

Because remove is not handled.

Rob

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-15 21:40     ` Rob Herring
@ 2021-06-15 21:57       ` Bjorn Andersson
  2021-06-15 22:20         ` Rob Herring
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-15 21:57 UTC (permalink / raw)
  To: Rob Herring
  Cc: Manivannan Sadhasivam, Lorenzo Pieralisi, Bjorn Helgaas,
	linux-arm-msm, PCI, devicetree, linux-kernel,
	Siddartha Mohanadoss

On Tue 15 Jun 16:40 CDT 2021, Rob Herring wrote:

> On Sat, Jun 5, 2021 at 9:07 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> >
> > > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > > the Designware core with added Qualcomm specific wrapper around the
> > > core. The driver support is very basic such that it supports only
> > > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > > for now but these will be added later.
> > >
> > > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > > operation and written on top of the DWC PCI framework.
> > >
> > > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > [mani: restructured the driver and fixed several bugs for upstream]
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
> > Really nice to see this working!
> 
> [...]
> 
> > > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > > +{
> > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> >
> > Please avoid _relaxed accessor unless there's a strong reason, and if so
> > document it.
> 
> Uhhh, what!? That's the wrong way around from what I've ever seen
> anyone say. Have you ever looked at the resulting code on arm32 with
> OMAP enabled? It's just a memory barrier and an indirect function call
> on every access.
> 
> Use readl/writel if you have an ordering requirement WRT DMA,
> otherwise use relaxed variants.
> 

That does make sense. Unfortunately I don't know where this started, but
I would guess it might have been a reaction to the fact that people
where just sprinkling wmb() all over the place to be on the safe side...

> > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> > > +}
> > > +
> 
> [...]
> 
> > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > +     .probe  = qcom_pcie_ep_probe,
> > > +     .driver = {
> > > +             .name           = "qcom-pcie-ep",
> >
> > Skip the indentation of the '='.
> >
> > > +             .suppress_bind_attrs = true,
> >
> > Why do we suppress_bind_attrs?
> 
> Because remove is not handled.
> 

Not handled in Mani's driver, or is this a PCI thing?

Regards,
Bjorn

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-15 21:57       ` Bjorn Andersson
@ 2021-06-15 22:20         ` Rob Herring
  2021-06-15 22:58           ` Bjorn Andersson
  0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2021-06-15 22:20 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Manivannan Sadhasivam, Lorenzo Pieralisi, Bjorn Helgaas,
	linux-arm-msm, PCI, devicetree, linux-kernel,
	Siddartha Mohanadoss

 On Tue, Jun 15, 2021 at 3:57 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 15 Jun 16:40 CDT 2021, Rob Herring wrote:
>
> > On Sat, Jun 5, 2021 at 9:07 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > >
> > > > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > > > the Designware core with added Qualcomm specific wrapper around the
> > > > core. The driver support is very basic such that it supports only
> > > > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > > > for now but these will be added later.
> > > >
> > > > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > > > operation and written on top of the DWC PCI framework.
> > > >
> > > > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > [mani: restructured the driver and fixed several bugs for upstream]
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> > > Really nice to see this working!
> >
> > [...]
> >
> > > > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > > > +{
> > > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> > >
> > > Please avoid _relaxed accessor unless there's a strong reason, and if so
> > > document it.
> >
> > Uhhh, what!? That's the wrong way around from what I've ever seen
> > anyone say. Have you ever looked at the resulting code on arm32 with
> > OMAP enabled? It's just a memory barrier and an indirect function call
> > on every access.
> >
> > Use readl/writel if you have an ordering requirement WRT DMA,
> > otherwise use relaxed variants.
> >
>
> That does make sense. Unfortunately I don't know where this started, but
> I would guess it might have been a reaction to the fact that people
> where just sprinkling wmb() all over the place to be on the safe side...

I think you could write a book on it. In the beginning, there was x86
and it was strongly ordered...

>
> > > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> > > > +}
> > > > +
> >
> > [...]
> >
> > > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > > +     .probe  = qcom_pcie_ep_probe,
> > > > +     .driver = {
> > > > +             .name           = "qcom-pcie-ep",
> > >
> > > Skip the indentation of the '='.
> > >
> > > > +             .suppress_bind_attrs = true,
> > >
> > > Why do we suppress_bind_attrs?
> >
> > Because remove is not handled.
> >
>
> Not handled in Mani's driver, or is this a PCI thing?

Only a PCI thing in the sense all the drivers happen to copy-n-paste
it and are mostly built-in. The Android modules thing doesn't seem to
have quite hit PCI yet. On the flipside, I'm sure there's lots of
drivers we can unbind and fun will ensue.

There is some work needed as the remove() implementations that we do
have are all unique (such as do we need a lock or not). I keep nudging
people to look into it.

Rob

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-15 22:20         ` Rob Herring
@ 2021-06-15 22:58           ` Bjorn Andersson
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-15 22:58 UTC (permalink / raw)
  To: Rob Herring
  Cc: Manivannan Sadhasivam, Lorenzo Pieralisi, Bjorn Helgaas,
	linux-arm-msm, PCI, devicetree, linux-kernel,
	Siddartha Mohanadoss

On Tue 15 Jun 17:20 CDT 2021, Rob Herring wrote:

>  On Tue, Jun 15, 2021 at 3:57 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Tue 15 Jun 16:40 CDT 2021, Rob Herring wrote:
> >
> > > On Sat, Jun 5, 2021 at 9:07 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > > >
> > > > > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > > > > the Designware core with added Qualcomm specific wrapper around the
> > > > > core. The driver support is very basic such that it supports only
> > > > > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > > > > for now but these will be added later.
> > > > >
> > > > > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > > > > operation and written on top of the DWC PCI framework.
> > > > >
> > > > > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > > [mani: restructured the driver and fixed several bugs for upstream]
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > >
> > > > Really nice to see this working!
> > >
> > > [...]
> > >
> > > > > +static void qcom_pcie_ep_configure_tcsr(struct qcom_pcie_ep *pcie_ep)
> > > > > +{
> > > > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PCIE_PERST_EN);
> > > >
> > > > Please avoid _relaxed accessor unless there's a strong reason, and if so
> > > > document it.
> > >
> > > Uhhh, what!? That's the wrong way around from what I've ever seen
> > > anyone say. Have you ever looked at the resulting code on arm32 with
> > > OMAP enabled? It's just a memory barrier and an indirect function call
> > > on every access.
> > >
> > > Use readl/writel if you have an ordering requirement WRT DMA,
> > > otherwise use relaxed variants.
> > >
> >
> > That does make sense. Unfortunately I don't know where this started, but
> > I would guess it might have been a reaction to the fact that people
> > where just sprinkling wmb() all over the place to be on the safe side...
> 
> I think you could write a book on it. In the beginning, there was x86
> and it was strongly ordered...
> 

I guess it would allow me to ask people to RTFM (RTFB) instead then :)

Jokes aside, I think we came to the conclusion that writel() was better
than incorrect use of writel_relaxed() followed by wmb(). And in this
particular case it's definitely not happening in a hot code path...

> >
> > > > > +     writel_relaxed(0x0, pcie_ep->tcsr + TCSR_PERST_SEPARATION_ENABLE);
> > > > > +}
> > > > > +
> > >
> > > [...]
> > >
> > > > > +static struct platform_driver qcom_pcie_ep_driver = {
> > > > > +     .probe  = qcom_pcie_ep_probe,
> > > > > +     .driver = {
> > > > > +             .name           = "qcom-pcie-ep",
> > > >
> > > > Skip the indentation of the '='.
> > > >
> > > > > +             .suppress_bind_attrs = true,
> > > >
> > > > Why do we suppress_bind_attrs?
> > >
> > > Because remove is not handled.
> > >
> >
> > Not handled in Mani's driver, or is this a PCI thing?
> 
> Only a PCI thing in the sense all the drivers happen to copy-n-paste
> it and are mostly built-in. The Android modules thing doesn't seem to
> have quite hit PCI yet. On the flipside, I'm sure there's lots of
> drivers we can unbind and fun will ensue.
> 
> There is some work needed as the remove() implementations that we do
> have are all unique (such as do we need a lock or not). I keep nudging
> people to look into it.
> 

Thanks, that confirms that my expectation. I would prefer to see this
tackled in a separate step then :)

Regards,
Bjorn

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

* Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver
  2021-06-09  8:51     ` Manivannan Sadhasivam
  2021-06-11  4:28       ` Bjorn Andersson
@ 2021-06-16 18:23       ` Manivannan Sadhasivam
  1 sibling, 0 replies; 17+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-16 18:23 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: lorenzo.pieralisi, robh, bhelgaas, linux-arm-msm, linux-pci,
	devicetree, linux-kernel, Siddartha Mohanadoss

On Wed, Jun 09, 2021 at 02:22:00PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > 
> > > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > > the Designware core with added Qualcomm specific wrapper around the
> > > core. The driver support is very basic such that it supports only
> > > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > > for now but these will be added later.
> > > 
> > > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > > operation and written on top of the DWC PCI framework.
> > > 
> > > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > [mani: restructured the driver and fixed several bugs for upstream]
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Really nice to see this working!
> > 
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig        |  10 +
> > >  drivers/pci/controller/dwc/Makefile       |   1 +
> > >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
> > >  3 files changed, 791 insertions(+)
> > >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 423d35872ce4..32e735b1fd85 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -180,6 +180,16 @@ config PCIE_QCOM
> > >  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> > >  	  hardware wrappers.
> > >  
> > > +config PCIE_QCOM_EP
> > > +	bool "Qualcomm PCIe controller - Endpoint mode"
> > > +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> > > +	depends on PCI_ENDPOINT
> > > +	select PCIE_DW_EP
> > > +	help
> > > +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> > > +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> > > +	  plus Qualcomm-specific hardware wrappers.
> > > +
> > >  config PCIE_ARMADA_8K
> > >  	bool "Marvell Armada-8K PCIe controller"
> > >  	depends on ARCH_MVEBU || COMPILE_TEST
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index eca805c1a023..abb27642d46b 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> > >  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> > >  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > >  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> > >  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > >  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > >  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > new file mode 100644
> > > index 000000000000..b68511bacc2a
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > @@ -0,0 +1,780 @@

[...]

> > > +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> > > +					     struct qcom_pcie_ep *pcie_ep)
> > > +{
> > > +	int irq, ret;
> > > +
> > > +	irq = platform_get_irq_byname(pdev, "global");
> > > +	if (irq < 0) {
> > > +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> > > +		return irq;
> > > +	}
> > > +
> > > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > > +					qcom_pcie_ep_global_threaded_irq,
> > > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > 
> > Leave out the trigger and rely on DT.
> > 
> 
> Okay
> 
> > > +					"global_irq", pcie_ep);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> > > +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> > 
> > Is the global interrupt needed for dw_pcie_ep_init()? Or could this
> > simply be done when things are ready to handle the interrupts?
> > 
> 
> No it is not needed. I can move this after dw_pcie_ep_init().
> 

I don't know why but I'm seeing issues when this gets called after
dw_pcie_ep_init(). So I'm keeping this function as it is for now.

Thanks,
Mani

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

end of thread, other threads:[~2021-06-16 18:23 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-03 10:38 [PATCH v2 0/3] Add Qualcomm PCIe Endpoint driver support Manivannan Sadhasivam
2021-06-03 10:38 ` [PATCH v2 1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller Manivannan Sadhasivam
2021-06-06  3:13   ` Bjorn Andersson
2021-06-10 21:46     ` Rob Herring
2021-06-03 10:38 ` [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver Manivannan Sadhasivam
2021-06-05 22:02   ` Stanimir Varbanov
2021-06-09  5:20     ` Manivannan Sadhasivam
2021-06-06  3:07   ` Bjorn Andersson
2021-06-09  8:51     ` Manivannan Sadhasivam
2021-06-11  4:28       ` Bjorn Andersson
2021-06-11  4:44         ` Manivannan Sadhasivam
2021-06-16 18:23       ` Manivannan Sadhasivam
2021-06-15 21:40     ` Rob Herring
2021-06-15 21:57       ` Bjorn Andersson
2021-06-15 22:20         ` Rob Herring
2021-06-15 22:58           ` Bjorn Andersson
2021-06-03 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add entry for Qualcomm PCIe Endpoint driver and binding Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).