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      |  12 +
 drivers/pci/controller/dwc/Makefile     |   1 +
 drivers/pci/controller/dwc/pcie-stm32.c | 402 ++++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-stm32.h |  15 +
 4 files changed, 430 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..0c18879b604c 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -389,6 +389,18 @@ config PCIE_SPEAR13XX
 	help
 	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
 
+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.
+
 config PCI_DRA7XX
 	tristate
 
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..fa787406c0e4
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-stm32.c
@@ -0,0 +1,402 @@
+// 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"
+#include "../../pci.h"
+
+struct stm32_pcie {
+	struct dw_pcie *pci;
+	struct regmap *regmap;
+	struct reset_control *rst;
+	struct phy *phy;
+	struct clk *clk;
+	struct gpio_desc *perst_gpio;
+	struct gpio_desc *wake_gpio;
+	unsigned int wake_irq;
+	bool link_is_up;
+};
+
+static int stm32_pcie_start_link(struct dw_pcie *pci)
+{
+	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
+	u32 ret;
+
+	if (stm32_pcie->perst_gpio) {
+		/* Make sure PERST# is asserted. */
+		gpiod_set_value(stm32_pcie->perst_gpio, 1);
+
+		fsleep(PCIE_T_PERST_CLK_US);
+		gpiod_set_value(stm32_pcie->perst_gpio, 0);
+	}
+
+	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
+				 STM32MP25_PCIECR_LTSSM_EN,
+				 STM32MP25_PCIECR_LTSSM_EN);
+
+	if (stm32_pcie->perst_gpio)
+		msleep(PCIE_T_RRS_READY_MS);
+
+	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->perst_gpio)
+		gpiod_set_value(stm32_pcie->perst_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 must be called first to force clk_req# gpio when no
+	 * device is plugged.
+	 */
+	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 = 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);
+	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 = {
+};
+
+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, NULL);
+	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, NULL);
+	if (IS_ERR(stm32_pcie->rst))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->rst),
+				     "Failed to get PCIe reset\n");
+
+	stm32_pcie->perst_gpio = devm_gpiod_get_optional(dev, "reset",
+							 GPIOD_OUT_HIGH);
+	if (IS_ERR(stm32_pcie->perst_gpio))
+		return dev_err_probe(dev, PTR_ERR(stm32_pcie->perst_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 runtime PM %d\n", ret);
+		return ret;
+	}
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get runtime PM %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 const struct of_device_id stm32_pcie_of_match[] = {
+	{ .compatible = "st,stm32mp25-pcie-rc" },
+	{},
+};
+
+static struct platform_driver stm32_pcie_driver = {
+	.probe = stm32_pcie_probe,
+	.remove = 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);
+}
+
+/*
+ * DMA masters can only access the first 4GB of memory space,
+ * so we setup the bus DMA limit accordingly.
+ */
+static int stm32_dma_limit(struct pci_dev *pdev, void *data)
+{
+	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
+
+	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..3efd00937d3d
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-stm32.h
@@ -0,0 +1,15 @@
+/* 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_LTSSM_EN	BIT(2)
+#define STM32MP25_PCIECR_RC		BIT(10)
+
+#define SYSCFG_PCIECR			0x6000
-- 
2.34.1

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.13-rc1 next-20241206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Bruel/dt-bindings-PCI-Add-STM32MP25-PCIe-root-complex-bindings/20241128-101958
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241126155119.1574564-3-christian.bruel%40foss.st.com
patch subject: [PATCH v2 2/5] PCI: stm32: Add PCIe host support for STM32MP25
config: openrisc-randconfig-r072-20241208 (https://download.01.org/0day-ci/archive/20241208/202412080849.1SXhxzpi-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241208/202412080849.1SXhxzpi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412080849.1SXhxzpi-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/controller/dwc/pcie-stm32.c: In function 'stm32_pcie_suspend_noirq':
>> drivers/pci/controller/dwc/pcie-stm32.c:101:16: error: implicit declaration of function 'pinctrl_pm_select_sleep_state' [-Wimplicit-function-declaration]
     101 |         return pinctrl_pm_select_sleep_state(dev);
         |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-stm32.c: In function 'stm32_pcie_resume_noirq':
>> drivers/pci/controller/dwc/pcie-stm32.c:114:24: error: 'struct device' has no member named 'pins'
     114 |         if (!IS_ERR(dev->pins->init_state))
         |                        ^~
>> drivers/pci/controller/dwc/pcie-stm32.c:115:23: error: implicit declaration of function 'pinctrl_select_state' [-Wimplicit-function-declaration]
     115 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
         |                       ^~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-stm32.c:115:47: error: 'struct device' has no member named 'pins'
     115 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
         |                                               ^~
   drivers/pci/controller/dwc/pcie-stm32.c:115:61: error: 'struct device' has no member named 'pins'
     115 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
         |                                                             ^~
>> drivers/pci/controller/dwc/pcie-stm32.c:117:23: error: implicit declaration of function 'pinctrl_pm_select_default_state' [-Wimplicit-function-declaration]
     117 |                 ret = pinctrl_pm_select_default_state(dev);
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-stm32.c: In function 'stm32_pcie_probe':
   drivers/pci/controller/dwc/pcie-stm32.c:243:29: warning: unused variable 'np' [-Wunused-variable]
     243 |         struct device_node *np = pdev->dev.of_node;
         |                             ^~


vim +/pinctrl_pm_select_sleep_state +101 drivers/pci/controller/dwc/pcie-stm32.c

    88	
    89	static int stm32_pcie_suspend_noirq(struct device *dev)
    90	{
    91		struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
    92	
    93		stm32_pcie->link_is_up = dw_pcie_link_up(stm32_pcie->pci);
    94	
    95		stm32_pcie_stop_link(stm32_pcie->pci);
    96		clk_disable_unprepare(stm32_pcie->clk);
    97	
    98		if (!device_may_wakeup(dev) && !device_wakeup_path(dev))
    99			phy_exit(stm32_pcie->phy);
   100	
 > 101		return pinctrl_pm_select_sleep_state(dev);
   102	}
   103	
   104	static int stm32_pcie_resume_noirq(struct device *dev)
   105	{
   106		struct stm32_pcie *stm32_pcie = dev_get_drvdata(dev);
   107		struct dw_pcie *pci = stm32_pcie->pci;
   108		struct dw_pcie_rp *pp = &pci->pp;
   109		int ret;
   110	
   111		/* init_state must be called first to force clk_req# gpio when no
   112		 * device is plugged.
   113		 */
 > 114		if (!IS_ERR(dev->pins->init_state))
 > 115			ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
   116		else
 > 117			ret = pinctrl_pm_select_default_state(dev);
   118	
   119		if (ret) {
   120			dev_err(dev, "Failed to activate pinctrl pm state: %d\n", ret);
   121			return ret;
   122		}
   123	
   124		if (!device_may_wakeup(dev) && !device_wakeup_path(dev)) {
   125			ret = phy_init(stm32_pcie->phy);
   126			if (ret) {
   127				pinctrl_pm_select_default_state(dev);
   128				return ret;
   129			}
   130		}
   131	
   132		ret = clk_prepare_enable(stm32_pcie->clk);
   133		if (ret)
   134			goto clk_err;
   135	
   136		ret = dw_pcie_setup_rc(pp);
   137		if (ret)
   138			goto pcie_err;
   139	
   140		if (stm32_pcie->link_is_up) {
   141			ret = stm32_pcie_start_link(stm32_pcie->pci);
   142			if (ret)
   143				goto pcie_err;
   144	
   145			/* Ignore errors, the link may come up later */
   146			dw_pcie_wait_for_link(stm32_pcie->pci);
   147		}
   148	
   149		pinctrl_pm_select_default_state(dev);
   150	
   151		return 0;
   152	
   153	pcie_err:
   154		dw_pcie_host_deinit(pp);
   155		clk_disable_unprepare(stm32_pcie->clk);
   156	clk_err:
   157		phy_exit(stm32_pcie->phy);
   158		pinctrl_pm_select_default_state(dev);
   159	
   160		return ret;
   161	}
   162	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Tue, Nov 26, 2024 at 04:51:16PM +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
> 
> 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.c | 402 ++++++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-stm32.h |  15 +
>  4 files changed, 430 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..0c18879b604c 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -389,6 +389,18 @@ config PCIE_SPEAR13XX
>  	help
>  	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
>  
> +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.
> +
>  config PCI_DRA7XX
>  	tristate
>  
> 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..fa787406c0e4
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-stm32.c
> @@ -0,0 +1,402 @@
> +// 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"
> +#include "../../pci.h"
> +
> +struct stm32_pcie {
> +	struct dw_pcie *pci;
> +	struct regmap *regmap;
> +	struct reset_control *rst;
> +	struct phy *phy;
> +	struct clk *clk;
> +	struct gpio_desc *perst_gpio;
> +	struct gpio_desc *wake_gpio;
> +	unsigned int wake_irq;
> +	bool link_is_up;
> +};
> +
> +static int stm32_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> +	u32 ret;
> +
> +	if (stm32_pcie->perst_gpio) {

gpiod_set_value() will bail out if 'perst_gpio' is NULL. So no need to add a
check.

> +		/* Make sure PERST# is asserted. */

Why?

> +		gpiod_set_value(stm32_pcie->perst_gpio, 1);
> +
> +		fsleep(PCIE_T_PERST_CLK_US);
> +		gpiod_set_value(stm32_pcie->perst_gpio, 0);

Why can't you deassert PERST# in stm32_add_pcie_port()?

> +	}
> +
> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> +				 STM32MP25_PCIECR_LTSSM_EN,
> +				 STM32MP25_PCIECR_LTSSM_EN);
> +
> +	if (stm32_pcie->perst_gpio)

