coreboot-gerrit February 2021

coreboot-gerrit@coreboot.org

1 participants
7192 discussions

Change in coreboot[master]: hatch: Modify genesis PcieClkSrc settings
by Joe Tessler (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Neill Corlett. Joe Tessler has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50101 ) Change subject: hatch: Modify genesis PcieClkSrc settings ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/50101/comment/d7c98516_acfb8fc0 PS2, Line 9: to work around issue with Longsys SSD > I think it would be good to provide some context as to why the change is being made. […] This was required to support a specific SSD module on the M.2 slot that uses PCIe slots 11 & 12 as advertised for "NVMe hybrid storage devices". Apparently my way of registering the clock sources only worked on the SSD module I had on hand. That being said, I'm completely stumped as to why this patch fixes the issue. I would've expected just `register "PcieRpEnable[11]" = "1"` change, but I don't have access to the hardware to verify it myself. Do you think this is sufficient / should be added to the commit message? -- To view, visit https://review.coreboot.org/c/coreboot/+/50101 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I30c2179be9b9ddd50168a7f35be800e969800e10 Gerrit-Change-Number: 50101 Gerrit-PatchSet: 2 Gerrit-Owner: Neill Corlett <corlett(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Joe Tessler <jrt(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matthew Ziegelbaum <ziegs(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-Comment-Date: Mon, 01 Feb 2021 21:50:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush: First pass GPIO configuriation for Guybrush
by Mathew King (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Angel Pons. Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50091 ) Change subject: mb/google/guybrush: First pass GPIO configuriation for Guybrush ...................................................................... Patch Set 5: (2 comments) File src/mainboard/google/guybrush/variants/baseboard/gpio.c: https://review.coreboot.org/c/coreboot/+/50091/comment/a3fc5e05_9432e411 PS5, Line 10: /* PWR_BTN_L */ > Do these comments provide any useful information? If not, I would remove them. These correspond to net names on the schematic and are helpful on other boards. https://review.coreboot.org/c/coreboot/+/50091/comment/5ba6502b_9a139bcc PS5, Line 80: /* AGPIO69 */ : PAD_NC(GPIO_69), : /* EGPIO70 */ : PAD_NC(GPIO_70), > What's the difference between AGPIO and EGPIO? This is how AMD refers to these gpios. The GPIO pins identified as AGPIO are capable of generating the interrupt as GPIO-signaled ACPI Events while EGPIO pins function as purely GPIO pins with no interrupt capability. -- To view, visit https://review.coreboot.org/c/coreboot/+/50091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia8211dbc3de09a61f264a0e5d44d1eac703b83c9 Gerrit-Change-Number: 50091 Gerrit-PatchSet: 5 Gerrit-Owner: Mathew King <mathewk(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 01 Feb 2021 21:43:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add `struct gpio_pad_table` and helpers
by Angel Pons (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Subrata Banik, Michael Niewöhner, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50176 ) Change subject: soc/intel/common: Add `struct gpio_pad_table` and helpers ...................................................................... Patch Set 1: (2 comments) File src/soc/intel/common/block/include/intelblocks/gpio.h: https://review.coreboot.org/c/coreboot/+/50176/comment/3c749d34_80236c67 PS1, Line 75: const struct pad_config *padcfg; : size_t length; : > modifying padcfg and not adapting the lenght could (will?) lead to problems; what about making both […] Hmmm, good point. If anything, the current `const` usage is a bit stupid: you can't modify the pad_config values (it's a pointer to `const`struct pad_config`), but you're free to replace the pointer and mess with the `length` value... :D https://review.coreboot.org/c/coreboot/+/50176/comment/dfa7165d_c50276fb PS1, Line 78: (struct gpio_pad_table) > this cast isn't needed The cast is there on purpose to allow this usage: const struct pad_config pad_array[] = { ... }; gpio_configure_pad_table(GPIO_PAD_ARRAY(pad_array)); -- To view, visit https://review.coreboot.org/c/coreboot/+/50176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If029bbafe186d36a971d91b3b6b9220616e04141 Gerrit-Change-Number: 50176 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 01 Feb 2021 21:34:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: arch/{arm,riscv}/boot.c: Add missing <commonlib/bsd/cbfs_serialized.h>
by Julius Werner (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Philipp Hug, ron minnich, HAOUAS Elyes. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50167 ) Change subject: arch/{arm,riscv}/boot.c: Add missing <commonlib/bsd/cbfs_serialized.h> ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/50167/comment/7aef98a0_cada15ac PS3, Line 7: arch/{arm,riscv}/boot.c: Add missing <commonlib/bsd/cbfs_serialized.h> It isn't missing. People use coreboot headers like <cbfs.h> to chain-include commonlib headers like <commonlib/bsd/cbfs.h> intentionally because it hides the implementation detail of which parts are factored out into shared code trees, making it easier to refactor those later. I feel like we've had this discussion a dozen times now? Whenever I mention this you abandon the patch implying you agree but then you just reupload the same thing again elsewhere a month later? If you disagree then please, let's have that discussion, but please don't just make me chase your submissions and write the same thing over and over again. It's tiresome and a waste of both our time. :/ -- To view, visit https://review.coreboot.org/c/coreboot/+/50167 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I98fb5df0148a3f5fe0bc5b08c5a0b8b1e95c4624 Gerrit-Change-Number: 50167 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Attention: Philipp Hug <philipp(a)hug.cx> Gerrit-Attention: ron minnich <rminnich(a)gmail.com> Gerrit-Attention: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-Comment-Date: Mon, 01 Feb 2021 21:23:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add `struct gpio_pad_table` and helpers
by Michael Niewöhner (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Angel Pons, Subrata Banik, Patrick Rudolph. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50176 ) Change subject: soc/intel/common: Add `struct gpio_pad_table` and helpers ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I just had another idea: what about this? #define GPIO_PAD_ARRAY(name, table) const struct gpio_pad_table name = { \ .padcfg = table, \ .length = ARRAY_SIZE(table), \ }; `const struct gpio_pad_table variant_gpio_table = GPIO_PAD_ARRAY(gpio_table);` would become: `GPIO_PAD_ARRAY(variant_gpio_table, gpio_table)` just like the bootstate macros. -- To view, visit https://review.coreboot.org/c/coreboot/+/50176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If029bbafe186d36a971d91b3b6b9220616e04141 Gerrit-Change-Number: 50176 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 01 Feb 2021 21:03:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/sarien: Use `struct gpio_pad_table`
by Michael Niewöhner (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Angel Pons, Subrata Banik. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50177 ) Change subject: mb/google/sarien: Use `struct gpio_pad_table` ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/50177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I407905c64aa70bd2c0c92f6abbbf3cdedb6271f5 Gerrit-Change-Number: 50177 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Comment-Date: Mon, 01 Feb 2021 20:56:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: Add `struct gpio_pad_table` and helpers
by Michael Niewöhner (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Furquan Shaikh, Angel Pons, Subrata Banik, Patrick Rudolph. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50176 ) Change subject: soc/intel/common: Add `struct gpio_pad_table` and helpers ...................................................................... Patch Set 1: Code-Review+1 (4 comments) Patchset: PS1: Nice! :-) File src/soc/intel/common/block/include/intelblocks/gpio.h: https://review.coreboot.org/c/coreboot/+/50176/comment/904b7b1f_3a77d567 PS1, Line 75: const struct pad_config *padcfg; : size_t length; : modifying padcfg and not adapting the lenght could (will?) lead to problems; what about making both const? otoh, it's not done for other structs as well, so... feel free to mark as resolved if you don't like that idea https://review.coreboot.org/c/coreboot/+/50176/comment/44ace442_dc42b7c2 PS1, Line 78: (struct gpio_pad_table) this cast isn't needed https://review.coreboot.org/c/coreboot/+/50176/comment/7b4b13f7_dfd5692d PS1, Line 81: ) when the cast gets dropped. the braces () need to be dropped, too, to make it compile -- To view, visit https://review.coreboot.org/c/coreboot/+/50176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If029bbafe186d36a971d91b3b6b9220616e04141 Gerrit-Change-Number: 50176 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 01 Feb 2021 20:55:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: hatch: Update fan and thermal settings for ambassador
by Joe Tessler (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Neill Corlett. Joe Tessler has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50100 ) Change subject: hatch: Update fan and thermal settings for ambassador ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Patchset: PS3: > Change looks okay to me. […] LGTM -- I walked through these numbers with Quanta. -- To view, visit https://review.coreboot.org/c/coreboot/+/50100 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I080859f872caf696f0c085defb8372de658da58a Gerrit-Change-Number: 50100 Gerrit-PatchSet: 3 Gerrit-Owner: Neill Corlett <corlett(a)google.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Joe Tessler <jrt(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matthew Ziegelbaum <ziegs(a)google.com> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-Comment-Date: Mon, 01 Feb 2021 20:42:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: Move L1_substates_control to pcie_rp.h
by Benjamin Doron (Code Review) 01 Feb '21

