qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 2/4] hw/intc: add sunxi interrupt controller


From: Li Guang
Subject: Re: [Qemu-devel] [PATCH v4 2/4] hw/intc: add sunxi interrupt controller device
Date: Wed, 27 Nov 2013 11:36:31 +0800
User-agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1.16) Gecko/20120421 Iceape/2.0.11

Li Guang wrote:
Peter Crosthwaite wrote:
On Tue, Nov 26, 2013 at 5:22 PM, liguang<address@hidden> wrote:
Signed-off-by: liguang<address@hidden>
---
  default-configs/arm-softmmu.mak |    1 +
  hw/intc/Makefile.objs           |    1 +
hw/intc/sunxi-pic.c | 238 +++++++++++++++++++++++++++++++++++++++
  include/hw/intc/sunxi-pic.h     |   20 ++++
  4 files changed, 260 insertions(+), 0 deletions(-)
  create mode 100644 hw/intc/sunxi-pic.c
  create mode 100644 include/hw/intc/sunxi-pic.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 7bf5ad0..bbe00e4 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -83,3 +83,4 @@ CONFIG_SDHCI=y
  CONFIG_INTEGRATOR_DEBUG=y

  CONFIG_SUNXI_PIT=y
+CONFIG_SUNXI_PIC=y
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 47ac442..dad8c43 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -12,6 +12,7 @@ common-obj-$(CONFIG_IOAPIC) += ioapic_common.o
  common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o
  common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
  common-obj-$(CONFIG_OPENPIC) += openpic.o
+common-obj-$(CONFIG_SUNXI_PIC) += sunxi-pic.o

  obj-$(CONFIG_APIC) += apic.o apic_common.o
  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
diff --git a/hw/intc/sunxi-pic.c b/hw/intc/sunxi-pic.c
new file mode 100644
index 0000000..e84fc55
--- /dev/null
+++ b/hw/intc/sunxi-pic.c
@@ -0,0 +1,238 @@
+/*
+ * Allwinner sunxi interrupt controller device emulation
+ *
+ * Copyright (C) 2013 Li Guang
+ * Written by Li Guang<address@hidden>
+ *
+ * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/devices.h"
+#include "sysemu/sysemu.h"
+#include "hw/intc/sunxi-pic.h"
+
+
+typedef struct SunxiPICState {
+    /*<  private>*/
+    SysBusDevice parent_obj;
+    /*<  public>*/
+    MemoryRegion iomem;
+    qemu_irq parent_fiq;
+    qemu_irq parent_irq;
Blank line here for readability.


OK
+    uint32_t vector;
+    uint32_t base_addr;
+    uint32_t protect;
+    uint32_t nmi;
+    uint32_t irq_pending[SUNXI_PIC_REG_IDX];
+    uint32_t fiq_pending[SUNXI_PIC_REG_IDX];
this appears to be constant 0. I cant find anywhere that sets fiq_pending.


here, only NMI can generate fiq,
and I did take care NMI now,
so there's no real case to set fiq.

