7 comments:
Patch Set #20, Line 159: /* We too much data. */
Oops
Done
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.
Patch Set #20, Line 659: Configures
Configure
Done
Patch Set #20, Line 675: switch
space after switch
Done
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
Patch Set #20, Line 729: free(ctx_data);
This should go with the patch that added the other `free(ctx_data)` call
That was months ago.
SPI
Done
To view, visit change 41532. To unsubscribe, or for help writing mail filters, visit settings.