Attention is currently required from: Pranava Y N, Subrata Banik.
Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83233?usp=email )
Change subject: soc/intel/cmn/cse: Conditionally disable ME status reporting
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83233?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5e360408a7847968117df475ff244d79ceafa23f
Gerrit-Change-Number: 83233
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:32:36 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Pranava Y N, Subrata Banik.
Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83231?usp=email )
Change subject: drivers/intel/ish: Skip ISH version call if CSE sync is done by payload
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83231?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1895a4d3c44838a9cc6380912f09aa4f0e6687bd
Gerrit-Change-Number: 83231
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:32:13 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Pranava Y N, Subrata Banik.
Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83230?usp=email )
Change subject: soc/intel/cmn/cse: Skip CSE version call if sync is done by payload
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83230?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1a9e5583c79ebd81291a4b3ae24529b4582502cb
Gerrit-Change-Number: 83230
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:32:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Pranava Y N, Subrata Banik.
Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83229?usp=email )
Change subject: soc/intel/cmn/cse: Modify dependency on CSE EOP configs
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83229?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia6b616163d02be8d637b134fd3728c391fc63c90
Gerrit-Change-Number: 83229
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:31:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Pranava Y N, Subrata Banik.
Nick Vaccaro has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/83228?usp=email )
Change subject: soc/intel/cmn/cse: Modify dependency on CSE lite configs
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83228?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5ddaf6e29949231db84b14bf7ea2d34866bb8e6c
Gerrit-Change-Number: 83228
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:31:31 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: YH Lin, dinesh.sharma(a)intel.com.
Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83200?usp=email )
Change subject: drivers/wifi: Support Bluetooth Regulator Domain Settings
......................................................................
Patch Set 10:
(1 comment)
File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/83200/comment/ba1e3bf7_70d01202?us… :
PS9, Line 493: bsar
> May be consistent with other static `sar_emit_*` functions?
The other functions do receive a potentially NULL pointer. This is part of the design and they should return without complaining if they receive a NULL pointer.
This logic does does not apply to the function I am adding.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83200?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iebe95815c944d045f4cf686abcd1874a8a45e209
Gerrit-Change-Number: 83200
Gerrit-PatchSet: 10
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: YH Lin <yueherngl(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: dinesh.sharma(a)intel.com
Gerrit-Attention: YH Lin <yueherngl(a)google.com>
Gerrit-Attention: dinesh.sharma(a)intel.com
Gerrit-Comment-Date: Wed, 03 Jul 2024 00:23:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: YH Lin <yueherngl(a)google.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Arthur Heymans, Felix Held, Jérémy Compostella, Karthik Ramasubramanian, Paul Menzel, Simon Glass, Simon Glass.
Julius Werner has posted comments on this change by Simon Glass. ( https://review.coreboot.org/c/coreboot/+/77712?usp=email )
Change subject: Introduce a coreboot Control Block (CCB)
......................................................................
Patch Set 12: Code-Review-2
(4 comments)
Patchset:
PS10:
> OK I have updated this to support FMAP. […]
My concern is about *not* adding the option that causes so many issues and requires this to be so complicated (embedding in bootblock and then having to pass it on in memory). I'm not just asking for additional options. For now you have only added additional stuff to this series, that's not really addressing my concerns. I think you should *remove* the bootblock-embedded option and settle on one or more of the others (whether that's a CBFS file or an extra FMAP section I don't care too much, although CBFS file seems cleaner to me).
Arthur's suggestion of dumping early lines from the CBMEM console to the UART once it has reached the point where it could decide to enable UART also sounds fine to me. Again, I really don't think the "hang in super early messages" argument makes sense here because that super early code doesn't really suddenly start failing on stable systems, you only get hangs there when bringing up a new platform and in those cases people have their UARTs compile-time enabled anyway.
PS10:
> The goal of CCB (for console) is to allow all output to be produced, or none. […]
You keep saying that but you're not really explaining why that is true. Why are those first few bootblock messages so important for this (we're just fighting over a few lines compared to all the rest of coreboot output here, that's not really "half-baked" it's more like "98% baked")? Which user in what practical use case who doesn't already have the console compile-time enabled cares about those few lines? coreboot just doesn't really hang that early on stable platforms in practice.
I'm not really sure who you are still doing this for anyway, to be honest. We are not going to use this at ChromeOS. I haven't seen anyone else here say that they'd be going to use this yet (and if there are people interested, I'd really like to hear from them whether they think super early hangs in the bootblock are going to be important to them, and why). I just don't want to make bad architectural decisions and add all this unnecessarily complex stuff to address a concern that no actual user is really going to care about afterwards.
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/77712/comment/9def3752_13ce9dc7?us… :
PS10, Line 45: CCB(., 0x10)
> Do timestamps suffer from the same issue? What is the solution to resolve this?
Yes but timestamps have been there for decades and are widely used by the majority of coreboot users. I am not saying that features are never allowed to need an extra memlayout region, I'm just saying that it is a notable maintenance burden that should be justified by how useful the feature is and we should try to avoid it if other alternatives are possible. In this case there's an alternative that doesn't require this (storing console loglevel in a CBFS file) and I still don't feel like you have justified why we need this way more complicated and burdensome option when we could just do that instead (because nobody who doesn't have serial output enabled at compile time already because they're doing early bring-up tends to care about those first few bootblock messages before CBFS becomes accessible).
File src/lib/hardwaremain.c:
https://review.coreboot.org/c/coreboot/+/77712/comment/874f349f_050bf820?us… :
PS10, Line 449: cbmem_initialize();
> But in that case you wouldn't set the console to silent, would you?
No, but the console still won't start printing until `console_init()` is called, even if it is enabled at compile time. That's the problem here, you can't move `console_init()` further down because you would be breaking important debugging use cases for people who are actually enabling the console via Kconfig.
--
To view, visit https://review.coreboot.org/c/coreboot/+/77712?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I04e946b33035a493e833500351a0483761252613
Gerrit-Change-Number: 77712
Gerrit-PatchSet: 12
Gerrit-Owner: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 03 Jul 2024 00:23:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Jérémy Compostella, YH Lin, dinesh.sharma(a)intel.com.
Eric Lai has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83200?usp=email )
Change subject: drivers/wifi: Support Bluetooth Regulator Domain Settings
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/83200?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iebe95815c944d045f4cf686abcd1874a8a45e209
Gerrit-Change-Number: 83200
Gerrit-PatchSet: 10
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: YH Lin <yueherngl(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: dinesh.sharma(a)intel.com
Gerrit-Attention: YH Lin <yueherngl(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: dinesh.sharma(a)intel.com
Gerrit-Comment-Date: Wed, 03 Jul 2024 00:14:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes