coreboot-gerrit May 2023

coreboot-gerrit@coreboot.org

1 participants
5262 discussions

[S] Change in coreboot[master]: soc/amd/mendocino: Add Kconfig option to generate manifest
by Martin L Roth (Code Review) 02 May '23

02 May '23

Attention is currently required from: Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Paul Menzel, Grzegorz Bernacki, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74267 ) Change subject: soc/amd/mendocino: Add Kconfig option to generate manifest ...................................................................... Patch Set 5: (2 comments) Patchset: PS5: Why have a Kconfig option? Is there a reason not to just add this all the time? If it does make sense to add a Kconfig file, why not default it to on? Does it make sense to add this to other AMD platforms? If it does, maybe add (most of) this to the common Kconfig and Makefile? Obviously the option needs to be added to each platform's makefile individually. File src/soc/amd/mendocino/Kconfig: https://review.coreboot.org/c/coreboot/+/74267/comment/c821474a_35fa09be PS5, Line 487: INCLUDE_MANIFEST_FILE Can this be more specific? STORE_AMDFW_MANIFEST maybe? -- To view, visit https://review.coreboot.org/c/coreboot/+/74267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I13c9d322735e0979484b120c665fb100cf187eab Gerrit-Change-Number: 74267 Gerrit-PatchSet: 5 Gerrit-Owner: Grzegorz Bernacki Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> 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: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Grzegorz Bernacki Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 02 May 2023 16:06:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: amdfwtool: Add --output-manifest option
by Martin L Roth (Code Review) 02 May '23

02 May '23

Attention is currently required from: Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Paul Menzel, Grzegorz Bernacki, Arthur Heymans, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74266 ) Change subject: amdfwtool: Add --output-manifest option ...................................................................... Patch Set 5: (1 comment) File util/amdfwtool/amdfwtool.c: https://review.coreboot.org/c/coreboot/+/74266/comment/f3c5f831_86d36e1c PS5, Line 248: output_manifest Why is this needed? Why not just set a flag to true? It just looks like this adds complexity that isn't needed right now. -- To view, visit https://review.coreboot.org/c/coreboot/+/74266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idaa3a02ace524f44cfa656e34308bd896016dff6 Gerrit-Change-Number: 74266 Gerrit-PatchSet: 5 Gerrit-Owner: Grzegorz Bernacki Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> 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: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Grzegorz Bernacki Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Tue, 02 May 2023 16:00:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mainboard/intel/mtlrvp: Refactor the kconfig selections
by Ronak Kanabar (Code Review) 02 May '23

02 May '23

Attention is currently required from: Harsha B R, Sridhar Siricilla. Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74838 ) Change subject: mainboard/intel/mtlrvp: Refactor the kconfig selections ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/74838 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib8557aab2848a384fba5203e5f3d62407b2566ef Gerrit-Change-Number: 74838 Gerrit-PatchSet: 1 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Reviewer: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Jan Samek <jan.samek(a)siemens.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Comment-Date: Tue, 02 May 2023 15:40:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/intel/speedstep: Use acpigen_write_processor_device()
by Nico Huber (Code Review) 02 May '23

02 May '23

Attention is currently required from: Arthur Heymans, Kyösti Mälkki. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74441 ) Change subject: cpu/intel/speedstep: Use acpigen_write_processor_device() ...................................................................... Patch Set 4: (2 comments) File src/southbridge/intel/i82801gx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/3fd026e7_3ec1cda8 PS4, Line 29: fadt->p_lvl3_lat = chip->c3_latency; > Well, the question is if _CST is a property of the installed CPU part and the location of get_cst_en […] I wouldn't say the current implementation is right. But if we want to drop Processor(), shouldn't we first have an alternative ready? In any case, we shouldn't leave dead code around. AFAICT, this was the only consumer of `c3_latency`. File src/southbridge/intel/i82801jx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/76aef374_b022ed28 PS4, Line 25: fadt->p_lvl3_lat = 0; /* FIXME: Is this correct? */ > Deprecation of Processor() object hides C2/C3 IO "trap" addresses (PROC_CNT or PROC_BLK +4 (C2) and […] Ok, I get it now. ACPI spec isn't very specific about this in the FADT description. I missed that we don't advertise P_BLK anymore. Dropping these settings is fine then. -- To view, visit https://review.coreboot.org/c/coreboot/+/74441 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If1950ceac7daf8d8e91c74f1090c7451cb92e100 Gerrit-Change-Number: 74441 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Tue, 02 May 2023 15:14:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/intel/speedstep: Use acpigen_write_processor_device()
by Kyösti Mälkki (Code Review) 02 May '23

02 May '23

Attention is currently required from: Nico Huber, Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74441 ) Change subject: cpu/intel/speedstep: Use acpigen_write_processor_device() ...................................................................... Patch Set 4: (1 comment) File src/southbridge/intel/i82801jx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/906d361c_9701b04e PS4, Line 25: fadt->p_lvl3_lat = 0; /* FIXME: Is this correct? */ > Removing the FIXME probably won't get heads up. At least that's how I understand […] Deprecation of Processor() object hides C2/C3 IO "trap" addresses (PROC_CNT or PROC_BLK +4 (C2) and +5 (C3)) from OSPM. If there is no _CST present, I see no mechanism to enter C2/C3 states? -- To view, visit https://review.coreboot.org/c/coreboot/+/74441 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If1950ceac7daf8d8e91c74f1090c7451cb92e100 Gerrit-Change-Number: 74441 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 02 May 2023 14:39:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/link: Apply symmetry for EC events defines
by Kyösti Mälkki (Code Review) 02 May '23

