coreboot-gerrit March 2022

coreboot-gerrit@coreboot.org

1 participants
6700 discussions

Change in coreboot[master]: amdfwtool: Detect the file name with absolute path in fw.cfg
by Bao Zheng (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Raul Rangel, Martin Roth, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held. Hello Jason Glenesk, build bot (Jenkins), Raul Rangel, Martin Roth, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/61971 to look at the new patch set (#7). Change subject: amdfwtool: Detect the file name with absolute path in fw.cfg ...................................................................... amdfwtool: Detect the file name with absolute path in fw.cfg Here is the example to add a file which is not in the default path. -------------- XXXXX_FILE 3rdparty/blobs/mainboard/google/guybrush/TypeId0xXXX.sbin ---------------- If the filename in fw.cfg contains a '/', we will treat it as an absolute (actually relative to top) path. Change-Id: Ia8e40e445556b972f3b00a0dfcc07b1512308177 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M Makefile.inc M src/soc/amd/common/Makefile.inc M util/amdfwtool/data_parse.c 3 files changed, 14 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/61971/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/61971 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia8e40e445556b972f3b00a0dfcc07b1512308177 Gerrit-Change-Number: 61971 Gerrit-PatchSet: 7 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: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Zheng Bao 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: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> 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

Change in coreboot[master]: mb/google/corsola: Revise power-on sequence of PS8640
by Rex-BC Chen (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Hung-Te Lin, Paul Menzel, Yu-Ping Wu. Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/62893 to look at the new patch set (#5). Change subject: mb/google/corsola: Revise power-on sequence of PS8640 ...................................................................... mb/google/corsola: Revise power-on sequence of PS8640 The power-on sequence of PS8640 needs to be modified because the waveform of power-on sequence do not meet the spec of PS8640. Datasheet name: PS8640_DS_V1.4_20200210.docx. Chapter: 14. BUG=b:222650141 TEST=show fw display normally in krabby. TEST=result of waveform meets the spec. Signed-off-by: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Change-Id: I7706c56dc7fc13ac84c0d52a6e534bc0988e8fd3 --- M src/mainboard/google/corsola/display.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/62893/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/62893 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7706c56dc7fc13ac84c0d52a6e534bc0988e8fd3 Gerrit-Change-Number: 62893 Gerrit-PatchSet: 5 Gerrit-Owner: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/corsola: Revise power-on sequence of PS8640
by Rex-BC Chen (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Hung-Te Lin, Paul Menzel, Yu-Ping Wu. Rex-BC Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62893 ) Change subject: mb/google/corsola: Revise power-on sequence of PS8640 ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/62893/comment/3dba902b_cd54dd4d PS3, Line 11: > So there is no real problem to be fixed? yes, it did cause some issue and I add the reason to commit message: The power-on sequence of PS8640 needs to be modified because the waveform of power-on sequence do not meet the spec for PS8640. https://review.coreboot.org/c/coreboot/+/62893/comment/37fb1455_e29c8031 PS3, Line 13: TEST=show fw display normally in krabby. > This adds 110 ms of delay? If so, please document that. done, I add the spec name and chapter in commit message. Patchset: PS1: > Keep this open. done, I add the spec name and chapter in commit message. -- To view, visit https://review.coreboot.org/c/coreboot/+/62893 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7706c56dc7fc13ac84c0d52a6e534bc0988e8fd3 Gerrit-Change-Number: 62893 Gerrit-PatchSet: 4 Gerrit-Owner: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Fri, 18 Mar 2022 02:57:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: $top/Makefile.inc: "-include" the folder common before other subdirs
by Bao Zheng (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Marshall Dawson, Paul Menzel, Julius Werner, Zheng Bao, Elyes Haouas, Felix Held. Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62750 ) Change subject: $top/Makefile.inc: "-include" the folder common before other subdirs ...................................................................... Patch Set 8: (1 comment) Patchset: PS8: subdirs-y+=$(wildcard src/drivers/*/*) Actually this definition will get some non-existing Makefile.inc, because there is not only subdir but some regular files. Then we get Makefile.inc like src/driver/usb/Kconfig/Makefile.inc This will also be eliminated by "-include". -- To view, visit https://review.coreboot.org/c/coreboot/+/62750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I99597af22cac6d12aaef348789664cd7db02ba06 Gerrit-Change-Number: 62750 Gerrit-PatchSet: 8 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> 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: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Zheng Bao Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 18 Mar 2022 02:57:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/corsola: Revise power-on sequence of PS8640
by Rex-BC Chen (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Hung-Te Lin, Rex-BC Chen. Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/62893 to look at the new patch set (#4). Change subject: mb/google/corsola: Revise power-on sequence of PS8640 ...................................................................... mb/google/corsola: Revise power-on sequence of PS8640 The power-on sequence of PS8640 needs to be modified because the waveform of power-on sequence do not meet the spec for PS8640. Described in chapter 14, PS8640_DS_V1.4_20200210.docx. BUG=b:222650141 TEST=show fw display normally in krabby. TEST=result of waveform meets PS8640 spec. Signed-off-by: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Change-Id: I7706c56dc7fc13ac84c0d52a6e534bc0988e8fd3 --- M src/mainboard/google/corsola/display.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/62893/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/62893 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7706c56dc7fc13ac84c0d52a6e534bc0988e8fd3 Gerrit-Change-Number: 62893 Gerrit-PatchSet: 4 Gerrit-Owner: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: amd/*/Makefile.inc: Put common words into common Makefile.inc
by Bao Zheng (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Marshall Dawson, Zheng Bao, Fred Reitberger, Elyes Haouas, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/62658 to look at the new patch set (#9). Change subject: amd/*/Makefile.inc: Put common words into common Makefile.inc ...................................................................... amd/*/Makefile.inc: Put common words into common Makefile.inc Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M src/soc/amd/cezanne/Makefile.inc M src/soc/amd/common/Makefile.inc M src/soc/amd/picasso/Makefile.inc M src/soc/amd/sabrina/Makefile.inc M src/soc/amd/stoneyridge/Makefile.inc M src/southbridge/amd/pi/hudson/Makefile.inc 6 files changed, 11 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/62658/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/62658 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5a0ea27002e09d0b879bafad37a5d418ddb4e644 Gerrit-Change-Number: 62658 Gerrit-PatchSet: 9 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Zheng Bao 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> 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

Change in coreboot[master]: $top/Makefile.inc: "-include" the folder common before other subdirs
by Bao Zheng (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Marshall Dawson, Paul Menzel, Julius Werner, Zheng Bao, Elyes Haouas, Felix Held. Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Martin Roth, Marshall Dawson, Paul Menzel, Zheng Bao, Elyes Haouas, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/62750 to look at the new patch set (#8). Change subject: $top/Makefile.inc: "-include" the folder common before other subdirs ...................................................................... $top/Makefile.inc: "-include" the folder common before other subdirs Put src/soc/*/common before src/soc/*/*, can make the variables in common Makefile get the expected value before they are used in other subdirs. The result subdirs-y gets duplicated "common", which is eliminated later by 1. "-include" directive when creating the list of Makefile.inc. 2. sort function when create xxx-src variable. Then we can put some common variables from all the subdir Makefile.inc to the common Makefile.inc to reduce code redundancy. Change-Id: I99597af22cac6d12aaef348789664cd7db02ba06 Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com> --- M Makefile.inc 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/62750/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/62750 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I99597af22cac6d12aaef348789664cd7db02ba06 Gerrit-Change-Number: 62750 Gerrit-PatchSet: 8 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> 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: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Zheng Bao Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: mb/google/brya/vell: Move WWAN devices for vell
by Tim Wawrzynczak (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Cliff Huang, Paul Menzel, Robert Chen. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62771 ) Change subject: mb/google/brya/vell: Move WWAN devices for vell ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/62771 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If27abcf31ed948899bfaecbe8ef494fe8a80609b Gerrit-Change-Number: 62771 Gerrit-PatchSet: 6 Gerrit-Owner: Robert Chen <robert.chen(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Cliff Huang <cliff.huang(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Robert Chen <robert.chen(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Fri, 18 Mar 2022 02:15:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Revert "Revert "drivers/intel/fsp2_0: Allow `mp_startup_all_cpus()` t...
by Nick Vaccaro (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Subrata Banik, Maulik V Vaghela, Paul Menzel, Sridhar Siricilla, Nick Vaccaro, Ronak Kanabar, Andrey Petrov. Hello build bot (Jenkins), Subrata Banik, Maulik V Vaghela, Tim Wawrzynczak, Sridhar Siricilla, Ronak Kanabar, Andrey Petrov, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/62907 to look at the new patch set (#6). Change subject: Revert "Revert "drivers/intel/fsp2_0: Allow `mp_startup_all_cpus()` to run serially"" ...................................................................... Revert "Revert "drivers/intel/fsp2_0: Allow `mp_startup_all_cpus()` to run serially"" This reverts a change that was causing hangs and exceptions during boot on an ADL brya4es. The hang (or APIC exception) occurs at what appears to be the FSP MP initialization sequence, prior to the "Display FSP Version Info HOB" log being displayed : [DEBUG] Detected 10 core, 12 thread CPU. [DEBUG] Display FSP Version Info HOB This reverts commit 40ca79714ad7d5f2aa201d83db4d97f21260d924. BUG=b:224873032 TEST=`emerge-brya coreboot chromeos-bootimage`, flash and verify brya4es is able to successfully reboot 200 times without any issues. Change-Id: I88c15a51c5d27fbd243478c923e75962d3f8d67d Signed-off-by: Nick Vaccaro <nvaccaro(a)google.com> --- M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c 1 file changed, 17 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/62907/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/62907 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I88c15a51c5d27fbd243478c923e75962d3f8d67d Gerrit-Change-Number: 62907 Gerrit-PatchSet: 6 Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(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)mailbox.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: util/cbmem: add an option to append timestamp
by Julius Werner (Code Review) 18 Mar '22

18 Mar '22

Attention is currently required from: Mattias Nissler. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62639 ) Change subject: util/cbmem: add an option to append timestamp ...................................................................... Patch Set 2: (5 comments) File util/cbmem/cbmem.c: https://review.coreboot.org/c/coreboot/+/62639/comment/4964fe47_eb4d6b50 PS2, Line 546: return __rdtsc(); It looks like this isn't correct for all x86 platforms... e.g. src/soc/amd/common/block/cpu/tsc/monotonic_timer.c overrides it differently. It's probably fine to keep it like this for the first version, but we should keep that in mind when expanding to more platforms (and maybe add a comment here to point it out). Getting this right across all platforms will be tricky. (We've been thinking for a while it would be better to normalize all timestamps for x86 to microseconds anyway, it's just that nobody had time to tackle that yet.) https://review.coreboot.org/c/coreboot/+/62639/comment/40f728fa_aaee2ef7 PS2, Line 713: if (timestamps.tag != LB_TAG_TIMESTAMPS) { nit: could consider moving this into main() to deduplicate https://review.coreboot.org/c/coreboot/+/62639/comment/18468824_3fe4802e PS2, Line 728: fprintf(stderr, "Not enough space to add timestamp.\n"); nit: should this return an error code (i.e. use die())? https://review.coreboot.org/c/coreboot/+/62639/comment/2db13995_90622d5b PS2, Line 1218: static void print_usage(const char *name, int exit_code) Please update to describe new mode. https://review.coreboot.org/c/coreboot/+/62639/comment/ad8a99f3_8c2194ed PS2, Line 1470: mem_fd = open("/dev/mem", O_RDWR, 0); You probably know this better than me: are there configurations where opening /dev/mem read-only would be allowed, but read-write not? If so we should depend this on timestamp_id to make sure other modes still work on restricted configurations. -- To view, visit https://review.coreboot.org/c/coreboot/+/62639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic99e5a11d8cc3f9fffae8eaf2787652105cf4842 Gerrit-Change-Number: 62639 Gerrit-PatchSet: 2 Gerrit-Owner: Mattias Nissler <mnissler(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Mattias Nissler <mnissler(a)chromium.org> Gerrit-Comment-Date: Fri, 18 Mar 2022 01:16:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2022