+    uint32_t select[SUNXI_PIC_REG_IDX];
+    uint32_t enable[SUNXI_PIC_REG_IDX];
+    uint32_t mask[SUNXI_PIC_REG_IDX];
+    /*priority setting here*/
+} SunxiPICState;
+
+static void sunxi_pic_update(SunxiPICState *s)
+{
+    uint8_t i = 0, j = 0;
Initialisers un-needed.

OK
+    bool irq = false;
+
+    for (i = 0, j = 0; i<  SUNXI_PIC_REG_IDX; i++) {
+        for (j = 0; j<  32; j++) {
+            if (!test_bit(j, (void *)&s->mask[i])) {
You can save on a level of indentation here with:

if (test_bit(j, (void *)&s->mask[i])) {
     continue;
}

OK
+                if (test_bit(j, (void *)&s->irq_pending[i])) {
+                    qemu_set_irq(s->parent_irq, 1);
qemu_irq_raise() is simpler.

I can't find anywhere in the code where the interrupts are lowered.
How do they come down, once they are up?




+                    irq = true;
+                }
+                if (test_bit(j, (void *)&s->fiq_pending[i])&&
+                    test_bit(j, (void *)&s->select[i])) {
+                    qemu_set_irq(s->parent_fiq, 1);
+                    irq = true;
+                }
+                if (irq) {
+                    goto out;
What happens if two interrupts are both active, the first setting IRQ
the second FIQ? Wont this escape logic cause FIQ raising to
potentionally be missed?

+                }
+            }
+        }
+    }
+out:
+    return;
+}
+
+static void sunxi_pic_set_irq(void *opaque, int irq, int level)
+{
+    SunxiPICState *s = opaque;
+
+    if (level) {
+        set_bit(irq, (void *)&s->irq_pending[irq/32]);
set_bit(irq % 32, ...)


OK

No, it is wrong,
irq/32 is right.


+    }
Is this supposed to set either fiq_pending or irq_pending depending on
the select bit?


Yes, but, I as I stated before, maybe I will take NMI later.
+    sunxi_pic_update(s);
+}
+
+static uint64_t sunxi_pic_read(void *opaque, hwaddr offset, unsigned size)
+{
+    SunxiPICState *s = opaque;
+    uint8_t index = (offset&  0xc)/4;
+
+    switch (offset) {
+    case SUNXI_PIC_VECTOR:
+        return s->vector;
+        break;
+    case SUNXI_PIC_BASE_ADDR:
+        return s->base_addr;
+        break;
+    case SUNXI_PIC_PROTECT:
+        return s->protect;
+        break;
+    case SUNXI_PIC_NMI:
+        return s->nmi;
+        break;
+    case SUNXI_PIC_IRQ_PENDING ... SUNXI_PIC_IRQ_PENDING + 8:
+        return s->irq_pending[index];
+        break;
+    case SUNXI_PIC_FIQ_PENDING ... SUNXI_PIC_FIQ_PENDING + 8:
+        return s->fiq_pending[index];
+        break;
+    case SUNXI_PIC_SELECT ... SUNXI_PIC_SELECT + 8:
+        return s->select[index];
+        break;
+    case SUNXI_PIC_ENABLE ... SUNXI_PIC_ENABLE + 8:
+        return s->enable[index];
+        break;
+    case SUNXI_PIC_MASK ... SUNXI_PIC_MASK + 8:
+        return s->mask[index];
+        break;
+    default:
+        break;
+    }
+
+    return 0;
+}
+
+static void sunxi_pic_write(void *opaque, hwaddr offset, uint64_t value,
+                             unsigned size)
+{
+    SunxiPICState *s = opaque;
+    uint8_t index = (offset&  0xc)/4;
+
+    switch (offset) {
+    case SUNXI_PIC_VECTOR:
+        s->vector = value&  ~0x3;
+        break;
+    case SUNXI_PIC_BASE_ADDR:
+        s->base_addr = value&  ~0x3;
+    case SUNXI_PIC_PROTECT:
+        s->protect = value;
+        break;
+    case SUNXI_PIC_NMI:
+        s->nmi = value;
+        break;
+    case SUNXI_PIC_IRQ_PENDING ... SUNXI_PIC_IRQ_PENDING + 8:
+        s->irq_pending[index]&= ~value;
+        break;
+    case SUNXI_PIC_FIQ_PENDING ... SUNXI_PIC_FIQ_PENDING + 8:
+        s->fiq_pending[index]&= ~value;
+        break;
+    case SUNXI_PIC_SELECT ... SUNXI_PIC_SELECT + 8:
+        s->select[index] = value;
+        break;
+    case SUNXI_PIC_ENABLE ... SUNXI_PIC_ENABLE + 8:
+        s->enable[index] = value;
+        break;
+    case SUNXI_PIC_MASK ... SUNXI_PIC_MASK + 8:
+        s->mask[index] = value;
+        break;
+    default:
+        break;
+    }
+
+    sunxi_pic_update(s);
+}
+
+static const MemoryRegionOps sunxi_pic_ops = {
+    .read = sunxi_pic_read,
+    .write = sunxi_pic_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription vmstate_sunxi_pic = {
+    .name = "sunxi.pic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(vector, SunxiPICState),
+        VMSTATE_UINT32(base_addr, SunxiPICState),
+        VMSTATE_UINT32(protect, SunxiPICState),
+        VMSTATE_UINT32(nmi, SunxiPICState),
+ VMSTATE_UINT32_ARRAY(irq_pending, SunxiPICState, SUNXI_PIC_REG_IDX), + VMSTATE_UINT32_ARRAY(fiq_pending, SunxiPICState, SUNXI_PIC_REG_IDX), + VMSTATE_UINT32_ARRAY(enable, SunxiPICState, SUNXI_PIC_REG_IDX), + VMSTATE_UINT32_ARRAY(select, SunxiPICState, SUNXI_PIC_REG_IDX),
+        VMSTATE_UINT32_ARRAY(mask, SunxiPICState, SUNXI_PIC_REG_IDX),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void sunxi_pic_realize(DeviceState *ds, Error **errp)
+{
+    SunxiPICState *s = SUNXI_PIC(ds);
+    SysBusDevice *dev = SYS_BUS_DEVICE(ds);
+
+ qdev_init_gpio_in(DEVICE(dev), sunxi_pic_set_irq, SUNXI_PIC_INT_NR);
+     sysbus_init_irq(dev,&s->parent_irq);
+     sysbus_init_irq(dev,&s->parent_fiq);
+     memory_region_init_io(&s->iomem, OBJECT(s),&sunxi_pic_ops, s,
+                           "sunxi-pic", 0x400);
+     sysbus_init_mmio(dev,&s->iomem);
+}
+
+static void sunxi_pic_reset(DeviceState *d)
+{
+    SunxiPICState *s = SUNXI_PIC(d);
+    uint8_t i;
+
+    s->base_addr = 0;
+    s->protect = 0;
+    s->nmi = 0;
+    s->vector = 0;
+    for (i = 0; i<  SUNXI_PIC_REG_IDX; i++) {
+        s->irq_pending[i] = 0;
+        s->fiq_pending[i] = 0;
+        s->select[i] = 0;
+        s->enable[i] = 0;
+        s->mask[i] = 0;
+    }
+}
+
+static void sunxi_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = sunxi_pic_realize;
+    dc->reset = sunxi_pic_reset;
+    dc->desc = "sunxi pic";
+    dc->vmsd =&vmstate_sunxi_pic;
+ }
+
+static const TypeInfo sunxi_pic_info = {
+    .name = TYPE_SUNXI_PIC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(SunxiPICState),
+    .class_init = sunxi_pic_class_init,
+};
+
+static void sunxi_register_types(void)
+{
+    type_register_static(&sunxi_pic_info);
+}
+
+type_init(sunxi_register_types);
diff --git a/include/hw/intc/sunxi-pic.h b/include/hw/intc/sunxi-pic.h
new file mode 100644
index 0000000..d52b53c
--- /dev/null
+++ b/include/hw/intc/sunxi-pic.h
@@ -0,0 +1,20 @@
+#ifndef SUNXI_PIC_H
+#define SUNXI_PIC_H
+
+#define TYPE_SUNXI_PIC  "sunxi-pic"
+#define SUNXI_PIC(obj) OBJECT_CHECK(SunxiPICState, (obj), TYPE_SUNXI_PIC)
+
+#define SUNXI_PIC_VECTOR       0
+#define SUNXI_PIC_BASE_ADDR    4
+#define SUNXI_PIC_PROTECT      8
+#define SUNXI_PIC_NMI          0xc
+#define SUNXI_PIC_IRQ_PENDING  0x10
+#define SUNXI_PIC_FIQ_PENDING  0x20
+#define SUNXI_PIC_SELECT       0x30
+#define SUNXI_PIC_ENABLE       0x40
+#define SUNXI_PIC_MASK 0x50
+
+#define SUNXI_PIC_INT_NR       95
Is this constant or configurable? Should it perhaps be a static prop?

NO, it's constant
+#define SUNXI_PIC_REG_IDX      (SUNXI_PIC_INT_NR / 32 + 1)
+
DIV_ROUND_UP is probably the intention here.


right, will fix

thank you so much!

+#endif
--
1.7.2.5







reply via email to

[Prev in Thread] Current Thread [Next in Thread]