New subject: Change in coreboot[master]: soc/intel/xeon_sp/skx: fix mem64 BAR assignment

17 Apr 2020

Hello Anjaneya "Reddy" Chagam,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40500
to review the following change.
Change subject: soc/intel/xeon_sp: support for mem64 non-prefetchable BAR assignment
......................................................................
soc/intel/xeon_sp: support for mem64 non-prefetchable BAR assignment
PCIe End Point device's BARS need to be accomondated in bridge device's
resource base/limit config registers. In particular, mem32/mem64
(non-prefetchable) BARs need to be accomondated in bridge device's
mem base/limit config registers.
This patches fixes the bug that mem64 BAR is not considered when
setting bridge device's mem base/limit config registers.
Without this patch, TiogaPass without riser card works fine; but on
TiogaPass with riser card, the boot fails with MTRR table overflow.
While at this, optimized the code, and also taken into account
whether FSP HOB indicates that mem32 address space is used for
PCIe mem64 allocation or not.
Signed-off-by: Jonathan Zhang jonzhang@fb.com
Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com
Tested-by: Morgan_Jang@wiwynn.com
Tested-by: Ryback.Hung@quantatw.com
Change-Id: I8dd7d94d52ad02f22c8e69b2e5d6dde2a79bc1f7
---
M src/soc/intel/xeon_sp/skx/chip.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
3 files changed, 114 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/40500/1

diff --git a/src/soc/intel/xeon_sp/skx/chip.c b/src/soc/intel/xeon_sp/skx/chip.c
index be452a0..e3b71cf 100644
--- a/src/soc/intel/xeon_sp/skx/chip.c
+++ b/src/soc/intel/xeon_sp/skx/chip.c
@@ -32,6 +32,74 @@
    struct stack_dev_resource *next;
 };
+typedef enum {
+	RES_TYPE_IO = 0,
+	RES_TYPE_NONPREF_MEM,
+	RES_TYPE_PREF_MEM,
+	MAX_RES_TYPES
+} ResType;
+
+/*
+ * This function maps from type 0 end point device BAR resource types
+ * to type 1 bridge device base/limit resource types. There are 3 base/limt
+ * resource types:
+ * RES_TYPE_IO: IO base/limit. IO BAR is mapped to it.
+ * RES_TYPE_NONPREF_MEM: Memory base/limit (below 4GB address space). mem64
+ *   and mem32 BARs are mapped to it.
+ * RES_TYPE_PREF_MEM: Prefetchable base/limit. Prefetchable BARs are mapped
+ *   to it.
+*/
+static ResType get_res_type(uint64_t flags)
+{
+	if (flags & IORESOURCE_IO)
+		return RES_TYPE_IO;
+	if (flags & IORESOURCE_MEM) {
+		if (flags & IORESOURCE_PREFETCH)
+			return RES_TYPE_PREF_MEM;
+		else
+			return RES_TYPE_NONPREF_MEM;
+	printk(BIOS_ERR, "Invalid resource type 0x%llx\n", flags);
+	die("Invalid resource type");
+}
+
+static bool need_assignment(uint64_t flags)
+{
+	if (flags & (IORESOURCE_STORED | IORESOURCE_RESERVE | IORESOURCE_FIXED |
+		IORESOURCE_ASSIGNED))
+		return false;
+	return true;
+}
+
+static uint64_t get_resource_base(STACK_RES *stack, ResType res_type)
+{
+	if (res_type == RES_TYPE_IO) {
+		assert(stack->PciResourceIoBase <= stack->PciResourceIoLimit);
+		return stack->PciResourceIoBase;
+	}
+
+	if (res_type == RES_TYPE_NONPREF_MEM) {
+		assert(stack->PciResourceMem32Base <= stack->PciResourceMem32Limit);
+		return stack->PciResourceMem32Base;
+	}
+
+	assert(stack->PciResourceMem64Base <= stack->PciResourceMem64Limit);
+	return stack->PciResourceMem64Base;
+}
+
+static void set_resource_base(STACK_RES *stack, ResType res_type, uint64_t base)
+{
+	if (res_type == RES_TYPE_IO) {
+		assert(base <= (stack->PciResourceIoLimit + 1));
+		stack->PciResourceIoBase = base;
+	} else if (res_type == RES_TYPE_NONPREF_MEM) {
+		assert(base <= (stack->PciResourceMem32Limit + 1));
+		stack->PciResourceMem32Base = base;
+	} else {
+		assert(base <= (stack->PciResourceMem64Limit + 1));
+		stack->PciResourceMem64Base = base;
+	}
+}
+
 static void assign_stack_resources(struct iiostack_resource *stack_list,
    struct device *dev, struct resource *bridge);
@@ -176,19 +244,13 @@
    }
 }
