lkml.org 
[lkml]   [2019]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH v3] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime
Hi!

Thanks for the changes Enric! I had just a few more nits.

On Wed, Jun 12, 2019 at 2:30 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> 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@collabora.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Nick Crews <ncrews@chromium.org>
> ---
> Hi,
>
> This is another 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)
>
> 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 v3:
> - Kconfig: Split across multiple lines to keep it under 80 characters.
> - Improve kernel-doc as suggested by Nick Crews.
> - Convert msg in write function to const.
> - Add rb and tb tags.
>
> 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 | 2 +-
> drivers/platform/chrome/cros_ec_lpc.c | 77 ++++++++++++++++-------
> drivers/platform/chrome/cros_ec_lpc_reg.c | 38 +++--------
> drivers/platform/chrome/cros_ec_lpc_reg.h | 17 ++++-
> drivers/platform/chrome/wilco_ec/Kconfig | 2 +-
> 6 files changed, 89 insertions(+), 76 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..da9aa08d9fa6 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -10,7 +10,7 @@ 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_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..579eb4b3629a 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -28,6 +28,23 @@
> #define DRV_NAME "cros_ec_lpcs"
> #define ACPI_DRV_NAME "GOOG0004"
>
> +/**
> + * struct lpc_driver_ops - LPC driver operations
> + * @read: Copy length bytes from EC address offset into buffer dest. Returns
> + * the 8-bit checksum of all bytes read.
> + * @write: Copy length bytes from buffer msg into EC address offset. Returns
> + * the 8-bit checksum of all bytes written.
> + */
> +struct lpc_driver_ops {
> + u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
> + u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
> +};
> +
> +static struct lpc_driver_ops cros_ec_lpc_ops = {
> + .read = cros_ec_lpc_mec_read_bytes,
> + .write = cros_ec_lpc_mec_write_bytes,
> +};
> +
> /* True if ACPI device is present */
> static bool cros_ec_lpc_acpi_device_found;
>
> @@ -38,7 +55,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 +75,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 +88,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 +109,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 +151,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 +170,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 +190,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 +221,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 +264,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;

nit: This is really splitting hairs, but I think I would prefer

cros_ec_lpc_ops.read(...)
if (fails) {
cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
cros_ec_lpc_ops.read(...)
if (fails)
really_fail();
}

This makes it more explicit that on the first test we are using the default
methods, and it makes it more explicit that after this, things should
continue to work. Feel free to ignore though.

> }
>
> 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..0876afe79746 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"

See below, I think you should add
#include "cros_ec_lpc_reg.h"
here so you get compiler errors if your function declarations and
definitions don't match (since they currently don't match and it compiles).

>
> -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)

msg should be const here to match the declaration.

> {
> int i;
> int sum = 0;
> @@ -37,9 +37,9 @@ 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 +50,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)

msg should be const here to match the declaration.

> {
> int in_range = cros_ec_lpc_mec_in_range(offset, length);
>
> @@ -64,7 +66,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) :

You will need to cast msg as (u8 *) to avoid compiler errors here.

> - lpc_write_bytes(offset, length, msg);
> + cros_ec_lpc_write_bytes(offset, length, msg);
> }
>
> void cros_ec_lpc_reg_init(void)
> @@ -77,25 +79,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..2ff89baec292 100644
> --- a/drivers/platform/chrome/cros_ec_lpc_reg.h
> +++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
> @@ -26,7 +26,22 @@ u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest);
> * @length: Number of bytes to write
> * @msg: Write data buffer
> */

Change these docstrings to be references, like the ones below?
Maybe merge the two docstrings below into one, as is done above?

> -u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg);
> +u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
> + const u8 *msg);
> +
> +/*
> + * An instance of the read function of struct lpc_driver_ops, used for the
> + * MEC variant of LPC EC.
> + */
> +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> + u8 *dest);
> +
> +/*
> + * An instance of the write function of struct lpc_driver_ops, used for the
> + * MEC variant of LPC EC.
> + */
> +u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> + const 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
>

\
 
 \ /
  Last update: 2019-06-14 00:34    [W:0.046 / U:8.564 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site