From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Mon, 14 Jan 2008 11:56:57 -0500 Subject: [U-Boot-Users] [PATCH] ppc4xx: Refactor ECC POST for AMCC Denali core In-Reply-To: <200801141740.09548.sr@denx.de> References: <4789BED4.1030105@acm.org> <478B7234.6030609@ge.com> <200801141740.09548.sr@denx.de> Message-ID: <478B9459.50103@ge.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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