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);
Hello build bot (Jenkins), Brian Nemec,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#2).
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, 381 insertions(+), 177 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/2
Hello build bot (Jenkins), Brian Nemec,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#3).
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, 381 insertions(+), 177 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/3
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(10 comments)
The general direction looks good however these changes are *way* way too big to review and definitely can be broken down into sizable chunks that each stand on their own merit.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@140 PS3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1 probably a enum
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count; the spi master already has these fields, you should fill in the data there.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@438 PS3, Line 438: ; \n
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@448 PS3, Line 448: if (write_count > ctx_data->max_spi_write_count) { validation should happen before packing structs
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@529 PS3, Line 529: .max_data_read = 0, : .max_data_write = 0, not sure why you are not using these fields and putting them into the local state data context?
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@568 PS3, Line 568: mixing tabs / spaces
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@584 PS3, Line 584: : ctx_data->protocol_version = : ctx_data->dev->interface_descriptor->bInterfaceProtocol; : switch(ctx_data->protocol_version) { this is its own function, `enum protocol_rev_t raiden_debug_detect_protocol_rev(struct usb_device *device)`
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@592 PS3, Line 592: : ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX; have two spi_master_raiden_debug spi_master struct's for each protocol and register the one with the correctly packed data.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@620 PS3, Line 620: more tabs and spaces mixed throughout this file.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco no, just declare the state you want (read/write max_size in this case) for each path and choose the correct state to register as the spi master after a simple detection function.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(10 comments)
Looks like your editor is putting spaces everywhere
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@307 PS3, Line 307: Can use tabs here
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@334 PS3, Line 334: Maybe use a tab instead of eight spaces
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@352 PS3, Line 352: ctx_data->out_ep, This looks pretty weird. Also, it could use tabs
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@372 PS3, Line 372: there are spaces
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@382 PS3, Line 382: Can use tabs here
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@394 PS3, Line 394: Can use tabs here
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@431 PS3, Line 431: Can use tabs here
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@518 PS3, Line 518: /* We were successful at performing the SPI transfer. */ : return status; This can be moved at line 498:
if (status == 0) { if (read_ctx.receive_size != read_ctx.receive_index) { status = USB_SPI_HOST_RX_READ_FAILURE;
} else { /* We were successful at performing the SPI transfer. */ return status; } }
Then, the "if (status) {" at line 501 can go away
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@624 PS3, Line 624: moar spaces that should be tabs
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@777 PS3, Line 777: use tabs
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(11 comments)
Patch Set 3:
(10 comments)
The general direction looks good however these changes are *way* way too big to review and definitely can be broken down into sizable chunks that each stand on their own merit.
Totally fair, I can break this down further if we need.
I'd also like to have just a single quick discussion about the tabs/spaces usage here so it's not scattered in 20 messages and multiple CL's. My goal with the indentation has been to use tabs to represent indentation levels and to use spaces whenever we are aligning text, like to the function declarations or as parameters, especially in the multi-line strings. This does result in more characters but it's an indentation / alignment style that is robust to different tab widths.
Same idea as https://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@140 PS3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1
probably a enum
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
the spi master already has these fields, you should fill in the data there.
These are directly replacing 2 constants that we've been using:
We need to make sure that flashrom will not attempt a SPI transfer larger than the platform will support. It looks like some parts of flashrom will treat the max_data_read/write values as the maximum size of the SPI transfer and construct a message smaller than it, other parts interpret that line to mean that's the maximum number of bytes in a message excluding the header meaning you end up with slightly larger packets, I've seen up to 61 bytes.
To get around this issue, we have been telling the rest of flashrom that it's maximum capacity is equal to the maximum SPI packet - JEDEC_BYTE_PROGRAM_OUTSIZE. We've been using 2 values to keep track of these fields before in the form of 'max_data_read = (PAYLOAD_SIZE - JEDEC_BYTE_PROGRAM_OUTSIZE)' but we've been comparing the 'read_count' against PAYLOAD_SIZE.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@307 PS3, Line 307:
Can use tabs here
My goal has been to indent with tabs and align with spaces. That style is the most robust to different editor / preferences since changing tab width keeps text in alignment with the functions above that they are using.
This comment applies to this line and many of the other similar lines, although I do have a few lines where I accidentally used spaces for indentation too so those need to be fixed.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@438 PS3, Line 438: ;
\n
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@448 PS3, Line 448: if (write_count > ctx_data->max_spi_write_count) {
validation should happen before packing structs
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@518 PS3, Line 518: /* We were successful at performing the SPI transfer. */ : return status;
This can be moved at line 498: […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@529 PS3, Line 529: .max_data_read = 0, : .max_data_write = 0,
not sure why you are not using these fields and putting them into the local state data context?
See comment above near the 'struct raiden_debug_spi_data'
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@584 PS3, Line 584: : ctx_data->protocol_version = : ctx_data->dev->interface_descriptor->bInterfaceProtocol; : switch(ctx_data->protocol_version) {
this is its own function, `enum protocol_rev_t raiden_debug_detect_protocol_rev(struct usb_device *d […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@592 PS3, Line 592: : ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX;
have two spi_master_raiden_debug spi_master struct's for each protocol and register the one with the […]
See comment about configure_protocol() from below: Short answer protocol 2 allows SPI transfers much larger than what platforms support and some of our chips have different maximum SPI transfer sizes.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@620 PS3, Line 620:
more tabs and spaces mixed throughout this file.
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
no, just declare the state you want (read/write max_size in this case) for each path and choose the […]
We need to communicate with the device in order to identify how large of a SPI transfer is supported. Some devices like the STM32's used in Servo Micros and C2D2 work for SPI transactions over 256 bytes, others like the Cr50's support 128 bytes. We could do a mapping of the protocol version number at this point to resolve that as a way around this issue:
For instance 1 is the existing path, 2 could be a value over 256 that the STM32F0's support, 3 could be 128 bytes.
It'd be a bit less flexible since changing the packet limit burns another version number, but there's some pros to having a simpler init process.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@10 PS3, Line 10: The changes include: These are essentially each their own commit to make this properly reviewable.
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@12 PS3, Line 12: Setting of USB SPI limits based on the device's USB protocol commit 1.
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@14 PS3, Line 14: * Creation of context states to keep track of bytes written to : and read from the device and individual USB packets. commit 3?
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@17 PS3, Line 17: * Creation of helper functions for handling transfer of data : between the USB packets and the data buffers. commit 2?
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@20 PS3, Line 20: * Removal of magic numbers on the USB return codes. Some renaming : of to make the protocol version clear. commit 0.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
These are directly replacing 2 constants that we've been using: […]
When you say "other parts interpret" can you be a bit more concrete. I am unclear if you are actually describing a bug here or not. However this looks to me like you are working around one or two issues:
1.) Programmatically changing the rw max count depending on protocol detected instead of detecting and selecting from declarative data in struct's (the spi_master struct instances). 2.) Some issues around max not respecting the header being included or not, in which case that just sounds like a bug that should be addressed.
I don't think having more local state in the spi master that replicated what should be core logic to flashrom to work around an identified bug is the right maintainable path forwards unless I have miss-understood something here?
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@307 PS3, Line 307:
My goal has been to indent with tabs and align with spaces. […]
I suggest just sticking to the style already inherent in the file. Flashrom unfortunately doesn't have a clang-format file and it has been an area of discussion many times however the general style follows that of coreboot if in unsure.
Regardless Angel is correct in his suggestions here.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@495 PS3, Line 495: status == 0 `if (!status)` is canonical.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
We need to communicate with the device in order to identify how large of a SPI transfer is supported […]
I am not saying to not communicate with the device to interrogate what it supports however the results of that interrogation should be to select the correct declarative state rather than programmatically mutating localised singleton state.
This is precisely what lead to the cros_ec spi master becoming unmaintable and so un-upstreamable until it is rewritten. It had tried to shove all these different chip support paths via the one entry-point and initial state then later tries to fix things up at run-time programmatically with quarks.
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
When you say "other parts interpret" can you be a bit more concrete. […]
I'm describing a bug in flashrom, the one that this block comment talks about:
/* * 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. */
Some of the flashrom drivers interpret the fields 'max_data_read' and 'max_data_write' as the maximum number of bytes we can fit in a payload, not the maximum size of the packet meaning it will generate packets too large.
To avoid this, we are currently storing 2 different constants, one which is keeping track of actual maximum packet size, the other is telling flashrom a false value that is 5 bytes smaller. As this value is now a variable, the behavior had to be duplicated.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
I am not saying to not communicate with the device to interrogate what it supports however the resul […]
Are you suggesting copying the data from 'spi_master_raiden_debug' into a malloc'd buffer. That pointer then gets passed to the configure_protocol() function and the shutdown will be responsible for freeing both buffers. This would prevent us from ever changing spi_master_raiden_debug and allow us to keep it const like it was before the data field was added to it.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
I'm describing a bug in flashrom, the one that this block comment talks about: […]
By flashrom drivers I believe you are talking about spi masters? In that case, it is up to them how they interpret as they are implementation specific so whatever they happen to do is basically irrelevant here. Only what the core logic in spi[25].c really applies here.
This sounds like the maximum USB packet size with maximum spi transaction size concepts are getting muddled together.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
Are you suggesting copying the data from 'spi_master_raiden_debug' into a malloc'd buffer. […]
No. Here is a two things to consider;
First, whatever happens in other spi masters isn't relevant to here as they are specific to their use-case and only the core flashrom logic matters. The values here are just bounds for sanity checking, I don't think you need too much complexity.
The first step is making the raiden_debug_detect_protocol_rev() helper function that isolates the rev detection, where and how bounds is determined is a separate problem.
The other thing to consider is to perhaps think about dispatch in this way,
``` static struct spi_master spi_master_raiden_debug_v1 = { .max_data_read = XXX, .max_data_write = XXX, ... };
static struct spi_master spi_master_raiden_debug_v2 = { .max_data_read = YYY, .max_data_write = YYY, ... };
init() { ... switch (raiden_debug_detect_protocol_rev()) { case PROTOCOL_V1: register_spi_master(&spi_master_raiden_debug_v1); } ```
Then either _v1 or _v2 struct members like `.command = send_command` have wrapper functions around the canonical path that deal with the specialization, if say v2 has somehow dynamic bounds.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
By flashrom drivers I believe you are talking about spi masters? In that case, it is up to them how […]
jlink_spi.c has some parallels, albit a more simplified version:
$ git grep max_data jlink_spi.c jlink_spi.c: .max_data_read = JTAG_MAX_TRANSFER_SIZE - 5, jlink_spi.c: .max_data_write = JTAG_MAX_TRANSFER_SIZE - 5,
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#4).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 101 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/4
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#5).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 103 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/5
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#6).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/6
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#7).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/7
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#8).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 639 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/8
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#9).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/9
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#10).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/10
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#11).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 66 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/11
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#12).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 69 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/12
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 12: Code-Review+2
(3 comments)
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@672 PS12, Line 672: } add \n
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@683 PS12, Line 683: */ remove stray \n after comment block
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@688 PS12, Line 688: ; \n
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#13).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 71 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/13
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#14).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 86 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/14
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#15).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 97 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/15
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/41532/15/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/15/raiden_debug_spi.c@729 PS15, Line 729: return ret; doesn't this early return result in a memory leak? Can be a follow up fix.
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@672 PS12, Line 672: }
add \n
Done
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@683 PS12, Line 683: */
remove stray \n after comment block
Done
https://review.coreboot.org/c/flashrom/+/41532/12/raiden_debug_spi.c@688 PS12, Line 688: ;
\n
Done
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 16:
(16 comments)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@140 PS3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1
Ack
.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
jlink_spi.c has some parallels, albit a more simplified version: […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@307 PS3, Line 307:
I suggest just sticking to the style already inherent in the file. […]
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@334 PS3, Line 334:
Maybe use a tab instead of eight spaces
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@352 PS3, Line 352: ctx_data->out_ep,
This looks pretty weird. […]
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@372 PS3, Line 372:
there are spaces
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@382 PS3, Line 382:
Can use tabs here
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@394 PS3, Line 394:
Can use tabs here
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@431 PS3, Line 431:
Can use tabs here
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@495 PS3, Line 495: status == 0
`if (!status)` is canonical.
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@529 PS3, Line 529: .max_data_read = 0, : .max_data_write = 0,
See comment above near the 'struct raiden_debug_spi_data'
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@568 PS3, Line 568:
mixing tabs / spaces
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@592 PS3, Line 592: : ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX;
See comment about configure_protocol() from below: Short answer protocol 2 allows SPI transfers much […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@624 PS3, Line 624:
moar spaces that should be tabs
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@777 PS3, Line 777:
use tabs
Done
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
No. Here is a two things to consider; […]
Done
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 16:
(5 comments)
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@10 PS3, Line 10: The changes include:
These are essentially each their own commit to make this properly reviewable.
Done
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@12 PS3, Line 12: Setting of USB SPI limits based on the device's USB protocol
commit 1.
Done
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@14 PS3, Line 14: * Creation of context states to keep track of bytes written to : and read from the device and individual USB packets.
commit 3?
Done
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@17 PS3, Line 17: * Creation of helper functions for handling transfer of data : between the USB packets and the data buffers.
commit 2?
Done
https://review.coreboot.org/c/flashrom/+/41532/3//COMMIT_MSG@20 PS3, Line 20: * Removal of magic numbers on the USB return codes. Some renaming : of to make the protocol version clear.
commit 0.
Done
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41532/15/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/15/raiden_debug_spi.c@729 PS15, Line 729: return ret;
doesn't this early return result in a memory leak? Can be a follow up fix.
Yes, it looks like it does. Looks like the allocations will need to be cleaned up later on in the configure_protocol() fail path.
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#17).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 105 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/17
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#18).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 105 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/18
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#19).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Adds a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and seperate out the logic flow used for the different protocols.
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, 105 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/19
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 20:
(7 comments)
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@159 PS20, Line 159: /* We too much data. */ Oops
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@211 PS20, Line 211: bugs Where? We might as well fix them
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@659 PS20, Line 659: Configures Configure
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@675 PS20, Line 675: switch space after switch
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@676 PS20, Line 676: 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. : */ : spi_config->command = send_command_v1; : 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; We don't indent case labels, AFAIK
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@729 PS20, Line 729: free(ctx_data); This should go with the patch that added the other `free(ctx_data)` call
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@917 PS20, Line 917: spi SPI
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG@9 PS20, Line 9: Adds Use imperative mood: Add
I like to think that the commit message is more like a "commit recipe", because recipes are usually written in imperative mood, e.g.: https://www.allrecipes.com/recipe/17481/simple-white-cake/
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG@13 PS20, Line 13: seperate sep*a*rate
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#22).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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, 105 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/22
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#23).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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, 105 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/23
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 23:
(7 comments)
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@159 PS20, Line 159: /* We too much data. */
Oops
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@211 PS20, Line 211: bugs
Where? We might as well fix them
The definition of the field itself creates this problem:
struct spi_master { uint32_t features; unsigned int max_data_read; // (Ideally,) maximum data read size in one go (excluding opcode+address). unsigned int max_data_write; // (Ideally,) maximum data write size in one go (excluding opcode+address). int (*command)(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int (*multicommand)(const struct flashctx *flash, struct spi_command *cmds);
/* Optimized functions for this master */ int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*write_aai)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); const void *data; };
The "(Ideally,) maximum data read size in one go (excluding opcode+address)." is a bad way to structure this variable. The SPI masters don't need to know what flash they are connected to so any field that relies on them knowing the maximum length of the opcode+address is going to introduce problems. A SPI master can provide it's maximum SPI transfer size easily using a datasheet or by identifying other limiting factors like a buffer, but there isn't a good way to identify the maximum op code size for a given platform. Either they support a maximum transfer length much larger than the flashes would use or they artificially subtract several bytes for the maximum operation size which depending on the flash driver may or may not be unused.
Due to WFH, I don't have access to all of the hardware, but different ChromeOS laptops used different flash drivers that had different interpretations for this length. The Nami platform interpreted the max_data_read / max_data_write has the maximum packet size including packet lengths, I think Atlas used a different interpretations where it was the literal max data size and added 4 bytes of header sizes on some of it's transfers. I'm not certain what chip is used in the other platform or how many flash chips contain that interpretation for the variable.
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@659 PS20, Line 659: Configures
Configure
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@675 PS20, Line 675: switch
space after switch
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@676 PS20, Line 676: 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. : */ : spi_config->command = send_command_v1; : 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;
We don't indent case labels, AFAIK
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@729 PS20, Line 729: free(ctx_data);
This should go with the patch that added the other `free(ctx_data)` call
That was months ago.
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@917 PS20, Line 917: spi
SPI
Done
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 23:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG@9 PS20, Line 9: Adds
Use imperative mood: Add […]
Done
https://review.coreboot.org/c/flashrom/+/41532/20//COMMIT_MSG@13 PS20, Line 13: seperate
sep*a*rate
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 23: Code-Review+1
(3 comments)
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@211 PS20, Line 211: bugs
The definition of the field itself creates this problem: […]
I see. Ugh. Welp, it can be fixed later.
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@729 PS20, Line 729: free(ctx_data);
That was months ago.
Oh well.
https://review.coreboot.org/c/flashrom/+/41532/23/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/23/raiden_debug_spi.c@675 PS23, Line 675: switch Still missing a space
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#24).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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, 105 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/24
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 24: Code-Review+2
(1 comment)
https://review.coreboot.org/c/flashrom/+/41532/23/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/23/raiden_debug_spi.c@675 PS23, Line 675: switch
Still missing a space
Done
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#25).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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, 105 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/25
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 25: Code-Review+2
Hello build bot (Jenkins), Brian Nemec, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/41532
to look at the new patch set (#26).
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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, 105 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/41532/26
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 26: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 26: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
raiden_debug_spi.c: Add protocol based configuration to init
Add a configuration stage to the initialization. This enables us to dynamically set the maximum SPI write and read limits based on the device we are connected to and switch the command function. These changes will enable us to have larger SPI transfers in protocol V2 and separate out the logic flow used for the different protocols.
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 Reviewed-on: https://review.coreboot.org/c/flashrom/+/41532 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M raiden_debug_spi.c 1 file changed, 105 insertions(+), 28 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 0067aab..d677384 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -184,6 +184,7 @@ #define PACKET_HEADER_SIZE (2) #define USB_MAX_PACKET_SIZE (64) #define PAYLOAD_SIZE_V1 (USB_MAX_PACKET_SIZE - PACKET_HEADER_SIZE) +#define SPI_TRANSFER_V1_MAX (PAYLOAD_SIZE_V1)
/* * Servo Micro has an error where it is capable of acknowledging USB packets @@ -204,6 +205,15 @@ struct usb_device *dev; uint8_t in_ep; uint8_t out_ep; + uint8_t protocol_version; + /* + * Note: Due to bugs, flashrom does not always treat the max_data_write + * and max_data_read counts as the maximum packet size. As a result, we + * have to store a local copy of the actual max packet sizes and validate + * against it when performing transfers. + */ + uint16_t max_spi_write_count; + uint16_t max_spi_read_count; };
/* @@ -288,10 +298,10 @@ return true; }
-static const struct raiden_debug_spi_data * +static struct raiden_debug_spi_data * get_raiden_data_from_context(const struct flashctx *flash) { - return (const struct raiden_debug_spi_data *)flash->mst->spi.data; + return (struct raiden_debug_spi_data *)flash->mst->spi.data; }
/* @@ -508,19 +518,19 @@ }; const struct raiden_debug_spi_data *ctx_data = get_raiden_data_from_context(flash);
- if (write_count > PAYLOAD_SIZE_V1) { + if (write_count > ctx_data->max_spi_write_count) { msg_perr("Raiden: Invalid write count\n" " write count = %u\n" " max write = %d\n", - write_count, PAYLOAD_SIZE_V1); + write_count, ctx_data->max_spi_write_count); return SPI_INVALID_LENGTH; }
- if (read_count > PAYLOAD_SIZE_V1) { + if (read_count > ctx_data->max_spi_read_count) { msg_perr("Raiden: Invalid read count\n" " read count = %d\n" " max read = %d\n", - read_count, PAYLOAD_SIZE_V1); + read_count, ctx_data->max_spi_read_count); return SPI_INVALID_LENGTH; }
@@ -591,24 +601,11 @@ 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_V1 - JEDEC_BYTE_PROGRAM_OUTSIZE) - -static struct spi_master spi_master_raiden_debug = { +static const 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_v1, + .max_data_read = 0, + .max_data_write = 0, + .command = NULL, .multicommand = default_spi_send_multicommand, .read = default_spi_read, .write_256 = default_spi_write_256, @@ -657,10 +654,64 @@ return 0; }
+/* + * Configure the USB SPI master based on the device we are connected to. + * It will use the device's bInterfaceProtocol to identify which protocol + * is being used by the device USB SPI interface and if needed query the + * device for its capabilities. + * + * @param spi_config Raiden SPI config which will be modified. + * + * @returns Returns status code with 0 on success. + */ +static int configure_protocol(struct spi_master *spi_config) +{ + struct raiden_debug_spi_data *ctx_data = + (struct raiden_debug_spi_data *)spi_config->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. + */ + spi_config->command = send_command_v1; + 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. If we didn't include the + * offset, flashrom may request a SPI transfer that is too large for the SPI + * device to support. + */ + spi_config->max_data_write = ctx_data->max_spi_write_count - + JEDEC_BYTE_PROGRAM_OUTSIZE; + spi_config->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 *)data; + struct spi_master *spi_config = data; + struct raiden_debug_spi_data *ctx_data = + (struct raiden_debug_spi_data *)spi_config->data;
int ret = LIBUSB(libusb_control_transfer( ctx_data->dev->handle, @@ -675,12 +726,15 @@ TRANSFER_TIMEOUT_MS)); if (ret != 0) { msg_perr("Raiden: Failed to disable SPI bridge\n"); + free(ctx_data); + free(spi_config); return ret; }
usb_device_free(ctx_data->dev); libusb_exit(NULL); free(ctx_data); + free(spi_config);
return 0; } @@ -840,20 +894,43 @@ (request_enable == RAIDEN_DEBUG_SPI_REQ_ENABLE_EC)) usleep(50 * 1000);
+ struct spi_master *spi_config = calloc(1, sizeof(struct spi_master)); + if (!spi_config) { + msg_perr("Unable to allocate space for SPI master.\n"); + return SPI_GENERIC_ERROR; + } struct raiden_debug_spi_data *data = calloc(1, sizeof(struct raiden_debug_spi_data)); if (!data) { + free(spi_config); msg_perr("Unable to allocate space for extra SPI master data.\n"); return SPI_GENERIC_ERROR; }
+ memcpy(spi_config, &spi_master_raiden_debug, sizeof(struct spi_master)); + data->dev = device; data->in_ep = in_endpoint; data->out_ep = out_endpoint;
- spi_master_raiden_debug.data = data; + spi_config->data = data; + /* + * The SPI master needs to be configured based on the device connected. + * Using the device protocol interrogation, we will set the limits on + * the write and read sizes and switch command functions. + */ + ret = configure_protocol(spi_config); + if (ret) { + msg_perr("Raiden: Error configuring protocol\n" + " protocol = %u\n" + " status = 0x%05x\n", + data->dev->interface_descriptor->bInterfaceProtocol, ret); + free(data); + free(spi_config); + return SPI_GENERIC_ERROR; + }
- register_spi_master(&spi_master_raiden_debug); - register_shutdown(raiden_debug_spi_shutdown, data); + register_spi_master(spi_config); + register_shutdown(raiden_debug_spi_shutdown, spi_config);
return 0; }