Re: [Qemu-devel] [PATCH] target-moxie: Add moxie Marin SoC support

[Peter Crosthwaite]

Hi Anthony,

On Sun, Dec 15, 2013 at 1:59 PM, Anthony Green <address@hidden> wrote:
>
> This adds initial support for the Marin SoC, including the SoC's uart
> interface.
>
>
> Signed-off-by: Anthony Green <address@hidden>
> ---
>  default-configs/moxie-softmmu.mak |   1 +
>  hw/char/Makefile.objs             |   1 +
>  hw/char/marin-uart.c              | 198 
> ++++++++++++++++++++++++++++++++++++++
>  hw/moxie/Makefile.objs            |   2 +-
>  hw/moxie/marin.c                  | 167 ++++++++++++++++++++++++++++++++

This should be at least two patches. One for the UART device and one
for your SoC. Maybe more depending on the descision regarding SoC v
board (see comment below).

>  5 files changed, 368 insertions(+), 1 deletion(-)
>  create mode 100644 hw/char/marin-uart.c
>  create mode 100644 hw/moxie/marin.c
>
> diff --git a/default-configs/moxie-softmmu.mak 
> b/default-configs/moxie-softmmu.mak
> index 1a95476..65a21de 100644
> --- a/default-configs/moxie-softmmu.mak
> +++ b/default-configs/moxie-softmmu.mak
> @@ -1,5 +1,6 @@
>  # Default configuration for moxie-softmmu
>
>  CONFIG_MC146818RTC=y
> +CONFIG_MOXIE=y
>  CONFIG_SERIAL=y
>  CONFIG_VGA=y
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index cbd6a00..48bc5d0 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -14,6 +14,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_uart.o
>  obj-$(CONFIG_OMAP) += omap_uart.o
>  obj-$(CONFIG_SH4) += sh_serial.o
>  obj-$(CONFIG_PSERIES) += spapr_vty.o
> +obj-$(CONFIG_MOXIE) += marin-uart.o
>
>  common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>  common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/marin-uart.c b/hw/char/marin-uart.c
> new file mode 100644
> index 0000000..f0d46d4
> --- /dev/null
> +++ b/hw/char/marin-uart.c
> @@ -0,0 +1,198 @@
> +/*
> + *  QEMU model of the Marin UART.
> + *
> + *  Copyright (c) 2013 Anthony Green <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "trace.h"
> +#include "sysemu/char.h"
> +#include "qemu/error-report.h"
> +
> +enum {
> +    R_RXREADY = 0,
> +    R_TXREADY,
> +    R_RXBYTE,
> +    R_TXBYTE,
> +    R_MAX
> +};
> +
> +#define TYPE_MARIN_UART "marin-uart"
> +#define MARIN_UART(obj) OBJECT_CHECK(MarinUartState, (obj), TYPE_MARIN_UART)
> +
> +struct MarinUartState {

Check your QoM conventions coding style.

http://wiki.qemu.org/QOMConventions

/* < private > */

> +    SysBusDevice busdev;

SysBusDevice parent_obj;

/* < public > */

> +    MemoryRegion regs_region;
> +    CharDriverState *chr;
> +    qemu_irq irq;
> +
> +    uint16_t regs[R_MAX];
> +};
> +typedef struct MarinUartState MarinUartState;
> +

You could just do the typedefing in the struct decl above to save this LOC.

> +static void uart_update_irq(MarinUartState *s)
> +{
> +}

Why the NOP function?

