Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/77999?usp=email )
Change subject: raiden: Support target index with generic REQ_ENABLE ......................................................................
Patch Set 2:
(3 comments)
Patchset:
PS2: I will address the smaller comments, once we have settled on the way ahead wrt. the meaning of `request_enable` and the `get_target()` signature.
File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/77999/comment/8065aaee_1d660c98 : PS2, Line 1596: request_enable & 0xFF
Does this still work for target values EC and AP ?
See below. Yes, the EC and AP values fit inside 8 bits. (The bRequest parameter here is `uint8_t`, so the `& 0xFF` is actually a no-op included for clarity, as with my change `request_enable` can for the first time be non-zero in the second byte.)
https://review.coreboot.org/c/flashrom/+/77999/comment/9cdbcfbd_03d2d743 : PS2, Line 1596: request_enable & 0xFF, : (request_enable >> 8) & 0xFF,
A question: why do you initialise `request_enable` to a value that you never use by itself, just as […]
It was already the case that some Raiden devices had multiple SPI ports, or at least knew multiple ways of controlling electrical mux'es or buffers. These SPI ports would be known as "ap" or "ec", and particular values of bRequest was used to access one or the other.
I have been developing firmware for another debugger using the Raiden protocol, which has three or more SPI ports, and they do not have any particular names, just numbers. I have extended the Raiden protocol such that the lower byte of `wValue` can indicate which one of such an array of similar SPI ports to use (while bRequest is set to the "default" value), and the goal of this CL is to be able to access that functionality.
The `int get_target()` method used to return a 8-bit value to be passed in the `bRequest` field of the control request to "open" the SPI port (while the wValue field was always set to zero). In a somewhat hacky way, I used the upper bits of the return value from `get_target()` to convey the desired value of wValue. Perhaps I should instead change the signature into `void get_target(const struct programmer_cfg *cfg, uint8_t *request_enable, uint16_t *request_parameter)` such that the function can "return" two separate values.
Let me know if you prefer that, and I can modify my CL.
-
Jes Klinke (Code Review)