[PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Tomasz Nowicki]

>From the functionality point of view this series might be split into two logic parts:
1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
   PCI config regions and used when necessary.
2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
   initialization for ARM64

Patches has been built on top of:
[Patch v7 0/7] Consolidate ACPI PCI root common code into ACPI core
https://lkml.org/lkml/2015/10/14/31
Git branch can be found here:
https://git.linaro.org/leg/acpi/acpi.git/shortlog/refs/heads/pci-acpi-upstream

This has been tested on Cavium ThunderX 1 socket server.
Any help in reviewing and testing is very appreciated.

Hanjun Guo (1):
  XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

Tomasz Nowicki (10):
  x86, pci: Reorder logic of pci_mmconfig_insert() function
  x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
    out of arch/x86/ directory
  pci, acpi, mcfg: Provide generic implementation of MCFG code
    initialization.
  x86, pci: mmconfig_{32,64}.c code refactoring - remove code
    duplication.
  x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
    driver.
  pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
  pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
    added or not.
  x86, pci: Use previously added ECAM hot_added flag to remove ECAM
    regions.
  pci, acpi: Provide generic way to assign bus domain number.
  arm64, pci, acpi: Support for ACPI based PCI hostbridge init

 arch/arm64/Kconfig             |   6 +
 arch/arm64/kernel/pci.c        | 208 ++++++++++++++++++++++++++++++++--
 arch/x86/Kconfig               |   4 +
 arch/x86/include/asm/pci_x86.h |  28 +----
 arch/x86/pci/acpi.c            |  17 +--
 arch/x86/pci/mmconfig-shared.c | 250 +++++++----------------------------------
 arch/x86/pci/mmconfig_32.c     |  11 +-
 arch/x86/pci/mmconfig_64.c     |  67 +----------
 arch/x86/pci/numachip.c        |   1 +
 drivers/acpi/Makefile          |   1 +
 drivers/acpi/mcfg.c            | 104 +++++++++++++++++
 drivers/acpi/pci_root.c        |   2 +-
 drivers/pci/Kconfig            |  10 ++
 drivers/pci/Makefile           |   5 +
 drivers/pci/ecam.c             | 234 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c              |  30 ++++-
 drivers/xen/pci.c              |   7 +-
 include/linux/acpi.h           |   2 +
 include/linux/ecam.h           |  44 ++++++++
 19 files changed, 691 insertions(+), 340 deletions(-)
 create mode 100644 drivers/acpi/mcfg.c
 create mode 100644 drivers/pci/ecam.c
 create mode 100644 include/linux/ecam.h

-- 
1.9.1

[PATCH V1 01/11] x86, pci: Reorder logic of pci_mmconfig_insert() function

[Tomasz Nowicki]

This patch is the first step for MMCONFIG refactoring process.

Code that uses pci_mmcfg_lock will be moved to common file and become
accessible for all architectures. pci_mmconfig_insert() cannot be moved
so easily since it is mixing generic mmconfig code with x86 specific logic
inside of mutual exclusive block guarded by pci_mmcfg_lock.

To get rid of that constraint, we reorder actions as follow:
1. sanity check for mmconfig region presence, if we already have such
region it doesn't make snese to alloc new mmconfig list entry
2. mmconfig entry allocation, no need to lock
3. insertion to iomem_resource has its own lock, no need to wrap it into mutex
4. insertion to mmconfig list can be done as the final step in separate
function (candidate for further refactoring) and needs another mmconfig
lookup to avoid race condition.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/pci/mmconfig-shared.c | 101 +++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 43 deletions(-)

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index dd30b7e..c8bb9b0 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -720,6 +720,38 @@ static int __init pci_mmcfg_late_insert_resources(void)
  */
 late_initcall(pci_mmcfg_late_insert_resources);
 
+static int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
+{
+	struct pci_mmcfg_region *cfg_conflict;
+	int err = 0;
+
+	mutex_lock(&pci_mmcfg_lock);
+	cfg_conflict = pci_mmconfig_lookup(cfg->segment, cfg->start_bus);
+	if (cfg_conflict) {
+		if (cfg_conflict->end_bus < cfg->end_bus)
+			pr_info(FW_INFO "MMCONFIG for "
+				"domain %04x [bus %02x-%02x] "
+				"only partially covers this bridge\n",
+				cfg_conflict->segment, cfg_conflict->start_bus,
+				cfg_conflict->end_bus);
+		err = -EEXIST;
+		goto out;
+	}
+
+	if (pci_mmcfg_arch_map(cfg)) {
+		pr_warn("fail to map MMCONFIG %pR.\n", &cfg->res);
+		err = -ENOMEM;
+		goto out;
+	} else {
+		list_add_sorted(cfg);
+		pr_info("MMCONFIG at %pR (base %#lx)\n",
+			&cfg->res, (unsigned long)cfg->address);
+	}
+out:
+	mutex_unlock(&pci_mmcfg_lock);
+	return err;
+}
+
 /* Add MMCFG information for host bridges */
 int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 			phys_addr_t addr)
@@ -734,63 +766,46 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 	if (start > end)
 		return -EINVAL;
 
-	mutex_lock(&pci_mmcfg_lock);
+	rcu_read_lock();
 	cfg = pci_mmconfig_lookup(seg, start);
-	if (cfg) {
-		if (cfg->end_bus < end)
-			dev_info(dev, FW_INFO
-				 "MMCONFIG for "
-				 "domain %04x [bus %02x-%02x] "
-				 "only partially covers this bridge\n",
-				  cfg->segment, cfg->start_bus, cfg->end_bus);
-		mutex_unlock(&pci_mmcfg_lock);
+	rcu_read_unlock();
+	if (cfg)
 		return -EEXIST;
-	}
 
-	if (!addr) {
-		mutex_unlock(&pci_mmcfg_lock);
+	if (!addr)
 		return -EINVAL;
-	}
 
-	rc = -EBUSY;
 	cfg = pci_mmconfig_alloc(seg, start, end, addr);
-	if (cfg == NULL) {
+	if (!cfg) {
 		dev_warn(dev, "fail to add MMCONFIG (out of memory)\n");
-		rc = -ENOMEM;
-	} else if (!pci_mmcfg_check_reserved(dev, cfg, 0)) {
+		return -ENOMEM;
+	}
+
+	rc = -EBUSY;
+	if (!pci_mmcfg_check_reserved(dev, cfg, 0)) {
 		dev_warn(dev, FW_BUG "MMCONFIG %pR isn't reserved\n",
 			 &cfg->res);
-	} else {
-		/* Insert resource if it's not in boot stage */
-		if (pci_mmcfg_running_state)
-			tmp = insert_resource_conflict(&iomem_resource,
-						       &cfg->res);
-
-		if (tmp) {
-			dev_warn(dev,
-				 "MMCONFIG %pR conflicts with "
-				 "%s %pR\n",
-				 &cfg->res, tmp->name, tmp);
-		} else if (pci_mmcfg_arch_map(cfg)) {
-			dev_warn(dev, "fail to map MMCONFIG %pR.\n",
-				 &cfg->res);
-		} else {
-			list_add_sorted(cfg);
-			dev_info(dev, "MMCONFIG at %pR (base %#lx)\n",
-				 &cfg->res, (unsigned long)addr);
-			cfg = NULL;
-			rc = 0;
-		}
+		goto error;
 	}
 
-	if (cfg) {
-		if (cfg->res.parent)
-			release_resource(&cfg->res);
-		kfree(cfg);
+	/* Insert resource if it's not in boot stage */
+	if (pci_mmcfg_running_state)
+		tmp = insert_resource_conflict(&iomem_resource, &cfg->res);
+
+	if (tmp) {
+		dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
+			 &cfg->res, tmp->name, tmp);
+		goto error;
 	}
 
-	mutex_unlock(&pci_mmcfg_lock);
+	rc = pci_mmconfig_inject(cfg);
+	if (!rc)
+		return 0;
 
+error:
+	if (cfg->res.parent)
+		release_resource(&cfg->res);
+	kfree(cfg);
 	return rc;
 }
 
-- 
1.9.1

[PATCH V1 02/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code out of arch/x86/ directory

[Tomasz Nowicki]

ECAM standard and MCFG table are architecture independent and it makes
sense to share common code across all architectures. Both are going to
corresponding files - ecam.c and mcfg.c

While we are here, rename pci_parse_mcfg to acpi_parse_mcfg.
We already have acpi_parse_mcfg prototype which is used nowhere.
At the same time, we need pci_parse_mcfg been global so acpi_parse_mcfg
can be used perfectly here.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/x86/Kconfig               |   3 +
 arch/x86/include/asm/pci_x86.h |  21 -----
 arch/x86/pci/acpi.c            |   1 +
 arch/x86/pci/mmconfig-shared.c | 205 +----------------------------------------
 arch/x86/pci/mmconfig_32.c     |   1 +
 arch/x86/pci/mmconfig_64.c     |   1 +
 arch/x86/pci/numachip.c        |   1 +
 drivers/acpi/Makefile          |   1 +
 drivers/acpi/mcfg.c            |  59 ++++++++++++
 drivers/pci/Kconfig            |   7 ++
 drivers/pci/Makefile           |   5 +
 drivers/pci/ecam.c             | 175 +++++++++++++++++++++++++++++++++++
 drivers/xen/pci.c              |   1 +
 include/linux/acpi.h           |   2 +
 include/linux/ecam.h           |  37 ++++++++
 15 files changed, 299 insertions(+), 221 deletions(-)
 create mode 100644 drivers/acpi/mcfg.c
 create mode 100644 drivers/pci/ecam.c
 create mode 100644 include/linux/ecam.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 96d058a..4ca8134 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -128,6 +128,7 @@ config X86
 	select HAVE_MIXED_BREAKPOINTS_REGS
 	select HAVE_OPROFILE
 	select HAVE_OPTPROBES
+	select HAVE_PCI_ECAM
 	select HAVE_PCSPKR_PLATFORM
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_EVENTS_NMI
@@ -2332,6 +2333,7 @@ config PCI_DIRECT
 
 config PCI_MMCONFIG
 	def_bool y
+	select PCI_ECAM
 	depends on X86_32 && PCI && (ACPI || SFI) && (PCI_GOMMCONFIG || PCI_GOANY)
 
 config PCI_OLPC
@@ -2349,6 +2351,7 @@ config PCI_DOMAINS
 
 config PCI_MMCONFIG
 	bool "Support mmconfig PCI config space access"
+	select PCI_ECAM
 	depends on X86_64 && PCI && ACPI
 
 config PCI_CNB20LE_QUIRK
diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index fa1195d..64cb514 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -122,33 +122,12 @@ extern int pci_legacy_init(void);
 extern void pcibios_fixup_irqs(void);
 
 /* pci-mmconfig.c */
-
-/* "PCI MMCONFIG %04x [bus %02x-%02x]" */
-#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)
-
-struct pci_mmcfg_region {
-	struct list_head list;
-	struct resource res;
-	u64 address;
-	char __iomem *virt;
-	u16 segment;
-	u8 start_bus;
-	u8 end_bus;
-	char name[PCI_MMCFG_RESOURCE_NAME_LEN];
-};
-
 extern int __init pci_mmcfg_arch_init(void);
 extern void __init pci_mmcfg_arch_free(void);
 extern int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
 extern void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
 extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 			       phys_addr_t addr);
-extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);
-extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
-
-extern struct list_head pci_mmcfg_list;
-
-#define PCI_MMCFG_BUS_OFFSET(bus)      ((bus) << 20)
 
 /*
  * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 3cd6983..28c9cd6 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -5,6 +5,7 @@
 #include <linux/dmi.h>
 #include <linux/slab.h>
 #include <linux/pci-acpi.h>
+#include <linux/ecam.h>
 #include <asm/numa.h>
 #include <asm/pci_x86.h>
 
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index c8bb9b0..ce2c2e4 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/rculist.h>
+#include <linux/ecam.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 #include <asm/acpi.h>
@@ -27,103 +28,6 @@
 /* Indicate if the mmcfg resources have been placed into the resource table. */
 static bool pci_mmcfg_running_state;
 static bool pci_mmcfg_arch_init_failed;
-static DEFINE_MUTEX(pci_mmcfg_lock);
-
-LIST_HEAD(pci_mmcfg_list);
-
-static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg)
-{
-	if (cfg->res.parent)
-		release_resource(&cfg->res);
-	list_del(&cfg->list);
-	kfree(cfg);
-}
-
-static void __init free_all_mmcfg(void)
-{
-	struct pci_mmcfg_region *cfg, *tmp;
-
-	pci_mmcfg_arch_free();
-	list_for_each_entry_safe(cfg, tmp, &pci_mmcfg_list, list)
-		pci_mmconfig_remove(cfg);
-}
-
-static void list_add_sorted(struct pci_mmcfg_region *new)
-{
-	struct pci_mmcfg_region *cfg;
-
-	/* keep list sorted by segment and starting bus number */
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
-		if (cfg->segment > new->segment ||
-		    (cfg->segment == new->segment &&
-		     cfg->start_bus >= new->start_bus)) {
-			list_add_tail_rcu(&new->list, &cfg->list);
-			return;
-		}
-	}
-	list_add_tail_rcu(&new->list, &pci_mmcfg_list);
-}
-
-static struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
-						   int end, u64 addr)
-{
-	struct pci_mmcfg_region *new;
-	struct resource *res;
-
-	if (addr == 0)
-		return NULL;
-
-	new = kzalloc(sizeof(*new), GFP_KERNEL);
-	if (!new)
-		return NULL;
-
-	new->address = addr;
-	new->segment = segment;
-	new->start_bus = start;
-	new->end_bus = end;
-
-	res = &new->res;
-	res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
-	res->end = addr + PCI_MMCFG_BUS_OFFSET(end + 1) - 1;
-	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-	snprintf(new->name, PCI_MMCFG_RESOURCE_NAME_LEN,
-		 "PCI MMCONFIG %04x [bus %02x-%02x]", segment, start, end);
-	res->name = new->name;
-
-	return new;
-}
-
-static struct pci_mmcfg_region *__init pci_mmconfig_add(int segment, int start,
-							int end, u64 addr)
-{
-	struct pci_mmcfg_region *new;
-
-	new = pci_mmconfig_alloc(segment, start, end, addr);
-	if (new) {
-		mutex_lock(&pci_mmcfg_lock);
-		list_add_sorted(new);
-		mutex_unlock(&pci_mmcfg_lock);
-
-		pr_info(PREFIX
-		       "MMCONFIG for domain %04x [bus %02x-%02x] at %pR "
-		       "(base %#lx)\n",
-		       segment, start, end, &new->res, (unsigned long)addr);
-	}
-
-	return new;
-}
-
-struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
-{
-	struct pci_mmcfg_region *cfg;
-
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
-		if (cfg->segment == segment &&
-		    cfg->start_bus <= bus && bus <= cfg->end_bus)
-			return cfg;
-
-	return NULL;
-}
 
 static const char *__init pci_mmcfg_e7520(void)
 {
@@ -543,8 +447,8 @@ static void __init pci_mmcfg_reject_broken(int early)
 	}
 }
 
-static int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
-					struct acpi_mcfg_allocation *cfg)
+int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+				 struct acpi_mcfg_allocation *cfg)
 {
 	int year;
 
@@ -566,50 +470,6 @@ static int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
 	return -EINVAL;
 }
 
-static int __init pci_parse_mcfg(struct acpi_table_header *header)
-{
-	struct acpi_table_mcfg *mcfg;
-	struct acpi_mcfg_allocation *cfg_table, *cfg;
-	unsigned long i;
-	int entries;
-
-	if (!header)
-		return -EINVAL;
-
-	mcfg = (struct acpi_table_mcfg *)header;
-
-	/* how many config structures do we have */
-	free_all_mmcfg();
-	entries = 0;
-	i = header->length - sizeof(struct acpi_table_mcfg);
-	while (i >= sizeof(struct acpi_mcfg_allocation)) {
-		entries++;
-		i -= sizeof(struct acpi_mcfg_allocation);
-	}
-	if (entries == 0) {
-		pr_err(PREFIX "MMCONFIG has no entries\n");
-		return -ENODEV;
-	}
-
-	cfg_table = (struct acpi_mcfg_allocation *) &mcfg[1];
-	for (i = 0; i < entries; i++) {
-		cfg = &cfg_table[i];
-		if (acpi_mcfg_check_entry(mcfg, cfg)) {
-			free_all_mmcfg();
-			return -ENODEV;
-		}
-
-		if (pci_mmconfig_add(cfg->pci_segment, cfg->start_bus_number,
-				   cfg->end_bus_number, cfg->address) == NULL) {
-			pr_warn(PREFIX "no memory for MCFG entries\n");
-			free_all_mmcfg();
-			return -ENOMEM;
-		}
-	}
-
-	return 0;
-}
-
 #ifdef CONFIG_ACPI_APEI
 extern int (*arch_apei_filter_addr)(int (*func)(__u64 start, __u64 size,
 				     void *data), void *data);
@@ -668,7 +528,7 @@ void __init pci_mmcfg_early_init(void)
 		if (pci_mmcfg_check_hostbridge())
 			known_bridge = 1;
 		else
-			acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
+			acpi_sfi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
 		__pci_mmcfg_init(1);
 
 		set_apei_filter();
@@ -686,7 +546,7 @@ void __init pci_mmcfg_late_init(void)
 
 	/* MMCONFIG hasn't been enabled yet, try again */
 	if (pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF) {
-		acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
+		acpi_sfi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
 		__pci_mmcfg_init(0);
 	}
 }
@@ -720,38 +580,6 @@ static int __init pci_mmcfg_late_insert_resources(void)
  */
 late_initcall(pci_mmcfg_late_insert_resources);
 
-static int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
-{
-	struct pci_mmcfg_region *cfg_conflict;
-	int err = 0;
-
-	mutex_lock(&pci_mmcfg_lock);
-	cfg_conflict = pci_mmconfig_lookup(cfg->segment, cfg->start_bus);
-	if (cfg_conflict) {
-		if (cfg_conflict->end_bus < cfg->end_bus)
-			pr_info(FW_INFO "MMCONFIG for "
-				"domain %04x [bus %02x-%02x] "
-				"only partially covers this bridge\n",
-				cfg_conflict->segment, cfg_conflict->start_bus,
-				cfg_conflict->end_bus);
-		err = -EEXIST;
-		goto out;
-	}
-
-	if (pci_mmcfg_arch_map(cfg)) {
-		pr_warn("fail to map MMCONFIG %pR.\n", &cfg->res);
-		err = -ENOMEM;
-		goto out;
-	} else {
-		list_add_sorted(cfg);
-		pr_info("MMCONFIG at %pR (base %#lx)\n",
-			&cfg->res, (unsigned long)cfg->address);
-	}
-out:
-	mutex_unlock(&pci_mmcfg_lock);
-	return err;
-}
-
 /* Add MMCFG information for host bridges */
 int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 			phys_addr_t addr)
@@ -808,26 +636,3 @@ error:
 	kfree(cfg);
 	return rc;
 }
