Attention is currently required from: Subrata Banik, Paul Menzel, Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62867 )
Change subject: ichspi: Unify timeouts across all SPI operations to 30s ......................................................................
Patch Set 12: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62867/comment/517d2446_608e6cfe PS12, Line 38: Here is the timeout calculation for any hardware sequencing operation: : Worst Case Operational Delay = : (Maximum Time consumed by a SPI operation + Any marginal : adjustment) : : Timeout Recommendation for Hardware Sequencing Operation = : ((Worst Case Operational Delay) * (#No. Of SPI Master - 1) + : Current Operational latency) : : Assume, on Intel platform with 2 SPI master like, Host CPU and CSE, the : Timeout Calculation for SPI Write Operation would look like as below: : : Maximum Time consumed by a SPI Operation = 2 seconds : (for SPI Erase Operation as per Winbond Data Sheet) : : Worst Case Operational Delay = (2 seconds + 50 milliseconds) = : 2 seconds 50 milliseconds : Timeout Recommendation for Hardware Seq Operation = : (2 seconds 50 milliseconds) * (2 - 1) + 15 milliseconds : (for SPI Write Operation as per Winbond Data Sheet) : = 2 seconds 65 milliseconds Doing the calculation with these numbers can be a bit confusing. I'd do it with the estimated worst-case numbers right away (i.e. 6 masters, 5s).
Shorting some things might make it more readable, e.g. `2.05s` instead of `2 seconds 50 milliseconds`, also shorter identifiers and removing unnecessary parentheses can help legibility.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/246b96e3_12ff084f PS8, Line 68: */
Another thing you mentioned:
Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered.
I found where 30s comes from the last paragraph: it says "defines 5 master sections ... The worst case operation ... might take ~5 seconds" so that's 5*5=25 and taking a bit larger value 30 just in case. I hope my understanding is correct :)
It's a bit of an arbitrary choice actually. I think I mentioned 30s on the ML because I expected up to 6 masters in the forseeable future. But 5s already has a lot of buffer. It doesn't really matter because we don't know exact values. Chances need to be sufficiently low that the arbiter won't delay execution any longer, and it shouldn't be too long so a user doesn't get impatient when there's an actual hardware error.
Do you think it's fine to move this comment to commit message? And then, as you said, one line comment in the code?
Ack.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/3e1b0904_7d07ba54 PS12, Line 1281: operation `operations`? because multiple masters would preform multiple operations?
https://review.coreboot.org/c/flashrom/+/62867/comment/6521a293_580d4fb2 PS12, Line 1282: introduces `introduce` without s, I guess
-
Nico Huber (Code Review)