[RFC PATCH 0/4] MMCFG refactoring + PCI ACPI probing for ARM64

[RFC PATCH 0/4] MMCFG refactoring + PCI ACPI probing for ARM64

[Tomasz Nowicki]

The patch set can be break down into two logic parts:
1. patch 1,2 - MMCONFIG code refacoring. MMCFG ACPI table has no arch
dependencies so it can be used across other architectures.
2. patch 3,4 aim for PCI ACPI probing on ARM64

Patches tested on Cavium ThunderX simulator with the latest ACPI ARM64 core set:
http://www.serverphorums.com/read.php?12,997228

Tomasz Nowicki (4):
  x86, acpi, pci: Move arch-agnostic MMCFG code out of arch/x86/
    directory.
  x86, acpi, pci: Isolate new PCI mmconfig entry insertion.
  arm64, acpi, pci: Add arch specific functions for mmconfig driver.
  arm64, acpi, pci: Provide arch-specific calls for PCI host bridge
    dirver (PNP0A03).

 arch/arm64/Kconfig             |   3 +
 arch/arm64/include/asm/pci.h   |   8 +
 arch/arm64/kernel/Makefile     |   1 +
 arch/arm64/kernel/mmconfig.c   |  69 +++++++
 arch/arm64/kernel/pci.c        | 401 +++++++++++++++++++++++++++++++++++++++--
 arch/x86/include/asm/pci_x86.h |  29 ---
 arch/x86/pci/acpi.c            |   1 +
 arch/x86/pci/init.c            |   1 +
 arch/x86/pci/mmconfig-shared.c | 176 +-----------------
 arch/x86/pci/mmconfig_32.c     |   1 +
 arch/x86/pci/mmconfig_64.c     |   1 +
 drivers/acpi/Makefile          |   1 +
 drivers/acpi/bus.c             |   1 +
 drivers/acpi/mmconfig.c        | 204 +++++++++++++++++++++
 include/linux/mmconfig.h       |  57 ++++++
 include/linux/pci.h            |   8 -
 16 files changed, 738 insertions(+), 224 deletions(-)
 create mode 100644 arch/arm64/kernel/mmconfig.c
 create mode 100644 drivers/acpi/mmconfig.c
 create mode 100644 include/linux/mmconfig.h

-- 
1.9.1

[RFC PATCH 1/4] x86, acpi, pci: Move arch-agnostic MMCFG code out of arch/x86/ directory.

[Tomasz Nowicki]

MMCFG table seems to be architecture independent and it makes sense
to share common code across all architectures. The ones that may need
architectural specific actions have default prototype (__weak).

Please note, there is not functional changes in this patch.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/x86/include/asm/pci_x86.h |  29 ------
 arch/x86/pci/acpi.c            |   1 +
 arch/x86/pci/init.c            |   1 +
 arch/x86/pci/mmconfig-shared.c | 167 +--------------------------------
 arch/x86/pci/mmconfig_32.c     |   1 +
 arch/x86/pci/mmconfig_64.c     |   1 +
 drivers/acpi/Makefile          |   1 +
 drivers/acpi/bus.c             |   1 +
 drivers/acpi/mmconfig.c        | 204 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mmconfig.h       |  57 ++++++++++++
 include/linux/pci.h            |   8 --
 11 files changed, 269 insertions(+), 202 deletions(-)
 create mode 100644 drivers/acpi/mmconfig.c
 create mode 100644 include/linux/mmconfig.h

diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index fa1195d..caba141 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -121,35 +121,6 @@ extern int __init pcibios_init(void);
 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
  * on their northbrige except through the * %eax register. As such, you MUST
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index cfd1b13..6d11131 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -4,6 +4,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
+#include <linux/mmconfig.h>
 #include <asm/numa.h>
 #include <asm/pci_x86.h>
 
diff --git a/arch/x86/pci/init.c b/arch/x86/pci/init.c
index adb62aa..b4a55df 100644
--- a/arch/x86/pci/init.c
+++ b/arch/x86/pci/init.c
@@ -1,5 +1,6 @@
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/mmconfig.h>
 #include <asm/pci_x86.h>
 #include <asm/x86_init.h>
 
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 326198a..94c3d38 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/mmconfig.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,7 +447,7 @@ static void __init pci_mmcfg_reject_broken(int early)
 	}
 }
 
-static int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+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;
-}
-
 static void __init __pci_mmcfg_init(int early)
 {
 	pci_mmcfg_reject_broken(early);
@@ -765,26 +625,3 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 
 	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..d774672 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/mmconfig.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..1209596 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/mmconfig.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index f96a2f1..02a0baa 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -69,6 +69,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)	+= mmconfig.o
 obj-$(CONFIG_ACPI_PROCESSOR)	+= processor.o
 obj-y				+= container.o
 obj-$(CONFIG_ACPI_THERMAL)	+= thermal.o
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index c412fdb..6d5412ab 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -41,6 +41,7 @@
 #include <acpi/apei.h>
 #include <linux/dmi.h>
 #include <linux/suspend.h>
+#include <linux/mmconfig.h>
 
 #include "internal.h"
 
diff --git a/drivers/acpi/mmconfig.c b/drivers/acpi/mmconfig.c
new file mode 100644
index 0000000..9da06b7
--- /dev/null
+++ b/drivers/acpi/mmconfig.c
@@ -0,0 +1,204 @@
+/*
+ * Arch agnostic low-level direct PCI config space access via MMCONFIG
+ *
+ * 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/mmconfig.h>
+
+#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;
+}
+
+int __init __weak acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+					struct acpi_mcfg_allocation *cfg)
+{
+	return 0;
+}
+
+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;
+}
+
+/* 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;
+}
+
+void __init __weak pci_mmcfg_early_init(void)
+{
+
+}
+
+void __init __weak pci_mmcfg_late_init(void)
+{
+	acpi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
+
+	if (list_empty(&pci_mmcfg_list))
+		return;
+
+	if (!pci_mmcfg_arch_init())
+		free_all_mmcfg();
+}
diff --git a/include/linux/mmconfig.h b/include/linux/mmconfig.h
new file mode 100644
index 0000000..3932d64
--- /dev/null
+++ b/include/linux/mmconfig.h
@@ -0,0 +1,57 @@
+#ifndef __MMCONFIG_H
+#define __MMCONFIG_H
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <linux/acpi.h>
+
+#ifdef CONFIG_PCI_MMCONFIG
+/* "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];
+};
+
+void pci_mmcfg_early_init(void);
+void pci_mmcfg_late_init(void);
+struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
+
+int pci_parse_mcfg(struct acpi_table_header *header);
+struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
+						   int end, u64 addr);
+struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
+						 int end, u64 addr);
+void list_add_sorted(struct pci_mmcfg_region *new);
+int acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,
+			  struct acpi_mcfg_allocation *cfg);
+void free_all_mmcfg(void);
+int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
+		       phys_addr_t addr);
+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)
+#else /* CONFIG_PCI_MMCONFIG */
+static inline void pci_mmcfg_late_init(void) { }
+static inline void pci_mmcfg_early_init(void) { }
+static inline void *pci_mmconfig_lookup(int segment, int bus)
+{ return NULL; }
+#endif /* CONFIG_PCI_MMCONFIG */
+
+#endif  /* __KERNEL__ */
+#endif  /* __MMCONFIG_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6afba72..0a8b82e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1658,14 +1658,6 @@ void pcibios_release_device(struct pci_dev *dev);
 extern struct dev_pm_ops pcibios_pm_ops;
 #endif
 
-#ifdef CONFIG_PCI_MMCONFIG
-void __init pci_mmcfg_early_init(void);
-void __init pci_mmcfg_late_init(void);
-#else
-static inline void pci_mmcfg_early_init(void) { }
-static inline void pci_mmcfg_late_init(void) { }
-#endif
-
 int pci_ext_cfg_avail(void);
 
 void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);
-- 
1.9.1

