[RFC PATCH 1/1] powerpc/embedded6xx: Add support for Motorola/Emerson MVME5100.

Stephen N Chivers schivers at csc.com.au
Tue Aug 20 12:28:20 EST 2013


Scott Wood <scottwood at freescale.com> wrote on 08/09/2013 11:35:20 AM:

> From: Scott Wood <scottwood at freescale.com>
> To: Stephen N Chivers <schivers at csc.com.au>
> Cc: <benh at kernel.crashing.org>, <paulus at samba.org>, Chris Proctor 
> <cproctor at csc.com.au>, <linuxppc-dev at lists.ozlabs.org>
> Date: 08/09/2013 11:36 AM
> Subject: Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for 
> Motorola/Emerson MVME5100.
> 
> On Thu, 2013-08-08 at 11:03 +1100, Stephen N Chivers wrote:
> > Add support for the Motorola/Emerson MVME5100 Single Board Computer.
> > 
> > The MVME5100 is a 6U form factor VME64 computer with:
> > 
> >         - A single MPC7410 or MPC750 CPU
> >         - A HAWK Processor Host Bridge (CPU to PCI) and
> >           MultiProcessor Interrupt Controller (MPIC)
> >         - Up to 500Mb of onboard memory
> >         - A M48T37 Real Time Clock (RTC) and Non-Volatile Memory chip
> >         - Two 16550 compatible UARTS
> >         - Two Intel E100 Fast Ethernets
> >         - Two PCI Mezzanine Card (PMC) Slots
> >         - PPCBug Firmware
> > 
> > The HAWK PHB/MPIC is compatible with the MPC10x devices.
> > 
> > There is no onboard disk support. This is usually provided by 
installing a 
> > PMC
> > in first PMC slot.
> > 
> > This patch revives the board support, it was present in early 2.6
> > series kernels. The board support in those days was by Matt Porter of
> > MontaVista Software.
> > 
> > CSC Australia has around 31 of these boards in service. The kernel in 
use
> > for the boards is based on 2.6.31. The boards are operated without 
disks
> > from a file server. 
> > 
> > This patch is based on linux-3.11-rc4 and has been boot tested.
> > 
> > Signed-off-by: Stephen Chivers <schivers at csc.com>
> > ---
> >  arch/powerpc/boot/dts/mvme5100.dts            |  195 ++
> >  arch/powerpc/boot/mvme5100.c                  |   28 +
> >  arch/powerpc/configs/mvme5100_defconfig       | 2597 
> > +++++++++++++++++++++++++
> >  arch/powerpc/platforms/embedded6xx/mvme5100.c |  288 +++
> >  4 files changed, 3108 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/boot/dts/mvme5100.dts 
> > b/arch/powerpc/boot/dts/mvme5100.dts
> > new file mode 100644
> > index 0000000..17fdbc7
> > --- /dev/null
> > +++ b/arch/powerpc/boot/dts/mvme5100.dts
> > @@ -0,0 +1,195 @@
> > +/*
> > + * Device Tree Souce for Motorola/Emerson MVME5100.
> > + *
> > + * Copyright 2013 CSC Australia Pty. Ltd.
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2.  This program is licensed "as is" without
> > + * any warranty of any kind, whether express or implied.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +       model = "MVME5100";
> > +       compatible = "MVME5100";
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       aliases {
> > +               serial0 = &serial0;
> > +               pci0 = &pci0;
> > +       };
> > +
> > +       cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               PowerPC,7410 {
> > +                       device_type = "cpu";
> > +                       reg = <0x0>;
> > +                       /* Following required by dtc but not used */
> > +                       d-cache-line-size = <32>;
> > +                       i-cache-line-size = <32>;
> > +                       i-cache-size = <32768>;
> > +                       d-cache-size = <32768>;
> > +                       timebase-frequency = <25000000>;
> > +                       clock-frequency = <500000000>;
> > +                        bus-frequency = <100000000>;
> 
> What does "following" mean?  certainly some of those are used, such as
> timebase-frequency.  In any case, it's not the device tree's job to
> document which properties are used by Linux.
> 
Agreed. Will remove.
The words were/are in the KuroboxHG.dts I modelled this dts on.
> > +               };
> > +       };
> > +
> > +       memory {
> > +               device_type = "memory";
> > +               reg = <0x0 0x20000000>;
> > +       };
> > +
> > +       hawk at fef8 {
> 
> Unit address does not match reg.
>
Will make it hawk at fef80000.

> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               device_type = "soc";
> 
> Is this really an SoC?  In any case, this use of device_type is
> deprecated.
> 
That was part of my early attempts to get legacy_serial to set up
the UARTS.
> > +               compatible = "mpc10x";
> 
> Don't use wildcards in compatible strings.
> 
> simple-bus may be applicable here (in addition to a specific
> compatible).
>
The HAWK ASIC is a difficult beast. I still cannot get a positive
identification as to what it is (Motorola/Freescale part number
unknown, not even the part number on the chip on the board helps....).
The best I can come up with is that it is a tsi108 without
the ethenets.
So device_type will be tsi-bridge and compatible will be
tsi108-bridge.
> > +               store-gathering = <0>;
> > +               ranges = <0x0 0xfef80000 0x10000>;
> > +               reg = <0xfef80000 0x10000>;
> 
> Where is "store-gathering" documented?
>
Good question. Again copied from the KuroboxHG dts (which
still has it).

> > +               serial0: serial at 8000 {
> > +                       cell-index = <0>;
> > +                       device_type = "serial";
> > +                       compatible = "ns16550";
> > +                       reg = <0x8000 0x80>;
> > +                       reg-shift = <4>;
> > +                       clock-frequency = <1843200>;
> > +                       current-speed = <9600>;
> > +                       interrupts = <1 1>; // IRQ1 Level Active Low.
> > +                       interrupt-parent = <&mpic>;
> > +               };
> > +
> > +               serial1: serial at 8200 {
> > +                       cell-index = <1>;
> > +                       device_type = "serial";
> > +                       compatible = "ns16550";
> > +                       reg = <0x8200 0x80>;
> > +                       reg-shift = <4>;
> > +                       clock-frequency = <1843200>;
> > +                       current-speed = <9600>;
> > +                       interrupts = <1 1>; // IRQ1 Level Active Low.
> > +                       interrupt-parent = <&mpic>;
> > +               };
> 
> What specifically does cell-index mean here?  Is it describing the
> hardware or just the terminology used in documentation?
>
Another case of a copying of what was already there. Will remove.

> > +       pci0: pci at feff0000 {
> > +               #address-cells = <3>;
> > +               #size-cells = <2>;
> > +               #interrupt-cells = <1>;
> > +               device_type = "pci";
> > +               compatible = "mpc10x-pci";
> > +               reg = <0xfec00000 0x400000>;
> > +               8259-interrupt-acknowledge = <0xfeff0030>;
> 
> Where is 8259-interrupt-acknowledge documented?
>
Good question. It is used in platforms/chrp/setup.c. It is also in the
amigaone dts, but that platform uses the PNP approach to managing its
8259 interrupts.
Will set compatible to "tsi108-pci" here.

> > +               ranges = <0x1000000 0x0        0x0 0xfe000000 0x0 
0x800000
> > +                         0x2000000 0x0 0x80000000 0x80000000 0x0 
> > 0x74000000>;
> > +               bus-range = <0 255>;
> > +               clock-frequency = <33333333>;
> > +               interrupt-parent = <&mpic>;
> > +               interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
> > +               interrupt-map = <
> > +
> > +                       /*
> > +                        * This definition (IDSEL 11) duplicates the
> > +                        * interrupts definition in the i8259
> > +                        * interrupt controller below.
> > +                        *
> > +                        * It isn't essential to provide it and the
> > +                        * board will run happily without it.
> 
> Should it be present then?
>
The interrupt comes via the PCI as it is sourced from a winbond
PCI to ISA bridge (and that is on a iPMC712 PMC).
But the interrupt has to be acknowledged via the
8259 interrupt acknowledge. Will remove the "essential" comment.

> > +               isa {
> > +                       #address-cells = <2>;
> > +                       #size-cells = <1>;
> > +                       #interrupt-cells = <2>;
> > +                       device_type = "isa";
> > +                       compatible = "isa";
> > +                       ranges = <0x00000001 0 0x01000000 0 0x00000000 

> > 0x00001000>;
> > +                       interrupt-parent = <&i8259>;
> > +
> > +                       i8259: interrupt-controller at 20 {
> > +                               #interrupt-cells = <2>;
> > +                               #address-cells = <0>;
> > +                               interrupts = <0 2>;
> > +                               device_type = "interrupt-controller";
> > +                               compatible = "chrp,iic";
> > +                               interrupt-controller;
> > +                               reg = <1 0x00000020 0x00000002
> > +                                       1 0x000000a0 0x00000002
> > +                                       1 0x000004d0 0x00000002>;
> > +                               clock-frequency = <0>;
> 
> Why does an i8259 have a clock-frequency?
>
Cut and paste considered harmful. Will remove.
> > +                               built-in;
> 
> What does this mean?
>
Don't know where I got that one from. Will remove.
> > diff --git a/arch/powerpc/platforms/embedded6xx/mvme5100.c 
> > b/arch/powerpc/platforms/embedded6xx/mvme5100.c
> > new file mode 100644
> > index 0000000..cc9ed8a
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/embedded6xx/mvme5100.c
> > @@ -0,0 +1,288 @@
> > +/*
> > + * Board setup routines for the Motorola/Emerson MVME5100.
> > + *
> > + * Copyright 2013 CSC Australia Pty. Ltd.
> > + *
> > + * Based on earlier code by:
> > + *
> > + *    Matt Porter, MontaVista Software Inc.
> > + *    Copyright 2001 MontaVista Software Inc.
> > + *
> > + * This program is free software; you can redistribute  it and/or 
modify 
> > it
> > + * under  the terms of  the GNU General  Public License as published 
by 
> > the
> > + * Free Software Foundation;  either version 2 of the  License, or 
(at 
> > your
> > + * option) any later version.
> > + *
> > + * Author: Stephen Chivers <schivers at csc.com>
> > + *
> > + */
> > +
> > +#include <linux/of_platform.h>
> > +
> > +#include <asm/i8259.h>
> > +#include <asm/pci-bridge.h>
> > +#include <asm/mpic.h>
> > +#include <asm/prom.h>
> > +#include <mm/mmu_decl.h>
> > +#include <asm/udbg.h>
> > +
> > +
> > +#define HAWK_MPIC_SIZE         0x00040000U
> > +#define MVME5100_PCI_MEM_OFFSET 0x00000000
> > +
> > +/* Board register addresses. */
> > +#define        BOARD_STATUS_REG        0xfef88080
> > +#define        BOARD_MODFAIL_REG       0xfef88090
> > +#define        BOARD_MODRST_REG        0xfef880a0
> > +#define        BOARD_TBEN_REG          0xfef880c0
> > +#define BOARD_SW_READ_REG      0xfef880e0
> > +#define        BOARD_GEO_ADDR_REG      0xfef880e8
> > +#define        BOARD_EXT_FEATURE1_REG  0xfef880f0
> > +#define        BOARD_EXT_FEATURE2_REG  0xfef88100
> > +
> > +static unsigned int pci_membase;
> > +
> > +
> > +#ifdef DEBUG
> > +#define DBG(fmt, args...) printk(KERN_ERR "%s: " fmt, __func__, ## 
args)
> > +#else
> > +#define DBG(fmt, args...)
> > +#endif
> 
> Use pr_debug().
>
Ok.
> > +static void
> > +mvme5100_8259_cascade(unsigned int irq, struct irq_desc *desc)
> > +{
> > +       struct irq_chip *chip = irq_desc_get_chip(desc);
> > +       unsigned int cascade_irq = i8259_irq();
> > +
> > +
> 
> Only one blank line.
>
Ok.
> > +       if (cascade_irq != NO_IRQ)
> > +               generic_handle_irq(cascade_irq);
> > +
> > +       chip->irq_eoi(&desc->irq_data);
> > +}
> > +
> > +
> 
> Only one blank line.  Likewise elsewhere.
>
Ok.
> > +static void __init
> > +mvme5100_pic_init(void)
> 
> Don't put a newline before the function name.
>
Ok. Old habits..... (from 1980 when I was writing stuff for
Level 7 Unix on PDP11s....).
> > +{
> > +       struct mpic *mpic;
> > +       void __iomem *mpic_addr;
> > +       struct device_node *np;
> > +       struct device_node *cp = NULL;
> > +       unsigned int cirq;
> > +       unsigned long intack = 0;
> > +       const unsigned long *prop = NULL;
> 
> Do not use non-fixed-size-types to decode properties.  In particular,
> non-string properties are normally composed of 32-bit cells.
>
Ok.
> > +       np = of_find_node_by_type(NULL, "open-pic");
> > +       if (!np) {
> > +               pr_err("Could not find open-pic node\n");
> > +               return;
> > +       }
> > +
> > +
> > +       pr_info("mvme5100_pic_init: pci_membase: %x\n", pci_membase);
> > +
> > +       mpic_addr = ioremap(pci_membase + MVME5100_PCI_MEM_OFFSET,
> > +                       HAWK_MPIC_SIZE);
> > +
> > +       pr_info("mvme5100_pic_init: mapped mpic_addr: 0x%p\n", 
mpic_addr);
> 
> The only thing you use this ioremap for is to print out the address.
>
Debug from initial port.
 
> > +       for_each_node_by_type(np, "interrupt-controller")
> > +               if (of_device_is_compatible(np, "chrp,iic")) {
> > +                       cp = np;
> > +                       break;
> > +               }
> > +
> 
> Please use braces around multi-line loop bodies.
>
Ok.

> Why not just look for a chrp,iic node directly?
>
I was following the model used in other places, like chrp/setup.c.

 
> > +       if ((np = of_find_compatible_node(NULL, "pci", "mpc10x-pci"))) 
{
> 
> Why insist on the device_type?
>
Following the model in the linkstation (kurobox) platform support. 
> > +static int __init
> > +mvme5100_add_bridge(struct device_node *dev)
> > +{
> > +       const int               *bus_range;
> > +       int                     len;
> > +       struct pci_controller   *hose;
> > +       unsigned short          devid;
> > +
> > +
> > +       pr_info("Adding PCI host bridge %s\n", dev->full_name);
> > +
> > +       bus_range = of_get_property(dev, "bus-range", &len);
> > +
> > +       if (bus_range == NULL || len < 2 * sizeof(int))
> > +               pr_warn("Can't get bus-range for %s, assuming bus 
0\n",
> > +                       dev->full_name);
> 
> Is this worth warning about?
>
Ok.
> > +       if (devid != PCI_DEVICE_ID_MOTOROLA_HAWK) {
> > +               pr_err("HAWK PHB not present?\n");
> > +
> > +               return 0;
> > +       }
> > +
> > +       early_read_config_dword( hose, 0, 0, PCI_BASE_ADDRESS_1, 
> > &pci_membase);
> 
> Patch is linewrapped.
>
Yes. Lotus Notes strikes again.
> > +static void
> > +mvme5100_restart(char *cmd)
> > +{
> > +       volatile ulong          i = 10000000;
> > +
> > +
> > +       local_irq_disable();
> > +       _nmask_and_or_msr(0, MSR_IP);
> 
> Does "mtmsr(mfmsr() | MSR_IP)" not work?
>
Don't know. Is from the original code by Matt Porter.

> > +       out_8((u_char *) BOARD_MODRST_REG, 0x01);
> > +
> > +       while (i-- > 0);
> 
> Do not use a loop to implement a delay.
>
Taken from the original code. But at this point the board
is going to reset and reboot via firmware, as /sbin/reboot
or /sbin/halt has been invoked.
 
> > +static void __init
> > +mvme5100_set_bat(void)
> > +{
> > +
> > +
> > +       mb();
> > +       mtspr(SPRN_DBAT1U, 0xf0001ffe);
> > +       mtspr(SPRN_DBAT1L, 0xf000002a);
> > +       mb();
> > +       setbat(1, 0xfe000000, 0xfe000000, 0x02000000, 
PAGE_KERNEL_NCG);
> > +}
> 
> It is no longer allowed to squat on random virtual address space like
> this.  If you really need a BAT you'll have to allocate the virtual
> address properly.
>
Yes. I found that this was an anathema when researching the port in
2010 but I couldn't find any practical solution at the time.
The code is called early to ensure that the hawk registers are available.
sysdev/cpm_common.c does the same thing.

What is the correct solution?
> -Scott
> 
> 
> 
Thanks for all the comments, they are appreciated.


More information about the Linuxppc-dev mailing list