It doesn't hurt to call msleep() always.

> +		msleep(PCIE_T_RRS_READY_MS);
> +
> +	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->perst_gpio)
> +		gpiod_set_value(stm32_pcie->perst_gpio, 1);

I don't like tying PERST# handling with start/stop link. PERST# should be
handled based on the power/clock state.

> +}
> +
> +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);

I don't understand how endpoint can wakeup the host if PERST# gets asserted.

> +	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 must be called first to force clk_req# gpio when no

CLKREQ#

Why RC should control CLKREQ#?

Also please use preferred style for multi-line comments:

	/*
	 * ...
	 */

> +	 * device is plugged.
> +	 */
> +	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;

Please name the goto labels of their purpose. Like err_phy_exit.

> +
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto pcie_err;

This should be, 'err_disable_clk'.

> +
> +	if (stm32_pcie->link_is_up) {

Why do you need this check? You cannot start the link in the absence of an
endpoint?

> +		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);
> +	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)

Can you make use of dw_pcie_{suspend/resume}_noirq() APIs?

> +	SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend, stm32_pcie_resume)
> +};
> +
> +static const struct dw_pcie_host_ops stm32_pcie_host_ops = {
> +};
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = stm32_pcie_start_link,
> +	.stop_link = stm32_pcie_stop_link
> +};
> +

[...]

> +	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);
> +	}

Can we move WAKE# gpio handling to DWC core? There is nothing STM32 specific
here.

> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable runtime PM %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get runtime PM %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 const struct of_device_id stm32_pcie_of_match[] = {
> +	{ .compatible = "st,stm32mp25-pcie-rc" },
> +	{},
> +};
> +
> +static struct platform_driver stm32_pcie_driver = {
> +	.probe = stm32_pcie_probe,
> +	.remove = stm32_pcie_remove,
> +	.driver = {
> +		.name = "stm32-pcie",
> +		.of_match_table = stm32_pcie_of_match,
> +		.pm		= &stm32_pcie_pm_ops,

Just use a single space instead of tab.

Could you also set PROBE_PREFER_ASYNCHRONOUS to allow async probing of
controller drivers?

> +	},
> +};
> +
> +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);
> +}
> +
> +/*
> + * DMA masters can only access the first 4GB of memory space,
> + * so we setup the bus DMA limit accordingly.
> + */
> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
> +{
> +	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
> +
> +	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);

