coreboot-gerrit February 2021

coreboot-gerrit@coreboot.org

1 participants
7192 discussions

Change in coreboot[master]: drivers/intel/fsp2_0/memory_init: check if UPD struct has expected size
by Felix Held (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Furquan Shaikh, Meera Ravindranath, Andrey Petrov, Patrick Rudolph, Felix Held. Hello Justin Frodsham, build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Meera Ravindranath, Subrata Banik, Andrey Petrov, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/50241 to look at the new patch set (#2). Change subject: drivers/intel/fsp2_0/memory_init: check if UPD struct has expected size ...................................................................... drivers/intel/fsp2_0/memory_init: check if UPD struct has expected size If the UPD size in coreboot sizes mismatches the one from the FSP-M binary, call die(). We ran into the issue in soc/amd/cezanne, where the UPD struct in coreboot was smaller than the one in the FSP, so the defaults didn't get completely copied. TEST=Mandolin still boots. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: Ia7e9f6f20d0091bbb4abfd42abb40b485da2079d --- M src/drivers/intel/fsp2_0/memory_init.c 1 file changed, 17 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/50241/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/50241 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia7e9f6f20d0091bbb4abfd42abb40b485da2079d Gerrit-Change-Number: 50241 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin Frodsham <justin.frodsham(a)amd.corp-partner.google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: {src}: Assign values before if statement
by Julius Werner (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Frans Hendriks, Kyösti Mälkki, Alexander Couzens, Aaron Durbin, Patrick Rudolph. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50040 ) Change subject: {src}: Assign values before if statement ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > I was not aware of history of checkpatch. […] I mean, are you implying that the current style was inconsistent? I would say this is currently allowed and code authors can write code that way at their discretion, what's not consistent about that? There's no single situation where you'd _have_ to write something like this, just like you don't _have_ to use long strings or lines that require breaking or any of the dozen other things we have code style rules for. Like I mentioned there is also a while() version of this which is used more often, and I think disallowing one while allowing the other would be the bigger inconsistency. -- To view, visit https://review.coreboot.org/c/coreboot/+/50040 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idd346d31f1da71271ad1545e5645e57530c7b374 Gerrit-Change-Number: 50040 Gerrit-PatchSet: 3 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Thu, 04 Feb 2021 23:21:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Frans Hendriks <fhendriks(a)eltan.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mainboards: Remove default CHROMEOS=y
by Tim Wawrzynczak (Code Review) 04 Feb '21

04 Feb '21

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50118 ) Change subject: mainboards: Remove default CHROMEOS=y ...................................................................... Patch Set 4: (1 comment) Patchset: PS3: > We should prepare the chorme ToT CL for this when this CL UPSTREAM to our tree. Everything should be OK, I went through and checked all of the boards to ensure we already select CONFIG_CHROMEOS in the config.${BOARD} files. -- To view, visit https://review.coreboot.org/c/coreboot/+/50118 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I16fcf62a23dae1b21c77cee275c867f9c1de893b Gerrit-Change-Number: 50118 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 04 Feb 2021 22:56:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tgl: Enable/Disable S0ix substates
by Furquan Shaikh (Code Review) 04 Feb '21

04 Feb '21

1 0

Change in coreboot[master]: soc/amd/cezanne/fch: add ACPI I/O port setup
by Raul Rangel (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50270 ) Change subject: soc/amd/cezanne/fch: add ACPI I/O port setup ...................................................................... Patch Set 2: Code-Review+2 (1 comment) File src/soc/amd/cezanne/include/soc/iomap.h: https://review.coreboot.org/c/coreboot/+/50270/comment/c0aceafd_d3889601 PS2, Line 25: 0x10 > also that is hex, so there are more than 2 bytes in between 8 and 10 ;) all your base are belong to us! -- To view, visit https://review.coreboot.org/c/coreboot/+/50270 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If80b841df12d351d5a0c1e0d2e7bf1e31b03447f Gerrit-Change-Number: 50270 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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-Comment-Date: Thu, 04 Feb 2021 21:57:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/cezanne/fch: add ACPI I/O port setup
by Felix Held (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50270 ) Change subject: soc/amd/cezanne/fch: add ACPI I/O port setup ...................................................................... Patch Set 2: (1 comment) File src/soc/amd/cezanne/include/soc/iomap.h: https://review.coreboot.org/c/coreboot/+/50270/comment/a0897c0f_7773d24b PS2, Line 25: 0x10 > TMR_BLK is 4 bytes. […] also that is hex, so there are more than 2 bytes in between 8 and 10 ;) -- To view, visit https://review.coreboot.org/c/coreboot/+/50270 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If80b841df12d351d5a0c1e0d2e7bf1e31b03447f Gerrit-Change-Number: 50270 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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-Comment-Date: Thu, 04 Feb 2021 21:56:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/cezanne/fch: add ACPI I/O port setup
by Felix Held (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50270 ) Change subject: soc/amd/cezanne/fch: add ACPI I/O port setup ...................................................................... Patch Set 2: (1 comment) File src/soc/amd/cezanne/include/soc/iomap.h: https://review.coreboot.org/c/coreboot/+/50270/comment/4bf47122_d4575a45 PS2, Line 25: 0x10 > So in CB:41128 this was changed to 0x0c. […] TMR_BLK is 4 bytes. the offsets in this patch are the ones from the reference code; the ppr doesn't match the code and i've already filed a bug report on that internally. the patch you linked matched that to the docs that don't match the refcode. see my comment above about the same line in an older revision -- To view, visit https://review.coreboot.org/c/coreboot/+/50270 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If80b841df12d351d5a0c1e0d2e7bf1e31b03447f Gerrit-Change-Number: 50270 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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-Comment-Date: Thu, 04 Feb 2021 21:55:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [UNTESTED] soc/amd/picasso/iomap: change ACPI_CPU_CONTROL to match AGESA
by Martin Roth (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Felix Held. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50288 ) Change subject: [UNTESTED] soc/amd/picasso/iomap: change ACPI_CPU_CONTROL to match AGESA ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Tested on Morphius. Goes into suspend & resumes. Powerstates C1 & C2 work according to Powertop. Chris said something about cc6 in the previous patch that set it to 0x13, but I don't have the tool he used to do that. -- To view, visit https://review.coreboot.org/c/coreboot/+/50288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I80e11d9792ee4138cb376ebbe0438dc304b54527 Gerrit-Change-Number: 50288 Gerrit-PatchSet: 2 Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Martin Roth <martinroth(a)google.com> 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: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 04 Feb 2021 21:51:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tgl: Enable/Disable S0ix substates
by Shreesh Chhabbi (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Furquan Shaikh. Shreesh Chhabbi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49766 ) Change subject: soc/intel/tgl: Enable/Disable S0ix substates ...................................................................... Patch Set 11: (1 comment) File src/soc/intel/tigerlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/49766/comment/937f4409_8cd07aeb PS9, Line 60: and UP4 > I don't think this is correct for UP4. Ok. I thought that its applicable for both up3 and up4. I can make this only for up3. Is there a way to differentiate up3 from up4 in soc code? Do you have any suggestions to implement this? -- To view, visit https://review.coreboot.org/c/coreboot/+/49766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f2ac8b72d0c9b05bc02c092188d0c742cc83af9 Gerrit-Change-Number: 49766 Gerrit-PatchSet: 11 Gerrit-Owner: Shreesh Chhabbi <shreesh.chhabbi(a)intel.com> Gerrit-Reviewer: Alex Levin <levinale(a)google.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> Gerrit-Reviewer: Sukumar Ghorai <sukumar.ghorai(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Comment-Date: Thu, 04 Feb 2021 21:51:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tgl: Enable/Disable S0ix substates
by Furquan Shaikh (Code Review) 04 Feb '21

04 Feb '21

Attention is currently required from: Shreesh Chhabbi. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49766 ) Change subject: soc/intel/tgl: Enable/Disable S0ix substates ...................................................................... Patch Set 11: (8 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/49766/comment/09e55669_edf706d6 PS9, Line 7: Disable S0i3.2 & S0i3.3 substates : : S0i3.2 and S0i3.3 are applicable only if wake on voice is : disabled. As per Platform Design Guide, S0i3.2 and S0i3.3 : substates need to be disabled for Tigerlake. This needs update https://review.coreboot.org/c/coreboot/+/49766/comment/4a4f0e1e_be107afc PS9, Line 12: Can you please check what is the lowest power state reported by coreboot and actually observed on volteer devices? I see you have commented that you need to update volteer and tglrvp devicetrees. It would be good to validate the entire series. File src/soc/intel/tigerlake/chip.h: https://review.coreboot.org/c/coreboot/+/49766/comment/78878b4e_dad500f2 PS9, Line 502: enabled Rather than saying "enabled", I think it should say "Mainboard design uses external clock gating" since this is very much dependent on the hardware design. Same for external phy gating and external bypass rails. https://review.coreboot.org/c/coreboot/+/49766/comment/03df9929_a518f390 PS9, Line 506: ExternalClkGated nit: use external_clk_gated instead of ExternalClkGated. I know this file uses a mix of both styles, but the former is preferred. Also, the latter is mostly used for FSP UPDs. Same for configs below. File src/soc/intel/tigerlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/49766/comment/2dae88e3_2af37d56 PS9, Line 60: and UP4 I don't think this is correct for UP4. https://review.coreboot.org/c/coreboot/+/49766/comment/41a822b2_f9873292 PS9, Line 67: recommended It would be good to capture in commit message what "recommended" means i.e. it is possible to achieve this state, but is known to provide lower savings than shallower states. https://review.coreboot.org/c/coreboot/+/49766/comment/f65d0c3f_f953247e PS9, Line 71: /* If external phy gating is not implemented, S0i3.3/S0i3.4/S0i2.2 are not recommended. */ > line over 96 characters Can you please split this into multi-line comment? https://review.coreboot.org/c/coreboot/+/49766/comment/17db7814_fbff986c PS9, Line 76: if (is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_CNVI_BT)) || is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_CNVI_WIFI)) || is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_ISH))) > line over 96 characters split into multiple lines: if (is_dev_enabled() || is_dev_enabled() || is_dev_enabled()) or you can add a helper: if (is_cnvi_or_ish_enabled()) and implement is_cnvi_or_ish_enabled() such that: bool is_cnvi_or_ish_enabled(void) { return is_dev_enabled() || is_dev_enabled() || is_dev_enabled(); } -- To view, visit https://review.coreboot.org/c/coreboot/+/49766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f2ac8b72d0c9b05bc02c092188d0c742cc83af9 Gerrit-Change-Number: 49766 Gerrit-PatchSet: 11 Gerrit-Owner: Shreesh Chhabbi <shreesh.chhabbi(a)intel.com> Gerrit-Reviewer: Alex Levin <levinale(a)google.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shreesh Chhabbi <shreesh.chhabbi(a)intel.corp-partner.google.com> Gerrit-Reviewer: Sukumar Ghorai <sukumar.ghorai(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shreesh Chhabbi <shreesh.chhabbi(a)intel.com> Gerrit-Comment-Date: Thu, 04 Feb 2021 21:36:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2021