On Di, 2025-07-15 at 11:43 +0800, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.
>
> Signed-off-by: Jacky Chou <jacky_chou@xxxxxxxxxxxxxx>
> ---
> drivers/pci/controller/Kconfig | 13 +
> drivers/pci/controller/Makefile | 1 +
> drivers/pci/controller/pcie-aspeed.c | 1137 ++++++++++++++++++++++++++
> 3 files changed, 1151 insertions(+)
> create mode 100644 drivers/pci/controller/pcie-aspeed.c
>
[...]
> diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
> new file mode 100644
> index 000000000000..a7e679d5fb42
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-aspeed.c
> @@ -0,0 +1,1137 @@
[...]
> +static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
> + struct device_node *node,
> + int slot)
> +{
> + struct aspeed_pcie_port *port;
> + struct device *dev = pcie->dev;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + port->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> + if (IS_ERR(port->pciephy))
> + return dev_err_probe(dev, PTR_ERR(port->pciephy),
> + "Failed to map pcie%d pciephy base\n", slot);
> +
> + port->clk = devm_get_clk_from_child(dev, node, NULL);
> + if (IS_ERR(port->clk))
> + return dev_err_probe(dev, PTR_ERR(port->clk),
> + "Failed to get pcie%d clock\n", slot);
> +
> + port->perst = of_reset_control_get_exclusive(node, "perst");
> + if (IS_ERR(port->perst))
> + return dev_err_probe(dev, PTR_ERR(port->perst),
> + "Failed to get pcie%d reset control\n", slot);
How about registering a reset_control_put() via
devm_add_action_or_reset()?
Otherwise these reset controls are not released on .remove.
[...]
> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pci_host_bridge *host;
> + struct aspeed_pcie *pcie;
> + struct aspeed_pcie_port *port;
> + struct device_node *node = dev->of_node;
> + const struct aspeed_pcie_rc_platform *md = of_device_get_match_data(dev);
> + int irq, ret;
> +
> + if (!md)
> + return -ENODEV;
> +
> + host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> + if (!host)
> + return -ENOMEM;
> +
> + pcie = pci_host_bridge_priv(host);
> + pcie->dev = dev;
> + pcie->tx_tag = 0;
> + platform_set_drvdata(pdev, pcie);
> +
> + pcie->platform = md;
> + pcie->host = host;
> + INIT_LIST_HEAD(&pcie->ports);
> +
> + pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(pcie->reg))
> + return PTR_ERR(pcie->reg);
> +
> + of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> +
> + pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> + if (IS_ERR(pcie->cfg))
> + return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> + pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> + if (IS_ERR(pcie->h2xrst))
> + return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> + ret = devm_mutex_init(dev, &pcie->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to init mutex\n");
> +
> + ret = pcie->platform->setup(pdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> +
> + ret = aspeed_pcie_parse_dt(pcie);
> + if (ret)
> + return ret;
> +
> + ret = aspeed_pcie_init_ports(pcie);
> + if (ret)
> + goto err_remove_resets;
> +
> + host->sysdata = pcie;
> +
> + ret = aspeed_pcie_init_irq_domain(pcie);
> + if (ret)
> + goto err_irq_init;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + ret = irq;
> + goto err_irq;
> + }
> +
> + ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> + pcie);
> + if (ret)
> + goto err_irq;
> +
> + ret = pci_host_probe(host);
> + if (ret)
> + goto err_irq;
> +
> + return 0;
> +err_irq:
> + aspeed_pcie_irq_domain_free(pcie);
If pci_host_probe() fails, aspeed_pcie_irq_domain_free() will be called
before the IRQ requested with devm_request_irq() above is released.
Also, this is never called on .remove. You can fix both with
devm_add_action_or_reset().
> +err_irq_init:
> +err_remove_resets:
> + list_for_each_entry(port, &pcie->ports, list)
> + reset_control_put(port->perst);
I suggest to let devres handle this (see above).
regards
Philipp
