Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
mb/google/{auron,slippy}: clear alt_gp_smi_en register
Commit 6ae8b50 [chromeec: Depend on events_copy_b to identify wake source] partially broke resume from suspend on Auron and Slippy variants when multiple events exist in the EC event queue. In the case of the device suspending manually and then subsequently having the lid closed, the device will be stuck in a resume/suspend/resume loop until the device is forcibly powered down.
Clearing the alt_gp_smi_en register mitigates this without any unintended side effects, best I can tell.
Test: build/boot several Auron/Slippy variants, test suspend/resume functional with both single and multiple events in EC event queue.
Change-Id: I7ec9ec575d41c5b7522c4e13fc32b0b7c77d20d9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/mainboard/google/auron/variants/auron_paine/devicetree.cb M src/mainboard/google/auron/variants/auron_yuna/devicetree.cb M src/mainboard/google/auron/variants/buddy/devicetree.cb M src/mainboard/google/auron/variants/gandof/devicetree.cb M src/mainboard/google/auron/variants/lulu/devicetree.cb M src/mainboard/google/auron/variants/samus/devicetree.cb M src/mainboard/google/slippy/variants/falco/devicetree.cb M src/mainboard/google/slippy/variants/leon/devicetree.cb M src/mainboard/google/slippy/variants/peppy/devicetree.cb M src/mainboard/google/slippy/variants/wolf/devicetree.cb 10 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/38984/1
diff --git a/src/mainboard/google/auron/variants/auron_paine/devicetree.cb b/src/mainboard/google/auron/variants/auron_paine/devicetree.cb index f6ec15a..8918f08 100644 --- a/src/mainboard/google/auron/variants/auron_paine/devicetree.cb +++ b/src/mainboard/google/auron/variants/auron_paine/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/auron/variants/auron_yuna/devicetree.cb b/src/mainboard/google/auron/variants/auron_yuna/devicetree.cb index db02565..2dd27bf 100644 --- a/src/mainboard/google/auron/variants/auron_yuna/devicetree.cb +++ b/src/mainboard/google/auron/variants/auron_yuna/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/auron/variants/buddy/devicetree.cb b/src/mainboard/google/auron/variants/buddy/devicetree.cb index e12882f..329dcba 100644 --- a/src/mainboard/google/auron/variants/buddy/devicetree.cb +++ b/src/mainboard/google/auron/variants/buddy/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/auron/variants/gandof/devicetree.cb b/src/mainboard/google/auron/variants/gandof/devicetree.cb index 230f5bd..c973014 100644 --- a/src/mainboard/google/auron/variants/gandof/devicetree.cb +++ b/src/mainboard/google/auron/variants/gandof/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/auron/variants/lulu/devicetree.cb b/src/mainboard/google/auron/variants/lulu/devicetree.cb index 1983045..fda25db 100644 --- a/src/mainboard/google/auron/variants/lulu/devicetree.cb +++ b/src/mainboard/google/auron/variants/lulu/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/auron/variants/samus/devicetree.cb b/src/mainboard/google/auron/variants/samus/devicetree.cb index 434ecc8..0dc8da2 100644 --- a/src/mainboard/google/auron/variants/samus/devicetree.cb +++ b/src/mainboard/google/auron/variants/samus/devicetree.cb @@ -34,7 +34,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/slippy/variants/falco/devicetree.cb b/src/mainboard/google/slippy/variants/falco/devicetree.cb index f2a9520..70459e0 100644 --- a/src/mainboard/google/slippy/variants/falco/devicetree.cb +++ b/src/mainboard/google/slippy/variants/falco/devicetree.cb @@ -60,7 +60,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/slippy/variants/leon/devicetree.cb b/src/mainboard/google/slippy/variants/leon/devicetree.cb index 8951e99..83a300b 100644 --- a/src/mainboard/google/slippy/variants/leon/devicetree.cb +++ b/src/mainboard/google/slippy/variants/leon/devicetree.cb @@ -60,7 +60,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/slippy/variants/peppy/devicetree.cb b/src/mainboard/google/slippy/variants/peppy/devicetree.cb index 6451d95..7bd40ac 100644 --- a/src/mainboard/google/slippy/variants/peppy/devicetree.cb +++ b/src/mainboard/google/slippy/variants/peppy/devicetree.cb @@ -60,7 +60,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010" diff --git a/src/mainboard/google/slippy/variants/wolf/devicetree.cb b/src/mainboard/google/slippy/variants/wolf/devicetree.cb index 2cad23b..89e6df7 100644 --- a/src/mainboard/google/slippy/variants/wolf/devicetree.cb +++ b/src/mainboard/google/slippy/variants/wolf/devicetree.cb @@ -60,7 +60,7 @@ register "gen2_dec" = "0x00fc0901"
# EC_SMI is GPIO34 - register "alt_gp_smi_en" = "0x0004" + register "alt_gp_smi_en" = "0x0000" register "gpe0_en_1" = "0x00000000" # EC_SCI is GPIO36 register "gpe0_en_2" = "0x00000010"
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
Doesn't this leave the whole EC SMI handler dead in the water?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
Patch Set 1:
Doesn't this leave the whole EC SMI handler dead in the water?
I guess, I couldn't figure out what it was actually doing given there was no apparent loss in functionality
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
Furquan, could you take a look please? If this is an issue there, I'd expect there to be similar issues on other Intel platforms.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
Patch Set 1:
Furquan, could you take a look please? If this is an issue there, I'd expect there to be similar issues on other Intel platforms.
it only seems to affect Haswell/Broadwell. I've tested from SandyBridge thru AmberLake, the rest have no issue. Skylake forward changed the EC SMI handling significantly.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
I'm starting to remember that already looked into this. IIRC, my conclusion was that the SMI handlers of these boards need to be updated accordingly, i.e. also use the B copy. However, I can't figure out why only these boards behave differently.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}: clear alt_gp_smi_en register ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Furquan, could you take a look please? If this is an issue there, I'd expect there to be similar issues on other Intel platforms.
it only seems to affect Haswell/Broadwell. I've tested from SandyBridge thru AmberLake, the rest have no issue. Skylake forward changed the EC SMI handling significantly.
I think one of the differences on the EC side is that with ToT version of EC, it drops any host events which are not present in the suspend mask - https://chromium.git.corp.google.com/chromiumos/platform/ec/+/0340484f40f22e.... This was not true on firmware branch for the devices mentioned here. Example on auron: https://chromium.git.corp.google.com/chromiumos/platform/ec/+/refs/heads/fir...
Thus, a lid closed event with ToT EC would never get captured in hostevents when host is sleeping. However, that is not true for older versions of the EC. There has been a lot of changes that have happened in handling of host events since then.
BTW,by making this change you are disabling all SMIs from EC. The only event for which SMI seems to be triggered from EC was lid closed event. Just curious: In commit message you mentioned that you are seeing a suspend->resume->suspend cycle. Does it go to S5 after the first resume? I believe the SMI handler puts the system into S5 and not S3?
Side-effect of this change would be that if you close lid in depthcharge or later phases of coreboot, it wouldn't result in shut down. In developer and normal mode, it would just boot to OS and shut down there. In case of recovery, I believe it would just be stuck at the recovery screen.
Another way to handle this would be to do a event drain in ec.c in the mainboard: diff --git a/src/mainboard/google/auron/ec.c b/src/mainboard/google/auron/ec.c index 3fc5373096e..790e5cd90a3 100644 --- a/src/mainboard/google/auron/ec.c +++ b/src/mainboard/google/auron/ec.c @@ -28,11 +28,17 @@ void mainboard_ec_init(void) .s3_wake_events = MAINBOARD_EC_S3_WAKE_EVENTS, .s5_wake_events = MAINBOARD_EC_S5_WAKE_EVENTS, }; + int s3_wakeup = acpi_is_wakeup_s3();
printk(BIOS_DEBUG, "mainboard_ec_init\n"); post_code(0xf0);
- google_chromeec_events_init(&info, acpi_is_wakeup_s3()); + google_chromeec_events_init(&info, s3_wakeup); + if (s3_wakeup) { + /* Clear pending events. */ + while (google_chromeec_get_event() != 0) + ; + }
post_code(0xf1); }
I believe that should solve your problem too.
Hello Angel Pons, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38984
to look at the new patch set (#2).
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup
Commit 6ae8b50 [chromeec: Depend on events_copy_b to identify wake source] partially broke resume from suspend on Auron and Slippy variants when multiple events exist in the EC event queue. In the case of the device suspending manually and then subsequently having the lid closed, the device will be stuck in a resume/suspend/resume loop until the device is forcibly powered down.
Mitigate this by clearing any pending EC events on S3 wakeup.
Test: build/boot several Auron/Slippy variants, test suspend/resume functional with both single and multiple events in EC event queue.
Change-Id: I7ec9ec575d41c5b7522c4e13fc32b0b7c77d20d9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/mainboard/google/auron/ec.c M src/mainboard/google/slippy/ec.c 2 files changed, 16 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/38984/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
Patch Set 1:
Patch Set 1:
Side-effect of this change would be that if you close lid in depthcharge or later phases of coreboot, it wouldn't result in shut down. In developer and normal mode, it would just boot to OS and shut down there. In case of recovery, I believe it would just be stuck at the recovery screen.
that seems less than ideal, and is definitely a regression
Another way to handle this would be to do a event drain in ec.c in the mainboard: diff --git a/src/mainboard/google/auron/ec.c b/src/mainboard/google/auron/ec.c index 3fc5373096e..790e5cd90a3 100644 --- a/src/mainboard/google/auron/ec.c +++ b/src/mainboard/google/auron/ec.c @@ -28,11 +28,17 @@ void mainboard_ec_init(void) .s3_wake_events = MAINBOARD_EC_S3_WAKE_EVENTS, .s5_wake_events = MAINBOARD_EC_S5_WAKE_EVENTS, };
int s3_wakeup = acpi_is_wakeup_s3(); printk(BIOS_DEBUG, "mainboard_ec_init\n"); post_code(0xf0);
google_chromeec_events_init(&info, acpi_is_wakeup_s3());
google_chromeec_events_init(&info, s3_wakeup);if (s3_wakeup) {/* Clear pending events. */while (google_chromeec_get_event() != 0);} post_code(0xf1);}
I believe that should solve your problem too.
it did, thanks. patchset updated
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
Patch Set 2: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup
Commit 6ae8b50 [chromeec: Depend on events_copy_b to identify wake source] partially broke resume from suspend on Auron and Slippy variants when multiple events exist in the EC event queue. In the case of the device suspending manually and then subsequently having the lid closed, the device will be stuck in a resume/suspend/resume loop until the device is forcibly powered down.
Mitigate this by clearing any pending EC events on S3 wakeup.
Test: build/boot several Auron/Slippy variants, test suspend/resume functional with both single and multiple events in EC event queue.
Change-Id: I7ec9ec575d41c5b7522c4e13fc32b0b7c77d20d9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38984 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/mainboard/google/auron/ec.c M src/mainboard/google/slippy/ec.c 2 files changed, 16 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/auron/ec.c b/src/mainboard/google/auron/ec.c index 3fc5373..0589864 100644 --- a/src/mainboard/google/auron/ec.c +++ b/src/mainboard/google/auron/ec.c @@ -29,10 +29,17 @@ .s5_wake_events = MAINBOARD_EC_S5_WAKE_EVENTS, };
+ int s3_wakeup = acpi_is_wakeup_s3(); + printk(BIOS_DEBUG, "mainboard_ec_init\n"); post_code(0xf0);
- google_chromeec_events_init(&info, acpi_is_wakeup_s3()); + google_chromeec_events_init(&info, s3_wakeup); + if (s3_wakeup) { + /* Clear pending events. */ + while (google_chromeec_get_event() != 0) + ; + }
post_code(0xf1); } diff --git a/src/mainboard/google/slippy/ec.c b/src/mainboard/google/slippy/ec.c index f8ab6b8..e296575 100644 --- a/src/mainboard/google/slippy/ec.c +++ b/src/mainboard/google/slippy/ec.c @@ -28,10 +28,17 @@ .s5_wake_events = MAINBOARD_EC_S5_WAKE_EVENTS, };
+ int s3_wakeup = acpi_is_wakeup_s3(); + printk(BIOS_DEBUG, "mainboard_ec_init\n"); post_code(0xf0);
- google_chromeec_events_init(&info, acpi_is_wakeup_s3()); + google_chromeec_events_init(&info, s3_wakeup); + if (s3_wakeup) { + /* Clear pending events. */ + while (google_chromeec_get_event() != 0) + ; + }
post_code(0xf1); }
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
Patch Set 3:
Side-effect of this change would be that if you close lid in depthcharge or later phases of coreboot, it wouldn't result in shut down. In developer and normal mode, it would just boot to OS and shut down there. In case of recovery, I believe it would just be stuck at the recovery screen.
I see some common code that checks to see if the lid is closed. Is that not being used for this platform? https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/pl...
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38984 )
Change subject: mb/google/{auron,slippy}/ec: clear pending events on S3 wakeup ......................................................................
Patch Set 3:
Patch Set 3:
Side-effect of this change would be that if you close lid in depthcharge or later phases of coreboot, it wouldn't result in shut down. In developer and normal mode, it would just boot to OS and shut down there. In case of recovery, I believe it would just be stuck at the recovery screen.
I see some common code that checks to see if the lid is closed. Is that not being used for this platform? https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/pl...
I suspect not with Matt's images since he isn't using vboot+depthcharge.
On x86 devices we were relying on the SMI handler for lid and power button events (and manipulating the EC event masks to enable/disable as needed) but it is used on ARM and Wilco devices.
-
Angel Pons (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)