[PATCH 0/5] Add STM32MP25 PCIe drivers

[PATCH 0/5] Add STM32MP25 PCIe drivers

[Christian Bruel]

This patch series adds PCIe drivers STM32MP25 SoC from STMicrolectronics
and respective yaml schema for the root complex and device modes.

Christian Bruel (5):
  dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings
  PCI: stm32: Add PCIe host support for STM32MP25
  dt-bindings: PCI: Add STM32MP25 PCIe endpoint bindings
  PCI: stm32: Add PCIe endpoint support for STM32MP25
  MAINTAINERS: add entry for ST STM32MP25 PCIe drivers

 .../bindings/pci/st,stm32-pcie-ep.yaml        |  97 ++++
 .../bindings/pci/st,stm32-pcie-host.yaml      | 149 ++++++
 MAINTAINERS                                   |   7 +
 drivers/pci/controller/dwc/Kconfig            |  23 +
 drivers/pci/controller/dwc/Makefile           |   2 +
 drivers/pci/controller/dwc/pcie-stm32-ep.c    | 433 +++++++++++++++
 drivers/pci/controller/dwc/pcie-stm32.c       | 493 ++++++++++++++++++
 drivers/pci/controller/dwc/pcie-stm32.h       |  24 +
 8 files changed, 1228 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32-ep.c
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32.c
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32.h

-- 
2.34.1

[PATCH 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Christian Bruel]

Document the bindings for STM32MP25 PCIe Controller configured in
root complex mode.
Supports 4 legacy interrupts and MSI interrupts from the ARM
GICv2m controller.

Allow tuning to change payload (default 128B) thanks to the
st,max-payload-size entry.
Can also limit the Maximum Read Request Size on downstream devices to the
minimum possible value between 128B and 256B.

STM32 PCIE may be in a power domain which is the case for the STM32MP25
based boards.
Supports wake# from wake-gpios

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 .../bindings/pci/st,stm32-pcie-host.yaml      | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml

diff --git a/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
new file mode 100644
index 000000000000..d7d360b63a08
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/st,stm32-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STM32MP25 PCIe root complex driver
+
+maintainers:
+  - Christian Bruel <christian.bruel@foss.st.com>
+
+description:
+  PCIe root complex controller based on the Synopsys DesignWare PCIe core.
+
+select:
+  properties:
+    compatible:
+      const: st,stm32mp25-pcie-rc
+  required:
+    - compatible
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#
+
+properties:
+  compatible:
+    const: st,stm32mp25-pcie-rc
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers.
+      - description: PCIe configuration registers.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: core
+
+  clocks:
+    maxItems: 1
+    description: PCIe system clock
+
+  clock-names:
+    const: core
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
+  num-lanes:
+    const: 1
+
+  msi-parent:
+    maxItems: 1
+
+  reset-gpios:
+    description: GPIO controlled connection to PERST# signal
+    maxItems: 1
+
+  wake-gpios:
+    description: GPIO controlled connection to WAKE# input signal
+    maxItems: 1
+
+  st,limit-mrrs:
+    description: If present limit downstream MRRS to 256B
+    type: boolean
+
+  st,max-payload-size:
+    description: Maximum Payload size to use
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [128, 256]
+    default: 128
+
+  wakeup-source: true
+
+  power-domains:
+    maxItems: 1
+
+  access-controllers:
+    maxItems: 1
+
+if:
+  required:
+    - wakeup-source
+then:
+  required:
+    - wake-gpios
+
+required:
+  - interrupt-map
+  - interrupt-map-mask
+  - ranges
+  - resets
+  - reset-names
+  - clocks
+  - clock-names
+  - phys
+  - phy-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/phy/phy.h>
+    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
+
+    pcie@48400000 {
+        compatible = "st,stm32mp25-pcie-rc";
+        device_type = "pci";
+        num-lanes = <1>;
+        reg = <0x48400000 0x400000>,
+              <0x10000000 0x10000>;
+        reg-names = "dbi", "config";
+        #interrupt-cells = <1>;
+        interrupt-map-mask = <0 0 0 7>;
+        interrupt-map = <0 0 0 1 &intc 0 0 GIC_SPI 264 IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 2 &intc 0 0 GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 3 &intc 0 0 GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,
+                        <0 0 0 4 &intc 0 0 GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x01000000 0 0x10010000 0x10010000 0 0x10000>,
+                 <0x02000000 0 0x10020000 0x10020000 0 0x7fe0000>,
+                 <0x42000000 0 0x18000000 0x18000000 0 0x8000000>;
+        bus-range = <0x00 0xff>;
+        clocks = <&rcc CK_BUS_PCIE>;
+        clock-names = "core";
+        phys = <&combophy PHY_TYPE_PCIE>;
+        phy-names = "pcie-phy";
+        resets = <&rcc PCIE_R>;
+        reset-names = "core";
+        msi-parent = <&v2m0>;
+        wakeup-source;
+        wake-gpios = <&gpioh 5 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
+        access-controllers = <&rifsc 76>;
+        power-domains = <&CLUSTER_PD>;
+    };
-- 
2.34.1

[PATCH 2/5] PCI: stm32: Add PCIe host support for STM32MP25

[Christian Bruel]

Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
DesignWare PCIe core.
Supports MSI via GICv2m, Single Virtual Channel, Single Function

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 drivers/pci/controller/dwc/Kconfig      |  11 +
 drivers/pci/controller/dwc/Makefile     |   1 +
 drivers/pci/controller/dwc/pcie-stm32.c | 493 ++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-stm32.h |  24 ++
 4 files changed, 529 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32.c
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32.h

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..50a014c2dfd0 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -459,4 +459,15 @@ config PCIE_VISCONTI_HOST
 	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
 	  This driver supports TMPV7708 SoC.
 
+config PCIE_STM32
+	tristate "STMicroelectronics STM32MP25 PCIe Controller (host mode)"
+	depends on ARCH_STM32 || COMPILE_TEST
+	depends on PCI_MSI
+	select PCIE_DW_HOST
+	help
+	  Enables support for the DesignWare core based PCIe host controller
+	  found in STM32MP25 SoC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pcie-stm32.
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a8308d9ea986..576d99cb3bc5 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
+obj-$(CONFIG_PCIE_STM32) += pcie-stm32.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-stm32.c b/drivers/pci/controller/dwc/pcie-stm32.c
new file mode 100644
index 000000000000..c27ab580a3bf
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-stm32.c
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * STMicroelectronics STM32MP25 PCIe root complex driver.
+ *
+ * Copyright (C) 2024 STMicroelectronics
+ * Author: Christian Bruel <christian.bruel@foss.st.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/pinctrl/devinfo.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include "pcie-designware.h"
+#include "pcie-stm32.h"
+
+struct stm32_pcie {
+	struct dw_pcie *pci;
+	struct regmap *regmap;
+	struct reset_control *rst;
+	struct phy *phy;
+	struct clk *clk;
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *wake_gpio;
+	unsigned int wake_irq;
+	u32 max_payload;
+	bool limit_downstream_mrrs;
+	bool link_is_up;
+};
+
+static const struct of_device_id stm32_pcie_of_match[] = {
+	{ .compatible = "st,stm32mp25-pcie-rc" },
+	{},
+};
+
+static int stm32_pcie_set_max_payload(struct dw_pcie *pci, int mps)
+{
+	int ret;
+	struct device *dev = pci->dev;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (mps != 128 && mps != 256) {
+		dev_err(dev, "Unexpected payload size %d\n", mps);
+		return -EINVAL;
+	}
+
+	ret = pcie_set_mps(pdev, mps);
+	if (ret)
+		dev_err(dev, "failed to set mps %d, error %d\n", mps, ret);
+
+	return ret;
+}
+
+static int stm32_pcie_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+
+	if (stm32_pcie->max_payload)
+		return stm32_pcie_set_max_payload(pci, stm32_pcie->max_payload);
+
+	return 0;
+}
+
+static int stm32_pcie_start_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	u32 ret;
+
+	if (stm32_pcie->reset_gpio) {
+		/* Make sure PERST# is asserted. */
+		gpiod_set_value(stm32_pcie->reset_gpio, 1);
+
+		/* Deassert PERST# after 100us */
+		usleep_range(100, 200);
+		gpiod_set_value(stm32_pcie->reset_gpio, 0);
+	}
+
+	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+				 STM32MP25_PCIECR_LTSSM_EN,
+				 STM32MP25_PCIECR_LTSSM_EN);
+
+	/*
+	 * PCIe specification states that you should not issue any config
+	 * requests until 100ms after asserting reset, so we enforce that here
+	 */
+	if (stm32_pcie->reset_gpio)
+		msleep(100);
+
+	return ret;
+}
+
+static void stm32_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);
+
+	/* Assert PERST# */
+	if (stm32_pcie->reset_gpio)
+		gpiod_set_value(stm32_pcie->reset_gpio, 1);
+}
+
+static int stm32_pcie_suspend(struct device *dev)
+{
+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev) || device_wakeup_path(dev))
+		enable_irq_wake(stm32_pcie->wake_irq);
+
+	return 0;
+}
+
+static int stm32_pcie_resume(struct device *dev)
+{
+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev) || device_wakeup_path(dev))
+		disable_irq_wake(stm32_pcie->wake_irq);
+
+	return 0;
+}
+
+static int stm32_pcie_suspend_noirq(struct device *dev)
+{
+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+
+	stm32_pcie->link_is_up = dw_pcie_link_up(stm32_pcie->pci);
+
+	stm32_pcie_stop_link(stm32_pcie->pci);
+	clk_disable_unprepare(stm32_pcie->clk);
+
+	if (!device_may_wakeup(dev) && !device_wakeup_path(dev))
+		phy_exit(stm32_pcie->phy);
+
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int stm32_pcie_resume_noirq(struct device *dev)
+{
+	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
+	struct dw_pcie *pci = stm32_pcie->pci;
+	struct dw_pcie_rp *pp = &pci->pp;
+	int ret;
+
+	/* init_state was set in pinctrl_bind_pins() before probe */
+	if (!IS_ERR(dev->pins->init_state))
+		ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
+	else
+		ret = pinctrl_pm_select_default_state(dev);
+
+	if (ret) {
+		dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
+		return ret;
+	}
+
+	if (!device_may_wakeup(dev) && !device_wakeup_path(dev)) {
+		ret = phy_init(stm32_pcie->phy);
+		if (ret) {
+			pinctrl_pm_select_default_state(dev);
+			return ret;
+		}
+	}
+
+	ret = clk_prepare_enable(stm32_pcie->clk);
+	if (ret)
+		goto clk_err;
+
+	ret = stm32_pcie_host_init(pp);
+	if (ret)
+		goto host_err;
+
+	ret = dw_pcie_setup_rc(pp);
+	if (ret)
+		goto pcie_err;
+
+	if (stm32_pcie->link_is_up) {
+		ret = stm32_pcie_start_link(stm32_pcie->pci);
+		if (ret)
+			goto pcie_err;
+
+		/* Ignore errors, the link may come up later */
+		dw_pcie_wait_for_link(stm32_pcie->pci);
+	}
+
+	pinctrl_pm_select_default_state(dev);
+
+	return 0;
+
+pcie_err:
+	dw_pcie_host_deinit(pp);
+host_err:
+	clk_disable_unprepare(stm32_pcie->clk);
+clk_err:
+	phy_exit(stm32_pcie->phy);
+	pinctrl_pm_select_default_state(dev);
+
+	return ret;
+}
+
+static const struct dev_pm_ops stm32_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend_noirq,
+				  stm32_pcie_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend, stm32_pcie_resume)
+};
+
+static const struct dw_pcie_host_ops stm32_pcie_host_ops = {
+	.init = stm32_pcie_host_init
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = stm32_pcie_start_link,
+	.stop_link = stm32_pcie_stop_link
+};
+
+static irqreturn_t stm32_pcie_wake_irq_handler(int irq, void *priv)
+{
+	struct stm32_pcie *stm32_pcie = priv;
+	struct device *dev = stm32_pcie->pci->dev;
+
+	dev_dbg(dev, "PCIE host wakeup by EP");
+
+	/* Notify PM core we are wakeup source */
+	pm_wakeup_event(dev, 0);
+	pm_system_wakeup();
+
+	return IRQ_HANDLED;
+}
+
+static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
+			       struct platform_device *pdev)
+{
+	struct dw_pcie *pci = stm32_pcie->pci;
+	struct device *dev = pci->dev;
+	struct dw_pcie_rp *pp = &pci->pp;
+	int ret;
+
+	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
+	if (ret)
+		return ret;
+
+	ret = phy_init(stm32_pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+				 STM32MP25_PCIECR_TYPE_MASK, STM32MP25_PCIECR_RC);
+	if (ret)
+		goto phy_disable;
+
+	reset_control_assert(stm32_pcie->rst);
+	reset_control_deassert(stm32_pcie->rst);
+
+	ret = clk_prepare_enable(stm32_pcie->clk);
+	if (ret) {
+		dev_err(dev, "Core clock enable failed %d\n", ret);
+		goto phy_disable;
+	}
+
+	pp->ops = &stm32_pcie_host_ops;
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host: %d\n", ret);
+		clk_disable_unprepare(stm32_pcie->clk);
+		goto phy_disable;
+	}
+
+	return 0;
+
+phy_disable:
+	phy_exit(stm32_pcie->phy);
+
+	return ret;
+}
+
+static int stm32_pcie_probe(struct platform_device *pdev)
+{
+	struct stm32_pcie *stm32_pcie;
+	struct dw_pcie *dw;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
+	if (!stm32_pcie)
+		return -ENOMEM;
+
+	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+	stm32_pcie->pci = dw;
+
+	dw->dev = dev;
+	dw->ops = &dw_pcie_ops;
+
+	stm32_pcie->regmap = syscon_regmap_lookup_by_compatible("st,stm32mp25-syscfg");
+	if (IS_ERR(stm32_pcie->regmap))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->regmap),
+				     "No syscfg specified\n");
+
+	stm32_pcie->phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(stm32_pcie->phy))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->phy),
+				     "failed to get pcie-phy\n");
+
+	stm32_pcie->clk = devm_clk_get(dev, "core");
+	if (IS_ERR(stm32_pcie->clk))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->clk),
+				     "Failed to get PCIe clock source\n");
+
+	stm32_pcie->rst = devm_reset_control_get_exclusive(dev, "core");
+	if (IS_ERR(stm32_pcie->rst))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->rst),
+				     "Failed to get PCIe reset\n");
+
+	/* Optionally limit payload */
+	ret = of_property_read_u32(np, "st,max-payload-size", &stm32_pcie->max_payload);
+	if (ret && ret != -EINVAL)
+		return dev_err_probe(dev, ret, "Error reading max-payload value\n");
+
+	if (of_property_read_bool(np, "st,limit-mrrs"))
+		stm32_pcie->limit_downstream_mrrs = true;
+
+	stm32_pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(stm32_pcie->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->reset_gpio),
+				     "Failed to get reset GPIO\n");
+
+	platform_set_drvdata(pdev, stm32_pcie);
+
+	if (device_property_read_bool(dev, "wakeup-source")) {
+		stm32_pcie->wake_gpio = devm_gpiod_get_optional(dev, "wake", GPIOD_IN);
+		if (IS_ERR(stm32_pcie->wake_gpio))
+			return dev_err_probe(dev, PTR_ERR(stm32_pcie->wake_gpio),
+					     "Failed to get wake GPIO\n");
+	}
+
+	if (stm32_pcie->wake_gpio) {
+		stm32_pcie->wake_irq = gpiod_to_irq(stm32_pcie->wake_gpio);
+
+		ret = devm_request_threaded_irq(&pdev->dev, stm32_pcie->wake_irq, NULL,
+						stm32_pcie_wake_irq_handler,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						"wake_irq", stm32_pcie);
+
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to request WAKE IRQ: %d\n", ret);
+	}
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable pm runtime %d\n", ret);
+		return ret;
+	}
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm runtime resume failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = stm32_add_pcie_port(stm32_pcie, pdev);
+	if (ret)  {
+		pm_runtime_put_sync(&pdev->dev);
+		return ret;
+	}
+
+	if (stm32_pcie->wake_gpio)
+		device_set_wakeup_capable(dev, true);
+
+	return 0;
+}
+
+static void stm32_pcie_remove(struct platform_device *pdev)
+{
+	struct stm32_pcie *stm32_pcie = platform_get_drvdata(pdev);
+	struct dw_pcie_rp *pp = &stm32_pcie->pci->pp;
+
+	if (stm32_pcie->wake_gpio)
+		device_init_wakeup(&pdev->dev, false);
+
+	dw_pcie_host_deinit(pp);
+	clk_disable_unprepare(stm32_pcie->clk);
+
+	phy_exit(stm32_pcie->phy);
+
+	pm_runtime_put_sync(&pdev->dev);
+}
+
+static struct platform_driver stm32_pcie_driver = {
+	.probe = stm32_pcie_probe,
+	.remove_new = stm32_pcie_remove,
+	.driver = {
+		.name = "stm32-pcie",
+		.of_match_table = stm32_pcie_of_match,
+		.pm		= &stm32_pcie_pm_ops,
+	},
+};
+
+static bool is_stm32_pcie_driver(struct device *dev)
+{
+	/* PCI bridge */
+	dev = get_device(dev);
+
+	/* Platform driver */
+	dev = get_device(dev->parent);
+
+	return (dev->driver == &stm32_pcie_driver.driver);
+}
+
+static bool apply_mrrs_quirk(struct pci_dev *root_port)
+{
+	struct dw_pcie_rp *pp;
+	struct dw_pcie *dw_pci;
+	struct stm32_pcie *stm32_pcie;
+
+	if (WARN_ON(!root_port) || !is_stm32_pcie_driver(root_port->dev.parent))
+		return false;
+
+	pp = root_port->bus->sysdata;
+	dw_pci = to_dw_pcie_from_pp(pp);
+	stm32_pcie = to_stm32_pcie(dw_pci);
+
+	if (!stm32_pcie->limit_downstream_mrrs)
+		return false;
+
+	return true;
+}
+
+static void quirk_stm32_pcie_limit_mrrs(struct pci_dev *pci)
+{
+	struct pci_dev *root_port;
+	struct pci_bus *bus = pci->bus;
+	int readrq;
+	int mps;
+
+	if (pci_is_root_bus(bus))
+		return;
+
+	root_port = pcie_find_root_port(pci);
+
+	if (!apply_mrrs_quirk(root_port))
+		return;
+
+	mps = pcie_get_mps(root_port);
+
+	/*
+	 * STM32 PCI controller has a h/w performance limitation on the AXI DDR requests.
+	 * Limit the maximum read request size to 256B on all downstream devices.
+	 */
+	readrq = pcie_get_readrq(pci);
+	if (readrq > 256) {
+		int mrrs = min(mps, 256);
+
+		pcie_set_readrq(pci, mrrs);
+
+		pci_info(pci, "Max Read Rq set to %4d (was %4d)\n", mrrs, readrq);
+	}
+}
+
+DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID,
+			 quirk_stm32_pcie_limit_mrrs);
+
+static int stm32_dma_limit(struct pci_dev *pdev, void *data)
+{
+	dev_dbg(&pdev->dev, "set bus_dma_limit");
+
+	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);
+
+	return 0;
+}
+
+static void quirk_stm32_dma_mask(struct pci_dev *pci)
+{
+	struct pci_dev *root_port;
+
+	root_port = pcie_find_root_port(pci);
+
+	if (root_port && is_stm32_pcie_driver(root_port->dev.parent))
+		pci_walk_bus(pci->bus, stm32_dma_limit, NULL);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SYNOPSYS, 0x0550, quirk_stm32_dma_mask);
+
+module_platform_driver(stm32_pcie_driver);
+
+MODULE_AUTHOR("Christian Bruel <christian.bruel@foss.st.com>");
+MODULE_DESCRIPTION("STM32MP25 PCIe Controller driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, stm32_pcie_of_match);
diff --git a/drivers/pci/controller/dwc/pcie-stm32.h b/drivers/pci/controller/dwc/pcie-stm32.h
new file mode 100644
index 000000000000..7e9300bfddce
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-stm32.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * ST PCIe driver definitions for STM32-MP25 SoC
+ *
+ * Copyright (C) 2024 STMicroelectronics - All Rights Reserved
+ * Author: Christian Bruel <christian.bruel@foss.st.com>
+ */
+
+#define to_stm32_pcie(x)	dev_get_drvdata((x)->dev)
+
+#define STM32MP25_PCIECR_TYPE_MASK	GENMASK(11, 8)
+#define STM32MP25_PCIECR_EP		0
+#define STM32MP25_PCIECR_RC		BIT(10)
+#define STM32MP25_PCIECR_REQ_RETRY_EN	BIT(3)
+#define STM32MP25_PCIECR_LTSSM_EN	BIT(2)
+
+#define SYSCFG_PCIECR			0x6000
+#define SYSCFG_PCIEPMEMSICR		0x6004
+#define SYSCFG_PCIEAERRCMSICR		0x6008
+#define SYSCFG_PCIESR1			0x6100
+#define SYSCFG_PCIESR2			0x6104
+
+#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
+#define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
-- 
2.34.1

[PATCH 3/5] dt-bindings: PCI: Add STM32MP25 PCIe endpoint bindings

[Christian Bruel]

STM32MP25 PCIe Controller is based on the DesignWare core configured as
end point mode from the SYSCFG register.

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 .../bindings/pci/st,stm32-pcie-ep.yaml        | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
new file mode 100644
index 000000000000..f0d215982794
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/st,stm32-pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STM32MP25 PCIe endpoint driver
+
+maintainers:
+  - Christian Bruel <christian.bruel@foss.st.com>
+
+description:
+  PCIe endpoint controller based on the Synopsys DesignWare PCIe core.
+
+allOf:
+  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#
+
+properties:
+  compatible:
+    const: st,stm32mp25-pcie-ep
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers.
+      - description: PCIe configuration registers.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: addr_space
+
+  clocks:
+    maxItems: 1
+    description: PCIe system clock
+
+  clock-names:
+    const: core
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: core
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
+  reset-gpios:
+    description: GPIO controlled connection to PERST# signal
+    maxItems: 1
+
+  access-controllers:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - resets
+  - reset-names
+  - clocks
+  - clock-names
+  - phys
+  - phy-names
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/phy/phy.h>
+    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
+
+    pcie-ep@48400000 {
+        compatible = "st,stm32mp25-pcie-ep";
+        num-lanes = <1>;
+        reg = <0x48400000 0x400000>,
+              <0x10000000 0x8000000>;
+        reg-names = "dbi", "addr_space";
+        clocks = <&rcc CK_BUS_PCIE>;
+        clock-names = "core";
+        phys = <&combophy PHY_TYPE_PCIE>;
+        phy-names = "pcie-phy";
+        resets = <&rcc PCIE_R>;
+        reset-names = "core";
+        pinctrl-names = "default", "init";
+        pinctrl-0 = <&pcie_pins_a>;
+        pinctrl-1 = <&pcie_init_pins_a>;
+        reset-gpios = <&gpioj 8 GPIO_ACTIVE_LOW>;
+        power-domains = <&CLUSTER_PD>;
+        access-controllers = <&rifsc 68>;
+    };
-- 
2.34.1

[PATCH 4/5] PCI: stm32: Add PCIe endpoint support for STM32MP25

[Christian Bruel]

Add driver to configure the STM32MP25 SoC PCIe Gen2 controller based on the
DesignWare PCIe core in endpoint mode.
Uses the common reference clock provided by the host.

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 drivers/pci/controller/dwc/Kconfig         |  12 +
 drivers/pci/controller/dwc/Makefile        |   1 +
 drivers/pci/controller/dwc/pcie-stm32-ep.c | 433 +++++++++++++++++++++
 3 files changed, 446 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-stm32-ep.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 50a014c2dfd0..29b7f45f82c7 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -470,4 +470,16 @@ config PCIE_STM32
 
 	  This driver can also be built as a module. If so, the module
 	  will be called pcie-stm32.
+
+config PCIE_STM32_EP
+	tristate "STMicroelectronics STM32MP25 PCIe Controller (endpoint mode)"
+	depends on ARCH_STM32 || COMPILE_TEST
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	help
+	  Enables endpoint support for DesignWare core based PCIe controller in found
+	  in STM32MP25 SoC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pcie-stm32-ep.
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 576d99cb3bc5..caebd98f6dd3 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
 obj-$(CONFIG_PCIE_STM32) += pcie-stm32.o
+obj-$(CONFIG_PCIE_STM32_EP) += pcie-stm32-ep.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-stm32-ep.c b/drivers/pci/controller/dwc/pcie-stm32-ep.c
new file mode 100644
index 000000000000..9fc43046531d
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-stm32-ep.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * STMicroelectronics STM32MP25 PCIe endpoint driver.
+ *
+ * Copyright (C) 2024 STMicroelectronics
+ * Author: Christian Bruel <christian.bruel@foss.st.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include "pcie-designware.h"
+#include "pcie-stm32.h"
+
+enum stm32_pcie_ep_link_status {
+	STM32_PCIE_EP_LINK_DISABLED,
+	STM32_PCIE_EP_LINK_ENABLED,
+};
+
+struct stm32_pcie {
+	struct dw_pcie *pci;
+	struct regmap *regmap;
+	struct reset_control *rst;
+	struct phy *phy;
+	struct clk *clk;
+	struct gpio_desc *reset_gpio;
+	enum stm32_pcie_ep_link_status link_status;
+	unsigned int perst_irq;
+};
+
+static const struct of_device_id stm32_pcie_ep_of_match[] = {
+	{ .compatible = "st,stm32mp25-pcie-ep" },
+	{},
+};
+
+static void stm32_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	enum pci_barno bar;
+
+	for (bar = BAR_0; bar <= PCI_STD_NUM_BARS; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+
+	/* Defer Completion Requests until link started */
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+			   STM32MP25_PCIECR_REQ_RETRY_EN,
+			   STM32MP25_PCIECR_REQ_RETRY_EN);
+}
+
+static int stm32_pcie_enable_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	int ret;
+
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+			   STM32MP25_PCIECR_LTSSM_EN,
+			   STM32MP25_PCIECR_LTSSM_EN);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		return ret;
+
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+			   STM32MP25_PCIECR_REQ_RETRY_EN,
+			   0);
+
+	return 0;
+}
+
+static void stm32_pcie_disable_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+			   STM32MP25_PCIECR_REQ_RETRY_EN,
+			   STM32MP25_PCIECR_REQ_RETRY_EN);
+
+	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);
+}
+
+static int stm32_pcie_start_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	int ret;
+
+	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_ENABLED) {
+		dev_dbg(pci->dev, "Link is already enabled\n");
+		return 0;
+	}
+
+	ret = stm32_pcie_enable_link(pci);
+	if (ret) {
+		dev_err(pci->dev, "PCIe cannot establish link: %d\n", ret);
+		return ret;
+	}
+
+	stm32_pcie->link_status = STM32_PCIE_EP_LINK_ENABLED;
+
+	enable_irq(stm32_pcie->perst_irq);
+
+	return 0;
+}
+
+static void stm32_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+
+	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_DISABLED) {
+		dev_dbg(pci->dev, "Link is already disabled\n");
+		return;
+	}
+
+	disable_irq(stm32_pcie->perst_irq);
+
+	stm32_pcie_disable_link(pci);
+
+	stm32_pcie->link_status = STM32_PCIE_EP_LINK_DISABLED;
+}
+
+static int stm32_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				unsigned int type, u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_IRQ_INTX:
+		return dw_pcie_ep_raise_intx_irq(ep, func_no);
+	case PCI_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;
+	}
+
+	return 0;
+}
+
+static const struct pci_epc_features stm32_pcie_epc_features = {
+	.msi_capable = true,
+	.align = 1 << 16,
+};
+
+static const struct pci_epc_features*
+stm32_pcie_get_features(struct dw_pcie_ep *ep)
+{
+	return &stm32_pcie_epc_features;
+}
+
+static const struct dw_pcie_ep_ops stm32_pcie_ep_ops = {
+	.init = stm32_pcie_ep_init,
+	.raise_irq = stm32_pcie_raise_irq,
+	.get_features = stm32_pcie_get_features,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = stm32_pcie_start_link,
+	.stop_link = stm32_pcie_stop_link,
+};
+
+static int stm32_pcie_enable_resources(struct stm32_pcie *stm32_pcie)
+{
+	int ret;
+
+	ret = phy_init(stm32_pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(stm32_pcie->clk);
+	if (ret)
+		phy_exit(stm32_pcie->phy);
+
+	return ret;
+}
+
+static void stm32_pcie_disable_resources(struct stm32_pcie *stm32_pcie)
+{
+	clk_disable_unprepare(stm32_pcie->clk);
+
+	phy_exit(stm32_pcie->phy);
+}
+
+static void stm32_pcie_perst_assert(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	struct device *dev = pci->dev;
+
+	dev_dbg(dev, "PERST asserted by host. Shutting down the PCIe link\n");
+
+	/*
+	 * Do not try to release resources if the PERST# is
+	 * asserted before the link is started.
+	 */
+	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_DISABLED) {
+		dev_dbg(pci->dev, "Link is already disabled\n");
+		return;
+	}
+
+	stm32_pcie_disable_link(pci);
+
+	stm32_pcie_disable_resources(stm32_pcie);
+
+	pm_runtime_put_sync(dev);
+
+	stm32_pcie->link_status = STM32_PCIE_EP_LINK_DISABLED;
+}
+
+static void stm32_pcie_perst_deassert(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	struct device *dev = pci->dev;
+	struct dw_pcie_ep *ep = &pci->ep;
+	int ret;
+
+	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_ENABLED) {
+		dev_dbg(pci->dev, "Link is already enabled\n");
+		return;
+	}
+
+	dev_dbg(dev, "PERST de-asserted by host. Starting link training\n");
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm runtime resume failed: %d\n", ret);
+		return;
+	}
+
+	ret = stm32_pcie_enable_resources(stm32_pcie);
+	if (ret) {
+		dev_err(dev, "Failed to enable resources: %d\n", ret);
+		pm_runtime_put_sync(dev);
+		return;
+	}
+
+	ret = dw_pcie_ep_init_registers(ep);
+	if (ret) {
+		dev_err(dev, "Failed to complete initialization: %d\n", ret);
+		stm32_pcie_disable_resources(stm32_pcie);
+		pm_runtime_put_sync(dev);
+		return;
+	}
+
+	pci_epc_init_notify(ep->epc);
+
+	ret = stm32_pcie_enable_link(pci);
+	if (ret) {
+		dev_err(dev, "PCIe Cannot establish link: %d\n", ret);
+		stm32_pcie_disable_resources(stm32_pcie);
+		pm_runtime_put_sync(dev);
+		return;
+	}
+
+	stm32_pcie->link_status = STM32_PCIE_EP_LINK_ENABLED;
+}
+
+static irqreturn_t stm32_pcie_ep_perst_irq_thread(int irq, void *data)
+{
+	struct stm32_pcie *stm32_pcie = data;
+	struct dw_pcie *pci = stm32_pcie->pci;
+	u32 perst;
+
+	perst = gpiod_get_value(stm32_pcie->reset_gpio);
+	if (perst)
+		stm32_pcie_perst_assert(pci);
+	else
+		stm32_pcie_perst_deassert(pci);
+
+	return IRQ_HANDLED;
+}
+
+static int stm32_add_pcie_ep(struct stm32_pcie *stm32_pcie,
+			     struct platform_device *pdev)
+{
+	struct dw_pcie *pci = stm32_pcie->pci;
+	struct dw_pcie_ep *ep = &pci->ep;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+				 STM32MP25_PCIECR_TYPE_MASK,
+				 STM32MP25_PCIECR_EP);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm runtime resume failed: %d\n", ret);
+		return ret;
+	}
+
+	reset_control_assert(stm32_pcie->rst);
+	reset_control_deassert(stm32_pcie->rst);
+
+	ep->ops = &stm32_pcie_ep_ops;
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret) {
+		dev_err(dev, "failed to initialize ep: %d\n", ret);
+		pm_runtime_put_sync(dev);
+		return ret;
+	}
+
+	ret = stm32_pcie_enable_resources(stm32_pcie);
+	if (ret) {
+		dev_err(dev, "failed to enable resources: %d\n", ret);
+		dw_pcie_ep_deinit(ep);
+		pm_runtime_put_sync(dev);
+		return ret;
+	}
+
+	ret = dw_pcie_ep_init_registers(ep);
+	if (ret) {
+		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
+		stm32_pcie_disable_resources(stm32_pcie);
+		dw_pcie_ep_deinit(ep);
+		pm_runtime_put_sync(dev);
+		return ret;
+	}
+
+	pci_epc_init_notify(ep->epc);
+
+	return 0;
+}
+
+static int stm32_pcie_probe(struct platform_device *pdev)
+{
+	struct stm32_pcie *stm32_pcie;
+	struct dw_pcie *dw;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
+	if (!stm32_pcie)
+		return -ENOMEM;
+
+	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+	stm32_pcie->pci = dw;
+
+	dw->dev = dev;
+	dw->ops = &dw_pcie_ops;
+
+	stm32_pcie->regmap = syscon_regmap_lookup_by_compatible("st,stm32mp25-syscfg");
+	if (IS_ERR(stm32_pcie->regmap))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->regmap),
+				     "No syscfg specified\n");
+
+	stm32_pcie->phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(stm32_pcie->phy))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->phy),
+				     "failed to get pcie-phy\n");
+
+	stm32_pcie->clk = devm_clk_get(dev, "core");
+	if (IS_ERR(stm32_pcie->clk))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->clk),
+				     "Failed to get PCIe clock source\n");
+
+	stm32_pcie->rst = devm_reset_control_get_exclusive(dev, "core");
+	if (IS_ERR(stm32_pcie->rst))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->rst),
+				     "Failed to get PCIe reset\n");
+
+	stm32_pcie->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_IN);
+	if (IS_ERR(stm32_pcie->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->reset_gpio),
+				     "Failed to get reset GPIO\n");
+
+	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, stm32_pcie);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable pm runtime %d\n", ret);
+		return ret;
+	}
+
+	stm32_pcie->perst_irq = gpiod_to_irq(stm32_pcie->reset_gpio);
+
+	/* Will be enabled in start_link when device is initialized. */
+	irq_set_status_flags(stm32_pcie->perst_irq, IRQ_NOAUTOEN);
+
+	ret = devm_request_threaded_irq(dev, stm32_pcie->perst_irq, NULL,
+					stm32_pcie_ep_perst_irq_thread,
+					IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"perst_irq", stm32_pcie);
+	if (ret) {
+		dev_err(dev, "Failed to request PERST IRQ: %d\n", ret);
+		return ret;
+	}
+
+	return stm32_add_pcie_ep(stm32_pcie, pdev);
+}
+
+static void stm32_pcie_remove(struct platform_device *pdev)
+{
+	struct stm32_pcie *stm32_pcie = platform_get_drvdata(pdev);
+	struct dw_pcie_ep *ep = &stm32_pcie->pci->ep;
+
+	disable_irq(stm32_pcie->perst_irq);
+
+	dw_pcie_ep_deinit(ep);
+
+	stm32_pcie_disable_resources(stm32_pcie);
+
+	pm_runtime_put_sync(&pdev->dev);
+}
+
+static struct platform_driver stm32_pcie_ep_driver = {
+	.probe = stm32_pcie_probe,
+	.remove_new = stm32_pcie_remove,
+	.driver = {
+		.name = "stm32-ep-pcie",
+		.of_match_table = stm32_pcie_ep_of_match,
+	},
+};
+
+module_platform_driver(stm32_pcie_ep_driver);
+
+MODULE_AUTHOR("Christian Bruel <christian.bruel@foss.st.com>");
+MODULE_DESCRIPTION("STM32MP25 PCIe Endpoint Controller driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, stm32_pcie_ep_of_match);
-- 
2.34.1

[PATCH 5/5] MAINTAINERS: add entry for ST STM32MP25 PCIe drivers

[Christian Bruel]

Add myself as STM32MP25 PCIe host and PCIe endpoint drivers

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4803908768e8..277e1cc0769e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17912,6 +17912,13 @@ L:	linux-samsung-soc@vger.kernel.org
 S:	Maintained
 F:	drivers/pci/controller/dwc/pci-exynos.c
 
+PCI DRIVER FOR STM32MP25
+M:	Christian Bruel <christian.bruel@foss.st.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/st,stm32-pcie-*.yaml
+F:	drivers/pci/controller/dwc/*stm32*
+
 PCI DRIVER FOR SYNOPSYS DESIGNWARE
 M:	Jingoo Han <jingoohan1@gmail.com>
 M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
-- 
2.34.1

Re: [PATCH 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Bjorn Helgaas]

On Tue, Nov 12, 2024 at 05:19:21PM +0100, Christian Bruel wrote:
> Document the bindings for STM32MP25 PCIe Controller configured in
> root complex mode.
> Supports 4 legacy interrupts and MSI interrupts from the ARM
> GICv2m controller.
> 
> Allow tuning to change payload (default 128B) thanks to the
> st,max-payload-size entry.
> Can also limit the Maximum Read Request Size on downstream devices to the
> minimum possible value between 128B and 256B.
> 
> STM32 PCIE may be in a power domain which is the case for the STM32MP25
> based boards.
> Supports wake# from wake-gpios

> +  st,limit-mrrs:
> +    description: If present limit downstream MRRS to 256B
> +    type: boolean
> +
> +  st,max-payload-size:
> +    description: Maximum Payload size to use
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [128, 256]
> +    default: 128

MRRS and MPS are not specific to this device.  Not sure why you need
them, but if you do need them, I think they should be generic.

Re: [PATCH 2/5] PCI: stm32: Add PCIe host support for STM32MP25

[Bjorn Helgaas]

On Tue, Nov 12, 2024 at 05:19:22PM +0100, Christian Bruel wrote:
> Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
> DesignWare PCIe core.
> Supports MSI via GICv2m, Single Virtual Channel, Single Function

Add blank lines between paragraphs.  Also applies to other patches in
the series.

> +config PCIE_STM32
> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (host mode)"
> +	depends on ARCH_STM32 || COMPILE_TEST
> +	depends on PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Enables support for the DesignWare core based PCIe host controller
> +	  found in STM32MP25 SoC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called pcie-stm32.

Move this so the drivers stay alphabetized.  There's already a
"STMicroelectronics SPEAr PCIe controller" entry, and this should go
right next to it.

> +++ b/drivers/pci/controller/dwc/pcie-stm32.c

> +static const struct of_device_id stm32_pcie_of_match[] = {
> +	{ .compatible = "st,stm32mp25-pcie-rc" },
> +	{},
> +};

Most drivers put this near the platform_driver struct that references
it.

> +static int stm32_pcie_set_max_payload(struct dw_pcie *pci, int mps)
> +{
> +	int ret;
> +	struct device *dev = pci->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (mps != 128 && mps != 256) {
> +		dev_err(dev, "Unexpected payload size %d\n", mps);
> +		return -EINVAL;
> +	}
> +
> +	ret = pcie_set_mps(pdev, mps);
> +	if (ret)
> +		dev_err(dev, "failed to set mps %d, error %d\n", mps, ret);

MPS config is normally not device-specific, and it's somewhat fragile
(see pci_configure_mps() and pcie_bus_config), so I kind of hate to
see more users.  Maybe there's some hardware issue involved here?

> +static int stm32_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> +	u32 ret;
> +
> +	if (stm32_pcie->reset_gpio) {
> +		/* Make sure PERST# is asserted. */
> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);
> +
> +		/* Deassert PERST# after 100us */
> +		usleep_range(100, 200);

