[coreboot] flashrom: Cache probed id:s

[Carl-Daniel Hailfinger]

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 at 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

-- 
http://www.hailfinger.org/