Re: [PATCH v2] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime

[Nick Crews]

Hi!

On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
>
> On Fri, 2019-06-07 at 12:27 +0200, Enric Balletbo i Serra wrote:
> > On many boards, communication between the kernel and the Embedded
> > Controller happens over an LPC bus. In these cases, the kernel config
> > CONFIG_CROS_EC_LPC is enabled. Some of these LPC boards contain a
> > Microchip Embedded Controller (MEC) that is different from the regular
> > EC. On these devices, the same LPC bus is used, but the protocol is
> > a little different. In these cases, the CONFIG_CROS_EC_LPC_MEC kernel
> > config is enabled. Currently, the kernel decides at compile-time whether
> > or not to use the MEC variant, and, when that kernel option is selected
> > it breaks the other boards. We would like a kind of runtime detection to
> > avoid this.
> >
> > This patch adds that detection mechanism by probing the protocol at
> > runtime, first we assume that a MEC variant is connected, and if the
> > protocol fails it fallbacks to the regular EC. This adds a bit of
> > overload because we try to read twice on those LPC boards that doesn't
> > contain a MEC variant, but is a better solution than having to select the
> > EC variant at compile-time.
> >
> > While here also fix the alignment in Kconfig file for this config option
> > replacing the spaces by tabs.
> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> > ---
> > Hi,
> >
> > This is the second attempt to solve the issue to be able to select at
> > runtime the CrOS MEC variant. My first thought was check for a device
> > ID,
> > the MEC1322 has a register that contains the device ID, however I am not
> > sure if we can read that register from the host without modifying the
> > firmware. Also, I am not sure if the MEC1322 is the only device used
> > that supports that LPC protocol variant, so I ended with a more easy
> > solution, check if the protocol fails or not. Some background on this
> > issue can be found [1] and [2]
> >
> > The patch has been tested on:
> >  - Acer Chromebook R11 (Cyan - MEC variant)
> >  - Pixel Chromebook 2015 (Samus - non-MEC variant)
> >  - Dell Chromebook 11 (Wolf - non-MEC variant)
> >  - Toshiba Chromebook (Leon - non-MEC variant)
> >
> > Nick, could you test the patch for Wilco?
> >
> > Best regards,
> >  Enric
> >
> > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=932626
> > [2] https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1474254
> >
> > Changes in v2:
> > - Remove global bool to indicate the kind of variant as suggested by Ezequiel.
> > - Create an internal operations struct to allow different variants.
> >
> >  drivers/platform/chrome/Kconfig           | 29 +++------
> >  drivers/platform/chrome/Makefile          |  3 +-
> >  drivers/platform/chrome/cros_ec_lpc.c     | 76 ++++++++++++++++-------
> >  drivers/platform/chrome/cros_ec_lpc_reg.c | 39 +++---------
> >  drivers/platform/chrome/cros_ec_lpc_reg.h | 26 ++++++++
> >  drivers/platform/chrome/wilco_ec/Kconfig  |  2 +-
> >  6 files changed, 98 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > index 2826f7136f65..453e69733842 100644
> > --- a/drivers/platform/chrome/Kconfig
> > +++ b/drivers/platform/chrome/Kconfig
> > @@ -83,28 +83,17 @@ config CROS_EC_SPI
> >         'pre-amble' bytes before the response actually starts.
> >
> >  config CROS_EC_LPC
> > -        tristate "ChromeOS Embedded Controller (LPC)"
> > -        depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> > -        help
> > -          If you say Y here, you get support for talking to the ChromeOS EC
> > -          over an LPC bus. This uses a simple byte-level protocol with a
> > -          checksum. This is used for userspace access only. The kernel
> > -          typically has its own communication methods.
> > -
> > -          To compile this driver as a module, choose M here: the
> > -          module will be called cros_ec_lpc.
> > -
> > -config CROS_EC_LPC_MEC
> > -     bool "ChromeOS Embedded Controller LPC Microchip EC (MEC) variant"
> > -     depends on CROS_EC_LPC
> > -     default n
> > +     tristate "ChromeOS Embedded Controller (LPC)"
> > +     depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> >       help
> > -       If you say Y here, a variant LPC protocol for the Microchip EC
> > -       will be used. Note that this variant is not backward compatible
> > -       with non-Microchip ECs.
> > +       If you say Y here, you get support for talking to the ChromeOS EC
> > +       over an LPC bus, including the LPC Microchip EC (MEC) variant.
> > +       This uses a simple byte-level protocol with a checksum. This is
> > +       used for userspace access only. The kernel typically has its own
> > +       communication methods.
> >
> > -       If you have a ChromeOS Embedded Controller Microchip EC variant
> > -       choose Y here.
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called cros_ec_lpcs.
> >
> >  config CROS_EC_PROTO
> >          bool
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index 1b2f1dcfcd5c..d6416411888f 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile
> > @@ -9,8 +9,7 @@ obj-$(CONFIG_CHROMEOS_TBMC)           += chromeos_tbmc.o
> >  obj-$(CONFIG_CROS_EC_I2C)            += cros_ec_i2c.o
> >  obj-$(CONFIG_CROS_EC_RPMSG)          += cros_ec_rpmsg.o
> >  obj-$(CONFIG_CROS_EC_SPI)            += cros_ec_spi.o
> > -cros_ec_lpcs-objs                    := cros_ec_lpc.o cros_ec_lpc_reg.o
> > -cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC)       += cros_ec_lpc_mec.o
> > +cros_ec_lpcs-objs                    := cros_ec_lpc.o cros_ec_lpc_reg.o cros_ec_lpc_mec.o
> >  obj-$(CONFIG_CROS_EC_LPC)            += cros_ec_lpcs.o
> >  obj-$(CONFIG_CROS_EC_PROTO)          += cros_ec_proto.o cros_ec_trace.o
> >  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> > index c9c240fbe7c6..91cb4dd34764 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc.c
> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
> > @@ -28,6 +28,22 @@
> >  #define DRV_NAME "cros_ec_lpcs"
> >  #define ACPI_DRV_NAME "GOOG0004"
> >
> > +/**
> > + * struct lpc_ops - LPC driver methods
> > + *
> > + * @read: Read bytes from a given LPC-mapped address.
> > + * @write: Write bytes to a given LPC-mapped address.
> > + */
> > +struct lpc_ops {
> > +     u8      (*read)(unsigned int offset, unsigned int length, u8 *dest);
> > +     u8      (*write)(unsigned int offset, unsigned int length, u8 *msg);
> > +};
> > +
> > +static struct lpc_ops cros_ec_lpc_ops = {
> > +     .read   = cros_ec_lpc_mec_read_bytes,
> > +     .write  = cros_ec_lpc_mec_write_bytes,
> > +};
> > +
>
> While this is better than a global boolean, it's still not
> per-device.
>
> I guess it's not an issue given you typically (always?)
> have one cros-ec device per platform.
>
> However, I'm still wondering if it's not better to make it
> per-device (as the bus is per-device?).

