PCI: INTx interrupt fixes & cleanup

This series fixes our handling of the PCI INTD interrupts in both Xilinx PCIe controller drivers, and cleans up a wasted IRQ domain entry in the Altera PCIe controller driver. It also adds a common PCI_INTX_NUM definition which all relevant drivers under drivers/pci/host/ are adjusted to make use of.

- Patch 1 moves enum pci_interrupt_pin to somewhere we can use it from PCI host code.

- Patch 2 introduces a helper function to translate 1-4 INTx ranges into the 0-3 range, by using it as a struct irq_domain's xlate callback.

- Patches 3 adjusts the Altera driver to avoid wasting an IRQ domain entry.

- Patches 4 & 5 fix the INTD interrupt for Xilinx PCIe drivers.

- Patches 6-8 make use of PCI_NUM_INTX to replace custom definitions or magic numbers in drivers.

The series versioning is continuing on from that of my "PCI: xilinx:
Fixes, optimisation & MIPS support" series which some of the earlier patches were originally a part of, in order to prevent individual patches from jumping backwards in versioning.

Applies atop v4.13-rc5.

Paul Burton (8):
PCI: Move enum pci_interrupt_pin to linux/pci.h
PCI: Introduce pci_irqd_intx_xlate()
PCI: altera: Use size=4 IRQ domain for legacy INTx
PCI: xilinx: Translate INTx range to hwirqs 0-3
PCI: xilinx-nwl: Translate INTx range to hwirqs 0-3
PCI: aardvark: Use PCI_NUM_INTX
PCI: ftpci100: Use PCI_NUM_INTX
PCI: rockchip: Use PCI_NUM_INTX

 drivers/pci/host/pci-aardvark.c    |  5 ++--
 drivers/pci/host/pci-ftpci100.c    |  2 +-
 drivers/pci/host/pcie-altera.c     |  9 +++----
 drivers/pci/host/pcie-rockchip.c   |  2 +-
 drivers/pci/host/pcie-xilinx-nwl.c |  9 +++----
 drivers/pci/host/pcie-xilinx.c     |  7 ++---
 include/linux/pci-epf.h            |  9 +------
 include/linux/pci.h                | 54 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 71 insertions(+), 26 deletions(-)

2.14.1

We currently have a definition of enum pci_interrupt_pin in a header specific to PCI endpoints - linux/pci-epf.h. In order to allow for use of this enum from PCI host code in a future commit, move its definition to linux/pci.h & include that from linux/pci-epf.h.

Additionally we add a PCI_NUM_INTX macro which indicates the number of PCI INTx interrupts, and will be used alongside enum pci_interrupt_pin in further patches.

---

Changes in v7:
- Move it to linux/pci.h rather than a new header.
- Add PCI_NUM_INTX.

Changes in v6:
- New patch.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/linux/pci-epf.h |  9 +--------
 include/linux/pci.h     | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

index 0d529cb90143..bc8750688348 100644
@@ -14,17 +14,10 @@
 
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/pci.h>
 
 struct pci_epf;
 
-enum pci_interrupt_pin {
-	PCI_INTERRUPT_UNKNOWN,
-	PCI_INTERRUPT_INTA,
-	PCI_INTERRUPT_INTB,
-	PCI_INTERRUPT_INTC,
-	PCI_INTERRUPT_INTD,
-};
-
 enum pci_barno {
 	BAR_0,
 	BAR_1,

index a75c13673852..8098c438d88a 100644
@@ -1395,6 +1395,28 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 					      NULL);
 }
 
+/**
+ * enum pci_interrupt_pin - PCI INTx interrupt values
+ * @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt
+ * @PCI_INTERRUPT_INTA: PCI INTA pin
+ * @PCI_INTERRUPT_INTB: PCI INTB pin
+ * @PCI_INTERRUPT_INTC: PCI INTC pin
+ * @PCI_INTERRUPT_INTD: PCI INTD pin
+ *
+ * Corresponds to values for legacy PCI INTx interrupts, as can be found in the
+ * PCI_INTERRUPT_PIN register.
+ */
+enum pci_interrupt_pin {
+	PCI_INTERRUPT_UNKNOWN,
+	PCI_INTERRUPT_INTA,
+	PCI_INTERRUPT_INTB,
+	PCI_INTERRUPT_INTC,
+	PCI_INTERRUPT_INTD,
+};
+
+/* The number of legacy PCI INTx interrupts */
+#define PCI_NUM_INTX	4
+
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
 extern bool pcie_ports_auto;

