Hello Brian Nemec,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to review the following change.
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
raiden_debug_spi.c: Refactor to support multiple protocols
Refactoring of the USB SPI protocol to support multiple protocols. The changes include:
* Setting of USB SPI limits based on the device's USB protocol
* Creation of context states to keep track of bytes written to and read from the device and individual USB packets.
* Creation of helper functions for handling transfer of data between the USB packets and the data buffers.
* Removal of magic numbers on the USB return codes. Some renaming of to make the protocol version clear.
BUG=b:139058552 BRANCH=none TEST=Manual testing of ServoMicro and Flashrom when performing reads, writes, and verification of the EC firmware on Nami. TEST=Builds
Signed-off-by: Brian J. Nemec bnemec@chromium.com Change-Id: Id404af14e55fa0884e29f28880206aaad4deba66 --- M raiden_debug_spi.c 1 file changed, 370 insertions(+), 166 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/1
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index ac74c0b..b08d9e0 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -42,28 +42,37 @@ * * https://chromium.googlesource.com/chromiumos/platform/ec * - * The protocol for the USB-SPI bridge is documented in the following file in - * that respository: + * The protocol for the USB-SPI bridge is implemented in the following files + * in that repository: * - * chip/stm32/usb_spi.c + * include/usb_spi.h + * common/usb_spi.c * - * Version 1: - * SPI transactions of up to 62B in each direction with every command having - * a response. The initial packet from host contains a 2B header indicating - * write and read counts with an optional payload length equal to the write - * count. The device will respond with a message that reports the 2B status - * code and an optional payload response length equal to read count. + * bInterfaceProtocol determines which protocol is used by the USB SPI device. * - * Message Format: * - * Command Packet: + * USB SPI Version 1: + * + * SPI transactions of up to 62B in each direction with every command having + * a response. The initial packet from host contains a 2B header indicating + * write and read counts with an optional payload length equal to the write + * count. The device will respond with a message that reports the 2B status + * code and an optional payload response length equal to read count. + * + * + * Command First Packet (Host to Device): + * + * USB SPI command, containing the number of bytes to write and read + * and a payload of bytes to write. + * * +------------------+-----------------+------------------------+ * | write count : 1B | read count : 1B | write payload : <= 62B | * +------------------+-----------------+------------------------+ * * write count: 1 byte, zero based count of bytes to write * - * read count: 1 byte, zero based count of bytes to read + * read count: 1 byte, zero based count of bytes to read. Half duplex + * mode is enabled with '-1' * * write payload: Up to 62 bytes of data to write to SPI, the total * length of all TX packets must match write count. @@ -71,6 +80,10 @@ * even number of bytes unless this is the final packet. * * Response Packet: + * + * USB SPI response, containing the status code and any bytes of the + * read payload. + * * +-------------+-----------------------+ * | status : 2B | read payload : <= 62B | * +-------------+-----------------------+ @@ -81,8 +94,8 @@ * 0x0002: Busy, try again * This can happen if someone else has acquired the shared memory * buffer that the SPI driver uses as /dev/null - * 0x0003: Write count invalid (V1 > 62B) - * 0x0004: Read count invalid (V1 > 62B) + * 0x0003: Write count invalid (over 62 bytes) + * 0x0004: Read count invalid (over 62 bytes) * 0x0005: The SPI bridge is disabled. * 0x8000: Unknown error mask * The bottom 15 bits will contain the bottom 15 bits from the EC @@ -115,24 +128,46 @@ #include <stdlib.h> #include <string.h> #include <unistd.h> +#include <stddef.h>
/* FIXME: Add some programmer IDs here */ const struct dev_entry devs_raiden[] = { {0}, };
-#define GOOGLE_VID (0x18D1) -#define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) -#define GOOGLE_RAIDEN_SPI_PROTOCOL (0x01) +#define GOOGLE_VID (0x18D1) +#define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) +#define GOOGLE_RAIDEN_SPI_PROTOCOL_V1 (0x01) + +enum { + /* The host failed to transfer the data with no libusb error. */ + USB_SPI_HOST_TX_BAD_TRANSFER = 0x10001, + /* We did not receive the expected USB packet. */ + USB_SPI_HOST_RX_UNEXPECTED_PACKET = 0x18002, + /* We received a continue packet with an invalid data index.*/ + USB_SPI_HOST_RX_BAD_DATA_INDEX = 0x18003, + /* We too much data. */ + USB_SPI_HOST_RX_DATA_OVERFLOW = 0x18004, + /* We too little data. */ + USB_SPI_HOST_RX_READ_FAILURE = 0x18005, + /* We were unable to configure the device. */ + USB_SPI_HOST_INIT_FAILURE = 0x18006, +};
enum usb_spi_error { - USB_SPI_SUCCESS = 0x0000, - USB_SPI_TIMEOUT = 0x0001, - USB_SPI_BUSY = 0x0002, - USB_SPI_WRITE_COUNT_INVALID = 0x0003, - USB_SPI_READ_COUNT_INVALID = 0x0004, - USB_SPI_DISABLED = 0x0005, - USB_SPI_UNKNOWN_ERROR = 0x8000, + USB_SPI_SUCCESS = 0x0000, + USB_SPI_TIMEOUT = 0x0001, + USB_SPI_BUSY = 0x0002, + USB_SPI_WRITE_COUNT_INVALID = 0x0003, + USB_SPI_READ_COUNT_INVALID = 0x0004, + USB_SPI_DISABLED = 0x0005, + /* The RX continue packet's data index is invalid. */ + USB_SPI_RX_BAD_DATA_INDEX = 0x0006, + /* The RX endpoint has received more data than write count. */ + USB_SPI_RX_DATA_OVERFLOW = 0x0007, + /* An unexpected packet arrived on the device. */ + USB_SPI_RX_UNEXPECTED_PACKET = 0x0008, + USB_SPI_UNKNOWN_ERROR = 0x8000, };
enum raiden_debug_spi_request { @@ -142,42 +177,95 @@ RAIDEN_DEBUG_SPI_REQ_ENABLE_EC = 0x0003, };
-#define PACKET_HEADER_SIZE (2) -#define MAX_PACKET_SIZE (64) -#define PAYLOAD_SIZE (MAX_PACKET_SIZE - PACKET_HEADER_SIZE) - /* * Servo Micro has an error where it is capable of acknowledging USB packets * without loading it into the USB endpoint buffers or triggering interrupts. * See crbug.com/952494. Retry mechanisms have been implemented to recover * from these rare failures allowing the process to continue. */ -#define WRITE_RETY_ATTEMPTS (3) -#define READ_RETY_ATTEMPTS (3) -#define RETY_INTERVAL_US (100 * 1000) +#define WRITE_RETRY_ATTEMPTS (3) +#define READ_RETRY_ATTEMPTS (3) +#define RETRY_INTERVAL_US (100 * 1000)
/* * This timeout is so large because the Raiden SPI timeout is 800ms. */ #define TRANSFER_TIMEOUT_MS (200 + 800)
-struct raiden_debug_spi_data { - struct usb_device *dev; - uint8_t in_ep; - uint8_t out_ep; -}; +/* + * USB permits a maximum bulk transfer of 64B. + */ +#define MAX_USB_PACKET_SIZE (64) +/* + * All of the USB SPI packets have size equal to the max USB packet size of 64B + */ +#define PAYLOAD_SIZE_V1 (62) + +#define SPI_TRANSFER_V1_MAX (PAYLOAD_SIZE_V1) + +/* + * Version 1 protocol specific attributes + */
typedef struct { int8_t write_count; /* -1 Indicates readback all on halfduplex compliant devices. */ int8_t read_count; - uint8_t data[PAYLOAD_SIZE]; -} __attribute__((packed)) usb_spi_command_t; + uint8_t data[PAYLOAD_SIZE_V1]; +} __attribute__((packed)) usb_spi_command_v1_t;
typedef struct { uint16_t status_code; - uint8_t data[PAYLOAD_SIZE]; -} __attribute__((packed)) usb_spi_response_t; + uint8_t data[PAYLOAD_SIZE_V1]; +} __attribute__((packed)) usb_spi_response_v1_t; + +typedef union { + usb_spi_command_v1_t command; + usb_spi_response_v1_t response; +} __attribute__((packed)) usb_spi_packet_v1_t; + +typedef struct { + union { + uint8_t bytes[MAX_USB_PACKET_SIZE]; + usb_spi_packet_v1_t packet_v1; + }; + /* + * By storing the number of bytes in the header and knowing that the + * USB data packets are all 64B long, we are able to use the header + * size to store the offset of the buffer and it's size without + * duplicating variables that can go out of sync. + */ + int header_size; + /* Number of bytes in the packet */ + int packet_size; +} usb_spi_packet_ctx_t; + +typedef struct { + /* Buffer we are reading data from. */ + const uint8_t *buffer; + /* Total size of the transmit transfer. */ + int transmit_size; + /* Number of bytes transmitted. */ + int transmit_index; +} usb_spi_transmit_ctx_t; + +typedef struct { + /* Buffer we are writing data into. */ + uint8_t *buffer; + /* Total size of the receive transfer. */ + int receive_size; + /* Number of bytes received. */ + int receive_index; +} usb_spi_receive_ctx_t; + +struct raiden_debug_spi_data { + struct usb_device *dev; + uint8_t in_ep; + uint8_t out_ep; + uint8_t protocol_version; + uint16_t max_spi_write_count; + uint16_t max_spi_read_count; +};
/* * This function will return true when an error code can potentially recover @@ -212,167 +300,239 @@ return (const struct raiden_debug_spi_data *)flash->mst->spi.data; }
-static int write_command(const struct flashctx *flash, - unsigned int write_count, - unsigned int read_count, - const unsigned char *write_buffer, - unsigned char *read_buffer) -{ +/* + * Fill the data section in the USB packets. + */ +static int fill_usb_packet(usb_spi_packet_ctx_t* dst, + usb_spi_transmit_ctx_t* src) { + int transfer_size = src->transmit_size - src->transmit_index; + int max_buffer_size = MAX_USB_PACKET_SIZE - dst->header_size; + uint8_t *packet_buffer = dst->bytes + dst->header_size;
- int transferred; - int ret; - usb_spi_command_t command_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); - - if (write_count > PAYLOAD_SIZE) { - msg_perr("Raiden: Invalid write_count of %d\n", write_count); - return SPI_INVALID_LENGTH; + if (transfer_size > max_buffer_size) { + transfer_size = max_buffer_size; } + memcpy(packet_buffer, src->buffer + src->transmit_index, transfer_size);
- if (read_count > PAYLOAD_SIZE) { - msg_perr("Raiden: Invalid read_count of %d\n", read_count); - return SPI_INVALID_LENGTH; - } - - command_packet.write_count = write_count; - command_packet.read_count = read_count; - - memcpy(command_packet.data, write_buffer, write_count); - - ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, - ctx_data->out_ep, - (void*)&command_packet, - write_count + PACKET_HEADER_SIZE, - &transferred, - TRANSFER_TIMEOUT_MS)); - if (ret != 0) { - msg_perr("Raiden: OUT transfer failed\n" - " write_count = %d\n" - " read_count = %d\n", - write_count, read_count); - return ret; - } - - if ((unsigned) transferred != write_count + PACKET_HEADER_SIZE) { - msg_perr("Raiden: Write failure (wrote %d, expected %d)\n", - transferred, write_count + PACKET_HEADER_SIZE); - return 0x10001; - } - + dst->packet_size = dst->header_size + transfer_size; + src->transmit_index += transfer_size; return 0; }
-static int read_response(const struct flashctx *flash, - unsigned int write_count, - unsigned int read_count, - const unsigned char *write_buffer, - unsigned char *read_buffer) -{ +static int read_usb_packet(usb_spi_receive_ctx_t* dst, + const usb_spi_packet_ctx_t* src) { + int max_read_length = dst->receive_size - dst->receive_index; + int bytes_in_buffer = src->packet_size - src->header_size; + const uint8_t *packet_buffer = src->bytes + src->header_size; + + if (bytes_in_buffer > max_read_length) { + /* + * An error occurred, we should not receive more data than + * the buffer can support. + */ + msg_perr("Raiden: Receive packet overflowed\n" + " bytes_in_buffer = %d\n" + " max_read_length = %d\n" + " receive_index = %d\n" + " receive_size = %d\n", + bytes_in_buffer, max_read_length, + dst->receive_size, dst->receive_index); + return USB_SPI_HOST_RX_DATA_OVERFLOW; + } + memcpy(dst->buffer + dst->receive_index, packet_buffer, bytes_in_buffer); + + dst->receive_index += bytes_in_buffer; + return 0; +} + +static int transmit_packet(const struct raiden_debug_spi_data * ctx_data, + usb_spi_packet_ctx_t* packet) { int transferred; - int ret; - usb_spi_response_t response_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->out_ep, + packet->bytes, + packet->packet_size, + &transferred, + TRANSFER_TIMEOUT_MS)); + if (status || transferred != packet->packet_size) { + msg_perr("Raiden: OUT transfer failed\n" + " transferred = %d\n" + " packet_size = %d\n" + " status = %d\n", + transferred, packet->packet_size, status); + if (status == 0) { + return USB_SPI_HOST_TX_BAD_TRANSFER; + } + } + return status; +}
- ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, - ctx_data->in_ep, - (void*)&response_packet, - read_count + PACKET_HEADER_SIZE, - &transferred, - TRANSFER_TIMEOUT_MS)); - if (ret != 0) { +static int receive_packet(const struct raiden_debug_spi_data * ctx_data, + usb_spi_packet_ctx_t* packet) { + int transferred; + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->in_ep, + packet->bytes, + MAX_USB_PACKET_SIZE, + &transferred, + TRANSFER_TIMEOUT_MS)); + packet->packet_size = transferred; + if (status) { msg_perr("Raiden: IN transfer failed\n" - " write_count = %d\n" - " read_count = %d\n", - write_count, read_count); - return ret; + " transferred = %d\n" + " status = %d\n", + transferred, status); } + return status; +}
- if ((unsigned) transferred != read_count + PACKET_HEADER_SIZE) { - msg_perr("Raiden: Read failure (read %d, expected %d)\n", - transferred, read_count + PACKET_HEADER_SIZE); - return 0x10002; +/* + * Version 1 Protocol + */ + +static int write_command_v1(const struct raiden_debug_spi_data * ctx_data, + usb_spi_transmit_ctx_t* write, + usb_spi_receive_ctx_t* read) +{ + int status; + usb_spi_packet_ctx_t command = { + .header_size = offsetof(usb_spi_command_v1_t, data), + .packet_v1.command.write_count = write->transmit_size, + .packet_v1.command.read_count = read->receive_size + }; + + fill_usb_packet(&command, write); + status = transmit_packet(ctx_data, &command); + return status; +} + +static int read_response_v1(const struct raiden_debug_spi_data * ctx_data, + usb_spi_transmit_ctx_t* write, + usb_spi_receive_ctx_t* read) +{ + int status; + usb_spi_packet_ctx_t response; + + status = receive_packet(ctx_data, &response); + if (status) { + /* Return the transfer error since the status_code is unreliable */ + return status; } + if (response.packet_v1.response.status_code) { + return response.packet_v1.response.status_code; + } + response.header_size = offsetof(usb_spi_response_v1_t, data);
- memcpy(read_buffer, response_packet.data, read_count); - - return response_packet.status_code; + status = read_usb_packet(read, &response); + return status; }
static int send_command(const struct flashctx *flash, - unsigned int write_count, - unsigned int read_count, - const unsigned char *write_buffer, - unsigned char *read_buffer) + unsigned int write_count, + unsigned int read_count, + const unsigned char *write_buffer, + unsigned char *read_buffer) { + const struct raiden_debug_spi_data * ctx_data = + get_raiden_data_from_context(flash); int status = -1; + usb_spi_transmit_ctx_t write_ctx = { + .buffer = write_buffer, + .transmit_size = write_count + }; + usb_spi_receive_ctx_t read_ctx = { + .buffer = read_buffer, + .receive_size = read_count + };
- for (int write_attempt = 0; write_attempt < WRITE_RETY_ATTEMPTS; + if (write_count > ctx_data->max_spi_write_count) { + msg_perr("Raiden: Invalid write count of %d, Max %d, \n", + write_count, ctx_data->max_spi_write_count); + return SPI_INVALID_LENGTH; + } + + if (read_count > ctx_data->max_spi_read_count) { + msg_perr("Raiden: Invalid read count of %d, Max %d\n", + read_count, ctx_data->max_spi_write_count); + return SPI_INVALID_LENGTH; + } + + for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; write_attempt++) {
- status = write_command(flash, write_count, read_count, - write_buffer, read_buffer); + /* Make sure we always reset the write context. */ + write_ctx.transmit_index = 0; + if (ctx_data->protocol_version == GOOGLE_RAIDEN_SPI_PROTOCOL_V1) { + status = write_command_v1(ctx_data, &write_ctx, &read_ctx); + }
if (status) { /* Write operation failed. */ msg_perr("Raiden: Write command failed\n" - "Write attempt = %d\n" - "status = %d\n", - write_attempt + 1, status); + " write count = %d\n" + " read count = %d\n" + " transmitted bytes = %d\n" + " write attempt = %d\n" + " status = %d\n", + write_count, read_count, write_ctx.transmit_index, + write_attempt + 1, status); if (!retry_recovery(status)) { /* Reattempting will not result in a recovery. */ return status; } - programmer_delay(RETY_INTERVAL_US); + programmer_delay(RETRY_INTERVAL_US); continue; } - for (int read_attempt = 0; read_attempt < READ_RETY_ATTEMPTS; read_attempt++) { + for (int read_attempt = 0; read_attempt < READ_RETRY_ATTEMPTS; + read_attempt++) {
- status = read_response(flash, write_count, read_count, - write_buffer, read_buffer); + /* Make sure we always reset the read context. */ + read_ctx.receive_index = 0; + if (ctx_data->protocol_version == GOOGLE_RAIDEN_SPI_PROTOCOL_V1) { + status = read_response_v1(ctx_data, &write_ctx, &read_ctx); + }
- if (status != 0) { + if (status == 0) { + if (read_ctx.receive_size != read_ctx.receive_index) { + status = USB_SPI_HOST_RX_READ_FAILURE; + } + } + + if (status) { /* Read operation failed. */ msg_perr("Raiden: Read response failed\n" - "Write attempt = %d\n" - "Read attempt = %d\n" - "status = %d\n", - write_attempt + 1, read_attempt + 1, status); + " write count = %d\n" + " read count = %d\n" + " received bytes = %d\n" + " write attempt = %d\n" + " read attempt = %d\n" + " status = %d\n", + write_count, read_count, read_ctx.receive_index, + write_attempt + 1, read_attempt + 1, status); if (!retry_recovery(status)) { /* Reattempting will not result in a recovery. */ return status; } - programmer_delay(RETY_INTERVAL_US); + programmer_delay(RETRY_INTERVAL_US); } else { /* We were successful at performing the SPI transfer. */ return status; } } } + return status; }
-/* - * Unfortunately there doesn't seem to be a way to specify the maximum number - * of bytes that your SPI device can read/write, these values are the maximum - * data chunk size that flashrom will package up with an additional five bytes - * of command for the flash device, resulting in a 62 byte packet, that we then - * add two bytes to in either direction, making our way up to the 64 byte - * maximum USB packet size for the device. - * - * The largest command that flashrom generates is the byte program command, so - * we use that command header maximum size here. - */ -#define MAX_DATA_SIZE (PAYLOAD_SIZE - JEDEC_BYTE_PROGRAM_OUTSIZE) - static struct spi_master spi_master_raiden_debug = { - .features = SPI_MASTER_4BA, - .max_data_read = MAX_DATA_SIZE, - .max_data_write = MAX_DATA_SIZE, - .command = send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, + .features = SPI_MASTER_4BA, + .max_data_read = 0, + .max_data_write = 0, + .command = send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, };
static int match_endpoint(struct libusb_endpoint_descriptor const *descriptor, @@ -405,9 +565,9 @@
if (in_count != 1 || out_count != 1) { msg_perr("Raiden: Failed to find one IN and one OUT endpoint\n" - " found %d IN and %d OUT endpoints\n", - in_count, - out_count); + " found %d IN and %d OUT endpoints\n", + in_count, + out_count); return 1; }
@@ -417,22 +577,60 @@ return 0; }
+static int configure_protocol(struct spi_master *ctx_spi) +{ + struct raiden_debug_spi_data *ctx_data = + (struct raiden_debug_spi_data *)ctx_spi->data; + + ctx_data->protocol_version = + ctx_data->dev->interface_descriptor->bInterfaceProtocol; + switch(ctx_data->protocol_version) { + case GOOGLE_RAIDEN_SPI_PROTOCOL_V1: + /* + * Protocol V1 is supported by adjusting the max data + * read and write sizes which results in no continue packets. + */ + ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; + ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX; + break; + default: + msg_pdbg("Raiden: Unknown USB SPI protocol version = %d", + ctx_data->protocol_version); + return USB_SPI_HOST_INIT_FAILURE; + } + /* + * Unfortunately there doesn't seem to be a way to specify the maximum number + * of bytes that your SPI device can read/write, these values are the maximum + * data chunk size that flashrom will package up with an additional five bytes + * of command for the flash device. + * + * The largest command that flashrom generates is the byte program command, so + * we use that command header maximum size here. + */ + + ctx_spi->max_data_write = ctx_data->max_spi_write_count - + JEDEC_BYTE_PROGRAM_OUTSIZE; + ctx_spi->max_data_read = ctx_data->max_spi_read_count - + JEDEC_BYTE_PROGRAM_OUTSIZE; + return 0; +} + static int raiden_debug_spi_shutdown(void * data) { - struct raiden_debug_spi_data * ctx_data = + struct raiden_debug_spi_data * ctx_data = (struct raiden_debug_spi_data *)data;
int ret = LIBUSB(libusb_control_transfer( - ctx_data->dev->handle, - LIBUSB_ENDPOINT_OUT | - LIBUSB_REQUEST_TYPE_VENDOR | - LIBUSB_RECIPIENT_INTERFACE, - RAIDEN_DEBUG_SPI_REQ_DISABLE, - 0, - ctx_data->dev->interface_descriptor->bInterfaceNumber, - NULL, - 0, - TRANSFER_TIMEOUT_MS)); + ctx_data->dev->handle, + LIBUSB_ENDPOINT_OUT | + LIBUSB_REQUEST_TYPE_VENDOR | + LIBUSB_RECIPIENT_INTERFACE, + RAIDEN_DEBUG_SPI_REQ_DISABLE, + 0, + ctx_data->dev->interface_descriptor->bInterfaceNumber, + NULL, + 0, + TRANSFER_TIMEOUT_MS)); if (ret != 0) { msg_perr("Raiden: Failed to disable SPI bridge\n"); return ret; @@ -610,7 +808,13 @@ data->in_ep = in_endpoint; data->out_ep = out_endpoint;
+ /* The host side needs to be configured based on the device connected */ spi_master_raiden_debug.data = data; + if (configure_protocol(&spi_master_raiden_debug)) { + msg_perr("Raiden: Error configuring protocol %d\n", + data->dev->interface_descriptor->bInterfaceProtocol); + return 1; + }
register_spi_master(&spi_master_raiden_debug); register_shutdown(raiden_debug_spi_shutdown, data);