[PATCH v4 0/5] Second batch of cleanups for cros_ec

[PATCH v4 0/5] Second batch of cleanups for cros_ec

[Javier Martinez Canillas]

Hello,

This is a second batch of cleanups patches for the mfd cros_ec
driver and its subdevices drivers. The first batch of cleanups
was posted by Doug Anderson [0] and have already been merged.
The patches were picked from the ChromeOS 3.8 kernel and after
these no cleanups patches for cros_ec are left, only commits
that add cros ec support not yet available in mainline.

This is a fourth version of the patch series that addresses
issues pointed out by Lee Jones on v3 [1]. I dropped patch #6
from this version since that change will make more sense once
the cros_ec_dev driver is added and the cells array isn't empty.

There is almost no functionality added on this series but the
idea is to reduce the delta between the mainline drivers and
the ones in the downstream Chrome OS 3.8 kernel so the missing
functionality can be added on top once these cleanups patches
are merged. The missing functionality currently in mainline is:

- Chrome OS Embedded Controller userspace device interface
- Chrome OS Embedded Controller Low Pin Count (LPC) inteface
- Access to vboot context stored on a block device
- Access to vboot context stored on EC's nvram

The patches in this series are authored by different people
(all on cc) and consist of the following:

Andrew Bresticker (3):
  mfd: cros_ec: stop calling ->cmd_xfer() directly
  mfd: cros_ec: move locking into cros_ec_cmd_xfer
  mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

Derek Basehore (1):
  i2c: i2c-cros-ec-tunnel: Set retries to 3

Doug Anderson (1):
  mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC

 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  5 +++-
 drivers/input/keyboard/cros_ec_keyb.c   |  2 +-
 drivers/mfd/cros_ec.c                   | 48 +++++++++++++++++++++++++++++++++
 drivers/mfd/cros_ec_spi.c               | 20 +++++++-------
 include/linux/mfd/cros_ec.h             | 24 ++++++++++++-----
 5 files changed, 80 insertions(+), 19 deletions(-)

Patches #1 and #2 can be applied independently but patches #3, #4 and #5
rely on the previous one so they should be applied together and in that order.

Best regards,
Javier

[0]: https://www.mail-archive.com/linux-input@vger.kernel.org/msg11385.html
[1]: https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg36505.html

[PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC

[Javier Martinez Canillas]

From: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

If someone sends a EC_CMD_REBOOT_EC to the EC, the EC will likely be
unresponsive for quite a while.  Add a delay to the end of the command
to prevent random failures of future commands.

NOTES:
* This could be optimized a bit by simply delaying the next command
  sent, but EC_CMD_REBOOT_EC is such a rare command that the extra
  complexity doesn't seem worth it.
* This is a bit of an "ugly hack" since the SPI driver is effectively
  snooping on the communication and making a lot of assumptions.  It
  would be nice to architect in some better solution long term.
* This same logic probably needs to be applied to the i2c driver.

Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Randall Spangler <rspangler-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Vadim Bendebury <vbendeb-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mfd/cros_ec_spi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 588c700..b396705 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -65,6 +65,12 @@
   */
 #define EC_SPI_RECOVERY_TIME_NS	(200 * 1000)
 
+/*
+ * The EC is unresponsive for a time after a reboot command.  Add a
+ * simple delay to make sure that the bus stays locked.
+ */
+#define EC_REBOOT_DELAY_MS	50
+
 /**
  * struct cros_ec_spi - information about a SPI-connected EC
  *
@@ -318,6 +324,9 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 
 	ret = len;
 exit:
+	if (ec_msg->command == EC_CMD_REBOOT_EC)
+		msleep(EC_REBOOT_DELAY_MS);
+
 	mutex_unlock(&ec_spi->lock);
 	return ret;
 }
-- 
2.1.0

[PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Javier Martinez Canillas]

From: Derek Basehore <dbasehore@chromium.org>

Since the i2c bus can get wedged on the EC sometimes, set the number of retries
to 3. Since we un-wedge the bus immediately after the wedge happens, this is the
correct fix since only one transfer will fail.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 3c15dcc..97e6369 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -16,6 +16,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#define I2C_MAX_RETRIES 3
+
 /**
  * struct ec_i2c_device - Driver data for I2C tunnel
  *
@@ -290,6 +292,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
 	bus->adap.algo_data = bus;
 	bus->adap.dev.parent = &pdev->dev;
 	bus->adap.dev.of_node = np;
+	bus->adap.retries = I2C_MAX_RETRIES;
 
 	err = i2c_add_adapter(&bus->adap);
 	if (err) {
-- 
2.1.0

[PATCH v4 3/5] mfd: cros_ec: stop calling ->cmd_xfer() directly

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic@chromium.org>

Instead of having users of the ChromeOS EC call the interface-specific
cmd_xfer() callback directly, introduce a central cros_ec_cmd_xfer()
to use instead.  This will allow us to put all the locking and retry
logic in one place instead of duplicating it across the different
drivers.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  2 +-
 drivers/input/keyboard/cros_ec_keyb.c   |  2 +-
 drivers/mfd/cros_ec.c                   |  7 +++++++
 include/linux/mfd/cros_ec.h             | 24 ++++++++++++++++++------
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 97e6369..ec5c38d 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -229,7 +229,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	msg.indata = response;
 	msg.insize = response_len;
 
-	result = bus->ec->cmd_xfer(bus->ec, &msg);
+	result = cros_ec_cmd_xfer(bus->ec, &msg);
 	if (result < 0)
 		goto exit;
 
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 8c09b3e..462bfcb 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -157,7 +157,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
 		.insize = ckdev->cols,
 	};
 
-	return ckdev->ec->cmd_xfer(ckdev->ec, &msg);
+	return cros_ec_cmd_xfer(ckdev->ec, &msg);
 }
 
 static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 4873f9c..a9faebd 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -62,6 +62,13 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_check_result);
 
+int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+		     struct cros_ec_command *msg)
+{
+	return ec_dev->cmd_xfer(ec_dev, msg);
+}
+EXPORT_SYMBOL(cros_ec_cmd_xfer);
+
 static const struct mfd_cell cros_devs[] = {
 	{
 		.name = "cros-ec-keyb",
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index fcbe9d1..0e166b9 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -62,10 +62,6 @@ struct cros_ec_command {
  * @dev: Device pointer
  * @was_wake_device: true if this device was set to wake the system from
  * sleep at the last suspend
- * @cmd_xfer: send command to EC and get response
- *     Returns the number of bytes received if the communication succeeded, but
- *     that doesn't mean the EC was happy with the command. The caller
- *     should check msg.result for the EC's result code.
  *
  * @priv: Private data
  * @irq: Interrupt to use
@@ -82,6 +78,10 @@ struct cros_ec_command {
  * @dout_size: size of dout buffer to allocate (zero to use static dout)
  * @parent: pointer to parent device (e.g. i2c or spi device)
  * @wake_enabled: true if this device can wake the system from sleep
+ * @cmd_xfer: send command to EC and get response
+ *     Returns the number of bytes received if the communication succeeded, but
+ *     that doesn't mean the EC was happy with the command. The caller
+ *     should check msg.result for the EC's result code.
  * @lock: one transaction at a time
  */
 struct cros_ec_device {
@@ -92,8 +92,6 @@ struct cros_ec_device {
 	struct device *dev;
 	bool was_wake_device;
 	struct class *cros_class;
-	int (*cmd_xfer)(struct cros_ec_device *ec,
-			struct cros_ec_command *msg);
 
 	/* These are used to implement the platform-specific interface */
 	void *priv;
@@ -104,6 +102,8 @@ struct cros_ec_device {
 	int dout_size;
 	struct device *parent;
 	bool wake_enabled;
+	int (*cmd_xfer)(struct cros_ec_device *ec,
+			struct cros_ec_command *msg);
 	struct mutex lock;
 };
 
@@ -153,6 +153,18 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
 			 struct cros_ec_command *msg);
 
 /**
+ * cros_ec_cmd_xfer - Send a command to the ChromeOS EC
+ *
+ * Call this to send a command to the ChromeOS EC.  This should be used
+ * instead of calling the EC's cmd_xfer() callback directly.
+ *
+ * @ec_dev: EC device
+ * @msg: Message to write
+ */
+int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+		     struct cros_ec_command *msg);
+
+/**
  * cros_ec_remove - Remove a ChromeOS EC
  *
  * Call this to deregister a ChromeOS EC, then clean up any private data.
-- 
2.1.0

[PATCH v4 4/5] mfd: cros_ec: move locking into cros_ec_cmd_xfer

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic@chromium.org>

Now that there's a central cros_ec_cmd_xfer(), move the locking
out of the SPI driver.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes since v1:
 - Remove mention of LPC driver in the commit message since it does not
   exist in mainline yet. Suggested by Doug Anderson.

 drivers/mfd/cros_ec.c     | 10 +++++++++-
 drivers/mfd/cros_ec_spi.c | 11 -----------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index a9faebd..c53804a 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -65,7 +65,13 @@ EXPORT_SYMBOL(cros_ec_check_result);
 int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg)
 {
-	return ec_dev->cmd_xfer(ec_dev, msg);
+	int ret;
+
+	mutex_lock(&ec_dev->lock);
+	ret = ec_dev->cmd_xfer(ec_dev, msg);
+	mutex_unlock(&ec_dev->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
@@ -98,6 +104,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 			return -ENOMEM;
 	}
 
+	mutex_init(&ec_dev->lock);
+
 	err = mfd_add_devices(dev, 0, cros_devs,
 			      ARRAY_SIZE(cros_devs),
 			      NULL, ec_dev->irq, NULL);
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index b396705..bf6e08e 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -79,13 +79,11 @@
  *	if no record
  * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
  *      is sent when we want to turn off CS at the end of a transaction.
- * @lock: mutex to ensure only one user of cros_ec_cmd_xfer_spi at a time
  */
 struct cros_ec_spi {
 	struct spi_device *spi;
 	s64 last_transfer_ns;
 	unsigned int end_of_msg_delay;
-	struct mutex lock;
 };
 
 static void debug_packet(struct device *dev, const char *name, u8 *ptr,
@@ -232,13 +230,6 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	int sum;
 	int ret = 0, final_ret;
 
-	/*
-	 * We have the shared ec_dev buffer plus we do lots of separate spi_sync
-	 * calls, so we need to make sure only one person is using this at a
-	 * time.
-	 */
-	mutex_lock(&ec_spi->lock);
-
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
 
@@ -327,7 +318,6 @@ exit:
 	if (ec_msg->command == EC_CMD_REBOOT_EC)
 		msleep(EC_REBOOT_DELAY_MS);
 
-	mutex_unlock(&ec_spi->lock);
 	return ret;
 }
 
@@ -359,7 +349,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	if (ec_spi == NULL)
 		return -ENOMEM;
 	ec_spi->spi = spi;
-	mutex_init(&ec_spi->lock);
 	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
 	if (!ec_dev)
 		return -ENOMEM;
-- 
2.1.0

[PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

When an EC command returns EC_RES_IN_PROGRESS, we need to query
the state of the EC until it indicates that it is no longer busy.
Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
it is working on the in-progress command.

The 10 milliseconds delay and the number of retries are the values
that were used by the flashrom tool when retrying commands.

Signed-off-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---

Changes since v3:
 - Fix us time and use raw values instead of a define. Suggested by Lee Jones.

Changes since v2:
 - Explain in the commit message from where the delay and retry values come from.
   Commented by Andrew Bresticker.
 - Move the needed definitions inside the if block. Suggested by Lee Jones.
 - Only check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
   -EAGAIN since the former implies the later. Suggested by Lee Jones.
 - Use usleep_range() instead of msleep() since doesn't handle values between 1~20.
   Suggested by Lee Jones.

Changes since v1:
 - The *xfer() calls don't modify the passed cros_ec_command so there is
   no need to populate it inside the for loop. Suggested by Lee Jones.

 drivers/mfd/cros_ec.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c53804a..fc0c81e 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -23,6 +23,9 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
+#include <linux/delay.h>
+
+#define EC_COMMAND_RETRIES	50
 
 int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 		       struct cros_ec_command *msg)
@@ -69,6 +72,36 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 
 	mutex_lock(&ec_dev->lock);
 	ret = ec_dev->cmd_xfer(ec_dev, msg);
+	if (msg->result == EC_RES_IN_PROGRESS) {
+		int i;
+		struct cros_ec_command status_msg;
+		struct ec_response_get_comms_status status;
+
+		status_msg.version = 0;
+		status_msg.command = EC_CMD_GET_COMMS_STATUS;
+		status_msg.outdata = NULL;
+		status_msg.outsize = 0;
+		status_msg.indata = (uint8_t *)&status;
+		status_msg.insize = sizeof(status);
+
+		/*
+		 * Query the EC's status until it's no longer busy or
+		 * we encounter an error.
+		 */
+		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
+			usleep_range(10000, 11000);
+
+			ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
+			if (ret < 0)
+				break;
+
+			msg->result = status_msg.result;
+			if (status_msg.result != EC_RES_SUCCESS)
+				break;
+			if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
+				break;
+		}
+	}
 	mutex_unlock(&ec_dev->lock);
 
 	return ret;
