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 <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)
OK.
>
>> + #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.
>> + 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.
Yes.
>> 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.
>> 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.
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.
>> --- /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.
I can drop it for this board as well, no problem.
>> +
>> +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.
OK.
>> 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.
>> +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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
