Refactor host command interface to support version 3 packets

This will fix EC flash commands on pit, once the host side (u-boot and
cros_ec driver) are upgraded to match.

This change is backwards-compatible the EC still supports the existing
version 2 protocols for talking to existing AP/kernel/ectool.

Once the AP-side supports version 3 for SPI (and existing systems are
upgraded), we will remove older SPI support since we haven't shipped a
product which uses SPI.

BUG=chrome-os-partner:20257
BRANCH=none
TEST=disable cros_ec driver support in ectool; 'ectool hello' works on link
    And with an old ectool which predates this CL, 'ectool hello' also works.
    On pit, from u-boot prompt, 'crosec test' and 'crosec version' work, and
    keyboard works.

Change-Id: I01f193e316e9aa442fe50d632dc8a4681723e282
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/58908
Reviewed-by: Simon Glass <sjg@chromium.org>
Commit-Queue: Doug Anderson <dianders@chromium.org>

6 files changed, 578 insertions, 18 deletions

diff --git a/chip/lm4/lpc.c b/chip/lm4/lpc.c
index b01bee60d5..90a912c6f4 100644
--- a/chip/lm4/lpc.c
+++ b/chip/lm4/lpc.c
@@ -62,11 +62,12 @@ static uint8_t acpi_mem_test;    /* Test byte in ACPI memory space */
 
 static uint32_t host_events;     /* Currently pending SCI/SMI events */
 static uint32_t event_mask[3];   /* Event masks for each type */
+static struct host_packet lpc_packet;
 static struct host_cmd_handler_args host_cmd_args;
 static uint8_t host_cmd_flags;   /* Flags from host command */
 
 /* Params must be 32-bit aligned */
-static uint8_t params_copy[EC_HOST_PARAM_SIZE] __attribute__((aligned(4)));
+static uint8_t params_copy[EC_HOST_PACKET_SIZE] __attribute__((aligned(4)));
 static int init_done;
 
 static uint8_t * const cmd_params = (uint8_t *)LPC_POOL_CMD_DATA +
@@ -222,6 +223,28 @@ static void lpc_send_response(struct host_cmd_handler_args *args)
 	task_enable_irq(LM4_IRQ_LPC);
 }
 
