Message ID | 1296487250-28254-5-git-send-email-dbaryshkov@gmail.com |
---|---|
State | New |
Headers | show |
On 31 January 2011 16:20, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > pxa2xx_pic duplicated some code from arm-pic. Drop it, replacing with > references to arm-pic. Also use qdev/sysbus framework to handle > pxa2xx-pic. The duplication involves about 4 lines of code, at this level I strongly prefer to not add a level of calls that can't be inlined in a fast path. I guess the choice not to involve arm_pic was conscious. Otherwise this patch and all the remaining patches look good to me, except some minor remarks: In patch 6 I'd prefer not to call qdev_get_gpio_in in pxa2xx_rtc_int_update and similarly in patch 10 in mst_fpga_update_gpio, let's store the irq's in the state struct. I also prefer not to make lines shorter than 80 chars longer, they wrap on my netbook. I pushed patches 1-3. Cheers
On 2/11/11, andrzej zaborowski <balrogg@gmail.com> wrote: > On 31 January 2011 16:20, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > wrote: >> pxa2xx_pic duplicated some code from arm-pic. Drop it, replacing with >> references to arm-pic. Also use qdev/sysbus framework to handle >> pxa2xx-pic. > > The duplication involves about 4 lines of code, at this level I > strongly prefer to not add a level of calls that can't be inlined in a > fast path. I guess the choice not to involve arm_pic was conscious. I just planned to later reuse allocated arm-pic IRQ's (the new one) to be passed to pxa2xx-gpio (to drop usage of cpu-env). I think. I can still allocate arm-pic but use only the last IRQ from it. Will that be suitable for you? > > Otherwise this patch and all the remaining patches look good to me, > except some minor remarks: > > In patch 6 I'd prefer not to call qdev_get_gpio_in in > pxa2xx_rtc_int_update and similarly in patch 10 in Fixed. > mst_fpga_update_gpio, let's store the irq's in the state struct. Will inlining qdev_get_gpio_in with direct access to qdev->gpio_in[] OK? > > I also prefer not to make lines shorter than 80 chars longer, they > wrap on my netbook. > > I pushed patches 1-3. > > Cheers >
Hello, On 2/11/11, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > On 2/11/11, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 31 January 2011 16:20, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> wrote: >>> pxa2xx_pic duplicated some code from arm-pic. Drop it, replacing with >>> references to arm-pic. Also use qdev/sysbus framework to handle >>> pxa2xx-pic. >> >> The duplication involves about 4 lines of code, at this level I >> strongly prefer to not add a level of calls that can't be inlined in a >> fast path. I guess the choice not to involve arm_pic was conscious. > > I just planned to later reuse allocated arm-pic IRQ's (the new one) to > be passed to pxa2xx-gpio (to drop usage of cpu-env). I think. I can > still allocate > arm-pic but use only the last IRQ from it. Will that be suitable for you? I recollected the reason for this change: there is no clean way to pass CPUState to qdev. So It's either DEFINE_PROP_PTR() - like hack, or encapsulating thins into qemu_irq arm_pic is used by several other ARM SoC emulators.
Hi, On 11 February 2011 21:24, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > On 2/11/11, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: >> I just planned to later reuse allocated arm-pic IRQ's (the new one) to >> be passed to pxa2xx-gpio (to drop usage of cpu-env). I think. I can >> still allocate >> arm-pic but use only the last IRQ from it. Will that be suitable for you? > > I recollected the reason for this change: there is no clean way to pass CPUState > to qdev. So It's either DEFINE_PROP_PTR() - like hack, or > encapsulating thins into > qemu_irq That's true, but in that patchset pxa2xx_pic is not turned into a qdev device (separate from pxa2xx core). Do you think there's any value in doing that? I think it's ok if it doesn't present any issues/overheads or if there's a clear benefit from doing that. Cheers
On 11 February 2011 21:18, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > On 2/11/11, andrzej zaborowski <balrogg@gmail.com> wrote: >> On 31 January 2011 16:20, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> wrote: >>> pxa2xx_pic duplicated some code from arm-pic. Drop it, replacing with >>> references to arm-pic. Also use qdev/sysbus framework to handle >>> pxa2xx-pic. >> >> The duplication involves about 4 lines of code, at this level I >> strongly prefer to not add a level of calls that can't be inlined in a >> fast path. I guess the choice not to involve arm_pic was conscious. > > I just planned to later reuse allocated arm-pic IRQ's (the new one) to > be passed to pxa2xx-gpio (to drop usage of cpu-env). I think. I can > still allocate > arm-pic but use only the last IRQ from it. Will that be suitable for you? > >> >> Otherwise this patch and all the remaining patches look good to me, >> except some minor remarks: >> >> In patch 6 I'd prefer not to call qdev_get_gpio_in in >> pxa2xx_rtc_int_update and similarly in patch 10 in > > Fixed. > >> mst_fpga_update_gpio, let's store the irq's in the state struct. > > Will inlining qdev_get_gpio_in with direct access to qdev->gpio_in[] OK? There's a comment in hw/qdev.h that says not to do that. I'm not sure why qdev_get_gpio_in is not a static inline function, but it'd be easiest to store the irq array in the mst_fpga's state. Cheers
diff --git a/hw/pxa2xx_pic.c b/hw/pxa2xx_pic.c index a36da23..5b5ce72 100644 --- a/hw/pxa2xx_pic.c +++ b/hw/pxa2xx_pic.c @@ -10,6 +10,8 @@ #include "hw.h" #include "pxa.h" +#include "arm-misc.h" +#include "sysbus.h" #define ICIP 0x00 /* Interrupt Controller IRQ Pending register */ #define ICMR 0x04 /* Interrupt Controller Mask register */ @@ -31,7 +33,10 @@ #define PXA2XX_PIC_SRCS 40 typedef struct { - CPUState *cpu_env; + SysBusDevice busdev; + qemu_irq hard_irq; + qemu_irq fiq_irq; + qemu_irq wake_irq; uint32_t int_enabled[2]; uint32_t int_pending[2]; uint32_t is_fiq[2]; @@ -44,25 +49,16 @@ static void pxa2xx_pic_update(void *opaque) uint32_t mask[2]; PXA2xxPICState *s = (PXA2xxPICState *) opaque; - if (s->cpu_env->halted) { - mask[0] = s->int_pending[0] & (s->int_enabled[0] | s->int_idle); - mask[1] = s->int_pending[1] & (s->int_enabled[1] | s->int_idle); - if (mask[0] || mask[1]) - cpu_interrupt(s->cpu_env, CPU_INTERRUPT_EXITTB); - } + mask[0] = s->int_pending[0] & (s->int_enabled[0] | s->int_idle); + mask[1] = s->int_pending[1] & (s->int_enabled[1] | s->int_idle); + qemu_set_irq(s->wake_irq, mask[0] || mask[1]); mask[0] = s->int_pending[0] & s->int_enabled[0]; mask[1] = s->int_pending[1] & s->int_enabled[1]; - if ((mask[0] & s->is_fiq[0]) || (mask[1] & s->is_fiq[1])) - cpu_interrupt(s->cpu_env, CPU_INTERRUPT_FIQ); - else - cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_FIQ); + qemu_set_irq(s->fiq_irq, ((mask[0] & s->is_fiq[0]) || (mask[1] & s->is_fiq[1]))); - if ((mask[0] & ~s->is_fiq[0]) || (mask[1] & ~s->is_fiq[1])) - cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD); - else - cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD); + qemu_set_irq(s->hard_irq, ((mask[0] & ~s->is_fiq[0]) || (mask[1] & ~s->is_fiq[1]))); } /* Note: Here level means state of the signal on a pin, not @@ -241,53 +237,36 @@ static CPUWriteMemoryFunc * const pxa2xx_pic_writefn[] = { pxa2xx_pic_mem_write, }; -static void pxa2xx_pic_save(QEMUFile *f, void *opaque) -{ - PXA2xxPICState *s = (PXA2xxPICState *) opaque; - int i; - - for (i = 0; i < 2; i ++) - qemu_put_be32s(f, &s->int_enabled[i]); - for (i = 0; i < 2; i ++) - qemu_put_be32s(f, &s->int_pending[i]); - for (i = 0; i < 2; i ++) - qemu_put_be32s(f, &s->is_fiq[i]); - qemu_put_be32s(f, &s->int_idle); - for (i = 0; i < PXA2XX_PIC_SRCS; i ++) - qemu_put_be32s(f, &s->priority[i]); -} - -static int pxa2xx_pic_load(QEMUFile *f, void *opaque, int version_id) +static int pxa2xx_pic_post_load(void *opaque, int version_id) { - PXA2xxPICState *s = (PXA2xxPICState *) opaque; - int i; - - for (i = 0; i < 2; i ++) - qemu_get_be32s(f, &s->int_enabled[i]); - for (i = 0; i < 2; i ++) - qemu_get_be32s(f, &s->int_pending[i]); - for (i = 0; i < 2; i ++) - qemu_get_be32s(f, &s->is_fiq[i]); - qemu_get_be32s(f, &s->int_idle); - for (i = 0; i < PXA2XX_PIC_SRCS; i ++) - qemu_get_be32s(f, &s->priority[i]); - pxa2xx_pic_update(opaque); return 0; } qemu_irq *pxa2xx_pic_init(target_phys_addr_t base, CPUState *env) { - PXA2xxPICState *s; - int iomemtype; qemu_irq *qi; + DeviceState *dev; - s = (PXA2xxPICState *) - qemu_mallocz(sizeof(PXA2xxPICState)); - if (!s) - return NULL; + qi = arm_pic_init_cpu(env); - s->cpu_env = env; + dev = sysbus_create_varargs("pxa2xx_pic", base, + qi[ARM_PIC_CPU_IRQ], + qi[ARM_PIC_CPU_FIQ], + qi[ARM_PIC_CPU_WAKE], + NULL); + + /* Enable IC coprocessor access. */ + cpu_arm_set_cp_io(env, 6, pxa2xx_pic_cp_read, pxa2xx_pic_cp_write, dev); + + /* FIXME */ + return dev->gpio_in; +} + +static int pxa2xx_pic_initfn(SysBusDevice *dev) +{ + PXA2xxPICState *s = FROM_SYSBUS(PXA2xxPICState, dev); + int iomemtype; s->int_pending[0] = 0; s->int_pending[1] = 0; @@ -296,18 +275,46 @@ qemu_irq *pxa2xx_pic_init(target_phys_addr_t base, CPUState *env) s->is_fiq[0] = 0; s->is_fiq[1] = 0; - qi = qemu_allocate_irqs(pxa2xx_pic_set_irq, s, PXA2XX_PIC_SRCS); + sysbus_init_irq(dev, &s->hard_irq); + sysbus_init_irq(dev, &s->fiq_irq); + sysbus_init_irq(dev, &s->wake_irq); + + qdev_init_gpio_in(&dev->qdev, pxa2xx_pic_set_irq, PXA2XX_PIC_SRCS); /* Enable IC memory-mapped registers access. */ iomemtype = cpu_register_io_memory(pxa2xx_pic_readfn, pxa2xx_pic_writefn, s, DEVICE_NATIVE_ENDIAN); - cpu_register_physical_memory(base, 0x00100000, iomemtype); + sysbus_init_mmio(dev, 0x00100000, iomemtype); - /* Enable IC coprocessor access. */ - cpu_arm_set_cp_io(env, 6, pxa2xx_pic_cp_read, pxa2xx_pic_cp_write, s); + return 0; +} + +static VMStateDescription vmstate_pxa2xx_pic_regs = { + .name = "pxa2xx_pic", + .version_id = 0, + .minimum_version_id = 0, + .minimum_version_id_old = 0, + .post_load = pxa2xx_pic_post_load, + .fields = (VMStateField []) { + VMSTATE_UINT32_ARRAY(int_enabled, PXA2xxPICState, 2), + VMSTATE_UINT32_ARRAY(int_pending, PXA2xxPICState, 2), + VMSTATE_UINT32_ARRAY(is_fiq, PXA2xxPICState, 2), + VMSTATE_UINT32(int_idle, PXA2xxPICState), + VMSTATE_UINT32_ARRAY(priority, PXA2xxPICState, PXA2XX_PIC_SRCS), + VMSTATE_END_OF_LIST(), + }, +}; - register_savevm(NULL, "pxa2xx_pic", 0, 0, pxa2xx_pic_save, - pxa2xx_pic_load, s); +static SysBusDeviceInfo pxa2xx_pic_info = { + .init = pxa2xx_pic_initfn, + .qdev.name = "pxa2xx_pic", + .qdev.desc = "PXA2xx PIC", + .qdev.size = sizeof(PXA2xxPICState), + .qdev.vmsd = &vmstate_pxa2xx_pic_regs, +}; - return qi; +static void pxa2xx_pic_register(void) +{ + sysbus_register_withprop(&pxa2xx_pic_info); } +device_init(pxa2xx_pic_register);
pxa2xx_pic duplicated some code from arm-pic. Drop it, replacing with references to arm-pic. Also use qdev/sysbus framework to handle pxa2xx-pic. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- hw/pxa2xx_pic.c | 125 +++++++++++++++++++++++++++++-------------------------- 1 files changed, 66 insertions(+), 59 deletions(-)