PCI: mvebu: subsystem ids, AER and INTx

Split struct pci_ops between ops and child_ops. Member ops is used for accessing PCIe Root Ports via pci-bridge-emul.c driver and child_ops for accessing real PCIe cards.

There is no need to mix these two struct pci_ops into one as PCI core code already provides separate callbacks via bridge->ops and bridge->child_ops.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 82 ++++++++++++++++--------------
 1 file changed, 44 insertions(+), 38 deletions(-)

drivers/pci/controller/pci-mvebu.c +26 -8

index 9ea2f6a7c2b0..1e90ab888075 100644
@@ -294,11 +294,29 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
        mvebu_writel(port, mask, PCIE_MASK_OFF);
 }
 
-static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
-                                struct pci_bus *bus,
-                                u32 devfn, int where, int size, u32 *val)
+static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
+                                                   struct pci_bus *bus,
+                                                   int devfn);
+
+static int mvebu_pcie_child_rd_conf(struct pci_bus *bus, u32 devfn, int where,
+                                   int size, u32 *val)
 {
-       void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
+       struct mvebu_pcie *pcie = bus->sysdata;
+       struct mvebu_pcie_port *port;
+       void __iomem *conf_data;
+
+       port = mvebu_pcie_find_port(pcie, bus, devfn);
+       if (!port) {
+               *val = 0xffffffff;
+               return PCIBIOS_DEVICE_NOT_FOUND;
+       }
+
+       if (!mvebu_pcie_link_up(port)) {
+               *val = 0xffffffff;
+               return PCIBIOS_DEVICE_NOT_FOUND;
+       }
+
+       conf_data = port->base + PCIE_CONF_DATA_OFF;
 
        mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
                     PCIE_CONF_ADDR_OFF);

@@ -321,11 +339,21 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
        return PCIBIOS_SUCCESSFUL;
 }
 
-static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
-                                struct pci_bus *bus,
-                                u32 devfn, int where, int size, u32 val)
+static int mvebu_pcie_child_wr_conf(struct pci_bus *bus, u32 devfn,
+                                   int where, int size, u32 val)
 {
-       void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
+       struct mvebu_pcie *pcie = bus->sysdata;
+       struct mvebu_pcie_port *port;

drivers/pci/controller/pci-mvebu.c +5 -0

        mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
                     PCIE_CONF_ADDR_OFF);

@@ -347,6 +375,11 @@ static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
        return PCIBIOS_SUCCESSFUL;
 }
 
+static struct pci_ops mvebu_pcie_child_ops = {
+       .read = mvebu_pcie_child_rd_conf,
+       .write = mvebu_pcie_child_wr_conf,
+};
+
 /*
  * Remove windows, starting from the largest ones to the smallest
  * ones.

Quoted 18 lines

drivers/pci/controller/pci-mvebu.c +5 -1

@@ -347,6 +375,11 @@ static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
        return PCIBIOS_SUCCESSFUL;
 }
 
+static struct pci_ops mvebu_pcie_child_ops = {
+       .read = mvebu_pcie_child_rd_conf,
+       .write = mvebu_pcie_child_wr_conf,
+};
+
 /*
  * Remove windows, starting from the largest ones to the smallest
  * ones.

@@ -862,25 +895,12 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 {
        struct mvebu_pcie *pcie = bus->sysdata;
        struct mvebu_pcie_port *port;
-       int ret;
 

drivers/pci/controller/pci-mvebu.c static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn, · line 901

Quoted 3 lines

drivers/pci/controller/pci-mvebu.c +0 -0

        port = mvebu_pcie_find_port(pcie, bus, devfn);
        if (!port)
                return PCIBIOS_DEVICE_NOT_FOUND;

It would be nice to go from 'bus' to 'bridge' ptr directly, but I still had this in my version. I guess a standard RP struct as part of decoupling host bridges from RPs would solve this issue.

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

drivers/pci/controller/pci-mvebu.c +16 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one

drivers/pci/controller/pci-mvebu.c +46 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+

drivers/pci/controller/pci-mvebu.c +24 -0

+	port->intx_irq_chip.irq_mask = mvebu_pcie_intx_irq_mask;
+	port->intx_irq_chip.irq_unmask = mvebu_pcie_intx_irq_unmask;
+
+	pcie_intc_node = of_get_next_child(port->dn, NULL);
+	if (!pcie_intc_node) {
+		dev_err(dev, "No PCIe Intc node found for %s\n", port->name);
+		return -ENODEV;
+	}
+
+	port->intx_irq_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
+						      &mvebu_pcie_intx_irq_domain_ops,
+						      port);
+	of_node_put(pcie_intc_node);
+	if (!port->intx_irq_domain) {
+		devm_kfree(dev, port->intx_irq_chip.name);
+		dev_err(dev, "Failed to get INTx IRQ domain for %s\n", port->name);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void mvebu_pcie_irq_handler(struct irq_desc *desc)
+{

drivers/pci/controller/pci-mvebu.c +46 -1

+	status = cause & unmask;
+
+	/* Process legacy INTx interrupts */
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		if (!(status & PCIE_INT_INTX(i)))
+			continue;
+
+		if (generic_handle_domain_irq(port->intx_irq_domain, i) == -EINVAL)
+			dev_err_ratelimited(dev, "unexpected INT%c IRQ\n", (char)i+'A');
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
 static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	/* Interrupt support on mvebu emulated bridges is not implemented yet */

