2019-06-07 10:34:25

by Enric Balletbo i Serra

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

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 <[email protected]>
---
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,
+};
+
/* 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

2019-06-07 18:59:57

by Ezequiel Garcia

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

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 <[email protected]>
> ---
> 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?).

> /* 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
>
>

2019-06-07 20:06:54

by Nick Crews

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

Hi!

On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
> >
>
>

2019-06-07 20:12:18

by Guenter Roeck

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

On Fri, Jun 7, 2019 at 12:27 PM Nick Crews <[email protected]> wrote:
>
> Hi!
>
> On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
>

I understand that the current implementation may be insufficient if
there is ever more than one EC in a given system. Maybe I am missing
something, but why even consider it right now, with no such system in
existence ? We would not even know if a more flexible implementation
actually works, since there would be no means to test it.

Thanks,
Guenter

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

2019-06-07 20:52:34

by Enric Balletbo Serra

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

Hi,

Missatge de Guenter Roeck <[email protected]> del dia dv., 7 de juny
2019 a les 22:11:
>
> On Fri, Jun 7, 2019 at 12:27 PM Nick Crews <[email protected]> wrote:
> >
> > Hi!
> >
> > On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
> > >

Indeed

> > > I guess it's not an issue given you typically (always?)
> > > have one cros-ec device per platform.
> > >

I don't think is expected, at least now, we don't have a real use case with it.

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

My two cents to the discussion and the reason why I did not implement
the per-device functionality:
- Before this patch, we didn't have support per-device.
- Make it per-device is not the purpose of this patch, the purpose is
detect at run-time the protocol used.
- Add support per-device needs more changes than the expected, which
IMO are out of the scope of this patch.
- We don't have a real use case and unlikely we will have in the
future, so why worry now.

> > 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.
> >
>
> I understand that the current implementation may be insufficient if
> there is ever more than one EC in a given system. Maybe I am missing
> something, but why even consider it right now, with no such system in
> existence ? We would not even know if a more flexible implementation
> actually works, since there would be no means to test it.

Agree.

Thanks,
Enric

>
> Thanks,
> Guenter
>
> > 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
> > > >
> > > >
> > >
> > >

2019-06-07 21:35:05

by Nick Crews

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

Actually adding Stefan Reinauer this time...

On Fri, Jun 7, 2019 at 1:26 PM Nick Crews <[email protected]> wrote:
>
> Hi!
>
> On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
> > >
> >
> >

2019-06-08 00:53:54

by Ezequiel Garcia

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

Hi Enric,

On Fri, 2019-06-07 at 22:51 +0200, Enric Balletbo Serra wrote:
> Hi,
>
> Missatge de Guenter Roeck <[email protected]> del dia dv., 7 de juny
> 2019 a les 22:11:
> > On Fri, Jun 7, 2019 at 12:27 PM Nick Crews <[email protected]> wrote:
> > > Hi!
> > >
> > > On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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.
> > > >
>
> Indeed
>
> > > > I guess it's not an issue given you typically (always?)
> > > > have one cros-ec device per platform.
> > > >
>
> I don't think is expected, at least now, we don't have a real use case with it.
>
> > > > 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.
> > >
>
> My two cents to the discussion and the reason why I did not implement
> the per-device functionality:
> - Before this patch, we didn't have support per-device.
> - Make it per-device is not the purpose of this patch, the purpose is
> detect at run-time the protocol used.
> - Add support per-device needs more changes than the expected, which
> IMO are out of the scope of this patch.
> - We don't have a real use case and unlikely we will have in the
> future, so why worry now.
>

I see.

> > > 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.
> > >
> >
> > I understand that the current implementation may be insufficient if
> > there is ever more than one EC in a given system. Maybe I am missing
> > something, but why even consider it right now, with no such system in
> > existence ? We would not even know if a more flexible implementation
> > actually works, since there would be no means to test it.
>
> Agree.
>

Thanks for the explanations, it's clear now!

The change looks good.

Reviewed-by: Ezequiel Garcia <[email protected]>

Thanks,
Eze

2019-06-10 17:45:47

by Nick Crews

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

A few nits below, and otherwise looks good to me!

On Fri, Jun 7, 2019 at 2:51 PM Enric Balletbo Serra <[email protected]> wrote:
>
> Hi,
>
> Missatge de Guenter Roeck <[email protected]> del dia dv., 7 de juny
> 2019 a les 22:11:
> >
> > On Fri, Jun 7, 2019 at 12:27 PM Nick Crews <[email protected]> wrote:
> > >
> > > Hi!
> > >
> > > On Fri, Jun 7, 2019 at 12:03 PM Ezequiel Garcia <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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?

It works, which makes sense, as Wilco hardcodes in using
the MEC variant.

Tested-by: Nick Crews <[email protected]>

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

To be consistent with the other parts of this file, this should
be split across multiple lines to keep it under 80 characters.
Also, this hunk did not apply cleanly to the chrome-platform/for-next branch.

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

It would be great if you documented the API of these two functions better,
so someone doesn't have to chase down the API of the instances. Then
I would remove (or better, simplify and make them point here) the docstrings
of the instance functions so we don't have duplicates of docstrings.

> > > > > + */
> > > > > +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.
> > > >
>
> Indeed
>
> > > > I guess it's not an issue given you typically (always?)
> > > > have one cros-ec device per platform.
> > > >
>
> I don't think is expected, at least now, we don't have a real use case with it.
>
> > > > 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.
> > >
>
> My two cents to the discussion and the reason why I did not implement
> the per-device functionality:
> - Before this patch, we didn't have support per-device.
> - Make it per-device is not the purpose of this patch, the purpose is
> detect at run-time the protocol used.
> - Add support per-device needs more changes than the expected, which
> IMO are out of the scope of this patch.
> - We don't have a real use case and unlikely we will have in the
> future, so why worry now.

Ack.

>
> > > 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.
> > >
> >
> > I understand that the current implementation may be insufficient if
> > there is ever more than one EC in a given system. Maybe I am missing
> > something, but why even consider it right now, with no such system in
> > existence ? We would not even know if a more flexible implementation
> > actually works, since there would be no means to test it.
>
> Agree.
>
> Thanks,
> Enric

Ack.

I was basing this "allow for multiple devices" paradigm from a past
suggestion from Dmitry. I applied this for the Wilco EC Event [1] and
Wilco EC Telemetry [2] drivers per Dmitry's suggestion, so in the future
we could use multiple EC devices. I agree that at this point it seems
a bit silly to plan that far ahead, but I was just trying to be consistent.
In future work that I do, I will now default to accommodating only one EC.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/commit/?h=for-next&id=f7b0bc5eafa44941ddf92df6f46dd82cbfacec3e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/commit/?h=for-next&id=1210d1e6bad1e7ccccb19627b880a50d7c15dd51

>
> >
> > Thanks,
> > Guenter
> >
> > > 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);

optional nit: msg could be const

> > > > >
> > > > > +/**
> > > > > + * 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);

optional nit: msg could be const

> > > > > +
> > > > > /**
> > > > > * 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

This SPDX line isn't in the chrome-platform/for-next branch so this
doesn't apply cleanly. I think adding the SPDX would be good though,
just make that part of this patch?

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

After these tweaks, looks good!

Reviewed-by: Nick Crews <[email protected]>