[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/4] hw/intc: add sunxi interrupt controller
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 2/4] hw/intc: add sunxi interrupt controller device |
|
Date: |
Mon, 25 Nov 2013 20:42:51 +1000 |
On Mon, Nov 25, 2013 at 5:41 PM, liguang <address@hidden> wrote:
> Signed-off-by: liguang <address@hidden>
> ---
> 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 <address@hidden>
> + *
> + * 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.
Regards,
Peter
> +#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
>
>