diff mbox

[v3,5/6] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

Message ID 1410428289-18229-6-git-send-email-javier.martinez@collabora.co.uk
State Superseded
Headers show

Commit Message

Javier Martinez Canillas Sept. 11, 2014, 9:38 a.m. UTC 
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 delay in milliseconds 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 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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Lee Jones Sept. 17, 2014, 4:23 p.m. UTC | #1 
On Thu, 11 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 delay in milliseconds 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 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 | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index c53804a..af2fadf 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -23,6 +23,10 @@
>  #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
> +#define EC_RETRY_DELAY_MS	10
>  
>  int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>  		       struct cros_ec_command *msg)
> @@ -69,6 +73,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(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);

Remove the EC_RETRY_DELAY_MS define and place the values in raw.

You're now sleeping for 10us.  Did you test the changes?

> +			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;
Javier Martinez Canillas Sept. 18, 2014, 7:42 a.m. UTC | #2 
Hello Lee,

Thanks a lot for your feedback.

On 09/17/2014 06:23 PM, Lee Jones wrote:
>>  
>>  	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(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);
> 
> Remove the EC_RETRY_DELAY_MS define and place the values in raw.
>

Ok, will do.

> You're now sleeping for 10us.  Did you test the changes?
> 

Duh, I must had been sleepy since I thought about changing the define but I
completely missed... which proves your point that raw values are more explicit
than using a define here.

Yes, I'm testing the changes and making sure that it does not add any
regression but I was not able to reproduce the case when an EC command result
is IN_PROGRESS. I'll investigate further on how to properly test that branch
before posting v4.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c53804a..af2fadf 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -23,6 +23,10 @@ 
 #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
+#define EC_RETRY_DELAY_MS	10
 
 int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 		       struct cros_ec_command *msg)
@@ -69,6 +73,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(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);
+
+			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;