Patchwork [05/11] pxa2xx_pic: update to use qdev and arm-pic

login
register
mail settings
Submitter Dmitry Eremin-Solenikov
Date Jan. 31, 2011, 3:20 p.m.
Message ID <1296487250-28254-5-git-send-email-dbaryshkov@gmail.com>
Download mbox | patch
Permalink /patch/81138/
State New
Headers show

Comments

Dmitry Eremin-Solenikov - Jan. 31, 2011, 3:20 p.m.
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(-)
andrzej zaborowski - Feb. 11, 2011, 1:20 a.m.
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
Dmitry Eremin-Solenikov - Feb. 11, 2011, 8:18 p.m.
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
>
Dmitry Eremin-Solenikov - Feb. 11, 2011, 8:24 p.m.
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.
andrzej zaborowski - Feb. 11, 2011, 10:17 p.m.
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
andrzej zaborowski - Feb. 11, 2011, 10:19 p.m.
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

Patch

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);