Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
ec/google/chromeec: Guard battery-related ACPI code
Boards which do not have a built-in battery, like Fizz and Puff-based Chromeboxes, still report the board using a battery but one not being connected, leading to a red battery icon with a disabled symbol in many OSes.
Mitigate this by guarding the inclusion of battery.asl with ifndef EC_NO_BATTERY_PRESENT, and define this for Fizz and Puff-based boards. Also guard _Qxx methods which reference BAT0.
Test: build/boot Puff variant, boot OS (eg, Windows 10 2H20), check that a missing battery is not reported by the OS.
Change-Id: I41df5a925c9a15553d9e818a2efed2210311fd45 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/ec/google/chromeec/acpi/ec.asl M src/mainboard/google/fizz/variants/baseboard/include/baseboard/ec.h M src/mainboard/google/hatch/variants/baseboard/include/puff/ec.h 3 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/48859/1
diff --git a/src/ec/google/chromeec/acpi/ec.asl b/src/ec/google/chromeec/acpi/ec.asl index 218d08b..effc2bf 100644 --- a/src/ec/google/chromeec/acpi/ec.asl +++ b/src/ec/google/chromeec/acpi/ec.asl @@ -263,20 +263,25 @@ // Battery Low Event Method (_Q06, 0, NotSerialized) { +#ifndef EC_NO_BATTERY_PRESENT Store ("EC: BATTERY LOW", Debug) Notify (BAT0, 0x80) +#endif }
// Battery Critical Event Method (_Q07, 0, NotSerialized) { +#ifndef EC_NO_BATTERY_PRESENT Store ("EC: BATTERY CRITICAL", Debug) Notify (BAT0, 0x80) +#endif }
// Battery Info Event Method (_Q08, 0, NotSerialized) { +#ifndef EC_NO_BATTERY_PRESENT Store ("EC: BATTERY INFO", Debug) Notify (BAT0, 0x81) #ifdef EC_ENABLE_SECOND_BATTERY_DEVICE @@ -284,6 +289,7 @@ Notify (BAT1, 0x81) } #endif +#endif }
// Thermal Overload Event @@ -323,8 +329,10 @@ // Battery Shutdown Imminent Method (_Q11, 0, NotSerialized) { +#ifndef EC_NO_BATTERY_PRESENT Store ("EC: BATTERY SHUTDOWN", Debug) Notify (BAT0, 0x80) +#endif }
// Throttle Start @@ -357,6 +365,7 @@ // Battery Status Method (_Q17, 0, NotSerialized) { +#ifndef EC_NO_BATTERY_PRESENT Store ("EC: BATTERY STATUS", Debug) Notify (BAT0, 0x80) #ifdef EC_ENABLE_SECOND_BATTERY_DEVICE @@ -364,6 +373,7 @@ Notify (BAT1, 0x80) } #endif +#endif }
// MKBP interrupt. @@ -567,7 +577,9 @@ #endif
#include "ac.asl" +#ifndef EC_NO_BATTERY_PRESENT #include "battery.asl" +#endif #include "cros_ec.asl"
#ifdef EC_ENABLE_ALS_DEVICE diff --git a/src/mainboard/google/fizz/variants/baseboard/include/baseboard/ec.h b/src/mainboard/google/fizz/variants/baseboard/include/baseboard/ec.h index 504b6de..b02a389 100644 --- a/src/mainboard/google/fizz/variants/baseboard/include/baseboard/ec.h +++ b/src/mainboard/google/fizz/variants/baseboard/include/baseboard/ec.h @@ -53,4 +53,7 @@
#define EC_ENABLE_MKBP_DEVICE /* Enable cros_ec_keyb device */
+/* Fizz-based boards do not have a built-in battery */ +#define EC_NO_BATTERY_PRESENT + #endif diff --git a/src/mainboard/google/hatch/variants/baseboard/include/puff/ec.h b/src/mainboard/google/hatch/variants/baseboard/include/puff/ec.h index 986cf61..36e1b73 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/puff/ec.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/puff/ec.h @@ -56,4 +56,7 @@ /* Enable EC sync interrupt, EC_SYNC_IRQ is defined in baseboard/gpio.h */ #define EC_ENABLE_SYNC_IRQ
+/* Puff-based boards do not have a built-in battery */ +#define EC_NO_BATTERY_PRESENT + #endif /* __BASEBOARD_EC_H__ */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48859/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/1/src/ec/google/chromeec/acpi... PS1, Line 266: EC_NO_BATTERY_PRESENT Instead of adding #ifs around this, can we use CondRefOf?
If (CondRefOf (BAT0)) { Notify (BAT0, 0x80) }
Same for other notifications below.
This will ensure that the Debug prints still show up in case the EC for any reason sends these events. I think it is helpful when debugging too.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48859/1/src/ec/google/chromeec/acpi... File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/1/src/ec/google/chromeec/acpi... PS1, Line 266: EC_NO_BATTERY_PRESENT
Instead of adding #ifs around this, can we use CondRefOf? […]
it would appear not, if battery.asl is not compiled in / BAT0 doesn't exist - iasl throws a compilation error. Testing if I can work around by setting _STA for BAT0 to 0x0 to achieve the same result
Attention is currently required from: Furquan Shaikh, Matt DeVillier.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Patchset:
PS1: Should this be done for beltino and jecht as well?
File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/comment/54b4c356_257d13be PS1, Line 266: EC_NO_BATTERY_PRESENT
it would appear not, if battery. […]
You could move the guards to not hide the debug prints.
Attention is currently required from: Furquan Shaikh, Angel Pons.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
Should this be done for beltino and jecht as well?
no, as they do not have an EC / use ChromeEC
File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/comment/bc5acadf_2402859c PS1, Line 266: EC_NO_BATTERY_PRESENT
You could move the guards to not hide the debug prints.
these are _Q methods, they are called by the OS, no?
Attention is currently required from: Furquan Shaikh, Matt DeVillier.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1:
(1 comment)
File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/comment/0848153a_68101f6b PS1, Line 266: EC_NO_BATTERY_PRESENT
these are _Q methods, they are called by the OS, no?
These _Qxx methods are EC queries. Can't recall how the exact mechanism works, but the EC pokes the host (via SCI?) and tells it to run one of these EC query methods.
If the EC asks the OS to invoke an EC query method for battery-related stuff when the device does not support batteries, it indicates the EC firmware thinks this device can have a battery, which is most likely a bug in the EC firmware. That's why Furquan wanted to leave the debug prints in: it would make debugging such things easier.
Attention is currently required from: Furquan Shaikh, Matt DeVillier, Angel Pons.
Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Patch Set 1:
(1 comment)
File src/ec/google/chromeec/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/48859/comment/e3cef612_343cf43d PS1, Line 266: EC_NO_BATTERY_PRESENT
These _Qxx methods are EC queries. […]
Could you not just guard the notify based on `BTEX`?
Matt DeVillier has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48859 )
Change subject: ec/google/chromeec: Guard battery-related ACPI code ......................................................................
Abandoned
dropping since this doesn't seem to be an issue in newer versions of Windows (and was never a problem in Linux)
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Matt DeVillier (Code Review)
-
Sean Rhodes (Code Review)