This is not the correct way of using DMA masks as Bjorn noted.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Hi Manivannan,

On 12/3/24 15:52, Manivannan Sadhasivam wrote:
> On Tue, Nov 26, 2024 at 04:51:16PM +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
>>
>> 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.c | 402 ++++++++++++++++++++++++
>>   drivers/pci/controller/dwc/pcie-stm32.h |  15 +
>>   4 files changed, 430 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..0c18879b604c 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -389,6 +389,18 @@ config PCIE_SPEAR13XX
>>   	help
>>   	  Say Y here if you want PCIe support on SPEAr13XX SoCs.
>>   
>> +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.
>> +
>>   config PCI_DRA7XX
>>   	tristate
>>   
>> 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..fa787406c0e4
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-stm32.c
>> @@ -0,0 +1,402 @@
>> +// 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"
>> +#include "../../pci.h"
>> +
>> +struct stm32_pcie {
>> +	struct dw_pcie *pci;
>> +	struct regmap *regmap;
>> +	struct reset_control *rst;
>> +	struct phy *phy;
>> +	struct clk *clk;
>> +	struct gpio_desc *perst_gpio;
>> +	struct gpio_desc *wake_gpio;
>> +	unsigned int wake_irq;
>> +	bool link_is_up;
>> +};
>> +
>> +static int stm32_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
>> +	u32 ret;
>> +
>> +	if (stm32_pcie->perst_gpio) {
> 
> gpiod_set_value() will bail out if 'perst_gpio' is NULL. So no need to add a
> check.

OK

> 
>> +		/* Make sure PERST# is asserted. */
> 
> Why?

Ack. We have GPIOD_OUT_HIGH already thanks devm_gpiod_get_optional, so 
PERST# is asserted already at probe


> 
>> +		gpiod_set_value(stm32_pcie->perst_gpio, 1);
>> +
>> +		fsleep(PCIE_T_PERST_CLK_US);
>> +		gpiod_set_value(stm32_pcie->perst_gpio, 0);
> 
> Why can't you deassert PERST# in stm32_add_pcie_port()?

Code reuse between probe and resume. (Need de-assert PERST# after the 
refclk is stable from both probe or resume_noirq paths).
Can move this from start_link to:
  - probe after the phy_init
  - and resume_no_irq after the phy_init
before dw_pcie_setup_rc

> 
>> +	}
>> +
>> +	ret = regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
>> +				 STM32MP25_PCIECR_LTSSM_EN,
>> +				 STM32MP25_PCIECR_LTSSM_EN);
>> +
>> +	if (stm32_pcie->perst_gpio)
> 
> It doesn't hurt to call msleep() always.

It is optional in the DT. Why wait if we don't have a reset signal ?

> 
>> +		msleep(PCIE_T_RRS_READY_MS);
>> +
>> +	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->perst_gpio)
>> +		gpiod_set_value(stm32_pcie->perst_gpio, 1);
> 
> I don't like tying PERST# handling with start/stop link. PERST# should be
> handled based on the power/clock state.

I don't understand your point: We turn off the PHY in suspend_noirq(), 
so that seems a logical place to de-assert in resume_noirq after the 
refclk is ready. PERST# should be kept active until the PHY stablilizes 
the clock in resume. From the PCIe electromechanical specs, PERST# goes 
active while the refclk is not stable/


> 
>> +}
>> +
>> +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);
> 
> I don't understand how endpoint can wakeup the host if PERST# gets asserted.

The stm32 PCIe doesn't support L2, we don't expect an in-band beacon for 
the wakeup. We support wakeup only from sideband WAKE#, that will 
restart the link from IRQ

> 
>> +	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 must be called first to force clk_req# gpio when no
> 
> CLKREQ#
> 
> Why RC should control CLKREQ#?

REFCLK is gated with CLKREQ#, So we cannot access the core
without CLKREQ# if no device is present. So force it with a init pinmux
the time to init the PHY and the core DBI registers

