On Sun, Dec 15, 2013 at 3:17 PM, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> Hi Anthony,
>
> On Sun, Dec 15, 2013 at 1:59 PM, Anthony Green <gr...@moxielogic.com> wrote:
>>
>> This adds initial support for the Marin SoC, including the SoC's uart
>> interface.
>>
>>
>> Signed-off-by: Anthony Green <gr...@moxielogic.com>
>> ---
>> 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 <gr...@moxielogic.com>
>> + *
>> + * 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
>>
>>
