Message-ID: <05dff726-d92a-70ed-1143-cb7a81c732c0@collabora.com>
Date:   Thu, 1 Dec 2016 17:55:55 +0100
From:   Thierry Escande <thierry.escande@...labora.com>
To:     Enric Balletbo Serra <eballetbo@...il.com>
Cc:     Benson Leung <bleung@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND 2/2] platform/chrome: cros_ec_lpc: Add support for
 mec1322 EC

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@...labora.com>
>
> 2016-11-08 13:27 GMT+01:00 Thierry Escande <thierry.escande@...labora.com>:
>> From: Shawn Nematbakhsh <shawnn@...omium.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@...omium.org>
>> Signed-off-by: Gwendal Grignou <gwendal@...omium.org>
>> Signed-off-by: Guenter Roeck <groeck@...omium.org>
>> Signed-off-by: Thierry Escande <thierry.escande@...labora.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
>>

Powered by blists - more mailing lists