Skip to contentSkip to conversation

Add correct PLL settings for DragonBall VZ.

PATCH In review Daniel Palmer 5 participants 11 messages Apr 2, 2014 – Apr 5, 2014 m68k architecture

lore
Share & tools
lore

Daniel Palmer

author
#0 lore raw

PATCH 1/2 Add correct PLL settings for DragonBall VZ.

Signed-off-by Daniel Palmer <[email protected]>
---
 arch/m68k/platform/68000/head.S | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/m68k/platform/68000/head.S +7 -1
index 536ef96..0d00489 100644
@@ -91,12 +91,18 @@ _start:
 	movew	#0xe100, 0xfffff900		/* enable */
 #endif /* DEBUG_HEAD */
 
-#ifdef CONFIG_PILOT
+#if defined(CONFIG_PILOT)
 	movew	#0x2410, 0xfffff200		/* PLLCR */
+#elif defined(CONFIG_M68VZ328)
+	movew	#0x2493, 0xfffff200		/* PLLCR */
 #else
 	movew	#0x2400, 0xfffff200		/* PLLCR */
 #endif
+#if defined(CONFIG_M68VZ328)
+	movew	#0x0347, 0xfffff202		/* PLLFSR */
+#else
 	movew	#0x0123, 0xfffff202		/* PLLFSR */
+#endif
 	moveq	#0, %d0
 	movew	#16384, %d0			/* PLL settle wait loop */
 _pll_settle:
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 2/2 Fix mach_sched_init for EZ and VZ DragonBall chips.

Signed-off-by Daniel Palmer <[email protected]>
---
 arch/m68k/platform/68000/m68EZ328.c | 3 ++-
 arch/m68k/platform/68000/m68VZ328.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)
arch/m68k/platform/68000/m68EZ328.c +2 -1
index 332b5e8..2195290 100644
@@ -69,7 +69,8 @@ void __init config_BSP(char *command, int len)
   if (p) strcpy(p,command);
   else command[0] = 0;
 #endif
- 
+
+  mach_sched_init = hw_timer_init;
   mach_hwclk = m68328_hwclk;
   mach_reset = m68ez328_reset;
 }
arch/m68k/platform/68000/m68VZ328.c +1 -0
index fd66583..0e5e5a1 100644
@@ -182,6 +182,7 @@ void __init config_BSP(char *command, int size)
 
 	init_hardware(command, size);
 
+	mach_sched_init = hw_timer_init;
 	mach_hwclk = m68328_hwclk;
 	mach_reset = m68vz328_reset;
 }
Signature · 1 line
1.9.1
#2 lore raw

Re: [PATCH 2/2] Fix mach_sched_init for EZ and VZ DragonBall chips.

Hi Daniel,

Quoted 1 line · Daniel Palmer
Signed-off-by Daniel Palmer <[email protected]>

Looks good. As soon as the current merge window has closed I'll push this into the m68knommu git tree.

Regards
Greg

Quoted 32 lines
---
  arch/m68k/platform/68000/m68EZ328.c | 3 ++-
  arch/m68k/platform/68000/m68VZ328.c | 1 +
  2 files changed, 3 insertions(+), 1 deletion(-)
arch/m68k/platform/68000/m68EZ328.c +2 -1
index 332b5e8..2195290 100644
@@ -69,7 +69,8 @@ void __init config_BSP(char *command, int len)
    if (p) strcpy(p,command);
    else command[0] = 0;
  #endif
-
+
+  mach_sched_init = hw_timer_init;
    mach_hwclk = m68328_hwclk;
    mach_reset = m68ez328_reset;
  }
arch/m68k/platform/68000/m68VZ328.c +1 -0
index fd66583..0e5e5a1 100644
@@ -182,6 +182,7 @@ void __init config_BSP(char *command, int size)
 
  	init_hardware(command, size);
 
+	mach_sched_init = hw_timer_init;
  	mach_hwclk = m68328_hwclk;
  	mach_reset = m68vz328_reset;
  }
#3 lore raw
In reply to Daniel Palmer

Hi Daniel,

Looks good, nice and small, a single fix.

Can I suggest that you add an arch name to the patch title, so:

[PATCH 1/2] m68k: Add correct PLL settings for DragonBall VZ

Obviously on this email list we know it is for m68k, but when merged up stream it makes it much easier from the title to know what it applies to.

Quoted 10 lines · Daniel Palmer
Signed-off-by Daniel Palmer <[email protected]>
---
  arch/m68k/platform/68000/head.S | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
arch/m68k/platform/68000/head.S +0 -0
index 536ef96..0d00489 100644

arch/m68k/platform/68000/head.S _start: · line 97