2.14.1

Legacy PCI INTx interrupts are represented in the PCI_INTERRUPT_PIN register using the range 1-4, which matches our enum pci_interrupt_pin. This is however not ideal for an IRQ domain, where with 4 interrupts we would ideally have a domain of size 4 & hwirq numbers in the range 0-3.

Different PCI host controller drivers have handled this in different ways. Of those under drivers/pci/ which register an INTx IRQ domain, we have:

- pcie-altera uses the range 1-4 in device trees and an IRQ domain of size 5 to cover that range, with entry 0 wasted.

- pcie-xilinx & pcie-xilinx-nwl use the range 1-4 in device trees but register an IRQ domain of size 4, which doesn't cover the hwirq=4/INTD case leading to that interrupt being broken.

- pci-ftpci100 & pci-aardvark use the range 0-3 in both device trees & as hwirq numbering in the driver & IRQ domain.

In order to introduce some level of consistency in at least the hwirq numbering used by the drivers & IRQ domains, this patch introduces a new pci_irqd_intx_xlate() helper function which drivers using the 1-4 range in device trees can assign as the xlate callback for their INTx IRQ domain. This translates the 1-4 range into a 0-3 range, allowing us to use an IRQ domain of size 4 & avoid a wasted entry. Further patches will make use of this in drivers to allow them to use an IRQ domain of size 4 for legacy INTx interrupts without breaking INTD.

---

Changes in v7:
- Rewrite the commit message.

Changes in v6:
- New patch.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/linux/pci.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

