Change in ...coreboot[master]: ec/google/chromeec: Add AP watchdog into reset causes and reset flags.

[You-Cheng Syu (Code Review)]

You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31834

Change subject: ec/google/chromeec: Add AP watchdog into reset causes and reset flags.
......................................................................

ec/google/chromeec: Add AP watchdog into reset causes and reset flags.

Add the new reset cause (from ChromiumOS CL:1293132) and new reset flag
(from ChromiumOS CL:1295890).

BUG=b:109900671,b:118654976
BRANCH=none
TEST=none

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/1

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index 629144c..a5f068e 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -963,6 +963,7 @@
    	"reset: debug warm reboot",
    	"reset: at AP's request",
    	"reset: during EC initialization",
+		"reset: AP watchdog",
    };

static const size_t shutdown_cause_begin = 1 << 15;
@@ -1016,7 +1017,8 @@
    	"usb-resume",
    	"rdd",
    	"rbox",
-		"security"
+		"security",
+		"ap-watchdog",
    };
    struct ec_response_uptime_info cmd_resp;
    int i, flag, flag_count;

[Nicolas Boichat (Code Review)]

Nicolas Boichat has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: ec/google/chromeec: Add AP watchdog into reset causes and reset flags.
......................................................................

Patch Set 1: Code-Review+1

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: ec/google/chromeec: Add AP watchdog into reset causes and reset flags.
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31834/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31834/1//COMMIT_MSG@14 
PS1, Line 14: none
nit: you probably want to update how to test and verify here.

Change in ...coreboot[master]: ec/google/chromeec: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#2).

Change subject: ec/google/chromeec: Support AP watchdog flag from Chrome EC
......................................................................

ec/google/chromeec: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC. Also update the
tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=none

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/Kconfig
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
3 files changed, 34 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/2

Change in ...coreboot[master]: ec/google/chromeec: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#3).

Change subject: ec/google/chromeec: Support AP watchdog flag from Chrome EC
......................................................................

ec/google/chromeec: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC. Also update the
tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/Kconfig
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
3 files changed, 34 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/3

Change in ...coreboot[master]: ec/google/chromeec: Support AP watchdog flag from Chrome EC

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: ec/google/chromeec: Support AP watchdog flag from Chrome EC
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig 
File src/ec/google/chromeec/Kconfig:

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig@27 
PS3, Line 27: config EC_GOOGLE_CHROMEEC_AP_WATCHDOG_FLAG
This should probably be somewhere in soc/mediatek, not here.

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/ec.c 
File src/ec/google/chromeec/ec.c:

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/ec.c@39 
PS3, Line 39: #define RESET_FLAG_AP_WATCHDOG (1 << 18)  /* AP experienced a watchdog reset */
Is this not in ec_command.h? Sounds like it should be...

Change in ...coreboot[master]: ec/google/chromeec: Support AP watchdog flag from Chrome EC

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: ec/google/chromeec: Support AP watchdog flag from Chrome EC
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig 
File src/ec/google/chromeec/Kconfig:

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig@27 
PS3, Line 27: config EC_GOOGLE_CHROMEEC_AP_WATCHDOG_FLAG
> This should probably be somewhere in soc/mediatek, not here.

Actually, scratch that. I don't really like the way you're hacking this into the MediaTek WDT driver either.

How about we leave this here, and then use it to add a check to src/vendorcode/google/chromeos/watchdog.c#elog_handle_watchdog_tombstone? Then you don't need to change the MTK code at all, and this would work out on any platform as long as you enable the Kconfig.

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#4).

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC.

Add a new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG, which makes
elog_handle_watchdog_tombstone() determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

Also update the tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/ec/google/chromeec/ec_commands.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
5 files changed, 49 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/4

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

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 4:

(2 comments)

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig 
File src/ec/google/chromeec/Kconfig:

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/Kconfig@27 
PS3, Line 27: config EC_GOOGLE_CHROMEEC_AP_WATCHDOG_FLAG
> Actually, scratch that. […]

Good idea!

I moved the Kconfig option to vendorcode/google/chromeos since we read it there now.

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/ec.c 
File src/ec/google/chromeec/ec.c:

