[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 16:29:54 +1000 |
Hi Kuo-Jung,
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;
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 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)
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