coreboot-gerrit July 2020

coreboot-gerrit@coreboot.org

1 participants
3167 discussions

Change in coreboot[master]: soc/intel/xeon_sp/skx: use mem32 address space for PCIe mem64 assignment
by Jonathan Zhang (Code Review) 06 Jul '20

06 Jul '20

Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40577 ) Change subject: soc/intel/xeon_sp/skx: use mem32 address space for PCIe mem64 assignment ...................................................................... soc/intel/xeon_sp/skx: use mem32 address space for PCIe mem64 assignment FSP HOB indicates whether mem32 address space is used for PCIe mem64 allocation or not. If that is the case, split PCIe mem32 address space to support both mem32 assignment and mem64 assignment. TESTED=booted TiogaPass with different PCIe configurations; Verified PCIe devices work in target OS, and resource assignments are correct. Change-Id: Ic7064526bdcba770ff377960045a8aea59e9c0bb --- 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, 21 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/40577/1 diff --git a/src/soc/intel/xeon_sp/skx/chip.c b/src/soc/intel/xeon_sp/skx/chip.c index 938edf1..c097457 100644 --- a/src/soc/intel/xeon_sp/skx/chip.c +++ b/src/soc/intel/xeon_sp/skx/chip.c @@ -444,7 +444,23 @@ xeonsp_pci_dev_iterator(link, xeonsp_reset_pci_op, NULL, NULL); struct iiostack_resource stack_info = {0}; - get_iiostack_info(&stack_info); + uint8_t pci64bit_alloc_flag = get_iiostack_info(&stack_info); + + /* + * 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 */ assign_stack_resources(&stack_info, dev, NULL); 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/+/40577 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic7064526bdcba770ff377960045a8aea59e9c0bb Gerrit-Change-Number: 40577 Gerrit-PatchSet: 1 Gerrit-Owner: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-MessageType: newchange

1 2

Change in coreboot[master]: soc/intel/xeon_sp: support for mem64 non-prefetchable BAR assignment
by Jonathan Zhang (Code Review) 06 Jul '20

06 Jul '20

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(a)fb.com> Signed-off-by: Reddy Chagam <anjaneya.chagam(a)intel.com> Tested-by: Morgan_Jang(a)wiwynn.com Tested-by: Ryback.Hung(a)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(a)fb.com> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-MessageType: newchange

2 8

Change in coreboot[master]: chromeec: fix error path return value
by Nick Vaccaro (Code Review) 06 Jul '20

06 Jul '20

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43045 ) Change subject: chromeec: fix error path return value ...................................................................... Patch Set 1: Code-Review+1 Fix looks fine to me. Gave a +1, wanted to wait to see what Duncan says. -- To view, visit https://review.coreboot.org/c/coreboot/+/43045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6bf8d74229f4ac2bf87f9ee83e0c88c3c94de765 Gerrit-Change-Number: 43045 Gerrit-PatchSet: 1 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 06 Jul 2020 21:05:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: volteer: enable VBOOT_EC_EFS
by Caveh Jalali (Code Review) 06 Jul '20

06 Jul '20

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43044 ) Change subject: volteer: enable VBOOT_EC_EFS ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/43044/1//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/43044/1//COMMIT_MSG@12 PS1, Line 12: TEST=used "V=1 emerge-${BOARD} coreboot" to verify "-DEC_EFS=1" is : used when compiling 2ec_sync.c > does it still boot??? LoL, ya 😊 ability to boot was never in question - the issue is whether we end up in recovery when we should not. daisuke mentioned b/160272400 that this flag is for EFS1 only, not EFS2, so i think more clarification is needed. -- To view, visit https://review.coreboot.org/c/coreboot/+/43044 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie71bb169242b2511b4a4102d46a6981d72d07806 Gerrit-Change-Number: 43044 Gerrit-PatchSet: 1 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Daisuke Nojiri <dnojiri(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jul 2020 21:00:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: Handle acpi_name and common code
by Tim Wawrzynczak (Code Review) 06 Jul '20

