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
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
