[PATCH 0/6] Fix MEC concurrency problems for Framework Laptop

[PATCH 0/6] Fix MEC concurrency problems for Framework Laptop

[Ben Walsh]

Framework Laptops with the Microchip EC have a problem where the EC
"stops working" after a while. Symptoms include the Fn key not
working, and "bad packet checksum" errors appearing in the system log.

The problem is caused by ACPI code which accesses the Microchip EC
(MEC) memory using the Microchip EMI protocol. It uses an AML mutex to
prevent concurrent access. But the cros_ec_lpc driver is not aware of
this mutex. The ACPI code and LPC driver both attempt to talk to the
EC at the same time, messing up communication with the EC.

The solution is to have the cros_ec_lpc_mec code find and use the AML
mutex. But to make it all work we have to do a few more things:

  * Allow the cros_ec_lpc_mec code to return error codes in case it
    can't lock the mutex.

  * Have the cros_ec_lpc code find the correct ACPI device (PNP0C09)
    and then the AML mutex itself.

  * Use the quirks mechanism to specify the AML mutex name.

The code has been tested on an 11th-generation Intel Framework Laptop
(with Microchip EC) and an AMD Framework Laptop (without Microchip
EC). It has _not_ been tested on any Chromebook devices.

Ben Walsh (6):
  platform/chrome: cros_ec_lpc: MEC access can return error code
  platform/chrome: cros_ec_lpc: MEC access can use an AML mutex
  platform/chrome: cros_ec_lpc: Pass driver_data in static variable
  platform/chrome: cros_ec_lpc: Add a new quirk for AML mutex
  platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop
  platform/chrome: cros_ec_lpc: Add AML mutex for Framework Laptop

 drivers/platform/chrome/cros_ec_lpc.c      | 232 ++++++++++++++-------
 drivers/platform/chrome/cros_ec_lpc_mec.c  |  89 +++++++-
 drivers/platform/chrome/cros_ec_lpc_mec.h  |  18 +-
 drivers/platform/chrome/wilco_ec/mailbox.c |  22 +-
 4 files changed, 272 insertions(+), 89 deletions(-)


base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
-- 
2.45.0

[PATCH 1/6] platform/chrome: cros_ec_lpc: MEC access can return error code

[Ben Walsh]

cros_ec_lpc_io_bytes_mec was returning a u8 checksum of all bytes
read/written, which didn't leave room to indicate errors. Change this
u8 to an int where negative values indicate an error, and non-negative
values are the checksum as before.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c      | 148 ++++++++++++++-------
 drivers/platform/chrome/cros_ec_lpc_mec.c  |   9 +-
 drivers/platform/chrome/cros_ec_lpc_mec.h  |   7 +-
 drivers/platform/chrome/wilco_ec/mailbox.c |  22 +--
 4 files changed, 124 insertions(+), 62 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index ddfbfec44f4c..e638c7d82e22 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -62,14 +62,16 @@ struct cros_ec_lpc {
 
 /**
  * struct lpc_driver_ops - LPC driver operations
- * @read: Copy length bytes from EC address offset into buffer dest. Returns
- *        the 8-bit checksum of all bytes read.
- * @write: Copy length bytes from buffer msg into EC address offset. Returns
- *         the 8-bit checksum of all bytes written.
+ * @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.
+ * @write: Copy length bytes from buffer msg into EC address offset.
+ *         Returns a negative error code on error, or the 8-bit checksum
+ *         of all bytes written.
  */
 struct lpc_driver_ops {
-	u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
-	u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
+	int (*read)(unsigned int offset, unsigned int length, u8 *dest);
+	int (*write)(unsigned int offset, unsigned int length, const u8 *msg);
 };
 
 static struct lpc_driver_ops cros_ec_lpc_ops = { };
@@ -78,10 +80,10 @@ 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 u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
-				 u8 *dest)
+static int cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
+				  u8 *dest)
 {
-	int sum = 0;
+	u8 sum = 0;
 	int i;
 
 	for (i = 0; i < length; ++i) {
@@ -97,10 +99,10 @@ static u8 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 u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
-				  const u8 *msg)
+static int cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
+				   const u8 *msg)
 {
-	int sum = 0;
+	u8 sum = 0;
 	int i;
 
 	for (i = 0; i < length; ++i) {
@@ -116,14 +118,19 @@ static u8 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 u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
-				     u8 *dest)
+static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
+				      u8 *dest)
 {
-	int in_range = cros_ec_lpc_mec_in_range(offset, length);
+	int in_range;
 
-	if (in_range < 0)
+	if (length == 0)
 		return 0;
 
+	in_range = cros_ec_lpc_mec_in_range(offset, length);
+
+	if (in_range < 0)
+		return in_range;
+
 	return in_range ?
 		cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
 					 offset - EC_HOST_CMD_REGION0,
@@ -135,14 +142,19 @@ static u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
  * An instance of the write function of struct lpc_driver_ops, used for the
  * MEC variant of LPC EC.
  */
-static u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
-				      const u8 *msg)
+static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
+				       const u8 *msg)
 {
-	int in_range = cros_ec_lpc_mec_in_range(offset, length);
+	int in_range;
 
-	if (in_range < 0)
+	if (length == 0)
 		return 0;
 
+	in_range = cros_ec_lpc_mec_in_range(offset, length);
+
+	if (in_range < 0)
+		return in_range;
+
 	return in_range ?
 		cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
 					 offset - EC_HOST_CMD_REGION0,
@@ -154,11 +166,14 @@ static int ec_response_timed_out(void)
 {
 	unsigned long one_second = jiffies + HZ;
 	u8 data;
+	int ret;
 
 	usleep_range(200, 300);
 	do {
-		if (!(cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data) &
-		    EC_LPC_STATUS_BUSY_MASK))
+		ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data);
+		if (ret < 0)
+			return ret;
+		if (!(data & EC_LPC_STATUS_BUSY_MASK))
 			return 0;
 		usleep_range(100, 200);
 	} while (time_before(jiffies, one_second));
