On Wed, Nov 19, 2014 at 05:04:47PM +0100, Tomasz Nowicki wrote:

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).

We have kind of a stew of abbreviations here: MCFG, MMCFG, MMCONFIG. In addition, the "MMCONFIG" term comes is used by ACPI but not by the PCI specs. The PCIe spec uses "ECAM" (Enhanced Configuration Access Mechanism) instead. If we're going to make this architecture-independent (which I think is a great idea), I sort of hate to expose it using terms that only mean something to ACPI folks.

So I want to moot the idea of replacing the "pci_mmcfg" and "pci_mmconfig" prefixes with something more generic, like "pci_ecam". I'm a little bit conflicted about this, because it would mean a fair amount of churn, but on the other hand, I do think good names are a huge aid to understanding.

I'm mostly concerned about the things exposed outside of x86. Things that are x86-specific or ACPI-specific could be left alone, so maybe it wouldn't be a huge change.

I noticed you made some functions __weak; I'd like to see that change split into a separate patch, with this one being basically mechanical code movement with no functional change. That way we can discuss the need for and merits of that approach separately. A mmcfg->ecam rename (if we do that) should also be its own separate patch.

Bjorn

Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Tested-by: Hanjun Guo hanjun.guo@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 | 200 +--------------------------------- 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 | 242 +++++++++++++++++++++++++++++++++++++++++ include/linux/mmconfig.h | 58 ++++++++++ include/linux/pci.h | 8 -- 11 files changed, 308 insertions(+), 235 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 ac24e1c..b397544 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); @@ -692,39 +552,6 @@ static int __init pci_mmcfg_late_insert_resources(void) */ late_initcall(pci_mmcfg_late_insert_resources); -static int __init 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) @@ -773,26 +600,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..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 c8a16e1..debacb5 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..d62dccda --- /dev/null +++ b/drivers/acpi/mmconfig.c @@ -0,0 +1,242 @@ +/*

• • 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;

+}

+int __init 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;

+}

+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) +{

• struct pci_mmcfg_region *cfg;
• acpi_table_parse(ACPI_SIG_MCFG, pci_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);
  

+} diff --git a/include/linux/mmconfig.h b/include/linux/mmconfig.h new file mode 100644 index 0000000..6ccd1ee --- /dev/null +++ b/include/linux/mmconfig.h @@ -0,0 +1,58 @@ +#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);
  

+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); +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

On Wed, Nov 19, 2014 at 05:04:48PM +0100, Tomasz Nowicki wrote:

We are going to use mmio_config_{} name convention across all architectures.

mmio_config_*() are workarounds for an AMD Fam10h defect [1]. I would prefer not to extend these names to other architectures, because they should be able to use readb()/writeb()/etc. directly, as we did on x86 before 3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h").

In fact, it would be nice if we could use readb()/writeb()/etc. on x86, and only use mmio_config_*() when we're on AMD Fam10h. Maybe there could be a "struct pci_raw_ops pci_mmcfg_amd_fam10h" that used mmio_config_*(), and we could set raw_pci_ext_ops to those ops instead of the default ones when we're on AMD Fam10h. Then x86 should be able to use the generic readb()-based ops on most platforms.

Bjorn

[1] 3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h")

Somewhere in this process, I'd like to update the AMD Fam10h comment to fix the typos and reference the spec that talks about this, i.e., the "BIOS and Kernel Developer's Guide (BKDG) For AMD Family 10h Processors," rev 3.48, sec 2.11.1, "MMIO Configuration Coding Requirements."

That can be a separate patch since it's only cosmetic.

Currently it belongs to asm/pci_x86.h header which should be included only for x86 specific files. From now on, those accessors are in asm/pci.h header which can be included in non-architecture code much easier.

Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Tested-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/x86/include/asm/pci.h | 42 +++++++++++++++++++++++++++++++++++++++++ arch/x86/include/asm/pci_x86.h | 43 ------------------------------------------ 2 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 0892ea0..5ba3720 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -71,6 +71,48 @@ void pcibios_set_master(struct pci_dev *dev); struct irq_routing_table *pcibios_get_irq_routing_table(void); int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq); +/*

• • AMD Fam10h CPUs are buggy, and cannot access MMIO config space
• • on their northbrige except through the * %eax register. As such, you MUST
• • NOT use normal IOMEM accesses, you need to only use the magic mmio-config
• • accessor functions.
• • In fact just use pci_config_*, nothing else please.
• */

