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.
64 comments:
Patch Set #11, 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.
Patch Set #11, 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.
Patch Set #11, 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.
Patch Set #11, Line 11: Updated define values reflect the newer changes to post_codes.h
I don't see anything like that anywhere.
File src/cpu/intel/car/core2/cache_as_ram.S:
Patch Set #11, Line 90: post_code(0x24)
Missed this one?
File src/cpu/intel/car/non-evict/cache_as_ram.S:
Patch Set #11, Line 16: post_code(POST_BOOTBLOCK_PRE_C_ENTRY)
Why is this moved?
Patch Set #11, Line 95: post_code(0x24)
Missed this one?
Patch Set #11, Line 181: post_code(POST_CAR_CACHE_EVICTION)
Why is this moved?
File src/cpu/intel/car/non-evict/exit_car.S:
Patch Set #11, Line 28: post_code(POST_EXIT_CAR_EVICTION_DISABLE)
Why is this moved?
Patch Set #11, Line 37: post_code(0x32)
Missed this one?
File src/cpu/intel/car/p3/cache_as_ram.S:
Patch Set #11, Line 51: post_code(0x22)
Missed this one?
File src/cpu/intel/car/p4-netburst/cache_as_ram.S:
Patch Set #11, Line 60: post_code(0x22)
Missed this one?
Patch Set #11, Line 118: post_code(0x24)
Missed this one?
Patch Set #11, Line 153: post_code(0x25)
Missed this one?
Patch Set #11, Line 168: post_code(0x26)
Missed this one?
Patch Set #11, Line 184: post_code(0x27)
Missed this one?
Patch Set #11, Line 191: post_code(0x28)
Missed this one?
Patch Set #11, Line 199: post_code(0x29)
Missed this one?
Patch Set #11, Line 342: post_code(POST_CAR_CACHE_CLEAR)
Why is this moved?
File src/cpu/intel/car/p4-netburst/exit_car.S:
Patch Set #11, Line 20: post_code(0x31)
Missed this one?
Patch Set #11, Line 28: post_code(0x32)
Missed this one?
File src/cpu/qemu-x86/cache_as_ram_bootblock.S:
Patch Set #11, Line 15: post_code(POST_CAR_CACHE_CLEAR)
Why is this moved?
Patch Set #11, Line 813: post_code(POST_BOOTBLOCK_MTRR_CHECK);
Why is this moved?
Patch Set #11, Line 1313: post_code(POST_PCI_SCAN_BRIDGES_ENTRY);
Why is this moved?
File src/drivers/intel/fsp1_1/cache_as_ram.S:
Patch Set #11, Line 19: post_code(POST_BOOTBLOCK_PRE_C_ENTRY)
Why is this moved?
File src/drivers/intel/fsp1_1/romstage.c:
Patch Set #11, Line 37: post_code(POST_SOC_RAM_PREINIT_ENTRY);
Why is this moved?
Patch Set #11, Line 103: post_code(0x30);
Missed this one?
Patch Set #11, Line 126: post_code(POST_BOARD_PREINIT_ENTRY);
Why is this moved?
Patch Set #11, Line 133: post_code(POST_MAINBOARD_SOC_INIT_ENTRY);
Why is this moved?
File src/mainboard/amd/thatcher/bootblock.c:
Patch Set #11, 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.
File src/mainboard/google/auron/ec.c:
Patch Set #11, Line 31: post_code(0xf1);
Missed this one?
File src/mainboard/google/cyan/ec.c:
Patch Set #11, Line 24: post_code(0xf1);
Missed this one?
File src/mainboard/google/link/ec.c:
Patch Set #11, Line 23: post_code(0xf1);
Missed this one?
File src/mainboard/google/rambi/ec.c:
Patch Set #11, Line 25: post_code(0xf1);
Missed this one?
File src/mainboard/google/slippy/ec.c:
Patch Set #11, Line 30: post_code(0xf1);
Missed this one?
File src/mainboard/intel/strago/ec.c:
Patch Set #11, Line 24: post_code(0xf1);
Missed this one?
File src/northbridge/intel/haswell/romstage.c:
Patch Set #11, Line 43: post_code(0x3a);
Missed this one?
Patch Set #11, Line 55: post_code(POST_ROM_SDRAM_INIT);
Why is this moved?
Patch Set #11, Line 79: post_code(0x3f);
Missed this one?
File src/northbridge/intel/pineview/romstage.c:
Patch Set #11, Line 56: post_code(POST_ROM_SDRAM_INIT);
Why is this moved?
Patch Set #11, Line 64: post_code(POST_ROM_RCBA_CONFIG);
Why is this moved?
File src/northbridge/intel/sandybridge/raminit_mrc.c:
Patch Set #11, Line 333: post_code(POST_ROM_PEI_DATA_FILL);
Why is this moved?
File src/northbridge/intel/sandybridge/romstage.c:
Patch Set #11, Line 77: post_code(0x3d);
Missed this one?
Patch Set #11, Line 81: post_code(0x3f);
Missed this one?
File src/soc/amd/picasso/bootblock/pre_c.S:
Patch Set #11, Line 7: post_code(0xb0)
Missed this one?
File src/soc/amd/picasso/romstage.c:
Patch Set #11, Line 82: post_code(0x41);
Missed this one?
Patch Set #11, Line 86: post_code(0x
Missed this one?
File src/soc/amd/stoneyridge/bootblock/bootblock.c:
Patch Set #11, Line 86: post_code(POST_BOOTBLOCK_SOC_EARLYINIT);
Why is this moved?
File src/soc/amd/stoneyridge/chip.c:
Patch Set #11, Line 144: post_code(0x46);
Missed this one?
File src/soc/amd/stoneyridge/romstage.c:
Patch Set #11, Line 72: post_code(0x40);
Missed this one?
Patch Set #11, Line 75: post_code(0x41);
Missed this one?
Patch Set #11, Line 105: post_code(0x60);
Missed this one?
Patch Set #11, Line 108: post_code(0x61);
Missed this one?
Patch Set #11, Line 114: post_code(0x43);
Missed this one?
Patch Set #11, Line 123: post_code(0x44);
Missed this one?
File src/soc/intel/broadwell/romstage/romstage.c:
Patch Set #11, Line 37: post_code(0x30);
Missed this one?
Patch Set #11, Line 59: post_code(POST_MEM_PREINIT_PREP_START);
Why is this moved?
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
Patch Set #11, Line 52: post_code(0x23)
Missed this one?
Patch Set #11, Line 79: post_code(0x24)
Missed this one?
Patch Set #11, Line 156: post_code(0x29)
Missed this one?
Patch Set #11, Line 340: post_code(0x26)
Missed this one?
File src/soc/intel/quark/romstage/fsp_params.c:
Patch Set #11, Line 37: post_code(POST_FSP_MEMORY_INIT);
Why is this moved?
File src/southbridge/intel/common/finalize.c:
Patch Set #11, Line 4: #include <console/console.h>
I don't think you meant to change this include?
Patch Set #11, Line 51: post_code(POST_OS_BOOT);
I'd replace this outb() in its own commit for reproducibility
To view, visit change 42503. To unsubscribe, or for help writing mail filters, visit settings.