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

Scott Wood scottwood at freescale.com
Fri Aug 9 11:35:20 EST 2013


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.

> +               };
> +       };
> +
> +       memory {
> +               device_type = "memory";
> +               reg = <0x0 0x20000000>;
> +       };
> +
> +       hawk at fef8 {

Unit address does not match reg.

> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               device_type = "soc";

Is this really an SoC?  In any case, this use of device_type is
deprecated.

> +               compatible = "mpc10x";

Don't use wildcards in compatible strings.

simple-bus may be applicable here (in addition to a specific
compatible).

> +               store-gathering = <0>;
> +               ranges = <0x0 0xfef80000 0x10000>;
> +               reg = <0xfef80000 0x10000>;

Where is "store-gathering" documented?

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

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

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

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

> +                               built-in;

What does this mean?

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

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

> +       if (cascade_irq != NO_IRQ)
> +               generic_handle_irq(cascade_irq);
> +
> +       chip->irq_eoi(&desc->irq_data);
> +}
> +
> +

Only one blank line.  Likewise elsewhere.

> +static void __init
> +mvme5100_pic_init(void)

Don't put a newline before the function name.

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

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

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

Why not just look for a chrp,iic node directly?

> +       if ((np = of_find_compatible_node(NULL, "pci", "mpc10x-pci"))) {

Why insist on the device_type?

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

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

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

> +       out_8((u_char *) BOARD_MODRST_REG, 0x01);
> +
> +       while (i-- > 0);

Do not use a loop to implement a delay.

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

-Scott





More information about the Linuxppc-dev mailing list