Re: [Qemu-devel] [PATCH] imx.31 and KZM board support

["Andreas Färber" <andreas.faerber@web.de>]

Hi Peter,

Am 21.11.2011 22:58, schrieb Peter Chubb:
> Hi Peter,
>    Please find appended a patch containing initial support for the
>    FreeScale i.MX31 and the KZM Arm11 evaluation board.

Your patch format is a bit unusual.

Please don't include personal messages in the description, keep it in a
format we can apply unchanged with git-am.

git-am complains:

/home/andreas/QEMU/qemu-arm/.git/rebase-apply/patch:43: new blank line
at EOF.
+
warning: 1 line adds whitespace errors.

>    The implementation was originally written by Hans Jang and Adam
>    Clench of OK-Labs; I've updated it to the current qdev and memory
>    region paradigms and implemented enough extra that Linux will boot
>    on the patched QEMU using a ram disk.
> 
>    The i.MX 31 Serial controller is found in most of the i.MX SoCs;
>    the AVIC and timer implementations can also be shared, albeit with
>    fewer chips.
> 
> Signed-off-by: Peter Chubb <peter.chubb@nicta.com.au>
> Signed-off-by: Hans Jang <hsjang@ok-labs.com>
> Signed-off-by: Adam Clench <adamc@ok-labs.com>

If as you describe above, you polished up patches originally by OK-Labs
then your SoB should be placed last.

> Index: qemu-working/hw/imx_avic.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_avic.c	2011-11-22 08:51:09.733239638 +1100
> @@ -0,0 +1,294 @@
> +/*
> + * IMX31 Vectored Interrupt Controller
> + *
> + * Note this is NOT the PL192 provided by ARM, but
> + * a custom implementation by FreeScale.
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This code is licenced under the GPL.

If you can, it would be nice to clarify the "GPL" license:

    ... the GNU General Public License as published by
    the Free Software Foundation; either version 1, or (at your option)
    any later version.

> + *
> + * TODO: implement vectors and priorities.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include <string.h> /* ffsll */
> +
> +#define DEBUG_INT 1
> +#undef DEBUG_INT /* comment out for debugging */

Usually we just do //#define DEBUG_...

> +
> +#ifdef DEBUG_INT
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_int: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif

> Index: qemu-working/hw/imx_serial.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_serial.c	2011-11-22 08:49:36.084276219 +1100
> @@ -0,0 +1,260 @@
> +/*
> + * IMX31 UARTS
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This code is licenced under the GPL.

Dito.

> + * This is a `bare-bones' implementation of the IMX series serial ports.
> + * TODO:
> + *  -- implement FIFOs.  The real hardware has 32 word transmit
> + *                       and receive FIFOs
> + *  -- implement DMA
> + *  -- implement BAUD-rate and modem lines, for when the backend
> + *     is a real serial device.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include "qemu-char.h"
> +
> +#define DEBUG_SERIAL 1
> +#undef DEBUG_SERIAL /* comment out for debugging */

Dito.

> +
> +#ifdef DEBUG_SERIAL
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_serial: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif

> +static int imx_serial_init(SysBusDevice *dev)
> +{
> +    imx_state *s = FROM_SYSBUS(imx_state, dev);
> +
> +    memory_region_init_io(&s->iomem, &imx_serial_ops, s, "imx-serial", 0x1000);
> +    sysbus_init_mmio_region(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->irq);
> +    s->chr = qdev_init_chardev(&dev->qdev);
> +
> +    s->usr1 = USR1_TRDY;
> +    s->usr2 = USR2_TXFE | USR2_TXDC;
> +    s->ucr1 = UCR1_TRDYEN | UCR1_RRDYEN | UCR1_UARTEN;
> +    s->uts1 = UTS1_RXEMPTY;
> +    s->readbuff = 0;
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> +                              imx_event, s);
> +    }
> +    return 0;
> +    /* ??? Save/restore.  */

What does this comment tell us? :)

> +}

> Index: qemu-working/hw/imx_timer.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_timer.c	2011-11-22 08:51:12.589269454 +1100
> @@ -0,0 +1,430 @@
> +/*
> + * IMX31 Timer
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + * Updated by Peter Chubb
> + *
> + * This code is licenced under the GPL.

Dito.

> + */
> +
> +#include "hw.h"
> +#include "qemu-timer.h"
> +#include "sysbus.h"
> +
> +#define DEBUG_TIMER 1
> +#undef DEBUG_TIMER  /* comment out for debugging */

Dito.