If this is PCIE_T_PERST_CLK_US, use that.  If not, please add a
relevant #define with a citation to the spec.

> +		gpiod_set_value(stm32_pcie->reset_gpio, 0);
> +	}
> +
> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +				 STM32MP25_PCIECR_LTSSM_EN,
> +				 STM32MP25_PCIECR_LTSSM_EN);
> +
> +	/*
> +	 * PCIe specification states that you should not issue any config
> +	 * requests until 100ms after asserting reset, so we enforce that here

I think it says 100ms after *deasserting* reset.  But if you use
PCIE_T_RRS_READY_MS below, I don't think you even need this comment.

> +	if (stm32_pcie->reset_gpio)
> +		msleep(100);

I think this is PCIE_T_RRS_READY_MS.

> +static void stm32_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> +
> +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);

With a half-dozen exceptions, this file fits nicely in 80 columns.
Can you wrap this and the similar exceptions?  No need to break printf
strings or the regmap strings that can't reasonably be wrapped.

> +	/* Assert PERST# */
> +	if (stm32_pcie->reset_gpio)
> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);

Might be nice to include "perst" in the "reset_gpio" name to identify
it more specifically.

> +}
> +
> +static int stm32_pcie_suspend(struct device *dev)
> +{
> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev) || device_wakeup_path(dev))
> +		enable_irq_wake(stm32_pcie->wake_irq);
> +
> +	return 0;
> +}
> +
> +static int stm32_pcie_resume(struct device *dev)
> +{
> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev) || device_wakeup_path(dev))
> +		disable_irq_wake(stm32_pcie->wake_irq);
> +
> +	return 0;
> +}
> +
> +static int stm32_pcie_suspend_noirq(struct device *dev)
> +{
> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> +
> +	stm32_pcie->link_is_up = dw_pcie_link_up(stm32_pcie->pci);
> +
> +	stm32_pcie_stop_link(stm32_pcie->pci);
> +	clk_disable_unprepare(stm32_pcie->clk);
> +
> +	if (!device_may_wakeup(dev) && !device_wakeup_path(dev))
> +		phy_exit(stm32_pcie->phy);
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int stm32_pcie_resume_noirq(struct device *dev)
> +{
> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = stm32_pcie->pci;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	int ret;
> +
> +	/* init_state was set in pinctrl_bind_pins() before probe */
> +	if (!IS_ERR(dev->pins->init_state))
> +		ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> +	else
> +		ret = pinctrl_pm_select_default_state(dev);
> +
> +	if (ret) {
> +		dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!device_may_wakeup(dev) && !device_wakeup_path(dev)) {
> +		ret = phy_init(stm32_pcie->phy);
> +		if (ret) {
> +			pinctrl_pm_select_default_state(dev);
> +			return ret;
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(stm32_pcie->clk);
> +	if (ret)
> +		goto clk_err;
> +
> +	ret = stm32_pcie_host_init(pp);
> +	if (ret)
> +		goto host_err;
> +
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto pcie_err;
> +
> +	if (stm32_pcie->link_is_up) {
> +		ret = stm32_pcie_start_link(stm32_pcie->pci);
> +		if (ret)
> +			goto pcie_err;
> +
> +		/* Ignore errors, the link may come up later */
> +		dw_pcie_wait_for_link(stm32_pcie->pci);
> +	}
> +
> +	pinctrl_pm_select_default_state(dev);

Interesting that pcie-stm32.c, pci-tegra.c, and pcie-tegra194.c are
the only PCI controller drivers that use this.  I have no idea what
this is; it just makes me wonder if these three are just special, or
if others should be using it?

> +static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
> +			       struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = stm32_pcie->pci;
> +	struct device *dev = pci->dev;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	int ret;
> +
> +	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_init(stm32_pcie->phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +				 STM32MP25_PCIECR_TYPE_MASK, STM32MP25_PCIECR_RC);
> +	if (ret)
> +		goto phy_disable;
> +
> +	reset_control_assert(stm32_pcie->rst);
> +	reset_control_deassert(stm32_pcie->rst);

Is there any reset pulse width requirement here?

> +	ret = clk_prepare_enable(stm32_pcie->clk);
> +	if (ret) {
> +		dev_err(dev, "Core clock enable failed %d\n", ret);
> +		goto phy_disable;
> +	}
> +
> +	pp->ops = &stm32_pcie_host_ops;
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize host: %d\n", ret);

Consider making all the messages consistent in terms of sentence
structure and capitalization.

> +static int stm32_pcie_probe(struct platform_device *pdev)
> +{
> +	struct stm32_pcie *stm32_pcie;
> +	struct dw_pcie *dw;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = pdev->dev.of_node;
> +	int ret;
> +
> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
> +	if (!stm32_pcie)
> +		return -ENOMEM;
> +
> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;

Add blank line.

> +static struct platform_driver stm32_pcie_driver = {
> +	.probe = stm32_pcie_probe,
> +	.remove_new = stm32_pcie_remove,

Use .remove instead of .remove_new; see 0edb555a65d1 ("platform: Make
platform_driver::remove() return void").

> +	.driver = {
> +		.name = "stm32-pcie",
> +		.of_match_table = stm32_pcie_of_match,
> +		.pm		= &stm32_pcie_pm_ops,
> +	},
> +};
> +
> +static bool is_stm32_pcie_driver(struct device *dev)
> +{
> +	/* PCI bridge */
> +	dev = get_device(dev);
> +
> +	/* Platform driver */
> +	dev = get_device(dev->parent);
> +
> +	return (dev->driver == &stm32_pcie_driver.driver);

Ugh.  Some MPS/MRRS magic going on here, evidently related to the STM
integration of DWC IP?

> +static bool apply_mrrs_quirk(struct pci_dev *root_port)
> +{
> +	struct dw_pcie_rp *pp;
> +	struct dw_pcie *dw_pci;
> +	struct stm32_pcie *stm32_pcie;
> +
> +	if (WARN_ON(!root_port) || !is_stm32_pcie_driver(root_port->dev.parent))
> +		return false;
> +
> +	pp = root_port->bus->sysdata;
> +	dw_pci = to_dw_pcie_from_pp(pp);
> +	stm32_pcie = to_stm32_pcie(dw_pci);
> +
> +	if (!stm32_pcie->limit_downstream_mrrs)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void quirk_stm32_pcie_limit_mrrs(struct pci_dev *pci)
> +{
> +	struct pci_dev *root_port;
> +	struct pci_bus *bus = pci->bus;
> +	int readrq;
> +	int mps;
> +
> +	if (pci_is_root_bus(bus))
> +		return;
> +
> +	root_port = pcie_find_root_port(pci);
> +
> +	if (!apply_mrrs_quirk(root_port))
> +		return;
> +
> +	mps = pcie_get_mps(root_port);
> +
> +	/*
> +	 * STM32 PCI controller has a h/w performance limitation on the AXI DDR requests.
> +	 * Limit the maximum read request size to 256B on all downstream devices.

I guess this is some kind of platform erratum, since there's no way
for us to discover a limit on supported MRRS values?

> +	readrq = pcie_get_readrq(pci);
> +	if (readrq > 256) {
> +		int mrrs = min(mps, 256);
> +
> +		pcie_set_readrq(pci, mrrs);
> +
> +		pci_info(pci, "Max Read Rq set to %4d (was %4d)\n", mrrs, readrq);
> +	}
> +}
> +
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID,
> +			 quirk_stm32_pcie_limit_mrrs);
> +
> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
> +{
> +	dev_dbg(&pdev->dev, "set bus_dma_limit");
> +
> +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);

This is quite unusual and deserves some comment about why we need
it.

> +	return 0;
> +}
> +
> +static void quirk_stm32_dma_mask(struct pci_dev *pci)
> +{
> +	struct pci_dev *root_port;
> +
> +	root_port = pcie_find_root_port(pci);
> +
> +	if (root_port && is_stm32_pcie_driver(root_port->dev.parent))
> +		pci_walk_bus(pci->bus, stm32_dma_limit, NULL);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SYNOPSYS, 0x0550, quirk_stm32_dma_mask);

I guess this applies to [16c3:0550] devices and everything below them?
It looks like these must be Root Ports?  And they identify as
PCI_VENDOR_ID_SYNOPSYS instead of PCI_VENDOR_ID_STMICRO (104a)?

Could be added at https://admin.pci-ids.ucw.cz/read/PC/16c3/ if you
want lspci to name them correctly.

> +++ b/drivers/pci/controller/dwc/pcie-stm32.h

> +#define STM32MP25_PCIECR_EP		0

Ideally would go in the patch that uses it.

> +#define SYSCFG_PCIEPMEMSICR		0x6004
> +#define SYSCFG_PCIEAERRCMSICR		0x6008
> +#define SYSCFG_PCIESR1			0x6100
> +#define SYSCFG_PCIESR2			0x6104
> +
> +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
> +#define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)

These are all unused, drop them until you need them.

Re: [PATCH 4/5] PCI: stm32: Add PCIe endpoint support for STM32MP25

[Bjorn Helgaas]

On Tue, Nov 12, 2024 at 05:19:24PM +0100, Christian Bruel wrote:
> Add driver to configure the STM32MP25 SoC PCIe Gen2 controller based on the
> DesignWare PCIe core in endpoint mode.
> Uses the common reference clock provided by the host.

> +++ b/drivers/pci/controller/dwc/Kconfig

> +config PCIE_STM32_EP
> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (endpoint mode)"
> +	depends on ARCH_STM32 || COMPILE_TEST
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Enables endpoint support for DesignWare core based PCIe controller in found
> +	  in STM32MP25 SoC.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called pcie-stm32-ep.

Move as for the host mode entry.

> +++ b/drivers/pci/controller/dwc/pcie-stm32-ep.c

> +static const struct of_device_id stm32_pcie_ep_of_match[] = {
> +	{ .compatible = "st,stm32mp25-pcie-ep" },
> +	{},
> +};

Move next to stm32_pcie_ep_driver.

> +static void stm32_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> +	enum pci_barno bar;
> +
> +	for (bar = BAR_0; bar <= PCI_STD_NUM_BARS; bar++)

Most users just use "bar = 0".  BAR_0 is 0, but there's no real
connection with PCI_STD_NUM_BARS, so I think 0 is probably better.

Looks like this should be "bar < PCI_STD_NUM_BARS"?

> +		dw_pcie_ep_reset_bar(pci, bar);
> +
> +	/* Defer Completion Requests until link started */

Not sure what a Completion Request is.  Is this some internal STM or
DWC thing?  Or is this related to Request Retry Status completions for
config requests?

> +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +			   STM32MP25_PCIECR_REQ_RETRY_EN,
> +			   STM32MP25_PCIECR_REQ_RETRY_EN);
> +}

> +static int stm32_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				unsigned int type, u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_IRQ_INTX:
> +		return dw_pcie_ep_raise_intx_irq(ep, func_no);
> +	case PCI_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;
> +	}
> +
> +	return 0;

Is the compiler not smart enough to notice that this is unreachable?

> +static void stm32_pcie_perst_deassert(struct dw_pcie *pci)
> +{
> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> +	struct device *dev = pci->dev;
> +	struct dw_pcie_ep *ep = &pci->ep;
> +	int ret;
> +
> +	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_ENABLED) {
> +		dev_dbg(pci->dev, "Link is already enabled\n");
> +		return;
> +	}
> +
> +	dev_dbg(dev, "PERST de-asserted by host. Starting link training\n");
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm runtime resume failed: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = stm32_pcie_enable_resources(stm32_pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
> +		pm_runtime_put_sync(dev);
> +		return;
> +	}
> +
> +	ret = dw_pcie_ep_init_registers(ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
> +		stm32_pcie_disable_resources(stm32_pcie);
> +		pm_runtime_put_sync(dev);
> +		return;
> +	}
> +
> +	pci_epc_init_notify(ep->epc);
> +
> +	ret = stm32_pcie_enable_link(pci);
> +	if (ret) {
> +		dev_err(dev, "PCIe Cannot establish link: %d\n", ret);
> +		stm32_pcie_disable_resources(stm32_pcie);
> +		pm_runtime_put_sync(dev);
> +		return;
> +	}
> +
> +	stm32_pcie->link_status = STM32_PCIE_EP_LINK_ENABLED;
> +}

> +static int stm32_add_pcie_ep(struct stm32_pcie *stm32_pcie,
> +			     struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = stm32_pcie->pci;
> +	struct dw_pcie_ep *ep = &pci->ep;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +				 STM32MP25_PCIECR_TYPE_MASK,
> +				 STM32MP25_PCIECR_EP);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm runtime resume failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	reset_control_assert(stm32_pcie->rst);
> +	reset_control_deassert(stm32_pcie->rst);
> +
> +	ep->ops = &stm32_pcie_ep_ops;
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize ep: %d\n", ret);
> +		pm_runtime_put_sync(dev);
> +		return ret;
> +	}
> +
> +	ret = stm32_pcie_enable_resources(stm32_pcie);
> +	if (ret) {
> +		dev_err(dev, "failed to enable resources: %d\n", ret);
> +		dw_pcie_ep_deinit(ep);
> +		pm_runtime_put_sync(dev);
> +		return ret;
> +	}
> +
> +	ret = dw_pcie_ep_init_registers(ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
> +		stm32_pcie_disable_resources(stm32_pcie);
> +		dw_pcie_ep_deinit(ep);
> +		pm_runtime_put_sync(dev);
> +		return ret;
> +	}

Consider gotos for the error cases with a cleanup block at the end.
There's a fair bit of repetition there as more things get initialized,
and it's error-prone to extend this in the future.

Same applies in stm32_pcie_perst_deassert().

> +	pci_epc_init_notify(ep->epc);
> +
> +	return 0;
> +}
> +
> +static int stm32_pcie_probe(struct platform_device *pdev)
> +{
> +	struct stm32_pcie *stm32_pcie;
> +	struct dw_pcie *dw;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
> +	if (!stm32_pcie)
> +		return -ENOMEM;
> +
> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;

Add blank line here.

> +	stm32_pcie->pci = dw;

> +static struct platform_driver stm32_pcie_ep_driver = {
> +	.probe = stm32_pcie_probe,
> +	.remove_new = stm32_pcie_remove,

.remove().

> +	.driver = {
> +		.name = "stm32-ep-pcie",
> +		.of_match_table = stm32_pcie_ep_of_match,
> +	},
> +};

Re: [PATCH 5/5] MAINTAINERS: add entry for ST STM32MP25 PCIe drivers

[Bjorn Helgaas]

On Tue, Nov 12, 2024 at 05:19:25PM +0100, Christian Bruel wrote:
> Add myself as STM32MP25 PCIe host and PCIe endpoint drivers

s/as STM32MP25/as maintainer of STM32MP25/

> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4803908768e8..277e1cc0769e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17912,6 +17912,13 @@ L:	linux-samsung-soc@vger.kernel.org
>  S:	Maintained
>  F:	drivers/pci/controller/dwc/pci-exynos.c
>  
> +PCI DRIVER FOR STM32MP25
> +M:	Christian Bruel <christian.bruel@foss.st.com>
> +L:	linux-pci@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/pci/st,stm32-pcie-*.yaml
> +F:	drivers/pci/controller/dwc/*stm32*
> +
>  PCI DRIVER FOR SYNOPSYS DESIGNWARE
>  M:	Jingoo Han <jingoohan1@gmail.com>
>  M:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> -- 
> 2.34.1
> 

Re: [PATCH 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Christian Bruel]

On 11/12/24 19:28, Bjorn Helgaas wrote:
> On Tue, Nov 12, 2024 at 05:19:21PM +0100, Christian Bruel wrote:
>> Document the bindings for STM32MP25 PCIe Controller configured in
>> root complex mode.
>> Supports 4 legacy interrupts and MSI interrupts from the ARM
>> GICv2m controller.
>>
>> Allow tuning to change payload (default 128B) thanks to the
>> st,max-payload-size entry.
>> Can also limit the Maximum Read Request Size on downstream devices to the
>> minimum possible value between 128B and 256B.
>>
>> STM32 PCIE may be in a power domain which is the case for the STM32MP25
>> based boards.
>> Supports wake# from wake-gpios
> 
>> +  st,limit-mrrs:
>> +    description: If present limit downstream MRRS to 256B
>> +    type: boolean
>> +
>> +  st,max-payload-size:
>> +    description: Maximum Payload size to use
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [128, 256]
>> +    default: 128
> 
> MRRS and MPS are not specific to this device.  Not sure why you need
> them, but if you do need them, I think they should be generic.

Agree. On a second thought, this was to fix an old errata and can be 
dropped now, as well as the associated quirks.

Will re-post as generic if needed later on

thanks,

Christian

Re: [PATCH 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Rob Herring]

On Tue, Nov 12, 2024 at 05:19:21PM +0100, Christian Bruel wrote:
> Document the bindings for STM32MP25 PCIe Controller configured in
> root complex mode.
> Supports 4 legacy interrupts and MSI interrupts from the ARM
> GICv2m controller.
> 
> Allow tuning to change payload (default 128B) thanks to the
> st,max-payload-size entry.
> Can also limit the Maximum Read Request Size on downstream devices to the
> minimum possible value between 128B and 256B.
> 
> STM32 PCIE may be in a power domain which is the case for the STM32MP25
> based boards.
> Supports wake# from wake-gpios
> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  .../bindings/pci/st,stm32-pcie-host.yaml      | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
> new file mode 100644
> index 000000000000..d7d360b63a08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/st,stm32-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STM32MP25 PCIe root complex driver
> +
> +maintainers:
> +  - Christian Bruel <christian.bruel@foss.st.com>
> +
> +description:
> +  PCIe root complex controller based on the Synopsys DesignWare PCIe core.
> +
> +select:
> +  properties:
> +    compatible:
> +      const: st,stm32mp25-pcie-rc
> +  required:
> +    - compatible
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#

snps,dw-pcie.yaml instead of these 2.

> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-pcie-rc
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers.
> +      - description: PCIe configuration registers.
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: core

-names with a single entry is kind of pointless.

> +
> +  clocks:
> +    maxItems: 1
> +    description: PCIe system clock
> +
> +  clock-names:
> +    const: core
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  num-lanes:
> +    const: 1
> +
> +  msi-parent:
> +    maxItems: 1

Just 'msi-parent: true'. It's already only ever 1 entry.

> +
> +  reset-gpios:
> +    description: GPIO controlled connection to PERST# signal
> +    maxItems: 1
> +
> +  wake-gpios:
> +    description: GPIO controlled connection to WAKE# input signal
> +    maxItems: 1
> +

> +  st,limit-mrrs:
> +    description: If present limit downstream MRRS to 256B
> +    type: boolean
> +
> +  st,max-payload-size:
> +    description: Maximum Payload size to use
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [128, 256]
> +    default: 128

IIRC, other hosts have similar restrictions, so you should be able to do 
the same and imply it from the compatible. Though I'm open to a common 
property as Bjorn suggested.

> +
> +  wakeup-source: true
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  access-controllers:
> +    maxItems: 1
> +
> +if:
> +  required:
> +    - wakeup-source
> +then:
> +  required:
> +    - wake-gpios

This can be just:

dependentRequired:
  wakeup-source: [ wake-gpios ]

(dependentRequired supercedes dependencies)

> +
> +required:
> +  - interrupt-map
> +  - interrupt-map-mask
> +  - ranges
> +  - resets
> +  - reset-names
> +  - clocks
> +  - clock-names
> +  - phys
> +  - phy-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/phy/phy.h>
> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
> +
> +    pcie@48400000 {
> +        compatible = "st,stm32mp25-pcie-rc";
> +        device_type = "pci";
> +        num-lanes = <1>;
> +        reg = <0x48400000 0x400000>,
> +              <0x10000000 0x10000>;
> +        reg-names = "dbi", "config";
> +        #interrupt-cells = <1>;
> +        interrupt-map-mask = <0 0 0 7>;
> +        interrupt-map = <0 0 0 1 &intc 0 0 GIC_SPI 264 IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 2 &intc 0 0 GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 3 &intc 0 0 GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,
> +                        <0 0 0 4 &intc 0 0 GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>;
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        ranges = <0x01000000 0 0x10010000 0x10010000 0 0x10000>,
> +                 <0x02000000 0 0x10020000 0x10020000 0 0x7fe0000>,
> +                 <0x42000000 0 0x18000000 0x18000000 0 0x8000000>;
> +        bus-range = <0x00 0xff>;

Don't need this unless it's restricted to less than bus 0-255.

> +        clocks = <&rcc CK_BUS_PCIE>;
> +        clock-names = "core";
> +        phys = <&combophy PHY_TYPE_PCIE>;
> +        phy-names = "pcie-phy";
> +        resets = <&rcc PCIE_R>;
> +        reset-names = "core";
> +        msi-parent = <&v2m0>;
> +        wakeup-source;
> +        wake-gpios = <&gpioh 5 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> +        access-controllers = <&rifsc 76>;
> +        power-domains = <&CLUSTER_PD>;
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] dt-bindings: PCI: Add STM32MP25 PCIe endpoint bindings

[Rob Herring]

On Tue, Nov 12, 2024 at 05:19:23PM +0100, Christian Bruel wrote:
> STM32MP25 PCIe Controller is based on the DesignWare core configured as
> end point mode from the SYSCFG register.
> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  .../bindings/pci/st,stm32-pcie-ep.yaml        | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
> new file mode 100644
> index 000000000000..f0d215982794
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/st,stm32-pcie-ep.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/st,stm32-pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STM32MP25 PCIe endpoint driver
> +
> +maintainers:
> +  - Christian Bruel <christian.bruel@foss.st.com>
> +
> +description:
> +  PCIe endpoint controller based on the Synopsys DesignWare PCIe core.
> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#

snps,dw-pcie-ep.yaml

> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-pcie-ep
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers.
> +      - description: PCIe configuration registers.
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: addr_space
> +
> +  clocks:
> +    maxItems: 1
> +    description: PCIe system clock
> +
> +  clock-names:
> +    const: core
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: core
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  reset-gpios:
> +    description: GPIO controlled connection to PERST# signal
> +    maxItems: 1
> +
> +  access-controllers:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1

All these properties common between RC and EP modes should be in 
a shared schema.

> +
> +required:
> +  - resets
> +  - reset-names
> +  - clocks
> +  - clock-names
> +  - phys
> +  - phy-names
> +  - reset-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/phy/phy.h>
> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
> +
> +    pcie-ep@48400000 {
> +        compatible = "st,stm32mp25-pcie-ep";
> +        num-lanes = <1>;
> +        reg = <0x48400000 0x400000>,
> +              <0x10000000 0x8000000>;
> +        reg-names = "dbi", "addr_space";
> +        clocks = <&rcc CK_BUS_PCIE>;
> +        clock-names = "core";
> +        phys = <&combophy PHY_TYPE_PCIE>;
> +        phy-names = "pcie-phy";
> +        resets = <&rcc PCIE_R>;
> +        reset-names = "core";
> +        pinctrl-names = "default", "init";
> +        pinctrl-0 = <&pcie_pins_a>;
> +        pinctrl-1 = <&pcie_init_pins_a>;
> +        reset-gpios = <&gpioj 8 GPIO_ACTIVE_LOW>;
> +        power-domains = <&CLUSTER_PD>;
> +        access-controllers = <&rifsc 68>;
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH 2/5] PCI: stm32: Add PCIe host support for STM32MP25

[Christian Bruel]

Hello,

On 11/12/24 20:32, Bjorn Helgaas wrote:
> On Tue, Nov 12, 2024 at 05:19:22PM +0100, Christian Bruel wrote:
>> Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
>> DesignWare PCIe core.
>> Supports MSI via GICv2m, Single Virtual Channel, Single Function
> 
> Add blank lines between paragraphs.  Also applies to other patches in
> the series.
> 
>> +config PCIE_STM32
>> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (host mode)"
>> +	depends on ARCH_STM32 || COMPILE_TEST
>> +	depends on PCI_MSI
>> +	select PCIE_DW_HOST
>> +	help
>> +	  Enables support for the DesignWare core based PCIe host controller
>> +	  found in STM32MP25 SoC.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called pcie-stm32.
> 
> Move this so the drivers stay alphabetized.  There's already a
> "STMicroelectronics SPEAr PCIe controller" entry, and this should go
> right next to it.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-stm32.c
> 
>> +static const struct of_device_id stm32_pcie_of_match[] = {
>> +	{ .compatible = "st,stm32mp25-pcie-rc" },
>> +	{},
>> +};
> 
> Most drivers put this near the platform_driver struct that references
> it.
> 
>> +static int stm32_pcie_set_max_payload(struct dw_pcie *pci, int mps)
>> +{
>> +	int ret;
>> +	struct device *dev = pci->dev;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	if (mps != 128 && mps != 256) {
>> +		dev_err(dev, "Unexpected payload size %d\n", mps);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = pcie_set_mps(pdev, mps);
>> +	if (ret)
>> +		dev_err(dev, "failed to set mps %d, error %d\n", mps, ret);
> 
> MPS config is normally not device-specific, and it's somewhat fragile
> (see pci_configure_mps() and pcie_bus_config), so I kind of hate to
> see more users.  Maybe there's some hardware issue involved here?

Indeed that fixed a debugging issue with the first designs.
Not necessary anymore, dropping.

> 
>> +static int stm32_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +	u32 ret;
>> +
>> +	if (stm32_pcie->reset_gpio) {
>> +		/* Make sure PERST# is asserted. */
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);
>> +
>> +		/* Deassert PERST# after 100us */
>> +		usleep_range(100, 200);
> 
> If this is PCIE_T_PERST_CLK_US, use that.  If not, please add a
> relevant #define with a citation to the spec.
> 
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 0);
>> +	}
>> +
>> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +				 STM32MP25_PCIECR_LTSSM_EN,
>> +				 STM32MP25_PCIECR_LTSSM_EN);
>> +
>> +	/*
>> +	 * PCIe specification states that you should not issue any config
>> +	 * requests until 100ms after asserting reset, so we enforce that here
> 
> I think it says 100ms after *deasserting* reset.  But if you use
> PCIE_T_RRS_READY_MS below, I don't think you even need this comment.

ack thanks

> 
>> +	if (stm32_pcie->reset_gpio)
>> +		msleep(100);
> 
> I think this is PCIE_T_RRS_READY_MS.
> 
>> +static void stm32_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +
>> +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);
> 
> With a half-dozen exceptions, this file fits nicely in 80 columns.
> Can you wrap this and the similar exceptions?  No need to break printf
> strings or the regmap strings that can't reasonably be wrapped.
> 
>> +	/* Assert PERST# */
>> +	if (stm32_pcie->reset_gpio)
>> +		gpiod_set_value(stm32_pcie->reset_gpio, 1);
> 
> Might be nice to include "perst" in the "reset_gpio" name to identify
> it more specifically.

