6 comments:
File src/ec/google/chromeec/ec.c:
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.
Patch Set #4, 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.
File src/ec/google/chromeec/ec_commands.h:
Patch Set #5, 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).
File src/vendorcode/google/chromeos/Kconfig:
Patch Set #5, 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').
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).
File src/vendorcode/google/chromeos/watchdog.c:
Patch Set #5, 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.
To view, visit change 31834. To unsubscribe, or for help writing mail filters, visit settings.