-- 
2.1.0

Re: [PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> From: Doug Anderson <dianders@chromium.org>
> 
> If someone sends a EC_CMD_REBOOT_EC to the EC, the EC will likely be
> unresponsive for quite a while.  Add a delay to the end of the command
> to prevent random failures of future commands.
> 
> NOTES:
> * This could be optimized a bit by simply delaying the next command
>   sent, but EC_CMD_REBOOT_EC is such a rare command that the extra
>   complexity doesn't seem worth it.
> * This is a bit of an "ugly hack" since the SPI driver is effectively
>   snooping on the communication and making a lot of assumptions.  It
>   would be nice to architect in some better solution long term.
> * This same logic probably needs to be applied to the i2c driver.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Randall Spangler <rspangler@chromium.org>
> Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/cros_ec_spi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index 588c700..b396705 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -65,6 +65,12 @@
>    */
>  #define EC_SPI_RECOVERY_TIME_NS	(200 * 1000)
>  
> +/*
> + * The EC is unresponsive for a time after a reboot command.  Add a
> + * simple delay to make sure that the bus stays locked.
> + */
> +#define EC_REBOOT_DELAY_MS	50
> +
>  /**
>   * struct cros_ec_spi - information about a SPI-connected EC
>   *
> @@ -318,6 +324,9 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>  
>  	ret = len;
>  exit:
> +	if (ec_msg->command == EC_CMD_REBOOT_EC)
> +		msleep(EC_REBOOT_DELAY_MS);
> +
>  	mutex_unlock(&ec_spi->lock);
>  	return ret;
>  }

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> From: Derek Basehore <dbasehore@chromium.org>
> 
> Since the i2c bus can get wedged on the EC sometimes, set the number of retries
> to 3. Since we un-wedge the bus immediately after the wedge happens, this is the
> correct fix since only one transfer will fail.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.

> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 3c15dcc..97e6369 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -16,6 +16,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> +#define I2C_MAX_RETRIES 3
> +
>  /**
>   * struct ec_i2c_device - Driver data for I2C tunnel
>   *
> @@ -290,6 +292,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
>  	bus->adap.algo_data = bus;
>  	bus->adap.dev.parent = &pdev->dev;
>  	bus->adap.dev.of_node = np;
> +	bus->adap.retries = I2C_MAX_RETRIES;
>  
>  	err = i2c_add_adapter(&bus->adap);
>  	if (err) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Lee Jones]

On Mon, 29 Sep 2014, Lee Jones wrote:

> On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:
> 
> > From: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > Since the i2c bus can get wedged on the EC sometimes, set the number of retries
> > to 3. Since we un-wedge the bus immediately after the wedge happens, this is the
> > correct fix since only one transfer will fail.
> > 
> > Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Acked-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > ---
> >  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Applied, thanks.

Wolfram,

Do you want a pull-request?

> > diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> > index 3c15dcc..97e6369 100644
> > --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> > +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> > @@ -16,6 +16,8 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  
> > +#define I2C_MAX_RETRIES 3
> > +
> >  /**
> >   * struct ec_i2c_device - Driver data for I2C tunnel
> >   *
> > @@ -290,6 +292,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
> >  	bus->adap.algo_data = bus;
> >  	bus->adap.dev.parent = &pdev->dev;
> >  	bus->adap.dev.of_node = np;
> > +	bus->adap.retries = I2C_MAX_RETRIES;
> >  
> >  	err = i2c_add_adapter(&bus->adap);
> >  	if (err) {
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 3/5] mfd: cros_ec: stop calling ->cmd_xfer() directly

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Instead of having users of the ChromeOS EC call the interface-specific
> cmd_xfer() callback directly, introduce a central cros_ec_cmd_xfer()
> to use instead.  This will allow us to put all the locking and retry
> logic in one place instead of duplicating it across the different
> drivers.
> 
> Signed-off-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8rSCDK34cm6iQ@public.gmane.org.uk>
> Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c |  2 +-
>  drivers/input/keyboard/cros_ec_keyb.c   |  2 +-
>  drivers/mfd/cros_ec.c                   |  7 +++++++
>  include/linux/mfd/cros_ec.h             | 24 ++++++++++++++++++------
>  4 files changed, 27 insertions(+), 8 deletions(-)

Applied, thanks.

> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 97e6369..ec5c38d 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -229,7 +229,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
>  	msg.indata = response;
>  	msg.insize = response_len;
>  
> -	result = bus->ec->cmd_xfer(bus->ec, &msg);
> +	result = cros_ec_cmd_xfer(bus->ec, &msg);
>  	if (result < 0)
>  		goto exit;
>  
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 8c09b3e..462bfcb 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -157,7 +157,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
>  		.insize = ckdev->cols,
>  	};
>  
> -	return ckdev->ec->cmd_xfer(ckdev->ec, &msg);
> +	return cros_ec_cmd_xfer(ckdev->ec, &msg);
>  }
>  
>  static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 4873f9c..a9faebd 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -62,6 +62,13 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_check_result);
>  
> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> +		     struct cros_ec_command *msg)
> +{
> +	return ec_dev->cmd_xfer(ec_dev, msg);
> +}
> +EXPORT_SYMBOL(cros_ec_cmd_xfer);
> +
>  static const struct mfd_cell cros_devs[] = {
>  	{
>  		.name = "cros-ec-keyb",
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index fcbe9d1..0e166b9 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -62,10 +62,6 @@ struct cros_ec_command {
>   * @dev: Device pointer
>   * @was_wake_device: true if this device was set to wake the system from
>   * sleep at the last suspend
> - * @cmd_xfer: send command to EC and get response
> - *     Returns the number of bytes received if the communication succeeded, but
> - *     that doesn't mean the EC was happy with the command. The caller
> - *     should check msg.result for the EC's result code.
>   *
>   * @priv: Private data
>   * @irq: Interrupt to use
> @@ -82,6 +78,10 @@ struct cros_ec_command {
>   * @dout_size: size of dout buffer to allocate (zero to use static dout)
>   * @parent: pointer to parent device (e.g. i2c or spi device)
>   * @wake_enabled: true if this device can wake the system from sleep
> + * @cmd_xfer: send command to EC and get response
> + *     Returns the number of bytes received if the communication succeeded, but
> + *     that doesn't mean the EC was happy with the command. The caller
> + *     should check msg.result for the EC's result code.
>   * @lock: one transaction at a time
>   */
>  struct cros_ec_device {
> @@ -92,8 +92,6 @@ struct cros_ec_device {
>  	struct device *dev;
>  	bool was_wake_device;
>  	struct class *cros_class;
> -	int (*cmd_xfer)(struct cros_ec_device *ec,
> -			struct cros_ec_command *msg);
>  
>  	/* These are used to implement the platform-specific interface */
>  	void *priv;
> @@ -104,6 +102,8 @@ struct cros_ec_device {
>  	int dout_size;
>  	struct device *parent;
>  	bool wake_enabled;
> +	int (*cmd_xfer)(struct cros_ec_device *ec,
> +			struct cros_ec_command *msg);
>  	struct mutex lock;
>  };
>  
> @@ -153,6 +153,18 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
>  			 struct cros_ec_command *msg);
>  
>  /**
> + * cros_ec_cmd_xfer - Send a command to the ChromeOS EC
> + *
> + * Call this to send a command to the ChromeOS EC.  This should be used
> + * instead of calling the EC's cmd_xfer() callback directly.
> + *
> + * @ec_dev: EC device
> + * @msg: Message to write
> + */
> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> +		     struct cros_ec_command *msg);
> +
> +/**
>   * cros_ec_remove - Remove a ChromeOS EC
>   *
>   * Call this to deregister a ChromeOS EC, then clean up any private data.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 4/5] mfd: cros_ec: move locking into cros_ec_cmd_xfer

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Now that there's a central cros_ec_cmd_xfer(), move the locking
> out of the SPI driver.
> 
> Signed-off-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8rSCDK34cm6iQ@public.gmane.org.uk>
> Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> Changes since v1:
>  - Remove mention of LPC driver in the commit message since it does not
>    exist in mainline yet. Suggested by Doug Anderson.
> 
>  drivers/mfd/cros_ec.c     | 10 +++++++++-
>  drivers/mfd/cros_ec_spi.c | 11 -----------
>  2 files changed, 9 insertions(+), 12 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index a9faebd..c53804a 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -65,7 +65,13 @@ EXPORT_SYMBOL(cros_ec_check_result);
>  int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  		     struct cros_ec_command *msg)
>  {
> -	return ec_dev->cmd_xfer(ec_dev, msg);
> +	int ret;
> +
> +	mutex_lock(&ec_dev->lock);
> +	ret = ec_dev->cmd_xfer(ec_dev, msg);
> +	mutex_unlock(&ec_dev->lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer);
>  
> @@ -98,6 +104,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  			return -ENOMEM;
>  	}
>  
> +	mutex_init(&ec_dev->lock);
> +
>  	err = mfd_add_devices(dev, 0, cros_devs,
>  			      ARRAY_SIZE(cros_devs),
>  			      NULL, ec_dev->irq, NULL);
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index b396705..bf6e08e 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -79,13 +79,11 @@
>   *	if no record
>   * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
>   *      is sent when we want to turn off CS at the end of a transaction.
> - * @lock: mutex to ensure only one user of cros_ec_cmd_xfer_spi at a time
>   */
>  struct cros_ec_spi {
>  	struct spi_device *spi;
>  	s64 last_transfer_ns;
>  	unsigned int end_of_msg_delay;
> -	struct mutex lock;
>  };
>  
>  static void debug_packet(struct device *dev, const char *name, u8 *ptr,
> @@ -232,13 +230,6 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>  	int sum;
>  	int ret = 0, final_ret;
>  
> -	/*
> -	 * We have the shared ec_dev buffer plus we do lots of separate spi_sync
> -	 * calls, so we need to make sure only one person is using this at a
> -	 * time.
> -	 */
> -	mutex_lock(&ec_spi->lock);
> -
>  	len = cros_ec_prepare_tx(ec_dev, ec_msg);
>  	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
>  
> @@ -327,7 +318,6 @@ exit:
>  	if (ec_msg->command == EC_CMD_REBOOT_EC)
>  		msleep(EC_REBOOT_DELAY_MS);
>  
> -	mutex_unlock(&ec_spi->lock);
>  	return ret;
>  }
>  
> @@ -359,7 +349,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>  	if (ec_spi == NULL)
>  		return -ENOMEM;
>  	ec_spi->spi = spi;
> -	mutex_init(&ec_spi->lock);
>  	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
>  	if (!ec_dev)
>  		return -ENOMEM;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> From: Andrew Bresticker <abrestic@chromium.org>
> 
> When an EC command returns EC_RES_IN_PROGRESS, we need to query
> the state of the EC until it indicates that it is no longer busy.
> Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
> commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
> it is working on the in-progress command.
> 
> The 10 milliseconds delay and the number of retries are the values
> that were used by the flashrom tool when retrying commands.
> 
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Changes since v3:
>  - Fix us time and use raw values instead of a define. Suggested by Lee Jones.
> 
> Changes since v2:
>  - Explain in the commit message from where the delay and retry values come from.
>    Commented by Andrew Bresticker.
>  - Move the needed definitions inside the if block. Suggested by Lee Jones.
>  - Only check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
>    -EAGAIN since the former implies the later. Suggested by Lee Jones.
>  - Use usleep_range() instead of msleep() since doesn't handle values between 1~20.
>    Suggested by Lee Jones.
> 
> Changes since v1:
>  - The *xfer() calls don't modify the passed cros_ec_command so there is
>    no need to populate it inside the for loop. Suggested by Lee Jones.
> 
>  drivers/mfd/cros_ec.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Assuming that it's okay to return success even on failure,

... applied, thanks.

> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index c53804a..fc0c81e 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -23,6 +23,9 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/mfd/cros_ec_commands.h>
> +#include <linux/delay.h>
> +
> +#define EC_COMMAND_RETRIES	50
>  
>  int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>  		       struct cros_ec_command *msg)
> @@ -69,6 +72,36 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  
>  	mutex_lock(&ec_dev->lock);
>  	ret = ec_dev->cmd_xfer(ec_dev, msg);
> +	if (msg->result == EC_RES_IN_PROGRESS) {
> +		int i;
> +		struct cros_ec_command status_msg;
> +		struct ec_response_get_comms_status status;
> +
> +		status_msg.version = 0;
> +		status_msg.command = EC_CMD_GET_COMMS_STATUS;
> +		status_msg.outdata = NULL;
> +		status_msg.outsize = 0;
> +		status_msg.indata = (uint8_t *)&status;
> +		status_msg.insize = sizeof(status);
> +
> +		/*
> +		 * Query the EC's status until it's no longer busy or
> +		 * we encounter an error.
> +		 */
> +		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> +			usleep_range(10000, 11000);
> +
> +			ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
> +			if (ret < 0)
> +				break;
> +
> +			msg->result = status_msg.result;
> +			if (status_msg.result != EC_RES_SUCCESS)
> +				break;
> +			if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
> +				break;
> +		}
> +	}
>  	mutex_unlock(&ec_dev->lock);
>  
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Mon, Sep 29, 2014 at 08:50:14AM +0100, Lee Jones wrote:
> On Mon, 29 Sep 2014, Lee Jones wrote:
> 
> > On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:
> > 
> > > From: Derek Basehore <dbasehore@chromium.org>
> > > 
> > > Since the i2c bus can get wedged on the EC sometimes, set the number of retries
> > > to 3. Since we un-wedge the bus immediately after the wedge happens, this is the
> > > correct fix since only one transfer will fail.
> > > 
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > Reviewed-by: Doug Anderson <dianders@chromium.org>
> > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > > ---
> > >  drivers/i2c/busses/i2c-cros-ec-tunnel.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > 
> > Applied, thanks.
> 
> Wolfram,
> 
> Do you want a pull-request?

Nope. All fine, there shouldn't be any conflict.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]