Hi Manivannan,
On 4/30/25 09:30, Manivannan Sadhasivam wrote:
On Wed, Apr 23, 2025 at 11:01:12AM +0200, Christian Bruel wrote:
Add driver for the STM32MP25 SoC PCIe Gen1 2.5 GT/s and Gen2 5GT/s
controller based on the DesignWare PCIe core.
Supports MSI via GICv2m, Single Virtual Channel, Single Function
Supports WAKE# GPIO.
Mostly looks good. Just a couple of comments below.
Signed-off-by: Christian Bruel <christian.bruel@xxxxxxxxxxx>
---
drivers/pci/controller/dwc/Kconfig | 12 +
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-stm32.c | 368 ++++++++++++++++++++++++
drivers/pci/controller/dwc/pcie-stm32.h | 15 +
4 files changed, 396 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-stm32.c
create mode 100644 drivers/pci/controller/dwc/pcie-stm32.h
[...]
+static int stm32_pcie_probe(struct platform_device *pdev)
+{
+ struct stm32_pcie *stm32_pcie;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
+ if (!stm32_pcie)
+ return -ENOMEM;
+
+ stm32_pcie->pci.dev = dev;
+ stm32_pcie->pci.ops = &dw_pcie_ops;
+ stm32_pcie->pci.pp.ops = &stm32_pcie_host_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->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");
+
+ ret = stm32_pcie_parse_port(stm32_pcie);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, stm32_pcie);
+
+ ret = pm_runtime_set_active(dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to activate runtime PM %d\n", ret);
Please use dev_err_probe() here and below.
OK, will report this in the EP driver also.
+ return ret;
+ }
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to enable runtime PM %d\n", ret);
+ return ret;
+ }
+
+ pm_runtime_get_noresume(dev);
+
I know that a lot of the controller drivers do this for no obvious reason. But
in this case, I believe you want to enable power domain or genpd before
registering the host bridge. Is that right?
We call pm_runtime_enable() before stm32_add_pcie_port() and
dw_pcie_host_init(). This ensures that PCIe is active during the PERST#
sequence and before accessing the DWC registers.
> And the fact that you are not
decrementing the runtime usage count means, you want to keep it ON all the time?
Beware that your system suspend/resume calls would never get executed.
We do not support PM runtime autosuspend, so we must notify PM runtime
that PCIe is always active. Without invoking pm_runtime_get_noresume(),
PCIe would mistakenly be marked as suspended.
The function stm32_pcie_suspend_noirq() is triggered when the system
transitions to Stop or Standby power states, (from /sys/power/state)
Also in any case, you need to call this before devm_pm_runtime_enable().
Otherwise, PM core will suspend the parent and enable it during
pm_runtime_get_noresume(), which is redundant.
OK, I will invert the calls.
Thank you
Christian
- Mani
