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

[Kim Phillips]

On Wed, 26 Aug 2009 08:28:37 +0200
Heiko Schocher <hs at denx.de> wrote:

> Hello Kim,
> 
> Kim Phillips wrote:
> > On Tue, 25 Aug 2009 13:31:34 +0200
> > Heiko Schocher <hs at denx.de> wrote:
> >> +	/* MPC8379E RM 10-34 says after writting this register
> >> +	 * the register should be reread and an isync should be
> >> +	 * executed.
> >> +	 */
> >> +	in_be32(&im->lbus.lcrr);
> >> +	isync();
> > 
> > in_be32 and friends does the isync for you.  In fact, you can probably
> > do it in one fell swoop by using setbits/clrsetbits?
> 
> Argh, of course. But I need the in_be32() for rereading the register
> again, as the RM says.

right on

> >> +++ b/include/mpc83xx.h
> >> @@ -198,6 +198,7 @@
> >>  #define SICRL_URT_CTPR			0x06000000
> >>  #define SICRL_IRQ_CTPR			0x00C00000
> >>
> >> +#define LCRR_MASK			0x0003000f
> > 
> > I thought we discussed this - shouldn't this be 0x7fffffff?
> 
> Hmm.. as I did this mpc832x specific, in my version it is not possible
> to set reserved 0 bits to 1 ... Ah, I reread your mail again, you wrote:
> > I guess I have a shoot-yourself-in-the-foot philosophy - you're free to
> > find out what happens when setting reserved bits to 1 if you so wish.
> > u-boot protects you up to the point where you veer off into using
> > hardcoded values instead of using the predefined CONFIG_SYS_SCCR_*
> > macros.
> 
> I think you mean the LCRR_* defines ...

yes, thank you

> Hmm... so we can say: "feel free to find out what happens, if setting
> reserved 1 bits to 0! and can drop this patch" ...

precisely - I'd rather have this kept in the board config file if at
all possible - that's how other 83xx boards do it atm.

> Hmmm, you can use for the mpc832x for example the LCRR_BUFCMDC_1,
> what is a valid define for mpc83xx, but it is not valid for the
> mpc832x ... so It is not the problem, that a u-boot user use hard-
> coded values, instead this processor don;t support all bits valid
> for other mpc83xx processors.
> 
> So I tend to protect an u-boot user from doing wrong things,
> (setting reserved 0/1 bits to 1/0) if it is easy possible ...

right but:

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.

o the user loses visibility into what is going on if they
decide to drop/add sensitive bits such as LCRR_DBYP in their board's
CONFIG_SYS_LCRR settings, and there's a mask lurking in the background.

o let's be practical here - in a board port, LCRR settings have to be
paid attention to, no matter what hidden behaviours or new bits there
are lying underneath - perhaps the form of 'protection' you seek is in
the form of a comment in the code?

Kim