Re: [Qemu-devel] [RFC v1 4/6] target_arm: Update armv7_init to support more parameters

[Peter Crosthwaite]

On Tue, Sep 9, 2014 at 6:24 PM, Alistair Francis <address@hidden> wrote:
> This patch is a hack
>
> This patch makes the smallest number of changes possible to extend
> armv7m_init() so that it can be used to init the Netduino 2.
>

s/Netduino/STM

> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
> I understand that this is probably not the way that everyone would
> like this done. What I do want to know though, is what the prefered
> method for extending this function would be? I am expecting that it
> will have to be a QOM device that can be attached.
>

That would be nice, but we might get there a shorter way first. A
bigger question to me is what is the content of this device supposed
to be?

>  hw/arm/armv7m.c      | 33 ++++++++++++++++++++++++++-------
>  hw/arm/stellaris.c   |  2 +-
>  include/hw/arm/arm.h |  3 ++-
>  3 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index aedef13..e4f7d8c 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -155,11 +155,21 @@ static void armv7m_bitband_init(void)
>
>  /* Board init.  */
>
> +typedef struct ARMV7MResetArgs {
> +    ARMCPU *cpu;
> +    uint32_t reset_sp;
> +    uint32_t reset_pc;
> +} ARMV7MResetArgs;
> +
>  static void armv7m_reset(void *opaque)
>  {
> -    ARMCPU *cpu = opaque;
> +    ARMV7MResetArgs *args = opaque;
> +
> +    cpu_reset(CPU(args->cpu));
>
> -    cpu_reset(CPU(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;
>  }
>
>  /* Init CPU and memory for a v7-M based board.
> @@ -167,14 +177,15 @@ static void armv7m_reset(void *opaque)
>     Returns the NVIC array.  */
>
>  qemu_irq *armv7m_init(MemoryRegion *system_memory,
> -                      int flash_size, int sram_size,
> +                      int flash_size, uint64_t flash_base,

Does ARMv7M actually mandate the existence of RAM and flash like this
at all? Given the complex setup of STM (with the memory aliases etc)
I'm now of the belief that all RAM and FLASH init should be handled by
the SoC level.

Can we just remove flash and RAM from this fn completely and push the
memory_region_init calls up to stellaris?

> +                      int sram_size, int num_irq,
>                        const char *kernel_filename, const char *cpu_model)
>  {
>      ARMCPU *cpu;
>      CPUARMState *env;
>      DeviceState *nvic;
>      /* FIXME: make this local state.  */
> -    static qemu_irq pic[64];
> +    qemu_irq *pic = g_new(qemu_irq, num_irq);
>      int image_size;
>      uint64_t entry;
>      uint64_t lowaddr;
> @@ -183,6 +194,7 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
>      MemoryRegion *sram = g_new(MemoryRegion, 1);
>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>      MemoryRegion *hack = g_new(MemoryRegion, 1);
> +    ARMV7MResetArgs reset_args;
>
>      flash_size *= 1024;
>      sram_size *= 1024;
> @@ -213,18 +225,19 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
>      memory_region_init_ram(flash, NULL, "armv7m.flash", flash_size);
>      vmstate_register_ram_global(flash);
>      memory_region_set_readonly(flash, true);
> -    memory_region_add_subregion(system_memory, 0, flash);
> +    memory_region_add_subregion(system_memory, flash_base, flash);
>      memory_region_init_ram(sram, NULL, "armv7m.sram", sram_size);
>      vmstate_register_ram_global(sram);
>      memory_region_add_subregion(system_memory, 0x20000000, sram);
>      armv7m_bitband_init();
>
>      nvic = qdev_create(NULL, "armv7m_nvic");
> +    qdev_prop_set_uint32(nvic, "num-irq", num_irq);
>      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 < 64; i++) {
> +    for (i = 0; i < num_irq; i++) {
>          pic[i] = qdev_get_gpio_in(nvic, i);
>      }
>
> @@ -259,7 +272,13 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
>      vmstate_register_ram_global(hack);
>      memory_region_add_subregion(system_memory, 0xfffff000, hack);
>
> -    qemu_register_reset(armv7m_reset, cpu);
> +    reset_args = (ARMV7MResetArgs) {
> +        .cpu = cpu,
> +        .reset_pc = entry,
> +        .reset_sp = (0x20000000 + ((192 * 1024) * 2)/3),

Why is the manual sp setting needed? I thought the V7M specific CPU
reset code handled this? Does Martins ARMv7M ROM reset patch help by
any chance?

Regards,
Peter

> +    };
> +    qemu_register_reset(armv7m_reset,
> +                        g_memdup(&reset_args, sizeof(reset_args)));
>      return pic;
>  }
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 64bd4b4..41dc712 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1223,7 +1223,7 @@ static void stellaris_init(const char *kernel_filename, 
> const char *cpu_model,
>      flash_size = ((board->dc0 & 0xffff) + 1) << 1;
>      sram_size = (board->dc0 >> 18) + 1;
>      pic = armv7m_init(get_system_memory(),
> -                      flash_size, sram_size, kernel_filename, cpu_model);
> +                      flash_size, 0, sram_size, 64, kernel_filename, 
> cpu_model);
>
>      if (board->dc1 & (1 << 16)) {
>          dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index cefc9e6..72f0266 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -16,7 +16,8 @@
>
>  /* armv7m.c */
>  qemu_irq *armv7m_init(MemoryRegion *system_memory,
> -                      int flash_size, int sram_size,
> +                      int flash_size, uint64_t flash_base,
> +                      int sram_size, int num_irq,
>                        const char *kernel_filename, const char *cpu_model);
>
>  /* arm_boot.c */
> --
> 1.9.1
>
>