Change in flashrom[master]: raiden_debug_spi.c: Adds context states and helper functions
Hello Brian Nemec, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/41608 to review the following change. Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 241 insertions(+), 94 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/1 diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index b18d580..4de8038 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -141,7 +141,21 @@ enum { GOOGLE_RAIDEN_SPI_PROTOCOL_V1 = 0x01, - GOOGLE_RAIDEN_SPI_PROTOCOL_V2 = 0x02, +}; + +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 { @@ -162,8 +176,8 @@ }; #define PACKET_HEADER_SIZE (2) -#define MAX_PACKET_SIZE (64) -#define PAYLOAD_SIZE_V1 (MAX_PACKET_SIZE - PACKET_HEADER_SIZE) +#define MAX_USB_PACKET_SIZE (64) +#define PAYLOAD_SIZE_V1 (MAX_USB_PACKET_SIZE - PACKET_HEADER_SIZE) /* * Servo Micro has an error where it is capable of acknowledging USB packets @@ -180,11 +194,10 @@ */ #define TRANSFER_TIMEOUT_MS (200 + 800) -struct raiden_debug_spi_data { - struct usb_device *dev; - uint8_t in_ep; - uint8_t out_ep; -}; + +/* + * Version 1 protocol specific attributes + */ typedef struct { int8_t write_count; @@ -198,6 +211,51 @@ 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; +}; + /* * This function will return true when an error code can potentially recover * if we attempt to write SPI data to the device or read from it. We know @@ -213,7 +271,7 @@ * during transfer errors to the device and can be recovered. */ if (USB_SPI_READ_COUNT_INVALID <= error_code && - error_code <= USB_SPI_DISABLED) { + error_code <= USB_SPI_DISABLED) { return false; } } else if (usb_device_is_libusb_error(error_code)) { @@ -231,90 +289,139 @@ 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_v1_t command_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); - - if (write_count > PAYLOAD_SIZE_V1) { - 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_V1) { - 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_v1_t response_packet; - const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); - - ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, - ctx_data->in_ep, - (void*)&response_packet, - read_count + PACKET_HEADER_SIZE, + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->out_ep, + packet->bytes, + packet->packet_size, &transferred, TRANSFER_TIMEOUT_MS)); - if (ret != 0) { + if (status || transferred != packet->packet_size) { + if (!status) { + /* No error was reported, but we didn't transmit the data expected. */ + status = USB_SPI_HOST_TX_BAD_TRANSFER; + } + msg_perr("Raiden: OUT transfer failed\n" + " transferred = %d\n" + " packet_size = %d\n" + " status = %d\n", + transferred, packet->packet_size, status); + + } + return status; +} + +static int receive_packet(const struct raiden_debug_spi_data * ctx_data, + usb_spi_packet_ctx_t* packet) +{ + int received; + int status = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, + ctx_data->in_ep, + packet->bytes, + MAX_USB_PACKET_SIZE, + &received, + TRANSFER_TIMEOUT_MS)); + packet->packet_size = received; + if (status) { msg_perr("Raiden: IN transfer failed\n" - " write_count = %d\n" - " read_count = %d\n", - write_count, read_count); - return ret; + " received = %d\n" + " status = %d\n", + received, 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) +{ + 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 + }; + + /* Reset the write context to the start. */ + write->transmit_index = 0; + + fill_usb_packet(&command, write); + return transmit_packet(ctx_data, &command); +} + +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; + + /* Reset the read context to the start. */ + read->receive_index = 0; + + 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, @@ -323,20 +430,50 @@ 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; - for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; - write_attempt++) { + 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 + }; - status = write_command(flash, write_count, read_count, - write_buffer, read_buffer); + if (write_count > PAYLOAD_SIZE_V1) { + msg_perr("Raiden: Invalid read count\n" + " write count = %d\n" + " max write = %d\n", + read_count, PAYLOAD_SIZE_V1); + return SPI_INVALID_LENGTH; + } + + if (read_count > PAYLOAD_SIZE_V1) { + msg_perr("Raiden: Invalid read count\n" + " read count = %d\n" + " max read = %d\n", + read_count, PAYLOAD_SIZE_V1); + return SPI_INVALID_LENGTH; + } + + for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; + write_attempt++) { + + 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; @@ -344,26 +481,36 @@ programmer_delay(RETRY_INTERVAL_US); continue; } - for (int read_attempt = 0; read_attempt < READ_RETRY_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); + status = read_response_v1(ctx_data, &write_ctx, &read_ctx); + + if (!status) { + if (read_ctx.receive_size == read_ctx.receive_index) { + /* No errors and read is complete. */ + return status; + } else { + 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(RETRY_INTERVAL_US); - } else { - /* We were successful at performing the SPI transfer. */ - return status; } } } -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-MessageType: newchange
Hello build bot (Jenkins), Brian Nemec, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/41608 to look at the new patch set (#2). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 250 insertions(+), 94 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello build bot (Jenkins), Brian Nemec, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/41608 to look at the new patch set (#3). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 250 insertions(+), 94 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#4). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 250 insertions(+), 93 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#5). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 253 insertions(+), 93 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 5 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#6). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 249 insertions(+), 88 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 6 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#7). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 249 insertions(+), 88 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 7 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#8). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 264 insertions(+), 99 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/8 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 8 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... Patch Set 8: Bump -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 8 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Mon, 01 Jun 2020 23:18:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
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/+/41608 to look at the new patch set (#9). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 322 insertions(+), 104 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 9 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#10). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 326 insertions(+), 110 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/10 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 10 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#11). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 326 insertions(+), 110 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/11 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 11 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... Patch Set 11: The changes from the code reviews on the EC side have been moved to this side when relevant to keep the 2 code bases closer in sync. -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 11 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 11 Jun 2020 09:51:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
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/+/41608 to look at the new patch set (#13). Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... raiden_debug_spi.c: Adds context states and helper functions Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol. Adds context states to handle the USB packets, these allow us to simplify the process of loading data from the transmit buffer into a USB packets' data section and from a USB packet to it's receive buffers. These will also keep track of the size of the USB packet allowing a simpler interface to transmit them. Helper functions have been added to help with copying data between the transmit and receive context states to and from the USB packets. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 324 insertions(+), 108 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/13 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 13 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... Patch Set 13: (5 comments) Uh, this is quite large to review in one go. Could you please split this into steps? The commit message has two rather long paragraphs. I'd put them (and the corresponding changes) in separate commits https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@7 PS13, Line 7: Adds Add https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@9 PS13, Line 9: Adds Add https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@14 PS13, Line 14: Adds Add https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@149 PS13, Line 149: using tabs here instead of spaces would make it easier to keep the alignment. it's hard to spot a single-space difference unless the lines are consecutive https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@158 PS13, Line 158: We too much data. Missing a verb? -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 13 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-Comment-Date: Thu, 11 Jun 2020 14:18:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... Patch Set 14: (2 comments)
Patch Set 13:
(5 comments)
Uh, this is quite large to review in one go. Could you please split this into steps?
The commit message has two rather long paragraphs. I'd put them (and the corresponding changes) in separate commits
Yes, I will break it up into separate commits. https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@149 PS13, Line 149:
using tabs here instead of spaces would make it easier to keep the alignment. […] There's pros and cons to using either spaces or tabs for alignment. Tabs allow consistent alignment if everyone uses the same width, but only about half of developers use any particular style. This convention takes slightly more effort when writing, but allows developers to optimize tab size to what they regard as comfortable to read which is done far more.
https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@158 PS13, Line 158: We too much data.
Missing a verb? Yes, it should be 'The host received more data than can fit in read buffer'
-- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 14 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-Comment-Date: Thu, 11 Jun 2020 17:50:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds context states and helper functions ...................................................................... Patch Set 14: (4 comments) https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@489 PS14, Line 489: send_comman Please keep the moving stuff around and rename patches separated from the one with the detail semantic changes. I think that should makes it easier for Angel and I to review. https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@538 PS14, Line 538: write count = %u\n" some of this hunk can go into the previous clean up patch. https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@570 PS14, Line 570: " write count = %u\n" ditto https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@657 PS14, Line 657: *ctx_data = : (struct raiden_debug_spi_d this change can go into the previous clean up patch. -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 14 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-Comment-Date: Mon, 15 Jun 2020 01:02:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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/+/41608 to look at the new patch set (#15). Change subject: raiden_debug_spi.c: Adds transfer context states ...................................................................... raiden_debug_spi.c: Adds transfer context states Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol and easier integration with a unified USB packet context. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 72 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/15 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 15 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#16). Change subject: raiden_debug_spi.c: Adds transfer context states ...................................................................... raiden_debug_spi.c: Adds transfer context states Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol and easier integration with a unified USB packet context. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 72 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 16 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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/+/41608 to look at the new patch set (#19). Change subject: raiden_debug_spi.c: Adds transfer context states ...................................................................... raiden_debug_spi.c: Adds transfer context states Adds context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol and easier integration with a unified USB packet context. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 67 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/19 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 19 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds transfer context states ...................................................................... Patch Set 19: (4 comments) https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@489 PS14, Line 489: send_comman
Please keep the moving stuff around and rename patches separated from the one with the detail semant […] Done
https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@538 PS14, Line 538: write count = %u\n"
some of this hunk can go into the previous clean up patch. Done
https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@570 PS14, Line 570: " write count = %u\n"
ditto Done
https://review.coreboot.org/c/flashrom/+/41608/14/raiden_debug_spi.c@657 PS14, Line 657: *ctx_data = : (struct raiden_debug_spi_d
this change can go into the previous clean up patch. Done
-- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 19 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 19 Jul 2020 22:58:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: comment
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Adds transfer context states ...................................................................... Patch Set 20: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 20 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Jul 2020 00:46:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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/+/41608 to look at the new patch set (#21). Change subject: raiden_debug_spi.c: Add transfer context states ...................................................................... raiden_debug_spi.c: Add transfer context states Add context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol and easier integration with a unified USB packet context. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c --- M raiden_debug_spi.c 1 file changed, 67 insertions(+), 32 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/41608/21 -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 21 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Add transfer context states ...................................................................... Patch Set 21: (5 comments) https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@7 PS13, Line 7: Adds
Add Done
https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@9 PS13, Line 9: Adds
Add Done
https://review.coreboot.org/c/flashrom/+/41608/13//COMMIT_MSG@14 PS13, Line 14: Adds
Add Done
https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@149 PS13, Line 149:
There's pros and cons to using either spaces or tabs for alignment. […] .
https://review.coreboot.org/c/flashrom/+/41608/13/raiden_debug_spi.c@158 PS13, Line 158: We too much data.
Yes, it should be 'The host received more data than can fit in read buffer' Fixed in next CL where this was moved to.
-- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 21 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Jul 2020 03:58:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Brian Nemec <bnemec@google.com> Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/41608 ) Change subject: raiden_debug_spi.c: Add transfer context states ...................................................................... raiden_debug_spi.c: Add transfer context states Add context states to handle the read and write buffers as transmit and receive states. These are used to keep track of the number of bytes transmitted and received allowing future support of multi-packet messages in the v2 protocol and easier integration with a unified USB packet context. 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: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Reviewed-on: https://review.coreboot.org/c/flashrom/+/41608 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, 67 insertions(+), 32 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index b43fc91..d6474ec 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -203,6 +203,24 @@ uint8_t data[PAYLOAD_SIZE_V1]; } __attribute__((packed)); +struct usb_spi_transmit_ctx { + /* Buffer we are reading data from. */ + const uint8_t *buffer; + /* Number of bytes in the transfer. */ + size_t transmit_size; + /* Number of bytes transferred. */ + size_t transmit_index; +}; + +struct usb_spi_receive_ctx { + /* Buffer we are writing data into. */ + uint8_t *buffer; + /* Number of bytes in the transfer. */ + size_t receive_size; + /* Number of bytes transferred. */ + size_t receive_index; +}; + /* * This function will return true when an error code can potentially recover * if we attempt to write SPI data to the device or read from it. We know @@ -241,13 +259,16 @@ * a USB SPI transfer. Write and read counts and payloads to write from * the write_buffer are transmitted to the device. * + * @param flash Flash context storing SPI capabilities and USB device + * information. + * @param write Write context of data to transmit and write payload. + * @param read Read context of data to receive and read buffer. + * * @returns Returns status code with 0 on success. */ static int write_command_v1(const struct flashctx *flash, - unsigned int write_count, - unsigned int read_count, - const unsigned char *write_buffer, - unsigned char *read_buffer) + struct usb_spi_transmit_ctx *write, + struct usb_spi_receive_ctx *read) { int transferred; @@ -255,28 +276,28 @@ struct usb_spi_command_v1 command_packet; const struct raiden_debug_spi_data * ctx_data = get_raiden_data_from_context(flash); - command_packet.write_count = write_count; - command_packet.read_count = read_count; + command_packet.write_count = write->transmit_size; + command_packet.read_count = read->receive_size; - memcpy(command_packet.data, write_buffer, write_count); + memcpy(command_packet.data, write->buffer, write->transmit_size); ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, ctx_data->out_ep, (void*)&command_packet, - write_count + PACKET_HEADER_SIZE, + write->transmit_size + 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); + " write_count = %zu\n" + " read_count = %zu\n", + write->transmit_size, read->receive_size); 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); + if ((unsigned) transferred != write->transmit_size + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Write failure (wrote %d, expected %zu)\n", + transferred, write->transmit_size + PACKET_HEADER_SIZE); return 0x10001; } @@ -288,13 +309,16 @@ * transfer. Status codes from the transfer and any read payload are copied * to the read_buffer. * + * @param flash Flash context storing SPI capabilities and USB device + * information. + * @param write Write context of data to transmit and write payload. + * @param read Read context of data to receive and read buffer. + * * @returns Returns status code with 0 on success. */ static int read_response_v1(const struct flashctx *flash, - unsigned int write_count, - unsigned int read_count, - const unsigned char *write_buffer, - unsigned char *read_buffer) + struct usb_spi_transmit_ctx *write, + struct usb_spi_receive_ctx *read) { int transferred; int ret; @@ -304,24 +328,24 @@ ret = LIBUSB(libusb_bulk_transfer(ctx_data->dev->handle, ctx_data->in_ep, (void*)&response_packet, - read_count + PACKET_HEADER_SIZE, + read->receive_size + PACKET_HEADER_SIZE, &transferred, TRANSFER_TIMEOUT_MS)); if (ret != 0) { msg_perr("Raiden: IN transfer failed\n" - " write_count = %d\n" - " read_count = %d\n", - write_count, read_count); + " write_count = %zu\n" + " read_count = %zu\n", + write->transmit_size, read->receive_size); return ret; } - if ((unsigned) transferred != read_count + PACKET_HEADER_SIZE) { - msg_perr("Raiden: Read failure (read %d, expected %d)\n", - transferred, read_count + PACKET_HEADER_SIZE); + if ((unsigned) transferred != read->receive_size + PACKET_HEADER_SIZE) { + msg_perr("Raiden: Read failure (read %d, expected %zu)\n", + transferred, read->receive_size + PACKET_HEADER_SIZE); return 0x10002; } - memcpy(read_buffer, response_packet.data, read_count); + memcpy(read->buffer, response_packet.data, read->receive_size); return response_packet.status_code; } @@ -348,6 +372,15 @@ { int status = -1; + struct usb_spi_transmit_ctx write_ctx = { + .buffer = write_buffer, + .transmit_size = write_count + }; + struct usb_spi_receive_ctx read_ctx = { + .buffer = read_buffer, + .receive_size = read_count + }; + if (write_count > PAYLOAD_SIZE_V1) { msg_perr("Raiden: Invalid write count\n" " write count = %u\n" @@ -367,17 +400,19 @@ for (unsigned int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; write_attempt++) { - status = write_command_v1(flash, write_count, read_count, - write_buffer, read_buffer); + + status = write_command_v1(flash, &write_ctx, &read_ctx); if (status) { /* Write operation failed. */ msg_perr("Raiden: Write command failed\n" " write count = %u\n" " read count = %u\n" + " transmitted bytes = %zu\n" " write attempt = %u\n" " status = 0x%05x\n", - write_count, read_count, + + write_count, read_count, write_ctx.transmit_index, write_attempt + 1, status); if (!retry_recovery(status)) { /* Reattempting will not result in a recovery. */ @@ -389,18 +424,18 @@ for (unsigned int read_attempt = 0; read_attempt < READ_RETRY_ATTEMPTS; read_attempt++) { - status = read_response_v1(flash, write_count, read_count, - write_buffer, read_buffer); + status = read_response_v1(flash, &write_ctx, &read_ctx); if (status) { /* Read operation failed. */ msg_perr("Raiden: Read response failed\n" " write count = %u\n" " read count = %u\n" + " received bytes = %zu\n" " write attempt = %u\n" " read attempt = %u\n" " status = 0x%05x\n", - write_count, read_count, + 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. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/41608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic6eea82ffc604ec56278f7aaa0deafe0cf75973c Gerrit-Change-Number: 41608 Gerrit-PatchSet: 22 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (3)
-
Angel Pons (Code Review) -
Brian Nemec (Code Review)
-
Edward O'Callaghan (Code Review)