PCI: X-Gene: Add APM X-Gene v1 MSI/MSIX termination driver

Quoted 84 lines · Duc Dang

X-Gene v1 SoC supports total 2688 MSI/MSIX vectors coalesced into 16 HW IRQ lines.

Signed-off-by Duc Dang <[email protected]>

Signed-off-by Tanmay Inamdar <[email protected]>

---
 drivers/pci/host/Kconfig         |   6 +
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pci-xgene-msi.c | 407 +++++++++++++++++++++++++++++++++++++++
 drivers/pci/host/pci-xgene.c     |  21 ++
 4 files changed, 435 insertions(+)
 create mode 100644 drivers/pci/host/pci-xgene-msi.c

drivers/pci/host/Kconfig +6 -0

index 7b892a9..c9b61fa 100644
@@ -89,11 +89,17 @@ config PCI_XGENE
        depends on ARCH_XGENE
        depends on OF
        select PCIEPORTBUS
+       select PCI_MSI_IRQ_DOMAIN if PCI_MSI
+       select PCI_XGENE_MSI if PCI_MSI
        help
          Say Y here if you want internal PCI support on APM X-Gene SoC.
          There are 5 internal PCIe ports available. Each port is GEN3 capable
          and have varied lanes from x1 to x8.
 
+config PCI_XGENE_MSI
+       bool "X-Gene v1 PCIe MSI feature"
+       depends on PCI_XGENE && PCI_MSI
+
 config PCI_LAYERSCAPE
        bool "Freescale Layerscape PCIe controller"
        depends on OF && ARM

drivers/pci/host/Makefile +1 -0

index e61d91c..f39bde3 100644
@@ -11,5 +11,6 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
+obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o

drivers/pci/host/pci-xgene-msi.c new file +32 -0

