Re: [Qemu-devel] [PATCH V2 3/4] imx.31 and KZM board support: interrupt controller

[Peter Maydell]

On 22 November 2011 04:34, Peter Chubb <address@hidden> wrote:
> Implement the FreeSCALE i.MX31 advanced vectored interrupt controller, at 
> least
> to the extent it is used by Linux 3.0.x
>
> Signed-off-by: Hans Jang <address@hidden>
> Signed-off-by: Adam Clench <address@hidden>
> Signed-off-by: Peter Chubb <address@hidden>
> ---
>  hw/imx_avic.c |  363 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 363 insertions(+)
>  create mode 100644 hw/imx_avic.c
>
> Index: qemu-working/hw/imx_avic.c
> ===================================================================
> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ qemu-working/hw/imx_avic.c  2011-11-22 14:47:10.706040936 +1100
> @@ -0,0 +1,363 @@
> +/*
> + * IMX31 Vectored Interrupt Controller
> + *
> + * Note this is NOT the PL192 provided by ARM, but
> + * a custom implementation by FreeScale.
> + *
> + * Copyright (c) 2008 OKL
> + * Written by Hans
> + *
> + * This code is licenced under the GPL version 2 or later.
> + *
> + * TODO: implement vectors and priorities.
> + */
> +
> +#include "hw.h"
> +#include "sysbus.h"
> +#include <string.h> /* ffsll */