-
-/* Delete MMCFG information for host bridges */
-int pci_mmconfig_delete(u16 seg, u8 start, u8 end)
-{
-	struct pci_mmcfg_region *cfg;
-
-	mutex_lock(&pci_mmcfg_lock);
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
-		if (cfg->segment == seg && cfg->start_bus == start &&
-		    cfg->end_bus == end) {
-			list_del_rcu(&cfg->list);
-			synchronize_rcu();
-			pci_mmcfg_arch_unmap(cfg);
-			if (cfg->res.parent)
-				release_resource(&cfg->res);
-			mutex_unlock(&pci_mmcfg_lock);
-			kfree(cfg);
-			return 0;
-		}
-	mutex_unlock(&pci_mmcfg_lock);
-
-	return -ENOENT;
-}
diff --git a/arch/x86/pci/mmconfig_32.c b/arch/x86/pci/mmconfig_32.c
index 43984bc..246f135 100644
--- a/arch/x86/pci/mmconfig_32.c
+++ b/arch/x86/pci/mmconfig_32.c
@@ -12,6 +12,7 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/rcupdate.h>
+#include <linux/ecam.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 
diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
index bea5249..b14fcd3 100644
--- a/arch/x86/pci/mmconfig_64.c
+++ b/arch/x86/pci/mmconfig_64.c
@@ -10,6 +10,7 @@
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/rcupdate.h>
+#include <linux/ecam.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 
diff --git a/arch/x86/pci/numachip.c b/arch/x86/pci/numachip.c
index 2e565e6..55fbd18 100644
--- a/arch/x86/pci/numachip.c
+++ b/arch/x86/pci/numachip.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/ecam.h>
 #include <linux/pci.h>
 #include <asm/pci_x86.h>
 
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 962f98e..607a879 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_ACPI_BUTTON)	+= button.o
 obj-$(CONFIG_ACPI_FAN)		+= fan.o
 obj-$(CONFIG_ACPI_VIDEO)	+= video.o
 obj-$(CONFIG_ACPI_PCI_SLOT)	+= pci_slot.o
+obj-$(CONFIG_PCI_MMCONFIG)	+= mcfg.o
 obj-$(CONFIG_ACPI_PROCESSOR)	+= processor.o
 obj-y				+= container.o
 obj-$(CONFIG_ACPI_THERMAL)	+= thermal.o
diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
new file mode 100644
index 0000000..5ecef20
--- /dev/null
+++ b/drivers/acpi/mcfg.c
@@ -0,0 +1,59 @@
+/*
+ * MCFG ACPI table parser.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/ecam.h>
+
+#include <asm/pci_x86.h> /* Temp hack before refactoring arch-specific calls */
+
+#define	PREFIX	"MCFG: "
+
+int __init acpi_parse_mcfg(struct acpi_table_header *header)
+{
+	struct acpi_table_mcfg *mcfg;
+	struct acpi_mcfg_allocation *cfg_table, *cfg;
+	unsigned long i;
+	int entries;
+
+	if (!header)
+		return -EINVAL;
+
+	mcfg = (struct acpi_table_mcfg *)header;
+
+	/* how many config structures do we have */
+	free_all_mmcfg();
+	entries = 0;
+	i = header->length - sizeof(struct acpi_table_mcfg);
+	while (i >= sizeof(struct acpi_mcfg_allocation)) {
+		entries++;
+		i -= sizeof(struct acpi_mcfg_allocation);
+	}
+	if (entries == 0) {
+		pr_err(PREFIX "MCFG table has no entries\n");
+		return -ENODEV;
+	}
+
+	cfg_table = (struct acpi_mcfg_allocation *) &mcfg[1];
+	for (i = 0; i < entries; i++) {
+		cfg = &cfg_table[i];
+		if (acpi_mcfg_check_entry(mcfg, cfg)) {
+			free_all_mmcfg();
+			return -ENODEV;
+		}
+
+		if (pci_mmconfig_add(cfg->pci_segment, cfg->start_bus_number,
+				 cfg->end_bus_number, cfg->address) == NULL) {
+			pr_warn(PREFIX "no memory for MCFG entries\n");
+			free_all_mmcfg();
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 73de4ef..9950248 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -26,6 +26,13 @@ config PCI_MSI_IRQ_DOMAIN
 	depends on PCI_MSI
 	select GENERIC_MSI_IRQ_DOMAIN
 
+config PCI_ECAM
+	bool "Enhanced Configuration Access Mechanism (ECAM)"
+	depends on PCI && HAVE_PCI_ECAM
+
+config HAVE_PCI_ECAM
+	bool
+
 config PCI_DEBUG
 	bool "PCI Debugging"
 	depends on PCI && DEBUG_KERNEL
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index be3f631..eb574f8 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -42,6 +42,11 @@ obj-$(CONFIG_SPARC_LEON) += setup-irq.o
 obj-$(CONFIG_M68K) += setup-irq.o
 
 #
+# Enhanced Configuration Access Mechanism (ECAM)
+#
+obj-$(CONFIG_PCI_ECAM)	+= ecam.o
+
+#
 # ACPI Related PCI FW Functions
 # ACPI _DSM provided firmware instance and string name
 #
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
new file mode 100644
index 0000000..d221dba
--- /dev/null
+++ b/drivers/pci/ecam.c
@@ -0,0 +1,175 @@
+/*
+ * Arch agnostic direct PCI config space access via
+ * ECAM (Enhanced Configuration Access Mechanism)
+ *
+ * Per-architecture code takes care of the mappings, region validation and
+ * accesses themselves.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/mutex.h>
+#include <linux/rculist.h>
+#include <linux/ecam.h>
+
+#include <asm/io.h>
+#include <asm/pci_x86.h> /* Temp hack before refactoring arch-specific calls */
+
+#define PREFIX "PCI: "
+
+static DEFINE_MUTEX(pci_mmcfg_lock);
+
+LIST_HEAD(pci_mmcfg_list);
+
+static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg)
+{
+	if (cfg->res.parent)
+		release_resource(&cfg->res);
+	list_del(&cfg->list);
+	kfree(cfg);
+}
+
+void __init free_all_mmcfg(void)
+{
+	struct pci_mmcfg_region *cfg, *tmp;
+
+	pci_mmcfg_arch_free();
+	list_for_each_entry_safe(cfg, tmp, &pci_mmcfg_list, list)
+		pci_mmconfig_remove(cfg);
+}
+
+void list_add_sorted(struct pci_mmcfg_region *new)
+{
+	struct pci_mmcfg_region *cfg;
+
+	/* keep list sorted by segment and starting bus number */
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+		if (cfg->segment > new->segment ||
+		    (cfg->segment == new->segment &&
+		     cfg->start_bus >= new->start_bus)) {
+			list_add_tail_rcu(&new->list, &cfg->list);
+			return;
+		}
+	}
+	list_add_tail_rcu(&new->list, &pci_mmcfg_list);
+}
+
+struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
+					    int end, u64 addr)
+{
+	struct pci_mmcfg_region *new;
+	struct resource *res;
+
+	if (addr == 0)
+		return NULL;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	new->address = addr;
+	new->segment = segment;
+	new->start_bus = start;
+	new->end_bus = end;
+
+	res = &new->res;
+	res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
+	res->end = addr + PCI_MMCFG_BUS_OFFSET(end + 1) - 1;
+	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+	snprintf(new->name, PCI_MMCFG_RESOURCE_NAME_LEN,
+		 "PCI MMCONFIG %04x [bus %02x-%02x]", segment, start, end);
+	res->name = new->name;
+
+	return new;
+}
+
+struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
+					  int end, u64 addr)
+{
+	struct pci_mmcfg_region *new;
+
+	new = pci_mmconfig_alloc(segment, start, end, addr);
+	if (new) {
+		mutex_lock(&pci_mmcfg_lock);
+		list_add_sorted(new);
+		mutex_unlock(&pci_mmcfg_lock);
+
+		pr_info(PREFIX
+		       "MMCONFIG for domain %04x [bus %02x-%02x] at %pR "
+		       "(base %#lx)\n",
+		       segment, start, end, &new->res, (unsigned long)addr);
+	}
+
+	return new;
+}
+
+struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
+{
+	struct pci_mmcfg_region *cfg;
+
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+		if (cfg->segment == segment &&
+		    cfg->start_bus <= bus && bus <= cfg->end_bus)
+			return cfg;
+
+	return NULL;
+}
+
+/* Delete MMCFG information for host bridges */
+int pci_mmconfig_delete(u16 seg, u8 start, u8 end)
+{
+	struct pci_mmcfg_region *cfg;
+
+	mutex_lock(&pci_mmcfg_lock);
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+		if (cfg->segment == seg && cfg->start_bus == start &&
+		    cfg->end_bus == end) {
+			list_del_rcu(&cfg->list);
+			synchronize_rcu();
+			pci_mmcfg_arch_unmap(cfg);
+			if (cfg->res.parent)
+				release_resource(&cfg->res);
+			mutex_unlock(&pci_mmcfg_lock);
+			kfree(cfg);
+			return 0;
+		}
+	mutex_unlock(&pci_mmcfg_lock);
+
+	return -ENOENT;
+}
+
+int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
+{
+	struct pci_mmcfg_region *cfg_conflict;
+	int err = 0;
+
+	mutex_lock(&pci_mmcfg_lock);
+	cfg_conflict = pci_mmconfig_lookup(cfg->segment, cfg->start_bus);
+	if (cfg_conflict) {
+		if (cfg_conflict->end_bus < cfg->end_bus)
+			pr_info(FW_INFO "MMCONFIG for "
+				"domain %04x [bus %02x-%02x] "
+				"only partially covers this bridge\n",
+				cfg_conflict->segment, cfg_conflict->start_bus,
+				cfg_conflict->end_bus);
+		err = -EEXIST;
+		goto out;
+	}
+
+	if (pci_mmcfg_arch_map(cfg)) {
+		pr_warn("fail to map MMCONFIG %pR.\n", &cfg->res);
+		err = -ENOMEM;
+		goto out;
+	} else {
+		list_add_sorted(cfg);
+		pr_info("MMCONFIG at %pR (base %#lx)\n",
+			&cfg->res, (unsigned long)cfg->address);
+
+	}
+out:
+	mutex_unlock(&pci_mmcfg_lock);
+	return err;
+}
diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 7494dbe..6785ebb 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -20,6 +20,7 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/pci-acpi.h>
+#include <linux/ecam.h>
 #include <xen/xen.h>
 #include <xen/interface/physdev.h>
 #include <xen/interface/xen.h>
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 127d68b..98f2ce4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -162,6 +162,8 @@ int acpi_table_parse_madt(enum acpi_madt_type id,
 			  acpi_tbl_entry_handler handler,
 			  unsigned int max_entries);
 int acpi_parse_mcfg (struct acpi_table_header *header);
+int acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+			  struct acpi_mcfg_allocation *cfg);
 void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
 
 /* the following four functions are architecture-dependent */
diff --git a/include/linux/ecam.h b/include/linux/ecam.h
new file mode 100644
index 0000000..dec3b52
--- /dev/null
+++ b/include/linux/ecam.h
@@ -0,0 +1,37 @@
+#ifndef __ECAM_H
+#define __ECAM_H
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <linux/acpi.h>
+
+/* "PCI MMCONFIG %04x [bus %02x-%02x]" */
+#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)
+
+struct pci_mmcfg_region {
+	struct list_head list;
+	struct resource res;
+	u64 address;
+	char __iomem *virt;
+	u16 segment;
+	u8 start_bus;
+	u8 end_bus;
+	char name[PCI_MMCFG_RESOURCE_NAME_LEN];
+};
+
+struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
+struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
+						   int end, u64 addr);
+int pci_mmconfig_inject(struct pci_mmcfg_region *cfg);
+struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
+						 int end, u64 addr);
+void list_add_sorted(struct pci_mmcfg_region *new);
+void free_all_mmcfg(void);
+int pci_mmconfig_delete(u16 seg, u8 start, u8 end);
+
+extern struct list_head pci_mmcfg_list;
+
+#define PCI_MMCFG_BUS_OFFSET(bus)      ((bus) << 20)
+
+#endif  /* __KERNEL__ */
+#endif  /* __ECAM_H */
-- 
1.9.1

[PATCH V1 03/11] pci, acpi, mcfg: Provide generic implementation of MCFG code initialization.

[Tomasz Nowicki]

First function acpi_mcfg_check_entry() does not apply any quirks by default.

Last two functions are required by ACPI subsystem to make PCI config
space accessible. Generic code assume to do nothing for early init call but
late init call does as follow:
- parse MCFG table and add regions to ECAM resource list
- map regions
- add regions to iomem_resource

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/mcfg.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index 5ecef20..fad9917 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -57,3 +57,29 @@ int __init acpi_parse_mcfg(struct acpi_table_header *header)
 
 	return 0;
 }
+
+int __init __weak acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+					struct acpi_mcfg_allocation *cfg)
+{
+	return 0;
+}
+
+void __init __weak pci_mmcfg_early_init(void)
+{
+
+}
+
+void __init __weak pci_mmcfg_late_init(void)
+{
+	struct pci_mmcfg_region *cfg;
+
+	acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
+
+	if (list_empty(&pci_mmcfg_list))
+		return;
+	if (!pci_mmcfg_arch_init())
+		free_all_mmcfg();
+
+	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+		insert_resource(&iomem_resource, &cfg->res);
+}
-- 
1.9.1

[PATCH V1 04/11] x86, pci: mmconfig_{32,64}.c code refactoring - remove code duplication.

[Tomasz Nowicki]

mmconfig_64.c version is going to be default implementation for low-level
operation on mmconfig regions. However, now it initializes raw_pci_ext_ops pointer which is specific for
x86 only. Moreover, mmconfig_32.c is doing the same thing at the same time.
So lets move it to mmconfig_shared.c so it becomes common for both and
mmconfig_64.c turns out to be purely arch agnostic.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/x86/include/asm/pci_x86.h |  5 +++++
 arch/x86/pci/mmconfig-shared.c | 10 ++++++++--
 arch/x86/pci/mmconfig_32.c     | 10 ++--------
 arch/x86/pci/mmconfig_64.c     | 11 ++---------
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index 64cb514..039f69e 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -129,6 +129,11 @@ extern void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
 extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 			       phys_addr_t addr);
 
+int pci_mmcfg_read(unsigned int seg, unsigned int bus, unsigned int devfn,
+		   int reg, int len, u32 *value);
+int pci_mmcfg_write(unsigned int seg, unsigned int bus, unsigned int devfn,
+		    int reg, int len, u32 value);
+
 /*
  * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
  * on their northbrige except through the * %eax register. As such, you MUST
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index ce2c2e4..980f304 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,11 @@
 static bool pci_mmcfg_running_state;
 static bool pci_mmcfg_arch_init_failed;
 
+const struct pci_raw_ops pci_mmcfg = {
+	.read =		pci_mmcfg_read,
+	.write =	pci_mmcfg_write,
+};
+
 static const char *__init pci_mmcfg_e7520(void)
 {
 	u32 win;
@@ -512,9 +517,10 @@ static void __init __pci_mmcfg_init(int early)
 		}
 	}
 
-	if (pci_mmcfg_arch_init())
+	if (pci_mmcfg_arch_init()) {
+		raw_pci_ext_ops = &pci_mmcfg;
 		pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
-	else {
+	} else {
 		free_all_mmcfg();
 		pci_mmcfg_arch_init_failed = true;
 	}
diff --git a/arch/x86/pci/mmconfig_32.c b/arch/x86/pci/mmconfig_32.c
index 246f135..2ded56f 100644
--- a/arch/x86/pci/mmconfig_32.c
+++ b/arch/x86/pci/mmconfig_32.c
@@ -50,7 +50,7 @@ static void pci_exp_set_dev_base(unsigned int base, int bus, int devfn)
 	}
 }
 
-static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
+int pci_mmcfg_read(unsigned int seg, unsigned int bus,
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
@@ -89,7 +89,7 @@ err:		*value = -1;
 	return 0;
 }
 
-static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
+int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 			   unsigned int devfn, int reg, int len, u32 value)
 {
 	unsigned long flags;
@@ -126,15 +126,9 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 	return 0;
 }
 
-const struct pci_raw_ops pci_mmcfg = {
-	.read =		pci_mmcfg_read,
-	.write =	pci_mmcfg_write,
-};
-
 int __init pci_mmcfg_arch_init(void)
 {
 	printk(KERN_INFO "PCI: Using MMCONFIG for extended config space\n");
-	raw_pci_ext_ops = &pci_mmcfg;
 	return 1;
 }
 
diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
index b14fcd3..d0c48eb 100644
--- a/arch/x86/pci/mmconfig_64.c
+++ b/arch/x86/pci/mmconfig_64.c
@@ -25,7 +25,7 @@ static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned i
 	return NULL;
 }
 
-static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
+int pci_mmcfg_read(unsigned int seg, unsigned int bus,
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
 	char __iomem *addr;
@@ -59,7 +59,7 @@ err:		*value = -1;
 	return 0;
 }
 
-static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
+int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 			   unsigned int devfn, int reg, int len, u32 value)
 {
 	char __iomem *addr;
@@ -91,11 +91,6 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 	return 0;
 }
 
-const struct pci_raw_ops pci_mmcfg = {
-	.read =		pci_mmcfg_read,
-	.write =	pci_mmcfg_write,
-};
-
 static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
 {
 	void __iomem *addr;
@@ -121,8 +116,6 @@ int __init pci_mmcfg_arch_init(void)
 			return 0;
 		}
 
-	raw_pci_ext_ops = &pci_mmcfg;
-
 	return 1;
 }
 
-- 
1.9.1

[PATCH V1 05/11] x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM driver.

[Tomasz Nowicki]

Hosts with custom ECAM hooks (like 32bit x86) should select
ARCH_HAS_CUSTOM_PCI_ECAM. Otherwise, host will use generic version
provided by this patch (like 64bit x86 does).

Note, we leaved x86-specific PCI config accessors in corresponding files.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/x86/Kconfig               |  1 +
 arch/x86/include/asm/pci_x86.h |  4 ---
 arch/x86/pci/mmconfig_64.c     | 55 ---------------------------------------
 drivers/acpi/mcfg.c            |  2 --
 drivers/pci/Kconfig            |  3 +++
 drivers/pci/ecam.c             | 59 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/ecam.h           |  6 +++++
 7 files changed, 68 insertions(+), 62 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4ca8134..b4b68eb 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -30,6 +30,7 @@ config X86
 	select ARCH_HAS_PMEM_API		if X86_64
 	select ARCH_HAS_MMIO_FLUSH
 	select ARCH_HAS_SG_CHAIN
+	select ARCH_HAS_CUSTOM_PCI_ECAM		if X86_32
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
 	select ARCH_MIGHT_HAVE_PC_PARPORT
diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index 039f69e..c9bda33 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -122,10 +122,6 @@ extern int pci_legacy_init(void);
 extern void pcibios_fixup_irqs(void);
 
 /* pci-mmconfig.c */
-extern int __init pci_mmcfg_arch_init(void);
-extern void __init pci_mmcfg_arch_free(void);
-extern int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
-extern void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
 extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 			       phys_addr_t addr);
 
diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
index d0c48eb..fd356cc 100644
--- a/arch/x86/pci/mmconfig_64.c
+++ b/arch/x86/pci/mmconfig_64.c
@@ -90,58 +90,3 @@ int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 
 	return 0;
 }
