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