[U-Boot] [PATCH v2] mpc83xx: update LCRR register handling

[Detlev Zundel]

Hi Kim,

> On Thu, 27 Aug 2009 13:11:25 +0200
> Detlev Zundel <dzu at denx.de> 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