From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlEhT-0004f0-I3 for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:13:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlEhO-0005CZ-TP for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:12:55 -0500 Received: from [222.73.24.84] (port=37070 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlEhN-0005BC-Jg for qemu-devel@nongnu.org; Tue, 26 Nov 2013 04:12:50 -0500 Message-ID: <529465AA.5030107@cn.fujitsu.com> Date: Tue, 26 Nov 2013 17:11:06 +0800 From: Li Guang MIME-Version: 1.0 References: <1385450531-3170-1-git-send-email-lig.fnst@cn.fujitsu.com> <1385450531-3170-3-git-send-email-lig.fnst@cn.fujitsu.com> In-Reply-To: Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1; format=flowed Subject: Re: [Qemu-devel] [PATCH v4 2/4] hw/intc: add sunxi interrupt controller device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" Peter Crosthwaite wrote: > On Tue, Nov 26, 2013 at 5:22 PM, liguang wrote: > >> Signed-off-by: liguang >> --- >> 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 >> + * >> + * 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 >> + } >> > 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 >> >> >> >