Change in flashrom[master]: raiden_debug_spi.c: Cleanup of the USB SPI protocol
Hello Brian Nemec, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/41597 to review the following change. Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Cleanup of the USB SPI protocol Performs some cleanup of the USB SPI protocol: * Minor cleanup of the comment descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formating changes. * Fixing typos in constants associated with the retry mechanism * Removed an explicit comparison of 0 in a conditional BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 38 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/1 diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 5259981..b18d580 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -42,35 +42,50 @@ * * 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.h * chip/stm32/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. + * + * + * Message Packets: + * + * 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. * Due to data alignment constraints, this must be an * even number of bytes unless this is the final packet. * - * Response Packet: + * Response Packet (Device to Host): + * + * USB SPI response, containing the status code and any bytes of the + * read payload. + * * +-------------+-----------------------+ * | status : 2B | read payload : <= 62B | * +-------------+-----------------------+ @@ -81,8 +96,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 @@ -100,7 +115,7 @@ * * 0x00000: Status code success. * 0x00001-0x0FFFF: Error code returned by the USB SPI device. - * 0x10001-0x1FFFF: The host has determined an error has occurred. + * 0x10001-0x1FFFF: USB SPI Host error codes * 0x20001-0x20063 Lower bits store the positive value representation * of the libusb_error enum. See the libusb documentation: * http://libusb.sourceforge.net/api-1.0/group__misc.html @@ -156,9 +171,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_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. @@ -310,7 +325,7 @@ { int status = -1; - for (int write_attempt = 0; write_attempt < WRITE_RETY_ATTEMPTS; + for (int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; write_attempt++) { status = write_command(flash, write_count, read_count, @@ -326,15 +341,15 @@ /* 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); - if (status != 0) { + if (status) { /* Read operation failed. */ msg_perr("Raiden: Read response failed\n" "Write attempt = %d\n" @@ -345,7 +360,7 @@ /* 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; -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 1 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-MessageType: newchange
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 1 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, 21 May 2020 02:41:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... Patch Set 1: Fixed a comment -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 1 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:54:23 +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/+/41597 to look at the new patch set (#2). Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Cleanup of the USB SPI protocol Performs some cleanup of the USB SPI protocol: * Minor cleanup of the comment descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formating changes. * Fixing typos in constants associated with the retry mechanism * Removed an explicit comparison of 0 in a conditional BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 38 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 2 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/+/41597 ) Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... Patch Set 2: Code-Review+2 a -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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 14:10:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Cleanup of the USB SPI protocol ...................................................................... Patch Set 2: (5 comments) Some nits about the commit message, which can be fixed using Gerrit's editor (see EDIT button below the commit message at the top of this change) https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@7 PS2, Line 7: Cleanup of Clean up https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@9 PS2, Line 9: Performs Perform https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@14 PS2, Line 14: formating format*ting https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@16 PS2, Line 16: Fixing Fix https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@18 PS2, Line 18: Removed Remove -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 2 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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 14:12:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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/+/41597 to look at the new patch set (#3). Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up of the USB SPI protocol Perform some clean up of the USB SPI protocol: * Minor clean up of the comment descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism * Remove an explicit comparison of 0 in a conditional BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 38 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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/+/41597 ) Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... Patch Set 3: (5 comments)
Patch Set 2:
(5 comments)
Some nits about the commit message, which can be fixed using Gerrit's editor (see EDIT button below the commit message at the top of this change)
Commit updated https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@7 PS2, Line 7: Cleanup of
Clean up Done
https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@9 PS2, Line 9: Performs
Perform Done
https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@14 PS2, Line 14: formating
format*ting Done
https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@16 PS2, Line 16: Fixing
Fix Done
https://review.coreboot.org/c/flashrom/+/41597/2//COMMIT_MSG@18 PS2, Line 18: Removed
Remove Done
-- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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 17:21:10 +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/+/41597 ) Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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, 15 Jun 2020 00:57:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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/+/41597 to look at the new patch set (#4). Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up of the USB SPI protocol Perform some clean up of the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up of the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Cleans declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 136 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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/+/41597 ) Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... Patch Set 4: Code-Review+1 (3 comments) https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@7 PS4, Line 7: of drop "of" https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@19 PS4, Line 19: Cleans Clean https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@21 PS4, Line 21: Updates Update -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 4 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Fri, 17 Jul 2020 09:31:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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/+/41597 to look at the new patch set (#5). Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up of the USB SPI protocol Perform some clean up of the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Clean declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. Overall to keep V1 and V2 closer aligned and reduce future changes in the V1 code. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 136 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 5 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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/+/41597 to look at the new patch set (#6). Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up of the USB SPI protocol Perform some clean up of the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Clean declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. Overall to keep V1 and V2 closer aligned and reduce future changes in the V1 code. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 136 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 6 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
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/+/41597 to look at the new patch set (#7). Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up of the USB SPI protocol Perform some clean up of the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Clean declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. Overall to keep V1 and V2 closer aligned and reduce future changes in the V1 code. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 126 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 7 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Clean up of the USB SPI protocol ...................................................................... Patch Set 7: (3 comments) https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@7 PS4, Line 7: of
drop "of" Done
https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@19 PS4, Line 19: Cleans
Clean Done
https://review.coreboot.org/c/flashrom/+/41597/4//COMMIT_MSG@21 PS4, Line 21: Updates
Update Done
-- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 7 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 19 Jul 2020 22:55:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Gerrit-MessageType: comment
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/+/41597 to look at the new patch set (#8). Change subject: raiden_debug_spi.c: Clean up the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up the USB SPI protocol Perform some clean up the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Clean declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. Overall to keep V1 and V2 closer aligned and reduce future changes in the V1 code. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df --- M raiden_debug_spi.c 1 file changed, 126 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/41597/8 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 8 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Clean up the USB SPI protocol ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 8 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 20 Jul 2020 00:42:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/41597 ) Change subject: raiden_debug_spi.c: Clean up the USB SPI protocol ...................................................................... raiden_debug_spi.c: Clean up the USB SPI protocol Perform some clean up the USB SPI protocol 1 prior to adding protocol 2 to improve consistency and correct minor issues. * Minor clean up the comments descriptor for the protocol. This adds the location of another relevant file, corrects the omission of one of the protocol modes, makes the direction of the packets explicit, and minor formatting changes. * Fix typos in constants associated with the retry mechanism. * Clean declarations to match the EC code formats. * Updates the error message formatting so protocol V1 closely matches the V2 protocol for consistency. * Minor changes to the structure, moving validation of the arguments earlier in the transfer. Overall to keep V1 and V2 closer aligned and reduce future changes in the V1 code. BUG=b:139058552 BRANCH=none TEST=Builds Signed-off-by: Brian J. Nemec <bnemec@chromium.com> Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Reviewed-on: https://review.coreboot.org/c/flashrom/+/41597 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, 126 insertions(+), 65 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 c1e2c2f..b43fc91 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -42,35 +42,50 @@ * * 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.h * chip/stm32/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. + * + * + * Message Packets: + * + * 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. Full duplex + * mode is enabled with UINT8_MAX * * write payload: Up to 62 bytes of data to write to SPI, the total * length of all TX packets must match write count. * Due to data alignment constraints, this must be an * even number of bytes unless this is the final packet. * - * Response Packet: + * Response Packet (Device to Host): + * + * USB SPI response, containing the status code and any bytes of the + * read payload. + * * +-------------+-----------------------+ * | status : 2B | read payload : <= 62B | * +-------------+-----------------------+ @@ -81,8 +96,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 @@ -94,13 +109,14 @@ * alignment constraints, this must be a even number * of bytes unless this is the final packet. * + * * USB Error Codes: * * send_command return codes have the following format: * * 0x00000: Status code success. * 0x00001-0x0FFFF: Error code returned by the USB SPI device. - * 0x10001-0x1FFFF: The host has determined an error has occurred. + * 0x10001-0x1FFFF: Error code returned by the USB SPI host. * 0x20001-0x20063 Lower bits store the positive value representation * of the libusb_error enum. See the libusb documentation: * http://libusb.sourceforge.net/api-1.0/group__misc.html @@ -130,13 +146,13 @@ }; 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, + USB_SPI_UNKNOWN_ERROR = 0x8000, }; enum raiden_debug_spi_request { @@ -147,8 +163,8 @@ }; #define PACKET_HEADER_SIZE (2) -#define MAX_PACKET_SIZE (64) -#define PAYLOAD_SIZE_V1 (MAX_PACKET_SIZE - PACKET_HEADER_SIZE) +#define USB_MAX_PACKET_SIZE (64) +#define PAYLOAD_SIZE_V1 (USB_MAX_PACKET_SIZE - PACKET_HEADER_SIZE) /* * Servo Micro has an error where it is capable of acknowledging USB packets @@ -156,9 +172,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_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. @@ -171,17 +187,21 @@ uint8_t out_ep; }; -typedef struct { - int8_t write_count; - /* -1 Indicates readback all on halfduplex compliant devices. */ - int8_t read_count; - uint8_t data[PAYLOAD_SIZE_V1]; -} __attribute__((packed)) usb_spi_command_v1_t; +/* + * Version 1 protocol specific attributes + */ -typedef struct { +struct usb_spi_command_v1 { + uint8_t write_count; + /* UINT8_MAX indicates full duplex mode on compliant devices. */ + uint8_t read_count; + uint8_t data[PAYLOAD_SIZE_V1]; +} __attribute__((packed)); + +struct usb_spi_response_v1 { uint16_t status_code; uint8_t data[PAYLOAD_SIZE_V1]; -} __attribute__((packed)) usb_spi_response_v1_t; +} __attribute__((packed)); /* * This function will return true when an error code can potentially recover @@ -216,7 +236,14 @@ return (const struct raiden_debug_spi_data *)flash->mst->spi.data; } -static int write_command(const struct flashctx *flash, +/* + * Version 1 Protocol: Responsible for constructing the packet to start + * a USB SPI transfer. Write and read counts and payloads to write from + * the write_buffer are transmitted to the device. + * + * @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, @@ -225,19 +252,9 @@ int transferred; int ret; - usb_spi_command_v1_t command_packet; + struct usb_spi_command_v1 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 (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; @@ -266,7 +283,14 @@ return 0; } -static int read_response(const struct flashctx *flash, +/* + * Version 1 Protocol: Responsible for reading the response of the USB SPI + * transfer. Status codes from the transfer and any read payload are copied + * to the 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, @@ -274,7 +298,7 @@ { int transferred; int ret; - usb_spi_response_v1_t response_packet; + struct usb_spi_response_v1 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, @@ -302,7 +326,21 @@ return response_packet.status_code; } -static int send_command(const struct flashctx *flash, +/* + * Version 1 Protocol: Sets up a USB SPI transfer, transmits data to the device, + * reads the status code and any payload from the device. This will also handle + * recovery if an error has occurred. + * + * @param flash Flash context storing SPI capabilities and USB device + * information. + * @param write_count Number of bytes to write + * @param read_count Number of bytes to read + * @param write_buffer Address of write buffer + * @param read_buffer Address of buffer to store read data + * + * @returns Returns status code with 0 on success. + */ +static int send_command_v1(const struct flashctx *flash, unsigned int write_count, unsigned int read_count, const unsigned char *write_buffer, @@ -310,42 +348,65 @@ { int status = -1; - for (int write_attempt = 0; write_attempt < WRITE_RETY_ATTEMPTS; + if (write_count > PAYLOAD_SIZE_V1) { + msg_perr("Raiden: Invalid write count\n" + " write count = %u\n" + " max write = %d\n", + write_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 (unsigned int write_attempt = 0; write_attempt < WRITE_RETRY_ATTEMPTS; write_attempt++) { - status = write_command(flash, write_count, read_count, + status = write_command_v1(flash, write_count, read_count, write_buffer, read_buffer); 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 = %u\n" + " read count = %u\n" + " write attempt = %u\n" + " status = 0x%05x\n", + write_count, read_count, + 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 (unsigned int read_attempt = 0; read_attempt < READ_RETRY_ATTEMPTS; + read_attempt++) { - status = read_response(flash, write_count, read_count, + status = read_response_v1(flash, write_count, read_count, write_buffer, read_buffer); - if (status != 0) { + 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 = %u\n" + " read count = %u\n" + " write attempt = %u\n" + " read attempt = %u\n" + " status = 0x%05x\n", + write_count, read_count, + 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; @@ -372,7 +433,7 @@ .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_SIZE, .max_data_write = MAX_DATA_SIZE, - .command = send_command, + .command = send_command_v1, .multicommand = default_spi_send_multicommand, .read = default_spi_read, .write_256 = default_spi_write_256, -- To view, visit https://review.coreboot.org/c/flashrom/+/41597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I17e62dabee2724eecf8d5a1a7827f06f0c7514df Gerrit-Change-Number: 41597 Gerrit-PatchSet: 10 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.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: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (3)
-
Angel Pons (Code Review) -
Brian Nemec (Code Review)
-
Edward O'Callaghan (Code Review)