[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] pxa2xx_gpio: switch to using qdev
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] pxa2xx_gpio: switch to using qdev |
Date: |
Fri, 21 Jan 2011 17:33:25 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Dmitry Eremin-Solenikov <address@hidden> writes:
> Signed-off-by: Dmitry Eremin-Solenikov <address@hidden>
> ---
> 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);
[...]