> 
> Also please use preferred style for multi-line comments:
> 
> 	/*
> 	 * ...
> 	 */
> 
>> +	 * device is plugged.
>> +	 */
>> +	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;
> 
> Please name the goto labels of their purpose. Like err_phy_exit.

OK

> 
>> +
>> +	ret = dw_pcie_setup_rc(pp);
>> +	if (ret)
>> +		goto pcie_err;
> 
> This should be, 'err_disable_clk'.
> 
>> +
>> +	if (stm32_pcie->link_is_up) {
> 
> Why do you need this check? You cannot start the link in the absence of an
> endpoint?
> 

It is an optimization to avoid unnecessary "dw_pcie_wait_for_link" if no 
device is present during suspend


>> +		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);
>> +	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)
> 
> Can you make use of dw_pcie_{suspend/resume}_noirq() APIs?

dw_pcie_suspend_noirq bails out with read_poll_timeout on ltssm = 
DW_PCIE_LTSS_L2_IDLE. We don't support L2.

> 
>> +	SYSTEM_SLEEP_PM_OPS(stm32_pcie_suspend, stm32_pcie_resume)
>> +};
>> +
>> +static const struct dw_pcie_host_ops stm32_pcie_host_ops = {
>> +};
>> +
>> +static const struct dw_pcie_ops dw_pcie_ops = {
>> +	.start_link = stm32_pcie_start_link,
>> +	.stop_link = stm32_pcie_stop_link
>> +};
>> +
> 
> [...]
> 
>> +	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);
>> +	}
> 
> Can we move WAKE# gpio handling to DWC core? There is nothing STM32 specific
> here.
> 

OK

>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to enable runtime PM %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to get runtime PM %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 const struct of_device_id stm32_pcie_of_match[] = {
>> +	{ .compatible = "st,stm32mp25-pcie-rc" },
>> +	{},
>> +};
>> +
>> +static struct platform_driver stm32_pcie_driver = {
>> +	.probe = stm32_pcie_probe,
>> +	.remove = stm32_pcie_remove,
>> +	.driver = {
>> +		.name = "stm32-pcie",
>> +		.of_match_table = stm32_pcie_of_match,
>> +		.pm		= &stm32_pcie_pm_ops,
> 
> Just use a single space instead of tab.
> 
> Could you also set PROBE_PREFER_ASYNCHRONOUS to allow async probing of
> controller drivers?

OK

> 
>> +	},
>> +};
>> +
>> +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);
>> +}
>> +
>> +/*
>> + * DMA masters can only access the first 4GB of memory space,
>> + * so we setup the bus DMA limit accordingly.
>> + */
>> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
>> +{
>> +	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
>> +
>> +	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);
> 
> This is not the correct way of using DMA masks as Bjorn noted.

Yes, fixed will dma-ranges

thanks

Christian

> 
> - Mani
>

On Mon, Dec 16, 2024 at 10:00:27AM +0100, Christian Bruel wrote:

[...]

> > 
> > > +		msleep(PCIE_T_RRS_READY_MS);
> > > +
> > > +	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->perst_gpio)
> > > +		gpiod_set_value(stm32_pcie->perst_gpio, 1);
> > 
> > I don't like tying PERST# handling with start/stop link. PERST# should be
> > handled based on the power/clock state.
> 
> I don't understand your point: We turn off the PHY in suspend_noirq(), so
> that seems a logical place to de-assert in resume_noirq after the refclk is
> ready. PERST# should be kept active until the PHY stablilizes the clock in
> resume. From the PCIe electromechanical specs, PERST# goes active while the
> refclk is not stable/
> 

While your understanding about PERST# is correct, your implementation is not.
You are toggling PERST# from start/stop link callbacks which are supposed to
control the LTSSM state only. I don't have issues with toggling PERST# in
stm32_pcie_{suspend/resume}_noirq().

> 
> > 
> > > +}
> > > +
> > > +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);
> > 
> > I don't understand how endpoint can wakeup the host if PERST# gets asserted.
> 
> The stm32 PCIe doesn't support L2, we don't expect an in-band beacon for the
> wakeup. We support wakeup only from sideband WAKE#, that will restart the
> link from IRQ
> 

