[PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Gavin Shan]

For allocating resource under bus path, we do have dev pass along,
and we could just use bus instead. Also, we'd like to make function
find_pci_host_bridge() global so that some platforms (e.g. PPC) can
access the pci host bridge directly.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host-bridge.c |   34 +++++++++++++++++++++-------------
 include/linux/pci.h       |    4 ++++
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index a68dc61..4ccf477 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -9,22 +9,19 @@
 
 #include "pci.h"
 
-static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
+static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
-	struct pci_bus *bus;
-
-	bus = dev->bus;
 	while (bus->parent)
 		bus = bus->parent;
 
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
-	struct pci_bus *bus = find_pci_root_bus(dev);
+	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
-	return to_pci_host_bridge(bus->bridge);
+	return to_pci_host_bridge(root_bus->bridge);
 }
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
@@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
 	return res1->start <= res2->start && res1->end >= res2->end;
 }
 
-void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
-			     struct resource *res)
+void __pcibios_resource_to_bus(struct pci_bus *bus,
+				      struct pci_bus_region *region,
+				      struct resource *res)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 	region->start = res->start - offset;
 	region->end = res->end - offset;
 }
+void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	__pcibios_resource_to_bus(dev->bus, region, res);
+}
 EXPORT_SYMBOL(pcibios_resource_to_bus);
 
 static bool region_contains(struct pci_bus_region *region1,
@@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *region1,
 	return region1->start <= region2->start && region1->end >= region2->end;
 }
 
-void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-			     struct pci_bus_region *region)
+static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+				      struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 	res->start = region->start + offset;
 	res->end = region->end + offset;
 }
+void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	__pcibios_bus_to_resource(dev->bus, res, region);
+}
 EXPORT_SYMBOL(pcibios_bus_to_resource);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fefb4e1..2b559f1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -385,6 +385,7 @@ struct pci_host_bridge {
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 		     void (*release_fn)(struct pci_host_bridge *),
 		     void *release_data);
@@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
 
 /* Generic PCI functions used internally */
 
+void __pcibios_resource_to_bus(struct pci_bus *bus,
+			       struct pci_bus_region *region,
+			       struct resource *res);
 void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 			     struct resource *res);
 void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-- 
1.7.9.5

[PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Gavin Shan]

For allocating resource under bus path, we do have dev pass along,
and we could just use bus instead. Also, we'd like to make function
find_pci_host_bridge() global so that some platforms (e.g. PPC) can
access the pci host bridge directly.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/host-bridge.c |   34 +++++++++++++++++++++-------------
 include/linux/pci.h       |    4 ++++
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index a68dc61..4ccf477 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -9,22 +9,19 @@
 
 #include "pci.h"
 
-static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
+static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
-	struct pci_bus *bus;
-
-	bus = dev->bus;
 	while (bus->parent)
 		bus = bus->parent;
 
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
-	struct pci_bus *bus = find_pci_root_bus(dev);
+	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
-	return to_pci_host_bridge(bus->bridge);
+	return to_pci_host_bridge(root_bus->bridge);
 }
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
@@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
 	return res1->start <= res2->start && res1->end >= res2->end;
 }
 
-void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
-			     struct resource *res)
+void __pcibios_resource_to_bus(struct pci_bus *bus,
+				      struct pci_bus_region *region,
+				      struct resource *res)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 	region->start = res->start - offset;
 	region->end = res->end - offset;
 }
+void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	__pcibios_resource_to_bus(dev->bus, region, res);
+}
 EXPORT_SYMBOL(pcibios_resource_to_bus);
 
 static bool region_contains(struct pci_bus_region *region1,
@@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *region1,
 	return region1->start <= region2->start && region1->end >= region2->end;
 }
 
-void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-			     struct pci_bus_region *region)
+static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+				      struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
 	struct pci_host_bridge_window *window;
 	resource_size_t offset = 0;
 
@@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
 	res->start = region->start + offset;
 	res->end = region->end + offset;
 }
+void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	__pcibios_bus_to_resource(dev->bus, res, region);
+}
 EXPORT_SYMBOL(pcibios_bus_to_resource);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fefb4e1..2b559f1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -385,6 +385,7 @@ struct pci_host_bridge {
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 		     void (*release_fn)(struct pci_host_bridge *),
 		     void *release_data);
@@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
 
 /* Generic PCI functions used internally */
 
+void __pcibios_resource_to_bus(struct pci_bus *bus,
+			       struct pci_bus_region *region,
+			       struct resource *res);
 void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
 			     struct resource *res);
 void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
-- 
1.7.9.5

[PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Gavin Shan]

On some powerpc platforms, device BARs need to be assigned to separate
"segments" of the address space in order for the error isolation and HW
virtualization mechanisms (EEH) to work properly. Those "segments" have
a minimum size that can be fairly large (16M). In order to be able to
use the generic resource assignment code rather than re-inventing our
own, we chose to group devices by bus. That way, a simple change of the
minimum alignment requirements of resources assigned to PCI to PCI (P2P)
bridges is enough to ensure that all BARs for devices below those bridges
will fit into contiguous sets of segments and there will be no overlap.

This patch provides a way for the host bridge to override the default
alignment values used by the resource allocation code for that purpose.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/probe.c     |    5 +++++
 drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
 include/linux/pci.h     |    8 ++++++++
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 658ac97..a196529 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 	if (bridge) {
 		INIT_LIST_HEAD(&bridge->windows);
 		bridge->bus = b;
+
+		/* Set minimal alignment shift of P2P bridges */
+		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
+		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
+		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
 	}
 
 	return bridge;
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8fa2d4b..caebe98 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
 static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		resource_size_t add_size, struct list_head *realloc_head)
 {
+	struct pci_host_bridge *phb;
 	struct pci_dev *dev;
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	unsigned long size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
+	resource_size_t io_align;
 
 	if (!b_res)
  		return;
@@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 				children_add_size += get_res_add_size(realloc_head, r);
 		}
 	}
+
+	phb = find_pci_host_bridge(bus);
+	io_align = (1 << phb->io_align_shift);
+
 	size0 = calculate_iosize(size, min_size, size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
 	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
 		calculate_iosize(size, min_size, add_size + size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "
@@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		return;
 	}
 	/* Alignment of the IO window is always 4K */
-	b_res->start = 4096;
+	b_res->start = io_align;
 	b_res->end = b_res->start + size0 - 1;
 	b_res->flags |= IORESOURCE_STARTALIGN;
 	if (size1 > size0 && realloc_head) {
-		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
+		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
 		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
 				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
 				 bus->secondary, bus->subordinate, size1-size0);
@@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			resource_size_t add_size,
 			struct list_head *realloc_head)
 {
+	struct pci_host_bridge *phb;
 	struct pci_dev *dev;
 	resource_size_t min_align, align, size, size0, size1;
 	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
@@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	struct resource *b_res = find_free_bus_resource(bus, type);
 	unsigned int mem64_mask = 0;
 	resource_size_t children_add_size = 0;
+	int mem_align_shift;
 
 	if (!b_res)
 		return 0;
 
+	phb = find_pci_host_bridge(bus);
+	if (type & IORESOURCE_PREFETCH)
+		mem_align_shift = phb->pmem_align_shift;
+	else
+		mem_align_shift = phb->mem_align_shift;
+
 	memset(aligns, 0, sizeof(aligns));
 	max_order = 0;
 	size = 0;
@@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 #endif
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
-			order = __ffs(align) - 20;
-			if (order > 11) {
+			order = __ffs(align) - mem_align_shift;
+			if (order > (11 - (mem_align_shift - 20))) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);
@@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	for (order = 0; order <= max_order; order++) {
 		resource_size_t align1 = 1;
 
-		align1 <<= (order + 20);
+		align1 <<= (order + mem_align_shift);
 
 		if (!align)
 			min_align = align1;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2b559f1..879de4e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,17 @@ struct pci_host_bridge_window {
 	resource_size_t offset;		/* bus address + offset = CPU address */
 };
 
+/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
+#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
+#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
+#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
+
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
+	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
+	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
-- 
1.7.9.5

[PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Gavin Shan]

On some powerpc platforms, device BARs need to be assigned to separate
"segments" of the address space in order for the error isolation and HW
virtualization mechanisms (EEH) to work properly. Those "segments" have
a minimum size that can be fairly large (16M). In order to be able to
use the generic resource assignment code rather than re-inventing our
own, we chose to group devices by bus. That way, a simple change of the
minimum alignment requirements of resources assigned to PCI to PCI (P2P)
bridges is enough to ensure that all BARs for devices below those bridges
will fit into contiguous sets of segments and there will be no overlap.

This patch provides a way for the host bridge to override the default
alignment values used by the resource allocation code for that purpose.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/probe.c     |    5 +++++
 drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
 include/linux/pci.h     |    8 ++++++++
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 658ac97..a196529 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 	if (bridge) {
 		INIT_LIST_HEAD(&bridge->windows);
 		bridge->bus = b;
+
+		/* Set minimal alignment shift of P2P bridges */
+		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
+		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
+		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
 	}
 
 	return bridge;
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8fa2d4b..caebe98 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
 static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		resource_size_t add_size, struct list_head *realloc_head)
 {
+	struct pci_host_bridge *phb;
 	struct pci_dev *dev;
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	unsigned long size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
+	resource_size_t io_align;
 
 	if (!b_res)
  		return;
@@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 				children_add_size += get_res_add_size(realloc_head, r);
 		}
 	}
