Hi Philipp

Thank you for your reply.

quoted

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@aspeedtech.com>
---
 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

[...]

quoted

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 @@

[...]

quoted

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

quoted

+	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.

Agreed. It is good method.
Thank you for your suggestion.

[...]

quoted

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

quoted

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

quoted

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

quoted

+			       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().

quoted

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

I will apply these parts with your suggestion in this driver.

Thanks,
Jacky