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

[Simon Glass (Code Review)]

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

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

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

Patch Set 6:

(18 comments)

File Documentation/technotes/ccb.md:

https://review.coreboot.org/c/coreboot/+/77712/comment/516cbd34_1bda7214 :
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. […]

Done

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

Done

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

Done

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

Done

File src/arch/x86/postcar.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/ae02335b_439b5c3f :
PS5, Line 30: 		cbmem_initialize();
> Looks like CBMEM init logs from postcar are no longer available. […]

That's right, which is why the current version can only work with ~SEPARATE_ROMSTAGE

I will see if I can deal with that

...done

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

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

I added 'Searching for the CCB' to the docs

File src/console/Kconfig:

https://review.coreboot.org/c/coreboot/+/77712/comment/a2266662_8b29e4b7 :
PS5, Line 440:
> (e.g. […]

Done

File src/console/init.c:

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

Could we leave that until later? Yes, I believe we could add something to control that too

File src/lib/Kconfig:

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

Done

File src/lib/ccb.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/b13184dd_9934a641 :
PS3, Line 8: struct ccb ccb_static __attribute__((section(".init"))) = {
> I think it's some compiler. […]

Not that I can see. The checkpath warning is applicable to Linux and U-Boot but I don't think it is correct for coreboot

If you can find something using it, please let me know

File src/lib/ccb.c:

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

Done

https://review.coreboot.org/c/coreboot/+/77712/comment/651009b7_7657d9b0 :
PS5, Line 23: 	return ccb->flags;
> When `!ENV_HOLDS_CCB`, this can result in a NULL dereference even when assuming that `ccb_init()` ha […]

Done

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

I slightly prefer what I have, since sizeof(ccb) is error prone depending on whether ccb is a pointer, or not.

Anyway, I don't know coreboot style very well, so I have changed this

https://review.coreboot.org/c/coreboot/+/77712/comment/6f47b23c_e6e979fc :
PS5, Line 26: static void add_ccb_to_cbmem(int is_recovery)
            : {
            : 	struct ccb *ptr;
            : 
            : 	ccb = ccb_get();
            : 	if (ccb) {
            : 		ptr = cbmem_add(CBMEM_ID_CCB, sizeof(struct ccb));
            : 		if (ptr) {
            : 			printk(BIOS_DEBUG, "CCB: Adding to CBMEM\n");
            : 			*ptr = *ccb;
            : 		}
            : 	}
            : }
> This will never works as romstage does not get the setting from bootblock.

Yes, it does not work with a separate romstage. I will see what I can do about that.

...I have figured that out, so am updating this.

File src/lib/hardwaremain.c:

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

OK I deleted it

File src/lib/prog_loaders.c:

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

Done

File util/cbfstool/cbfstool.c:

https://review.coreboot.org/c/coreboot/+/77712/comment/12c8dd85_2692da0e :
PS5, Line 747: serial
> "serial" can be confused with "serial number". […]

Done

https://review.coreboot.org/c/coreboot/+/77712/comment/ae976479_313ab503 :
PS5, Line 753: "normal"
> I wouldn't use "normal" here, as logging is typically only needed when things don't work. […]

I am trying to express the normal state.

I'll go with 'loud' for now