Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 )
Change subject: tree/: Move programmer_delay() out of programmer state machine ......................................................................
Patch Set 4:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67393/comment/5ab8260b_81e1d2c0 PS4, Line 13: I just read this conversation once again, I think I understand a bit more what Nico is saying.
I'm missing the reason for the change
The change comes from an earlier discovery that two programmers (serprog and ch341a_spi) have custom delay functions and are using programmer data in those functions. As we have an ongoing effort to remove global state from all programmers, the question came up. Essentially, we need a way to access programmer data inside custom delay functions.
As a result, a combination of 3 patches achieves the goal: CB:67391 (previous to this) this patch CB:66373
After these 3 patches, global state can be removed from serprog and ch341a_spi
Programmers that register multiple masters would need redundant code.
I see what you mean. In practice, the change impacts only programmers with custom delay functions, out of them only one has multiple masters, serprog. So this results in two same lines in serprog. Maybe it's not too bad?
I understand you have more longer-term ideas of modifying/improving libflashrom API. Do you think you can start a discussion about it? Can we maybe unblock removing global state from serprog and ch341a_spi and then start with the effort around libflashrom API (whatever would the an outcome of the discussion).
Also a note about:
There are more complex programmer drivers that most likely have global state that isn't associated with a specific master.
We actually should be able to answer the question exactly, like which drivers and what is that state (if any). As of today, most of programmers have global state removed and few left. The most complicated left, for sure, but not many by the count, so we can go through and answer the question.
I can answer that the ones I haven't looked closely are ichspi and internal, the rest of what is left comes down to using data in programmer_delay or {un}map_flash functions.
File serprog.c:
https://review.coreboot.org/c/flashrom/+/67393/comment/8f9df498_cf2ffca4 PS4, Line 451: };
That will be default delay, see first patch in the chain CB:67391
Actually I looked at this again, and yes this probably needs delay too. Same as in `par_master_serprog` as I understand. (somehow I got confused when I looked here earlier at the time of my previous comment) I also added some more thoughts to the other longer comment thread.