Re: IRQ handling (was [Qemu-devel] qemu Makefile.target vl.h hw/acpi.c hw/adlib.c ...)

[J. Mayer]

On Sun, 2007-04-08 at 01:04 +0100, Thiemo Seufer wrote:
> J. Mayer wrote:
> [snip]
> > To give you an real example why arbitrary limits are not acceptable AT
> > ALL: I know an embedded Mips device (widely used !) with 2 CPU, 8 PIC
> > and about 500 IRQ sources.
> 
> Care to tell which one this is?

I'm sorry, I'm no sure I can (NDA rules....).
Let's say it's a chips used in some consumer electronics products.

> > How can you even pretend add a limited
> > structure in the CPUState structure when this is exactly the kind of
> > device some people want to emulate in Qemu ? Your concept is completely
> > broken, you have to admit it. You can never put peripheral informations
> > in the CPUState structure.
> 
> At least for MIPS it makes sense to put the CPU-internal controller
> in exactly that place.

It does not. If you look well, the IRQ controller is not in the CPU.
Only the exception are managed in the CPU. The "internal" IRQ controller
is a peripheral device located in the CP0. OK, the CP0 is tightly
coupled to the CPU so it's easier to consider it as part of the CPU,
when emulating it. But it seems like you could imagine a MIPS CPU
without a CP0 coprocessor (even if it will never happen in the real
world), no ?

The problem is also: what does this patch adds, but complexity and
arbitrary limitations ?
What do you need to route an IRQ ?
-> A peripheral destination
What we got now ?
-> a callback with 3 parameters: an opaque, a PIN (the n_IRQ) and a
state
Is more needed to have a generic routing approach ?
-> no. This can work to route any signal
Can we do with less ?
-> no. You need the 3 parameters.
Can we share data ?
-> It seems not. The opaque is used internally by the IRQ controller.
The PIN number has a meaning only inside the IRQ controller. And the
meaning of the state depends on the IRQ controller state.
The only thing that could be changed to hide the complexity is define a
structure that contain all information, like the struct IRQState.
But this structure cannot be shared as it has no signification outside
of an IRQ controller.
What can be done (but once again, it changes nothing, just hide the
"complexity"), is to replace the  { callback, n_IRQ } in devices
structure by a IRQState structure and have the inline functions.

static inline void qemu_irq_set_state(struct IRQState *st, int level)
{
      if (st->handler != NULL)
        (*st->handler)(st->opaque, st->n, level);
}
static inline void qemu_irq_raise(struct IRQState *st)
{
    qemu_irq_set_state(st, 1);
}
static inline void qemu_irq_lower(struct IRQState *st)
{
    qemu_irq_set_state(st, 0);
}

If you want to be generic, as it is said it's supposed to be (but is not) to be 
able to manage any signal, then this is not sufficient.
One will also have to get the value. If you want to emulate GPIOs, for example, 
you need to store the current state
to be able to get it back easily. And for GPIO, you may prefer that changing 
the state has no action but just be able
to read back the current PIN level at any time.

To achieve this, you have to have a structure:
struct PINState {
    qemu_pin_handler handler;
    void *opaque;
    int n;
    int level;
};

static inline void qemu_pin_set_state(struct PINState *st, int level)
{
      if (st->handler != NULL && level != st->level != level) {
        (*st->handler)(st->opaque, st->n, level);
        st->level = level;
    }
}
static inline int qemu_pin_get_state(struct PINState *st)
{
        return st->level;
}

static inline void qemu_ping_raise(struct PINState *st)
{
    qemu_pin_pin_state(st, 1);
}
static inline void qemu_pin_lower(struct PINState *st)
{
    qemu_pin_set_state(st, 0);
}

The hw/irq.c code is not needed and meaningless. The first function should be 
inlined, unless the goal is to slowdown the emulator.
The second function has no meaning: you can never globally allocate resources 
that have no meaning outside of a limited
scope, which is the case for IRQ or any physical connection on a real hardware.

The PINState structure can be store only in the peripheral device that will 
change the PIN state and the peripheral that will read this state.
Trying to store it in a generic place is completely meaningless: a PIN state (I 
talk about real hardware, now) has absolutelly
no generic meaning. It's only significant between two devices. You  eventually 
can have more than one device changing the
PIN state (shared IRQ, I2C bus, ....) and more than one device using this PIN 
state (I2C, once again). But outside of the scope
of those devices, it means nothing. So the information is to be stored inside 
those devices, anywhere else is meaningless.


-- 
J. Mayer <address@hidden>
Never organized