[U-Boot] 83xx and LCRR setting

[U-Boot] 83xx and LCRR setting

[Heiko Schocher]

Hello Kim,

I actually work on an u-boot mpc8321 port (mostly identical with the kmeter1
port already in mainline), and I have to set the LCRR (Clock Ratio Register
Reference Manual 10.3.1.14). As I see in

cpu/mpc83xx/cpu_init.c cpu_init_f()

this is done while running from flash. Hmm... the Reference manual
says in chapter 10.3.1.14 page 474:

NOTE
For proper operation of the system, this register setting must not be altered
while local bus memories or devices are being accessed. Special care needs
to be taken when running instructions from an local bus controller memory.

Hmm...

On my board (and for example on the MPC832XEMDS) the flash is connected
to the localbus ... and this register setting is done, while
running from flash ... Hmm.. is this safe?

I only can set the LCRR register succesfully on my board port, if
I set the LCRR_DBYP bit in the CONFIG_SYS_LCRR define, without it
I couldn;t run u-boot (with it, it works fine)

Unfortunately this LCRR_DBYP bit (0x80000000) is not documented in
the MPC8323ERM ... there, it is just marked as reserved (and set
to 1 on reset)

So, it is ok, just to set this LCRR_DBYP bit? Or should the LCRR
register only changed, if u-boot runs from ram? Or ...?

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

[U-Boot] 83xx and LCRR setting

[Kim Phillips]

On Tue, 18 Aug 2009 15:23:47 +0200
Heiko Schocher <hs@denx.de> wrote:

> Hello Kim,

Hello Heiko, sorry for the late reply,

> I actually work on an u-boot mpc8321 port (mostly identical with the kmeter1
> port already in mainline), and I have to set the LCRR (Clock Ratio Register
> Reference Manual 10.3.1.14). As I see in
> 
> cpu/mpc83xx/cpu_init.c cpu_init_f()
> 
> this is done while running from flash. Hmm... the Reference manual
> says in chapter 10.3.1.14 page 474:
> 
> NOTE
> For proper operation of the system, this register setting must not be altered
> while local bus memories or devices are being accessed. Special care needs
> to be taken when running instructions from an local bus controller memory.
> 
> Hmm...
> 
> On my board (and for example on the MPC832XEMDS) the flash is connected
> to the localbus ... and this register setting is done, while
> running from flash ... Hmm.. is this safe?

yeah, I'm not quite sure how that works myself!

> I only can set the LCRR register succesfully on my board port, if
> I set the LCRR_DBYP bit in the CONFIG_SYS_LCRR define, without it
> I couldn;t run u-boot (with it, it works fine)
> 
> Unfortunately this LCRR_DBYP bit (0x80000000) is not documented in
> the MPC8323ERM ... there, it is just marked as reserved (and set
> to 1 on reset)
> 
> So, it is ok, just to set this LCRR_DBYP bit? Or should the LCRR
> register only changed, if u-boot runs from ram? Or ...?

I'd say set the bit - my guess it the bit always existed, but it just
got documented in later parts' docs. E.g, this is from the mpc8315
documentation:

0 PBYP PLL bypass. This bit should be set when using low bus clock frequencies (66 MHz or lower).
       When in PLL bypass mode, incoming data is captured in the middle of the bus clock cycle.
       0 The PLL is enabled.
       1 The PLL is bypassed.

hth,

Kim

[U-Boot] 83xx and LCRR setting

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Tue, 18 Aug 2009 15:23:47 +0200
> Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Kim,
> 
> Hello Heiko, sorry for the late reply,

No problem, thanks for your response!

>> I actually work on an u-boot mpc8321 port (mostly identical with the kmeter1
>> port already in mainline), and I have to set the LCRR (Clock Ratio Register
>> Reference Manual 10.3.1.14). As I see in
>>
>> cpu/mpc83xx/cpu_init.c cpu_init_f()
>>
>> this is done while running from flash. Hmm... the Reference manual
>> says in chapter 10.3.1.14 page 474:
>>
>> NOTE
>> For proper operation of the system, this register setting must not be altered
>> while local bus memories or devices are being accessed. Special care needs
>> to be taken when running instructions from an local bus controller memory.
>>
>> Hmm...
>>
>> On my board (and for example on the MPC832XEMDS) the flash is connected
>> to the localbus ... and this register setting is done, while
>> running from flash ... Hmm.. is this safe?
> 
> yeah, I'm not quite sure how that works myself!

