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

[Finn Thain <fthain@telegraphics.com.au>]

On Thu, 3 Apr 2014, Geert Uytterhoeven wrote:

> On Thu, Apr 3, 2014 at 2:41 PM, Greg Ungerer <gregungerer00@gmail.com> wrote:
> >> -#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 */

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

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

--