Please see attached fr.cacheprobe_goto.patch.
For those who fiercly advocate against goto; please compare fr.cacheprobe_goto.patch (which I propose to commit) with the mess in fr.cacheprobe_if.patch (which is not signed off and only included for reference) - goto ftw!
Timing:
flashrom r3386: 1.099s flashrom r3387 (10ms patch): 1.880s flashrom r3387 + this patch: 0.882s
May be considered petty to optimize away this one second, but it is quite noticeable, and not neccessary.
//Peter
On Tue, Jun 24, 2008 at 04:17:47AM +0200, Peter Stuge wrote:
fr.cacheprobe_if.patch (which is not signed off and only included for reference)
read = 1; in _if.patch isn't in the right place in every file, it's a quick hack to prove a point.
//Peter
On 24.06.2008 04:17, Peter Stuge wrote:
Please see attached fr.cacheprobe_goto.patch.
For those who fiercly advocate against goto; please compare fr.cacheprobe_goto.patch (which I propose to commit) with the mess in fr.cacheprobe_if.patch (which is not signed off and only included for reference) - goto ftw!
Timing:
flashrom r3386: 1.099s flashrom r3387 (10ms patch): 1.880s flashrom r3387 + this patch: 0.882s
May be considered petty to optimize away this one second, but it is quite noticeable, and not neccessary.
flashrom: Cache probed id:s
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.
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.
Regards, Carl-Daniel
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.
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.
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.
If you are fiddling with flash chips electrically on the bus so that there is actually a different chip connected, I think it is very reasonable to require flashrom to be rerun.
//Peter
Peter Stuge wrote:
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.
but it does not necessarily run with the same address being used for probing.
Stefan
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
On Tue, Jun 24, 2008 at 11:16:37AM +0200, Carl-Daniel Hailfinger wrote:
NACK. This patch randomly (depending on flashchips.c order) breaks probing for ~80% of the chips we currently support.
Can you expand on why this would happen?
The pointer to the (bios) probe location changes depending on flash chip size.
Yes of course! Thank you to Stefan and you for cluestick on head.
There are three ways to solve this:
- Keep probe results per function per chip size (ugly)
I like a variation on this:
- Cache probe results per function per start address
I don't think it's so ugly because as you pointed out the address probed is input to the sequence.
Needs a little more code changed though.
- Create different probe_jedec_* functions per chip size
(probe_jedec_512k, probe_jedec_1024k...) (embarrassing)
Haha, yes. :)
- Leave the code as-is (preferred).
Mh, not so nice. :\
//Peter
On 24.06.2008 11:36, Peter Stuge wrote:
On Tue, Jun 24, 2008 at 11:16:37AM +0200, Carl-Daniel Hailfinger wrote:
NACK. This patch randomly (depending on flashchips.c order) breaks probing for ~80% of the chips we currently support.
Can you expand on why this would happen?
The pointer to the (bios) probe location changes depending on flash chip size.
Yes of course! Thank you to Stefan and you for cluestick on head.
You're welcome. (A few months ago, I had the same idea, but I buried it pretty fast ;-) )
There are three ways to solve this:
- Keep probe results per function per chip size (ugly)
I like a variation on this:
- Cache probe results per function per start address
I don't think it's so ugly because as you pointed out the address probed is input to the sequence.
Needs a little more code changed though.
And it is definitely post-1.0 material if we ever commit it. Things to do if you want to move forward with that: - Generalize probing so that you can probe for the same chip at multiple memory locations (ID strapping) - Same for multiple buses - Probe only for buses (SPI, LPC, ...) which make sense
The last point would automatically fix the AMIC/W29EE conflict because both can't reside on the same bus.
Regards, Carl-Daniel
Peter Stuge wrote:
There are three ways to solve this:
- Keep probe results per function per chip size (ugly)
I like a variation on this:
- Cache probe results per function per start address
I don't think it's so ugly because as you pointed out the address probed is input to the sequence.
Needs a little more code changed though.
Can you try to do this outside of the probe function?
ie. use a hash made from ptr to probe function and size (or rather base address!)
Also, I think since the multi flashchip support went in, flashrom continues to probe for 512K flash chips at 0xfff80000 even though a 512K flash chip was found at that very address -- This is a rather useless behavior and was introduced only recently. If we stop probing in a certain memory range once we know that range is reserved by a given flash chip, we can greatly reduce the probing time again (back to what it was a while ago)
Stefan
On 24.06.2008 11:57, Stefan Reinauer wrote:
Peter Stuge wrote:
There are three ways to solve this:
- Keep probe results per function per chip size (ugly)
I like a variation on this:
- Cache probe results per function per start address
I don't think it's so ugly because as you pointed out the address probed is input to the sequence.
Needs a little more code changed though.
Can you try to do this outside of the probe function?
ie. use a hash made from ptr to probe function and size (or rather base address!)
No hash please (unless you are willing to prove there are no collisions). For our purposes, a sorted list of struct {function pointer and base address} is ideal.
Regards, Carl-Daniel
Yes of course! Thank you to Stefan and you for cluestick on head.
What's a "cluestick on head"?
On Tue, Jun 24, 2008 at 05:59:00AM -0400, Joseph Smith wrote:
Yes of course! Thank you to Stefan and you for cluestick on head.
What's a "cluestick on head"?
A figure of speech.
A hits B over the head with a cluestick to give B a clue, or at least to make B realize that B didn't have a clue and needs to think for a minute.
//Peter
On Tue, 24 Jun 2008 12:02:27 +0200, Peter Stuge peter@stuge.se wrote:
On Tue, Jun 24, 2008 at 05:59:00AM -0400, Joseph Smith wrote:
Yes of course! Thank you to Stefan and you for cluestick on head.
What's a "cluestick on head"?
A figure of speech.
A hits B over the head with a cluestick to give B a clue, or at least to make B realize that B didn't have a clue and needs to think for a minute.
Ah, got ya. I have never heard that before...
-
Carl-Daniel Hailfinger -
Joseph Smith -
Peter Stuge -
Stefan Reinauer