:-(

>> I only can set the LCRR register succesfully on my board port, if
>> I set the LCRR_DBYP bit in the CONFIG_SYS_LCRR define, without it
>> I couldn;t run u-boot (with it, it works fine)
>>
>> Unfortunately this LCRR_DBYP bit (0x80000000) is not documented in
>> the MPC8323ERM ... there, it is just marked as reserved (and set
>> to 1 on reset)
>>
>> So, it is ok, just to set this LCRR_DBYP bit? Or should the LCRR
>> register only changed, if u-boot runs from ram? Or ...?
> 
> I'd say set the bit - my guess it the bit always existed, but it just
> got documented in later parts' docs. E.g, this is from the mpc8315
> documentation:
> 
> 0 PBYP PLL bypass. This bit should be set when using low bus clock frequencies (66 MHz or lower).
>        When in PLL bypass mode, incoming data is captured in the middle of the bus clock cycle.
>        0 The PLL is enabled.
>        1 The PLL is bypassed.

Thanks, I found this bit also in the 8360 Reference Manual.

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

[U-Boot] 83xx and LCRR setting

[Heiko Schocher]

Hello Kim,

Kim Phillips schrieb:
> On Tue, 18 Aug 2009 15:23:47 +0200
> Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Kim,
> 
> Hello Heiko, sorry for the late reply,
> 
>> I actually work on an u-boot mpc8321 port (mostly identical with the kmeter1
>> port already in mainline), and I have to set the LCRR (Clock Ratio Register
>> Reference Manual 10.3.1.14). As I see in
>>
>> cpu/mpc83xx/cpu_init.c cpu_init_f()
>>
>> this is done while running from flash. Hmm... the Reference manual
>> says in chapter 10.3.1.14 page 474:
>>
>> NOTE
>> For proper operation of the system, this register setting must not be altered
>> while local bus memories or devices are being accessed. Special care needs
>> to be taken when running instructions from an local bus controller memory.
>>
>> Hmm...
>>
>> On my board (and for example on the MPC832XEMDS) the flash is connected
>> to the localbus ... and this register setting is done, while
>> running from flash ... Hmm.. is this safe?
> 
> yeah, I'm not quite sure how that works myself!

I stumbled over this, just because I didn;t set this
LCRR_DBYP bit, which the CPU sets after a reset, so
what Do you think about this patch?

832x, LCRR: change only the valid bits for this register

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 cpu/mpc83xx/cpu_init.c    |    6 ++++++
 include/asm-ppc/fsl_lbc.h |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/cpu/mpc83xx/cpu_init.c b/cpu/mpc83xx/cpu_init.c
index ea4f2af..b5f64a8 100644
--- a/cpu/mpc83xx/cpu_init.c
+++ b/cpu/mpc83xx/cpu_init.c
@@ -193,8 +193,14 @@ void cpu_init_f (volatile immap_t * im)
 	 */
 	im->reset.rmr = (RMR_CSRE & (1<<RMR_CSRE_SHIFT));

+#if defined(CONFIG_MPC832x)
+	/* LCRR - Clock Ratio Register (10.3.1.14) */
+	im->lbus.lcrr = (im->lbus.lcrr & LCRR_MASK) | \
+			(CONFIG_SYS_LCRR & ~LCRR_MASK);
+#else
 	/* LCRR - Clock Ratio Register (10.3.1.16) */
 	im->lbus.lcrr = CONFIG_SYS_LCRR;
+#endif

 	/* Enable Time Base & Decrimenter ( so we will have udelay() )*/
 	im->sysconf.spcr |= SPCR_TBEN;
diff --git a/include/asm-ppc/fsl_lbc.h b/include/asm-ppc/fsl_lbc.h
index a28082e..2c7a94b 100644
--- a/include/asm-ppc/fsl_lbc.h
+++ b/include/asm-ppc/fsl_lbc.h
@@ -315,6 +315,10 @@
 #define LCRR_CLKDIV_4			0x00000004
 #define LCRR_CLKDIV_8			0x00000008

+#if defined(CONFIG_MPC832x)
+#define LCRR_MASK			0xFFFCFFF0
+#endif
+
 /* LTEDR - Transfer Error Check Disable Register
  */
 #define LTEDR_BMD	0x80000000 /* Bus monitor disable				*/
-- 
1.6.0.6

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

[U-Boot] 83xx and LCRR setting

[Detlev Zundel]

Hi Heiko,

> I stumbled over this, just because I didn;t set this
> LCRR_DBYP bit, which the CPU sets after a reset, so
> what Do you think about this patch?
>
> 832x, LCRR: change only the valid bits for this register
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>  cpu/mpc83xx/cpu_init.c    |    6 ++++++
>  include/asm-ppc/fsl_lbc.h |    4 ++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/cpu/mpc83xx/cpu_init.c b/cpu/mpc83xx/cpu_init.c
> index ea4f2af..b5f64a8 100644
> --- a/cpu/mpc83xx/cpu_init.c
> +++ b/cpu/mpc83xx/cpu_init.c
> @@ -193,8 +193,14 @@ void cpu_init_f (volatile immap_t * im)
>  	 */
>  	im->reset.rmr = (RMR_CSRE & (1<<RMR_CSRE_SHIFT));
>
> +#if defined(CONFIG_MPC832x)
> +	/* LCRR - Clock Ratio Register (10.3.1.14) */
> +	im->lbus.lcrr = (im->lbus.lcrr & LCRR_MASK) | \
> +			(CONFIG_SYS_LCRR & ~LCRR_MASK);
> +#else
>  	/* LCRR - Clock Ratio Register (10.3.1.16) */
>  	im->lbus.lcrr = CONFIG_SYS_LCRR;
> +#endif

Please don't invent yet another conditional here.  Why not do a 
#if defined LCCR_MASK
...
#endif

or maybe even always use the mask, define it in the board config and do
a

#if !defined(LCCR_MASK)
#define LCCR_MASK 0xFFFFFFFF
#endif

This really depends if and how this applies to the other members of the
83xx family.

And, by the way, we should _really_ be using accessor macros all around
;)