index 8098c438d88a..e97ce5d1bbb6 100644
@@ -1417,6 +1417,38 @@ enum pci_interrupt_pin {
 /* The number of legacy PCI INTx interrupts */
 #define PCI_NUM_INTX	4
 
+/**
+ * pci_irqd_intx_xlate() - Translate PCI INTx value to an IRQ domain hwirq
+ * @d: the INTx IRQ domain
+ * @node: the DT node for the device whose interrupt we're translating
+ * @intspec: the interrupt specifier data from the DT
+ * @intsize: the number of entries in @intspec
+ * @out_hwirq: pointer at which to write the hwirq number
+ * @out_type: pointer at which to write the interrupt type
+ *
+ * Translate a PCI INTx interrupt number from device tree in the range 1-4, as
+ * stored in the standard PCI_INTERRUPT_PIN register, to a value in the range
+ * 0-3 suitable for use in a 4 entry IRQ domain. That is, subtract one from the
+ * INTx value to obtain the hwirq number.
+ *
+ * Returns 0 on success, or -EINVAL if the interrupt specifier is out of range.
+ */
+static inline int pci_irqd_intx_xlate(struct irq_domain *d,
+				      struct device_node *node,
+				      const u32 *intspec,
+				      unsigned int intsize,
+				      unsigned long *out_hwirq,
+				      unsigned int *out_type)
+{
+	const u32 intx = intspec[0];
+
+	if (intx < PCI_INTERRUPT_INTA || intx > PCI_INTERRUPT_INTD)
+		return -EINVAL;
+
+	*out_hwirq = intx - PCI_INTERRUPT_INTA;
+	return 0;
+}
+
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
 extern bool pcie_ports_auto;

2.14.1

The devicetree binding documentation for the Altera PCIe controller shows an example which uses an interrupt-map property to map PCI INTx interrupts to hardware IRQ numbers 1-4. The driver creates an IRQ domain with size 5 in order to cover this range, with hwirq=0 left unused.

This patch cleans up this wasted IRQ domain entry, modifying the driver to use an IRQ domain of size 4 which matches the actual number of PCI INTx interrupts. Since the hwirq numbers 1-4 are part of the devicetree binding, and this is considered ABI, we cannot simply change the interrupt-map property to use the range 0-3. Instead we make use of the pci_irqd_intx_xlate() helper function to translate the range 1-4 used at the DT level into the range 0-3 which is now used within the driver, and stop adding 1 to decoded hwirq numbers in altera_pcie_isr().

Whilst cleaning up INTx handling we make use of the new PCI_NUM_INTX macro & drop the custom INTX_NUM definition.

---
I have only build tested this.

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pcie-altera.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 4ea4f8f5dc77..6fced590eb87 100644
@@ -76,8 +76,6 @@
 #define LINK_UP_TIMEOUT			HZ
 #define LINK_RETRAIN_TIMEOUT		HZ
 
-#define INTX_NUM			4
-
 #define DWORD_MASK			3
 
 struct altera_pcie {

@@ -464,6 +462,7 @@ static int altera_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 
 static const struct irq_domain_ops intx_domain_ops = {
 	.map = altera_pcie_intx_map,
+	.xlate = pci_irqd_intx_xlate,
 };
 
 static void altera_pcie_isr(struct irq_desc *desc)

@@ -481,11 +480,11 @@ static void altera_pcie_isr(struct irq_desc *desc)
 
 	while ((status = cra_readl(pcie, P2A_INT_STATUS)
 		& P2A_INT_STS_ALL) != 0) {
-		for_each_set_bit(bit, &status, INTX_NUM) {
+		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
 			/* clear interrupts */
 			cra_writel(pcie, 1 << bit, P2A_INT_STATUS);
 
-			virq = irq_find_mapping(pcie->irq_domain, bit + 1);
+			virq = irq_find_mapping(pcie->irq_domain, bit);
 			if (virq)
 				generic_handle_irq(virq);
 			else

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

2.14.1

The pcie-xilinx driver creates an IRQ domain of size 4 for legacy PCI INTx interrupts, which at first glance seems reasonable since there are 4 possible such interrupts. Unfortunately the driver then proceeds to use the range 1-4 as the hwirq numbers for INTA-INTD, causing warnings & broken interrupts when attempting to use INTD/hwirq=4 due to it being beyond the range of the IRQ domain:

  WARNING: CPU: 0 PID: 1 at kernel/irq/irqdomain.c:365
      irq_domain_associate+0x170/0x220
  error: hwirq 0x4 is too large for dummy
  Modules linked in:
  CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W
      4.12.0-rc5-00126-g19e1b3a10aad-dirty #427
  Stack : 0000000000000000 0000000000000004 0000000000000006 ffffffff8092c78a
          0000000000000061 ffffffff8018bf60 0000000000000000 0000000000000000
          ffffffff8088c287 ffffffff80811d18 a8000000ffc60000 ffffffff80926678
          0000000000000001 0000000000000000 ffffffff80887880 ffffffff80960000
          ffffffff80920000 ffffffff801e6744 ffffffff80887880 a8000000ffc4f8f8
          000000000000089c ffffffff8018d260 0000000000010000 ffffffff80811d18
          0000000000000000 0000000000000001 0000000000000000 0000000000000000
          0000000000000000 a8000000ffc4f840 0000000000000000 ffffffff8042cf34
          0000000000000000 0000000000000000 0000000000000000 0000000000040c00
          0000000000000000 ffffffff8010d1c8 0000000000000000 ffffffff8042cf34
          ...
  Call Trace:
  [<ffffffff8010d1c8>] show_stack+0x80/0xa0
  [<ffffffff8042cf34>] dump_stack+0xd4/0x110
  [<ffffffff8013ea98>] __warn+0xf0/0x108
  [<ffffffff8013eb14>] warn_slowpath_fmt+0x3c/0x48
  [<ffffffff80196528>] irq_domain_associate+0x170/0x220
  [<ffffffff80196bf0>] irq_create_mapping+0x88/0x118
  [<ffffffff801976a8>] irq_create_fwspec_mapping+0xb8/0x320
  [<ffffffff80197970>] irq_create_of_mapping+0x60/0x70
  [<ffffffff805d1318>] of_irq_parse_and_map_pci+0x20/0x38
  [<ffffffff8049c210>] pci_fixup_irqs+0x60/0xe0
  [<ffffffff8049cd64>] xilinx_pcie_probe+0x28c/0x478
  [<ffffffff804e8ca8>] platform_drv_probe+0x50/0xd0
  [<ffffffff804e73a4>] driver_probe_device+0x2c4/0x3a0
  [<ffffffff804e7544>] __driver_attach+0xc4/0xd0
  [<ffffffff804e5254>] bus_for_each_dev+0x64/0xa8
  [<ffffffff804e5e40>] bus_add_driver+0x1f0/0x268
  [<ffffffff804e8000>] driver_register+0x68/0x118
  [<ffffffff801001a4>] do_one_initcall+0x4c/0x178
  [<ffffffff808d3ca8>] kernel_init_freeable+0x204/0x2b0
  [<ffffffff80730b68>] kernel_init+0x10/0xf8
  [<ffffffff80106218>] ret_from_kernel_thread+0x14/0x1c

Fix this by making use of the new pci_irqd_intx_xlate() helper to translate the INTx 1-4 range into the 0-3 range suitable for the IRQ domain of size 4, and stop adding 1 to the hwirq number decoded from the interrupt FIFO which is already in the range 0-3.

Whilst we're here we switch to using PCI_NUM_INTX rather than the magic number 4, making it clearer what the 4 means.

---

Changes in v7: None

Changes in v6:
- New patch, replacing "PCI: xilinx: Create legacy IRQ domain with size 5".

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pcie-xilinx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index f63fa5e0278c..9a8d5ad1dd70 100644
@@ -369,6 +369,7 @@ static int xilinx_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
 /* INTx IRQ Domain operations */
 static const struct irq_domain_ops intx_domain_ops = {
 	.map = xilinx_pcie_intx_map,
+	.xlate = pci_irqd_intx_xlate,
 };
 
 /* PCIe HW Functions */

@@ -440,8 +441,8 @@ static irqreturn_t xilinx_pcie_intr_handler(int irq, void *data)
 				   XILINX_PCIE_REG_RPIFR1);
 
 			/* Handle INTx Interrupt */
-			val = ((val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
-				XILINX_PCIE_RPIFR1_INTR_SHIFT) + 1;
+			val = (val & XILINX_PCIE_RPIFR1_INTR_MASK) >>
+				XILINX_PCIE_RPIFR1_INTR_SHIFT;
 			generic_handle_irq(irq_find_mapping(port->leg_domain,
 							    val));
 		}

@@ -524,7 +525,7 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port)
 		return -ENODEV;
 	}
 
-	port->leg_domain = irq_domain_add_linear(pcie_intc_node, 4,
+	port->leg_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
 						 &intx_domain_ops,
 						 port);
 	if (!port->leg_domain) {

2.14.1

The devicetree binding documentation for the Xilinx NWL PCIe root port bridge shows an example which uses an interrupt-map property to map PCI INTx interrupts to hardware IRQ numbers 1-4. The driver creates an IRQ domain with size 4, which therefore covers the hwirq range 0-3.

This means that if we attempt to make use of the INTD interrupt then we're likely to hit a WARN() in irq_domain_associate() because INTD, or hwirw=4, is outside of the range covered by the IRQ domain. irq_domain_associate() will then return -EINVAL and we'll be unable to make use of INTD.

Fix this by making use of the pci_irqd_intx_xlate() helper function to translate the 1-4 range used in the DT to a 0-3 range used within the driver, and stop adding 1 to decoded hwirq numbers.

Whilst cleaning up INTx handling we make use of the new PCI_NUM_INTX macro & drop the custom INTX definitions.

---
I have only build tested this. The problem is identical to that in the pcie-xilinx driver, which is fixed similarly in an earlier patch.

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pcie-xilinx-nwl.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index eec641a34fc5..573847f4b9bc 100644
@@ -133,7 +133,6 @@
 #define CFG_DMA_REG_BAR			GENMASK(2, 0)
 
 #define INT_PCI_MSI_NR			(2 * 32)
-#define INTX_NUM			4
 
 /* Readin the PS_LINKUP */
 #define PS_LINKUP_OFFSET		0x00000238

@@ -334,9 +333,8 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
 
 	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
 				MSGF_LEG_SR_MASKALL) != 0) {
-		for_each_set_bit(bit, &status, INTX_NUM) {
-			virq = irq_find_mapping(pcie->legacy_irq_domain,
-						bit + 1);
+		for_each_set_bit(bit, &status, PCI_NUM_INTX) {
+			virq = irq_find_mapping(pcie->legacy_irq_domain, bit);
 			if (virq)
 				generic_handle_irq(virq);
 		}

@@ -436,6 +434,7 @@ static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
 
 static const struct irq_domain_ops legacy_domain_ops = {
 	.map = nwl_legacy_map,
+	.xlate = pci_irqd_intx_xlate,
 };
 
 #ifdef CONFIG_PCI_MSI

@@ -559,7 +558,7 @@ static int nwl_pcie_init_irq_domain(struct nwl_pcie *pcie)
 	}
 
 	pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
-							INTX_NUM,
+							PCI_NUM_INTX,
 							&legacy_domain_ops,
 							pcie);
 