-
-static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
-{
-	void __iomem *addr;
-	u64 start, size;
-	int num_buses;
-
-	start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
-	num_buses = cfg->end_bus - cfg->start_bus + 1;
-	size = PCI_MMCFG_BUS_OFFSET(num_buses);
-	addr = ioremap_nocache(start, size);
-	if (addr)
-		addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
-	return addr;
-}
-
-int __init pci_mmcfg_arch_init(void)
-{
-	struct pci_mmcfg_region *cfg;
-
-	list_for_each_entry(cfg, &pci_mmcfg_list, list)
-		if (pci_mmcfg_arch_map(cfg)) {
-			pci_mmcfg_arch_free();
-			return 0;
-		}
-
-	return 1;
-}
-
-void __init pci_mmcfg_arch_free(void)
-{
-	struct pci_mmcfg_region *cfg;
-
-	list_for_each_entry(cfg, &pci_mmcfg_list, list)
-		pci_mmcfg_arch_unmap(cfg);
-}
-
-int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
-{
-	cfg->virt = mcfg_ioremap(cfg);
-	if (!cfg->virt) {
-		pr_err(PREFIX "can't map MMCONFIG at %pR\n", &cfg->res);
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
-void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
-{
-	if (cfg && cfg->virt) {
-		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
-		cfg->virt = NULL;
-	}
-}
diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index fad9917..745b83e 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -10,8 +10,6 @@
 #include <linux/acpi.h>
 #include <linux/ecam.h>
 
-#include <asm/pci_x86.h> /* Temp hack before refactoring arch-specific calls */
-
 #define	PREFIX	"MCFG: "
 
 int __init acpi_parse_mcfg(struct acpi_table_header *header)
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 9950248..b2e27c8 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -33,6 +33,9 @@ config PCI_ECAM
 config HAVE_PCI_ECAM
 	bool
 
+config ARCH_HAS_CUSTOM_PCI_ECAM
+	bool
+
 config PCI_DEBUG
 	bool "PCI Debugging"
 	depends on PCI && DEBUG_KERNEL
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index d221dba..8a5eef7 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -16,7 +16,6 @@
 #include <linux/ecam.h>
 
 #include <asm/io.h>
-#include <asm/pci_x86.h> /* Temp hack before refactoring arch-specific calls */
 
 #define PREFIX "PCI: "
 
@@ -24,6 +23,64 @@ static DEFINE_MUTEX(pci_mmcfg_lock);
 
 LIST_HEAD(pci_mmcfg_list);
 
+#ifndef CONFIG_ARCH_HAS_CUSTOM_PCI_ECAM
+
+static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
+{
+	void __iomem *addr;
+	u64 start, size;
+	int num_buses;
+
+	start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
+	num_buses = cfg->end_bus - cfg->start_bus + 1;
+	size = PCI_MMCFG_BUS_OFFSET(num_buses);
+	addr = ioremap_nocache(start, size);
+	if (addr)
+		addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
+	return addr;
+}
+
+int __init pci_mmcfg_arch_init(void)
+{
+	struct pci_mmcfg_region *cfg;
+
+	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+		if (pci_mmcfg_arch_map(cfg)) {
+			pci_mmcfg_arch_free();
+			return 0;
+		}
+
+	return 1;
+}
+
+void __init pci_mmcfg_arch_free(void)
+{
+	struct pci_mmcfg_region *cfg;
+
+	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+		pci_mmcfg_arch_unmap(cfg);
+}
+
+int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
+{
+	cfg->virt = mcfg_ioremap(cfg);
+	if (!cfg->virt) {
+		pr_err(PREFIX "can't map MMCONFIG at %pR\n", &cfg->res);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+{
+	if (cfg && cfg->virt) {
+		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
+		cfg->virt = NULL;
+	}
+}
+#endif
+
 static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg)
 {
 	if (cfg->res.parent)
diff --git a/include/linux/ecam.h b/include/linux/ecam.h
index dec3b52..813acd1 100644
--- a/include/linux/ecam.h
+++ b/include/linux/ecam.h
@@ -29,6 +29,12 @@ void list_add_sorted(struct pci_mmcfg_region *new);
 void free_all_mmcfg(void);
 int pci_mmconfig_delete(u16 seg, u8 start, u8 end);
 
+/* Arch specific calls */
+int pci_mmcfg_arch_init(void);
+void pci_mmcfg_arch_free(void);
+int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
+void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
+
 extern struct list_head pci_mmcfg_list;
 
 #define PCI_MMCFG_BUS_OFFSET(bus)      ((bus) << 20)
-- 
1.9.1

[PATCH V1 06/11] pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.

[Tomasz Nowicki]

Lets keep RAW ACPI PCI config space accessors empty by default,
since we are note sure if they are necessary accross all archs.
Once we sort this out, we can provide generic version or let
architectures to overwrite, like now x86.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/mcfg.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
index 745b83e..3e1e7be 100644
--- a/drivers/acpi/mcfg.c
+++ b/drivers/acpi/mcfg.c
@@ -9,9 +9,30 @@
 
 #include <linux/acpi.h>
 #include <linux/ecam.h>
+#include <linux/pci.h>
 
 #define	PREFIX	"MCFG: "
 
+/*
+ * raw_pci_read/write - raw ACPI PCI config space accessors.
+ *
+ * By defauly (__weak) these accessors are empty and should be overwritten
+ * by architectures which support operations on ACPI PCI_Config regions,
+ * see osl.c file.
+ */
+
+int __weak raw_pci_read(unsigned int domain, unsigned int bus,
+			unsigned int devfn, int reg, int len, u32 *val)
+{
+	return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+int __weak raw_pci_write(unsigned int domain, unsigned int bus,
+			 unsigned int devfn, int reg, int len, u32 val)
+{
+	return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
 int __init acpi_parse_mcfg(struct acpi_table_header *header)
 {
 	struct acpi_table_mcfg *mcfg;
-- 
1.9.1

[PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Tomasz Nowicki]

From: Hanjun Guo <hanjun.guo@linaro.org>

In drivers/xen/pci.c, there are arch x86 dependent codes when
CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
depends on ACPI, so this will prevent XEN PCI running on other
architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).

Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
and it's defined in asm/pci_x86.h, the code means that
if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
ingnore the xen mcfg init. Actually this is duplicate, because
if PCI resource is not probed in PCI_PROBE_MMCONF way, the
pci_mmconfig_list will be empty, and the if (list_empty())
after it will do the same job.

So just remove the arch related code and the head file, this
will be no functional change for x86, and also makes xen/pci.c
usable for other architectures.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/xen/pci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index 6785ebb..9a8dbe3 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -28,9 +28,6 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 #include "../pci/pci.h"
-#ifdef CONFIG_PCI_MMCONFIG
-#include <asm/pci_x86.h>
-#endif
 
 static bool __read_mostly pci_seg_supported = true;
 
@@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
 	if (!xen_initial_domain())
 		return 0;
 
-	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
-		return 0;
-
 	if (list_empty(&pci_mmcfg_list))
 		return 0;
 
-- 
1.9.1

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Tomasz Nowicki]

+ Stefano

On 27.10.2015 17:38, Tomasz Nowicki wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
>
> In drivers/xen/pci.c, there are arch x86 dependent codes when
> CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
> depends on ACPI, so this will prevent XEN PCI running on other
> architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
>
> Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
> the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
> and it's defined in asm/pci_x86.h, the code means that
> if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
> ingnore the xen mcfg init. Actually this is duplicate, because
> if PCI resource is not probed in PCI_PROBE_MMCONF way, the
> pci_mmconfig_list will be empty, and the if (list_empty())
> after it will do the same job.
>
> So just remove the arch related code and the head file, this
> will be no functional change for x86, and also makes xen/pci.c
> usable for other architectures.
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>   drivers/xen/pci.c | 6 ------
>   1 file changed, 6 deletions(-)
>
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 6785ebb..9a8dbe3 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -28,9 +28,6 @@
>   #include <asm/xen/hypervisor.h>
>   #include <asm/xen/hypercall.h>
>   #include "../pci/pci.h"
> -#ifdef CONFIG_PCI_MMCONFIG
> -#include <asm/pci_x86.h>
> -#endif
>
>   static bool __read_mostly pci_seg_supported = true;
>
> @@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
>   	if (!xen_initial_domain())
>   		return 0;
>
> -	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
> -		return 0;
> -
>   	if (list_empty(&pci_mmcfg_list))
>   		return 0;
>
>

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Boris Ostrovsky]

On 10/27/2015 12:47 PM, Tomasz Nowicki wrote:
> + Stefano
>
> On 27.10.2015 17:38, Tomasz Nowicki wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> In drivers/xen/pci.c, there are arch x86 dependent codes when
>> CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
>> depends on ACPI, so this will prevent XEN PCI running on other
>> architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
>>
>> Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
>> the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
>> and it's defined in asm/pci_x86.h, the code means that
>> if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
>> ingnore the xen mcfg init. Actually this is duplicate, because
>> if PCI resource is not probed in PCI_PROBE_MMCONF way, the
>> pci_mmconfig_list will be empty, and the if (list_empty())
>> after it will do the same job.
>>
>> So just remove the arch related code and the head file, this
>> will be no functional change for x86, and also makes xen/pci.c
>> usable for other architectures.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   drivers/xen/pci.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>> index 6785ebb..9a8dbe3 100644
>> --- a/drivers/xen/pci.c
>> +++ b/drivers/xen/pci.c
>> @@ -28,9 +28,6 @@
>>   #include <asm/xen/hypervisor.h>
>>   #include <asm/xen/hypercall.h>
>>   #include "../pci/pci.h"
>> -#ifdef CONFIG_PCI_MMCONFIG
>> -#include <asm/pci_x86.h>
>> -#endif

Assuming this still compiles on x86 now that this include file is removed

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

>>
>>   static bool __read_mostly pci_seg_supported = true;
>>
>> @@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
>>       if (!xen_initial_domain())
>>           return 0;
>>
>> -    if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>> -        return 0;
>> -
>>       if (list_empty(&pci_mmcfg_list))
>>           return 0;
>>
>>

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

On Tue, 27 Oct 2015, Boris Ostrovsky wrote:
> On 10/27/2015 12:47 PM, Tomasz Nowicki wrote:
> > + Stefano
> > 
> > On 27.10.2015 17:38, Tomasz Nowicki wrote:
> > > From: Hanjun Guo <hanjun.guo@linaro.org>
> > > 
> > > In drivers/xen/pci.c, there are arch x86 dependent codes when
> > > CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
> > > depends on ACPI, so this will prevent XEN PCI running on other
> > > architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
> > > 
> > > Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
> > > the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
> > > and it's defined in asm/pci_x86.h, the code means that
> > > if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
> > > ingnore the xen mcfg init. Actually this is duplicate, because
> > > if PCI resource is not probed in PCI_PROBE_MMCONF way, the
> > > pci_mmconfig_list will be empty, and the if (list_empty())
> > > after it will do the same job.
> > > 
> > > So just remove the arch related code and the head file, this
> > > will be no functional change for x86, and also makes xen/pci.c
> > > usable for other architectures.
> > > 
> > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> > > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > ---
> > >   drivers/xen/pci.c | 6 ------
> > >   1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> > > index 6785ebb..9a8dbe3 100644
> > > --- a/drivers/xen/pci.c
> > > +++ b/drivers/xen/pci.c
> > > @@ -28,9 +28,6 @@
> > >   #include <asm/xen/hypervisor.h>
> > >   #include <asm/xen/hypercall.h>
> > >   #include "../pci/pci.h"
> > > -#ifdef CONFIG_PCI_MMCONFIG
> > > -#include <asm/pci_x86.h>
> > > -#endif
> 
> Assuming this still compiles on x86 now that this include file is removed
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

I think it does not:

drivers/xen/pci.c: In function ‘xen_mcfg_late’:
drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first use in this function)
drivers/xen/pci.c:221:18: note: each undeclared identifier is reported only once for each f


> > > 
> > >   static bool __read_mostly pci_seg_supported = true;
> > > 
> > > @@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
> > >       if (!xen_initial_domain())
> > >           return 0;
> > > 
> > > -    if ((pci_probe & PCI_PROBE_MMCONF) == 0)
> > > -        return 0;
> > > -
> > >       if (list_empty(&pci_mmcfg_list))
> > >           return 0;
> > > 
> > > 
> 

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Tomasz Nowicki]

On 28.10.2015 11:49, Stefano Stabellini wrote:
> On Tue, 27 Oct 2015, Boris Ostrovsky wrote:
>> On 10/27/2015 12:47 PM, Tomasz Nowicki wrote:
>>> + Stefano
>>>
>>> On 27.10.2015 17:38, Tomasz Nowicki wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> In drivers/xen/pci.c, there are arch x86 dependent codes when
>>>> CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
>>>> depends on ACPI, so this will prevent XEN PCI running on other
>>>> architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
>>>>
>>>> Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
>>>> the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF) == 0),
>>>> and it's defined in asm/pci_x86.h, the code means that
>>>> if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
>>>> ingnore the xen mcfg init. Actually this is duplicate, because
>>>> if PCI resource is not probed in PCI_PROBE_MMCONF way, the
>>>> pci_mmconfig_list will be empty, and the if (list_empty())
>>>> after it will do the same job.
>>>>
>>>> So just remove the arch related code and the head file, this
>>>> will be no functional change for x86, and also makes xen/pci.c
>>>> usable for other architectures.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>>    drivers/xen/pci.c | 6 ------
>>>>    1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>>>> index 6785ebb..9a8dbe3 100644
>>>> --- a/drivers/xen/pci.c
>>>> +++ b/drivers/xen/pci.c
>>>> @@ -28,9 +28,6 @@
>>>>    #include <asm/xen/hypervisor.h>
>>>>    #include <asm/xen/hypercall.h>
>>>>    #include "../pci/pci.h"
>>>> -#ifdef CONFIG_PCI_MMCONFIG
>>>> -#include <asm/pci_x86.h>
>>>> -#endif
>>
>> Assuming this still compiles on x86 now that this include file is removed
>>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> I think it does not:
>
> drivers/xen/pci.c: In function ‘xen_mcfg_late’:
> drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first use in this function)
> drivers/xen/pci.c:221:18: note: each undeclared identifier is reported only once for each f

Right, we need:
+#include <linux/ecam.h>

Will fix this in next version, thanks!

Tomasz

>
>
>>>>
>>>>    static bool __read_mostly pci_seg_supported = true;
>>>>
>>>> @@ -222,9 +219,6 @@ static int __init xen_mcfg_late(void)
>>>>        if (!xen_initial_domain())
>>>>            return 0;
>>>>
>>>> -    if ((pci_probe & PCI_PROBE_MMCONF) == 0)
>>>> -        return 0;
>>>> -
>>>>        if (list_empty(&pci_mmcfg_list))
>>>>            return 0;
>>>>
>>>>

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Hanjun Guo]

On 10/28/2015 06:56 PM, Tomasz Nowicki wrote:
> On 28.10.2015 11:49, Stefano Stabellini wrote:
>> On Tue, 27 Oct 2015, Boris Ostrovsky wrote:
>>> On 10/27/2015 12:47 PM, Tomasz Nowicki wrote:
>>>> + Stefano
>>>>
>>>> On 27.10.2015 17:38, Tomasz Nowicki wrote:
>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>
>>>>> In drivers/xen/pci.c, there are arch x86 dependent codes when
>>>>> CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
>>>>> depends on ACPI, so this will prevent XEN PCI running on other
>>>>> architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
>>>>>
>>>>> Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
>>>>> the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF)
>>>>> == 0),
>>>>> and it's defined in asm/pci_x86.h, the code means that
>>>>> if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
>>>>> ingnore the xen mcfg init. Actually this is duplicate, because
>>>>> if PCI resource is not probed in PCI_PROBE_MMCONF way, the
>>>>> pci_mmconfig_list will be empty, and the if (list_empty())
>>>>> after it will do the same job.
>>>>>
>>>>> So just remove the arch related code and the head file, this
>>>>> will be no functional change for x86, and also makes xen/pci.c
>>>>> usable for other architectures.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> ---
>>>>>    drivers/xen/pci.c | 6 ------
>>>>>    1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>>>>> index 6785ebb..9a8dbe3 100644
>>>>> --- a/drivers/xen/pci.c
>>>>> +++ b/drivers/xen/pci.c
>>>>> @@ -28,9 +28,6 @@
>>>>>    #include <asm/xen/hypervisor.h>
>>>>>    #include <asm/xen/hypercall.h>
>>>>>    #include "../pci/pci.h"
>>>>> -#ifdef CONFIG_PCI_MMCONFIG
>>>>> -#include <asm/pci_x86.h>
>>>>> -#endif
>>>
>>> Assuming this still compiles on x86 now that this include file is
>>> removed
>>>
>>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> I think it does not:
>>
>> drivers/xen/pci.c: In function ‘xen_mcfg_late’:
>> drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first
>> use in this function)
>> drivers/xen/pci.c:221:18: note: each undeclared identifier is reported
>> only once for each f
>
> Right, we need:
> +#include <linux/ecam.h>
>
> Will fix this in next version, thanks!

Hmm, I think we just missed the head file, but the logic
of this patch is still right.

Thanks
Hanjun

Re: [Linaro-acpi] [PATCH V1 07/11] XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y

[Boris Ostrovsky]

On 10/28/2015 09:45 AM, Hanjun Guo wrote:
> On 10/28/2015 06:56 PM, Tomasz Nowicki wrote:
>> On 28.10.2015 11:49, Stefano Stabellini wrote:
>>> On Tue, 27 Oct 2015, Boris Ostrovsky wrote:
>>>> On 10/27/2015 12:47 PM, Tomasz Nowicki wrote:
>>>>> + Stefano
>>>>>
>>>>> On 27.10.2015 17:38, Tomasz Nowicki wrote:
>>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>>
>>>>>> In drivers/xen/pci.c, there are arch x86 dependent codes when
>>>>>> CONFIG_PCI_MMCONFIG is enabled, since CONFIG_PCI_MMCONFIG
>>>>>> depends on ACPI, so this will prevent XEN PCI running on other
>>>>>> architectures using ACPI with PCI_MMCONFIG enabled (such as ARM64).
>>>>>>
>>>>>> Fortunatly, it can be sloved in a simple way. In drivers/xen/pci.c,
>>>>>> the only x86 dependent code is if ((pci_probe & PCI_PROBE_MMCONF)
>>>>>> == 0),
>>>>>> and it's defined in asm/pci_x86.h, the code means that
>>>>>> if the PCI resource is not probed in PCI_PROBE_MMCONF way, just
>>>>>> ingnore the xen mcfg init. Actually this is duplicate, because
>>>>>> if PCI resource is not probed in PCI_PROBE_MMCONF way, the
>>>>>> pci_mmconfig_list will be empty, and the if (list_empty())
>>>>>> after it will do the same job.
>>>>>>
>>>>>> So just remove the arch related code and the head file, this
>>>>>> will be no functional change for x86, and also makes xen/pci.c
>>>>>> usable for other architectures.
>>>>>>
>>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> ---
>>>>>>    drivers/xen/pci.c | 6 ------
>>>>>>    1 file changed, 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
>>>>>> index 6785ebb..9a8dbe3 100644
>>>>>> --- a/drivers/xen/pci.c
>>>>>> +++ b/drivers/xen/pci.c
>>>>>> @@ -28,9 +28,6 @@
>>>>>>    #include <asm/xen/hypervisor.h>
>>>>>>    #include <asm/xen/hypercall.h>
>>>>>>    #include "../pci/pci.h"
>>>>>> -#ifdef CONFIG_PCI_MMCONFIG
>>>>>> -#include <asm/pci_x86.h>
>>>>>> -#endif
>>>>
>>>> Assuming this still compiles on x86 now that this include file is
>>>> removed
>>>>
>>>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>
>>> I think it does not:
>>>
>>> drivers/xen/pci.c: In function ‘xen_mcfg_late’:
>>> drivers/xen/pci.c:221:18: error: ‘pci_mmcfg_list’ undeclared (first
>>> use in this function)
>>> drivers/xen/pci.c:221:18: note: each undeclared identifier is reported
>>> only once for each f
>>
>> Right, we need:
>> +#include <linux/ecam.h>
>>
>> Will fix this in next version, thanks!
>
> Hmm, I think we just missed the head file, but the logic
> of this patch is still right.
>


Yes, removing the test should be safe.

-boris

[PATCH V1 08/11] pci, acpi, ecam: Add flag to indicate whether ECAM region was hot added or not.

[Tomasz Nowicki]

There are two ways we can get ECAM (aka MCFG) regions using ACPI,
from MCFG static table and from _CBA method. We cannot remove static
regions, however regions coming from _CBA should be removed while removing
bridge device.

In the light of above we need flag to mark hot added ECAM entries
so that user should use pci_mmconfig_inject while adding regions from
_CBA method.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/pci/ecam.c   | 2 ++
 include/linux/ecam.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 8a5eef7..da35b4c 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -131,6 +131,7 @@ struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
 	new->segment = segment;
 	new->start_bus = start;
 	new->end_bus = end;
+	new->hot_added = false;
 
 	res = &new->res;
 	res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
@@ -221,6 +222,7 @@ int pci_mmconfig_inject(struct pci_mmcfg_region *cfg)
 		err = -ENOMEM;
 		goto out;
 	} else {
+		cfg->hot_added = true;
 		list_add_sorted(cfg);
 		pr_info("MMCONFIG at %pR (base %#lx)\n",
 			&cfg->res, (unsigned long)cfg->address);
diff --git a/include/linux/ecam.h b/include/linux/ecam.h
index 813acd1..e0f322e 100644
--- a/include/linux/ecam.h
+++ b/include/linux/ecam.h
@@ -17,6 +17,7 @@ struct pci_mmcfg_region {
 	u8 start_bus;
 	u8 end_bus;
 	char name[PCI_MMCFG_RESOURCE_NAME_LEN];
+	bool hot_added;
 };
 
 struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
-- 
1.9.1

[PATCH V1 09/11] x86, pci: Use previously added ECAM hot_added flag to remove ECAM regions.

[Tomasz Nowicki]

Now that we have hot_added flag we can get rid of arch specific mcfg_added.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 arch/x86/pci/acpi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 28c9cd6..b4ef761 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -13,7 +13,6 @@ struct pci_root_info {
 	struct acpi_pci_root_info common;
 	struct pci_sysdata sd;
 #ifdef	CONFIG_PCI_MMCONFIG
-	bool mcfg_added;
 	u8 start_bus;
 	u8 end_bus;
 #endif
@@ -188,7 +187,6 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)
 	info = container_of(ci, struct pci_root_info, common);
 	info->start_bus = (u8)root->secondary.start;
 	info->end_bus = (u8)root->secondary.end;
-	info->mcfg_added = false;
 	seg = info->sd.domain;
 
 	/* return success if MMCFG is not in use */
@@ -204,7 +202,6 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)
 		/* enable MMCFG if it hasn't been enabled yet */
 		if (raw_pci_ext_ops == NULL)
 			raw_pci_ext_ops = &pci_mmcfg;
-		info->mcfg_added = true;
 	} else if (result != -EEXIST)
 		return check_segment(seg, dev,
 			 "fail to add MMCONFIG information,");
@@ -214,14 +211,17 @@ static int setup_mcfg_map(struct acpi_pci_root_info *ci)
 
 static void teardown_mcfg_map(struct acpi_pci_root_info *ci)
 {
+	struct pci_mmcfg_region *cfg;
 	struct pci_root_info *info;
 
 	info = container_of(ci, struct pci_root_info, common);
-	if (info->mcfg_added) {
-		pci_mmconfig_delete(info->sd.domain,
-				    info->start_bus, info->end_bus);
-		info->mcfg_added = false;
-	}
+	cfg = pci_mmconfig_lookup(info->sd.domain, info->start_bus);
+	if (!cfg)
+		return;
+
+	if (cfg->hot_added)
+		pci_mmconfig_delete(info->sd.domain, info->start_bus,
+				    info->end_bus);
 }
 #else
 static int setup_mcfg_map(struct acpi_pci_root_info *ci)
-- 
1.9.1

[PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Tomasz Nowicki]

Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
initialization since this function needs valid parent device reference
to be able to retrieve domain number (aka segment).

We can omit that blocker and pass down host bridge device via
pci_create_root_bus parameter and then be able to evaluate _SEG method
being in pci_bus_assign_domain_nr.

Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/pci_root.c |  2 +-
 drivers/pci/pci.c       | 32 +++++++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 850d7bf..e682dc6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	pci_acpi_root_add_resources(info);
 	pci_add_resource(&info->resources, &root->secondary);
-	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
 				  sysdata, &info->resources);
 	if (!bus)
 		goto out_release_info;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..17d1857 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -25,6 +25,7 @@
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci_hotplug.h>
+#include <linux/acpi.h>
 #include <asm-generic/pci-bridge.h>
 #include <asm/setup.h>
 #include "pci.h"
@@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain;
 
 	/*
 	 * Check DT domain and use_dt_domains values.
@@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 	 * API and update the use_dt_domains value to keep track of method we
 	 * are using to assign domain numbers (use_dt_domains = 0).
 	 *
+	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
+	 * and call _SEG method for corresponding host bridge device.
+	 * If _SEG method does not exist, following ACPI spec (6.5.6)
+	 * all PCI buses belong to domain 0.
+	 *
 	 * All other combinations imply we have a platform that is trying
-	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
-	 * which is a recipe for domain mishandling and it is prevented by
-	 * invalidating the domain value (domain = -1) and printing a
-	 * corresponding error.
+	 * to mix domain numbers obtained from DT, ACPI and
+	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
+	 * it is prevented by invalidating the domain value (domain = -1) and
+	 * printing a corresponding error.
 	 */
+
+	domain = of_get_pci_domain_nr(parent->of_node);
 	if (domain >= 0 && use_dt_domains) {
 		use_dt_domains = 1;
+#ifdef CONFIG_ACPI
+	} else if (!acpi_disabled && use_dt_domains == -1) {
+		struct acpi_device *acpi_dev = to_acpi_device(parent);
+		unsigned long long segment = 0;
+		acpi_status status;
+
+		status = acpi_evaluate_integer(acpi_dev->handle,
+					       METHOD_NAME__SEG, NULL,
+					       &segment);
+		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+			dev_err(&acpi_dev->dev,  "can't evaluate _SEG\n");
+
+		domain = segment;
+#endif
 	} else if (domain < 0 && use_dt_domains != 1) {
 		use_dt_domains = 0;
 		domain = pci_get_new_domain_nr();
-- 
1.9.1

Re: [PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Liviu.Dudau]

On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote:
> Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
> initialization since this function needs valid parent device reference
> to be able to retrieve domain number (aka segment).
> 
> We can omit that blocker and pass down host bridge device via
> pci_create_root_bus parameter and then be able to evaluate _SEG method
> being in pci_bus_assign_domain_nr.
> 
> Note that _SEG method is optional, therefore _SEG absence means
> that all PCI buses belong to domain 0.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_root.c |  2 +-
>  drivers/pci/pci.c       | 32 +++++++++++++++++++++++++++-----
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 850d7bf..e682dc6 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	pci_acpi_root_add_resources(info);
>  	pci_add_resource(&info->resources, &root->secondary);
> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>  				  sysdata, &info->resources);

Not sure this change should be in this patch, I don't see the relation.

To put it differently: I think the patch should introduce the retrieval of the
domain number from _SEG method and leave the passing of a valid host bridge device
to a more appropriate patch.


>  	if (!bus)
>  		goto out_release_info;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6a9a111..17d1857 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -25,6 +25,7 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/acpi.h>
>  #include <asm-generic/pci-bridge.h>
>  #include <asm/setup.h>
>  #include "pci.h"
> @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
>  void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  {
>  	static int use_dt_domains = -1;
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> +	int domain;
>  
>  	/*
>  	 * Check DT domain and use_dt_domains values.
> @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  	 * API and update the use_dt_domains value to keep track of method we
>  	 * are using to assign domain numbers (use_dt_domains = 0).
>  	 *
> +	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
> +	 * and call _SEG method for corresponding host bridge device.
> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> +	 * all PCI buses belong to domain 0.
> +	 *
>  	 * All other combinations imply we have a platform that is trying
> -	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> -	 * which is a recipe for domain mishandling and it is prevented by
> -	 * invalidating the domain value (domain = -1) and printing a
> -	 * corresponding error.
> +	 * to mix domain numbers obtained from DT, ACPI and
> +	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
> +	 * it is prevented by invalidating the domain value (domain = -1) and
> +	 * printing a corresponding error.
>  	 */
> +
> +	domain = of_get_pci_domain_nr(parent->of_node);

Not sure what you've got here by splitting the original line into two other than an increase
in the change count.

Otherwise, it looks sensible.

Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>


>  	if (domain >= 0 && use_dt_domains) {
>  		use_dt_domains = 1;
> +#ifdef CONFIG_ACPI
> +	} else if (!acpi_disabled && use_dt_domains == -1) {
> +		struct acpi_device *acpi_dev = to_acpi_device(parent);
> +		unsigned long long segment = 0;
> +		acpi_status status;
> +
> +		status = acpi_evaluate_integer(acpi_dev->handle,
> +					       METHOD_NAME__SEG, NULL,
> +					       &segment);
> +		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +			dev_err(&acpi_dev->dev,  "can't evaluate _SEG\n");
> +
> +		domain = segment;
> +#endif
>  	} else if (domain < 0 && use_dt_domains != 1) {
>  		use_dt_domains = 0;
>  		domain = pci_get_new_domain_nr();
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [Linaro-acpi] [PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Tomasz Nowicki]

Hi Liviu,

On 28.10.2015 12:38, Liviu.Dudau@arm.com wrote:
> On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote:
>> Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
>> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
>> initialization since this function needs valid parent device reference
>> to be able to retrieve domain number (aka segment).
>>
>> We can omit that blocker and pass down host bridge device via
>> pci_create_root_bus parameter and then be able to evaluate _SEG method
>> being in pci_bus_assign_domain_nr.
>>
>> Note that _SEG method is optional, therefore _SEG absence means
>> that all PCI buses belong to domain 0.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>   drivers/acpi/pci_root.c |  2 +-
>>   drivers/pci/pci.c       | 32 +++++++++++++++++++++++++++-----
>>   2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 850d7bf..e682dc6 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>
>>   	pci_acpi_root_add_resources(info);
>>   	pci_add_resource(&info->resources, &root->secondary);
>> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>>   				  sysdata, &info->resources);
>
> Not sure this change should be in this patch, I don't see the relation.
>
> To put it differently: I think the patch should introduce the retrieval of the
> domain number from _SEG method and leave the passing of a valid host bridge device
> to a more appropriate patch.

I wanted to highlight that ACPI kernel using PCI_DOMAINS_GENERIC needs 
to have both in place:
1. Obtaining domain from _SEG method
2. And host bridge device reference for which we can call _SEG.
But you are right, it will be more clear if I split up patch and 
describe it in separate changelog.

>
>
>>   	if (!bus)
>>   		goto out_release_info;
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 6a9a111..17d1857 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/device.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/pci_hotplug.h>
>> +#include <linux/acpi.h>
>>   #include <asm-generic/pci-bridge.h>
>>   #include <asm/setup.h>
>>   #include "pci.h"
>> @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
>>   void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>   {
>>   	static int use_dt_domains = -1;
>> -	int domain = of_get_pci_domain_nr(parent->of_node);
>> +	int domain;
>>
>>   	/*
>>   	 * Check DT domain and use_dt_domains values.
>> @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>   	 * API and update the use_dt_domains value to keep track of method we
>>   	 * are using to assign domain numbers (use_dt_domains = 0).
>>   	 *
>> +	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
>> +	 * and call _SEG method for corresponding host bridge device.
>> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
>> +	 * all PCI buses belong to domain 0.
>> +	 *
>>   	 * All other combinations imply we have a platform that is trying
>> -	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
>> -	 * which is a recipe for domain mishandling and it is prevented by
>> -	 * invalidating the domain value (domain = -1) and printing a
>> -	 * corresponding error.
>> +	 * to mix domain numbers obtained from DT, ACPI and
>> +	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
>> +	 * it is prevented by invalidating the domain value (domain = -1) and
>> +	 * printing a corresponding error.
>>   	 */
>> +
>> +	domain = of_get_pci_domain_nr(parent->of_node);
>
> Not sure what you've got here by splitting the original line into two other than an increase
> in the change count.

Yes, it does not make sense to split the original line. I will fix that.

>
> Otherwise, it looks sensible.
>
> Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>

Thanks Liviu!

Regards,
Tomasz

Re: [PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Lorenzo Pieralisi]

On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote:
> Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
> initialization since this function needs valid parent device reference
> to be able to retrieve domain number (aka segment).
> 
> We can omit that blocker and pass down host bridge device via
> pci_create_root_bus parameter and then be able to evaluate _SEG method
> being in pci_bus_assign_domain_nr.
> 
> Note that _SEG method is optional, therefore _SEG absence means
> that all PCI buses belong to domain 0.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_root.c |  2 +-
>  drivers/pci/pci.c       | 32 +++++++++++++++++++++++++++-----
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 850d7bf..e682dc6 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	pci_acpi_root_add_resources(info);
>  	pci_add_resource(&info->resources, &root->secondary);
> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>  				  sysdata, &info->resources);

If I read x86 code correctly, they rely on the first argument to be
NULL, I think you would break x86 by doing that, see:

arch/x86/pci/acpi.c (pcibios_root_bridge_prepare())

By the way, can't we move the code setting up the ACPI_COMPANION
to core PCI code and stop relying on sysdata for that ?

Thanks,
Lorenzo

>  	if (!bus)
>  		goto out_release_info;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6a9a111..17d1857 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -25,6 +25,7 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/acpi.h>
>  #include <asm-generic/pci-bridge.h>
>  #include <asm/setup.h>
>  #include "pci.h"
> @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void)
>  void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  {
>  	static int use_dt_domains = -1;
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> +	int domain;
>  
>  	/*
>  	 * Check DT domain and use_dt_domains values.
> @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  	 * API and update the use_dt_domains value to keep track of method we
>  	 * are using to assign domain numbers (use_dt_domains = 0).
>  	 *
> +	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
> +	 * and call _SEG method for corresponding host bridge device.
> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> +	 * all PCI buses belong to domain 0.
> +	 *
>  	 * All other combinations imply we have a platform that is trying
> -	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> -	 * which is a recipe for domain mishandling and it is prevented by
> -	 * invalidating the domain value (domain = -1) and printing a
> -	 * corresponding error.
> +	 * to mix domain numbers obtained from DT, ACPI and
> +	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
> +	 * it is prevented by invalidating the domain value (domain = -1) and
> +	 * printing a corresponding error.
>  	 */
> +
> +	domain = of_get_pci_domain_nr(parent->of_node);
>  	if (domain >= 0 && use_dt_domains) {
>  		use_dt_domains = 1;
> +#ifdef CONFIG_ACPI
> +	} else if (!acpi_disabled && use_dt_domains == -1) {
> +		struct acpi_device *acpi_dev = to_acpi_device(parent);
> +		unsigned long long segment = 0;
> +		acpi_status status;
> +
> +		status = acpi_evaluate_integer(acpi_dev->handle,
> +					       METHOD_NAME__SEG, NULL,
> +					       &segment);
> +		if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +			dev_err(&acpi_dev->dev,  "can't evaluate _SEG\n");
> +
> +		domain = segment;
> +#endif
>  	} else if (domain < 0 && use_dt_domains != 1) {
>  		use_dt_domains = 0;
>  		domain = pci_get_new_domain_nr();
> -- 
> 1.9.1
> 

Re: [Linaro-acpi] [PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Tomasz Nowicki]

On 03.11.2015 17:10, Lorenzo Pieralisi wrote:
> On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote:
>> Architectures which support PCI_DOMAINS_GENERIC (like ARM64)
>> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge
>> initialization since this function needs valid parent device reference
>> to be able to retrieve domain number (aka segment).
>>
>> We can omit that blocker and pass down host bridge device via
>> pci_create_root_bus parameter and then be able to evaluate _SEG method
>> being in pci_bus_assign_domain_nr.
>>
>> Note that _SEG method is optional, therefore _SEG absence means
>> that all PCI buses belong to domain 0.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>   drivers/acpi/pci_root.c |  2 +-
>>   drivers/pci/pci.c       | 32 +++++++++++++++++++++++++++-----
>>   2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 850d7bf..e682dc6 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>
>>   	pci_acpi_root_add_resources(info);
>>   	pci_add_resource(&info->resources, &root->secondary);
>> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>>   				  sysdata, &info->resources);
>
> If I read x86 code correctly, they rely on the first argument to be
> NULL, I think you would break x86 by doing that, see:
>
> arch/x86/pci/acpi.c (pcibios_root_bridge_prepare())
>
> By the way, can't we move the code setting up the ACPI_COMPANION
> to core PCI code and stop relying on sysdata for that ?
>

Yes, that will break x86&ia64 but as you said it may be overcome with 
consolidation of ACPI_COMPANION into the core code.

Tomasz

[PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Tomasz Nowicki]

Because of two patch series:
1. Jiang Liu's common interface to support PCI host bridge init
2. Refactoring of MMCONFIG, part of this patch set
now we can think about PCI buses enumeration for ARM64 and ACPI tables.

This patch introduce ACPI based PCI hostbridge init calls which
use information from MCFG table (PCI config space regions) and
_CRS (IO/irq resources) to initialize PCI hostbridge.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Liviu Dudau <Liviu.Dudau@arm.com>
CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig      |   6 ++
 arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 202 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 07d1811..bbcc6b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -89,6 +89,7 @@ config ARM64
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select HAVE_CONTEXT_TRACKING
+	select HAVE_PCI_ECAM
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
 source "drivers/pci/pcie/Kconfig"
 source "drivers/pci/hotplug/Kconfig"
 
+config PCI_MMCONFIG
+	def_bool y
+	select PCI_ECAM
+	depends on ACPI
+
 endmenu
 
 menu "Kernel Features"
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index b3d098b..66cc1ae 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -11,12 +11,15 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/ecam.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/of_address.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
+#include <linux/pci-acpi.h>
 #include <linux/slab.h>
 
 #include <asm/pci-bridge.h>
@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
  */
 int pcibios_add_device(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
+		acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	struct acpi_pci_root *root = bridge->bus->sysdata;
+
+	ACPI_COMPANION_SET(&bridge->dev, root->device);
+	return 0;
+}
+
+void pcibios_add_bus(struct pci_bus *bus)
+{
+	acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+	acpi_pci_remove_bus(bus);
+}
+
+static int __init pcibios_assign_resources(void)
+{
+	if (acpi_disabled)
+		return 0;
+
+	pci_assign_unassigned_resources();
+	return 0;
+}
 /*
- * raw_pci_read/write - Platform-specific PCI config space access.
+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
  */
-int raw_pci_read(unsigned int domain, unsigned int bus,
-		  unsigned int devfn, int reg, int len, u32 *val)
+rootfs_initcall(pcibios_assign_resources);
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
 {
-	return -ENXIO;
+	struct pci_mmcfg_region *cfg;
+
+	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+	if (cfg && cfg->virt)
+		return cfg->virt +
+			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+			offset;
+	return NULL;
 }
 
-int raw_pci_write(unsigned int domain, unsigned int bus,
-		unsigned int devfn, int reg, int len, u32 val)
+struct pci_ops pci_root_ops = {
+	.map_bus = pci_mcfg_dev_base,
+	.read = pci_generic_config_read,
+	.write = pci_generic_config_write,
+};
+
+#ifdef CONFIG_PCI_MMCONFIG
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
 {
-	return -ENXIO;
+	struct pci_mmcfg_region *cfg;
+	struct acpi_pci_root *root;
+	int seg, start, end, err;
+
+	root = ci->root;
+	seg = root->segment;
+	start = root->secondary.start;
+	end = root->secondary.end;
+
+	cfg = pci_mmconfig_lookup(seg, start);
+	if (cfg)
+		return 0;
+
+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
+	if (!cfg)
+		return -ENOMEM;
+
+	err = pci_mmconfig_inject(cfg);
+	return err;
 }
 
-#ifdef CONFIG_ACPI
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+	struct acpi_pci_root *root = ci->root;
+	struct pci_mmcfg_region *cfg;
+
+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+	if (cfg)
+		return;
+
+	if (cfg->hot_added)
+		pci_mmconfig_delete(root->segment, root->secondary.start,
+				    root->secondary.end);
+}
+#else
+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
+{
+	return 0;
+}
+
+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
+#endif
+
+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
+{
+	return pci_add_mmconfig_region(ci);
+}
+
+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
+{
+	pci_remove_mmconfig_region(ci);
+	kfree(ci);
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+	struct resource_entry *entry, *tmp;
+	int ret;
+
+	ret = acpi_pci_probe_root_resources(ci);
+	if (ret < 0)
+		return ret;
+
+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+		struct resource *res = entry->res;
+
+		/*
+		 * Special handling for ARM IO range
+		 * TODO: need to move pci_register_io_range() function out
+		 * of drivers/of/address.c for both used by DT and ACPI
+		 */
+		if (res->flags & IORESOURCE_IO) {
+			unsigned long port;
+			int err;
+			resource_size_t length = res->end - res->start;
+
+			err = pci_register_io_range(res->start, length);
+			if (err) {
+				resource_list_destroy_entry(entry);
+				continue;
+			}
+
+			port = pci_address_to_pio(res->start);
+			if (port == (unsigned long)-1) {
+				resource_list_destroy_entry(entry);
+				continue;
+			}
+
+			res->start = port;
+			res->end = res->start + length - 1;
+
+			if (pci_remap_iospace(res, res->start) < 0)
+				resource_list_destroy_entry(entry);
+		}
+	}
+
+	return 0;
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.pci_ops = &pci_root_ops,
+	.init_info = pci_acpi_root_init_info,
+	.release_info = pci_acpi_root_release_info,
+	.prepare_resources = pci_acpi_root_prepare_resources,
+};
+
 /* Root bridge scanning */
 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);
