Patchwork [v5,03/24] hw/arm: add Faraday FTINTC020 interrupt controller support

login
register
mail settings
Submitter Kuo-Jung Su
Date Feb. 27, 2013, 7:15 a.m.
Message ID <1361949350-22241-4-git-send-email-dantesu@gmail.com>
Download mbox | patch
Permalink /patch/223524/
State New
Headers show

Comments

Kuo-Jung Su - Feb. 27, 2013, 7:15 a.m.
From: Kuo-Jung Su <dantesu@faraday-tech.com>

The FTINTC020 interrupt controller supports both FIQ and IRQ signals
to the microprocessor.
It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
The output signals to the microprocessor can be configured as
level-high/low active or edge-rising/falling triggered.

Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
---
 hw/arm/Makefile.objs      |    1 +
 hw/arm/faraday.h          |    3 +
 hw/arm/faraday_a369_soc.c |   10 +-
 hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/ftintc020.h        |   48 ++++++
 5 files changed, 425 insertions(+), 3 deletions(-)
 create mode 100644 hw/arm/ftintc020.c
 create mode 100644 hw/arm/ftintc020.h
Peter Crosthwaite - March 2, 2013, 4:13 a.m.
Hi Kuo-Jung,

On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>
> The FTINTC020 interrupt controller supports both FIQ and IRQ signals
> to the microprocessor.
> It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
> The output signals to the microprocessor can be configured as
> level-high/low active or edge-rising/falling triggered.
>
> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
> ---
>  hw/arm/Makefile.objs      |    1 +
>  hw/arm/faraday.h          |    3 +
>  hw/arm/faraday_a369_soc.c |   10 +-
>  hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/ftintc020.h        |   48 ++++++
>  5 files changed, 425 insertions(+), 3 deletions(-)
>  create mode 100644 hw/arm/ftintc020.c
>  create mode 100644 hw/arm/ftintc020.h
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index f6fd60d..6771072 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -37,3 +37,4 @@ obj-y += faraday_a369.o \
>              faraday_a369_soc.o \
>              faraday_a369_scu.o \
>              faraday_a369_kpd.o
> +obj-y += ftintc020.o
> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
> index d6ed860..e5f611d 100644
> --- a/hw/arm/faraday.h
> +++ b/hw/arm/faraday.h
> @@ -62,4 +62,7 @@ typedef struct FaradaySoCState {
>      FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
>                                                TYPE_FARADAY_SOC))
>
> +/* ftintc020.c */
> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
> +
>  #endif
> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
> index 0372868..3d861d2 100644
> --- a/hw/arm/faraday_a369_soc.c
> +++ b/hw/arm/faraday_a369_soc.c
> @@ -73,6 +73,7 @@ a369soc_device_init(FaradaySoCState *s)
>  {
>      DriveInfo *dinfo;
>      DeviceState *ds;
> +    qemu_irq *pic;
>
>      s->as = get_system_memory();
>      s->ram = g_new(MemoryRegion, 1);
> @@ -115,12 +116,15 @@ a369soc_device_init(FaradaySoCState *s)
>          exit(1);
>      }
>
> +    /* Interrupt Controller */
> +    pic = ftintc020_init(0x90100000, s->cpu);
> +
>      /* Serial (FTUART010 which is 16550A compatible) */
>      if (serial_hds[0]) {
>          serial_mm_init(s->as,
>                         0x92b00000,
>                         2,
> -                       NULL,
> +                       pic[53],
>                         18432000,
>                         serial_hds[0],
>                         DEVICE_LITTLE_ENDIAN);
> @@ -129,7 +133,7 @@ a369soc_device_init(FaradaySoCState *s)
>          serial_mm_init(s->as,
>                         0x92c00000,
>                         2,
> -                       NULL,
> +                       pic[54],
>                         18432000,
>                         serial_hds[1],
>                         DEVICE_LITTLE_ENDIAN);
> @@ -140,7 +144,7 @@ a369soc_device_init(FaradaySoCState *s)
>      s->scu = ds;
>
>      /* ftkbc010 */
> -    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
> +    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
>  }
>
>  static int a369soc_init(SysBusDevice *busdev)
> diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
> new file mode 100644
> index 0000000..a7f6454
> --- /dev/null
> +++ b/hw/arm/ftintc020.c
> @@ -0,0 +1,366 @@
> +/*
> + * Faraday FTINTC020 Programmable Interrupt Controller.
> + *
> + * Copyright (c) 2012 Faraday Technology
> + * Written by Dante Su <dantesu@faraday-tech.com>
> + *
> + * This code is licensed under GNU GPL v2+.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +
> +#include "faraday.h"
> +#include "ftintc020.h"
> +
> +#define TYPE_FTINTC020  "ftintc020"
> +
> +typedef struct Ftintc020State {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    ARMCPU *cpu;
> +    qemu_irq irqs[64];
> +
> +    uint32_t irq_pin[2];    /* IRQ pin state */
> +    uint32_t fiq_pin[2];    /* IRQ pin state */
> +
> +    /* HW register caches */
> +    uint32_t irq_src[2];    /* IRQ source register */
> +    uint32_t irq_ena[2];    /* IRQ enable register */
> +    uint32_t irq_mod[2];    /* IRQ mode register */
> +    uint32_t irq_lvl[2];    /* IRQ level register */
> +    uint32_t fiq_src[2];    /* FIQ source register */
> +    uint32_t fiq_ena[2];    /* FIQ enable register */
> +    uint32_t fiq_mod[2];    /* FIQ mode register */
> +    uint32_t fiq_lvl[2];    /* FIQ level register */
> +} Ftintc020State;
> +
> +#define FTINTC020(obj) \
> +    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
> +
> +static void
> +ftintc020_update(Ftintc020State *s)
> +{
> +    uint32_t mask[2];
> +
> +    /* FIQ */
> +    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
> +    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
> +
> +    if (mask[0] || mask[1]) {
> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
> +    } else {
> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
> +    }
> +
> +    /* IRQ */
> +    mask[0] = s->irq_src[0] & s->irq_ena[0];
> +    mask[1] = s->irq_src[1] & s->irq_ena[1];
> +
> +    if (mask[0] || mask[1]) {
> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
> +    } else {
> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
> +    }
> +}
> +
> +/* Note: Here level means state of the signal on a pin */
> +static void
> +ftintc020_set_irq(void *opaque, int irq, int level)
> +{
> +    Ftintc020State *s = FTINTC020(opaque);
> +    uint32_t i = irq / 32;
> +    uint32_t mask = 1 << (irq % 32);
> +
> +    if (s->irq_mod[i] & mask) {
> +        /* Edge Triggered */
> +        if (s->irq_lvl[i] & mask) {
> +            /* Falling Active */
> +            if ((s->irq_pin[i] & mask) && !level) {
> +                s->irq_src[i] |=  mask;
> +                s->fiq_src[i] |=  mask;
> +            }
> +        } else {
> +            /* Rising Active */
> +            if (!(s->irq_pin[i] & mask) && level) {
> +                s->irq_src[i] |=  mask;
> +                s->fiq_src[i] |=  mask;
> +            }
> +        }
> +    } else {
> +        /* Level Triggered */
> +        if (s->irq_lvl[i] & mask) {
> +            /* Low Active */
> +            if (level) {
> +                s->irq_src[i] &= ~mask;
> +                s->fiq_src[i] &= ~mask;
> +            } else {
> +                s->irq_src[i] |=  mask;
> +                s->fiq_src[i] |=  mask;
> +            }
> +        } else {
> +            /* High Active */
> +            if (!level) {
> +                s->irq_src[i] &= ~mask;
> +                s->fiq_src[i] &= ~mask;
> +            } else {
> +                s->irq_src[i] |=  mask;
> +                s->fiq_src[i] |=  mask;
> +            }
> +        }
> +    }
> +
> +    /* update IRQ/FIQ pin states */
> +    if (level) {
> +        s->irq_pin[i] |= mask;
> +        s->fiq_pin[i] |= mask;
> +    } else {
> +        s->irq_pin[i] &= ~mask;
> +        s->fiq_pin[i] &= ~mask;
> +    }
> +
> +    ftintc020_update(s);
> +}
> +
> +static uint64_t
> +ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    Ftintc020State *s = FTINTC020(opaque);
> +
> +    switch (addr) {
> +    /*
> +     * IRQ
> +     */
> +    case REG_IRQSRC:
> +        return s->irq_src[0];
> +    case REG_IRQENA:
> +        return s->irq_ena[0];
> +    case REG_IRQMDR:
> +        return s->irq_mod[0];
> +    case REG_IRQLVR:
> +        return s->irq_lvl[0];
> +    case REG_IRQSR:
> +        return s->irq_src[0] & s->irq_ena[0];
> +    case REG_EIRQSRC:
> +        return s->irq_src[1];
> +    case REG_EIRQENA:
> +        return s->irq_ena[1];
> +    case REG_EIRQMDR:
> +        return s->irq_mod[1];
> +    case REG_EIRQLVR:
> +        return s->irq_lvl[1];
> +    case REG_EIRQSR:
> +        return s->irq_src[1] & s->irq_ena[1];
> +

AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
Can you #define some symbols accrordingly and remove all the magic
numberage with the [0]'s and [1]'s?

> +    /*
> +     * FIQ
> +     */
> +    case REG_FIQSRC:
> +        return s->fiq_src[0];
> +    case REG_FIQENA:
> +        return s->fiq_ena[0];
> +    case REG_FIQMDR:
> +        return s->fiq_mod[0];
> +    case REG_FIQLVR:
> +        return s->fiq_lvl[0];
> +    case REG_FIQSR:
> +        return s->fiq_src[0] & s->fiq_ena[0];
> +    case REG_EFIQSRC:
> +        return s->fiq_src[1];
> +    case REG_EFIQENA:
> +        return s->fiq_ena[1];
> +    case REG_EFIQMDR:
> +        return s->fiq_mod[1];
> +    case REG_EFIQLVR:
> +        return s->fiq_lvl[1];
> +    case REG_EFIQSR:
> +        return s->fiq_src[1] & s->fiq_ena[1];
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
> +        return 0;
> +    }
> +}
> +
> +static void
> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
> +{
> +    Ftintc020State *s = FTINTC020(opaque);
> +
> +    switch (addr) {
> +    /*
> +     * IRQ
> +     */

Ok to use one line comment. And elsewhere

> +    case REG_IRQENA:
> +        s->irq_ena[0] = (uint32_t)value;
> +        break;
> +    case REG_IRQSCR:
> +        value = ~(value & s->irq_mod[0]);
> +        s->irq_src[0] &= (uint32_t)value;
> +        break;
> +    case REG_IRQMDR:
> +        s->irq_mod[0] = (uint32_t)value;
> +        break;
> +    case REG_IRQLVR:
> +        s->irq_lvl[0] = (uint32_t)value;
> +        break;
> +    case REG_EIRQENA:
> +        s->irq_ena[1] = (uint32_t)value;
> +        break;
> +    case REG_EIRQSCR:
> +        value = ~(value & s->irq_mod[1]);
> +        s->irq_src[1] &= (uint32_t)value;
> +        break;
> +    case REG_EIRQMDR:
> +        s->irq_mod[1] = (uint32_t)value;
> +        break;
> +    case REG_EIRQLVR:
> +        s->irq_lvl[1] = (uint32_t)value;
> +        break;
> +
> +    /*
> +     * FIQ
> +     */
> +    case REG_FIQENA:
> +        s->fiq_ena[0] = (uint32_t)value;
> +        break;
> +    case REG_FIQSCR:
> +        value = ~(value & s->fiq_mod[0]);
> +        s->fiq_src[0] &= (uint32_t)value;
> +        break;
> +    case REG_FIQMDR:
> +        s->fiq_mod[0] = (uint32_t)value;
> +        break;
> +    case REG_FIQLVR:
> +        s->fiq_lvl[0] = (uint32_t)value;
> +        break;
> +    case REG_EFIQENA:
> +        s->fiq_ena[1] = (uint32_t)value;
> +        break;
> +    case REG_EFIQSCR:
> +        value = ~(value & s->fiq_mod[1]);
> +        s->fiq_src[1] &= (uint32_t)value;
> +        break;
> +    case REG_EFIQMDR:
> +        s->fiq_mod[1] = (uint32_t)value;
> +        break;
> +    case REG_EFIQLVR:
> +        s->fiq_lvl[1] = (uint32_t)value;
> +        break;
> +
> +    /* don't care */
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
> +        return;
> +    }
> +
> +    ftintc020_update(s);
> +}
> +
> +static const MemoryRegionOps mmio_ops = {
> +    .read  = ftintc020_mem_read,
> +    .write = ftintc020_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    }
> +};
> +
> +static void ftintc020_reset(DeviceState *ds)
> +{
> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
> +
> +    s->irq_pin[0] = 0;
> +    s->irq_pin[1] = 0;
> +    s->fiq_pin[0] = 0;
> +    s->fiq_pin[1] = 0;
> +
> +    s->irq_src[0] = 0;
> +    s->irq_src[1] = 0;
> +    s->irq_ena[0] = 0;
> +    s->irq_ena[1] = 0;
> +    s->fiq_src[0] = 0;
> +    s->fiq_src[1] = 0;
> +    s->fiq_ena[0] = 0;
> +    s->fiq_ena[1] = 0;
> +
> +    ftintc020_update(s);
> +}
> +
> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)

