From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39614 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PgJvi-0002hG-4d for qemu-devel@nongnu.org; Fri, 21 Jan 2011 11:33:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PgJvg-0000CW-6I for qemu-devel@nongnu.org; Fri, 21 Jan 2011 11:33:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:64097) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PgJvf-0000CS-P9 for qemu-devel@nongnu.org; Fri, 21 Jan 2011 11:33:40 -0500 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH 5/5] pxa2xx_gpio: switch to using qdev References: <1295538751-9567-2-git-send-email-dbaryshkov@gmail.com> <1295604733-5280-3-git-send-email-dbaryshkov@gmail.com> Date: Fri, 21 Jan 2011 17:33:25 +0100 In-Reply-To: <1295604733-5280-3-git-send-email-dbaryshkov@gmail.com> (Dmitry Eremin-Solenikov's message of "Fri, 21 Jan 2011 13:12:13 +0300") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Eremin-Solenikov Cc: qemu-devel@nongnu.org Dmitry Eremin-Solenikov writes: > Signed-off-by: Dmitry Eremin-Solenikov > --- > 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); [...]