From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vl6oV-0004np-JW for qemu-devel@nongnu.org; Mon, 25 Nov 2013 19:47:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vl6oQ-0004oB-SP for qemu-devel@nongnu.org; Mon, 25 Nov 2013 19:47:39 -0500 Received: from [222.73.24.84] (port=8610 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vl6oP-0004nr-CS for qemu-devel@nongnu.org; Mon, 25 Nov 2013 19:47:34 -0500 Message-ID: <5293EF39.5080605@cn.fujitsu.com> Date: Tue, 26 Nov 2013 08:45:45 +0800 From: Li Guang MIME-Version: 1.0 References: <1385365306-2189-1-git-send-email-lig.fnst@cn.fujitsu.com> <1385365306-2189-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 v3 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 Mon, Nov 25, 2013 at 5:41 PM, liguang wrote: > >> Signed-off-by: liguang >> --- >> default-configs/arm-softmmu.mak | 1 + >> hw/intc/Makefile.objs | 1 + >> hw/intc/sunxi-pic.c | 301 +++++++++++++++++++++++++++++++++++++++ >> include/hw/intc/sunxi-pic.h | 27 ++++ >> 4 files changed, 330 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..09a3d09 >> --- /dev/null >> +++ b/hw/intc/sunxi-pic.c >> @@ -0,0 +1,301 @@ >> +/* >> + * 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; >> + uint32_t vector; >> + uint32_t base_addr; >> + uint32_t protect; >> + uint32_t nmi; >> + uint32_t irq_pending0; >> + uint32_t irq_pending1; >> + uint32_t irq_pending2; >> + uint32_t select0; >> + uint32_t select1; >> + uint32_t select2; >> + uint32_t enable0; >> + uint32_t enable1; >> + uint32_t enable2; >> + uint32_t mask0; >> + uint32_t mask1; >> + uint32_t mask2; >> + /*priority setting here*/ >> +} SunxiPICState; >> + >> +static void sunxi_pic_update(SunxiPICState *s) >> +{ >> + uint32_t flags = 0; >> > initializer to 0 not needed. > > >> + >> + flags = s->irq_pending0 | s->irq_pending1 | s->irq_pending0; >> + qemu_set_irq(s->parent_irq, flags != 0); >> + flags&= s->select0 | s->select1 | s->select2; >> + qemu_set_irq(s->parent_fiq, flags != 0); >> > This logic seems strange to me, in that that behavior of individual > interrupts is not self contained. > > e.g. if interrupt 0 (bit 0 of irq_pending0) is pending and interrupt > 32 is selected (bit 0 of select1) then flags is non-zero regardless of > state of bit 0 of s->select0). Whats the intended behavior of the > select bits? > > If an interrupt is "selected" it will also route to both IRQ and FIQ it seems. > > >> +} >> + >> +static void sunxi_pic_set_irq(void *opaque, int irq, int level) >> +{ >> + SunxiPICState *s = opaque; >> + bool allow_irq = false; >> + >> + if (level) { >> + if (irq< 32) { >> + set_bit(irq, (void *)&s->irq_pending0); >> + if (test_bit(irq, (void *)&s->enable0)&& >> + !test_bit(irq, (void *)&s->mask0)) { >> + allow_irq = true; >> > this check inhibits immediate interrupt generation of masked intr, but > it doesnt stop a future event from triggering the IRQ. Should the > enable and mask checks be in the interrupt generation code? > > For example, your bus write handler calls sunxi_pic_update() which > means these pending-but-masked interrupts will potentially cause the > irq to raise at bus write time. > > >> + } >> + } else if (irq< 64) { >> + irq -= 32; >> + set_bit(irq, (void *)&s->irq_pending1); >> + if (test_bit(irq, (void *)&s->enable1)&& >> + !test_bit(irq, (void *)&s->mask1)) { >> + allow_irq = true; >> + } >> + } else if (irq< 95) { >> + irq -= 64; >> + set_bit(irq, (void *)&s->irq_pending2); >> + if (test_bit(irq, (void *)&s->enable2)&& >> + !test_bit(irq, (void *)&s->mask2)) { >> + allow_irq = true; >> + } >> + } >> + } else { >> + if (irq< 32) { >> + clear_bit(irq, (void *)&s->irq_pending0); >> + } else if (irq< 64) { >> + irq -= 32; >> + clear_bit(irq, (void *)&s->irq_pending1); >> + } else if (irq< 95) { >> + irq -= 64; >> + clear_bit(irq, (void *)&s->irq_pending2); >> + } >> + } >> + if (allow_irq) { >> + sunxi_pic_update(s); >> + } >> +} >> + >> +static uint64_t sunxi_pic_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + SunxiPICState *s = opaque; >> + >> + switch (offset) { >> + case PIC_VECTOR: >> + return s->vector; >> + break; >> + case PIC_BASE_ADDR: >> + return s->base_addr; >> + break; >> + case PIC_PROTECT: >> + return s->protect; >> + break; >> + case PIC_NMI: >> + return s->nmi; >> + break; >> + case PIC_IRQ_PENDING0: >> + case PIC_FIQ_PENDING0: >> + return s->irq_pending0; >> + break; >> + case PIC_IRQ_PENDING1: >> + case PIC_FIQ_PENDING1: >> + return s->irq_pending1; >> + break; >> + case PIC_IRQ_PENDING2: >> + case PIC_FIQ_PENDING2: >> + return s->irq_pending2; >> + break; >> + case PIC_SELECT0: >> + return s->select0; >> + break; >> + case PIC_SELECT1: >> + return s->select1; >> + break; >> + case PIC_SELECT2: >> + return s->select2; >> + break; >> + case PIC_ENABLE0: >> + return s->enable0; >> + break; >> + case PIC_ENABLE1: >> + return s->enable1; >> + break; >> + case PIC_ENABLE2: >> + return s->enable2; >> + break; >> + case PIC_MASK0: >> + return s->mask0; >> + break; >> + case PIC_MASK1: >> + return s->mask1; >> + break; >> + case PIC_MASK2: >> + return s->mask2; >> + 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 = 0; >> + >> + switch (offset) { >> + case PIC_VECTOR: >> + s->vector = value& ~0x3; >> + break; >> + case PIC_BASE_ADDR: >> + s->base_addr = value& ~0x3; >> + case PIC_PROTECT: >> + case PIC_NMI: >> + break; >> + case PIC_IRQ_PENDING0: >> + case PIC_FIQ_PENDING0: >> + for (index = 0; index< 32; index++) { >> + if (test_bit(index, (void *)&value)) { >> + clear_bit(index, (void *)&s->irq_pending0); >> + } >> > &= ~foo to implement write to clear. > > >> + } >> + break; >> + case PIC_IRQ_PENDING1: >> + case PIC_FIQ_PENDING1: >> + for (index = 0; index< 32; index++) { >> + if (test_bit(index, (void *)&value)) { >> + clear_bit(index, (void *)&s->irq_pending1); >> + } >> + } >> + break; >> + case PIC_IRQ_PENDING2: >> + case PIC_FIQ_PENDING2: >> + for (index = 0; index< 32; index++) { >> + if (test_bit(index, (void *)&value)) { >> + clear_bit(index, (void *)&s->irq_pending2); >> + } >> + } >> + break; >> + case PIC_SELECT0: >> + s->select0 = value; >> + break; >> + case PIC_SELECT1: >> + s->select1 = value; >> + break; >> + case PIC_SELECT2: >> + s->select2 = value; >> + break; >> + case PIC_ENABLE0: >> + s->enable0 = value; >> + break; >> + case PIC_ENABLE1: >> + s->enable1 = value; >> + break; >> + case PIC_ENABLE2: >> + s->enable2 = value; >> + break; >> + case PIC_MASK0: >> + s->mask0 = value; >> + break; >> + case PIC_MASK1: >> + s->mask1 = value; >> + break; >> + case PIC_MASK2: >> + s->mask2 = value; >> > You can make this much shorter by making the reigsters an array, and > indexing into it using offset for the default case. > > >> + 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 int sunxi_pic_init(SysBusDevice *dev) >> +{ >> + SunxiPICState *s = SUNXI_PIC(dev); >> + >> + qdev_init_gpio_in(DEVICE(dev), sunxi_pic_set_irq, 95); >> + 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); >> + >> + return 0; >> +} >> + >> +static void sunxi_pic_reset(DeviceState *d) >> +{ >> + SunxiPICState *s = SUNXI_PIC(d); >> + >> + s->base_addr = 0; >> + s->protect = 0; >> + s->nmi = 0; >> + s->vector = 0; >> + s->irq_pending0 = 0; >> + s->irq_pending1 = 0; >> + s->irq_pending2 = 0; >> + s->select0 = 0; >> + s->select1 = 0; >> + s->select2 = 0; >> + s->enable0 = 0; >> + s->enable1 = 0; >> + s->enable2 = 0; >> + s->mask0 = 0; >> + s->mask1 = 0; >> + s->mask2 = 0; >> +} >> + >> +static void sunxi_pic_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); >> + >> + k->init = sunxi_pic_init; >> > Use of SysBusDevice::init is depracated. Please use Device::realize or > the object init instead. With no properties deps, I think the latter > is more correct. > > >> + dc->reset = sunxi_pic_reset; >> > missing VMSD support. > > >> + } >> + >> +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..fd07b33 >> --- /dev/null >> +++ b/include/hw/intc/sunxi-pic.h >> @@ -0,0 +1,27 @@ >> +#ifndef SUNXI_PIC_H >> +#define SUNXI_PIC_H >> + >> +#define TYPE_SUNXI_PIC "sunxi_PIC" >> > I think -'s are encouraged over _'s. The casing should be made > consistent as well. > > All are reasonable, will fix. Thanks a lot! > >> +#define SUNXI_PIC(obj) OBJECT_CHECK(SunxiPICState, (obj), TYPE_SUNXI_PIC) >> + >> +#define PIC_VECTOR 0 >> +#define PIC_BASE_ADDR 4 >> +#define PIC_PROTECT 8 >> +#define PIC_NMI 0xc >> +#define PIC_IRQ_PENDING0 0x10 >> +#define PIC_IRQ_PENDING1 0x14 >> +#define PIC_IRQ_PENDING2 0x18 >> +#define PIC_FIQ_PENDING0 0x20 >> +#define PIC_FIQ_PENDING1 0x24 >> +#define PIC_FIQ_PENDING2 0x28 >> +#define PIC_SELECT0 0x30 >> +#define PIC_SELECT1 0x34 >> +#define PIC_SELECT2 0x38 >> +#define PIC_ENABLE0 0x40 >> +#define PIC_ENABLE1 0x44 >> +#define PIC_ENABLE2 0x48 >> +#define PIC_MASK0 0x50 >> +#define PIC_MASK1 0x54 >> +#define PIC_MASK2 0x58 >> + >> +#endif >> -- >> 1.7.2.5 >> >> >> >