ffsll is a glibc extension. Use ctz64() from host-utils.h
instead (but check that the edge case of no bits set is
handled the way you need, the semantics aren't identical.)

> +
> +#define DEBUG_INT 1
> +#undef DEBUG_INT /* comment out for debugging */
> +
> +#ifdef DEBUG_INT
> +#define DPRINTF(fmt, args...) \
> +do { printf("imx_int: " fmt , ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +/*
> + * Print a message at most ten times.
> + */
> +#define scream(fmt, args...) \
> +    do { \
> +        static int printable = 10;\
> +        if (printable--) { \
> +            fprintf(stderr, fmt, ##args); \
> +        } \
> +    } while (0)
> +
> +
> +#define IMX_INT_NUM_IRQS 64
> +
> +/* Interrupt Control Bits */
> +#define ABFLAG (1<<25)
> +#define ABFEN (1<<24)
> +#define NIDIS (1<<22) /* Normal Interrupt disable */
> +#define FIDIS (1<<21) /* Fast interrupt disable */
> +#define NIAD  (1<<20) /* Normal Interrupt Arbiter Rise ARM level */
> +#define FIAD  (1<<19) /* Fast Interrupt Arbiter Rise ARM level */
> +#define NM    (1<<18) /* Normal interrupt mode */
> +
> +
> +#define PRIO_PER_WORD (sizeof (uint32_t) * 8 / 4)
> +#define PRIO_WORDS (IMX_INT_NUM_IRQS/PRIO_PER_WORD)
> +
> +typedef struct {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    uint64_t pending;
> +    uint64_t enabled;
> +    uint64_t is_fiq;
> +    uint32_t intcntl;
> +    uint32_t intmask;
> +    qemu_irq irq;
> +    qemu_irq fiq;
> +    uint32_t prio[PRIO_WORDS]; /* Priorities are 4-bits each */
> +} imx_int_state;
> +
> +static const VMStateDescription vmstate_imx_avic = {
> +    .name = "imx-avic",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_UINT64(pending, imx_int_state),
> +        VMSTATE_UINT64(enabled, imx_int_state),
> +        VMSTATE_UINT64(is_fiq, imx_int_state),
> +        VMSTATE_UINT32(intcntl, imx_int_state),
> +        VMSTATE_UINT32(intmask, imx_int_state),
> +        VMSTATE_UINT32_ARRAY(prio, imx_int_state, PRIO_WORDS),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +
> +
> +static inline int imx_int_prio(imx_int_state *s, int irq)
> +{
> +    uint32_t word = irq / PRIO_PER_WORD;
> +    uint32_t part = 4 * (irq % PRIO_PER_WORD);
> +    return 0xf & (s->prio[word] >> part);
> +}
> +
> +static inline void imx_int_set_prio(imx_int_state *s, int irq, int prio)
> +{
> +    uint32_t word = irq / PRIO_PER_WORD;
> +    uint32_t part = 4 * (irq % PRIO_PER_WORD);
> +    uint32_t mask = ~(0xf << part);
> +    s->prio[word] &= mask;
> +    s->prio[word] |= prio << part;
> +}

We don't seem to ever call this function?

> +/* Update interrupts.  */
> +static void imx_int_update(imx_int_state *s)
> +{
> +    int i;
> +    uint64_t new = s->pending;
> +    uint64_t flags;
> +
> +    flags = new & s->enabled & s->is_fiq;
> +    qemu_set_irq(s->fiq, !!flags);
> +
> +    flags = new & s->enabled & ~s->is_fiq;
> +    if (!flags || ((s->intmask & 0x1f) == 0x1f)) {

This &0x1f is only needed because you're incorrectly allowing
the high bits of the register to get set (see below).

> +        qemu_set_irq(s->irq, !!flags);
> +        return;
> +    }
> +
> +    /* Take interrupt if  prio lower than the value of intmask */

stray space.

> +    for (i = 0; i < IMX_INT_NUM_IRQS; i++) {
> +        if (flags & (1UL << i)) {
> +            if (imx_int_prio(s, i) > s->intmask) {
> +                qemu_set_irq(s->irq, 1);
> +                return;
> +            }
> +        }
> +    }

Should we be deasserting the irq line if none of the pending
interrupts have sufficient priority?

> +
> +}
> +
> +static void imx_int_set_irq(void *opaque, int irq, int level)
> +{
> +    imx_int_state *s = (imx_int_state *)opaque;
> +
> +    if (level) {
> +        s->pending |= (1ULL << irq);
> +    } else {
> +        s->pending &= ~(1ULL << irq);
> +    }
> +
> +    imx_int_update(s);
> +}
> +
> +
> +static uint64_t imx_int_read(void *opaque,
> +                             target_phys_addr_t offset, unsigned size)
> +{
> +    imx_int_state *s = (imx_int_state *)opaque;
> +
> +
> +    DPRINTF("read(offset = 0x%x)\n", offset >> 2);
> +    switch (offset >> 2) {
> +    case 0: /* INTCNTL */
> +        return s->intcntl;
> +
> +    case 1: /* Normal Interrupt Mask Register, NIMASK */
> +        return s->intmask;
> +
> +    case 2: /* Interrupt Enable Number Register, INTENNUM */
> +    case 3: /* Interrupt Disable Number Register, INTDISNUM */
> +        return 0;
> +
> +    case 4: /* Interrupt Enabled Number Register High */
> +        return s->enabled >> 32;
> +
> +    case 5: /* Interrupt Enabled Number Register Low */
> +        return s->enabled & 0xffffffffULL;
> +
> +    case 6: /* Interrupt Type Register High */
> +        return s->is_fiq >> 32;
> +
> +    case 7: /* Interrupt Type Register Low */
> +        return s->is_fiq & 0xFFFFFFFFULL;
> +
> +    case 8: /* Normal Interrupt Priority Register 7 */
> +    case 9: /* Normal Interrupt Priority Register 6 */
> +    case 10:/* Normal Interrupt Priority Register 5 */
> +    case 11:/* Normal Interrupt Priority Register 4 */
> +    case 12:/* Normal Interrupt Priority Register 3 */
> +    case 13:/* Normal Interrupt Priority Register 2 */
> +    case 14:/* Normal Interrupt Priority Register 1 */
> +    case 15:/* Normal Interrupt Priority Register 0 */
> +        return s->prio[15-(offset>>2)];
> +
> +    case 16: /* Normal interrupt vector and status register */
> +    {
> +        uint64_t flags = s->pending & s->enabled & ~s->is_fiq;
> +        int i = ffsll(flags);
> +        if (i) {
> +            imx_int_set_irq(opaque, i-1, 0);
> +            return (i-1) << 16;
> +        }
> +        return 0xFFFFULL<<16;
> +    }
> +    case 17:/* Fast Interrupt vector and status register */
> +    {
> +        uint64_t flags = s->pending & s->enabled & s->is_fiq;
> +        int i = ffsll(flags);
> +        if (i) {
> +            imx_int_set_irq(opaque, i-1, 0);
> +            return (i-1) << 16;
> +        }
> +        return 0xFFFF<<16;

Why ULL in the previous cases and not this one?

> +    }
> +    case 18:/* Interrupt source register high */
> +        return s->pending >> 32;
> +
> +    case 19:/* Interrupt source register low */
> +        return s->pending & 0xFFFFFFFFULL;
> +
> +    case 20:/* Interrupt Force Register high */
> +    case 21:/* Interrupt Force Register low */
> +        return 0;
> +
> +    case 22:/* Normal Interrupt Pending Register High */
> +        return (s->pending & s->enabled & ~s->is_fiq) >> 32;
> +
> +    case 23:/* Normal Interrupt Pending Register Low */
> +        return (s->pending & s->enabled & ~s->is_fiq) & 0xFFFFFFFFULL;
> +
> +    case 24: /* Fast Interrupt Pending Register High  */
> +        return (s->pending & s->enabled & s->is_fiq) >> 32;
> +
> +    case 25: /* Fast Interrupt Pending Register Low  */
> +        return (s->pending & s->enabled & s->is_fiq) & 0xFFFFFFFFULL;
> +
> +    case 0x40:            /* AVIC vector 0, use for WFI WAR */
> +        return 0x4;
> +
> +    default:
> +        scream("imx_int_read: Bad offset 0x%x\n", (int)offset);
> +        return 0;
> +    }
> +}
> +
> +static void imx_int_write(void *opaque, target_phys_addr_t offset,
> +                          uint64_t val, unsigned size)
> +{
> +    imx_int_state *s = (imx_int_state *)opaque;
> +
> +    /* Vector Registers not yet supported */
> +    if (offset >= 0x100 && offset <= 0x2fc) {
> +        DPRINTF("imx_int_write to vector register %d\n",
> +                (offset - 0x100)>>2);
> +        return;
> +    }
> +
> +    DPRINTF("imx_int_write(0x%x) = %x\n",
> +            (unsigned int)offset>>2, (unsigned int)val);
> +    switch (offset >> 2) {
> +    case 0: /* Interrupt Control Register, INTCNTL */
> +        s->intcntl = val;
> +        break;
> +
> +    case 1: /* Normal Interrupt Mask Register, NIMASK */
> +        s->intmask = val;

The manual I have documents this register as only having five
significant bits, with the upper bits all being reads-as-zero,
writes-ignored. This implements them as being read-write.
(Please check the other registers to see if they have similar bugs.)

> +        break;
> +
> +    case 2: /* Interrupt Enable Number Register, INTENNUM */
> +        DPRINTF("enable(%d)\n", (int)val);
> +        s->enabled |= (1ULL << val);
> +        break;
> +
> +    case 3: /* Interrupt Disable Number Register, INTDISNUM */
> +        s->enabled &= ~(1ULL << val);
> +        DPRINTF("disabled(%d)\n", (int)val);
> +        break;
> +
> +    case 4: /* Interrupt Enable Number Register High */
> +        s->enabled = (s->enabled & 0xffffffffULL) | (val << 32);
> +        break;
> +
> +    case 5: /* Interrupt Enable Number Register Low */
> +        s->enabled = (s->enabled & 0xffffffff00000000ULL) | val;
> +        break;
> +
> +    case 6: /* Interrupt Type Register High */
> +        s->is_fiq = (s->is_fiq & 0xffffffffULL) | (val << 32);
> +        break;
> +
> +    case 7: /* Interrupt Type Register Low */
> +        s->is_fiq = (s->is_fiq & 0xffffffff00000000ULL) | val;
> +        break;
> +
> +    case 8: /* Normal Interrupt Priority Register 7 */
> +    case 9: /* Normal Interrupt Priority Register 6 */
> +    case 10:/* Normal Interrupt Priority Register 5 */
> +    case 11:/* Normal Interrupt Priority Register 4 */
> +    case 12:/* Normal Interrupt Priority Register 3 */
> +    case 13:/* Normal Interrupt Priority Register 2 */
> +    case 14:/* Normal Interrupt Priority Register 1 */
> +    case 15:/* Normal Interrupt Priority Register 0 */
> +        s->prio[15-(offset>>2)] = val;
> +        return;
> +
> +        /* Read-only registers, writes ignored */
> +    case 16:/* Normal Interrupt Vector and Status register */
> +    case 17:/* Fast Interrupt vector and status register */
> +    case 18:/* Interrupt source register high */
> +    case 19:/* Interrupt source register low */
> +        return;
> +
> +    case 20:/* Interrupt Force Register high */
> +        s->pending = (s->pending & 0xffffffffULL) | (val << 32);
> +        break;
> +
> +    case 21:/* Interrupt Force Register low */
> +        s->pending = (s->pending & 0xffffffff00000000ULL) | val;
> +        break;
> +
> +    case 22:/* Normal Interrupt Pending Register High */
> +    case 23:/* Normal Interrupt Pending Register Low */
> +    case 24: /* Fast Interrupt Pending Register High  */
> +    case 25: /* Fast Interrupt Pending Register Low  */
> +        return;
> +
> +    default:
> +        scream("imx_int_write: Bad offset %x\n", (int)offset);
> +    }
> +    imx_int_update(s);
> +}
> +
> +static const MemoryRegionOps imx_int_ops = {
> +    .read = imx_int_read,
> +    .write = imx_int_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx_int_reset(imx_int_state *s)
> +{
> +    s->intmask = 0x1f;
> +    s->enabled = 0;
> +}
> +
> +static int imx_int_init(SysBusDevice *dev)
> +{
> +    imx_int_state *s = FROM_SYSBUS(imx_int_state, dev);;
> +
> +    memory_region_init_io(&s->iomem, &imx_int_ops, s, "imx_int", 0x1000);
> +    sysbus_init_mmio_region(dev, &s->iomem);
> +
> +    qdev_init_gpio_in(&dev->qdev, imx_int_set_irq, IMX_INT_NUM_IRQS);
> +    sysbus_init_irq(dev, &s->irq);
> +    sysbus_init_irq(dev, &s->fiq);
> +
> +    imx_int_reset(s);
> +
> +    vmstate_register(&dev->qdev, -1, &vmstate_imx_avic, s);

Use a SysBusDeviceInfo struct and set .qdev.vmsd.

> +    return 0;
> +}
> +
> +static void imx_int_register_devices(void)
> +{
> +    SysBusDeviceInfo *info = g_malloc0(sizeof *info);
> +    info->qdev.name = "imx_int";
> +    info->qdev.desc = "i.MX Advanced Vector Interrupt Controller";
> +    info->qdev.size = sizeof(imx_int_state);
> +    info->init = imx_int_init;

No, this should be a static struct. See hw/pl190.c for a random example.

> +    sysbus_register_withprop(info);
> +}
> +
> +device_init(imx_int_register_devices)

-- PMM