[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 03/24] hw/arm: add Faraday FTINTC020 interrup
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH v5 03/24] hw/arm: add Faraday FTINTC020 interrupt controller support |
|
Date: |
Sat, 2 Mar 2013 14:13:19 +1000 |
Hi Kuo-Jung,
On Wed, Feb 27, 2013 at 5:15 PM, Kuo-Jung Su <address@hidden> wrote:
> From: Kuo-Jung Su <address@hidden>
>
> 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 <address@hidden>
> ---
> 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 <address@hidden>
> + *
> + * 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 address@hidden", 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 address@hidden", 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 <address@hidden>
> + *
> + * 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
>
>
- Re: [Qemu-devel] [PATCH v5 03/24] hw/arm: add Faraday FTINTC020 interrupt controller support,
Peter Crosthwaite <=