Re: [Qemu-devel] [PATCH v12 3/9] target-avr: adding a sample AVR board

[Michael Rolnik]

do you mean that I should remove the board and/or device? I use them for
testing.

*char const** and *const char** are the same.

On Mon, Jul 25, 2016 at 9:50 PM, Peter Maydell <address@hidden>
wrote:

> On 24 July 2016 at 01:02, Michael Rolnik <address@hidden> wrote:
> > Signed-off-by: Michael Rolnik <address@hidden>
> > ---
> >  MAINTAINERS          |   6 ++
> >  hw/avr/Makefile.objs |  21 ++++++
> >  hw/avr/sample-io.c   | 176
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/avr/sample.c      | 137 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 340 insertions(+)
> >  create mode 100644 hw/avr/Makefile.objs
> >  create mode 100644 hw/avr/sample-io.c
> >  create mode 100644 hw/avr/sample.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d1439a8..1435040 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -110,6 +110,12 @@ F: disas/arm.c
> >  F: disas/arm-a64.cc
> >  F: disas/libvixl/
> >
> > +AVR
> > +M: Michael Rolnik <address@hidden>
> > +S: Maintained
> > +F: target-avr/
> > +F: hw/avr/
> > +
> >  CRIS
> >  M: Edgar E. Iglesias <address@hidden>
> >  S: Maintained
> > diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
> > new file mode 100644
> > index 0000000..c080e4e
> > --- /dev/null
> > +++ b/hw/avr/Makefile.objs
> > @@ -0,0 +1,21 @@
> > +#
> > +#  QEMU AVR CPU
> > +#
> > +#  Copyright (c) 2016 Michael Rolnik
> > +#
> > +#  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.1 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/lgpl-2.1.html>
> > +#
> > +
> > +obj-y   += sample.o sample-io.o
> > diff --git a/hw/avr/sample-io.c b/hw/avr/sample-io.c
> > new file mode 100644
> > index 0000000..fd657aa
> > --- /dev/null
> > +++ b/hw/avr/sample-io.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + *  QEMU AVR CPU
> > + *
> > + *  Copyright (c) 2016 Michael Rolnik
> > + *
> > + *  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.1 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/lgpl-2.1.html>
> > + */
> > +
> > +/*
> > +    NOTE:
> > +        This is not a real AVR device !!! This is an example !!!
> > +
> > +        This example can be used to build a real AVR device.
> > +
> > +        AVR has the following layout of data memory
> > +
> > +        LD/ST(addr)                         IN/OUT(addr)
> > +        -----------                         ------------
> > +
> > +        0000    #-----------------------#
> > +           .    |                       |
> > +           .    |   32 CPU registers    |
> > +        001f    |                       |
> > +        0020    #-----------------------#   0000
> > +           .    |                       |   .
> > +           .    |   64 CPU IO registers |   .
> > +        005f    |                       |   003f
> > +        0060    #-----------------------#
> > +           .    |                       |
> > +           .    |  160 EXT IO registers |
> > +        00ff    |                       |
> > +        0100    #-----------------------#
> > +                |                       |
> > +                |  Internal RAM         |
> > +                |                       |
> > +                #-----------------------#
> > +                |                       |
> > +                |  External RAM         |
> > +                |                       |
> > +                #-----------------------#
> > +
> > +        Current AVR/CPU implementation assumes that IO device
> responsible to
> > +        implement functionality of IO and EXT IO registers is a memory
> mapped
> > +        device, mapped to addresses in the range [0x0020 .. 0x0100)
> > +
> > +        IN/OUT are implemented as an alias to LD/ST instructions
> > +
> > +        Some of CPU IO registers are implemented within the CPU itself,
> any
> > +        access to them either by LD/ST or IN/OUT won't be routed to the
> device.
> > +
> > +*/
>
> This device doesn't do anything, so it doesn't need to be here at all.
> If you have some real hardware that you want to emulate then you
> can write a model of that. (In particular it's not clear whether
> some device implementing EXT IO should be like this, or whether
> it ought to be provided together with an SoC container object,
> and it's hard to make that design choice unless we have an actual
> real world example. Better to leave it until we do.)
>
> > diff --git a/hw/avr/sample.c b/hw/avr/sample.c
> > new file mode 100644
> > index 0000000..173cf9c
> > --- /dev/null
> > +++ b/hw/avr/sample.c
> > @@ -0,0 +1,137 @@
> > +/*
> > + * QEMU AVR CPU
> > + *
> > + * Copyright (c) 2016 Michael Rolnik
> > + *
> > + * 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.1 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/lgpl-2.1.html>
> > + */
> > +
> > +/*
> > +    NOTE:
> > +        This is not a real AVR board !!! This is an example !!!
> > +
> > +        This example can be used to build a real AVR board.
> > +
> > +        This example board loads provided binary file into flash memory
> and
> > +        executes it from 0x00000000 address in the code memory space.
> > +
> > +        This example does not implement/install any AVR specific on
> board
> > +        devices except SampleIO device which is an example as well.
> > +
> > +        Currently used for AVR CPU validation
> > +
> > +*/
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qemu-common.h"
> > +#include "cpu.h"
> > +#include "hw/hw.h"
> > +#include "sysemu/sysemu.h"
> > +#include "sysemu/qtest.h"
> > +#include "ui/console.h"
> > +#include "hw/boards.h"
> > +#include "hw/devices.h"
> > +#include "hw/loader.h"
> > +#include "qemu/error-report.h"
> > +#include "exec/address-spaces.h"
> > +#include "include/hw/sysbus.h"
> > +
> > +#define VIRT_BASE_FLASH     0x00000000
> > +#define VIRT_BASE_ISRAM     0x00000100
> > +#define VIRT_BASE_EXMEM     0x00001100
> > +#define VIRT_BASE_EEPROM    0x00000000
> > +
> > +#define SIZE_FLASH          0x00020000
> > +#define SIZE_ISRAM          0x00001000
> > +#define SIZE_EXMEM          0x00010000
> > +#define SIZE_EEPROM         0x00001000
> > +#define SIZE_IOREG          SIZE_REGS
> > +
> > +#define PHYS_BASE_FLASH     (PHYS_BASE_CODE)
> > +
> > +#define PHYS_BASE_ISRAM     (PHYS_BASE_DATA)
> > +#define PHYS_BASE_EXMEM     (PHYS_BASE_ISRAM + SIZE_ISRAM)
> > +#define PHYS_BASE_EEPROM    (PHYS_BASE_EXMEM + SIZE_EXMEM)
> > +
> > +#define PHYS_BASE_IOREG     (PHYS_BASE_REGS + 0x20)
> > +
> > +static void sample_init(MachineState *machine)
> > +{
> > +    MemoryRegion *address_space_mem = get_system_memory();
> > +
> > +    MemoryRegion *ram;
> > +    MemoryRegion *flash;
> > +    MemoryRegion *isram;
> > +    MemoryRegion *exmem;
> > +    unsigned ram_size = SIZE_FLASH + SIZE_ISRAM + SIZE_EXMEM;
> > +
> > +    AVRCPU *cpu_avr ATTRIBUTE_UNUSED;
> > +    DeviceState *io;
> > +    SysBusDevice *bus;
> > +
> > +    ram = g_new(MemoryRegion, 1);
> > +    flash = g_new(MemoryRegion, 1);
> > +    isram = g_new(MemoryRegion, 1);
> > +    exmem = g_new(MemoryRegion, 1);
> > +
> > +    cpu_avr = cpu_avr_init("avr5");
> > +    io = qdev_create(NULL, "SampleIO");
> > +    qdev_init_nofail(io);
> > +    bus = SYS_BUS_DEVICE(io);
> > +
> > +    memory_region_allocate_system_memory(ram, NULL, "avr.ram",
> ram_size);
>
> You allocate this with a size which is equal to all your individual bits
> of RAM added together...
>
> > +
> > +    memory_region_init_ram(flash, NULL, "flash", SIZE_FLASH,
> &error_fatal);
> > +    memory_region_init_ram(isram, NULL, "isram", SIZE_ISRAM,
> &error_fatal);
> > +    memory_region_init_ram(exmem, NULL, "exmem", SIZE_EXMEM,
> &error_fatal);
>
> ...and then you allocate the pieces of RAM again separately...
>
> > +
> > +    memory_region_add_subregion(address_space_mem, PHYS_BASE_FLASH,
> flash);
> > +    memory_region_add_subregion(address_space_mem, PHYS_BASE_ISRAM,
> isram);
> > +    memory_region_add_subregion(address_space_mem, PHYS_BASE_EXMEM,
> exmem);
>
> ...and then you don't actually map your main RAM region.
>
> > +
> > +    vmstate_register_ram_global(flash);
> > +    vmstate_register_ram_global(isram);
> > +    vmstate_register_ram_global(exmem);
> > +
> > +    memory_region_set_readonly(flash, true);
>
> We have a memory_region_init_rom() now which is equivalent to
> doing memory_region_init_ram() + memory_region_set_readonly().
>
> > +
> > +    char const *firmware = NULL;
> > +    char const *filename;
>
> "const char *", not "char const *", and declarations should
> go at the start of {} blocks, not in the middle.
>
> > +
> > +    if (machine->firmware) {
> > +        firmware = machine->firmware;
> > +    }
> > +
> > +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
> > +    if (!filename) {
> > +        error_report("Could not find flash image file '%s'", firmware);
> > +        exit(1);
> > +    }
> > +
> > +    load_image_targphys(filename, PHYS_BASE_FLASH, SIZE_FLASH);
> > +
> > +    sysbus_mmio_map(bus, 0, PHYS_BASE_IOREG);
> > +}
> > +
> > +static void sample_machine_init(MachineClass *mc)
> > +{
> > +    mc->desc = "AVR sample/example board";
> > +    mc->init = sample_init;
> > +    mc->is_default = 1;
> > +}
> > +
> > +DEFINE_MACHINE("sample", sample_machine_init)
> > +
> > --
> > 2.4.9 (Apple Git-60)
> >
>
> thanks
> -- PMM
>



-- 
Best Regards,
Michael Rolnik