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

[Jerry Van Baren]

Stefan Roese wrote:
> Hi Jerry,
> 
> On Monday 14 January 2008, Jerry Van Baren wrote:

[snip]

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

[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).

> 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