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

[Peter Crosthwaite]

This series looks good, up to Peter if is qualifies for 2.5. It was
clearly on list well before soft freeze and really my tardiness as to
why it is late with review. So I'll make a case for inclusion.

It does seem to be missing a cover, and/or the in-reply-to looks a bit
strange. Maybe patches missed it? Can you try an all-in-one resend
with git-generated cover?

Regards,
Peter

On Fri, Oct 30, 2015 at 2:15 PM, Peter Crosthwaite
<address@hidden> wrote:
> Missing CC of Alistair for STM32F205.
>
> On Sun, Oct 11, 2015 at 8:36 PM, Michael Davidsaver
> <address@hidden> 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 <address@hidden>
>
> This is a good step towards QOMification, and I like it in its own right.
>
> Reviewed-by: Peter Crosthwaite <address@hidden>
>
> 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
>>