Support for ARM64 ACPI based PCI host controller

From the functionality point of view this series may be split into the

According to PCI firmware specifications, on systems booting with ACPI, PCI configuration for a host bridge must be set-up through the MCFG table regions for non-hotpluggable bridges and _CBA method for hotpluggable ones.

Current MCFG table handling code, as implemented for x86, cannot be easily generalized owing to x86 specific quirks handling and related code, which makes it hard to reuse on other architectures.

In order to implement MCFG PCI configuration handling for new platforms booting with ACPI (eg ARM64) this patch re-implements MCFG handling from scratch in a streamlined fashion and provides (through a generic interface available to all arches):

- Simplified MCFG table parsing (executed through the pci_mmcfg_late_init() hook as in current x86)
- MCFG regions look-up interface through domain:bus_start:bus_end tuple

The new MCFG regions handling interface is added to generic ACPI code so that existing architectures (eg x86) can be moved over to it and architectures relying on MCFG for ACPI PCI config space can rely on it without having to resort to arch specific implementations.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Jayachandran C <[email protected]>

Reviewed-by Lorenzo Pieralisi <[email protected]>

---
 drivers/acpi/Kconfig     |  3 ++
 drivers/acpi/Makefile    |  1 +
 drivers/acpi/pci_mcfg.c  | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  2 ++
 include/linux/pci.h      |  2 +-
 5 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pci_mcfg.c

drivers/acpi/Kconfig +3 -0

index b7e2e77..f98c328 100644
@@ -217,6 +217,9 @@ config ACPI_PROCESSOR_IDLE
 	bool
 	select CPU_IDLE
 
+config ACPI_MCFG
+	bool
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR

drivers/acpi/Makefile +1 -0

index 251ce85..632e81f 100644
@@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
+obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o

drivers/acpi/pci_mcfg.c new file +39 -0

new file mode 100644
index 0000000..d3c3e85
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ *	Author: Jayachandran C <[email protected]>
+ * Copyright (C) 2016 Semihalf
+ * 	Author: Tomasz Nowicki <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * 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 version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+
+#define pr_fmt(fmt) "ACPI: " fmt
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+
+/* Structure to hold entries from the MCFG table */
+struct mcfg_entry {
+	struct list_head	list;
+	phys_addr_t		addr;
+	u16			segment;
+	u8			bus_start;
+	u8			bus_end;
+};
+
+/* List to save mcfg entries */
+static LIST_HEAD(pci_mcfg_list);
+
+phys_addr_t pci_mcfg_lookup(u16 seg, struct resource *bus_res)
+{

drivers/acpi/pci_mcfg.c new file +43 -0

+	}
+
+	return 0;
+}
+
+static __init int pci_mcfg_parse(struct acpi_table_header *header)
+{
+	struct acpi_table_mcfg *mcfg;
+	struct acpi_mcfg_allocation *mptr;
+	struct mcfg_entry *e, *arr;
+	int i, n;
+
+	if (header->length < sizeof(struct acpi_table_mcfg))
+		return -EINVAL;
+
+	n = (header->length - sizeof(struct acpi_table_mcfg)) /
+					sizeof(struct acpi_mcfg_allocation);
+	mcfg = (struct acpi_table_mcfg *)header;
+	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+
+	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
+	if (!arr)
+		return -ENOMEM;
+
+	for (i = 0, e = arr; i < n; i++, mptr++, e++) {
+		e->segment = mptr->pci_segment;
+		e->addr =  mptr->address;
+		e->bus_start = mptr->start_bus_number;
+		e->bus_end = mptr->end_bus_number;
+		list_add(&e->list, &pci_mcfg_list);
+	}
+
+	pr_info("MCFG table detected, %d entries\n", n);
+	return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mmcfg_late_init(void)
+{
+	int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+	if (err)
+		pr_err("Failed to parse MCFG (%d)\n", err);
+}

include/linux/pci-acpi.h +2 -0

index 89ab057..7d63a66 100644
@@ -24,6 +24,8 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 }
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
+extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource *bus_res);
+
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
 	struct pci_bus *pbus = pdev->bus;

include/linux/pci.h +1 -1

index 12349de..ce03d65 100644
@@ -1723,7 +1723,7 @@ void pcibios_free_irq(struct pci_dev *dev);
 extern struct dev_pm_ops pcibios_pm_ops;
 #endif
 
-#ifdef CONFIG_PCI_MMCONFIG
+#if defined(CONFIG_PCI_MMCONFIG) || defined(CONFIG_ACPI_MCFG)
 void __init pci_mmcfg_early_init(void);
 void __init pci_mmcfg_late_init(void);
 #else

Signature · 1 line

1.9.1

Change the way PCI bus domain number is assigned and improve function name to reflect what function does. No functional changes.

Instead of assigning bus domain number inside of pci_bus_assign_domain_nr() simply return domain number and let pci_create_root_bus() do assignment. This way pci_create_root_bus() setups bus structure data in the consistent way. Since pci_bus_assign_domain_nr() now does not assign but retrieves domain number instead, rename it to pci_bus_find_domain_nr().

Signed-off-by Tomasz Nowicki <[email protected]>

---
 drivers/pci/pci.c   | 4 ++--
 drivers/pci/probe.c | 4 +++-
 include/linux/pci.h | 7 +------
 3 files changed, 6 insertions(+), 9 deletions(-)

drivers/pci/pci.c +2 -2

index eb431b5..b9a7833 100644
@@ -4941,7 +4941,7 @@ int pci_get_new_domain_nr(void)
 }
 
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
 	int domain = -1;

@@ -4985,7 +4985,7 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 		domain = -1;
 	}
 
-	bus->domain_nr = domain;
+	return domain;
 }
 #endif
 #endif

drivers/pci/probe.c +3 -1

index 8e3ef72..380d46d 100644
@@ -2127,7 +2127,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->sysdata = sysdata;
 	b->ops = ops;
 	b->number = b->busn_res.start = bus;
-	pci_bus_assign_domain_nr(b, parent);
+#ifdef CONFIG_PCI_DOMAINS_GENERIC
+	b->domain_nr = pci_bus_find_domain_nr(b, parent);
+#endif
 	b2 = pci_find_bus(pci_domain_nr(b), bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */

include/linux/pci.h +1 -6

index ce03d65..48839e8 100644
@@ -1390,12 +1390,7 @@ static inline int pci_domain_nr(struct pci_bus *bus)
 {
 	return bus->domain_nr;
 }
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent);
-#else
-static inline void pci_bus_assign_domain_nr(struct pci_bus *bus,
-					struct device *parent)
-{
-}
+int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent);
 #endif
 
 /* some architectures require additional setup to direct VGA traffic */

Signature · 1 line

1.9.1

pci_bus_find_domain_nr() retrieves the host bridge domain number in a DT specific way. Factor our pci_bus_find_domain_nr() in a separate DT

function (ie of_pci_bus_find_domain_nr()) so that DT code is self contained, paving the way for retrieving domain number in pci_bus_find_domain_nr() with additional firmware methods (ie ACPI).

Signed-off-by Tomasz Nowicki <[email protected]>

---
 drivers/pci/pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/pci/pci.c +6 -1

index b9a7833..327828d 100644
@@ -4941,7 +4941,7 @@ int pci_get_new_domain_nr(void)
 }
 
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
-int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
+static int of_pci_bus_find_domain_nr(struct device *parent)
 {
 	static int use_dt_domains = -1;
 	int domain = -1;

@@ -4987,6 +4987,11 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
 
 	return domain;
 }
+
+int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+	return of_pci_bus_find_domain_nr(parent);
+}
 #endif
 #endif
 

Signature · 1 line

1.9.1

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -3

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		return acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }

Signature · 1 line

1.9.1

Quoted 6 lines · Tomasz Nowicki

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

Uh, OK :) I can't figure out exactly what the problem is here -- I don't know where to look if I wanted to fix the scan handler ordering issues, and I don't know how I could tell if it would ever be safe to move this from driver probe-time back to device add-time.

I also notice that x86 and ia64 call acpi_pci_irq_enable() even later, when the driver *enables* the device. Is there a reason you didn't do it at the same time as x86 and ia64? This is another of those pcibios hooks that really don't do anything arch-specific, so I can imagine refactoring this somehow, someday.

Did we have this conversation before? It seems vaguely familiar, so I apologize if you already explained this once.

Quoted 43 lines

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
  arch/arm64/kernel/pci.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -4

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
  }
 
  /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
   */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
  {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		return acpi_pci_irq_enable(dev);
+#endif
 
  	return 0;
  }
--

1.9.1

Quoted 6 lines · Tomasz Nowicki

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

Uh, OK :) I can't figure out exactly what the problem is here -- I don't know where to look if I wanted to fix the scan handler ordering issues, and I don't know how I could tell if it would ever be safe to move this from driver probe-time back to device add-time.

I also notice that x86 and ia64 call acpi_pci_irq_enable() even later, when the driver *enables* the device. Is there a reason you didn't do it at the same time as x86 and ia64? This is another of those pcibios hooks that really don't do anything arch-specific, so I can imagine refactoring this somehow, someday.

Did we have this conversation before? It seems vaguely familiar, so I apologize if you already explained this once.

Quoted 42 lines

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -3

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		return acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }

Signature · 1 line

1.9.1

Quoted 8 lines · Bjorn Helgaas

Quoted 6 lines · Tomasz Nowicki

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

Quoted 4 lines

Uh, OK :) I can't figure out exactly what the problem is here -- I don't know where to look if I wanted to fix the scan handler ordering issues, and I don't know how I could tell if it would ever be safe to move this from driver probe-time back to device add-time.

Right, the commit log could have been more informative.

pcibios_add_device() was added in:

commit d1e6dc91b532 ("arm64: Add architectural support for PCI")

whose commit log does not specify why legacy IRQ parsing should be done at pcibios_add_device() either, so honestly we had to do with the information we have at hand.

Quoted 6 lines

I also notice that x86 and ia64 call acpi_pci_irq_enable() even later, when the driver *enables* the device. Is there a reason you didn't do it at the same time as x86 and ia64? This is another of those pcibios hooks that really don't do anything arch-specific, so I can imagine refactoring this somehow, someday.

Yes, with [1], that was the goal, that stopped because [1] does not work on x86.

Only DT platform(s) affected by this change are all platforms relying on drivers/pci/host/pci-xgene.c (others rely on pci_fixup_irqs() that should be removed too), if on those platforms probing IRQs at device enable time works ok I can update this patch (it can be done through [1] once we figure out what to do with it on x86) and move the IRQ set-up at pcibios_enable_device() time.

@Duc: any feedback on this ?

Thanks,
Lorenzo

[1] http://www.spinics.net/lists/linux-pci/msg45950.html

Quoted 46 lines

Did we have this conversation before? It seems vaguely familiar, so I apologize if you already explained this once.

Quoted 42 lines

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -3

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		return acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }

Signature · 1 line

1.9.1

Quoted 8 lines · Bjorn Helgaas

Quoted 6 lines · Tomasz Nowicki

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

Quoted 4 lines

Uh, OK :) I can't figure out exactly what the problem is here -- I don't know where to look if I wanted to fix the scan handler ordering issues, and I don't know how I could tell if it would ever be safe to move this from driver probe-time back to device add-time.

Right, the commit log could have been more informative.

pcibios_add_device() was added in:

commit d1e6dc91b532 ("arm64: Add architectural support for PCI")

whose commit log does not specify why legacy IRQ parsing should be done at pcibios_add_device() either, so honestly we had to do with the information we have at hand.

Quoted 5 lines

I also notice that x86 and ia64 call acpi_pci_irq_enable() even later, when the driver *enables* the device. Is there a reason you didn't do it at the same time as x86 and ia64? This is another of those pcibios hooks that really don't do anything arch-specific, so I can imagine refactoring this somehow, someday.

Yes, with [1], that was the goal, that stopped because [1] does not work on x86.

Only DT platform(s) affected by this change are all platforms relying on drivers/pci/host/pci-xgene.c (others rely on pci_fixup_irqs() that should be removed too), if on those platforms probing IRQs at device enable time works ok I can update this patch (it can be done through [1] once we figure out what to do with it on x86) and move the IRQ set-up at pcibios_enable_device() time.

@Duc: any feedback on this ?

Thanks,
Lorenzo

[1] http://www.spinics.net/lists/linux-pci/msg45950.html

Quoted 46 lines

Did we have this conversation before? It seems vaguely familiar, so I apologize if you already explained this once.

Quoted 42 lines

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -4

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-   dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+   if (acpi_disabled)
+           dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+   else
+           return acpi_pci_irq_enable(dev);
+#endif
 
    return 0;
 }
--

1.9.1

Quoted 34 lines · Lorenzo Pieralisi

Quoted 8 lines · Bjorn Helgaas

Quoted 6 lines · Tomasz Nowicki

To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

Quoted 4 lines

Uh, OK :) I can't figure out exactly what the problem is here -- I don't know where to look if I wanted to fix the scan handler ordering issues, and I don't know how I could tell if it would ever be safe to move this from driver probe-time back to device add-time.

Right, the commit log could have been more informative.

pcibios_add_device() was added in:

commit d1e6dc91b532 ("arm64: Add architectural support for PCI")

whose commit log does not specify why legacy IRQ parsing should be done at pcibios_add_device() either, so honestly we had to do with the information we have at hand.

Quoted 5 lines

I also notice that x86 and ia64 call acpi_pci_irq_enable() even later, when the driver *enables* the device. Is there a reason you didn't do it at the same time as x86 and ia64? This is another of those pcibios hooks that really don't do anything arch-specific, so I can imagine refactoring this somehow, someday.

Yes, with [1], that was the goal, that stopped because [1] does not work on x86.

Quoted 8 lines

Only DT platform(s) affected by this change are all platforms relying on drivers/pci/host/pci-xgene.c (others rely on pci_fixup_irqs() that should be removed too), if on those platforms probing IRQs at device enable time works ok I can update this patch (it can be done through [1] once we figure out what to do with it on x86) and move the IRQ set-up at pcibios_enable_device() time.

@Duc: any feedback on this ?

Hi Lorenzo,

The changes to add pcibios_alloc_irq works fine on X-Gene PCIe

I also tried to remove pcibios_alloc_irq and move its code into pcibios_enable_device
after pci_enable_resource call and legacy IRQ also works.

Can you also point me to the discussion thread or some info. about the issue on x86 with [1]?
I want to check if there is any more test case I need to verify.

Regards,
Duc Dang.

Quoted 52 lines

Thanks,
Lorenzo

[1] http://www.spinics.net/lists/linux-pci/msg45950.html

Quoted 46 lines

Did we have this conversation before? It seems vaguely familiar, so I apologize if you already explained this once.

Quoted 42 lines

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code.

Signed-off-by Tomasz Nowicki <[email protected]>

Suggested-by Lorenzo Pieralisi <[email protected]>

---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

arch/arm64/kernel/pci.c +8 -4

index d5d3d26..b3b8a2c 100644
@@ -51,11 +51,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-   dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+   if (acpi_disabled)
+           dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+   else
+           return acpi_pci_irq_enable(dev);
+#endif
 
    return 0;
 }
--

1.9.1

ACPI spec6.1 - chapter: 5.5.2.4 defines OperationRegion (Declare Operation Region). Following the spec: " [...] An Operation Region is a specific region of operation within an address space that is declared as a subset of the entire address space using a starting address (offset) and a length. Control methods must have exclusive access to any address accessed via fields declared in Operation Regions. [...]".

OperationRegion allows to declare various of operation region address space

identifiers including PCI_Config. PCI_Config is meant to access PCI configuration space from the ASL. So every time ASL opcode operates on PCI_Config space region, ASL interpreter dispatches accesses to OS low-level calls - raw_pci_write() and raw_pci_read() for Linux - so-called ACPI RAW accessors.

In order to support PCI_Config operation region, implement mentioned raw_pci_write() and raw_pci_read() calls so they find associated bus and call read/write ops.

Waiting for clarification in the ACPI specifications in relation to PCI_Config space handling before PCI bus enumeration is completed, current code does not support PCI_Config region accesses before PCI bus enumeration whilst providing full AML PCI_Config access availability when the PCI bus enumeration is completed by the kernel so that RAW accessors can look-up PCI operations through the struct pci_bus associated with a PCI bus.

Signed-off-by Tomasz Nowicki <[email protected]>

Signed-off-by Jayachandran C <[email protected]>

---
 arch/arm64/kernel/pci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/kernel/pci.c +10 -2

index b3b8a2c..328f857 100644
@@ -71,13 +71,21 @@ int pcibios_alloc_irq(struct pci_dev *dev)
 int raw_pci_read(unsigned int domain, unsigned int bus,
 		  unsigned int devfn, int reg, int len, u32 *val)
 {
-	return -ENXIO;
+	struct pci_bus *b = pci_find_bus(domain, bus);
+
+	if (!b)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return b->ops->read(b, devfn, reg, len, val);
 }
 
 int raw_pci_write(unsigned int domain, unsigned int bus,
 		unsigned int devfn, int reg, int len, u32 val)
 {
-	return -ENXIO;
+	struct pci_bus *b = pci_find_bus(domain, bus);
+
+	if (!b)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return b->ops->write(b, devfn, reg, len, val);
 }
 
 #ifdef CONFIG_NUMA

Signature · 1 line

1.9.1

Implement pci_acpi_scan_root and other arch-specific call so that ARM64 can start using ACPI to setup and enumerate PCI buses.

Prior to buses enumeration the pci_acpi_scan_root() implementation looks for configuration space start address (obtained through ACPI _CBA method or MCFG interface). If succeed, it uses ECAM library to create new mapping. Then it attaches generic ECAM ops (pci_generic_ecam_ops) which are used for accessing configuration space later on.
...

arch/arm64/kernel/pci.c +10 -2

+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
+	int node = acpi_get_node(root->device->handle);
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+
+	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);

Hi Tomasz,

Quoted 24 lines · Tomasz Nowicki

Implement pci_acpi_scan_root and other arch-specific call so that ARM64 can start using ACPI to setup and enumerate PCI buses.

Prior to buses enumeration the pci_acpi_scan_root() implementation looks for configuration space start address (obtained through ACPI _CBA method or MCFG interface). If succeed, it uses ECAM library to create new mapping. Then it attaches generic ECAM ops (pci_generic_ecam_ops) which are used for accessing configuration space later on.
...

(quoted diff) +10 -2

+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
+	int node = acpi_get_node(root->device->handle);
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+
+	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!ri)
+		return NULL;
+
+	ri->cfg = pci_acpi_setup_ecam_mapping(root);
+	if (!ri->cfg) {
+		kfree(ri);
+		return NULL;
+	}
+
+	acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;

This has already been merged, but this isn't right, is it? We're writing a host controller-specific pointer into the single system-wide acpi_pci_root_ops, then passing it on to acpi_pci_root_create().

Today, I think ri->cfg->ops->pci_ops is always &pci_generic_ecam_ops, from this path:

ri->cfg = pci_acpi_setup_ecam_mapping

    cfg = pci_ecam_create(..., &pci_generic_ecam_ops)
      cfg = kzalloc(...)
      cfg->ops = ops             # &pci_generic_ecam_ops

But we're about to merge the ECAM quirks series, which will mean it may not be &pci_generic_ecam_ops. Even apart from the ECAM quirks, we should avoid this pattern of putting device-specific info in a single shared structure because it's too difficult to verify that it's correct.

Hi Bjorn,

Quoted 47 lines · Bjorn Helgaas

Hi Tomasz,

Quoted 24 lines · Tomasz Nowicki

Implement pci_acpi_scan_root and other arch-specific call so that ARM64 can start using ACPI to setup and enumerate PCI buses.

Prior to buses enumeration the pci_acpi_scan_root() implementation looks for configuration space start address (obtained through ACPI _CBA method or MCFG interface). If succeed, it uses ECAM library to create new mapping. Then it attaches generic ECAM ops (pci_generic_ecam_ops) which are used for accessing configuration space later on.
...

(quoted diff) +5 -0

+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */

struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) {

(quoted diff) +5 -2

-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
+	int node = acpi_get_node(root->device->handle);
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+
+	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!ri)
+		return NULL;
+
+	ri->cfg = pci_acpi_setup_ecam_mapping(root);
+	if (!ri->cfg) {
+		kfree(ri);
+		return NULL;
+	}
+
+	acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;

This has already been merged, but this isn't right, is it? We're writing a host controller-specific pointer into the single system-wide acpi_pci_root_ops, then passing it on to acpi_pci_root_create().

Today, I think ri->cfg->ops->pci_ops is always &pci_generic_ecam_ops, from this path:

ri->cfg = pci_acpi_setup_ecam_mapping
cfg = pci_ecam_create(..., &pci_generic_ecam_ops)

Quoted 9 lines

     cfg = kzalloc(...)
     cfg->ops = ops             # &pci_generic_ecam_ops

But we're about to merge the ECAM quirks series, which will mean it may not be &pci_generic_ecam_ops. Even apart from the ECAM quirks, we should avoid this pattern of putting device-specific info in a single shared structure because it's too difficult to verify that it's correct.

Well spotted. I agree, we need to fix this. How about this:

arch/arm64/kernel/pci.c +10 -7

index fb439c7..31c0e1c 100644
@@ -152,33 +152,35 @@ static void
 pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
 
        ri = container_of(ci, struct acpi_pci_generic_root_info, common);
        pci_ecam_free(ri->cfg);
+       kfree(ci->ops);
        kfree(ri);
 }
 
-static struct acpi_pci_root_ops acpi_pci_root_ops = {
-       .release_info = pci_acpi_generic_release_info,
-};
-
 /* Interface called from ACPI code to setup PCI host controller */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
        int node = acpi_get_node(root->device->handle);
        struct acpi_pci_generic_root_info *ri;
        struct pci_bus *bus, *child;
+       struct acpi_pci_root_ops *root_ops;
 
        ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
        if (!ri)
                return NULL;
 
+       root_ops = kzalloc_node(sizeof(*root_ops), GFP_KERNEL, node);
+       if (!root_ops)
+               return NULL;
+
        ri->cfg = pci_acpi_setup_ecam_mapping(root);
        if (!ri->cfg) {
                kfree(ri);
+               kfree(root_ops);
                return NULL;
        }
 
-       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
-       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
-                                  ri->cfg);
+       root_ops->release_info = pci_acpi_generic_release_info;
+       root_ops->pci_ops = &ri->cfg->ops->pci_ops;
+       bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
        if (!bus)
                return NULL;

Of course, this should be the part of ECAM quirks core patches.

The other option we have is to remove "struct pci_ops *pci_ops;" from acpi_pci_root_ops structure and pass struct pci_ops as an extra argument to acpi_pci_root_create(). What do you think?

Quoted 112 lines · Tomasz Nowicki

Hi Bjorn,

Quoted 47 lines · Bjorn Helgaas

Hi Tomasz,

Quoted 24 lines · Tomasz Nowicki

Implement pci_acpi_scan_root and other arch-specific call so that ARM64 can start using ACPI to setup and enumerate PCI buses.

Prior to buses enumeration the pci_acpi_scan_root() implementation looks for configuration space start address (obtained through ACPI _CBA method or MCFG interface). If succeed, it uses ECAM library to create new mapping. Then it attaches generic ECAM ops (pci_generic_ecam_ops) which are used for accessing configuration space later on.
...

(quoted diff) +5 -0

+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */

struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) {

(quoted diff) +5 -2

-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
+	int node = acpi_get_node(root->device->handle);
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+
+	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!ri)
+		return NULL;
+
+	ri->cfg = pci_acpi_setup_ecam_mapping(root);
+	if (!ri->cfg) {
+		kfree(ri);
+		return NULL;
+	}
+
+	acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;

This has already been merged, but this isn't right, is it? We're writing a host controller-specific pointer into the single system-wide acpi_pci_root_ops, then passing it on to acpi_pci_root_create().

Today, I think ri->cfg->ops->pci_ops is always &pci_generic_ecam_ops, from this path:

ri->cfg = pci_acpi_setup_ecam_mapping
cfg = pci_ecam_create(..., &pci_generic_ecam_ops)

Quoted 9 lines

     cfg = kzalloc(...)
     cfg->ops = ops             # &pci_generic_ecam_ops

But we're about to merge the ECAM quirks series, which will mean it may not be &pci_generic_ecam_ops. Even apart from the ECAM quirks, we should avoid this pattern of putting device-specific info in a single shared structure because it's too difficult to verify that it's correct.

Well spotted. I agree, we need to fix this. How about this:

arch/arm64/kernel/pci.c +10 -7

index fb439c7..31c0e1c 100644
@@ -152,33 +152,35 @@ static void
 pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
 
        ri = container_of(ci, struct acpi_pci_generic_root_info, common);
        pci_ecam_free(ri->cfg);
+       kfree(ci->ops);
        kfree(ri);
 }
 
-static struct acpi_pci_root_ops acpi_pci_root_ops = {
-       .release_info = pci_acpi_generic_release_info,
-};
-
 /* Interface called from ACPI code to setup PCI host controller */
 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 {
        int node = acpi_get_node(root->device->handle);
        struct acpi_pci_generic_root_info *ri;
        struct pci_bus *bus, *child;
+       struct acpi_pci_root_ops *root_ops;
 
        ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
        if (!ri)
                return NULL;
 
+       root_ops = kzalloc_node(sizeof(*root_ops), GFP_KERNEL, node);
+       if (!root_ops)
+               return NULL;
+
        ri->cfg = pci_acpi_setup_ecam_mapping(root);
        if (!ri->cfg) {
                kfree(ri);
+               kfree(root_ops);
                return NULL;
        }
 
-       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
-       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
-                                  ri->cfg);
+       root_ops->release_info = pci_acpi_generic_release_info;
+       root_ops->pci_ops = &ri->cfg->ops->pci_ops;
+       bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
        if (!bus)
                return NULL;

Of course, this should be the part of ECAM quirks core patches.

Quoted 3 lines

The other option we have is to remove "struct pci_ops *pci_ops;" from acpi_pci_root_ops structure and pass struct pci_ops as an extra argument to acpi_pci_root_create(). What do you think?

I think your patch above is fine and avoids the need to change the x86 and ia64 code. Would you mind packaging this up with a changelog and signed-off-by? I can take care of putting it in the ECAM series.

From the functionality point of view this series may be split into the following logic parts:
1. Export ECAM API and add parent device to pci_config_window
2. Add IO resources handling to PCI core code
3. New MCFG driver
4. Cleanups and support for generic domain assignment based on ACPI
5. Implement ARM64 ACPI based PCI host controller driver under arch/arm64/

Quoted 9 lines · Tomasz Nowicki

From the functionality point of view this series may be split into the following logic parts:
1. Export ECAM API and add parent device to pci_config_window
2. Add IO resources handling to PCI core code
3. New MCFG driver
4. Cleanups and support for generic domain assignment based on ACPI
5. Implement ARM64 ACPI based PCI host controller driver under arch/arm64/

This is beautiful! It really turned out well.

I applied all these to pci/arm64-acpi for v4.8.

Bjorn

Quoted 7 lines · Tomasz Nowicki

From the functionality point of view this series may be split into the following logic parts:
1. Export ECAM API and add parent device to pci_config_window
2. Add IO resources handling to PCI core code
3. New MCFG driver
4. Cleanups and support for generic domain assignment based on ACPI
5. Implement ARM64 ACPI based PCI host controller driver under arch/arm64/

This is beautiful! It really turned out well.

I applied all these to pci/arm64-acpi for v4.8.

Quoted 7 lines · Tomasz Nowicki

From the functionality point of view this series may be split into the
following logic parts:
1. Export ECAM API and add parent device to pci_config_window
2. Add IO resources handling to PCI core code
3. New MCFG driver
4. Cleanups and support for generic domain assignment based on ACPI
5. Implement ARM64 ACPI based PCI host controller driver under arch/arm64/

This is beautiful! It really turned out well.

I applied all these to pci/arm64-acpi for v4.8.