https://review.coreboot.org/#/c/31834/3/src/ec/google/chromeec/ec.c@39 
PS3, Line 39: #define RESET_FLAG_AP_WATCHDOG (1 << 18)  /* AP experienced a watchdog reset */
> Is this not in ec_command.h? Sounds like it should be...

Moved to ec_command.h

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Nicolas Boichat (Code Review)]

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);

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#5).

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC.

Add a new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG, which makes
elog_handle_watchdog_tombstone() determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

Also update the tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/ec/google/chromeec/ec_commands.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
5 files changed, 48 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/5

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

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 5:

(2 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@747 
PS4, Line 747: #ifndef __PRE_RAM__
> Do the functions above make sense if __PRE_RAM__ is defined?

In patchset 3, I make mtk_wdt_init() call google_chromeec_get_ap_watchdog_flag() (in CL:31843),
and mtk_wdt_init should be called before ramstage.

In the current patchset, google_chromeec_get_ap_watchdog_flag() won't be called until ramstage. So yes we can move it back into __PRE_RAM__ scope.
But I think there is no reason to forbid AP from asking EC for uptime_info/ap_watchdog_flag before ramstage.
For example, google_chromeec_is_uhepi_supported() might be called in verstage. So I think communicating with EC before ramstage is allowed.

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. […]

Done

The code in patchset 4 actually changed the behavior of always reseting _watchdog_tombstone to zero after reading...

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Julius Werner (Code Review)]

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.

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#6).

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC.

Add a new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG, which makes
elog_handle_watchdog_tombstone() determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

Also update the tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
4 files changed, 44 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/6

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

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)

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Julius Werner (Code Review)]

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 7:

(1 comment)

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)) {
> It seems like currently MTK WDT driver is the only one who calls mark_watchdog_tombstone... […]

Right, I missed the assertion. And your argument makes sense that we may want to leave it in the memlayout since other 8183 board may want to use it. But I'm also not really happy about the dissymmetry of setting but not using the tombstone.

How about making the two things completely independent? So you'd write the function above like this:

bool flag = false;

if (CONFIG(...EC...)
   flag |= google_chromeec_...();

if (REGION_SIZE(...)) {
   flag |= read32(...) == ...MAGIC;
   write32(..., 0);
 }

if (flag)
   elog_add_event();

So if a watchdog event was recorded through either of the two mechanism, we log it.

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

Hello Nicolas Boichat, Julius Werner, Jonathan Brandmeyer, Hung-Te Lin, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31834

to look at the new patch set (#8).

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was trigger by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which can read the AP watchdog flag from Chrome EC.

Add a new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG, which makes
elog_handle_watchdog_tombstone() determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

Also update the tables of reset causes and reset flags.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
4 files changed, 46 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/8

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[You-Cheng Syu (Code Review)]

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 8:

(1 comment)

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)) {
> Right, I missed the assertion. […]

Sounds good. Done.

Change in ...coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Julius Werner (Code Review)]

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 8: Code-Review+2

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Martin Roth (Code Review)]

Martin Roth 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 8:

Waiting for updates to previous commit before merging.

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Martin Roth (Code Review)]

Martin Roth 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 9:

Waiting for previous patch before merging.

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu 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 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/31834/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/31834/1//COMMIT_MSG@14 
PS1, Line 14: none
> nit: you probably want to update how to test and verify here.

Done

https://review.coreboot.org/c/coreboot/+/31834/3/src/ec/google/chromeec/ec.c 
File src/ec/google/chromeec/ec.c:

https://review.coreboot.org/c/coreboot/+/31834/3/src/ec/google/chromeec/ec.c... 
PS3, Line 39: #define RESET_FLAG_AP_WATCHDOG (1 << 18)  /* AP experienced a watchdog reset */
> Moved to ec_command. […]

Done

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu 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 9:

(5 comments)

https://review.coreboot.org/c/coreboot/+/31834/3/src/ec/google/chromeec/Kcon... 
File src/ec/google/chromeec/Kconfig:

https://review.coreboot.org/c/coreboot/+/31834/3/src/ec/google/chromeec/Kcon... 
PS3, Line 27: config EC_GOOGLE_CHROMEEC_AP_WATCHDOG_FLAG
> Good idea! […]

Done

https://review.coreboot.org/c/coreboot/+/31834/4/src/ec/google/chromeec/ec.c 
File src/ec/google/chromeec/ec.c:

