coreboot-gerrit March 2024

coreboot-gerrit@coreboot.org

1 participants
3668 discussions

[S] Change in coreboot[main]: amdfwtool: Set the table size only for FWs
by Bao Zheng (Code Review) 15 Mar '24

15 Mar '24

Attention is currently required from: Felix Held, Fred Reitberger, Zheng Bao. Hello Felix Held, Fred Reitberger, Zheng Bao, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81255?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: amdfwtool: Set the table size only for FWs ...................................................................... amdfwtool: Set the table size only for FWs The entry in the table has two categaries, file and pointer. For the pointer, it does not take table space. The ISH, PSP level 2, BIOS table are all the pointer type. So integration function only packs FWs located in folder amd_blobs. And only FWs increase the table size. So the table size is only set once. Later calls only update the count and fletcher. The fill_dir_header can take the parameter count as 0, such PSP level 1 only with ISH-A and ISH-B. It doesn't have any file type entries. This actually reverts https://review.coreboot.org/c/coreboot/+/78274 and add other change. TEST=Identical test on all AMD SOC platform Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 21 insertions(+), 27 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/81255/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/81255?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Gerrit-Change-Number: 81255 Gerrit-PatchSet: 5 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: amdfwtool: Set the table size only for FWs
by Bao Zheng (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Felix Held, Fred Reitberger, Zheng Bao. Hello Felix Held, Fred Reitberger, Zheng Bao, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81255?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: amdfwtool: Set the table size only for FWs ...................................................................... amdfwtool: Set the table size only for FWs The entry in the table has two categaries, file and pointer. For the pointer, it does not take table space. The ISH, PSP level 2, BIOS table are all the pointer type. So integration function only packs FWs located in folder amd_blobs. And only FWs increase the table size. The fill_dir_header can take the parameter count as 0, such PSP level 1 only with ISH-A and ISH-B. It doesn't have any file type entries. This actually reverts https://review.coreboot.org/c/coreboot/+/78274 and add other change. TEST=Identical test on all AMD SOC platform Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 16 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/81255/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/81255?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Gerrit-Change-Number: 81255 Gerrit-PatchSet: 4 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[XS] Change in coreboot[main]: mb/google/brya/var/xol: Disable unused controllers
by SH Kim (Code Review) 14 Mar '24

14 Mar '24

SH Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81105?usp=email ) Change subject: mb/google/brya/var/xol: Disable unused controllers ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/81105/comment/13dff819_50d29353 : PS3, Line 9: proto2 > Remember, you're not supposed to mention phases in public reviews. Thanks, well noticed. -- To view, visit https://review.coreboot.org/c/coreboot/+/81105?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1be7caf8234c32406aa2cff8fc7fe9fa39b16d89 Gerrit-Change-Number: 81105 Gerrit-PatchSet: 3 Gerrit-Owner: SH Kim <sh_.kim(a)samsung.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 04:51:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: amdfwtool: Set the table size only for FWs
by Bao Zheng (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Felix Held, Fred Reitberger, Zheng Bao. Hello Felix Held, Fred Reitberger, Zheng Bao, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81255?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: amdfwtool: Set the table size only for FWs ...................................................................... amdfwtool: Set the table size only for FWs The entry in the table has two categaries, file and pointer. For the pointer, it does not take table space. The ISH, PSP level 2, BIOS table are all the pointer type. So integration function only packs FWs located in folder amd_blobs. And only FWs increase the table size. The fill_dir_header can take the parameter count as 0, such PSP level 1 only with ISH-A and ISH-B. It doesn't have any file type entries. This actually reverts https://review.coreboot.org/c/coreboot/+/78274 and add other change. TEST=Identical test on all AMD SOC platform Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M util/amdfwtool/amdfwtool.c 1 file changed, 18 insertions(+), 26 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/81255/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/81255?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5dfbbb55912c8e37243c351427a8df89c12e5da8 Gerrit-Change-Number: 81255 Gerrit-PatchSet: 3 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Zheng Bao Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions
by Julius Werner (Code Review) 14 Mar '24

14 Mar '24

1 0

[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions
by Julius Werner (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email ) Change subject: lib/device_tree: Add some FDT helper functions ...................................................................... Patch Set 13: (8 comments) Patchset: PS13: BTW can you please fix all the checkpatch errors? File src/include/device_tree.h: https://review.coreboot.org/c/coreboot/+/81081/comment/1fbdb834_d1830d3f : PS13, Line 210: fdt_find_nodes_by_prefix nit: I would choose a different name because all the other `fdt_find_...` functions operate on he whole blob, this just under a specific node. Maybe `fdt_subnodes_with_prefix()`? File src/lib/device_tree.c: https://review.coreboot.org/c/coreboot/+/81081/comment/08c69354_f03399eb : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) { > I fixed the `strncmp` to also check the character after the 'subpath'. […] Okay, then why not use the stack? These path strings aren't gonna be very long. Just use `alloca(strlen(path) + 1)` and then copy it in. https://review.coreboot.org/c/coreboot/+/81081/comment/437924cd_590b1418 : PS12, Line 234: (offset - size) > How would you call the variable? `node_offset` is already taken (but I guess we could just use that […] How about calling it `prop_offset` (because that's where the properties start) and the other one `node_offset`? https://review.coreboot.org/c/coreboot/+/81081/comment/226865dc_6ea4d4b1 : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE); > Using your approach you cannot distinguish between a faulty FDT and just running to the end of your […] I don't really see how your code does anything different? You're still calling `fdt_next_node_name()` and erroring out if it returns 0, just in a way that's harder to read (I think). I also don't think there's a point in going too overboard in error detection. It's not like the firmware can really do much about that anyway. We're already taking a blob pointer without a size and parsing it, so we're already putting all our faith in the FDT being well-formatted anyway. https://review.coreboot.org/c/coreboot/+/81081/comment/5f09353a_f4618bd1 : PS12, Line 337: results[(*count_results)++] = offset; > Done […] Maybe make it return the actual count of entries, but only fill as much as fits in the array? Then the caller can determine that there was an overrun if the function returned a larger number than the array size it put in. (But that might also make badly-written callers read beyond the end of their own array, so maybe it's safer to just ignore that case and always return the number of entries written.) File src/lib/device_tree.c: https://review.coreboot.org/c/coreboot/+/81081/comment/3b73bbc3_d36d8799 : PS13, Line 169: return offset; // return offset to reg property Should this maybe return `count` instead? Otherwise how does the caller know how many addresses it got? https://review.coreboot.org/c/coreboot/+/81081/comment/ba4a9a60_9f4f99bc : PS13, Line 342: if (count_results == 0) nit: This case seems to be redundant with the main return path below. -- To view, visit https://review.coreboot.org/c/coreboot/+/81081?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8 Gerrit-Change-Number: 81081 Gerrit-PatchSet: 13 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 04:16:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/google/brox: Select USE_UNIFIED_AP_FIRMWARE_FOR_UFS_AND_NON_UFS
by Ashish Kumar Mishra (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Karthik Ramasubramanian, Shelley Chen. Ashish Kumar Mishra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81224?usp=email ) Change subject: mb/google/brox: Select USE_UNIFIED_AP_FIRMWARE_FOR_UFS_AND_NON_UFS ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > So I tried applying this patch and was still getting failures on suspend_stress_test. […] We're not seeing any S0ix failures. Are you observing premature wake specifically with this patch? I have seen cros bug update that the general EC premature wake is rootcaused. -- To view, visit https://review.coreboot.org/c/coreboot/+/81224?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Iabd0b3a83aa386e09310b671632368807a4018d4 Gerrit-Change-Number: 81224 Gerrit-PatchSet: 2 Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 04:01:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shelley Chen <shchen(a)google.com> Comment-In-Reply-To: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: symbols: Add __maybe_unused flag to region variable symbols
by Shuo Liu (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Arthur Heymans, Julius Werner, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph. Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81209?usp=email ) Change subject: symbols: Add __maybe_unused flag to region variable symbols ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > Since we received +2 from Julius, let me close the open. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/81209?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ia5ed183b2dd7a474ce51de47dbc1f9e3f61e5a41 Gerrit-Change-Number: 81209 Gerrit-PatchSet: 4 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 03:56:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: symbols: Add __maybe_unused flag to region variable symbols
by Shuo Liu (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Arthur Heymans, Julius Werner, Jérémy Compostella, Lean Sheng Tan, Patrick Rudolph. Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81209?usp=email ) Change subject: symbols: Add __maybe_unused flag to region variable symbols ...................................................................... Patch Set 4: (1 comment) Patchset: PS2: > I like the idea of being able to use this macro directly in the code to localize the symbol usage an […] Since we received +2 from Julius, let me close the open. -- To view, visit https://review.coreboot.org/c/coreboot/+/81209?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ia5ed183b2dd7a474ce51de47dbc1f9e3f61e5a41 Gerrit-Change-Number: 81209 Gerrit-PatchSet: 4 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 03:56:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com> Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: symbols: Add __maybe_unused flag to region variable symbols
by Julius Werner (Code Review) 14 Mar '24

14 Mar '24

Attention is currently required from: Arthur Heymans, Lean Sheng Tan, Patrick Rudolph, Shuo Liu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81209?usp=email ) Change subject: symbols: Add __maybe_unused flag to region variable symbols ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81209?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ia5ed183b2dd7a474ce51de47dbc1f9e3f61e5a41 Gerrit-Change-Number: 81209 Gerrit-PatchSet: 4 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Fri, 15 Mar 2024 03:49:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2024