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 5:
(5 comments)
File dirtyjtag_spi.c:
https://review.coreboot.org/c/flashrom/+/67878/comment/fa6f589c_7d414453 PS5, Line 18: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Please remove this paragraph. The address became outdated in the past so we decided to drop it.
https://review.coreboot.org/c/flashrom/+/67878/comment/f15291b3_10509adf PS5, Line 121: size_t num_xfer = (len + max_xfer_size - 1 ) / max_xfer_size; // ceil(len/max_xfer_size) So this looks like your implementation supports arbitrary `writecnt` and `readcnt`. Given that flashrom can limit these based on `.max_data_write` and `.max_data_read`, the implementation could look much simpler. But doesn't have to; I think there are two options:
1. 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.
2. Set `.max_data_*` such that we can expect `len <= 30` here¹. Then we wouldn't need the loop and malloc(). In theory, even a single 32B buffer would suffice.
I assume both could speed things up as currently we probably have a lot of send,receive,send,receive,send sequences with little data in the second send/receive. (Btw. is it necessary to drain input buffers with the receive call even if we don't need the data?)
¹As the `max_xfer_size` accounts for both write and read counts, this is a little tricky: we'd have to account for the SPI25 protocol overhead. For the expected SPI commands, `25` should be the right limit (30B minus 1B opcode and 4B addresses).
https://review.coreboot.org/c/flashrom/+/67878/comment/92fb6bd0_88e784ac PS5, Line 174: .max_data_read = 30, : .max_data_write = 30, This tells flashrom to limit `writecnt` and `readcnt`. It might not be necessary with the current implementation, see comment above.
https://review.coreboot.org/c/flashrom/+/67878/comment/666a312e_aa77fe95 PS5, Line 267: } This could probably be written much simpler if we'd focus on the strcmp. For instance, something like ``` if (units[0] == '\0' || !strcasecmp(units, "Hz")) { freq /= 1000; } else if (!strcasecmp(units, "kHz")) { /* nothing to do as we want kHz */ } else if (!strcasecmp(units, "MHz")) { freq *= 1000; } else { ... return 1; } ```
https://review.coreboot.org/c/flashrom/+/67878/comment/e84535bb_2789a264 PS5, Line 275: except TRST What about SRST and TMS? I might read the code wrong, but it looks like there's actually more set to high than to low?
-
Nico Huber (Code Review)