You-Cheng Syu 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 6:
(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 don't really care either way, but if you want to use Nicolas' variant please make sure to do […]
Ack
https://review.coreboot.org/#/c/31834/4/src/ec/google/chromeec/ec.c@747 PS4, Line 747: #ifndef __PRE_RAM__
I'm actually not sure why this is __PRE_RAM__ scoped, that might just be wrong or waaaayy outdated. […]
Thanks for the explanation.
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. […]
I have uploaded two CLs for ec_commands.h changes: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1520574 https://review.coreboot.org/c/coreboot/+/31885
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').
Done
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 (a […]
Done
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 […]
It seems like currently MTK WDT driver is the only one who calls mark_watchdog_tombstone... For Kukui, actually it won't call mark_watchdog_tombstone during watchdog initialization (instead, AP thinks it were doing cold reboot). I just wanted to reduce useless actions (as we're not reading the tombstone in this case).
If we check that in MTK WDT driver, then we're still changing their code, which is against your earlier comment...? When you say "check !REGION_SIZE(watchdog_tombstone) here", do you mean to abort or to return directly? (There is already an assertion here) And, I am not sure if we should remove the tombstone region from Kukui. (As it only takes 4 bytes, removing it also requires changing MT8183 memlayout.ld)