Cheers
  Detlev

-- 
Bacchus, n. A convenient deity invented by the ancients as an excuse for
getting drunk.
--
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

[U-Boot] 83xx and LCRR setting

[Kim Phillips]

On Thu, 20 Aug 2009 12:05:58 +0200
Heiko Schocher <hs@denx.de> wrote:

> >> On my board (and for example on the MPC832XEMDS) the flash is connected
> >> to the localbus ... and this register setting is done, while
> >> running from flash ... Hmm.. is this safe?
> > 
> > yeah, I'm not quite sure how that works myself!
> 
> I stumbled over this, just because I didn;t set this
> LCRR_DBYP bit, which the CPU sets after a reset, so
> what Do you think about this patch?
> 
> 832x, LCRR: change only the valid bits for this register
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>

> +#if defined(CONFIG_MPC832x)
> +#define LCRR_MASK			0xFFFCFFF0

if it's only the DBYP bit that is set out of reset, can we make this
0x80000000?

Kim

[U-Boot] 83xx and LCRR setting

[Kim Phillips]

On Thu, 20 Aug 2009 13:03:09 +0200
Detlev Zundel <dzu@denx.de> wrote:

> or maybe even always use the mask, define it in the board config and do
> a
> 
> #if !defined(LCCR_MASK)
> #define LCCR_MASK 0xFFFFFFFF
> #endif

ack.

> This really depends if and how this applies to the other members of the
> 83xx family.

they're all the same, really.

> And, by the way, we should _really_ be using accessor macros all around
> ;)

actually LCRR itself has specific instructions that say if written, it
then should be read, and then an isync be issued.

Kim

