coreboot-gerrit September 2022

coreboot-gerrit@coreboot.org

1 participants
4678 discussions

[S] Change in coreboot[master]: lint/checkpatch: Fix incorrect camelcase detection on numeric constant
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Elyes Haouas. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67337 ) Change subject: lint/checkpatch: Fix incorrect camelcase detection on numeric constant ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/67337 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I15e1a935665c38b8a2109d412b1d16f935cbb402 Gerrit-Change-Number: 67337 Gerrit-PatchSet: 1 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Mon, 05 Sep 2022 14:34:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in gerrit-avatars[master]: Add avatar for userid: 1004484 (Himanshu Sahdev)
by Himanshu Sahdev (Code Review) 05 Sep '22

05 Sep '22

Himanshu Sahdev has submitted this change. ( https://review.coreboot.org/c/gerrit-avatars/+/67344 ) Change subject: Add avatar for userid: 1004484 (Himanshu Sahdev) ...................................................................... Add avatar for userid: 1004484 (Himanshu Sahdev) Signed-off-by: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Change-Id: Id723adef02680d4334e71298cb099d5ed7ddb3b0 --- A 1004484.jpg 1 file changed, 10 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified; Looks good to me, approved diff --git a/1004484.jpg b/1004484.jpg new file mode 100644 index 0000000..e5e8589 --- /dev/null +++ b/1004484.jpg Binary files differ -- To view, visit https://review.coreboot.org/c/gerrit-avatars/+/67344 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: gerrit-avatars Gerrit-Branch: master Gerrit-Change-Id: Id723adef02680d4334e71298cb099d5ed7ddb3b0 Gerrit-Change-Number: 67344 Gerrit-PatchSet: 1 Gerrit-Owner: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in gerrit-avatars[master]: Add avatar for userid: 1004484 (Himanshu Sahdev)
by Himanshu Sahdev (Code Review) 05 Sep '22

05 Sep '22

Himanshu Sahdev has uploaded this change for review. ( https://review.coreboot.org/c/gerrit-avatars/+/67344 ) Change subject: Add avatar for userid: 1004484 (Himanshu Sahdev) ...................................................................... Add avatar for userid: 1004484 (Himanshu Sahdev) Signed-off-by: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Change-Id: Id723adef02680d4334e71298cb099d5ed7ddb3b0 --- A 1004484.jpg 1 file changed, 10 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/gerrit-avatars refs/changes/44/67344/1 diff --git a/1004484.jpg b/1004484.jpg new file mode 100644 index 0000000..e5e8589 --- /dev/null +++ b/1004484.jpg Binary files differ -- To view, visit https://review.coreboot.org/c/gerrit-avatars/+/67344 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: gerrit-avatars Gerrit-Branch: master Gerrit-Change-Id: Id723adef02680d4334e71298cb099d5ed7ddb3b0 Gerrit-Change-Number: 67344 Gerrit-PatchSet: 1 Gerrit-Owner: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-MessageType: newchange

1 0

[L] Change in coreboot[master]: qualcomm/sc7280: remove unnecessary malloc and early return on failure
by Venkat Thogaru (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Shelley Chen, Venkat Thogaru, Julius Werner, Yu-Ping Wu. Hello Shelley Chen, build bot (Jenkins), Julius Werner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/66975 to look at the new patch set (#4). Change subject: qualcomm/sc7280: remove unnecessary malloc and early return on failure ...................................................................... qualcomm/sc7280: remove unnecessary malloc and early return on failure Instead of just printing the fatal errors, do early return so that boot up time will be reduced during display init failure. Remove malloc allocation and make tu a local variable. BUG=b:182963902,b:216687885 TEST=Validated on qualcomm sc7280 development board. Monitor name: LQ140M1JW49 Change-Id: I51f7a86d143128d2c426fb8940ff34a66152b426 Signed-off-by: Vinod Polimera <quic_vpolimer(a)quicinc.com> --- M src/soc/qualcomm/sc7280/display/edp_ctrl.c 1 file changed, 164 insertions(+), 131 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/66975/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/66975 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I51f7a86d143128d2c426fb8940ff34a66152b426 Gerrit-Change-Number: 66975 Gerrit-PatchSet: 4 Gerrit-Owner: Venkat Thogaru <thogaru(a)qualcomm.corp-partner.google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Vinod Polimera <vpolimer(a)qualcomm.corp-partner.google.com> Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Venkat Thogaru <thogaru(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: allocator_v4: Treat above 4G resources more natively
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/65413 ) Change subject: allocator_v4: Treat above 4G resources more natively ...................................................................... Patch Set 9: (1 comment) Patchset: PS9: > This was merged too soon... […] Was there some sign that I missed? I didn't see it marked as WIP, and it looked like all of the patches below it were merged. -- To view, visit https://review.coreboot.org/c/coreboot/+/65413 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4c7fcd1f5146f6cc287bd3aa5582da55bc5d6955 Gerrit-Change-Number: 65413 Gerrit-PatchSet: 9 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 05 Sep 2022 14:15:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: Revert "allocator_v4: Treat above 4G resources more natively"
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/67329 ) Change subject: Revert "allocator_v4: Treat above 4G resources more natively" ...................................................................... Revert "allocator_v4: Treat above 4G resources more natively" This reverts commit 117e436115484f0ce184114b22b716616592e77e. Depends on top-down allocation to keep the behavior to place hot-plug reservations above 4G. The latter was merged prema- turely, though. Change-Id: I5721cb84b29fc42240dff94f49a94461d88e7fbc Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/coreboot/+/67329 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Lean Sheng Tan <sheng.tan(a)9elements.com> Reviewed-by: Martin L Roth <gaumless(a)gmail.com> --- M src/device/Kconfig M src/device/resource_allocator_v4.c 2 files changed, 187 insertions(+), 30 deletions(-) Approvals: build bot (Jenkins): Verified Martin L Roth: Looks good to me, approved Angel Pons: Looks good to me, approved Lean Sheng Tan: Looks good to me, approved diff --git a/src/device/Kconfig b/src/device/Kconfig index 8d10c17f..af9beb3 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -934,7 +934,7 @@ above 4G boundary as well. config RESOURCE_ALLOCATION_TOP_DOWN - bool "Allocate resources from top down" if !PCIEXP_HOTPLUG_PREFETCH_MEM_ABOVE_4G + bool "Allocate resources from top down" default y depends on RESOURCE_ALLOCATOR_V4 diff --git a/src/device/resource_allocator_v4.c b/src/device/resource_allocator_v4.c index 03cec3e..222f1e9 100644 --- a/src/device/resource_allocator_v4.c +++ b/src/device/resource_allocator_v4.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#include <stdint.h> -#include <commonlib/helpers.h> #include <console/console.h> #include <device/device.h> #include <memrange.h> @@ -24,17 +22,6 @@ return bus && bus->children; } -static resource_t effective_limit(const struct resource *const res) -{ - /* Always allow bridge resources above 4G. */ - if (res->flags & IORESOURCE_BRIDGE) - return res->limit; - - const resource_t quirk_4g_limit = - res->flags & IORESOURCE_ABOVE_4G ? UINT64_MAX : UINT32_MAX; - return MIN(res->limit, quirk_4g_limit); -} - #define res_printk(depth, str, ...) printk(BIOS_DEBUG, "%*c"str, depth, ' ', __VA_ARGS__) /* @@ -108,8 +95,22 @@ * starts at the domain level takes into account all these constraints * thus working on a global view. */ - if (effective_limit(child_res) < bridge_res->limit) - bridge_res->limit = effective_limit(child_res); + if (child_res->limit < bridge_res->limit) + bridge_res->limit = child_res->limit; + + /* + * Propagate the downstream resource request to allocate above 4G + * boundary to upstream bridge resource. This ensures that during + * pass 2, the resource allocator at domain level has a global view + * of all the downstream device requirements and thus address space + * is allocated as per updated flags in the bridge resource. + * + * Since the bridge resource is a single window, all the downstream + * resources of this bridge resource will be allocated in space above + * the 4G boundary. + */ + if (child_res->flags & IORESOURCE_ABOVE_4G) + bridge_res->flags |= IORESOURCE_ABOVE_4G; /* * Alignment value of 0 means that the child resource has no alignment @@ -222,6 +223,129 @@ die("Unexpected resource type: flags(%d)!\n", res->flags); } +/* + * If the resource is NULL or if the resource is not assigned, then it + * cannot be used for allocation for downstream devices. + */ +static bool is_resource_invalid(const struct resource *res) +{ + return (res == NULL) || !(res->flags & IORESOURCE_ASSIGNED); +} + +static void initialize_domain_io_resource_memranges(struct memranges *ranges, + const struct resource *res, + unsigned long memrange_type) +{ + memranges_insert(ranges, res->base, res->limit - res->base + 1, memrange_type); +} + +static void initialize_domain_mem_resource_memranges(struct memranges *ranges, + const struct resource *res, + unsigned long memrange_type) +{ + resource_t res_base; + resource_t res_limit; + + const resource_t limit_4g = 0xffffffff; + + res_base = res->base; + res_limit = res->limit; + + /* + * Split the resource into two separate ranges if it crosses the 4G + * boundary. Memrange type is set differently to ensure that memrange + * does not merge these two ranges. For the range above 4G boundary, + * given memrange type is ORed with IORESOURCE_ABOVE_4G. + */ + if (res_base <= limit_4g) { + + resource_t range_limit; + + /* Clip the resource limit at 4G boundary if necessary. */ + range_limit = MIN(res_limit, limit_4g); + memranges_insert(ranges, res_base, range_limit - res_base + 1, memrange_type); + + /* + * If the resource lies completely below the 4G boundary, nothing more + * needs to be done. + */ + if (res_limit <= limit_4g) + return; + + /* + * If the resource window crosses the 4G boundary, then update res_base + * to add another entry for the range above the boundary. + */ + res_base = limit_4g + 1; + } + + if (res_base > res_limit) + return; + + /* + * If resource lies completely above the 4G boundary or if the resource + * was clipped to add two separate ranges, the range above 4G boundary + * has the resource flag IORESOURCE_ABOVE_4G set. This allows domain to + * handle any downstream requests for resource allocation above 4G + * differently. + */ + memranges_insert(ranges, res_base, res_limit - res_base + 1, + memrange_type | IORESOURCE_ABOVE_4G); +} + +/* + * This function initializes memranges for domain device. If the + * resource crosses 4G boundary, then this function splits it into two + * ranges -- one for the window below 4G and the other for the window + * above 4G. The latter range has IORESOURCE_ABOVE_4G flag set to + * satisfy resource requests from downstream devices for allocations + * above 4G. + */ +static void initialize_domain_memranges(struct memranges *ranges, const struct resource *res, + unsigned long memrange_type) +{ + unsigned char align = get_alignment_by_resource_type(res); + + memranges_init_empty_with_alignment(ranges, NULL, 0, align); + + if (is_resource_invalid(res)) + return; + + if (res->flags & IORESOURCE_IO) + initialize_domain_io_resource_memranges(ranges, res, memrange_type); + else + initialize_domain_mem_resource_memranges(ranges, res, memrange_type); +} + +/* + * This function initializes memranges for bridge device. Unlike domain, + * bridge does not need to care about resource window crossing 4G + * boundary. This is handled by the resource allocator at domain level + * to ensure that all downstream bridges are allocated space either + * above or below 4G boundary as per the state of IORESOURCE_ABOVE_4G + * for the respective bridge resource. + * + * So, this function creates a single range of the entire resource + * window available for the bridge resource. Thus all downstream + * resources of the bridge for the given resource type get allocated + * space from the same window. If there is any downstream resource of + * the bridge which requests allocation above 4G, then all other + * downstream resources of the same type under the bridge get allocated + * above 4G. + */ +static void initialize_bridge_memranges(struct memranges *ranges, const struct resource *res, + unsigned long memrange_type) +{ + unsigned char align = get_alignment_by_resource_type(res); + + memranges_init_empty_with_alignment(ranges, NULL, 0, align); + + if (is_resource_invalid(res)) + return; + + memranges_insert(ranges, res->base, res->limit - res->base + 1, memrange_type); +} + static void print_resource_ranges(const struct device *dev, const struct memranges *ranges) { const struct range_entry *r; @@ -259,13 +383,12 @@ if (!resource->size) continue; - if (memranges_steal(ranges, effective_limit(resource), resource->size, - resource->align, type_match, &resource->base, - allocate_top_down) == false) { + if (memranges_steal(ranges, resource->limit, resource->size, resource->align, + type_match, &resource->base, allocate_top_down) == false) { printk(BIOS_ERR, " ERROR: Resource didn't fit!!! "); printk(BIOS_DEBUG, " %s %02lx * size: 0x%llx limit: %llx %s\n", - dev_path(dev), resource->index, resource->size, - effective_limit(resource), resource2str(resource)); + dev_path(dev), resource->index, + resource->size, resource->limit, resource2str(resource)); continue; } @@ -348,8 +471,8 @@ /* * This function creates a list of memranges of given type using the - * resource that is provided. If the given resource is unassigned or if - * the resource window size is 0, then it creates an empty list. This + * resource that is provided. If the given resource is NULL or if the + * resource window size is 0, then it creates an empty list. This * results in resource allocation for that resource type failing for * all downstream devices since there is nothing to allocate from. * @@ -363,17 +486,16 @@ static void setup_resource_ranges(const struct device *dev, const struct resource *res, unsigned long type, struct memranges *ranges) { - const unsigned char alignment = get_alignment_by_resource_type(res); - printk(BIOS_DEBUG, "%s %s: base: %llx size: %llx align: %d gran: %d limit: %llx\n", dev_path(dev), resource2str(res), res->base, res->size, res->align, res->gran, res->limit); - memranges_init_empty_with_alignment(ranges, NULL, 0, alignment); - if (res->flags & IORESOURCE_ASSIGNED) - memranges_insert(ranges, res->base, res->limit - res->base + 1, type); - if (dev->path.type == DEVICE_PATH_DOMAIN) + if (dev->path.type == DEVICE_PATH_DOMAIN) { + initialize_domain_memranges(ranges, res, type); constrain_domain_resources(dev, ranges, type); + } else { + initialize_bridge_memranges(ranges, res, type); + } print_resource_ranges(dev, ranges); } @@ -486,12 +608,26 @@ * the resource allocation at domain level considers mem and prefmem * together when finding the best fit based on the biggest resource * requirement. + * + * However, resource requests for allocation above 4G boundary need to + * be handled separately if the domain resource window crosses this + * boundary. There is a single window for resource of type + * IORESOURCE_MEM. When creating memranges, this resource is split into + * two separate ranges -- one for the window below 4G boundary and other + * for the window above 4G boundary (with IORESOURCE_ABOVE_4G flag set). + * Thus, when allocating child resources, requests for below and above + * the 4G boundary are handled separately by setting the type_mask and + * type_match to allocate_child_resources() accordingly. */ res = find_domain_resource(domain, IORESOURCE_MEM); if (res) { setup_resource_ranges(domain, res, IORESOURCE_MEM, &ranges); allocate_child_resources(domain->link_list, &ranges, - IORESOURCE_TYPE_MASK, IORESOURCE_MEM); + IORESOURCE_TYPE_MASK | IORESOURCE_ABOVE_4G, + IORESOURCE_MEM); + allocate_child_resources(domain->link_list, &ranges, + IORESOURCE_TYPE_MASK | IORESOURCE_ABOVE_4G, + IORESOURCE_MEM | IORESOURCE_ABOVE_4G); cleanup_resource_ranges(domain, &ranges, res); } -- To view, visit https://review.coreboot.org/c/coreboot/+/67329 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5721cb84b29fc42240dff94f49a94461d88e7fbc Gerrit-Change-Number: 67329 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: Revert "allocator_v4: Treat above 4G resources more natively"
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Attention is currently required from: Nico Huber, Tim Wawrzynczak, Arthur Heymans. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67329 ) Change subject: Revert "allocator_v4: Treat above 4G resources more natively" ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/67329 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5721cb84b29fc42240dff94f49a94461d88e7fbc Gerrit-Change-Number: 67329 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Mon, 05 Sep 2022 14:10:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: cpu/amd/agesa/*/fixme.c: Drop some white space issues
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/65887 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cpu/amd/agesa/*/fixme.c: Drop some white space issues ...................................................................... cpu/amd/agesa/*/fixme.c: Drop some white space issues Signed-off-by: Elyes Haouas <ehaouas(a)noos.fr> Change-Id: I335b5ac415b76067c70c8e2e6f1703eb10f69c50 --- M src/cpu/amd/agesa/family14/fixme.c M src/cpu/amd/agesa/family16kb/fixme.c 2 files changed, 18 insertions(+), 8 deletions(-) Approvals: Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved build bot (Jenkins): Verified diff --git a/src/cpu/amd/agesa/family14/fixme.c b/src/cpu/amd/agesa/family14/fixme.c index 16caf40..4c16cc3 100644 --- a/src/cpu/amd/agesa/family14/fixme.c +++ b/src/cpu/amd/agesa/family14/fixme.c @@ -39,7 +39,7 @@ LibAmdMsrRead(TOP_MEM, &MsrReg, &StdHeader); MsrReg = (MsrReg >> 8) | 3; PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0x88); - PciData = (UINT32) MsrReg; + PciData = (UINT32)MsrReg; LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); /* Send all IO (0000-FFFF) to southbridge. */ diff --git a/src/cpu/amd/agesa/family16kb/fixme.c b/src/cpu/amd/agesa/family16kb/fixme.c index 5e43b17..2960f6e 100644 --- a/src/cpu/amd/agesa/family16kb/fixme.c +++ b/src/cpu/amd/agesa/family16kb/fixme.c @@ -16,7 +16,7 @@ AMD_CONFIG_PARAMS StdHeader; /* Enable legacy video routing: D18F1xF4 VGA Enable */ - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0xF4); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0xF4); PciData = 1; LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); @@ -24,29 +24,29 @@ * devices (TPM, HPET, BIOS RAM, Watchdog Timer, I/O APIC and ACPI) are * set to non-posted regions. */ - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0x84); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0x84); PciData = 0x00FEDF00; /* last address before processor local APIC at FEE00000 */ PciData |= 1 << 7; /* set NP (non-posted) bit */ LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0x80); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0x80); PciData = (HPET_BASE_ADDRESS >> 8) | 3; /* lowest NP address is HPET at FED00000 */ LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); /* Map the remaining PCI hole as posted MMIO */ - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0x8C); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0x8C); PciData = 0x00FECF00; /* last address before non-posted range */ LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); LibAmdMsrRead(TOP_MEM, &MsrReg, &StdHeader); MsrReg = (MsrReg >> 8) | 3; - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0x88); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0x88); PciData = (UINT32)MsrReg; LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); /* Send all IO (0000-FFFF) to southbridge. */ - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0xC4); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0xC4); PciData = 0x0000F000; LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); - PciAddress.AddressValue = MAKE_SBDFO (0, 0, 0x18, 1, 0xC0); + PciAddress.AddressValue = MAKE_SBDFO(0, 0, 0x18, 1, 0xC0); PciData = 0x00000003; LibAmdPciWrite(AccessWidth32, PciAddress, &PciData, &StdHeader); } -- To view, visit https://review.coreboot.org/c/coreboot/+/65887 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I335b5ac415b76067c70c8e2e6f1703eb10f69c50 Gerrit-Change-Number: 65887 Gerrit-PatchSet: 3 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: soc/intel/alderlake: Remove dependency of FSP-S CpuMpPei Module
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/66706 ) Change subject: soc/intel/alderlake: Remove dependency of FSP-S CpuMpPei Module ...................................................................... soc/intel/alderlake: Remove dependency of FSP-S CpuMpPei Module This patch fixes a hidden issue present inside FSP-S while coreboot decides to skip performing MP initialization by overriding FSP-S UPDs as below: 1. CpuMpPpi ------> Passing `NULL` as coreboot assume FSP don't need to use coreboot wrapper for performing any operation over APs. 2. SkipMpInit -----> Set `1` to let FSP know that coreboot decided to skip FSP running CPU feature programming. Unfortunately, the assumption of coreboot is not aligned with FSP when it comes to the behaviour of `CpuMpPpi` UPD. FSP assumes ownership of the APs (Application Processors) upon passing `NULL` pointer to the `CpuMpPpi` FSP-S UPD. FSP-S creates its own infrastructure code after seeing the CpuMpPpi UPD is set to `NULL`. FSP requires the CpuMpPei module, file name `UefiCpuPkg/CpuMpPei/CpuMpPei.c`, function name `InitializeCpuMpWorker` to perform those additional initialization which is not relevant for the coreboot upon selecting the SkipMpInit UPD to 1 (a.k.a avoid running CPU feature programming on APs). Additionally, FSP-S binary size has increased by ~30KB (irrespective of being compressed) with the inclusion of the CpuMpPei module, which is eventually not meaningful for coreboot. Hence, this patch selects `MP_SERVICES_PPI_V2_NOOP` config unconditionally to ensure pass a valid pointer to the `CpuMpPpi` UPD and avoid APs getting hijacked by FSP while coreboot decides to set SkipMpInit UPD. Ideally, FSP should have avoided all AP related operations when coreboot requested FSP to skip MP init by overriding required UPDs. TEST=Able to drop CpuMpPei Module from FSP and boot to Chrome OS on Google/Redrix, Kano, Taeko devices with SkipMpInit=1. Without this patch: Here is the CPU AP logs coming from the EDK2 (open-source) [UefiCpuPkg/CpuMpPei/CpuMpPei.c] when coreboot sets `NULL` to the CpuMpPpi UPD. [SPEW ] Loading PEIM EDADEB9D-DDBA-48BD-9D22-C1C169C8C5C6 [SPEW ] Loading PEIM at 0x00076F9A000 EntryPoint=0x00076FA24E2 CpuMpPei.efi PROGRESS CODE: V03020002 I0 [SPEW ] Register PPI Notify: F894643D-C449-42D1-8EA8-85BDD8C65BDE [SPEW ] Notify: PPI Guid: F894643D-C449-42D1-8EA8-85BDD8C65BDE, Peim notify entry point: 76FA0239 AP Loop Mode is 2 GetMicrocodePatchInfoFromHob: Microcode patch cache HOB is not found. CPU[0000]: Microcode revision = 00000000, expected = 00000000 [SPEW ] Register PPI Notify: 8F9D4825-797D-48FC-8471-845025792EF6 Does not find any stored CPU BIST information from PPI! APICID - 0x00000000, BIST - 0x00000000 [SPEW ] Install PPI: 9E9F374B-8F16-4230-9824-5846EE766A97 [SPEW ] Install PPI: 5CB9CB3D-31A4-480C-9498-29D269BACFBA [SPEW ] Install PPI: EE16160A-E8BE-47A6-820A-C6900DB0250A PROGRESS CODE: V03020003 I0 With this patch: No instance of `CpuMpPei` has been found in the AP UART log with FSP debug enabled. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I8ebe0bcfda513e79e791df7ab54b357aa23d295c Reviewed-on: https://review.coreboot.org/c/coreboot/+/66706 Reviewed-by: Lean Sheng Tan <sheng.tan(a)9elements.com> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/soc/intel/alderlake/Kconfig M src/soc/intel/alderlake/fsp_params.c 2 files changed, 97 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved Lean Sheng Tan: Looks good to me, but someone else must approve diff --git a/src/soc/intel/alderlake/Kconfig b/src/soc/intel/alderlake/Kconfig index f2b7d6f..c79eb08 100644 --- a/src/soc/intel/alderlake/Kconfig +++ b/src/soc/intel/alderlake/Kconfig @@ -430,6 +430,13 @@ config USE_COREBOOT_MP_INIT bool "Use coreboot MP init" + # FSP assumes ownership of the APs (Application Processors) + # upon passing `NULL` pointer to the CpuMpPpi FSP-S UPD. + # Hence, select `MP_SERVICES_PPI_V2_NOOP` config to pass a valid + # pointer to the CpuMpPpi UPD with FSP_UNSUPPORTED type APIs. + # This will protect APs from getting hijacked by FSP while coreboot + # decides to set SkipMpInit UPD. + select MP_SERVICES_PPI_V2_NOOP select RELOAD_MICROCODE_PATCH help Upon selection, coreboot performs MP Init. diff --git a/src/soc/intel/alderlake/fsp_params.c b/src/soc/intel/alderlake/fsp_params.c index 6826306..dd17e33 100644 --- a/src/soc/intel/alderlake/fsp_params.c +++ b/src/soc/intel/alderlake/fsp_params.c @@ -586,22 +586,23 @@ static void fill_fsps_cpu_params(FSP_S_CONFIG *s_cfg, const struct soc_intel_alderlake_config *config) { - if (CONFIG(USE_FSP_MP_INIT)) { + /* + * FIXME: FSP assumes ownership of the APs (Application Processors) + * upon passing `NULL` pointer to the CpuMpPpi FSP-S UPD. + * Hence, pass a valid pointer to the CpuMpPpi UPD unconditionally. + * This would avoid APs from getting hijacked by FSP while coreboot + * decides to set SkipMpInit UPD. + */ + s_cfg->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); + + if (CONFIG(USE_FSP_MP_INIT)) /* * Fill `2nd microcode loading FSP UPD` if FSP is running CPU feature * programming. */ fill_fsps_microcode_params(s_cfg, config); - /* - * Use FSP running MP PPI services to perform CPU feature programming - * if Kconfig is enabled - */ - s_cfg->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); - } else { - /* Use coreboot native driver to perform MP init by default */ - s_cfg->CpuMpPpi = (uintptr_t)NULL; + else s_cfg->SkipMpInit = !CONFIG(USE_INTEL_FSP_MP_INIT); - } } static void fill_fsps_igd_params(FSP_S_CONFIG *s_cfg, -- To view, visit https://review.coreboot.org/c/coreboot/+/66706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8ebe0bcfda513e79e791df7ab54b357aa23d295c Gerrit-Change-Number: 66706 Gerrit-PatchSet: 7 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: ec/google/chromeec: Modify ufp from type-c role
by Martin L Roth (Code Review) 05 Sep '22