Quoted 8 lines
arch/m68k/platform/68000/head.S +3 -1
@@ -91,12 +91,18 @@ _start:
  	movew	#0xe100, 0xfffff900		/* enable */
  #endif /* DEBUG_HEAD */
 
-#ifdef CONFIG_PILOT
+#if defined(CONFIG_PILOT)
  	movew	#0x2410, 0xfffff200		/* PLLCR */
+#elif defined(CONFIG_M68VZ328)
+	movew	#0x2493, 0xfffff200		/* PLLCR */

Would it be cleaner to have a defined value here instead? This moves the #ifdef'ery to just the PLLCR value, and keeps the code instructions here cleaner. So something like this at the top of this head.S:

#if defined(CONFIG_PILOT)
#define PLLCR 0x2410
#elif defined(CONFIG_M68VZ328)
#define PLLCR 0x2493
#else
#define PLLCR 0x2400
#endif

and then just

	movew	#PLLCR, 0xfffff200		/* PLLCR */

Regards
Greg

Quoted 12 lines
arch/m68k/platform/68000/head.S +4 -0
  #else
  	movew	#0x2400, 0xfffff200		/* PLLCR */
  #endif
+#if defined(CONFIG_M68VZ328)
+	movew	#0x0347, 0xfffff202		/* PLLFSR */
+#else
  	movew	#0x0123, 0xfffff202		/* PLLFSR */
+#endif
  	moveq	#0, %d0
  	movew	#16384, %d0			/* PLL settle wait loop */
  _pll_settle:
maintainer
#4 lore raw
Quoted 20 lines · Greg Ungerer

arch/m68k/platform/68000/head.S _start: · line 97

Quoted 5 lines
arch/m68k/platform/68000/head.S +3 -1
-#ifdef CONFIG_PILOT
+#if defined(CONFIG_PILOT)
        movew   #0x2410, 0xfffff200             /* PLLCR */
+#elif defined(CONFIG_M68VZ328)
+       movew   #0x2493, 0xfffff200             /* PLLCR */

Would it be cleaner to have a defined value here instead? This moves the #ifdef'ery to just the PLLCR value, and keeps the code instructions here cleaner. So something like this at the top of this head.S:

#if defined(CONFIG_PILOT)
#define PLLCR 0x2410
#elif defined(CONFIG_M68VZ328)
#define PLLCR 0x2493
#else
#define PLLCR 0x2400
#endif

Quoted 3 lines

and then just

        movew   #PLLCR, 0xfffff200              /* PLLCR */

While I like this advice in general, I prefer to use a different name than "PLLCR" for this definition.

One day someone may want to put "#define PLLCR 0xfffff200" in a header file...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

author
#5 lore raw

Only slightly related to the patch itself;

At the moment you can select to support multiple DragonBall chips in a single kernel but I don't think that'll actually work if the PLL register values are hardcoded like they are at the moment. So I think the kconfig needs to be changed to only allow support for one chip. Should I make this change and send a patch?

I doubt there are many people that are attempting to run Linux on Palms or extremely rare development boards but it does seem like it's possible to detect what chip the kernel is booting on from the chip id register. So the kernel could pick the right PLL settings based on the chip it's booting on and then the same kernel image could be used for all DragonBall based machines.

#6 lore raw
Quoted 7 lines · Daniel Palmer

I doubt there are many people that are attempting to run Linux on Palms or extremely rare development boards but it does seem like it's possible to detect what chip the kernel is booting on from the chip id register. So the kernel could pick the right PLL settings based on the chip it's booting on and then the same kernel image could be used for all DragonBall based machines.

A look at the user manual for the VZ says that the CHIPID register takes the value of 0x5600 upon reset, but does anyone know what crystals are in use in the various VZ hardware that Linux runs on?

It's more difficult for EZ: a quick look at wikipedia shows that Palm shipped MC68EZ328 devices clocked at 16 MHz and 20 MHz. And the user manual for the chip doesn't say what the reset value for the silicon ID register is.

#7 lore raw
In reply to Daniel Palmer

Hi Daniel,

Quoted 7 lines · Daniel Palmer

Only slightly related to the patch itself;

At the moment you can select to support multiple DragonBall chips in a single kernel but I don't think that'll actually work if the PLL register values are hardcoded like they are at the moment. So I think the kconfig needs to be changed to only allow support for one chip. Should I make this change and send a patch?

Sure, that sounds reasonable.

Quoted 6 lines

I doubt there are many people that are attempting to run Linux on Palms or extremely rare development boards but it does seem like it's possible to detect what chip the kernel is booting on from the chip id register. So the kernel could pick the right PLL settings based on the chip it's booting on and then the same kernel image could be used for all DragonBall based machines.

