On 24.06.2008 06:38, Peter Stuge wrote:
On Tue, Jun 24, 2008 at 04:51:00AM +0200, Carl-Daniel Hailfinger wrote:
Best case it is merely pointless to repeat a probe sequence. Worst case it causes system instability, as we saw with the AMIC A49LF040A.
This problem is not fixed by this patch.
No, but it is one step closer to well-defined, developer-friendly behavior. The next step is to add a simple list with probe functions, and execute each one exactly once before the list of flash chips is even considered.
That way there is a single, simple, definition of the order in which probes happen, regardless of the order of entries in flashchips.c, which I very much like having sorted alphabetically.
And that won't help the AMIC chip. Remember that one probing for the W29EE already kills the AMIC chip. Your change is simply orthogonal to the problem.
This also shortens flashrom's execution time roughly one second.
Signed-off-by: Peter Stuge peter@stuge.se
NACK. This patch randomly (depending on flashchips.c order) breaks probing for ~80% of the chips we currently support. If it works for you. you are just lucky.
Can you expand on why this would happen?
Keep in mind that each probe function always executes the same code, no matter which chip is being probed for.
Each probe function implements a particular hardware communication sequence to read some ID from chip. The function then compares that ID (same ID every call) to the parameters specified by the caller (different ID every call) and returns true if they match. My patch optimizes away the hardware communication for all probe function calls except the very first one. The ID can not change between calls, because the hardware communication sequence is identical.
Au contraire. The pointer to the (bios) probe location changes depending on flash chip size. So the hardware communication sequence is NOT identical.
If the ID read from hardware _did_ change between calls (as was the case in a report to flashrom@ when the delay in probe_jedec() was too short, before the 2ms patch) that particular probe function is not working correctly.
We haven dozens of reports (unrelated to probing delays) where probe_jedec gives different results for each probe location, exactly like expected.
There are three ways to solve this: - Keep probe results per function per chip size (ugly) - Create different probe_jedec_* functions per chip size (probe_jedec_512k, probe_jedec_1024k...) (embarrassing) - Leave the code as-is (preferred).
Regards, Carl-Daniel