coreboot-gerrit July 2021

coreboot-gerrit@coreboot.org

1 participants
5824 discussions

Change in coreboot[master]: soc/amd/common/block/lpc/spi_dma: Implement readat_async
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Marshall Dawson, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/55857 to look at the new patch set (#7). Change subject: soc/amd/common/block/lpc/spi_dma: Implement readat_async ...................................................................... soc/amd/common/block/lpc/spi_dma: Implement readat_async This change implements the region_device async readat API. The implementation is a simple state machine that iterates over a queue starting new transactions whenever the previous one has completed. There is a magic bit that needs to be set for the SPI DMA controller to function correctly. This is only available in RN/CZN+. BUG=b:179699789 TEST=Boot guybrush to OS Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa00 --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/common/block/lpc/spi_dma.c 2 files changed, 282 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/55857/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/55857 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0be555956581fd82bbe1482d8afa8828c61aaa00 Gerrit-Change-Number: 55857 Gerrit-PatchSet: 7 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: commonlib/region: Add support for asynchronous readat
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Raul Rangel, Furquan Shaikh, Marshall Dawson, Julius Werner, Angel Pons. Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Julius Werner, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/55856 to look at the new patch set (#6). Change subject: commonlib/region: Add support for asynchronous readat ...................................................................... commonlib/region: Add support for asynchronous readat This CL adds support asynchronous readat support for region_devices. It uses the new futures API. Asynchronous operations be be performed in the background leaving the AP free to perform other work. AMD SoCs have a dedicated SPI DMA controller that can be used to perform asynchronous reads. The goal is to load certain stages, payloads, and oproms asynchronously into a cache before the items are actually needed. This will reduce the amount of time we spend waiting for SPI transactions. BUG=b:179699789 TEST=compiles Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I9702c9e6abe992ea401fa08ba71e5080c3c29d9a --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c 2 files changed, 51 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/55856/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/55856 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9702c9e6abe992ea401fa08ba71e5080c3c29d9a Gerrit-Change-Number: 55856 Gerrit-PatchSet: 6 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: lib/future: Implement Futures API
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Julius Werner, Eric Peers. Hello build bot (Jenkins), Aaron Durbin, Furquan Shaikh, Marshall Dawson, Julius Werner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56048 to look at the new patch set (#4). Change subject: lib/future: Implement Futures API ...................................................................... lib/future: Implement Futures API This is implemented in coreboot instead of commonlib because commonlib doesn't have things like printk, die and stopwatch. A later CL will add calls to poll_futures to hardwaremain.c. BUG=b:177909625 TEST=boot guybrush and see payload read/decompress time drop 23ms. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I350aebcd07024a00b90495bf937f8bca5193d02f --- M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/future.c 3 files changed, 142 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/56048/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/56048 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I350aebcd07024a00b90495bf937f8bca5193d02f Gerrit-Change-Number: 56048 Gerrit-PatchSet: 4 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Martin Roth <martinroth(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: commonlib/bsd/future: Introduce future API
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Aaron Durbin, Furquan Shaikh, Marshall Dawson, Angel Pons, Julius Werner, Eric Peers. Hello build bot (Jenkins), Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Julius Werner, Angel Pons, Eric Peers, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56047 to look at the new patch set (#3). Change subject: commonlib/bsd/future: Introduce future API ...................................................................... commonlib/bsd/future: Introduce future API This CL introduces a futures API. It allows modeling asynchronous operations. This will make it possible to model DMA transfers, I2C transactions, EC operations, etc. It was loosely modeled off the Rust Futures API: https://rust-lang.github.io/async-book/02_execution/02_future.html Since we don't use interrupts while booting, the only option we have is to poll. For this reason I omitted the `wake` callback since there is nothing to call it. I evaluated the existing threads.h API and it has the following down sides: * Each thread requires its own stack. This means that there is a pretty low limit to the number of concurrent async operations, and a lot of overhead. * There are no thread synchronization primitives (e.g., thread handles, join, etc), so it's difficult to wait for an asynchronous operation to complete. * It currently only works in ramstage. * It requires code to be reentrant since we can now have multiple threads of executing going through the same code. This will require adding support for mutexes to make sure critical sections are protected. Auditing this is complicated and error prone. * It makes it hard to tell from looking at the code if something is running in a different thread. * Timestamp tracking becomes difficult because we can context switch to a task that hogs the CPU for a while. * The code was introduced in 2013, and is not currently used by any SoC or mainboard in the tree. It looks like what ever features depended on this were removed. So maybe we should also remove threads.h? The futures API has the following advantages: * We can easily scale the number of async operations since each operation only needs to store the context to run its state machine. * It provides a synchronization primitive so it's possible to block until the operation is complete. * It works in all stages. * There is no need for mutexes since there is only 1 stack and thread of execution. * Async operations are explicitly modeled so it's clear if the code is asynchronous. * The busy_loop parameter allows putting off CPU intensive operations until they are required. This will allow for proper timestamp accounting. * Creating futures that depend on other futures is pretty simple. BUG=b:179699789 TEST=Implemented the API for rdev and cbfs, it seems pretty ergonomic Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I125a946233f122c2848704296efbcb3ca3c96079 --- A src/commonlib/bsd/include/commonlib/bsd/future.h 1 file changed, 86 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/56047/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/56047 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I125a946233f122c2848704296efbcb3ca3c96079 Gerrit-Change-Number: 56047 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Eric Peers <epeers(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/alderlake: Switch to runtime generation of Intel Power Engine
by Cliff Huang (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Tim Wawrzynczak, Patrick Rudolph. Cliff Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56006 ) Change subject: soc/intel/alderlake: Switch to runtime generation of Intel Power Engine ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/alderlake/pmc.c: https://review.coreboot.org/c/coreboot/+/56006/comment/b96f3de5_65138a23 PS1, Line 126: /* Add Intel Power Engine device */ > `generate_acpi_power_engine` will not be compiled in unless `SOC_INTEL_COMMON_BLOCK_ACPI_PEP` is sel […] If SOC_INTEL_COMMON_BLOCK_ACPI_PEP is unselected temporary for some test reason, this line also need to be commented out in order to build. -- To view, visit https://review.coreboot.org/c/coreboot/+/56006 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I617bc3d1c3cf4ac6b6cbbd790dcf62e731024834 Gerrit-Change-Number: 56006 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 07 Jul 2021 17:54:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/../car: Calculate SF Mask#1 based on MSR 0xc87
by Tim Wawrzynczak (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Furquan Shaikh, Subrata Banik, Nick Vaccaro, EricR Lai. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51374 ) Change subject: soc/intel/common/../car: Calculate SF Mask#1 based on MSR 0xc87 ...................................................................... Patch Set 8: (2 comments) File src/soc/intel/common/block/cpu/car/cache_as_ram.S: https://review.coreboot.org/c/coreboot/+/51374/comment/8e925e5b_61b3c2e1 PS8, Line 527: 2. Set SF_MASK_1 = ((1 << SFWayCnt) - 1) - ((1 << data_ways) - 1) This can be simplified further to remove 1 subtraction: `((1 << (SFWayCnt - data_ways)) - 1) << data_ways)` It's the same number of shifts, but 1 fewer subtraction, which IMO it's almost always good to reduce instruction count in CAR, see below for a sequence that should work; I've also worked it to remove the need to save `ebx` https://review.coreboot.org/c/coreboot/+/51374/comment/83bee73e_8c4265a6 PS8, Line 532: mov %al, %cl : mov $0x01, %eax : shl %cl, %eax : subl $0x01, %eax /* contains ((1 << SFWayCnt) - 1) */ : mov %bl, %cl : mov $0x01, %ebx : shl %cl, %ebx : subl $0x01, %ebx /* contains ((1 << data_ways) - 1) */ : sub %ebx, %eax /* contains SF mask */ `edx` is also free at this point, so I think that could be used instead of backing of `ebx` and using it, combined with the above, does this work? I think this saves something like 4 instructions, including the restore on 549 ``` sequence: ebx == non-eviction mask ecx == number of ways /* save number of ways to edi because rdmsr has to use ecx */ mov %ecx, %edi /* edi == data_ways */ mov $IA32_SF_QOS_INFO, %ecx /* ecx == IA32_SF_QOS_INFO */ rdmsr and $0x3f, %eax /* eax == SFWayCnt */ mov %ecx, %edx /* ecx == SFWayCnt */ subl %ecx, %edi /* ecx == SFWayCnt - data_ways */ movl $0x01, %eax /* eax == 1 */ shl $cl, %eax /* eax == 1 << (SFWayCnt - data_ways) */ subl $0x01, %eax /* eax == (1 << (SFWayCnt - data_ways)) - 1) */ mov %edi, %ecx /* ecx == data_ways */ shl %cl, %eax /* eax == ((1 << (SFWayCnt - data_ways)) - 1)) << data_ways */ ``` and then the mask value we want is in eax ready to be used with `wrmsr` WDYT? -- To view, visit https://review.coreboot.org/c/coreboot/+/51374 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifd0b7e1a90cad4a4837adf6067fe8301dcd0a941 Gerrit-Change-Number: 51374 Gerrit-PatchSet: 8 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 17:47:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP] mb/intel/adlrvp: Enable audio SMBIOS OEM string support for ADL...
by Anil Kumar K (Code Review) 07 Jul '21

07 Jul '21

Anil Kumar K has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/55369 ) Change subject: [WIP] mb/intel/adlrvp: Enable audio SMBIOS OEM string support for ADL-M RVP ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/coreboot/+/55369 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3af79fc0120a52f3e9947412030b382e78aa12fd Gerrit-Change-Number: 55369 Gerrit-PatchSet: 1 Gerrit-Owner: Anil Kumar K <anil.kumar.k(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jairaj Arava <jairaj.arava(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: abandon

1 0

Change in coreboot[master]: commonlib/region: Add support for asynchronous readat
by build bot (Jenkins) (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Raul Rangel, Furquan Shaikh, Marshall Dawson, Julius Werner, Angel Pons. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55856 ) Change subject: commonlib/region: Add support for asynchronous readat ...................................................................... Patch Set 5: (6 comments) File src/commonlib/include/commonlib/region.h: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/efe94026_70221cf5 PS5, Line 98: cb_err_t (*readat_async)(const struct region_device *, function definition argument 'const struct region_device *' should also have an identifier name Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/83861f5e_f7dbf1a8 PS5, Line 98: cb_err_t (*readat_async)(const struct region_device *, function definition argument 'struct rdev_readat_async_context *' should also have an identifier name Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/d6868a7e_ac82e75c PS5, Line 98: cb_err_t (*readat_async)(const struct region_device *, function definition argument 'void *' should also have an identifier name Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/143b715a_a2c8281e PS5, Line 98: cb_err_t (*readat_async)(const struct region_device *, function definition argument 'size_t' should also have an identifier name Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/65844a66_00dd3347 PS5, Line 98: cb_err_t (*readat_async)(const struct region_device *, function definition argument 'size_t' should also have an identifier name File src/commonlib/region.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-123573): https://review.coreboot.org/c/coreboot/+/55856/comment/41803724_149f3000 PS5, Line 136: cb_err_t rdev_readat_async(const struct region_device *rd, struct rdev_readat_async_context *context, line over 96 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/55856 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9702c9e6abe992ea401fa08ba71e5080c3c29d9a Gerrit-Change-Number: 55856 Gerrit-PatchSet: 5 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 17:21:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib/future: Implement Futures API
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Julius Werner, Eric Peers. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56048 ) Change subject: lib/future: Implement Futures API ...................................................................... Patch Set 3: (4 comments) File src/lib/future.c: https://review.coreboot.org/c/coreboot/+/56048/comment/39586af5_d46c6968 PS2, Line 14: static struct future *futures[8]; > you never init this to NULL but count on it being null. […] BSS section is guaranteed to be 0 before c main() starts. So no need to explicitly set it. https://review.coreboot.org/c/coreboot/+/56048/comment/2fca3839_ccd2cf53 PS2, Line 34: die > Maybe assert? This shouldn't be a die in my opinion - it should be the same as not having the confi […] Good point. https://review.coreboot.org/c/coreboot/+/56048/comment/e26aa4ae_89a297f7 PS2, Line 54: die > Similar to the last die - Maybe assert and return? This is where I dislike the assert API... If fatal asserts are disabled, then we continue and bad things will happen. So we need to add a conditional to check the assert again afterwards. https://review.coreboot.org/c/coreboot/+/56048/comment/989aa6ac_8dce1562 PS2, Line 68: state = futures[i]->poll(futures[i], false); > For debug, maybe add an optional print before and after and include the amount of time it took? So I was tempted to add a bunch of debug style information in the the futures object. i.e., start time, total poll time, end time, etc. I didn't since I wanted to keep the API and object model very simple. I didn't add any prints here since it will get called in a tight loop by `wait_for_future`, so it could potentially produce a lot of output. Do you still think I should do it? -- To view, visit https://review.coreboot.org/c/coreboot/+/56048 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I350aebcd07024a00b90495bf937f8bca5193d02f Gerrit-Change-Number: 56048 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Martin Roth <martinroth(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Comment-Date: Wed, 07 Jul 2021 17:05:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth <martinroth(a)google.com> Comment-In-Reply-To: Eric Peers <epeers(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: lib/future: Implement Futures API
by Raul Rangel (Code Review) 07 Jul '21

07 Jul '21

Attention is currently required from: Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Julius Werner, Eric Peers. Hello build bot (Jenkins), Aaron Durbin, Furquan Shaikh, Marshall Dawson, Julius Werner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/56048 to look at the new patch set (#3). Change subject: lib/future: Implement Futures API ...................................................................... lib/future: Implement Futures API This is implemented in coreboot instead of commonlib because commonlib doesn't have things like printk, die and stopwatch. A later CL will add calls to poll_futures to hardwaremain.c. BUG=b:177909625 TEST=boot guybrush and see payload read/decompress time drop 23ms. Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> Change-Id: I350aebcd07024a00b90495bf937f8bca5193d02f --- M src/lib/Kconfig M src/lib/Makefile.inc A src/lib/future.c 3 files changed, 142 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/56048/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/56048 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I350aebcd07024a00b90495bf937f8bca5193d02f Gerrit-Change-Number: 56048 Gerrit-PatchSet: 3 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Martin Roth <martinroth(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aaron Durbin <adurbin(a)gmail.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit July 2021