> +
> +#ifdef DEBUG_TIMER
> +#   define DPRINTF(fmt, args...) \
> +        do { printf("imx_timer: " fmt , ##args); } while (0)
> +#else
> +#   define DPRINTF(fmt, args...) do {} while (0)
> +#endif

> Index: qemu-working/hw/kzm.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/kzm.c	2011-11-22 08:42:26.292661540 +1100
> @@ -0,0 +1,159 @@
> +/*
> + * KZM Board System emulation.
> + *
> + * Copyright (c) 2008 OKL and 2011 NICTA
> + * Written by Hans
> + * Updated by Peter Chubb.
> + *
> + * This code is licenced under the GPL.

Dito.

> + * It (partially) emulates a Kyoto Microcomputer
> + * KZM-ARM11-01 evaluation board, with a FreeScale
> + * I.MX31 SoC
> + */
> +
> +#include "sysbus.h"
> +#include "exec-memory.h"
> +#include "hw.h"
> +#include "arm-misc.h"
> +#include "primecell.h"
> +#include "devices.h"
> +#include "pci.h"
> +#include "net.h"
> +#include "sysemu.h"
> +#include "boards.h"
> +#include "pc.h" /* for the FPGA UART that emulates a 16550 */
> +
> +    /* Memory map for Kzm Emulation Baseboard:
> +     * 0x00000000-0x00003fff 16k secure ROM       IGNORED
> +     * 0x00004000-0x00407fff Reserved             IGNORED
> +     * 0x00404000-0x00407fff ROM                  IGNORED
> +     * 0x00408000-0x0fffffff Reserved             IGNORED
> +     * 0x10000000-0x1fffBfff RAM aliasing         IGNORED
> +     * 0x1fffc000-0x1fffffff RAM                  EMULATED
> +     * 0x20000000-0x2fffffff Reserved             IGNORED
> +     * 0x30000000-0x7fffffff I.MX31 Internal Register Space
> +     *   0x43f00000 IO_AREA0
> +     *   0x43f90000 UART1                         EMULATED
> +     *   0x43f94000 UART2                         EMULATED
> +     *   0x68000000 PIC                           EMULATED
> +     *   0x53f94000 PIT 1                         EMULATED
> +     *   0x53f98000 PIT 2                         EMULATED
> +     *   0x53f90000 GPT                           EMULATED
> +     * 0x80000000-0x87ffffff RAM                  EMULATED
> +     * 0x88000000-0x8fffffff RAM Aliasing         EMULATED
> +     * 0xa0000000-0xafffffff NAND Flash           IGNORED
> +     * 0xb0000000-0xb3ffffff Unavailable          IGNORED
> +     * 0xb4000000-0xb4000fff 8-bit free space     IGNORED
> +     * 0xb4001000-0xb400100f Board control        IGNORED
> +     *  0xb4001003           DIP switch
> +     * 0xb4001010-0xb400101f 7-segment LED        IGNORED
> +     * 0xb4001020-0xb400102f LED                  IGNORED
> +     * 0xb4001030-0xb400103f LED                  IGNORED
> +     * 0xb4001040-0xb400104f FPGA, UART           EMULATED
> +     * 0xb4001050-0xb400105f FPGA, UART           EMULATED
> +     * 0xb4001060-0xb40fffff FPGA                 IGNORED
> +     * 0xb6000000-0xb61fffff LAN controller       EMULATED
> +     * 0xb6200000-0xb62fffff FPGA NAND Controller IGNORED
> +     * 0xb6300000-0xb7ffffff Free                 IGNORED
> +     * 0xb8000000-0xb8004fff Memory control registers IGNORED
> +     * 0xc0000000-0xc3ffffff PCMCIA/CF            IGNORED
> +     * 0xc4000000-0xffffffff Reserved             IGNORED
> +     */

Nice overview!

> +static void kzm_init(ram_addr_t ram_size,
> +                     const char *boot_device,
> +                     const char *kernel_filename, const char *kernel_cmdline,
> +                     const char *initrd_filename, const char *cpu_model)
> +{
> +    CPUState *env;
> +    MemoryRegion *address_space_mem = get_system_memory();
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    MemoryRegion *sram = g_new(MemoryRegion, 1);
> +    MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
> +    qemu_irq *cpu_pic;
> +    DeviceState *dev;
> +
> +    if (!cpu_model) {
> +        cpu_model = "arm1136";

[ My "favorite" CPU... ;) ]

> +    }
> +
> +    env = cpu_init(cpu_model);
> +    if (!env) {
> +        fprintf(stderr, "Unable to find CPU definition\n");
> +        exit(1);
> +    }

My sharp eye also spotted an | without spaces somewhere. If you haven't
already, try running scripts/checkpatch.pl.

Regards,
Andreas