Re: [Qemu-devel] [PATCH v3 1/3] armv7-m: Return DeviceState* from armv7m_init()

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

Missing CC of Alistair for STM32F205.

On Sun, Oct 11, 2015 at 8:36 PM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
> Change armv7m_init to return the DeviceState* for the NVIC.
> This allows access to all GPIO blocks, not just the IRQ inputs.
> Move qdev_get_gpio_in() calls out of armv7m_init() into
> board code for stellaris and stm32f205 boards.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>

This is a good step towards QOMification, and I like it in its own right.

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

> ---
>  hw/arm/armv7m.c        |  9 ++-------
>  hw/arm/stellaris.c     | 29 ++++++++++++++++++-----------
>  hw/arm/stm32f205_soc.c | 15 ++++++++-------
>  include/hw/arm/arm.h   |  2 +-
>  4 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index eb214db..a80d2ad 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -166,17 +166,15 @@ static void armv7m_reset(void *opaque)
>     mem_size is in bytes.
>     Returns the NVIC array.  */
>
> -qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> +DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>                        const char *kernel_filename, const char *cpu_model)
>  {
>      ARMCPU *cpu;
>      CPUARMState *env;
>      DeviceState *nvic;
> -    qemu_irq *pic = g_new(qemu_irq, num_irq);
>      int image_size;
>      uint64_t entry;
>      uint64_t lowaddr;
> -    int i;
>      int big_endian;
>      MemoryRegion *hack = g_new(MemoryRegion, 1);
>
> @@ -198,9 +196,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      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 < num_irq; i++) {
> -        pic[i] = qdev_get_gpio_in(nvic, i);
> -    }
>
>  #ifdef TARGET_WORDS_BIGENDIAN
>      big_endian = 1;
> @@ -234,7 +229,7 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      memory_region_add_subregion(system_memory, 0xfffff000, hack);
>
>      qemu_register_reset(armv7m_reset, cpu);
> -    return pic;
> +    return nvic;
>  }
>
>  static Property bitband_properties[] = {
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 3d6486f..82a4ad5 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1210,8 +1210,7 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>          0x40024000, 0x40025000, 0x40026000};
>      static const int gpio_irq[7] = {0, 1, 2, 3, 4, 30, 31};
>
> -    qemu_irq *pic;
> -    DeviceState *gpio_dev[7];
> +    DeviceState *gpio_dev[7], *nvic;
>      qemu_irq gpio_in[7][8];
>      qemu_irq gpio_out[7][8];
>      qemu_irq adc;
> @@ -1241,12 +1240,16 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>      vmstate_register_ram_global(sram);
>      memory_region_add_subregion(system_memory, 0x20000000, sram);
>
> -    pic = armv7m_init(system_memory, flash_size, NUM_IRQ_LINES,
> +    nvic = armv7m_init(system_memory, flash_size, NUM_IRQ_LINES,
>                        kernel_filename, cpu_model);
>
>      if (board->dc1 & (1 << 16)) {
>          dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
> -                                    pic[14], pic[15], pic[16], pic[17], NULL);
> +                                    qdev_get_gpio_in(nvic, 14),
> +                                    qdev_get_gpio_in(nvic, 15),
> +                                    qdev_get_gpio_in(nvic, 16),
> +                                    qdev_get_gpio_in(nvic, 17),
> +                                    NULL);
>          adc = qdev_get_gpio_in(dev, 0);
>      } else {
>          adc = NULL;
> @@ -1255,19 +1258,21 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>          if (board->dc2 & (0x10000 << i)) {
>              dev = sysbus_create_simple(TYPE_STELLARIS_GPTM,
>                                         0x40030000 + i * 0x1000,
> -                                       pic[timer_irq[i]]);
> +                                       qdev_get_gpio_in(nvic, timer_irq[i]));
>              /* TODO: This is incorrect, but we get away with it because
>                 the ADC output is only ever pulsed.  */
>              qdev_connect_gpio_out(dev, 0, adc);
>          }
>      }
>
> -    stellaris_sys_init(0x400fe000, pic[28], board, nd_table[0].macaddr.a);
> +    stellaris_sys_init(0x400fe000, qdev_get_gpio_in(nvic, 28),
> +                       board, nd_table[0].macaddr.a);
>
>      for (i = 0; i < 7; i++) {
>          if (board->dc4 & (1 << i)) {
>              gpio_dev[i] = sysbus_create_simple("pl061_luminary", gpio_addr[i],
> -                                               pic[gpio_irq[i]]);
> +                                               qdev_get_gpio_in(nvic,
> +                                                                gpio_irq[i]));
>              for (j = 0; j < 8; j++) {
>                  gpio_in[i][j] = qdev_get_gpio_in(gpio_dev[i], j);
>                  gpio_out[i][j] = NULL;
> @@ -1276,7 +1281,8 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>      }
>
>      if (board->dc2 & (1 << 12)) {
> -        dev = sysbus_create_simple(TYPE_STELLARIS_I2C, 0x40020000, pic[8]);
> +        dev = sysbus_create_simple(TYPE_STELLARIS_I2C, 0x40020000,
> +                                   qdev_get_gpio_in(nvic, 8));
>          i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
>          if (board->peripherals & BP_OLED_I2C) {
>              i2c_create_slave(i2c, "ssd0303", 0x3d);
> @@ -1286,11 +1292,12 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>      for (i = 0; i < 4; i++) {
>          if (board->dc2 & (1 << i)) {
>              sysbus_create_simple("pl011_luminary", 0x4000c000 + i * 0x1000,
> -                                 pic[uart_irq[i]]);
> +                                 qdev_get_gpio_in(nvic, uart_irq[i]));
>          }
>      }
>      if (board->dc2 & (1 << 4)) {
> -        dev = sysbus_create_simple("pl022", 0x40008000, pic[7]);
> +        dev = sysbus_create_simple("pl022", 0x40008000,
> +                                   qdev_get_gpio_in(nvic, 7));
>          if (board->peripherals & BP_OLED_SSI) {
>              void *bus;
>              DeviceState *sddev;
> @@ -1326,7 +1333,7 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
>          qdev_set_nic_properties(enet, &nd_table[0]);
>          qdev_init_nofail(enet);
>          sysbus_mmio_map(SYS_BUS_DEVICE(enet), 0, 0x40048000);
> -        sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, pic[42]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 42));
>      }
>      if (board->peripherals & BP_GAMEPAD) {
>          qemu_irq gpad_irq[5];
> diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
> index 4d26a7e..3f99340 100644
> --- a/hw/arm/stm32f205_soc.c
> +++ b/hw/arm/stm32f205_soc.c
> @@ -59,9 +59,8 @@ static void stm32f205_soc_initfn(Object *obj)
>  static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      STM32F205State *s = STM32F205_SOC(dev_soc);
> -    DeviceState *syscfgdev, *usartdev, *timerdev;
> +    DeviceState *syscfgdev, *usartdev, *timerdev, *nvic;
>      SysBusDevice *syscfgbusdev, *usartbusdev, *timerbusdev;
> -    qemu_irq *pic;
>      Error *err = NULL;
>      int i;
>
> @@ -88,8 +87,8 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>      vmstate_register_ram_global(sram);
>      memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, sram);
>
> -    pic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> -                      s->kernel_filename, s->cpu_model);
> +    nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> +                       s->kernel_filename, s->cpu_model);
>
>      /* System configuration controller */
>      syscfgdev = DEVICE(&s->syscfg);
> @@ -100,7 +99,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>      syscfgbusdev = SYS_BUS_DEVICE(syscfgdev);
>      sysbus_mmio_map(syscfgbusdev, 0, 0x40013800);
> -    sysbus_connect_irq(syscfgbusdev, 0, pic[71]);
> +    sysbus_connect_irq(syscfgbusdev, 0, qdev_get_gpio_in(nvic, 71));
>
>      /* Attach UART (uses USART registers) and USART controllers */
>      for (i = 0; i < STM_NUM_USARTS; i++) {
> @@ -112,7 +111,8 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>          }
>          usartbusdev = SYS_BUS_DEVICE(usartdev);
>          sysbus_mmio_map(usartbusdev, 0, usart_addr[i]);
> -        sysbus_connect_irq(usartbusdev, 0, pic[usart_irq[i]]);
> +        sysbus_connect_irq(usartbusdev, 0,
> +                           qdev_get_gpio_in(nvic, usart_irq[i]));
>      }
>
>      /* Timer 2 to 5 */
> @@ -126,7 +126,8 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>          }
>          timerbusdev = SYS_BUS_DEVICE(timerdev);
>          sysbus_mmio_map(timerbusdev, 0, timer_addr[i]);
> -        sysbus_connect_irq(timerbusdev, 0, pic[timer_irq[i]]);
> +        sysbus_connect_irq(timerbusdev, 0,
> +                           qdev_get_gpio_in(nvic, timer_irq[i]));
>      }
>  }
>
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 4dcd4f9..7916b6b 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -17,7 +17,7 @@
>  #include "cpu.h"
>
>  /* armv7m.c */
> -qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> +DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>                        const char *kernel_filename, const char *cpu_model);
>
>  /*
> --
> 2.1.4
>