[PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou <jacky_chou@aspeedtech.com>]

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@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
> >
> [...]
> > 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