* [PATCH 0/2] Add support for IO memory mapped EC
@ 2025-01-03 0:19 Gwendal Grignou
2025-01-03 0:19 ` [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure Gwendal Grignou
2025-01-03 0:19 ` [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access Gwendal Grignou
0 siblings, 2 replies; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-03 0:19 UTC (permalink / raw)
To: bleung, tzungbi; +Cc: chrome-platform, dustin, ben, Gwendal Grignou
Some EC - Realtek - have their register IO mapped by the BIOS -
coreboot. They are not using the well know io register.
The memory mapping information is retrieved through the ACPI CRS
resource, and is used to access the registers.
To ease the support of these ECs, the global structure and accessor
funcions are now aware of the EC device private structure.
Gwendal Grignou (2):
platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private
structure
platform/chrome: Add Support for direct EC register memory access
drivers/platform/chrome/cros_ec_lpc.c | 208 ++++++++++++++++++--------
1 file changed, 144 insertions(+), 64 deletions(-)
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure
2025-01-03 0:19 [PATCH 0/2] Add support for IO memory mapped EC Gwendal Grignou
@ 2025-01-03 0:19 ` Gwendal Grignou
2025-01-06 7:06 ` Tzung-Bi Shih
2025-01-03 0:19 ` [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access Gwendal Grignou
1 sibling, 1 reply; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-03 0:19 UTC (permalink / raw)
To: bleung, tzungbi; +Cc: chrome-platform, dustin, ben, Gwendal Grignou
Remove cros_ec_lpc_ops global variable, since EC specific info can be
stored in the device private structure, introduced in
commit e4dbf9d65e4218 ("platform/chrome: cros_ec_lpc: add a "quirks" system").
Add ec_lpc pointer to read/write function to be able to access ec
specific data.
MEC support still uses global variables.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/platform/chrome/cros_ec_lpc.c | 86 +++++++++++++--------------
1 file changed, 41 insertions(+), 45 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index c559596a5e20b..413c969235a84 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -70,13 +70,6 @@ struct lpc_driver_data {
/**
* struct cros_ec_lpc - LPC device-specific data
* @mmio_memory_base: The first I/O port addressing EC mapped memory.
- */
-struct cros_ec_lpc {
- u16 mmio_memory_base;
-};
-
-/**
- * struct lpc_driver_ops - LPC driver operations
* @read: Copy length bytes from EC address offset into buffer dest.
* Returns a negative error code on error, or the 8-bit checksum
* of all bytes read.
@@ -84,18 +77,19 @@ struct cros_ec_lpc {
* Returns a negative error code on error, or the 8-bit checksum
* of all bytes written.
*/
-struct lpc_driver_ops {
- int (*read)(unsigned int offset, unsigned int length, u8 *dest);
- int (*write)(unsigned int offset, unsigned int length, const u8 *msg);
+struct cros_ec_lpc {
+ u16 mmio_memory_base;
+ int (*read)(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, u8 *dest);
+ int (*write)(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, const u8 *msg);
};
-static struct lpc_driver_ops cros_ec_lpc_ops = { };
-
/*
* A generic instance of the read function of struct lpc_driver_ops, used for
* the LPC EC.
*/
-static int cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
+static int cros_ec_lpc_read_bytes(struct cros_ec_lpc *_, unsigned int offset, unsigned int length,
u8 *dest)
{
u8 sum = 0;
@@ -114,7 +108,7 @@ static int cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
* A generic instance of the write function of struct lpc_driver_ops, used for
* the LPC EC.
*/
-static int cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
+static int cros_ec_lpc_write_bytes(struct cros_ec_lpc *_, unsigned int offset, unsigned int length,
const u8 *msg)
{
u8 sum = 0;
@@ -133,8 +127,8 @@ static int cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
* An instance of the read function of struct lpc_driver_ops, used for the
* MEC variant of LPC EC.
*/
-static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
- u8 *dest)
+static int cros_ec_lpc_mec_read_bytes(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, u8 *dest)
{
int in_range = cros_ec_lpc_mec_in_range(offset, length);
@@ -145,15 +139,15 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
offset - EC_HOST_CMD_REGION0,
length, dest) :
- cros_ec_lpc_read_bytes(offset, length, dest);
+ cros_ec_lpc_read_bytes(ec_lpc, offset, length, dest);
}
/*
* An instance of the write function of struct lpc_driver_ops, used for the
* MEC variant of LPC EC.
*/
-static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
- const u8 *msg)
+static int cros_ec_lpc_mec_write_bytes(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, const u8 *msg)
{
int in_range = cros_ec_lpc_mec_in_range(offset, length);
@@ -164,10 +158,11 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
offset - EC_HOST_CMD_REGION0,
length, (u8 *)msg) :
- cros_ec_lpc_write_bytes(offset, length, msg);
+ cros_ec_lpc_write_bytes(ec_lpc, offset, length, msg);
+}
}
-static int ec_response_timed_out(void)
+static int ec_response_timed_out(struct cros_ec_lpc *ec_lpc)
{
unsigned long one_second = jiffies + HZ;
u8 data;
@@ -175,7 +170,7 @@ static int ec_response_timed_out(void)
usleep_range(200, 300);
do {
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data);
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_CMD, 1, &data);
if (ret < 0)
return ret;
if (!(data & EC_LPC_STATUS_BUSY_MASK))
@@ -189,6 +184,7 @@ static int ec_response_timed_out(void)
static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
struct cros_ec_command *msg)
{
+ struct cros_ec_lpc *ec_lpc = ec->priv;
struct ec_host_response response;
u8 sum;
int ret = 0;
@@ -199,17 +195,17 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
goto done;
/* Write buffer */
- ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
+ ret = ec_lpc->write(ec_lpc, EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
if (ret < 0)
goto done;
/* Here we go */
sum = EC_COMMAND_PROTOCOL_3;
- ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+ ret = ec_lpc->write(ec_lpc, EC_LPC_ADDR_HOST_CMD, 1, &sum);
if (ret < 0)
goto done;
- ret = ec_response_timed_out();
+ ret = ec_response_timed_out(ec_lpc);
if (ret < 0)
goto done;
if (ret) {
@@ -219,7 +215,7 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
}
/* Check result */
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_DATA, 1, &sum);
if (ret < 0)
goto done;
msg->result = ret;
@@ -229,7 +225,7 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
/* Read back response */
dout = (u8 *)&response;
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_PACKET, sizeof(response),
dout);
if (ret < 0)
goto done;
@@ -246,7 +242,7 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
}
/* Read response and process checksum */
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_PACKET +
sizeof(response), response.data_len,
msg->data);
if (ret < 0)
@@ -270,6 +266,7 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
struct cros_ec_command *msg)
{
+ struct cros_ec_lpc *ec_lpc = ec->priv;
struct ec_lpc_host_args args;
u8 sum;
int ret = 0;
@@ -291,7 +288,7 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
sum = msg->command + args.flags + args.command_version + args.data_size;
/* Copy data and update checksum */
- ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
+ ret = ec_lpc->write(ec_lpc, EC_LPC_ADDR_HOST_PARAM, msg->outsize,
msg->data);
if (ret < 0)
goto done;
@@ -299,18 +296,18 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
/* Finalize checksum and write args */
args.checksum = sum;
- ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
+ ret = ec_lpc->write(ec_lpc, EC_LPC_ADDR_HOST_ARGS, sizeof(args),
(u8 *)&args);
if (ret < 0)
goto done;
/* Here we go */
sum = msg->command;
- ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+ ret = ec_lpc->write(ec_lpc, EC_LPC_ADDR_HOST_CMD, 1, &sum);
if (ret < 0)
goto done;
- ret = ec_response_timed_out();
+ ret = ec_response_timed_out(ec_lpc);
if (ret < 0)
goto done;
if (ret) {
@@ -320,7 +317,7 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
}
/* Check result */
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_DATA, 1, &sum);
if (ret < 0)
goto done;
msg->result = ret;
@@ -329,7 +326,7 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
goto done;
/* Read back args */
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
if (ret < 0)
goto done;
@@ -345,7 +342,7 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
sum = msg->command + args.flags + args.command_version + args.data_size;
/* Read response and update checksum */
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_HOST_PARAM, args.data_size,
msg->data);
if (ret < 0)
goto done;
@@ -381,7 +378,7 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
/* fixed length */
if (bytes) {
- ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + offset, bytes, s);
+ ret = ec_lpc->read(ec_lpc, ec_lpc->mmio_memory_base + offset, bytes, s);
if (ret < 0)
return ret;
return bytes;
@@ -389,7 +386,7 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
/* string */
for (; i < EC_MEMMAP_SIZE; i++, s++) {
- ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + i, 1, s);
+ ret = ec_lpc->read(ec_lpc, ec_lpc->mmio_memory_base + i, 1, s);
if (ret < 0)
return ret;
cnt++;
@@ -492,8 +489,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
}
if (quirks & CROS_EC_LPC_QUIRK_AML_MUTEX) {
- const char *name
- = driver_data->quirk_aml_mutex_name;
+ const char *name = driver_data->quirk_aml_mutex_name;
ret = cros_ec_lpc_mec_acpi_mutex(ACPI_COMPANION(dev), name);
if (ret) {
dev_err(dev, "failed to get AML mutex '%s'", name);
@@ -523,9 +519,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
* protocol fails, fallback to the non MEC variant and try to
* read again the ID.
*/
- cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
- cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
- ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
+ ec_lpc->read = cros_ec_lpc_mec_read_bytes;
+ ec_lpc->write = cros_ec_lpc_mec_write_bytes;
+ ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
if (ret < 0)
return ret;
if (buf[0] != 'E' || buf[1] != 'C') {
@@ -536,9 +532,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
}
/* Re-assign read/write operations for the non MEC variant */
- cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
- cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
- ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
+ ec_lpc->read = cros_ec_lpc_read_bytes;
+ ec_lpc->write = cros_ec_lpc_write_bytes;
+ ret = ec_lpc->read(ec_lpc, ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
buf);
if (ret < 0)
return ret;
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access
2025-01-03 0:19 [PATCH 0/2] Add support for IO memory mapped EC Gwendal Grignou
2025-01-03 0:19 ` [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure Gwendal Grignou
@ 2025-01-03 0:19 ` Gwendal Grignou
2025-01-06 7:06 ` Tzung-Bi Shih
1 sibling, 1 reply; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-03 0:19 UTC (permalink / raw)
To: bleung, tzungbi; +Cc: chrome-platform, dustin, ben, Gwendal Grignou
Add support to access EC memory region HOST command and ACPI memory
region directly.
The memory region comes from the CRS ACPI resource descriptor.
Able to communicate with the EC:
```
ectool version
...
Build info: brtk-0.0.0-df0ad93+ 2024-12-13 19:25:55 elmo@396-a1a
```
Coreboot must exposed the memory region:
```
cat /proc/iomem | grep GOOG0004
fe0b0000-fe0bffff : GOOG0004:00
```
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
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/platform/chrome/cros_ec_lpc.c | 126 +++++++++++++++++++++-----
1 file changed, 105 insertions(+), 21 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 413c969235a84..342a488afa8a3 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -70,6 +70,8 @@ struct lpc_driver_data {
/**
* struct cros_ec_lpc - LPC device-specific data
* @mmio_memory_base: The first I/O port addressing EC mapped memory.
+ * @base: For EC supporting memory mapping, base address of the mapped region.
+ * @mem32: Information about the memory mapped register region, if present.
* @read: Copy length bytes from EC address offset into buffer dest.
* Returns a negative error code on error, or the 8-bit checksum
* of all bytes read.
@@ -79,6 +81,8 @@ struct lpc_driver_data {
*/
struct cros_ec_lpc {
u16 mmio_memory_base;
+ void __iomem *base;
+ struct acpi_resource_fixed_memory32 mem32;
int (*read)(struct cros_ec_lpc *ec_lpc, unsigned int offset,
unsigned int length, u8 *dest);
int (*write)(struct cros_ec_lpc *ec_lpc, unsigned int offset,
@@ -160,6 +164,47 @@ static int cros_ec_lpc_mec_write_bytes(struct cros_ec_lpc *ec_lpc, unsigned int
length, (u8 *)msg) :
cros_ec_lpc_write_bytes(ec_lpc, offset, length, msg);
}
+
+static int cros_ec_lpc_direct_read(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, u8 *dest)
+{
+ int sum = 0;
+ int i;
+
+ // Check range
+ if (offset < EC_HOST_CMD_REGION0 || offset > EC_LPC_ADDR_MEMMAP +
+ EC_MEMMAP_SIZE) {
+ return cros_ec_lpc_read_bytes(ec_lpc, offset, length, dest);
+ }
+
+ for (i = 0; i < length; ++i) {
+ dest[i] = readb(ec_lpc->base + offset - EC_HOST_CMD_REGION0 + i);
+ sum += dest[i];
+ }
+
+ /* Return checksum of all bytes read */
+ return sum;
+}
+
+static int cros_ec_lpc_direct_write(struct cros_ec_lpc *ec_lpc, unsigned int offset,
+ unsigned int length, const u8 *msg)
+{
+ int sum = 0;
+ int i;
+
+ // Check range
+ if (offset < EC_HOST_CMD_REGION0 || offset > EC_LPC_ADDR_MEMMAP +
+ EC_MEMMAP_SIZE) {
+ return cros_ec_lpc_write_bytes(ec_lpc, offset, length, msg);
+ }
+
+ for (i = 0; i < length; ++i) {
+ writeb(msg[i], ec_lpc->base + offset - EC_HOST_CMD_REGION0 + i);
+ sum += msg[i];
+ }
+
+ /* Return checksum of all bytes written */
+ return sum;
}
static int ec_response_timed_out(struct cros_ec_lpc *ec_lpc)
@@ -450,6 +495,23 @@ static struct acpi_device *cros_ec_lpc_get_device(const char *id)
return adev;
}
+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:
+ default:
+ break;
+ }
+ return AE_OK;
+}
+
static int cros_ec_lpc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -498,29 +560,52 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
dev_info(dev, "got AML mutex '%s'", name);
}
}
-
- /*
- * The Framework Laptop (and possibly other non-ChromeOS devices)
- * only exposes the eight I/O ports that are required for the Microchip EC.
- * Requesting a larger reservation will fail.
- */
- if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
- EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
- dev_err(dev, "couldn't reserve MEC region\n");
- return -EBUSY;
+ adev = ACPI_COMPANION(dev);
+ if (adev) {
+ /*
+ * Retrieve the resource information in the CRS register, if available.
+ */
+ status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+ cros_ec_lpc_resources, ec_lpc);
+ if (ACPI_FAILURE(status)) {
+ dev_err(dev, "failed to get resources\n");
+ return -ENODEV;
+ }
+ 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;
+
+ ec_lpc->read = cros_ec_lpc_direct_read;
+ ec_lpc->write = cros_ec_lpc_direct_write;
+ }
}
+ if (!ec_lpc->read) {
+ /*
+ * The Framework Laptop (and possibly other non-ChromeOS devices)
+ * only exposes the eight I/O ports that are required for the Microchip EC.
+ * Requesting a larger reservation will fail.
+ */
+ if (!devm_request_region(dev, EC_HOST_CMD_REGION0,
+ EC_HOST_CMD_MEC_REGION_SIZE, dev_name(dev))) {
+ dev_err(dev, "couldn't reserve MEC region\n");
+ return -EBUSY;
+ }
- cros_ec_lpc_mec_init(EC_HOST_CMD_REGION0,
- EC_LPC_ADDR_MEMMAP + EC_MEMMAP_SIZE);
+ cros_ec_lpc_mec_init(EC_HOST_CMD_REGION0,
+ EC_LPC_ADDR_MEMMAP + EC_MEMMAP_SIZE);
- /*
- * Read the mapped ID twice, the first one is assuming the
- * EC is a Microchip Embedded Controller (MEC) variant, if the
- * protocol fails, fallback to the non MEC variant and try to
- * read again the ID.
- */
- ec_lpc->read = cros_ec_lpc_mec_read_bytes;
- ec_lpc->write = cros_ec_lpc_mec_write_bytes;
+ /*
+ * Read the mapped ID twice, the first one is assuming the
+ * EC is a Microchip Embedded Controller (MEC) variant, if the
+ * protocol fails, fallback to the non MEC variant and try to
+ * read again the ID.
+ */
+ ec_lpc->read = cros_ec_lpc_mec_read_bytes;
+ ec_lpc->write = cros_ec_lpc_mec_write_bytes;
+ }
ret = ec_lpc->read(ec_lpc, EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
if (ret < 0)
return ret;
@@ -594,7 +679,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
* Connect a notify handler to process MKBP messages if we have a
* companion ACPI device.
*/
- adev = ACPI_COMPANION(dev);
if (adev) {
status = acpi_install_notify_handler(adev->handle,
ACPI_ALL_NOTIFY,
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure
2025-01-03 0:19 ` [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure Gwendal Grignou
@ 2025-01-06 7:06 ` Tzung-Bi Shih
2025-01-06 18:50 ` Gwendal Grignou
0 siblings, 1 reply; 7+ messages in thread
From: Tzung-Bi Shih @ 2025-01-06 7:06 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: bleung, chrome-platform, dustin, ben
On Thu, Jan 02, 2025 at 04:19:49PM -0800, Gwendal Grignou wrote:
> MEC support still uses global variables.
Is the sentence redundant?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access
2025-01-03 0:19 ` [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access Gwendal Grignou
@ 2025-01-06 7:06 ` Tzung-Bi Shih
2025-01-06 18:51 ` Gwendal Grignou
0 siblings, 1 reply; 7+ messages in thread
From: Tzung-Bi Shih @ 2025-01-06 7:06 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: bleung, chrome-platform, dustin, ben
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(...)`?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure
2025-01-06 7:06 ` Tzung-Bi Shih
@ 2025-01-06 18:50 ` Gwendal Grignou
0 siblings, 0 replies; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-06 18:50 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, chrome-platform, dustin, ben
On Sun, Jan 5, 2025 at 11:06 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Thu, Jan 02, 2025 at 04:19:49PM -0800, Gwendal Grignou wrote:
> > MEC support still uses global variables.
>
> Is the sentence redundant?
I wanted to point out that "cros_ec_lpc_mec.c" still uses global
variables and assumes only EC in the device uses MEC. Removed in v2.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access
2025-01-06 7:06 ` Tzung-Bi Shih
@ 2025-01-06 18:51 ` Gwendal Grignou
0 siblings, 0 replies; 7+ messages in thread
From: Gwendal Grignou @ 2025-01-06 18:51 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: bleung, chrome-platform, dustin, ben
On Sun, Jan 5, 2025 at 11:06 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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.
Before this patch, the commands are not flowing between the AP and the EC.
>
>
> 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(...)`?
_probe returns an int, using another error.
All comments fixed in v2.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-01-06 18:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-03 0:19 [PATCH 0/2] Add support for IO memory mapped EC Gwendal Grignou
2025-01-03 0:19 ` [PATCH 1/2] platform/chrome: cros_ec_lpc: Merge lpc_driver_ops into ec private structure Gwendal Grignou
2025-01-06 7:06 ` Tzung-Bi Shih
2025-01-06 18:50 ` Gwendal Grignou
2025-01-03 0:19 ` [PATCH 2/2] platform/chrome: Add Support for direct EC register memory access Gwendal Grignou
2025-01-06 7:06 ` Tzung-Bi Shih
2025-01-06 18:51 ` Gwendal Grignou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox