Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42503 )
Change subject: postcodes: replaced postcode values with their respective defined constants ......................................................................
Patch Set 1:
(11 comments)
This commit does way to many things at the same time, making it hard to review.
https://review.coreboot.org/c/coreboot/+/42503/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42503/1//COMMIT_MSG@9 PS1, Line 9: values that should be one commit, as it's only replacing hardcoded numbers with defines it's easy to review. Maybe even the first commit?
https://review.coreboot.org/c/coreboot/+/42503/1/src/arch/x86/postcar.c File src/arch/x86/postcar.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/arch/x86/postcar.c@36 PS1, Line 36: post_code(POST_RAMSTAGE_DIE); is this code reachable? doesn't run_ramstage call die()?
https://review.coreboot.org/c/coreboot/+/42503/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/arch/x86/postcar_loader... PS1, Line 101: die please use die_with_post_code()
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/car/bootblock... File src/cpu/intel/car/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/car/bootblock... PS1, Line 26: POST_BOOTBLOCK_SOC_EARLYINIT does postcodes need to be send before entering a function, or after all work has been done? That should be defined first and written down somewhere.
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/car/core2/cac... File src/cpu/intel/car/core2/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/car/core2/cac... PS1, Line 82: 0x23 that one got lost
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/haswell/romst... File src/cpu/intel/haswell/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/haswell/romst... PS1, Line 22: post_code move before enable_lapic
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/intel/haswell/romst... PS1, Line 48: post_code this one is missing
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/qemu-x86/bootblock.... File src/cpu/qemu-x86/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/qemu-x86/bootblock.... PS1, Line 9: POST_ENTRY_C_BOOTBLOCK that doesn't match the description
https://review.coreboot.org/c/coreboot/+/42503/1/src/cpu/qemu-x86/bootblock.... PS1, Line 14: post_code move into console_init()
https://review.coreboot.org/c/coreboot/+/42503/1/src/soc/intel/broadwell/rom... File src/soc/intel/broadwell/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/soc/intel/broadwell/rom... PS1, Line 37: post_code(POST_ENTRY_ROMSTAGE); move to src/cpu/intel/car/romstage.c
https://review.coreboot.org/c/coreboot/+/42503/1/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/42503/1/src/soc/intel/skylake/bootb... PS1, Line 24: POST_BOOTBLOCK_SOC_EARLYINIT move to src/lib/bootblock.c