Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
ec/google/chromeec: Pass reference of object to BBST() method
The BBST() method writes an updated status flag mask that is intended to be stored back in the battery object. This value needs to be passed as a reference to an object to prevent it from being evaluated at the time the method is loaded or it will not actually update the BSTP value in the battery device.
This was tested by instrumenting the _BST method in the primary battery and ensuring the value can be updated by the BBST method.
Change-Id: Ia8e207a2990059a60d96d8e0f3ed3c16a55c50f4 Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/ec/google/chromeec/acpi/battery.asl 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/34356/1
diff --git a/src/ec/google/chromeec/acpi/battery.asl b/src/ec/google/chromeec/acpi/battery.asl index 1ff5099..0253395 100644 --- a/src/ec/google/chromeec/acpi/battery.asl +++ b/src/ec/google/chromeec/acpi/battery.asl @@ -194,7 +194,7 @@ Store (Local1, Index (Arg1, 0))
// Notify if battery state has changed since last time - If (LNotEqual (Local1, Arg2)) { + If (LNotEqual (Local1, DeRefOf (Arg2))) { Store (Local1, Arg2) If (LEqual(Arg0, 0)) { Notify (BAT0, 0x80) @@ -326,7 +326,7 @@
Method (_BST, 0, Serialized) { - Return (BBST (0, PBST, BSTP, BFWK)) + Return (BBST (0, PBST, RefOf (BSTP), BFWK)) } }
@@ -416,7 +416,7 @@
Method (_BST, 0, Serialized) { - Return (BBST (1, PBST, BSTP, BFWK)) + Return (BBST (1, PBST, RefOf (BSTP), BFWK)) } } #endif
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... PS1, Line 198: Store (Local1, Arg2) I guess statements like this should only be valid if Arg2 (which is an input) is a RefOf?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... PS1, Line 198: Store (Local1, Arg2)
I guess statements like this should only be valid if Arg2 (which is an input) is a RefOf?
I think it is still valid to store to a local copy of Arg2 (treating it as another local variable) but if you didn't realize it was pass by value and was not a reference it may not have the intended effect.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
We're trying something new in gerrit. All comments have to be marked as resolved before a patch can be merged.
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/34356/1/src/ec/google/chromeec/acpi... PS1, Line 198: Store (Local1, Arg2)
I think it is still valid to store to a local copy of Arg2 (treating it as another local variable) b […]
Done
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34356 )
Change subject: ec/google/chromeec: Pass reference of object to BBST() method ......................................................................
ec/google/chromeec: Pass reference of object to BBST() method
The BBST() method writes an updated status flag mask that is intended to be stored back in the battery object. This value needs to be passed as a reference to an object to prevent it from being evaluated at the time the method is loaded or it will not actually update the BSTP value in the battery device.
This was tested by instrumenting the _BST method in the primary battery and ensuring the value can be updated by the BBST method.
Change-Id: Ia8e207a2990059a60d96d8e0f3ed3c16a55c50f4 Signed-off-by: Duncan Laurie dlaurie@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34356 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Martin Roth martinroth@google.com --- M src/ec/google/chromeec/acpi/battery.asl 1 file changed, 3 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/ec/google/chromeec/acpi/battery.asl b/src/ec/google/chromeec/acpi/battery.asl index 1ff5099..0253395 100644 --- a/src/ec/google/chromeec/acpi/battery.asl +++ b/src/ec/google/chromeec/acpi/battery.asl @@ -194,7 +194,7 @@ Store (Local1, Index (Arg1, 0))
// Notify if battery state has changed since last time - If (LNotEqual (Local1, Arg2)) { + If (LNotEqual (Local1, DeRefOf (Arg2))) { Store (Local1, Arg2) If (LEqual(Arg0, 0)) { Notify (BAT0, 0x80) @@ -326,7 +326,7 @@
Method (_BST, 0, Serialized) { - Return (BBST (0, PBST, BSTP, BFWK)) + Return (BBST (0, PBST, RefOf (BSTP), BFWK)) } }
@@ -416,7 +416,7 @@
Method (_BST, 0, Serialized) { - Return (BBST (1, PBST, BSTP, BFWK)) + Return (BBST (1, PBST, RefOf (BSTP), BFWK)) } } #endif