02 May '23

Attention is currently required from: Matt DeVillier. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74602 ) Change subject: mb/google/link: Apply symmetry for EC events defines ...................................................................... Patch Set 4: (1 comment) File src/mainboard/google/link/mainboard.c: https://review.coreboot.org/c/coreboot/+/74602/comment/194971a3_84796b28 PS4, Line 111: mainboard_ec_init(); > looks like this is missing a declaration Yeah, <ec/ec.h>, existing "ec.h" did not make it. I'll get back to this mb-smihandler after tagged release. -- To view, visit https://review.coreboot.org/c/coreboot/+/74602 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I97d9d28963c97e780156d75b39deac069028866a Gerrit-Change-Number: 74602 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Comment-Date: Tue, 02 May 2023 14:29:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/link: Apply symmetry for EC events defines
by Daniel Rosa Franzini (Code Review) 02 May '23

02 May '23

Attention is currently required from: Kyösti Mälkki. Daniel Rosa Franzini has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74602 ) Change subject: mb/google/link: Apply symmetry for EC events defines ...................................................................... Patch Set 4: (1 comment) File src/mainboard/google/link/ec.h: https://review.coreboot.org/c/coreboot/+/74602/comment/56b2cac9_8707a802 PS4, Line 38: If I got it correctly, the defines from lines 39-41 should be kept and the API name should be changed. This is why a definition is missing in mainboard.c (line 111). -- To view, visit https://review.coreboot.org/c/coreboot/+/74602 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I97d9d28963c97e780156d75b39deac069028866a Gerrit-Change-Number: 74602 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Tue, 02 May 2023 14:23:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/intel/speedstep: Use acpigen_write_processor_device()
by Nico Huber (Code Review) 02 May '23

02 May '23

Attention is currently required from: Arthur Heymans, Kyösti Mälkki. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74441 ) Change subject: cpu/intel/speedstep: Use acpigen_write_processor_device() ...................................................................... Patch Set 4: (1 comment) File src/southbridge/intel/i82801jx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/c6919fff_84199849 PS4, Line 25: fadt->p_lvl3_lat = 0; /* FIXME: Is this correct? */ > Commit message hopefully raises enough eyebrows for someone to act on a fix without degraded perform […] Removing the FIXME probably won't get heads up. At least that's how I understand it, the patch doesn't effectively change the value, right? And that's where I see it potentially disagreeing with the commit message. But let me ask differently, what's the incentive to drop the FADT fallback values? -- To view, visit https://review.coreboot.org/c/coreboot/+/74441 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If1950ceac7daf8d8e91c74f1090c7451cb92e100 Gerrit-Change-Number: 74441 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Tue, 02 May 2023 14:15:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/intel/speedstep: Use acpigen_write_processor_device()
by Kyösti Mälkki (Code Review) 02 May '23

02 May '23

Attention is currently required from: Nico Huber, Arthur Heymans. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74441 ) Change subject: cpu/intel/speedstep: Use acpigen_write_processor_device() ...................................................................... Patch Set 4: (2 comments) File src/southbridge/intel/i82801gx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/67e09efe_04835e1b PS4, Line 29: fadt->p_lvl3_lat = chip->c3_latency; > This leaves the `c3_latency` field blind. Would it be much hassle to port […] Well, the question is if _CST is a property of the installed CPU part and the location of get_cst_entries() implementations under mb/ is wrong to begin with. File src/southbridge/intel/i82801jx/fadt.c: https://review.coreboot.org/c/coreboot/+/74441/comment/5025336c_ba6e8df2 PS4, Line 25: fadt->p_lvl3_lat = 0; /* FIXME: Is this correct? */ > ACPI spec: "A value > 1000 indicates the system does not support a C3 state.". […] Commit message hopefully raises enough eyebrows for someone to act on a fix without degraded performance / battery life. -- To view, visit https://review.coreboot.org/c/coreboot/+/74441 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If1950ceac7daf8d8e91c74f1090c7451cb92e100 Gerrit-Change-Number: 74441 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Tue, 02 May 2023 14:02:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: security/tpm: Add Kconfig to allow payload control of TPM1
by Matt DeVillier (Code Review) 02 May '23

02 May '23

Attention is currently required from: Christian Walter, Julius Werner. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74856 ) Change subject: security/tpm: Add Kconfig to allow payload control of TPM1 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Well, did you rewrite your payload to actually send the TPM_Startup command, then? I'm not saying th […] currently, edk2 expects coreboot to have initialized the TPM and set up the necessary ACPI tables - it's just providing the front end to manage the TPM state / allow the user to confirm TPM changes initiated by the OS. The only problem which I'm trying to work around is coreboot force-enabling the TPM, since it prevents the user / edk2 from disabling/deactivating it. -- To view, visit https://review.coreboot.org/c/coreboot/+/74856 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ieb7db109cbcc1a0166d95b6130b624b635bb7ac9 Gerrit-Change-Number: 74856 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Tue, 02 May 2023 13:59:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2023