Attention is currently required from: Martin Roth.
Simon Glass has posted comments on this change by Simon Glass. ( https://review.coreboot.org/c/coreboot/+/83293?usp=email )
Change subject: Support CCB at a fixed SPI-flash offset
......................................................................
Patch Set 3:
(7 comments)
File Documentation/technotes/ccb.md:
https://review.coreboot.org/c/coreboot/+/83293/comment/3e79b469_c18251fd?us… :
PS2, Line 88: Care should be taken that
: this region is in read-only flash, if preventing users from changing it is
: important.
:
> This is true for chromeos, but not every platform.
Indeed. What change do you suggest to the language here?
File Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83293/comment/b95540f7_5f26ccac?us… :
PS2, Line 1124: ifeq ($(CONFIG_CCB_FMAP),y)
> Maybe print a warning or just error out if the CCB FMAP alignment is different than what's in the fm […]
Do you mean the .fmd file? I cannot find the alignment you are referring to. The code here seems to be what sets the alignment, so far as I can tell.
https://review.coreboot.org/c/coreboot/+/83293/comment/549bcfbe_7f70c092?us… :
PS2, Line 1259: ifneq ($(CONFIG_ARCH_X86),)
> Make this the `else` of the above? Reverse the logic?
Done
https://review.coreboot.org/c/coreboot/+/83293/comment/67b0168e_534bda4b?us… :
PS2, Line 1274: /usr/bin/printf
> Maybe use `env printf` instead of the path?
Done
File src/lib/bootblock.c:
https://review.coreboot.org/c/coreboot/+/83293/comment/817e0762_3b05269b?us… :
PS2, Line 63: or FMAP
> Why does it need to be initialized late if it's at a known location in the SPI rom? Should this be […]
I am using the FMAP to indicate the region, so have to read the FMAP. But...
File src/lib/ccb.c:
https://review.coreboot.org/c/coreboot/+/83293/comment/d4c3abf4_0b6b90f8?us… :
PS2, Line 102: if (fmap_locate_area_as_rdev(CCB_REGION, rdev)) {
> It seems to me that the advantage of using FMAP vs CBFS is that we can parse the fmap at build time […]
...yes, we could plumb the FMAP address through to the build and avoid that.
I can take a look at this, if you think it would make a big difference?
I was hoping to show the different trade-offs for the three methods.
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/83293/comment/ccdb3cbb_6035e4d8?us… :
PS2, Line 731: /* Now try FMAP */
> Based on the Kconfig, we can't have CCB in more than one location. […]
coreboot itself is set to use one of the three options, but there is only one build of cbfstool, so it must support all three at once. It can tell which is being used by looking at the image, as it does here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83293?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: I8abc5ba55d75a3defdea548fffcedba74d4737c2
Gerrit-Change-Number: 83293
Gerrit-PatchSet: 3
Gerrit-Owner: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 07:48:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Attention is currently required from: Jérémy Compostella, Martin Roth.
Simon Glass has posted comments on this change by Simon Glass. ( https://review.coreboot.org/c/coreboot/+/79594?usp=email )
Change subject: Post-build control of serial
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS4:
> I'm not sure if this intends to just silence the serial console or all consoles. […]
It is intended to control what coreboot calls the 'interactive console', i.e. one that is sent to the user, normally over a serial port but not always. See console_interactive_tx_byte().
It does not affect writing console output to CBMEM, etc.
CONSOLE_INTERACTIVE_SILENT seems a little long? What do you suggest?
--
To view, visit https://review.coreboot.org/c/coreboot/+/79594?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: Ibd867950f117cc6b3dbc582505f3983a0dd714fb
Gerrit-Change-Number: 79594
Gerrit-PatchSet: 5
Gerrit-Owner: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 07:48:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Attention is currently required from: Arthur Heymans, Felix Held, Julius Werner, Jérémy Compostella, Karthik Ramasubramanian, Martin Roth, Paul Menzel, Simon Glass.
Simon Glass 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:
(5 comments)
Patchset:
PS10:
> My concern is about *not* adding the option that causes so many issues and requires this to be so co […]
Just accessing FMAP or CBFS produces console output, so it is not just one stage. Each stage would produce output before the banner if we don't use the cache and CBMEM. This series resolves that in a nice, clean way.
Stacking early CBMEM output is itself an added complexity. Why not just solve the problem properly?
As you can see, putting CCB in bootblock is pretty tidy.
PS10:
> You keep saying that but you're not really explaining why that is true. […]
Would you mind downloading the patches and trying them out? It might help you understand the trade-offs better.
ChromeOS had this feature 10 years ago on Snow, but it was lost somehow along the way.
A silent console needs to be silent. It can't output a small number of lines just because it is annoying to suppress those. Nor can it suppress the bootblock banner in 'loud' mode. We just need to know, right at the start, whether the console is silenced or not.
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/77712/comment/b070a783_cba4de04?us… :
PS10, Line 45: CCB(., 0x10)
> Yes but timestamps have been there for decades and are widely used by the majority of coreboot users […]
Open source projects can change and evolve. I actually feel that coreboot has lagged a bit in this area. It could really do with some updates, and a more positive attitude to genuinely new features, IMO.
File src/lib/ccb.c:
https://review.coreboot.org/c/coreboot/+/77712/comment/8d0ea4f0_f742e4c3?us… :
PS12, Line 82: BUG()
> return NULL after this to avoid returning an uninitialized ccb variable?
Done
File src/lib/hardwaremain.c:
https://review.coreboot.org/c/coreboot/+/77712/comment/f8ab1d33_bd40e694?us… :
PS10, Line 449: cbmem_initialize();
> No, but the console still won't start printing until `console_init()` is called, even if it is enabl […]
Well you can disable CCB in that case. Once it is debugged you can re-enable it. What am I missing?
--
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: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 03 Jul 2024 07:47:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
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: Simon Glass.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83293?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified-1 by build bot (Jenkins)
Change subject: Support CCB at a fixed SPI-flash offset
......................................................................
Support CCB at a fixed SPI-flash offset
Enhance the CCB to allow it to be in an FMAP region, instead of embedded
in the bootblock. This adds quite a bit of logic, particularly in
cbfstool which must now support adjusting the CCB region automatically.
This creates a CCB region in the FMAP containing the CCB. This is added
to the ROM during the build.
BUG=b:172341184, b:262546009, b:249105972
BRANCH=none
TEST=manually test each of the three options using:
make menuconfig; (select option); rm -r build && make -j30
qemu-system-x86_64 -bios build/coreboot.rom -nographic
(see that console appears)
build/util/cbfstool/cbfstool build/coreboot.rom
configure -n console -V quiet
qemu-system-x86_64 -bios build/coreboot.rom -nographic
(see that console is suppressed, except for first part of bootblock with
CCB_CBFS and CCB_FMAP)
Change-Id: I8abc5ba55d75a3defdea548fffcedba74d4737c2
Signed-off-by: Simon Glass <sjg(a)chromium.org>
---
M Documentation/technotes/ccb.md
M Makefile.mk
M src/Kconfig
M src/commonlib/include/commonlib/ccb.h
M src/commonlib/include/commonlib/region.h
M src/lib/bootblock.c
M src/lib/ccb.c
M util/cbfstool/cbfstool.c
M util/cbfstool/default-x86.fmd
9 files changed, 144 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/83293/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/83293?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: I8abc5ba55d75a3defdea548fffcedba74d4737c2
Gerrit-Change-Number: 83293
Gerrit-PatchSet: 3
Gerrit-Owner: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Attention: Simon Glass <sjg(a)chromium.org>
Attention is currently required from: Arthur Heymans, Felix Held, Jérémy Compostella, Karthik Ramasubramanian, Paul Menzel, Simon Glass, Simon Glass.
Hello Felix Singer, Julius Werner, Jérémy Compostella, Karthik Ramasubramanian, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/77712?usp=email
to look at the new patch set (#13).
The following approvals got outdated and were removed:
Verified-1 by build bot (Jenkins)
Change subject: Introduce a coreboot Control Block (CCB)
......................................................................
Introduce a coreboot Control Block (CCB)
It is annoying to have to create and maintain two completely
different builds of coreboot just make minor changes, such as to enable
or disable the console.
It would be much more convenient to have a 'silent' flag in the
image, which can be updated as needed, without needing to rebuild
coreboot.
Introduce the 'coreboot Control Block' (CCB) which can hold such
settings. It is designed to be available very early in bootblock,
before CBFS is ready. It is able to control the output of the very
first bootblock banner.
The CCB is then passed through the cache and placed in cbmem so it is
available to other stages.
Provide options in cbfstool to get and set settings in the CCB. This
makes it easy to use this feature.
BUG=b:172341184, b:262546009, b:249105972
BRANCH=none
TEST=see next CL
Change-Id: I04e946b33035a493e833500351a0483761252613
Signed-off-by: Simon Glass <sjg(a)chromium.org>
---
M 3rdparty/arm-trusted-firmware
M 3rdparty/cmocka
M 3rdparty/intel-microcode
M 3rdparty/libgfxinit
M 3rdparty/vboot
A Documentation/technotes/ccb.md
M Documentation/technotes/index.md
A Documentation/util/cbfstool/ccb.md
M Documentation/util/cbfstool/index.md
M src/Kconfig
M src/arch/x86/car.ld
M src/arch/x86/postcar.c
M src/arch/x86/romstage.c
M src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h
A src/commonlib/include/commonlib/ccb.h
A src/commonlib/include/commonlib/ccb_api.h
M src/include/memlayout.h
M src/include/rules.h
M src/include/symbols.h
M src/lib/Makefile.mk
M src/lib/bootblock.c
A src/lib/ccb.c
M src/lib/hardwaremain.c
M util/cbfstool/cbfstool.c
24 files changed, 510 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/77712/13
--
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: newpatchset
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: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Daniel Maslowski has posted comments on this change by Felix Held. ( https://review.coreboot.org/c/coreboot/+/83311?usp=email )
Change subject: Revert "Makefile.mk: Use Walloc-size GCC option"
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/83311?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: I0bdc909c0e53b5353743dca521c963bbec792f7e
Gerrit-Change-Number: 83311
Gerrit-PatchSet: 3
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Maslowski <info(a)orangecms.org>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com>
Gerrit-CC: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 06:40:12 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Nicholas Chin.
PugzAreCute has posted comments on this change by PugzAreCute. ( https://review.coreboot.org/c/coreboot/+/83267?usp=email )
Change subject: mb/gigabyte/ga-h61m-series: Initial GA-H61M-S2P-R3 bringup
......................................................................
Patch Set 6:
(2 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/83267/comment/06ad1b5d_72affdbb?us… :
PS5, Line 17: current version
> It probably would be good to list the actual version number so that this statement is valid if SeaBI […]
Done
https://review.coreboot.org/c/coreboot/+/83267/comment/8b443097_963680bd?us… :
PS5, Line 18: current version
> Same here. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83267?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: I106c195c890823f07227739c6b30133b996f6510
Gerrit-Change-Number: 83267
Gerrit-PatchSet: 6
Gerrit-Owner: PugzAreCute <me(a)pugzarecute.com>
Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 06:30:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com>