@@ -1121,6 +1249,16 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->io_attr = -1;
 	}
 
+	/*
+	 * Old DT bindings do not contain "intx" interrupt
+	 * so do not fail probing driver when interrupt does not exist.
+	 */
+	port->intx_irq = of_irq_get_byname(child, "intx");
+	if (port->intx_irq == -EPROBE_DEFER) {
+		ret = port->intx_irq;
+		goto err;
+	}
+
 	reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
 	if (reset_gpio == -EPROBE_DEFER) {
 		ret = reset_gpio;

@@ -1317,6 +1455,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 	for (i = 0; i < pcie->nports; i++) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
+		int irq = port->intx_irq;
 
 		child = port->dn;
 		if (!child)

@@ -1344,6 +1483,22 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		if (irq > 0) {
+			ret = mvebu_pcie_init_irq_domain(port);
+			if (ret) {
+				dev_err(dev, "%s: cannot init irq domain\n",
+					port->name);
+				pci_bridge_emul_cleanup(&port->bridge);
+				devm_iounmap(dev, port->base);
+				port->base = NULL;
+				mvebu_pcie_powerdown(port);
+				continue;
+			}
+			irq_set_chained_handler_and_data(irq,
+							 mvebu_pcie_irq_handler,
+							 port);
+		}
+
 		/*
 		 * PCIe topology exported by mvebu hw is quite complicated. In
 		 * reality has something like N fully independent host bridges

@@ -1448,6 +1603,7 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 
 	for (i = 0; i < pcie->nports; i++) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
+		int irq = port->intx_irq;
 
 		if (!port->base)
 			continue;

@@ -1458,7 +1614,17 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		mvebu_writel(port, cmd, PCIE_CMD_OFF);
 
 		/* Mask all interrupt sources. */
-		mvebu_writel(port, 0, PCIE_MASK_OFF);
+		mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+		/* Clear all interrupt causes. */
+		mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);

Quoted 38 lines · Pali Rohár

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,10 @@ struct mvebu_pcie_port {

drivers/pci/controller/pci-mvebu.c struct mvebu_pcie_port { · line 115

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +2 -0

 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	struct irq_chip intx_irq_chip;

Why is this structure per port? It really should be global. Printing the port number in the name isn't enough of a reason.

Quoted 40 lines

drivers/pci/controller/pci-mvebu.c +21 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 318

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+		 */
+		unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+		unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
+			  PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
+		mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);

Maybe worth printing a warning here, so that the user knows they are on thin ice.

Quoted 61 lines

drivers/pci/controller/pci-mvebu.c +54 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int mvebu_pcie_init_irq_domain(struct mvebu_pcie_port *port)
+{
+	struct device *dev = &port->pcie->pdev->dev;
+	struct device_node *pcie_intc_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	port->intx_irq_chip.name = devm_kasprintf(dev, GFP_KERNEL,

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1006

Quoted 2 lines

drivers/pci/controller/pci-mvebu.c +2 -0

+						  "mvebu-%s-INTx",
+						  port->name);

That's exactly what I really don't want to see. It prevents sharing of the irq_chip structure, and gets in the way of making it const in the future. Yes, I know that some drivers do that. I can't fix those, because /proc/interrupts is ABI. But I really don't want to see more of these.

/sys/kernel/debug/irqs already has all the information you need, as it will happily give you the domain name and the interrupt topology.

Quoted 29 lines

drivers/pci/controller/pci-mvebu.c +29 -0

+	port->intx_irq_chip.irq_mask = mvebu_pcie_intx_irq_mask;
+	port->intx_irq_chip.irq_unmask = mvebu_pcie_intx_irq_unmask;
+
+	pcie_intc_node = of_get_next_child(port->dn, NULL);
+	if (!pcie_intc_node) {
+		dev_err(dev, "No PCIe Intc node found for %s\n", port->name);
+		return -ENODEV;
+	}
+
+	port->intx_irq_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
+						      &mvebu_pcie_intx_irq_domain_ops,
+						      port);
+	of_node_put(pcie_intc_node);
+	if (!port->intx_irq_domain) {
+		devm_kfree(dev, port->intx_irq_chip.name);
+		dev_err(dev, "Failed to get INTx IRQ domain for %s\n", port->name);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void mvebu_pcie_irq_handler(struct irq_desc *desc)
+{
+	struct mvebu_pcie_port *port = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct device *dev = &port->pcie->pdev->dev;
+	u32 cause, unmask, status;
+	int i;

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1040

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+
+	chained_irq_enter(chip, desc);
+
+	cause = mvebu_readl(port, PCIE_INT_CAUSE_OFF);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);

Why do you need to read this? If the CAUSE register also returns the masked interrupts that are pending,

it may be worth keeping a shadow
copy of the this register, as you end-up having an extra MMIO read on each and every interrupt, which can't be great for performance.

Quoted 82 lines

drivers/pci/controller/pci-mvebu.c +46 -1

+	status = cause & unmask;
+
+	/* Process legacy INTx interrupts */
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		if (!(status & PCIE_INT_INTX(i)))
+			continue;
+
+		if (generic_handle_domain_irq(port->intx_irq_domain, i) == -EINVAL)
+			dev_err_ratelimited(dev, "unexpected INT%c IRQ\n", (char)i+'A');
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
 static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	/* Interrupt support on mvebu emulated bridges is not implemented yet */

@@ -1121,6 +1249,16 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->io_attr = -1;
 	}
 
+	/*
+	 * Old DT bindings do not contain "intx" interrupt
+	 * so do not fail probing driver when interrupt does not exist.
+	 */
+	port->intx_irq = of_irq_get_byname(child, "intx");
+	if (port->intx_irq == -EPROBE_DEFER) {
+		ret = port->intx_irq;
+		goto err;
+	}
+
 	reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
 	if (reset_gpio == -EPROBE_DEFER) {
 		ret = reset_gpio;

@@ -1317,6 +1455,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 	for (i = 0; i < pcie->nports; i++) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
+		int irq = port->intx_irq;
 
 		child = port->dn;
 		if (!child)

@@ -1344,6 +1483,22 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		if (irq > 0) {
+			ret = mvebu_pcie_init_irq_domain(port);
+			if (ret) {
+				dev_err(dev, "%s: cannot init irq domain\n",
+					port->name);
+				pci_bridge_emul_cleanup(&port->bridge);
+				devm_iounmap(dev, port->base);
+				port->base = NULL;
+				mvebu_pcie_powerdown(port);
+				continue;
+			}
+			irq_set_chained_handler_and_data(irq,
+							 mvebu_pcie_irq_handler,
+							 port);
+		}
+
 		/*
 		 * PCIe topology exported by mvebu hw is quite complicated. In
 		 * reality has something like N fully independent host bridges

@@ -1448,6 +1603,7 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 
 	for (i = 0; i < pcie->nports; i++) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
+		int irq = port->intx_irq;
 
 		if (!port->base)
 			continue;

@@ -1458,7 +1614,17 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		mvebu_writel(port, cmd, PCIE_CMD_OFF);
 
 		/* Mask all interrupt sources. */
-		mvebu_writel(port, 0, PCIE_MASK_OFF);
+		mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+		/* Clear all interrupt causes. */
+		mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);

drivers/pci/controller/pci-mvebu.c static int mvebu_pcie_remove(struct platform_device *pdev) · line 1630

Quoted 10 lines

drivers/pci/controller/pci-mvebu.c +7 -0

+
+		/* Remove IRQ domains. */
+		if (port->intx_irq_domain)
+			irq_domain_remove(port->intx_irq_domain);
+
+		if (irq > 0)
+			irq_set_chained_handler_and_data(irq, NULL, NULL);
 
 		/* Free config space for emulated root bridge. */
 		pci_bridge_emul_cleanup(&port->bridge);

Thanks,

	M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Quoted 40 lines · On Thursday 06 January 2022 15:28:20 Marc Zyngier wrote:

Quoted 38 lines · Pali Rohár

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,10 @@ struct mvebu_pcie_port {

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c struct mvebu_pcie_port { · line 115

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +2 -0

 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	struct irq_chip intx_irq_chip;

Why is this structure per port? It really should be global. Printing the port number in the name isn't enough of a reason.

Because each port has its own independent set of INTA-INTD interrupts.

Quoted 41 lines

Quoted 41 lines

drivers/pci/controller/pci-mvebu.c +21 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 318

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+		 */
+		unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+		unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
+			  PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
+		mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);

Maybe worth printing a warning here, so that the user knows they are on thin ice.

Ok. I can add it here. Anyway, this is default current state without this patch.

Quoted 61 lines

Quoted 61 lines

drivers/pci/controller/pci-mvebu.c +54 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int mvebu_pcie_init_irq_domain(struct mvebu_pcie_port *port)
+{
+	struct device *dev = &port->pcie->pdev->dev;
+	struct device_node *pcie_intc_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	port->intx_irq_chip.name = devm_kasprintf(dev, GFP_KERNEL,

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1006

Quoted 2 lines

drivers/pci/controller/pci-mvebu.c +2 -0

+						  "mvebu-%s-INTx",
+						  port->name);

That's exactly what I really don't want to see. It prevents sharing of the irq_chip structure, and gets in the way of making it const in the future. Yes, I know that some drivers do that. I can't fix those, because /proc/interrupts is ABI. But I really don't want to see more of these.

Well, I do not understand why it should be shared and with who. HW has N independent IRQ chips for legacy interrupts. And each one will be specified in DT per HW layout / design.

Quoted 51 lines · Pali Rohár

Quoted 40 lines · On Thursday 06 January 2022 15:28:20 Marc Zyngier wrote:

Quoted 38 lines · Pali Rohár

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,10 @@ struct mvebu_pcie_port {

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c struct mvebu_pcie_port { · line 115

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +2 -0

 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	struct irq_chip intx_irq_chip;

Why is this structure per port? It really should be global. Printing the port number in the name isn't enough of a reason.

Quoted 2 lines

Because each port has its own independent set of INTA-INTD interrupts.

That doesn't warrant a copy of an irq_chip structure that contains the exact same callbacks, and only differs by *a string*. And the use of this string is only to end-up in /proc/interrupts, which is totally pointless.

Quoted 123 lines

Quoted 41 lines

Quoted 41 lines

drivers/pci/controller/pci-mvebu.c +21 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 318

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+		 */
+		unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+		unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
+			  PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
+		mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);

Maybe worth printing a warning here, so that the user knows they are on thin ice.

Ok. I can add it here. Anyway, this is default current state without this patch.

Quoted 69 lines

Quoted 53 lines

drivers/pci/controller/pci-mvebu.c +46 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1006

Quoted 10 lines

drivers/pci/controller/pci-mvebu.c +10 -0

+static int mvebu_pcie_init_irq_domain(struct mvebu_pcie_port *port)
+{
+	struct device *dev = &port->pcie->pdev->dev;
+	struct device_node *pcie_intc_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	port->intx_irq_chip.name = devm_kasprintf(dev, GFP_KERNEL,
+						  "mvebu-%s-INTx",
+						  port->name);

That's exactly what I really don't want to see. It prevents sharing of the irq_chip structure, and gets in the way of making it const in the future. Yes, I know that some drivers do that. I can't fix those, because /proc/interrupts is ABI. But I really don't want to see more of these.

Quoted 3 lines

Well, I do not understand why it should be shared and with who. HW has N independent IRQ chips for legacy interrupts. And each one will be specified in DT per HW layout / design.

If you have multiple ports, all the ports can share the irq_chip structure. Actually scratch that. They *MUST* share the structure. The only reason you're not sharing it is to be able to print this useless string in /proc/interrupts.

Quoted 184 lines · On Thursday 06 January 2022 15:55:11 Marc Zyngier wrote:

Quoted 51 lines · Pali Rohár

Quoted 40 lines · On Thursday 06 January 2022 15:28:20 Marc Zyngier wrote:

Quoted 38 lines · Pali Rohár

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,10 @@ struct mvebu_pcie_port {

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c struct mvebu_pcie_port { · line 115

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +2 -0

 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	struct irq_chip intx_irq_chip;

Why is this structure per port? It really should be global. Printing the port number in the name isn't enough of a reason.

Quoted 2 lines

Because each port has its own independent set of INTA-INTD interrupts.

That doesn't warrant a copy of an irq_chip structure that contains the exact same callbacks, and only differs by *a string*. And the use of this string is only to end-up in /proc/interrupts, which is totally pointless.

Quoted 123 lines

Quoted 41 lines

Quoted 41 lines

drivers/pci/controller/pci-mvebu.c +21 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 318

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+		 */
+		unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+		unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
+			  PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
+		mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);

Maybe worth printing a warning here, so that the user knows they are on thin ice.

Ok. I can add it here. Anyway, this is default current state without this patch.

Quoted 69 lines

Quoted 53 lines

drivers/pci/controller/pci-mvebu.c +46 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1006

Quoted 10 lines

drivers/pci/controller/pci-mvebu.c +10 -0

+static int mvebu_pcie_init_irq_domain(struct mvebu_pcie_port *port)
+{
+	struct device *dev = &port->pcie->pdev->dev;
+	struct device_node *pcie_intc_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	port->intx_irq_chip.name = devm_kasprintf(dev, GFP_KERNEL,
+						  "mvebu-%s-INTx",
+						  port->name);

That's exactly what I really don't want to see. It prevents sharing of the irq_chip structure, and gets in the way of making it const in the future. Yes, I know that some drivers do that. I can't fix those, because /proc/interrupts is ABI. But I really don't want to see more of these.

Quoted 8 lines

Quoted 3 lines

Well, I do not understand why it should be shared and with who. HW has N independent IRQ chips for legacy interrupts. And each one will be specified in DT per HW layout / design.

If you have multiple ports, all the ports can share the irq_chip structure. Actually scratch that. They *MUST* share the structure. The only reason you're not sharing it is to be able to print this useless string in /proc/interrupts.

What is the point of sharing one irq chip if HW has N independent irq chips (for legacy interrupts)? I do not catch it yet. And I do not care here for /proc/interrupts, so also I have not caught what do you mean be last sentence with "the only reason".

And I still do not see how it could even work to have just one irq chip and one irq domain as each irq domain needs to know to which port it belongs, so it can mask/unmask interrupts from correct port. Also initialization of domain is taking DT node and for each port it is different.

So I'm somehow confused here...

The improvement in this patch is to be able to mask INTA interrupts on port 1 and let INTA interrupts unmasked on port 2 if there drivers are interested only for interrupts from device connected to port 2.

And if all interrupts are going to be shared (again) then it does not solve any problem.

Quoted 206 lines · Pali Rohár

Quoted 184 lines · On Thursday 06 January 2022 15:55:11 Marc Zyngier wrote:

Quoted 51 lines · Pali Rohár

Quoted 40 lines · On Thursday 06 January 2022 15:28:20 Marc Zyngier wrote:

Quoted 38 lines · Pali Rohár

This adds support for legacy INTx interrupts received from other PCIe devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD interrupts.

Note that for this support, device tree files has to be properly adjusted to provide "interrupts" or "interrupts-extended" property with intx interrupt source, "interrupt-names" property with "intx" string and also 'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as before.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/pci/controller/pci-mvebu.c | 182 +++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 8 deletions(-)

drivers/pci/controller/pci-mvebu.c +3 -2

index 1e90ab888075..04bcdd7b7a6d 100644
@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910
+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910
-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,10 @@ struct mvebu_pcie_port {

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c struct mvebu_pcie_port { · line 115

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +2 -0

 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	struct irq_chip intx_irq_chip;  

Why is this structure per port? It really should be global. Printing the port number in the name isn't enough of a reason.

Quoted 2 lines

Because each port has its own independent set of INTA-INTD interrupts.

That doesn't warrant a copy of an irq_chip structure that contains the exact same callbacks, and only differs by *a string*. And the use of this string is only to end-up in /proc/interrupts, which is totally pointless.

Quoted 123 lines

Quoted 41 lines

Quoted 41 lines

drivers/pci/controller/pci-mvebu.c +21 -5

+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +240,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +293,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.

Quoted 8 lines

drivers/pci/controller/pci-mvebu.c static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port) · line 318

Quoted 5 lines

drivers/pci/controller/pci-mvebu.c +5 -0

+		 */
+		unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+		unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
+			  PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
+		mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);  

Maybe worth printing a warning here, so that the user knows they are on thin ice.

Ok. I can add it here. Anyway, this is default current state without this patch.

Quoted 69 lines

Quoted 53 lines

drivers/pci/controller/pci-mvebu.c +46 -0

+	}
 }
 
 static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,

@@ -924,6 +949,109 @@ static struct pci_ops mvebu_pcie_ops = {
 	.write = mvebu_pcie_wr_conf,
 };
 
+static void mvebu_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask &= ~PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static void mvebu_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct mvebu_pcie_port *port = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 unmask;
+
+	raw_spin_lock_irqsave(&port->irq_lock, flags);
+	unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
+	unmask |= PCIE_INT_INTX(hwirq);
+	mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
+	raw_spin_unlock_irqrestore(&port->irq_lock, flags);
+}
+
+static int mvebu_pcie_intx_irq_map(struct irq_domain *h,
+				   unsigned int virq, irq_hw_number_t hwirq)
+{
+	struct mvebu_pcie_port *port = h->host_data;
+
+	irq_set_status_flags(virq, IRQ_LEVEL);
+	irq_set_chip_and_handler(virq, &port->intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(virq, port);
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_pcie_intx_irq_domain_ops = {
+	.map = mvebu_pcie_intx_irq_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+

drivers/pci/controller/pci-mvebu.c static struct pci_ops mvebu_pcie_ops = { · line 1006

Quoted 10 lines

drivers/pci/controller/pci-mvebu.c +10 -0

+static int mvebu_pcie_init_irq_domain(struct mvebu_pcie_port *port)
+{
+	struct device *dev = &port->pcie->pdev->dev;
+	struct device_node *pcie_intc_node;
+
+	raw_spin_lock_init(&port->irq_lock);
+
+	port->intx_irq_chip.name = devm_kasprintf(dev, GFP_KERNEL,
+						  "mvebu-%s-INTx",
+						  port->name);  

That's exactly what I really don't want to see. It prevents sharing of the irq_chip structure, and gets in the way of making it const in the future. Yes, I know that some drivers do that. I can't fix those, because /proc/interrupts is ABI. But I really don't want to see more of these.

Quoted 8 lines

Quoted 3 lines

Well, I do not understand why it should be shared and with who. HW has N independent IRQ chips for legacy interrupts. And each one will be specified in DT per HW layout / design.

If you have multiple ports, all the ports can share the irq_chip structure. Actually scratch that. They *MUST* share the structure. The only reason you're not sharing it is to be able to print this useless string in /proc/interrupts.

What is the point of sharing one irq chip if HW has N independent irq chips (for legacy interrupts)? I do not catch it yet. And I do not care here for /proc/interrupts, so also I have not caught what do you mean be last sentence with "the only reason".

And I still do not see how it could even work to have just one irq chip and one irq domain as each irq domain needs to know to which port it belongs, so it can mask/unmask interrupts from correct port. Also initialization of domain is taking DT node and for each port it is different.

Quoted 8 lines

So I'm somehow confused here...

The improvement in this patch is to be able to mask INTA interrupts on port 1 and let INTA interrupts unmasked on port 2 if there drivers are interested only for interrupts from device connected to port 2.

And if all interrupts are going to be shared (again) then it does not solve any problem.

You are completely missing my point. I'm talking about data structures, you're talking about interrupts. You have this:

struct mvebu_pcie_port {
       // Tons of stuff
       struct irq_chip intx_chip;
};

What I want you to do is:

struct mvebu_pcie_port {
       // Tons of stuff
};

static struct irq_chip intx_chip = {
	.name		= "INTx",
	.irq_mask	= mvebu_pcie_intx_irq_mask,
	.irq_unmask	= mvebu_pcie_intx_irq_unmask;
};

That's it. No more, no less.

	M.

Quoted 20 lines · On Thu, 06 Jan 2022 16:27:44 +0000 Marc Zyngier <[email protected]> wrote:

You are completely missing my point. I'm talking about data structures, you're talking about interrupts. You have this:

struct mvebu_pcie_port {
       // Tons of stuff
       struct irq_chip intx_chip;
};

What I want you to do is:

struct mvebu_pcie_port {
       // Tons of stuff
};

static struct irq_chip intx_chip = {
	.name		= "INTx",
	.irq_mask	= mvebu_pcie_intx_irq_mask,
	.irq_unmask	= mvebu_pcie_intx_irq_unmask;
};

Quoted 4 lines

That's it. No more, no less.

	M.

Hmm, but struct irq_chip contains a dynamic member,
struct device *parent_device;
Isn't that used? Or are you planning to kill it?

Quoted 28 lines · Marek Behún

Quoted 24 lines · On Thu, 06 Jan 2022 16:27:44 +0000 Marc Zyngier <[email protected]> wrote:

You are completely missing my point. I'm talking about data structures, you're talking about interrupts. You have this:

struct mvebu_pcie_port {
       // Tons of stuff
       struct irq_chip intx_chip;
};

What I want you to do is:

struct mvebu_pcie_port {
       // Tons of stuff
};

static struct irq_chip intx_chip = {
	.name		= "INTx",
	.irq_mask	= mvebu_pcie_intx_irq_mask,
	.irq_unmask	= mvebu_pcie_intx_irq_unmask;
};

That's it. No more, no less.

	M.

Quoted 3 lines

Hmm, but struct irq_chip contains a dynamic member,
struct device *parent_device;
Isn't that used? Or are you planning to kill it?

Indeed, and I am definitely planning to kill it. This is the wrong place for this stuff, and I want it gone. There are thankfully very few users of this misfeature.

Quoted 30 lines · On Thursday 06 January 2022 17:31:36 Marc Zyngier wrote:

Quoted 28 lines · Marek Behún

Quoted 24 lines · On Thu, 06 Jan 2022 16:27:44 +0000 Marc Zyngier <[email protected]> wrote:

You are completely missing my point. I'm talking about data structures, you're talking about interrupts. You have this:

struct mvebu_pcie_port {
       // Tons of stuff
       struct irq_chip intx_chip;
};

What I want you to do is:

struct mvebu_pcie_port {
       // Tons of stuff
};

static struct irq_chip intx_chip = {
	.name		= "INTx",
	.irq_mask	= mvebu_pcie_intx_irq_mask,
	.irq_unmask	= mvebu_pcie_intx_irq_unmask;
};

That's it. No more, no less.

	M.

Quoted 7 lines

Quoted 3 lines

Hmm, but struct irq_chip contains a dynamic member,
struct device *parent_device;
Isn't that used? Or are you planning to kill it?

Indeed, and I am definitely planning to kill it. This is the wrong place for this stuff, and I want it gone. There are thankfully very few users of this misfeature.

Ok, so what about this change?

drivers/pci/controller/pci-mvebu.c +1 -1

@@ -1458,7 +1617,17 @@ static int mvebu_pcie_remove(struct platform_device *pdev)
 		mvebu_writel(port, cmd, PCIE_CMD_OFF);
 
 		/* Mask all interrupt sources. */
-		mvebu_writel(port, 0, PCIE_MASK_OFF);
+		mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);

Controller driver needs to correctly configure PCIe link if it contains 1 or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard way how number of lanes is specified in other PCIe controllers.

Signed-off-by Pali Rohár <[email protected]>

---
 Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

Signed-off-by Pali Rohár <[email protected]>

---
 Documentation/devicetree/bindings/pci/mvebu-pci.txt | 5 +++++
 1 file changed, 5 insertions(+)

Quoted 7 lines · Pali Rohár

Split struct pci_ops between ops and child_ops. Member ops is used for accessing PCIe Root Ports via pci-bridge-emul.c driver and child_ops for accessing real PCIe cards.

There is no need to mix these two struct pci_ops into one as PCI core code already provides separate callbacks via bridge->ops and bridge->child_ops.

I had similar patches including mvebu that I never got around to sending out. I pushed the branch out now at least[1].

Quoted 9 lines · On Wednesday 05 January 2022 09:41:51 Rob Herring wrote:

Quoted 7 lines · Pali Rohár

Split struct pci_ops between ops and child_ops. Member ops is used for accessing PCIe Root Ports via pci-bridge-emul.c driver and child_ops for accessing real PCIe cards.

There is no need to mix these two struct pci_ops into one as PCI core code already provides separate callbacks via bridge->ops and bridge->child_ops.

Quoted 2 lines

I had similar patches including mvebu that I never got around to sending out. I pushed the branch out now at least[1].

Are you going to finish your patch series and send it? Because if yes, I can drop this my patch in v3 and let all ->child_ops conversion for you.