Hi Philipp
Thank you for your reply.
> > 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.
>
Agreed. It is good method.
Thank you for your suggestion.
> [...]
> > +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).
>
I will apply these parts with your suggestion in this driver.
Thanks,
Jacky
