[U-Boot-Users] [PATCH] ppc4xx: Refactor ECC POST for AMCC Denali core

[Larry Johnson <lrj@acm.org>]

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