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

[Tanmay Inamdar]

Hello Arnd,

Thanks for the comments. Please see inline replies.

Regards,
Tanmay

On Wed, Nov 23, 2011 at 7:45 PM, Arnd Bergmann <arnd at arndb.de> wrote:

> On Wednesday 23 November 2011, Tanmay Inamdar wrote:
> > The AppliedMicro APM8018X embedded processor targets embedded
> applications that
> > require low power and a small footprint. It features a PowerPC 405
> processor
> > core built in a 65nm low-power CMOS process with a five-stage pipeline
> executing
> > up to one instruction per cycle. The family has 128-kbytes of on-chip
> memory,
> > a 128-bit local bus and on-chip DDR2 SDRAM controller with 16-bit
> interface.
>
> Hi Tanmay,
>
> > +#if defined(CONFIG_APM8018X)
> > +#define DCRN_CPR0_ADDR       0xa
> > +#define DCRN_CPR0_DATA       0xb
> > +#else
> >  /* 440EP Clock/Power-on Reset regs */
> >  #define DCRN_CPR0_ADDR       0xc
> >  #define DCRN_CPR0_DATA       0xd
> > +#endif /* CONFIG_APM8018X */
>
> This prevents you from building one kernel that runs on both APM8018X and
> others. Better define a new constant name for the new registers and select
> the right one at run-time.
>
> > diff --git a/arch/powerpc/boot/dts/klondike.dts
> b/arch/powerpc/boot/dts/klondike.dts
> > new file mode 100644
> > index 0000000..9372a52
> > --- /dev/null
> > +++ b/arch/powerpc/boot/dts/klondike.dts
> > @@ -0,0 +1,668 @@
> > +/*
> > + * Device Tree Source for AMCC Klondike (405)
> > + *
>
> The device tree file for the most part describes the chip, but partly also
> the board. Have you considered splitting the soc parts into a .dtsi file
> and moving all the configuration into a separate file?
>

Thanks for suggesting this. I will consider using .dtsi now.

>
>
> > diff --git a/arch/powerpc/configs/40x/klondike_defconfig
> b/arch/powerpc/configs/40x/klondike_defconfig
> > new file mode 100644
> > index 0000000..840f438
> > --- /dev/null
> > +++ b/arch/powerpc/configs/40x/klondike_defconfig
> > @@ -0,0 +1,1353 @@
> > +#
> > +# Automatically generated file; DO NOT EDIT.
> > +# Linux/powerpc 3.2.0-rc2 Kernel Configuration
> > +#
> > +# CONFIG_PPC64 is not set
> > +
>
> Please use 'make savedefconfig' to create a minimal defconfig file instead
> of listing
> the full configuration here.
>

Yes.

>
> > diff --git a/arch/powerpc/include/asm/dcr-regs.h
> b/arch/powerpc/include/asm/dcr-regs.h
> > index 380274d..c900cfd 100644
> > --- a/arch/powerpc/include/asm/dcr-regs.h
> > +++ b/arch/powerpc/include/asm/dcr-regs.h
> > @@ -24,9 +24,18 @@
> >   * of the driver main register set
> >   */
> >
> > +#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.

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

>
> > --- a/arch/powerpc/kernel/udbg_16550.c
> > +++ b/arch/powerpc/kernel/udbg_16550.c
> > @@ -18,6 +18,19 @@ extern void real_writeb(u8 data, volatile u8 __iomem
> *addr);
> >  extern u8 real_205_readb(volatile u8 __iomem  *addr);
> >  extern void real_205_writeb(u8 data, volatile u8 __iomem *addr);
> >
> > +#ifdef CONFIG_UART_16550_WORD_ADDRESSABLE
> > +struct NS16550 {
> > +     /* this struct must be packed */
> > +     unsigned char rbr;  /* 0 */ u8 s0[3];
> > +     unsigned char ier;  /* 1 */ u8 s1[3];
> > +     unsigned char fcr;  /* 2 */ u8 s2[3];
> > +     unsigned char lcr;  /* 3 */ u8 s3[3];
> > +     unsigned char mcr;  /* 4 */ u8 s4[3];
> > +     unsigned char lsr;  /* 5 */ u8 s5[3];
> > +     unsigned char msr;  /* 6 */ u8 s6[3];
> > +     unsigned char scr;  /* 7 */ u8 s7[3];
> > +};
> > +#else
> >  struct NS16550 {
> >       /* this struct must be packed */
> >       unsigned char rbr;  /* 0 */
> > @@ -29,6 +42,7 @@ struct NS16550 {
> >       unsigned char msr;  /* 6 */
> >       unsigned char scr;  /* 7 */
> >  };
> > +#endif /* CONFIG_UART_16550_WORD_ADDRESSABLE */
> >
>
> Same things as with the register definitions. Please make this
> run-time selectable to allow build-time configurations that
> support both layouts.
>

Yes. I will try to find better solution.

>
>        Arnd
>

CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, 
is for the sole use of the intended recipient(s) and contains information 
that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries. 
It is to be used solely for the purpose of furthering the parties' business relationship. 
All unauthorized review, use, disclosure or distribution is prohibited. 
If you are not the intended recipient, please contact the sender by reply e-mail 
and destroy all copies of the original message.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20111125/452851d7/attachment.html>