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

[Peter Crosthwaite]

On Sun, Dec 15, 2013 at 3:17 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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.
>

hw/microblaze/boot.c. Sry.

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