[RFC PATCH 2/4] x86, acpi, pci: Isolate new PCI mmconfig entry insertion.

[Tomasz Nowicki]

Add special pci_mmcfg_insert_lock mutex since pci_mmcfg_lock was moved
to common file. No functional changes.

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

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 94c3d38..d1e45e7 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -28,6 +28,7 @@
 /* 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_insert_lock);
 
 static const char *__init pci_mmcfg_e7520(void)
 {
@@ -566,7 +567,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 	if (start > end)
 		return -EINVAL;
 
-	mutex_lock(&pci_mmcfg_lock);
+	mutex_lock(&pci_mmcfg_insert_lock);
 	cfg = pci_mmconfig_lookup(seg, start);
 	if (cfg) {
 		if (cfg->end_bus < end)
@@ -575,12 +576,12 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 				 "domain %04x [bus %02x-%02x] "
 				 "only partially covers this bridge\n",
 				  cfg->segment, cfg->start_bus, cfg->end_bus);
-		mutex_unlock(&pci_mmcfg_lock);
+		mutex_unlock(&pci_mmcfg_insert_lock);
 		return -EEXIST;
 	}
 
 	if (!addr) {
-		mutex_unlock(&pci_mmcfg_lock);
+		mutex_unlock(&pci_mmcfg_insert_lock);
 		return -EINVAL;
 	}
 
@@ -621,7 +622,7 @@ int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,
 		kfree(cfg);
 	}
 
-	mutex_unlock(&pci_mmcfg_lock);
+	mutex_unlock(&pci_mmcfg_insert_lock);
 
 	return rc;
 }
-- 
1.9.1

[RFC PATCH 3/4] arm64, acpi, pci: Add arch specific functions for mmconfig driver.

[Tomasz Nowicki]

These calls allow to map/unmap PCI config space ranges (which are specified in
MMCFG ACPI table).

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/arm64/Kconfig           |  3 ++
 arch/arm64/kernel/Makefile   |  1 +
 arch/arm64/kernel/mmconfig.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 arch/arm64/kernel/mmconfig.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index aee6a60..f2bcb58 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -186,6 +186,9 @@ config PCI_DOMAINS_GENERIC
 config PCI_SYSCALL
 	def_bool PCI
 
+config PCI_MMCONFIG
+	def_bool PCI && ACPI
+
 source "drivers/pci/Kconfig"
 source "drivers/pci/pcie/Kconfig"
 source "drivers/pci/hotplug/Kconfig"
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index f48e3f7..ec576e6 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -32,6 +32,7 @@ arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
 arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
 arm64-obj-$(CONFIG_PCI)			+= pci.o
 arm64-obj-$(CONFIG_ACPI)		+= acpi.o
+arm64-obj-$(CONFIG_PCI_MMCONFIG)		+= mmconfig.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/mmconfig.c b/arch/arm64/kernel/mmconfig.c
new file mode 100644
index 0000000..b0ca0ec
--- /dev/null
+++ b/arch/arm64/kernel/mmconfig.c
@@ -0,0 +1,69 @@
+/*
+ * mmconfig.c - Low-level direct PCI config space access via MMCONFIG
+ *
+ * This is an ARM64 version that allows the mmconfig space to be mapped.
+ *
+ * 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/mmconfig.h>
+
+#define PREFIX "PCI: "
+
+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;
+	}
+}
-- 
1.9.1

[RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Tomasz Nowicki]

Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files.
The main reasons why we need this:
- parse and manage resources (BUS, IO and MEM)
- create pci root bus and enumerate its children
- provide PCI config space accessors (via MMCONFIG)

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/arm64/include/asm/pci.h |   8 +
 arch/arm64/kernel/pci.c      | 401 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 391 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index fded096..ecd940f 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
 extern int isa_dma_bridge_buggy;
 
 #ifdef CONFIG_PCI
+struct pci_controller {
+	struct acpi_device *companion;
+	int segment;
+	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
+};
+
+#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
+
 static inline int pci_proc_domain(struct pci_bus *bus)
 {
 	return 1;
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 42fb195..cc34ded 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -1,6 +1,10 @@
 /*
- * Code borrowed from powerpc/kernel/pci-common.c
+ * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/pci.c
  *
+ * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P.
+ *	David Mosberger-Tang <davidm@hpl.hp.com>
+ *	Bjorn Helgaas <bjorn.helgaas@hp.com>
+ * Copyright (C) 2004 Silicon Graphics, Inc.
  * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
  * Copyright (C) 2014 ARM Ltd.
  *
@@ -17,10 +21,16 @@
 #include <linux/mm.h>
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/acpi.h>
+#include <linux/mmconfig.h>
 
 #include <asm/pci-bridge.h>
 
+#define PREFIX "PCI: "
+
 /*
  * Called after each bus is probed, but before its children are examined
  */
@@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
  */
 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);
 
 	return 0;
 }
@@ -54,45 +65,399 @@ static bool dt_domain_found = false;
 
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
-	if (domain >= 0) {
-		dt_domain_found = true;
-	} else if (dt_domain_found == true) {
-		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
-			parent->of_node->full_name);
-		return;
+	if (acpi_disabled) {
+		domain = of_get_pci_domain_nr(parent->of_node);
+
+		if (domain >= 0) {
+			dt_domain_found = true;
+		} else if (dt_domain_found == true) {
+			dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
+				parent->of_node->full_name);
+			return;
+		} else {
+			domain = pci_get_new_domain_nr();
+		}
 	} else {
-		domain = pci_get_new_domain_nr();
+		/*
+		 * Take the domain nr from associated private data.
+		 * Case where bus has parent is covered in pci_alloc_bus()
+		 */
+		if (!parent)
+			domain = PCI_CONTROLLER(bus)->segment;
 	}
 
 	bus->domain_nr = domain;
 }
 #endif
 
+static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus,
+				  unsigned int devfn)
+{
+	struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus);
+
+	if (cfg && cfg->virt)
+		return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12));
+	return NULL;
+}
+
 /*
  * raw_pci_read/write - Platform-specific PCI config space access.
- *
- * Default empty implementation.  Replace with an architecture-specific setup
- * routine, if necessary.
  */
 int raw_pci_read(unsigned int domain, unsigned int bus,
 		  unsigned int devfn, int reg, int len, u32 *val)
 {
-	return -EINVAL;
+	char __iomem *addr;
+
+	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
+err:		*val = -1;
+		return -EINVAL;
+	}
+
+	rcu_read_lock();
+	addr = pci_dev_base(domain, bus, devfn);
+	if (!addr) {
+		rcu_read_unlock();
+		goto err;
+	}
+
+	switch (len) {
+	case 1:
+		*val = readb(addr + reg);
+		break;
+	case 2:
+		*val = readw(addr + reg);
+		break;
+	case 4:
+		*val = readl(addr + reg);
+		break;
+	}
+	rcu_read_unlock();
+
+	return 0;
 }
 
 int raw_pci_write(unsigned int domain, unsigned int bus,
 		unsigned int devfn, int reg, int len, u32 val)
 {
-	return -EINVAL;
+	char __iomem *addr;
+
+	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
+		return -EINVAL;
+
+	rcu_read_lock();
+	addr = pci_dev_base(domain, bus, devfn);
+	if (!addr) {
+		rcu_read_unlock();
+		return -EINVAL;
+	}
+
+	switch (len) {
+	case 1:
+		writeb(val, addr + reg);
+		break;
+	case 2:
+		writew(val, addr + reg);
+		break;
+	case 4:
+		writel(val, addr + reg);
+		break;
+	}
+	rcu_read_unlock();
+
+	return 0;
 }
 
 #ifdef CONFIG_ACPI
