From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlVxG-0004cs-FQ for qemu-devel@nongnu.org; Tue, 26 Nov 2013 22:38:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlVxB-00010S-QE for qemu-devel@nongnu.org; Tue, 26 Nov 2013 22:38:22 -0500 Received: from [222.73.24.84] (port=57319 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlVxA-0000yo-HE for qemu-devel@nongnu.org; Tue, 26 Nov 2013 22:38:17 -0500 Message-ID: <529568BF.2070507@cn.fujitsu.com> Date: Wed, 27 Nov 2013 11:36:31 +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> <529465AA.5030107@cn.fujitsu.com> In-Reply-To: <529465AA.5030107@cn.fujitsu.com> 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" Li Guang wrote: > 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 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 >>> >>> >