Enric and I were discussing this. Up to this point, there has only been
one EC device per platform, and I think this is a reasonable
expectation to maintain. I'm adding Stefan Reinauer, the Chrome OS
EC lead, for their thoughts. Stefan, we are discussing whether or not we
need to support multiple communication protocols at the same time,
for instance if a device had multiple ECs, each with a different protocol.

If we really wanted to support multiple ECs, there would be some other
work to do besides this one fix, since the memory addresses that
we write to are hardcoded into the drivers. In order to support
multiple devices,
not only would we need to make the xfer algorithms per-device, but would
also need to make the memory addresses per-device. I would love
some feedback on this, but my initial thought would be to add a
"void *xfer_protocol_data" field to struct cros_ec_device, alongside
the two existing
int (*cmd_xfer)(struct cros_ec_device *ec, struct cros_ec_command *msg);
int (*pkt_xfer)(struct cros_ec_device *ec, struct cros_ec_command *msg);
fields. Then, each different protocol (lpc, i2c, spi, rpmsg, ishtp;
some of these
are only in the Chromium tree as of now) would be able to use this
field as needed,
for example to store the I2C address or the is_MEC flag for each device.

Thanks,
Nick

>
> >  /* True if ACPI device is present */
> >  static bool cros_ec_lpc_acpi_device_found;
> >
> > @@ -38,7 +54,7 @@ static int ec_response_timed_out(void)
> >
> >       usleep_range(200, 300);
> >       do {
> > -             if (!(cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_CMD, 1, &data) &
> > +             if (!(cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data) &
> >                   EC_LPC_STATUS_BUSY_MASK))
> >                       return 0;
> >               usleep_range(100, 200);
> > @@ -58,11 +74,11 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
> >       ret = cros_ec_prepare_tx(ec, msg);
> >
> >       /* Write buffer */
> > -     cros_ec_lpc_write_bytes(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
> > +     cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
> >
> >       /* Here we go */
> >       sum = EC_COMMAND_PROTOCOL_3;
> > -     cros_ec_lpc_write_bytes(EC_LPC_ADDR_HOST_CMD, 1, &sum);
> > +     cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
> >
> >       if (ec_response_timed_out()) {
> >               dev_warn(ec->dev, "EC responsed timed out\n");
> > @@ -71,15 +87,15 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
> >       }
> >
> >       /* Check result */
> > -     msg->result = cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> > +     msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> >       ret = cros_ec_check_result(ec, msg);
> >       if (ret)
> >               goto done;
> >
> >       /* Read back response */
> >       dout = (u8 *)&response;
> > -     sum = cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
> > -                                  dout);
> > +     sum = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
> > +                                dout);
> >
> >       msg->result = response.result;
> >
> > @@ -92,9 +108,9 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
> >       }
> >
> >       /* Read response and process checksum */
> > -     sum += cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_PACKET +
> > -                                   sizeof(response), response.data_len,
> > -                                   msg->data);
> > +     sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
> > +                                 sizeof(response), response.data_len,
> > +                                 msg->data);
> >
> >       if (sum) {
> >               dev_err(ec->dev,
> > @@ -134,17 +150,17 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
> >       sum = msg->command + args.flags + args.command_version + args.data_size;
> >
> >       /* Copy data and update checksum */
> > -     sum += cros_ec_lpc_write_bytes(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
> > -                                    msg->data);
> > +     sum += cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
> > +                                  msg->data);
> >
> >       /* Finalize checksum and write args */
> >       args.checksum = sum;
> > -     cros_ec_lpc_write_bytes(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
> > -                             (u8 *)&args);
> > +     cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
> > +                           (u8 *)&args);
> >
> >       /* Here we go */
> >       sum = msg->command;
> > -     cros_ec_lpc_write_bytes(EC_LPC_ADDR_HOST_CMD, 1, &sum);
> > +     cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
> >
> >       if (ec_response_timed_out()) {
> >               dev_warn(ec->dev, "EC responsed timed out\n");
> > @@ -153,14 +169,13 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
> >       }
> >
> >       /* Check result */
> > -     msg->result = cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> > +     msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> >       ret = cros_ec_check_result(ec, msg);
> >       if (ret)
> >               goto done;
> >
> >       /* Read back args */
> > -     cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
> > -                            (u8 *)&args);
> > +     cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
> >
> >       if (args.data_size > msg->insize) {
> >               dev_err(ec->dev,
> > @@ -174,8 +189,8 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
> >       sum = msg->command + args.flags + args.command_version + args.data_size;
> >
> >       /* Read response and update checksum */
> > -     sum += cros_ec_lpc_read_bytes(EC_LPC_ADDR_HOST_PARAM, args.data_size,
> > -                                   msg->data);
> > +     sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
> > +                                 msg->data);
> >
> >       /* Verify checksum */
> >       if (args.checksum != sum) {
> > @@ -205,13 +220,13 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
> >
> >       /* fixed length */
> >       if (bytes) {
> > -             cros_ec_lpc_read_bytes(EC_LPC_ADDR_MEMMAP + offset, bytes, s);
> > +             cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + offset, bytes, s);
> >               return bytes;
> >       }
> >
> >       /* string */
> >       for (; i < EC_MEMMAP_SIZE; i++, s++) {
> > -             cros_ec_lpc_read_bytes(EC_LPC_ADDR_MEMMAP + i, 1, s);
> > +             cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + i, 1, s);
> >               cnt++;
> >               if (!*s)
> >                       break;
> > @@ -248,10 +263,23 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> >               return -EBUSY;
> >       }
> >
> > -     cros_ec_lpc_read_bytes(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
> > +     /*
> > +      * Read the mapped ID twice, the first one is assuming the
> > +      * EC is a Microchip Embedded Controller (MEC) variant, if the
> > +      * protocol fails, fallback to the non MEC variant and try to
> > +      * read again the ID.
> > +      */
> > +     cros_ec_lpc_mec_read_bytes(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
> >       if (buf[0] != 'E' || buf[1] != 'C') {
> > -             dev_err(dev, "EC ID not detected\n");
> > -             return -ENODEV;
> > +             cros_ec_lpc_read_bytes(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2,
> > +                                    buf);
> > +             if (buf[0] != 'E' || buf[1] != 'C') {
> > +                     dev_err(dev, "EC ID not detected\n");
> > +                     return -ENODEV;
> > +             }
> > +             /* Re-assign read/write operations for the non MEC variant */
> > +             cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
> > +             cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
> >       }
> >
> >       if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> > diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c b/drivers/platform/chrome/cros_ec_lpc_reg.c
> > index 0f5cd0ac8b49..389d3329616f 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc_reg.c
> > +++ b/drivers/platform/chrome/cros_ec_lpc_reg.c
> > @@ -9,7 +9,7 @@
> >
> >  #include "cros_ec_lpc_mec.h"
> >
> > -static u8 lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> > +u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> >  {
> >       int i;
> >       int sum = 0;
> > @@ -23,7 +23,7 @@ static u8 lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> >       return sum;
> >  }
> >
> > -static u8 lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> > +u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> >  {
> >       int i;
> >       int sum = 0;
> > @@ -37,9 +37,8 @@ static u8 lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> >       return sum;
> >  }
> >
> > -#ifdef CONFIG_CROS_EC_LPC_MEC
> > -
> > -u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> > +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> > +                           u8 *dest)
> >  {
> >       int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > @@ -50,10 +49,12 @@ u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> >               cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
> >                                        offset - EC_HOST_CMD_REGION0,
> >                                        length, dest) :
> > -             lpc_read_bytes(offset, length, dest);
> > +             cros_ec_lpc_read_bytes(offset, length, dest);
> > +
> >  }
> >
> > -u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> > +u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> > +                            u8 *msg)
> >  {
> >       int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > @@ -64,7 +65,7 @@ u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> >               cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
> >                                        offset - EC_HOST_CMD_REGION0,
> >                                        length, msg) :
> > -             lpc_write_bytes(offset, length, msg);
> > +             cros_ec_lpc_write_bytes(offset, length, msg);
> >  }
> >
> >  void cros_ec_lpc_reg_init(void)
> > @@ -77,25 +78,3 @@ void cros_ec_lpc_reg_destroy(void)
> >  {
> >       cros_ec_lpc_mec_destroy();
> >  }
> > -
> > -#else /* CONFIG_CROS_EC_LPC_MEC */
> > -
> > -u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> > -{
> > -     return lpc_read_bytes(offset, length, dest);
> > -}
> > -
> > -u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> > -{
> > -     return lpc_write_bytes(offset, length, msg);
> > -}
> > -
> > -void cros_ec_lpc_reg_init(void)
> > -{
> > -}
> > -
> > -void cros_ec_lpc_reg_destroy(void)
> > -{
> > -}
> > -
> > -#endif /* CONFIG_CROS_EC_LPC_MEC */
> > diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.h b/drivers/platform/chrome/cros_ec_lpc_reg.h
> > index 416fd2572182..e8d53fb8a2bc 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc_reg.h
> > +++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
> > @@ -28,6 +28,32 @@ u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest);
> >   */
> >  u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg);
> >
> > +/**
> > + * cros_ec_lpc_mec_read_bytes() - Read bytes from a given LPC-mapped address.
> > + * @offset: Base read address
> > + * @length: Number of bytes to read
> > + * @dest: Destination buffer
> > + *
> > + * This function is for the MEC (Microchip Embedded Controller) variant.
> > + *
> > + * Return: 8-bit checksum of all bytes read.
> > + */
> > +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> > +                           u8 *dest);
> > +
> > +/**
> > + * cros_ec_lpc_mec_write_bytes - Write bytes to a given LPC-mapped address.
> > + * @offset: Base write address
> > + * @length: Number of bytes to write
> > + * @msg: Write data buffer
> > + *
> > + * This function is for the MEC (Microchip Embedded Controller) variant.
> > + *
> > + * Return: 8-bit checksum of all bytes written.
> > + */
> > +u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> > +                            u8 *msg);
> > +
> >  /**
> >   * cros_ec_lpc_reg_init
> >   *
> > diff --git a/drivers/platform/chrome/wilco_ec/Kconfig b/drivers/platform/chrome/wilco_ec/Kconfig
> > index fd29cbfd3d5d..c63ff2508409 100644
> > --- a/drivers/platform/chrome/wilco_ec/Kconfig
> > +++ b/drivers/platform/chrome/wilco_ec/Kconfig
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  config WILCO_EC
> >       tristate "ChromeOS Wilco Embedded Controller"
> > -     depends on ACPI && X86 && CROS_EC_LPC && CROS_EC_LPC_MEC
> > +     depends on ACPI && X86 && CROS_EC_LPC
> >       help
> >         If you say Y here, you get support for talking to the ChromeOS
> >         Wilco EC over an eSPI bus. This uses a simple byte-level protocol
> > --
> > 2.20.1
> >
> >
>
>