ok

> 
>> +}
>> +
>> +static int stm32_pcie_suspend(struct device *dev)
>> +{
>> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
>> +
>> +	if (device_may_wakeup(dev) || device_wakeup_path(dev))
>> +		enable_irq_wake(stm32_pcie->wake_irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_pcie_resume(struct device *dev)
>> +{
>> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
>> +
>> +	if (device_may_wakeup(dev) || device_wakeup_path(dev))
>> +		disable_irq_wake(stm32_pcie->wake_irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_pcie_suspend_noirq(struct device *dev)
>> +{
>> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
>> +
>> +	stm32_pcie->link_is_up = dw_pcie_link_up(stm32_pcie->pci);
>> +
>> +	stm32_pcie_stop_link(stm32_pcie->pci);
>> +	clk_disable_unprepare(stm32_pcie->clk);
>> +
>> +	if (!device_may_wakeup(dev) && !device_wakeup_path(dev))
>> +		phy_exit(stm32_pcie->phy);
>> +
>> +	return pinctrl_pm_select_sleep_state(dev);
>> +}
>> +
>> +static int stm32_pcie_resume_noirq(struct device *dev)
>> +{
>> +	struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
>> +	struct dw_pcie *pci = stm32_pcie->pci;
>> +	struct dw_pcie_rp *pp = &pci->pp;
>> +	int ret;
>> +
>> +	/* init_state was set in pinctrl_bind_pins() before probe */
>> +	if (!IS_ERR(dev->pins->init_state))
>> +		ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
>> +	else
>> +		ret = pinctrl_pm_select_default_state(dev);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!device_may_wakeup(dev) && !device_wakeup_path(dev)) {
>> +		ret = phy_init(stm32_pcie->phy);
>> +		if (ret) {
>> +			pinctrl_pm_select_default_state(dev);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = clk_prepare_enable(stm32_pcie->clk);
>> +	if (ret)
>> +		goto clk_err;
>> +
>> +	ret = stm32_pcie_host_init(pp);
>> +	if (ret)
>> +		goto host_err;
>> +
>> +	ret = dw_pcie_setup_rc(pp);
>> +	if (ret)
>> +		goto pcie_err;
>> +
>> +	if (stm32_pcie->link_is_up) {
>> +		ret = stm32_pcie_start_link(stm32_pcie->pci);
>> +		if (ret)
>> +			goto pcie_err;
>> +
>> +		/* Ignore errors, the link may come up later */
>> +		dw_pcie_wait_for_link(stm32_pcie->pci);
>> +	}
>> +
>> +	pinctrl_pm_select_default_state(dev);
> 
> Interesting that pcie-stm32.c, pci-tegra.c, and pcie-tegra194.c are
> the only PCI controller drivers that use this.  I have no idea what
> this is; it just makes me wonder if these three are just special, or
> if others should be using it?

Here default_state balances with pinctrl_pm_select_sleep_state in 
suspend_noirq. So we should have the same probing sequence:

suspend_noirq()
   pinctrl_pm_select_sleep_state()

resume_noirq()
   pinctrl_select_state(dev->pins->p, dev->pins->init_state);
   ... restart resources and link
   pinctrl_pm_select_default_state()

for the other targets, I have no idea

> 
>> +static int stm32_add_pcie_port(struct stm32_pcie *stm32_pcie,
>> +			       struct platform_device *pdev)
>> +{
>> +	struct dw_pcie *pci = stm32_pcie->pci;
>> +	struct device *dev = pci->dev;
>> +	struct dw_pcie_rp *pp = &pci->pp;
>> +	int ret;
>> +
>> +	ret = phy_set_mode(stm32_pcie->phy, PHY_MODE_PCIE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_init(stm32_pcie->phy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +				 STM32MP25_PCIECR_TYPE_MASK, STM32MP25_PCIECR_RC);
>> +	if (ret)
>> +		goto phy_disable;
>> +
>> +	reset_control_assert(stm32_pcie->rst);
>> +	reset_control_deassert(stm32_pcie->rst);
> 
> Is there any reset pulse width requirement here?

looking at the timing diagram, I don't think so, the transition looks 
quite fast

At the end we will use the reset_control_deassert API to hide this 
mechanism, when the stm32-reset driver is ready

> 
>> +	ret = clk_prepare_enable(stm32_pcie->clk);
>> +	if (ret) {
>> +		dev_err(dev, "Core clock enable failed %d\n", ret);
>> +		goto phy_disable;
>> +	}
>> +
>> +	pp->ops = &stm32_pcie_host_ops;
>> +	ret = dw_pcie_host_init(pp);
>> +	if (ret) {
>> +		dev_err(dev, "failed to initialize host: %d\n", ret);
> 
> Consider making all the messages consistent in terms of sentence
> structure and capitalization.
> 
>> +static int stm32_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct stm32_pcie *stm32_pcie;
>> +	struct dw_pcie *dw;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int ret;
>> +
>> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
>> +	if (!stm32_pcie)
>> +		return -ENOMEM;
>> +
>> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
>> +	if (!dw)
>> +		return -ENOMEM;
> 
> Add blank line.
> 
>> +static struct platform_driver stm32_pcie_driver = {
>> +	.probe = stm32_pcie_probe,
>> +	.remove_new = stm32_pcie_remove,
> 
> Use .remove instead of .remove_new; see 0edb555a65d1 ("platform: Make
> platform_driver::remove() return void").
> 
>> +	.driver = {
>> +		.name = "stm32-pcie",
>> +		.of_match_table = stm32_pcie_of_match,
>> +		.pm		= &stm32_pcie_pm_ops,
>> +	},
>> +};
>> +
>> +static bool is_stm32_pcie_driver(struct device *dev)
>> +{
>> +	/* PCI bridge */
>> +	dev = get_device(dev);
>> +
>> +	/* Platform driver */
>> +	dev = get_device(dev->parent);
>> +
>> +	return (dev->driver == &stm32_pcie_driver.driver);
> 
> Ugh.  Some MPS/MRRS magic going on here, evidently related to the STM
> integration of DWC IP? >
>> +static bool apply_mrrs_quirk(struct pci_dev *root_port)
>> +{
>> +	struct dw_pcie_rp *pp;
>> +	struct dw_pcie *dw_pci;
>> +	struct stm32_pcie *stm32_pcie;
>> +
>> +	if (WARN_ON(!root_port) || !is_stm32_pcie_driver(root_port->dev.parent))
>> +		return false;
>> +
>> +	pp = root_port->bus->sysdata;
>> +	dw_pci = to_dw_pcie_from_pp(pp);
>> +	stm32_pcie = to_stm32_pcie(dw_pci);
>> +
>> +	if (!stm32_pcie->limit_downstream_mrrs)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static void quirk_stm32_pcie_limit_mrrs(struct pci_dev *pci)
>> +{
>> +	struct pci_dev *root_port;
>> +	struct pci_bus *bus = pci->bus;
>> +	int readrq;
>> +	int mps;
>> +
>> +	if (pci_is_root_bus(bus))
>> +		return;
>> +
>> +	root_port = pcie_find_root_port(pci);
>> +
>> +	if (!apply_mrrs_quirk(root_port))
>> +		return;
>> +
>> +	mps = pcie_get_mps(root_port);
>> +
>> +	/*
>> +	 * STM32 PCI controller has a h/w performance limitation on the AXI DDR requests.
>> +	 * Limit the maximum read request size to 256B on all downstream devices.
> 
> I guess this is some kind of platform erratum, since there's no way
> for us to discover a limit on supported MRRS values?

Yes. Those quirk are not necessary anymore. will drop

> 
>> +	readrq = pcie_get_readrq(pci);
>> +	if (readrq > 256) {
>> +		int mrrs = min(mps, 256);
>> +
>> +		pcie_set_readrq(pci, mrrs);
>> +
>> +		pci_info(pci, "Max Read Rq set to %4d (was %4d)\n", mrrs, readrq);
>> +	}
>> +}
>> +
>> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID,
>> +			 quirk_stm32_pcie_limit_mrrs);
>> 
>> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
>> +{
>> +	dev_dbg(&pdev->dev, "set bus_dma_limit");
>> +
>> +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);
> 
> This is quite unusual and deserves some comment about why we need
> it.
> 

32 bus master DMA cannot access the last 2GB in addressing space (out of 
a 6GB addressing space).

Proposing the following comment
"DMA masters can only access the first 4GB of memory space, so setup the 
bus DMA limit accordingly."

Saw a similar quirk in mips/pci/fixup-sb1250.c



>> +	return 0;
>> +}
>> +
>> +static void quirk_stm32_dma_mask(struct pci_dev *pci)
>> +{
>> +	struct pci_dev *root_port;
>> +
>> +	root_port = pcie_find_root_port(pci);
>> +
>> +	if (root_port && (root_port->dev.parent))
>> +		pci_walk_bus(pci->bus, stm32_dma_limit, NULL);
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_SYNOPSYS, 0x0550, quirk_stm32_dma_mask);
> 
> I guess this applies to [16c3:0550] devices and everything below them?
> It looks like these must be Root Ports?  And they identify as
> PCI_VENDOR_ID_SYNOPSYS instead of PCI_VENDOR_ID_STMICRO (104a)?

Yes we are based on the designware v5.50a PCIe version. The idea here is 
to set bus_dma_limit for all devices on the root_port.
is_stm32_pcie_driver is a sanity double check in case the quirk is 
applied to another target linked at compile time

> 
> Could be added at https://admin.pci-ids.ucw.cz/read/PC/16c3/ if you
> want lspci to name them correctly.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-stm32.h
> 
>> +#define STM32MP25_PCIECR_EP		0
> 
> Ideally would go in the patch that uses it.

This is a bit num. Similar to STM32MP25_PCIECR_RC BIT(10), I preper to 
see the definition close to the GENMASK that uses it, making the bit
checking a little bit easier to follow...

> 
>> +#define SYSCFG_PCIEPMEMSICR		0x6004
>> +#define SYSCFG_PCIEAERRCMSICR		0x6008
>> +#define SYSCFG_PCIESR1			0x6100
>> +#define SYSCFG_PCIESR2			0x6104
>> +
>> +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
>> +#define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
> 
> These are all unused, drop them until you need them.

OK, thanks !!

Christian

Re: [PATCH 1/5] dt-bindings: PCI: Add STM32MP25 PCIe root complex bindings

[Christian Bruel]

On 11/15/24 17:36, Rob Herring wrote:
> On Tue, Nov 12, 2024 at 05:19:21PM +0100, Christian Bruel wrote:
>> Document the bindings for STM32MP25 PCIe Controller configured in
>> root complex mode.
>> Supports 4 legacy interrupts and MSI interrupts from the ARM
>> GICv2m controller.
>>
>> Allow tuning to change payload (default 128B) thanks to the
>> st,max-payload-size entry.
>> Can also limit the Maximum Read Request Size on downstream devices to the
>> minimum possible value between 128B and 256B.
>>
>> STM32 PCIE may be in a power domain which is the case for the STM32MP25
>> based boards.
>> Supports wake# from wake-gpios
>>
>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>> ---
>>   .../bindings/pci/st,stm32-pcie-host.yaml      | 149 ++++++++++++++++++
>>   1 file changed, 149 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
>> new file mode 100644
>> index 000000000000..d7d360b63a08
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/st,stm32-pcie-host.yaml
>> @@ -0,0 +1,149 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/st,stm32-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STM32MP25 PCIe root complex driver
>> +
>> +maintainers:
>> +  - Christian Bruel <christian.bruel@foss.st.com>
>> +
>> +description:
>> +  PCIe root complex controller based on the Synopsys DesignWare PCIe core.
>> +
>> +select:
>> +  properties:
>> +    compatible:
>> +      const: st,stm32mp25-pcie-rc
>> +  required:
>> +    - compatible
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
>> +  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#
> 
> snps,dw-pcie.yaml instead of these 2.
> 
>> +
>> +properties:
>> +  compatible:
>> +    const: st,stm32mp25-pcie-rc
>> +
>> +  reg:
>> +    items:
>> +      - description: Data Bus Interface (DBI) registers.
>> +      - description: PCIe configuration registers.
>> +
>> +  reg-names:
>> +    items:
>> +      - const: dbi
>> +      - const: config
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    const: core
> 
> -names with a single entry is kind of pointless.

ok, will update the driver accordinly

> 
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: PCIe system clock
>> +
>> +  clock-names:
>> +    const: core
>> +
>> +  phys:
>> +    maxItems: 1
>> +
>> +  phy-names:
>> +    const: pcie-phy
>> +
>> +  num-lanes:
>> +    const: 1
>> +
>> +  msi-parent:
>> +    maxItems: 1
> 
> Just 'msi-parent: true'. It's already only ever 1 entry.
> 
>> +
>> +  reset-gpios:
>> +    description: GPIO controlled connection to PERST# signal
>> +    maxItems: 1
>> +
>> +  wake-gpios:
>> +    description: GPIO controlled connection to WAKE# input signal
>> +    maxItems: 1
>> +
> 
>> +  st,limit-mrrs:
>> +    description: If present limit downstream MRRS to 256B
>> +    type: boolean
>> +
>> +  st,max-payload-size:
>> +    description: Maximum Payload size to use
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [128, 256]
>> +    default: 128
> 
> IIRC, other hosts have similar restrictions, so you should be able to do
> the same and imply it from the compatible. Though I'm open to a common
> property as Bjorn suggested.
> 
>> +
>> +  wakeup-source: true
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  access-controllers:
>> +    maxItems: 1
>> +
>> +if:
>> +  required:
>> +    - wakeup-source
>> +then:
>> +  required:
>> +    - wake-gpios
> 
> This can be just:
> 
> dependentRequired:
>    wakeup-source: [ wake-gpios ]
> 
> (dependentRequired supercedes dependencies)
> 
>> +
>> +required:
>> +  - interrupt-map
>> +  - interrupt-map-mask
>> +  - ranges
>> +  - resets
>> +  - reset-names
>> +  - clocks
>> +  - clock-names
>> +  - phys
>> +  - phy-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/phy/phy.h>
>> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
>> +
>> +    pcie@48400000 {
>> +        compatible = "st,stm32mp25-pcie-rc";
>> +        device_type = "pci";
>> +        num-lanes = <1>;
>> +        reg = <0x48400000 0x400000>,
>> +              <0x10000000 0x10000>;
>> +        reg-names = "dbi", "config";
>> +        #interrupt-cells = <1>;
>> +        interrupt-map-mask = <0 0 0 7>;
>> +        interrupt-map = <0 0 0 1 &intc 0 0 GIC_SPI 264 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <0 0 0 2 &intc 0 0 GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <0 0 0 3 &intc 0 0 GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,
>> +                        <0 0 0 4 &intc 0 0 GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>;
>> +        #address-cells = <3>;
>> +        #size-cells = <2>;
>> +        ranges = <0x01000000 0 0x10010000 0x10010000 0 0x10000>,
>> +                 <0x02000000 0 0x10020000 0x10020000 0 0x7fe0000>,
>> +                 <0x42000000 0 0x18000000 0x18000000 0 0x8000000>;
>> +        bus-range = <0x00 0xff>;
> 
> Don't need this unless it's restricted to less than bus 0-255.
> 
>> +        clocks = <&rcc CK_BUS_PCIE>;
>> +        clock-names = "core";
>> +        phys = <&combophy PHY_TYPE_PCIE>;
>> +        phy-names = "pcie-phy";
>> +        resets = <&rcc PCIE_R>;
>> +        reset-names = "core";
>> +        msi-parent = <&v2m0>;
>> +        wakeup-source;
>> +        wake-gpios = <&gpioh 5 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
>> +        access-controllers = <&rifsc 76>;
>> +        power-domains = <&CLUSTER_PD>;
>> +    };
>> -- 
>> 2.34.1
>>

thanks

Re: [PATCH 4/5] PCI: stm32: Add PCIe endpoint support for STM32MP25

[Christian Bruel]

On 11/12/24 21:38, Bjorn Helgaas wrote:
> On Tue, Nov 12, 2024 at 05:19:24PM +0100, Christian Bruel wrote:
>> Add driver to configure the STM32MP25 SoC PCIe Gen2 controller based on the
>> DesignWare PCIe core in endpoint mode.
>> Uses the common reference clock provided by the host.
> 
>> +++ b/drivers/pci/controller/dwc/Kconfig
> 
>> +config PCIE_STM32_EP
>> +	tristate "STMicroelectronics STM32MP25 PCIe Controller (endpoint mode)"
>> +	depends on ARCH_STM32 || COMPILE_TEST
>> +	depends on PCI_ENDPOINT
>> +	select PCIE_DW_EP
>> +	help
>> +	  Enables endpoint support for DesignWare core based PCIe controller in found
>> +	  in STM32MP25 SoC.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called pcie-stm32-ep.
> 
> Move as for the host mode entry.
> 
>> +++ b/drivers/pci/controller/dwc/pcie-stm32-ep.c
> 
>> +static const struct of_device_id stm32_pcie_ep_of_match[] = {
>> +	{ .compatible = "st,stm32mp25-pcie-ep" },
>> +	{},
>> +};
> 
> Move next to stm32_pcie_ep_driver.
> 
>> +static void stm32_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +	enum pci_barno bar;
>> +
>> +	for (bar = BAR_0; bar <= PCI_STD_NUM_BARS; bar++)
> 
> Most users just use "bar = 0".  BAR_0 is 0, but there's no real
> connection with PCI_STD_NUM_BARS, so I think 0 is probably better.
> 
> Looks like this should be "bar < PCI_STD_NUM_BARS"?

oops, thanks

> 
>> +		dw_pcie_ep_reset_bar(pci, bar);
>> +
>> +	/* Defer Completion Requests until link started */
> 
> Not sure what a Completion Request is.  Is this some internal STM or
> DWC thing?  Or is this related to Request Retry Status completions for
> config requests?

this is sysconf bit maps to the app_req_retry_en Synopsys controller 
signal. "When app_req_retry_en is asserted, the controller completes 
incoming configuration requess with a CRS"

> 
>> +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +			   STM32MP25_PCIECR_REQ_RETRY_EN,
>> +			   STM32MP25_PCIECR_REQ_RETRY_EN);
>> +}
> 
>> +static int stm32_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +				unsigned int type, u16 interrupt_num)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> +	switch (type) {
>> +	case PCI_IRQ_INTX:
>> +		return dw_pcie_ep_raise_intx_irq(ep, func_no);
>> +	case PCI_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;
>> +	}
>> +
>> +	return 0;
> 
> Is the compiler not smart enough to notice that this is unreachable?
> 
>> +static void stm32_pcie_perst_deassert(struct dw_pcie *pci)
>> +{
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +	struct device *dev = pci->dev;
>> +	struct dw_pcie_ep *ep = &pci->ep;
>> +	int ret;
>> +
>> +	if (stm32_pcie->link_status == STM32_PCIE_EP_LINK_ENABLED) {
>> +		dev_dbg(pci->dev, "Link is already enabled\n");
>> +		return;
>> +	}
>> +
>> +	dev_dbg(dev, "PERST de-asserted by host. Starting link training\n");
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "pm runtime resume failed: %d\n", ret);
>> +		return;
>> +	}
>> +
>> +	ret = stm32_pcie_enable_resources(stm32_pcie);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable resources: %d\n", ret);
>> +		pm_runtime_put_sync(dev);
>> +		return;
>> +	}
>> +
>> +	ret = dw_pcie_ep_init_registers(ep);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to complete initialization: %d\n", ret);
>> +		stm32_pcie_disable_resources(stm32_pcie);
>> +		pm_runtime_put_sync(dev);
>> +		return;
>> +	}
>> +
>> +	pci_epc_init_notify(ep->epc);
>> +
>> +	ret = stm32_pcie_enable_link(pci);
>> +	if (ret) {
>> +		dev_err(dev, "PCIe Cannot establish link: %d\n", ret);
>> +		stm32_pcie_disable_resources(stm32_pcie);
>> +		pm_runtime_put_sync(dev);
>> +		return;
>> +	}
>> +
>> +	stm32_pcie->link_status = STM32_PCIE_EP_LINK_ENABLED;
>> +}
> 
>> +static int stm32_add_pcie_ep(struct stm32_pcie *stm32_pcie,
>> +			     struct platform_device *pdev)
>> +{
>> +	struct dw_pcie *pci = stm32_pcie->pci;
>> +	struct dw_pcie_ep *ep = &pci->ep;
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +				 STM32MP25_PCIECR_TYPE_MASK,
>> +				 STM32MP25_PCIECR_EP);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "pm runtime resume failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	reset_control_assert(stm32_pcie->rst);
>> +	reset_control_deassert(stm32_pcie->rst);
>> +
>> +	ep->ops = &stm32_pcie_ep_ops;
>> +
>> +	ret = dw_pcie_ep_init(ep);
>> +	if (ret) {
>> +		dev_err(dev, "failed to initialize ep: %d\n", ret);
>> +		pm_runtime_put_sync(dev);
>> +		return ret;
>> +	}
>> +
>> +	ret = stm32_pcie_enable_resources(stm32_pcie);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable resources: %d\n", ret);
>> +		dw_pcie_ep_deinit(ep);
>> +		pm_runtime_put_sync(dev);
>> +		return ret;
>> +	}
>> +
>> +	ret = dw_pcie_ep_init_registers(ep);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to initialize DWC endpoint registers\n");
>> +		stm32_pcie_disable_resources(stm32_pcie);
>> +		dw_pcie_ep_deinit(ep);
>> +		pm_runtime_put_sync(dev);
>> +		return ret;
>> +	}
> 
> Consider gotos for the error cases with a cleanup block at the end.
> There's a fair bit of repetition there as more things get initialized,
> and it's error-prone to extend this in the future.
> 
> Same applies in stm32_pcie_perst_deassert().
> 
>> +	pci_epc_init_notify(ep->epc);
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct stm32_pcie *stm32_pcie;
>> +	struct dw_pcie *dw;
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
>> +	if (!stm32_pcie)
>> +		return -ENOMEM;
>> +
>> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
>> +	if (!dw)
>> +		return -ENOMEM;
> 
> Add blank line here.
> 
>> +	stm32_pcie->pci = dw;
> 
>> +static struct platform_driver stm32_pcie_ep_driver = {
>> +	.probe = stm32_pcie_probe,
>> +	.remove_new = stm32_pcie_remove,
> 
> .remove().
> 
>> +	.driver = {
>> +		.name = "stm32-ep-pcie",
>> +		.of_match_table = stm32_pcie_ep_of_match,
>> +	},
>> +};