[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: |
Kuo-Jung Su |
|
Subject: |
Re: [Qemu-devel] [PATCH v5 03/24] hw/arm: add Faraday FTINTC020 interrupt controller support |
|
Date: |
Mon, 4 Mar 2013 14:29:47 +0800 |
2013/3/3 Peter Crosthwaite <address@hidden>:
> 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).
>
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 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.
>
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