Jingle Hsu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43004 )
Change subject: soc/intel/xeon_sp: Add RTC failure checking ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/43004/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43004/3//COMMIT_MSG@9 PS3, Line 9: Add a weak function mainboard_rtc_failed() : for mainboard customization. : : Check RTC_PWR_STS bit for RTC battery removal : or CMOS clear jumper triggered event.
Please re-flow for 75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/inclu... File src/soc/intel/xeon_sp/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/inclu... PS3, Line 28:
nit: two spaces here (it's a bit inside a register)
Done
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/pmuti... File src/soc/intel/xeon_sp/pmutil.c:
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/pmuti... PS3, Line 14: int
bool
Just follow declaration of weak function.
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/pmuti... PS3, Line 16: PCH_DEVFN_PMC<<12
huh? Why this shift?
Done
https://review.coreboot.org/c/coreboot/+/43004/3/src/soc/intel/xeon_sp/pmuti... PS3, Line 16: RTC_BATTERY_DEAD
Instead of masking here, I'd read the register as-is inside a variable and mask the desired bit else […]
Done