+
+	phb = find_pci_host_bridge(bus);
+	io_align = (1 << phb->io_align_shift);
+
 	size0 = calculate_iosize(size, min_size, size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
 	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
 		calculate_iosize(size, min_size, add_size + size1,
-			resource_size(b_res), 4096);
+			resource_size(b_res), io_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "
@@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		return;
 	}
 	/* Alignment of the IO window is always 4K */
-	b_res->start = 4096;
+	b_res->start = io_align;
 	b_res->end = b_res->start + size0 - 1;
 	b_res->flags |= IORESOURCE_STARTALIGN;
 	if (size1 > size0 && realloc_head) {
-		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
+		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
 		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
 				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
 				 bus->secondary, bus->subordinate, size1-size0);
@@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			resource_size_t add_size,
 			struct list_head *realloc_head)
 {
+	struct pci_host_bridge *phb;
 	struct pci_dev *dev;
 	resource_size_t min_align, align, size, size0, size1;
 	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
@@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	struct resource *b_res = find_free_bus_resource(bus, type);
 	unsigned int mem64_mask = 0;
 	resource_size_t children_add_size = 0;
+	int mem_align_shift;
 
 	if (!b_res)
 		return 0;
 
+	phb = find_pci_host_bridge(bus);
+	if (type & IORESOURCE_PREFETCH)
+		mem_align_shift = phb->pmem_align_shift;
+	else
+		mem_align_shift = phb->mem_align_shift;
+
 	memset(aligns, 0, sizeof(aligns));
 	max_order = 0;
 	size = 0;
@@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 #endif
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
-			order = __ffs(align) - 20;
-			if (order > 11) {
+			order = __ffs(align) - mem_align_shift;
+			if (order > (11 - (mem_align_shift - 20))) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);
@@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	for (order = 0; order <= max_order; order++) {
 		resource_size_t align1 = 1;
 
-		align1 <<= (order + 20);
+		align1 <<= (order + mem_align_shift);
 
 		if (!align)
 			min_align = align1;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2b559f1..879de4e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,17 @@ struct pci_host_bridge_window {
 	resource_size_t offset;		/* bus address + offset = CPU address */
 };
 
+/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
+#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
+#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
+#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
+
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
+	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
+	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
-- 
1.7.9.5

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Bjorn Helgaas]

On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> For allocating resource under bus path, we do have dev pass along,
> and we could just use bus instead. Also, we'd like to make function
> find_pci_host_bridge() global so that some platforms (e.g. PPC) can
> access the pci host bridge directly.

This patch appears to have multiple unrelated changes:

  - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
  - change find_pci_host_bridge() argument from dev to bus
  - fiddle with pcibios_bus_to_resource() and pcibios_resource_to_bus()

These should be split out to make your patches easier to review.

What's the rationale for preferring the pci_bus over the pci_dev?

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/host-bridge.c |   34 +++++++++++++++++++++-------------
>  include/linux/pci.h       |    4 ++++
>  2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index a68dc61..4ccf477 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -9,22 +9,19 @@
>
>  #include "pci.h"
>
> -static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
> +static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>  {
> -       struct pci_bus *bus;
> -
> -       bus = dev->bus;
>        while (bus->parent)
>                bus = bus->parent;
>
>        return bus;
>  }
>
> -static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
>  {
> -       struct pci_bus *bus = find_pci_root_bus(dev);
> +       struct pci_bus *root_bus = find_pci_root_bus(bus);
>
> -       return to_pci_host_bridge(bus->bridge);
> +       return to_pci_host_bridge(root_bus->bridge);
>  }
>
>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
> @@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
>        return res1->start <= res2->start && res1->end >= res2->end;
>  }
>
> -void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
> -                            struct resource *res)
> +void __pcibios_resource_to_bus(struct pci_bus *bus,
> +                                     struct pci_bus_region *region,
> +                                     struct resource *res)
>  {
> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>        struct pci_host_bridge_window *window;
>        resource_size_t offset = 0;
>
> @@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>        region->start = res->start - offset;
>        region->end = res->end - offset;
>  }
> +void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
> +                            struct resource *res)
> +{
> +       __pcibios_resource_to_bus(dev->bus, region, res);
> +}
>  EXPORT_SYMBOL(pcibios_resource_to_bus);
>
>  static bool region_contains(struct pci_bus_region *region1,
> @@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *region1,
>        return region1->start <= region2->start && region1->end >= region2->end;
>  }
>
> -void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> -                            struct pci_bus_region *region)
> +static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> +                                     struct pci_bus_region *region)
>  {
> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>        struct pci_host_bridge_window *window;
>        resource_size_t offset = 0;
>
> @@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>        res->start = region->start + offset;
>        res->end = region->end + offset;
>  }
> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> +                            struct pci_bus_region *region)
> +{
> +       __pcibios_bus_to_resource(dev->bus, res, region);
> +}
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fefb4e1..2b559f1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -385,6 +385,7 @@ struct pci_host_bridge {
>  };
>
>  #define        to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>                     void (*release_fn)(struct pci_host_bridge *),
>                     void *release_data);
> @@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
>
>  /* Generic PCI functions used internally */
>
> +void __pcibios_resource_to_bus(struct pci_bus *bus,
> +                              struct pci_bus_region *region,
> +                              struct resource *res);
>  void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>                             struct resource *res);
>  void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> --
> 1.7.9.5
>

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Bjorn Helgaas]

On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wro=
te:
> For allocating resource under bus path, we do have dev pass along,
> and we could just use bus instead. Also, we'd like to make function
> find_pci_host_bridge() global so that some platforms (e.g. PPC) can
> access the pci host bridge directly.

This patch appears to have multiple unrelated changes:

  - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
  - change find_pci_host_bridge() argument from dev to bus
  - fiddle with pcibios_bus_to_resource() and pcibios_resource_to_bus()

These should be split out to make your patches easier to review.

What's the rationale for preferring the pci_bus over the pci_dev?

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
> =A0drivers/pci/host-bridge.c | =A0 34 +++++++++++++++++++++-------------
> =A0include/linux/pci.h =A0 =A0 =A0 | =A0 =A04 ++++
> =A02 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index a68dc61..4ccf477 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -9,22 +9,19 @@
>
> =A0#include "pci.h"
>
> -static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
> +static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> =A0{
> - =A0 =A0 =A0 struct pci_bus *bus;
> -
> - =A0 =A0 =A0 bus =3D dev->bus;
> =A0 =A0 =A0 =A0while (bus->parent)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bus =3D bus->parent;
>
> =A0 =A0 =A0 =A0return bus;
> =A0}
>
> -static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
> =A0{
> - =A0 =A0 =A0 struct pci_bus *bus =3D find_pci_root_bus(dev);
> + =A0 =A0 =A0 struct pci_bus *root_bus =3D find_pci_root_bus(bus);
>
> - =A0 =A0 =A0 return to_pci_host_bridge(bus->bridge);
> + =A0 =A0 =A0 return to_pci_host_bridge(root_bus->bridge);
> =A0}
>
> =A0void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
> @@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, =
struct resource *res2)
> =A0 =A0 =A0 =A0return res1->start <=3D res2->start && res1->end >=3D res2=
->end;
> =A0}
>
> -void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region =
*region,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct resource =
*res)
> +void __pcibios_resource_to_bus(struct pci_bus *bus,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 struct pci_bus_region *region,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 struct resource *res)
> =A0{
> - =A0 =A0 =A0 struct pci_host_bridge *bridge =3D find_pci_host_bridge(dev=
);
> + =A0 =A0 =A0 struct pci_host_bridge *bridge =3D find_pci_host_bridge(bus=
);
> =A0 =A0 =A0 =A0struct pci_host_bridge_window *window;
> =A0 =A0 =A0 =A0resource_size_t offset =3D 0;
>
> @@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, stru=
ct pci_bus_region *region,
> =A0 =A0 =A0 =A0region->start =3D res->start - offset;
> =A0 =A0 =A0 =A0region->end =3D res->end - offset;
> =A0}
> +void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region =
*region,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct resource =
*res)
> +{
> + =A0 =A0 =A0 __pcibios_resource_to_bus(dev->bus, region, res);
> +}
> =A0EXPORT_SYMBOL(pcibios_resource_to_bus);
>
> =A0static bool region_contains(struct pci_bus_region *region1,
> @@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *re=
gion1,
> =A0 =A0 =A0 =A0return region1->start <=3D region2->start && region1->end =
>=3D region2->end;
> =A0}
>
> -void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct pci_bus_r=
egion *region)
> +static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resour=
ce *res,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 struct pci_bus_region *region)
> =A0{
> - =A0 =A0 =A0 struct pci_host_bridge *bridge =3D find_pci_host_bridge(dev=
);
> + =A0 =A0 =A0 struct pci_host_bridge *bridge =3D find_pci_host_bridge(bus=
);
> =A0 =A0 =A0 =A0struct pci_host_bridge_window *window;
> =A0 =A0 =A0 =A0resource_size_t offset =3D 0;
>
> @@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struc=
t resource *res,
> =A0 =A0 =A0 =A0res->start =3D region->start + offset;
> =A0 =A0 =A0 =A0res->end =3D region->end + offset;
> =A0}
> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct pci_bus_r=
egion *region)
> +{
> + =A0 =A0 =A0 __pcibios_bus_to_resource(dev->bus, res, region);
> +}
> =A0EXPORT_SYMBOL(pcibios_bus_to_resource);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fefb4e1..2b559f1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -385,6 +385,7 @@ struct pci_host_bridge {
> =A0};
>
> =A0#define =A0 =A0 =A0 =A0to_pci_host_bridge(n) container_of(n, struct pc=
i_host_bridge, dev)
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
> =A0void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 void (*release_fn)(struct pci_hos=
t_bridge *),
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 void *release_data);
> @@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
>
> =A0/* Generic PCI functions used internally */
>
> +void __pcibios_resource_to_bus(struct pci_bus *bus,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct pci_b=
us_region *region,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct resou=
rce *res);
> =A0void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_regio=
n *region,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct resource *=
res);
> =A0void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res=
,
> --
> 1.7.9.5
>

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Bjorn Helgaas]

On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> On some powerpc platforms, device BARs need to be assigned to separate
> "segments" of the address space in order for the error isolation and HW
> virtualization mechanisms (EEH) to work properly. Those "segments" have
> a minimum size that can be fairly large (16M). In order to be able to
> use the generic resource assignment code rather than re-inventing our
> own, we chose to group devices by bus. That way, a simple change of the
> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> bridges is enough to ensure that all BARs for devices below those bridges
> will fit into contiguous sets of segments and there will be no overlap.

Is this something that is currently broken on powerpc?  I don't see
any corresponding powerpc change, like a removal of whatever the
previous way of doing this was.

I'm not sure this is generic enough to warrant putting it in the core
code (though I don't know whether we have any pcibios_*() hooks that
would allow us to do it in the arch).

> This patch provides a way for the host bridge to override the default
> alignment values used by the resource allocation code for that purpose.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/probe.c     |    5 +++++
>  drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
>  include/linux/pci.h     |    8 ++++++++
>  3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 658ac97..a196529 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>        if (bridge) {
>                INIT_LIST_HEAD(&bridge->windows);
>                bridge->bus = b;
> +
> +               /* Set minimal alignment shift of P2P bridges */
> +               bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> +               bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> +               bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
>        }
>
>        return bridge;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..caebe98 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
>  static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                resource_size_t add_size, struct list_head *realloc_head)
>  {
> +       struct pci_host_bridge *phb;
>        struct pci_dev *dev;
>        struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>        unsigned long size = 0, size0 = 0, size1 = 0;
>        resource_size_t children_add_size = 0;
> +       resource_size_t io_align;
>
>        if (!b_res)
>                return;
> @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                                children_add_size += get_res_add_size(realloc_head, r);
>                }
>        }
> +
> +       phb = find_pci_host_bridge(bus);

I guess this explains why you want find_pci_host_bridge() to take a
pci_bus, not a pci_dev..

