Attention is currently required from: Sean Rhodes, Patrick Rudolph.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58149 )
Change subject: soc/intel/*/me.c: Check more than PCI interface for printing ME info
......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/alderlake/me.c:
https://review.coreboot.org/c/coreboot/+/58149/comment/2d4cf973_eadd77c6
PS1, Line 103: !cse_is_hfs1_cws_normal() ||
: !cse_is_hfs1_com_normal() ||
: !cse_is_hfs1_com_soft_temp_disable())
: return;
> > Frankly speaking, I *always* want these printed in my logs. […]
Oh sorry I misunderstood.
So IIUC you want to bypass the `!cse_dev->enabled` check from `is_cse_enabled()` here?
I think it might be OK to change the condition from
```
if (!is_cse_enabled())
return;
```
to something more like:
```
if (pci_read_config16(PCH_DEV_CSE, PCI_VENDOR_ID) == 0xFFFF) {
printk(BIOS_WARN, "CSE: device is hidden\n");
return false;
}
```
which is obviously the condition where the PCI config space reads won't return anything useful anyway.
and then this is checking *only* the PCI interface, instead of the devicetree device status.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58149
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ad1215cceb75651b7890f3bab5df39b1b72ecf6
Gerrit-Change-Number: 58149
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
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: Sean Rhodes <admin(a)starlabs.systems>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:52:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Rhodes <admin(a)starlabs.systems>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Angel Pons, Julius Werner.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56410 )
Change subject: [RFC] kconfig_lint: Drop overly restrictive rules about choice configs
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56410/comment/a12a6d8c_be228e5e
PS4, Line 19: On top, the linter treats every occurence of a `config` entry as a
: symbol declaration, even when it's just setting a default or adding
: selects.
Every config entry is treated as a symbol definition because any given config entry could be the symbol declaration. What makes one config entry different than another? That a type is specified vs not being specified? That it comes first, last?
Please keep in mind that when this was written almost every entry had a type associated because people didn't seem to realise that they didn't have to.
If anyone would like to take a stab at writing a better linter, I'd welcome the effort. Before I wrote this, there was absolutely zero checking. Even the kconfig tool itself, which now errors out if there are issues did not do that when this tool was written.
Again, if the coreboot community doesn't think that linting the Kconfigs is a good idea, I have no issue with this being deleted. I actually supported the idea of using kconfiglib, which can do what this linter does and more, but that idea was shot down.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56410
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I48a17f6403470251be6b6d44bb82a8bdcbefe9f6
Gerrit-Change-Number: 56410
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:50:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Cliff Huang, Furquan Shaikh, Sridhar Siricilla, Bernardo Perez Priego.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55989 )
Change subject: cbfstool: Add helper function `buffer_from_file_aligned_size`
......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12:
not sure if i'm misinterpreting what this is supposed to do or what it does, but shouldn't this be granularity instead of alignment? alignment is about the beginning of the section
--
To view, visit https://review.coreboot.org/c/coreboot/+/55989
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad3430d476abcdad850505ac50e36cd5d5deecb4
Gerrit-Change-Number: 55989
Gerrit-PatchSet: 12
Gerrit-Owner: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:48:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Tim Crawford, Sean Rhodes, SRIDHAR SIRICILLA, Furquan Shaikh, Paul Menzel, Rizwan Qureshi, Angel Pons, Subrata Banik, Sridhar Siricilla, Arthur Heymans, Evgeny Zinoviev, Patrick Rudolph.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52800 )
Change subject: soc/intel: Allow enable/disable ME via CMOS
......................................................................
Patch Set 77:
(3 comments)
Patchset:
PS77:
not sure if i should already hit submit; had a quick look and found some possible error paths that probably could be handled differently. since i don't know the intel soc code too well, feel free to ack those if you think that that's the right thing to do there
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/52800/comment/90772a17_6067b557
PS77, Line 1003: printk(BIOS_ERR, "Error: Failed to change ME state in %u attempts!\n",
is there a way to recover from this failure case? maybe clear the me_state_counter on a cold reset? assuming that each do_global_reset will result in a warm reset; this assumption might be wrong, but if not it might be worth a look/thought
https://review.coreboot.org/c/coreboot/+/52800/comment/d9a15ae8_81bb1f4f
PS77, Line 1009: do_global_reset();
me_reset_with_count only gets called when some sort of option table is used and the me_state option exists. when the second option me_state_counter doesn't exist in this case, this will result in a reboot-loop. i'd print a more descriptive message here and also change the log level to BIOS_ERR. haven't looked into the cmos user option table code for a while, so not 100% sure if this could be a problem or not
--
To view, visit https://review.coreboot.org/c/coreboot/+/52800
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I374db3b7c0ded71cdc18f27970252fec7220cc20
Gerrit-Change-Number: 52800
Gerrit-PatchSet: 77
Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems>
Gerrit-Reviewer: Evgeny Zinoviev <me(a)ch1p.io>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: SRIDHAR SIRICILLA
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
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-CC: Andy Pont <andy.pont(a)sdcsystems.com>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Crawford <tcrawford(a)system76.com>
Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com>
Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems>
Gerrit-Attention: SRIDHAR SIRICILLA
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Evgeny Zinoviev <me(a)ch1p.io>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:42:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Bernardo Perez Priego.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58215 )
Change subject: util/cse_serger: Add command `create-cse-region`
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
File util/cbfstool/cse_serger.c:
https://review.coreboot.org/c/coreboot/+/58215/comment/1eef5c40_2e7bfdc6
PS3, Line 720: if (i == 0) {
> braces {} are not necessary for any arm of this statement
Please fix.
https://review.coreboot.org/c/coreboot/+/58215/comment/3b99b73d_44dc5e77
PS3, Line 733: memcpy(buffer_get(&wbuff), buffer_get(&rbuff), buffer_size(&rbuff));
should the size of wbuff be verified?
--
To view, visit https://review.coreboot.org/c/coreboot/+/58215
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib087f5516e5beb6390831ef4e34b0b067d3fbc8b
Gerrit-Change-Number: 58215
Gerrit-PatchSet: 3
Gerrit-Owner: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.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: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:35:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Bora Guvendik, Anil Kumar K, Cliff Huang, Selma Bensaid, Sridhar Siricilla, Bernardo Perez Priego.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58101 )
Change subject: lib/spd_bin: Fix for LDDR5
......................................................................
Patch Set 6:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58101/comment/50e95ebc_a10b9cf5
PS5, Line 9: Added LDDR5 for the param
: Fixed SPD name for LDDR5
> Done
List would be:
1. Add LDDR5 for the param
2. Fix SPD name for LDDR5
https://review.coreboot.org/c/coreboot/+/58101/comment/ae7f718e_29188752
PS5, Line 11: with this change, we won't get this warning message:
> Done
I do not see a blank line.
--
To view, visit https://review.coreboot.org/c/coreboot/+/58101
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I52ecf24a313d4cbdd0859c623533630c6a6c3713
Gerrit-Change-Number: 58101
Gerrit-PatchSet: 6
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Selma Bensaid <selma.bensaid(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.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: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Selma Bensaid <selma.bensaid(a)intel.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:29:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58224 )
Change subject: soc/amd/common/block/include/psp_efs: use unsigned type for bitfield
......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/common/block/include/amdblocks/psp_efs.h:
https://review.coreboot.org/c/coreboot/+/58224/comment/fff433e0_1757e458
PS1, Line 29: uint32_t
Why not use the generic type `unsigned int`?
--
To view, visit https://review.coreboot.org/c/coreboot/+/58224
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic630d1709174d90336746bc37da504437c12643c
Gerrit-Change-Number: 58224
Gerrit-PatchSet: 1
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: Paul Menzel <paulepanter(a)mailbox.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: Mon, 11 Oct 2021 21:27:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Karthik Ramasubramanian.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58114 )
Change subject: soc/amd/common/block/lpc: Refactor ESPI Setup
......................................................................
Patch Set 2:
(1 comment)
File src/soc/amd/common/block/lpc/lpc_util.c:
https://review.coreboot.org/c/coreboot/+/58114/comment/7d3b7e5e_4597fd14
PS2, Line 365: configure_espi
I think I would rather keep eSPI code out of lpc_util.c.
Can we call configure_espi from fch_pre_init?
--
To view, visit https://review.coreboot.org/c/coreboot/+/58114
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icfeba17dae0a964c9ca73686e29c18d965589934
Gerrit-Change-Number: 58114
Gerrit-PatchSet: 2
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
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: 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: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Mon, 11 Oct 2021 21:23:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment