Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source.

[Ravi Chandra Sadineni (Code Review)]

Ravi Chandra Sadineni has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

chromeec: Depend on events_copy_b to identify the wake source.

Using google_chromeec_get_event() clears the event too. Thus if the
kernel has to identify the wake source, it has no way to do that. Thus
instead depend on events_copy_b to log the wake source. Please look at
go/hostevent-refactor for more info.

BUG=b:133262012
BRANCH=None
TEST=Hack hatch bios and make sure hostevent log is correct.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
---
M src/ec/google/chromeec/ec.c
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34801/1

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index 5a2630e..9ba9b1b 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -428,7 +428,8 @@

void google_chromeec_log_events(uint64_t mask)
 {
-	u8 event;
+	u64 events;
+	int i;
    uint64_t wake_mask;
    bool restore_wake_mask = false;

@@ -445,11 +446,14 @@
    	restore_wake_mask = true;
    }

-	while ((event = google_chromeec_get_event()) != 0) {
-		if (EC_HOST_EVENT_MASK(event) & mask)
-			elog_add_event_byte(ELOG_TYPE_EC_EVENT, event);
+	events = google_chromeec_get_events_b() & mask;
+	for (i = 0; i < sizeof(events) * 8; i++) {
+		if (EC_HOST_EVENT_MASK(i) & events)
+			elog_add_event_byte(ELOG_TYPE_EC_EVENT, i);
    }

+	google_chromeec_clear_events_b(events);
+
    if (restore_wake_mask)
    	google_chromeec_set_wake_mask(wake_mask);
 }
@@ -467,10 +471,6 @@
    	/* Disable SMI and wake events. */
    	google_chromeec_set_smi_mask(0);

-		/* Clear pending events. */
-		while (google_chromeec_get_event() != 0)
-			;
-
    	/* Restore SCI event mask. */
    	google_chromeec_set_sci_mask(info->sci_events);

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Ravi Chandra Sadineni (Code Review)]

Ravi Chandra Sadineni has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

ec/google/chromeec: Depend on events_copy_b to identify the wake source.