06 Jul '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 ) Change subject: drivers/intel/mipi_camera: Handle acpi_name and common code ...................................................................... Patch Set 32: (20 comments) https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG@21 PS17, Line 21: in to > Done Ack https://review.coreboot.org/c/coreboot/+/41607/28//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/41607/28//COMMIT_MSG@10 PS28, Line 10: , if acpi_name is not set in the devicetree and > @Paul, the second statement "moves some of the common code to separate methods. […] Ack https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_cam… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_cam… PS6, Line 188: /* Method (_DSM, 4, NotSerialized) */ : acpigen_write_method("_DSM", 0x4); : : /* ToBuffer (Arg0, Local0) */ : acpigen_write_to_buffer(ARG0_OP, LOCAL0_OP); : : /* If (LEqual (Local0, ToUUID(uuid))) */ : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_uuid("822ace8f-2814-4174-a56b-5f029fe079ee"); : acpigen_write_return_string(config->sensor_name ? config->sensor_name : "UNKNOWN"); : acpigen_pop_len(); /* If */ : : /* If (LEqual (Local0, ToUUID(uuid))) */ : acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_uuid("26257549-9271-4ca4-bb43-c4899d5a4881"); : /* ToInteger (Arg2, Local1) */ : acpigen_write_to_integer(ARG2_OP, LOCAL1_OP); : : /* If (LEqual (Local1, 1)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 1); : acpigen_write_return_integer((config->ssdb.vcm_type ? 1 : 0) + : (config->ssdb.rom_type ? 1 : 0) + 1); : acpigen_pop_len(); /* If Arg2=1 */ : : /* If (LEqual (Local1, 2)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 2); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | : ((uint32_t)i2c_addr) << 8 | 0x0); /* type 0 for sensor */ : acpigen_pop_len(); /* If Arg2=2 */ : : if (config->ssdb.vcm_type) { : /* If (LEqual (Local1, 3)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 1 for vcm */ : ((uint32_t)config->vcm_address) << 8 | 0x01); : acpigen_pop_len(); /* If Arg2=3 */ : } : if (config->ssdb.rom_type) { : /* If (LEqual (Local1, 3 or 4)) */ : acpigen_write_if_lequal_op_int(LOCAL1_OP, 3 + (config->ssdb.vcm_type ? 1 : 0)); : acpigen_write_return_integer(((uint32_t)i2c_bus) << 24 | /* type 2 for rom */ : ((uint32_t)config->rom_address) << 8 | 0x02); : acpigen_pop_len(); /* If Arg2=3 or 4 */ : } : : acpigen_pop_len(); /* If uuid */ : : /* Return (Buffer (One) { 0x0 }) */ : acpigen_write_return_singleton_buffer(0x0); : : acpigen_pop_len(); /* Method _DSM */ > I checked internally and _DSM is used only by the Windows drivers. […] Ack https://review.coreboot.org/c/coreboot/+/41607/6/src/drivers/intel/mipi_cam… PS6, Line 445: if (config->dep) { : acpigen_write_name("_DEP"); : acpigen_write_package(1); : acpigen_emit_namestring(config->dep); : acpigen_pop_len(); : } > Is the kernel driver using _DEP for something unusual? _DEP is used to "designate device objects tha […] Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 42: struct acpi_dp *ep_table = NULL; : struct acpi_dp *port_table = NULL; : struct acpi_dp *remote = NULL; > acpi_dp_new_table is internally calling malloc which calls die incase if it unable to allocated any […] Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 48: strdup > As mentioned above strdup is calling malloc Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 50: acpi_dp_add_integer > acpi_dp_add_integer is calling acpi_dp_new which calls malloc Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 91: *config > camera_fill_ssdt is be called only if required configs are enabled in devicetree. […] Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 216: config > As mentioned above. Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 272: 0 > It's zero for all the existing systems in coreboot, though the docs in the Linux kernel have example […] Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 303: /* endpoint? */ > acpi_graph_get_remote_endpoint() seems to confirm my guess, where a comment says that port and endpo […] Ack https://review.coreboot.org/c/coreboot/+/41607/11/src/drivers/intel/mipi_ca… PS11, Line 315: 180 > So far only nautilus's cam0.asl specifies a "rotation" and it uses 180. Ack https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_ca… PS20, Line 27: CIO2 > might be nice to get this spelled out the first time. Ack https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_ca… PS20, Line 50: EP%u0 > You look like you're being purposeful having the table named EP10 instead of EP01 for example. […] Ack https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_ca… PS20, Line 92: camera_pld_defaults > suggestion: apply_pld_defaults or fill_defaults like you have below Ack https://review.coreboot.org/c/coreboot/+/41607/20/src/drivers/intel/mipi_ca… PS20, Line 139: camera_dsm_return > suggestion: address_for_dev_type Ack https://review.coreboot.org/c/coreboot/+/41607/25/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41607/25/src/drivers/intel/mipi_ca… PS25, Line 101: const char *scope = acpi_device_scope(dev); > Now I have moved the IPU device outside camera device scope and due to this now the device that is p […] Ack https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/chip.h: https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… PS17, Line 174: uint32_t nvm_readonly; > The final output is going to be int so it was kept as int. […] Ack https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/chip.h: https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_ca… PS12, Line 148: uint8_t num_freq_entries; /* # of elements in link_freq */ > Any specific reason to change them to native. […] Ack https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_ca… PS12, Line 167: uint32_t nvm_size; > Both works, but If there is not a explicit length requirement, the reader would think, that there is […] Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/41607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I15979f345fb823df2560db269e902a1ea650b69e Gerrit-Change-Number: 41607 Gerrit-PatchSet: 32 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Matt Delco <delco(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Mon, 06 Jul 2020 20:52:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt Delco <delco(a)chromium.org> Comment-In-Reply-To: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: Handle acpi_name and common code
by Tim Wawrzynczak (Code Review) 06 Jul '20

06 Jul '20

1 0

Change in coreboot[master]: mb/google/volteer: Enable HotPlug on PCIe root port for the SD express
by Nick Chen (Code Review) 06 Jul '20

06 Jul '20

Nick Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42897 ) Change subject: mb/google/volteer: Enable HotPlug on PCIe root port for the SD express ...................................................................... mb/google/volteer: Enable HotPlug on PCIe root port for the SD express Enable HotPlug for the PCIe root port that the SD express is on so the OS can re-train the link without needing a reboot if it goes down unexpectedly at runtime. BUG=b:156879564 BRANCH=master TEST=enable HotPlug on Volteer Root Port 7 (SD express) and check in linux that it is identified as a HotPlug capable root port Signed-off-by: Nick Chen <nick_xr_chen(a)wistron.corp-partner.google.com> Change-Id: Ie9d427dd297567f06123119a670b5ed2e1f73701 --- M src/mainboard/google/volteer/variants/baseboard/devicetree.cb 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/42897/1 diff --git a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb index 0c581a5..48ea18f 100644 --- a/src/mainboard/google/volteer/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/volteer/variants/baseboard/devicetree.cb @@ -88,6 +88,7 @@ register "PcieRpLtrEnable[7]" = "1" register "PcieClkSrcUsage[3]" = "7" register "PcieClkSrcClkReq[3]" = "3" + register "PcieRpHotPlug[7]" = "1" # Enable WLAN PCIE 7 using clk 1 register "PcieRpEnable[6]" = "1" -- To view, visit https://review.coreboot.org/c/coreboot/+/42897 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie9d427dd297567f06123119a670b5ed2e1f73701 Gerrit-Change-Number: 42897 Gerrit-PatchSet: 1 Gerrit-Owner: Nick Chen <nick_xr_chen(a)wistron.corp-partner.google.com> Gerrit-MessageType: newchange

3 11

Change in coreboot[master]: chromeec: fix error path return value
by Tim Wawrzynczak (Code Review) 06 Jul '20

06 Jul '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43045 ) Change subject: chromeec: fix error path return value ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/43045/1/src/ec/google/chromeec/ec_… File src/ec/google/chromeec/ec_lpc.c: https://review.coreboot.org/c/coreboot/+/43045/1/src/ec/google/chromeec/ec_… PS1, Line 247: 1 There are an awful lot of calls to google_chromeec_command() in this codebase.... but I tend to agree, returning -sizeof(rq) when the EC returned an error code seems very odd. If it was -cec_comand->cmd_code I could see that. git blame shows that Duncan wrote this, wdyt Duncan? -- To view, visit https://review.coreboot.org/c/coreboot/+/43045 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6bf8d74229f4ac2bf87f9ee83e0c88c3c94de765 Gerrit-Change-Number: 43045 Gerrit-PatchSet: 1 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 06 Jul 2020 20:29:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: volteer: enable VBOOT_EC_EFS
by Tim Wawrzynczak (Code Review) 06 Jul '20

06 Jul '20

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43044 ) Change subject: volteer: enable VBOOT_EC_EFS ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/43044/1//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/43044/1//COMMIT_MSG@12 PS1, Line 12: TEST=used "V=1 emerge-${BOARD} coreboot" to verify "-DEC_EFS=1" is : used when compiling 2ec_sync.c does it still boot??? -- To view, visit https://review.coreboot.org/c/coreboot/+/43044 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie71bb169242b2511b4a4102d46a6981d72d07806 Gerrit-Change-Number: 43044 Gerrit-PatchSet: 1 Gerrit-Owner: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Daisuke Nojiri <dnojiri(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jul 2020 20:24:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: arch/x86/boot: Jump to payload in protected mode
by Arthur Heymans (Code Review) 06 Jul '20

06 Jul '20

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30118 ) Change subject: arch/x86/boot: Jump to payload in protected mode ...................................................................... Patch Set 32: (2 comments) https://review.coreboot.org/c/coreboot/+/30118/32/Documentation/arch/x86/in… File Documentation/arch/x86/index.md: https://review.coreboot.org/c/coreboot/+/30118/32/Documentation/arch/x86/in… PS32, Line 18: run called/'jumped to' is more accurate? https://review.coreboot.org/c/coreboot/+/30118/32/src/include/program_loadi… File src/include/program_loading.h: https://review.coreboot.org/c/coreboot/+/30118/32/src/include/program_loadi… PS32, Line 177: /* : * Jump to function in protected mode. : * @arg func_ptr Function to jump to in protected mode : * @arg Argument to pass to called function : * : * @noreturn : */ : void protected_mode_jump(uint32_t func_ptr, : uint32_t argument); I'm not sure if this generic program loading file is supposed to hold x86 specific function declarations. -- To view, visit https://review.coreboot.org/c/coreboot/+/30118 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6552ac30f1b6205e08e16d251328e01ce3fbfd14 Gerrit-Change-Number: 30118 Gerrit-PatchSet: 32 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jul 2020 20:02:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2020