coreboot-gerrit December 2022

coreboot-gerrit@coreboot.org

1 participants
6828 discussions

[L] Change in coreboot[master]: util/amdfwtool: Deal with psp position in flash offset directly
by Arthur Heymans (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Jason Nien, Matt DeVillier, Martin Roth, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69852 ) Change subject: util/amdfwtool: Deal with psp position in flash offset directly ...................................................................... Patch Set 4: (1 comment) File src/soc/amd/common/Makefile.inc: https://review.coreboot.org/c/coreboot/+/69852/comment/740ce315_9dac6afc PS4, Line 25: $(call int-subtract, 0xffffffff $(CONFIG_ROM_SIZE)) 1 $(CONFIG_AMD_FWM_POSITION)) > I agree with what Karthik's saying here. The values that are listed all assume offsets from the bottom of a 16MiB ROM space. When changing that to starting at the bottom of an 8MiB ROM, the 0xC20000 needs to become 0x420000. I don't think we want that. > > So either you need to subtract the rom size in two different locations, > Once as you are to adjust the start of the rom area > > and again subtracting (16Mib-ROM size) to correctly adjust the location selected. > > Since we have a set 16MiB ROM area now, I think we can just keep the actual ROM size out of the equation (Though maybe we want to check that the selected space is inside the ROM). > > For upcoming chips, my understanding is that this is all going to go away, and the 16MiB ROM (or larger) becomes mandatory, and the only supported offset is 0x020000. I'm not sure I understand the concern. The memory mapped argument is to make sure that it actually lands the flash offset which the hardware expects. The PSP really only cares about flash offsets, not x86 mmap offsets. This is just a limitation of cbfstool (it only operates on offsets relative to the fmap of mmapped ones), but should not be blocking this. -- To view, visit https://review.coreboot.org/c/coreboot/+/69852 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I89c9e73e7db748379c97e3c0ad69af3faedc8d66 Gerrit-Change-Number: 69852 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: 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: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Marvin Drees <marvin.drees(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> 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: Sat, 03 Dec 2022 03:34:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: coreboot_tables: Make existing alignment conventions more explicit
by Arthur Heymans (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Peter Stuge, Julius Werner, Martin Roth, ron minnich. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70158 ) Change subject: coreboot_tables: Make existing alignment conventions more explicit ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/70158 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaeef29ef255047a855066469e03b5481812e5975 Gerrit-Change-Number: 70158 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Peter Stuge <peter(a)stuge.se> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-Attention: Peter Stuge <peter(a)stuge.se> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: ron minnich <rminnich(a)gmail.com> Gerrit-Comment-Date: Sat, 03 Dec 2022 02:31:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/alderlake: Disable L1 substates for PCIe compliance test mode
by Bora Guvendik (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Bora Guvendik, Tarun Tuli, Subrata Banik. Hello build bot (Jenkins), Tarun Tuli, Subrata Banik, Jérémy Compostella, Bora Guvendik, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/70165 to look at the new patch set (#2). Change subject: soc/intel/alderlake: Disable L1 substates for PCIe compliance test mode ...................................................................... soc/intel/alderlake: Disable L1 substates for PCIe compliance test mode Disable L1 substates for PCIe compliance test mode in order to get continuous clock output. BUG=b:235863379 TEST=Boot in compliance mode, check FSP settings Signed-off-by: Bora Guvendik <bora.guvendik(a)intel.corp-partner.google.com> Change-Id: I2a3b313425e00fe11f616d964f825baaef463c71 --- M src/soc/intel/alderlake/fsp_params.c 1 file changed, 19 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/70165/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/70165 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2a3b313425e00fe11f616d964f825baaef463c71 Gerrit-Change-Number: 70165 Gerrit-PatchSet: 2 Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.corp-partner.google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/google/herobrine: NVMe id determined by logical (not physical) bit
by Shelley Chen (Code Review) 02 Dec '22

02 Dec '22

Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/70230 ) Change subject: mb/google/herobrine: NVMe id determined by logical (not physical) bit ...................................................................... mb/google/herobrine: NVMe id determined by logical (not physical) bit NVMe is determined by a logical bit 1, not the physical SKU pin. Thus, (logical) sku_id & 0x2 == 0x2 would mean that the device has NVMe enabled on it. Previously, I thought that it was tied to a physical pin, but this is not correct. BUG=b:254281839 BRANCH=None TEST=flash and boot on villager and make sure that NVMe is not initialized in coreboot. Change-Id: Iaa75d2418d6a2351d874842e8678bd6ad3c92526 Signed-off-by: Shelley Chen <shchen(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/70230 Reviewed-by: Douglas Anderson <dianders(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/mainboard/google/herobrine/mainboard.c 1 file changed, 29 insertions(+), 14 deletions(-) Approvals: build bot (Jenkins): Verified Douglas Anderson: Looks good to me, approved diff --git a/src/mainboard/google/herobrine/mainboard.c b/src/mainboard/google/herobrine/mainboard.c index c9bca73..8a51b2b 100644 --- a/src/mainboard/google/herobrine/mainboard.c +++ b/src/mainboard/google/herobrine/mainboard.c @@ -85,18 +85,14 @@ } /* - * Determine if board need to perform PCIe initialization. On Herobrine, - * resistor strapping will be such that bit 0 will be assigned 2 (high Z) if it - * is an NVMe enabled platform. + * Determine if board need to perform PCIe initialization. Will return true if + * NVMe initialization is needed, or false if it is an eMMC device. On + * Herobrine, if it is an NVMe enabled platform, logical sku_id & 2 will be + * true. */ bool mainboard_needs_pcie_init(void) { - /* - * Mask out everything above the actual SKU bits We have 3 sku pins, - * each tristate, so we can represent numbers up to 27, or 5 bits - */ - uint32_t sku_bits_mask = 0xff; - uint32_t sku = sku_id() & sku_bits_mask; + uint32_t sku = sku_id(); if (sku == CROS_SKU_UNKNOWN) { printk(BIOS_WARNING, "Unknown SKU (%#x); assuming PCIe", sku); @@ -106,11 +102,7 @@ return true; } - if ((sku % 3) == 2) - return true; - - /* Otherwise, eMMC */ - return false; + return !!(sku & 0x2); } static void mainboard_init(struct device *dev) -- To view, visit https://review.coreboot.org/c/coreboot/+/70230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa75d2418d6a2351d874842e8678bd6ad3c92526 Gerrit-Change-Number: 70230 Gerrit-PatchSet: 4 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> 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-MessageType: merged

1 0

[S] Change in coreboot[master]: mb/google/herobrine: NVMe id determined by logical (not physical) bit
by Shelley Chen (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Julius Werner. Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70230 ) Change subject: mb/google/herobrine: NVMe id determined by logical (not physical) bit ...................................................................... Patch Set 3: (1 comment) File src/mainboard/google/herobrine/mainboard.c: https://review.coreboot.org/c/coreboot/+/70230/comment/fd210ad9_208738fd PS2, Line 100: & sku_bits_mask > Remove this. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/70230 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa75d2418d6a2351d874842e8678bd6ad3c92526 Gerrit-Change-Number: 70230 Gerrit-PatchSet: 3 Gerrit-Owner: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Sat, 03 Dec 2022 01:04:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in libhwbase[master]: hw-debug: Ensure global variable default are zeros and in .bss
by Jérémy Compostella (Code Review) 02 Dec '22

02 Dec '22

Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/libhwbase/+/69854 ) Change subject: hw-debug: Ensure global variable default are zeros and in .bss ...................................................................... Patch Set 1: (1 comment) File debug/hw-debug.adb: https://review.coreboot.org/c/libhwbase/+/69854/comment/4fafe753_f116b2e5 PS1, Line 26: Not_Start_Of_Line : Boolean; @Nico and @Angel, I am considering a few things: 1. renaming `Not_Start_Of_Line` with `Middle_Of_Line` 2. Add both `hw__debug__middle_of_line` and `hw__debug__register_write_delay_nanoseconds` in https://review.coreboot.org/c/coreboot/+/70276/2/src/arch/x86/car.ld#76 What do you think ? -- To view, visit https://review.coreboot.org/c/libhwbase/+/69854 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: libhwbase Gerrit-Branch: master Gerrit-Change-Id: Ib23699f3f3446219c34c60ccd9987346e17769f2 Gerrit-Change-Number: 69854 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Tarun Tuli <taruntuli(a)google.com> Gerrit-Comment-Date: Fri, 02 Dec 2022 22:52:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[master]: vendorcode/intel/fsp: Add Raptor Lake FSP headers for FSP RPL.3361.12
by Bora Guvendik (Code Review) 02 Dec '22

02 Dec '22

Hello Bora Guvendik, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/70273 to look at the new patch set (#2). Change subject: vendorcode/intel/fsp: Add Raptor Lake FSP headers for FSP RPL.3361.12 ...................................................................... vendorcode/intel/fsp: Add Raptor Lake FSP headers for FSP RPL.3361.12 The headers added are generated as per FSP v3361.12 BUG=b:261159242 BRANCH=firmware-brya-14505.B TEST=Boot to OS Signed-off-by: Bora Guvendik <bora.guvendik(a)intel.corp-partner.google.com> Change-Id: Id7986017e1256627027a45325238bf29e0c00cc4 --- M src/vendorcode/intel/fsp/fsp2_0/raptorlake/FspmUpd.h M src/vendorcode/intel/fsp/fsp2_0/raptorlake/FspsUpd.h 2 files changed, 878 insertions(+), 878 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/70273/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/70273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id7986017e1256627027a45325238bf29e0c00cc4 Gerrit-Change-Number: 70273 Gerrit-PatchSet: 2 Gerrit-Owner: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.corp-partner.google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in libgfxinit[master]: gma: Ensure global variable default are zeros and in .bss
by Jérémy Compostella (Code Review) 02 Dec '22

02 Dec '22

1 0

[S] Change in coreboot[master]: tests: Ignore RWX segment warning
by Jakub Czapiga (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Elyes Haouas. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70252 ) Change subject: tests: Ignore RWX segment warning ...................................................................... Patch Set 2: (1 comment) File tests/Makefile.common: https://review.coreboot.org/c/coreboot/+/70252/comment/ab867736_c256d1c6 PS2, Line 69: TEST_LDFLAGS += --no-warn-rwx-segments > Can you elaborate on why this is needed? I do not see where it would be useful right now, and we do […] And, you will have to use `-Wl,...` because `$(HOSTLD)` is set to GCC -- To view, visit https://review.coreboot.org/c/coreboot/+/70252 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I593eea3bcac8e9e46883b73b25a31e20b4c27c51 Gerrit-Change-Number: 70252 Gerrit-PatchSet: 2 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Comment-Date: Fri, 02 Dec 2022 22:48:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-MessageType: comment

1 0

[S] Change in libgfxinit[master]: gma: add a Turn_Off procedure to clear the graphic configuration
by Jérémy Compostella (Code Review) 02 Dec '22

02 Dec '22

Attention is currently required from: Nico Huber, Tarun Tuli, Angel Pons. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/libgfxinit/+/69938 ) Change subject: gma: add a Turn_Off procedure to clear the graphic configuration ...................................................................... Patch Set 1: (1 comment) File common/hw-gfx-gma.adb: https://review.coreboot.org/c/libgfxinit/+/69938/comment/0344c969_bcd8f467 PS1, Line 630: Connectors.Pre_All_Off; : Display_Controller.All_Off; : Connectors.Post_All_Off; : PLLs.All_Off; : Power_And_Clocks.Post_All_Off; > This part should be covered by calling Update_Outputs with all ports set to […] As said over emails using `Update_Outputs` did not work. The minimal working Turn_Off implementation is: ``` procedure Turn_Off is begin Debug.Put_Line("J2M: Turning off"); Connectors.Pre_All_Off; Display_Controller.All_Off; Connectors.Post_All_Off; end Turn_Off; ``` which lead to the call of the following functions: ``` Panel.Backlight_Off Panel.Off Panel.Backlight_Off Panel.Off Pipe_Setup.All_Off Connectors.Off Connectors.Off Connectors.Off ``` Compared to the Update_Outputs call (with `Primary port` at `Disabled`): ``` Panel.Backlight_Off Panel.Off Pipe_Setup.Off Connectors.Off PLLs.Free Power_And_Clocks.Power_Down Power_And_Clocks.XELPD.PD_Off Power_And_Clocks.XELPD.PD_Off Power_And_Clocks.XELPD.PD_Off ``` I have not looked into the differences more than this. However, I realized that considering we are addressing a corner case a simple and working approach consist in calling `Initialize` with the `Clean_State` argument set to `True`. This is what I did in https://review.coreboot.org/c/coreboot/+/70299. -- To view, visit https://review.coreboot.org/c/libgfxinit/+/69938 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: libgfxinit Gerrit-Branch: master Gerrit-Change-Id: Ida65bf3303d4dbf956d6fc9340f935e6c4e6b447 Gerrit-Change-Number: 69938 Gerrit-PatchSet: 1 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 02 Dec 2022 22:46:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2022