https://review.coreboot.org/c/coreboot/+/31834/4/src/ec/google/chromeec/ec.c... 
PS4, Line 745: }
> Ack

Ack

https://review.coreboot.org/c/coreboot/+/31834/4/src/ec/google/chromeec/ec.c... 
PS4, Line 747: #ifndef __PRE_RAM__
> Thanks for the explanation.

__PRE_RAM__ has been removed.

https://review.coreboot.org/c/coreboot/+/31834/5/src/ec/google/chromeec/ec_c... 
File src/ec/google/chromeec/ec_commands.h:

https://review.coreboot.org/c/coreboot/+/31834/5/src/ec/google/chromeec/ec_c... 
PS5, Line 4824: #define RESET_FLAG_AP_WATCHDOG (1 << 18)  /* AP experienced a watchdog reset */
> I have uploaded two CLs for ec_commands.h changes: […]

Done

https://review.coreboot.org/c/coreboot/+/31834/5/src/vendorcode/google/chrom... 
File src/vendorcode/google/chromeos/watchdog.c:

https://review.coreboot.org/c/coreboot/+/31834/5/src/vendorcode/google/chrom... 
PS5, Line 51: 	if (!CONFIG(CHROMEOS_USE_EC_WATCHDOG_FLAG)) {
> Sounds good. Done.

Done

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded a new patch set (#10) to the change originally created by You-Cheng Syu. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was triggered by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which reads the AP watchdog flag from Chrome EC, and updates the tables
of reset causes and reset flags.

A new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG is added for
elog_handle_watchdog_tombstone() to determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
4 files changed, 46 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/10

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded a new patch set (#12) to the change originally created by You-Cheng Syu. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was triggered by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which reads the AP watchdog flag from Chrome EC, and updates the tables
of reset causes and reset flags.

A new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG is added for
elog_handle_watchdog_tombstone() to determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
4 files changed, 46 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31834/12

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Hung-Te Lin (Code Review)]

Hung-Te Lin 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 12: Code-Review+2

Change in coreboot[master]: google/chromeos: Support AP watchdog flag from Chrome EC

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31834 )

Change subject: google/chromeos: Support AP watchdog flag from Chrome EC
......................................................................

google/chromeos: Support AP watchdog flag from Chrome EC

After ChromiumOS CL:1293132 and CL:1295890, Chrome EC can store the flag
telling if the last reboot was triggered by AP watchdog for some boards
(e.g., Kukui).

This CL adds a new function google_chromeec_get_ap_watchdog_flag(),
which reads the AP watchdog flag from Chrome EC, and updates the tables
of reset causes and reset flags.

A new Kconfig option CHROMEOS_USE_EC_WATCHDOG_FLAG is added for
elog_handle_watchdog_tombstone() to determine if watchdog reset was
triggered by the AP watchdog flag from EC instead of the tombstone in
AP.

BUG=b:109900671,b:118654976
BRANCH=none
TEST=test with https://review.coreboot.org/c/coreboot/+/31843

Change-Id: I7a970666a8c6da32ac1c6af8280e808fe7fc106d
Signed-off-by: You-Cheng Syu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31834
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
---
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/ec.h
M src/vendorcode/google/chromeos/Kconfig
M src/vendorcode/google/chromeos/watchdog.c
4 files changed, 46 insertions(+), 17 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index c34faaa..1b0f7ee 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -704,6 +704,29 @@
    return cec_cmd.cmd_code;
 }

+static uint16_t google_chromeec_get_uptime_info(
+	struct ec_response_uptime_info *rsp)
+{
+	struct chromeec_command cmd = {
+		.cmd_code = EC_CMD_GET_UPTIME_INFO,
+		.cmd_version = 0,
+		.cmd_data_in = NULL,
+		.cmd_size_in = 0,
+		.cmd_data_out = rsp,
+		.cmd_size_out = sizeof(*rsp),
+		.cmd_dev_index = 0,
+	};
+	google_chromeec_command(&cmd);
+	return cmd.cmd_code;
+}
+
+bool google_chromeec_get_ap_watchdog_flag(void)
+{
+	struct ec_response_uptime_info rsp;
+	return (!google_chromeec_get_uptime_info(&rsp) &&
+		(rsp.ec_reset_flags & EC_RESET_FLAG_AP_WATCHDOG));
+}
+
 int google_chromeec_i2c_xfer(uint8_t chip, uint8_t addr, int alen,
    		     uint8_t *buffer, int len, int is_read)
 {
@@ -944,6 +967,7 @@
    	"reset: debug warm reboot",
    	"reset: at AP's request",
    	"reset: during EC initialization",
+		"reset: AP watchdog",
    };