@@ -179,28 +194,41 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
 		goto done;
 
 	/* Write buffer */
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
+	if (ret < 0)
+		goto done;
 
 	/* Here we go */
 	sum = EC_COMMAND_PROTOCOL_3;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	if (ret < 0)
+		goto done;
 
-	if (ec_response_timed_out()) {
+	ret = ec_response_timed_out();
+	if (ret < 0)
+		goto done;
+	if (ret) {
 		dev_warn(ec->dev, "EC response timed out\n");
 		ret = -EIO;
 		goto done;
 	}
 
 	/* Check result */
-	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	if (ret < 0)
+		goto done;
+	msg->result = sum;
 	ret = cros_ec_check_result(ec, msg);
 	if (ret)
 		goto done;
 
 	/* Read back response */
 	dout = (u8 *)&response;
-	sum = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
 				   dout);
+	if (ret < 0)
+		goto done;
+	sum = ret;
 
 	msg->result = response.result;
 
@@ -213,9 +241,12 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
 	}
 
 	/* Read response and process checksum */
-	sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
-				    sizeof(response), response.data_len,
-				    msg->data);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
+				   sizeof(response), response.data_len,
+				   msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	if (sum) {
 		dev_err(ec->dev,
@@ -255,32 +286,47 @@ 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 */
-	sum += cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
-				     msg->data);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
+				    msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	/* Finalize checksum and write args */
 	args.checksum = sum;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
-			      (u8 *)&args);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
+				    (u8 *)&args);
+	if (ret < 0)
+		goto done;
 
 	/* Here we go */
 	sum = msg->command;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	if (ret < 0)
+		goto done;
 
-	if (ec_response_timed_out()) {
+	ret = ec_response_timed_out();
+	if (ret < 0)
+		goto done;
+	if (ret) {
 		dev_warn(ec->dev, "EC response timed out\n");
 		ret = -EIO;
 		goto done;
 	}
 
 	/* Check result */
-	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	if (ret < 0)
+		goto done;
+	msg->result = sum;
 	ret = cros_ec_check_result(ec, msg);
 	if (ret)
 		goto done;
 
 	/* Read back args */
-	cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
+	if (ret < 0)
+		goto done;
 
 	if (args.data_size > msg->insize) {
 		dev_err(ec->dev,
@@ -294,8 +340,11 @@ 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 */
-	sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
-				    msg->data);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
+				   msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	/* Verify checksum */
 	if (args.checksum != sum) {
@@ -320,19 +369,24 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
 	int i = offset;
 	char *s = dest;
 	int cnt = 0;
+	int ret;
 
 	if (offset >= EC_MEMMAP_SIZE - bytes)
 		return -EINVAL;
 
 	/* fixed length */
 	if (bytes) {
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + offset, bytes, s);
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + offset, bytes, s);
+		if (ret < 0)
+			return ret;
 		return bytes;
 	}
 
 	/* string */
 	for (; i < EC_MEMMAP_SIZE; i++, s++) {
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + i, 1, s);
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + i, 1, s);
+		if (ret < 0)
+			return ret;
 		cnt++;
 		if (!*s)
 			break;
@@ -425,8 +479,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 */
 	cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
 	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
-	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
-	if (buf[0] != 'E' || buf[1] != 'C') {
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
+	if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
 		if (!devm_request_region(dev, ec_lpc->mmio_memory_base, EC_MEMMAP_SIZE,
 					 dev_name(dev))) {
 			dev_err(dev, "couldn't reserve memmap region\n");
@@ -436,9 +490,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;
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
-				     buf);
-		if (buf[0] != 'E' || buf[1] != 'C') {
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
+					   buf);
+		if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
 			dev_err(dev, "EC ID not detected\n");
 			return -ENODEV;
 		}
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c
index 0d9c79b270ce..395dc3a6fb5e 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.c
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.c
@@ -67,11 +67,12 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length)
  * @length:  Number of bytes to read / write
  * @buf:     Destination / source buffer
  *
- * Return: 8-bit checksum of all bytes read / written
+ * @return:  A negative error code on error, or 8-bit checksum of all
+ *           bytes read / written
  */
-u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
-			    unsigned int offset, unsigned int length,
-			    u8 *buf)
+int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
+			     unsigned int offset, unsigned int length,
+			     u8 *buf)
 {
 	int i = 0;
 	int io_addr;
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
index 9d0521b23e8a..69670832f187 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.h
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
@@ -64,9 +64,10 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length);
  * @length:  Number of bytes to read / write
  * @buf:     Destination / source buffer
  *
- * @return 8-bit checksum of all bytes read / written
+ * @return:  A negative error code on error, or 8-bit checksum of all
+ *           bytes read / written
  */
-u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
-			    unsigned int offset, unsigned int length, u8 *buf);
+int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
+			     unsigned int offset, unsigned int length, u8 *buf);
 
 #endif /* __CROS_EC_LPC_MEC_H */
diff --git a/drivers/platform/chrome/wilco_ec/mailbox.c b/drivers/platform/chrome/wilco_ec/mailbox.c
index 0f98358ea824..4d8273b47cde 100644
--- a/drivers/platform/chrome/wilco_ec/mailbox.c
+++ b/drivers/platform/chrome/wilco_ec/mailbox.c
@@ -117,13 +117,17 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 			     struct wilco_ec_request *rq)
 {
 	struct wilco_ec_response *rs;
-	u8 checksum;
+	int ret;
 	u8 flag;
 
 	/* Write request header, then data */
-	cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, 0, sizeof(*rq), (u8 *)rq);
-	cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, sizeof(*rq), msg->request_size,
-				 msg->request_data);
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, 0, sizeof(*rq), (u8 *)rq);
+	if (ret < 0)
+		return ret;
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, sizeof(*rq), msg->request_size,
+				       msg->request_data);
+	if (ret < 0)
+		return ret;
 
 	/* Start the command */
 	outb(EC_MAILBOX_START_COMMAND, ec->io_command->start);
@@ -149,10 +153,12 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 
 	/* Read back response */
 	rs = ec->data_buffer;
-	checksum = cros_ec_lpc_io_bytes_mec(MEC_IO_READ, 0,
-					    sizeof(*rs) + EC_MAILBOX_DATA_SIZE,
-					    (u8 *)rs);
-	if (checksum) {
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_READ, 0,
+				       sizeof(*rs) + EC_MAILBOX_DATA_SIZE,
+				       (u8 *)rs);
+	if (ret < 0)
+		return ret;
+	if (ret) {
 		dev_dbg(ec->dev, "bad packet checksum 0x%02x\n", rs->checksum);
 		return -EBADMSG;
 	}
-- 
2.45.0

Re: [PATCH 1/6] platform/chrome: cros_ec_lpc: MEC access can return error code

[Tzung-Bi Shih]

On Wed, May 15, 2024 at 06:56:26AM +0100, Ben Walsh wrote:
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
[...]
> @@ -116,14 +118,19 @@ static u8 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 u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> -				     u8 *dest)
> +static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> +				      u8 *dest)
>  {
> -	int in_range = cros_ec_lpc_mec_in_range(offset, length);
> +	int in_range;
>  
> -	if (in_range < 0)
> +	if (length == 0)
>  		return 0;
>  
> +	in_range = cros_ec_lpc_mec_in_range(offset, length);
> +
> +	if (in_range < 0)
> +		return in_range;
> +
>  	return in_range ?
>  		cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
>  					 offset - EC_HOST_CMD_REGION0,

The `in_range` change looks irrelevant to the patch.  Or it should rather be
an independent patch if it fixes something.

> @@ -135,14 +142,19 @@ static u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
>   * An instance of the write function of struct lpc_driver_ops, used for the
>   * MEC variant of LPC EC.
>   */
> -static u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> -				      const u8 *msg)
> +static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> +				       const u8 *msg)
>  {
> -	int in_range = cros_ec_lpc_mec_in_range(offset, length);
> +	int in_range;
>  
> -	if (in_range < 0)
> +	if (length == 0)
>  		return 0;
>  
> +	in_range = cros_ec_lpc_mec_in_range(offset, length);
> +
> +	if (in_range < 0)
> +		return in_range;
> +
>  	return in_range ?
>  		cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
>  					 offset - EC_HOST_CMD_REGION0,

Same as above.

> @@ -179,28 +194,41 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
[...]
>  	/* Check result */
> -	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> +	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> +	if (ret < 0)
> +		goto done;
> +	msg->result = sum;

Even though they are equivalent, `msg->result = ret` looks more intuitive.

> @@ -255,32 +286,47 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
[...]
>  	/* Check result */
> -	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> +	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
> +	if (ret < 0)
> +		goto done;
> +	msg->result = sum;

Same as above.

[PATCH 2/6] platform/chrome: cros_ec_lpc: MEC access can use an AML mutex

[Ben Walsh]

Framework Laptops have ACPI code which accesses the MEC memory. It
uses an AML mutex to prevent concurrent access. But the cros_ec_lpc
driver was not aware of this mutex. The ACPI code and LPC driver both
attempted to talk to the EC at the same time, messing up communication
with the EC.

Allow the LPC driver MEC code to find and use the AML mutex.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc_mec.c | 80 ++++++++++++++++++++++-
 drivers/platform/chrome/cros_ec_lpc_mec.h | 11 ++++
 2 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c
index 395dc3a6fb5e..9f9aa14e96d4 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.c
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.c
@@ -10,13 +10,71 @@
 
 #include "cros_ec_lpc_mec.h"
 
+#define ACPI_LOCK_DELAY_MS 500
+
 /*
  * This mutex must be held while accessing the EMI unit. We can't rely on the
  * EC mutex because memmap data may be accessed without it being held.
  */
 static DEFINE_MUTEX(io_mutex);
+/*
+ * An alternative mutex to be used when the ACPI AML code may also
+ * access memmap data.  When set, this mutex is used in preference to
+ * io_mutex.
+ */
+static acpi_handle aml_mutex;
+
 static u16 mec_emi_base, mec_emi_end;
 
+/**
+ * cros_ec_lpc_mec_lock() - Acquire mutex for EMI
+ *
+ * @return: Negative error code, or zero for success
+ */
+static int cros_ec_lpc_mec_lock(void)
+{
+	bool success;
+
+	if (!aml_mutex) {
+		mutex_lock(&io_mutex);
+		return 0;
+	}
+
+	success = ACPI_SUCCESS(acpi_acquire_mutex(aml_mutex,
+						  NULL, ACPI_LOCK_DELAY_MS));
+
+	if (!success) {
+		pr_info("%s failed.", __func__);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+/**
+ * cros_ec_lpc_mec_unlock() - Release mutex for EMI
+ *
+ * @return: Negative error code, or zero for success
+ */
+static int cros_ec_lpc_mec_unlock(void)
+{
+	bool success;
+
+	if (!aml_mutex) {
+		mutex_unlock(&io_mutex);
+		return 0;
+	}
+
+	success = ACPI_SUCCESS(acpi_release_mutex(aml_mutex, NULL));
+
+	if (!success) {
+		pr_err("%s failed.", __func__);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 /**
  * cros_ec_lpc_mec_emi_write_address() - Initialize EMI at a given address.
  *
@@ -78,6 +136,7 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	int io_addr;
 	u8 sum = 0;
 	enum cros_ec_lpc_mec_emi_access_mode access, new_access;
+	int ret;
 
 	/* Return checksum of 0 if window is not initialized */
 	WARN_ON(mec_emi_base == 0 || mec_emi_end == 0);
@@ -93,7 +152,9 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	else
 		access = ACCESS_TYPE_LONG_AUTO_INCREMENT;
 
-	mutex_lock(&io_mutex);
+	ret = cros_ec_lpc_mec_lock();
+	if (ret)
+		return ret;
 
 	/* Initialize I/O at desired address */
 	cros_ec_lpc_mec_emi_write_address(offset, access);
@@ -135,7 +196,7 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	}
 
 done:
-	mutex_unlock(&io_mutex);
+	cros_ec_lpc_mec_unlock();
 
 	return sum;
 }
@@ -147,3 +208,18 @@ void cros_ec_lpc_mec_init(unsigned int base, unsigned int end)
 	mec_emi_end = end;
 }
 EXPORT_SYMBOL(cros_ec_lpc_mec_init);
+
+int cros_ec_lpc_mec_acpi_mutex(struct acpi_device *adev, const char *pathname)
+{
+	int status;
+
+	if (!adev)
+		return -ENOENT;
+
+	status = acpi_get_handle(adev->handle, pathname, &aml_mutex);
+	if (ACPI_FAILURE(status))
+		return -ENOENT;
+
+	return 0;
+}
+EXPORT_SYMBOL(cros_ec_lpc_mec_acpi_mutex);
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
index 69670832f187..69f9d8786f61 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.h
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
@@ -8,6 +8,8 @@
 #ifndef __CROS_EC_LPC_MEC_H
 #define __CROS_EC_LPC_MEC_H
 
+#include <linux/acpi.h>
+
 enum cros_ec_lpc_mec_emi_access_mode {
 	/* 8-bit access */
 	ACCESS_TYPE_BYTE = 0x0,
@@ -45,6 +47,15 @@ enum cros_ec_lpc_mec_io_type {
  */
 void cros_ec_lpc_mec_init(unsigned int base, unsigned int end);
 
+/**
+ * cros_ec_lpc_mec_acpi_mutex() - Find and set ACPI mutex for MEC
+ *
+ * @adev:     Parent ACPI device
+ * @pathname: Name of AML mutex
+ * @return:   Negative error code, or zero for success
+ */
+int cros_ec_lpc_mec_acpi_mutex(struct acpi_device *adev, const char *pathname);
+
 /**
  * cros_ec_lpc_mec_in_range() - Determine if addresses are in MEC EMI range.
  *
-- 
2.45.0

Re: [PATCH 2/6] platform/chrome: cros_ec_lpc: MEC access can use an AML mutex

[Tzung-Bi Shih]

On Wed, May 15, 2024 at 06:56:27AM +0100, Ben Walsh wrote:
> diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c
[...]
> +static int cros_ec_lpc_mec_lock(void)
> +{
> +	bool success;
> +
> +	if (!aml_mutex) {
> +		mutex_lock(&io_mutex);
> +		return 0;
> +	}
> +
> +	success = ACPI_SUCCESS(acpi_acquire_mutex(aml_mutex,
> +						  NULL, ACPI_LOCK_DELAY_MS));
> +
> +	if (!success) {
> +		pr_info("%s failed.", __func__);
> +		return -EBUSY;

I guess it doesn't need to print anything as the -EBUSY should be propagated
correctly now.

> +static int cros_ec_lpc_mec_unlock(void)
> +{
> +	bool success;
> +
> +	if (!aml_mutex) {
> +		mutex_unlock(&io_mutex);
> +		return 0;
> +	}
> +
> +	success = ACPI_SUCCESS(acpi_release_mutex(aml_mutex, NULL));
> +
> +	if (!success) {
> +		pr_err("%s failed.", __func__);
> +		return -EBUSY;

Same here.

[PATCH 3/6] platform/chrome: cros_ec_lpc: Pass driver_data in static variable

[Ben Walsh]

The module init passed extra data to the driver via the driver_data of
a new platform device. But when an ACPI device already existed, no
platform device was created, so this extra data wasn't passed to the
driver.

Stop using the driver_data and use a static variable instead.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index e638c7d82e22..b27519fbdf58 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -76,6 +76,8 @@ struct lpc_driver_ops {
 
 static struct lpc_driver_ops cros_ec_lpc_ops = { };
 
+static const struct lpc_driver_data *cros_ec_lpc_driver_data;
+
 /*
  * A generic instance of the read function of struct lpc_driver_ops, used for
  * the LPC EC.
@@ -435,7 +437,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	acpi_status status;
 	struct cros_ec_device *ec_dev;
 	struct cros_ec_lpc *ec_lpc;
-	struct lpc_driver_data *driver_data;
 	u8 buf[2] = {};
 	int irq, ret;
 	u32 quirks;
@@ -446,15 +447,15 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
 
-	driver_data = platform_get_drvdata(pdev);
-	if (driver_data) {
-		quirks = driver_data->quirks;
+	if (cros_ec_lpc_driver_data) {
+		quirks = cros_ec_lpc_driver_data->quirks;
 
 		if (quirks)
 			dev_info(dev, "loaded with quirks %8.08x\n", quirks);
 
 		if (quirks & CROS_EC_LPC_QUIRK_REMAP_MEMORY)
-			ec_lpc->mmio_memory_base = driver_data->quirk_mmio_memory_base;
+			ec_lpc->mmio_memory_base
+				= cros_ec_lpc_driver_data->quirk_mmio_memory_base;
 	}
 
 	/*
@@ -735,7 +736,10 @@ static int __init cros_ec_lpc_init(void)
 
 	dmi_match = dmi_first_match(cros_ec_lpc_dmi_table);
 
-	if (!cros_ec_lpc_acpi_device_found && !dmi_match) {
+	if (dmi_match) {
+		/* Pass the DMI match's driver data down to the platform device */
+		cros_ec_lpc_driver_data = dmi_match->driver_data;
+	} else if (!cros_ec_lpc_acpi_device_found) {
 		pr_err(DRV_NAME ": unsupported system.\n");
 		return -ENODEV;
 	}
@@ -748,9 +752,6 @@ static int __init cros_ec_lpc_init(void)
 	}
 
 	if (!cros_ec_lpc_acpi_device_found) {
-		/* Pass the DMI match's driver data down to the platform device */
-		platform_set_drvdata(&cros_ec_lpc_device, dmi_match->driver_data);
-
 		/* Register the device, and it'll get hooked up automatically */
 		ret = platform_device_register(&cros_ec_lpc_device);
 		if (ret) {
-- 
2.45.0

Re: [PATCH 3/6] platform/chrome: cros_ec_lpc: Pass driver_data in static variable

[Tzung-Bi Shih]

On Wed, May 15, 2024 at 06:56:28AM +0100, Ben Walsh wrote:
> The module init passed extra data to the driver via the driver_data of
> a new platform device. But when an ACPI device already existed, no
> platform device was created, so this extra data wasn't passed to the
> driver.
> 
> Stop using the driver_data and use a static variable instead.

Could it be possible to put the driver data in the acpi_device_id and
somewhere calls acpi_device_get_match_data()?

[PATCH 4/6] platform/chrome: cros_ec_lpc: Add a new quirk for AML mutex

[Ben Walsh]

Add a new quirk "CROS_EC_LPC_QUIRK_AML_MUTEX" which provides the name
of an AML mutex to protect MEC memory access.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index b27519fbdf58..ab7779b57a13 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -39,6 +39,11 @@ static bool cros_ec_lpc_acpi_device_found;
  * be used as the base port for EC mapped memory.
  */
 #define CROS_EC_LPC_QUIRK_REMAP_MEMORY              BIT(0)
+/*
+ * Indicates that an AML mutex should be used to protect access to
+ * Microchip EC.
+ */
+#define CROS_EC_LPC_QUIRK_AML_MUTEX                 BIT(1)
 
 /**
  * struct lpc_driver_data - driver data attached to a DMI device ID to indicate
@@ -46,10 +51,13 @@ static bool cros_ec_lpc_acpi_device_found;
  * @quirks: a bitfield composed of quirks from CROS_EC_LPC_QUIRK_*
  * @quirk_mmio_memory_base: The first I/O port addressing EC mapped memory (used
  *                          when quirk ...REMAP_MEMORY is set.)
+ * @quirk_aml_mutex_name: The name of an AML mutex to be used to protect access
+ *                        to Microchip EC.
  */
 struct lpc_driver_data {
 	u32 quirks;
 	u16 quirk_mmio_memory_base;
+	const char *quirk_aml_mutex_name;
 };
 
 /**
@@ -447,6 +455,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
 
+	adev = ACPI_COMPANION(dev);
+
 	if (cros_ec_lpc_driver_data) {
 		quirks = cros_ec_lpc_driver_data->quirks;
 
@@ -456,6 +466,18 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 		if (quirks & CROS_EC_LPC_QUIRK_REMAP_MEMORY)
 			ec_lpc->mmio_memory_base
 				= cros_ec_lpc_driver_data->quirk_mmio_memory_base;
+
+		if (quirks & CROS_EC_LPC_QUIRK_AML_MUTEX) {
+			const char *name
+				= cros_ec_lpc_driver_data->quirk_aml_mutex_name;
+			ret = cros_ec_lpc_mec_acpi_mutex(adev, name);
+			if (ret) {
+				dev_err(dev, "failed to get AML mutex '%s'", name);
+				return ret;
+			}
+
+			dev_info(dev, "got AML mutex '%s'", name);
+		}
 	}
 
 	/*
@@ -549,7 +571,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.45.0

[PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Ben Walsh]

Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to
find the device. This makes it easy to find the AML mutex via the
ACPI_COMPANION device.

The name "PNP0C09" is part of the ACPI standard, not Chrome-specific,
so only recognise the device if the DMI data is recognised too.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 38 ++++++++++++++++-----------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index ab7779b57a13..a9200f0a1a37 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -29,10 +29,12 @@
 #include "cros_ec_lpc_mec.h"
 
 #define DRV_NAME "cros_ec_lpcs"
-#define ACPI_DRV_NAME "GOOG0004"
 
-/* True if ACPI device is present */
-static bool cros_ec_lpc_acpi_device_found;
+/*
+ * Index into cros_ec_lpc_acpi_device_ids of ACPI device,
+ * negative for ACPI device not found.
+ */
+static int cros_ec_lpc_acpi_device_found;
 
 /*
  * Indicates that lpc_driver_data.quirk_mmio_memory_base should
@@ -598,7 +600,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id cros_ec_lpc_acpi_device_ids[] = {
-	{ ACPI_DRV_NAME, 0 },
+	{ "PNP0C09", 0 },    /* Standard ACPI EC name. Must be first in list */
+	{ "GOOG0004", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, cros_ec_lpc_acpi_device_ids);
@@ -737,30 +740,33 @@ static struct platform_device cros_ec_lpc_device = {
 	.name = DRV_NAME
 };
 
-static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level,
-					    void *context, void **retval)
+static int cros_ec_lpc_find_acpi_dev(const struct acpi_device_id *acpi_ids)
 {
-	*(bool *)context = true;
-	return AE_CTRL_TERMINATE;
+	int i;
+
+	for (i = 0; acpi_ids[i].id[0]; ++i) {
+		if (acpi_dev_present(acpi_ids[i].id, NULL, -1))
+			return i;
+	}
+
+	return -1;
 }
 
 static int __init cros_ec_lpc_init(void)
 {
 	int ret;
-	acpi_status status;
 	const struct dmi_system_id *dmi_match;
 
-	status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
-				  &cros_ec_lpc_acpi_device_found, NULL);
-	if (ACPI_FAILURE(status))
-		pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
+	cros_ec_lpc_acpi_device_found = cros_ec_lpc_find_acpi_dev(
+		cros_ec_lpc_driver.driver.acpi_match_table);
 
 	dmi_match = dmi_first_match(cros_ec_lpc_dmi_table);
 
 	if (dmi_match) {
 		/* Pass the DMI match's driver data down to the platform device */
 		cros_ec_lpc_driver_data = dmi_match->driver_data;
-	} else if (!cros_ec_lpc_acpi_device_found) {
+	} else if (cros_ec_lpc_acpi_device_found <= 0) {
+		/* Standard EC "PNP0C09" not supported without DMI data */
 		pr_err(DRV_NAME ": unsupported system.\n");
 		return -ENODEV;
 	}
@@ -772,7 +778,7 @@ static int __init cros_ec_lpc_init(void)
 		return ret;
 	}
 
-	if (!cros_ec_lpc_acpi_device_found) {
+	if (cros_ec_lpc_acpi_device_found < 0) {
 		/* Register the device, and it'll get hooked up automatically */
 		ret = platform_device_register(&cros_ec_lpc_device);
 		if (ret) {
@@ -786,7 +792,7 @@ static int __init cros_ec_lpc_init(void)
 
 static void __exit cros_ec_lpc_exit(void)
 {
-	if (!cros_ec_lpc_acpi_device_found)
+	if (cros_ec_lpc_acpi_device_found < 0)
 		platform_device_unregister(&cros_ec_lpc_device);
 	platform_driver_unregister(&cros_ec_lpc_driver);
 }
-- 
2.45.0

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Tzung-Bi Shih]

On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote:
> Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to
> find the device. This makes it easy to find the AML mutex via the
> ACPI_COMPANION device.
> 
> The name "PNP0C09" is part of the ACPI standard, not Chrome-specific,
> so only recognise the device if the DMI data is recognised too.

I don't quite understand the statement.  Why it needs DMI data?

> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
[...]
> -/* True if ACPI device is present */
> -static bool cros_ec_lpc_acpi_device_found;
> +/*
> + * Index into cros_ec_lpc_acpi_device_ids of ACPI device,
> + * negative for ACPI device not found.
> + */
> +static int cros_ec_lpc_acpi_device_found;

Rename it to `cros_ec_lpc_acpi_device_index` for clarity?

>  static int __init cros_ec_lpc_init(void)
>  {
>  	int ret;
> -	acpi_status status;
>  	const struct dmi_system_id *dmi_match;
>  
> -	status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
> -				  &cros_ec_lpc_acpi_device_found, NULL);
> -	if (ACPI_FAILURE(status))
> -		pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
> +	cros_ec_lpc_acpi_device_found = cros_ec_lpc_find_acpi_dev(
> +		cros_ec_lpc_driver.driver.acpi_match_table);

Or just use `cros_ec_lpc_acpi_device_ids`.

> -	} else if (!cros_ec_lpc_acpi_device_found) {
> +	} else if (cros_ec_lpc_acpi_device_found <= 0) {
> +		/* Standard EC "PNP0C09" not supported without DMI data */

Asked the same question above, why PNP0C09 needs DMI data?  Also the way is
a bit confusing as "PNP0C09" must be at index 0 in the acpi_device_id.

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

> On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote:
>> Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to
>> find the device. This makes it easy to find the AML mutex via the
>> ACPI_COMPANION device.
>>
>> The name "PNP0C09" is part of the ACPI standard, not Chrome-specific,
>> so only recognise the device if the DMI data is recognised too.
>
> I don't quite understand the statement.  Why it needs DMI data?

There are lots of computers with EC chips with ACPI name "PNP0C09"
because it's part of the ACPI standard (for example I have an Intel NUC
with one of these). Most of them don't support the cros_ec protocol, so
the cros_ec driver should ignore these chips. The Framework EC is
unusual in that it's called "PNP0C09" and supports the cros_ec protocol.

Before these patches, the cros_ec code just ignored PNP0C09 because it
wasn't in the match table. The cros_ec_lpc_init logic looked like:

  * dmi_match => ok
  * acpi_name == "GOOG0004" => ok
  * otherwise fail.

After the patch, cros_ec_lpc_init still has this behaviour. We have
"PNP0C09" in the match table so the driver gets hooked up correctly
with the right "ACPI_COMPANION" device, but we don't allow the match
to proceed unless we have the DMI data indicating it's a Framework EC.

>> -	} else if (!cros_ec_lpc_acpi_device_found) {
>> +	} else if (cros_ec_lpc_acpi_device_found <= 0) {
>> +		/* Standard EC "PNP0C09" not supported without DMI data */
>
> Also the way is a bit confusing as "PNP0C09" must be at index 0 in the
> acpi_device_id.

I need some way of saying "will we match PNP0C09?" The table index seems
a simple way of doing it. I could use a strcmp on the table match
instead?

Regarding your other emails, I agree with all your suggestions. Thanks
for reviewing!

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Tzung-Bi Shih]

On Thu, May 23, 2024 at 07:42:00PM +0100, Ben Walsh wrote:
> Tzung-Bi Shih <tzungbi@kernel.org> writes:
> 
> > On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote:
> >> Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to
> >> find the device. This makes it easy to find the AML mutex via the
> >> ACPI_COMPANION device.
> >>
> >> The name "PNP0C09" is part of the ACPI standard, not Chrome-specific,
> >> so only recognise the device if the DMI data is recognised too.
> >
> > I don't quite understand the statement.  Why it needs DMI data?
> 
> There are lots of computers with EC chips with ACPI name "PNP0C09"
> because it's part of the ACPI standard (for example I have an Intel NUC
> with one of these). Most of them don't support the cros_ec protocol, so
> the cros_ec driver should ignore these chips. The Framework EC is
> unusual in that it's called "PNP0C09" and supports the cros_ec protocol.
> 
> Before these patches, the cros_ec code just ignored PNP0C09 because it
> wasn't in the match table. The cros_ec_lpc_init logic looked like:
> 
>   * dmi_match => ok
>   * acpi_name == "GOOG0004" => ok
>   * otherwise fail.
> 
> After the patch, cros_ec_lpc_init still has this behaviour. We have
> "PNP0C09" in the match table so the driver gets hooked up correctly
> with the right "ACPI_COMPANION" device, but we don't allow the match
> to proceed unless we have the DMI data indicating it's a Framework EC.

From the context you provided, instead of matching "PNP0C09" in the driver,
it makes more sense to me (for Framework EC):

* Mainly use DMI match.
* Add a quirk for looking up (acpi_get_devices()?) and binding
  (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe().

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

> From the context you provided, instead of matching "PNP0C09" in the driver,
> it makes more sense to me (for Framework EC):
>
> * Mainly use DMI match.
> * Add a quirk for looking up (acpi_get_devices()?) and binding
>   (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe().

Sorry, I don't think I provided enough context. There is already a
platform device /sys/bus/platform/devices/PNP0C09:00 with a companion
acpi device /sys/bus/acpi/devices/PNP0C09:00. I think it makes sense to
bind the driver to the existing platform device.

I could add a new quirk which provides an alternative ACPI match table
to be used instead of the default. In the default case the match_table
will contain only "GOOG0004" as before. But in the Framework EC case the
match table will be "PNP0C09".

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Dustin Howett]

On Fri, May 24, 2024 at 1:35 PM Ben Walsh <ben@jubnut.com> wrote:
>
> I could add a new quirk which provides an alternative ACPI match table
> to be used instead of the default. In the default case the match_table
> will contain only "GOOG0004" as before. But in the Framework EC case the
> match table will be "PNP0C09".

My biggest concern with putting PNP0C09 in the direct match table is
that it would cause cros_ec_lpcs to be loaded for *all* devices with
an ACPI-compatible embedded controller; it would likely print an error
and bail out early on, but it would still be unnecessary on 99% of
platforms.

This is my misunderstanding the driver model, but is it possible to
add a second companion device?
d

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Ben Walsh]

Dustin Howett <dustin@howett.net> writes:

> On Fri, May 24, 2024 at 1:35 PM Ben Walsh <ben@jubnut.com> wrote:
>>
>> I could add a new quirk which provides an alternative ACPI match table
>> to be used instead of the default. In the default case the match_table
>> will contain only "GOOG0004" as before. But in the Framework EC case the
>> match table will be "PNP0C09".
>
> My biggest concern with putting PNP0C09 in the direct match table is
> that it would cause cros_ec_lpcs to be loaded for *all* devices with
> an ACPI-compatible embedded controller; it would likely print an error
> and bail out early on, but it would still be unnecessary on 99% of
> platforms.

That's exactly what we're talking about: how to *avoid* putting PNP0C09
in the match table.

My original idea was to put it in the match table, but only allow a
match to proceed if Framework EC is detected by DMI.

My new idea is to not to put it in the match table *at all*, but to
provide a new match table if Framework EC is detected by DMI.

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Tzung-Bi Shih]