05 Sep '22

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/67168 ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: ec/google/chromeec: Modify ufp from type-c role ...................................................................... ec/google/chromeec: Modify ufp from type-c role In order to fix the USB port of type-C dongle has no function after reboot/shutdown, modify ufp which is in google_chromeec_usb_pd_get_info from the bit1 of type-c role (PD_CTRL_RESP_ROLE_DATA). BUG=b:239138412 TEST=Built coreboot image and verified that using this patch can detect usb drive after reboot. Signed-off-by: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Change-Id: I73a4a6ec37129388783599125f067068d155d93f Reviewed-on: https://review.coreboot.org/c/coreboot/+/67168 Reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/ec/google/chromeec/ec.c 1 file changed, 22 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 6604abd..7ee2300 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -1462,7 +1462,7 @@ if (google_chromeec_command(&cmd) < 0) return -1; - *ufp = (resp.cc_state == PD_CC_DFP_ATTACHED); + *ufp = !(resp.role & PD_CTRL_RESP_ROLE_DATA); *dbg_acc = (resp.cc_state == PD_CC_DFP_DEBUG_ACC); *active_cable = !!(resp.control_flags & USB_PD_CTRL_ACTIVE_CABLE); *dp_mode = resp.dp_mode; -- To view, visit https://review.coreboot.org/c/coreboot/+/67168 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I73a4a6ec37129388783599125f067068d155d93f Gerrit-Change-Number: 67168 Gerrit-PatchSet: 5 Gerrit-Owner: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Ricky Chang <rickytlchang(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit September 2022