Hi Thierry,
I reviewed your patches and looks good to me, I only found a few style
things that is up to maintainer decide if are needed or not, most of
them are feedback I received on other subsystems. Ah, and I've a
question about runtime detection of the EC (see below), but guess the
answer is no.
Reviewed-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
2016-11-08 13:27 GMT+01:00 Thierry Escande <thierry.esca...@collabora.com>:
From: Shawn Nematbakhsh <sha...@chromium.org>
This adds support for the ChromeOS LPC Microchip Embedded Controller
(mec1322) variant.
mec1322 accesses I/O region [800h, 9ffh] through embedded memory
interface (EMI) rather than LPC.
Signed-off-by: Shawn Nematbakhsh <sha...@chromium.org>
Signed-off-by: Gwendal Grignou <gwen...@chromium.org>
Signed-off-by: Guenter Roeck <gro...@chromium.org>
Signed-off-by: Thierry Escande <thierry.esca...@collabora.com>
---
drivers/platform/chrome/Kconfig | 9 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_lpc.c | 5 ++
drivers/platform/chrome/cros_ec_lpc_mec.c | 144 ++++++++++++++++++++++++++++++
drivers/platform/chrome/cros_ec_lpc_reg.c | 69 ++++++++++++++
include/linux/mfd/cros_ec_lpc_mec.h | 93 +++++++++++++++++++
include/linux/mfd/cros_ec_lpc_reg.h | 14 +++
7 files changed, 335 insertions(+)
create mode 100644 drivers/platform/chrome/cros_ec_lpc_mec.c
create mode 100644 include/linux/mfd/cros_ec_lpc_mec.h
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 76bdae1..55149f2 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -59,6 +59,15 @@ config CROS_EC_LPC
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
+ 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.
+
As reported by checkpatch, write a paragraph that describes the config
symbol fully. Maybe adding something like this
If you have a ChromeOS Embedded Controller Microchip EC variant
choose Y here.
According to the help if you have a non-Microchip EC you should leave
this as N. Would be possible some kind of runtime detection of the EC
? Just thinking in out loud.
config CROS_EC_PROTO
bool
help
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 127fbe8..b8f7a3b 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -5,6 +5,7 @@ cros_ec_devs-objs := cros_ec_dev.o
cros_ec_sysfs.o \
cros_ec_lightbar.o cros_ec_vbc.o
obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_devs.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
obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.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 617074e..264234b 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -349,10 +349,13 @@ static int __init cros_ec_lpc_init(void)
return -ENODEV;
}
+ cros_ec_lpc_reg_init();
+
/* Register the driver */
ret = platform_driver_register(&cros_ec_lpc_driver);
if (ret) {
pr_err(DRV_NAME ": can't register driver: %d\n", ret);
+ cros_ec_lpc_reg_destroy();
return ret;
}
@@ -361,6 +364,7 @@ static int __init cros_ec_lpc_init(void)
if (ret) {
pr_err(DRV_NAME ": can't register device: %d\n", ret);
platform_driver_unregister(&cros_ec_lpc_driver);
+ cros_ec_lpc_reg_destroy();
return ret;
}
@@ -371,6 +375,7 @@ static void __exit cros_ec_lpc_exit(void)
{
platform_device_unregister(&cros_ec_lpc_device);
platform_driver_unregister(&cros_ec_lpc_driver);
+ cros_ec_lpc_reg_destroy();
}
module_init(cros_ec_lpc_init);
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c
b/drivers/platform/chrome/cros_ec_lpc_mec.c
new file mode 100644
index 0000000..09e2e21
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.c
@@ -0,0 +1,144 @@
+/*
+ * cros_ec_lpc_mec - LPC variant I/O for Microchip EC
+ *
+ * Copyright (C) 2015 Google, Inc
+ *
Update the copyright to 2016
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver uses the Chrome OS EC byte-level message-based protocol for
+ * communicating the keyboard state (which keys are pressed) from a keyboard EC
+ * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
+ * but everything else (including deghosting) is done here. The main
+ * motivation for this is to keep the EC firmware as simple as possible, since
+ * it cannot be easily upgraded and EC flash/IRAM space is relatively
+ * expensive.
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/mfd/cros_ec_lpc_mec.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+/*
+ * This mutex must be held while accessing the EMI unit. We can't rely on the
+ * EC mutex because memmap data may be accessed without it being held.
+ */
+static struct mutex io_mutex;
+
+/*
+ * cros_ec_lpc_mec_emi_write_address
+ *
+ * Initialize EMI read / write at a given address.
+ *
+ * @addr: Starting read / write address
+ * @access_type: Type of access, typically 32-bit auto-increment
+ */
+static void cros_ec_lpc_mec_emi_write_address(
+ uint16_t addr,
It is preferred use type u16 over uint16_t
+ enum cros_ec_lpc_mec_emi_access_mode access_type)
+{
+ /* Address relative to start of EMI range */
+ addr -= MEC_EMI_RANGE_START;
+ outb((addr & 0xfc) | access_type, MEC_EMI_EC_ADDRESS_B0);
+ outb((addr >> 8) & 0x7f, MEC_EMI_EC_ADDRESS_B1);
+}
+
+/*
+ * cros_ec_lpc_io_bytes_mec - Read / write bytes to MEC EMI port
+ *
+ * @io_type: MEC_IO_READ or MEC_IO_WRITE, depending on request
+ * @offset: Base read / write address
+ * @length: Number of bytes to read / write
+ * @buf: Destination / source buffer
+ *
+ * @return 8-bit checksum of all bytes read / written
+ */
+u8 cros_ec_lpc_io_bytes_mec(
+ enum cros_ec_lpc_mec_io_type io_type,
+ unsigned int offset,
+ unsigned int length,
+ u8 *buf)
nit: When possible alignment should match open parenthesis. I'd define
the function as:
u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
unsigned int offset,
unsigned int length, u8 *buf)
+{
+ int i = 0;
+ int io_addr;
+ u8 sum = 0;
+ enum cros_ec_lpc_mec_emi_access_mode access, new_access;
+
+ /*
+ * Long access cannot be used on misaligned data since reading B0 loads
+ * the data register and writing B3 flushes.
+ */
+ if ((offset & 0x3) || length < 4)
+ access = ACCESS_TYPE_BYTE;
+ else
+ access = ACCESS_TYPE_LONG_AUTO_INCREMENT;
+
+ mutex_lock(&io_mutex);
+
+ /* Initialize I/O at desired address */
+ cros_ec_lpc_mec_emi_write_address(
+ offset,
+ access);
+
This fits in one line.
+ /* Skip bytes in case of misaligned offset */
+ io_addr = MEC_EMI_EC_DATA_B0 + (offset & 0x3);
+ while (i < length) {
+ while (io_addr <= MEC_EMI_EC_DATA_B3) {
+ if (io_type == MEC_IO_READ)
+ buf[i] = inb(io_addr++);
+ else
+ outb(buf[i], io_addr++);
+
+ sum += buf[i++];
+ offset++;
+
+ /* Extra bounds check in case of misaligned length */
+ if (i == length)
+ goto done;
+ }
+
+ /*
+ * Use long auto-increment access except for misaligned write,
+ * since writing B3 triggers the flush.
+ */
+ if (length - i < 4 && io_type == MEC_IO_WRITE)
+ new_access = ACCESS_TYPE_BYTE;
+ else
+ new_access = ACCESS_TYPE_LONG_AUTO_INCREMENT;
+ if (new_access != access ||
+ access != ACCESS_TYPE_LONG_AUTO_INCREMENT) {
+ access = new_access;
+ cros_ec_lpc_mec_emi_write_address(
+ offset,
+ access);
This also fits in one line.
+ }
+
+ /* Access [B0, B3] on each loop pass */
+ io_addr = MEC_EMI_EC_DATA_B0;
+ }
+done:
+ mutex_unlock(&io_mutex);
Add an empty line here
+ return sum;
+}
+EXPORT_SYMBOL(cros_ec_lpc_io_bytes_mec);
+
+void cros_ec_lpc_mec_init(void)
+{
+ mutex_init(&io_mutex);
+}
+EXPORT_SYMBOL(cros_ec_lpc_mec_init);
+
+void cros_ec_lpc_mec_destroy(void)
+{
+ mutex_destroy(&io_mutex);
+}
+EXPORT_SYMBOL(cros_ec_lpc_mec_destroy);
diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c
b/drivers/platform/chrome/cros_ec_lpc_reg.c
index 672d08c..afb29c4 100644
--- a/drivers/platform/chrome/cros_ec_lpc_reg.c
+++ b/drivers/platform/chrome/cros_ec_lpc_reg.c
@@ -24,6 +24,7 @@
#include <linux/io.h>
#include <linux/mfd/cros_ec.h>
#include <linux/mfd/cros_ec_commands.h>
+#include <linux/mfd/cros_ec_lpc_mec.h>
static u8 lpc_read_bytes(unsigned int offset, unsigned int length, u8 *dest)
{
@@ -53,12 +54,80 @@ 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)
{
+ if (length == 0)
+ return 0;
+
+ /* Access desired range through EMI interface */
+ if (offset >= MEC_EMI_RANGE_START && offset <= MEC_EMI_RANGE_END) {
+ /* Ensure we don't straddle EMI region */
+ if (WARN_ON(offset + length - 1 > MEC_EMI_RANGE_END))
+ return 0;
+
+ return cros_ec_lpc_io_bytes_mec(
+ MEC_IO_READ, offset, length, dest);
+ }
+
+ if (WARN_ON(offset + length > MEC_EMI_RANGE_START &&
+ offset < MEC_EMI_RANGE_START))
+ return 0;
+
return lpc_read_bytes(offset, length, dest);
}
u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length, u8 *msg)
{
+ if (length == 0)
+ return 0;
+
+ /* Access desired range through EMI interface */
+ if (offset >= MEC_EMI_RANGE_START && offset <= MEC_EMI_RANGE_END) {
+ /* Ensure we don't straddle EMI region */
+ if (WARN_ON(offset + length - 1 > MEC_EMI_RANGE_END))
+ return 0;
+
+ return cros_ec_lpc_io_bytes_mec(
+ MEC_IO_WRITE, offset, length, msg);
+ }
+
+ if (WARN_ON(offset + length > MEC_EMI_RANGE_START &&
+ offset < MEC_EMI_RANGE_START))
+ return 0;
+
return lpc_write_bytes(offset, length, msg);
}
+
+void cros_ec_lpc_reg_init(void)
+{
+ cros_ec_lpc_mec_init();
+}
+
+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/include/linux/mfd/cros_ec_lpc_mec.h
b/include/linux/mfd/cros_ec_lpc_mec.h
new file mode 100644
index 0000000..69da593
--- /dev/null
+++ b/include/linux/mfd/cros_ec_lpc_mec.h
@@ -0,0 +1,93 @@
+/*
+ * cros_ec_lpc_mec - LPC variant I/O for Microchip EC
+ *
+ * Copyright (C) 2015 Google, Inc
+ *
Update the copyright.
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver uses the Chrome OS EC byte-level message-based protocol for
+ * communicating the keyboard state (which keys are pressed) from a keyboard EC
+ * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
+ * but everything else (including deghosting) is done here. The main
+ * motivation for this is to keep the EC firmware as simple as possible, since
+ * it cannot be easily upgraded and EC flash/IRAM space is relatively
+ * expensive.
+ */
+
+#ifndef __LINUX_MFD_CROS_EC_MEC_H
+#define __LINUX_MFD_CROS_EC_MEC_H
+
+#include <linux/mfd/cros_ec_commands.h>
+
+enum cros_ec_lpc_mec_emi_access_mode {
+ /* 8-bit access */
+ ACCESS_TYPE_BYTE = 0x0,
+ /* 16-bit access */
+ ACCESS_TYPE_WORD = 0x1,
+ /* 32-bit access */
+ ACCESS_TYPE_LONG = 0x2,
+ /*
+ * 32-bit access, read or write of MEC_EMI_EC_DATA_B3 causes the
+ * EC data register to be incremented.
+ */
+ ACCESS_TYPE_LONG_AUTO_INCREMENT = 0x3,
+};
+
+enum cros_ec_lpc_mec_io_type {
+ MEC_IO_READ,
+ MEC_IO_WRITE,
+};
+
+/* Access IO ranges 0x800 thru 0x9ff using EMI interface instead of LPC */
+#define MEC_EMI_RANGE_START EC_HOST_CMD_REGION0
+#define MEC_EMI_RANGE_END (EC_LPC_ADDR_MEMMAP + EC_MEMMAP_SIZE)
+
+/* EMI registers are relative to base */
+#define MEC_EMI_BASE 0x800
+#define MEC_EMI_HOST_TO_EC (MEC_EMI_BASE + 0)
+#define MEC_EMI_EC_TO_HOST (MEC_EMI_BASE + 1)
+#define MEC_EMI_EC_ADDRESS_B0 (MEC_EMI_BASE + 2)
+#define MEC_EMI_EC_ADDRESS_B1 (MEC_EMI_BASE + 3)
+#define MEC_EMI_EC_DATA_B0 (MEC_EMI_BASE + 4)
+#define MEC_EMI_EC_DATA_B1 (MEC_EMI_BASE + 5)
+#define MEC_EMI_EC_DATA_B2 (MEC_EMI_BASE + 6)
+#define MEC_EMI_EC_DATA_B3 (MEC_EMI_BASE + 7)
+
+/*
+ * cros_ec_lpc_mec_init
+ *
+ * Initialize MEC I/O.
+ */
+void cros_ec_lpc_mec_init(void);
+
+/*
+ * cros_ec_lpc_mec_destroy
+ *
+ * Cleanup MEC I/O.
+ */
+void cros_ec_lpc_mec_destroy(void);
+
+/**
+ * cros_ec_lpc_io_bytes_mec - Read / write bytes to MEC EMI port
+ *
+ * @io_type: MEC_IO_READ or MEC_IO_WRITE, depending on request
+ * @offset: Base read / write address
+ * @length: Number of bytes to read / write
+ * @buf: Destination / source buffer
+ *
+ * @return 8-bit checksum of all bytes read / written
+ */
+u8 cros_ec_lpc_io_bytes_mec(
+ enum cros_ec_lpc_mec_io_type io_type,
+ unsigned int offset,
+ unsigned int length,
+ u8 *buf);
+
+#endif /* __LINUX_MFD_CROS_EC_MEC_H */
diff --git a/include/linux/mfd/cros_ec_lpc_reg.h
b/include/linux/mfd/cros_ec_lpc_reg.h
index f3668ab..daede3a 100644
--- a/include/linux/mfd/cros_ec_lpc_reg.h
+++ b/include/linux/mfd/cros_ec_lpc_reg.h
@@ -44,4 +44,18 @@ 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_reg_init
+ *
+ * Initialize register I/O.
+ */
+void cros_ec_lpc_reg_init(void);
+
+/**
+ * cros_ec_lpc_reg_destroy
+ *
+ * Cleanup reg I/O.
+ */
+void cros_ec_lpc_reg_destroy(void);
+
#endif /* __LINUX_MFD_CROS_EC_REG_H */
--
2.7.4