> +       io_align = (1 << phb->io_align_shift);
> +
>        size0 = calculate_iosize(size, min_size, size1,
> -                       resource_size(b_res), 4096);
> +                       resource_size(b_res), io_align);
>        if (children_add_size > add_size)
>                add_size = children_add_size;
>        size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
>                calculate_iosize(size, min_size, add_size + size1,
> -                       resource_size(b_res), 4096);
> +                       resource_size(b_res), io_align);
>        if (!size0 && !size1) {
>                if (b_res->start || b_res->end)
>                        dev_info(&bus->self->dev, "disabling bridge window "
> @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                return;
>        }
>        /* Alignment of the IO window is always 4K */
> -       b_res->start = 4096;
> +       b_res->start = io_align;

This looks like something that will collide with the changes in the
pipe to support I/O windows smaller than 4K.

>        b_res->end = b_res->start + size0 - 1;
>        b_res->flags |= IORESOURCE_STARTALIGN;
>        if (size1 > size0 && realloc_head) {
> -               add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> +               add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
>                dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
>                                 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
>                                 bus->secondary, bus->subordinate, size1-size0);
> @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>                        resource_size_t add_size,
>                        struct list_head *realloc_head)
>  {
> +       struct pci_host_bridge *phb;
>        struct pci_dev *dev;
>        resource_size_t min_align, align, size, size0, size1;
>        resource_size_t aligns[12];     /* Alignments from 1Mb to 2Gb */
> @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>        struct resource *b_res = find_free_bus_resource(bus, type);
>        unsigned int mem64_mask = 0;
>        resource_size_t children_add_size = 0;
> +       int mem_align_shift;
>
>        if (!b_res)
>                return 0;
>
> +       phb = find_pci_host_bridge(bus);
> +       if (type & IORESOURCE_PREFETCH)
> +               mem_align_shift = phb->pmem_align_shift;
> +       else
> +               mem_align_shift = phb->mem_align_shift;
> +
>        memset(aligns, 0, sizeof(aligns));
>        max_order = 0;
>        size = 0;
> @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  #endif
>                        /* For bridges size != alignment */
>                        align = pci_resource_alignment(dev, r);
> -                       order = __ffs(align) - 20;
> -                       if (order > 11) {
> +                       order = __ffs(align) - mem_align_shift;
> +                       if (order > (11 - (mem_align_shift - 20))) {
>                                dev_warn(&dev->dev, "disabling BAR %d: %pR "
>                                         "(bad alignment %#llx)\n", i, r,
>                                         (unsigned long long) align);
> @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>        for (order = 0; order <= max_order; order++) {
>                resource_size_t align1 = 1;
>
> -               align1 <<= (order + 20);
> +               align1 <<= (order + mem_align_shift);

This code must encode somewhere the assumption that mem windows must
be at least 1MB aligned.  Maybe it has something to do with the "20"
constants above.  Independent of your patch, it'd be nice to make this
more explicit.

>
>                if (!align)
>                        min_align = align1;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2b559f1..879de4e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
>        resource_size_t offset;         /* bus address + offset = CPU address */
>  };
>
> +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */

"Default shifts"

> +#define PCI_DEFAULT_IO_ALIGN_SHIFT     12      /* 4KB  */
> +#define PCI_DEFAULT_MEM_ALIGN_SHIFT    20      /* 1MB  */
> +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT   20      /* 1MB */
> +
>  struct pci_host_bridge {
>        struct device dev;
>        struct pci_bus *bus;            /* root bus */
> +       int io_align_shift;             /* P2P I/O bar minimal alignment shift  */
> +       int mem_align_shift;            /* P2P MMIO bar minimal alignment shift */
> +       int pmem_align_shift;           /* P2P prefetchable MMIO bar minimal alignment shift */
>        struct list_head windows;       /* pci_host_bridge_windows */
>        void (*release_fn)(struct pci_host_bridge *);
>        void *release_data;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Bjorn Helgaas]

On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wro=
te:
> On some powerpc platforms, device BARs need to be assigned to separate
> "segments" of the address space in order for the error isolation and HW
> virtualization mechanisms (EEH) to work properly. Those "segments" have
> a minimum size that can be fairly large (16M). In order to be able to
> use the generic resource assignment code rather than re-inventing our
> own, we chose to group devices by bus. That way, a simple change of the
> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> bridges is enough to ensure that all BARs for devices below those bridges
> will fit into contiguous sets of segments and there will be no overlap.

Is this something that is currently broken on powerpc?  I don't see
any corresponding powerpc change, like a removal of whatever the
previous way of doing this was.

I'm not sure this is generic enough to warrant putting it in the core
code (though I don't know whether we have any pcibios_*() hooks that
would allow us to do it in the arch).

> This patch provides a way for the host bridge to override the default
> alignment values used by the resource allocation code for that purpose.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> ---
> =A0drivers/pci/probe.c =A0 =A0 | =A0 =A05 +++++
> =A0drivers/pci/setup-bus.c | =A0 28 +++++++++++++++++++++-------
> =A0include/linux/pci.h =A0 =A0 | =A0 =A08 ++++++++
> =A03 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 658ac97..a196529 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge=
(struct pci_bus *b)
> =A0 =A0 =A0 =A0if (bridge) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0INIT_LIST_HEAD(&bridge->windows);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bridge->bus =3D b;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Set minimal alignment shift of P2P bridg=
es */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bridge->io_align_shift =3D PCI_DEFAULT_IO_A=
LIGN_SHIFT;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bridge->mem_align_shift =3D PCI_DEFAULT_MEM=
_ALIGN_SHIFT;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bridge->pmem_align_shift =3D PCI_DEFAULT_PM=
EM_ALIGN_SHIFT;
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0return bridge;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..caebe98 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_s=
ize_t size,
> =A0static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size=
,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0resource_size_t add_size, struct list_head=
 *realloc_head)
> =A0{
> + =A0 =A0 =A0 struct pci_host_bridge *phb;
> =A0 =A0 =A0 =A0struct pci_dev *dev;
> =A0 =A0 =A0 =A0struct resource *b_res =3D find_free_bus_resource(bus, IOR=
ESOURCE_IO);
> =A0 =A0 =A0 =A0unsigned long size =3D 0, size0 =3D 0, size1 =3D 0;
> =A0 =A0 =A0 =A0resource_size_t children_add_size =3D 0;
> + =A0 =A0 =A0 resource_size_t io_align;
>
> =A0 =A0 =A0 =A0if (!b_res)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
> @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resou=
rce_size_t min_size,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0children_a=
dd_size +=3D get_res_add_size(realloc_head, r);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
> +
> + =A0 =A0 =A0 phb =3D find_pci_host_bridge(bus);

I guess this explains why you want find_pci_host_bridge() to take a
pci_bus, not a pci_dev..

> + =A0 =A0 =A0 io_align =3D (1 << phb->io_align_shift);
> +
> =A0 =A0 =A0 =A0size0 =3D calculate_iosize(size, min_size, size1,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 resource_size(b_res), 4096)=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 resource_size(b_res), io_al=
ign);
> =A0 =A0 =A0 =A0if (children_add_size > add_size)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0add_size =3D children_add_size;
> =A0 =A0 =A0 =A0size1 =3D (!realloc_head || (realloc_head && !add_size)) ?=
 size0 :
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0calculate_iosize(size, min_size, add_size =
+ size1,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 resource_size(b_res), 4096)=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 resource_size(b_res), io_al=
ign);
> =A0 =A0 =A0 =A0if (!size0 && !size1) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (b_res->start || b_res->end)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_info(&bus->self->dev, =
"disabling bridge window "
> @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resou=
rce_size_t min_size,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Alignment of the IO window is always 4K */
> - =A0 =A0 =A0 b_res->start =3D 4096;
> + =A0 =A0 =A0 b_res->start =3D io_align;

This looks like something that will collide with the changes in the
pipe to support I/O windows smaller than 4K.

> =A0 =A0 =A0 =A0b_res->end =3D b_res->start + size0 - 1;
> =A0 =A0 =A0 =A0b_res->flags |=3D IORESOURCE_STARTALIGN;
> =A0 =A0 =A0 =A0if (size1 > size0 && realloc_head) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 add_to_list(realloc_head, bus->self, b_res,=
 size1-size0, 4096);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 add_to_list(realloc_head, bus->self, b_res,=
 size1-size0, io_align);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_printk(KERN_DEBUG, &bus->self->dev, "b=
ridge window "
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "%pR to [=
bus %02x-%02x] add_size %lx\n", b_res,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bus->seco=
ndary, bus->subordinate, size1-size0);
> @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigne=
d long mask,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0resource_size_t add_size,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct list_head *realloc_=
head)
> =A0{
> + =A0 =A0 =A0 struct pci_host_bridge *phb;
> =A0 =A0 =A0 =A0struct pci_dev *dev;
> =A0 =A0 =A0 =A0resource_size_t min_align, align, size, size0, size1;
> =A0 =A0 =A0 =A0resource_size_t aligns[12]; =A0 =A0 /* Alignments from 1Mb=
 to 2Gb */
> @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsig=
ned long mask,
> =A0 =A0 =A0 =A0struct resource *b_res =3D find_free_bus_resource(bus, typ=
e);
> =A0 =A0 =A0 =A0unsigned int mem64_mask =3D 0;
> =A0 =A0 =A0 =A0resource_size_t children_add_size =3D 0;
> + =A0 =A0 =A0 int mem_align_shift;
>
> =A0 =A0 =A0 =A0if (!b_res)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0;
>
> + =A0 =A0 =A0 phb =3D find_pci_host_bridge(bus);
> + =A0 =A0 =A0 if (type & IORESOURCE_PREFETCH)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mem_align_shift =3D phb->pmem_align_shift;
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mem_align_shift =3D phb->mem_align_shift;
> +
> =A0 =A0 =A0 =A0memset(aligns, 0, sizeof(aligns));
> =A0 =A0 =A0 =A0max_order =3D 0;
> =A0 =A0 =A0 =A0size =3D 0;
> @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigne=
d long mask,
> =A0#endif
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* For bridges size !=3D a=
lignment */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0align =3D pci_resource_ali=
gnment(dev, r);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 order =3D __ffs(align) - 20=
;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (order > 11) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 order =3D __ffs(align) - me=
m_align_shift;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (order > (11 - (mem_alig=
n_shift - 20))) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_warn(&=
dev->dev, "disabling BAR %d: %pR "
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 "(bad alignment %#llx)\n", i, r,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 (unsigned long long) align);
> @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigne=
d long mask,
> =A0 =A0 =A0 =A0for (order =3D 0; order <=3D max_order; order++) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0resource_size_t align1 =3D 1;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 align1 <<=3D (order + 20);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 align1 <<=3D (order + mem_align_shift);

This code must encode somewhere the assumption that mem windows must
be at least 1MB aligned.  Maybe it has something to do with the "20"
constants above.  Independent of your patch, it'd be nice to make this
more explicit.

>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!align)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0min_align =3D align1;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2b559f1..879de4e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
> =A0 =A0 =A0 =A0resource_size_t offset; =A0 =A0 =A0 =A0 /* bus address + o=
ffset =3D CPU address */
> =A0};
>
> +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */

"Default shifts"

> +#define PCI_DEFAULT_IO_ALIGN_SHIFT =A0 =A0 12 =A0 =A0 =A0/* 4KB =A0*/
> +#define PCI_DEFAULT_MEM_ALIGN_SHIFT =A0 =A020 =A0 =A0 =A0/* 1MB =A0*/
> +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT =A0 20 =A0 =A0 =A0/* 1MB */
> +
> =A0struct pci_host_bridge {
> =A0 =A0 =A0 =A0struct device dev;
> =A0 =A0 =A0 =A0struct pci_bus *bus; =A0 =A0 =A0 =A0 =A0 =A0/* root bus */
> + =A0 =A0 =A0 int io_align_shift; =A0 =A0 =A0 =A0 =A0 =A0 /* P2P I/O bar =
minimal alignment shift =A0*/
> + =A0 =A0 =A0 int mem_align_shift; =A0 =A0 =A0 =A0 =A0 =A0/* P2P MMIO bar=
 minimal alignment shift */
> + =A0 =A0 =A0 int pmem_align_shift; =A0 =A0 =A0 =A0 =A0 /* P2P prefetchab=
le MMIO bar minimal alignment shift */
> =A0 =A0 =A0 =A0struct list_head windows; =A0 =A0 =A0 /* pci_host_bridge_w=
indows */
> =A0 =A0 =A0 =A0void (*release_fn)(struct pci_host_bridge *);
> =A0 =A0 =A0 =A0void *release_data;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Benjamin Herrenschmidt]

On Wed, 2012-06-27 at 12:48 -0600, Bjorn Helgaas wrote:
> On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> > On some powerpc platforms, device BARs need to be assigned to separate
> > "segments" of the address space in order for the error isolation and HW
> > virtualization mechanisms (EEH) to work properly. Those "segments" have
> > a minimum size that can be fairly large (16M). In order to be able to
> > use the generic resource assignment code rather than re-inventing our
> > own, we chose to group devices by bus. That way, a simple change of the
> > minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> > bridges is enough to ensure that all BARs for devices below those bridges
> > will fit into contiguous sets of segments and there will be no overlap.
> 
> Is this something that is currently broken on powerpc?  I don't see
> any corresponding powerpc change, like a removal of whatever the
> previous way of doing this was.