> +
> +static uint64_t uart_read(void *opaque, hwaddr addr,
> +                          unsigned size)
> +{
> +    MarinUartState *s = opaque;
> +    uint32_t r = 0;
> +
> +    addr >>= 1;
> +    switch (addr) {
> +    case R_RXREADY:
> +        r = s->regs[R_RXREADY];

You do kind of defeat the purpose of arrayified regs, if you just
index them all one by one maually. Can you have a default of r =
s->regs[addr]? ...

> +        break;
> +    case R_TXREADY:
> +        r = 1;

which is then overriden by this exceptional case.

> +        break;
> +    case R_TXBYTE:
> +        r = s->regs[R_TXBYTE];
> +        break;
> +    case R_RXBYTE:
> +        r = s->regs[R_RXBYTE];
> +        s->regs[R_RXREADY] = 0;
> +        qemu_chr_accept_input(s->chr);

Do you need a NULL guard on s->chr here?

> +        break;
> +    default:
> +        error_report("marin_uart: read access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 1);
> +        break;

This is a guest error and should use qemu_log_mask(LOG_GUEST_ERROR, .
Same for write handler below.

> +    }
> +
> +    return r;
> +}
> +
> +static void uart_write(void *opaque, hwaddr addr, uint64_t value,
> +                       unsigned size)
> +{
> +    MarinUartState *s = opaque;
> +    unsigned char ch = value;
> +
> +    addr >>= 1;
> +    switch (addr) {
> +    case R_TXBYTE:
> +        if (s->chr) {
> +            qemu_chr_fe_write(s->chr, &ch, 1);

What happens if qemu_chr_fe_write short returns? Do you just drop your char?

qemu_chr_fe_write_all will improve this, although it has problems too
(that i'm working on myself).

> +        }
> +        break;
> +
> +    default:
> +        error_report("marin_uart: write access to unknown register 0x"
> +                TARGET_FMT_plx, addr << 1);
> +        break;
> +    }
> +
> +    uart_update_irq(s);
> +}
> +
> +static const MemoryRegionOps uart_mmio_ops = {
> +    .read = uart_read,
> +    .write = uart_write,
> +    .valid = {
> +        .min_access_size = 2,
> +        .max_access_size = 2,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void uart_rx(void *opaque, const uint8_t *buf, int size)
> +{
> +    MarinUartState *s = opaque;
> +
> +    s->regs[R_RXBYTE] = *buf;
> +    s->regs[R_RXREADY] = 1;
> +
> +    uart_update_irq(s);
> +}
> +
> +static int uart_can_rx(void *opaque)
> +{
> +    MarinUartState *s = opaque;
> +
> +    return !(s->regs[R_RXREADY]);
> +}
> +
> +static void uart_event(void *opaque, int event)
> +{
> +}
> +
> +static void marin_uart_reset(DeviceState *d)
> +{
> +    MarinUartState *s = MARIN_UART(d);
> +    int i;
> +
> +    for (i = 0; i < R_MAX; i++) {
> +        s->regs[i] = 0;

Can you just reset your TX_READY bit to 1? This would cleanup the
exceptional case i mentioned above, and anyone introspecting your
device with gdb will see the true and correct value in s->regs.

> +    }
> +}
> +
> +static int marin_uart_init(SysBusDevice *dev)

Use of the SysBusDevice::init function is depracted, Please use the
device::realise or object::init functions instead. Check Antony
Pavlovs Digic UART (on list and in late stages of review) for the most
modern example of a QEMU UART.

> +{
> +    MarinUartState *s = MARIN_UART(dev);
> +
> +    sysbus_init_irq(dev, &s->irq);
> +
> +    memory_region_init_io(&s->regs_region, OBJECT(s), &uart_mmio_ops, s,
> +            TYPE_MARIN_UART, R_MAX * 4);
> +    sysbus_init_mmio(dev, &s->regs_region);
> +
> +    s->chr = qemu_char_get_next_serial();
> +    if (s->chr) {
> +        qemu_chr_add_handlers(s->chr, uart_can_rx, uart_rx, uart_event, s);
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_marin_uart = {
> +    .name = TYPE_MARIN_UART,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16_ARRAY(regs, MarinUartState, R_MAX),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void marin_uart_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);

This will go away when you convert the init fn.

> +
> +    k->init = marin_uart_init;
> +    dc->reset = marin_uart_reset;
> +    dc->vmsd = &vmstate_marin_uart;
> +}
> +
> +static const TypeInfo marin_uart_info = {
> +    .name          = TYPE_MARIN_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MarinUartState),
> +    .class_init    = marin_uart_class_init,
> +};
> +
> +static void marin_uart_register_types(void)
> +{
> +    type_register_static(&marin_uart_info);
> +}
> +
> +type_init(marin_uart_register_types)
> diff --git a/hw/moxie/Makefile.objs b/hw/moxie/Makefile.objs
> index bfc9001..4fa3b30 100644
> --- a/hw/moxie/Makefile.objs
> +++ b/hw/moxie/Makefile.objs
> @@ -1,2 +1,2 @@
>  # moxie boards
> -obj-y += moxiesim.o
> +obj-y += moxiesim.o marin.o
> diff --git a/hw/moxie/marin.c b/hw/moxie/marin.c
> new file mode 100644
> index 0000000..0a998e4
> --- /dev/null
> +++ b/hw/moxie/marin.c
> @@ -0,0 +1,167 @@
> +/*
> + * QEMU/marin SoC emulation
> + *
> + * Emulates the FPGA-hosted Marin SoC
> + *
> + * Copyright (c) 2013 Anthony Green
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "hw/sysbus.h"
> +#include "hw/hw.h"
> +#include "hw/boards.h"
> +#include "hw/loader.h"
> +#include "exec/address-spaces.h"
> +
> +typedef struct {
> +    uint64_t ram_size;
> +    const char *kernel_filename;
> +    const char *kernel_cmdline;
> +    const char *initrd_filename;
> +} LoaderParams;
> +
> +static void load_kernel(MoxieCPU *cpu, LoaderParams *loader_params)
> +{
> +    uint64_t entry, kernel_low, kernel_high;
> +    long kernel_size;
> +    long initrd_size;
> +    ram_addr_t initrd_offset;
> +
> +    kernel_size = load_elf(loader_params->kernel_filename,  NULL, NULL,
> +                           &entry, &kernel_low, &kernel_high, 1,
> +                           ELF_MACHINE, 0);
> +
> +    if (!kernel_size) {
> +        fprintf(stderr, "qemu: could not load kernel '%s'\n",
> +                loader_params->kernel_filename);

error_report()

> +        exit(1);
> +    }
> +
> +    /* load initrd */
> +    initrd_size = 0;
> +    initrd_offset = 0;
> +    if (loader_params->initrd_filename) {
> +        initrd_size = get_image_size(loader_params->initrd_filename);
> +        if (initrd_size > 0) {
> +            initrd_offset = (kernel_high + ~TARGET_PAGE_MASK)
> +              & TARGET_PAGE_MASK;
> +            if (initrd_offset + initrd_size > loader_params->ram_size) {
> +                fprintf(stderr,
> +                        "qemu: memory too small for initial ram disk '%s'\n",
> +                        loader_params->initrd_filename);
> +                exit(1);
> +            }
> +            initrd_size = load_image_targphys(loader_params->initrd_filename,
> +                                              initrd_offset,
> +                                              ram_size);
> +        }
> +        if (initrd_size == (target_ulong)-1) {
> +            fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
> +                    loader_params->initrd_filename);
> +            exit(1);
> +        }
> +    }
> +}

You should consider pulling your bootloader our into a seperate file
(and patch). Does Moxie define a specific linux boot protocol? Check
hw/arm/boot.c or hw/arm/microblaze.c for examples of modular
bootloaders.

> +
> +static void main_cpu_reset(void *opaque)
> +{
> +    MoxieCPU *cpu = opaque;
> +
> +    cpu_reset(CPU(cpu));
> +}
> +
> +static inline DeviceState *marin_uart_create(hwaddr base,
> +        qemu_irq irq)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "marin-uart");
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
> +
> +    return dev;
> +}

This is an old style qdev init function.

> +
> +static void marin_init(QEMUMachineInitArgs *args)
> +{
> +    MoxieCPU *cpu = NULL;
> +    ram_addr_t ram_size = args->ram_size;
> +    const char *cpu_model = args->cpu_model;
> +    const char *kernel_filename = args->kernel_filename;
> +    const char *kernel_cmdline = args->kernel_cmdline;
> +    const char *initrd_filename = args->initrd_filename;
> +    CPUMoxieState *env;
> +    MemoryRegion *address_space_mem = get_system_memory();
> +    MemoryRegion *ocram = g_new(MemoryRegion, 1);
> +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    MemoryRegion *rom = g_new(MemoryRegion, 1);
> +    hwaddr ram_base = 0x30000000;
> +    LoaderParams loader_params;
> +
> +    /* Init CPUs. */
> +    if (cpu_model == NULL) {
> +        cpu_model = "MoxieLite-moxie-cpu";
> +    }
> +    cpu = cpu_moxie_init(cpu_model);
> +    if (!cpu) {
> +        fprintf(stderr, "Unable to find CPU definition\n");

error_report()

> +        exit(1);
> +    }
> +    env = &cpu->env;
> +
> +    qemu_register_reset(main_cpu_reset, cpu);
> +
> +    /* Allocate RAM. */
> +    memory_region_init_ram(ocram, NULL, "marin-onchip.ram", 0x1000*4);
> +    vmstate_register_ram_global(ocram);
> +    memory_region_add_subregion(address_space_mem, 0x10000000, ocram);
> +
> +    memory_region_init_ram(ram, NULL, "marin-external.ram", ram_size);
> +    vmstate_register_ram_global(ram);
> +    memory_region_add_subregion(address_space_mem, ram_base, ram);
> +
> +    memory_region_init_ram(rom, NULL, "moxie.rom", 128*0x1000);
> +    vmstate_register_ram_global(rom);
> +    memory_region_add_subregion(get_system_memory(), 0x1000, rom);
> +
> +    if (kernel_filename) {
> +        loader_params.ram_size = ram_size;
> +        loader_params.kernel_filename = kernel_filename;
> +        loader_params.kernel_cmdline = kernel_cmdline;
> +        loader_params.initrd_filename = initrd_filename;
> +        load_kernel(cpu, &loader_params);
> +    }
> +
> +    marin_uart_create(0xF0000008, env->irq[4]);
> +}
> +
> +static QEMUMachine marin_machine = {
> +    .name = "marin",
> +    .desc = "Marin SoC",

So SoCs should generally be implemented on two levels. There is the
SoC device, which contains the devices that are on the SoC chip, then
the board level instantiates the SoC. This looks like a flat
board-and-SoC in one (on board level). Your deisgn is trivial so far
(and good for a first series), but long term what is the organsation?
Is this going towards a particular board emulation? Have a look at
Liguangs Allwinner series (and some of the early review comments) for
a discussion on this topic:

http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03940.html

As a starting point, can you tell us what is and isn't hosted on the
FPGA in this board model? That might be the best way to split this.

Regards,
Peter

> +    .init = marin_init,
> +    .is_default = 1,
> +};
> +
> +static void moxie_machine_init(void)
> +{
> +    qemu_register_machine(&marin_machine);
> +}
> +
> +machine_init(moxie_machine_init)
> --
> 1.8.3.1
>
>