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 <w...@grandegger.com> wrote: > + soc8...@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-...@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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev