Re: [Qemu-devel] [PATCH v10 3/5] hw/intc: add allwinner A10 interrupt controller

[Li Guang <lig.fnst@cn.fujitsu.com>]

Peter Crosthwaite wrote:
> On Mon, Dec 9, 2013 at 10:10 AM, liguang<lig.fnst@cn.fujitsu.com>  wrote:
>    
>> Signed-off-by: liguang<lig.fnst@cn.fujitsu.com>
>> ---
>>   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<lig.fnst@cn.fujitsu.com>
>> + *
>> + * 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
>>
>>
>>      
>
>