[L] Change in coreboot[main]: Post-build control of serial

[Angel Pons (Code Review)]

Attention is currently required from: Jérémy Compostella, Karthik Ramasubramanian, Paul Menzel, Simon Glass, Stefan Reinauer.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77712?usp=email )

Change subject: Post-build control of serial
......................................................................

Patch Set 5:

(22 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/77712/comment/74609987_cdaf8d77 :
PS2, Line 7: Post-build control of serial
> Can you please give me an example?

This change adds the CCB subsystem, *and* it also uses the CCB subsystem to allow post-build control of logging. The "and" in the previous sentence tells me that this could've been split into two commits.

I imagine Paul wants a more descriptive commit summary. The problem is that summarizing what this change does (add CCB + post-build logging control), without exceeding the length limit for the commit summary, is not easy.

File Documentation/technotes/ccb.md:

https://review.coreboot.org/c/coreboot/+/77712/comment/6f81a540_16e937cc :
PS5, Line 37: New flags can be added to `enum ccb_flags` as needed. Note that the default
            : value of the flag should be zero, i.e. the normal state of coreboot is to have
            : all flags be zero. Flags should be used to change that normal state. Be careful
            : not to introduce flags which requires a non-zero value for normal operation.
It would be nice if you could reword this section. I'd also mention that **not-yet-defined** flag bits default to 0, and that the default value should maximize compatibility (as we do for Kconfig/runtime options; e.g. console enabled, overclocking disabled...).

I would've proposed a paragraph myself, but I'm feeling somewhat unimaginative at the moment...

https://review.coreboot.org/c/coreboot/+/77712/comment/48d5e678_fb759dad :
PS5, Line 42: extended 
nit: extend*ing*

https://review.coreboot.org/c/coreboot/+/77712/comment/073a479b_60b235d0 :
PS5, Line 43: ,
nit: Either continue the sentence after the comma, or replace it with a period.

https://review.coreboot.org/c/coreboot/+/77712/comment/3d24fab2_35b913f6 :
PS5, Line 54: build 
nit: buil*t*

https://review.coreboot.org/c/coreboot/+/77712/comment/08b8a341_ac708ff7 :
PS5, Line 68: CMOS option feature
That's the "option API", which has historically been coupled with CMOS options.

https://review.coreboot.org/c/coreboot/+/77712/comment/524d489b_0541a2d1 :
PS5, Line 68: could be expanded to provide an
            : API for CCB.
I agree that CCB could benefit from some "read the value for a given option" API, but I think the option API isn't appropriate for CCB. Only one implementation of the option API can be compiled in. If support for runtime configurable options is undesired, the "null" backend (reading options always returns the fallback value, writing options always fails) can be used.

However, one can implement a similar API for CCB which is separate from the option API.

File src/arch/x86/postcar.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/9113910f_a38fb957 :
PS5, Line 30: 		cbmem_initialize();
Looks like CBMEM init logs from postcar are no longer available. I'm not sure, but I think most platforms init CBMEM in late romstage.

File src/commonlib/include/commonlib/ccb.h:

https://review.coreboot.org/c/coreboot/+/77712/comment/e4a0e37b_a3f77f5b :
PS5, Line 26: #define CCB_MAGIC	0xc043b001
How does one guarantee that this magic appears exactly once? The docs don't describe it.

File src/console/Kconfig:

https://review.coreboot.org/c/coreboot/+/77712/comment/f4de09e9_293635da :
PS5, Line 440:  
(e.g. CBMEM)

File src/console/init.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/e814788c_99c169b3 :
PS5, Line 45: 		return CONSOLE_LOG_FAST;
I wonder if this "silent" CCB flag could be hooked up to the "fast" console stuff

File src/lib/Kconfig:

https://review.coreboot.org/c/coreboot/+/77712/comment/1cee0b62_873cd794 :
PS5, Line 100: #
nit: Stray `#`, please remove

File src/lib/ccb.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/be4ccd68_7fe57b98 :
PS3, Line 8: struct ccb ccb_static __attribute__((section(".init"))) = {
> Actually that one doesn't seem to work. […]

I think it's some compiler.h thing, but I thought it was automatically included?

File src/lib/ccb.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/2b3dcc89_0459d77a :
PS5, Line 16: struct ccb *ccb_get(void)
Is the CCB meant to be modifiable at runtime?

https://review.coreboot.org/c/coreboot/+/77712/comment/4fb24c1a_ddd43898 :
PS5, Line 23: 	return ccb->flags;
When `!ENV_HOLDS_CCB`, this can result in a NULL dereference even when assuming that `ccb_init()` has been called: CBMEM operations may fail.

https://review.coreboot.org/c/coreboot/+/77712/comment/7a1a2b2e_3b2e3d22 :
PS5, Line 32: sizeof(struct ccb)
nit: I'd prefer `sizeof(*ccb)` or `sizeof(*ptr)`.

File src/lib/hardwaremain.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/a0527dad_cfaf6656 :
PS5, Line 452: 	/* console_init() MUST PRECEDE ALL printk()! Additionally, ensure
This is no longer the case, there's prints in the CCB code.

File src/lib/prog_loaders.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/62a46740_f70523f1 :
PS5, Line 5: #include <commonlib/ccb_api.h>
Why?

File util/cbfstool/cbfstool.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/692fe0c3_cd1df7ba :
PS5, Line 745: table 
A table used as a name-to-bit-location map would be very useful. The same table could be used to both get and set values, as well as to list all known CCB flags/options.

https://review.coreboot.org/c/coreboot/+/77712/comment/f760441e_dc0e6ac6 :
PS5, Line 747: serial
"serial" can be confused with "serial number". How about using "console" as name?

https://review.coreboot.org/c/coreboot/+/77712/comment/e54fa98d_78d9e0f1 :
PS5, Line 752: 		val = CCB_CONSOLE_SILENT;
This will clobber all the other flags. Hopefully someone changes this when adding other flags.

https://review.coreboot.org/c/coreboot/+/77712/comment/fd5c1ef0_0b998b0a :
PS5, Line 753: "normal"
I wouldn't use "normal" here, as logging is typically only needed when things don't work. And we shouldn't imply (rather tangentially, admittedly) that it's normal for coreboot to not work, right? 😜

TL;DR Let's use another term for "enable non-CBMEM logging"