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

[Heiko Schocher <hs@denx.de>]

Hello Kim,

Kim Phillips wrote:
> On Tue, 25 Aug 2009 13:31:34 +0200
> Heiko Schocher <hs@denx.de> wrote:
> 
>> MPC8379E RM says (10-34):
>> Once LCRR[CLKDIV] is written, the register should be read, and then
>> an isync should be executed.
>> So update this in code.
>> Also define a LCRR mask for processors, which uses not all bits
>> in the LCRR register (as for example mpc832x did).
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
> 
> thanks for this Heiko...some comments:
> 
>>  int cpu_init_r (void)
>>  {
>> +	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
>>  #ifdef CONFIG_QE
>>  	uint qe_base = CONFIG_SYS_IMMR + 0x00100000; /* QE immr base */
>> +#endif
>> +
>> +	/* LCRR - Clock Ratio Register (10.3.1.16) */
>> +	out_be32(&im->lbus.lcrr, ((in_be32(&im->lbus.lcrr) & ~LCRR_MASK) | \
>> +			(CONFIG_SYS_LCRR & LCRR_MASK)));
> 
> ..
> 
>> +	/* 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.

>> +++ 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 ...

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

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 ...

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany