Julius Werner 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 5:
(6 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. […]
I don't really care either way, but if you want to use Nicolas' variant please make sure to do
return !!(rsp.ec_reset_flags & RESET_FLAG_AP_WATCHDOG);
to force the value to be either 0 or 1.
https://review.coreboot.org/#/c/31834/4/src/ec/google/chromeec/ec.c@747 PS4, Line 747: #ifndef __PRE_RAM__
In patchset 3, I make mtk_wdt_init() call google_chromeec_get_ap_watchdog_flag() (in CL:31843), […]
I'm actually not sure why this is __PRE_RAM__ scoped, that might just be wrong or waaaayy outdated. Some 4+ years ago we didn't use -Wl,--gc-sections yet, so hiding things that weren't needed from the compiler was sometimes used to save binary size. These days unused code is garbage-collected automatically, so there should be no reason to #ifndef anything unless it actually doesn't compile without that.
https://review.coreboot.org/#/c/31834/5/src/ec/google/chromeec/ec_commands.h File src/ec/google/chromeec/ec_commands.h:
https://review.coreboot.org/#/c/31834/5/src/ec/google/chromeec/ec_commands.h... PS5, Line 4824: #define RESET_FLAG_AP_WATCHDOG (1 << 18) /* AP experienced a watchdog reset */ Is there a corresponding EC patch for this? Please always update ec_commands.h by landing the change in the EC repository first and then copying the whole new version of ec_commands.h into coreboot (ideally in a separate commit). Never diverge the coreboot copy from upstream.
Also, we should probably move *all* the reset reasons into ec_commands.h on the EC side, not just one? And we should probably prefix them with EC_ to not pollute the global namespace too much (since this file is copied into and included in a lot of different projects).
https://review.coreboot.org/#/c/31834/5/src/vendorcode/google/chromeos/Kconf... File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/#/c/31834/5/src/vendorcode/google/chromeos/Kconf... PS5, Line 93: bool "Use the AP watchdog flag stored in EC" This option should not be user-selectable, so don't put a naming string here (just plain 'bool').
https://review.coreboot.org/#/c/31834/5/src/vendorcode/google/chromeos/Kconf... PS5, Line 95: Even though it's not user-selectable, please add a small 'help' paragraph explaining what it does (as a comment for developers reading this file).
https://review.coreboot.org/#/c/31834/5/src/vendorcode/google/chromeos/watch... File src/vendorcode/google/chromeos/watchdog.c:
https://review.coreboot.org/#/c/31834/5/src/vendorcode/google/chromeos/watch... PS5, Line 51: if (!CONFIG(CHROMEOS_USE_EC_WATCHDOG_FLAG)) { I assume you're just trying to prevent the MTK driver from doing this? I don't think it makes sense to guard it like this, though. I would either check CONFIG(CHROMEOS_USE_EC_WATCHDOG_FLAG) in the MTK driver (and disable all the watchdog tracking/reporting stuff when it is set), or check !REGION_SIZE(watchdog_tombstone) here.