The mutually exclusive board selection can be changed back if or when code gets added to do this. No problem there.

Regards
Greg

#8 lore raw
In reply to Geert Uytterhoeven
Quoted 21 lines · Geert Uytterhoeven
Quoted 20 lines · Greg Ungerer

arch/m68k/platform/68000/head.S _start: · line 97

Quoted 5 lines
arch/m68k/platform/68000/head.S +3 -1
-#ifdef CONFIG_PILOT
+#if defined(CONFIG_PILOT)
        movew   #0x2410, 0xfffff200             /* PLLCR */
+#elif defined(CONFIG_M68VZ328)
+       movew   #0x2493, 0xfffff200             /* PLLCR */

Would it be cleaner to have a defined value here instead? This moves the #ifdef'ery to just the PLLCR value, and keeps the code instructions here cleaner. So something like this at the top of this head.S:

#if defined(CONFIG_PILOT)
#define PLLCR 0x2410
#elif defined(CONFIG_M68VZ328)
#define PLLCR 0x2493
#else
#define PLLCR 0x2400
#endif

Quoted 3 lines
Quoted 3 lines

and then just

        movew   #PLLCR, 0xfffff200              /* PLLCR */

In this case, the #ifdef ... #elif ... #else ... #endif cascade is already present, so I wonder whether your way is in fact better here.

Quoted 7 lines

While I like this advice in general, I prefer to use a different name than "PLLCR" for this definition.

One day someone may want to put "#define PLLCR 0xfffff200" in a header file...

It's defined in asm/MC68328.h:

#define PLLCR_ADDR 0xfffff200
...

#define PLLCR_DISPLL           0x0008   /* Disable PLL */
#define PLLCR_CLKEN            0x0010   /* Clock (CLKO pin) enable */
#define PLLCR_SYSCLK_SEL_MASK  0x0700   /* System Clock Selection */

#define PLLCR_SYSCLK_SEL_SHIFT 8
#define PLLCR_PIXCLK_SEL_MASK 0x3800 /* LCD Clock Selection */
#define PLLCR_PIXCLK_SEL_SHIFT 11

Some bit name definitions are missing, i.e.

#define PLLCR_PRESC1 0x0080

#9 lore raw
Quoted 22 lines · Finn Thain
Quoted 21 lines · Geert Uytterhoeven
Quoted 20 lines · Greg Ungerer

arch/m68k/platform/68000/head.S _start: · line 97

Quoted 5 lines
arch/m68k/platform/68000/head.S +3 -1
-#ifdef CONFIG_PILOT
+#if defined(CONFIG_PILOT)
         movew   #0x2410, 0xfffff200             /* PLLCR */
+#elif defined(CONFIG_M68VZ328)
+       movew   #0x2493, 0xfffff200             /* PLLCR */

Would it be cleaner to have a defined value here instead? This moves the #ifdef'ery to just the PLLCR value, and keeps the code instructions here cleaner. So something like this at the top of this head.S:

#if defined(CONFIG_PILOT)
#define PLLCR 0x2410
#elif defined(CONFIG_M68VZ328)
#define PLLCR 0x2493
#else
#define PLLCR 0x2400
#endif

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

and then just

         movew   #PLLCR, 0xfffff200              /* PLLCR */

In this case, the #ifdef ... #elif ... #else ... #endif cascade is already present, so I wonder whether your way is in fact better here.

I don't follow what you mean. The intention is to move it away from the actual code - to make it easier to read. Not add another #elif and make it worse than it currently is.

Regards
Greg

Quoted 22 lines
Quoted 6 lines

While I like this advice in general, I prefer to use a different name than "PLLCR" for this definition.

One day someone may want to put "#define PLLCR 0xfffff200" in a header file...

It's defined in asm/MC68328.h:

#define PLLCR_ADDR 0xfffff200
...

#define PLLCR_DISPLL           0x0008   /* Disable PLL */
#define PLLCR_CLKEN            0x0010   /* Clock (CLKO pin) enable */
#define PLLCR_SYSCLK_SEL_MASK  0x0700   /* System Clock Selection */

#define PLLCR_SYSCLK_SEL_SHIFT 8
#define PLLCR_PIXCLK_SEL_MASK 0x3800 /* LCD Clock Selection */
#define PLLCR_PIXCLK_SEL_SHIFT 11

Some bit name definitions are missing, i.e.

#define PLLCR_PRESC1 0x0080

#10 lore raw
Quoted 1 line · Greg Ungerer

I don't follow what you mean.

I wasn't being very clear I'm afraid.

Quoted 2 lines

The intention is to move it away from the actual code - to make it easier to read.

Your call.

Quoted 1 line

Not add another #elif and make it worse than it currently is.

Right.