-static void reserve_dev_resources(STACK_RES *stack, unsigned long res_type,
+static void reserve_dev_resources(STACK_RES *stack, ResType res_type,
    struct stack_dev_resource *res_root, struct resource *bridge)
 {
    uint8_t align;
    uint64_t orig_base, base;
-	if (res_type & IORESOURCE_IO)
-		orig_base = stack->PciResourceIoBase;
-	else if ((res_type & IORESOURCE_MEM) && ((res_type & IORESOURCE_PCI64) ||
-		(!res_root && bridge && (bridge->flags & IORESOURCE_PREFETCH))))
-		orig_base = stack->PciResourceMem64Base;
-	else
-		orig_base = stack->PciResourceMem32Base;
+	orig_base = get_resource_base(stack, res_type);
align = 0;
    base = orig_base;
@@ -236,14 +298,7 @@
    	base = bridge->limit + 1;
    }
-	/* update new limits */
-	if (res_type & IORESOURCE_IO)
-		stack->PciResourceIoBase = base;
-	else if ((res_type & IORESOURCE_MEM) && ((res_type & IORESOURCE_PCI64) ||
-		(!res_root && bridge && (bridge->flags & IORESOURCE_PREFETCH))))
-		stack->PciResourceMem64Base = base;
-	else
-		stack->PciResourceMem32Base = base;
+	set_resource_base(stack, res_type, base);
 }
static void reclaim_resource_mem(struct stack_dev_resource *res_root)
@@ -273,15 +328,14 @@
for (res = dev->resource_list; res; res = res->next) {
    	if (!(res->flags & IORESOURCE_BRIDGE) ||
-			(bridge && ((bridge->flags & (IORESOURCE_IO | IORESOURCE_MEM |
-				IORESOURCE_PREFETCH | IORESOURCE_PCI64)) !=
-					(res->flags & (IORESOURCE_IO | IORESOURCE_MEM |
-						IORESOURCE_PREFETCH | IORESOURCE_PCI64)))))
+		    (bridge && (get_res_type(bridge->flags) != get_res_type(res->flags))))
    		continue;
assign_stack_resources(stack_list, dev, res);
+
    	if (!bridge)
    		continue;
+
    	/* for 1st time update, overlading IORESOURCE_ASSIGNED */
    	if (!(bridge->flags & IORESOURCE_ASSIGNED)) {
    		bridge->base = res->base;
@@ -317,36 +371,40 @@
    		assign_bridge_resources(stack_list, curdev, bridge);
/* Pick non-bridged resources for resource allocation for each resource type */
-		unsigned long flags[5] = {IORESOURCE_IO, IORESOURCE_MEM,
-			(IORESOURCE_PCI64|IORESOURCE_MEM), (IORESOURCE_MEM|IORESOURCE_PREFETCH),
-			(IORESOURCE_PCI64|IORESOURCE_MEM|IORESOURCE_PREFETCH)};
-		uint8_t no_res_types = 5;
+		ResType res_types[MAX_RES_TYPES] = {
+			RES_TYPE_IO,
+			RES_TYPE_NONPREF_MEM,
+			RES_TYPE_PREF_MEM
+		};
+		uint8_t no_res_types = MAX_RES_TYPES;
+
    	if (bridge) {
-			flags[0] = bridge->flags &
-				(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
-			if ((bridge->flags & IORESOURCE_MEM) &&
-				(bridge->flags & IORESOURCE_PREFETCH))
-				flags[0] |= IORESOURCE_PCI64;
+			res_types[0] = get_res_type(bridge->flags);
    		no_res_types = 1;
    	}
+		printk(BIOS_SPEW, "%s:%d no_res_types: %d\n", __func__, __LINE__, no_res_types);
+
    	/* Process each resource type */
    	for (int rt = 0; rt < no_res_types; ++rt) {
    		struct stack_dev_resource *res_root = NULL;
+			printk(BIOS_SPEW, "%s:%d rt: %d\n", __func__, __LINE__, rt);
for (curdev = bus->children; curdev; curdev = curdev->sibling) {
    			struct resource *res;
+				printk(BIOS_SPEW, "%s:%d dev: %s\n", __func__, __LINE__,
+					dev_path(curdev));
    			if (!curdev->enabled)
    				continue;
for (res = curdev->resource_list; res; res = res->next) {
-					if ((res->flags & IORESOURCE_BRIDGE) || (res->flags &
-						(IORESOURCE_STORED | IORESOURCE_RESERVE |
-							IORESOURCE_FIXED | IORESOURCE_ASSIGNED)
-						) || ((res->flags & (IORESOURCE_IO |
-							IORESOURCE_MEM | IORESOURCE_PCI64
-							| IORESOURCE_PREFETCH))
-							!= flags[rt]) || res->size == 0)
+					printk(BIOS_SPEW, "%s:%d dev: %s, flags: 0x%lx\n",
+						__func__, __LINE__,
+						dev_path(curdev), res->flags);
+					if (res->size == 0                          ||
+					  get_res_type(res->flags) != res_types[rt] ||
+					  (res->flags & IORESOURCE_BRIDGE)          ||
+					  !need_assignment(res->flags))
    					continue;
    				else
    					add_res_to_stack(&res_root, curdev, res);
@@ -355,51 +413,13 @@
/* Allocate resources and update bridge range */
    		if (res_root || (bridge && !(bridge->flags & IORESOURCE_ASSIGNED))) {
-				reserve_dev_resources(stack, flags[rt], res_root, bridge);
+				reserve_dev_resources(stack, res_types[rt], res_root, bridge);
    			reclaim_resource_mem(res_root);
    		}
    	}
    }
 }
-static void xeonsp_constrain_pci_resources(struct device *dev, struct resource *res, void *data)
-{
-	STACK_RES *stack = (STACK_RES *) data;
-	if (!(res->flags & IORESOURCE_FIXED))
-		return;
-
-	uint64_t base, limit;
-	if (res->flags & IORESOURCE_IO) {
-		base = stack->PciResourceIoBase;
-		limit = stack->PciResourceIoLimit;
-	} else if ((res->flags & IORESOURCE_MEM) && (res->flags & IORESOURCE_PCI64)) {
-		base = stack->PciResourceMem64Base;
-		limit = stack->PciResourceMem64Limit;
-	} else {
-		base = stack->PciResourceMem32Base;
-		limit = stack->PciResourceMem32Limit;
-	}
-
-	if (((res->base + res->size - 1) < base) || (res->base > limit)) /* outside window */
-		return;
-
-	if (res->limit > limit) /* resource end is out of limit */
-		limit = res->base - 1;
-	else
-		base = res->base + res->size;
-
-	if (res->flags & IORESOURCE_IO) {
-		stack->PciResourceIoBase = base;
-		stack->PciResourceIoLimit = limit;
-	} else if ((res->flags & IORESOURCE_MEM) && (res->flags & IORESOURCE_PCI64)) {
-		stack->PciResourceMem64Base = base;
-		stack->PciResourceMem64Limit = limit;
-	} else {
-		stack->PciResourceMem32Base = base;
-		stack->PciResourceMem32Limit = limit;
-	}
-}
-
 static void xeonsp_pci_domain_read_resources(struct device *dev)
 {
    struct bus *link;
@@ -421,18 +441,23 @@
    for (link = dev->link_list; link; link = link->next)
    	xeonsp_pci_dev_iterator(link, xeonsp_reset_pci_op, NULL, NULL);
-	/*
-	 * 1. group devices, resources for each stack
-	 * 2. order resources in descending order of requested resource allocation sizes
-	 */
    struct iiostack_resource stack_info = {0};
-	get_iiostack_info(&stack_info);
+	uint8_t pci64bit_alloc_flag = get_iiostack_info(&stack_info);
-	/* constrain stack window */
-	for (link = dev->link_list; link; link = link->next) {
-		STACK_RES *stack = find_stack_for_bus(&stack_info, link->secondary);
-		assert(stack != 0);
-		xeonsp_pci_dev_iterator(link, NULL, xeonsp_constrain_pci_resources, stack);
+	/*
+	 * pci64bit_alloc_flag is obtained from FSP HOB. It indicates whether
+	 * 32bit address space needs to be split between mem32 and
+	 * mem64 windows.
+	 */
+	if (!pci64bit_alloc_flag) {
+		for (int s = 0; s < stack_info.no_of_stacks; ++s) {
+			STACK_RES *res = &stack_info.res[s];
+			uint64_t length = (res->PciResourceMem32Limit -
+				res->PciResourceMem32Base + 1) / 2;
+			res->PciResourceMem64Limit = res->PciResourceMem32Limit;
+			res->PciResourceMem32Limit = (res->PciResourceMem32Base + length - 1);
+			res->PciResourceMem64Base = res->PciResourceMem32Limit + 1;
+		}
    }
/* assign resources */
@@ -491,11 +516,9 @@
 #endif
 };
-/* Attach IIO stack bus numbers with dummy device to PCI DOMAIN 0000 device */
 static void attach_iio_stacks(struct device *dev)
 {
    struct bus *iiostack_bus;
-	struct device dummy;
    struct iiostack_resource stack_info = {0};
DEV_FUNC_ENTER(dev);
@@ -518,14 +541,6 @@
    	iiostack_bus->next = NULL;
    	iiostack_bus->link_num = 1;
-		dummy.bus = iiostack_bus;
-		dummy.path.type = DEVICE_PATH_PCI;
-		dummy.path.pci.devfn = 0;
-		uint32_t id = pci_read_config32(&dummy, PCI_VENDOR_ID);
-		if (id == 0xffffffff)
-			printk(BIOS_WARNING, "IIO Stack device %s not visible\n",
-				dev_path(&dummy));
-
    	if (dev->link_list == NULL) {
    		dev->link_list = iiostack_bus;
    	} else {
diff --git a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
index 8ba4b29..002fb2b 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
@@ -22,7 +22,7 @@
 uint32_t pci_read_mmio_reg(int bus, uint32_t dev, uint32_t func, int offset);
uint32_t get_socket_stack_busno(uint32_t socket, uint32_t stack);
-void get_iiostack_info(struct iiostack_resource *info);
+uint8_t get_iiostack_info(struct iiostack_resource *info);
int get_threads_per_package(void);
 int get_platform_thread_count(void);
diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c
index edacafd..94f2a38 100644
--- a/src/soc/intel/xeon_sp/skx/soc_util.c
+++ b/src/soc/intel/xeon_sp/skx/soc_util.c
@@ -364,7 +364,7 @@
    return get_cpu_count() * get_threads_per_package();
 }
-void get_iiostack_info(struct iiostack_resource *info)
+uint8_t get_iiostack_info(struct iiostack_resource *info)
 {
    size_t hob_size;
    const uint8_t fsp_hob_iio_universal_data_guid[16] = FSP_HOB_IIO_UNIVERSAL_DATA_GUID;
@@ -386,6 +386,8 @@
    		memcpy(&info->res[info->no_of_stacks++], ri, sizeof(STACK_RES));
    	}
    }
+
+	return hob->PlatformData.Pci64BitResourceAllocation;
 }
#if ENV_RAMSTAGE
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/40500
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8dd7d94d52ad02f22c8e69b2e5d6dde2a79bc1f7
Gerrit-Change-Number: 40500
Gerrit-PatchSet: 1
Gerrit-Owner: Jonathan Zhang jonzhang@fb.com
Gerrit-Reviewer: Anjaneya "Reddy" Chagam anjaneya.chagam@intel.com
Gerrit-MessageType: newchange



    

Change in coreboot[master]: soc/intel/xeon_sp: support for mem64 non-prefetchable BAR assignment