Messages in this thread | | | Subject | Re: [PATCH RESEND 2/2] platform/chrome: cros_ec_lpc: Add support for mec1322 EC | From | Thierry Escande <> | Date | Thu, 1 Dec 2016 17:55:55 +0100 |
| |
Hi Enric,
Thanks for the review.
On 30/11/2016 16:48, Enric Balletbo Serra wrote: > 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.balletbo@collabora.com> > > 2016-11-08 13:27 GMT+01:00 Thierry Escande <thierry.escande@collabora.com>: >> From: Shawn Nematbakhsh <shawnn@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 <shawnn@chromium.org> >> Signed-off-by: Gwendal Grignou <gwendal@chromium.org> >> Signed-off-by: Guenter Roeck <groeck@chromium.org> >> Signed-off-by: Thierry Escande <thierry.escande@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. Well, we can use the EC_CMD_GET_CHIP_INFO command and check for the chip name as it is "mec1322" (at least on the cyan that I have).
Regards, Thierry
> >> 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 >>
| |