Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42503 )
Change subject: post_code: replace postcode values with their respective defined constants ......................................................................
Patch Set 11:
(64 comments)
I've marked all comments about "Why is this moved?" and "Missed this one?" as resolved because there's lots of them. This commit is very large and so thus very hard to review. I would not dare submit it as I am not sure if it could break something, like what Kyösti said about clobbering eax.
https://review.coreboot.org/c/coreboot/+/42503/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42503/11//COMMIT_MSG@7 PS11, Line 7: post_code: replace postcode values with their respective defined constants Reading this commit summary, I'd expect that this commit only does one thing: replace `post_code(0xFOO)` with `post_code(POST_FOO)`. However, it does more than that, and since it touches everything at once, it's hard to review.
Plus, if there's a list of things in a commit message, it's a good sign that they should be split up in separate commits.
https://review.coreboot.org/c/coreboot/+/42503/11//COMMIT_MSG@9 PS11, Line 9: Replace existing post_code call values with their defined constants If this is only about replacing hex numbers with equivalent macros (same value), it should be reproducible (result in the exact same coreboot binaries built with `BUILD_TIMELESS=1` or `abuild --timeless`). It would be good if this change could be in its own commit so that it can be verified easily.
https://review.coreboot.org/c/coreboot/+/42503/11//COMMIT_MSG@10 PS11, Line 10: Reorganize certain post_code calls to match their respective defines I would prefer to handle this on a platform-by-platform basis, especially because changing assembly code could accidentally result in clobbered registers. Plus, I've got questions as to why certain postcode prints were moved and why some others were left unchanged (still using hex values). The reasoning behind all those decisions would be easier to explain in the multiple commit messages of smaller commits.
https://review.coreboot.org/c/coreboot/+/42503/11//COMMIT_MSG@11 PS11, Line 11: Updated define values reflect the newer changes to post_codes.h I don't see anything like that anywhere.
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/core2/ca... File src/cpu/intel/car/core2/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/core2/ca... PS11, Line 90: post_code(0x24) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... File src/cpu/intel/car/non-evict/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... PS11, Line 16: post_code(POST_BOOTBLOCK_PRE_C_ENTRY) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... PS11, Line 95: post_code(0x24) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... PS11, Line 181: post_code(POST_CAR_CACHE_EVICTION) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... File src/cpu/intel/car/non-evict/exit_car.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... PS11, Line 28: post_code(POST_EXIT_CAR_EVICTION_DISABLE) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/non-evic... PS11, Line 37: post_code(0x32) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p3/cache... File src/cpu/intel/car/p3/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p3/cache... PS11, Line 51: post_code(0x22) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... File src/cpu/intel/car/p4-netburst/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 60: post_code(0x22) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 118: post_code(0x24) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 153: post_code(0x25) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 168: post_code(0x26) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 184: post_code(0x27) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 191: post_code(0x28) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 199: post_code(0x29) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 342: post_code(POST_CAR_CACHE_CLEAR) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... File src/cpu/intel/car/p4-netburst/exit_car.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 20: post_code(0x31) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/intel/car/p4-netbu... PS11, Line 28: post_code(0x32) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/qemu-x86/cache_as_... File src/cpu/qemu-x86/cache_as_ram_bootblock.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/qemu-x86/cache_as_... PS11, Line 15: post_code(POST_CAR_CACHE_CLEAR) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/x86/mtrr/mtrr.c File src/cpu/x86/mtrr/mtrr.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/cpu/x86/mtrr/mtrr.c@81... PS11, Line 813: post_code(POST_BOOTBLOCK_MTRR_CHECK); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/device/pci_device.c@13... PS11, Line 1313: post_code(POST_PCI_SCAN_BRIDGES_ENTRY); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/c... File src/drivers/intel/fsp1_1/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/c... PS11, Line 19: post_code(POST_BOOTBLOCK_PRE_C_ENTRY) Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/r... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/r... PS11, Line 37: post_code(POST_SOC_RAM_PREINIT_ENTRY); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/r... PS11, Line 103: post_code(0x30); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/r... PS11, Line 126: post_code(POST_BOARD_PREINIT_ENTRY); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/drivers/intel/fsp1_1/r... PS11, Line 133: post_code(POST_MAINBOARD_SOC_INIT_ENTRY); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/amd/thatcher... File src/mainboard/amd/thatcher/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/amd/thatcher... PS11, Line 13: post_code(0x30); It would make more sense to drop this unnecessary post_code call before this commit. I think it is dropped from several other places in CB:43132, because it is emitted elsewhere in common code?
It would be good (easier to review) to have a commit that drops unneeded `post_code(0x30)` calls and explains *why* in the commit message.
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/auron... File src/mainboard/google/auron/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/auron... PS11, Line 31: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/cyan/... File src/mainboard/google/cyan/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/cyan/... PS11, Line 24: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/link/... File src/mainboard/google/link/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/link/... PS11, Line 23: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/rambi... File src/mainboard/google/rambi/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/rambi... PS11, Line 25: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/slipp... File src/mainboard/google/slippy/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/google/slipp... PS11, Line 30: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/intel/strago... File src/mainboard/intel/strago/ec.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/mainboard/intel/strago... PS11, Line 24: post_code(0xf1); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/hasw... File src/northbridge/intel/haswell/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/hasw... PS11, Line 43: post_code(0x3a); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/hasw... PS11, Line 55: post_code(POST_ROM_SDRAM_INIT); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/hasw... PS11, Line 79: post_code(0x3f); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/pine... File src/northbridge/intel/pineview/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/pine... PS11, Line 56: post_code(POST_ROM_SDRAM_INIT); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/pine... PS11, Line 64: post_code(POST_ROM_RCBA_CONFIG); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/sand... PS11, Line 333: post_code(POST_ROM_PEI_DATA_FILL); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/sand... PS11, Line 77: post_code(0x3d); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/northbridge/intel/sand... PS11, Line 81: post_code(0x3f); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/pre_c.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/picasso/bootbl... PS11, Line 7: post_code(0xb0) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/picasso/romsta... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/picasso/romsta... PS11, Line 82: post_code(0x41); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/picasso/romsta... PS11, Line 86: post_code(0x Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/bo... File src/soc/amd/stoneyridge/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/bo... PS11, Line 86: post_code(POST_BOOTBLOCK_SOC_EARLYINIT); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ch... File src/soc/amd/stoneyridge/chip.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ch... PS11, Line 144: post_code(0x46); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... File src/soc/amd/stoneyridge/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 72: post_code(0x40); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 75: post_code(0x41); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 105: post_code(0x60); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 108: post_code(0x61); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 114: post_code(0x43); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/amd/stoneyridge/ro... PS11, Line 123: post_code(0x44); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/broadwell/ro... File src/soc/intel/broadwell/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/broadwell/ro... PS11, Line 37: post_code(0x30); Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/broadwell/ro... PS11, Line 59: post_code(POST_MEM_PREINIT_PREP_START); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/common/block... PS11, Line 52: post_code(0x23) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/common/block... PS11, Line 79: post_code(0x24) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/common/block... PS11, Line 156: post_code(0x29) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/common/block... PS11, Line 340: post_code(0x26) Missed this one?
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/quark/romsta... File src/soc/intel/quark/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/soc/intel/quark/romsta... PS11, Line 37: post_code(POST_FSP_MEMORY_INIT); Why is this moved?
https://review.coreboot.org/c/coreboot/+/42503/11/src/southbridge/intel/comm... File src/southbridge/intel/common/finalize.c:
https://review.coreboot.org/c/coreboot/+/42503/11/src/southbridge/intel/comm... PS11, Line 4: #include <console/console.h> I don't think you meant to change this include?
https://review.coreboot.org/c/coreboot/+/42503/11/src/southbridge/intel/comm... PS11, Line 51: post_code(POST_OS_BOOT); I'd replace this outb() in its own commit for reproducibility