From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34106) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vqdrg-0001qe-M6 for qemu-devel@nongnu.org; Wed, 11 Dec 2013 02:05:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vqdrc-0000ib-0j for qemu-devel@nongnu.org; Wed, 11 Dec 2013 02:05:48 -0500 Received: from [222.73.24.84] (port=51809 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vqdra-0000hd-PA for qemu-devel@nongnu.org; Wed, 11 Dec 2013 02:05:43 -0500 Message-ID: <52A80E55.8010500@cn.fujitsu.com> Date: Wed, 11 Dec 2013 15:03:49 +0800 From: Li Guang MIME-Version: 1.0 References: <1386547806-22298-1-git-send-email-lig.fnst@cn.fujitsu.com> <1386547806-22298-4-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 v10 3/5] hw/intc: add allwinner A10 interrupt controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" , =?ISO-8859-1?Q?Andreas_F=E4rber?= Peter Crosthwaite wrote: > On Mon, Dec 9, 2013 at 10:10 AM, liguang wrote: > >> Signed-off-by: liguang >> --- >> default-configs/arm-softmmu.mak | 1 + >> hw/intc/Makefile.objs | 1 + >> hw/intc/allwinner-a10-pic.c | 217 +++++++++++++++++++++++++++++++++++ >> include/hw/intc/allwinner-a10-pic.h | 40 +++++++ >> 4 files changed, 259 insertions(+), 0 deletions(-) >> create mode 100644 hw/intc/allwinner-a10-pic.c >> create mode 100644 include/hw/intc/allwinner-a10-pic.h >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index 7858abf..e965068 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_ALLWINNER_A10_PIT=y >> +CONFIG_ALLWINNER_A10_PIC=y >> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs >> index 47ac442..60eb936 100644 >> --- a/hw/intc/Makefile.objs >> +++ b/hw/intc/Makefile.objs >> @@ -24,3 +24,4 @@ obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o >> obj-$(CONFIG_SH4) += sh_intc.o >> obj-$(CONFIG_XICS) += xics.o >> obj-$(CONFIG_XICS_KVM) += xics_kvm.o >> +obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o >> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c >> new file mode 100644 >> index 0000000..9345741 >> --- /dev/null >> +++ b/hw/intc/allwinner-a10-pic.c >> @@ -0,0 +1,217 @@ >> +/* >> + * Allwinner A10 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/allwinner-a10-pic.h" >> + >> +static void aw_a10_pic_update(AwA10PICState *s) >> +{ >> + uint8_t i, j; >> + bool irq = false, fiq = false; >> + >> + for (i = 0, j = 0; i< AW_A10_PIC_REG_NUM; i++) { >> + if (s->irq_pending[i] == 0&& s->fiq_pending[i] == 0) { >> + continue; >> + } >> + for (j = 0; j< 32; j++) { >> + if (test_bit(j, (void *)&s->mask[i])) { >> + continue; >> + } >> + if (test_bit(j, (void *)&s->irq_pending[i])) { >> + irq = true; >> + } >> + if (test_bit(j, (void *)&s->fiq_pending[i])&& >> + test_bit(j, (void *)&s->select[i])) { >> + fiq = true; >> + } >> + if (irq&& fiq) { >> + goto out; >> + } >> + } >> > So looking at this more closely, why not just: > > irq ||= s->mask[i]& s->irq_pending[i]; > fiq ||= s->mask[i]& s->fiq_pending[i]& s->select[i]; > > Instead of this inner for loop? Drop the fast path if above it as well. > > I remember getting rid of the word-wise& | logic in an earlier > version because of cross-over between the three regs. But thats now > resolved here with the outer loop. This inner loop should be able to > be done with word-wise& | logic. > > fine, fixed like this: diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c index 9345741..49a0fd1 100644 --- a/hw/intc/allwinner-a10-pic.c +++ b/hw/intc/allwinner-a10-pic.c @@ -22,31 +22,14 @@ static void aw_a10_pic_update(AwA10PICState *s) { - uint8_t i, j; - bool irq = false, fiq = false; - - for (i = 0, j = 0; i < AW_A10_PIC_REG_NUM; i++) { - if (s->irq_pending[i] == 0 && s->fiq_pending[i] == 0) { - continue; - } - for (j = 0; j < 32; j++) { - if (test_bit(j, (void *)&s->mask[i])) { - continue; - } - if (test_bit(j, (void *)&s->irq_pending[i])) { - irq = true; - } - if (test_bit(j, (void *)&s->fiq_pending[i]) && - test_bit(j, (void *)&s->select[i])) { - fiq = true; - } - if (irq && fiq) { - goto out; - } - } + uint8_t i; + int irq = 0, fiq = 0; + + for (i = 0; i < AW_A10_PIC_REG_NUM; i++) { + irq |= s->irq_pending[i] & ~s->mask[i]; + fiq |= s->select[i] & s->irq_pending[i] & ~s->mask[i]; } -out: qemu_set_irq(s->parent_irq, irq); qemu_set_irq(s->parent_fiq, fiq); Thanks! >> + } >> + >> +out: >> + qemu_set_irq(s->parent_irq, irq); >> + qemu_set_irq(s->parent_fiq, fiq); >> +} >> + >> +static void aw_a10_pic_set_irq(void *opaque, int irq, int level) >> +{ >> + AwA10PICState *s = opaque; >> + >> + if (level) { >> + set_bit(irq%32, (void *)&s->irq_pending[irq/32]); >> + } >> + aw_a10_pic_update(s); >> +} >> + >> +static uint64_t aw_a10_pic_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + AwA10PICState *s = opaque; >> + uint8_t index = (offset& 0xc)/4; >> + >> + switch (offset) { >> + case AW_A10_PIC_VECTOR: >> + return s->vector; >> + case AW_A10_PIC_BASE_ADDR: >> + return s->base_addr; >> + case AW_A10_PIC_PROTECT: >> + return s->protect; >> + case AW_A10_PIC_NMI: >> + return s->nmi; >> + case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8: >> + return s->irq_pending[index]; >> + case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8: >> + return s->fiq_pending[index]; >> + case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8: >> + return s->select[index]; >> + case AW_A10_PIC_ENABLE ... AW_A10_PIC_ENABLE + 8: >> + return s->enable[index]; >> + case AW_A10_PIC_MASK ... AW_A10_PIC_MASK + 8: >> + return s->mask[index]; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + >> + return 0; >> +} >> + >> +static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value, >> + unsigned size) >> +{ >> + AwA10PICState *s = opaque; >> + uint8_t index = (offset& 0xc)/4; >> + >> + switch (offset) { >> + case AW_A10_PIC_VECTOR: >> + s->vector = value& ~0x3; >> + break; >> + case AW_A10_PIC_BASE_ADDR: >> + s->base_addr = value& ~0x3; >> + case AW_A10_PIC_PROTECT: >> + s->protect = value; >> + break; >> + case AW_A10_PIC_NMI: >> + s->nmi = value; >> + break; >> + case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8: >> + s->irq_pending[index]&= ~value; >> + break; >> + case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8: >> + s->fiq_pending[index]&= ~value; >> + break; >> + case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8: >> + s->select[index] = value; >> + break; >> + case AW_A10_PIC_ENABLE ... AW_A10_PIC_ENABLE + 8: >> + s->enable[index] = value; >> + break; >> + case AW_A10_PIC_MASK ... AW_A10_PIC_MASK + 8: >> + s->mask[index] = value; >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Bad offset 0x%x\n", __func__, (int)offset); >> + break; >> + } >> + >> + aw_a10_pic_update(s); >> +} >> + >> +static const MemoryRegionOps aw_a10_pic_ops = { >> + .read = aw_a10_pic_read, >> + .write = aw_a10_pic_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> +}; >> + >> +static const VMStateDescription vmstate_aw_a10_pic = { >> + .name = "a10.pic", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .minimum_version_id_old = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT32(vector, AwA10PICState), >> + VMSTATE_UINT32(base_addr, AwA10PICState), >> + VMSTATE_UINT32(protect, AwA10PICState), >> + VMSTATE_UINT32(nmi, AwA10PICState), >> + VMSTATE_UINT32_ARRAY(irq_pending, AwA10PICState, AW_A10_PIC_REG_NUM), >> + VMSTATE_UINT32_ARRAY(fiq_pending, AwA10PICState, AW_A10_PIC_REG_NUM), >> + VMSTATE_UINT32_ARRAY(enable, AwA10PICState, AW_A10_PIC_REG_NUM), >> + VMSTATE_UINT32_ARRAY(select, AwA10PICState, AW_A10_PIC_REG_NUM), >> + VMSTATE_UINT32_ARRAY(mask, AwA10PICState, AW_A10_PIC_REG_NUM), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static void aw_a10_pic_realize(DeviceState *ds, Error **errp) >> +{ >> + AwA10PICState *s = AW_A10_PIC(ds); >> + SysBusDevice *dev = SYS_BUS_DEVICE(ds); >> + >> + qdev_init_gpio_in(DEVICE(dev), aw_a10_pic_set_irq, AW_A10_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),&aw_a10_pic_ops, s, >> + TYPE_AW_A10_PIC, 0x400); >> + sysbus_init_mmio(dev,&s->iomem); >> +} >> > Same comment as for the timer patch re init conversion. > > Regards, > Peter > > >> + >> +static void aw_a10_pic_reset(DeviceState *d) >> +{ >> + AwA10PICState *s = AW_A10_PIC(d); >> + uint8_t i; >> + >> + s->base_addr = 0; >> + s->protect = 0; >> + s->nmi = 0; >> + s->vector = 0; >> + for (i = 0; i< AW_A10_PIC_REG_NUM; 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 aw_a10_pic_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + dc->realize = aw_a10_pic_realize; >> + dc->reset = aw_a10_pic_reset; >> + dc->desc = "allwinner a10 pic"; >> + dc->vmsd =&vmstate_aw_a10_pic; >> + } >> + >> +static const TypeInfo aw_a10_pic_info = { >> + .name = TYPE_AW_A10_PIC, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .instance_size = sizeof(AwA10PICState), >> + .class_init = aw_a10_pic_class_init, >> +}; >> + >> +static void aw_a10_register_types(void) >> +{ >> + type_register_static(&aw_a10_pic_info); >> +} >> + >> +type_init(aw_a10_register_types); >> diff --git a/include/hw/intc/allwinner-a10-pic.h b/include/hw/intc/allwinner-a10-pic.h >> new file mode 100644 >> index 0000000..5721b2e >> --- /dev/null >> +++ b/include/hw/intc/allwinner-a10-pic.h >> @@ -0,0 +1,40 @@ >> +#ifndef AW_A10_PIC_H >> +#define AW_A10_PIC_H >> + >> +#define TYPE_AW_A10_PIC "allwinner-a10-pic" >> +#define AW_A10_PIC(obj) OBJECT_CHECK(AwA10PICState, (obj), TYPE_AW_A10_PIC) >> + >> +#define AW_A10_PIC_VECTOR 0 >> +#define AW_A10_PIC_BASE_ADDR 4 >> +#define AW_A10_PIC_PROTECT 8 >> +#define AW_A10_PIC_NMI 0xc >> +#define AW_A10_PIC_IRQ_PENDING 0x10 >> +#define AW_A10_PIC_FIQ_PENDING 0x20 >> +#define AW_A10_PIC_SELECT 0x30 >> +#define AW_A10_PIC_ENABLE 0x40 >> +#define AW_A10_PIC_MASK 0x50 >> + >> +#define AW_A10_PIC_INT_NR 95 >> +#define AW_A10_PIC_REG_NUM DIV_ROUND_UP(AW_A10_PIC_INT_NR, 32) >> + >> +typedef struct AwA10PICState { >> + /*< 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_pending[AW_A10_PIC_REG_NUM]; >> + uint32_t fiq_pending[AW_A10_PIC_REG_NUM]; >> + uint32_t select[AW_A10_PIC_REG_NUM]; >> + uint32_t enable[AW_A10_PIC_REG_NUM]; >> + uint32_t mask[AW_A10_PIC_REG_NUM]; >> + /*priority setting here*/ >> +} AwA10PICState; >> + >> +#endif >> -- >> 1.7.2.5 >> >> >> > >