Date:   Thu, 30 May 2019 19:11:20 +0200
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     linux-kernel@...r.kernel.org
Cc:     Nick Crews <ncrews@...omium.org>,
        Guenter Roeck <groeck@...omium.org>, kernel@...labora.com,
        Duncan Laurie <dlaurie@...gle.com>,
        Benson Leung <bleung@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: [PATCH] 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 <enric.balletbo@...labora.com>
---
Hi,

This is the first 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

 drivers/platform/chrome/Kconfig           | 29 +++++-----------
 drivers/platform/chrome/Makefile          |  3 +-
 drivers/platform/chrome/cros_ec_lpc.c     | 15 ++++++--
 drivers/platform/chrome/cros_ec_lpc_reg.c | 42 +++++++++--------------
 drivers/platform/chrome/cros_ec_lpc_reg.h |  3 ++
 drivers/platform/chrome/wilco_ec/Kconfig  |  2 +-
 6 files changed, 43 insertions(+), 51 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..2cbc71c8edba 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -248,10 +248,21 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
+	/*
+	 * 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_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_is_microchip = false;
+		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;
+		}
 	}
 
 	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..953580ac207e 100644
--- a/drivers/platform/chrome/cros_ec_lpc_reg.c
+++ b/drivers/platform/chrome/cros_ec_lpc_reg.c
@@ -9,6 +9,12 @@
 
 #include "cros_ec_lpc_mec.h"
 
+/*
+ * Assume that the Embedded Controller is the Microhcip variant, we will
+ * mark as false if that's not the case.
+ */
+bool cros_ec_is_microchip = true;
+
 static u8 lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
 {
 	int i;
@@ -37,12 +43,14 @@ 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)
 {
-	int in_range = cros_ec_lpc_mec_in_range(offset, length);
+	int in_range;
+
+	if (!cros_ec_is_microchip)
+		return lpc_read_bytes(offset, length, dest);
 
+	in_range = cros_ec_lpc_mec_in_range(offset, length);
 	if (in_range < 0)
 		return 0;
 
@@ -55,8 +63,12 @@ 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)
 {
-	int in_range = cros_ec_lpc_mec_in_range(offset, length);
+	int in_range;
 
+	if (!cros_ec_is_microchip)
+		return lpc_write_bytes(offset, length, msg);
+
+	in_range = cros_ec_lpc_mec_in_range(offset, length);
 	if (in_range < 0)
 		return 0;
 
@@ -77,25 +89,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..4705b3d27dc0 100644
--- a/drivers/platform/chrome/cros_ec_lpc_reg.h
+++ b/drivers/platform/chrome/cros_ec_lpc_reg.h
@@ -8,6 +8,9 @@
 #ifndef __CROS_EC_LPC_REG_H
 #define __CROS_EC_LPC_REG_H
 
+/* True if the CrOS Embedded Controller is a Microchip variant */
+extern bool cros_ec_is_microchip;
+
 /**
  * cros_ec_lpc_read_bytes - Read bytes from a given LPC-mapped address.
  * Returns 8-bit checksum of all bytes read.
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

Powered by blists - more mailing lists