Subsequent patch. The goal is to get rid of the bulk of the custom
resource allocation code in arch/powerpc/platforms/powernv/pci-ioda.c
where we basically re-implement it all (well I did) but without handling
all the special & corner cases that the generic code does.

The root of the problem is the need to segment the MMIO and IO spaces
based on a somewhat fixed (HW driven) segment size and have devices
isolated in their own groups of segments.

This is related to our "EEH" advanced error handling scheme which among
other things can properly connect errors triggered by MMIO (target
aborts, UE responses, etc...) to the actual device or driver.

> I'm not sure this is generic enough to warrant putting it in the core
> code (though I don't know whether we have any pcibios_*() hooks that
> would allow us to do it in the arch).

We don't have such hooks. This is the less invasive approach as far as I
can tell.

Cheers,
Ben.

> > This patch provides a way for the host bridge to override the default
> > alignment values used by the resource allocation code for that purpose.
> >
> > Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> > Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> > Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> > ---
> >  drivers/pci/probe.c     |    5 +++++
> >  drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
> >  include/linux/pci.h     |    8 ++++++++
> >  3 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 658ac97..a196529 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> >        if (bridge) {
> >                INIT_LIST_HEAD(&bridge->windows);
> >                bridge->bus = b;
> > +
> > +               /* Set minimal alignment shift of P2P bridges */
> > +               bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> > +               bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> > +               bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
> >        }
> >
> >        return bridge;
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 8fa2d4b..caebe98 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
> >  static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                resource_size_t add_size, struct list_head *realloc_head)
> >  {
> > +       struct pci_host_bridge *phb;
> >        struct pci_dev *dev;
> >        struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> >        unsigned long size = 0, size0 = 0, size1 = 0;
> >        resource_size_t children_add_size = 0;
> > +       resource_size_t io_align;
> >
> >        if (!b_res)
> >                return;
> > @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                                children_add_size += get_res_add_size(realloc_head, r);
> >                }
> >        }
> > +
> > +       phb = find_pci_host_bridge(bus);
> 
> I guess this explains why you want find_pci_host_bridge() to take a
> pci_bus, not a pci_dev..
> 
> > +       io_align = (1 << phb->io_align_shift);
> > +
> >        size0 = calculate_iosize(size, min_size, size1,
> > -                       resource_size(b_res), 4096);
> > +                       resource_size(b_res), io_align);
> >        if (children_add_size > add_size)
> >                add_size = children_add_size;
> >        size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
> >                calculate_iosize(size, min_size, add_size + size1,
> > -                       resource_size(b_res), 4096);
> > +                       resource_size(b_res), io_align);
> >        if (!size0 && !size1) {
> >                if (b_res->start || b_res->end)
> >                        dev_info(&bus->self->dev, "disabling bridge window "
> > @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                return;
> >        }
> >        /* Alignment of the IO window is always 4K */
> > -       b_res->start = 4096;
> > +       b_res->start = io_align;
> 
> This looks like something that will collide with the changes in the
> pipe to support I/O windows smaller than 4K.
> 
> >        b_res->end = b_res->start + size0 - 1;
> >        b_res->flags |= IORESOURCE_STARTALIGN;
> >        if (size1 > size0 && realloc_head) {
> > -               add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> > +               add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
> >                dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
> >                                 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
> >                                 bus->secondary, bus->subordinate, size1-size0);
> > @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >                        resource_size_t add_size,
> >                        struct list_head *realloc_head)
> >  {
> > +       struct pci_host_bridge *phb;
> >        struct pci_dev *dev;
> >        resource_size_t min_align, align, size, size0, size1;
> >        resource_size_t aligns[12];     /* Alignments from 1Mb to 2Gb */
> > @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >        struct resource *b_res = find_free_bus_resource(bus, type);
> >        unsigned int mem64_mask = 0;
> >        resource_size_t children_add_size = 0;
> > +       int mem_align_shift;
> >
> >        if (!b_res)
> >                return 0;
> >
> > +       phb = find_pci_host_bridge(bus);
> > +       if (type & IORESOURCE_PREFETCH)
> > +               mem_align_shift = phb->pmem_align_shift;
> > +       else
> > +               mem_align_shift = phb->mem_align_shift;
> > +
> >        memset(aligns, 0, sizeof(aligns));
> >        max_order = 0;
> >        size = 0;
> > @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >  #endif
> >                        /* For bridges size != alignment */
> >                        align = pci_resource_alignment(dev, r);
> > -                       order = __ffs(align) - 20;
> > -                       if (order > 11) {
> > +                       order = __ffs(align) - mem_align_shift;
> > +                       if (order > (11 - (mem_align_shift - 20))) {
> >                                dev_warn(&dev->dev, "disabling BAR %d: %pR "
> >                                         "(bad alignment %#llx)\n", i, r,
> >                                         (unsigned long long) align);
> > @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >        for (order = 0; order <= max_order; order++) {
> >                resource_size_t align1 = 1;
> >
> > -               align1 <<= (order + 20);
> > +               align1 <<= (order + mem_align_shift);
> 
> This code must encode somewhere the assumption that mem windows must
> be at least 1MB aligned.  Maybe it has something to do with the "20"
> constants above.  Independent of your patch, it'd be nice to make this
> more explicit.
> 
> >
> >                if (!align)
> >                        min_align = align1;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2b559f1..879de4e 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
> >        resource_size_t offset;         /* bus address + offset = CPU address */
> >  };
> >
> > +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
> 
> "Default shifts"
> 
> > +#define PCI_DEFAULT_IO_ALIGN_SHIFT     12      /* 4KB  */
> > +#define PCI_DEFAULT_MEM_ALIGN_SHIFT    20      /* 1MB  */
> > +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT   20      /* 1MB */
> > +
> >  struct pci_host_bridge {
> >        struct device dev;
> >        struct pci_bus *bus;            /* root bus */
> > +       int io_align_shift;             /* P2P I/O bar minimal alignment shift  */
> > +       int mem_align_shift;            /* P2P MMIO bar minimal alignment shift */
> > +       int pmem_align_shift;           /* P2P prefetchable MMIO bar minimal alignment shift */
> >        struct list_head windows;       /* pci_host_bridge_windows */
> >        void (*release_fn)(struct pci_host_bridge *);
> >        void *release_data;
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Benjamin Herrenschmidt]

On Wed, 2012-06-27 at 12:48 -0600, Bjorn Helgaas wrote:
> On Wed, Jun 27, 2012 at 8:48 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> > On some powerpc platforms, device BARs need to be assigned to separate
> > "segments" of the address space in order for the error isolation and HW
> > virtualization mechanisms (EEH) to work properly. Those "segments" have
> > a minimum size that can be fairly large (16M). In order to be able to
> > use the generic resource assignment code rather than re-inventing our
> > own, we chose to group devices by bus. That way, a simple change of the
> > minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> > bridges is enough to ensure that all BARs for devices below those bridges
> > will fit into contiguous sets of segments and there will be no overlap.
> 
> Is this something that is currently broken on powerpc?  I don't see
> any corresponding powerpc change, like a removal of whatever the
> previous way of doing this was.

Subsequent patch. The goal is to get rid of the bulk of the custom
resource allocation code in arch/powerpc/platforms/powernv/pci-ioda.c
where we basically re-implement it all (well I did) but without handling
all the special & corner cases that the generic code does.

The root of the problem is the need to segment the MMIO and IO spaces
based on a somewhat fixed (HW driven) segment size and have devices
isolated in their own groups of segments.

This is related to our "EEH" advanced error handling scheme which among
other things can properly connect errors triggered by MMIO (target
aborts, UE responses, etc...) to the actual device or driver.

> I'm not sure this is generic enough to warrant putting it in the core
> code (though I don't know whether we have any pcibios_*() hooks that
> would allow us to do it in the arch).

We don't have such hooks. This is the less invasive approach as far as I
can tell.

Cheers,
Ben.

> > This patch provides a way for the host bridge to override the default
> > alignment values used by the resource allocation code for that purpose.
> >
> > Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> > Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> > Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> > ---
> >  drivers/pci/probe.c     |    5 +++++
> >  drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
> >  include/linux/pci.h     |    8 ++++++++
> >  3 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 658ac97..a196529 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> >        if (bridge) {
> >                INIT_LIST_HEAD(&bridge->windows);
> >                bridge->bus = b;
> > +
> > +               /* Set minimal alignment shift of P2P bridges */
> > +               bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> > +               bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> > +               bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
> >        }
> >
> >        return bridge;
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 8fa2d4b..caebe98 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
> >  static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                resource_size_t add_size, struct list_head *realloc_head)
> >  {
> > +       struct pci_host_bridge *phb;
> >        struct pci_dev *dev;
> >        struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> >        unsigned long size = 0, size0 = 0, size1 = 0;
> >        resource_size_t children_add_size = 0;
> > +       resource_size_t io_align;
> >
> >        if (!b_res)
> >                return;
> > @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                                children_add_size += get_res_add_size(realloc_head, r);
> >                }
> >        }
> > +
> > +       phb = find_pci_host_bridge(bus);
> 
> I guess this explains why you want find_pci_host_bridge() to take a
> pci_bus, not a pci_dev..
> 
> > +       io_align = (1 << phb->io_align_shift);
> > +
> >        size0 = calculate_iosize(size, min_size, size1,
> > -                       resource_size(b_res), 4096);
> > +                       resource_size(b_res), io_align);
> >        if (children_add_size > add_size)
> >                add_size = children_add_size;
> >        size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
> >                calculate_iosize(size, min_size, add_size + size1,
> > -                       resource_size(b_res), 4096);
> > +                       resource_size(b_res), io_align);
> >        if (!size0 && !size1) {
> >                if (b_res->start || b_res->end)
> >                        dev_info(&bus->self->dev, "disabling bridge window "
> > @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >                return;
> >        }
> >        /* Alignment of the IO window is always 4K */
> > -       b_res->start = 4096;
> > +       b_res->start = io_align;
> 
> This looks like something that will collide with the changes in the
> pipe to support I/O windows smaller than 4K.
> 
> >        b_res->end = b_res->start + size0 - 1;
> >        b_res->flags |= IORESOURCE_STARTALIGN;
> >        if (size1 > size0 && realloc_head) {
> > -               add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> > +               add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
> >                dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
> >                                 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
> >                                 bus->secondary, bus->subordinate, size1-size0);
> > @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >                        resource_size_t add_size,
> >                        struct list_head *realloc_head)
> >  {
> > +       struct pci_host_bridge *phb;
> >        struct pci_dev *dev;
> >        resource_size_t min_align, align, size, size0, size1;
> >        resource_size_t aligns[12];     /* Alignments from 1Mb to 2Gb */
> > @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >        struct resource *b_res = find_free_bus_resource(bus, type);
> >        unsigned int mem64_mask = 0;
> >        resource_size_t children_add_size = 0;
> > +       int mem_align_shift;
> >
> >        if (!b_res)
> >                return 0;
> >
> > +       phb = find_pci_host_bridge(bus);
> > +       if (type & IORESOURCE_PREFETCH)
> > +               mem_align_shift = phb->pmem_align_shift;
> > +       else
> > +               mem_align_shift = phb->mem_align_shift;
> > +
> >        memset(aligns, 0, sizeof(aligns));
> >        max_order = 0;
> >        size = 0;
> > @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >  #endif
> >                        /* For bridges size != alignment */
> >                        align = pci_resource_alignment(dev, r);
> > -                       order = __ffs(align) - 20;
> > -                       if (order > 11) {
> > +                       order = __ffs(align) - mem_align_shift;
> > +                       if (order > (11 - (mem_align_shift - 20))) {
> >                                dev_warn(&dev->dev, "disabling BAR %d: %pR "
> >                                         "(bad alignment %#llx)\n", i, r,
> >                                         (unsigned long long) align);
> > @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >        for (order = 0; order <= max_order; order++) {
> >                resource_size_t align1 = 1;
> >
> > -               align1 <<= (order + 20);
> > +               align1 <<= (order + mem_align_shift);
> 
> This code must encode somewhere the assumption that mem windows must
> be at least 1MB aligned.  Maybe it has something to do with the "20"
> constants above.  Independent of your patch, it'd be nice to make this
> more explicit.
> 
> >
> >                if (!align)
> >                        min_align = align1;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2b559f1..879de4e 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
> >        resource_size_t offset;         /* bus address + offset = CPU address */
> >  };
> >
> > +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
> 
> "Default shifts"
> 
> > +#define PCI_DEFAULT_IO_ALIGN_SHIFT     12      /* 4KB  */
> > +#define PCI_DEFAULT_MEM_ALIGN_SHIFT    20      /* 1MB  */
> > +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT   20      /* 1MB */
> > +
> >  struct pci_host_bridge {
> >        struct device dev;
> >        struct pci_bus *bus;            /* root bus */
> > +       int io_align_shift;             /* P2P I/O bar minimal alignment shift  */
> > +       int mem_align_shift;            /* P2P MMIO bar minimal alignment shift */
> > +       int pmem_align_shift;           /* P2P prefetchable MMIO bar minimal alignment shift */
> >        struct list_head windows;       /* pci_host_bridge_windows */
> >        void (*release_fn)(struct pci_host_bridge *);
> >        void *release_data;
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Ram Pai]

On Wed, Jun 27, 2012 at 10:48:45PM +0800, Gavin Shan wrote:
> On some powerpc platforms, device BARs need to be assigned to separate
> "segments" of the address space in order for the error isolation and HW
> virtualization mechanisms (EEH) to work properly. Those "segments" have
> a minimum size that can be fairly large (16M). In order to be able to
> use the generic resource assignment code rather than re-inventing our
> own, we chose to group devices by bus. That way, a simple change of the
> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> bridges is enough to ensure that all BARs for devices below those bridges
> will fit into contiguous sets of segments and there will be no overlap.
> 
> This patch provides a way for the host bridge to override the default
> alignment values used by the resource allocation code for that purpose.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/probe.c     |    5 +++++
>  drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
>  include/linux/pci.h     |    8 ++++++++
>  3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 658ac97..a196529 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>  	if (bridge) {
>  		INIT_LIST_HEAD(&bridge->windows);
>  		bridge->bus = b;
> +
> +		/* Set minimal alignment shift of P2P bridges */
> +		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> +		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> +		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
>  	}
> 
>  	return bridge;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..caebe98 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
>  static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		resource_size_t add_size, struct list_head *realloc_head)
>  {
> +	struct pci_host_bridge *phb;
>  	struct pci_dev *dev;
>  	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>  	unsigned long size = 0, size0 = 0, size1 = 0;
>  	resource_size_t children_add_size = 0;
> +	resource_size_t io_align;
> 
>  	if (!b_res)
>   		return;
> @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  				children_add_size += get_res_add_size(realloc_head, r);
>  		}
>  	}
> +
> +	phb = find_pci_host_bridge(bus);
> +	io_align = (1 << phb->io_align_shift);

I prefer to see something like
	io_align = pci_align_boundary(bus, IORESOURCE_IO);

pbus_size_io() and pbus_size_mem() are already quite complicated.
They need not have to know the host_bridge the bus belongs to, in order
to determine the alignment boundary. I would rather prefer to hide those details
in some function.


> +
>  	size0 = calculate_iosize(size, min_size, size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (children_add_size > add_size)
>  		add_size = children_add_size;
>  	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
>  		calculate_iosize(size, min_size, add_size + size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (!size0 && !size1) {
>  		if (b_res->start || b_res->end)
>  			dev_info(&bus->self->dev, "disabling bridge window "
> @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		return;
>  	}
>  	/* Alignment of the IO window is always 4K */
> -	b_res->start = 4096;
> +	b_res->start = io_align;
>  	b_res->end = b_res->start + size0 - 1;
>  	b_res->flags |= IORESOURCE_STARTALIGN;
>  	if (size1 > size0 && realloc_head) {
> -		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> +		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
>  		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
>  				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
>  				 bus->secondary, bus->subordinate, size1-size0);
> @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  			resource_size_t add_size,
>  			struct list_head *realloc_head)
>  {
> +	struct pci_host_bridge *phb;
>  	struct pci_dev *dev;
>  	resource_size_t min_align, align, size, size0, size1;
>  	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
> @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	struct resource *b_res = find_free_bus_resource(bus, type);
>  	unsigned int mem64_mask = 0;
>  	resource_size_t children_add_size = 0;
> +	int mem_align_shift;
> 
>  	if (!b_res)
>  		return 0;
> 
> +	phb = find_pci_host_bridge(bus);
> +	if (type & IORESOURCE_PREFETCH)
> +		mem_align_shift = phb->pmem_align_shift;

		mem_align = pci_align_boundary(bus, IORESOURCE_PREFETCH);
> +	else
> +		mem_align_shift = phb->mem_align_shift;

		mem_align = pci_align_boundary(bus, IORESOURCE_MEM);

	mem_align_shift = __ffs(mem_align);



> +
>  	memset(aligns, 0, sizeof(aligns));
>  	max_order = 0;
>  	size = 0;
> @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  #endif
>  			/* For bridges size != alignment */
>  			align = pci_resource_alignment(dev, r);
> -			order = __ffs(align) - 20;
> -			if (order > 11) {
> +			order = __ffs(align) - mem_align_shift;
> +			if (order > (11 - (mem_align_shift - 20))) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);
> @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	for (order = 0; order <= max_order; order++) {
>  		resource_size_t align1 = 1;
> 
> -		align1 <<= (order + 20);
> +		align1 <<= (order + mem_align_shift);
> 
>  		if (!align)
>  			min_align = align1;

btw, there is good potential for cleanup here. This entire order/alignment
calculation has to go in some other function; in a different patch.


RP

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2b559f1..879de4e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
>  	resource_size_t offset;		/* bus address + offset = CPU address */
>  };
> 
> +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
> +#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
> +#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
> +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
> +
>  struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
> +	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
> +	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
> +	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ram Pai

Re: [PATCH V4 2/2] PCI: minimal alignment for bars of P2P bridges

[Ram Pai]

On Wed, Jun 27, 2012 at 10:48:45PM +0800, Gavin Shan wrote:
> On some powerpc platforms, device BARs need to be assigned to separate
> "segments" of the address space in order for the error isolation and HW
> virtualization mechanisms (EEH) to work properly. Those "segments" have
> a minimum size that can be fairly large (16M). In order to be able to
> use the generic resource assignment code rather than re-inventing our
> own, we chose to group devices by bus. That way, a simple change of the
> minimum alignment requirements of resources assigned to PCI to PCI (P2P)
> bridges is enough to ensure that all BARs for devices below those bridges
> will fit into contiguous sets of segments and there will be no overlap.
> 
> This patch provides a way for the host bridge to override the default
> alignment values used by the resource allocation code for that purpose.
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Richard Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/probe.c     |    5 +++++
>  drivers/pci/setup-bus.c |   28 +++++++++++++++++++++-------
>  include/linux/pci.h     |    8 ++++++++
>  3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 658ac97..a196529 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -431,6 +431,11 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>  	if (bridge) {
>  		INIT_LIST_HEAD(&bridge->windows);
>  		bridge->bus = b;
> +
> +		/* Set minimal alignment shift of P2P bridges */
> +		bridge->io_align_shift = PCI_DEFAULT_IO_ALIGN_SHIFT;
> +		bridge->mem_align_shift = PCI_DEFAULT_MEM_ALIGN_SHIFT;
> +		bridge->pmem_align_shift = PCI_DEFAULT_PMEM_ALIGN_SHIFT;
>  	}
> 
>  	return bridge;
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8fa2d4b..caebe98 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -706,10 +706,12 @@ static resource_size_t calculate_memsize(resource_size_t size,
>  static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		resource_size_t add_size, struct list_head *realloc_head)
>  {
> +	struct pci_host_bridge *phb;
>  	struct pci_dev *dev;
>  	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>  	unsigned long size = 0, size0 = 0, size1 = 0;
>  	resource_size_t children_add_size = 0;
> +	resource_size_t io_align;
> 
>  	if (!b_res)
>   		return;
> @@ -735,13 +737,17 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  				children_add_size += get_res_add_size(realloc_head, r);
>  		}
>  	}
> +
> +	phb = find_pci_host_bridge(bus);
> +	io_align = (1 << phb->io_align_shift);

I prefer to see something like
	io_align = pci_align_boundary(bus, IORESOURCE_IO);

pbus_size_io() and pbus_size_mem() are already quite complicated.
They need not have to know the host_bridge the bus belongs to, in order
to determine the alignment boundary. I would rather prefer to hide those details
in some function.


> +
>  	size0 = calculate_iosize(size, min_size, size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (children_add_size > add_size)
>  		add_size = children_add_size;
>  	size1 = (!realloc_head || (realloc_head && !add_size)) ? size0 :
>  		calculate_iosize(size, min_size, add_size + size1,
> -			resource_size(b_res), 4096);
> +			resource_size(b_res), io_align);
>  	if (!size0 && !size1) {
>  		if (b_res->start || b_res->end)
>  			dev_info(&bus->self->dev, "disabling bridge window "
> @@ -751,11 +757,11 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		return;
>  	}
>  	/* Alignment of the IO window is always 4K */
> -	b_res->start = 4096;
> +	b_res->start = io_align;
>  	b_res->end = b_res->start + size0 - 1;
>  	b_res->flags |= IORESOURCE_STARTALIGN;
>  	if (size1 > size0 && realloc_head) {
> -		add_to_list(realloc_head, bus->self, b_res, size1-size0, 4096);
> +		add_to_list(realloc_head, bus->self, b_res, size1-size0, io_align);
>  		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
>  				 "%pR to [bus %02x-%02x] add_size %lx\n", b_res,
>  				 bus->secondary, bus->subordinate, size1-size0);
> @@ -778,6 +784,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  			resource_size_t add_size,
>  			struct list_head *realloc_head)
>  {
> +	struct pci_host_bridge *phb;
>  	struct pci_dev *dev;
>  	resource_size_t min_align, align, size, size0, size1;
>  	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
> @@ -785,10 +792,17 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	struct resource *b_res = find_free_bus_resource(bus, type);
>  	unsigned int mem64_mask = 0;
>  	resource_size_t children_add_size = 0;
> +	int mem_align_shift;
> 
>  	if (!b_res)
>  		return 0;
> 
> +	phb = find_pci_host_bridge(bus);
> +	if (type & IORESOURCE_PREFETCH)
> +		mem_align_shift = phb->pmem_align_shift;

		mem_align = pci_align_boundary(bus, IORESOURCE_PREFETCH);
> +	else
> +		mem_align_shift = phb->mem_align_shift;

		mem_align = pci_align_boundary(bus, IORESOURCE_MEM);

	mem_align_shift = __ffs(mem_align);



> +
>  	memset(aligns, 0, sizeof(aligns));
>  	max_order = 0;
>  	size = 0;
> @@ -818,8 +832,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  #endif
>  			/* For bridges size != alignment */
>  			align = pci_resource_alignment(dev, r);
> -			order = __ffs(align) - 20;
> -			if (order > 11) {
> +			order = __ffs(align) - mem_align_shift;
> +			if (order > (11 - (mem_align_shift - 20))) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);
> @@ -846,7 +860,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	for (order = 0; order <= max_order; order++) {
>  		resource_size_t align1 = 1;
> 
> -		align1 <<= (order + 20);
> +		align1 <<= (order + mem_align_shift);
> 
>  		if (!align)
>  			min_align = align1;

btw, there is good potential for cleanup here. This entire order/alignment
calculation has to go in some other function; in a different patch.


RP

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2b559f1..879de4e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,17 @@ struct pci_host_bridge_window {
>  	resource_size_t offset;		/* bus address + offset = CPU address */
>  };
> 
> +/* Default shits for P2P I/O and MMIO bar minimal alignment shifts */
> +#define PCI_DEFAULT_IO_ALIGN_SHIFT	12	/* 4KB  */
> +#define PCI_DEFAULT_MEM_ALIGN_SHIFT	20	/* 1MB  */
> +#define PCI_DEFAULT_PMEM_ALIGN_SHIFT	20	/* 1MB */
> +
>  struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
> +	int io_align_shift;		/* P2P I/O bar minimal alignment shift  */
> +	int mem_align_shift;		/* P2P MMIO bar minimal alignment shift */
> +	int pmem_align_shift;		/* P2P prefetchable MMIO bar minimal alignment shift */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ram Pai

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Gavin Shan]

>> For allocating resource under bus path, we do have dev pass along,
>> and we could just use bus instead. Also, we'd like to make function
>> find_pci_host_bridge() global so that some platforms (e.g. PPC) can
>> access the pci host bridge directly.
>
>This patch appears to have multiple unrelated changes:
>
>  - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
>  - change find_pci_host_bridge() argument from dev to bus
>  - fiddle with pcibios_bus_to_resource() and pcibios_resource_to_bus()
>

Leave those questions to Yinghai.

>These should be split out to make your patches easier to review.
>

I will split it for easy review :-)

>What's the rationale for preferring the pci_bus over the pci_dev?
>

We probablly need retrieve the pci_host_bridge through pci_bus
and pci_dev. When converting pci_dev to pci_host_bridge, we can
simply pass "pci_dev->bus".

Thanks,
Gavin

>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/host-bridge.c |   34 +++++++++++++++++++++-------------
>>  include/linux/pci.h       |    4 ++++
>>  2 files changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
>> index a68dc61..4ccf477 100644
>> --- a/drivers/pci/host-bridge.c
>> +++ b/drivers/pci/host-bridge.c
>> @@ -9,22 +9,19 @@
>>
>>  #include "pci.h"
>>
>> -static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
>> +static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>>  {
>> -       struct pci_bus *bus;
>> -
>> -       bus = dev->bus;
>>        while (bus->parent)
>>                bus = bus->parent;
>>
>>        return bus;
>>  }
>>
>> -static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
>> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
>>  {
>> -       struct pci_bus *bus = find_pci_root_bus(dev);
>> +       struct pci_bus *root_bus = find_pci_root_bus(bus);
>>
>> -       return to_pci_host_bridge(bus->bridge);
>> +       return to_pci_host_bridge(root_bus->bridge);
>>  }
>>
>>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>> @@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
>>        return res1->start <= res2->start && res1->end >= res2->end;
>>  }
>>
>> -void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>> -                            struct resource *res)
>> +void __pcibios_resource_to_bus(struct pci_bus *bus,
>> +                                     struct pci_bus_region *region,
>> +                                     struct resource *res)
>>  {
>> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>>        struct pci_host_bridge_window *window;
>>        resource_size_t offset = 0;
>>
>> @@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>        region->start = res->start - offset;
>>        region->end = res->end - offset;
>>  }
>> +void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>> +                            struct resource *res)
>> +{
>> +       __pcibios_resource_to_bus(dev->bus, region, res);
>> +}
>>  EXPORT_SYMBOL(pcibios_resource_to_bus);
>>
>>  static bool region_contains(struct pci_bus_region *region1,
>> @@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *region1,
>>        return region1->start <= region2->start && region1->end >= region2->end;
>>  }
>>
>> -void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> -                            struct pci_bus_region *region)
>> +static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>> +                                     struct pci_bus_region *region)
>>  {
>> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>>        struct pci_host_bridge_window *window;
>>        resource_size_t offset = 0;
>>
>> @@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>>        res->start = region->start + offset;
>>        res->end = region->end + offset;
>>  }
>> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> +                            struct pci_bus_region *region)
>> +{
>> +       __pcibios_bus_to_resource(dev->bus, res, region);
>> +}
>>  EXPORT_SYMBOL(pcibios_bus_to_resource);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index fefb4e1..2b559f1 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -385,6 +385,7 @@ struct pci_host_bridge {
>>  };
>>
>>  #define        to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
>> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
>>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>                     void (*release_fn)(struct pci_host_bridge *),
>>                     void *release_data);
>> @@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
>>
>>  /* Generic PCI functions used internally */
>>
>> +void __pcibios_resource_to_bus(struct pci_bus *bus,
>> +                              struct pci_bus_region *region,
>> +                              struct resource *res);
>>  void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>                             struct resource *res);
>>  void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> --
>> 1.7.9.5
>>
>

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Gavin Shan]

