Attention is currently required from: Marc Jones, Jonathan Zhang, Johnny Lin, Christian Walter, Angel Pons, Subrata Banik, Patrick Rudolph. Deomid "rojer" Ryabkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51230 )
Change subject: soc/intel/xeon_sp/cpx: Set the MRC "cold boot required" status bit ......................................................................
Patch Set 4:
(5 comments)
File src/soc/intel/xeon_sp/cpx/chip.h:
https://review.coreboot.org/c/coreboot/+/51230/comment/672ef06e_9855f717 PS4, Line 107: #define CMOS_MRC_STATUS_ADDR 0x47
Let's make this more robust. […]
Done
File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/51230/comment/580ce63f_574a1558 PS4, Line 128:
Since the CMOS offset is only used in this file, I'd move its definition right here. […]
Done
https://review.coreboot.org/c/coreboot/+/51230/comment/0aa69ca3_49938c98 PS4, Line 132: ((uint8_t)
cast should not be necessary
Done
https://review.coreboot.org/c/coreboot/+/51230/comment/6c5f2571_c09967eb PS4, Line 132: uint8_t new_mrc_status = (mrc_status & 0xfe) | ((uint8_t) cold_boot_required);
Wouldn't it make more sense if cold_boot_required == 0? This way, if a machine loses RTC power, it w […]
agree, but for whatever reason it is what it is.
https://review.coreboot.org/c/coreboot/+/51230/comment/25a70a31_670b63ad PS4, Line 207: set_cmos_mrc_cold_boot_flag(cold_boot_required);
Simpler: […]
not as readable imo, but ok.