Brian Nemec has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(11 comments)
Patch Set 3:
(10 comments)
The general direction looks good however these changes are *way* way too big to review and definitely can be broken down into sizable chunks that each stand on their own merit.
Totally fair, I can break this down further if we need.
I'd also like to have just a single quick discussion about the tabs/spaces usage here so it's not scattered in 20 messages and multiple CL's. My goal with the indentation has been to use tabs to represent indentation levels and to use spaces whenever we are aligning text, like to the function declarations or as parameters, especially in the multi-line strings. This does result in more characters but it's an indentation / alignment style that is robust to different tab widths.
Same idea as https://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@140 PS3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1
probably a enum
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
the spi master already has these fields, you should fill in the data there.
These are directly replacing 2 constants that we've been using:
We need to make sure that flashrom will not attempt a SPI transfer larger than the platform will support. It looks like some parts of flashrom will treat the max_data_read/write values as the maximum size of the SPI transfer and construct a message smaller than it, other parts interpret that line to mean that's the maximum number of bytes in a message excluding the header meaning you end up with slightly larger packets, I've seen up to 61 bytes.
To get around this issue, we have been telling the rest of flashrom that it's maximum capacity is equal to the maximum SPI packet - JEDEC_BYTE_PROGRAM_OUTSIZE. We've been using 2 values to keep track of these fields before in the form of 'max_data_read = (PAYLOAD_SIZE - JEDEC_BYTE_PROGRAM_OUTSIZE)' but we've been comparing the 'read_count' against PAYLOAD_SIZE.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@307 PS3, Line 307:
Can use tabs here
My goal has been to indent with tabs and align with spaces. That style is the most robust to different editor / preferences since changing tab width keeps text in alignment with the functions above that they are using.
This comment applies to this line and many of the other similar lines, although I do have a few lines where I accidentally used spaces for indentation too so those need to be fixed.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@438 PS3, Line 438: ;
\n
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@448 PS3, Line 448: if (write_count > ctx_data->max_spi_write_count) {
validation should happen before packing structs
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@518 PS3, Line 518: /* We were successful at performing the SPI transfer. */ : return status;
This can be moved at line 498: […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@529 PS3, Line 529: .max_data_read = 0, : .max_data_write = 0,
not sure why you are not using these fields and putting them into the local state data context?
See comment above near the 'struct raiden_debug_spi_data'
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@584 PS3, Line 584: : ctx_data->protocol_version = : ctx_data->dev->interface_descriptor->bInterfaceProtocol; : switch(ctx_data->protocol_version) {
this is its own function, `enum protocol_rev_t raiden_debug_detect_protocol_rev(struct usb_device *d […]
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@592 PS3, Line 592: : ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX;
have two spi_master_raiden_debug spi_master struct's for each protocol and register the one with the […]
See comment about configure_protocol() from below: Short answer protocol 2 allows SPI transfers much larger than what platforms support and some of our chips have different maximum SPI transfer sizes.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@620 PS3, Line 620:
more tabs and spaces mixed throughout this file.
Ack
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
no, just declare the state you want (read/write max_size in this case) for each path and choose the […]
We need to communicate with the device in order to identify how large of a SPI transfer is supported. Some devices like the STM32's used in Servo Micros and C2D2 work for SPI transactions over 256 bytes, others like the Cr50's support 128 bytes. We could do a mapping of the protocol version number at this point to resolve that as a way around this issue:
For instance 1 is the existing path, 2 could be a value over 256 that the STM32F0's support, 3 could be 128 bytes.
It'd be a bit less flexible since changing the packet limit burns another version number, but there's some pros to having a simpler init process.