Attention is currently required from: Arthur Heymans, Felix Held, Jérémy Compostella, Karthik Ramasubramanian, Martin Roth, 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 13: Code-Review-2
(4 comments)
Patchset:
PS10:
> Would you mind downloading the patches and trying them out? It might help you understand the trade-o […]
I'm not talking about always outputting the first few messages, I'm talking about always suppressing them. Log output is used to debug something when it goes wrong, but it is incredibly unlikely that something in those first few lines of bootblock code goes wrong after a platform has passed the early bring-up stage where everyone has the console always-on anyway. (And of course, like Arthur suggested, we can still output those messages slightly after the fact by replaying them from the CBMEM console, so that it looks right on people's terminals.)
PS10:
> Just accessing FMAP or CBFS produces console output, so it is not just one stage. […]
There's already a `cbmem_dump_console_to_uart()` function, you can just call it. It's not complex in any way.
This patch is not "tidy" at all, it adds yet another configuration backend, yet another memlayout space that every single platform needs to provide space for, yet more ENV rules that complicate the code, and not anywhere near enough value to the proposed alternatives to justify all of that.
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/77712/comment/ef61bdbd_4575584a?us… :
PS10, Line 45: CCB(., 0x10)
> Open source projects can change and evolve. […]
We're adding new features all the time. Just a few weeks ago, for example, we added 64-bit payload support. That patch set went through a long and lively discussion about the pros and cons of different alternatives, the author was very receptive to feedback and redesigned the whole thing from scratch multiple times to follow the concerns and suggestions brought up by reviewers, and in the end we merged something that I think we're all quite happy with and that solves the problems much better than any solution any of us had initially suggested. That's how the process is supposed to work.
File src/lib/hardwaremain.c:
https://review.coreboot.org/c/coreboot/+/77712/comment/b1d79445_302d4b25?us… :
PS10, Line 449: cbmem_initialize();
> Well you can disable CCB in that case. Once it is debugged you can re-enable it. […]
Well, now reverted to the original order of calls here in patch set 13 so now it doesn't break things anymore. But now your CCB wouldn't work in ramstage anymore either because it's not available in `console_init()`. There is no good way to have both with your approach here.
--
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: 13
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: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
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: Thu, 11 Jul 2024 20:26:06 +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: Ashish Kumar Mishra, Julius Werner, Karthik Ramasubramanian, Shelley Chen.
Subrata Banik has posted comments on this change by Ashish Kumar Mishra. ( https://review.coreboot.org/c/coreboot/+/83420?usp=email )
Change subject: vc/google/chromeos: Add configurable compression for logo file in cbfs
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Is there really a platform where the most efficient compression algorithm for ramstage is LZMA but for the logo is LZ4? They get loaded under very similar conditions, so usually what's best for one should be best for the other.
>
> I'd prefer to avoid having separate compression Kconfigs for every single file that all do their own thing. There should usually be a single answer per platform which algorithm is better that applies the same to all post-RAM files.
I don't think there is any good or golden answer to this because for 32MB platform, we can take liberty to use LZ4 for most blobs to gain boot time in exchange of SPI size growth bt that is not true for 16MB platform where the SPI is so full that even 10KB makes significant impact hence, we have to choose the optimal ones to ensure size is taking higher priority.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83420?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: I57fbd0d3a39eaba3fb9d61e7a3fb5eeb44e3a839
Gerrit-Change-Number: 83420
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 11 Jul 2024 20:08:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Ashish Kumar Mishra, Karthik Ramasubramanian, Shelley Chen.
Julius Werner has posted comments on this change by Ashish Kumar Mishra. ( https://review.coreboot.org/c/coreboot/+/83420?usp=email )
Change subject: vc/google/chromeos: Add configurable compression for logo file in cbfs
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Is there really a platform where the most efficient compression algorithm for ramstage is LZMA but for the logo is LZ4? They get loaded under very similar conditions, so usually what's best for one should be best for the other.
I'd prefer to avoid having separate compression Kconfigs for every single file that all do their own thing. There should usually be a single answer per platform which algorithm is better that applies the same to all post-RAM files.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83420?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: I57fbd0d3a39eaba3fb9d61e7a3fb5eeb44e3a839
Gerrit-Change-Number: 83420
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 11 Jul 2024 19:44:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Felix Singer.
Elyes Haouas has posted comments on this change by Elyes Haouas. ( https://review.coreboot.org/c/coreboot/+/82245?usp=email )
Change subject: device/dram/spd: Add missing <smbios.h>
......................................................................
Patch Set 7:
(1 comment)
File src/device/dram/spd.c:
https://review.coreboot.org/c/coreboot/+/82245/comment/144f4cf9_69c6a940?us… :
PS7, Line 182: smbios_memory_type
it needs <smbios.h>
--
To view, visit https://review.coreboot.org/c/coreboot/+/82245?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: Iacab6171c61abd047c09ff7e20313a455bd8414f
Gerrit-Change-Number: 82245
Gerrit-PatchSet: 7
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Comment-Date: Thu, 11 Jul 2024 19:12:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Martin L Roth.
Jamie Ryu has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83225?usp=email )
Change subject: payloads/depthcharge: support depthcharge build with local changes
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83225/comment/5ec4944a_1ca13ab5?us… :
PS1, Line 14: deptcharge
depthcharge
--
To view, visit https://review.coreboot.org/c/coreboot/+/83225?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: I95c1f71ae149537a2191734a62f416b7b6f41fc6
Gerrit-Change-Number: 83225
Gerrit-PatchSet: 2
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
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: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Thu, 11 Jul 2024 19:06:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Anil Kumar K, Bora Guvendik, Martin L Roth.
Cliff Huang has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83225?usp=email )
Change subject: payloads/depthcharge: support depthcharge build with local changes
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83225/comment/5f282f59_c0443061?us… :
PS1, Line 11: geneerated
> generated
Done
https://review.coreboot.org/c/coreboot/+/83225/comment/08713c2b_6d40b15d?us… :
PS1, Line 12: Fix 'git fetch' for the repo other than origin/main.
> Can u share more details on the issue that u fixed here ?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83225?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: I95c1f71ae149537a2191734a62f416b7b6f41fc6
Gerrit-Change-Number: 83225
Gerrit-PatchSet: 1
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Thu, 11 Jul 2024 19:04:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bora Guvendik <bora.guvendik(a)intel.com>
Comment-In-Reply-To: Anil Kumar K <anil.kumar.k(a)intel.com>
Attention is currently required from: Cliff Huang, Martin L Roth.
Hello Anil Kumar K, Bora Guvendik, Martin L Roth, Wonkyu Kim, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83225?usp=email
to look at the new patch set (#2).
Change subject: payloads/depthcharge: support depthcharge build with local changes
......................................................................
payloads/depthcharge: support depthcharge build with local changes
Allow building with local changes.
Add dependency and target for generated static_fw_config.h.
This ensures this file is generated prior to building depthcharge.
Fix 'git fetch' for the repo other than origin/main. It only fetches
latest code if the branch name is origin/main. Use $(DEPTHCHARGE_BRANCH)
to fetch when different branch is used.
TEST=deptcharge should build with local changes
Signed-off-by: Cliff Huang <cliff.huang(a)intel.com>
Change-Id: I95c1f71ae149537a2191734a62f416b7b6f41fc6
---
M Makefile.mk
M payloads/external/Makefile.mk
M payloads/external/depthcharge/Makefile
3 files changed, 15 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/83225/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83225?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I95c1f71ae149537a2191734a62f416b7b6f41fc6
Gerrit-Change-Number: 83225
Gerrit-PatchSet: 2
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>