Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model

[Alistair Francis <alistair23@gmail.com>]

On Sun, Aug 24, 2014 at 11:20 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the Netduion Plus 2 machine to QEMU.
>>
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>>  hw/arm/Makefile.objs   |   2 +-
>>  hw/arm/netduinoplus2.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 203 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/arm/netduinoplus2.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 6088e53..616f1ae 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -2,7 +2,7 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
>>  obj-$(CONFIG_DIGIC) += digic_boards.o
>>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>> -obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>> +obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o netduinoplus2.o
>>
>>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>>  obj-$(CONFIG_DIGIC) += digic.o
>> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
>> new file mode 100644
>> index 0000000..32acac5
>> --- /dev/null
>> +++ b/hw/arm/netduinoplus2.c
>> @@ -0,0 +1,202 @@
>> +/*
>> + * Netduino Plus 2 Machine Model
>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * 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/ssi.h"
>> +#include "hw/arm/arm.h"
>> +#include "hw/devices.h"
>> +#include "qemu/timer.h"
>> +#include "net/net.h"
>> +#include "elf.h"
>> +#include "hw/loader.h"
>> +#include "hw/boards.h"
>> +#include "exec/address-spaces.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/qtest.h"
>> +
>> +#define FLASH_BASE_ADDRESS 0x08000000
>> +#define FLASH_SIZE (1024 * 1024)
>> +#define SRAM_BASE_ADDRESS 0x20000000
>> +#define SRAM_SIZE (192 * 1024)
>> +
>> +typedef struct ARMV7MResetArgs {
>> +    ARMCPU *cpu;
>> +    uint32_t reset_sp;
>> +    uint32_t reset_pc;
>> +} ARMV7MResetArgs;
>> +
>> +static void armv7m_reset(void *opaque)
>> +{
>> +    ARMV7MResetArgs *args = opaque;
>> +
>> +    cpu_reset(CPU(args->cpu));
>> +
>> +    args->cpu->env.regs[13] = args->reset_sp & 0xFFFFFFFC;
>> +    args->cpu->env.thumb = args->reset_pc & 1;
>> +    args->cpu->env.regs[15] = args->reset_pc & ~1;
>> +}
>> +
>> +static void netduinoplus2_init(MachineState *machine)
>> +{
>> +    static const uint32_t gpio_addr[] = { 0x40020000, 0x40020400, 0x40020800,
>> +        0x40020C00, 0x40021000, 0x40021400, 0x40021800, 0x40021C00,
>> +        0x40022000, 0x40022400, 0x40022800 };
>> +    static const uint8_t gpio_letters[] = { 'a', 'b', 'c',
>> +        'd', 'e', 'f', 'g', 'h',
>> +        'i', 'j', 'k' };
>> +    static const uint32_t tim2_5_addr[] = { 0x40000000, 0x40000400,
>> +        0x40000800, 0x40000C00 };
>> +    static const uint32_t usart_addr[] = { 0x40011000, 0x40004400,
>> +        0x40004800, 0x40004C00, 0x40005000, 0x40011400, 0x40007800,
>> +        0x40007C00 };
>> +    const char *kernel_filename = machine->kernel_filename;
>> +
>> +    static const int tim2_5_irq[] = {28, 29, 30, 50};
>> +    static const int usart_irq[] = {37, 38, 39, 52, 53, 71, 82, 83};
>> +
>> +    MemoryRegion *address_space_mem = get_system_memory();
>
> system_memory. address_space is a miselading name for a MR.
>
>> +    MemoryRegion *sram = g_new(MemoryRegion, 1);
>> +    MemoryRegion *flash = g_new(MemoryRegion, 1);
>> +    MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>> +    MemoryRegion *hack = g_new(MemoryRegion, 1);
>> +    ARMV7MResetArgs reset_args;
>> +
>> +    qemu_irq pic[96];
>> +    ARMCPU *cpu;
>> +    CPUARMState *env;
>> +    DeviceState *nvic;
>> +    DeviceState *gpio;
>> +    SysBusDevice *busdev;
>> +
>> +    int image_size;
>> +    uint64_t entry;
>> +    uint64_t lowaddr;
>> +    int i;
>> +    int big_endian = 0;
>> +
>> +    /* The Netduinio Plus 2 uses a Cortex-M4, while QEMU currently supports
>> +     * the Cortex-M3, so that is being used instead
>> +     */
>> +    cpu = cpu_arm_init("cortex-m3");
>> +    env = &cpu->env;
>> +
>> +    memory_region_init_ram(flash, NULL, "netduino.flash", FLASH_SIZE);
>> +    memory_region_init_alias(flash_alias, NULL, "netduino.flash.alias",
>> +                             flash, 0, FLASH_SIZE);
>> +
>> +    vmstate_register_ram_global(flash);
>> +
>> +    memory_region_set_readonly(flash, true);
>> +    memory_region_set_readonly(flash_alias, true);
>> +
>> +    memory_region_add_subregion(address_space_mem, FLASH_BASE_ADDRESS, flash);
>> +    memory_region_add_subregion(address_space_mem, 0, flash_alias);
>> +
>> +    memory_region_init_ram(sram, NULL, "netduino.sram", SRAM_SIZE);
>> +    vmstate_register_ram_global(sram);
>> +    memory_region_add_subregion(address_space_mem, SRAM_BASE_ADDRESS, sram);
>> +
>
>
> Most of this function is the same as armv7m_init(). This seems the be
> the main difference - the SRAM and FLASH setup. Everything else is
> board-level additive. The fact that it's different means that either
> ARMv7M should be parameterized, or Stellaris specific setup should be
> pushed up to stellaris.c to get armv7m_init cleaned up into a state
> where it's usable for you.
>
> Specifically, what in armv7m_init() is stopping the code share?
>

It is generally similar, although there are enough differences that I don't
think it's wroth it. Ideally one day the Netduino Plus 2 will use a
Cortex-M4 (one can hope) and this will need to be parameterised.
The Netduino also has different base addresses
which would need to be parameterised into armv7m_init(). The Netduio
then has the memory alias as well as a different number of interrupt lines.

It is because of all of these differences that I don't think it is worth using
the armv7m_init() function. It's really not that much code.

It could be taken out of the Stellaris boards, but that is somebody
else's decision.

Thanks,

Alistair

>> +    nvic = qdev_create(NULL, "armv7m_nvic");
>> +    qdev_prop_set_uint32(nvic, "num-irq", 96);
>
> nvic num-irqs sounds config specific so that's one that should
> probably be a parameter.
>
>> +    env->nvic = nvic;
>> +    qdev_init_nofail(nvic);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(nvic), 0,
>> +                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
>> +    for (i = 0; i < 96; i++) {
>> +        pic[i] = qdev_get_gpio_in(nvic, i);
>> +    }
>> +
>> +    /* Load the kernel */
>> +    if (!kernel_filename && !qtest_enabled()) {
>> +        fprintf(stderr, "Guest image must be specified (using -kernel)\n");
>> +        exit(1);
>> +    }
>> +    if (kernel_filename) {
>> +        image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
>> +                              NULL, big_endian, ELF_MACHINE, 1);
>> +        if (image_size < 0) {
>> +            image_size = load_image_targphys(kernel_filename, 0, FLASH_SIZE);
>> +            lowaddr = 0;
>> +        }
>> +        if (image_size < 0) {
>> +            error_report("Could not load kernel '%s'", kernel_filename);
>> +            exit(1);
>> +        }
>> +    }
>
> It seems strange to bootload before you have finished creating the
> machine (although it should be functionally safe).
>
>> +
>> +    /* System configuration controller */
>> +    sysbus_create_simple("netduino_syscfg", 0x40013800,
>> +                         pic[71]);
>> +
>> +    /* Attach a UART (uses USART registers) and USART controllers */
>> +    for (i = 0; i < 7; i++) {
>> +        sysbus_create_simple("netduino_usart", usart_addr[i],
>> +                             pic[usart_irq[i]]);
>> +    }
>> +
>> +    /* Timer 2 to 5 */
>> +    for (i = 0; i < 4; i++) {
>> +        sysbus_create_simple("netduino_timer", tim2_5_addr[i],
>> +                             pic[tim2_5_irq[i]]);
>> +    }
>> +
>> +    /* Attach GPIO devices */
>> +    for (i = 0; i < 11; i++) {
>> +        gpio = qdev_create(NULL, "netduino_gpio");
>> +        qdev_prop_set_uint8(gpio, "gpio-letter", gpio_letters[i]);
>> +        qdev_init_nofail(gpio);
>> +        busdev = SYS_BUS_DEVICE(gpio);
>> +        sysbus_mmio_map(busdev, 0, gpio_addr[i]);
>> +    }
>
> In the code-shared approach, these devs are added after the armv7_init() call.
>
> Regards,
> Peter
>
>> +
>> +    /* Hack to map an additional page of ram at the top of the address
>> +     * space.  This stops qemu complaining about executing code outside RAM
>> +     * when returning from an exception.
>> +     */
>> +    memory_region_init_ram(hack, NULL, "netduino.hack", 0x1000);
>> +    vmstate_register_ram_global(hack);
>> +    memory_region_set_readonly(hack, true);
>> +    memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
>> +
>> +    reset_args = (ARMV7MResetArgs) {
>> +        .cpu = cpu,
>> +        .reset_pc = entry,
>> +        .reset_sp = (SRAM_BASE_ADDRESS + (SRAM_SIZE * 2)/3),
>> +    };
>> +    qemu_register_reset(armv7m_reset,
>> +                        g_memdup(&reset_args, sizeof(reset_args)));
>> +}
>> +
>> +static QEMUMachine netduinoplus2_machine = {
>> +    .name = "netduinoplus2",
>> +    .desc = "Netduino Plus 2 Machine (Except with a Cortex-M3)",
>> +    .init = netduinoplus2_init,
>> +};
>> +
>> +static void netduinoplus2_machine_init(void)
>> +{
>> +    qemu_register_machine(&netduinoplus2_machine);
>> +}
>> +
>> +machine_init(netduinoplus2_machine_init);
>> --
>> 1.9.1
>>
>>