01 Feb '21

Attention is currently required from: Nico Huber. Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50050 ) Change subject: soc/intel/skylake: Move L1_substates_control to pcie_rp.h ...................................................................... Patch Set 2: (1 comment) This change is ready for review. Patchset: PS2: Should the config name be reverted to match the UPD (PcieRpL1Substates) for consistency with other platforms (applies to pcie_rp_aspm as well) or should other platforms be adjusted? I recall the discussion on CB:39538 (and then CB:45001), but I think that consistency would be good. This difference might be why Skylake was omitted from the change before. -- To view, visit https://review.coreboot.org/c/coreboot/+/50050 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaafc507624151ec871ffcc0fba6da02b012cd038 Gerrit-Change-Number: 50050 Gerrit-PatchSet: 2 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> 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-Comment-Date: Mon, 01 Feb 2021 20:39:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/siemens/mc_apl1/variants/mc_apl2/mainboard.c: Clean includes
by HAOUAS Elyes (Code Review) 01 Feb '21

01 Feb '21

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/50204 ) Change subject: mb/siemens/mc_apl1/variants/mc_apl2/mainboard.c: Clean includes ...................................................................... mb/siemens/mc_apl1/variants/mc_apl2/mainboard.c: Clean includes Change-Id: I14ec7d6faa20542707a1b6041e1ce358ce4a537a Signed-off-by: Elyes HAOUAS <ehaouas(a)noos.fr> --- M src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c 1 file changed, 1 insertion(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/50204/1 diff --git a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c index 9cfd86f..1baa8ec 100644 --- a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c +++ b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c @@ -1,15 +1,10 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#include <bootstate.h> -#include <console/console.h> #include <device/pci_def.h> #include <device/pci_ids.h> #include <device/pci_ops.h> -#include <hwilib.h> -#include <intelblocks/lpc_lib.h> +#include <device/device.h> #include <baseboard/variants.h> -#include <soc/pci_devs.h> -#include <types.h> void variant_mainboard_final(void) { -- To view, visit https://review.coreboot.org/c/coreboot/+/50204 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I14ec7d6faa20542707a1b6041e1ce358ce4a537a Gerrit-Change-Number: 50204 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr> Gerrit-MessageType: newchange

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2021