Hello Brian Nemec,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/41596
to review the following change.
Change subject: raiden_debug_spi.c: Renaming Protocol V1 specific fields ......................................................................
raiden_debug_spi.c: Renaming Protocol V1 specific fields
Renames the structures from the USB SPI which are specific to the V1 protocol.
BUG=b:139058552 BRANCH=none TEST=Builds
Signed-off-by: Brian J. Nemec bnemec@chromium.com Change-Id: I70b43af50d872d850dae287d99bcd768107a1cad --- M raiden_debug_spi.c 1 file changed, 27 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/41596/1
diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index ac74c0b..5259981 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -123,7 +123,11 @@
#define GOOGLE_VID (0x18D1) #define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) -#define GOOGLE_RAIDEN_SPI_PROTOCOL (0x01) + +enum { + GOOGLE_RAIDEN_SPI_PROTOCOL_V1 = 0x01, + GOOGLE_RAIDEN_SPI_PROTOCOL_V2 = 0x02, +};
enum usb_spi_error { USB_SPI_SUCCESS = 0x0000, @@ -144,7 +148,7 @@
#define PACKET_HEADER_SIZE (2) #define MAX_PACKET_SIZE (64) -#define PAYLOAD_SIZE (MAX_PACKET_SIZE - PACKET_HEADER_SIZE) +#define PAYLOAD_SIZE_V1 (MAX_PACKET_SIZE - PACKET_HEADER_SIZE)
/* * Servo Micro has an error where it is capable of acknowledging USB packets @@ -152,9 +156,9 @@ * 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_RETY_ATTEMPTS (3) +#define READ_RETY_ATTEMPTS (3) +#define RETY_INTERVAL_US (100 * 1000)
/* * This timeout is so large because the Raiden SPI timeout is 800ms. @@ -171,13 +175,13 @@ 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;
/* * This function will return true when an error code can potentially recover @@ -221,15 +225,15 @@
int transferred; int ret; - usb_spi_command_t command_packet; + 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) { + if (write_count > PAYLOAD_SIZE_V1) { msg_perr("Raiden: Invalid write_count of %d\n", write_count); return SPI_INVALID_LENGTH; }
- if (read_count > PAYLOAD_SIZE) { + if (read_count > PAYLOAD_SIZE_V1) { msg_perr("Raiden: Invalid read_count of %d\n", read_count); return SPI_INVALID_LENGTH; } @@ -270,7 +274,7 @@ { int transferred; int ret; - usb_spi_response_t response_packet; + 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, @@ -362,17 +366,17 @@ * 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) +#define MAX_DATA_SIZE (PAYLOAD_SIZE_V1 - 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 = 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, };
static int match_endpoint(struct libusb_endpoint_descriptor const *descriptor, @@ -494,7 +498,7 @@ usb_match_value_default(&match.vid, GOOGLE_VID); usb_match_value_default(&match.class, LIBUSB_CLASS_VENDOR_SPEC); usb_match_value_default(&match.subclass, GOOGLE_RAIDEN_SPI_SUBCLASS); - usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL); + usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL_V1);
ret = LIBUSB(libusb_init(NULL)); if (ret != 0) {
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Renaming Protocol V1 specific fields ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Renaming Protocol V1 specific fields ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG@7 PS1, Line 7: Renaming Rename
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG@9 PS1, Line 9: Renames Rename
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c@372 PS1, Line 372: .features = SPI_MASTER_4BA, Why do tabs become spaces here?
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Renaming Protocol V1 specific fields ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c@372 PS1, Line 372: .features = SPI_MASTER_4BA,
Why do tabs become spaces here?
So they are consistently aligned. Every other project I work with switches between 4 or 8 space tabs making tabs terrible for alignment since one of the 2 options will always break. So I try to adopt the tabs for indentation, spaces for alignment convention.
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/+/41596
to look at the new patch set (#2).
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
raiden_debug_spi.c: Rename Protocol V1 specific fields
Rename the structures from the USB SPI which are specific to the V1 protocol.
BUG=b:139058552 BRANCH=none TEST=Builds
Signed-off-by: Brian J. Nemec bnemec@chromium.com Change-Id: I70b43af50d872d850dae287d99bcd768107a1cad --- M raiden_debug_spi.c 1 file changed, 27 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/41596/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41596/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41596/3/raiden_debug_spi.c@159 PS3, Line 159: #define WRITE_RETY_ATTEMPTS (3) Why was a space added here?
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/+/41596
to look at the new patch set (#4).
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
raiden_debug_spi.c: Rename Protocol V1 specific fields
Rename the structures from the USB SPI which are specific to the V1 protocol.
BUG=b:139058552 BRANCH=none TEST=Builds
Signed-off-by: Brian J. Nemec bnemec@chromium.com Change-Id: I70b43af50d872d850dae287d99bcd768107a1cad --- M raiden_debug_spi.c 1 file changed, 24 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/41596/4
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/flashrom/+/41596/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41596/3/raiden_debug_spi.c@159 PS3, Line 159: #define WRITE_RETY_ATTEMPTS (3)
Why was a space added here?
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
Patch Set 4: Code-Review+2
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG@7 PS1, Line 7: Renaming
Rename
Done
https://review.coreboot.org/c/flashrom/+/41596/1//COMMIT_MSG@9 PS1, Line 9: Renames
Rename
Done
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41596/1/raiden_debug_spi.c@372 PS1, Line 372: .features = SPI_MASTER_4BA,
So they are consistently aligned. […]
.
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/41596 )
Change subject: raiden_debug_spi.c: Rename Protocol V1 specific fields ......................................................................
raiden_debug_spi.c: Rename Protocol V1 specific fields
Rename the structures from the USB SPI which are specific to the V1 protocol.
BUG=b:139058552 BRANCH=none TEST=Builds
Signed-off-by: Brian J. Nemec bnemec@chromium.com Change-Id: I70b43af50d872d850dae287d99bcd768107a1cad Reviewed-on: https://review.coreboot.org/c/flashrom/+/41596 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M raiden_debug_spi.c 1 file changed, 24 insertions(+), 20 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 ac74c0b..c1e2c2f 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -123,7 +123,11 @@
#define GOOGLE_VID (0x18D1) #define GOOGLE_RAIDEN_SPI_SUBCLASS (0x51) -#define GOOGLE_RAIDEN_SPI_PROTOCOL (0x01) + +enum { + GOOGLE_RAIDEN_SPI_PROTOCOL_V1 = 0x01, + GOOGLE_RAIDEN_SPI_PROTOCOL_V2 = 0x02, +};
enum usb_spi_error { USB_SPI_SUCCESS = 0x0000, @@ -144,7 +148,7 @@
#define PACKET_HEADER_SIZE (2) #define MAX_PACKET_SIZE (64) -#define PAYLOAD_SIZE (MAX_PACKET_SIZE - PACKET_HEADER_SIZE) +#define PAYLOAD_SIZE_V1 (MAX_PACKET_SIZE - PACKET_HEADER_SIZE)
/* * Servo Micro has an error where it is capable of acknowledging USB packets @@ -171,13 +175,13 @@ 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;
/* * This function will return true when an error code can potentially recover @@ -221,15 +225,15 @@
int transferred; int ret; - usb_spi_command_t command_packet; + 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) { + if (write_count > PAYLOAD_SIZE_V1) { msg_perr("Raiden: Invalid write_count of %d\n", write_count); return SPI_INVALID_LENGTH; }
- if (read_count > PAYLOAD_SIZE) { + if (read_count > PAYLOAD_SIZE_V1) { msg_perr("Raiden: Invalid read_count of %d\n", read_count); return SPI_INVALID_LENGTH; } @@ -270,7 +274,7 @@ { int transferred; int ret; - usb_spi_response_t response_packet; + 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, @@ -362,17 +366,17 @@ * 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) +#define MAX_DATA_SIZE (PAYLOAD_SIZE_V1 - 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 = 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, };
static int match_endpoint(struct libusb_endpoint_descriptor const *descriptor, @@ -494,7 +498,7 @@ usb_match_value_default(&match.vid, GOOGLE_VID); usb_match_value_default(&match.class, LIBUSB_CLASS_VENDOR_SPEC); usb_match_value_default(&match.subclass, GOOGLE_RAIDEN_SPI_SUBCLASS); - usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL); + usb_match_value_default(&match.protocol, GOOGLE_RAIDEN_SPI_PROTOCOL_V1);
ret = LIBUSB(libusb_init(NULL)); if (ret != 0) {
-
Angel Pons (Code Review) -
Brian Nemec (Code Review)
-
Edward O'Callaghan (Code Review)