qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Green
Subject: Re: [Qemu-devel] [PATCH] target-moxie: Add moxie Marin SoC support
Date: Sun, 15 Dec 2013 07:48:49 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Peter - thank you for taking the time to review my patch.  Comments
below.

Peter Crosthwaite <address@hidden> writes:

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

Ok, I can split these.


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

Thanks for the pointer.


>
> /* < 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.

Yes.


>
>> +static void uart_update_irq(MarinUartState *s)
>> +{
>> +}
>
> Why the NOP function?

Place holder for a WIP.  I'll remove it.  The Marin SoC has an interrupt
controller as well. but I haven't modeled it in QEMU yet.


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

I suppose you could, but even if you fix the R_TXREADY special case,
you'd still have to handle the R_RXBYTE special case, and the range test
currently handled by 'default'.  I don't think it's much savings, and
the current switch statement is simpler to understand IMO.

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

I can add one.


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

Ok, thanks.


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

That's right.

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

I'll set it to '1' for the gdb reason. 

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

Ok.


>
>> +{
>> +    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()

Ok.

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

No, not yet.  I have an oldish uClinux kernel port that used device
trees, but it will be a while before I get back to that - there's so
much more to do first!

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

Any good pointers for a new style 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.

The Marin SoC currently runs on two boards: the Nexys3 (Xilinx) and DE-2
(Altera).  They are pretty much identical from the software side of
things.  Marin currently provides the UART, PIC, 7 segment display and
timer devices, as well as various memory controllers.  There's no useful
distinction between SoC and board at this time.  I'd like to keep it
simple as per my patch rather than try to factor them out prematurely.

Thanks,

Anthony Green


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]