2.14.1

Switch from using a custom LEGACY_IRQ_NUM macro to the generic PCI_NUM_INTX definition for the number of INTx interrupts.

---
I have only build tested this.

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pci-aardvark.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 5fb9b620ac78..89f4e3d072d7 100644
@@ -191,7 +191,6 @@
 #define LINK_WAIT_USLEEP_MIN		90000
 #define LINK_WAIT_USLEEP_MAX		100000
 
-#define LEGACY_IRQ_NUM			4
 #define MSI_IRQ_NUM			32
 
 struct advk_pcie {

@@ -729,7 +728,7 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie)
 	irq_chip->irq_unmask = advk_pcie_irq_unmask;
 
 	pcie->irq_domain =
-		irq_domain_add_linear(pcie_intc_node, LEGACY_IRQ_NUM,
+		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
 				      &advk_pcie_irq_domain_ops, pcie);
 	if (!pcie->irq_domain) {
 		dev_err(dev, "Failed to get a INTx IRQ domain\n");

@@ -786,7 +785,7 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie)
 		advk_pcie_handle_msi(pcie);
 
 	/* Process legacy interrupts */
-	for (i = 0; i < LEGACY_IRQ_NUM; i++) {
+	for (i = 0; i < PCI_NUM_INTX; i++) {
 		if (!(status & PCIE_ISR0_INTX_ASSERT(i)))
 			continue;
 

2.14.1

Hello,

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Use the PCI_NUM_INTX macro to indicate the number of PCI INTx interrupts rather than the magic number 4. This makes it clearer where the number comes from & what it relates to.

---
I have only build tested this.

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pci-ftpci100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5162dffc102b..1f1f780182de 100644
@@ -355,7 +355,7 @@ static int faraday_pci_setup_cascaded_irq(struct faraday_pci *p)
 		return -EINVAL;
 	}
 
-	p->irqdomain = irq_domain_add_linear(intc, 4,
+	p->irqdomain = irq_domain_add_linear(intc, PCI_NUM_INTX,
 					     &faraday_pci_irqdomain_ops, p);
 	if (!p->irqdomain) {
 		dev_err(p->dev, "failed to create Gemini PCI IRQ domain\n");

2.14.1

Use the PCI_NUM_INTX macro to indicate the number of PCI INTx interrupts rather than the magic number 4. This makes it clearer where the number comes from & what it relates to.

---
I have only build tested this.

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/host/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7bb9870f6d8c..eae195ca06ab 100644
@@ -1116,7 +1116,7 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
 		return -EINVAL;
 	}
 
-	rockchip->irq_domain = irq_domain_add_linear(intc, 4,
+	rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
 						    &intx_domain_ops, rockchip);
 	if (!rockchip->irq_domain) {
 		dev_err(dev, "failed to get a INTx IRQ domain\n");

2.14.1

Thanks for updating these, Paul. I applied them on pci/irq-intx and the individual driver branches. I added Thomas' reviewed-by for Aardvark, and I expect we'll get more for the others.

I found two more places we can use PCI_NUM_INTX; I'll post those in a minute.

Thanks for all your work on this!

Hi Bjorn,

Great, thanks :)

Both look good to me - I'd only looked under drivers/pci/host/ so drivers/pci/ dwc/ had escaped my gaze.

You're welcome, thanks for merging.

Paul

Hi Paul,

Nice work!

For pcie-rockchip,

and for the whole series,