[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: |
Sun, 3 Mar 2013 14:58:39 +1000 |
Hi All,
On Sat, Mar 2, 2013 at 2:13 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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
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 <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
>>
>>