Re: [Qemu-devel] [PATCH v7 5/8] irq: Add a new irq device that allows the ORing of lines

[Alistair Francis]

On Mon, Sep 19, 2016 at 7:03 AM, Peter Maydell <address@hidden> wrote:
> On 11 September 2016 at 15:54, Alistair Francis <address@hidden> wrote:
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> As the migration framework is not included in user mode this needs to be a
>> new file.
>>
>> V7:
>>  - Use the standard QEMU init/realise functions
>> V6:
>>  - Make the OR IRQ device a TYPE_DEVICE
>>  - Add vmstate
>> +static void or_irq_handler(void *opaque, int n, int level)
>> +{
>> +    qemu_or_irq *s = OR_IRQ(opaque);
>> +    int or_level = 0;
>> +    int i;
>> +
>> +    s->levels[n] = level;
>> +
>> +    for (i = 0; i < s->num_lines; i++) {
>> +        or_level |= s->levels[i];
>> +    }
>> +
>> +    qemu_set_irq(s->out_irq, or_level);
>> +}
>> +
>> +static void or_irq_reset(DeviceState *dev)
>> +{
>> +    qemu_or_irq *s = OR_IRQ(dev);
>> +    int i;
>> +
>> +    for (i = 0; i < MAX_OR_LINES; i++) {
>> +        s->levels[i] = false;
>> +    }
>> +
>> +    /* Trigger an update of the out irqs. We just set the level to 0 */
>> +    or_irq_handler(s, 0, 0);
>
> Changing an output qdev IRQ line on reset() is generally
> not recommended. Is it necessary here?

No, it isn't necessary. I have removed it.

>
>> +#include "hw/irq.h"
>> +#include "hw/sysbus.h"
>> +#include "qom/object.h"
>> +
>> +#define TYPE_OR_IRQ "or-irq"
>> +
>> +#define MAX_OR_LINES      16
>
> This is a slightly arbitrary limit, but it's bigger than anybody's
> really likely to need and there's a forward compatible path for
> migration if we need to support larger sizes in future, so it's OK.
> (if num_lines > 16 you would dynamically allocate a levels array
> and migrate it with a migration subsection that's only present if the
> num_lines property is >16)
>
>> +typedef struct OrIRQState qemu_or_irq;
>> +
>> +#define OR_IRQ(obj) OBJECT_CHECK(qemu_or_irq, (obj), TYPE_OR_IRQ)
>> +
>> +struct OrIRQState {
>> +    DeviceState parent_obj;
>> +
>> +    qemu_irq out_irq;
>> +    qemu_irq *in_irqs;
>> +    bool levels[MAX_OR_LINES];
>> +    uint16_t num_lines;
>> +};
>> +
>> +qemu_irq *qemu_get_or_irqs(DeviceState *dev);
>
> Why this rather than defining the input lines with qdev_init_gpio_in()
> so the users can use qdev_get_gpio_in() ?

I was worried that I couldn't create the interrupts in the realise
function, but it seems to work.

Thanks,

Alistair

>
> thanks
> -- PMM