Re: [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access

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

On Thu, Jan 02, 2025 at 04:19:50PM -0800, Gwendal Grignou wrote:
> Able to communicate with the EC:
> ```
>  ectool version
> ...
> Build info:    brtk-0.0.0-df0ad93+ 2024-12-13 19:25:55 elmo@396-a1a
> ```

Not sure if I understand: does it originally intend to show EC firmware
version (i.e. "RW version:")?

> Coreboot must exposed the memory region:
> ```
> cat /proc/iomem | grep GOOG0004
>     fe0b0000-fe0bffff : GOOG0004:00
> ```

`grep GOOG0004 /proc/iomem`.

> Can observe the commands flowing between AP and EC:
> cd /sys/kernel/debug/tracing
> echo 1 > events/cros_ec/enable
> echo 1 > tracing_on
> cat trace_pipe
>      kworker/7:0-51      [007] .....   188.619393: cros_ec_request_start: version: 0, offset: 0, command: EC_CMD_CONSOLE_SNAPSHOT, outsize: 0, insize: 0
>      kworker/7:0-51      [007] .....   188.625656: cros_ec_request_done: version: 0, offset: 0, command: EC_CMD_CONSOLE_SNAPSHOT, outsize: 0, insize: 0, ec result: EC_RES_SUCCESS, retval: 0
>      kworker/7:0-51      [007] .....   188.625663: cros_ec_request_start: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248
>      kworker/7:0-51      [007] .....   188.635119: cros_ec_request_done: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248, ec result: EC_RES_SUCCESS, retval: 147
>      kworker/7:0-51      [007] .....   188.635130: cros_ec_request_start: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248
>      kworker/7:0-51      [007] .....   188.642764: cros_ec_request_done: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248, ec result: EC_RES_SUCCESS, retval: 0
>      timberslide-3755    [005] .....   188.643039: cros_ec_request_start: version: 0, offset: 0, command: EC_CMD_GET_UPTIME_INFO, outsize: 0, insize: 44
>      timberslide-3755    [006] .....   188.650876: cros_ec_request_done: version: 0, offset: 0, command: EC_CMD_GET_UPTIME_INFO, outsize: 0, insize: 44, ec result: EC_RES_SUCCESS, retval: 44
>      timberslide-3755    [006] .....   188.650912: cros_ec_request_start: version: 0, offset: 0, command: EC_CMD_GET_UPTIME_INFO, outsize: 0, insize: 44
>      timberslide-3755    [006] .....   188.657907: cros_ec_request_done: version: 0, offset: 0, command: EC_CMD_GET_UPTIME_INFO, outsize: 0, insize: 44, ec result: EC_RES_SUCCESS, retval: 44
>      kworker/7:0-51      [007] .....   198.858864: cros_ec_request_start: version: 0, offset: 0, command: EC_CMD_CONSOLE_SNAPSHOT, outsize: 0, insize: 0
>      kworker/7:0-51      [007] .....   198.866866: cros_ec_request_done: version: 0, offset: 0, command: EC_CMD_CONSOLE_SNAPSHOT, outsize: 0, insize: 0, ec result: EC_RES_SUCCESS, retval: 0
>      kworker/7:0-51      [007] .....   198.866882: cros_ec_request_start: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248
>      kworker/7:0-51      [007] .....   198.874414: cros_ec_request_done: version: 1, offset: 0, command: EC_CMD_CONSOLE_READ, outsize: 1, insize: 248, ec result: EC_RES_SUCCESS, retval: 0

Can't it observe the trace before applying the patch?  I failed to
connect the trace and the direct memory accesses.


On a related note, the patch should use prefix "cros_ec_lpc" too.

> +static int cros_ec_lpc_direct_read(struct cros_ec_lpc *ec_lpc, unsigned int offset,
> +				   unsigned int length, u8 *dest)
> +{
[...]
> +	// Check range

To be consitent to the rest of code, don't use C99 comment style.  Or just
drop it as the intent is clear.

> +static int cros_ec_lpc_direct_write(struct cros_ec_lpc *ec_lpc, unsigned int offset,
> +				    unsigned int length, const u8 *msg)
> +{
[...]
> +	// Check range

Same here.

> +static acpi_status cros_ec_lpc_resources(struct acpi_resource *res, void *data)
> +{
> +	struct cros_ec_lpc *ec_lpc = data;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> +		ec_lpc->mem32 = res->data.fixed_memory32;
> +		break;
> +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> +		/* Ignored, using a more generic way to find IRQ. */
> +	case ACPI_RESOURCE_TYPE_END_TAG:

If nothing to do (or comment) in the case, could it drop the case?

> @@ -498,29 +560,52 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
[...]
> +		if (ec_lpc->mem32.address_length) {
> +			ec_lpc->base = devm_ioremap(dev,
> +						    ec_lpc->mem32.address,
> +						    ec_lpc->mem32.address_length);
> +			if (IS_ERR(ec_lpc->base))
> +				return AE_ERROR;

`PTR_ERR(...)`?