+static inline unsigned char mmio_config_readb(void __iomem *pos) +{

• u8 val;
• asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline unsigned short mmio_config_readw(void __iomem *pos) +{

• u16 val;
• asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline unsigned int mmio_config_readl(void __iomem *pos) +{

• u32 val;
• asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline void mmio_config_writeb(void __iomem *pos, u8 val) +{

• asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");

+}

+static inline void mmio_config_writew(void __iomem *pos, u16 val) +{

• asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");

+}

+static inline void mmio_config_writel(void __iomem *pos, u32 val) +{

• asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");

+} #define HAVE_PCI_MMAP extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h index caba141..42e7332 100644 --- a/arch/x86/include/asm/pci_x86.h +++ b/arch/x86/include/asm/pci_x86.h @@ -121,49 +121,6 @@ extern int __init pcibios_init(void); extern int pci_legacy_init(void); extern void pcibios_fixup_irqs(void); -/*

• • AMD Fam10h CPUs are buggy, and cannot access MMIO config space
• • on their northbrige except through the * %eax register. As such, you MUST
• • NOT use normal IOMEM accesses, you need to only use the magic mmio-config
• • accessor functions.
• • In fact just use pci_config_*, nothing else please.
• */

-static inline unsigned char mmio_config_readb(void __iomem *pos) -{

• u8 val;
• asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline unsigned short mmio_config_readw(void __iomem *pos) -{

• u16 val;
• asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline unsigned int mmio_config_readl(void __iomem *pos) -{

• u32 val;
• asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline void mmio_config_writeb(void __iomem *pos, u8 val) -{

• asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

-static inline void mmio_config_writew(void __iomem *pos, u16 val) -{

• asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

-static inline void mmio_config_writel(void __iomem *pos, u32 val) -{

• asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

#ifdef CONFIG_PCI # ifdef CONFIG_ACPI

# define x86_default_pci_init pci_acpi_init

1.9.1

On 11.12.2014 00:17, Bjorn Helgaas wrote:

On Wed, Nov 19, 2014 at 05:04:48PM +0100, Tomasz Nowicki wrote:

...

We are going to use mmio_config_{} name convention across all architectures.

mmio_config_*() are workarounds for an AMD Fam10h defect [1]. I would prefer not to extend these names to other architectures, because they should be able to use readb()/writeb()/etc. directly, as we did on x86 before 3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h").

In fact, it would be nice if we could use readb()/writeb()/etc. on x86, and only use mmio_config_*() when we're on AMD Fam10h. Maybe there could be a "struct pci_raw_ops pci_mmcfg_amd_fam10h" that used mmio_config_*(), and we could set raw_pci_ext_ops to those ops instead of the default ones when we're on AMD Fam10h. Then x86 should be able to use the generic readb()-based ops on most platforms.

While I do agree we should use readb()/writeb()... methods, I am not sure there is a nice way to use mmio_config_*() exclusively for AMD Fam10h. For x86, right now, we have three PCI config accessors sets: mmconfig_32.c, mmconfig_64.c, numachip.c and each are different. So one pci_mmcfg_amd_fam10h pci_raw_ops is not enough. I am thinking of having additional structure (integrated with "struct pci_mmcfg_region") that keeps MMIO accessors where readb()/writeb() would be the default one. For AMD Fam10h case we could tweak it as necessary. What do you thing Bjorn?

Tomasz

Bjorn

[1] 3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h")

Somewhere in this process, I'd like to update the AMD Fam10h comment to fix the typos and reference the spec that talks about this, i.e., the "BIOS and Kernel Developer's Guide (BKDG) For AMD Family 10h Processors," rev 3.48, sec 2.11.1, "MMIO Configuration Coding Requirements."

That can be a separate patch since it's only cosmetic.

...

Currently it belongs to asm/pci_x86.h header which should be included only for x86 specific files. From now on, those accessors are in asm/pci.h header which can be included in non-architecture code much easier.

Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Tested-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/x86/include/asm/pci.h | 42 +++++++++++++++++++++++++++++++++++++++++ arch/x86/include/asm/pci_x86.h | 43 ------------------------------------------ 2 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 0892ea0..5ba3720 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -71,6 +71,48 @@ void pcibios_set_master(struct pci_dev *dev); struct irq_routing_table *pcibios_get_irq_routing_table(void); int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq);

+/*

• • AMD Fam10h CPUs are buggy, and cannot access MMIO config space
• • on their northbrige except through the * %eax register. As such, you MUST
• • NOT use normal IOMEM accesses, you need to only use the magic mmio-config
• • accessor functions.
• • In fact just use pci_config_*, nothing else please.
• */

+static inline unsigned char mmio_config_readb(void __iomem *pos) +{

• u8 val;
• asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline unsigned short mmio_config_readw(void __iomem *pos) +{

• u16 val;
• asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline unsigned int mmio_config_readl(void __iomem *pos) +{

• u32 val;
• asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
• return val;

+}

+static inline void mmio_config_writeb(void __iomem *pos, u8 val) +{

• asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");

+}

+static inline void mmio_config_writew(void __iomem *pos, u16 val) +{

• asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");

+}

+static inline void mmio_config_writel(void __iomem *pos, u32 val) +{

• asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");

+}

#define HAVE_PCI_MMAP extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma, diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h index caba141..42e7332 100644 --- a/arch/x86/include/asm/pci_x86.h +++ b/arch/x86/include/asm/pci_x86.h @@ -121,49 +121,6 @@ extern int __init pcibios_init(void); extern int pci_legacy_init(void); extern void pcibios_fixup_irqs(void);

-/*

• • AMD Fam10h CPUs are buggy, and cannot access MMIO config space
• • on their northbrige except through the * %eax register. As such, you MUST
• • NOT use normal IOMEM accesses, you need to only use the magic mmio-config
• • accessor functions.
• • In fact just use pci_config_*, nothing else please.
• */

-static inline unsigned char mmio_config_readb(void __iomem *pos) -{

• u8 val;
• asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline unsigned short mmio_config_readw(void __iomem *pos) -{

• u16 val;
• asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline unsigned int mmio_config_readl(void __iomem *pos) -{

• u32 val;
• asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
• return val;

-}

-static inline void mmio_config_writeb(void __iomem *pos, u8 val) -{

• asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

-static inline void mmio_config_writew(void __iomem *pos, u16 val) -{

• asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

-static inline void mmio_config_writel(void __iomem *pos, u32 val) -{

• asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");

-}

• #ifdef CONFIG_PCI # ifdef CONFIG_ACPI # define x86_default_pci_init pci_acpi_init

-- 1.9.1