On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote:
> Tzung-Bi Shih <tzungbi@kernel.org> writes:
> 
> > From the context you provided, instead of matching "PNP0C09" in the driver,
> > it makes more sense to me (for Framework EC):
> >
> > * Mainly use DMI match.
> > * Add a quirk for looking up (acpi_get_devices()?) and binding
> >   (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe().
> 
> Sorry, I don't think I provided enough context. There is already a
> platform device /sys/bus/platform/devices/PNP0C09:00 with a companion
> acpi device /sys/bus/acpi/devices/PNP0C09:00. I think it makes sense to
> bind the driver to the existing platform device.
> 
> I could add a new quirk which provides an alternative ACPI match table
> to be used instead of the default. In the default case the match_table
> will contain only "GOOG0004" as before. But in the Framework EC case the
> match table will be "PNP0C09".

I think it doesn't work as the current quirk is handling in
cros_ec_lpc_probe() which is after matching.

My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe()
via any lookup API?  If yes, it could still use DMI match and get `adev` if
required.

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

> On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote:

>> I could add a new quirk which provides an alternative ACPI match table
>> to be used instead of the default. In the default case the match_table
>> will contain only "GOOG0004" as before. But in the Framework EC case the
>> match table will be "PNP0C09".
>
> I think it doesn't work as the current quirk is handling in
> cros_ec_lpc_probe() which is after matching.

I was thinking of a new quirk called CROS_EC_LPC_QUIRK_ACPI_MATCH, and
putting it in cros_ec_lpc_init(), not cros_ec_lpc_probe(). Do we have to
do all quirk handling in cros_ec_lpc_probe()?

> My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe()
> via any lookup API?  If yes, it could still use DMI match and get `adev` if
> required.

That works; I've tested it.

In this scenario we're not using the existing PNP0C09 platform device,
which means I can't look at
/sys/bus/acpi/devices/PNP0C09\:00/physical_node/driver and see the
driver. Is this OK?

(Note that ACPI_COMPANION_SET() doesn't fix this. You can use
acpi_bind_one() but that seems more like internal plumbing).

Re: [PATCH 5/6] platform/chrome: cros_ec_lpc: Correct ACPI name for Framework Laptop

[Tzung-Bi Shih]

On Mon, May 27, 2024 at 07:06:40PM +0100, Ben Walsh wrote:
> Tzung-Bi Shih <tzungbi@kernel.org> writes:
> 
> > On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote:
> 
> >> I could add a new quirk which provides an alternative ACPI match table
> >> to be used instead of the default. In the default case the match_table
> >> will contain only "GOOG0004" as before. But in the Framework EC case the
> >> match table will be "PNP0C09".
> >
> > I think it doesn't work as the current quirk is handling in
> > cros_ec_lpc_probe() which is after matching.
> 
> I was thinking of a new quirk called CROS_EC_LPC_QUIRK_ACPI_MATCH, and
> putting it in cros_ec_lpc_init(), not cros_ec_lpc_probe(). Do we have to
> do all quirk handling in cros_ec_lpc_probe()?

No, but there is already code in cros_ec_lpc_probe() for handling quirks and
we would like to reuse them if we could.

Also a possible issue: MODULE_DEVICE_TABLE() wouldn't work if the table changes
during runtime.

> 
> > My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe()
> > via any lookup API?  If yes, it could still use DMI match and get `adev` if
> > required.
> 
> That works; I've tested it.
> 
> In this scenario we're not using the existing PNP0C09 platform device,
> which means I can't look at
> /sys/bus/acpi/devices/PNP0C09\:00/physical_node/driver and see the
> driver. Is this OK?
> 
> (Note that ACPI_COMPANION_SET() doesn't fix this. You can use
> acpi_bind_one() but that seems more like internal plumbing).

As long as ACPI_COMPANION() in the driver can get the correct `adev`, I guess
it's fine.  Otherwise, put the `adev` to somewhere device specific (e.g.
struct cros_ec_lpc) should also work.

[PATCH 6/6] platform/chrome: cros_ec_lpc: Add AML mutex for Framework Laptop

[Ben Walsh]

For Framework Laptops with Microchip EC (MEC), use the AML mutex
"ECMT" to protect EC memory access.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index a9200f0a1a37..eebc66a39a23 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -611,6 +611,11 @@ static const struct lpc_driver_data framework_laptop_amd_lpc_driver_data __initc
 	.quirk_mmio_memory_base = 0xE00,
 };
 
+static const struct lpc_driver_data framework_laptop_11_lpc_driver_data __initconst = {
+	.quirks = CROS_EC_LPC_QUIRK_AML_MUTEX,
+	.quirk_aml_mutex_name = "ECMT",
+};
+
 static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
 	{
 		/*
@@ -679,6 +684,7 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Laptop"),
 		},
+		.driver_data = (void *)&framework_laptop_11_lpc_driver_data,
 	},
 	{ /* sentinel */ }
 };
-- 
2.45.0