I don't understand how WAKE# is supported without L2. Only in L2 state, endpoint
will make use of Vaux and it will wakeup the host using either beacon or WAKE#.
If you don't support L2, then the endpoint will reach L3 (link off) state.

> > 
> > > +	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 must be called first to force clk_req# gpio when no
> > 
> > CLKREQ#
> > 
> > Why RC should control CLKREQ#?
> 
> REFCLK is gated with CLKREQ#, So we cannot access the core
> without CLKREQ# if no device is present. So force it with a init pinmux
> the time to init the PHY and the core DBI registers
> 

Ok. You should add a comment to clarify it in the code as this is not a standard
behavior.

> > 
> > Also please use preferred style for multi-line comments:
> > 
> > 	/*
> > 	 * ...
> > 	 */
> > 
> > > +	 * device is plugged.
> > > +	 */
> > > +	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;
> > 
> > Please name the goto labels of their purpose. Like err_phy_exit.
> 
> OK
> 
> > 
> > > +
> > > +	ret = dw_pcie_setup_rc(pp);
> > > +	if (ret)
> > > +		goto pcie_err;
> > 
> > This should be, 'err_disable_clk'.
> > 
> > > +
> > > +	if (stm32_pcie->link_is_up) {
> > 
> > Why do you need this check? You cannot start the link in the absence of an
> > endpoint?
> > 
> 
> It is an optimization to avoid unnecessary "dw_pcie_wait_for_link" if no
> device is present during suspend
> 

In that case you'll not trigger LTSSM if there was no endpoint connected before
suspend. What if users connect an endpoint after resume?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On 12/18/24 10:46, Manivannan Sadhasivam wrote:
> On Mon, Dec 16, 2024 at 10:00:27AM +0100, Christian Bruel wrote:
> 
> [...]
> 
>>>
>>>> +		msleep(PCIE_T_RRS_READY_MS);
>>>> +
>>>> +	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->perst_gpio)
>>>> +		gpiod_set_value(stm32_pcie->perst_gpio, 1);
>>>
>>> I don't like tying PERST# handling with start/stop link. PERST# should be
>>> handled based on the power/clock state.
>>
>> I don't understand your point: We turn off the PHY in suspend_noirq(), so
>> that seems a logical place to de-assert in resume_noirq after the refclk is
>> ready. PERST# should be kept active until the PHY stablilizes the clock in
>> resume. From the PCIe electromechanical specs, PERST# goes active while the
>> refclk is not stable/
>>
> 
> While your understanding about PERST# is correct, your implementation is not.
> You are toggling PERST# from start/stop link callbacks which are supposed to
> control the LTSSM state only. I don't have issues with toggling PERST# in
> stm32_pcie_{suspend/resume}_noirq().

Ah OK. this function is split now into 2 functional blocks in the 
upcoming version

> 
>>
>>>
>>>> +}
>>>> +
>>>> +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);
>>>
>>> I don't understand how endpoint can wakeup the host if PERST# gets asserted.
>>
>> The stm32 PCIe doesn't support L2, we don't expect an in-band beacon for the
>> wakeup. We support wakeup only from sideband WAKE#, that will restart the
>> link from IRQ
>>
> 
> I don't understand how WAKE# is supported without L2. Only in L2 state, endpoint
> will make use of Vaux and it will wakeup the host using either beacon or WAKE#.
> If you don't support L2, then the endpoint will reach L3 (link off) state.

I think this is what happens, my understanding is that the device is 
still powered to get the wakeup event (eg WoL magic packet), triggers 
the PCIe wake_IRQ from the WAKE#.

> 
>>>
>>>> +	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 must be called first to force clk_req# gpio when no
>>>
>>> CLKREQ#
>>>
>>> Why RC should control CLKREQ#?
>>
>> REFCLK is gated with CLKREQ#, So we cannot access the core
>> without CLKREQ# if no device is present. So force it with a init pinmux
>> the time to init the PHY and the core DBI registers
>>
> 
> Ok. You should add a comment to clarify it in the code as this is not a standard
> behavior.
> 

OK

>>>
>>> Also please use preferred style for multi-line comments:
>>>
>>> 	/*
>>> 	 * ...
>>> 	 */
>>>
>>>> +	 * device is plugged.
>>>> +	 */
>>>> +	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;
>>>
>>> Please name the goto labels of their purpose. Like err_phy_exit.
>>
>> OK
>>
>>>
>>>> +
>>>> +	ret = dw_pcie_setup_rc(pp);
>>>> +	if (ret)
>>>> +		goto pcie_err;
>>>
>>> This should be, 'err_disable_clk'.
>>>
>>>> +
>>>> +	if (stm32_pcie->link_is_up) {
>>>
>>> Why do you need this check? You cannot start the link in the absence of an
>>> endpoint?
>>>
>>
>> It is an optimization to avoid unnecessary "dw_pcie_wait_for_link" if no
>> device is present during suspend
>>
> 
> In that case you'll not trigger LTSSM if there was no endpoint connected before
> suspend. What if users connect an endpoint after resume?

Yes, exactly. We don't support hotplug, and plugging a device while the 
system is in stand-by is something that we don't expect. The imx6 
platform does this also.

thanks,

Christian

> 
> - Mani
>

On Wed, Dec 18, 2024 at 12:24:21PM +0100, Christian Bruel wrote:

[...]

> > > > > +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);
> > > > 
> > > > I don't understand how endpoint can wakeup the host if PERST# gets asserted.
> > > 
> > > The stm32 PCIe doesn't support L2, we don't expect an in-band beacon for the
> > > wakeup. We support wakeup only from sideband WAKE#, that will restart the
> > > link from IRQ
> > > 
> > 
> > I don't understand how WAKE# is supported without L2. Only in L2 state, endpoint
> > will make use of Vaux and it will wakeup the host using either beacon or WAKE#.
> > If you don't support L2, then the endpoint will reach L3 (link off) state.
> 
> I think this is what happens, my understanding is that the device is still
> powered to get the wakeup event (eg WoL magic packet), triggers the PCIe
> wake_IRQ from the WAKE#.
> 

Honestly, I still cannot understand how this can happen.

> > 
> > > > 
> > > > > +	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 must be called first to force clk_req# gpio when no
> > > > 
> > > > CLKREQ#
> > > > 
> > > > Why RC should control CLKREQ#?
> > > 
> > > REFCLK is gated with CLKREQ#, So we cannot access the core
> > > without CLKREQ# if no device is present. So force it with a init pinmux
> > > the time to init the PHY and the core DBI registers
> > > 
> > 
> > Ok. You should add a comment to clarify it in the code as this is not a standard
> > behavior.
> > 
> 
> OK
> 
> > > > 
> > > > Also please use preferred style for multi-line comments:
> > > > 
> > > > 	/*
> > > > 	 * ...
> > > > 	 */
> > > > 
> > > > > +	 * device is plugged.
> > > > > +	 */
> > > > > +	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;
> > > > 
> > > > Please name the goto labels of their purpose. Like err_phy_exit.
> > > 
> > > OK
> > > 
> > > > 
> > > > > +
> > > > > +	ret = dw_pcie_setup_rc(pp);
> > > > > +	if (ret)
> > > > > +		goto pcie_err;
> > > > 
> > > > This should be, 'err_disable_clk'.
> > > > 
> > > > > +
> > > > > +	if (stm32_pcie->link_is_up) {
> > > > 
> > > > Why do you need this check? You cannot start the link in the absence of an
> > > > endpoint?
> > > > 
> > > 
> > > It is an optimization to avoid unnecessary "dw_pcie_wait_for_link" if no
> > > device is present during suspend
> > > 
> > 
> > In that case you'll not trigger LTSSM if there was no endpoint connected before
> > suspend. What if users connect an endpoint after resume?
> 
> Yes, exactly. We don't support hotplug, and plugging a device while the
> system is in stand-by is something that we don't expect. The imx6 platform
> does this also.
> 

You should reconsider this approach. You'll never know how the OEMs are going to
use the PCIe slot. And lack of standard hotplug is not preventing users from
doing hotplug (they could try to do rescan themselves etc...)

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[+to Rob, DMA mask question]

On Tue, Nov 26, 2024 at 04:51:16PM +0100, Christian Bruel wrote:
> Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
> DesignWare PCIe core.

Can you include the numeric rate, not just "gen2", so we don't have to
search for it?

> +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 must be called first to force clk_req# gpio when no
> +	 * device is plugged.
> +	 */

Use drivers/pci/ conventional comment style:

  /*
   * text ...
   */

> +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);
> +}
> +
> +/*
> + * DMA masters can only access the first 4GB of memory space,
> + * so we setup the bus DMA limit accordingly.
> + */
> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
> +{
> +	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
> +
> +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);

I don't think this is the right way to do this.  Surely there's a way
to describe the DMA capability of the bridge once instead of iterating
over all the downstream devices?  This quirk can't work for hot-added
devices anyway.

> +	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);

Am Freitag, dem 29.11.2024 um 14:58 -0600 schrieb Bjorn Helgaas:
> [+to Rob, DMA mask question]
> 
> On Tue, Nov 26, 2024 at 04:51:16PM +0100, Christian Bruel wrote:
> > Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
> > DesignWare PCIe core.
> 
> Can you include the numeric rate, not just "gen2", so we don't have to
> search for it?
> 
> > +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 must be called first to force clk_req# gpio when no
> > +	 * device is plugged.
> > +	 */
> 
> Use drivers/pci/ conventional comment style:
> 
>   /*
>    * text ...
>    */
> 
> > +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);
> > +}
> > +
> > +/*
> > + * DMA masters can only access the first 4GB of memory space,
> > + * so we setup the bus DMA limit accordingly.
> > + */
> > +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
> > +{
> > +	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
> > +
> > +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);
> 
> I don't think this is the right way to do this.  Surely there's a way
> to describe the DMA capability of the bridge once instead of iterating
> over all the downstream devices?  This quirk can't work for hot-added
> devices anyway.
> 
This should simply be a dma-ranges property in the PCIe host controller
DT node, which should describe the DMA address range limits for
transactions passing through the host.

Regards,
Lucas

> > +	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);
> 

Hello Bjorn and Lucas,

On 11/29/24 22:18, Lucas Stach wrote:
> Am Freitag, dem 29.11.2024 um 14:58 -0600 schrieb Bjorn Helgaas:
>> [+to Rob, DMA mask question]
>>
>> On Tue, Nov 26, 2024 at 04:51:16PM +0100, Christian Bruel wrote:
>>> Add driver for the STM32MP25 SoC PCIe Gen2 controller based on the
>>> DesignWare PCIe core.
>>
>> Can you include the numeric rate, not just "gen2", so we don't have to
>> search for it?
>>
>>> +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 must be called first to force clk_req# gpio when no
>>> +	 * device is plugged.
>>> +	 */
>>
>> Use drivers/pci/ conventional comment style:
>>
>>    /*
>>     * text ...
>>     */
>>
>>> +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);
>>> +}
>>> +
>>> +/*
>>> + * DMA masters can only access the first 4GB of memory space,
>>> + * so we setup the bus DMA limit accordingly.
>>> + */
>>> +static int stm32_dma_limit(struct pci_dev *pdev, void *data)
>>> +{
>>> +	dev_dbg(&pdev->dev, "disabling DMA DAC for device");
>>> +
>>> +	pdev->dev.bus_dma_limit = DMA_BIT_MASK(32);
>>
>> I don't think this is the right way to do this.  Surely there's a way
>> to describe the DMA capability of the bridge once instead of iterating
>> over all the downstream devices?  This quirk can't work for hot-added
>> devices anyway.
>>

agree,

> This should simply be a dma-ranges property in the PCIe host controller
> DT node, which should describe the DMA address range limits for
> transactions passing through the host.

far better indeed, dma-ranges works like a charm

thanks,

> 
> Regards,
> Lucas
> 
>>> +	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);
>>
>