+static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
+		    int size, u32 *value)
+{
+	return raw_pci_read(pci_domain_nr(bus), bus->number,
+			    devfn, where, size, value);
+}
+
+static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
+		     int size, u32 value)
+{
+	return raw_pci_write(pci_domain_nr(bus), bus->number,
+			     devfn, where, size, value);
+}
+
+struct pci_ops pci_root_ops = {
+	.read = pci_read,
+	.write = pci_write,
+};
+
+static struct pci_controller *alloc_pci_controller(int seg)
+{
+	struct pci_controller *controller;
+
+	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+	if (!controller)
+		return NULL;
+
+	controller->segment = seg;
+	return controller;
+}
+
+struct pci_root_info {
+	struct acpi_device *bridge;
+	struct pci_controller *controller;
+	struct list_head resources;
+	struct resource *res;
+	resource_size_t *res_offset;
+	unsigned int res_num;
+	char *name;
+};
+
+static acpi_status resource_to_window(struct acpi_resource *resource,
+				      struct acpi_resource_address64 *addr)
+{
+	acpi_status status;
+
+	/*
+	 * We're only interested in _CRS descriptors that are
+	 *	- address space descriptors for memory
+	 *	- non-zero size
+	 *	- producers, i.e., the address space is routed downstream,
+	 *	  not consumed by the bridge itself
+	 */
+	status = acpi_resource_to_address64(resource, addr);
+	if (ACPI_SUCCESS(status) &&
+	    (addr->resource_type == ACPI_MEMORY_RANGE ||
+	     addr->resource_type == ACPI_IO_RANGE) &&
+	    addr->address_length &&
+	    addr->producer_consumer == ACPI_PRODUCER)
+		return AE_OK;
+
+	return AE_ERROR;
+}
+
+static acpi_status count_window(struct acpi_resource *resource, void *data)
+{
+	unsigned int *windows = (unsigned int *) data;
+	struct acpi_resource_address64 addr;
+	acpi_status status;
+
+	status = resource_to_window(resource, &addr);
+	if (ACPI_SUCCESS(status))
+		(*windows)++;
+
+	return AE_OK;
+}
+
+static acpi_status add_window(struct acpi_resource *res, void *data)
+{
+	struct pci_root_info *info = data;
+	struct resource *resource;
+	struct acpi_resource_address64 addr;
+	acpi_status status;
+	unsigned long flags;
+	struct resource *root;
+	u64 start;
+
+	/* Return AE_OK for non-window resources to keep scanning for more */
+	status = resource_to_window(res, &addr);
+	if (!ACPI_SUCCESS(status))
+		return AE_OK;
+
+	if (addr.resource_type == ACPI_MEMORY_RANGE) {
+		flags = IORESOURCE_MEM;
+		root = &iomem_resource;
+	} else if (addr.resource_type == ACPI_IO_RANGE) {
+		flags = IORESOURCE_IO;
+		root = &ioport_resource;
+	} else
+		return AE_OK;
+
+	start = addr.minimum + addr.translation_offset;
+
+	resource = &info->res[info->res_num];
+	resource->name = info->name;
+	resource->flags = flags;
+	resource->start = start;
+	resource->end = resource->start + addr.address_length - 1;
+
+	if (flags & IORESOURCE_IO) {
+		unsigned long port;
+		int err;
+
+		err = pci_register_io_range(start, addr.address_length);
+		if (err)
+			return AE_OK;
+
+		port = pci_address_to_pio(start);
+		if (port == (unsigned long)-1)
+			return AE_OK;
+
+		resource->start = port;
+		resource->end = port + addr.address_length - 1;
+
+		if (pci_remap_iospace(resource, start) < 0)
+			return AE_OK;
+
+		info->res_offset[info->res_num] = 0;
+	} else
+		info->res_offset[info->res_num] = addr.translation_offset;
+
+	if (insert_resource(root, resource)) {
+		dev_err(&info->bridge->dev,
+			"can't allocate host bridge window %pR\n",
+			resource);
+	} else {
+		if (addr.translation_offset)
+			dev_info(&info->bridge->dev, "host bridge window %pR "
+				 "(PCI address [%#llx-%#llx])\n",
+				 resource,
+				 resource->start - addr.translation_offset,
+				 resource->end - addr.translation_offset);
+		else
+			dev_info(&info->bridge->dev,
+				 "host bridge window %pR\n", resource);
+	}
+
+	pci_add_resource_offset(&info->resources, resource,
+				info->res_offset[info->res_num]);
+	info->res_num++;
+	return AE_OK;
+}
+
+static void free_pci_root_info_res(struct pci_root_info *info)
+{
+	kfree(info->name);
+	kfree(info->res);
+	info->res = NULL;
+	kfree(info->res_offset);
+	info->res_offset = NULL;
+	info->res_num = 0;
+	kfree(info->controller);
+	info->controller = NULL;
+}
+
+static void __release_pci_root_info(struct pci_root_info *info)
+{
+	int i;
+	struct resource *res;
+
+	for (i = 0; i < info->res_num; i++) {
+		res = &info->res[i];
+
+		if (!res->parent)
+			continue;
+
+		if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
+			continue;
+
+		release_resource(res);
+	}
+
+	free_pci_root_info_res(info);
+	kfree(info);
+}
+
+static void release_pci_root_info(struct pci_host_bridge *bridge)
+{
+	struct pci_root_info *info = bridge->release_data;
+
+	__release_pci_root_info(info);
+}
+
+static int
+probe_pci_root_info(struct pci_root_info *info, struct acpi_device *device,
+		int busnum, int domain)
+{
+	char *name;
+
+	name = kmalloc(16, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+
+	sprintf(name, "PCI Bus %04x:%02x", domain, busnum);
+	info->bridge = device;
+	info->name = name;
+
+	acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_window,
+			&info->res_num);
+	if (info->res_num) {
+		info->res =
+			kzalloc_node(sizeof(*info->res) * info->res_num,
+				     GFP_KERNEL, info->controller->node);
+		if (!info->res) {
+			kfree(name);
+			return -ENOMEM;
+		}
+
+		info->res_offset =
+			kzalloc_node(sizeof(*info->res_offset) * info->res_num,
+					GFP_KERNEL, info->controller->node);
+		if (!info->res_offset) {
+			kfree(name);
+			kfree(info->res);
+			info->res = NULL;
+			return -ENOMEM;
+		}
+
+		info->res_num = 0;
+		acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+			add_window, info);
+	} else
+		kfree(name);
+
+	return 0;
+}
+
 /* 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;
+	struct acpi_device *device = root->device;
+	int domain = root->segment;
+	int bus = root->secondary.start;
+	struct pci_controller *controller;
+	struct pci_root_info *info = NULL;
+	int busnum = root->secondary.start;
+	struct pci_bus *pbus;
+	int ret;
+
+	controller = alloc_pci_controller(domain);
+	if (!controller)
+		return NULL;
+
+	controller->companion = device;
+	controller->node = acpi_get_node(device->handle);
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&device->dev,
+				"pci_bus %04x:%02x: ignored (out of memory)\n",
+				domain, busnum);
+		kfree(controller);
+		return NULL;
+	}
+
+	info->controller = controller;
+	INIT_LIST_HEAD(&info->resources);
+
+	ret = probe_pci_root_info(info, device, busnum, domain);
+	if (ret) {
+		kfree(info->controller);
+		kfree(info);
+		return NULL;
+	}
+	/* insert busn resource at first */
+	pci_add_resource(&info->resources, &root->secondary);
+
+	pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, controller,
+				   &info->resources);
+	if (!pbus) {
+		pci_free_resource_list(&info->resources);
+		__release_pci_root_info(info);
+		return NULL;
+	}
+
+	pci_set_host_bridge_release(to_pci_host_bridge(pbus->bridge),
+			release_pci_root_info, info);
+	pci_scan_child_bus(pbus);
+	return pbus;
 }
-#endif
+#endif /* CONFIG_ACPI */
-- 
1.9.1

Re: [RFC PATCH 2/4] x86, acpi, pci: Isolate new PCI mmconfig entry insertion.

[Arnd Bergmann]

On Friday 07 November 2014 14:27:54 Tomasz Nowicki wrote:
> Add special pci_mmcfg_insert_lock mutex since pci_mmcfg_lock was moved
> to common file. No functional changes.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> 

What is the new lock protecting against? You no longer have a mutual
exclusion with the other code, which looks like it is a functional
change, and also incorrect.

It also sounds like the code was broken after the first patch and
you need this one to make it compile again. You can't do that, and
instead need to change the locking rules first.

	Arnd

Re: [Linaro-acpi] [RFC PATCH 3/4] arm64, acpi, pci: Add arch specific functions for mmconfig driver.

[Arnd Bergmann]

On Friday 07 November 2014 14:27:55 Tomasz Nowicki wrote:
> These calls allow to map/unmap PCI config space ranges (which are specified in
> MMCFG ACPI table).
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> 

Nothing in this patch looks arm64 specific, and most of it looks like a
copy of the x86 code.

	Arnd

Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Arnd Bergmann]

On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
>  
>  #ifdef CONFIG_PCI
> +struct pci_controller {
> +	struct acpi_device *companion;
> +	int segment;
> +	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
> +};
> +
> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
> +

Don't use busdev->sysdata in architecture specific code, it belongs to the
host bridge driver with the new model. For ACPI you don't have a host bridge
driver, but it's better to keep these separate.

The segment is always the same as the domain number, so just use that.
The node and companion members here can get added to struct pci_host_bridge.

> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>   */
>  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);
>  
>  	return 0;
>  }

How do you assign the irq number with ACPI? Do you only support MSI?

>  /*
>   * raw_pci_read/write - Platform-specific PCI config space access.
> - *
> - * Default empty implementation.  Replace with an architecture-specific setup
> - * routine, if necessary.
>   */
>  int raw_pci_read(unsigned int domain, unsigned int bus,
>  		  unsigned int devfn, int reg, int len, u32 *val)
>  {
> -	return -EINVAL;
> +	char __iomem *addr;
> +
> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> +err:		*val = -1;
> +		return -EINVAL;
> +	}
> +
> +	rcu_read_lock();
> +	addr = pci_dev_base(domain, bus, devfn);
> +	if (!addr) {
> +		rcu_read_unlock();
> +		goto err;
> +	}

The config space accessors should probably be shared with
drivers/pci/host/pci-host-generic.c, e.g. by moving the rest of
the new code in there as well, or by moving the config space
accessors from that file to drivers/pci/mmconfig.c.

	Arnd

Re: [Linaro-acpi] [RFC PATCH 3/4] arm64, acpi, pci: Add arch specific functions for mmconfig driver.

[Tomasz Nowicki]

On 07.11.2014 15:12, Arnd Bergmann wrote:
> On Friday 07 November 2014 14:27:55 Tomasz Nowicki wrote:
>> These calls allow to map/unmap PCI config space ranges (which are specified in
>> MMCFG ACPI table).
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>
>
> Nothing in this patch looks arm64 specific, and most of it looks like a
> copy of the x86 code.

Yes, most of the logic was borrowed from mmconfig_64.c file, 
mmconfig_32.c looks differently, though. It is not simple to merge them 
both. IMO, we have two choices:
1. Refactor and move mmconfig_64.c out of x86 to e.g. drivers/acpi/ and 
let it be default.
2. Stay with solution presented in this patch.
3. Thoughts ?

Regards,
Tomasz

Re: [RFC PATCH 2/4] x86, acpi, pci: Isolate new PCI mmconfig entry insertion.

[Tomasz Nowicki]

On 07.11.2014 15:09, Arnd Bergmann wrote:
> On Friday 07 November 2014 14:27:54 Tomasz Nowicki wrote:
>> Add special pci_mmcfg_insert_lock mutex since pci_mmcfg_lock was moved
>> to common file. No functional changes.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>
>
> What is the new lock protecting against? You no longer have a mutual
> exclusion with the other code, which looks like it is a functional
> change, and also incorrect.
>
> It also sounds like the code was broken after the first patch and
> you need this one to make it compile again. You can't do that, and
> instead need to change the locking rules first.

Oh, that's right! Sorry I will take a look again.

Tomasz

Re: [Linaro-acpi] [RFC PATCH 3/4] arm64, acpi, pci: Add arch specific functions for mmconfig driver.

[Arnd Bergmann]

On Friday 07 November 2014 15:39:32 Tomasz Nowicki wrote:
> On 07.11.2014 15:12, Arnd Bergmann wrote:
> > On Friday 07 November 2014 14:27:55 Tomasz Nowicki wrote:
> >> These calls allow to map/unmap PCI config space ranges (which are specified in
> >> MMCFG ACPI table).
> >>
> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >>
> >
> > Nothing in this patch looks arm64 specific, and most of it looks like a
> > copy of the x86 code.
> 
> Yes, most of the logic was borrowed from mmconfig_64.c file, 
> mmconfig_32.c looks differently, though. It is not simple to merge them 
> both. IMO, we have two choices:
> 1. Refactor and move mmconfig_64.c out of x86 to e.g. drivers/acpi/ and 
> let it be default.
> 2. Stay with solution presented in this patch.
> 3. Thoughts ?

If the code is generic, it should be shared with as many architectures as
possible. Moving the x86-64 implementation to drivers/acpi/ would immediately
let you share it with two out of the four architectures (x86-64 and arm64,
but not x86-32 and ia64) as well as any potential other architectures that
might implement ACPI in the future.

	Arnd

Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Liviu Dudau]

On Fri, Nov 07, 2014 at 01:27:56PM +0000, Tomasz Nowicki wrote:
> Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files.
> The main reasons why we need this:
> - parse and manage resources (BUS, IO and MEM)
> - create pci root bus and enumerate its children
> - provide PCI config space accessors (via MMCONFIG)

Hi Tomasz,

I have some comments to your code:

> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/arm64/include/asm/pci.h |   8 +
>  arch/arm64/kernel/pci.c      | 401 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 391 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index fded096..ecd940f 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>  extern int isa_dma_bridge_buggy;
> 
>  #ifdef CONFIG_PCI
> +struct pci_controller {
> +       struct acpi_device *companion;
> +       int segment;
> +       int node;               /* nearest node with memory or NUMA_NO_NODE for global allocation */
> +};
> +
> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)

I am trying to move away from the model where the architecture dictates the shape
of the pci_controller structure. Can I suggest that you put all this code and the
one under #ifdef CONFIG_ACPI in a separate file, say arch/arm64/kernel/pci-acpi.c ?
Or that you add an #ifdef CONFIG_ACPI around this structure definition?

I can't see anyone using this definition outside ACPI (due to struct acpi_device
dependency) and I would like to avoid it conflicting with other host bridge
drivers trying to define the same name structure.