static const size_t shutdown_cause_begin = 1 << 15;
@@ -997,22 +1021,13 @@
    	"usb-resume",
    	"rdd",
    	"rbox",
-		"security"
+		"security",
+		"ap-watchdog",
    };
    struct ec_response_uptime_info cmd_resp;
    int i, flag, flag_count;

-	struct chromeec_command get_uptime_cmd = {
-		.cmd_code = EC_CMD_GET_UPTIME_INFO,
-		.cmd_version = 0,
-		.cmd_data_in = NULL,
-		.cmd_size_in = 0,
-		.cmd_data_out = &cmd_resp,
-		.cmd_size_out = sizeof(cmd_resp),
-		.cmd_dev_index = 0,
-	};
-	google_chromeec_command(&get_uptime_cmd);
-	if (get_uptime_cmd.cmd_code) {
+	if (google_chromeec_get_uptime_info(&cmd_resp)) {
    	/*
    	 * Deliberately say nothing for EC's that don't support this
    	 * command
diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h
index d7fe733..019f9c1 100644
--- a/src/ec/google/chromeec/ec.h
+++ b/src/ec/google/chromeec/ec.h
@@ -64,6 +64,7 @@
 void google_chromeec_post(u8 postcode);
 int google_chromeec_vbnv_context(int is_read, uint8_t *data, int len);
 uint8_t google_chromeec_get_switches(void);
+bool google_chromeec_get_ap_watchdog_flag(void);

/* Temporary secure storage commands */
 int google_chromeec_vstore_supported(void);
diff --git a/src/vendorcode/google/chromeos/Kconfig b/src/vendorcode/google/chromeos/Kconfig
index 26ee31e..2ff7ec7 100644
--- a/src/vendorcode/google/chromeos/Kconfig
+++ b/src/vendorcode/google/chromeos/Kconfig
@@ -89,5 +89,11 @@
      on normal boot as well as resume and coreboot is only involved
      in the resume piece w.r.t. the platform hierarchy.

+config CHROMEOS_USE_EC_WATCHDOG_FLAG
+	bool
+	default n
+	help
+	  Use the AP watchdog flag stored in EC.
+
 endif # CHROMEOS
 endmenu
diff --git a/src/vendorcode/google/chromeos/watchdog.c b/src/vendorcode/google/chromeos/watchdog.c
index 4557251..2b2959f 100644
--- a/src/vendorcode/google/chromeos/watchdog.c
+++ b/src/vendorcode/google/chromeos/watchdog.c
@@ -17,6 +17,7 @@
 #include <assert.h>
 #include <bootstate.h>
 #include <console/console.h>
+#include <ec/google/chromeec/ec.h>
 #include <elog.h>
 #include <reset.h>
 #include <symbols.h>
@@ -30,13 +31,19 @@

static void elog_handle_watchdog_tombstone(void *unused)
 {
-	if (!REGION_SIZE(watchdog_tombstone))
-		return;
+	bool flag = false;

-	if (read32(_watchdog_tombstone) == WATCHDOG_TOMBSTONE_MAGIC)
+	if (CONFIG(CHROMEOS_USE_EC_WATCHDOG_FLAG))
+		flag |= google_chromeec_get_ap_watchdog_flag();
+
+	if (REGION_SIZE(watchdog_tombstone)) {
+		flag |= (read32(_watchdog_tombstone) ==
+			 WATCHDOG_TOMBSTONE_MAGIC);
+		write32(_watchdog_tombstone, 0);
+	}
+
+	if (flag)
    	elog_add_event(ELOG_TYPE_ASYNC_HW_TIMER_EXPIRED);
-
-	write32(_watchdog_tombstone, 0);
 }

BOOT_STATE_INIT_ENTRY(BS_POST_DEVICE, BS_ON_ENTRY,