powerpc/85xx: Add support for the "socrates" board (MPC8544)

[Grant Likely]

I agree 100% with David's comments, and I have some additional ones below.

On Thu, Mar 19, 2009 at 9:26 AM, Wolfgang Grandegger <wg at grandegger.com> wrote:
> +       soc8544 at e0000000 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               device_type = "soc";

Drop device_type here too.

> +
> +               ranges = <0x00000000 0xe0000000 0x00100000>;
> +               reg = <0xe0000000 0x00001000>;  // CCSRBAR 1M
> +               bus-frequency = <0>;            // Filled in by U-Boot
> +               compatible = "fsl,socrates-immr", "simple-bus";

As David said, fix this to be SoC specific, not board specific.

> +       localbus {
> +               compatible = "fsl,socrates-localbus",
> +                            "fsl,mpc85xx-localbus",
> +                            "fsl,pq3-localbus";

1st entry shouldn't be there.
2nd entry should specify exact chip
3rd entry I don't like much, but others may debate me on it
Also, add "simple-bus" to this list.  (important for a later comment)

> +               #address-cells = <2>;
> +               #size-cells = <1>;
> +               reg = <0xe0005000 0x40>;
> +
> +               ranges = <0 0 0xfc000000 0x04000000
> +                         2 0 0xc8000000 0x04000000
> +                         3 0 0xc0000000 0x00100000
> +                       >; /* Overwritten by U-Boot */

Just curious, why is U-Boot overwriting the ranges property?

> +               fpga_pic: fpga-pic at 3,10 {
> +                       compatible = "abb,socrates-fpga-pic";

Is 'abb' the companies' stock ticker symbol?  If not, then use the
real name and not an abbreviation.

> Index: linux-2.6/arch/powerpc/boot/wrapper
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/boot/wrapper
> +++ linux-2.6/arch/powerpc/boot/wrapper
> @@ -183,7 +183,7 @@ cuboot*)
>     *-tqm8541|*-mpc8560*|*-tqm8560|*-tqm8555|*-ksi8560*)
>         platformo=$object/cuboot-85xx-cpm2.o
>         ;;
> -    *-mpc85*|*-tqm85*|*-sbc85*)
> +    *-mpc85*|*-tqm85*|*-sbc85*|*-socrates)
>         platformo=$object/cuboot-85xx.o
>         ;;

Is this a new or old platform?  Can U-Boot on the board boot with a
uImage + dtb instead of a cuImage?  I'd prefer to avoid adding new
cuImages to the wrapper script if at all possible.

> Index: linux-2.6/arch/powerpc/configs/85xx/socrates_defconfig
> ===================================================================
> --- /dev/null
> +++ linux-2.6/arch/powerpc/configs/85xx/socrates_defconfig

Is a socrates-specific defconfig really warranted?

> --- linux-2.6.orig/arch/powerpc/platforms/85xx/Makefile
> +++ linux-2.6/arch/powerpc/platforms/85xx/Makefile
> @@ -13,4 +13,6 @@ obj-$(CONFIG_STX_GP3)   += stx_gp3.o
>  obj-$(CONFIG_TQM85xx)    += tqm85xx.o
>  obj-$(CONFIG_SBC8560)     += sbc8560.o
>  obj-$(CONFIG_SBC8548)     += sbc8548.o
> +obj-$(CONFIG_SOCRATES)    += socrates.o
> +obj-$(CONFIG_SOCRATES)    += socrates_fpga_pic.o

The pic stuff isn't all that big.  Personally I'd roll it all into the
socrates.c file.

