Re: [Qemu-devel] [PATCH 05/13] mxs/imx23: Add the interrupt collector

[Peter Maydell]

On 11 December 2013 13:56, Michel Pollet <address@hidden> wrote:
> Implements the interrupt collector IO block
>
> Signed-off-by: Michel Pollet <address@hidden>
> ---
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/mxs_icoll.c   | 200 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 201 insertions(+)
>  create mode 100644 hw/intc/mxs_icoll.c
>
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 47ac442..e934b3c 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_MXS) += mxs_icoll.o
> diff --git a/hw/intc/mxs_icoll.c b/hw/intc/mxs_icoll.c
> new file mode 100644
> index 0000000..a1fd7d9
> --- /dev/null
> +++ b/hw/intc/mxs_icoll.c
> @@ -0,0 +1,200 @@
> +/*
> + * mxs_icoll.c
> + *
> + * Copyright: Michel Pollet <address@hidden>
> + *
> + * QEMU Licence
> + */

I'm not going to keep making the same remarks about reset,
licence header, coding style, vmstate, but you can assume
they apply to all these patches.

> +
> +/*
> + * This block implements the interrupt collector of the mxs
> + * Currently no priority is handled, as linux doesn't use them anyway
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/arm/mxs.h"
> +
> +enum {
> +    ICOLL_VECTOR = 0,
> +    ICOLL_LEVELACK = 1,
> +    ICOLL_CTRL = 2,
> +    // 3, reserved?
> +    ICOLL_VBASE = 4,
> +    ICOLL_STAT = 7,
> +
> +    ICOLL_REG_MAX,
> +
> +    ICOLL_RAW0 = 0xa,
> +    ICOLL_RAW1,
> +    ICOLL_RAW2,
> +    ICOLL_RAW3,
> +
> +    ICOLL_INT0 = 0x12,
> +    ICOLL_INT127 = 0x91,
> +};
> +
> +typedef struct mxs_icoll_state {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    uint32_t   reg[ICOLL_REG_MAX];
> +
> +    uint32_t   raised[4];
> +    uint32_t   fiq[4];
> +    uint32_t   irq[4];
> +
> +    uint8_t    r[128];
> +
> +    qemu_irq parent_irq;
> +    qemu_irq parent_fiq;
> +} mxs_icoll_state;
> +
> +static void mxs_icoll_update(mxs_icoll_state *s)
> +{
> +    int fiq = 0, irq = 0;
> +    int i;
> +
> +    for (i = 0; i < 4; i++) {
> +        int id = ffs(s->raised[i]);
> +        int vector = (i * 32) + id - 1;
> +        if (s->raised[i] & s->fiq[i]) {
> +            fiq++;
> +            s->reg[ICOLL_STAT] = vector;
> +            break;
> +        }
> +        if (s->raised[i] & s->irq[i]) {
> +            irq++;
> +            s->reg[ICOLL_STAT] = vector;
> +            break;
> +        }
> +    }
> +    qemu_set_irq(s->parent_irq, irq != 0);
> +    qemu_set_irq(s->parent_fiq, fiq != 0);
> +}
> +
> +static void mxs_icoll_set_irq(void *opaque, int irq, int level)
> +{
> +    mxs_icoll_state *s = (mxs_icoll_state *) opaque;
> +    if (level)
> +        s->raised[(irq / 32)] |= 1 << (irq % 32);
> +    else
> +        s->raised[(irq / 32)] &= ~(1 << (irq % 32));

This if needs braces around both arms.

> +    mxs_icoll_update(s);
> +}
> +
> +static uint64_t mxs_icoll_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    mxs_icoll_state *s = (mxs_icoll_state *) opaque;
> +
> +    switch (offset >> 4) {
> +        case 0 ... ICOLL_REG_MAX:
> +            return s->reg[offset >> 4];
> +        case ICOLL_RAW0 ... ICOLL_RAW3:
> +            return s->raised[(offset >> 4) - ICOLL_RAW0];
> +        case ICOLL_INT0 ... ICOLL_INT127:
> +            return s->r[(offset >> 4) - ICOLL_INT0];
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: bad offset 0x%x\n", __func__, (int) offset);
> +            break;
> +    }
> +    return 0;
> +}
> +
> +static void mxs_icoll_write(
> +        void *opaque, hwaddr offset, uint64_t value, unsigned size)
> +{
> +    mxs_icoll_state *s = (mxs_icoll_state *) opaque;
> +    uint32_t irqval, irqi = 0;
> +    uint32_t * dst = NULL;
> +    uint32_t oldvalue = 0;
> +
> +    switch (offset >> 4) {
> +        case 0 ... ICOLL_REG_MAX:
> +            dst = s->reg + (offset >> 4);
> +            break;
> +        case ICOLL_INT0 ... ICOLL_INT127:
> +            irqi = (offset >> 4) - ICOLL_INT0;
> +            irqval = s->r[irqi];
> +            dst = &irqval;
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: bad offset 0x%x\n", __func__, (int) offset);
> +            break;
> +    }
> +    if (!dst) {
> +        return;
> +    }
> +    oldvalue = mxs_write(dst, offset, value, size);
> +
> +    switch (offset >> 4) {
> +        case ICOLL_CTRL:
> +            if ((oldvalue ^ s->r[ICOLL_CTRL]) == 0x80000000
> +                    && !(oldvalue & 0x80000000)) {
> +                //     printf("%s reseting, anding clockgate\n", __func__);
> +                s->r[ICOLL_CTRL] |= 0x40000000;
> +            }

I've seen this bit of magic in several patches now. It could
use explanation and maybe being factored out somehow?

> +            break;
> +        case ICOLL_LEVELACK:
> +            irqi = s->reg[ICOLL_STAT] & 0x7f;
> +            s->raised[(irqi / 32)] &= ~(1 << (irqi % 32));
> +            s->reg[ICOLL_STAT] = 0x7f;
> +            break;
> +        case ICOLL_INT0 ... ICOLL_INT127:
> +            s->r[irqi] = irqval & ~(0x40); // dont' set softirq bit
> +            if (irqval & 0x4) // ENABLE
> +                s->irq[irqi / 32] |= (1 << (irqi % 32));
> +            else
> +                s->irq[irqi / 32] &= ~(1 << (irqi % 32));
> +            if (irqval & 0x10) // ENFIQ
> +                s->fiq[irqi / 32] |= (1 << (irqi % 32));
> +            else
> +                s->fiq[irqi / 32] &= ~(1 << (irqi % 32));
> +            if (irqval & 0x8) // SOFTIRQ
> +                mxs_icoll_set_irq(s, irqi, 1);
> +            break;
> +    }
> +
> +    mxs_icoll_update(s);
> +}
> +
> +static const MemoryRegionOps mxs_icoll_ops = {
> +    .read = mxs_icoll_read,
> +    .write = mxs_icoll_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int mxs_icoll_init(SysBusDevice *dev)
> +{
> +    mxs_icoll_state *s = OBJECT_CHECK(mxs_icoll_state, dev, "mxs_icoll");
> +    DeviceState *qdev = DEVICE(dev);
> +
> +    qdev_init_gpio_in(qdev, mxs_icoll_set_irq, 128);
> +    sysbus_init_irq(dev, &s->parent_irq);
> +    sysbus_init_irq(dev, &s->parent_fiq);
> +    memory_region_init_io(&s->iomem, OBJECT(s), &mxs_icoll_ops, s,
> +            "mxs_icoll", 0x2000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    return 0;
> +}
> +
> +static void mxs_icoll_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    sdc->init = mxs_icoll_init;
> +}
> +
> +static TypeInfo icoll_info = {
> +    .name          = "mxs_icoll",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(mxs_icoll_state),
> +    .class_init    = mxs_icoll_class_init,
> +};
> +
> +static void mxs_icoll_register(void)
> +{
> +    type_register_static(&icoll_info);
> +}
> +
> +type_init(mxs_icoll_register)
> --
> 1.8.5.1
>
>

-- PMM