* [PATCH v4] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime
@ 2019-06-14 4:34 Enric Balletbo i Serra
2019-06-14 16:23 ` Nick Crews
0 siblings, 1 reply; 3+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-14 4:34 UTC (permalink / raw)
To: linux-kernel
Cc: Collabora Kernel ML, groeck, bleung, dtor, ncrews,
Ezequiel Garcia, Arnd Bergmann, Thomas Gleixner, Duncan Laurie,
Greg Kroah-Hartman
On many boards, communication between the kernel and the Embedded
Controller happens over an LPC bus. In these cases, the kernel config
CONFIG_CROS_EC_LPC is enabled. Some of these LPC boards contain a
Microchip Embedded Controller (MEC) that is different from the regular
EC. On these devices, the same LPC bus is used, but the protocol is
a little different. In these cases, the CONFIG_CROS_EC_LPC_MEC kernel
config is enabled. Currently, the kernel decides at compile-time whether
or not to use the MEC variant, and, when that kernel option is selected
it breaks the other boards. We would like a kind of runtime detection to
avoid this.
This patch adds that detection mechanism by probing the protocol at
runtime, first we assume that a MEC variant is connected, and if the
protocol fails it fallbacks to the regular EC. This adds a bit of
overload because we try to read twice on those LPC boards that doesn't
contain a MEC variant, but is a better solution than having to select the
EC variant at compile-time.
While here also fix the alignment in Kconfig file for this config option
replacing the spaces by tabs.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Nick Crews <ncrews@chromium.org>
---
Hi,
This is another attempt to solve the issue to be able to select at
runtime the CrOS MEC variant. My first thought was check for a device
ID, the MEC1322 has a register that contains the device ID, however I
am not sure if we can read that register from the host without
modifying the firmware. Also, I am not sure if the MEC1322 is the only
device used that supports that LPC protocol variant, so I ended with a
more easy solution, check if the protocol fails or not. Some background
on this issue can be found [1] and [2]
The patch has been tested on:
- Acer Chromebook R11 (Cyan - MEC variant)
- Pixel Chromebook 2015 (Samus - non-MEC variant)
- Dell Chromebook 11 (Wolf - non-MEC variant)
- Toshiba Chromebook (Leon - non-MEC variant)
Best regards,
Enric
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=932626
[2] https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1474254
Changes in v4:
- Change the logic to test the protocols as suggested by Nick Crews.
- Add the proper cros_ec_lpc_mec.h include. (Nick Crews)
- Fix some const and missing casts. (Nick Crews)
- Clean up related doc-strings. (Nick Crews)
Changes in v3:
- Kconfig: Split across multiple lines to keep it under 80 characters.
- Improve kernel-doc as suggested by Nick Crews.
- Convert msg in write function to const.
- Add rb and tb tags.
Changes in v2:
- Remove global bool to indicate the kind of variant as suggested by Ezequiel.
- Create an internal operations struct to allow different variants.
drivers/platform/chrome/Kconfig | 29 +++------
drivers/platform/chrome/Makefile | 2 +-
drivers/platform/chrome/cros_ec_lpc.c | 78 ++++++++++++++++-------
drivers/platform/chrome/cros_ec_lpc_mec.h | 14 ++++
drivers/platform/chrome/cros_ec_lpc_reg.c | 41 ++++--------
drivers/platform/chrome/cros_ec_lpc_reg.h | 23 +++----
drivers/platform/chrome/wilco_ec/Kconfig | 2 +-
7 files changed, 98 insertions(+), 91 deletions(-)
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 2826f7136f65..453e69733842 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -83,28 +83,17 @@ config CROS_EC_SPI
'pre-amble' bytes before the response actually starts.
config CROS_EC_LPC
- tristate "ChromeOS Embedded Controller (LPC)"
- depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
- help
- If you say Y here, you get support for talking to the ChromeOS EC
- over an LPC bus. This uses a simple byte-level protocol with a
- checksum. This is used for userspace access only. The kernel
- typically has its own communication methods.
-
- To compile this driver as a module, choose M here: the
- module will be called cros_ec_lpc.
-
-config CROS_EC_LPC_MEC
- bool "ChromeOS Embedded Controller LPC Microchip EC (MEC) variant"
- depends on CROS_EC_LPC
- default n
+ tristate "ChromeOS Embedded Controller (LPC)"
+ depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
help
- If you say Y here, a variant LPC protocol for the Microchip EC
- will be used. Note that this variant is not backward compatible
- with non-Microchip ECs.
+ If you say Y here, you get support for talking to the ChromeOS EC
+ over an LPC bus, including the LPC Microchip EC (MEC) variant.
+ This uses a simple byte-level protocol with a checksum. This is
+ used for userspace access only. The kernel typically has its own
+ communication methods.
- If you have a ChromeOS Embedded Controller Microchip EC variant
- choose Y here.
+ To compile this driver as a module, choose M here: the
+ module will be called cros_ec_lpcs.
config CROS_EC_PROTO
bool
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 1b2f1dcfcd5c..da9aa08d9fa6 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_reg.o
-cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
+cros_ec_lpcs-objs += cros_ec_lpc_mec.o
obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index c9c240fbe7c6..d6ba6b464abc 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -23,11 +23,29 @@
#include <linux/printk.h>
#include <linux/suspend.h>
+#include "cros_ec_lpc_mec.h"
#include "cros_ec_lpc_reg.h"
#define DRV_NAME "cros_ec_lpcs"
#define ACPI_DRV_NAME "GOOG0004"
+/**
+ * struct lpc_driver_ops - LPC driver operations
+ * @read: Copy length bytes from EC address offset into buffer dest. Returns
+ * the 8-bit checksum of all bytes read.
+ * @write: Copy length bytes from buffer msg into EC address offset. Returns
+ * the 8-bit checksum of all bytes written.
+ */
+struct lpc_driver_ops {
+ u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
+ u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
+};
+
+static struct lpc_driver_ops cros_ec_lpc_ops = {
+ .read = cros_ec_lpc_mec_read_bytes,
+ .write = cros_ec_lpc_mec_write_bytes,
+};
+
/* True if ACPI device is present */
static bool cros_ec_lpc_acpi_device_found;
@@ -38,7 +56,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 +76,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 +89,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 +110,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 +152,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 +171,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 +191,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 +222,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 +265,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_ops.read(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;
+ cros_ec_lpc_ops.read(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;
+ }
}
if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
index aa1018f6b0f2..fcaabdbb5e62 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.h
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
@@ -76,4 +76,18 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length);
u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
unsigned int offset, unsigned int length, u8 *buf);
+/*
+ * An instance of the read function of struct lpc_driver_ops, used for the
+ * MEC variant of LPC EC.
+ */
+u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
+ u8 *dest);
+
+/*
+ * An instance of the write function of struct lpc_driver_ops, used for the
+ * MEC variant of LPC EC.
+ */
+u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
+ const u8 *msg);
+
#endif /* __CROS_EC_LPC_MEC_H */
diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c b/drivers/platform/chrome/cros_ec_lpc_reg.c
index 0f5cd0ac8b49..3bfeb5ad59ba 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,8 @@ 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,
+ const u8 *msg)
{
int i;
int sum = 0;
@@ -37,9 +38,9 @@ static u8 lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
return sum;
}
-#ifdef CONFIG_CROS_EC_LPC_MEC
-u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
+u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
+ u8 *dest)
{
int in_range = cros_ec_lpc_mec_in_range(offset, length);
@@ -50,10 +51,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,
+ const u8 *msg)
{
int in_range = cros_ec_lpc_mec_in_range(offset, length);
@@ -63,8 +66,8 @@ u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
return in_range ?
cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
offset - EC_HOST_CMD_REGION0,
- length, msg) :
- lpc_write_bytes(offset, length, msg);
+ length, (u8 *)msg) :
+ cros_ec_lpc_write_bytes(offset, length, msg);
}
void cros_ec_lpc_reg_init(void)
@@ -77,25 +80,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..f12115293e47 100644
--- a/drivers/platform/chrome/cros_ec_lpc_reg.h
+++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
@@ -8,25 +8,18 @@
#ifndef __CROS_EC_LPC_REG_H
#define __CROS_EC_LPC_REG_H
-/**
- * cros_ec_lpc_read_bytes - Read bytes from a given LPC-mapped address.
- * Returns 8-bit checksum of all bytes read.
- *
- * @offset: Base read address
- * @length: Number of bytes to read
- * @dest: Destination buffer
+/*
+ * A generic instance of the read function of struct lpc_driver_ops, used for
+ * the LPC EC.
*/
u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest);
-/**
- * cros_ec_lpc_write_bytes - Write bytes to a given LPC-mapped address.
- * Returns 8-bit checksum of all bytes written.
- *
- * @offset: Base write address
- * @length: Number of bytes to write
- * @msg: Write data buffer
+/*
+ * A generic instance of the write function of struct lpc_driver_ops, used for
+ * the LPC EC.
*/
-u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg);
+u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
+ const u8 *msg);
/**
* 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime
2019-06-14 4:34 [PATCH v4] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime Enric Balletbo i Serra
@ 2019-06-14 16:23 ` Nick Crews
2019-06-14 17:01 ` Enric Balletbo Serra
0 siblings, 1 reply; 3+ messages in thread
From: Nick Crews @ 2019-06-14 16:23 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Collabora Kernel ML, Guenter Roeck, Benson Leung,
Dmitry Torokhov, Ezequiel Garcia, Arnd Bergmann, Thomas Gleixner,
Duncan Laurie, Greg Kroah-Hartman
On Thu, Jun 13, 2019 at 10:34 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> On many boards, communication between the kernel and the Embedded
> Controller happens over an LPC bus. In these cases, the kernel config
> CONFIG_CROS_EC_LPC is enabled. Some of these LPC boards contain a
> Microchip Embedded Controller (MEC) that is different from the regular
> EC. On these devices, the same LPC bus is used, but the protocol is
> a little different. In these cases, the CONFIG_CROS_EC_LPC_MEC kernel
> config is enabled. Currently, the kernel decides at compile-time whether
> or not to use the MEC variant, and, when that kernel option is selected
> it breaks the other boards. We would like a kind of runtime detection to
> avoid this.
>
> This patch adds that detection mechanism by probing the protocol at
> runtime, first we assume that a MEC variant is connected, and if the
> protocol fails it fallbacks to the regular EC. This adds a bit of
> overload because we try to read twice on those LPC boards that doesn't
> contain a MEC variant, but is a better solution than having to select the
> EC variant at compile-time.
>
> While here also fix the alignment in Kconfig file for this config option
> replacing the spaces by tabs.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Nick Crews <ncrews@chromium.org>
> ---
> Hi,
>
> This is another attempt to solve the issue to be able to select at
> runtime the CrOS MEC variant. My first thought was check for a device
> ID, the MEC1322 has a register that contains the device ID, however I
> am not sure if we can read that register from the host without
> modifying the firmware. Also, I am not sure if the MEC1322 is the only
> device used that supports that LPC protocol variant, so I ended with a
> more easy solution, check if the protocol fails or not. Some background
> on this issue can be found [1] and [2]
>
> The patch has been tested on:
> - Acer Chromebook R11 (Cyan - MEC variant)
> - Pixel Chromebook 2015 (Samus - non-MEC variant)
> - Dell Chromebook 11 (Wolf - non-MEC variant)
> - Toshiba Chromebook (Leon - non-MEC variant)
>
> Best regards,
> Enric
>
> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=932626
> [2] https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1474254
>
> Changes in v4:
> - Change the logic to test the protocols as suggested by Nick Crews.
> - Add the proper cros_ec_lpc_mec.h include. (Nick Crews)
> - Fix some const and missing casts. (Nick Crews)
> - Clean up related doc-strings. (Nick Crews)
>
> Changes in v3:
> - Kconfig: Split across multiple lines to keep it under 80 characters.
> - Improve kernel-doc as suggested by Nick Crews.
> - Convert msg in write function to const.
> - Add rb and tb tags.
>
> Changes in v2:
> - Remove global bool to indicate the kind of variant as suggested by Ezequiel.
> - Create an internal operations struct to allow different variants.
>
> drivers/platform/chrome/Kconfig | 29 +++------
> drivers/platform/chrome/Makefile | 2 +-
> drivers/platform/chrome/cros_ec_lpc.c | 78 ++++++++++++++++-------
> drivers/platform/chrome/cros_ec_lpc_mec.h | 14 ++++
> drivers/platform/chrome/cros_ec_lpc_reg.c | 41 ++++--------
> drivers/platform/chrome/cros_ec_lpc_reg.h | 23 +++----
> drivers/platform/chrome/wilco_ec/Kconfig | 2 +-
> 7 files changed, 98 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 2826f7136f65..453e69733842 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -83,28 +83,17 @@ config CROS_EC_SPI
> 'pre-amble' bytes before the response actually starts.
>
> config CROS_EC_LPC
> - tristate "ChromeOS Embedded Controller (LPC)"
> - depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> - help
> - If you say Y here, you get support for talking to the ChromeOS EC
> - over an LPC bus. This uses a simple byte-level protocol with a
> - checksum. This is used for userspace access only. The kernel
> - typically has its own communication methods.
> -
> - To compile this driver as a module, choose M here: the
> - module will be called cros_ec_lpc.
> -
> -config CROS_EC_LPC_MEC
> - bool "ChromeOS Embedded Controller LPC Microchip EC (MEC) variant"
> - depends on CROS_EC_LPC
> - default n
> + tristate "ChromeOS Embedded Controller (LPC)"
> + depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> help
> - If you say Y here, a variant LPC protocol for the Microchip EC
> - will be used. Note that this variant is not backward compatible
> - with non-Microchip ECs.
> + If you say Y here, you get support for talking to the ChromeOS EC
> + over an LPC bus, including the LPC Microchip EC (MEC) variant.
> + This uses a simple byte-level protocol with a checksum. This is
> + used for userspace access only. The kernel typically has its own
> + communication methods.
>
> - If you have a ChromeOS Embedded Controller Microchip EC variant
> - choose Y here.
> + To compile this driver as a module, choose M here: the
> + module will be called cros_ec_lpcs.
>
> config CROS_EC_PROTO
> bool
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 1b2f1dcfcd5c..da9aa08d9fa6 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -10,7 +10,7 @@ obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_reg.o
> -cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
> +cros_ec_lpcs-objs += cros_ec_lpc_mec.o
> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index c9c240fbe7c6..d6ba6b464abc 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -23,11 +23,29 @@
> #include <linux/printk.h>
> #include <linux/suspend.h>
>
> +#include "cros_ec_lpc_mec.h"
Good catch, but this isn't everything. See below.
> #include "cros_ec_lpc_reg.h"
>
> #define DRV_NAME "cros_ec_lpcs"
> #define ACPI_DRV_NAME "GOOG0004"
>
> +/**
> + * struct lpc_driver_ops - LPC driver operations
> + * @read: Copy length bytes from EC address offset into buffer dest. Returns
> + * the 8-bit checksum of all bytes read.
> + * @write: Copy length bytes from buffer msg into EC address offset. Returns
> + * the 8-bit checksum of all bytes written.
> + */
> +struct lpc_driver_ops {
> + u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
> + u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
> +};
> +
> +static struct lpc_driver_ops cros_ec_lpc_ops = {
> + .read = cros_ec_lpc_mec_read_bytes,
> + .write = cros_ec_lpc_mec_write_bytes,
> +};
As I mentioned to you directly, maybe don't define these statically here,
but assign the methods in cros_ec_lpc_probe(), so down there
you can see explicitly what the defaults are?
> +
> /* True if ACPI device is present */
> static bool cros_ec_lpc_acpi_device_found;
>
> @@ -38,7 +56,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 +76,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 +89,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 +110,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 +152,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 +171,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 +191,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 +222,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 +265,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.
> + */
As metioned above, maybe assign cros_ec_lpc_mec_read_bytes and
cros_ec_lpc_mec_write_bytes here, so it's clear what the defaults are,
instead of assigning them statically above.
> + cros_ec_lpc_ops.read(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;
> + cros_ec_lpc_ops.read(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;
> + }
> }
>
> if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
> index aa1018f6b0f2..fcaabdbb5e62 100644
> --- a/drivers/platform/chrome/cros_ec_lpc_mec.h
> +++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
> @@ -76,4 +76,18 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length);
> u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
> unsigned int offset, unsigned int length, u8 *buf);
>
> +/*
> + * An instance of the read function of struct lpc_driver_ops, used for the
> + * MEC variant of LPC EC.
> + */
> +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> + u8 *dest);
> +
> +/*
> + * An instance of the write function of struct lpc_driver_ops, used for the
> + * MEC variant of LPC EC.
> + */
> +u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> + const u8 *msg);
> +
> #endif /* __CROS_EC_LPC_MEC_H */
> diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c b/drivers/platform/chrome/cros_ec_lpc_reg.c
> index 0f5cd0ac8b49..3bfeb5ad59ba 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"
Good call above, but you should include cros_ec_lpc_reg.h here as well.
>
> -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,8 @@ 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,
> + const u8 *msg)
> {
> int i;
> int sum = 0;
> @@ -37,9 +38,9 @@ static u8 lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> return sum;
> }
>
> -#ifdef CONFIG_CROS_EC_LPC_MEC
>
> -u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> + u8 *dest)
> {
> int in_range = cros_ec_lpc_mec_in_range(offset, length);
>
> @@ -50,10 +51,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,
> + const u8 *msg)
> {
> int in_range = cros_ec_lpc_mec_in_range(offset, length);
>
> @@ -63,8 +66,8 @@ u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> return in_range ?
> cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
> offset - EC_HOST_CMD_REGION0,
> - length, msg) :
> - lpc_write_bytes(offset, length, msg);
> + length, (u8 *)msg) :
> + cros_ec_lpc_write_bytes(offset, length, msg);
> }
>
> void cros_ec_lpc_reg_init(void)
> @@ -77,25 +80,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..f12115293e47 100644
> --- a/drivers/platform/chrome/cros_ec_lpc_reg.h
> +++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
> @@ -8,25 +8,18 @@
> #ifndef __CROS_EC_LPC_REG_H
> #define __CROS_EC_LPC_REG_H
>
> -/**
> - * cros_ec_lpc_read_bytes - Read bytes from a given LPC-mapped address.
> - * Returns 8-bit checksum of all bytes read.
> - *
> - * @offset: Base read address
> - * @length: Number of bytes to read
> - * @dest: Destination buffer
> +/*
> + * A generic instance of the read function of struct lpc_driver_ops, used for
> + * the LPC EC.
> */
> u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest);
>
> -/**
> - * cros_ec_lpc_write_bytes - Write bytes to a given LPC-mapped address.
> - * Returns 8-bit checksum of all bytes written.
> - *
> - * @offset: Base write address
> - * @length: Number of bytes to write
> - * @msg: Write data buffer
> +/*
> + * A generic instance of the write function of struct lpc_driver_ops, used for
> + * the LPC EC.
> */
> -u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg);
> +u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
> + const u8 *msg);
>
> /**
> * 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime
2019-06-14 16:23 ` Nick Crews
@ 2019-06-14 17:01 ` Enric Balletbo Serra
0 siblings, 0 replies; 3+ messages in thread
From: Enric Balletbo Serra @ 2019-06-14 17:01 UTC (permalink / raw)
To: Nick Crews
Cc: Enric Balletbo i Serra, linux-kernel, Collabora Kernel ML,
Guenter Roeck, Benson Leung, Dmitry Torokhov, Ezequiel Garcia,
Arnd Bergmann, Thomas Gleixner, Duncan Laurie,
Greg Kroah-Hartman
Hi Nick,
Missatge de Nick Crews <ncrews@chromium.org> del dia dv., 14 de juny
2019 a les 18:25:
>
> On Thu, Jun 13, 2019 at 10:34 PM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > On many boards, communication between the kernel and the Embedded
> > Controller happens over an LPC bus. In these cases, the kernel config
> > CONFIG_CROS_EC_LPC is enabled. Some of these LPC boards contain a
> > Microchip Embedded Controller (MEC) that is different from the regular
> > EC. On these devices, the same LPC bus is used, but the protocol is
> > a little different. In these cases, the CONFIG_CROS_EC_LPC_MEC kernel
> > config is enabled. Currently, the kernel decides at compile-time whether
> > or not to use the MEC variant, and, when that kernel option is selected
> > it breaks the other boards. We would like a kind of runtime detection to
> > avoid this.
> >
> > This patch adds that detection mechanism by probing the protocol at
> > runtime, first we assume that a MEC variant is connected, and if the
> > protocol fails it fallbacks to the regular EC. This adds a bit of
> > overload because we try to read twice on those LPC boards that doesn't
> > contain a MEC variant, but is a better solution than having to select the
> > EC variant at compile-time.
> >
> > While here also fix the alignment in Kconfig file for this config option
> > replacing the spaces by tabs.
> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Tested-by: Nick Crews <ncrews@chromium.org>
> > ---
> > Hi,
> >
> > This is another attempt to solve the issue to be able to select at
> > runtime the CrOS MEC variant. My first thought was check for a device
> > ID, the MEC1322 has a register that contains the device ID, however I
> > am not sure if we can read that register from the host without
> > modifying the firmware. Also, I am not sure if the MEC1322 is the only
> > device used that supports that LPC protocol variant, so I ended with a
> > more easy solution, check if the protocol fails or not. Some background
> > on this issue can be found [1] and [2]
> >
> > The patch has been tested on:
> > - Acer Chromebook R11 (Cyan - MEC variant)
> > - Pixel Chromebook 2015 (Samus - non-MEC variant)
> > - Dell Chromebook 11 (Wolf - non-MEC variant)
> > - Toshiba Chromebook (Leon - non-MEC variant)
> >
> > Best regards,
> > Enric
> >
> > [1] https://bugs.chromium.org/p/chromium/issues/detail?id=932626
> > [2] https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1474254
> >
> > Changes in v4:
> > - Change the logic to test the protocols as suggested by Nick Crews.
> > - Add the proper cros_ec_lpc_mec.h include. (Nick Crews)
> > - Fix some const and missing casts. (Nick Crews)
> > - Clean up related doc-strings. (Nick Crews)
> >
> > Changes in v3:
> > - Kconfig: Split across multiple lines to keep it under 80 characters.
> > - Improve kernel-doc as suggested by Nick Crews.
> > - Convert msg in write function to const.
> > - Add rb and tb tags.
> >
> > Changes in v2:
> > - Remove global bool to indicate the kind of variant as suggested by Ezequiel.
> > - Create an internal operations struct to allow different variants.
> >
> > drivers/platform/chrome/Kconfig | 29 +++------
> > drivers/platform/chrome/Makefile | 2 +-
> > drivers/platform/chrome/cros_ec_lpc.c | 78 ++++++++++++++++-------
> > drivers/platform/chrome/cros_ec_lpc_mec.h | 14 ++++
> > drivers/platform/chrome/cros_ec_lpc_reg.c | 41 ++++--------
> > drivers/platform/chrome/cros_ec_lpc_reg.h | 23 +++----
> > drivers/platform/chrome/wilco_ec/Kconfig | 2 +-
> > 7 files changed, 98 insertions(+), 91 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > index 2826f7136f65..453e69733842 100644
> > --- a/drivers/platform/chrome/Kconfig
> > +++ b/drivers/platform/chrome/Kconfig
> > @@ -83,28 +83,17 @@ config CROS_EC_SPI
> > 'pre-amble' bytes before the response actually starts.
> >
> > config CROS_EC_LPC
> > - tristate "ChromeOS Embedded Controller (LPC)"
> > - depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> > - help
> > - If you say Y here, you get support for talking to the ChromeOS EC
> > - over an LPC bus. This uses a simple byte-level protocol with a
> > - checksum. This is used for userspace access only. The kernel
> > - typically has its own communication methods.
> > -
> > - To compile this driver as a module, choose M here: the
> > - module will be called cros_ec_lpc.
> > -
> > -config CROS_EC_LPC_MEC
> > - bool "ChromeOS Embedded Controller LPC Microchip EC (MEC) variant"
> > - depends on CROS_EC_LPC
> > - default n
> > + tristate "ChromeOS Embedded Controller (LPC)"
> > + depends on MFD_CROS_EC && ACPI && (X86 || COMPILE_TEST)
> > help
> > - If you say Y here, a variant LPC protocol for the Microchip EC
> > - will be used. Note that this variant is not backward compatible
> > - with non-Microchip ECs.
> > + If you say Y here, you get support for talking to the ChromeOS EC
> > + over an LPC bus, including the LPC Microchip EC (MEC) variant.
> > + This uses a simple byte-level protocol with a checksum. This is
> > + used for userspace access only. The kernel typically has its own
> > + communication methods.
> >
> > - If you have a ChromeOS Embedded Controller Microchip EC variant
> > - choose Y here.
> > + To compile this driver as a module, choose M here: the
> > + module will be called cros_ec_lpcs.
> >
> > config CROS_EC_PROTO
> > bool
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index 1b2f1dcfcd5c..da9aa08d9fa6 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile
> > @@ -10,7 +10,7 @@ obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
> > obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> > obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> > cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_reg.o
> > -cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
> > +cros_ec_lpcs-objs += cros_ec_lpc_mec.o
> > obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> > obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> > obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> > index c9c240fbe7c6..d6ba6b464abc 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc.c
> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
> > @@ -23,11 +23,29 @@
> > #include <linux/printk.h>
> > #include <linux/suspend.h>
> >
> > +#include "cros_ec_lpc_mec.h"
>
> Good catch, but this isn't everything. See below.
>
> > #include "cros_ec_lpc_reg.h"
> >
> > #define DRV_NAME "cros_ec_lpcs"
> > #define ACPI_DRV_NAME "GOOG0004"
> >
> > +/**
> > + * struct lpc_driver_ops - LPC driver operations
> > + * @read: Copy length bytes from EC address offset into buffer dest. Returns
> > + * the 8-bit checksum of all bytes read.
> > + * @write: Copy length bytes from buffer msg into EC address offset. Returns
> > + * the 8-bit checksum of all bytes written.
> > + */
> > +struct lpc_driver_ops {
> > + u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
> > + u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
> > +};
> > +
> > +static struct lpc_driver_ops cros_ec_lpc_ops = {
> > + .read = cros_ec_lpc_mec_read_bytes,
> > + .write = cros_ec_lpc_mec_write_bytes,
> > +};
>
> As I mentioned to you directly, maybe don't define these statically here,
> but assign the methods in cros_ec_lpc_probe(), so down there
> you can see explicitly what the defaults are?
>
> > +
> > /* True if ACPI device is present */
> > static bool cros_ec_lpc_acpi_device_found;
> >
> > @@ -38,7 +56,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 +76,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 +89,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 +110,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 +152,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 +171,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 +191,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 +222,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 +265,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.
> > + */
>
> As metioned above, maybe assign cros_ec_lpc_mec_read_bytes and
> cros_ec_lpc_mec_write_bytes here, so it's clear what the defaults are,
> instead of assigning them statically above.
>
> > + cros_ec_lpc_ops.read(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;
> > + cros_ec_lpc_ops.read(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;
> > + }
> > }
> >
> > if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
> > diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
> > index aa1018f6b0f2..fcaabdbb5e62 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc_mec.h
> > +++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
> > @@ -76,4 +76,18 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length);
> > u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
> > unsigned int offset, unsigned int length, u8 *buf);
> >
> > +/*
> > + * An instance of the read function of struct lpc_driver_ops, used for the
> > + * MEC variant of LPC EC.
> > + */
> > +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> > + u8 *dest);
> > +
> > +/*
> > + * An instance of the write function of struct lpc_driver_ops, used for the
> > + * MEC variant of LPC EC.
> > + */
> > +u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> > + const u8 *msg);
> > +
> > #endif /* __CROS_EC_LPC_MEC_H */
> > diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c b/drivers/platform/chrome/cros_ec_lpc_reg.c
> > index 0f5cd0ac8b49..3bfeb5ad59ba 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"
>
> Good call above, but you should include cros_ec_lpc_reg.h here as well.
>
I think I'll change my strategy, because this patch will end fixing
more the mess with the includes than add support for runtime
detection. I'll send another patch series merging the cros_ec_lpc_*
files first, and second one on top of it adding runtime detection.
Thanks for the review,
~ Enric
> >
> > -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,8 @@ 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,
> > + const u8 *msg)
> > {
> > int i;
> > int sum = 0;
> > @@ -37,9 +38,9 @@ static u8 lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> > return sum;
> > }
> >
> > -#ifdef CONFIG_CROS_EC_LPC_MEC
> >
> > -u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
> > +u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> > + u8 *dest)
> > {
> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > @@ -50,10 +51,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,
> > + const u8 *msg)
> > {
> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > @@ -63,8 +66,8 @@ u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
> > return in_range ?
> > cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
> > offset - EC_HOST_CMD_REGION0,
> > - length, msg) :
> > - lpc_write_bytes(offset, length, msg);
> > + length, (u8 *)msg) :
> > + cros_ec_lpc_write_bytes(offset, length, msg);
> > }
> >
> > void cros_ec_lpc_reg_init(void)
> > @@ -77,25 +80,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..f12115293e47 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc_reg.h
> > +++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
> > @@ -8,25 +8,18 @@
> > #ifndef __CROS_EC_LPC_REG_H
> > #define __CROS_EC_LPC_REG_H
> >
> > -/**
> > - * cros_ec_lpc_read_bytes - Read bytes from a given LPC-mapped address.
> > - * Returns 8-bit checksum of all bytes read.
> > - *
> > - * @offset: Base read address
> > - * @length: Number of bytes to read
> > - * @dest: Destination buffer
> > +/*
> > + * A generic instance of the read function of struct lpc_driver_ops, used for
> > + * the LPC EC.
> > */
> > u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest);
> >
> > -/**
> > - * cros_ec_lpc_write_bytes - Write bytes to a given LPC-mapped address.
> > - * Returns 8-bit checksum of all bytes written.
> > - *
> > - * @offset: Base write address
> > - * @length: Number of bytes to write
> > - * @msg: Write data buffer
> > +/*
> > + * A generic instance of the write function of struct lpc_driver_ops, used for
> > + * the LPC EC.
> > */
> > -u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg);
> > +u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
> > + const u8 *msg);
> >
> > /**
> > * 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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-14 17:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 4:34 [PATCH v4] platform/chrome: cros_ec_lpc: Choose Microchip EC at runtime Enric Balletbo i Serra
2019-06-14 16:23 ` Nick Crews
2019-06-14 17:01 ` Enric Balletbo Serra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).