Re: [PATCH] powerpc/40x: Add APM8018X SOC support

[Arnd Bergmann <arnd@arndb.de>]

On Friday 25 November 2011, Tanmay Inamdar wrote:
> > >
> > > +#if defined(CONFIG_APM8018X)
> > > +/* CPR */
> > > +#define DCRN_CPR0_CONFIG_ADDR        0xa
> > > +#define DCRN_CPR1_CONFIG_DATA        0xb
> > > +/* AHB */
> > > +#define DCRN_SDR1_CONFIG_ADDR        0xc
> > > +#define DCRN_SDR1_CONFIG_DATA        0xd
> > > +#else
> > >  /* CPRs (440GX and 440SP/440SPe) */
> > >  #define DCRN_CPR0_CONFIG_ADDR        0xc
> > >  #define DCRN_CPR0_CONFIG_DATA        0xd
> > > +#endif /* CONFIG_APM8018X */
> >
> > same comment as above.
> >
> 
> Some existing drivers use these macros. If I change the names, I will have
> to update the
> driver code.

Not so many apparently:

$ git grep DCRN_CPR._CONFIG 
arch/powerpc/include/asm/dcr-regs.h:#define DCRN_CPR0_CONFIG_ADDR       0xc
arch/powerpc/include/asm/dcr-regs.h:#define DCRN_CPR0_CONFIG_DATA       0xd
arch/powerpc/kvm/44x_emulate.c:                 case DCRN_CPR0_CONFIG_ADDR:
arch/powerpc/kvm/44x_emulate.c:                 case DCRN_CPR0_CONFIG_DATA:
arch/powerpc/kvm/44x_emulate.c:                         mtdcr(DCRN_CPR0_CONFIG_ADDR,
arch/powerpc/kvm/44x_emulate.c:                                        mfdcr(DCRN_CPR0_CONFIG_DATA));
arch/powerpc/kvm/44x_emulate.c:                 case DCRN_CPR0_CONFIG_ADDR:

Don't worry about out of tree drivers. If you want to be nice to the authors,
rename the macros so that the drivers break in an obvious way rather than
silently.

Note that any drivers relying on these as a constant are already broken
as soon as you try to build a kernel with both APM8018X and 440 enabled,
so you absolutely have to change the device drivers to work with those
kernels.

> > > diff --git a/arch/powerpc/kernel/cputable.c
> > b/arch/powerpc/kernel/cputable.c
> > > index edae5bb..e5c51a6 100644
> > > --- a/arch/powerpc/kernel/cputable.c
> > > +++ b/arch/powerpc/kernel/cputable.c
> > > @@ -1505,6 +1505,58 @@ static struct cpu_spec __initdata cpu_specs[] = {
> > >               .machine_check          = machine_check_4xx,
> > >               .platform               = "ppc405",
> > >       },
> > > +     {       /* APM80186-SK */
> > > +             .pvr_mask               = 0xffffffff,
> > > +             .pvr_value              = 0x7ff11432,
> >
> > If you mask out the lower bits, you only need a single entry instead of
> > four identical ones.
> >
> 
> You are right. But each PVR represent different version of SOC. If I create
> single
> entry, then I will have to give generic cpu_name which I don't want.

Isn't the common name for these always "APM80186"? If you don't need to tell
the difference between them for the purposes of cpu_spec, then don't do
it here.

	Arnd