+static void lpc_send_response_packet(struct host_packet *pkt)
+{
+	/* Ignore in-progress on LPC since interface is synchronous anyway */
+	if (pkt->driver_result == EC_RES_IN_PROGRESS)
+		return;
+
+	/*
+	 * Write result to the data byte.  This sets the TOH bit in the
+	 * status byte and triggers an IRQ on the host so the host can read
+	 * the result.
+	 *
+	 * TODO: (crosbug.com/p/7496) or it would, if we actually set up host
+	 * IRQs
+	 */
+	LPC_POOL_CMD[1] = pkt->driver_result;
+
+	/* Clear the busy bit */
+	task_disable_irq(LM4_IRQ_LPC);
+	LM4_LPC_ST(LPC_CH_CMD) &= ~LM4_LPC_ST_BUSY;
+	task_enable_irq(LM4_IRQ_LPC);
+}
+
 int lpc_keyboard_has_char(void)
 {
 	return (LM4_LPC_ST(LPC_CH_KEYBOARD) & LM4_LPC_ST_TOH) ? 1 : 0;
@@ -483,8 +506,25 @@ static void handle_host_write(int is_cmd)
 	host_cmd_flags = lpc_host_args->flags;
 
 	/* See if we have an old or new style command */
-	if (host_cmd_flags & EC_HOST_ARGS_FLAG_FROM_HOST) {
-		/* New style command */
+	if (host_cmd_args.command == EC_COMMAND_PROTOCOL_3) {
+		lpc_packet.send_response = lpc_send_response_packet;
+
+		lpc_packet.request = (const void *)LPC_POOL_CMD_DATA;
+		lpc_packet.request_temp = params_copy;
+		lpc_packet.request_max = sizeof(params_copy);
+		/* Don't know the request size so pass in the entire buffer */
+		lpc_packet.request_size = EC_HOST_PACKET_SIZE;
+
+		lpc_packet.response = (void *)LPC_POOL_CMD_DATA;
+		lpc_packet.response_max = EC_HOST_PACKET_SIZE;
+		lpc_packet.response_size = 0;
+
+		lpc_packet.driver_result = EC_RES_SUCCESS;
+		host_packet_receive(&lpc_packet);
+		return;
+
+	} else if (host_cmd_flags & EC_HOST_ARGS_FLAG_FROM_HOST) {
+		/* Version 2 (link) style command */
 		int size = lpc_host_args->data_size;
 		int csum, i;
 
@@ -761,9 +801,10 @@ static void lpc_init(void)
 	memset(lpc_host_args, 0, sizeof(*lpc_host_args));
 	memset(lpc_get_memmap_range(), 0, EC_MEMMAP_SIZE);
 
-	/* We support LPC args */
+	/* We support LPC args and version 3 protocol */
 	*(lpc_get_memmap_range() + EC_MEMMAP_HOST_CMD_FLAGS) =
-		EC_HOST_CMD_FLAG_LPC_ARGS_SUPPORTED;
+		EC_HOST_CMD_FLAG_LPC_ARGS_SUPPORTED |
+		EC_HOST_CMD_FLAG_VERSION_3;
 
 	/* Enable LPC interrupt */
 	task_enable_irq(LM4_IRQ_LPC);
diff --git a/chip/stm32/spi.c b/chip/stm32/spi.c
index f488eee801..51a8e50cfd 100644
--- a/chip/stm32/spi.c
+++ b/chip/stm32/spi.c
@@ -52,6 +52,8 @@ enum {
  * We allow a preamble and a header byte so that SPI can function at all.
  * We also add an 8-bit length so that we can tell that we got the whole
  * message, since the master decides how many bytes to read.
+ *
+ * TODO: move these constants to ec_commands.h
  */
 enum {
 	/* The bytes which appear before the header in a message */
@@ -85,6 +87,24 @@ static uint8_t in_msg[EC_HOST_PARAM_SIZE + SPI_MSG_PROTO_LEN];
 static uint8_t active;
 static uint8_t enabled;
 static struct host_cmd_handler_args args;
+static struct host_packet spi_packet;
+
+/*
+ * The AP blindly clocks back bytes over the SPI interface looking for the
+ * header bytes.  So this preamble must always precede the actual response
+ * packet.  The preamble must be 32-bit aligned so that the response buffer is
+ * also 32-bit aligned.
+ *
+ * Really, only HEADER_BYTE2 matters, since the SPI driver ignores everything
+ * until it sees that byte code.  Search for "spi-frame-header" in U-boot to
+ * see how that's implemented.
+ */
+static const uint8_t out_preamble[4] = {
+	SPI_MSG_PREAMBLE_BYTE,
+	SPI_MSG_PREAMBLE_BYTE,
+	SPI_MSG_HEADER_BYTE1,
+	SPI_MSG_HEADER_BYTE2,  /* This is the byte which matters */
+};
 
 /**
  * Wait until we have received a certain number of bytes
@@ -249,6 +269,28 @@ static void spi_send_response(struct host_cmd_handler_args *args)
 }
 
 /**
+ * Called to send a response back to the host.
+ *
+ * Some commands can continue for a while. This function is called by
+ * host_command when it completes.
+ *
+ */
+static void spi_send_response_packet(struct host_packet *pkt)
+{
+	struct dma_channel *txdma;
+
+	/* If we are too late, don't bother */
+	if (!active)
+		return;
+
+	/* Transmit the reply */
+	txdma = dma_get_channel(DMAC_SPI1_TX);
+	dma_prepare_tx(&dma_tx_option,
+		       sizeof(out_preamble) + pkt->response_size, out_msg);
+	dma_go(txdma);
+}
+
+/**
  * Handle an event on the NSS pin
  *
  * A falling edge of NSS indicates that the master is starting a new
@@ -286,17 +328,73 @@ void spi_event(enum gpio_signal signal)
 		return;
 	}
 
-	if (in_msg[0] >= EC_CMD_VERSION0) {
+	if (in_msg[0] == EC_HOST_REQUEST_VERSION) {
+		/* Protocol version 3 */
+		struct ec_host_request *r = (struct ec_host_request *)in_msg;
+		int pkt_size;
+
+		/* Wait for the rest of the command header */
+		if (wait_for_bytes(rxdma, sizeof(*r), nss_reg, nss_mask)) {
+			setup_for_transaction();
+			return;
+		}
+
+		/*
+		 * Check how big the packet should be.  We can't just wait to
+		 * see how much data the host sends, because it will keep
+		 * sending dummy data until we respond.
+		 */
+		pkt_size = host_request_expected_size(r);
+		if (pkt_size == 0 || pkt_size > sizeof(in_msg)) {
+			setup_for_transaction();
+			return;
+		}
+
+		/* Wait for the packet data */
+		if (wait_for_bytes(rxdma, pkt_size, nss_reg, nss_mask)) {
+			setup_for_transaction();
+			return;
+		}
+
+		spi_packet.send_response = spi_send_response_packet;
+
+		spi_packet.request = in_msg;
+		spi_packet.request_temp = NULL;
+		spi_packet.request_max = sizeof(in_msg);
+		spi_packet.request_size = pkt_size;
+
+		/* Response must start with the preamble */
+		memcpy(out_msg, out_preamble, sizeof(out_preamble));
+		spi_packet.response = out_msg + sizeof(out_preamble);
+		spi_packet.response_max =
+			sizeof(out_msg) - sizeof(out_preamble);
+		spi_packet.response_size = 0;
+
+		spi_packet.driver_result = EC_RES_SUCCESS;
+
+		host_packet_receive(&spi_packet);
+		return;
+
+	} else if (in_msg[0] >= EC_CMD_VERSION0) {
+		/*
+		 * Protocol version 2
+		 *
+		 * TODO: remove once all systems upgraded to version 3 */
 		args.version = in_msg[0] - EC_CMD_VERSION0;
 		args.command = in_msg[1];
 		args.params_size = in_msg[2];
 		args.params = in_msg + 3;
 	} else {
+		/*
+		 * Protocol version 1
+		 *
+		 * TODO: remove; nothing sends this.  Ignore this packet?
+		 * Send back an error response?
+		 */
 		args.version = 0;
 		args.command = in_msg[0];
 		args.params = in_msg + 1;
 		args.params_size = 0;
-		args.version = 0;
 	}
 
 	/* Wait for parameters */
diff --git a/common/host_command.c b/common/host_command.c
index 0563e5b023..bf310991b6 100644
--- a/common/host_command.c
+++ b/common/host_command.c
@@ -132,6 +132,189 @@ void host_command_received(struct host_cmd_handler_args *args)
 	host_send_response(args);
 }
 
+/* TODO(rspangler): less awful names. */
+static struct host_cmd_handler_args args0;
+static struct host_packet *pkt0;
+
+void host_packet_respond(struct host_cmd_handler_args *args)
+{
+	struct ec_host_response *r = (struct ec_host_response *)pkt0->response;
+	uint8_t *out = (uint8_t *)pkt0->response;
+	int csum = 0;
+	int i;
+
+	/* Clip result size to what we can accept */
+	if (args->result) {
+		/* Error results don't have data */
+		args->response_size = 0;
+	} else if (args->response_size > pkt0->response_max - sizeof(*r)) {
+		/* Too much data */
+		args->result = EC_RES_RESPONSE_TOO_BIG;
+		args->response_size = 0;
+	}
+
+	/* Fill in response struct */
+	r->struct_version = EC_HOST_RESPONSE_VERSION;
+	r->checksum = 0;
+	r->result = args->result;
+	r->data_len = args->response_size;
+	r->reserved = 0;
+
+	/* Start checksum; this also advances *out to end of response */
+	for (i = sizeof(*r); i > 0; i--)
+		csum += *out++;
+
+	/* Checksum and copy response data, if any */
+	if (!args->response_size) {
+		/* No data to copy */
+	} else if (args->response != out) {
+		/* Copy and checksum */
+		const uint8_t *outr = (const uint8_t *)args->response;
+
+		for (i = args->response_size; i > 0; i--) {
+			*out = *outr++;
+			csum += *out++;
+		}
+	} else {
+		/* Response already in right place; just checksum it */
+		for (i = args->response_size; i > 0; i--)
+			csum += *out++;
+	}
+
+	/* Write checksum field so the entire packet sums to 0 */
+	r->checksum = (uint8_t)(-csum);
+
+	pkt0->response_size = sizeof(*r) + r->data_len;
+	pkt0->driver_result = args->result;
+	pkt0->send_response(pkt0);
+}
+
+int host_request_expected_size(const struct ec_host_request *r)
+{
+	/* Check host request version */
+	if (r->struct_version != EC_HOST_REQUEST_VERSION)
+		return 0;
+
+	/* Reserved byte should be 0 */
+	/* TODO: maybe we should have a header checksum instead? */
+	if (r->reserved)
+		return 0;
+
+	return sizeof(*r) + r->data_len;
+}
+
+void host_packet_receive(struct host_packet *pkt)
+{
+	const struct ec_host_request *r =
+		(const struct ec_host_request *)pkt->request;
+	const uint8_t *in = (const uint8_t *)pkt->request;
+	uint8_t *itmp = (uint8_t *)pkt->request_temp;
+	int csum = 0;
+	int i;
+
+	/* Track the packet we're handling */
+	pkt0 = pkt;
+
+	/* If driver indicates error, don't even look at the data */
+	if (pkt->driver_result) {
+		args0.result = pkt->driver_result;
+		goto host_packet_bad;
+	}
+
+	if (pkt->request_size < sizeof(*r)) {
+		/* Packet too small for even a header */
+		args0.result = EC_RES_REQUEST_TRUNCATED;
+		goto host_packet_bad;
+	}
+
+	if (pkt->request_size > pkt->request_max) {
+		/* Got a bigger request than the interface can handle */
+		args0.result = EC_RES_REQUEST_TRUNCATED;
+		goto host_packet_bad;
+	}
+
+	/*
+	 * Response buffer needs to be big enough for a header.  If it's not
+	 * we can't even return an error packet.
+	 */
+	ASSERT(pkt->response_max >= sizeof(struct ec_host_response));
+
+	/* Start checksum and copy request header if necessary */
+	if (pkt->request_temp) {
+		/* Copy to temp buffer and checksum */
+		for (i = sizeof(*r); i > 0; i--) {
+			*itmp = *in++;
+			csum += *itmp++;
+		}
+		r = (const struct ec_host_request *)pkt->request_temp;
+	} else {
+		/* Just checksum */
+		for (i = sizeof(*r); i > 0; i--)
+			csum += *in++;
+	}
+
+	if (r->struct_version != EC_HOST_REQUEST_VERSION) {
+		/* Request header we don't know how to handle */
+		args0.result = EC_RES_INVALID_HEADER;
+		goto host_packet_bad;
+	}
+
+	if (pkt->request_size < sizeof(*r) + r->data_len) {
+		/*
+		 * Packet too small for expected params.  Note that it's ok if
+		 * the received packet data is too big; some interfaces may pad
+		 * the data at the end (SPI) or may not know how big the
+		 * received data is (LPC).
+		 */
+		args0.result = EC_RES_REQUEST_TRUNCATED;
+		goto host_packet_bad;
+	}
+
+	/* Copy request data and validate checksum */
+	if (pkt->request_temp) {
+		/* Params go in temporary buffer */
+		args0.params = itmp;
+
+		/* Copy request data and checksum */
+		for (i = r->data_len; i > 0; i--) {
+			*itmp = *in++;
+			csum += *itmp++;
+		}
+	} else {
+		/* Params read directly from request */
+		args0.params = in;
+
+		/* Just checksum */
+		for (i = r->data_len; i > 0; i--)
+			csum += *in++;
+	}
+
+	/* Validate checksum */
+	if ((uint8_t)csum) {
+		args0.result = EC_RES_INVALID_CHECKSUM;
+		goto host_packet_bad;
+	}
+
+	/* Set up host command handler args */
+	args0.send_response = host_packet_respond;
+	args0.command = r->command;
+	args0.version = r->command_version;
+	args0.params_size = r->data_len;
+	args0.response = (struct ec_host_response *)(pkt->response) + 1;
+	args0.response_max = pkt->response_max -
+		sizeof(struct ec_host_response);
+	args0.response_size = 0;
+	args0.result = EC_RES_SUCCESS;
+
+	/* Chain to host command received */
+	host_command_received(&args0);
+	return;
+
+host_packet_bad:
+	/* Improperly formed packet from host, so send an error response */
+	host_packet_respond(&args0);
+}
+
 /**
  * Find a command by command number.
  *
diff --git a/include/ec_commands.h b/include/ec_commands.h
index a2cc70ca8e..d17bb444d9 100644
--- a/include/ec_commands.h
+++ b/include/ec_commands.h
@@ -28,6 +28,7 @@
  */
 
 /* Current version of this protocol */
+/* TODO: This is effectively useless; protocol is determined in other ways */
 #define EC_PROTO_VERSION          0x00000002
 
 /* Command version mask */
@@ -44,7 +45,9 @@
 /* I/O addresses for host command args and params */
 #define EC_LPC_ADDR_HOST_ARGS  0x800    /* and 0x801, 0x802, 0x803 */
 #define EC_LPC_ADDR_HOST_PARAM 0x804
-#define EC_HOST_PARAM_SIZE     0x0fc  /* Size of param area in bytes */
+#define EC_HOST_PARAM_SIZE     0x0fc   /* Size of param area in bytes */
+#define EC_LPC_ADDR_HOST_PACKET 0x800  /* Offset of version 3 packet */
+#define EC_HOST_PACKET_SIZE     0x100  /* Max size of version 3 packet */
 
 /* The actual block is 0x800-0x8ff, but some BIOSes think it's 0x880-0x8ff
  * and they tell the kernel that so we have to think of it as two parts. */
@@ -134,6 +137,8 @@
 /* Host command interface flags */
 /* Host command interface supports LPC args (LPC interface only) */
 #define EC_HOST_CMD_FLAG_LPC_ARGS_SUPPORTED  0x01
+/* Host command interface supports version 3 protocol */
+#define EC_HOST_CMD_FLAG_VERSION_3   0x02
 
 /* Wireless switch flags */
 #define EC_WIRELESS_SWITCH_WLAN      0x01
@@ -194,6 +199,9 @@ enum ec_status {
 	EC_RES_UNAVAILABLE = 9,		/* No response available */
 	EC_RES_TIMEOUT = 10,		/* We got a timeout */
 	EC_RES_OVERFLOW = 11,		/* Table / data overflow */
+	EC_RES_INVALID_HEADER = 12,     /* Header contains invalid data */
+	EC_RES_REQUEST_TRUNCATED = 13,  /* Didn't get the entire request */
+	EC_RES_RESPONSE_TOO_BIG = 14    /* Response was too big to handle */
 };
 
 /*
@@ -275,6 +283,68 @@ struct ec_lpc_host_args {
  */
 #define EC_HOST_ARGS_FLAG_TO_HOST   0x02
 
+/*****************************************************************************/
+
+/*
+ * Value written to legacy command port / prefix byte to indicate protocol
+ * 3+ structs are being used.  Usage is bus-dependent.
+ */
+#define EC_COMMAND_PROTOCOL_3 0xda
+
+#define EC_HOST_REQUEST_VERSION 3
+
+/* Version 3 request from host */
+struct ec_host_request {
+	/* Struct version (=3)
+	 *
+	 * EC will return EC_RES_INVALID_HEADER if it receives a header with a
+	 * version it doesn't know how to parse.
+	 */
+	uint8_t struct_version;
+
+	/*
+	 * Checksum of request and data; sum of all bytes including checksum
+	 * should total to 0.
+	 */
+	uint8_t checksum;
+
+	/* Command code */
+	uint16_t command;
+
+	/* Command version */
+	uint8_t command_version;
+
+	/* Unused byte in current protocol version; set to 0 */
+	uint8_t reserved;
+
+	/* Length of data which follows this header */
+	uint16_t data_len;
+} __packed;
+
+#define EC_HOST_RESPONSE_VERSION 3
+
+/* Version 3 response from EC */
+struct ec_host_response {
+	/* Struct version (=3) */
+	uint8_t struct_version;
+
+	/*
+	 * Checksum of response and data; sum of all bytes including checksum
+	 * should total to 0.
+	 */
+	uint8_t checksum;
+
+	/* Result code (EC_RES_*) */
+	uint16_t result;
+
+	/* Length of data which follows this header */
+	uint16_t data_len;
+
+	/* Unused bytes in current protocol version; set to 0 */
+	uint16_t reserved;
+} __packed;
+
+/*****************************************************************************/
 /*
  * Notes on commands:
  *
diff --git a/include/host_command.h b/include/host_command.h
index cc14d672af..4c994463f6 100644
--- a/include/host_command.h
+++ b/include/host_command.h
@@ -19,10 +19,12 @@ struct host_cmd_handler_args {
 	 * send the response back to the host.
 	 */
 	void (*send_response)(struct host_cmd_handler_args *args);
-	uint8_t command;       /* Command (e.g., EC_CMD_FLASH_GET_INFO) */
+	uint16_t command;      /* Command (e.g., EC_CMD_FLASH_GET_INFO) */
 	uint8_t version;       /* Version of command (0-31) */
-	uint8_t params_size;   /* Size of input parameters in bytes */
+
 	const void *params; /* Input parameters */
+	uint16_t params_size;  /* Size of input parameters in bytes */
+
 	/*
 	 * Pointer to output response data buffer.  On input to the handler,
 	 * points to a buffer of size response_max.  Command handler can change
@@ -35,8 +37,8 @@ struct host_cmd_handler_args {
 	 * handler changes response to point to its own larger buffer, it may
 	 * return a response_size greater than response_max.
 	 */
-	uint8_t response_max;
-	uint8_t response_size; /* Size of data pointed to by resp_ptr */
+	uint16_t response_max;
+	uint16_t response_size; /* Size of data pointed to by response */
 
 	/*
 	 * This is the result returned by command and therefore the status to
@@ -50,6 +52,53 @@ struct host_cmd_handler_args {
 	enum ec_status result;
 };
 
+/* Args for host packet handler */
+struct host_packet {
+	/*
+	 * The driver that receives the command sets up the send_response()
+	 * handler. Once the command is processed this handler is called to
+	 * send the response back to the host.
+	 */
+	void (*send_response)(struct host_packet *pkt);
+
+	/* Input request data */
+	const void *request;
+
+	/*
+	 * Input request temp buffer.  If this is non-null, the data has not
+	 * been copied from here into the request buffer yet.  The host command
+	 * handler should do so while verifying the command.  The interface
+	 * can't do it, because it doesn't know how much to copy.
+	 */
+	void *request_temp;
+
+	/*
+	 * Maximum size of request the interface can handle, in bytes.  The
+	 * buffers pointed to by *request and *request_temp must be at least
+	 * this big.
+	 */
+	uint16_t request_max;
+
+	/* Size of input request data, in bytes */
+	uint16_t request_size;
+
+	/* Pointer to output response data buffer */
+	void *response;
+
+	/* Maximum size of response buffer provided to command handler */
+	uint16_t response_max;
+
+	/* Size of output response data, in bytes */
+	uint16_t response_size;
+
+	/*
+	 * Error from driver; if this is non-zero, host command handler will
+	 * return a properly formatted error response packet rather than
+	 * calling a command handler.
+	 */
+	enum ec_status driver_result;
+};
+
 /* Host command */
 struct host_command {
 	/*
@@ -132,6 +181,23 @@ void host_send_response(struct host_cmd_handler_args *args);
  */
 void host_command_received(struct host_cmd_handler_args *args);
 
+/**
+ * Return the expected host packet size given its header.
+ *
+ * Also does some sanity checking on the host request.
+ *
+ * @param r		Host request header
+ * @return The expected packet size, or 0 if error.
+ */
+int host_request_expected_size(const struct ec_host_request *r);
+
+/**
+ * Handle a received host packet.
+ *
+ * @param packet	Host packet args
+ */
+void host_packet_receive(struct host_packet *pkt);
+
 /* Register a host command handler */
 #define DECLARE_HOST_COMMAND(command, routine, version_mask)		\
 	const struct host_command __host_cmd_##command			\
diff --git a/util/comm-lpc.c b/util/comm-lpc.c
index 3548f4eb29..c6ddee93c5 100644
--- a/util/comm-lpc.c
+++ b/util/comm-lpc.c
@@ -126,6 +126,99 @@ static int ec_command_lpc(int command, int version,
 	return args.data_size;
 }
 
+static int ec_command_lpc_3(int command, int version,
+			  const void *outdata, int outsize,
+			  void *indata, int insize)
+{
+	struct ec_host_request rq;
+	struct ec_host_response rs;
+	const uint8_t *d;
+	uint8_t *dout;
+	int csum = 0;
+	int i;
+
+	/* Fail if output size is too big */
+	if (outsize + sizeof(rq) > EC_HOST_PACKET_SIZE)
+		return -EC_RES_REQUEST_TRUNCATED;
+
+	/* Fill in request packet */
+	/* TODO: this should be common to all protocols */
+	rq.struct_version = EC_HOST_REQUEST_VERSION;
+	rq.checksum = 0;
+	rq.command = command;
+	rq.command_version = version;
+	rq.reserved = 0;
+	rq.data_len = outsize;
+
+	/* Copy data and start checksum */
+	for (i = 0, d = (const uint8_t *)outdata; i < outsize; i++, d++) {
+		outb(*d, EC_LPC_ADDR_HOST_PACKET + sizeof(rq) + i);
+		csum += *d;
+	}
+
+	/* Finish checksum */
+	for (i = 0, d = (const uint8_t *)&rq; i < sizeof(rq); i++, d++)
+		csum += *d;
+
+	/* Write checksum field so the entire packet sums to 0 */
+	rq.checksum = (uint8_t)(-csum);
+
+	/* Copy header */
+	for (i = 0, d = (const uint8_t *)&rq; i < sizeof(rq); i++, d++)
+		outb(*d, EC_LPC_ADDR_HOST_PACKET + i);
+
+	/* Start the command */
+	outb(EC_COMMAND_PROTOCOL_3, EC_LPC_ADDR_HOST_CMD);
+
+	if (wait_for_ec(EC_LPC_ADDR_HOST_CMD, 1000000)) {
+		fprintf(stderr, "Timeout waiting for EC response\n");
+		return -EC_RES_ERROR;
+	}
+
+	/* Check result */
+	i = inb(EC_LPC_ADDR_HOST_DATA);
+	if (i) {
+		fprintf(stderr, "EC returned error result code %d\n", i);
+		return -i;
+	}
+
+	/* Read back response header and start checksum */
+	csum = 0;
+	for (i = 0, dout = (uint8_t *)&rs; i < sizeof(rs); i++, dout++) {
+		*dout = inb(EC_LPC_ADDR_HOST_PACKET + i);
+		csum += *dout;
+	}
+
+	if (rs.struct_version != EC_HOST_RESPONSE_VERSION) {
+		fprintf(stderr, "EC response version mismatch\n");
+		return -EC_RES_INVALID_RESPONSE;
+	}
+
+	if (rs.reserved) {
+		fprintf(stderr, "EC response reserved != 0\n");
+		return -EC_RES_INVALID_RESPONSE;
+	}
+
+	if (rs.data_len > insize) {
+		fprintf(stderr, "EC returned too much data\n");
+		return -EC_RES_RESPONSE_TOO_BIG;
+	}
+
+	/* Read back data and update checksum */
+	for (i = 0, dout = (uint8_t *)indata; i < rs.data_len; i++, dout++) {
+		*dout = inb(EC_LPC_ADDR_HOST_PACKET + sizeof(rs) + i);
+		csum += *dout;
+	}
+
+	/* Verify checksum */
+	if ((uint8_t)csum) {
+		fprintf(stderr, "EC response has invalid checksum\n");
+		return -EC_RES_INVALID_CHECKSUM;
+	}
+
+	/* Return actual amount of data received */
+	return rs.data_len;
+}
 
 static int ec_readmem_lpc(int offset, int bytes, void *dest)
 {
@@ -194,15 +287,24 @@ int comm_init_lpc(void)
 	 * in args when it responds.
 	 */
 	if (inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID) != 'E' ||
-	    inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID + 1) != 'C' ||
-	    !(inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_HOST_CMD_FLAGS) &
-	      EC_HOST_CMD_FLAG_LPC_ARGS_SUPPORTED)) {
-		fprintf(stderr, "EC doesn't support command args.\n");
+	    inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID + 1) != 'C') {
+		fprintf(stderr, "Missing Chromium EC memory map.\n");
+		return -5;
+	}
+
+	/* Check which command version we'll use */
+	i = inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_HOST_CMD_FLAGS);
+
+	if (i & EC_HOST_CMD_FLAG_VERSION_3) {
+		ec_command = ec_command_lpc_3;
+	} else if (i & EC_HOST_CMD_FLAG_LPC_ARGS_SUPPORTED) {
+		ec_command = ec_command_lpc;
+	} else {
+		fprintf(stderr, "EC doesn't support protocols we need.\n");
 		return -5;
 	}
 
-	/* Okay, this works */
-	ec_command = ec_command_lpc;
+	/* Either one supports reading mapped memory directly */
 	ec_readmem = ec_readmem_lpc;
 	return 0;
 }