>> For allocating resource under bus path, we do have dev pass along,
>> and we could just use bus instead. Also, we'd like to make function
>> find_pci_host_bridge() global so that some platforms (e.g. PPC) can
>> access the pci host bridge directly.
>
>This patch appears to have multiple unrelated changes:
>
>  - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
>  - change find_pci_host_bridge() argument from dev to bus
>  - fiddle with pcibios_bus_to_resource() and pcibios_resource_to_bus()
>

Leave those questions to Yinghai.

>These should be split out to make your patches easier to review.
>

I will split it for easy review :-)

>What's the rationale for preferring the pci_bus over the pci_dev?
>

We probablly need retrieve the pci_host_bridge through pci_bus
and pci_dev. When converting pci_dev to pci_host_bridge, we can
simply pass "pci_dev->bus".

Thanks,
Gavin

>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>>  drivers/pci/host-bridge.c |   34 +++++++++++++++++++++-------------
>>  include/linux/pci.h       |    4 ++++
>>  2 files changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
>> index a68dc61..4ccf477 100644
>> --- a/drivers/pci/host-bridge.c
>> +++ b/drivers/pci/host-bridge.c
>> @@ -9,22 +9,19 @@
>>
>>  #include "pci.h"
>>
>> -static struct pci_bus *find_pci_root_bus(struct pci_dev *dev)
>> +static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>>  {
>> -       struct pci_bus *bus;
>> -
>> -       bus = dev->bus;
>>        while (bus->parent)
>>                bus = bus->parent;
>>
>>        return bus;
>>  }
>>
>> -static struct pci_host_bridge *find_pci_host_bridge(struct pci_dev *dev)
>> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
>>  {
>> -       struct pci_bus *bus = find_pci_root_bus(dev);
>> +       struct pci_bus *root_bus = find_pci_root_bus(bus);
>>
>> -       return to_pci_host_bridge(bus->bridge);
>> +       return to_pci_host_bridge(root_bus->bridge);
>>  }
>>
>>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>> @@ -40,10 +37,11 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
>>        return res1->start <= res2->start && res1->end >= res2->end;
>>  }
>>
>> -void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>> -                            struct resource *res)
>> +void __pcibios_resource_to_bus(struct pci_bus *bus,
>> +                                     struct pci_bus_region *region,
>> +                                     struct resource *res)
>>  {
>> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>>        struct pci_host_bridge_window *window;
>>        resource_size_t offset = 0;
>>
>> @@ -60,6 +58,11 @@ void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>        region->start = res->start - offset;
>>        region->end = res->end - offset;
>>  }
>> +void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>> +                            struct resource *res)
>> +{
>> +       __pcibios_resource_to_bus(dev->bus, region, res);
>> +}
>>  EXPORT_SYMBOL(pcibios_resource_to_bus);
>>
>>  static bool region_contains(struct pci_bus_region *region1,
>> @@ -68,10 +71,10 @@ static bool region_contains(struct pci_bus_region *region1,
>>        return region1->start <= region2->start && region1->end >= region2->end;
>>  }
>>
>> -void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> -                            struct pci_bus_region *region)
>> +static void __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>> +                                     struct pci_bus_region *region)
>>  {
>> -       struct pci_host_bridge *bridge = find_pci_host_bridge(dev);
>> +       struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
>>        struct pci_host_bridge_window *window;
>>        resource_size_t offset = 0;
>>
>> @@ -93,4 +96,9 @@ void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>>        res->start = region->start + offset;
>>        res->end = region->end + offset;
>>  }
>> +void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> +                            struct pci_bus_region *region)
>> +{
>> +       __pcibios_bus_to_resource(dev->bus, res, region);
>> +}
>>  EXPORT_SYMBOL(pcibios_bus_to_resource);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index fefb4e1..2b559f1 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -385,6 +385,7 @@ struct pci_host_bridge {
>>  };
>>
>>  #define        to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
>> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
>>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>                     void (*release_fn)(struct pci_host_bridge *),
>>                     void *release_data);
>> @@ -657,6 +658,9 @@ void pci_fixup_cardbus(struct pci_bus *);
>>
>>  /* Generic PCI functions used internally */
>>
>> +void __pcibios_resource_to_bus(struct pci_bus *bus,
>> +                              struct pci_bus_region *region,
>> +                              struct resource *res);
>>  void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>                             struct resource *res);
>>  void pcibios_bus_to_resource(struct pci_dev *dev, struct resource *res,
>> --
>> 1.7.9.5
>>
>

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Benjamin Herrenschmidt]