+	int domain = root->segment;
+	int busnum = root->secondary.start;
+	struct acpi_pci_root_info *info;
+	struct pci_bus *bus;
+
+	if (domain && !pci_domains_supported) {
+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
+			domain, busnum);
+		return NULL;
+	}
+
+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
+	if (!info) {
+		dev_err(&root->device->dev,
+			"pci_bus %04x:%02x: ignored (out of memory)\n",
+			domain, busnum);
+		return NULL;
+	}
+
+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
+
+	/* After the PCI-E bus has been walked and all devices discovered,
+	 * configure any settings of the fabric that might be necessary.
+	 */
+	if (bus) {
+		struct pci_bus *child;
+
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
+
+	return bus;
 }
 #endif
-- 
1.9.1

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Liviu.Dudau]

On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> Because of two patch series:
> 1. Jiang Liu's common interface to support PCI host bridge init
> 2. Refactoring of MMCONFIG, part of this patch set
> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
> 
> This patch introduce ACPI based PCI hostbridge init calls which
> use information from MCFG table (PCI config space regions) and
> _CRS (IO/irq resources) to initialize PCI hostbridge.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Liviu Dudau <Liviu.Dudau@arm.com>
> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/Kconfig      |   6 ++
>  arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 202 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 07d1811..bbcc6b1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -89,6 +89,7 @@ config ARM64
>  	select SPARSE_IRQ
>  	select SYSCTL_EXCEPTION_TRACE
>  	select HAVE_CONTEXT_TRACKING
> +	select HAVE_PCI_ECAM
>  	help
>  	  ARM 64-bit (AArch64) Linux support.
>  
> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>  source "drivers/pci/pcie/Kconfig"
>  source "drivers/pci/hotplug/Kconfig"
>  
> +config PCI_MMCONFIG
> +	def_bool y
> +	select PCI_ECAM
> +	depends on ACPI
> +
>  endmenu
>  
>  menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/ecam.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> +#include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/slab.h>
>  
>  #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>   */
>  int pcibios_add_device(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
> +		acpi_pci_irq_enable(dev);
> +#endif
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;
> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}
> +
> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;

You can change this function into:
{
	if (!acpi_disabled)
		pci_assign_unassigned_resources();

	return 0;
}

as the equivalent but shorter form.

> +}
>  /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>   */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)

What happened with raw_pci_{read,write} ? Why do you remove them?


> +rootfs_initcall(pcibios_assign_resources);

Would you be so kind and explain to me why you need this initcall?
Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
pci_scan_root_bus() ?

I haven't focused on ACPI before so I'm a bit hazy on the init order when
that is enabled. That being said, I don't like adding in the architecture
code initcall hooks just to fix up some dependency orders that could / should
be fixed some other way.

> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;
>  }
>  
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>  }
>  
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif
> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range

There is nothing ARM specific here. It should apply to any memory mapped IO range.

> +		 * TODO: need to move pci_register_io_range() function out
> +		 * of drivers/of/address.c for both used by DT and ACPI
> +		 */
> +		if (res->flags & IORESOURCE_IO) {
> +			unsigned long port;
> +			int err;
> +			resource_size_t length = res->end - res->start;
> +
> +			err = pci_register_io_range(res->start, length);
> +			if (err) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			port = pci_address_to_pio(res->start);
> +			if (port == (unsigned long)-1) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			res->start = port;
> +			res->end = res->start + length - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>  /* Root bridge scanning */
>  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);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;
>  }
>  #endif
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Tomasz Nowicki]

On 28.10.2015 12:49, Liviu.Dudau@arm.com wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>> Because of two patch series:
>> 1. Jiang Liu's common interface to support PCI host bridge init
>> 2. Refactoring of MMCONFIG, part of this patch set
>> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
>>
>> This patch introduce ACPI based PCI hostbridge init calls which
>> use information from MCFG table (PCI config space regions) and
>> _CRS (IO/irq resources) to initialize PCI hostbridge.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Liviu Dudau <Liviu.Dudau@arm.com>
>> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
>> CC: Will Deacon <will.deacon@arm.com>
>> ---
>>   arch/arm64/Kconfig      |   6 ++
>>   arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 202 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 07d1811..bbcc6b1 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -89,6 +89,7 @@ config ARM64
>>   	select SPARSE_IRQ
>>   	select SYSCTL_EXCEPTION_TRACE
>>   	select HAVE_CONTEXT_TRACKING
>> +	select HAVE_PCI_ECAM
>>   	help
>>   	  ARM 64-bit (AArch64) Linux support.
>>
>> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>>   source "drivers/pci/pcie/Kconfig"
>>   source "drivers/pci/hotplug/Kconfig"
>>
>> +config PCI_MMCONFIG
>> +	def_bool y
>> +	select PCI_ECAM
>> +	depends on ACPI
>> +
>>   endmenu
>>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(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
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>> +
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> You can change this function into:
> {
> 	if (!acpi_disabled)
> 		pci_assign_unassigned_resources();
>
> 	return 0;
> }
>
> as the equivalent but shorter form.

Sure, will do.

>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>
> What happened with raw_pci_{read,write} ? Why do you remove them?

We do not need raw_pci_{read,write} any more, we will use empty 
raw_pci_{read,write} from mcfg.c introduced in patch 6/11.

I think this is another candidate to split up, first I will remove these 
raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge 
init, it will be easier to review, what do you think?

>
>
>> +rootfs_initcall(pcibios_assign_resources);
>
> Would you be so kind and explain to me why you need this initcall?
> Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> pci_scan_root_bus() ?
>
> I haven't focused on ACPI before so I'm a bit hazy on the init order when
> that is enabled. That being said, I don't like adding in the architecture
> code initcall hooks just to fix up some dependency orders that could / should
> be fixed some other way.

My idea was to defer resource reassigning to give a chance for running 
DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set 
IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to 
defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me 
get back to you on this.

>
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>>   }
>>
>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>> -		unsigned int devfn, int reg, int len, u32 val)
>> +struct pci_ops pci_root_ops = {
>> +	.map_bus = pci_mcfg_dev_base,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>> +#ifdef CONFIG_PCI_MMCONFIG
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +	struct acpi_pci_root *root;
>> +	int seg, start, end, err;
>> +
>> +	root = ci->root;
>> +	seg = root->segment;
>> +	start = root->secondary.start;
>> +	end = root->secondary.end;
>> +
>> +	cfg = pci_mmconfig_lookup(seg, start);
>> +	if (cfg)
>> +		return 0;
>> +
>> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
>> +	if (!cfg)
>> +		return -ENOMEM;
>> +
>> +	err = pci_mmconfig_inject(cfg);
>> +	return err;
>>   }
>>
>> -#ifdef CONFIG_ACPI
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	struct acpi_pci_root *root = ci->root;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +	if (cfg)
>> +		return;
>> +
>> +	if (cfg->hot_added)
>> +		pci_mmconfig_delete(root->segment, root->secondary.start,
>> +				    root->secondary.end);
>> +}
>> +#else
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
>> +#endif
>> +
>> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
>> +{
>> +	return pci_add_mmconfig_region(ci);
>> +}
>> +
>> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +	pci_remove_mmconfig_region(ci);
>> +	kfree(ci);
>> +}
>> +
>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> +{
>> +	struct resource_entry *entry, *tmp;
>> +	int ret;
>> +
>> +	ret = acpi_pci_probe_root_resources(ci);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>> +		struct resource *res = entry->res;
>> +
>> +		/*
>> +		 * Special handling for ARM IO range
>
> There is nothing ARM specific here. It should apply to any memory mapped IO range.

OK, I will remove that comment.

>
>> +		 * TODO: need to move pci_register_io_range() function out
>> +		 * of drivers/of/address.c for both used by DT and ACPI
>> +		 */
>> +		if (res->flags & IORESOURCE_IO) {
>> +			unsigned long port;
>> +			int err;
>> +			resource_size_t length = res->end - res->start;
>> +
>> +			err = pci_register_io_range(res->start, length);
>> +			if (err) {
>> +				resource_list_destroy_entry(entry);
>> +				continue;
>> +			}
>> +
>> +			port = pci_address_to_pio(res->start);
>> +			if (port == (unsigned long)-1) {
>> +				resource_list_destroy_entry(entry);
>> +				continue;
>> +			}
>> +
>> +			res->start = port;
>> +			res->end = res->start + length - 1;
>> +
>> +			if (pci_remap_iospace(res, res->start) < 0)
>> +				resource_list_destroy_entry(entry);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +	.pci_ops = &pci_root_ops,
>> +	.init_info = pci_acpi_root_init_info,
>> +	.release_info = pci_acpi_root_release_info,
>> +	.prepare_resources = pci_acpi_root_prepare_resources,
>> +};
>> +
>>   /* Root bridge scanning */
>>   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);
>> +	int domain = root->segment;
>> +	int busnum = root->secondary.start;
>> +	struct acpi_pci_root_info *info;
>> +	struct pci_bus *bus;
>> +
>> +	if (domain && !pci_domains_supported) {
>> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
>> +	if (!info) {
>> +		dev_err(&root->device->dev,
>> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>> +
>> +	/* After the PCI-E bus has been walked and all devices discovered,
>> +	 * configure any settings of the fabric that might be necessary.
>> +	 */
>> +	if (bus) {
>> +		struct pci_bus *child;
>> +
>> +		list_for_each_entry(child, &bus->children, node)
>> +			pcie_bus_configure_settings(child);
>> +	}
>> +
>> +	return bus;
>>   }
>>   #endif
>> --
>> 1.9.1
>>
>

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Liviu.Dudau]

On Wed, Oct 28, 2015 at 02:42:30PM +0100, Tomasz Nowicki wrote:
> On 28.10.2015 12:49, Liviu.Dudau@arm.com wrote:
> >On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> >>Because of two patch series:
> >>1. Jiang Liu's common interface to support PCI host bridge init
> >>2. Refactoring of MMCONFIG, part of this patch set
> >>now we can think about PCI buses enumeration for ARM64 and ACPI tables.
> >>
> >>This patch introduce ACPI based PCI hostbridge init calls which
> >>use information from MCFG table (PCI config space regions) and
> >>_CRS (IO/irq resources) to initialize PCI hostbridge.
> >>
> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >>Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >>CC: Arnd Bergmann <arnd@arndb.de>
> >>CC: Catalin Marinas <catalin.marinas@arm.com>
> >>CC: Liviu Dudau <Liviu.Dudau@arm.com>
> >>CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> >>CC: Will Deacon <will.deacon@arm.com>
> >>---
> >>  arch/arm64/Kconfig      |   6 ++
> >>  arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 202 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >>index 07d1811..bbcc6b1 100644
> >>--- a/arch/arm64/Kconfig
> >>+++ b/arch/arm64/Kconfig
> >>@@ -89,6 +89,7 @@ config ARM64
> >>  	select SPARSE_IRQ
> >>  	select SYSCTL_EXCEPTION_TRACE
> >>  	select HAVE_CONTEXT_TRACKING
> >>+	select HAVE_PCI_ECAM
> >>  	help
> >>  	  ARM 64-bit (AArch64) Linux support.
> >>
> >>@@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
> >>  source "drivers/pci/pcie/Kconfig"
> >>  source "drivers/pci/hotplug/Kconfig"
> >>
> >>+config PCI_MMCONFIG
> >>+	def_bool y
> >>+	select PCI_ECAM
> >>+	depends on ACPI
> >>+
> >>  endmenu
> >>
> >>  menu "Kernel Features"
> >>diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >>index b3d098b..66cc1ae 100644
> >>--- a/arch/arm64/kernel/pci.c
> >>+++ b/arch/arm64/kernel/pci.c
> >>@@ -11,12 +11,15 @@
> >>   */
> >>
> >>  #include <linux/acpi.h>
> >>+#include <linux/ecam.h>
> >>  #include <linux/init.h>
> >>  #include <linux/io.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/mm.h>
> >>+#include <linux/of_address.h>
> >>  #include <linux/of_pci.h>
> >>  #include <linux/of_platform.h>
> >>+#include <linux/pci-acpi.h>
> >>  #include <linux/slab.h>
> >>
> >>  #include <asm/pci-bridge.h>
> >>@@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
> >>   */
> >>  int pcibios_add_device(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
> >>+		acpi_pci_irq_enable(dev);
> >>+#endif
> >>
> >>  	return 0;
> >>  }
> >>
> >>+#ifdef CONFIG_ACPI
> >>+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> >>+{
> >>+	struct acpi_pci_root *root = bridge->bus->sysdata;
> >>+
> >>+	ACPI_COMPANION_SET(&bridge->dev, root->device);
> >>+	return 0;
> >>+}
> >>+
> >>+void pcibios_add_bus(struct pci_bus *bus)
> >>+{
> >>+	acpi_pci_add_bus(bus);
> >>+}
> >>+
> >>+void pcibios_remove_bus(struct pci_bus *bus)
> >>+{
> >>+	acpi_pci_remove_bus(bus);
> >>+}
> >>+
> >>+static int __init pcibios_assign_resources(void)
> >>+{
> >>+	if (acpi_disabled)
> >>+		return 0;
> >>+
> >>+	pci_assign_unassigned_resources();
> >>+	return 0;
> >
> >You can change this function into:
> >{
> >	if (!acpi_disabled)
> >		pci_assign_unassigned_resources();
> >
> >	return 0;
> >}
> >
> >as the equivalent but shorter form.
> 
> Sure, will do.
> 
> >
> >>+}
> >>  /*
> >>- * raw_pci_read/write - Platform-specific PCI config space access.
> >>+ * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> >>+ * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> >>   */
> >>-int raw_pci_read(unsigned int domain, unsigned int bus,
> >>-		  unsigned int devfn, int reg, int len, u32 *val)
> >
> >What happened with raw_pci_{read,write} ? Why do you remove them?
> 
> We do not need raw_pci_{read,write} any more, we will use empty
> raw_pci_{read,write} from mcfg.c introduced in patch 6/11.
> 
> I think this is another candidate to split up, first I will remove these
> raw_pci_{read,write} accessors and then introduce ACPI PCI hostbridge init,
> it will be easier to review, what do you think?

Yes, I like that. If the calls were redundant after 6/11 then they should
not have been kept until this patch to be removed.

> 
> >
> >
> >>+rootfs_initcall(pcibios_assign_resources);
> >
> >Would you be so kind and explain to me why you need this initcall?
> >Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> >pci_scan_root_bus() ?
> >
> >I haven't focused on ACPI before so I'm a bit hazy on the init order when
> >that is enabled. That being said, I don't like adding in the architecture
> >code initcall hooks just to fix up some dependency orders that could / should
> >be fixed some other way.
> 
> My idea was to defer resource reassigning to give a chance for running
> DECLARE_PCI_FIXUP_FINAL. I used DECLARE_PCI_FIXUP_FINAL to set
> IORESOURCE_PCI_FIXED for some PCI devices. Now I am not sure we need to
> defer it, my DECLARE_PCI_FIXUP_FINALs might be done in firmware, let me get
> back to you on this.

If fixups can be done in firmware then that is the preferred direction nowadays.

> 
> >
> >>+
> >>+static void __iomem *
> >>+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> >>  {
> >>-	return -ENXIO;
> >>+	struct pci_mmcfg_region *cfg;
> >>+
> >>+	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> >>+	if (cfg && cfg->virt)
> >>+		return cfg->virt +
> >>+			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> >>+			offset;
> >>+	return NULL;
> >>  }
> >>
> >>-int raw_pci_write(unsigned int domain, unsigned int bus,
> >>-		unsigned int devfn, int reg, int len, u32 val)
> >>+struct pci_ops pci_root_ops = {
> >>+	.map_bus = pci_mcfg_dev_base,
> >>+	.read = pci_generic_config_read,
> >>+	.write = pci_generic_config_write,
> >>+};
> >>+
> >>+#ifdef CONFIG_PCI_MMCONFIG
> >>+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >>  {
> >>-	return -ENXIO;
> >>+	struct pci_mmcfg_region *cfg;
> >>+	struct acpi_pci_root *root;
> >>+	int seg, start, end, err;
> >>+
> >>+	root = ci->root;
> >>+	seg = root->segment;
> >>+	start = root->secondary.start;
> >>+	end = root->secondary.end;
> >>+
> >>+	cfg = pci_mmconfig_lookup(seg, start);
> >>+	if (cfg)
> >>+		return 0;
> >>+
> >>+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> >>+	if (!cfg)
> >>+		return -ENOMEM;
> >>+
> >>+	err = pci_mmconfig_inject(cfg);
> >>+	return err;
> >>  }
> >>
> >>-#ifdef CONFIG_ACPI
> >>+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> >>+{
> >>+	struct acpi_pci_root *root = ci->root;
> >>+	struct pci_mmcfg_region *cfg;
> >>+
> >>+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> >>+	if (cfg)
> >>+		return;
> >>+
> >>+	if (cfg->hot_added)
> >>+		pci_mmconfig_delete(root->segment, root->secondary.start,
> >>+				    root->secondary.end);
> >>+}
> >>+#else
> >>+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >>+{
> >>+	return 0;
> >>+}
> >>+
> >>+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> >>+#endif
> >>+
> >>+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> >>+{
> >>+	return pci_add_mmconfig_region(ci);
> >>+}
> >>+
> >>+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> >>+{
> >>+	pci_remove_mmconfig_region(ci);
> >>+	kfree(ci);
> >>+}
> >>+
> >>+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> >>+{
> >>+	struct resource_entry *entry, *tmp;
> >>+	int ret;
> >>+
> >>+	ret = acpi_pci_probe_root_resources(ci);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> >>+		struct resource *res = entry->res;
> >>+
> >>+		/*
> >>+		 * Special handling for ARM IO range
> >
> >There is nothing ARM specific here. It should apply to any memory mapped IO range.
> 
> OK, I will remove that comment.

Thanks!

Liviu

> 
> >
> >>+		 * TODO: need to move pci_register_io_range() function out
> >>+		 * of drivers/of/address.c for both used by DT and ACPI
> >>+		 */
> >>+		if (res->flags & IORESOURCE_IO) {
> >>+			unsigned long port;
> >>+			int err;
> >>+			resource_size_t length = res->end - res->start;
> >>+
> >>+			err = pci_register_io_range(res->start, length);
> >>+			if (err) {
> >>+				resource_list_destroy_entry(entry);
> >>+				continue;
> >>+			}
> >>+
> >>+			port = pci_address_to_pio(res->start);
> >>+			if (port == (unsigned long)-1) {
> >>+				resource_list_destroy_entry(entry);
> >>+				continue;
> >>+			}
> >>+
> >>+			res->start = port;
> >>+			res->end = res->start + length - 1;
> >>+
> >>+			if (pci_remap_iospace(res, res->start) < 0)
> >>+				resource_list_destroy_entry(entry);
> >>+		}
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static struct acpi_pci_root_ops acpi_pci_root_ops = {
> >>+	.pci_ops = &pci_root_ops,
> >>+	.init_info = pci_acpi_root_init_info,
> >>+	.release_info = pci_acpi_root_release_info,
> >>+	.prepare_resources = pci_acpi_root_prepare_resources,
> >>+};
> >>+
> >>  /* Root bridge scanning */
> >>  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);
> >>+	int domain = root->segment;
> >>+	int busnum = root->secondary.start;
> >>+	struct acpi_pci_root_info *info;
> >>+	struct pci_bus *bus;
> >>+
> >>+	if (domain && !pci_domains_supported) {
> >>+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> >>+			domain, busnum);
> >>+		return NULL;
> >>+	}
> >>+
> >>+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> >>+	if (!info) {
> >>+		dev_err(&root->device->dev,
> >>+			"pci_bus %04x:%02x: ignored (out of memory)\n",
> >>+			domain, busnum);
> >>+		return NULL;
> >>+	}
> >>+
> >>+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> >>+
> >>+	/* After the PCI-E bus has been walked and all devices discovered,
> >>+	 * configure any settings of the fabric that might be necessary.
> >>+	 */
> >>+	if (bus) {
> >>+		struct pci_bus *child;
> >>+
> >>+		list_for_each_entry(child, &bus->children, node)
> >>+			pcie_bus_configure_settings(child);
> >>+	}
> >>+
> >>+	return bus;
> >>  }
> >>  #endif
> >>--
> >>1.9.1
> >>
> >
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Lorenzo Pieralisi]

