Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 )
Change subject: dirtyjtag: Add DirtyJTAG programmer ......................................................................
Patch Set 6: Code-Review+1
(5 comments)
Patchset:
PS6:
Hi Nico, thanks for your feedback. […]
Depends on the reader, I guess. I was a little confused by the double memcpy() at first. Other flashrom developers might have avoided the big allocation. I guess it doesn't matter. On systems with USB we probably have resources to spare.
Current implementation looks good.
PS6: One more style nit I've noticed: we also place a space before the asterisk in pointer casts, also always spaces around operators with two or more operands.
File dirtyjtag_spi.c:
https://review.coreboot.org/c/flashrom/+/67878/comment/24a53a86_7683491a PS5, Line 121: size_t num_xfer = (len + max_xfer_size - 1 ) / max_xfer_size; // ceil(len/max_xfer_size)
- Lift the `.max_data_*` limits so this function gets called for bigger
chunks. There should be less overhead as we wouldn't have to send the TMS/STOP sequence as often. I assume this could speed up long reads. Some of the memcpy() also seems unnecessary, but the USB transfers probably limit speed so much that it doesn't matter.
I went with that option.
Did you test if it affects the read/write speed?
https://review.coreboot.org/c/flashrom/+/67878/comment/ef6fcd78_9f74d05c PS5, Line 267: }
This could probably be written much simpler if we'd focus on the strcmp. […]
Minor misunderstanding, I think. I meant this `if` block is all that is needed. The `if ((units > tmp) && (units < end))` is not needed (or actually points to another problem, see below). `units < end` checks if there is a unit given at all. I misinterpreted your original intention but you fixed that already :) using only strcasecmp() implicitly also checks that there is a string.
`units > tmp` could actually only be false if there was no number given. In this case we shouldn't complain about the unit. Probably best to check for `freq != 0` explicitly. Or for the final value `1 <= freq && freq <= UINT16_MAX`?
NB. The canonical name for this option in flashrom is `spispeed`. Your code will probably be the most generic version so it might make sense to turn it into an API. Effort to unify this setting was already discussed a few times, but people seem to always get distracted ;)
https://review.coreboot.org/c/flashrom/+/67878/comment/f938f198_65742b2b PS5, Line 275: except TRST
Yes SRST and TMS are also high during init. […]
Looks good, thank you :)
-
Nico Huber (Code Review)