[coreboot-gerrit] Change in coreboot[master]: mb/google/{auron,slippy}: clear alt_gp_smi_en register

[Furquan Shaikh (Code Review)]

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.