new file mode 100644
index 0000000..4f0ff42
@@ -0,0 +1,407 @@
+/*
+ * APM X-Gene MSI Driver
+ *
+ * Copyright (c) 2014, Applied Micro Circuits Corporation
+ * Author: Tanmay Inamdar <[email protected]>
+ *        Duc Dang <[email protected]>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/of_pci.h>
+
+#define MSI_INDEX0             0x000000
+#define MSI_INT0               0x800000
+
+struct xgene_msi_settings {
+       u32     index_per_group;
+       u32     irqs_per_index;
+       u32     nr_msi_vec;

drivers/pci/host/pci-xgene-msi.c line 42

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +10 -0

+       u32     nr_hw_irqs;
+};
+
+struct xgene_msi {
+       struct device_node              *node;
+       struct msi_controller           mchip;
+       struct irq_domain               *domain;
+       struct xgene_msi_settings       *settings;
+       u32                             msi_addr_lo;
+       u32                             msi_addr_hi;

I'd rather see the mailbox address directly, and only do the split when assigning it to the message (you seem to play all kind of tricks on the address anyway).

drivers/pci/host/pci-xgene-msi.c line 54

Quoted 12 lines

drivers/pci/host/pci-xgene-msi.c new file +12 -0

+       void __iomem                    *msi_regs;
+       unsigned long                   *bitmap;
+       struct mutex                    bitmap_lock;
+       int                             *msi_virqs;
+};
+
+struct xgene_msi_settings storm_msi_settings = {
+       .index_per_group        = 8,
+       .irqs_per_index         = 21,
+       .nr_msi_vec             = 2688,
+       .nr_hw_irqs             = 16,
+};

It would really help understanding how index, group and hw irq lines are structured. nr_msi_vec is obviously the product of these three numbers, so maybe you can loose it (we have computers, remember... ;-).

Do you expect more than this single "storm" implementation? If so, maybe they should be described in the DT. If not, why do we need a separate structure with an indirection if we know these are constants?

Quoted 25 lines

drivers/pci/host/pci-xgene-msi.c new file +25 -0

+
+typedef int (*xgene_msi_initcall_t)(struct xgene_msi *);
+
+static int xgene_msi_init_storm_settings(struct xgene_msi *xgene_msi)
+{
+       xgene_msi->settings = &storm_msi_settings;
+       return 0;
+}
+
+static struct irq_chip xgene_msi_top_irq_chip = {
+       .name           = "X-Gene1 MSI",
+       .irq_enable     = pci_msi_unmask_irq,
+       .irq_disable    = pci_msi_mask_irq,
+       .irq_mask       = pci_msi_mask_irq,
+       .irq_unmask     = pci_msi_unmask_irq,
+};
+
+static struct  msi_domain_info xgene_msi_domain_info = {
+       .flags  = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+                 MSI_FLAG_PCI_MSIX),
+       .chip   = &xgene_msi_top_irq_chip,
+};
+
+static void xgene_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{

drivers/pci/host/pci-xgene-msi.c line 89

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +10 -0

+       struct xgene_msi *msi = irq_data_get_irq_chip_data(data);
+       u32 nr_hw_irqs = msi->settings->nr_hw_irqs;
+       u32 irqs_per_index = msi->settings->irqs_per_index;
+       u32 reg_set = data->hwirq / (nr_hw_irqs * irqs_per_index);
+       u32 group = data->hwirq % nr_hw_irqs;
+
+       msg->address_hi = msi->msi_addr_hi;
+       msg->address_lo = msi->msi_addr_lo +
+                         (((8 * group) + reg_set) << 16);
+       msg->data = (data->hwirq / nr_hw_irqs) % irqs_per_index;

... (sound of head exploding...). I hate it when I have to reverse engineer the hardware by looking at the code...

Please give us some clue on how interrupts are spread across the various pages, how the various indexes are combined to give an actual address.

drivers/pci/host/pci-xgene-msi.c line 100

Quoted 11 lines

drivers/pci/host/pci-xgene-msi.c new file +11 -0

+}
+
+static int xgene_msi_set_affinity(struct irq_data *irq_data,
+                                 const struct cpumask *mask, bool force)
+{
+       struct xgene_msi *msi = irq_data_get_irq_chip_data(irq_data);
+       unsigned int gic_irq;
+       int ret;
+
+       gic_irq = msi->msi_virqs[irq_data->hwirq % msi->settings->nr_hw_irqs];
+       ret = irq_set_affinity(gic_irq, mask);

Erm... This as the effect of moving *all* the MSIs hanging off this interrupt to another CPU. I'm not sure that's an acceptable effect... What if another MSI requires a different affinity?

Quoted 105 lines

drivers/pci/host/pci-xgene-msi.c new file +105 -0

+       if (ret == IRQ_SET_MASK_OK)
+               ret = IRQ_SET_MASK_OK_DONE;
+
+       return ret;
+}
+
+static struct irq_chip xgene_msi_bottom_irq_chip = {
+       .name                   = "MSI",
+       .irq_set_affinity       = xgene_msi_set_affinity,
+       .irq_compose_msi_msg    = xgene_compose_msi_msg,
+};
+
+static int xgene_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                                 unsigned int nr_irqs, void *args)
+{
+
+       struct xgene_msi *msi = domain->host_data;
+       u32 msi_irq_count = msi->settings->nr_msi_vec;
+       int msi_irq;
+
+       mutex_lock(&msi->bitmap_lock);
+
+       msi_irq = find_first_zero_bit(msi->bitmap, msi_irq_count);
+       if (msi_irq < msi_irq_count)
+               set_bit(msi_irq, msi->bitmap);
+       else
+               msi_irq = -ENOSPC;
+
+       mutex_unlock(&msi->bitmap_lock);
+
+       if (msi_irq < 0)
+               return msi_irq;
+
+       irq_domain_set_info(domain, virq, msi_irq, &xgene_msi_bottom_irq_chip,
+                           domain->host_data, handle_simple_irq, NULL, NULL);
+       set_irq_flags(virq, IRQF_VALID);
+
+       return 0;
+}
+
+static void xgene_irq_domain_free(struct irq_domain *domain,
+                                 unsigned int virq, unsigned int nr_irqs)
+{
+       struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+       struct xgene_msi *msi = irq_data_get_irq_chip_data(d);
+
+       mutex_lock(&msi->bitmap_lock);
+
+       if (!test_bit(d->hwirq, msi->bitmap))
+               pr_err("trying to free unused MSI#%lu\n", d->hwirq);
+       else
+               clear_bit(d->hwirq, msi->bitmap);
+
+       mutex_unlock(&msi->bitmap_lock);
+
+       irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops msi_domain_ops = {
+       .alloc  = xgene_irq_domain_alloc,
+       .free   = xgene_irq_domain_free,
+};
+
+static int xgene_allocate_domains(struct xgene_msi *msi)
+{
+       msi->domain = irq_domain_add_linear(NULL, msi->settings->nr_msi_vec,
+                                           &msi_domain_ops, msi);
+
+       if (!msi->domain) {
+               pr_err("failed to create parent MSI domain\n");
+               return -ENOMEM;
+       }
+
+       msi->mchip.of_node = msi->node;
+       msi->mchip.domain = pci_msi_create_irq_domain(msi->mchip.of_node,
+                                                     &xgene_msi_domain_info,
+                                                     msi->domain);
+
+       if (!msi->mchip.domain) {
+               pr_err("failed to create MSI domain\n");
+               irq_domain_remove(msi->domain);
+               return -ENOMEM;
+       }
+
+       return 0;
+}
+
+static void xgene_free_domains(struct xgene_msi *msi)
+{
+       if (msi->mchip.domain)
+               irq_domain_remove(msi->mchip.domain);
+       if (msi->domain)
+               irq_domain_remove(msi->domain);
+}
+
+static int xgene_msi_init_allocator(struct xgene_msi *xgene_msi)
+{
+       u32 msi_irq_count = xgene_msi->settings->nr_msi_vec;
+       u32 hw_irq_count = xgene_msi->settings->nr_hw_irqs;
+       int size = BITS_TO_LONGS(msi_irq_count) * sizeof(long);
+
+       xgene_msi->bitmap = kzalloc(size, GFP_KERNEL);
+       if (!xgene_msi->bitmap)
+               return -ENOMEM;
+

drivers/pci/host/pci-xgene-msi.c line 215

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +10 -0

+       mutex_init(&xgene_msi->bitmap_lock);
+
+       xgene_msi->msi_virqs = kcalloc(hw_irq_count, sizeof(int), GFP_KERNEL);
+       if (!xgene_msi->msi_virqs)
+               return -ENOMEM;
+
+       return 0;
+}
+
+static irqreturn_t xgene_msi_isr(int irq, void *data)

This is not a "normal" interrupt handler. This is really a chained interrupt controller. Please use the proper infrastructure for this (irq_set_chained_handler, chained_irq_enter, chained_irq_exit). Otherwise, your usage of irq_find_mapping is illegal wrt RCU.

Quoted 16 lines

drivers/pci/host/pci-xgene-msi.c new file +16 -0

+{
+       struct xgene_msi *xgene_msi = (struct xgene_msi *) data;
+       unsigned int virq;
+       int msir_index, msir_reg, msir_val, hw_irq;
+       u32 intr_index, grp_select, msi_grp, processed = 0;
+       u32 nr_hw_irqs, irqs_per_index, index_per_group;
+
+       msi_grp = irq - xgene_msi->msi_virqs[0];
+       if (msi_grp >= xgene_msi->settings->nr_hw_irqs) {
+               pr_err("invalid msi received\n");
+               return IRQ_NONE;
+       }
+
+       nr_hw_irqs = xgene_msi->settings->nr_hw_irqs;
+       irqs_per_index = xgene_msi->settings->irqs_per_index;
+       index_per_group = xgene_msi->settings->index_per_group;

drivers/pci/host/pci-xgene-msi.c line 241

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +10 -0

+
+       grp_select = readl(xgene_msi->msi_regs + MSI_INT0 + (msi_grp << 16));
+       while (grp_select) {
+               msir_index = ffs(grp_select) - 1;
+               msir_reg = (msi_grp << 19) + (msir_index << 16);
+               msir_val = readl(xgene_msi->msi_regs + MSI_INDEX0 + msir_reg);
+               while (msir_val) {
+                       intr_index = ffs(msir_val) - 1;
+                       hw_irq = (((msir_index * irqs_per_index) + intr_index) *
+                                nr_hw_irqs) + msi_grp;

Same thing here. This requires some explaination on how the HW is organised.

Quoted 39 lines

drivers/pci/host/pci-xgene-msi.c new file +39 -0

+                       virq = irq_find_mapping(xgene_msi->domain, hw_irq);
+                       if (virq != 0)
+                               generic_handle_irq(virq);
+                       msir_val &= ~(1 << intr_index);
+                       processed++;
+               }
+               grp_select &= ~(1 << msir_index);
+       }
+
+       return processed > 0 ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int xgene_msi_remove(struct platform_device *pdev)
+{
+       int virq, i;
+       struct xgene_msi *msi = platform_get_drvdata(pdev);
+       u32 nr_hw_irqs = msi->settings->nr_hw_irqs;
+
+       for (i = 0; i < nr_hw_irqs; i++) {
+               virq = msi->msi_virqs[i];
+               if (virq != 0)
+                       free_irq(virq, msi);
+       }
+
+       kfree(msi->bitmap);
+       msi->bitmap = NULL;
+
+       xgene_free_domains(msi);
+
+       return 0;
+}
+
+static int xgene_msi_setup_hwirq(struct xgene_msi *msi,
+                                struct platform_device *pdev,
+                                int irq_index)
+{
+       int virt_msir;
+       cpumask_var_t mask;
+       int err;

drivers/pci/host/pci-xgene-msi.c line 290

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +10 -0

+
+       virt_msir = platform_get_irq(pdev, irq_index);
+       if (virt_msir < 0) {
+               dev_err(&pdev->dev, "Cannot translate IRQ index %d\n",
+                       irq_index);
+               return -EINVAL;
+       }
+
+       err = request_irq(virt_msir, xgene_msi_isr, 0,
+                         xgene_msi_top_irq_chip.name, msi);

This is where you should use irq_set_chained_handler.

Quoted 157 lines

drivers/pci/host/pci-xgene-msi.c new file +117 -0

+       if (err) {
+               dev_err(&pdev->dev, "request irq failed\n");
+               return err;
+       }
+
+       if (alloc_cpumask_var(&mask, GFP_KERNEL)) {
+               cpumask_setall(mask);
+               irq_set_affinity(virt_msir, mask);
+               free_cpumask_var(mask);
+       }
+
+       msi->msi_virqs[irq_index] = virt_msir;
+
+       return 0;
+}
+
+static const struct of_device_id xgene_msi_match_table[] = {
+       {.compatible = "apm,xgene1-msi",
+        .data = xgene_msi_init_storm_settings},
+       {},
+};
+
+static int xgene_msi_probe(struct platform_device *pdev)
+{
+       struct resource *res;
+       int rc, irq_index;
+       struct device_node *np;
+       const struct of_device_id *matched_np;
+       struct xgene_msi *xgene_msi;
+       xgene_msi_initcall_t init_fn;
+       u32 nr_hw_irqs, nr_msi_vecs;
+
+       np = of_find_matching_node_and_match(NULL,
+                            xgene_msi_match_table, &matched_np);
+       if (!np)
+               return -ENODEV;
+
+       xgene_msi = kzalloc(sizeof(struct xgene_msi), GFP_KERNEL);
+       if (!xgene_msi) {
+               dev_err(&pdev->dev, "failed to allocate X-Gene MSI data\n");
+               return -ENOMEM;
+       }
+
+       xgene_msi->node = np;
+
+       init_fn = (xgene_msi_initcall_t) matched_np->data;
+       rc = init_fn(xgene_msi);
+       if (rc)
+               return rc;
+
+       platform_set_drvdata(pdev, xgene_msi);
+
+       nr_msi_vecs = xgene_msi->settings->nr_msi_vec;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       xgene_msi->msi_regs = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(xgene_msi->msi_regs)) {
+               dev_err(&pdev->dev, "no reg space\n");
+               rc = -EINVAL;
+               goto error;
+       }
+
+       xgene_msi->msi_addr_hi = upper_32_bits(res->start);
+       xgene_msi->msi_addr_lo = lower_32_bits(res->start);
+
+       rc = xgene_msi_init_allocator(xgene_msi);
+       if (rc) {
+               dev_err(&pdev->dev, "Error allocating MSI bitmap\n");
+               goto error;
+       }
+
+       rc = xgene_allocate_domains(xgene_msi);
+       if (rc) {
+               dev_err(&pdev->dev, "Failed to allocate MSI domain\n");
+               goto error;
+       }
+
+       nr_hw_irqs = xgene_msi->settings->nr_hw_irqs;
+       for (irq_index = 0; irq_index < nr_hw_irqs; irq_index++) {
+               rc = xgene_msi_setup_hwirq(xgene_msi, pdev, irq_index);
+               if (rc)
+                       goto error;
+       }
+
+       rc = of_pci_msi_chip_add(&xgene_msi->mchip);
+       if (rc) {
+               dev_err(&pdev->dev, "failed to add MSI controller chip\n");
+               goto error;
+       }
+
+       dev_info(&pdev->dev, "APM X-Gene PCIe MSI driver loaded\n");
+
+       return 0;
+error:
+       xgene_msi_remove(pdev);
+       return rc;
+}
+
+static struct platform_driver xgene_msi_driver = {
+       .driver = {
+               .name = "xgene-msi",
+               .owner = THIS_MODULE,
+               .of_match_table = xgene_msi_match_table,
+       },
+       .probe = xgene_msi_probe,
+       .remove = xgene_msi_remove,
+};
+
+static int __init xgene_pcie_msi_init(void)
+{
+       return platform_driver_register(&xgene_msi_driver);
+}
+subsys_initcall(xgene_pcie_msi_init);
+
+MODULE_AUTHOR("Duc Dang <[email protected]>");
+MODULE_DESCRIPTION("APM X-Gene PCIe MSI driver");
+MODULE_LICENSE("GPL v2");

drivers/pci/host/pci-xgene.c +21 -1

index 22751ed..3e6faa1 100644
@@ -468,6 +468,23 @@ static int xgene_pcie_setup(struct xgene_pcie_port *port,
        return 0;
 }
 
+static int xgene_pcie_msi_enable(struct pci_bus *bus)
+{
+       struct device_node *msi_node;
+
+       msi_node = of_parse_phandle(bus->dev.of_node,
+                                       "msi-parent", 0);
+       if (!msi_node)
+               return -ENODEV;
+
+       bus->msi = of_pci_find_msi_chip_by_node(msi_node);
+       if (bus->msi)
+               bus->msi->dev = &bus->dev;
+       else
+               return -ENODEV;
+       return 0;
+}
+
 static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 {
        struct device_node *dn = pdev->dev.of_node;

@@ -504,6 +521,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
        if (!bus)
                return -ENOMEM;
 
+       if (IS_ENABLED(CONFIG_PCI_MSI))
+               if (xgene_pcie_msi_enable(bus))
+                       dev_info(port->dev, "failed to enable MSI\n");
+
        pci_scan_child_bus(bus);
        pci_assign_unassigned_bus_resources(bus);
        pci_bus_add_devices(bus);
--

Quoted 2 lines

1.9.1

Thanks,

        M.

--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Is there any plans to support ACPI for GICv2m MSI? Both this driver and the
GICv2m seems to support OF model of discovery for msi controller. X-Gene1
uses this driver and X-Gene2 uses GICv2m, there needs to be a way to associate msi controller with the PCIe bus. I haven't found a standard way of doing finding "msi-parent" for ACPI. Do you have
any suggestion.

Hi Marc:

Is there any plans to support ACPI for GICv2m MSI? Both this driver and the GICv2m seems to support OF model of discovery for msi controller. X-Gene1 uses this driver and X-Gene2 uses GICv2m, there needs to be a way to associate msi controller with the PCIe bus. I haven't
found a standard way of doing finding "msi-parent" for ACPI. Do you have any suggestion.

Sorry for top posting.

drivers/pci/host/Kconfig +3 -0

 
+config PCI_XGENE_MSI
+	bool "X-Gene v1 PCIe MSI feature"
+	depends on PCI_XGENE && PCI_MSI

drivers/pci/host/Makefile +1 -0

 
+obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o

+MODULE_LICENSE("GPL v2");

Just a nit about a license mismatch.

Quoted 14 lines · Duc Dang

drivers/pci/host/Kconfig +3 -0

 
+config PCI_XGENE_MSI
+     bool "X-Gene v1 PCIe MSI feature"
+     depends on PCI_XGENE && PCI_MSI

drivers/pci/host/Makefile +1 -0

 
+obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o

drivers/pci/host/pci-xgene-msi.c line 16

Quoted 10 lines

drivers/pci/host/pci-xgene-msi.c new file +9 -0

 
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.

This states the license of this file is GPL v2 or later.

Quoted 1 line

+MODULE_LICENSE("GPL v2");

And, according to include/linux/module.h, this states the license of this driver is GPL v2. So either the comment at the top of this file or the license ident used in this macro needs to change.

(Now, as far as I can tell, this driver is built-in only. So I think this macro - and all other module specific code in this file - is actually unneeded. But I already know Bjorn's position here, so I won't bother you about that.)

Thanks,

Paul Bolle

On Tue, 7 Apr 2015 20:56:48 +0100
Duc Dang <[email protected]> wrote:

Hi Duc,

Quoted 45 lines · Marc Zyngier

Quoted 34 lines · Duc Dang

Quoted 19 lines · Marc Zyngier

Quoted 18 lines · Duc Dang

Quoted 5 lines

X-Gene v1 SOC supports total 2688 MSI/MSIX vectors coalesced into 16 HW IRQ lines.

Signed-off-by Duc Dang <[email protected]>

Signed-off-by Tanmay Inamdar <[email protected]>

I just had a quick look at this, and this seems to be going in the exact opposite direction compared to what we now have on arm64, where we move away from using struct msi_controller for most thing, and implement PCI MSI/MSIX in a generic way, using MSI domains.

I suggest you have a look at how GICv2m and GICv3 ITS implement the MSI support. You can also have a look at what I did for the Tegra MSI controller in this patch:

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/kill-msi-controller&id=83b3602fcee7972b9d549ed729b56ec28de16081

Quoted 7 lines

Quoted 2 lines

Eventually, the plan is to kill msi_controller entirely, so introducing new drivers that rely on it is not something I'm eager to see.

Thanks, Marc.

X-Gene 1 MSI is handled by separate MSI controller block, so its
driver implementation is different from GICv2m and GICv3. I will refer

It will certainly be different in the sense that you won't use a stacked domain on top of the GIC. But what I want to see is the use of a generic pci_msi_domain on top of an irq_domain, just like we have on v2m and v3. Thomas has also been vocal enough about that in the past, and x86 is going down that road as well.

Quoted 9 lines

Quoted 2 lines

to what you did for Tegra MSI, but I don't see your latest changes in 4.0-rc4. Is the change you made for Tegra MSI going to mainline soon?

Not yet. As you can see in this branch, this relies on some other cleanups. But you can already convert most of your driver and put it in the shape that matches what we have for v2m and v3. Once the required cleanups are in, I'll remove the last traces of msi_controller myself if necessary.

Quoted 8 lines

Hi Marc, Bjorn,

I follow Marc's suggestion and convert my X-Gene 1 MSI driver to remove msi_controller struct and use generic pci_msi_domain on top of an irq_domain. The code requires Marc's changes in irq/kill-msi-controller branch to be compiled and function correctly, so I plan to post the patch on top of Marc's tree. Please let me know if you think I should have different approach to post this patch.

I don't think you should rely on this branch just yet (this is why I suggested matching what we currently have for GICv2m and v3). Keeping msi_controller is fine at the moment, even if we only use to store the MSI domain. If and when this series makes it to mainline, I'll go over the individual drivers to convert them to the new scheme, just like I did for GIC and Tegra.

Quoted 4 lines

Another question I have is when do you plan to roll out this hierarchy irq domain implementation for MSI, as I see some of Marc's changes to kill msi_controller structure and implement MSI irq domain still does not get into Bjorn's tree.

I'm still tinkering with it (I have some long standing comments from Bjorn to address), and I'm working on non-PCI MSI support at the moment.

But the core MSI irq domain stuff is already in (this is what GICv2m and GICv3 ITS are using), and this should be an easy thing to convert your code.

Please let me know if I can be of any help.

Thanks,

        M.

--
Without deviation from the norm, progress is not possible.

This patch set adds MSI/MSIX termination driver support for APM X-Gene v1 SoC. APM X-Gene v1 SoC supports its own implementation of MSI, which is not compliant to GIC V2M specification for MSI Termination.

There is single MSI block in X-Gene v1 SOC which serves all 5 PCIe ports. This MSI block supports 2688 MSI termination ports coalesced into 16 physical HW IRQ lines and shared across all 5 PCIe ports.

v2 changes:
1. Use msi_controller structure
2. Remove arch hooks arch_teardown_msi_irqs and arch_setup_msi_irqs

 .../devicetree/bindings/pci/xgene-pci-msi.txt      |  61 ++++
 MAINTAINERS                                        |   8 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |  27 ++
 drivers/pci/host/Kconfig                           |   4 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-xgene-msi.c                   | 393 +++++++++++++++++++++
 drivers/pci/host/pci-xgene.c                       |  25 ++
 7 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci-msi.txt
 create mode 100644 drivers/pci/host/pci-xgene-msi.c

--
1.9.1

Quoted 23 lines · Duc Dang

This patch set adds MSI/MSIX termination driver support for APM X-Gene v1 SoC. APM X-Gene v1 SoC supports its own implementation of MSI, which is not compliant to GIC V2M specification for MSI Termination.

There is single MSI block in X-Gene v1 SOC which serves all 5 PCIe ports. This MSI block supports 2688 MSI termination ports coalesced into 16 physical HW IRQ lines and shared across all 5 PCIe ports.

v2 changes:
1. Use msi_controller structure
2. Remove arch hooks arch_teardown_msi_irqs and arch_setup_msi_irqs

 .../devicetree/bindings/pci/xgene-pci-msi.txt      |  61 ++++
 MAINTAINERS                                        |   8 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |  27 ++
 drivers/pci/host/Kconfig                           |   4 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-xgene-msi.c                   | 393 +++++++++++++++++++++
 drivers/pci/host/pci-xgene.c                       |  25 ++
 7 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci-msi.txt
 create mode 100644 drivers/pci/host/pci-xgene-msi.c

Quoted 3 lines

--
1.9.1

Hi Bjorn, Arnd, and All,

Did you have a chance to take a look at this v2 patch set for X-Gene 1 MSI support?

[+cc Marc]

Quoted 27 lines · Duc Dang

Quoted 26 lines · Duc Dang

This patch set adds MSI/MSIX termination driver support for APM X-Gene v1 SoC. APM X-Gene v1 SoC supports its own implementation of MSI, which is not compliant to GIC V2M specification for MSI Termination.

There is single MSI block in X-Gene v1 SOC which serves all 5 PCIe ports. This MSI block supports 2688 MSI termination ports coalesced into 16 physical HW IRQ lines and shared across all 5 PCIe ports.

v2 changes:
1. Use msi_controller structure
2. Remove arch hooks arch_teardown_msi_irqs and arch_setup_msi_irqs

 .../devicetree/bindings/pci/xgene-pci-msi.txt      |  61 ++++
 MAINTAINERS                                        |   8 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |  27 ++
 drivers/pci/host/Kconfig                           |   4 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-xgene-msi.c                   | 393 +++++++++++++++++++++
 drivers/pci/host/pci-xgene.c                       |  25 ++
 7 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci-msi.txt
 create mode 100644 drivers/pci/host/pci-xgene-msi.c

--
1.9.1

Quoted 4 lines

Hi Bjorn, Arnd, and All,

Did you have a chance to take a look at this v2 patch set for X-Gene 1 MSI support?

Marc had some comments, and as far as I can tell, you haven't addressed them yet. Am I mistaken?

Bjorn

Quoted 35 lines · Bjorn Helgaas

[+cc Marc]

Quoted 31 lines · Duc Dang

Quoted 23 lines · Duc Dang

This patch set adds MSI/MSIX termination driver support for APM X-Gene v1 SoC. APM X-Gene v1 SoC supports its own implementation of MSI, which is not compliant to GIC V2M specification for MSI Termination.

There is single MSI block in X-Gene v1 SOC which serves all 5 PCIe ports. This MSI block supports 2688 MSI termination ports coalesced into 16 physical HW IRQ lines and shared across all 5 PCIe ports.

v2 changes:
1. Use msi_controller structure
2. Remove arch hooks arch_teardown_msi_irqs and arch_setup_msi_irqs

 .../devicetree/bindings/pci/xgene-pci-msi.txt      |  61 ++++
 MAINTAINERS                                        |   8 +
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |  27 ++
 drivers/pci/host/Kconfig                           |   4 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pci-xgene-msi.c                   | 393 +++++++++++++++++++++
 drivers/pci/host/pci-xgene.c                       |  25 ++
 7 files changed, 519 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/xgene-pci-msi.txt
 create mode 100644 drivers/pci/host/pci-xgene-msi.c

Quoted 3 lines

--
1.9.1

Hi Bjorn, Arnd, and All,

Did you have a chance to take a look at this v2 patch set for X-Gene 1 MSI support?

Quoted 3 lines

Marc had some comments, and as far as I can tell, you haven't addressed them yet. Am I mistaken?

Hi Bjorn,

You are correct. I am making the changes as Marc pointed out. Will update a new version soon.

Documentation/devicetree/bindings/pci/xgene-pci-msi.txt new file +24 -0

@@ -0,0 +1,34 @@
+* AppliedMicro X-Gene PCIe MSI interface
+
+Required properties:
+
+- compatible: should contain "apm,xgene-pcie-msi" or "apm,xgene-storm-pcie-msi"
+       to identify the core.
+- reg: A list of physical base address and length for each set of controller
+       registers.
+- interrupts: A list of interrupt outputs of the controller.
+
+Examples:
+
+SoC DTSI:
+
+       msi at 79000000 {
+            compatible = "apm,xgene-storm-pcie-msi", "apm,xgene-pcie-msi";
+            reg = <0x00 0x79000000 0x0 0x900000>;
+            interrupts = <  0x0 0x10 0x4
+                    0x0 0x11 0x4
+                    0x0 0x12 0x4
+                    0x0 0x13 0x4
+                    0x0 0x14 0x4
+                    0x0 0x15 0x4
+                    0x0 0x16 0x4