On Fri, Jul 31, 2015 at 8:34 PM, Marc Zyngier [off-list ref] wrote:

On 31/07/15 11:15, 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       |   8 +
 drivers/pci/host/Makefile      |   1 +
 drivers/pci/host/pcie-altera.c | 526 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 535 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 675c2d1..108500a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig

@@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
        Say Y here if you want to use the Broadcom iProc PCIe controller
        through the BCMA bus interface

+config PCIE_ALTERA
+     tristate "Altera PCIe controller"
+     depends on ARCH_SOCFPGA
+     select PCI_MSI_IRQ_DOMAIN if PCI_MSI

Why would you select this here? MSI support is in another driver, so I
don't see the point.

Will move select PCI_MSI_IRQ_DOMAIN to config  PCIE_ALTERA_MSI section.

quoted

+     help
+       Say Y here if you want to enable PCIe controller support for Altera
+       SoCFPGA family of SoCs.
+
 endmenu

quoted

+static const struct irq_domain_ops intx_domain_ops = {
+     .map = altera_pcie_intx_map,
+};
+
+static irqreturn_t altera_pcie_isr(int irq, void *arg)
+{
+     struct altera_pcie *pcie = arg;
+     u32 status, i;
+
+     status = cra_readl(pcie, P2A_INT_STATUS) & P2A_INT_STS_ALL;
+
+     /* clear interrupts */
+     cra_writel(pcie, status, P2A_INT_STATUS);
+
+     for (i = 0; i < INTX_NUM; i++) {
+             if (status & (1 << i))

find_first/next_set_bit?

Yes, change this to find_first_bit().

quoted

+                     generic_handle_irq(irq_find_mapping(pcie->irq_domain,
+                                                     i + 1));

Rule of thumb: if you're calling generic_handle_irq from an interrupt
handler, this is a chained IRQ controller. Please implement it as such.

Okay, will change this to chained_irq_enter/chained_irq_exit implementation.

quoted

+     }
+
+     return IRQ_HANDLED;
+}
+
+static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
+{
+     pci_free_resource_list(&pcie->resources);
+}
+
+static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
+{
+     int err, res_valid = 0;
+     struct device *dev = &pcie->pdev->dev;
+     struct device_node *np = dev->of_node;
+     resource_size_t iobase;
+     struct resource_entry *win;
+     int offset = 0;
+
+     err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
+                                            &iobase);
+     if (err)
+             return err;
+
+     resource_list_for_each_entry(win, &pcie->resources) {
+             struct resource *parent, *res = win->res;
+
+             switch (resource_type(res)) {
+             case IORESOURCE_MEM:
+                     parent = &iomem_resource;
+                     res_valid |= !(res->flags & IORESOURCE_PREFETCH);
+                     cra_writel(pcie, res->start,
+                             A2P_ADDR_MAP_LO0 + offset);
+                     cra_writel(pcie, 0,
+                             A2P_ADDR_MAP_HI0 + offset);
+                     offset += ATT_ENTRY_SIZE;
+                     break;
+             default:
+                     continue;
+             }
+
+             err = devm_request_resource(dev, parent, res);
+             if (err)
+                     goto out_release_res;
+     }
+
+     if (!res_valid) {
+             dev_err(dev, "non-prefetchable memory resource required\n");
+             err = -EINVAL;
+             goto out_release_res;
+     }
+
+     return 0;
+
+out_release_res:
+     altera_pcie_release_of_pci_ranges(pcie);
+     return err;
+}
+
+static void altera_pcie_free_irq_domain(struct altera_pcie *pcie)
+{
+     int i;
+     u32 irq;
+
+     for (i = 0; i < INTX_NUM; i++) {
+             irq = irq_find_mapping(pcie->irq_domain, i);
+             if (irq > 0)
+                     irq_dispose_mapping(irq);
+     }
+
+     irq_domain_remove(pcie->irq_domain);
+}
+
+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);
+     }
+
+     return 0;
+}
+
+static int altera_pcie_parse_dt(struct altera_pcie *pcie)
+{
+     struct resource *cra;
+     int ret;
+     struct platform_device *pdev = pcie->pdev;
+
+     cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");
+     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->hwirq = platform_get_irq(pdev, 0);
+     if (pcie->hwirq <= 0) {

This is definitely not a hardware interrupt number. It is completely
virtual. I suggest you give it a better name.

Noted.

quoted

+             dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->hwirq);
+             return -EINVAL;
+     }
+     ret = devm_request_irq(&pdev->dev, pcie->hwirq, altera_pcie_isr,
+                     IRQF_SHARED, pdev->name, pcie);

You're implementing a cascading interrupt controller again. Please fix
it just like you did for your MSI support.

Sure, will change to irq_set_chained_handler_and_data here.


Thanks for reviewing.

Regards
Ley Foon