From mboxrd@z Thu Jan 1 00:00:00 1970 From: Detlev Zundel Date: Fri, 28 Aug 2009 12:36:00 +0200 Subject: [U-Boot] [PATCH v2] mpc83xx: update LCRR register handling In-Reply-To: <20090827154941.0bd08f5a.kim.phillips@freescale.com> (Kim Phillips's message of "Thu, 27 Aug 2009 15:49:41 -0500") References: <4A93A975.40009@denx.de> <4A93CB96.8000701@denx.de> <20090825103925.dc7ec42c.kim.phillips@freescale.com> <4A94D615.1060504@denx.de> <20090826173628.ddb2bdd4.kim.phillips@freescale.com> <20090827154941.0bd08f5a.kim.phillips@freescale.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Kim, > On Thu, 27 Aug 2009 13:11:25 +0200 > Detlev Zundel wrote: > >> Hi Kim, >> >> > o LCRR_PDYP, granted dangerous in your case, is obviously a writeable >> > bit (not read-only), and documented as such in later documentation. In >> > fact, there are no non-writeable bits in LCRR. >> >> Well, "reserved" != "non-writable" (usually there is a comment that >> writing reserved bits produces undefined behaviour) so I agree with >> Heiko that as long the documentation that we have access to, designates >> bits as reserved, it makes sense to have such a mask. > > I think we should allow board-configurable writes to the DBYP bit, which > is documented as "reserved" on some 83xx, on the 83xx parts that /do/ > implement it. So instead of having a mask, perhaps setting absolute > values for CONFIG_SYS_LCRR should be replaced with a better scheme that > allows board configs to just set LCRR bits by field, such as what the > SCCR setting code does. I.e, deprecate CONFIG_SYS_LCRR and replace with > individually-specified CONFIG_SYS_LCRR_{CLKDIV,EADC,ECL,BUDCMDC,DBYP} > values. > > This will allow the reserved bits, whether 1 on reset or > not, to be preserved across all 83xx (and 85xx for that matter). Hm, you mean like the SCCR stuff in mpc83xx/cpu_init.c? Please don't. This code is plain ugly - and even more, as I have pointed out multiple times, in not more than 50 lines there are "only" 1024 non-trivially differing c input possibilities coded. This is what I call bad. Thinking about it, why not do a compromise like e.g. the following: u32 sccr_mask = 0 \ #ifdef CONFIG_SYS_SCCR_ENCCM | SCCR_ENCCM \ #endif #ifdef CONFIG_SYS_SCCR_PCICM | SCCR_PCICM \ #endif .... #endif ; u32 sccr_value = 0 \ #ifdef CONFIG_SYS_SCCR_ENCCM | CONFIG_SYS_SCCR_ENCCM \ #endif #ifdef CONFIG_SYS_SCCR_PCICM | CONFIG_SYS_SCCR_PCICM \ #endif .... #endif ; out_be32(&im->clk.sccr, i(n_be32(&im->clk.sccr) & ~sccr_mask) | sccr_value); This not only looks a bit nicer, but also (I hope) compiles the *exact* same code for all possibilites, only with changing data values. Cheers Detlev -- Or go for generality ... Add a programming language for extensibility and write part of the program in that language. --- GNU Coding Standards -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de