On mkbp systems, EC wastes time / power reporting events the kernel doesn't care about |
|||||||
Issue descriptionAs far as I can tell there is no kernel driver that cares about the host event "EC_HOST_EVENT_BATTERY". However, I can see it being sent to the AP sometimes, even going so far as to fire a cros_ec interrupt. See bug http://crosbug.com/p/55875 as an example. Possible there's some good reason for EC_HOST_EVENT_BATTERY to exist and maybe some future version of the kernel will use it? ...but I'm pretty sure it's just wasting power now. Should we add some sort of API for the kernel to tell the EC which commands it cares about? --- Not terribly critical--more of an optimization.
,
Aug 11 2016
EC_HOST_EVENT_BATTERY is handled by ACPI code on x86 to tell the kernel battery driver to re-read the battery info (_BIF or _BIX) structure as one of the values inside it may have changed: https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/chromeos-2016.05/src/ec/google/chromeec/acpi/ec.asl#213 This event should only be sent if one of the values in the info structure actually changed: in the EC it looks like either presence state or full capacity.
,
Aug 11 2016
Also the event masks can be set by the board, in coreboot there are handlers for google_chromeec_set_{sci,smi,wake}_mask() which can be used to configure what events to get different interrupts for.
,
Aug 11 2016
Updating to make it obvious that problems appear to only be on mkbp systems.
,
Apr 7 2017
,
May 12 2017
I have a proposed fix for this currently up for review here : https://chromium-review.googlesource.com/502078 common: allow report disabling of host events https://chromium-review.googlesource.com/503536 kevin: disable reporting of unneeded host events
,
Jun 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/e9215ba711d337e4cfc9524c4ef07b03a813c8fb commit e9215ba711d337e4cfc9524c4ef07b03a813c8fb Author: Nick Vaccaro <nvaccaro@chromium.org> Date: Sat Jun 10 04:44:03 2017 common: allow report disabling of host events Adds a mechanism that allows a board to disable interrupting the AP / kernel when the status of any one of the EC_HOST_EVENTS included in CONFIG_HOST_EVENT_REPORT_MASK changes state. Default state enables reporting of all events; a board can override this by defining CONFIG_HOST_EVENT_REPORT_MASK in its board.h file. NOTE: The host_set_events() and host_clear_events() routines no longer interrupt the AP if none of the host events the AP is interested in changed state. BRANCH=none BUG= chromium:637061 TEST=make buildall passes Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> Change-Id: I678fb9d9dab6890848b94b314efd711842b1fd48 Reviewed-on: https://chromium-review.googlesource.com/502078 Reviewed-by: Aseda Aboagye <aaboagye@chromium.org> [modify] https://crrev.com/e9215ba711d337e4cfc9524c4ef07b03a813c8fb/common/host_event_commands.c [modify] https://crrev.com/e9215ba711d337e4cfc9524c4ef07b03a813c8fb/include/config.h
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/2ef78186c980120560123b149d7092a51edbeb98 commit 2ef78186c980120560123b149d7092a51edbeb98 Author: Nick Vaccaro <nvaccaro@chromium.org> Date: Thu Jul 20 22:00:37 2017 kevin: disable reporting of unneeded host events Disable reporting of EC events to the linux kernel that are not used by kevin's kernel. BRANCH=none BUG= chromium:637061 TEST=make buildall passes, lid close puts AP into suspend, lid open turns on display Change-Id: I7841294aec0853f4820a262ec14e8ad6037e1060 Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/503536 Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/2ef78186c980120560123b149d7092a51edbeb98/board/kevin/board.h
,
Jul 21 2017
,
Jul 21 2017
Nick: do you have any info on how big of a deal this was? If it's non-trivial do we want to port it to the gru firmware branch?
,
Aug 3 2017
Not sure how to best quantify the savings, but on boot, without this change, there are ~130 chromeos-ec interrupts. With this change, I see ~103, or an approximate 20% reduction of ec interrupts.
,
Aug 3 2017
My vote would be to pick it to the gru firmware branch, but I'd let Shawn and/or Aseda make the final call.
,
Aug 3 2017
I'd rather not make this change on stable firmware unless we have a specific bug or performance issue that we're trying to fix. If we do want to land it, I would like to spend some time profiling EC interrupt frequency / behavior both with and without the patch (Comment #11 is a start) just to convince myself that the patch is working as intended and won't cause unintended side-effects.
,
Aug 3 2017
@13: OK fair enough. We can just take this going forward.
,
Jan 22 2018
,
Apr 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/4e3d2579bc4d7ad82cb87e955da666af38ba12b8 commit 4e3d2579bc4d7ad82cb87e955da666af38ba12b8 Author: Nick Vaccaro <nvaccaro@chromium.org> Date: Tue Apr 03 10:14:50 2018 common: allow report disabling of host events Adds a mechanism that allows a board to disable interrupting the AP / kernel when the status of any one of the EC_HOST_EVENTS included in CONFIG_HOST_EVENT_REPORT_MASK changes state. Default state enables reporting of all events; a board can override this by defining CONFIG_HOST_EVENT_REPORT_MASK in its board.h file. NOTE: The host_set_events() and host_clear_events() routines no longer interrupt the AP if none of the host events the AP is interested in changed state. BRANCH=none BUG= chromium:637061 TEST=make buildall passes Change-Id: Ifbea6a76a13c56c3f499d193ee39dc5fee7ca977 Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/502078 Reviewed-by: Aseda Aboagye <aaboagye@chromium.org> (cherry picked from commit e9215ba711d337e4cfc9524c4ef07b03a813c8fb) Reviewed-on: https://chromium-review.googlesource.com/989860 Reviewed-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Tested-by: Joel Kitching <kitching@chromium.org> Trybot-Ready: Joel Kitching <kitching@chromium.org> [modify] https://crrev.com/4e3d2579bc4d7ad82cb87e955da666af38ba12b8/common/host_event_commands.c [modify] https://crrev.com/4e3d2579bc4d7ad82cb87e955da666af38ba12b8/include/config.h
,
Jun 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/a470caeabe1618972bfd258320efa12c7c86b39c commit a470caeabe1618972bfd258320efa12c7c86b39c Author: Joel Kitching <kitching@chromium.org> Date: Mon Jun 25 21:54:45 2018 Revert "common: allow report disabling of host events" This reverts commit 4e3d2579bc4d7ad82cb87e955da666af38ba12b8. Reason for revert: Re-using 32-bit host events instead. See b/110292722 Original change's description: > common: allow report disabling of host events > > Adds a mechanism that allows a board to disable interrupting the AP / > kernel when the status of any one of the EC_HOST_EVENTS included in > CONFIG_HOST_EVENT_REPORT_MASK changes state. Default state enables > reporting of all events; a board can override this by defining > CONFIG_HOST_EVENT_REPORT_MASK in its board.h file. > > NOTE: The host_set_events() and host_clear_events() routines no longer > interrupt the AP if none of the host events the AP is interested in > changed state. > > BRANCH=none > BUG= chromium:637061 > TEST=make buildall passes > > Change-Id: Ifbea6a76a13c56c3f499d193ee39dc5fee7ca977 > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org> > Reviewed-on: https://chromium-review.googlesource.com/502078 > Reviewed-by: Aseda Aboagye <aaboagye@chromium.org> > (cherry picked from commit e9215ba711d337e4cfc9524c4ef07b03a813c8fb) > Reviewed-on: https://chromium-review.googlesource.com/989860 > Reviewed-by: Joel Kitching <kitching@chromium.org> > Commit-Queue: Joel Kitching <kitching@chromium.org> > Tested-by: Joel Kitching <kitching@chromium.org> > Trybot-Ready: Joel Kitching <kitching@chromium.org> Bug: chromium:637061 Change-Id: Idb30d9f5aca5997f408b963b06fd7ac417519496 Reviewed-on: https://chromium-review.googlesource.com/1113790 Reviewed-by: Joel Kitching <kitching@chromium.org> Commit-Queue: Joel Kitching <kitching@chromium.org> Tested-by: Joel Kitching <kitching@chromium.org> Trybot-Ready: Joel Kitching <kitching@chromium.org> [modify] https://crrev.com/a470caeabe1618972bfd258320efa12c7c86b39c/common/host_event_commands.c [modify] https://crrev.com/a470caeabe1618972bfd258320efa12c7c86b39c/include/config.h |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by snanda@chromium.org
, Aug 11 2016