Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Add protocol based configuration to init ......................................................................
Patch Set 23:
(7 comments)
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@159 PS20, Line 159: /* We too much data. */
Oops
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@211 PS20, Line 211: bugs
Where? We might as well fix them
The definition of the field itself creates this problem:
struct spi_master { uint32_t features; unsigned int max_data_read; // (Ideally,) maximum data read size in one go (excluding opcode+address). unsigned int max_data_write; // (Ideally,) maximum data write size in one go (excluding opcode+address). int (*command)(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int (*multicommand)(const struct flashctx *flash, struct spi_command *cmds);
/* Optimized functions for this master */ int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*write_aai)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); const void *data; };
The "(Ideally,) maximum data read size in one go (excluding opcode+address)." is a bad way to structure this variable. The SPI masters don't need to know what flash they are connected to so any field that relies on them knowing the maximum length of the opcode+address is going to introduce problems. A SPI master can provide it's maximum SPI transfer size easily using a datasheet or by identifying other limiting factors like a buffer, but there isn't a good way to identify the maximum op code size for a given platform. Either they support a maximum transfer length much larger than the flashes would use or they artificially subtract several bytes for the maximum operation size which depending on the flash driver may or may not be unused.
Due to WFH, I don't have access to all of the hardware, but different ChromeOS laptops used different flash drivers that had different interpretations for this length. The Nami platform interpreted the max_data_read / max_data_write has the maximum packet size including packet lengths, I think Atlas used a different interpretations where it was the literal max data size and added 4 bytes of header sizes on some of it's transfers. I'm not certain what chip is used in the other platform or how many flash chips contain that interpretation for the variable.
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@659 PS20, Line 659: Configures
Configure
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@675 PS20, Line 675: switch
space after switch
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@676 PS20, Line 676: case GOOGLE_RAIDEN_SPI_PROTOCOL_V1: : /* : * Protocol V1 is supported by adjusting the max data : * read and write sizes which results in no continue packets. : */ : spi_config->command = send_command_v1; : ctx_data->max_spi_write_count = SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX; : break; : default: : msg_pdbg("Raiden: Unknown USB SPI protocol version = %d", : ctx_data->protocol_version); : return USB_SPI_HOST_INIT_FAILURE;
We don't indent case labels, AFAIK
Done
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@729 PS20, Line 729: free(ctx_data);
This should go with the patch that added the other `free(ctx_data)` call
That was months ago.
https://review.coreboot.org/c/flashrom/+/41532/20/raiden_debug_spi.c@917 PS20, Line 917: spi
SPI
Done