On Wed, Oct 28, 2015 at 11:49:40AM +0000, Liviu.Dudau@arm.com wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:

[...]

> > +static int __init pcibios_assign_resources(void)
> > +{
> > +	if (acpi_disabled)
> > +		return 0;
> > +
> > +	pci_assign_unassigned_resources();
> > +	return 0;
> 
> You can change this function into:
> {
> 	if (!acpi_disabled)
> 		pci_assign_unassigned_resources();
> 
> 	return 0;
> }
> 
> as the equivalent but shorter form.

I do not think it is a matter of code style here, it is a matter
of understanding when and if we want to reassign resources on ACPI
systems, it is an open question on ARM64 that must be sorted out (ie we
ignore FW/BARs set-up entirely).

> > +}
> >  /*
> > - * raw_pci_read/write - Platform-specific PCI config space access.
> > + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> > + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> >   */
> > -int raw_pci_read(unsigned int domain, unsigned int bus,
> > -		  unsigned int devfn, int reg, int len, u32 *val)
> 
> What happened with raw_pci_{read,write} ? Why do you remove them?
> 
> 
> > +rootfs_initcall(pcibios_assign_resources);
> 
> Would you be so kind and explain to me why you need this initcall?
> Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> pci_scan_root_bus() ?

On what basis ? BTW, PCI core code does not assign unassigned resources
anyway even if that flag is set, so some policy has to be defined here.

Thanks,
Lorenzo

> I haven't focused on ACPI before so I'm a bit hazy on the init order when
> that is enabled. That being said, I don't like adding in the architecture
> code initcall hooks just to fix up some dependency orders that could / should
> be fixed some other way.
> 
> > +
> > +static void __iomem *
> > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> >  {
> > -	return -ENXIO;
> > +	struct pci_mmcfg_region *cfg;
> > +
> > +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> > +	if (cfg && cfg->virt)
> > +		return cfg->virt +
> > +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> > +			offset;
> > +	return NULL;
> >  }
> >  
> > -int raw_pci_write(unsigned int domain, unsigned int bus,
> > -		unsigned int devfn, int reg, int len, u32 val)
> > +struct pci_ops pci_root_ops = {
> > +	.map_bus = pci_mcfg_dev_base,
> > +	.read = pci_generic_config_read,
> > +	.write = pci_generic_config_write,
> > +};
> > +
> > +#ifdef CONFIG_PCI_MMCONFIG
> > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >  {
> > -	return -ENXIO;
> > +	struct pci_mmcfg_region *cfg;
> > +	struct acpi_pci_root *root;
> > +	int seg, start, end, err;
> > +
> > +	root = ci->root;
> > +	seg = root->segment;
> > +	start = root->secondary.start;
> > +	end = root->secondary.end;
> > +
> > +	cfg = pci_mmconfig_lookup(seg, start);
> > +	if (cfg)
> > +		return 0;
> > +
> > +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> > +	if (!cfg)
> > +		return -ENOMEM;
> > +
> > +	err = pci_mmconfig_inject(cfg);
> > +	return err;
> >  }
> >  
> > -#ifdef CONFIG_ACPI
> > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> > +{
> > +	struct acpi_pci_root *root = ci->root;
> > +	struct pci_mmcfg_region *cfg;
> > +
> > +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> > +	if (cfg)
> > +		return;
> > +
> > +	if (cfg->hot_added)
> > +		pci_mmconfig_delete(root->segment, root->secondary.start,
> > +				    root->secondary.end);
> > +}
> > +#else
> > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > +{
> > +	return 0;
> > +}
> > +
> > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> > +#endif
> > +
> > +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> > +{
> > +	return pci_add_mmconfig_region(ci);
> > +}
> > +
> > +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> > +{
> > +	pci_remove_mmconfig_region(ci);
> > +	kfree(ci);
> > +}
> > +
> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > +{
> > +	struct resource_entry *entry, *tmp;
> > +	int ret;
> > +
> > +	ret = acpi_pci_probe_root_resources(ci);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> > +		struct resource *res = entry->res;
> > +
> > +		/*
> > +		 * Special handling for ARM IO range
> 
> There is nothing ARM specific here. It should apply to any memory mapped IO range.
> 
> > +		 * TODO: need to move pci_register_io_range() function out
> > +		 * of drivers/of/address.c for both used by DT and ACPI
> > +		 */
> > +		if (res->flags & IORESOURCE_IO) {
> > +			unsigned long port;
> > +			int err;
> > +			resource_size_t length = res->end - res->start;
> > +
> > +			err = pci_register_io_range(res->start, length);
> > +			if (err) {
> > +				resource_list_destroy_entry(entry);
> > +				continue;
> > +			}
> > +
> > +			port = pci_address_to_pio(res->start);
> > +			if (port == (unsigned long)-1) {
> > +				resource_list_destroy_entry(entry);
> > +				continue;
> > +			}
> > +
> > +			res->start = port;
> > +			res->end = res->start + length - 1;
> > +
> > +			if (pci_remap_iospace(res, res->start) < 0)
> > +				resource_list_destroy_entry(entry);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> > +	.pci_ops = &pci_root_ops,
> > +	.init_info = pci_acpi_root_init_info,
> > +	.release_info = pci_acpi_root_release_info,
> > +	.prepare_resources = pci_acpi_root_prepare_resources,
> > +};
> > +
> >  /* Root bridge scanning */
> >  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);
> > +	int domain = root->segment;
> > +	int busnum = root->secondary.start;
> > +	struct acpi_pci_root_info *info;
> > +	struct pci_bus *bus;
> > +
> > +	if (domain && !pci_domains_supported) {
> > +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> > +			domain, busnum);
> > +		return NULL;
> > +	}
> > +
> > +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> > +	if (!info) {
> > +		dev_err(&root->device->dev,
> > +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> > +			domain, busnum);
> > +		return NULL;
> > +	}
> > +
> > +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> > +
> > +	/* After the PCI-E bus has been walked and all devices discovered,
> > +	 * configure any settings of the fabric that might be necessary.
> > +	 */
> > +	if (bus) {
> > +		struct pci_bus *child;
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> > +	}
> > +
> > +	return bus;
> >  }
> >  #endif
> > -- 
> > 1.9.1
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Liviu.Dudau]

On Tue, Nov 03, 2015 at 02:32:14PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 11:49:40AM +0000, Liviu.Dudau@arm.com wrote:
> > On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
> 
> [...]
> 
> > > +static int __init pcibios_assign_resources(void)
> > > +{
> > > +	if (acpi_disabled)
> > > +		return 0;
> > > +
> > > +	pci_assign_unassigned_resources();
> > > +	return 0;
> > 
> > You can change this function into:
> > {
> > 	if (!acpi_disabled)
> > 		pci_assign_unassigned_resources();
> > 
> > 	return 0;
> > }
> > 
> > as the equivalent but shorter form.
> 
> I do not think it is a matter of code style here, it is a matter
> of understanding when and if we want to reassign resources on ACPI
> systems, it is an open question on ARM64 that must be sorted out (ie we
> ignore FW/BARs set-up entirely).

I was reviewing the code here, not doing any technical guidance, I'm
leaving that to you as you are more involved around ACPI.

> 
> > > +}
> > >  /*
> > > - * raw_pci_read/write - Platform-specific PCI config space access.
> > > + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> > > + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
> > >   */
> > > -int raw_pci_read(unsigned int domain, unsigned int bus,
> > > -		  unsigned int devfn, int reg, int len, u32 *val)
> > 
> > What happened with raw_pci_{read,write} ? Why do you remove them?
> > 
> > 
> > > +rootfs_initcall(pcibios_assign_resources);
> > 
> > Would you be so kind and explain to me why you need this initcall?
> > Can you not set the PCI_REASSIGN_ALL_RSRC flag before calling
> > pci_scan_root_bus() ?
> 
> On what basis ? BTW, PCI core code does not assign unassigned resources
> anyway even if that flag is set, so some policy has to be defined here.

I was thinking that ACPI code can do that if they seem to depend on the resources
being assigned during root bus scan. I was not implying that PCI core code enforces
that.

Best regards,
Liviu

> 
> Thanks,
> Lorenzo
> 
> > I haven't focused on ACPI before so I'm a bit hazy on the init order when
> > that is enabled. That being said, I don't like adding in the architecture
> > code initcall hooks just to fix up some dependency orders that could / should
> > be fixed some other way.
> > 
> > > +
> > > +static void __iomem *
> > > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> > >  {
> > > -	return -ENXIO;
> > > +	struct pci_mmcfg_region *cfg;
> > > +
> > > +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> > > +	if (cfg && cfg->virt)
> > > +		return cfg->virt +
> > > +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> > > +			offset;
> > > +	return NULL;
> > >  }
> > >  
> > > -int raw_pci_write(unsigned int domain, unsigned int bus,
> > > -		unsigned int devfn, int reg, int len, u32 val)
> > > +struct pci_ops pci_root_ops = {
> > > +	.map_bus = pci_mcfg_dev_base,
> > > +	.read = pci_generic_config_read,
> > > +	.write = pci_generic_config_write,
> > > +};
> > > +
> > > +#ifdef CONFIG_PCI_MMCONFIG
> > > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > >  {
> > > -	return -ENXIO;
> > > +	struct pci_mmcfg_region *cfg;
> > > +	struct acpi_pci_root *root;
> > > +	int seg, start, end, err;
> > > +
> > > +	root = ci->root;
> > > +	seg = root->segment;
> > > +	start = root->secondary.start;
> > > +	end = root->secondary.end;
> > > +
> > > +	cfg = pci_mmconfig_lookup(seg, start);
> > > +	if (cfg)
> > > +		return 0;
> > > +
> > > +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> > > +	if (!cfg)
> > > +		return -ENOMEM;
> > > +
> > > +	err = pci_mmconfig_inject(cfg);
> > > +	return err;
> > >  }
> > >  
> > > -#ifdef CONFIG_ACPI
> > > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> > > +{
> > > +	struct acpi_pci_root *root = ci->root;
> > > +	struct pci_mmcfg_region *cfg;
> > > +
> > > +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> > > +	if (cfg)
> > > +		return;
> > > +
> > > +	if (cfg->hot_added)
> > > +		pci_mmconfig_delete(root->segment, root->secondary.start,
> > > +				    root->secondary.end);
> > > +}
> > > +#else
> > > +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> > > +#endif
> > > +
> > > +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> > > +{
> > > +	return pci_add_mmconfig_region(ci);
> > > +}
> > > +
> > > +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> > > +{
> > > +	pci_remove_mmconfig_region(ci);
> > > +	kfree(ci);
> > > +}
> > > +
> > > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > > +{
> > > +	struct resource_entry *entry, *tmp;
> > > +	int ret;
> > > +
> > > +	ret = acpi_pci_probe_root_resources(ci);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> > > +		struct resource *res = entry->res;
> > > +
> > > +		/*
> > > +		 * Special handling for ARM IO range
> > 
> > There is nothing ARM specific here. It should apply to any memory mapped IO range.
> > 
> > > +		 * TODO: need to move pci_register_io_range() function out
> > > +		 * of drivers/of/address.c for both used by DT and ACPI
> > > +		 */
> > > +		if (res->flags & IORESOURCE_IO) {
> > > +			unsigned long port;
> > > +			int err;
> > > +			resource_size_t length = res->end - res->start;
> > > +
> > > +			err = pci_register_io_range(res->start, length);
> > > +			if (err) {
> > > +				resource_list_destroy_entry(entry);
> > > +				continue;
> > > +			}
> > > +
> > > +			port = pci_address_to_pio(res->start);
> > > +			if (port == (unsigned long)-1) {
> > > +				resource_list_destroy_entry(entry);
> > > +				continue;
> > > +			}
> > > +
> > > +			res->start = port;
> > > +			res->end = res->start + length - 1;
> > > +
> > > +			if (pci_remap_iospace(res, res->start) < 0)
> > > +				resource_list_destroy_entry(entry);
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> > > +	.pci_ops = &pci_root_ops,
> > > +	.init_info = pci_acpi_root_init_info,
> > > +	.release_info = pci_acpi_root_release_info,
> > > +	.prepare_resources = pci_acpi_root_prepare_resources,
> > > +};
> > > +
> > >  /* Root bridge scanning */
> > >  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);
> > > +	int domain = root->segment;
> > > +	int busnum = root->secondary.start;
> > > +	struct acpi_pci_root_info *info;
> > > +	struct pci_bus *bus;
> > > +
> > > +	if (domain && !pci_domains_supported) {
> > > +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> > > +			domain, busnum);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> > > +	if (!info) {
> > > +		dev_err(&root->device->dev,
> > > +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> > > +			domain, busnum);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> > > +
> > > +	/* After the PCI-E bus has been walked and all devices discovered,
> > > +	 * configure any settings of the fabric that might be necessary.
> > > +	 */
> > > +	if (bus) {
> > > +		struct pci_bus *child;
> > > +
> > > +		list_for_each_entry(child, &bus->children, node)
> > > +			pcie_bus_configure_settings(child);
> > > +	}
> > > +
> > > +	return bus;
> > >  }
> > >  #endif
> > > -- 
> > > 1.9.1
> > > 
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Sinan Kaya]

On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:
> Because of two patch series:
> 1. Jiang Liu's common interface to support PCI host bridge init
> 2. Refactoring of MMCONFIG, part of this patch set
> now we can think about PCI buses enumeration for ARM64 and ACPI tables.
>
> This patch introduce ACPI based PCI hostbridge init calls which
> use information from MCFG table (PCI config space regions) and
> _CRS (IO/irq resources) to initialize PCI hostbridge.
>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Liviu Dudau <Liviu.Dudau@arm.com>
> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> ---
>   arch/arm64/Kconfig      |   6 ++
>   arch/arm64/kernel/pci.c | 208 +++++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 202 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 07d1811..bbcc6b1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -89,6 +89,7 @@ config ARM64
>   	select SPARSE_IRQ
>   	select SYSCTL_EXCEPTION_TRACE
>   	select HAVE_CONTEXT_TRACKING
> +	select HAVE_PCI_ECAM
>   	help
>   	  ARM 64-bit (AArch64) Linux support.
>
> @@ -202,6 +203,11 @@ source "drivers/pci/Kconfig"
>   source "drivers/pci/pcie/Kconfig"
>   source "drivers/pci/hotplug/Kconfig"
>
> +config PCI_MMCONFIG
> +	def_bool y
> +	select PCI_ECAM
> +	depends on ACPI
> +
>   endmenu
>
>   menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>    */
>
>   #include <linux/acpi.h>
> +#include <linux/ecam.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
>   #include <linux/mm.h>
> +#include <linux/of_address.h>
>   #include <linux/of_pci.h>
>   #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>   #include <linux/slab.h>
>
>   #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>    */
>   int pcibios_add_device(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
> +		acpi_pci_irq_enable(dev);
> +#endif
>
>   	return 0;
>   }
>
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;
> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}
> +
> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;
> +}
>   /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>    */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)
> +rootfs_initcall(pcibios_assign_resources);
> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>   {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;
>   }
>
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,


Can you change these with pci_generic_config_read32 and 
pci_generic_config_write32? We have some targets that can only do 32 
bits PCI config space access.

> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>   {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>   }
>
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif
> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range
> +		 * TODO: need to move pci_register_io_range() function out
> +		 * of drivers/of/address.c for both used by DT and ACPI
> +		 */
> +		if (res->flags & IORESOURCE_IO) {
> +			unsigned long port;
> +			int err;
> +			resource_size_t length = res->end - res->start;
> +
> +			err = pci_register_io_range(res->start, length);
> +			if (err) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			port = pci_address_to_pio(res->start);
> +			if (port == (unsigned long)-1) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			res->start = port;
> +			res->end = res->start + length - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>   /* Root bridge scanning */
>   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);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;
>   }
>   #endif
>

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Lorenzo Pieralisi]

On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:

[...]

> >-int raw_pci_write(unsigned int domain, unsigned int bus,
> >-		unsigned int devfn, int reg, int len, u32 val)
> >+struct pci_ops pci_root_ops = {
> >+	.map_bus = pci_mcfg_dev_base,
> >+	.read = pci_generic_config_read,
> >+	.write = pci_generic_config_write,
> 
> 
> Can you change these with pci_generic_config_read32 and
> pci_generic_config_write32? We have some targets that can only do 32
> bits PCI config space access.

No.

http://www.spinics.net/lists/linux-pci/msg44869.html

Can you be a bit more specific please ?

Sigh. Looks like we have to start adding platform specific quirks even
before we merged the generic ACPI PCIe host controller implementation.

Lorenzo

> >+};
> >+
> >+#ifdef CONFIG_PCI_MMCONFIG
> >+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >  {
> >-	return -ENXIO;
> >+	struct pci_mmcfg_region *cfg;
> >+	struct acpi_pci_root *root;
> >+	int seg, start, end, err;
> >+
> >+	root = ci->root;
> >+	seg = root->segment;
> >+	start = root->secondary.start;
> >+	end = root->secondary.end;
> >+
> >+	cfg = pci_mmconfig_lookup(seg, start);
> >+	if (cfg)
> >+		return 0;
> >+
> >+	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> >+	if (!cfg)
> >+		return -ENOMEM;
> >+
> >+	err = pci_mmconfig_inject(cfg);
> >+	return err;
> >  }
> >
> >-#ifdef CONFIG_ACPI
> >+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> >+{
> >+	struct acpi_pci_root *root = ci->root;
> >+	struct pci_mmcfg_region *cfg;
> >+
> >+	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> >+	if (cfg)
> >+		return;
> >+
> >+	if (cfg->hot_added)
> >+		pci_mmconfig_delete(root->segment, root->secondary.start,
> >+				    root->secondary.end);
> >+}
> >+#else
> >+static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> >+{
> >+	return 0;
> >+}
> >+
> >+static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> >+#endif
> >+
> >+static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> >+{
> >+	return pci_add_mmconfig_region(ci);
> >+}
> >+
> >+static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> >+{
> >+	pci_remove_mmconfig_region(ci);
> >+	kfree(ci);
> >+}
> >+
> >+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> >+{
> >+	struct resource_entry *entry, *tmp;
> >+	int ret;
> >+
> >+	ret = acpi_pci_probe_root_resources(ci);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> >+		struct resource *res = entry->res;
> >+
> >+		/*
> >+		 * Special handling for ARM IO range
> >+		 * TODO: need to move pci_register_io_range() function out
> >+		 * of drivers/of/address.c for both used by DT and ACPI
> >+		 */
> >+		if (res->flags & IORESOURCE_IO) {
> >+			unsigned long port;
> >+			int err;
> >+			resource_size_t length = res->end - res->start;
> >+
> >+			err = pci_register_io_range(res->start, length);
> >+			if (err) {
> >+				resource_list_destroy_entry(entry);
> >+				continue;
> >+			}
> >+
> >+			port = pci_address_to_pio(res->start);
> >+			if (port == (unsigned long)-1) {
> >+				resource_list_destroy_entry(entry);
> >+				continue;
> >+			}
> >+
> >+			res->start = port;
> >+			res->end = res->start + length - 1;
> >+
> >+			if (pci_remap_iospace(res, res->start) < 0)
> >+				resource_list_destroy_entry(entry);
> >+		}
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static struct acpi_pci_root_ops acpi_pci_root_ops = {
> >+	.pci_ops = &pci_root_ops,
> >+	.init_info = pci_acpi_root_init_info,
> >+	.release_info = pci_acpi_root_release_info,
> >+	.prepare_resources = pci_acpi_root_prepare_resources,
> >+};
> >+
> >  /* Root bridge scanning */
> >  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);
> >+	int domain = root->segment;
> >+	int busnum = root->secondary.start;
> >+	struct acpi_pci_root_info *info;
> >+	struct pci_bus *bus;
> >+
> >+	if (domain && !pci_domains_supported) {
> >+		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> >+			domain, busnum);
> >+		return NULL;
> >+	}
> >+
> >+	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> >+	if (!info) {
> >+		dev_err(&root->device->dev,
> >+			"pci_bus %04x:%02x: ignored (out of memory)\n",
> >+			domain, busnum);
> >+		return NULL;
> >+	}
> >+
> >+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> >+
> >+	/* After the PCI-E bus has been walked and all devices discovered,
> >+	 * configure any settings of the fabric that might be necessary.
> >+	 */
> >+	if (bus) {
> >+		struct pci_bus *child;
> >+
> >+		list_for_each_entry(child, &bus->children, node)
> >+			pcie_bus_configure_settings(child);
> >+	}
> >+
> >+	return bus;
> >  }
> >  #endif
> >
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Tomasz Nowicki]

