On Fri, Aug 7, 2015 at 10:34 PM, Marc Zyngier [off-list ref] wrote:

On 07/08/15 08:42, Ley Foon Tan wrote:

quoted

This patch adds the Altera PCIe host controller driver.

Signed-off-by: Ley Foon Tan <redacted>
---
 drivers/pci/host/Kconfig       |   7 +
 drivers/pci/host/Makefile      |   1 +
 drivers/pci/host/pcie-altera.c | 532 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 540 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera.c

<snip>

quoted

+static int tlp_read_packet(struct altera_pcie *pcie, u32 *value)
+{
+       u8 loop;
+       struct tlp_rp_regpair_t tlp_rp_regdata;
+
+       for (loop = TLP_LOOP; loop > 0; loop--) {

I'd prefer to see the more common idiom:

        for (loop = 0; loop < TLP_LOOP; loop++) {

Okay, will change this.

quoted

+               tlp_read_rx(pcie, &tlp_rp_regdata);
+               if (tlp_rp_regdata.ctrl & RP_RXCPL_EOP) {
+                       if (value)
+                               *value = tlp_rp_regdata.reg0;
+                       return PCIBIOS_SUCCESSFUL;
+               }

Don't you need some form of relaxation here? A delay of some sort?
Repeatedly polling the HW is usually not helping it to recover...

Yes, will add udelay here.

<snip>

quoted

+static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
+{
+       struct device *dev = &pcie->pdev->dev;
+       struct device_node *node = dev->of_node;
+
+       /* Setup INTx */
+       pcie->irq_domain = irq_domain_add_linear(node, INTX_NUM,
+                                       &intx_domain_ops, pcie);
+       if (!pcie->irq_domain) {
+               dev_err(dev, "Failed to get a INTx IRQ domain\n");
+               return PTR_ERR(pcie->irq_domain);

Err, no. irq_domain_add returns NULL on error, so PTR_ERR(NULL) is still
going to be zero.

Change to return -ENOMEM.

quoted

+       }
+
+       return 0;
+}
+
+static int altera_pcie_parse_dt(struct altera_pcie *pcie)
+{
+       struct resource *cra;
+       struct platform_device *pdev = pcie->pdev;
+
+       cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");

Probably worth checking that this hasn't failed.

Noted. Will add error checking.

quoted

+       pcie->cra_base = devm_ioremap_resource(&pdev->dev, cra);
+       if (IS_ERR(pcie->cra_base)) {
+               dev_err(&pdev->dev, "get Cra resource failed\n");
+               return PTR_ERR(pcie->cra_base);
+       }
+
+       /* setup IRQ */
+       pcie->irq = platform_get_irq(pdev, 0);
+       if (pcie->irq <= 0) {
+               dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->irq);
+               return -EINVAL;
+       }
+
+       irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
+
+       return 0;
+}
+
+static int altera_pcie_probe(struct platform_device *pdev)
+{
+       struct altera_pcie *pcie;
+       struct pci_bus *bus;
+       int ret;
+
+       pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+       if (!pcie)
+               return -ENOMEM;
+
+       pcie->pdev = pdev;
+
+       ret = altera_pcie_parse_dt(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Parsing DT failed\n");
+               return ret;
+       }
+
+       INIT_LIST_HEAD(&pcie->resources);
+
+       ret = altera_pcie_parse_request_of_pci_ranges(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed add resources\n");
+               return ret;
+       }
+
+       ret = altera_pcie_init_irq_domain(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
+               return ret;
+       }
+
+       pcie->root_bus_nr = 0;
+
+       /* clear all interrupts */
+       cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
+       /* enable all interrupts */
+       cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
+
+       bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
+                               pcie, &pcie->resources);
+       if (!bus)
+               return -ENOMEM;
+
+       pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+       pci_assign_unassigned_bus_resources(bus);
+       pci_bus_add_devices(bus);
+
+       platform_set_drvdata(pdev, pcie);
+       return ret;
+}
+
+static int __exit altera_pcie_remove(struct platform_device *pdev)
+{
+       struct altera_pcie *pcie = platform_get_drvdata(pdev);
+
+       altera_pcie_free_irq_domain(pcie);
+       platform_set_drvdata(pdev, NULL);
+       return 0;
+}
+
+static const struct of_device_id altera_pcie_of_match[] = {
+       { .compatible = "altr,pcie-root-port-1.0", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, altera_pcie_of_match);
+
+static struct platform_driver altera_pcie_driver = {
+       .probe          = altera_pcie_probe,
+       .remove         = altera_pcie_remove,
+       .driver = {
+               .name   = "altera-pcie",
+               .of_match_table = altera_pcie_of_match,
+       },
+};
+
+module_platform_driver(altera_pcie_driver);
+
+MODULE_AUTHOR("Ley Foon Tan [off-list ref]");
+MODULE_DESCRIPTION("Altera PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
--
1.8.2.1

This is starting to look better. You should get someone to review the DT
parts though.

I have CC-ed DT maintainers. Hope someone can help to review.


Thanks for reviewing.

Regards
Ley Foon