> +
>  static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>         return 1;
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 42fb195..cc34ded 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -1,6 +1,10 @@
>  /*
> - * Code borrowed from powerpc/kernel/pci-common.c
> + * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/pci.c
>   *
> + * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P.
> + *     David Mosberger-Tang <davidm@hpl.hp.com>
> + *     Bjorn Helgaas <bjorn.helgaas@hp.com>
> + * Copyright (C) 2004 Silicon Graphics, Inc.
>   * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
>   * Copyright (C) 2014 ARM Ltd.
>   *
> @@ -17,10 +21,16 @@
>  #include <linux/mm.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_address.h>
>  #include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/acpi.h>
> +#include <linux/mmconfig.h>
> 
>  #include <asm/pci-bridge.h>
> 
> +#define PREFIX "PCI: "

Where is this used?

> +
>  /*
>   * Called after each bus is probed, but before its children are examined
>   */
> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>   */
>  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);
> 
>         return 0;
>  }
> @@ -54,45 +65,399 @@ static bool dt_domain_found = false;
> 
>  void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  {
> -       int domain = of_get_pci_domain_nr(parent->of_node);
> +       int domain = -1;
> 
> -       if (domain >= 0) {
> -               dt_domain_found = true;
> -       } else if (dt_domain_found == true) {
> -               dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -                       parent->of_node->full_name);
> -               return;
> +       if (acpi_disabled) {
> +               domain = of_get_pci_domain_nr(parent->of_node);
> +
> +               if (domain >= 0) {
> +                       dt_domain_found = true;
> +               } else if (dt_domain_found == true) {
> +                       dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> +                               parent->of_node->full_name);
> +                       return;
> +               } else {
> +                       domain = pci_get_new_domain_nr();
> +               }
>         } else {
> -               domain = pci_get_new_domain_nr();
> +               /*
> +                * Take the domain nr from associated private data.
> +                * Case where bus has parent is covered in pci_alloc_bus()
> +                */
> +               if (!parent)
> +                       domain = PCI_CONTROLLER(bus)->segment;

I would also like to wrap this into an ACPI specific function. Reason for it
is that when I get to split the pci_host_bridge creation out of pci_create_root_bus()
this will become a pci_controller property.

>         }
> 
>         bus->domain_nr = domain;
>  }
>  #endif
> 
> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus,
> +                                 unsigned int devfn)
> +{
> +       struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus);
> +
> +       if (cfg && cfg->virt)
> +               return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12));
> +       return NULL;
> +}
> +
>  /*
>   * raw_pci_read/write - Platform-specific PCI config space access.
> - *
> - * Default empty implementation.  Replace with an architecture-specific setup
> - * routine, if necessary.
>   */
>  int raw_pci_read(unsigned int domain, unsigned int bus,
>                   unsigned int devfn, int reg, int len, u32 *val)
>  {
> -       return -EINVAL;
> +       char __iomem *addr;
> +
> +       if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> +err:           *val = -1;

I believe the general usage is to have labels on their own line.

> +               return -EINVAL;
> +       }
> +
> +       rcu_read_lock();
> +       addr = pci_dev_base(domain, bus, devfn);
> +       if (!addr) {
> +               rcu_read_unlock();
> +               goto err;
> +       }
> +
> +       switch (len) {
> +       case 1:
> +               *val = readb(addr + reg);
> +               break;
> +       case 2:
> +               *val = readw(addr + reg);
> +               break;
> +       case 4:
> +               *val = readl(addr + reg);
> +               break;
> +       }
> +       rcu_read_unlock();
> +
> +       return 0;
>  }
> 
>  int raw_pci_write(unsigned int domain, unsigned int bus,
>                 unsigned int devfn, int reg, int len, u32 val)
>  {
> -       return -EINVAL;
> +       char __iomem *addr;
> +
> +       if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> +               return -EINVAL;
> +
> +       rcu_read_lock();
> +       addr = pci_dev_base(domain, bus, devfn);
> +       if (!addr) {
> +               rcu_read_unlock();
> +               return -EINVAL;
> +       }
> +
> +       switch (len) {
> +       case 1:
> +               writeb(val, addr + reg);
> +               break;
> +       case 2:
> +               writew(val, addr + reg);
> +               break;
> +       case 4:
> +               writel(val, addr + reg);
> +               break;
> +       }
> +       rcu_read_unlock();
> +
> +       return 0;
>  }
> 

These raw_pci_{read,write} functions are all ACPI specific so they can move
into the new file as well.


>  #ifdef CONFIG_ACPI
> +static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
> +                   int size, u32 *value)
> +{
> +       return raw_pci_read(pci_domain_nr(bus), bus->number,
> +                           devfn, where, size, value);
> +}
> +
> +static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
> +                    int size, u32 value)
> +{
> +       return raw_pci_write(pci_domain_nr(bus), bus->number,
> +                            devfn, where, size, value);
> +}
> +
> +struct pci_ops pci_root_ops = {
> +       .read = pci_read,
> +       .write = pci_write,
> +};
> +
> +static struct pci_controller *alloc_pci_controller(int seg)
> +{
> +       struct pci_controller *controller;
> +
> +       controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +       if (!controller)
> +               return NULL;
> +
> +       controller->segment = seg;
> +       return controller;
> +}
> +
> +struct pci_root_info {
> +       struct acpi_device *bridge;
> +       struct pci_controller *controller;
> +       struct list_head resources;
> +       struct resource *res;
> +       resource_size_t *res_offset;

Why do you need to keep an array of res_offsets here? You only seem
to be using the values once when you add the resource to the host
bridge windows.

> +       unsigned int res_num;
> +       char *name;
> +};
> +
> +static acpi_status resource_to_window(struct acpi_resource *resource,
> +                                     struct acpi_resource_address64 *addr)
> +{
> +       acpi_status status;
> +
> +       /*
> +        * We're only interested in _CRS descriptors that are
> +        *      - address space descriptors for memory
> +        *      - non-zero size
> +        *      - producers, i.e., the address space is routed downstream,
> +        *        not consumed by the bridge itself
> +        */
> +       status = acpi_resource_to_address64(resource, addr);
> +       if (ACPI_SUCCESS(status) &&
> +           (addr->resource_type == ACPI_MEMORY_RANGE ||
> +            addr->resource_type == ACPI_IO_RANGE) &&
> +           addr->address_length &&
> +           addr->producer_consumer == ACPI_PRODUCER)
> +               return AE_OK;
> +
> +       return AE_ERROR;
> +}
> +
> +static acpi_status count_window(struct acpi_resource *resource, void *data)
> +{
> +       unsigned int *windows = (unsigned int *) data;
> +       struct acpi_resource_address64 addr;
> +       acpi_status status;
> +
> +       status = resource_to_window(resource, &addr);
> +       if (ACPI_SUCCESS(status))
> +               (*windows)++;
> +
> +       return AE_OK;
> +}
> +
> +static acpi_status add_window(struct acpi_resource *res, void *data)
> +{
> +       struct pci_root_info *info = data;
> +       struct resource *resource;
> +       struct acpi_resource_address64 addr;
> +       acpi_status status;
> +       unsigned long flags;
> +       struct resource *root;
> +       u64 start;
> +
> +       /* Return AE_OK for non-window resources to keep scanning for more */
> +       status = resource_to_window(res, &addr);
> +       if (!ACPI_SUCCESS(status))
> +               return AE_OK;
> +
> +       if (addr.resource_type == ACPI_MEMORY_RANGE) {
> +               flags = IORESOURCE_MEM;
> +               root = &iomem_resource;
> +       } else if (addr.resource_type == ACPI_IO_RANGE) {
> +               flags = IORESOURCE_IO;
> +               root = &ioport_resource;
> +       } else
> +               return AE_OK;
> +
> +       start = addr.minimum + addr.translation_offset;
> +
> +       resource = &info->res[info->res_num];
> +       resource->name = info->name;
> +       resource->flags = flags;
> +       resource->start = start;
> +       resource->end = resource->start + addr.address_length - 1;
> +
> +       if (flags & IORESOURCE_IO) {
> +               unsigned long port;
> +               int err;
> +
> +               err = pci_register_io_range(start, addr.address_length);
> +               if (err)
> +                       return AE_OK;
> +
> +               port = pci_address_to_pio(start);
> +               if (port == (unsigned long)-1)
> +                       return AE_OK;
> +
> +               resource->start = port;
> +               resource->end = port + addr.address_length - 1;
> +
> +               if (pci_remap_iospace(resource, start) < 0)
> +                       return AE_OK;

You seem to leave around invalid resources every time you return in this code
path due to your choice of ignoring error conditions.

I think there are a few things that can be streamlined in this patch, but
overall I think it looks promising.

Best regards,
Liviu


> +
> +               info->res_offset[info->res_num] = 0;
> +       } else
> +               info->res_offset[info->res_num] = addr.translation_offset;
> +
> +       if (insert_resource(root, resource)) {
> +               dev_err(&info->bridge->dev,
> +                       "can't allocate host bridge window %pR\n",
> +                       resource);
> +       } else {
> +               if (addr.translation_offset)
> +                       dev_info(&info->bridge->dev, "host bridge window %pR "
> +                                "(PCI address [%#llx-%#llx])\n",
> +                                resource,
> +                                resource->start - addr.translation_offset,
> +                                resource->end - addr.translation_offset);
> +               else
> +                       dev_info(&info->bridge->dev,
> +                                "host bridge window %pR\n", resource);
> +       }
> +
> +       pci_add_resource_offset(&info->resources, resource,
> +                               info->res_offset[info->res_num]);
> +       info->res_num++;
> +       return AE_OK;
> +}
> +
> +static void free_pci_root_info_res(struct pci_root_info *info)
> +{
> +       kfree(info->name);
> +       kfree(info->res);
> +       info->res = NULL;
> +       kfree(info->res_offset);
> +       info->res_offset = NULL;
> +       info->res_num = 0;
> +       kfree(info->controller);
> +       info->controller = NULL;
> +}
> +
> +static void __release_pci_root_info(struct pci_root_info *info)
> +{
> +       int i;
> +       struct resource *res;
> +
> +       for (i = 0; i < info->res_num; i++) {
> +               res = &info->res[i];
> +
> +               if (!res->parent)
> +                       continue;
> +
> +               if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +                       continue;
> +
> +               release_resource(res);
> +       }
> +
> +       free_pci_root_info_res(info);
> +       kfree(info);
> +}
> +
> +static void release_pci_root_info(struct pci_host_bridge *bridge)
> +{
> +       struct pci_root_info *info = bridge->release_data;
> +
> +       __release_pci_root_info(info);
> +}
> +
> +static int
> +probe_pci_root_info(struct pci_root_info *info, struct acpi_device *device,
> +               int busnum, int domain)
> +{
> +       char *name;
> +
> +       name = kmalloc(16, GFP_KERNEL);
> +       if (!name)
> +               return -ENOMEM;
> +
> +       sprintf(name, "PCI Bus %04x:%02x", domain, busnum);
> +       info->bridge = device;
> +       info->name = name;
> +
> +       acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_window,
> +                       &info->res_num);
> +       if (info->res_num) {
> +               info->res =
> +                       kzalloc_node(sizeof(*info->res) * info->res_num,
> +                                    GFP_KERNEL, info->controller->node);
> +               if (!info->res) {
> +                       kfree(name);
> +                       return -ENOMEM;
> +               }
> +
> +               info->res_offset =
> +                       kzalloc_node(sizeof(*info->res_offset) * info->res_num,
> +                                       GFP_KERNEL, info->controller->node);
> +               if (!info->res_offset) {
> +                       kfree(name);
> +                       kfree(info->res);
> +                       info->res = NULL;
> +                       return -ENOMEM;
> +               }
> +
> +               info->res_num = 0;
> +               acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +                       add_window, info);
> +       } else
> +               kfree(name);
> +
> +       return 0;
> +}
> +
>  /* 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;
> +       struct acpi_device *device = root->device;
> +       int domain = root->segment;
> +       int bus = root->secondary.start;
> +       struct pci_controller *controller;
> +       struct pci_root_info *info = NULL;
> +       int busnum = root->secondary.start;
> +       struct pci_bus *pbus;
> +       int ret;
> +
> +       controller = alloc_pci_controller(domain);
> +       if (!controller)
> +               return NULL;
> +
> +       controller->companion = device;
> +       controller->node = acpi_get_node(device->handle);
> +
> +       info = kzalloc(sizeof(*info), GFP_KERNEL);
> +       if (!info) {
> +               dev_err(&device->dev,
> +                               "pci_bus %04x:%02x: ignored (out of memory)\n",
> +                               domain, busnum);
> +               kfree(controller);
> +               return NULL;
> +       }
> +
> +       info->controller = controller;
> +       INIT_LIST_HEAD(&info->resources);
> +
> +       ret = probe_pci_root_info(info, device, busnum, domain);
> +       if (ret) {
> +               kfree(info->controller);
> +               kfree(info);
> +               return NULL;
> +       }
> +       /* insert busn resource at first */
> +       pci_add_resource(&info->resources, &root->secondary);
> +
> +       pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, controller,
> +                                  &info->resources);
> +       if (!pbus) {
> +               pci_free_resource_list(&info->resources);
> +               __release_pci_root_info(info);
> +               return NULL;
> +       }
> +
> +       pci_set_host_bridge_release(to_pci_host_bridge(pbus->bridge),
> +                       release_pci_root_info, info);
> +       pci_scan_child_bus(pbus);
> +       return pbus;
>  }
> -#endif
> +#endif /* CONFIG_ACPI */
> --
> 1.9.1
> 
> 

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

Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Tomasz Nowicki]

W dniu 07.11.2014 o 15:55, Liviu Dudau pisze:
> On Fri, Nov 07, 2014 at 01:27:56PM +0000, Tomasz Nowicki wrote:
>> Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files.
>> The main reasons why we need this:
>> - parse and manage resources (BUS, IO and MEM)
>> - create pci root bus and enumerate its children
>> - provide PCI config space accessors (via MMCONFIG)
>
> Hi Tomasz,
>
> I have some comments to your code:
>
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>>   arch/arm64/include/asm/pci.h |   8 +
>>   arch/arm64/kernel/pci.c      | 401 +++++++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 391 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>> index fded096..ecd940f 100644
>> --- a/arch/arm64/include/asm/pci.h
>> +++ b/arch/arm64/include/asm/pci.h
>> @@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>>   extern int isa_dma_bridge_buggy;
>>
>>   #ifdef CONFIG_PCI
>> +struct pci_controller {
>> +       struct acpi_device *companion;
>> +       int segment;
>> +       int node;               /* nearest node with memory or NUMA_NO_NODE for global allocation */
>> +};
>> +
>> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
>
> I am trying to move away from the model where the architecture dictates the shape
> of the pci_controller structure. Can I suggest that you put all this code and the
> one under #ifdef CONFIG_ACPI in a separate file, say arch/arm64/kernel/pci-acpi.c ?
> Or that you add an #ifdef CONFIG_ACPI around this structure definition?
>
> I can't see anyone using this definition outside ACPI (due to struct acpi_device
> dependency) and I would like to avoid it conflicting with other host bridge
> drivers trying to define the same name structure.

That is good point, will do, thanks.

>
>
>> +
>>   static inline int pci_proc_domain(struct pci_bus *bus)
>>   {
>>          return 1;
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 42fb195..cc34ded 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -1,6 +1,10 @@
>>   /*
>> - * Code borrowed from powerpc/kernel/pci-common.c
>> + * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/pci.c
>>    *
>> + * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P.
>> + *     David Mosberger-Tang <davidm@hpl.hp.com>
>> + *     Bjorn Helgaas <bjorn.helgaas@hp.com>
>> + * Copyright (C) 2004 Silicon Graphics, Inc.
>>    * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
>>    * Copyright (C) 2014 ARM Ltd.
>>    *
>> @@ -17,10 +21,16 @@
>>   #include <linux/mm.h>
>>   #include <linux/of_pci.h>
>>   #include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>>   #include <linux/slab.h>
>> +#include <linux/pci.h>
>> +#include <linux/acpi.h>
>> +#include <linux/mmconfig.h>
>>
>>   #include <asm/pci-bridge.h>
>>
>> +#define PREFIX "PCI: "
>
> Where is this used?

Nowhere here, will remove/improve.

>
>> +
>>   /*
>>    * Called after each bus is probed, but before its children are examined
>>    */
>> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>    */
>>   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);
>>
>>          return 0;
>>   }
>> @@ -54,45 +65,399 @@ static bool dt_domain_found = false;
>>
>>   void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>   {
>> -       int domain = of_get_pci_domain_nr(parent->of_node);
>> +       int domain = -1;
>>
>> -       if (domain >= 0) {
>> -               dt_domain_found = true;
>> -       } else if (dt_domain_found == true) {
>> -               dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
>> -                       parent->of_node->full_name);
>> -               return;
>> +       if (acpi_disabled) {
>> +               domain = of_get_pci_domain_nr(parent->of_node);
>> +
>> +               if (domain >= 0) {
>> +                       dt_domain_found = true;
>> +               } else if (dt_domain_found == true) {
>> +                       dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
>> +                               parent->of_node->full_name);
>> +                       return;
>> +               } else {
>> +                       domain = pci_get_new_domain_nr();
>> +               }
>>          } else {
>> -               domain = pci_get_new_domain_nr();
>> +               /*
>> +                * Take the domain nr from associated private data.
>> +                * Case where bus has parent is covered in pci_alloc_bus()
>> +                */
>> +               if (!parent)
>> +                       domain = PCI_CONTROLLER(bus)->segment;
>
> I would also like to wrap this into an ACPI specific function. Reason for it
> is that when I get to split the pci_host_bridge creation out of pci_create_root_bus()
> this will become a pci_controller property.

Make sense for me.

>
>>          }
>>
>>          bus->domain_nr = domain;
>>   }
>>   #endif
>>
>> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus,
>> +                                 unsigned int devfn)
>> +{
>> +       struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus);
>> +
>> +       if (cfg && cfg->virt)
>> +               return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12));
>> +       return NULL;
>> +}
>> +
>>   /*
>>    * raw_pci_read/write - Platform-specific PCI config space access.
>> - *
>> - * Default empty implementation.  Replace with an architecture-specific setup
>> - * routine, if necessary.
>>    */
>>   int raw_pci_read(unsigned int domain, unsigned int bus,
>>                    unsigned int devfn, int reg, int len, u32 *val)
>>   {
>> -       return -EINVAL;
>> +       char __iomem *addr;
>> +
>> +       if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
>> +err:           *val = -1;
>
> I believe the general usage is to have labels on their own line.
>
>> +               return -EINVAL;
>> +       }
>> +
>> +       rcu_read_lock();
>> +       addr = pci_dev_base(domain, bus, devfn);
>> +       if (!addr) {
>> +               rcu_read_unlock();
>> +               goto err;
>> +       }
>> +
>> +       switch (len) {
>> +       case 1:
>> +               *val = readb(addr + reg);
>> +               break;
>> +       case 2:
>> +               *val = readw(addr + reg);
>> +               break;
>> +       case 4:
>> +               *val = readl(addr + reg);
>> +               break;
>> +       }
>> +       rcu_read_unlock();
>> +
>> +       return 0;
>>   }
>>
>>   int raw_pci_write(unsigned int domain, unsigned int bus,
>>                  unsigned int devfn, int reg, int len, u32 val)
>>   {
>> -       return -EINVAL;
>> +       char __iomem *addr;
>> +
>> +       if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
>> +               return -EINVAL;
>> +
>> +       rcu_read_lock();
>> +       addr = pci_dev_base(domain, bus, devfn);
>> +       if (!addr) {
>> +               rcu_read_unlock();
>> +               return -EINVAL;
>> +       }
>> +
>> +       switch (len) {
>> +       case 1:
>> +               writeb(val, addr + reg);
>> +               break;
>> +       case 2:
>> +               writew(val, addr + reg);
>> +               break;
>> +       case 4:
>> +               writel(val, addr + reg);
>> +               break;
>> +       }
>> +       rcu_read_unlock();
>> +
>> +       return 0;
>>   }
>>
>
> These raw_pci_{read,write} functions are all ACPI specific so they can move
> into the new file as well.

Got it!

>
>
>>   #ifdef CONFIG_ACPI
>> +static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
>> +                   int size, u32 *value)
>> +{
>> +       return raw_pci_read(pci_domain_nr(bus), bus->number,
>> +                           devfn, where, size, value);
>> +}
>> +
>> +static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
>> +                    int size, u32 value)
>> +{
>> +       return raw_pci_write(pci_domain_nr(bus), bus->number,
>> +                            devfn, where, size, value);
>> +}
>> +
>> +struct pci_ops pci_root_ops = {
>> +       .read = pci_read,
>> +       .write = pci_write,
>> +};
>> +
>> +static struct pci_controller *alloc_pci_controller(int seg)
>> +{
>> +       struct pci_controller *controller;
>> +
>> +       controller = kzalloc(sizeof(*controller), GFP_KERNEL);
>> +       if (!controller)
>> +               return NULL;
>> +
>> +       controller->segment = seg;
>> +       return controller;
>> +}
>> +
>> +struct pci_root_info {
>> +       struct acpi_device *bridge;
>> +       struct pci_controller *controller;
>> +       struct list_head resources;
>> +       struct resource *res;
>> +       resource_size_t *res_offset;
>
> Why do you need to keep an array of res_offsets here? You only seem
> to be using the values once when you add the resource to the host
> bridge windows.

This is what I noticed too but did not improve that at the end. Let me 
fix that in the next ver.

>
>> +       unsigned int res_num;
>> +       char *name;
>> +};
>> +
>> +static acpi_status resource_to_window(struct acpi_resource *resource,
>> +                                     struct acpi_resource_address64 *addr)
>> +{
>> +       acpi_status status;
>> +
>> +       /*
>> +        * We're only interested in _CRS descriptors that are
>> +        *      - address space descriptors for memory
>> +        *      - non-zero size
>> +        *      - producers, i.e., the address space is routed downstream,
>> +        *        not consumed by the bridge itself
>> +        */
>> +       status = acpi_resource_to_address64(resource, addr);
>> +       if (ACPI_SUCCESS(status) &&
>> +           (addr->resource_type == ACPI_MEMORY_RANGE ||
>> +            addr->resource_type == ACPI_IO_RANGE) &&
>> +           addr->address_length &&
>> +           addr->producer_consumer == ACPI_PRODUCER)
>> +               return AE_OK;
>> +
>> +       return AE_ERROR;
>> +}
>> +
>> +static acpi_status count_window(struct acpi_resource *resource, void *data)
>> +{
>> +       unsigned int *windows = (unsigned int *) data;
>> +       struct acpi_resource_address64 addr;
>> +       acpi_status status;
>> +
>> +       status = resource_to_window(resource, &addr);
>> +       if (ACPI_SUCCESS(status))
>> +               (*windows)++;
>> +
>> +       return AE_OK;
>> +}
>> +
>> +static acpi_status add_window(struct acpi_resource *res, void *data)
>> +{
>> +       struct pci_root_info *info = data;
>> +       struct resource *resource;
>> +       struct acpi_resource_address64 addr;
>> +       acpi_status status;
>> +       unsigned long flags;
>> +       struct resource *root;
>> +       u64 start;
>> +
>> +       /* Return AE_OK for non-window resources to keep scanning for more */
>> +       status = resource_to_window(res, &addr);
>> +       if (!ACPI_SUCCESS(status))
>> +               return AE_OK;
>> +
>> +       if (addr.resource_type == ACPI_MEMORY_RANGE) {
>> +               flags = IORESOURCE_MEM;
>> +               root = &iomem_resource;
>> +       } else if (addr.resource_type == ACPI_IO_RANGE) {
>> +               flags = IORESOURCE_IO;
>> +               root = &ioport_resource;
>> +       } else
>> +               return AE_OK;
>> +
>> +       start = addr.minimum + addr.translation_offset;
>> +
>> +       resource = &info->res[info->res_num];
>> +       resource->name = info->name;
>> +       resource->flags = flags;
>> +       resource->start = start;
>> +       resource->end = resource->start + addr.address_length - 1;
>> +
>> +       if (flags & IORESOURCE_IO) {
>> +               unsigned long port;
>> +               int err;
>> +
>> +               err = pci_register_io_range(start, addr.address_length);
>> +               if (err)
>> +                       return AE_OK;
>> +
>> +               port = pci_address_to_pio(start);
>> +               if (port == (unsigned long)-1)
>> +                       return AE_OK;
>> +
>> +               resource->start = port;
>> +               resource->end = port + addr.address_length - 1;
>> +
>> +               if (pci_remap_iospace(resource, start) < 0)
>> +                       return AE_OK;
>
> You seem to leave around invalid resources every time you return in this code
> path due to your choice of ignoring error conditions.
>
> I think there are a few things that can be streamlined in this patch, but
> overall I think it looks promising.
>

Thanks Liviu!

Tomasz

Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Tomasz Nowicki]

On 07.11.2014 15:24, Arnd Bergmann wrote:
> On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
>>
>>   #ifdef CONFIG_PCI
>> +struct pci_controller {
>> +	struct acpi_device *companion;
>> +	int segment;
>> +	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
>> +};
>> +
>> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
>> +
>
> Don't use busdev->sysdata in architecture specific code, it belongs to the
> host bridge driver with the new model. For ACPI you don't have a host bridge
> driver, but it's better to keep these separate.
>
> The segment is always the same as the domain number, so just use that.
> The node and companion members here can get added to struct pci_host_bridge.

The reason why I put segment field to struct pci_controller is to 
initialize domain_nr of struct pci_bus being in pci_create_root_bus(), 
domain_nr can be used later on though. Correct me I am wrong.

Honestly I do not see the way to create root bus without e.g. 
sysdata.segment here.

>
>> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>    */
>>   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);
>>
>>   	return 0;
>>   }
>
> How do you assign the irq number with ACPI? Do you only support MSI?

I missed that, will add in next ver.

>
>>   /*
>>    * raw_pci_read/write - Platform-specific PCI config space access.
>> - *
>> - * Default empty implementation.  Replace with an architecture-specific setup
>> - * routine, if necessary.
>>    */
>>   int raw_pci_read(unsigned int domain, unsigned int bus,
>>   		  unsigned int devfn, int reg, int len, u32 *val)
>>   {
>> -	return -EINVAL;
>> +	char __iomem *addr;
>> +
>> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
>> +err:		*val = -1;
>> +		return -EINVAL;
>> +	}
>> +
>> +	rcu_read_lock();
>> +	addr = pci_dev_base(domain, bus, devfn);
>> +	if (!addr) {
>> +		rcu_read_unlock();
>> +		goto err;
>> +	}
>
> The config space accessors should probably be shared with
> drivers/pci/host/pci-host-generic.c, e.g. by moving the rest of
> the new code in there as well, or by moving the config space
> accessors from that file to drivers/pci/mmconfig.c.

Tomasz

Re: [Linaro-acpi] [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Arnd Bergmann]

On Friday 14 November 2014 15:10:12 Tomasz Nowicki wrote:
> On 07.11.2014 15:24, Arnd Bergmann wrote:
> > On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
> >>
> >>   #ifdef CONFIG_PCI
> >> +struct pci_controller {
> >> +	struct acpi_device *companion;
> >> +	int segment;
> >> +	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
> >> +};
> >> +
> >> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
> >> +
> >
> > Don't use busdev->sysdata in architecture specific code, it belongs to the
> > host bridge driver with the new model. For ACPI you don't have a host bridge
> > driver, but it's better to keep these separate.
> >
> > The segment is always the same as the domain number, so just use that.
> > The node and companion members here can get added to struct pci_host_bridge.
> 
> The reason why I put segment field to struct pci_controller is to 
> initialize domain_nr of struct pci_bus being in pci_create_root_bus(), 
> domain_nr can be used later on though. Correct me I am wrong.
> 
> Honestly I do not see the way to create root bus without e.g. 
> sysdata.segment here.

See the patches that Liviu and Lorenzo have been posting recently. This
should be straightforward in 3.19.

	Arnd

Re: [Linaro-acpi] [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Tomasz Nowicki]

On 14.11.2014 15:53, Arnd Bergmann wrote:
> On Friday 14 November 2014 15:10:12 Tomasz Nowicki wrote:
>> On 07.11.2014 15:24, Arnd Bergmann wrote:
>>> On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
>>>>
>>>>    #ifdef CONFIG_PCI
>>>> +struct pci_controller {
>>>> +	struct acpi_device *companion;
>>>> +	int segment;
>>>> +	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
>>>> +};
>>>> +
>>>> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
>>>> +
>>>
>>> Don't use busdev->sysdata in architecture specific code, it belongs to the
>>> host bridge driver with the new model. For ACPI you don't have a host bridge
>>> driver, but it's better to keep these separate.
>>>
>>> The segment is always the same as the domain number, so just use that.
>>> The node and companion members here can get added to struct pci_host_bridge.
>>
>> The reason why I put segment field to struct pci_controller is to
>> initialize domain_nr of struct pci_bus being in pci_create_root_bus(),
>> domain_nr can be used later on though. Correct me I am wrong.
>>
>> Honestly I do not see the way to create root bus without e.g.
>> sysdata.segment here.
>
> See the patches that Liviu and Lorenzo have been posting recently. This
> should be straightforward in 3.19.

Hi Arnd,

I have been looking into patch (if that what you meant) :
[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

This is not enough for me since it gets domain number from its OF parent 
or just uses sequential increased domain number. In my case, I do not 
have parent so I need to get segment from ACPI node which is not 
available there.

On the other hand, patch set posted by Wang Yijing:
[RFC PATCH 00/16] Refine PCI host bridge scan interfaces
tends to solve that issue.

What would be the best way for this patch, keep busdev->sysdata usage 
and rebase once Wang Yijing patch set will be accepted?

Regards,
Tomasz

Re: [Linaro-acpi] [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Arnd Bergmann]

On Tuesday 18 November 2014 11:17:10 Tomasz Nowicki wrote:
> On 14.11.2014 15:53, Arnd Bergmann wrote:
> > On Friday 14 November 2014 15:10:12 Tomasz Nowicki wrote:
> >> On 07.11.2014 15:24, Arnd Bergmann wrote:
> >>> On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
> >>>>
> >>>>    #ifdef CONFIG_PCI
> >>>> +struct pci_controller {
> >>>> +  struct acpi_device *companion;
> >>>> +  int segment;
> >>>> +  int node;               /* nearest node with memory or NUMA_NO_NODE for global allocation */
> >>>> +};
> >>>> +
> >>>> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
> >>>> +
> >>>
> >>> Don't use busdev->sysdata in architecture specific code, it belongs to the
> >>> host bridge driver with the new model. For ACPI you don't have a host bridge
> >>> driver, but it's better to keep these separate.
> >>>
> >>> The segment is always the same as the domain number, so just use that.
> >>> The node and companion members here can get added to struct pci_host_bridge.
> >>
> >> The reason why I put segment field to struct pci_controller is to
> >> initialize domain_nr of struct pci_bus being in pci_create_root_bus(),
> >> domain_nr can be used later on though. Correct me I am wrong.
> >>
> >> Honestly I do not see the way to create root bus without e.g.
> >> sysdata.segment here.
> >
> > See the patches that Liviu and Lorenzo have been posting recently. This
> > should be straightforward in 3.19.
> 
> Hi Arnd,
> 
> I have been looking into patch (if that what you meant) :
> [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code
> 
> This is not enough for me since it gets domain number from its OF parent 
> or just uses sequential increased domain number. In my case, I do not 
> have parent so I need to get segment from ACPI node which is not 
> available there.
> 
> On the other hand, patch set posted by Wang Yijing:
> [RFC PATCH 00/16] Refine PCI host bridge scan interfaces
> tends to solve that issue.
> 
> What would be the best way for this patch, keep busdev->sysdata usage 
> and rebase once Wang Yijing patch set will be accepted?

I think the "Refine PCI host bridge scan interfaces" series is still in an
early stage, you may need something sooner than that.

What I meant is that with "drivers: pci: move PCI domain assignment to
generic PCI code", we already have a way to put the domain number into
'struct pci_bus' and retrieve it from there without ever consulting
the sysdata pointer. You should build on top of that and do it the same
way. Of course you will have to put it in there differently, but you can
get it out the same way.

	Arnd