On Wed, 2012-06-27 at 12:15 -0600, Bjorn Helgaas wrote:
> 
> This patch appears to have multiple unrelated changes:
> 
>   - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
>   - change find_pci_host_bridge() argument from dev to bus
>   - fiddle with pcibios_bus_to_resource() and
> pcibios_resource_to_bus()
> 
> These should be split out to make your patches easier to review.
> 
> What's the rationale for preferring the pci_bus over the pci_dev? 

We really want to stop doing that crap and put a pointer to the host
bridge directly in each pci_dev (and maybe pci_bus).

We already do that on powerpc via sysdata (pointing to our private
structure but the plan is to move that toward struct pci_host_bridge
eventually).

We'll need host bridge access to deal with resource "offsets" in a few
more places and having to do a lookup is just a pain.

For example, see my earlier email about the issues with the way we
calculate the minimum address to assign devices. This is currently
broken when we have an offset and to fix it we'll probably want to deal
with offsets all the way down in the resource allocation code.

Cheers,
Ben.

Re: [PATCH V4 1/2] PCI: pcibus address to resource converting take bus directly

[Benjamin Herrenschmidt]

On Wed, 2012-06-27 at 12:15 -0600, Bjorn Helgaas wrote:
> 
> This patch appears to have multiple unrelated changes:
> 
>   - change "struct pci_bus *bus" to "struct pci_bus *root_bus"
>   - change find_pci_host_bridge() argument from dev to bus
>   - fiddle with pcibios_bus_to_resource() and
> pcibios_resource_to_bus()
> 
> These should be split out to make your patches easier to review.
> 
> What's the rationale for preferring the pci_bus over the pci_dev? 

We really want to stop doing that crap and put a pointer to the host
bridge directly in each pci_dev (and maybe pci_bus).

We already do that on powerpc via sysdata (pointing to our private
structure but the plan is to move that toward struct pci_host_bridge
eventually).

We'll need host bridge access to deal with resource "offsets" in a few
more places and having to do a lookup is just a pain.

For example, see my earlier email about the issues with the way we
calculate the minimum address to assign devices. This is currently
broken when we have an offset and to fix it we'll probably want to deal
with offsets all the way down in the resource allocation code.

Cheers,
Ben.