Using google_chromeec_get_event() clears the event too. Thus if the
kernel has to identify the wake source, it has no way to do that. Thus
instead depend on events_copy_b to log the wake source.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
---
M src/ec/google/chromeec/ec.c
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34801/2

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Ravi Chandra Sadineni (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

ec/google/chromeec: Depend on events_copy_b to identify the wake source.

Using google_chromeec_get_event() clears the event too. Thus if the
kernel has to identify the wake source, it has no way to do that. Thus
instead depend on events_copy_b to log the wake source.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
---
M src/ec/google/chromeec/ec.c
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34801/3

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Ravi Chandra Sadineni (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

ec/google/chromeec: Depend on events_copy_b to identify the wake source.

Using google_chromeec_get_event() clears the event too. Thus if the
kernel has to identify the wake source, it has no way to do that.
Instead depend on events_copy_b to log the wake source which is
primarily used for non-ACPI use cases.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
---
M src/ec/google/chromeec/ec.c
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34801/4

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

Patch Set 4:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/34801/4/src/ec/google/chromeec/ec.c... 
PS4, Line 431: u64
nit: uint64_t

https://review.coreboot.org/c/coreboot/+/34801/4/src/ec/google/chromeec/ec.c... 
PS4, Line 439: /*
             : 	 * If the EC supports unified wake masks, then there is no need to set
             : 	 * wake mask before reading out the host events.
             : 	 */
             : 	if (google_chromeec_check_feature(EC_FEATURE_UNIFIED_WAKE_MASKS) != 1) {
             : 		wake_mask = google_chromeec_get_wake_mask();
             : 		google_chromeec_set_wake_mask(mask);
             : 		restore_wake_mask = true;
             : 	}
             : 
Now that we are relying on events_copy_b there is no need to do this setting/restoring of wake masks any more. That was only required when reading the host events using the ACPI command. Can you please confirm if this is required now?

Change in coreboot[master]: ec/google/chromeec: Depend on events_copy_b to identify the wake source.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG@7 
PS4, Line 7: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
Please remove the dot at the end of the commit message summary.

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG@13 
PS4, Line 13: 
Can you please elaborate on the problem that is solved?

Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source

[Ravi Chandra Sadineni (Code Review)]

Hello Duncan Laurie, build bot (Jenkins),

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

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

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

Change subject: chromeec: Depend on events_copy_b to identify the wake source
......................................................................

chromeec: Depend on events_copy_b to identify the wake source

google_chromec_get_event() depends on the main copy of EC which is
used by ACPI subsytem in the kernel for querying events.
google_chromeec_get_event() also clears the event from EC. Thus if the
kernel has to identify the wake source, it has no way to do that. Thus
instead depend on events_copy_b to log the wake source. Please look at
go/hostevent-refactor for more info.

BUG=b:133262012
BRANCH=None
TEST=Hack hatch bios and make sure hostevent log is correct.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
---
M src/ec/google/chromeec/ec.c
1 file changed, 7 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/34801/5

Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source

[Ravi Chandra Sadineni (Code Review)]

Ravi Chandra Sadineni has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: chromeec: Depend on events_copy_b to identify the wake source
......................................................................

Patch Set 5:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG@7 
PS4, Line 7: ec/google/chromeec: Depend on events_copy_b to identify the wake source.
> Please remove the dot at the end of the commit message summary.

Done

https://review.coreboot.org/c/coreboot/+/34801/4//COMMIT_MSG@13 
PS4, Line 13:
> Can you please elaborate on the problem that is solved?

Will the additional details I added help. Please let me know if it still does not make sense.

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

https://review.coreboot.org/c/coreboot/+/34801/4/src/ec/google/chromeec/ec.c... 
PS4, Line 431: u64
> nit: uint64_t

Done

https://review.coreboot.org/c/coreboot/+/34801/4/src/ec/google/chromeec/ec.c... 
PS4, Line 439: /*
             : 	 * If the EC supports unified wake masks, then there is no need to set
             : 	 * wake mask before reading out the host events.
             : 	 */
             : 	if (google_chromeec_check_feature(EC_FEATURE_UNIFIED_WAKE_MASKS) != 1) {
             : 		wake_mask = google_chromeec_get_wake_mask();
             : 		google_chromeec_set_wake_mask(mask);
             : 		restore_wake_mask = true;
             : 	}
             :
> Now that we are relying on events_copy_b there is no need to do this setting/restoring of wake masks […]

My bad. removed this code now.

Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: chromeec: Depend on events_copy_b to identify the wake source
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: chromeec: Depend on events_copy_b to identify the wake source
......................................................................

chromeec: Depend on events_copy_b to identify the wake source

google_chromec_get_event() depends on the main copy of EC which is
used by ACPI subsytem in the kernel for querying events.
google_chromeec_get_event() also clears the event from EC. Thus if the
kernel has to identify the wake source, it has no way to do that. Thus
instead depend on events_copy_b to log the wake source. Please look at
go/hostevent-refactor for more info.

BUG=b:133262012
BRANCH=None
TEST=Hack hatch bios and make sure hostevent log is correct.

Change-Id: I39caae2689e0c2a7bec16416978877885a9afc6c
Signed-off-by: Ravi Chandra Sadineni 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34801
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/ec/google/chromeec/ec.c
1 file changed, 7 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c
index ed53c61..89da3ae 100644
--- a/src/ec/google/chromeec/ec.c
+++ b/src/ec/google/chromeec/ec.c
@@ -428,30 +428,19 @@

void google_chromeec_log_events(uint64_t mask)
 {
-	u8 event;
-	uint64_t wake_mask;
-	bool restore_wake_mask = false;
+	uint64_t events;
+	int i;

if (!CONFIG(ELOG))
    	return;

-	/*
-	 * If the EC supports unified wake masks, then there is no need to set
-	 * wake mask before reading out the host events.
-	 */
-	if (google_chromeec_check_feature(EC_FEATURE_UNIFIED_WAKE_MASKS) != 1) {
-		wake_mask = google_chromeec_get_wake_mask();
-		google_chromeec_set_wake_mask(mask);
-		restore_wake_mask = true;
+	events = google_chromeec_get_events_b() & mask;
+	for (i = 0; i < sizeof(events) * 8; i++) {
+		if (EC_HOST_EVENT_MASK(i) & events)
+			elog_add_event_byte(ELOG_TYPE_EC_EVENT, i);
    }

-	while ((event = google_chromeec_get_event()) != 0) {
-		if (EC_HOST_EVENT_MASK(event) & mask)
-			elog_add_event_byte(ELOG_TYPE_EC_EVENT, event);
-	}
-
-	if (restore_wake_mask)
-		google_chromeec_set_wake_mask(wake_mask);
+	google_chromeec_clear_events_b(events);
 }

void google_chromeec_events_init(const struct google_chromeec_event_info *info,
@@ -467,10 +456,6 @@
    	/* Disable SMI and wake events. */
    	google_chromeec_set_smi_mask(0);

-		/* Clear pending events. */
-		while (google_chromeec_get_event() != 0)
-			;
-
    	/* Restore SCI event mask. */
    	google_chromeec_set_sci_mask(info->sci_events);

Change in coreboot[master]: chromeec: Depend on events_copy_b to identify the wake source

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34801 )

Change subject: chromeec: Depend on events_copy_b to identify the wake source
......................................................................

Patch Set 6:

this change breaks resume from S3 on older ChromeEC platforms which use alt_gp_smi_en (Haswell/Broadwell at least), in the case where there are multiple events in the primary EC event queue on resume.

EG, if device is suspended and then lid closed, on lid open the device will attempt to resume from S3, then immediately return to S3 once the alt_gp_smi_en register is set (in the LPC init code). If the alt_gp_smi_en register is not used (val 0x0000), or this change is partially reverted to still clear the primary event queue, then there is no issue.