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 <[EMAIL PROTECTED]> >>> >> 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 -- http://www.hailfinger.org/ -- coreboot mailing list [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
