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?usp... : 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?usp... : 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.
-
Julius Werner (Code Review)