Attention is currently required from: Subrata Banik, Paul Menzel, Angel Pons, 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 8:
(2 comments)
Patchset:
PS5:
I think the reason to use smaller timeout values in some cases is to avoid unnecessary delays. […]
Angel, the background is that there is a hardware component that arbitrates between the multiple integrated masters (host, me, gbe, ec, ...). When the bus is kept busy by one master, execution of a transaction of another master, e.g. ours, will be delayed. There is no way for us to know for how long so we need to wait for the worst case (every master happens to erase a block at the same time). This arbitration was never documented AFAIK but if one thinks about it, it's the only reasonable way to explain how it works with the given register interfaces.
We could still have distinct timeouts, but the difference would seem marginal because it would be something like
<worst-case-other-masters> + 100ms
vs.
<worst-case-other-masters> + 5s
If that worst case time is high, it makes no difference.
(the above information is roughly what I would expect in the commit message)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62867/comment/27513014_0473af01 PS8, Line 68: */ This comment is much too long, starts by describing the past state of the code and references a specific flash chip which it shouldn't. Also, first calculating 2.065s and then concluding 30s probably raises more questions than are answered.
It should be possible to boil it down to 2 or 3 sentences, preferably in the commit message and not as a code comment. If Intel would update the public datasheets, we could also reference those. If not, we could also add it to Documentation/.
-
Nico Huber (Code Review)