Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 )
Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ......................................................................
Patch Set 16:
(3 comments)
Thanks. Much better than no feedback! I will work on this when I get time.
May be anyone (Nico?) can give a comment on ft2232_spi.c#497 meanwhile.
https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c File ft2232_spi.c:
https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@172 PS16, Line 172: .multicommand = default_spi_send_multicommand,
You just need to implement this and will get both commands passed at once.
I got it. This looks much better than my suggested patch!
https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@497 PS16, Line 497: bufsize = max(writecnt + 19, 261 + 19);
This assumes `i` is at most 10. But there is really no guarantee because […]
The optimization done in this change (to 261+19) was not due to USB-Transfer itself, but due to FTDI-Response time after every ftdi_write_data-call (which in this case is the some number as the number of USB-transfers). So this is a significant optimization. But with the good idea of implementing spi_send_multicommand for ftdi I have to check what this does with bufsize of (261+19).
What do you want to drop? Just the change from 260+9 to 261+19 or the whole max()-line?
I would drop the whole line (see PS11). IMHO the easiest and best way would be so set bufsize (to chunksize) to 4096. Is there any benefit of saving ca. 3.8kByte of space on a PC to the cost of more complex code?
As Angel Pons suggested, this could be done in an extra patch.
https://review.coreboot.org/c/flashrom/+/40477/16/ft2232_spi.c@562 PS16, Line 562: if (writearr[0] == JEDEC_WREN) {
What if the chip isn't JEDEC? 0x06 (WREN) could mean something very different. […]
You are right, I was assuming JEDEC_WREN in any case. Thank´s for that hint!
-
Simon Buhrow (Code Review)