[U-Boot] 83xx and LCRR setting
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
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
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
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 */
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
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
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
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
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
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
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
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
participants (3)
-
Detlev Zundel -
Heiko Schocher -
Kim Phillips