Re: [Qemu-devel] [PATCH 5/5] pxa2xx_gpio: switch to using qdev

[Markus Armbruster <armbru@redhat.com>]

Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:

> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  hw/gumstix.c     |    4 +-
>  hw/pxa.h         |   10 +---
>  hw/pxa2xx.c      |    4 +-
>  hw/pxa2xx_gpio.c |  151 ++++++++++++++++++++++++++----------------------------
>  hw/spitz.c       |   34 ++++++------
>  hw/tosa.c        |   12 ++--
>  6 files changed, 102 insertions(+), 113 deletions(-)
>
> diff --git a/hw/gumstix.c b/hw/gumstix.c
> index af8b464..ee63f63 100644
> --- a/hw/gumstix.c
> +++ b/hw/gumstix.c
> @@ -78,7 +78,7 @@ static void connex_init(ram_addr_t ram_size,
>  
>      /* Interrupt line of NIC is connected to GPIO line 36 */
>      smc91c111_init(&nd_table[0], 0x04000300,
> -                    pxa2xx_gpio_in_get(cpu->gpio)[36]);
> +                    qdev_get_gpio_in(cpu->gpio, 36));
>  }
>  
>  static void verdex_init(ram_addr_t ram_size,
> @@ -117,7 +117,7 @@ static void verdex_init(ram_addr_t ram_size,
>  
>      /* Interrupt line of NIC is connected to GPIO line 99 */
>      smc91c111_init(&nd_table[0], 0x04000300,
> -                    pxa2xx_gpio_in_get(cpu->gpio)[99]);
> +                    qdev_get_gpio_in(cpu->gpio, 99));
>  }
>  
>  static QEMUMachine connex_machine = {
> diff --git a/hw/pxa.h b/hw/pxa.h
> index 8d6a8c3..f73d33b 100644
> --- a/hw/pxa.h
> +++ b/hw/pxa.h
> @@ -70,13 +70,9 @@ void pxa25x_timer_init(target_phys_addr_t base, qemu_irq *irqs);
>  void pxa27x_timer_init(target_phys_addr_t base, qemu_irq *irqs, qemu_irq irq4);
>  
>  /* pxa2xx_gpio.c */
> -typedef struct PXA2xxGPIOInfo PXA2xxGPIOInfo;
> -PXA2xxGPIOInfo *pxa2xx_gpio_init(target_phys_addr_t base,
> +DeviceState *pxa2xx_gpio_init(target_phys_addr_t base,
>                  CPUState *env, qemu_irq *pic, int lines);
> -qemu_irq *pxa2xx_gpio_in_get(PXA2xxGPIOInfo *s);
> -void pxa2xx_gpio_out_set(PXA2xxGPIOInfo *s,
> -                int line, qemu_irq handler);
> -void pxa2xx_gpio_read_notifier(PXA2xxGPIOInfo *s, qemu_irq handler);
> +void pxa2xx_gpio_read_notifier(DeviceState *dev, qemu_irq handler);
>  
>  /* pxa2xx_dma.c */
>  typedef struct PXA2xxDMAState PXA2xxDMAState;
> @@ -132,7 +128,7 @@ typedef struct {
>      qemu_irq *pic;
>      qemu_irq reset;
>      PXA2xxDMAState *dma;
> -    PXA2xxGPIOInfo *gpio;
> +    DeviceState *gpio;
>      PXA2xxLCDState *lcd;
>      SSIBus **ssp;
>      PXA2xxI2CState *i2c[2];
> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
> index 6e72a5c..d966846 100644
> --- a/hw/pxa2xx.c
> +++ b/hw/pxa2xx.c
> @@ -2158,7 +2158,7 @@ PXA2xxState *pxa270_init(unsigned int sdram_size, const char *revision)
>  
>      /* GPIO1 resets the processor */
>      /* The handler can be overridden by board-specific code */
> -    pxa2xx_gpio_out_set(s->gpio, 1, s->reset);
> +    qdev_connect_gpio_out(s->gpio, 1, s->reset);
>      return s;
>  }
>  
> @@ -2279,7 +2279,7 @@ PXA2xxState *pxa255_init(unsigned int sdram_size)
>  
>      /* GPIO1 resets the processor */
>      /* The handler can be overridden by board-specific code */
> -    pxa2xx_gpio_out_set(s->gpio, 1, s->reset);
> +    qdev_connect_gpio_out(s->gpio, 1, s->reset);
>      return s;
>  }
>  
> diff --git a/hw/pxa2xx_gpio.c b/hw/pxa2xx_gpio.c
> index 0d03446..295a0ff 100644
> --- a/hw/pxa2xx_gpio.c
> +++ b/hw/pxa2xx_gpio.c
> @@ -8,15 +8,17 @@
>   */
>  
>  #include "hw.h"
> +#include "sysbus.h"
>  #include "pxa.h"
>  
>  #define PXA2XX_GPIO_BANKS	4
>  
> +typedef struct PXA2xxGPIOInfo PXA2xxGPIOInfo;
>  struct PXA2xxGPIOInfo {
> -    qemu_irq *pic;
> +    SysBusDevice busdev;
> +    qemu_irq irq0, irq1, irqX;
>      int lines;
> -    CPUState *cpu_env;
> -    qemu_irq *in;
> +    void *cpu_env;

cpu_env made void * here because you DEFINE_PROP_PTR() it in
pxa2xxgpioinfo.  DEFINE_PROP_PTR() is for dirty hacks only.  Which means
you got one around here.  See use of cpu_env below.

>  
>      /* XXX: GNU C vectors are more suitable */
>      uint32_t ilevel[PXA2XX_GPIO_BANKS];
> @@ -66,19 +68,19 @@ static struct {
>  static void pxa2xx_gpio_irq_update(PXA2xxGPIOInfo *s)
>  {
>      if (s->status[0] & (1 << 0))
> -        qemu_irq_raise(s->pic[PXA2XX_PIC_GPIO_0]);
> +        qemu_irq_raise(s->irq0);
>      else
> -        qemu_irq_lower(s->pic[PXA2XX_PIC_GPIO_0]);
> +        qemu_irq_lower(s->irq0);
>  
>      if (s->status[0] & (1 << 1))
> -        qemu_irq_raise(s->pic[PXA2XX_PIC_GPIO_1]);
> +        qemu_irq_raise(s->irq1);
>      else
> -        qemu_irq_lower(s->pic[PXA2XX_PIC_GPIO_1]);
> +        qemu_irq_lower(s->irq1);
>  
>      if ((s->status[0] & ~3) | s->status[1] | s->status[2] | s->status[3])
> -        qemu_irq_raise(s->pic[PXA2XX_PIC_GPIO_X]);
> +        qemu_irq_raise(s->irqX);
>      else
> -        qemu_irq_lower(s->pic[PXA2XX_PIC_GPIO_X]);
> +        qemu_irq_lower(s->irqX);
>  }
>  
>  /* Bitmap of pins used as standby and sleep wake-up sources.  */
> @@ -114,7 +116,7 @@ static void pxa2xx_gpio_set(void *opaque, int line, int level)
>          pxa2xx_gpio_irq_update(s);
>  
>      /* Wake-up GPIOs */
> -    if (s->cpu_env->halted && (mask & ~s->dir[bank] & pxa2xx_gpio_wake[bank]))
> +    if (((CPUARMState*)s->cpu_env)->halted && (mask & ~s->dir[bank] & pxa2xx_gpio_wake[bank]))
>          cpu_interrupt(s->cpu_env, CPU_INTERRUPT_EXITTB);
>  }
>  

Cast needed because cpu_env is now void *.

This use of cpu_env is your dirty hack.  Any way to avoid it?

For what it's worth, it's similar to the existing dirty hack in
hw/apic.c.

If you really can't avoid the hack, at least cast to CPUState.

> @@ -249,96 +251,87 @@ static CPUWriteMemoryFunc * const pxa2xx_gpio_writefn[] = {
>      pxa2xx_gpio_write
>  };
>  
> -static void pxa2xx_gpio_save(QEMUFile *f, void *opaque)
> -{
> -    PXA2xxGPIOInfo *s = (PXA2xxGPIOInfo *) opaque;
> -    int i;
> -
> -    qemu_put_be32(f, s->lines);
> -
> -    for (i = 0; i < PXA2XX_GPIO_BANKS; i ++) {
> -        qemu_put_be32s(f, &s->ilevel[i]);
> -        qemu_put_be32s(f, &s->olevel[i]);
> -        qemu_put_be32s(f, &s->dir[i]);
> -        qemu_put_be32s(f, &s->rising[i]);
> -        qemu_put_be32s(f, &s->falling[i]);
> -        qemu_put_be32s(f, &s->status[i]);
> -        qemu_put_be32s(f, &s->gafr[i * 2 + 0]);
> -        qemu_put_be32s(f, &s->gafr[i * 2 + 1]);
> -
> -        qemu_put_be32s(f, &s->prev_level[i]);
> -    }
> -}
> -
> -static int pxa2xx_gpio_load(QEMUFile *f, void *opaque, int version_id)
> +DeviceState *pxa2xx_gpio_init(target_phys_addr_t base,
> +                CPUState *env, qemu_irq *pic, int lines)
>  {
> -    PXA2xxGPIOInfo *s = (PXA2xxGPIOInfo *) opaque;
> -    int i;
> +    DeviceState *dev;
>  
> -    if (qemu_get_be32(f) != s->lines)
> -        return -EINVAL;
> +    dev = qdev_create(NULL, "pxa2xx-gpio");
> +    qdev_prop_set_int32(dev, "lines", lines);
> +    qdev_prop_set_ptr(dev, "cpu", env);
> +    qdev_init_nofail(dev);
>  
> -    for (i = 0; i < PXA2XX_GPIO_BANKS; i ++) {
> -        qemu_get_be32s(f, &s->ilevel[i]);
> -        qemu_get_be32s(f, &s->olevel[i]);
> -        qemu_get_be32s(f, &s->dir[i]);
> -        qemu_get_be32s(f, &s->rising[i]);
> -        qemu_get_be32s(f, &s->falling[i]);
> -        qemu_get_be32s(f, &s->status[i]);
> -        qemu_get_be32s(f, &s->gafr[i * 2 + 0]);
> -        qemu_get_be32s(f, &s->gafr[i * 2 + 1]);
> -
> -        qemu_get_be32s(f, &s->prev_level[i]);
> -    }
> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
> +    sysbus_connect_irq(sysbus_from_qdev(dev), 0, pic[PXA2XX_PIC_GPIO_0]);
> +    sysbus_connect_irq(sysbus_from_qdev(dev), 1, pic[PXA2XX_PIC_GPIO_1]);
> +    sysbus_connect_irq(sysbus_from_qdev(dev), 2, pic[PXA2XX_PIC_GPIO_X]);
>  
> -    return 0;
> +    return dev;
>  }
>  
> -PXA2xxGPIOInfo *pxa2xx_gpio_init(target_phys_addr_t base,
> -                CPUState *env, qemu_irq *pic, int lines)
> +static int pxa2xx_gpio_initfn(SysBusDevice *dev)
>  {
>      int iomemtype;
>      PXA2xxGPIOInfo *s;
>  
> -    s = (PXA2xxGPIOInfo *)
> -            qemu_mallocz(sizeof(PXA2xxGPIOInfo));
> -    memset(s, 0, sizeof(PXA2xxGPIOInfo));
> -    s->pic = pic;
> -    s->lines = lines;
> -    s->cpu_env = env;
> -    s->in = qemu_allocate_irqs(pxa2xx_gpio_set, s, lines);
> +    s = FROM_SYSBUS(PXA2xxGPIOInfo, dev);
> +
> +    qdev_init_gpio_in(&dev->qdev, pxa2xx_gpio_set, s->lines);
> +    qdev_init_gpio_out(&dev->qdev, s->handler, s->lines);
>  
>      iomemtype = cpu_register_io_memory(pxa2xx_gpio_readfn,
>                      pxa2xx_gpio_writefn, s, DEVICE_NATIVE_ENDIAN);
> -    cpu_register_physical_memory(base, 0x00001000, iomemtype);
> -
> -    register_savevm(NULL, "pxa2xx_gpio", 0, 0,
> -                    pxa2xx_gpio_save, pxa2xx_gpio_load, s);
>  
> -    return s;
> -}
> +    sysbus_init_mmio(dev, 0x1000, iomemtype);
> +    sysbus_init_irq(dev, &s->irq0);
> +    sysbus_init_irq(dev, &s->irq1);
> +    sysbus_init_irq(dev, &s->irqX);
>  
> -qemu_irq *pxa2xx_gpio_in_get(PXA2xxGPIOInfo *s)
> -{
> -    return s->in;
> -}
> -
> -void pxa2xx_gpio_out_set(PXA2xxGPIOInfo *s,
> -                int line, qemu_irq handler)
> -{
> -    if (line >= s->lines) {
> -        printf("%s: No GPIO pin %i\n", __FUNCTION__, line);
> -        return;
> -    }
> -
> -    s->handler[line] = handler;
> +    return 0;
>  }
>  
>  /*
>   * Registers a callback to notify on GPLR reads.  This normally
>   * shouldn't be needed but it is used for the hack on Spitz machines.
>   */
> -void pxa2xx_gpio_read_notifier(PXA2xxGPIOInfo *s, qemu_irq handler)
> +void pxa2xx_gpio_read_notifier(DeviceState *dev, qemu_irq handler)
>  {
> +    PXA2xxGPIOInfo *s = FROM_SYSBUS(PXA2xxGPIOInfo, sysbus_from_qdev(dev));
>      s->read_notify = handler;
>  }
> +
> +static const VMStateDescription vmstate_pxa2xx_gpio_regs = {
> +    .name = "pxa2xx-gpio",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_INT32(lines, PXA2xxGPIOInfo),
> +        VMSTATE_UINT32_ARRAY(ilevel, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(olevel, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(dir, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(rising, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(falling, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(status, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS),
> +        VMSTATE_UINT32_ARRAY(gafr, PXA2xxGPIOInfo, PXA2XX_GPIO_BANKS * 2),
> +        VMSTATE_END_OF_LIST(),

pxa2xx_gpio_save() saved in the order ilevel[0], olevel[0], ...  But
this is ordered ilevel[0], ilevel[1], ...  Are you sure you can restore
old saves?  Hmm, looks like you bumped the version_id.  Fine with me,
but it needs to be stated *prominently* in the commit message.

> +    },
> +};
> +
> +static SysBusDeviceInfo pxa2xx_gpio_info = {
> +    .init       = pxa2xx_gpio_initfn,
> +    .qdev.name  = "pxa2xx-gpio",
> +    .qdev.desc  = "PXA2xx GPIO controller",
> +    .qdev.size  = sizeof(PXA2xxGPIOInfo),
> +    .qdev.props = (Property []) {
> +        DEFINE_PROP_INT32("lines", PXA2xxGPIOInfo, lines, 0),
> +        DEFINE_PROP_PTR("cpu", PXA2xxGPIOInfo, cpu_env),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +static void pxa2xx_gpio_register(void)
> +{
> +    sysbus_register_withprop(&pxa2xx_gpio_info);
> +}
> +device_init(pxa2xx_gpio_register);
[...]