I'm not sure this is the right place for this, I think device creation
helpers belong on the machine / SoC level. Keep the device model self
contained and clients call qdev_init themselves. Andreas have the
rules change on this recently?

> +{
> +    int i;
> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);

As the device is intended for use in an SoC, the SoC potentially needs
to hold a pointer to it in order to call device destructors. This
function should return ds for future use by the instantiator.

> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
> +

Use an Object cast macro. Andreas, can we make this easier for
reviewers and developers by adding blacklisted identifiers to
checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?

> +    s->cpu = cpu;

Im thinking this is a QOM link. Its a connection from one device to
another and the machine should be aware of it.

> +    ftintc020_reset(ds);

Doesn't look right but this is just the already registered
DeviceClass::reset function anyways. You should just be able to delete
this. Does your system function without it?

> +    qdev_init_nofail(ds);

Cutting from here ...

> +    qdev_init_gpio_in(ds, ftintc020_set_irq, 64);
> +    for (i = 0; i < 64; ++i) {
> +        s->irqs[i] = qdev_get_gpio_in(ds, i);
> +    }
> +
> +    /* Enable IC memory-mapped registers access.  */
> +    memory_region_init_io(&s->iomem,
> +                          &mmio_ops,
> +                          s,
> +                          TYPE_FTINTC020,
> +                          0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(ds), &s->iomem);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(ds), 0, base);
> +

... Im pritty sure all of this belongs in either the ObjectClass::init
or Device::realize functions.

