Nicolas Boichat has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31834 )
Change subject: google/chromeos: Support AP watchdog flag from Chrome EC ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/31834/4/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/#/c/31834/4/src/ec/google/chromeec/ec.c@745 PS4, Line 745: } I have mild preference for this, which is a little more explicit about what we are doing. But I'll let coreboot people decide ,-)
if (google_chromeec_get_uptime_info(&rsp)) return false;
return (rsp.ec_reset_flags & RESET_FLAG_AP_WATCHDOG);
https://review.coreboot.org/#/c/31834/4/src/ec/google/chromeec/ec.c@747 PS4, Line 747: #ifndef __PRE_RAM__ Do the functions above make sense if __PRE_RAM__ is defined?
https://review.coreboot.org/#/c/31834/4/src/vendorcode/google/chromeos/watch... File src/vendorcode/google/chromeos/watchdog.c:
https://review.coreboot.org/#/c/31834/4/src/vendorcode/google/chromeos/watch... PS4, Line 43: elog_add_event(ELOG_TYPE_ASYNC_HW_TIMER_EXPIRED); I find this code difficult to parse.
I'd rather do:
int hw_timer_expired = 0;
if (CONFIG) { hw_timer_expired = google_chromeec_get_ap_watchdog_flag(); } else { ... }
if (hw_timer_expired) elog_add_event(ELOG_TYPE_ASYNC_HW_TIMER_EXPIRED);