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

Grant Likely wrote:

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

OK.

On Thu, Mar 19, 2009 at 9:26 AM, Wolfgang Grandegger [off-list ref] wrote:

quoted

+       soc8544@e0000000 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               device_type = "soc";

Drop device_type here too.

quoted

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

quoted

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

OK.

quoted

+               #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?

Because there are boards without FPGA or graphic controller on the local
bus.

quoted

+               fpga_pic: fpga-pic@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.

Yes.

quoted

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.

It's a new platform. Therefore I will drop cuboot support.

quoted

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?

quoted

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

Well,

  $ wc -l socrates_fpga_pic.c
  156 socrates.c
  320 socrates_fpga_pic.c

Personally, I'd prefer to separate the pic from the board init code,
especially with that size.

quoted

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

quoted

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

I can drop it for this board as well, no problem.

quoted

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

quoted

+       {},
+};
+
+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.

OK.

quoted

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

Just three interrupt lines are routed to the MPIC. A BUG_ON would be
more appropriate, though.

quoted

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

Yep, will fix the not commented issues as well.

Wolfgang.