On 03.11.2015 15:15, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>
> [...]
>
>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>> -		unsigned int devfn, int reg, int len, u32 val)
>>> +struct pci_ops pci_root_ops = {
>>> +	.map_bus = pci_mcfg_dev_base,
>>> +	.read = pci_generic_config_read,
>>> +	.write = pci_generic_config_write,
>>
>>
>> Can you change these with pci_generic_config_read32 and
>> pci_generic_config_write32? We have some targets that can only do 32
>> bits PCI config space access.
>
> No.
>
> http://www.spinics.net/lists/linux-pci/msg44869.html
>
> Can you be a bit more specific please ?
>
> Sigh. Looks like we have to start adding platform specific quirks even
> before we merged the generic ACPI PCIe host controller implementation.
>

The sad reality... But my next version will be still generic. Once that 
one appear to be in good shape then we can add quirks.

Regards,
Tomasz

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Sinan Kaya]

On 11/3/2015 9:39 AM, Tomasz Nowicki wrote:
>>>> +struct pci_ops pci_root_ops = {
>>>> +    .map_bus = pci_mcfg_dev_base,
>>>> +    .read = pci_generic_config_read,
>>>> +    .write = pci_generic_config_write,
>>>
>>>
>>> Can you change these with pci_generic_config_read32 and
>>> pci_generic_config_write32? We have some targets that can only do 32
>>> bits PCI config space access.
>>
>> No.
>>
>> http://www.spinics.net/lists/linux-pci/msg44869.html
>>
>> Can you be a bit more specific please ?
>>
>> Sigh. Looks like we have to start adding platform specific quirks even
>> before we merged the generic ACPI PCIe host controller implementation.
>>
>
> The sad reality... But my next version will be still generic. Once that
> one appear to be in good shape then we can add quirks.

Thanks.

I don't see anywhere in the SBSA spec addendum that the PCI 
configuration space section that unaligned accesses *MUST* be supported.

If this is required, please have this info added to the spec. I can work 
with the designers for the next chip.

Unaligned access on the current hardware returns incomplete values or 
can cause bus faults. The behavior is undefined.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Arnd Bergmann]

On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
> 
> I don't see anywhere in the SBSA spec addendum that the PCI 
> configuration space section that unaligned accesses *MUST* be supported.
> 
> If this is required, please have this info added to the spec. I can work 
> with the designers for the next chip.
> 
> Unaligned access on the current hardware returns incomplete values or 
> can cause bus faults. The behavior is undefined.

Unaligned accesses are not allowed, but any PCI compliant device must
support aligned 1, 2 or 4 byte accesses on its configuration space,
though the byte-enable mechanism. In an ECAM host bridge, those are
mapped to load/store accesses from the CPU with the respective width
and natural alignment.

	Arnd

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Sinan Kaya]

On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
> On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
>>
>> I don't see anywhere in the SBSA spec addendum that the PCI
>> configuration space section that unaligned accesses *MUST* be supported.
>>
>> If this is required, please have this info added to the spec. I can work
>> with the designers for the next chip.
>>
>> Unaligned access on the current hardware returns incomplete values or
>> can cause bus faults. The behavior is undefined.
>
> Unaligned accesses are not allowed, but any PCI compliant device must
> support aligned 1, 2 or 4 byte accesses on its configuration space,
> though the byte-enable mechanism. In an ECAM host bridge, those are
> mapped to load/store accesses from the CPU with the respective width
> and natural alignment.
>
> 	Arnd
>

As far as I see, the endpoints do not have any problems with unaligned 
accesses. It is the host bridge itself (stuff that doesn't get on the 
PCIe bus and uses traditional AXI kind bus internally) has problems with 
alignment.

If Linux is expecting all HW vendors to implement alignment support, 
this needs to be put in the SBSA spec as a hard requirement.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Arnd Bergmann]

On Tuesday 03 November 2015 11:33:18 Sinan Kaya wrote:
> 
> On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
> > On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
> >>
> >> I don't see anywhere in the SBSA spec addendum that the PCI
> >> configuration space section that unaligned accesses *MUST* be supported.
> >>
> >> If this is required, please have this info added to the spec. I can work
> >> with the designers for the next chip.
> >>
> >> Unaligned access on the current hardware returns incomplete values or
> >> can cause bus faults. The behavior is undefined.
> >
> > Unaligned accesses are not allowed, but any PCI compliant device must
> > support aligned 1, 2 or 4 byte accesses on its configuration space,
> > though the byte-enable mechanism. In an ECAM host bridge, those are
> > mapped to load/store accesses from the CPU with the respective width
> > and natural alignment.
> 
> As far as I see, the endpoints do not have any problems with unaligned 
> accesses. It is the host bridge itself (stuff that doesn't get on the 
> PCIe bus and uses traditional AXI kind bus internally) has problems with 
> alignment.
> 
> If Linux is expecting all HW vendors to implement alignment support, 
> this needs to be put in the SBSA spec as a hard requirement.

As I said, it's not unaligned accesses at all, just 1-byte and aligned
2-byte accesses, and it's not Linux mandating this but the PCI
spec. Please read Russell's email again, it is not possible for PCI
to work according to the specification unless the host bridge allows
sub-32-bit accesses.

You can probably work around this by using the legacy I/O port method
rather than ECAM, if the PCI host bridge itself is functional and just
the host bus it is connected to is buggy.

	Arnd

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Sinan Kaya]

On 11/3/2015 11:55 AM, Arnd Bergmann wrote:
> On Tuesday 03 November 2015 11:33:18 Sinan Kaya wrote:
>>
>> On 11/3/2015 10:59 AM, Arnd Bergmann wrote:
>>> On Tuesday 03 November 2015 10:10:21 Sinan Kaya wrote:
>>>>
>>>> I don't see anywhere in the SBSA spec addendum that the PCI
>>>> configuration space section that unaligned accesses *MUST* be supported.
>>>>
>>>> If this is required, please have this info added to the spec. I can work
>>>> with the designers for the next chip.
>>>>
>>>> Unaligned access on the current hardware returns incomplete values or
>>>> can cause bus faults. The behavior is undefined.
>>>
>>> Unaligned accesses are not allowed, but any PCI compliant device must
>>> support aligned 1, 2 or 4 byte accesses on its configuration space,
>>> though the byte-enable mechanism. In an ECAM host bridge, those are
>>> mapped to load/store accesses from the CPU with the respective width
>>> and natural alignment.
>>
>> As far as I see, the endpoints do not have any problems with unaligned
>> accesses. It is the host bridge itself (stuff that doesn't get on the
>> PCIe bus and uses traditional AXI kind bus internally) has problems with
>> alignment.
>>
>> If Linux is expecting all HW vendors to implement alignment support,
>> this needs to be put in the SBSA spec as a hard requirement.
>
> As I said, it's not unaligned accesses at all, just 1-byte and aligned
> 2-byte accesses, and it's not Linux mandating this but the PCI
> spec. Please read Russell's email again, it is not possible for PCI
> to work according to the specification unless the host bridge allows
> sub-32-bit accesses.

I'll check back with the hardware designers. Seeing readb/readw/readl 
made me nervous that we are trying unaligned access from any boundaries.

In any case, the hardware document says 32 bit configuration space 
access to the host bridge only. I'll get more clarification.

>
> You can probably work around this by using the legacy I/O port method
> rather than ECAM, if the PCI host bridge itself is functional and just
> the host bus it is connected to is buggy.

 From the sounds of it, we'll need a quirk for config space. We support 
legacy I/O only to make the endpoints happy. Some endpoints do not get 
initialized if they don't have a BAR address assigned to all the BAR 
resources.

I just saw David Daney's email. I like his idea. I think this chip will 
fit into the same category.

>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [Linaro-acpi] [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Sinan Kaya]

On 11/3/2015 12:43 PM, Sinan Kaya wrote:
> In any case, the hardware document says 32 bit configuration space
> access to the host bridge only. I'll get more clarification.
>
I got confirmation this morning that this chip supports 32 bit access to 
the root complex configuration space. 8/16/32 bits accesses to the 
endpoints are supported.

>>
>> You can probably work around this by using the legacy I/O port method
>> rather than ECAM, if the PCI host bridge itself is functional and just
>> the host bus it is connected to is buggy.
>
>  From the sounds of it, we'll need a quirk for config space. We support
> legacy I/O only to make the endpoints happy. Some endpoints do not get
> initialized if they don't have a BAR address assigned to all the BAR
> resources.

We'll need an MCFG fix up.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Hanjun Guo]

On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>
> [...]
>
>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>> -		unsigned int devfn, int reg, int len, u32 val)
>>> +struct pci_ops pci_root_ops = {
>>> +	.map_bus = pci_mcfg_dev_base,
>>> +	.read = pci_generic_config_read,
>>> +	.write = pci_generic_config_write,
>>
>>
>> Can you change these with pci_generic_config_read32 and
>> pci_generic_config_write32? We have some targets that can only do 32
>> bits PCI config space access.
>
> No.
>
> http://www.spinics.net/lists/linux-pci/msg44869.html
>
> Can you be a bit more specific please ?
>
> Sigh. Looks like we have to start adding platform specific quirks even
> before we merged the generic ACPI PCIe host controller implementation.

Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
support.

I think so, some platform may not support ECAM for root complex,
which needs special handling of access config space, we may need
to consider those cases.

Thanks
Hanjun

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[David Daney]

On 11/03/2015 07:19 AM, Hanjun Guo wrote:
> On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
>> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
>>
>> [...]
>>
>>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>>>> -        unsigned int devfn, int reg, int len, u32 val)
>>>> +struct pci_ops pci_root_ops = {
>>>> +    .map_bus = pci_mcfg_dev_base,
>>>> +    .read = pci_generic_config_read,
>>>> +    .write = pci_generic_config_write,
>>>
>>>
>>> Can you change these with pci_generic_config_read32 and
>>> pci_generic_config_write32? We have some targets that can only do 32
>>> bits PCI config space access.
>>
>> No.
>>
>> http://www.spinics.net/lists/linux-pci/msg44869.html
>>
>> Can you be a bit more specific please ?
>>
>> Sigh. Looks like we have to start adding platform specific quirks even
>> before we merged the generic ACPI PCIe host controller implementation.
>
> Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
> support.
>
> I think so, some platform may not support ECAM for root complex,
> which needs special handling of access config space, we may need
> to consider those cases.
>

Yes, it is indeed true.  For example, some Cavium ThunderX processors 
fall into this category.

Some options I thought of are:

  o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed 
config space accessors.

  o Define additional root_device_ids that imply the needed config space 
accessors.


> Thanks
> Hanjun

RE: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Gabriele Paoloni]

Hi David

> -----Original Message-----
> From: David Daney [mailto:ddaney@caviumnetworks.com]
> Sent: 03 November 2015 17:39
> To: Hanjun Guo
> Cc: Lorenzo Pieralisi; Sinan Kaya; Tomasz Nowicki; bhelgaas@google.com;
> arnd@arndb.de; will.deacon@arm.com; catalin.marinas@arm.com; rjw@rjwysocki.net;
> jiang.liu@linux.intel.com; robert.richter@caviumnetworks.com;
> Narinder.Dhillon@caviumnetworks.com; Liviu.Dudau@arm.com; tglx@linutronix.de;
> Wangyijing; Suravee.Suthikulpanit@amd.com; msalter@redhat.com; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; Gabriele Paoloni; Wangzhou
> (B); liudongdong (C)
> Subject: Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI
> hostbridge init
> 
> On 11/03/2015 07:19 AM, Hanjun Guo wrote:
> > On 11/03/2015 10:15 PM, Lorenzo Pieralisi wrote:
> >> On Wed, Oct 28, 2015 at 02:46:37PM -0400, Sinan Kaya wrote:
> >>
> >> [...]
> >>
> >>>> -int raw_pci_write(unsigned int domain, unsigned int bus,
> >>>> -        unsigned int devfn, int reg, int len, u32 val)
> >>>> +struct pci_ops pci_root_ops = {
> >>>> +    .map_bus = pci_mcfg_dev_base,
> >>>> +    .read = pci_generic_config_read,
> >>>> +    .write = pci_generic_config_write,
> >>>
> >>>
> >>> Can you change these with pci_generic_config_read32 and
> >>> pci_generic_config_write32? We have some targets that can only do 32
> >>> bits PCI config space access.
> >>
> >> No.
> >>
> >> http://www.spinics.net/lists/linux-pci/msg44869.html
> >>
> >> Can you be a bit more specific please ?
> >>
> >> Sigh. Looks like we have to start adding platform specific quirks even
> >> before we merged the generic ACPI PCIe host controller implementation.
> >
> > Cc Gab, Zhou, and Dondong who upstream the hip05 (designware) PCIe host
> > support.
> >
> > I think so, some platform may not support ECAM for root complex,
> > which needs special handling of access config space, we may need
> > to consider those cases.
> >
> 
> Yes, it is indeed true.  For example, some Cavium ThunderX processors
> fall into this category.
> 
> Some options I thought of are:
> 
>   o Use DECLARE_ACPI_MCFG_FIXUP() in the kernel to supply the needed
> config space accessors.
> 
>   o Define additional root_device_ids that imply the needed config space
> accessors.

Yes I like this

it would fit designware too

Gab

> 
> 
> > Thanks
> > Hanjun

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Lorenzo Pieralisi]

On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:

[...]

>  menu "Kernel Features"
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index b3d098b..66cc1ae 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -11,12 +11,15 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/ecam.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> +#include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/slab.h>
>  
>  #include <asm/pci-bridge.h>
> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>   */
>  int pcibios_add_device(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
> +		acpi_pci_irq_enable(dev);
> +#endif

This series:

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

will allow us to initialize the irq mapping function according to
the boot method, code above is getting cumbersome and it is already
overriden when booting with DT, so we will remove it altogether.

>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct acpi_pci_root *root = bridge->bus->sysdata;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
> +	return 0;

This should be made part of core code IMO.

> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}

Two functions above are identical for arm64, ia64 and x86, I do
not think they belong in arch code.

> +static int __init pcibios_assign_resources(void)
> +{
> +	if (acpi_disabled)
> +		return 0;
> +
> +	pci_assign_unassigned_resources();
> +	return 0;

Already commented on this.

> +}
>  /*
> - * raw_pci_read/write - Platform-specific PCI config space access.
> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>   */
> -int raw_pci_read(unsigned int domain, unsigned int bus,
> -		  unsigned int devfn, int reg, int len, u32 *val)
> +rootfs_initcall(pcibios_assign_resources);
> +
> +static void __iomem *
> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> +	if (cfg && cfg->virt)
> +		return cfg->virt +
> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> +			offset;
> +	return NULL;

Why is this code arm64 specific ?

>  }
>  
> -int raw_pci_write(unsigned int domain, unsigned int bus,
> -		unsigned int devfn, int reg, int len, u32 val)
> +struct pci_ops pci_root_ops = {
> +	.map_bus = pci_mcfg_dev_base,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
> +#ifdef CONFIG_PCI_MMCONFIG
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>  {
> -	return -ENXIO;
> +	struct pci_mmcfg_region *cfg;
> +	struct acpi_pci_root *root;
> +	int seg, start, end, err;
> +
> +	root = ci->root;
> +	seg = root->segment;
> +	start = root->secondary.start;
> +	end = root->secondary.end;
> +
> +	cfg = pci_mmconfig_lookup(seg, start);
> +	if (cfg)
> +		return 0;
> +
> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
> +	if (!cfg)
> +		return -ENOMEM;
> +
> +	err = pci_mmconfig_inject(cfg);
> +	return err;
>  }
>  
> -#ifdef CONFIG_ACPI
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_root *root = ci->root;
> +	struct pci_mmcfg_region *cfg;
> +
> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +	if (cfg)
> +		return;
> +
> +	if (cfg->hot_added)
> +		pci_mmconfig_delete(root->segment, root->secondary.start,
> +				    root->secondary.end);
> +}
> +#else
> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
> +{
> +	return 0;
> +}
> +
> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
> +#endif

Ditto.

> +
> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
> +{
> +	return pci_add_mmconfig_region(ci);
> +}
> +
> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
> +{
> +	pci_remove_mmconfig_region(ci);
> +	kfree(ci);
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = acpi_pci_probe_root_resources(ci);
> +	if (ret < 0)
> +		return ret;

Code above is identical on arm64, ia64 and x86.

> +
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		struct resource *res = entry->res;
> +
> +		/*
> +		 * Special handling for ARM IO range
> +		 * TODO: need to move pci_register_io_range() function out
> +		 * of drivers/of/address.c for both used by DT and ACPI
> +		 */
> +		if (res->flags & IORESOURCE_IO) {
> +			unsigned long port;
> +			int err;
> +			resource_size_t length = res->end - res->start;
> +
> +			err = pci_register_io_range(res->start, length);
> +			if (err) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			port = pci_address_to_pio(res->start);
> +			if (port == (unsigned long)-1) {
> +				resource_list_destroy_entry(entry);
> +				continue;
> +			}
> +
> +			res->start = port;
> +			res->end = res->start + length - 1;
> +
> +			if (pci_remap_iospace(res, res->start) < 0)
> +				resource_list_destroy_entry(entry);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +	.pci_ops = &pci_root_ops,
> +	.init_info = pci_acpi_root_init_info,
> +	.release_info = pci_acpi_root_release_info,
> +	.prepare_resources = pci_acpi_root_prepare_resources,
> +};
> +
>  /* Root bridge scanning */
>  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);
> +	int domain = root->segment;
> +	int busnum = root->secondary.start;
> +	struct acpi_pci_root_info *info;
> +	struct pci_bus *bus;
> +
> +	if (domain && !pci_domains_supported) {
> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
> +	if (!info) {
> +		dev_err(&root->device->dev,
> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
> +			domain, busnum);
> +		return NULL;
> +	}
> +
> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> +
> +	/* After the PCI-E bus has been walked and all devices discovered,
> +	 * configure any settings of the fabric that might be necessary.
> +	 */
> +	if (bus) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	return bus;

Code above is entirely arch agnostic (apart from what data is passed to
sysdata) and I think there is room for further consolidation with
x86 and ia64, I will have a look into this.

Thanks,
Lorenzo

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Tomasz Nowicki]