[U-Boot] 83xx and LCRR setting

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Thu, 20 Aug 2009 12:05:58 +0200
> Heiko Schocher <hs@denx.de> wrote:
> 
>>>> On my board (and for example on the MPC832XEMDS) the flash is connected
>>>> to the localbus ... and this register setting is done, while
>>>> running from flash ... Hmm.. is this safe?
>>> yeah, I'm not quite sure how that works myself!
>> I stumbled over this, just because I didn;t set this
>> LCRR_DBYP bit, which the CPU sets after a reset, so
>> what Do you think about this patch?
>>
>> 832x, LCRR: change only the valid bits for this register
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
> 
>> +#if defined(CONFIG_MPC832x)
>> +#define LCRR_MASK			0xFFFCFFF0
> 
> if it's only the DBYP bit that is set out of reset, can we make this
> 0x80000000?

Hmm.. but all the other bits are reserved=0. What happens if they
are set to 1?

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

[U-Boot] 83xx and LCRR setting

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Thu, 20 Aug 2009 13:03:09 +0200
> Detlev Zundel <dzu@denx.de> wrote:
> 
>> or maybe even always use the mask, define it in the board config and do
>> a
>>
>> #if !defined(LCCR_MASK)
>> #define LCCR_MASK 0xFFFFFFFF
>> #endif
> 
> ack.

nack, it should be

#if !defined(LCCR_MASK)
#define LCCR_MASK 0x00000000
#endif

because I did:

+#if defined(CONFIG_MPC832x)
+	/* LCRR - Clock Ratio Register (10.3.1.14) */
+	im->lbus.lcrr = (im->lbus.lcrr & LCRR_MASK) | \
+			(CONFIG_SYS_LCRR & ~LCRR_MASK);
+#else

but I can do of course a:

+	/* LCRR - Clock Ratio Register (10.3.1.14) */
+	im->lbus.lcrr = (im->lbus.lcrr & ~LCRR_MASK) | \
+			(CONFIG_SYS_LCRR & LCRR_MASK);

Which way is prefered?

But I am not sure if this is necessary, I prefer here the conditional
in code, because, if I port a new board to u-boot, I immidiately
see, there is something special, if I have an 832x ... and this
construct is not so ugly I think ...

>> This really depends if and how this applies to the other members of the
>> 83xx family.
> 
> they're all the same, really.

OK, if it is so ;-)

>> And, by the way, we should _really_ be using accessor macros all around
>> ;)
> 
> actually LCRR itself has specific instructions that say if written, it
> then should be read, and then an isync be issued.

Hmm.. actual code did not this! Where is this documented? And shouldn;t
we update this in code?

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

[U-Boot] 83xx and LCRR setting

[Detlev Zundel]

Hi Heiko,

> Hello Kim,
>
> Kim Phillips wrote:
>> On Thu, 20 Aug 2009 13:03:09 +0200
>> Detlev Zundel <dzu@denx.de> wrote:
>> 
>>> or maybe even always use the mask, define it in the board config and do
>>> a
>>>
>>> #if !defined(LCCR_MASK)
>>> #define LCCR_MASK 0xFFFFFFFF
>>> #endif
>> 
>> ack.
>
> nack, it should be
>
> #if !defined(LCCR_MASK)
> #define LCCR_MASK 0x00000000
> #endif
>
> because I did:
>
> +#if defined(CONFIG_MPC832x)
> +	/* LCRR - Clock Ratio Register (10.3.1.14) */
> +	im->lbus.lcrr = (im->lbus.lcrr & LCRR_MASK) | \
> +			(CONFIG_SYS_LCRR & ~LCRR_MASK);
> +#else
>
> but I can do of course a:
>
> +	/* LCRR - Clock Ratio Register (10.3.1.14) */
> +	im->lbus.lcrr = (im->lbus.lcrr & ~LCRR_MASK) | \
> +			(CONFIG_SYS_LCRR & LCRR_MASK);
>
> Which way is prefered?

Personally I'd prefer the latter, as I expect a bitmask to specifiy
which bits I'm allowed to _write_.

> But I am not sure if this is necessary, I prefer here the conditional
> in code, because, if I port a new board to u-boot, I immidiately
> see, there is something special, if I have an 832x ... and this
> construct is not so ugly I think ...

Well, I really do not see any need for a conditional _in code_.
Remember, such conditionals multiply the number of possible "source
input to the compiler" by two, i.e. they double correct testing whereas
different constants usually preserve the c level input to the compiler.

>>> This really depends if and how this applies to the other members of the
>>> 83xx family.
>> 
>> they're all the same, really.
>
> OK, if it is so ;-)

Then why not put this into mpc83xx.h?

>>> And, by the way, we should _really_ be using accessor macros all around
>>> ;)
>> 
>> actually LCRR itself has specific instructions that say if written, it
>> then should be read, and then an isync be issued.
>
> Hmm.. actual code did not this! Where is this documented? And shouldn;t
> we update this in code?

Of course "we" should update this ;)

Cheers
  Detlev

-- 
Greenspun's Tenth Rule of Programming: "Any sufficiently complicated C
or Fortran program contains an ad-hoc, informally-specified bug-ridden
slow implementation of half of Common Lisp."
--
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

[U-Boot] 83xx and LCRR setting

[Kim Phillips]

On Sat, 22 Aug 2009 08:17:51 +0200
Heiko Schocher <hs@denx.de> wrote:

> Hello Kim,
> 
> Kim Phillips wrote:
> > On Thu, 20 Aug 2009 12:05:58 +0200
> > Heiko Schocher <hs@denx.de> wrote:
> > 
> >>>> On my board (and for example on the MPC832XEMDS) the flash is connected
> >>>> to the localbus ... and this register setting is done, while
> >>>> running from flash ... Hmm.. is this safe?
> >>> yeah, I'm not quite sure how that works myself!
> >> I stumbled over this, just because I didn;t set this
> >> LCRR_DBYP bit, which the CPU sets after a reset, so
> >> what Do you think about this patch?
> >>
> >> 832x, LCRR: change only the valid bits for this register
> >>
> >> Signed-off-by: Heiko Schocher <hs@denx.de>
> > 
> >> +#if defined(CONFIG_MPC832x)
> >> +#define LCRR_MASK			0xFFFCFFF0
> > 
> > if it's only the DBYP bit that is set out of reset, can we make this
> > 0x80000000?
> 
> Hmm.. but all the other bits are reserved=0. What happens if they
> are set to 1?

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.

Kim

[U-Boot] 83xx and LCRR setting

[Kim Phillips]

On Mon, 24 Aug 2009 12:15:39 +0200
Detlev Zundel <dzu@denx.de> wrote:

> > but I can do of course a:
> >
> > +	/* LCRR - Clock Ratio Register (10.3.1.14) */
> > +	im->lbus.lcrr = (im->lbus.lcrr & ~LCRR_MASK) | \
> > +			(CONFIG_SYS_LCRR & LCRR_MASK);
> >
> > Which way is prefered?
> 
> Personally I'd prefer the latter, as I expect a bitmask to specifiy
> which bits I'm allowed to _write_.

which is consistent with the rest of the assignment style in that file.

> >>> This really depends if and how this applies to the other members of the
> >>> 83xx family.
> >> 
> >> they're all the same, really.
> >
> > OK, if it is so ;-)
> 
> Then why not put this into mpc83xx.h?

that makes a lot of sense - I don't know why original code has multiple
#ifdef configuration based sccr assignments in cpu_init.c.

> >>> And, by the way, we should _really_ be using accessor macros all around
> >>> ;)
> >> 
> >> actually LCRR itself has specific instructions that say if written, it
> >> then should be read, and then an isync be issued.
> >
> > Hmm.. actual code did not this! Where is this documented? And shouldn;t
> > we update this in code?
> 
> Of course "we" should update this ;)

of course.

Heiko, I got tired of looking up all the errata docs for common parts
of 83xx, so now I look at the latest device manuals.  In this case, the
source was "MPC8379E PowerQUICC II Pro Integrated Host Processor Family
Reference Manual, Rev. 1", p. 10-34, in the description for the CLKDIV
bitfield.

let's not duplicate existing bad code, and use the io accessor fns
(e.g., out_be, etc.) for new code here please.

Thanks,

Kim