> +    return s->irqs;
> +}
> +
> +static VMStateDescription vmstate_ftintc020 = {
> +    .name = TYPE_FTINTC020,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(irq_src, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(irq_ena, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(irq_mod, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(irq_lvl, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(fiq_src, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(fiq_ena, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(fiq_mod, Ftintc020State, 2),
> +        VMSTATE_UINT32_ARRAY(fiq_lvl, Ftintc020State, 2),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static int ftintc020_initfn(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static void ftintc020_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    k->init     = ftintc020_initfn;

SysBusDevice::init is deprecated in favour of the Device::init. Your
SBD::init doesnt do anything so you can just delete it. But you should
add a device Init here to bring the sysbus foo from your helper
instantiator into the device model.

Regards,
Peter

> +    dc->vmsd    = &vmstate_ftintc020;
> +    dc->reset   = ftintc020_reset;
> +    dc->no_user = 1;
> +}
> +
> +static const TypeInfo ftintc020_info = {
> +    .name          = TYPE_FTINTC020,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Ftintc020State),
> +    .class_init    = ftintc020_class_init,
> +};
> +
> +static void ftintc020_register_types(void)
> +{
> +    type_register_static(&ftintc020_info);
> +}
> +
> +type_init(ftintc020_register_types)
> diff --git a/hw/arm/ftintc020.h b/hw/arm/ftintc020.h
> new file mode 100644
> index 0000000..f17fb58
> --- /dev/null
> +++ b/hw/arm/ftintc020.h
> @@ -0,0 +1,48 @@
> +/*
> + * Faraday FTINTC020 Programmable Interrupt Controller.
> + *
> + * Copyright (c) 2012 Faraday Technology
> + * Written by Dante Su <dantesu@faraday-tech.com>
> + *
> + * This code is licensed under GNU GPL v2+.
> + */
> +
> +#ifndef HW_ARM_FTINTC020_H
> +#define HW_ARM_FTINTC020_H
> +
> +/*
> + * IRQ/FIO: 0 ~ 31
> + */
> +#define REG_IRQSRC      0x00    /* IRQ source register */
> +#define REG_IRQENA      0x04    /* IRQ enable register */
> +#define REG_IRQSCR      0x08    /* IRQ status clear register */
> +#define REG_IRQMDR      0x0C    /* IRQ trigger mode register */
> +#define REG_IRQLVR      0x10    /* IRQ trigger level register */
> +#define REG_IRQSR       0x14    /* IRQ status register */
> +
> +#define REG_FIQSRC      0x20    /* FIQ source register */
> +#define REG_FIQENA      0x24    /* FIQ enable register */
> +#define REG_FIQSCR      0x28    /* FIQ status clear register */
> +#define REG_FIQMDR      0x2C    /* FIQ trigger mode register */
> +#define REG_FIQLVR      0x30    /* FIQ trigger level register */
> +#define REG_FIQSR       0x34    /* FIQ status register */
> +
> +/*
> + * Extended IRQ/FIO: 32 ~ 63
> + */
> +
> +#define REG_EIRQSRC      0x60   /* Extended IRQ source register */
> +#define REG_EIRQENA      0x64   /* Extended IRQ enable register */
> +#define REG_EIRQSCR      0x68   /* Extended IRQ status clear register */
> +#define REG_EIRQMDR      0x6C   /* Extended IRQ trigger mode register */
> +#define REG_EIRQLVR      0x70   /* Extended IRQ trigger level register */
> +#define REG_EIRQSR       0x74   /* Extended IRQ status register */
> +
> +#define REG_EFIQSRC      0x80   /* Extended FIQ source register */
> +#define REG_EFIQENA      0x84   /* Extended FIQ enable register */
> +#define REG_EFIQSCR      0x88   /* Extended FIQ status clear register */
> +#define REG_EFIQMDR      0x8C   /* Extended FIQ trigger mode register */
> +#define REG_EFIQLVR      0x90   /* Extended FIQ trigger level register */
> +#define REG_EFIQSR       0x94   /* Extended FIQ status register */
> +
> +#endif
> --
> 1.7.9.5
>
>
Peter Crosthwaite - March 3, 2013, 4:58 a.m.
Hi All,

On Sat, Mar 2, 2013 at 2:13 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Kuo-Jung,
>
> On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> The FTINTC020 interrupt controller supports both FIQ and IRQ signals
>> to the microprocessor.
>> It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
>> The output signals to the microprocessor can be configured as
>> level-high/low active or edge-rising/falling triggered.
>>
>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> ---
>>  hw/arm/Makefile.objs      |    1 +
>>  hw/arm/faraday.h          |    3 +
>>  hw/arm/faraday_a369_soc.c |   10 +-
>>  hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
>>  hw/arm/ftintc020.h        |   48 ++++++
>>  5 files changed, 425 insertions(+), 3 deletions(-)
>>  create mode 100644 hw/arm/ftintc020.c
>>  create mode 100644 hw/arm/ftintc020.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index f6fd60d..6771072 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -37,3 +37,4 @@ obj-y += faraday_a369.o \
>>              faraday_a369_soc.o \
>>              faraday_a369_scu.o \
>>              faraday_a369_kpd.o
>> +obj-y += ftintc020.o
>> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
>> index d6ed860..e5f611d 100644
>> --- a/hw/arm/faraday.h
>> +++ b/hw/arm/faraday.h
>> @@ -62,4 +62,7 @@ typedef struct FaradaySoCState {
>>      FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
>>                                                TYPE_FARADAY_SOC))
>>
>> +/* ftintc020.c */
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
>> +
>>  #endif
>> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
>> index 0372868..3d861d2 100644
>> --- a/hw/arm/faraday_a369_soc.c
>> +++ b/hw/arm/faraday_a369_soc.c
>> @@ -73,6 +73,7 @@ a369soc_device_init(FaradaySoCState *s)
>>  {
>>      DriveInfo *dinfo;
>>      DeviceState *ds;
>> +    qemu_irq *pic;
>>
>>      s->as = get_system_memory();
>>      s->ram = g_new(MemoryRegion, 1);
>> @@ -115,12 +116,15 @@ a369soc_device_init(FaradaySoCState *s)
>>          exit(1);
>>      }
>>
>> +    /* Interrupt Controller */
>> +    pic = ftintc020_init(0x90100000, s->cpu);
>> +
>>      /* Serial (FTUART010 which is 16550A compatible) */
>>      if (serial_hds[0]) {
>>          serial_mm_init(s->as,
>>                         0x92b00000,
>>                         2,
>> -                       NULL,
>> +                       pic[53],
>>                         18432000,
>>                         serial_hds[0],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -129,7 +133,7 @@ a369soc_device_init(FaradaySoCState *s)
>>          serial_mm_init(s->as,
>>                         0x92c00000,
>>                         2,
>> -                       NULL,
>> +                       pic[54],
>>                         18432000,
>>                         serial_hds[1],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -140,7 +144,7 @@ a369soc_device_init(FaradaySoCState *s)
>>      s->scu = ds;
>>
>>      /* ftkbc010 */
>> -    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
>> +    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
>>  }
>>
>>  static int a369soc_init(SysBusDevice *busdev)
>> diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
>> new file mode 100644
>> index 0000000..a7f6454
>> --- /dev/null
>> +++ b/hw/arm/ftintc020.c
>> @@ -0,0 +1,366 @@
>> +/*
>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>> + *
>> + * Copyright (c) 2012 Faraday Technology
>> + * Written by Dante Su <dantesu@faraday-tech.com>
>> + *
>> + * This code is licensed under GNU GPL v2+.
>> + */
>> +
>> +#include "hw/hw.h"
>> +#include "hw/sysbus.h"
>> +
>> +#include "faraday.h"
>> +#include "ftintc020.h"
>> +
>> +#define TYPE_FTINTC020  "ftintc020"
>> +
>> +typedef struct Ftintc020State {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    ARMCPU *cpu;
>> +    qemu_irq irqs[64];
>> +
>> +    uint32_t irq_pin[2];    /* IRQ pin state */
>> +    uint32_t fiq_pin[2];    /* IRQ pin state */
>> +
>> +    /* HW register caches */
>> +    uint32_t irq_src[2];    /* IRQ source register */
>> +    uint32_t irq_ena[2];    /* IRQ enable register */
>> +    uint32_t irq_mod[2];    /* IRQ mode register */
>> +    uint32_t irq_lvl[2];    /* IRQ level register */
>> +    uint32_t fiq_src[2];    /* FIQ source register */
>> +    uint32_t fiq_ena[2];    /* FIQ enable register */
>> +    uint32_t fiq_mod[2];    /* FIQ mode register */
>> +    uint32_t fiq_lvl[2];    /* FIQ level register */
>> +} Ftintc020State;
>> +
>> +#define FTINTC020(obj) \
>> +    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
>> +
>> +static void
>> +ftintc020_update(Ftintc020State *s)
>> +{
>> +    uint32_t mask[2];
>> +
>> +    /* FIQ */
>> +    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
>> +    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>> +    }
>> +
>> +    /* IRQ */
>> +    mask[0] = s->irq_src[0] & s->irq_ena[0];
>> +    mask[1] = s->irq_src[1] & s->irq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    }
>> +}
>> +
>> +/* Note: Here level means state of the signal on a pin */
>> +static void
>> +ftintc020_set_irq(void *opaque, int irq, int level)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +    uint32_t i = irq / 32;
>> +    uint32_t mask = 1 << (irq % 32);
>> +
>> +    if (s->irq_mod[i] & mask) {
>> +        /* Edge Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Falling Active */
>> +            if ((s->irq_pin[i] & mask) && !level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* Rising Active */
>> +            if (!(s->irq_pin[i] & mask) && level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    } else {
>> +        /* Level Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Low Active */
>> +            if (level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* High Active */
>> +            if (!level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* update IRQ/FIQ pin states */
>> +    if (level) {
>> +        s->irq_pin[i] |= mask;
>> +        s->fiq_pin[i] |= mask;
>> +    } else {
>> +        s->irq_pin[i] &= ~mask;
>> +        s->fiq_pin[i] &= ~mask;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static uint64_t
>> +ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>> +    case REG_IRQSRC:
>> +        return s->irq_src[0];
>> +    case REG_IRQENA:
>> +        return s->irq_ena[0];
>> +    case REG_IRQMDR:
>> +        return s->irq_mod[0];
>> +    case REG_IRQLVR:
>> +        return s->irq_lvl[0];
>> +    case REG_IRQSR:
>> +        return s->irq_src[0] & s->irq_ena[0];
>> +    case REG_EIRQSRC:
>> +        return s->irq_src[1];
>> +    case REG_EIRQENA:
>> +        return s->irq_ena[1];
>> +    case REG_EIRQMDR:
>> +        return s->irq_mod[1];
>> +    case REG_EIRQLVR:
>> +        return s->irq_lvl[1];
>> +    case REG_EIRQSR:
>> +        return s->irq_src[1] & s->irq_ena[1];
>> +
>
> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
> Can you #define some symbols accrordingly and remove all the magic
> numberage with the [0]'s and [1]'s?
>
>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQSRC:
>> +        return s->fiq_src[0];
>> +    case REG_FIQENA:
>> +        return s->fiq_ena[0];
>> +    case REG_FIQMDR:
>> +        return s->fiq_mod[0];
>> +    case REG_FIQLVR:
>> +        return s->fiq_lvl[0];
>> +    case REG_FIQSR:
>> +        return s->fiq_src[0] & s->fiq_ena[0];
>> +    case REG_EFIQSRC:
>> +        return s->fiq_src[1];
>> +    case REG_EFIQENA:
>> +        return s->fiq_ena[1];
>> +    case REG_EFIQMDR:
>> +        return s->fiq_mod[1];
>> +    case REG_EFIQLVR:
>> +        return s->fiq_lvl[1];
>> +    case REG_EFIQSR:
>> +        return s->fiq_src[1] & s->fiq_ena[1];
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void
>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>
> Ok to use one line comment. And elsewhere
>
>> +    case REG_IRQENA:
>> +        s->irq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQSCR:
>> +        value = ~(value & s->irq_mod[0]);
>> +        s->irq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_IRQMDR:
>> +        s->irq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQLVR:
>> +        s->irq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQENA:
>> +        s->irq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQSCR:
>> +        value = ~(value & s->irq_mod[1]);
>> +        s->irq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EIRQMDR:
>> +        s->irq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQLVR:
>> +        s->irq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQENA:
>> +        s->fiq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQSCR:
>> +        value = ~(value & s->fiq_mod[0]);
>> +        s->fiq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_FIQMDR:
>> +        s->fiq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQLVR:
>> +        s->fiq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQENA:
>> +        s->fiq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQSCR:
>> +        value = ~(value & s->fiq_mod[1]);
>> +        s->fiq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EFIQMDR:
>> +        s->fiq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQLVR:
>> +        s->fiq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /* don't care */
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static const MemoryRegionOps mmio_ops = {
>> +    .read  = ftintc020_mem_read,
>> +    .write = ftintc020_mem_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    }
>> +};
>> +
>> +static void ftintc020_reset(DeviceState *ds)
>> +{
>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>> +
>> +    s->irq_pin[0] = 0;
>> +    s->irq_pin[1] = 0;
>> +    s->fiq_pin[0] = 0;
>> +    s->fiq_pin[1] = 0;
>> +
>> +    s->irq_src[0] = 0;
>> +    s->irq_src[1] = 0;
>> +    s->irq_ena[0] = 0;
>> +    s->irq_ena[1] = 0;
>> +    s->fiq_src[0] = 0;
>> +    s->fiq_src[1] = 0;
>> +    s->fiq_ena[0] = 0;
>> +    s->fiq_ena[1] = 0;
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>
> I'm not sure this is the right place for this, I think device creation
> helpers belong on the machine / SoC level. Keep the device model self
> contained and clients call qdev_init themselves. Andreas have the
> rules change on this recently?
>
>> +{
>> +    int i;
>> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
>
> As the device is intended for use in an SoC, the SoC potentially needs
> to hold a pointer to it in order to call device destructors. This
> function should return ds for future use by the instantiator.
>
>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>> +
>
> Use an Object cast macro. Andreas, can we make this easier for
> reviewers and developers by adding blacklisted identifiers to
> checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?
>
>> +    s->cpu = cpu;
>
> Im thinking this is a QOM link. Its a connection from one device to
> another and the machine should be aware of it.
>
>> +    ftintc020_reset(ds);
>
> Doesn't look right but this is just the already registered
> DeviceClass::reset function anyways. You should just be able to delete
> this. Does your system function without it?
>
>> +    qdev_init_nofail(ds);
>
> Cutting from here ...
>
>> +    qdev_init_gpio_in(ds, ftintc020_set_irq, 64);
>> +    for (i = 0; i < 64; ++i) {
>> +        s->irqs[i] = qdev_get_gpio_in(ds, i);
>> +    }
>> +
>> +    /* Enable IC memory-mapped registers access.  */
>> +    memory_region_init_io(&s->iomem,
>> +                          &mmio_ops,
>> +                          s,
>> +                          TYPE_FTINTC020,
>> +                          0x1000);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ds), &s->iomem);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(ds), 0, base);
>> +
>
> ... Im pritty sure all of this belongs in either the ObjectClass::init
> or Device::realize functions.
>
>> +    return s->irqs;
>> +}
>> +
>> +static VMStateDescription vmstate_ftintc020 = {
>> +    .name = TYPE_FTINTC020,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(irq_src, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_ena, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_mod, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_lvl, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_src, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_ena, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_mod, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_lvl, Ftintc020State, 2),
>> +        VMSTATE_END_OF_LIST(),
>> +    },
>> +};
>> +
>> +static int ftintc020_initfn(SysBusDevice *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void ftintc020_class_init(ObjectClass *klass, void *data)
>> +{
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    k->init     = ftintc020_initfn;
>
> SysBusDevice::init is deprecated in favour of the Device::init. Your
> SBD::init doesnt do anything so you can just delete it. But you should

I did some further digging on this and it looks like the dummy
SysBusDevice::init function is needed only in the absence of a
Device::realize fn. Doesnt seem right to me. Posting fix to list
shortly.

Regards,
Peter

> add a device Init here to bring the sysbus foo from your helper
> instantiator into the device model.
>
> Regards,
> Peter
>
>> +    dc->vmsd    = &vmstate_ftintc020;
>> +    dc->reset   = ftintc020_reset;
>> +    dc->no_user = 1;
>> +}
>> +
>> +static const TypeInfo ftintc020_info = {
>> +    .name          = TYPE_FTINTC020,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(Ftintc020State),
>> +    .class_init    = ftintc020_class_init,
>> +};
>> +
>> +static void ftintc020_register_types(void)
>> +{
>> +    type_register_static(&ftintc020_info);
>> +}
>> +
>> +type_init(ftintc020_register_types)
>> diff --git a/hw/arm/ftintc020.h b/hw/arm/ftintc020.h
>> new file mode 100644
>> index 0000000..f17fb58
>> --- /dev/null
>> +++ b/hw/arm/ftintc020.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>> + *
>> + * Copyright (c) 2012 Faraday Technology
>> + * Written by Dante Su <dantesu@faraday-tech.com>
>> + *
>> + * This code is licensed under GNU GPL v2+.
>> + */
>> +
>> +#ifndef HW_ARM_FTINTC020_H
>> +#define HW_ARM_FTINTC020_H
>> +
>> +/*
>> + * IRQ/FIO: 0 ~ 31
>> + */
>> +#define REG_IRQSRC      0x00    /* IRQ source register */
>> +#define REG_IRQENA      0x04    /* IRQ enable register */
>> +#define REG_IRQSCR      0x08    /* IRQ status clear register */
>> +#define REG_IRQMDR      0x0C    /* IRQ trigger mode register */
>> +#define REG_IRQLVR      0x10    /* IRQ trigger level register */
>> +#define REG_IRQSR       0x14    /* IRQ status register */
>> +
>> +#define REG_FIQSRC      0x20    /* FIQ source register */
>> +#define REG_FIQENA      0x24    /* FIQ enable register */
>> +#define REG_FIQSCR      0x28    /* FIQ status clear register */
>> +#define REG_FIQMDR      0x2C    /* FIQ trigger mode register */
>> +#define REG_FIQLVR      0x30    /* FIQ trigger level register */
>> +#define REG_FIQSR       0x34    /* FIQ status register */
>> +
>> +/*
>> + * Extended IRQ/FIO: 32 ~ 63
>> + */
>> +
>> +#define REG_EIRQSRC      0x60   /* Extended IRQ source register */
>> +#define REG_EIRQENA      0x64   /* Extended IRQ enable register */
>> +#define REG_EIRQSCR      0x68   /* Extended IRQ status clear register */
>> +#define REG_EIRQMDR      0x6C   /* Extended IRQ trigger mode register */
>> +#define REG_EIRQLVR      0x70   /* Extended IRQ trigger level register */
>> +#define REG_EIRQSR       0x74   /* Extended IRQ status register */
>> +
>> +#define REG_EFIQSRC      0x80   /* Extended FIQ source register */
>> +#define REG_EFIQENA      0x84   /* Extended FIQ enable register */
>> +#define REG_EFIQSCR      0x88   /* Extended FIQ status clear register */
>> +#define REG_EFIQMDR      0x8C   /* Extended FIQ trigger mode register */
>> +#define REG_EFIQLVR      0x90   /* Extended FIQ trigger level register */
>> +#define REG_EFIQSR       0x94   /* Extended FIQ status register */
>> +
>> +#endif
>> --
>> 1.7.9.5
>>
>>
Peter Crosthwaite - March 3, 2013, 6:29 a.m.
Hi Kuo-Jung,

On Sat, Mar 2, 2013 at 2:13 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Kuo-Jung,
>
> On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> The FTINTC020 interrupt controller supports both FIQ and IRQ signals
>> to the microprocessor.
>> It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
>> The output signals to the microprocessor can be configured as
>> level-high/low active or edge-rising/falling triggered.
>>
>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> ---
>>  hw/arm/Makefile.objs      |    1 +
>>  hw/arm/faraday.h          |    3 +
>>  hw/arm/faraday_a369_soc.c |   10 +-
>>  hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
>>  hw/arm/ftintc020.h        |   48 ++++++
>>  5 files changed, 425 insertions(+), 3 deletions(-)
>>  create mode 100644 hw/arm/ftintc020.c
>>  create mode 100644 hw/arm/ftintc020.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index f6fd60d..6771072 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -37,3 +37,4 @@ obj-y += faraday_a369.o \
>>              faraday_a369_soc.o \
>>              faraday_a369_scu.o \
>>              faraday_a369_kpd.o
>> +obj-y += ftintc020.o
>> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
>> index d6ed860..e5f611d 100644
>> --- a/hw/arm/faraday.h
>> +++ b/hw/arm/faraday.h
>> @@ -62,4 +62,7 @@ typedef struct FaradaySoCState {
>>      FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
>>                                                TYPE_FARADAY_SOC))
>>
>> +/* ftintc020.c */
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
>> +
>>  #endif
>> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
>> index 0372868..3d861d2 100644
>> --- a/hw/arm/faraday_a369_soc.c
>> +++ b/hw/arm/faraday_a369_soc.c
>> @@ -73,6 +73,7 @@ a369soc_device_init(FaradaySoCState *s)
>>  {
>>      DriveInfo *dinfo;
>>      DeviceState *ds;
>> +    qemu_irq *pic;
>>
>>      s->as = get_system_memory();
>>      s->ram = g_new(MemoryRegion, 1);
>> @@ -115,12 +116,15 @@ a369soc_device_init(FaradaySoCState *s)
>>          exit(1);
>>      }
>>
>> +    /* Interrupt Controller */
>> +    pic = ftintc020_init(0x90100000, s->cpu);
>> +
>>      /* Serial (FTUART010 which is 16550A compatible) */
>>      if (serial_hds[0]) {
>>          serial_mm_init(s->as,
>>                         0x92b00000,
>>                         2,
>> -                       NULL,
>> +                       pic[53],
>>                         18432000,
>>                         serial_hds[0],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -129,7 +133,7 @@ a369soc_device_init(FaradaySoCState *s)
>>          serial_mm_init(s->as,
>>                         0x92c00000,
>>                         2,
>> -                       NULL,
>> +                       pic[54],
>>                         18432000,
>>                         serial_hds[1],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -140,7 +144,7 @@ a369soc_device_init(FaradaySoCState *s)
>>      s->scu = ds;
>>
>>      /* ftkbc010 */
>> -    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
>> +    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
>>  }
>>
>>  static int a369soc_init(SysBusDevice *busdev)
>> diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
>> new file mode 100644
>> index 0000000..a7f6454
>> --- /dev/null
>> +++ b/hw/arm/ftintc020.c
>> @@ -0,0 +1,366 @@
>> +/*
>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>> + *
>> + * Copyright (c) 2012 Faraday Technology
>> + * Written by Dante Su <dantesu@faraday-tech.com>
>> + *
>> + * This code is licensed under GNU GPL v2+.
>> + */
>> +
>> +#include "hw/hw.h"
>> +#include "hw/sysbus.h"
>> +
>> +#include "faraday.h"
>> +#include "ftintc020.h"
>> +
>> +#define TYPE_FTINTC020  "ftintc020"
>> +
>> +typedef struct Ftintc020State {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    ARMCPU *cpu;

So Ive looked into your init routine problem a little more and
checkout out how its handled by other ARM interrupt controllers. I
think its gone wrong here, in that an interrupt controller should not
have a handle to a CPU at all. It should just have GPIO outputs for
the interrupt wires. Replace this with GPIO outputs for your intcs IRQ
and FIQ output. This removes the need for your machine model to pass
in an ARMCPU to the device (whether that be via your Ad-Hoc creation
helper or via a QOM link).

>> +    qemu_irq irqs[64];
>> +
>> +    uint32_t irq_pin[2];    /* IRQ pin state */
>> +    uint32_t fiq_pin[2];    /* IRQ pin state */
>> +
>> +    /* HW register caches */
>> +    uint32_t irq_src[2];    /* IRQ source register */
>> +    uint32_t irq_ena[2];    /* IRQ enable register */
>> +    uint32_t irq_mod[2];    /* IRQ mode register */
>> +    uint32_t irq_lvl[2];    /* IRQ level register */
>> +    uint32_t fiq_src[2];    /* FIQ source register */
>> +    uint32_t fiq_ena[2];    /* FIQ enable register */
>> +    uint32_t fiq_mod[2];    /* FIQ mode register */
>> +    uint32_t fiq_lvl[2];    /* FIQ level register */
>> +} Ftintc020State;
>> +
>> +#define FTINTC020(obj) \
>> +    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
>> +
>> +static void
>> +ftintc020_update(Ftintc020State *s)
>> +{
>> +    uint32_t mask[2];
>> +
>> +    /* FIQ */
>> +    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
>> +    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);

Access to the cpu->env from device land is generally bad. Replacing
this logic with GPIO outputs would remove usage of the env.

>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>> +    }
>> +
>> +    /* IRQ */
>> +    mask[0] = s->irq_src[0] & s->irq_ena[0];
>> +    mask[1] = s->irq_src[1] & s->irq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    }
>> +}
>> +
>> +/* Note: Here level means state of the signal on a pin */
>> +static void
>> +ftintc020_set_irq(void *opaque, int irq, int level)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +    uint32_t i = irq / 32;
>> +    uint32_t mask = 1 << (irq % 32);
>> +
>> +    if (s->irq_mod[i] & mask) {
>> +        /* Edge Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Falling Active */
>> +            if ((s->irq_pin[i] & mask) && !level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* Rising Active */
>> +            if (!(s->irq_pin[i] & mask) && level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    } else {
>> +        /* Level Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Low Active */
>> +            if (level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* High Active */
>> +            if (!level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* update IRQ/FIQ pin states */
>> +    if (level) {
>> +        s->irq_pin[i] |= mask;
>> +        s->fiq_pin[i] |= mask;
>> +    } else {
>> +        s->irq_pin[i] &= ~mask;
>> +        s->fiq_pin[i] &= ~mask;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static uint64_t
>> +ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>> +    case REG_IRQSRC:
>> +        return s->irq_src[0];
>> +    case REG_IRQENA:
>> +        return s->irq_ena[0];
>> +    case REG_IRQMDR:
>> +        return s->irq_mod[0];
>> +    case REG_IRQLVR:
>> +        return s->irq_lvl[0];
>> +    case REG_IRQSR:
>> +        return s->irq_src[0] & s->irq_ena[0];
>> +    case REG_EIRQSRC:
>> +        return s->irq_src[1];
>> +    case REG_EIRQENA:
>> +        return s->irq_ena[1];
>> +    case REG_EIRQMDR:
>> +        return s->irq_mod[1];
>> +    case REG_EIRQLVR:
>> +        return s->irq_lvl[1];
>> +    case REG_EIRQSR:
>> +        return s->irq_src[1] & s->irq_ena[1];
>> +
>
> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
> Can you #define some symbols accrordingly and remove all the magic
> numberage with the [0]'s and [1]'s?
>
>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQSRC:
>> +        return s->fiq_src[0];
>> +    case REG_FIQENA:
>> +        return s->fiq_ena[0];
>> +    case REG_FIQMDR:
>> +        return s->fiq_mod[0];
>> +    case REG_FIQLVR:
>> +        return s->fiq_lvl[0];
>> +    case REG_FIQSR:
>> +        return s->fiq_src[0] & s->fiq_ena[0];
>> +    case REG_EFIQSRC:
>> +        return s->fiq_src[1];
>> +    case REG_EFIQENA:
>> +        return s->fiq_ena[1];
>> +    case REG_EFIQMDR:
>> +        return s->fiq_mod[1];
>> +    case REG_EFIQLVR:
>> +        return s->fiq_lvl[1];
>> +    case REG_EFIQSR:
>> +        return s->fiq_src[1] & s->fiq_ena[1];
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void
>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>
> Ok to use one line comment. And elsewhere
>
>> +    case REG_IRQENA:
>> +        s->irq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQSCR:
>> +        value = ~(value & s->irq_mod[0]);
>> +        s->irq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_IRQMDR:
>> +        s->irq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQLVR:
>> +        s->irq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQENA:
>> +        s->irq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQSCR:
>> +        value = ~(value & s->irq_mod[1]);
>> +        s->irq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EIRQMDR:
>> +        s->irq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQLVR:
>> +        s->irq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQENA:
>> +        s->fiq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQSCR:
>> +        value = ~(value & s->fiq_mod[0]);
>> +        s->fiq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_FIQMDR:
>> +        s->fiq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQLVR:
>> +        s->fiq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQENA:
>> +        s->fiq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQSCR:
>> +        value = ~(value & s->fiq_mod[1]);
>> +        s->fiq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EFIQMDR:
>> +        s->fiq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQLVR:
>> +        s->fiq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /* don't care */
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static const MemoryRegionOps mmio_ops = {
>> +    .read  = ftintc020_mem_read,
>> +    .write = ftintc020_mem_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    }
>> +};
>> +
>> +static void ftintc020_reset(DeviceState *ds)
>> +{
>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>> +
>> +    s->irq_pin[0] = 0;
>> +    s->irq_pin[1] = 0;
>> +    s->fiq_pin[0] = 0;
>> +    s->fiq_pin[1] = 0;
>> +
>> +    s->irq_src[0] = 0;
>> +    s->irq_src[1] = 0;
>> +    s->irq_ena[0] = 0;
>> +    s->irq_ena[1] = 0;
>> +    s->fiq_src[0] = 0;
>> +    s->fiq_src[1] = 0;
>> +    s->fiq_ena[0] = 0;
>> +    s->fiq_ena[1] = 0;
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)


This problematic function then goes away completely. The
machine-model/SoC just instantiates your intc and connects your GPIO
outputs to the ARM cpu. You will need to add a call to
arm_pic_init_cpu to your machine-model/SoC. Have a look at vexpress.c
for an example of a machine that does this. ARM gic and mpcore both
export FIQ and IRQ as GPIOs which allows for this much cleaner
solution.

Regards,
Peter
Kuo-Jung Su - March 4, 2013, 6:20 a.m.
2013/3/2 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
> Hi Kuo-Jung,
>
> On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>
>> The FTINTC020 interrupt controller supports both FIQ and IRQ signals
>> to the microprocessor.
>> It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
>> The output signals to the microprocessor can be configured as
>> level-high/low active or edge-rising/falling triggered.
>>
>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>> ---
>>  hw/arm/Makefile.objs      |    1 +
>>  hw/arm/faraday.h          |    3 +
>>  hw/arm/faraday_a369_soc.c |   10 +-
>>  hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
>>  hw/arm/ftintc020.h        |   48 ++++++
>>  5 files changed, 425 insertions(+), 3 deletions(-)
>>  create mode 100644 hw/arm/ftintc020.c
>>  create mode 100644 hw/arm/ftintc020.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index f6fd60d..6771072 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -37,3 +37,4 @@ obj-y += faraday_a369.o \
>>              faraday_a369_soc.o \
>>              faraday_a369_scu.o \
>>              faraday_a369_kpd.o
>> +obj-y += ftintc020.o
>> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
>> index d6ed860..e5f611d 100644
>> --- a/hw/arm/faraday.h
>> +++ b/hw/arm/faraday.h
>> @@ -62,4 +62,7 @@ typedef struct FaradaySoCState {
>>      FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
>>                                                TYPE_FARADAY_SOC))
>>
>> +/* ftintc020.c */
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
>> +
>>  #endif
>> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
>> index 0372868..3d861d2 100644
>> --- a/hw/arm/faraday_a369_soc.c
>> +++ b/hw/arm/faraday_a369_soc.c
>> @@ -73,6 +73,7 @@ a369soc_device_init(FaradaySoCState *s)
>>  {
>>      DriveInfo *dinfo;
>>      DeviceState *ds;
>> +    qemu_irq *pic;
>>
>>      s->as = get_system_memory();
>>      s->ram = g_new(MemoryRegion, 1);
>> @@ -115,12 +116,15 @@ a369soc_device_init(FaradaySoCState *s)
>>          exit(1);
>>      }
>>
>> +    /* Interrupt Controller */
>> +    pic = ftintc020_init(0x90100000, s->cpu);
>> +
>>      /* Serial (FTUART010 which is 16550A compatible) */
>>      if (serial_hds[0]) {
>>          serial_mm_init(s->as,
>>                         0x92b00000,
>>                         2,
>> -                       NULL,
>> +                       pic[53],
>>                         18432000,
>>                         serial_hds[0],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -129,7 +133,7 @@ a369soc_device_init(FaradaySoCState *s)
>>          serial_mm_init(s->as,
>>                         0x92c00000,
>>                         2,
>> -                       NULL,
>> +                       pic[54],
>>                         18432000,
>>                         serial_hds[1],
>>                         DEVICE_LITTLE_ENDIAN);
>> @@ -140,7 +144,7 @@ a369soc_device_init(FaradaySoCState *s)
>>      s->scu = ds;
>>
>>      /* ftkbc010 */
>> -    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
>> +    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
>>  }
>>
>>  static int a369soc_init(SysBusDevice *busdev)
>> diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
>> new file mode 100644
>> index 0000000..a7f6454
>> --- /dev/null
>> +++ b/hw/arm/ftintc020.c
>> @@ -0,0 +1,366 @@
>> +/*
>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>> + *
>> + * Copyright (c) 2012 Faraday Technology
>> + * Written by Dante Su <dantesu@faraday-tech.com>
>> + *
>> + * This code is licensed under GNU GPL v2+.
>> + */
>> +
>> +#include "hw/hw.h"
>> +#include "hw/sysbus.h"
>> +
>> +#include "faraday.h"
>> +#include "ftintc020.h"
>> +
>> +#define TYPE_FTINTC020  "ftintc020"
>> +
>> +typedef struct Ftintc020State {
>> +    SysBusDevice busdev;
>> +    MemoryRegion iomem;
>> +    ARMCPU *cpu;
>> +    qemu_irq irqs[64];
>> +
>> +    uint32_t irq_pin[2];    /* IRQ pin state */
>> +    uint32_t fiq_pin[2];    /* IRQ pin state */
>> +
>> +    /* HW register caches */
>> +    uint32_t irq_src[2];    /* IRQ source register */
>> +    uint32_t irq_ena[2];    /* IRQ enable register */
>> +    uint32_t irq_mod[2];    /* IRQ mode register */
>> +    uint32_t irq_lvl[2];    /* IRQ level register */
>> +    uint32_t fiq_src[2];    /* FIQ source register */
>> +    uint32_t fiq_ena[2];    /* FIQ enable register */
>> +    uint32_t fiq_mod[2];    /* FIQ mode register */
>> +    uint32_t fiq_lvl[2];    /* FIQ level register */
>> +} Ftintc020State;
>> +
>> +#define FTINTC020(obj) \
>> +    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
>> +
>> +static void
>> +ftintc020_update(Ftintc020State *s)
>> +{
>> +    uint32_t mask[2];
>> +
>> +    /* FIQ */
>> +    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
>> +    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>> +    }
>> +
>> +    /* IRQ */
>> +    mask[0] = s->irq_src[0] & s->irq_ena[0];
>> +    mask[1] = s->irq_src[1] & s->irq_ena[1];
>> +
>> +    if (mask[0] || mask[1]) {
>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    } else {
>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> +    }
>> +}
>> +
>> +/* Note: Here level means state of the signal on a pin */
>> +static void
>> +ftintc020_set_irq(void *opaque, int irq, int level)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +    uint32_t i = irq / 32;
>> +    uint32_t mask = 1 << (irq % 32);
>> +
>> +    if (s->irq_mod[i] & mask) {
>> +        /* Edge Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Falling Active */
>> +            if ((s->irq_pin[i] & mask) && !level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* Rising Active */
>> +            if (!(s->irq_pin[i] & mask) && level) {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    } else {
>> +        /* Level Triggered */
>> +        if (s->irq_lvl[i] & mask) {
>> +            /* Low Active */
>> +            if (level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        } else {
>> +            /* High Active */
>> +            if (!level) {
>> +                s->irq_src[i] &= ~mask;
>> +                s->fiq_src[i] &= ~mask;
>> +            } else {
>> +                s->irq_src[i] |=  mask;
>> +                s->fiq_src[i] |=  mask;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* update IRQ/FIQ pin states */
>> +    if (level) {
>> +        s->irq_pin[i] |= mask;
>> +        s->fiq_pin[i] |= mask;
>> +    } else {
>> +        s->irq_pin[i] &= ~mask;
>> +        s->fiq_pin[i] &= ~mask;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static uint64_t
>> +ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>> +    case REG_IRQSRC:
>> +        return s->irq_src[0];
>> +    case REG_IRQENA:
>> +        return s->irq_ena[0];
>> +    case REG_IRQMDR:
>> +        return s->irq_mod[0];
>> +    case REG_IRQLVR:
>> +        return s->irq_lvl[0];
>> +    case REG_IRQSR:
>> +        return s->irq_src[0] & s->irq_ena[0];
>> +    case REG_EIRQSRC:
>> +        return s->irq_src[1];
>> +    case REG_EIRQENA:
>> +        return s->irq_ena[1];
>> +    case REG_EIRQMDR:
>> +        return s->irq_mod[1];
>> +    case REG_EIRQLVR:
>> +        return s->irq_lvl[1];
>> +    case REG_EIRQSR:
>> +        return s->irq_src[1] & s->irq_ena[1];
>> +
>
> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
> Can you #define some symbols accrordingly and remove all the magic
> numberage with the [0]'s and [1]'s?
>

Sure, the ftintc020 is going to be redesigned with the 'hw/pl190.c' as template.
And all the coding style issues would be updated.

>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQSRC:
>> +        return s->fiq_src[0];
>> +    case REG_FIQENA:
>> +        return s->fiq_ena[0];
>> +    case REG_FIQMDR:
>> +        return s->fiq_mod[0];
>> +    case REG_FIQLVR:
>> +        return s->fiq_lvl[0];
>> +    case REG_FIQSR:
>> +        return s->fiq_src[0] & s->fiq_ena[0];
>> +    case REG_EFIQSRC:
>> +        return s->fiq_src[1];
>> +    case REG_EFIQENA:
>> +        return s->fiq_ena[1];
>> +    case REG_EFIQMDR:
>> +        return s->fiq_mod[1];
>> +    case REG_EFIQLVR:
>> +        return s->fiq_lvl[1];
>> +    case REG_EFIQSR:
>> +        return s->fiq_src[1] & s->fiq_ena[1];
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void
>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>> +{
>> +    Ftintc020State *s = FTINTC020(opaque);
>> +
>> +    switch (addr) {
>> +    /*
>> +     * IRQ
>> +     */
>
> Ok to use one line comment. And elsewhere
>
>> +    case REG_IRQENA:
>> +        s->irq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQSCR:
>> +        value = ~(value & s->irq_mod[0]);
>> +        s->irq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_IRQMDR:
>> +        s->irq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_IRQLVR:
>> +        s->irq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQENA:
>> +        s->irq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQSCR:
>> +        value = ~(value & s->irq_mod[1]);
>> +        s->irq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EIRQMDR:
>> +        s->irq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EIRQLVR:
>> +        s->irq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /*
>> +     * FIQ
>> +     */
>> +    case REG_FIQENA:
>> +        s->fiq_ena[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQSCR:
>> +        value = ~(value & s->fiq_mod[0]);
>> +        s->fiq_src[0] &= (uint32_t)value;
>> +        break;
>> +    case REG_FIQMDR:
>> +        s->fiq_mod[0] = (uint32_t)value;
>> +        break;
>> +    case REG_FIQLVR:
>> +        s->fiq_lvl[0] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQENA:
>> +        s->fiq_ena[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQSCR:
>> +        value = ~(value & s->fiq_mod[1]);
>> +        s->fiq_src[1] &= (uint32_t)value;
>> +        break;
>> +    case REG_EFIQMDR:
>> +        s->fiq_mod[1] = (uint32_t)value;
>> +        break;
>> +    case REG_EFIQLVR:
>> +        s->fiq_lvl[1] = (uint32_t)value;
>> +        break;
>> +
>> +    /* don't care */
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>> +        return;
>> +    }
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +static const MemoryRegionOps mmio_ops = {
>> +    .read  = ftintc020_mem_read,
>> +    .write = ftintc020_mem_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    }
>> +};
>> +
>> +static void ftintc020_reset(DeviceState *ds)
>> +{
>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>> +
>> +    s->irq_pin[0] = 0;
>> +    s->irq_pin[1] = 0;
>> +    s->fiq_pin[0] = 0;
>> +    s->fiq_pin[1] = 0;
>> +
>> +    s->irq_src[0] = 0;
>> +    s->irq_src[1] = 0;
>> +    s->irq_ena[0] = 0;
>> +    s->irq_ena[1] = 0;
>> +    s->fiq_src[0] = 0;
>> +    s->fiq_src[1] = 0;
>> +    s->fiq_ena[0] = 0;
>> +    s->fiq_ena[1] = 0;
>> +
>> +    ftintc020_update(s);
>> +}
>> +
>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>
> I'm not sure this is the right place for this, I think device creation
> helpers belong on the machine / SoC level. Keep the device model self
> contained and clients call qdev_init themselves. Andreas have the
> rules change on this recently?
>
>> +{
>> +    int i;
>> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
>
> As the device is intended for use in an SoC, the SoC potentially needs
> to hold a pointer to it in order to call device destructors. This
> function should return ds for future use by the instantiator.
>
>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>> +
>
> Use an Object cast macro. Andreas, can we make this easier for
> reviewers and developers by adding blacklisted identifiers to
> checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?
>
>> +    s->cpu = cpu;
>
> Im thinking this is a QOM link. Its a connection from one device to
> another and the machine should be aware of it.
>

Sorry, I can't get you well....
Did you mean adding the QOM link to cpu like this?

    .........
    s->cpu = cpu_arm_init(s->cpu_model);
    if (!s->cpu) {
        hw_error("a369: Unable to find CPU definition\n");
        exit(1);
    }
    object_property_add_child(qdev_get_machine(),
                              "cpu",
                              OBJECT(s->cpu),
                              NULL);
    .........

However this would lead to an error like this:

**
ERROR:qom/object.c:899:object_property_add_child: assertion failed:
(child->parent == NULL)

>> +    ftintc020_reset(ds);
>
> Doesn't look right but this is just the already registered
> DeviceClass::reset function anyways. You should just be able to delete
> this. Does your system function without it?
>
>> +    qdev_init_nofail(ds);
>
> Cutting from here ...
>
>> +    qdev_init_gpio_in(ds, ftintc020_set_irq, 64);
>> +    for (i = 0; i < 64; ++i) {
>> +        s->irqs[i] = qdev_get_gpio_in(ds, i);
>> +    }
>> +
>> +    /* Enable IC memory-mapped registers access.  */
>> +    memory_region_init_io(&s->iomem,
>> +                          &mmio_ops,
>> +                          s,
>> +                          TYPE_FTINTC020,
>> +                          0x1000);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ds), &s->iomem);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(ds), 0, base);
>> +
>
> ... Im pritty sure all of this belongs in either the ObjectClass::init
> or Device::realize functions.
>
>> +    return s->irqs;
>> +}
>> +
>> +static VMStateDescription vmstate_ftintc020 = {
>> +    .name = TYPE_FTINTC020,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(irq_src, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_ena, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_mod, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(irq_lvl, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_src, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_ena, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_mod, Ftintc020State, 2),
>> +        VMSTATE_UINT32_ARRAY(fiq_lvl, Ftintc020State, 2),
>> +        VMSTATE_END_OF_LIST(),
>> +    },
>> +};
>> +
>> +static int ftintc020_initfn(SysBusDevice *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void ftintc020_class_init(ObjectClass *klass, void *data)
>> +{
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    k->init     = ftintc020_initfn;
>
> SysBusDevice::init is deprecated in favour of the Device::init. Your
> SBD::init doesnt do anything so you can just delete it. But you should
> add a device Init here to bring the sysbus foo from your helper
> instantiator into the device model.
>
> Regards,
> Peter
>
>> +    dc->vmsd    = &vmstate_ftintc020;
>> +    dc->reset   = ftintc020_reset;
>> +    dc->no_user = 1;
>> +}
>> +
>> +static const TypeInfo ftintc020_info = {
>> +    .name          = TYPE_FTINTC020,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(Ftintc020State),
>> +    .class_init    = ftintc020_class_init,
>> +};
>> +
>> +static void ftintc020_register_types(void)
>> +{
>> +    type_register_static(&ftintc020_info);
>> +}
>> +
>> +type_init(ftintc020_register_types)
>> diff --git a/hw/arm/ftintc020.h b/hw/arm/ftintc020.h
>> new file mode 100644
>> index 0000000..f17fb58
>> --- /dev/null
>> +++ b/hw/arm/ftintc020.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>> + *
>> + * Copyright (c) 2012 Faraday Technology
>> + * Written by Dante Su <dantesu@faraday-tech.com>
>> + *
>> + * This code is licensed under GNU GPL v2+.
>> + */
>> +
>> +#ifndef HW_ARM_FTINTC020_H
>> +#define HW_ARM_FTINTC020_H
>> +
>> +/*
>> + * IRQ/FIO: 0 ~ 31
>> + */
>> +#define REG_IRQSRC      0x00    /* IRQ source register */
>> +#define REG_IRQENA      0x04    /* IRQ enable register */
>> +#define REG_IRQSCR      0x08    /* IRQ status clear register */
>> +#define REG_IRQMDR      0x0C    /* IRQ trigger mode register */
>> +#define REG_IRQLVR      0x10    /* IRQ trigger level register */
>> +#define REG_IRQSR       0x14    /* IRQ status register */
>> +
>> +#define REG_FIQSRC      0x20    /* FIQ source register */
>> +#define REG_FIQENA      0x24    /* FIQ enable register */
>> +#define REG_FIQSCR      0x28    /* FIQ status clear register */
>> +#define REG_FIQMDR      0x2C    /* FIQ trigger mode register */
>> +#define REG_FIQLVR      0x30    /* FIQ trigger level register */
>> +#define REG_FIQSR       0x34    /* FIQ status register */
>> +
>> +/*
>> + * Extended IRQ/FIO: 32 ~ 63
>> + */
>> +
>> +#define REG_EIRQSRC      0x60   /* Extended IRQ source register */
>> +#define REG_EIRQENA      0x64   /* Extended IRQ enable register */
>> +#define REG_EIRQSCR      0x68   /* Extended IRQ status clear register */
>> +#define REG_EIRQMDR      0x6C   /* Extended IRQ trigger mode register */
>> +#define REG_EIRQLVR      0x70   /* Extended IRQ trigger level register */
>> +#define REG_EIRQSR       0x74   /* Extended IRQ status register */
>> +
>> +#define REG_EFIQSRC      0x80   /* Extended FIQ source register */
>> +#define REG_EFIQENA      0x84   /* Extended FIQ enable register */
>> +#define REG_EFIQSCR      0x88   /* Extended FIQ status clear register */
>> +#define REG_EFIQMDR      0x8C   /* Extended FIQ trigger mode register */
>> +#define REG_EFIQLVR      0x90   /* Extended FIQ trigger level register */
>> +#define REG_EFIQSR       0x94   /* Extended FIQ status register */
>> +
>> +#endif
>> --
>> 1.7.9.5
>>
>>



--
Best wishes,
Kuo-Jung Su
Kuo-Jung Su - March 4, 2013, 6:29 a.m.
2013/3/3 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
> Hi Kuo-Jung,
>
> On Sat, Mar 2, 2013 at 2:13 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Hi Kuo-Jung,
>>
>> On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>>> From: Kuo-Jung Su <dantesu@faraday-tech.com>
>>>
>>> The FTINTC020 interrupt controller supports both FIQ and IRQ signals
>>> to the microprocessor.
>>> It can handle up to 64 configurable IRQ sources and 64 FIQ sources.
>>> The output signals to the microprocessor can be configured as
>>> level-high/low active or edge-rising/falling triggered.
>>>
>>> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com>
>>> ---
>>>  hw/arm/Makefile.objs      |    1 +
>>>  hw/arm/faraday.h          |    3 +
>>>  hw/arm/faraday_a369_soc.c |   10 +-
>>>  hw/arm/ftintc020.c        |  366 +++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/arm/ftintc020.h        |   48 ++++++
>>>  5 files changed, 425 insertions(+), 3 deletions(-)
>>>  create mode 100644 hw/arm/ftintc020.c
>>>  create mode 100644 hw/arm/ftintc020.h
>>>
>>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>>> index f6fd60d..6771072 100644
>>> --- a/hw/arm/Makefile.objs
>>> +++ b/hw/arm/Makefile.objs
>>> @@ -37,3 +37,4 @@ obj-y += faraday_a369.o \
>>>              faraday_a369_soc.o \
>>>              faraday_a369_scu.o \
>>>              faraday_a369_kpd.o
>>> +obj-y += ftintc020.o
>>> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
>>> index d6ed860..e5f611d 100644
>>> --- a/hw/arm/faraday.h
>>> +++ b/hw/arm/faraday.h
>>> @@ -62,4 +62,7 @@ typedef struct FaradaySoCState {
>>>      FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
>>>                                                TYPE_FARADAY_SOC))
>>>
>>> +/* ftintc020.c */
>>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
>>> +
>>>  #endif
>>> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
>>> index 0372868..3d861d2 100644
>>> --- a/hw/arm/faraday_a369_soc.c
>>> +++ b/hw/arm/faraday_a369_soc.c
>>> @@ -73,6 +73,7 @@ a369soc_device_init(FaradaySoCState *s)
>>>  {
>>>      DriveInfo *dinfo;
>>>      DeviceState *ds;
>>> +    qemu_irq *pic;
>>>
>>>      s->as = get_system_memory();
>>>      s->ram = g_new(MemoryRegion, 1);
>>> @@ -115,12 +116,15 @@ a369soc_device_init(FaradaySoCState *s)
>>>          exit(1);
>>>      }
>>>
>>> +    /* Interrupt Controller */
>>> +    pic = ftintc020_init(0x90100000, s->cpu);
>>> +
>>>      /* Serial (FTUART010 which is 16550A compatible) */
>>>      if (serial_hds[0]) {
>>>          serial_mm_init(s->as,
>>>                         0x92b00000,
>>>                         2,
>>> -                       NULL,
>>> +                       pic[53],
>>>                         18432000,
>>>                         serial_hds[0],
>>>                         DEVICE_LITTLE_ENDIAN);
>>> @@ -129,7 +133,7 @@ a369soc_device_init(FaradaySoCState *s)
>>>          serial_mm_init(s->as,
>>>                         0x92c00000,
>>>                         2,
>>> -                       NULL,
>>> +                       pic[54],
>>>                         18432000,
>>>                         serial_hds[1],
>>>                         DEVICE_LITTLE_ENDIAN);
>>> @@ -140,7 +144,7 @@ a369soc_device_init(FaradaySoCState *s)
>>>      s->scu = ds;
>>>
>>>      /* ftkbc010 */
>>> -    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
>>> +    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
>>>  }
>>>
>>>  static int a369soc_init(SysBusDevice *busdev)
>>> diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
>>> new file mode 100644
>>> index 0000000..a7f6454
>>> --- /dev/null
>>> +++ b/hw/arm/ftintc020.c
>>> @@ -0,0 +1,366 @@
>>> +/*
>>> + * Faraday FTINTC020 Programmable Interrupt Controller.
>>> + *
>>> + * Copyright (c) 2012 Faraday Technology
>>> + * Written by Dante Su <dantesu@faraday-tech.com>
>>> + *
>>> + * This code is licensed under GNU GPL v2+.
>>> + */
>>> +
>>> +#include "hw/hw.h"
>>> +#include "hw/sysbus.h"
>>> +
>>> +#include "faraday.h"
>>> +#include "ftintc020.h"
>>> +
>>> +#define TYPE_FTINTC020  "ftintc020"
>>> +
>>> +typedef struct Ftintc020State {
>>> +    SysBusDevice busdev;
>>> +    MemoryRegion iomem;
>>> +    ARMCPU *cpu;
>
> So Ive looked into your init routine problem a little more and
> checkout out how its handled by other ARM interrupt controllers. I
> think its gone wrong here, in that an interrupt controller should not
> have a handle to a CPU at all. It should just have GPIO outputs for
> the interrupt wires. Replace this with GPIO outputs for your intcs IRQ
> and FIQ output. This removes the need for your machine model to pass
> in an ARMCPU to the device (whether that be via your Ad-Hoc creation
> helper or via a QOM link).
>

At the time I was writing the model for FTINTC020, I found that
hw/pxa2xx_pic.c is more straightforward to me.
However it turns out that I've made a bad decision.

>>> +    qemu_irq irqs[64];
>>> +
>>> +    uint32_t irq_pin[2];    /* IRQ pin state */
>>> +    uint32_t fiq_pin[2];    /* IRQ pin state */
>>> +
>>> +    /* HW register caches */
>>> +    uint32_t irq_src[2];    /* IRQ source register */
>>> +    uint32_t irq_ena[2];    /* IRQ enable register */
>>> +    uint32_t irq_mod[2];    /* IRQ mode register */
>>> +    uint32_t irq_lvl[2];    /* IRQ level register */
>>> +    uint32_t fiq_src[2];    /* FIQ source register */
>>> +    uint32_t fiq_ena[2];    /* FIQ enable register */
>>> +    uint32_t fiq_mod[2];    /* FIQ mode register */
>>> +    uint32_t fiq_lvl[2];    /* FIQ level register */
>>> +} Ftintc020State;
>>> +
>>> +#define FTINTC020(obj) \
>>> +    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
>>> +
>>> +static void
>>> +ftintc020_update(Ftintc020State *s)
>>> +{
>>> +    uint32_t mask[2];
>>> +
>>> +    /* FIQ */
>>> +    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
>>> +    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
>>> +
>>> +    if (mask[0] || mask[1]) {
>>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>
> Access to the cpu->env from device land is generally bad. Replacing
> this logic with GPIO outputs would remove usage of the env.
>
>>> +    } else {
>>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
>>> +    }
>>> +
>>> +    /* IRQ */
>>> +    mask[0] = s->irq_src[0] & s->irq_ena[0];
>>> +    mask[1] = s->irq_src[1] & s->irq_ena[1];
>>> +
>>> +    if (mask[0] || mask[1]) {
>>> +        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>>> +    } else {
>>> +        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>>> +    }
>>> +}
>>> +
>>> +/* Note: Here level means state of the signal on a pin */
>>> +static void
>>> +ftintc020_set_irq(void *opaque, int irq, int level)
>>> +{
>>> +    Ftintc020State *s = FTINTC020(opaque);
>>> +    uint32_t i = irq / 32;
>>> +    uint32_t mask = 1 << (irq % 32);
>>> +
>>> +    if (s->irq_mod[i] & mask) {
>>> +        /* Edge Triggered */
>>> +        if (s->irq_lvl[i] & mask) {
>>> +            /* Falling Active */
>>> +            if ((s->irq_pin[i] & mask) && !level) {
>>> +                s->irq_src[i] |=  mask;
>>> +                s->fiq_src[i] |=  mask;
>>> +            }
>>> +        } else {
>>> +            /* Rising Active */
>>> +            if (!(s->irq_pin[i] & mask) && level) {
>>> +                s->irq_src[i] |=  mask;
>>> +                s->fiq_src[i] |=  mask;
>>> +            }
>>> +        }
>>> +    } else {
>>> +        /* Level Triggered */
>>> +        if (s->irq_lvl[i] & mask) {
>>> +            /* Low Active */
>>> +            if (level) {
>>> +                s->irq_src[i] &= ~mask;
>>> +                s->fiq_src[i] &= ~mask;
>>> +            } else {
>>> +                s->irq_src[i] |=  mask;
>>> +                s->fiq_src[i] |=  mask;
>>> +            }
>>> +        } else {
>>> +            /* High Active */
>>> +            if (!level) {
>>> +                s->irq_src[i] &= ~mask;
>>> +                s->fiq_src[i] &= ~mask;
>>> +            } else {
>>> +                s->irq_src[i] |=  mask;
>>> +                s->fiq_src[i] |=  mask;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    /* update IRQ/FIQ pin states */
>>> +    if (level) {
>>> +        s->irq_pin[i] |= mask;
>>> +        s->fiq_pin[i] |= mask;
>>> +    } else {
>>> +        s->irq_pin[i] &= ~mask;
>>> +        s->fiq_pin[i] &= ~mask;
>>> +    }
>>> +
>>> +    ftintc020_update(s);
>>> +}
>>> +
>>> +static uint64_t
>>> +ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
>>> +{
>>> +    Ftintc020State *s = FTINTC020(opaque);
>>> +
>>> +    switch (addr) {
>>> +    /*
>>> +     * IRQ
>>> +     */
>>> +    case REG_IRQSRC:
>>> +        return s->irq_src[0];
>>> +    case REG_IRQENA:
>>> +        return s->irq_ena[0];
>>> +    case REG_IRQMDR:
>>> +        return s->irq_mod[0];
>>> +    case REG_IRQLVR:
>>> +        return s->irq_lvl[0];
>>> +    case REG_IRQSR:
>>> +        return s->irq_src[0] & s->irq_ena[0];
>>> +    case REG_EIRQSRC:
>>> +        return s->irq_src[1];
>>> +    case REG_EIRQENA:
>>> +        return s->irq_ena[1];
>>> +    case REG_EIRQMDR:
>>> +        return s->irq_mod[1];
>>> +    case REG_EIRQLVR:
>>> +        return s->irq_lvl[1];
>>> +    case REG_EIRQSR:
>>> +        return s->irq_src[1] & s->irq_ena[1];
>>> +
>>
>> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
>> Can you #define some symbols accrordingly and remove all the magic
>> numberage with the [0]'s and [1]'s?
>>
>>> +    /*
>>> +     * FIQ
>>> +     */
>>> +    case REG_FIQSRC:
>>> +        return s->fiq_src[0];
>>> +    case REG_FIQENA:
>>> +        return s->fiq_ena[0];
>>> +    case REG_FIQMDR:
>>> +        return s->fiq_mod[0];
>>> +    case REG_FIQLVR:
>>> +        return s->fiq_lvl[0];
>>> +    case REG_FIQSR:
>>> +        return s->fiq_src[0] & s->fiq_ena[0];
>>> +    case REG_EFIQSRC:
>>> +        return s->fiq_src[1];
>>> +    case REG_EFIQENA:
>>> +        return s->fiq_ena[1];
>>> +    case REG_EFIQMDR:
>>> +        return s->fiq_mod[1];
>>> +    case REG_EFIQLVR:
>>> +        return s->fiq_lvl[1];
>>> +    case REG_EFIQSR:
>>> +        return s->fiq_src[1] & s->fiq_ena[1];
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> +static void
>>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>>> +{
>>> +    Ftintc020State *s = FTINTC020(opaque);
>>> +
>>> +    switch (addr) {
>>> +    /*
>>> +     * IRQ
>>> +     */
>>
>> Ok to use one line comment. And elsewhere
>>
>>> +    case REG_IRQENA:
>>> +        s->irq_ena[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQSCR:
>>> +        value = ~(value & s->irq_mod[0]);
>>> +        s->irq_src[0] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQMDR:
>>> +        s->irq_mod[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQLVR:
>>> +        s->irq_lvl[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQENA:
>>> +        s->irq_ena[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQSCR:
>>> +        value = ~(value & s->irq_mod[1]);
>>> +        s->irq_src[1] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQMDR:
>>> +        s->irq_mod[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQLVR:
>>> +        s->irq_lvl[1] = (uint32_t)value;
>>> +        break;
>>> +
>>> +    /*
>>> +     * FIQ
>>> +     */
>>> +    case REG_FIQENA:
>>> +        s->fiq_ena[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQSCR:
>>> +        value = ~(value & s->fiq_mod[0]);
>>> +        s->fiq_src[0] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQMDR:
>>> +        s->fiq_mod[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQLVR:
>>> +        s->fiq_lvl[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQENA:
>>> +        s->fiq_ena[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQSCR:
>>> +        value = ~(value & s->fiq_mod[1]);
>>> +        s->fiq_src[1] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQMDR:
>>> +        s->fiq_mod[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQLVR:
>>> +        s->fiq_lvl[1] = (uint32_t)value;
>>> +        break;
>>> +
>>> +    /* don't care */
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>> +        return;
>>> +    }
>>> +
>>> +    ftintc020_update(s);
>>> +}
>>> +
>>> +static const MemoryRegionOps mmio_ops = {
>>> +    .read  = ftintc020_mem_read,
>>> +    .write = ftintc020_mem_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +    .valid = {
>>> +        .min_access_size = 4,
>>> +        .max_access_size = 4,
>>> +    }
>>> +};
>>> +
>>> +static void ftintc020_reset(DeviceState *ds)
>>> +{
>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>> +
>>> +    s->irq_pin[0] = 0;
>>> +    s->irq_pin[1] = 0;
>>> +    s->fiq_pin[0] = 0;
>>> +    s->fiq_pin[1] = 0;
>>> +
>>> +    s->irq_src[0] = 0;
>>> +    s->irq_src[1] = 0;
>>> +    s->irq_ena[0] = 0;
>>> +    s->irq_ena[1] = 0;
>>> +    s->fiq_src[0] = 0;
>>> +    s->fiq_src[1] = 0;
>>> +    s->fiq_ena[0] = 0;
>>> +    s->fiq_ena[1] = 0;
>>> +
>>> +    ftintc020_update(s);
>>> +}
>>> +
>>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>
>
> This problematic function then goes away completely. The
> machine-model/SoC just instantiates your intc and connects your GPIO
> outputs to the ARM cpu. You will need to add a call to
> arm_pic_init_cpu to your machine-model/SoC. Have a look at vexpress.c
> for an example of a machine that does this. ARM gic and mpcore both
> export FIQ and IRQ as GPIOs which allows for this much cleaner
> solution.
>

Got it, thanks

The FTINTC020 has been re-written with vexpress.c, versatilepb.c and
pl190.c as templates

> Regards,
> Peter



--
Best wishes,
Kuo-Jung Su
Peter Crosthwaite - March 4, 2013, 6:33 a.m.
Hi Kuo-Jung,

On Mon, Mar 4, 2013 at 4:20 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
> 2013/3/2 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
>> Hi Kuo-Jung,
[Snip]
>>> +        return s->irq_lvl[1];
>>> +    case REG_EIRQSR:
>>> +        return s->irq_src[1] & s->irq_ena[1];
>>> +
>>
>> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
>> Can you #define some symbols accrordingly and remove all the magic
>> numberage with the [0]'s and [1]'s?
>>
>
> Sure, the ftintc020 is going to be redesigned with the 'hw/pl190.c' as template.
> And all the coding style issues would be updated.
>
>>> +    /*
>>> +     * FIQ
>>> +     */
>>> +    case REG_FIQSRC:
>>> +        return s->fiq_src[0];
>>> +    case REG_FIQENA:
>>> +        return s->fiq_ena[0];
>>> +    case REG_FIQMDR:
>>> +        return s->fiq_mod[0];
>>> +    case REG_FIQLVR:
>>> +        return s->fiq_lvl[0];
>>> +    case REG_FIQSR:
>>> +        return s->fiq_src[0] & s->fiq_ena[0];
>>> +    case REG_EFIQSRC:
>>> +        return s->fiq_src[1];
>>> +    case REG_EFIQENA:
>>> +        return s->fiq_ena[1];
>>> +    case REG_EFIQMDR:
>>> +        return s->fiq_mod[1];
>>> +    case REG_EFIQLVR:
>>> +        return s->fiq_lvl[1];
>>> +    case REG_EFIQSR:
>>> +        return s->fiq_src[1] & s->fiq_ena[1];
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>> +static void
>>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>>> +{
>>> +    Ftintc020State *s = FTINTC020(opaque);
>>> +
>>> +    switch (addr) {
>>> +    /*
>>> +     * IRQ
>>> +     */
>>
>> Ok to use one line comment. And elsewhere
>>
>>> +    case REG_IRQENA:
>>> +        s->irq_ena[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQSCR:
>>> +        value = ~(value & s->irq_mod[0]);
>>> +        s->irq_src[0] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQMDR:
>>> +        s->irq_mod[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_IRQLVR:
>>> +        s->irq_lvl[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQENA:
>>> +        s->irq_ena[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQSCR:
>>> +        value = ~(value & s->irq_mod[1]);
>>> +        s->irq_src[1] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQMDR:
>>> +        s->irq_mod[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EIRQLVR:
>>> +        s->irq_lvl[1] = (uint32_t)value;
>>> +        break;
>>> +
>>> +    /*
>>> +     * FIQ
>>> +     */
>>> +    case REG_FIQENA:
>>> +        s->fiq_ena[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQSCR:
>>> +        value = ~(value & s->fiq_mod[0]);
>>> +        s->fiq_src[0] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQMDR:
>>> +        s->fiq_mod[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_FIQLVR:
>>> +        s->fiq_lvl[0] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQENA:
>>> +        s->fiq_ena[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQSCR:
>>> +        value = ~(value & s->fiq_mod[1]);
>>> +        s->fiq_src[1] &= (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQMDR:
>>> +        s->fiq_mod[1] = (uint32_t)value;
>>> +        break;
>>> +    case REG_EFIQLVR:
>>> +        s->fiq_lvl[1] = (uint32_t)value;
>>> +        break;
>>> +
>>> +    /* don't care */
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>> +        return;
>>> +    }
>>> +
>>> +    ftintc020_update(s);
>>> +}
>>> +
>>> +static const MemoryRegionOps mmio_ops = {
>>> +    .read  = ftintc020_mem_read,
>>> +    .write = ftintc020_mem_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +    .valid = {
>>> +        .min_access_size = 4,
>>> +        .max_access_size = 4,
>>> +    }
>>> +};
>>> +
>>> +static void ftintc020_reset(DeviceState *ds)
>>> +{
>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>> +
>>> +    s->irq_pin[0] = 0;
>>> +    s->irq_pin[1] = 0;
>>> +    s->fiq_pin[0] = 0;
>>> +    s->fiq_pin[1] = 0;
>>> +
>>> +    s->irq_src[0] = 0;
>>> +    s->irq_src[1] = 0;
>>> +    s->irq_ena[0] = 0;
>>> +    s->irq_ena[1] = 0;
>>> +    s->fiq_src[0] = 0;
>>> +    s->fiq_src[1] = 0;
>>> +    s->fiq_ena[0] = 0;
>>> +    s->fiq_ena[1] = 0;
>>> +
>>> +    ftintc020_update(s);
>>> +}
>>> +
>>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>>
>> I'm not sure this is the right place for this, I think device creation
>> helpers belong on the machine / SoC level. Keep the device model self
>> contained and clients call qdev_init themselves. Andreas have the
>> rules change on this recently?
>>
>>> +{
>>> +    int i;
>>> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
>>
>> As the device is intended for use in an SoC, the SoC potentially needs
>> to hold a pointer to it in order to call device destructors. This
>> function should return ds for future use by the instantiator.
>>
>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>> +
>>
>> Use an Object cast macro. Andreas, can we make this easier for
>> reviewers and developers by adding blacklisted identifiers to
>> checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?
>>
>>> +    s->cpu = cpu;
>>
>> Im thinking this is a QOM link. Its a connection from one device to
>> another and the machine should be aware of it.
>>
>
> Sorry, I can't get you well....
> Did you mean adding the QOM link to cpu like this?
>
>     .........
>     s->cpu = cpu_arm_init(s->cpu_model);
>     if (!s->cpu) {
>         hw_error("a369: Unable to find CPU definition\n");
>         exit(1);
>     }
>     object_property_add_child(qdev_get_machine(),
>                               "cpu",
>                               OBJECT(s->cpu),
>                               NULL);
>     .........
>

Definately not a child note. object_property_add_link.

> However this would lead to an error like this:
>
> **
> ERROR:qom/object.c:899:object_property_add_child: assertion failed:
> (child->parent == NULL)
>

The CPU is already parented, which is why this is asserting. You can't
adopt it as your own child here.

But I wouldn't bother with a link approach at all anymore, see my
later reply regarding changing over to a GPIO out approach which is
more modern. Removes the need for this cpu linkage completely. Your
rewrite based on the pl190 VIC should fix this anyway. This will all
go away.

Regards,
Peter
Kuo-Jung Su - March 4, 2013, 6:54 a.m.
2013/3/4 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
> Hi Kuo-Jung,
>
> On Mon, Mar 4, 2013 at 4:20 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
>> 2013/3/2 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
>>> Hi Kuo-Jung,
> [Snip]
>>>> +        return s->irq_lvl[1];
>>>> +    case REG_EIRQSR:
>>>> +        return s->irq_src[1] & s->irq_ena[1];
>>>> +
>>>
>>> AFAICT, index 0 of there arrays in for IRQ and index 1 is for EIRQ.
>>> Can you #define some symbols accrordingly and remove all the magic
>>> numberage with the [0]'s and [1]'s?
>>>
>>
>> Sure, the ftintc020 is going to be redesigned with the 'hw/pl190.c' as template.
>> And all the coding style issues would be updated.
>>
>>>> +    /*
>>>> +     * FIQ
>>>> +     */
>>>> +    case REG_FIQSRC:
>>>> +        return s->fiq_src[0];
>>>> +    case REG_FIQENA:
>>>> +        return s->fiq_ena[0];
>>>> +    case REG_FIQMDR:
>>>> +        return s->fiq_mod[0];
>>>> +    case REG_FIQLVR:
>>>> +        return s->fiq_lvl[0];
>>>> +    case REG_FIQSR:
>>>> +        return s->fiq_src[0] & s->fiq_ena[0];
>>>> +    case REG_EFIQSRC:
>>>> +        return s->fiq_src[1];
>>>> +    case REG_EFIQENA:
>>>> +        return s->fiq_ena[1];
>>>> +    case REG_EFIQMDR:
>>>> +        return s->fiq_mod[1];
>>>> +    case REG_EFIQLVR:
>>>> +        return s->fiq_lvl[1];
>>>> +    case REG_EFIQSR:
>>>> +        return s->fiq_src[1] & s->fiq_ena[1];
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>>> +        return 0;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void
>>>> +ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
>>>> +{
>>>> +    Ftintc020State *s = FTINTC020(opaque);
>>>> +
>>>> +    switch (addr) {
>>>> +    /*
>>>> +     * IRQ
>>>> +     */
>>>
>>> Ok to use one line comment. And elsewhere
>>>
>>>> +    case REG_IRQENA:
>>>> +        s->irq_ena[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQSCR:
>>>> +        value = ~(value & s->irq_mod[0]);
>>>> +        s->irq_src[0] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQMDR:
>>>> +        s->irq_mod[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_IRQLVR:
>>>> +        s->irq_lvl[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQENA:
>>>> +        s->irq_ena[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQSCR:
>>>> +        value = ~(value & s->irq_mod[1]);
>>>> +        s->irq_src[1] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQMDR:
>>>> +        s->irq_mod[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EIRQLVR:
>>>> +        s->irq_lvl[1] = (uint32_t)value;
>>>> +        break;
>>>> +
>>>> +    /*
>>>> +     * FIQ
>>>> +     */
>>>> +    case REG_FIQENA:
>>>> +        s->fiq_ena[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQSCR:
>>>> +        value = ~(value & s->fiq_mod[0]);
>>>> +        s->fiq_src[0] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQMDR:
>>>> +        s->fiq_mod[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_FIQLVR:
>>>> +        s->fiq_lvl[0] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQENA:
>>>> +        s->fiq_ena[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQSCR:
>>>> +        value = ~(value & s->fiq_mod[1]);
>>>> +        s->fiq_src[1] &= (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQMDR:
>>>> +        s->fiq_mod[1] = (uint32_t)value;
>>>> +        break;
>>>> +    case REG_EFIQLVR:
>>>> +        s->fiq_lvl[1] = (uint32_t)value;
>>>> +        break;
>>>> +
>>>> +    /* don't care */
>>>> +    default:
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "ftintc020: undefined memory access@0x%llx\n", addr);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ftintc020_update(s);
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps mmio_ops = {
>>>> +    .read  = ftintc020_mem_read,
>>>> +    .write = ftintc020_mem_write,
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +    .valid = {
>>>> +        .min_access_size = 4,
>>>> +        .max_access_size = 4,
>>>> +    }
>>>> +};
>>>> +
>>>> +static void ftintc020_reset(DeviceState *ds)
>>>> +{
>>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>>> +
>>>> +    s->irq_pin[0] = 0;
>>>> +    s->irq_pin[1] = 0;
>>>> +    s->fiq_pin[0] = 0;
>>>> +    s->fiq_pin[1] = 0;
>>>> +
>>>> +    s->irq_src[0] = 0;
>>>> +    s->irq_src[1] = 0;
>>>> +    s->irq_ena[0] = 0;
>>>> +    s->irq_ena[1] = 0;
>>>> +    s->fiq_src[0] = 0;
>>>> +    s->fiq_src[1] = 0;
>>>> +    s->fiq_ena[0] = 0;
>>>> +    s->fiq_ena[1] = 0;
>>>> +
>>>> +    ftintc020_update(s);
>>>> +}
>>>> +
>>>> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
>>>
>>> I'm not sure this is the right place for this, I think device creation
>>> helpers belong on the machine / SoC level. Keep the device model self
>>> contained and clients call qdev_init themselves. Andreas have the
>>> rules change on this recently?
>>>
>>>> +{
>>>> +    int i;
>>>> +    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
>>>
>>> As the device is intended for use in an SoC, the SoC potentially needs
>>> to hold a pointer to it in order to call device destructors. This
>>> function should return ds for future use by the instantiator.
>>>
>>>> +    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
>>>> +    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
>>>> +
>>>
>>> Use an Object cast macro. Andreas, can we make this easier for
>>> reviewers and developers by adding blacklisted identifiers to
>>> checkpatch perhaps? throw a warning on +FROM_SYSBUS, DO_UPCAST etc?
>>>
>>>> +    s->cpu = cpu;
>>>
>>> Im thinking this is a QOM link. Its a connection from one device to
>>> another and the machine should be aware of it.
>>>
>>
>> Sorry, I can't get you well....
>> Did you mean adding the QOM link to cpu like this?
>>
>>     .........
>>     s->cpu = cpu_arm_init(s->cpu_model);
>>     if (!s->cpu) {
>>         hw_error("a369: Unable to find CPU definition\n");
>>         exit(1);
>>     }
>>     object_property_add_child(qdev_get_machine(),
>>                               "cpu",
>>                               OBJECT(s->cpu),
>>                               NULL);
>>     .........
>>
>
> Definately not a child note. object_property_add_link.
>

Got it, thanks.

>> However this would lead to an error like this:
>>
>> **
>> ERROR:qom/object.c:899:object_property_add_child: assertion failed:
>> (child->parent == NULL)
>>
>
> The CPU is already parented, which is why this is asserting. You can't
> adopt it as your own child here.
>
> But I wouldn't bother with a link approach at all anymore, see my
> later reply regarding changing over to a GPIO out approach which is
> more modern. Removes the need for this cpu linkage completely. Your
> rewrite based on the pl190 VIC should fix this anyway. This will all
> go away.
>

There is one more place that the 'cpu' pointer would be referenced,
please check here:

hw/arm/faraday_a369.c:

......
    if (args->kernel_filename) {
        ......
        arm_load_kernel(s->cpu, s->bi);
        ......
    }
......

I guess I still have to make a QOM link to the cpu pointer.

> Regards,
> Peter



--
Best wishes,
Kuo-Jung Su

Patch

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index f6fd60d..6771072 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -37,3 +37,4 @@  obj-y += faraday_a369.o \
             faraday_a369_soc.o \
             faraday_a369_scu.o \
             faraday_a369_kpd.o
+obj-y += ftintc020.o
diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
index d6ed860..e5f611d 100644
--- a/hw/arm/faraday.h
+++ b/hw/arm/faraday.h
@@ -62,4 +62,7 @@  typedef struct FaradaySoCState {
     FARADAY_SOC(object_resolve_path_component(qdev_get_machine(), \
                                               TYPE_FARADAY_SOC))
 
+/* ftintc020.c */
+qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
+
 #endif
diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
index 0372868..3d861d2 100644
--- a/hw/arm/faraday_a369_soc.c
+++ b/hw/arm/faraday_a369_soc.c
@@ -73,6 +73,7 @@  a369soc_device_init(FaradaySoCState *s)
 {
     DriveInfo *dinfo;
     DeviceState *ds;
+    qemu_irq *pic;
 
     s->as = get_system_memory();
     s->ram = g_new(MemoryRegion, 1);
@@ -115,12 +116,15 @@  a369soc_device_init(FaradaySoCState *s)
         exit(1);
     }
 
+    /* Interrupt Controller */
+    pic = ftintc020_init(0x90100000, s->cpu);
+
     /* Serial (FTUART010 which is 16550A compatible) */
     if (serial_hds[0]) {
         serial_mm_init(s->as,
                        0x92b00000,
                        2,
-                       NULL,
+                       pic[53],
                        18432000,
                        serial_hds[0],
                        DEVICE_LITTLE_ENDIAN);
@@ -129,7 +133,7 @@  a369soc_device_init(FaradaySoCState *s)
         serial_mm_init(s->as,
                        0x92c00000,
                        2,
-                       NULL,
+                       pic[54],
                        18432000,
                        serial_hds[1],
                        DEVICE_LITTLE_ENDIAN);
@@ -140,7 +144,7 @@  a369soc_device_init(FaradaySoCState *s)
     s->scu = ds;
 
     /* ftkbc010 */
-    sysbus_create_simple("a369.keypad", 0x92f00000, NULL);
+    sysbus_create_simple("a369.keypad", 0x92f00000, pic[21]);
 }
 
 static int a369soc_init(SysBusDevice *busdev)
diff --git a/hw/arm/ftintc020.c b/hw/arm/ftintc020.c
new file mode 100644
index 0000000..a7f6454
--- /dev/null
+++ b/hw/arm/ftintc020.c
@@ -0,0 +1,366 @@ 
+/*
+ * Faraday FTINTC020 Programmable Interrupt Controller.
+ *
+ * Copyright (c) 2012 Faraday Technology
+ * Written by Dante Su <dantesu@faraday-tech.com>
+ *
+ * This code is licensed under GNU GPL v2+.
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+
+#include "faraday.h"
+#include "ftintc020.h"
+
+#define TYPE_FTINTC020  "ftintc020"
+
+typedef struct Ftintc020State {
+    SysBusDevice busdev;
+    MemoryRegion iomem;
+    ARMCPU *cpu;
+    qemu_irq irqs[64];
+
+    uint32_t irq_pin[2];    /* IRQ pin state */
+    uint32_t fiq_pin[2];    /* IRQ pin state */
+
+    /* HW register caches */
+    uint32_t irq_src[2];    /* IRQ source register */
+    uint32_t irq_ena[2];    /* IRQ enable register */
+    uint32_t irq_mod[2];    /* IRQ mode register */
+    uint32_t irq_lvl[2];    /* IRQ level register */
+    uint32_t fiq_src[2];    /* FIQ source register */
+    uint32_t fiq_ena[2];    /* FIQ enable register */
+    uint32_t fiq_mod[2];    /* FIQ mode register */
+    uint32_t fiq_lvl[2];    /* FIQ level register */
+} Ftintc020State;
+
+#define FTINTC020(obj) \
+    OBJECT_CHECK(Ftintc020State, obj, TYPE_FTINTC020)
+
+static void
+ftintc020_update(Ftintc020State *s)
+{
+    uint32_t mask[2];
+
+    /* FIQ */
+    mask[0] = s->fiq_src[0] & s->fiq_ena[0];
+    mask[1] = s->fiq_src[1] & s->fiq_ena[1];
+
+    if (mask[0] || mask[1]) {
+        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
+    } else {
+        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_FIQ);
+    }
+
+    /* IRQ */
+    mask[0] = s->irq_src[0] & s->irq_ena[0];
+    mask[1] = s->irq_src[1] & s->irq_ena[1];
+
+    if (mask[0] || mask[1]) {
+        cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
+    } else {
+        cpu_reset_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
+    }
+}
+
+/* Note: Here level means state of the signal on a pin */
+static void
+ftintc020_set_irq(void *opaque, int irq, int level)
+{
+    Ftintc020State *s = FTINTC020(opaque);
+    uint32_t i = irq / 32;
+    uint32_t mask = 1 << (irq % 32);
+
+    if (s->irq_mod[i] & mask) {
+        /* Edge Triggered */
+        if (s->irq_lvl[i] & mask) {
+            /* Falling Active */
+            if ((s->irq_pin[i] & mask) && !level) {
+                s->irq_src[i] |=  mask;
+                s->fiq_src[i] |=  mask;
+            }
+        } else {
+            /* Rising Active */
+            if (!(s->irq_pin[i] & mask) && level) {
+                s->irq_src[i] |=  mask;
+                s->fiq_src[i] |=  mask;
+            }
+        }
+    } else {
+        /* Level Triggered */
+        if (s->irq_lvl[i] & mask) {
+            /* Low Active */
+            if (level) {
+                s->irq_src[i] &= ~mask;
+                s->fiq_src[i] &= ~mask;
+            } else {
+                s->irq_src[i] |=  mask;
+                s->fiq_src[i] |=  mask;
+            }
+        } else {
+            /* High Active */
+            if (!level) {
+                s->irq_src[i] &= ~mask;
+                s->fiq_src[i] &= ~mask;
+            } else {
+                s->irq_src[i] |=  mask;
+                s->fiq_src[i] |=  mask;
+            }
+        }
+    }
+
+    /* update IRQ/FIQ pin states */
+    if (level) {
+        s->irq_pin[i] |= mask;
+        s->fiq_pin[i] |= mask;
+    } else {
+        s->irq_pin[i] &= ~mask;
+        s->fiq_pin[i] &= ~mask;
+    }
+
+    ftintc020_update(s);
+}
+
+static uint64_t
+ftintc020_mem_read(void *opaque, hwaddr addr, unsigned size)
+{
+    Ftintc020State *s = FTINTC020(opaque);
+
+    switch (addr) {
+    /*
+     * IRQ
+     */
+    case REG_IRQSRC:
+        return s->irq_src[0];
+    case REG_IRQENA:
+        return s->irq_ena[0];
+    case REG_IRQMDR:
+        return s->irq_mod[0];
+    case REG_IRQLVR:
+        return s->irq_lvl[0];
+    case REG_IRQSR:
+        return s->irq_src[0] & s->irq_ena[0];
+    case REG_EIRQSRC:
+        return s->irq_src[1];
+    case REG_EIRQENA:
+        return s->irq_ena[1];
+    case REG_EIRQMDR:
+        return s->irq_mod[1];
+    case REG_EIRQLVR:
+        return s->irq_lvl[1];
+    case REG_EIRQSR:
+        return s->irq_src[1] & s->irq_ena[1];
+
+    /*
+     * FIQ
+     */
+    case REG_FIQSRC:
+        return s->fiq_src[0];
+    case REG_FIQENA:
+        return s->fiq_ena[0];
+    case REG_FIQMDR:
+        return s->fiq_mod[0];
+    case REG_FIQLVR:
+        return s->fiq_lvl[0];
+    case REG_FIQSR:
+        return s->fiq_src[0] & s->fiq_ena[0];
+    case REG_EFIQSRC:
+        return s->fiq_src[1];
+    case REG_EFIQENA:
+        return s->fiq_ena[1];
+    case REG_EFIQMDR:
+        return s->fiq_mod[1];
+    case REG_EFIQLVR:
+        return s->fiq_lvl[1];
+    case REG_EFIQSR:
+        return s->fiq_src[1] & s->fiq_ena[1];
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ftintc020: undefined memory access@0x%llx\n", addr);
+        return 0;
+    }
+}
+
+static void
+ftintc020_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size)
+{
+    Ftintc020State *s = FTINTC020(opaque);
+
+    switch (addr) {
+    /*
+     * IRQ
+     */
+    case REG_IRQENA:
+        s->irq_ena[0] = (uint32_t)value;
+        break;
+    case REG_IRQSCR:
+        value = ~(value & s->irq_mod[0]);
+        s->irq_src[0] &= (uint32_t)value;
+        break;
+    case REG_IRQMDR:
+        s->irq_mod[0] = (uint32_t)value;
+        break;
+    case REG_IRQLVR:
+        s->irq_lvl[0] = (uint32_t)value;
+        break;
+    case REG_EIRQENA:
+        s->irq_ena[1] = (uint32_t)value;
+        break;
+    case REG_EIRQSCR:
+        value = ~(value & s->irq_mod[1]);
+        s->irq_src[1] &= (uint32_t)value;
+        break;
+    case REG_EIRQMDR:
+        s->irq_mod[1] = (uint32_t)value;
+        break;
+    case REG_EIRQLVR:
+        s->irq_lvl[1] = (uint32_t)value;
+        break;
+
+    /*
+     * FIQ
+     */
+    case REG_FIQENA:
+        s->fiq_ena[0] = (uint32_t)value;
+        break;
+    case REG_FIQSCR:
+        value = ~(value & s->fiq_mod[0]);
+        s->fiq_src[0] &= (uint32_t)value;
+        break;
+    case REG_FIQMDR:
+        s->fiq_mod[0] = (uint32_t)value;
+        break;
+    case REG_FIQLVR:
+        s->fiq_lvl[0] = (uint32_t)value;
+        break;
+    case REG_EFIQENA:
+        s->fiq_ena[1] = (uint32_t)value;
+        break;
+    case REG_EFIQSCR:
+        value = ~(value & s->fiq_mod[1]);
+        s->fiq_src[1] &= (uint32_t)value;
+        break;
+    case REG_EFIQMDR:
+        s->fiq_mod[1] = (uint32_t)value;
+        break;
+    case REG_EFIQLVR:
+        s->fiq_lvl[1] = (uint32_t)value;
+        break;
+
+    /* don't care */
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "ftintc020: undefined memory access@0x%llx\n", addr);
+        return;
+    }
+
+    ftintc020_update(s);
+}
+
+static const MemoryRegionOps mmio_ops = {
+    .read  = ftintc020_mem_read,
+    .write = ftintc020_mem_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    }
+};
+
+static void ftintc020_reset(DeviceState *ds)
+{
+    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
+    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
+
+    s->irq_pin[0] = 0;
+    s->irq_pin[1] = 0;
+    s->fiq_pin[0] = 0;
+    s->fiq_pin[1] = 0;
+
+    s->irq_src[0] = 0;
+    s->irq_src[1] = 0;
+    s->irq_ena[0] = 0;
+    s->irq_ena[1] = 0;
+    s->fiq_src[0] = 0;
+    s->fiq_src[1] = 0;
+    s->fiq_ena[0] = 0;
+    s->fiq_ena[1] = 0;
+
+    ftintc020_update(s);
+}
+
+qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu)
+{
+    int i;
+    DeviceState *ds = qdev_create(NULL, TYPE_FTINTC020);
+    SysBusDevice *busdev = SYS_BUS_DEVICE(ds);
+    Ftintc020State *s = FTINTC020(FROM_SYSBUS(Ftintc020State, busdev));
+
+    s->cpu = cpu;
+    ftintc020_reset(ds);
+    qdev_init_nofail(ds);
+    qdev_init_gpio_in(ds, ftintc020_set_irq, 64);
+    for (i = 0; i < 64; ++i) {
+        s->irqs[i] = qdev_get_gpio_in(ds, i);
+    }
+
+    /* Enable IC memory-mapped registers access.  */
+    memory_region_init_io(&s->iomem,
+                          &mmio_ops,
+                          s,
+                          TYPE_FTINTC020,
+                          0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(ds), &s->iomem);
+    sysbus_mmio_map(SYS_BUS_DEVICE(ds), 0, base);
+
+    return s->irqs;
+}
+
+static VMStateDescription vmstate_ftintc020 = {
+    .name = TYPE_FTINTC020,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(irq_src, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(irq_ena, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(irq_mod, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(irq_lvl, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(fiq_src, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(fiq_ena, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(fiq_mod, Ftintc020State, 2),
+        VMSTATE_UINT32_ARRAY(fiq_lvl, Ftintc020State, 2),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static int ftintc020_initfn(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static void ftintc020_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    k->init     = ftintc020_initfn;
+    dc->vmsd    = &vmstate_ftintc020;
+    dc->reset   = ftintc020_reset;
+    dc->no_user = 1;
+}
+
+static const TypeInfo ftintc020_info = {
+    .name          = TYPE_FTINTC020,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Ftintc020State),
+    .class_init    = ftintc020_class_init,
+};
+
+static void ftintc020_register_types(void)
+{
+    type_register_static(&ftintc020_info);
+}
+
+type_init(ftintc020_register_types)
diff --git a/hw/arm/ftintc020.h b/hw/arm/ftintc020.h
new file mode 100644
index 0000000..f17fb58
--- /dev/null
+++ b/hw/arm/ftintc020.h
@@ -0,0 +1,48 @@ 
+/*
+ * Faraday FTINTC020 Programmable Interrupt Controller.
+ *
+ * Copyright (c) 2012 Faraday Technology
+ * Written by Dante Su <dantesu@faraday-tech.com>
+ *
+ * This code is licensed under GNU GPL v2+.
+ */
+
+#ifndef HW_ARM_FTINTC020_H
+#define HW_ARM_FTINTC020_H
+
+/*
+ * IRQ/FIO: 0 ~ 31
+ */
+#define REG_IRQSRC      0x00    /* IRQ source register */
+#define REG_IRQENA      0x04    /* IRQ enable register */
+#define REG_IRQSCR      0x08    /* IRQ status clear register */
+#define REG_IRQMDR      0x0C    /* IRQ trigger mode register */
+#define REG_IRQLVR      0x10    /* IRQ trigger level register */
+#define REG_IRQSR       0x14    /* IRQ status register */
+
+#define REG_FIQSRC      0x20    /* FIQ source register */
+#define REG_FIQENA      0x24    /* FIQ enable register */
+#define REG_FIQSCR      0x28    /* FIQ status clear register */
+#define REG_FIQMDR      0x2C    /* FIQ trigger mode register */
+#define REG_FIQLVR      0x30    /* FIQ trigger level register */
+#define REG_FIQSR       0x34    /* FIQ status register */
+
+/*
+ * Extended IRQ/FIO: 32 ~ 63
+ */
+
+#define REG_EIRQSRC      0x60   /* Extended IRQ source register */
+#define REG_EIRQENA      0x64   /* Extended IRQ enable register */
+#define REG_EIRQSCR      0x68   /* Extended IRQ status clear register */
+#define REG_EIRQMDR      0x6C   /* Extended IRQ trigger mode register */
+#define REG_EIRQLVR      0x70   /* Extended IRQ trigger level register */
+#define REG_EIRQSR       0x74   /* Extended IRQ status register */
+
+#define REG_EFIQSRC      0x80   /* Extended FIQ source register */
+#define REG_EFIQENA      0x84   /* Extended FIQ enable register */
+#define REG_EFIQSCR      0x88   /* Extended FIQ status clear register */
+#define REG_EFIQMDR      0x8C   /* Extended FIQ trigger mode register */
+#define REG_EFIQLVR      0x90   /* Extended FIQ trigger level register */
+#define REG_EFIQSR       0x94   /* Extended FIQ status register */
+
+#endif