On 03.11.2015 17:55, Lorenzo Pieralisi wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(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
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>
> This series:
>
> http://www.spinics.net/lists/linux-pci/msg45950.html
>
> will allow us to initialize the irq mapping function according to
> the boot method, code above is getting cumbersome and it is already
> overriden when booting with DT, so we will remove it altogether.
>
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>
> This should be made part of core code IMO.
>
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>
> Two functions above are identical for arm64, ia64 and x86, I do
> not think they belong in arch code.
>
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> Already commented on this.
>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>> +rootfs_initcall(pcibios_assign_resources);
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>
> Why is this code arm64 specific ?
>
>>   }
>>
>> -int raw_pci_write(unsigned int domain, unsigned int bus,
>> -		unsigned int devfn, int reg, int len, u32 val)
>> +struct pci_ops pci_root_ops = {
>> +	.map_bus = pci_mcfg_dev_base,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>> +#ifdef CONFIG_PCI_MMCONFIG
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +	struct acpi_pci_root *root;
>> +	int seg, start, end, err;
>> +
>> +	root = ci->root;
>> +	seg = root->segment;
>> +	start = root->secondary.start;
>> +	end = root->secondary.end;
>> +
>> +	cfg = pci_mmconfig_lookup(seg, start);
>> +	if (cfg)
>> +		return 0;
>> +
>> +	cfg = pci_mmconfig_alloc(seg, start, end, root->mcfg_addr);
>> +	if (!cfg)
>> +		return -ENOMEM;
>> +
>> +	err = pci_mmconfig_inject(cfg);
>> +	return err;
>>   }
>>
>> -#ifdef CONFIG_ACPI
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	struct acpi_pci_root *root = ci->root;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +	if (cfg)
>> +		return;
>> +
>> +	if (cfg->hot_added)
>> +		pci_mmconfig_delete(root->segment, root->secondary.start,
>> +				    root->secondary.end);
>> +}
>> +#else
>> +static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void pci_remove_mmconfig_region(struct acpi_pci_root_info *ci) { }
>> +#endif
>
> Ditto.
>
>> +
>> +static int pci_acpi_root_init_info(struct acpi_pci_root_info *ci)
>> +{
>> +	return pci_add_mmconfig_region(ci);
>> +}
>> +
>> +static void pci_acpi_root_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +	pci_remove_mmconfig_region(ci);
>> +	kfree(ci);
>> +}
>> +
>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> +{
>> +	struct resource_entry *entry, *tmp;
>> +	int ret;
>> +
>> +	ret = acpi_pci_probe_root_resources(ci);
>> +	if (ret < 0)
>> +		return ret;
>
> Code above is identical on arm64, ia64 and x86.
>
>> +
>> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
>> +		struct resource *res = entry->res;
>> +
>> +		/*
>> +		 * Special handling for ARM IO range
>> +		 * TODO: need to move pci_register_io_range() function out
>> +		 * of drivers/of/address.c for both used by DT and ACPI
>> +		 */
>> +		if (res->flags & IORESOURCE_IO) {
>> +			unsigned long port;
>> +			int err;
>> +			resource_size_t length = res->end - res->start;
>> +
>> +			err = pci_register_io_range(res->start, length);
>> +			if (err) {
>> +				resource_list_destroy_entry(entry);
>> +				continue;
>> +			}
>> +
>> +			port = pci_address_to_pio(res->start);
>> +			if (port == (unsigned long)-1) {
>> +				resource_list_destroy_entry(entry);
>> +				continue;
>> +			}
>> +
>> +			res->start = port;
>> +			res->end = res->start + length - 1;
>> +
>> +			if (pci_remap_iospace(res, res->start) < 0)
>> +				resource_list_destroy_entry(entry);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +	.pci_ops = &pci_root_ops,
>> +	.init_info = pci_acpi_root_init_info,
>> +	.release_info = pci_acpi_root_release_info,
>> +	.prepare_resources = pci_acpi_root_prepare_resources,
>> +};
>> +
>>   /* Root bridge scanning */
>>   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);
>> +	int domain = root->segment;
>> +	int busnum = root->secondary.start;
>> +	struct acpi_pci_root_info *info;
>> +	struct pci_bus *bus;
>> +
>> +	if (domain && !pci_domains_supported) {
>> +		pr_warn("PCI %04x:%02x: multiple domains not supported.\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	info = kzalloc_node(sizeof(*info), GFP_KERNEL, node);
>> +	if (!info) {
>> +		dev_err(&root->device->dev,
>> +			"pci_bus %04x:%02x: ignored (out of memory)\n",
>> +			domain, busnum);
>> +		return NULL;
>> +	}
>> +
>> +	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>> +
>> +	/* After the PCI-E bus has been walked and all devices discovered,
>> +	 * configure any settings of the fabric that might be necessary.
>> +	 */
>> +	if (bus) {
>> +		struct pci_bus *child;
>> +
>> +		list_for_each_entry(child, &bus->children, node)
>> +			pcie_bus_configure_settings(child);
>> +	}
>> +
>> +	return bus;
>
> Code above is entirely arch agnostic (apart from what data is passed to
> sysdata) and I think there is room for further consolidation with
> x86 and ia64, I will have a look into this.
>

I agree with your comments, currently I do not know how much of it can 
be consolidated but I will rework my next version in this direction.

Thanks,
Tomasz

Re: [PATCH V1 11/11] arm64, pci, acpi: Support for ACPI based PCI hostbridge init

[Tomasz Nowicki]

On 03.11.2015 17:55, Lorenzo Pieralisi wrote:
> On Tue, Oct 27, 2015 at 05:38:42PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>>   menu "Kernel Features"
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index b3d098b..66cc1ae 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -11,12 +11,15 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/ecam.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mm.h>
>> +#include <linux/of_address.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/slab.h>
>>
>>   #include <asm/pci-bridge.h>
>> @@ -52,35 +55,216 @@ 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 from DT/ACPI when adding a new device
>>    */
>>   int pcibios_add_device(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
>> +		acpi_pci_irq_enable(dev);
>> +#endif
>
> This series:
>
> http://www.spinics.net/lists/linux-pci/msg45950.html
>
> will allow us to initialize the irq mapping function according to
> the boot method, code above is getting cumbersome and it is already
> overriden when booting with DT, so we will remove it altogether.
>
>>
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct acpi_pci_root *root = bridge->bus->sysdata;
>> +
>> +	ACPI_COMPANION_SET(&bridge->dev, root->device);
>> +	return 0;
>
> This should be made part of core code IMO.
>
>> +}
>> +
>> +void pcibios_add_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_add_bus(bus);
>> +}
>> +
>> +void pcibios_remove_bus(struct pci_bus *bus)
>> +{
>> +	acpi_pci_remove_bus(bus);
>> +}
>
> Two functions above are identical for arm64, ia64 and x86, I do
> not think they belong in arch code.
>
>> +static int __init pcibios_assign_resources(void)
>> +{
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	pci_assign_unassigned_resources();
>> +	return 0;
>
> Already commented on this.
>
>> +}
>>   /*
>> - * raw_pci_read/write - Platform-specific PCI config space access.
>> + * rootfs_initcall comes after subsys_initcall and fs_initcall_sync,
>> + * so we know acpi scan and PCI_FIXUP_FINAL quirks have both run.
>>    */
>> -int raw_pci_read(unsigned int domain, unsigned int bus,
>> -		  unsigned int devfn, int reg, int len, u32 *val)
>> +rootfs_initcall(pcibios_assign_resources);
>> +
>> +static void __iomem *
>> +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
>>   {
>> -	return -ENXIO;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
>> +	if (cfg && cfg->virt)
>> +		return cfg->virt +
>> +			(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
>> +			offset;
>> +	return NULL;
>
> Why is this code arm64 specific ?

It is not, I will move it out of here, probably to mcfg.c file where we 
can apply quirks.

Thanks,
Tomasz

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Jon Masters]

Hi Tomasz,

Thanks for posting this series.

On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:

> From the functionality point of view this series might be split into two logic parts:
> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>    PCI config regions and used when necessary.
> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>    initialization for ARM64

Can I confirm that the intention here is that this replaces Hanjun's
previous patch series? Here's the previous one we were tracking:

https://lkml.kernel.org/g/1432644564-24746-1-git-send-email-hanjun.guo@linaro.org

Assuming that is the case, then we will ping a number of folks to
conduct testing and provide acks (this has already been discussed in a
number of conversations with semiconductors over the past few days).

Thanks,

Jon.

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Hanjun Guo]

Hi Jon,

On 2015/10/30 12:07, Jon Masters wrote:
> Hi Tomasz,
>
> Thanks for posting this series.
>
> On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:
>
>> From the functionality point of view this series might be split into two logic parts:
>> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>>    PCI config regions and used when necessary.
>> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>>    initialization for ARM64
> Can I confirm that the intention here is that this replaces Hanjun's
> previous patch series? Here's the previous one we were tracking:
>
> https://lkml.kernel.org/g/1432644564-24746-1-git-send-email-hanjun.guo@linaro.org

Yes, that's the new version according to the discussion from my version.

>
> Assuming that is the case, then we will ping a number of folks to
> conduct testing and provide acks (this has already been discussed in a
> number of conversations with semiconductors over the past few days).

Much appreciated.

Thanks
Hanjun

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Tomasz Nowicki]

On 10/30/2015 05:07 AM, Jon Masters wrote:
> Hi Tomasz,
>
> Thanks for posting this series.
>
> On 10/27/2015 12:38 PM, Tomasz Nowicki wrote:
>
>>  From the functionality point of view this series might be split into two logic parts:
>> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>>     PCI config regions and used when necessary.
>> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>>     initialization for ARM64
> Can I confirm that the intention here is that this replaces Hanjun's
> previous patch series? Here's the previous one we were tracking:
>
> https://lkml.kernel.org/g/1432644564-24746-1-git-send-email-hanjun.guo@linaro.org
Indeed, we can call it next version.
>
> Assuming that is the case, then we will ping a number of folks to
> conduct testing and provide acks (this has already been discussed in a
> number of conversations with semiconductors over the past few days).
Yes, please, thanks!

Tomasz

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Suravee Suthikulpanit]

Tested on AMD Seattle platform (Overdrive revB) w/ both MSI and legacy 
interrupt.

Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Thanks,
Suravee

On 10/27/15 11:38, Tomasz Nowicki wrote:
>  From the functionality point of view this series might be split into two logic parts:
> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>     PCI config regions and used when necessary.
> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>     initialization for ARM64
>
> Patches has been built on top of:
> [Patch v7 0/7] Consolidate ACPI PCI root common code into ACPI core
> https://lkml.org/lkml/2015/10/14/31
> Git branch can be found here:
> https://git.linaro.org/leg/acpi/acpi.git/shortlog/refs/heads/pci-acpi-upstream
>
> This has been tested on Cavium ThunderX 1 socket server.
> Any help in reviewing and testing is very appreciated.
>
> Hanjun Guo (1):
>    XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y
>
> Tomasz Nowicki (10):
>    x86, pci: Reorder logic of pci_mmconfig_insert() function
>    x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
>      out of arch/x86/ directory
>    pci, acpi, mcfg: Provide generic implementation of MCFG code
>      initialization.
>    x86, pci: mmconfig_{32,64}.c code refactoring - remove code
>      duplication.
>    x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
>      driver.
>    pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
>    pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
>      added or not.
>    x86, pci: Use previously added ECAM hot_added flag to remove ECAM
>      regions.
>    pci, acpi: Provide generic way to assign bus domain number.
>    arm64, pci, acpi: Support for ACPI based PCI hostbridge init
>
>   arch/arm64/Kconfig             |   6 +
>   arch/arm64/kernel/pci.c        | 208 ++++++++++++++++++++++++++++++++--
>   arch/x86/Kconfig               |   4 +
>   arch/x86/include/asm/pci_x86.h |  28 +----
>   arch/x86/pci/acpi.c            |  17 +--
>   arch/x86/pci/mmconfig-shared.c | 250 +++++++----------------------------------
>   arch/x86/pci/mmconfig_32.c     |  11 +-
>   arch/x86/pci/mmconfig_64.c     |  67 +----------
>   arch/x86/pci/numachip.c        |   1 +
>   drivers/acpi/Makefile          |   1 +
>   drivers/acpi/mcfg.c            | 104 +++++++++++++++++
>   drivers/acpi/pci_root.c        |   2 +-
>   drivers/pci/Kconfig            |  10 ++
>   drivers/pci/Makefile           |   5 +
>   drivers/pci/ecam.c             | 234 ++++++++++++++++++++++++++++++++++++++
>   drivers/pci/pci.c              |  30 ++++-
>   drivers/xen/pci.c              |   7 +-
>   include/linux/acpi.h           |   2 +
>   include/linux/ecam.h           |  44 ++++++++
>   19 files changed, 691 insertions(+), 340 deletions(-)
>   create mode 100644 drivers/acpi/mcfg.c
>   create mode 100644 drivers/pci/ecam.c
>   create mode 100644 include/linux/ecam.h
>

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Bjorn Helgaas]

On Tue, Oct 27, 2015 at 05:38:31PM +0100, Tomasz Nowicki wrote:
> From the functionality point of view this series might be split into two logic parts:
> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>    PCI config regions and used when necessary.
> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>    initialization for ARM64
> 
> Patches has been built on top of:
> [Patch v7 0/7] Consolidate ACPI PCI root common code into ACPI core
> https://lkml.org/lkml/2015/10/14/31
> Git branch can be found here:
> https://git.linaro.org/leg/acpi/acpi.git/shortlog/refs/heads/pci-acpi-upstream
> 
> This has been tested on Cavium ThunderX 1 socket server.
> Any help in reviewing and testing is very appreciated.
> 
> Hanjun Guo (1):
>   XEN / PCI: Remove the dependence on arch x86 when PCI_MMCONFIG=y
> 
> Tomasz Nowicki (10):
>   x86, pci: Reorder logic of pci_mmconfig_insert() function
>   x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
>     out of arch/x86/ directory
>   pci, acpi, mcfg: Provide generic implementation of MCFG code
>     initialization.
>   x86, pci: mmconfig_{32,64}.c code refactoring - remove code
>     duplication.
>   x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
>     driver.
>   pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
>   pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
>     added or not.
>   x86, pci: Use previously added ECAM hot_added flag to remove ECAM
>     regions.
>   pci, acpi: Provide generic way to assign bus domain number.
>   arm64, pci, acpi: Support for ACPI based PCI hostbridge init
> 
>  arch/arm64/Kconfig             |   6 +
>  arch/arm64/kernel/pci.c        | 208 ++++++++++++++++++++++++++++++++--
>  arch/x86/Kconfig               |   4 +
>  arch/x86/include/asm/pci_x86.h |  28 +----
>  arch/x86/pci/acpi.c            |  17 +--
>  arch/x86/pci/mmconfig-shared.c | 250 +++++++----------------------------------
>  arch/x86/pci/mmconfig_32.c     |  11 +-
>  arch/x86/pci/mmconfig_64.c     |  67 +----------
>  arch/x86/pci/numachip.c        |   1 +
>  drivers/acpi/Makefile          |   1 +
>  drivers/acpi/mcfg.c            | 104 +++++++++++++++++
>  drivers/acpi/pci_root.c        |   2 +-
>  drivers/pci/Kconfig            |  10 ++
>  drivers/pci/Makefile           |   5 +
>  drivers/pci/ecam.c             | 234 ++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c              |  30 ++++-
>  drivers/xen/pci.c              |   7 +-
>  include/linux/acpi.h           |   2 +
>  include/linux/ecam.h           |  44 ++++++++
>  19 files changed, 691 insertions(+), 340 deletions(-)
>  create mode 100644 drivers/acpi/mcfg.c
>  create mode 100644 drivers/pci/ecam.c
>  create mode 100644 include/linux/ecam.h

Waiting for an updated version after all the discussion.

Bjorn

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Lorenzo Pieralisi]

On Wed, Dec 09, 2015 at 03:31:41PM +0530, Jayachandran C. wrote:
> [trimmed the cc list a bit]
> On Mon, Dec 07, 2015 at 02:31:17PM -0600, Bjorn Helgaas wrote:
> > On Tue, Oct 27, 2015 at 05:38:31PM +0100, Tomasz Nowicki wrote:
> [...]
> > > 
> > > Tomasz Nowicki (10):
> > >   x86, pci: Reorder logic of pci_mmconfig_insert() function
> > >   x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code
> > >     out of arch/x86/ directory
> > >   pci, acpi, mcfg: Provide generic implementation of MCFG code
> > >     initialization.
> > >   x86, pci: mmconfig_{32,64}.c code refactoring - remove code
> > >     duplication.
> > >   x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM
> > >     driver.
> > >   pci, acpi, mcfg: Provide default RAW ACPI PCI config space accessors.
> > >   pci, acpi, ecam: Add flag to indicate whether ECAM region was hot
> > >     added or not.
> > >   x86, pci: Use previously added ECAM hot_added flag to remove ECAM
> > >     regions.
> [...]
> > 
> > Waiting for an updated version after all the discussion.
> 
> Hi Bjorn,
> 
> After looking thru the code, I don't think moving the mmconfig code out
> of x86 and using it in ARM64 is a good idea.
> 
> The code maintains a list of mapping of ECAM regions which can modified
> at run time, and ends doing this sequence for doing a config access:
> 
>         rcu_read_lock();
>         addr = pci_dev_base(seg, bus, devfn);
> 	        list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
> 			...find mapping...
> 	do pci operation
> 	rcu_read_unlock();
> 
> A lot of the complexity in mmconfig_*.c is to maintain and validate
> the pci_mmcfg_list which seems to be unecessary.

That list is there to manage hotplug bridges, what makes you think
it is not necessary ? Jiang (in CC) can certainly comment on that and
how that list handling can be updated/simplified, if possible.

> The better way would be to keep the mapping of ECAM region in the PCI
> host bridge sysdata like pci-host-generic and directly access using
> map_bus/read/write.
> 
> I see that early raw_pci_read/raw_pci_write may need to be supported.
> I am not sure if this is needed in ARM64 - and if it is, we can handle
> this by taking an early mapping for ECAM regions until the PCI host
> bridges are setup, and dropping the mapping at that point.

I do not think it is needed (and if it is it can be added later, it
should not block this series), this was already debated in previous
threads (and in the ASWG, where basically nobody could provide a reason
why those raw accessors in ACPICA are meant to exist, they are there for
AML to access config space, why that has to be happen before PCI busses
are enumerated is still a mystery to me).

Lorenzo

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Lorenzo Pieralisi]

On Wed, Dec 16, 2015 at 06:21:37PM +0530, Jayachandran C. wrote:

[...]

> > That list is there to manage hotplug bridges, what makes you think
> > it is not necessary ? Jiang (in CC) can certainly comment on that and
> > how that list handling can be updated/simplified, if possible.
> 
> Looking thru the code, I think moving the MCFG code to common can be
> done in a simpler way. I have posted a new patchset for this.
> (And I don't see how the hotplug case needs the list, but that is not
>  relevant now)

You must work with Tomasz and come up with a unified patchset that
implements ACPI PCIe support for ARM64. We do not need more churn, we
need a working set, period. Posting separate RFCs just adds to the
confusion and to maintainers' review backlog, with the end result of
confusing everyone and achieving nothing.

Lorenzo

Re: [PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI

[Jeremy Linton]

On 10/27/2015 11:38 AM, Tomasz Nowicki wrote:
>  From the functionality point of view this series might be split into two logic parts:
> 1. Making MMCONFIG code arch-agnostic which allows all architectures to collect
>     PCI config regions and used when necessary.
> 2. Using generic MMCONFIG code and introducing ACPI based PCI hostbridge
>     initialization for ARM64


Tested with legacy interrupts on ARM Juno R1, HP m400 (APM Potenza), and 
APM mustang. The latter two platforms have an additional pci quirk patch 
applied (to be posted shortly) to fix their problems with the config 
space accessors.

Tested-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks!