Jerry Van Baren wrote:
Stefan Roese wrote:
Hi Jerry,
On Monday 14 January 2008, Jerry Van Baren wrote:
[snip]
Hi Stefan and Jerry,
I just sent a response to Jerry's original e-mail, before seeing these comments.
As you should have picked up by now, a sync (forcing all I/O to complete) followed by eieio is silly - the eieio is superfluous. Seeing syncs/isyncs/eieios sprinkled in code is an indication that the author didn't understand what was going on and, as a result, kept hitting the problem with a bigger and bigger hammer until it appeared to have gone away.
Now I'm glad that I'm not the author of this code. ;) But I admit that I did use this "hammer" in the past.
As have we all. The only difference is that most of us don't get caught. ;-) Open source and git: it both exposes and attributes all stupidity. :-D
So, unlike proprietary code, it gets fixed. :-)
[snip]
From what I see, the ECC test code uses in_be32() and friends to access the memory. And these access functions have all necessary barriers already built into. So most likely the additional barriers were never necessary at all. Or perhaps the code was changed from using pointer access to in_be32() access.
Nevertheless the changes from Larry are looking good to me. But I also forwarded them to the original author of the code for review.
Good, that is what I wanted to get across - someone familiar with the code and the processor reviews what, why, when, and how (Larry, you, the original author, the list, etc.).
I figured that there must have been barriers that didn't show up in the patch since it "mostly works." I'm suspicious that there is a missing or misplaced barrier. The "sync ; eieio" pixie dust that Larry removed makes me suspicious that something is missing going into the test. In that case, the removed "sync" probably *IS* necessary, but that needs to be understood and commented (and possibly moved to a better location).
I'm not convinced about in_be32() et al. yet. Section 2.10.3 of the AMCC PPC440 User's manual says that an "msync" (sync) is required between the memory access and the control-register access. ("mbar" (eieio) is not sufficient because the control-register access is not treated as I/O.)
If I'm reading these correctly, in_be32() does a "sync", load, "twi" (which I don't understand), and "isync". out_be32() does a "sync" and a store. Thus, neither force completion of the I/O before exiting.
Am I right then that I should include a specific sync before accessing the SDRAM control registers?
Thanks again for your comments.
/me goes to mark jvb's mail as important to easier find it as reference. :)
:-) Thanks.
Best regards, Stefan
Ditto, gvb
Best regards, Larry