* [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3
2014-09-18 15:18 [PATCH v4 0/5] Second batch of cleanups for cros_ec Javier Martinez Canillas
@ 2014-09-18 15:18 ` Javier Martinez Canillas
2014-09-29 7:49 ` Lee Jones
2014-09-18 15:18 ` [PATCH v4 3/5] mfd: cros_ec: stop calling ->cmd_xfer() directly Javier Martinez Canillas
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-09-18 15:18 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3
2014-09-18 15:18 ` [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3 Javier Martinez Canillas
@ 2014-09-29 7:49 ` Lee Jones
2014-09-29 7:50 ` Lee Jones
0 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2014-09-29 7:49 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3
2014-09-29 7:49 ` Lee Jones
@ 2014-09-29 7:50 ` Lee Jones
2014-09-29 9:19 ` Wolfram Sang
0 siblings, 1 reply; 13+ messages in thread
From: Lee Jones @ 2014-09-29 7:50 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3
2014-09-29 7:50 ` Lee Jones
@ 2014-09-29 9:19 ` Wolfram Sang
0 siblings, 0 replies; 13+ messages in thread
From: Wolfram Sang @ 2014-09-29 9:19 UTC (permalink / raw)
To: Lee Jones
Cc: Javier Martinez Canillas, Dmitry Torokhov, Doug Anderson,
Simon Glass, Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 3/5] mfd: cros_ec: stop calling ->cmd_xfer() directly
2014-09-18 15:18 [PATCH v4 0/5] Second batch of cleanups for cros_ec Javier Martinez Canillas
2014-09-18 15:18 ` [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3 Javier Martinez Canillas
@ 2014-09-18 15:18 ` Javier Martinez Canillas
[not found] ` <1411053538-17237-4-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
2014-09-18 15:18 ` [PATCH v4 4/5] mfd: cros_ec: move locking into cros_ec_cmd_xfer Javier Martinez Canillas
[not found] ` <1411053538-17237-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
3 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-09-18 15:18 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 4/5] mfd: cros_ec: move locking into cros_ec_cmd_xfer
2014-09-18 15:18 [PATCH v4 0/5] Second batch of cleanups for cros_ec Javier Martinez Canillas
2014-09-18 15:18 ` [PATCH v4 2/5] i2c: i2c-cros-ec-tunnel: Set retries to 3 Javier Martinez Canillas
2014-09-18 15:18 ` [PATCH v4 3/5] mfd: cros_ec: stop calling ->cmd_xfer() directly Javier Martinez Canillas
@ 2014-09-18 15:18 ` Javier Martinez Canillas
[not found] ` <1411053538-17237-5-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
[not found] ` <1411053538-17237-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
3 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-09-18 15:18 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
[parent not found: <1411053538-17237-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>]
* [PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC
[not found] ` <1411053538-17237-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
@ 2014-09-18 15:18 ` Javier Martinez Canillas
2014-09-29 7:49 ` Lee Jones
2014-09-18 15:18 ` [PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS Javier Martinez Canillas
1 sibling, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-09-18 15:18 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC
2014-09-18 15:18 ` [PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC Javier Martinez Canillas
@ 2014-09-29 7:49 ` Lee Jones
0 siblings, 0 replies; 13+ messages in thread
From: Lee Jones @ 2014-09-29 7:49 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
[not found] ` <1411053538-17237-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
2014-09-18 15:18 ` [PATCH v4 1/5] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC Javier Martinez Canillas
@ 2014-09-18 15:18 ` Javier Martinez Canillas
2014-09-29 7:52 ` Lee Jones
1 sibling, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-09-18 15:18 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
2014-09-18 15:18 ` [PATCH v4 5/5] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS Javier Martinez Canillas
@ 2014-09-29 7:52 ` Lee Jones
0 siblings, 0 replies; 13+ messages in thread
From: Lee Jones @ 2014-09-29 7:52 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore,
Gwendal Grignou, linux-i2c, linux-input, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 13+ messages in thread