> --- /dev/null
> +++ linux-2.6/arch/powerpc/platforms/85xx/socrates.c
> +static void __init socrates_pic_init(void)
> +{
> +       struct mpic *mpic;
> +       struct resource r;
> +       struct device_node *np;
> +
> +       np = of_find_node_by_type(NULL, "open-pic");
> +       if (!np) {
> +               printk(KERN_ERR "Could not find open-pic node\n");
> +               return;
> +       }
> +
> +       if (of_address_to_resource(np, 0, &r)) {
> +               printk(KERN_ERR "Could not map mpic register space\n");
> +               of_node_put(np);
> +               return;
> +       }
> +
> +       mpic = mpic_alloc(np, r.start,
> +                       MPIC_PRIMARY | MPIC_WANTS_RESET | MPIC_BIG_ENDIAN,
> +                       0, 256, " OpenPIC  ");
> +       BUG_ON(mpic == NULL);
> +       of_node_put(np);
> +
> +       mpic_init(mpic);

Heh, this is a block of code cloned between all the 85xx boards it
seems.  Smells like a small refactoring candidate.  This isn't really
a critique of this patch, but I noticed it so I thought I'd mention
it.

> +static void socrates_show_cpuinfo(struct seq_file *m)
> +{
> +       uint pvid, svid, phid1;
> +       uint memsize = total_memory;
> +
> +       pvid = mfspr(SPRN_PVR);
> +       svid = mfspr(SPRN_SVR);
> +
> +       seq_printf(m, "PVR\t\t: 0x%x\n", pvid);
> +       seq_printf(m, "SVR\t\t: 0x%x\n", svid);
> +
> +       /* Display cpu Pll setting */
> +       phid1 = mfspr(SPRN_HID1);
> +       seq_printf(m, "PLL setting\t: 0x%x\n", ((phid1 >> 24) & 0x3f));
> +
> +       /* Display the amount of memory */
> +       seq_printf(m, "Memory\t\t: %d MB\n", memsize / (1024 * 1024));
> +}

Another block of duplicated code.  In fact, many platforms have
dropped the cpuinfo hook entirely and just use the default output.

> +
> +static struct of_device_id __initdata of_bus_ids[] = {
> +       { .name = "soc", },
> +       { .type = "soc", },
> +       { .name = "localbus", },

Drop these three lines.  It is considered bad form now to bind on
either name or type for flattened device trees.  Instead add one {
.compatible = "simple-bus", }, entry and make sure the immr and
localbus nodes include "simple-bus" in the compatible string.

> +       {},
> +};
> +
> +static int __init declare_of_platform_devices(void)
> +{
> +       of_platform_bus_probe(NULL, of_bus_ids, NULL);
> +
> +       return 0;
> +}
> +machine_device_initcall(socrates, declare_of_platform_devices);

Don't add an initcall for this.  Instead assign
declar_of_platform_devices to the .init member of in the
define_machine() block below.

> Index: linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/arch/powerpc/platforms/85xx/socrates_fpga_pic.c
> @@ -0,0 +1,320 @@
> +/*
> + *  Copyright (C) 2008 Ilya Yanok, Emcraft Systems
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/io.h>
> +
> +#define SOCRATES_FPGA_NUM_IRQS 9
> +
> +#define FPGA_PIC_IRQCFG                (0x0)
> +#define FPGA_PIC_IRQMASK(n)    (0x4 + 0x4 * (n))
> +
> +#define SOCRATES_FPGA_IRQ_MASK ((1 << SOCRATES_FPGA_NUM_IRQS) - 1)
> +
> +struct socrates_fpga_irq_info {
> +       unsigned int irq_line;
> +       int type;
> +};
> +
> +/*
> + * Interrupt routing and type table
> + *
> + * IRQ_TYPE_NONE means the interrupt type is configurable,
> + * otherwise it's fixed to the specified value.
> + */
> +static struct socrates_fpga_irq_info fpga_irqs[SOCRATES_FPGA_NUM_IRQS] = {
> +       [0] = {0, IRQ_TYPE_NONE},
> +       [1] = {0, IRQ_TYPE_LEVEL_HIGH},
> +       [2] = {0, IRQ_TYPE_LEVEL_LOW},
> +       [3] = {0, IRQ_TYPE_NONE},
> +       [4] = {0, IRQ_TYPE_NONE},
> +       [5] = {0, IRQ_TYPE_NONE},
> +       [6] = {0, IRQ_TYPE_NONE},
> +       [7] = {0, IRQ_TYPE_NONE},
> +       [8] = {0, IRQ_TYPE_LEVEL_HIGH},
> +};
> +
> +#define socrates_fpga_irq_to_hw(virq)    ((unsigned int)irq_map[virq].hwirq)
> +
> +static DEFINE_SPINLOCK(socrates_fpga_pic_lock);
> +
> +static void __iomem *socrates_fpga_pic_iobase;
> +static struct irq_host *socrates_fpga_pic_irq_host;
> +static unsigned int socrates_fpga_irqs[3];
> +
> +static inline uint32_t socrates_fpga_pic_read(int reg)
> +{
> +       return in_be32(socrates_fpga_pic_iobase + reg);
> +}
> +
> +static inline void socrates_fpga_pic_write(int reg, uint32_t val)
> +{
> +       out_be32(socrates_fpga_pic_iobase + reg, val);
> +}
> +
> +static inline unsigned int socrates_fpga_pic_get_irq(unsigned int irq)
> +{
> +       uint32_t cause;
> +       unsigned long flags;
> +       int i;
> +
> +       for (i = 0; i < 3; i++) {
> +               if (irq == socrates_fpga_irqs[i])
> +                       break;
> +       }
> +       if (i == 3)
> +               return NO_IRQ;

This is interesting.  What does it mean?  It would be helpful to have
a theory of operation blurb in this file for stuff like this..

> +static int socrates_fpga_pic_host_xlate(struct irq_host *h,
> +               struct device_node *ct, u32 *intspec, unsigned int intsize,
> +               irq_hw_number_t *out_hwirq, unsigned int *out_flags)
> +{
> +       struct socrates_fpga_irq_info *fpga_irq = &fpga_irqs[intspec[0]];
> +
> +       *out_hwirq = intspec[0];
> +       if  (fpga_irq->type == IRQ_TYPE_NONE) {
> +               /* type is configurable */
> +               if (intspec[1] != IRQ_TYPE_LEVEL_LOW &&
> +                   intspec[1] != IRQ_TYPE_LEVEL_HIGH) {
> +                       printk(KERN_WARNING "FPGA PIC: invalid irq type, "
> +                              "setting default active low\n");

Nit: pr_warn() perhaps?  And same through the rest of the file.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.