Re: [RFC PATCH 00/10] cros_ec: Add support for Wilco EC

[Enric Balletbo Serra]

Hi Nick,

Many thanks for sending this upstream. Adding Guenter as also might be
interested and some few questions regarding the driver structure.

Missatge de Nick Crews <ncrews@xxxxxxxxxx> del dia ds., 15 de des.
2018 a les 1:21:
>
> The Chromebook named wilco contains a different Embedded Controller
> than the rest of the chromebook series, and thus the kernel requires
> a different driver than the already existing and generalized
> cros_ec_* drivers. Specifically, this driver adds support for getting
> and setting the RTC on the EC, adding a binary sysfs attribute
> that receives ACPI events from the EC, adding a binary sysfs
> attribute to request telemetry data from the EC (useful for enterprise
> applications), and adding normal sysfs attributes to get/set various
> other properties on the EC. The core of the communication with the EC
> is implemented in wilco_ec_mailbox.c, using a simple byte-level protocol
> with a checksum, transmitted over an eSPI bus. For debugging purposes,
> a raw attribute is also provided which can write/read arbitrary
> bytes to/from the eSPI bus.
>
> We attempted to adhere to the sysfs principles of "one piece of data per
> attribute" as much as possible, and mostly succeded. However, with the
> wilco_ec_adv_power.h attributes, which deal with scheduling power usage,
> we found it most elegant to bundle setting event times for an entire day
> into a single attribute, so at most you are using attributes formatted
> as "%d %d %d %d %d %d". With the telemetry attribute, we had to use a
> binary attribute, instead of the preferable human-readable ascii, in
> order to keep secure the information which is proprietary to the
> enterprise service provider. This opaque binary data will be read and
> sent using a proprietary daemon running on the OS. Finally, the
> "version" attribute returns a formatted result that looks something
> like:
> > cat /sys/bus/platform/devices/GOOG000C\:00/version
> Label        : 95.00.06
> SVN Revision : 5960a.06
> Model Number : 08;8
> Build Date   : 11/29/18
>
> The RTC driver is exposed as a standard RTC class driver with
> read/write functionality.
>
> For event notification, the Wilco EC can return extended events that
> are not handled by standard ACPI objects. These events can
> include hotkeys which map to standard functions like brightness
> controls, or information about EC controlled features like the
> charger or battery. These events are triggered with an
> ACPI Notify(0x90) and the event data buffer is read through an ACPI
> method provided by the BIOS which reads the event buffer from EC RAM.
> These events are then processed, with hotkey events being sent
> to the input subsystem and other events put into a queue which
> can be read by a userspace daemon via a sysfs attribute.
>
> The rest of the attributes are categorized as either "properties" or
> "legacy". "legacy" implies that the attribute existed on the EC before it
> was modified for ChromeOS, and "properties" implies that the attribute
> exposes functionality that was added to the EC specifically for
> ChromeOS. They are mostly boolean flags or percentages.
>
> A full thread of the development of these patches can be found at
> https://chromium-review.googlesource.com/c/1371034. This thread contains
> comments and revisions that could be helpful in understanding how the
> driver arrived at the state it is in now. The thread also contains some
> ChromeOS specific patches that actually enable the driver. If you want
> to test the patch yourself, you would have to install the ChromeOS SDK
> and cherry pick in these patches.
>
> I also wrote some integration tests using the Tast testing framework that
> ChromeOS uses. It would require a full ChromeOS SDK to actually run the
> tests, but the source of the tests, written in Go, are useful for
> understanding what the desired behavior is. You can view the tests here:
> https://chromium-review.googlesource.com/c/1372575
>
> This is still an initial version of the driver, and we are sending it
> upstream for comments now, so that we can incorporate any requested
> changes such that it eventually can be merged. Thank you for your
> comments!
>
>
> Duncan Laurie (6):
>   CHROMIUM: cros_ec: Remove cros_ec dependency in lpc_mec
>   CHROMIUM: wilco_ec: Add new driver for Wilco EC
>   CHROMIUM: wilco_ec: Add sysfs attributes
>   CHROMIUM: wilco_ec: Add support for raw commands in sysfs
>   CHROMIUM: wilco_ec: Add RTC class driver
>   CHROMIUM: wilco_ec: Add event handling
>
> Nick Crews (4):
>   CHROMIUM: wilco_ec: Move legacy attributes to separate file
>   CHROMIUM: wilco_ec: Add EC properties
>   CHROMIUM: wilco_ec: Add peakshift and adv_batt_charging
>   CHROMIUM: wilco_ec: Add binary telemetry attributes
>
>  drivers/platform/chrome/Kconfig               |  24 +-
>  drivers/platform/chrome/Makefile              |   9 +-
>  drivers/platform/chrome/cros_ec_lpc_mec.c     |  54 +-
>  drivers/platform/chrome/cros_ec_lpc_mec.h     |  45 +-
>  drivers/platform/chrome/cros_ec_lpc_reg.c     |  43 +-
>  drivers/platform/chrome/wilco_ec.h            | 180 ++++++
>  drivers/platform/chrome/wilco_ec_adv_power.c  | 533 ++++++++++++++++++
>  drivers/platform/chrome/wilco_ec_adv_power.h  | 193 +++++++
>  drivers/platform/chrome/wilco_ec_event.c      | 343 +++++++++++
>  drivers/platform/chrome/wilco_ec_legacy.c     | 204 +++++++
>  drivers/platform/chrome/wilco_ec_legacy.h     |  96 ++++
>  drivers/platform/chrome/wilco_ec_mailbox.c    | 427 ++++++++++++++
>  drivers/platform/chrome/wilco_ec_properties.c | 327 +++++++++++
>  drivers/platform/chrome/wilco_ec_properties.h | 163 ++++++
>  drivers/platform/chrome/wilco_ec_rtc.c        | 163 ++++++

Shouldn't this go to the RTC subsystem? Not sure if there are more
drivers that can go to a specific subsystem.

>  drivers/platform/chrome/wilco_ec_sysfs.c      | 253 +++++++++
>  drivers/platform/chrome/wilco_ec_sysfs_util.h |  47 ++
>  drivers/platform/chrome/wilco_ec_telemetry.c  |  66 +++
>  drivers/platform/chrome/wilco_ec_telemetry.h  |  42 ++
>  19 files changed, 3153 insertions(+), 59 deletions(-)

Wow, seems that this is going to be big, might make sense add a
wilco_ec subdirectory for all this?

>  create mode 100644 drivers/platform/chrome/wilco_ec.h
>  create mode 100644 drivers/platform/chrome/wilco_ec_adv_power.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_adv_power.h
>  create mode 100644 drivers/platform/chrome/wilco_ec_event.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_legacy.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_legacy.h
>  create mode 100644 drivers/platform/chrome/wilco_ec_mailbox.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_properties.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_properties.h
>  create mode 100644 drivers/platform/chrome/wilco_ec_rtc.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_sysfs.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_sysfs_util.h
>  create mode 100644 drivers/platform/chrome/wilco_ec_telemetry.c
>  create mode 100644 drivers/platform/chrome/wilco_ec_telemetry.h
>
> --
> 2.20.0.405.gbc1bbc6f85-goog
>
Thanks,
  Enric