[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/common/block/crashlog: Add Early CrashLog feature

[Arthur Heymans (Code Review)]

Attention is currently required from: Benjamin Doron.

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81937?usp=email )

Change subject: soc/intel/common/block/crashlog: Add Early CrashLog feature
......................................................................

Patch Set 1:

(5 comments)

File src/soc/intel/common/block/crashlog/early_crashlog.c:

https://review.coreboot.org/c/coreboot/+/81937/comment/b1b58618_e4775a21 :
PS1, Line 7: void __weak early_cl_enable_bars(void)
           : {
           : 	printk(BIOS_WARNING, "%s: Operation unsupported!\n", __func__);
           : }
           : 
           : void __weak early_cl_disable_bars(void)
           : {
           : 	printk(BIOS_WARNING, "%s: Operation unsupported!\n", __func__);
           : }
why weak functions? Linker errors when the functions are lacking at buildtime would be better than runtime warnings.

https://review.coreboot.org/c/coreboot/+/81937/comment/873e751d_0eb87fd5 :
PS1, Line 56: while (copied < size) {
I don't understand the naming of the variables. It looks like stuff only gets printed and not copied?

https://review.coreboot.org/c/coreboot/+/81937/comment/2419e673_2643cdcd :
PS1, Line 58: u32
void *, or maybe use read32p?

https://review.coreboot.org/c/coreboot/+/81937/comment/6b72ce69_91de8ceb :
PS1, Line 59: BIOS_DEBUG
BIOS_SPEW?

https://review.coreboot.org/c/coreboot/+/81937/comment/93f1adbc_c56f01c1 :
PS1, Line 59: printk(BIOS_DEBUG, "0x%x 0x%x 0x%x 0x%x ",
            : 			(data & 0xff000000) >> 24,
            : 			(data & 0x00ff0000) >> 16,
            : 			(data & 0x0000ff00) >> 8,
            : 			(data & 0x000000ff));
Any reason for big endian representation?