Re: [PATCH 1/2] platform/chrome: cros_ec: jump to RW before probing

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, Nov 25, 2024 at 06:44:45PM +0000, Dawid Niedzwiecki wrote:
> To avoid such problems, send the RWSIG continue command first, which
> skips waiting for the jump to RW. Send the command more times, to make
> sure EC is ready in RW before the start of the actual probing process. If
> a EC device doesn't support the RWSIG, it will respond with invalid
> command error code and probing will continue as usual.

I'm wondering should it only send RWSIG_ACTION_CONTINUE if EC_CMD_GET_VERSION
shows the FW is still in RO.

Curious about: who (in which use case) is responsible for sending
RWSIG_ACTION_ABORT if it wants the EC stays in RO?

> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
[...]
> @@ -204,6 +204,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  	mutex_init(&ec_dev->lock);
>  	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
>  
> +	/* Send RWSIG continue to jump to RW for devices using RWSIG. */
> +	err = cros_ec_rwsig_continue(ec_dev);
> +	if (err)
> +		dev_warn(dev, "Failed to continue RWSIG: %d\n", err);

Too verbose, use dev_info() instead.

> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
[...]
> +int cros_ec_rwsig_continue(struct cros_ec_device *ec_dev)
> +{
[...]
> +	for (int i = 0; i < RWSIG_CONTINUE_RETRIES; i++) {
> +		ret = cros_ec_send_command(ec_dev, msg);
> +
> +		if (ret < 0)
> +			error_count++;

Should it just return the error if the transmission fails?

> +		else if (msg->result == EC_RES_INVALID_COMMAND)
> +			/*
> +			 * If EC_RES_INVALID_COMMAND is retured, it means RWSIG
> +			 * is not supported or EC is already in RW, so there is
> +			 * nothing left to do.
> +			 */
> +			break;
> +		else if (msg->result != EC_RES_SUCCESS)
> +			/* Unexpected command error. */
> +			error_count++;

Same as `ret < 0`, should it just return if any unexpected errors?

> +		else
> +			/*
> +			 * The EC_CMD_RWSIG_ACTION succeed. Send the command
> +			 * more times, to make sure EC is in RW. A following
> +			 * command can timeout, because EC may need some time to
> +			 * initialize after jump to RW.
> +			 */
> +			error_count = 0;
> +
> +		if (error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW)
> +			break;

It could return 0 if `error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW`.

> +
> +		if (ret != -ETIMEDOUT)
> +			usleep_range(90000, 100000);
> +	}
> +
> +	kfree(msg);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(cros_ec_rwsig_continue);