On Tue, Aug 9, 2016 at 9:42 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 3 August 2016 at 04:10, Alistair Francis <alistai...@gmail.com> wrote:
>> Signed-off-by: Alistair Francis <alist...@alistair23.me>
>> ---
>> As the migration framework is not included in user mode this needs to be a
>> new file.
>>
>> V6:
>> - Make the OR IRQ device a TYPE_DEVICE
>> - Add vmstate
>>
>> hw/core/Makefile.objs | 1 +
>> hw/core/irq.c | 1 +
>> hw/core/or-irq.c | 102
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/irq.h | 15 ++++++++
>> 4 files changed, 119 insertions(+)
>> create mode 100644 hw/core/or-irq.c
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index cfd4840..b47241b 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -16,4 +16,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>> common-obj-$(CONFIG_SOFTMMU) += loader.o
>> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>> common-obj-$(CONFIG_SOFTMMU) += register.o
>> +common-obj-$(CONFIG_SOFTMMU) += or-irq.o
>> common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
>> diff --git a/hw/core/irq.c b/hw/core/irq.c
>> index 49ff2e6..dc874cc 100644
>> --- a/hw/core/irq.c
>> +++ b/hw/core/irq.c
>> @@ -24,6 +24,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu-common.h"
>> #include "hw/irq.h"
>> +#include "hw/sysbus.h"
>> #include "qom/object.h"
>>
>> #define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
>
> We don't change anything else in this file so I think this added include
> is leftover from development?
Yep, it is. I have fixed this.
>
>> diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
>> new file mode 100644
>> index 0000000..2bd181b
>> --- /dev/null
>> +++ b/hw/core/or-irq.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * QEMU IRQ/GPIO common code.
>> + *
>> + * Copyright (c) 2016 Alistair Francis <alist...@alistair23.me>.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "hw/irq.h"
>> +#include "hw/sysbus.h"
>> +#include "qom/object.h"
>> +
>> +#define OR_IRQ(obj) OBJECT_CHECK(qemu_or_irq, (obj), TYPE_OR_IRQ)
>> +
>> +struct OrIRQState {
>> + Object parent_obj;
>> +
>> + qemu_irq in_irq;
>> + qemu_irq *out_irqs;
>
> Aren't these names backwards? An OR gate has multiple inputs
> and one output.
I think I was thinking of the direction from the devices. So the
out_irqs are the lines that go out from the device and the in_irq goes
into the next device (the GIC in this case).
Although that probably is somewhat confusing.
>
>> + int16_t levels[MAX_OR_LINES];
>
> Why is this an int16_t array? I was expecting an array of bool
> (or whatever the equivalent is for vmstate), or alternatively
> a bitmap.h bitmap with one bit per input.
It should be bools, I just couldn't find the VMstate macro for bools
at the time.
I have fixed this.
>
>> + int n;
>> +};
>> +
>> +static void or_irq_handler(void *opaque, int n, int level)
>> +{
>> + qemu_or_irq *or_irq = OR_IRQ(opaque);
>> + int or_level = 0;
>> + int i;
>> +
>> + or_irq->levels[n] = level;
>> +
>> + for (i = 0; i < or_irq->n; i++) {
>> + or_level |= or_irq->levels[i];
>> + }
>> +
>> + qemu_set_irq(or_irq->in_irq, or_level);
>> +}
>> +
>> +qemu_irq *qemu_allocate_or_irqs(qemu_irq in_irq, int n)
>
> This API is awkward because it means that there's no way for
> the caller to arrange for the OR_IRQ object to be freed
> (for instance if you want to use it in a hotpluggable device),
> unless I'm missing something.
>
> I was sort of expecting this to be just-another-device that
> you would wire up with the existing sysbus gpio APIs.
Ok, I have fixed this up. It now follows the standard API.
>
>> +{
>> + qemu_or_irq *or_irq;
>> +
>> + assert(n < MAX_OR_LINES);
>> +
>> + or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
>> + object_initialize(or_irq, sizeof(qemu_or_irq),
>> + TYPE_OR_IRQ);
>> +
>> + or_irq->out_irqs = qemu_allocate_irqs(or_irq_handler, or_irq, n);
>> + or_irq->in_irq = in_irq;
>> + or_irq->n = n;
>> +
>> + return or_irq->out_irqs;
>> +}
>> +
>> +static const VMStateDescription vmstate_or_irq = {
>> + .name = TYPE_OR_IRQ,
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_INT16_ARRAY(levels, qemu_or_irq, MAX_OR_LINES),
>> + VMSTATE_END_OF_LIST(),
>> + }
>> +};
>> +
>> +static void or_irq_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->vmsd = &vmstate_or_irq;
>
> I think this also needs a reset method that just zeroes
> the levels array.
Fixed.
Thanks,
Alistair
>
>> +}
>> +
>> +static const TypeInfo or_irq_type_info = {
>> + .name = TYPE_OR_IRQ,
>> + .parent = TYPE_DEVICE,
>> + .instance_size = sizeof(qemu_or_irq),
>> + .class_init = or_irq_class_init,
>> +};
>> +
>> +static void or_irq_register_types(void)
>> +{
>> + type_register_static(&or_irq_type_info);
>> +}
>> +
>> +type_init(or_irq_register_types)
>> diff --git a/include/hw/irq.h b/include/hw/irq.h
>> index 4c4c2ea..5e8a3b6 100644
>> --- a/include/hw/irq.h
>> +++ b/include/hw/irq.h
>> @@ -4,8 +4,12 @@
>> /* Generic IRQ/GPIO pin infrastructure. */
>>
>> #define TYPE_IRQ "irq"
>> +#define TYPE_OR_IRQ "or-irq"
>> +
>> +#define MAX_OR_LINES 16
>>
>> typedef struct IRQState *qemu_irq;
>> +typedef struct OrIRQState qemu_or_irq;
>>
>> typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
>>
>> @@ -38,6 +42,17 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler,
>> void *opaque, int n);
>> */
>> qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n);
>>
>> +/*
>> + * qemu_allocate_or_irqs
>> + * @in_irq: An input IRQ. It will be the result of the @out_irqs ORed
>> together
>> + * @n: The number of interrupt lines that should be ORed together
>> + *
>> + * returns: An array of interrupts that should be ORed together
>> + *
>> + * OR all of the interrupts returned in the array into a single @in_irq.
>> + */
>> +qemu_irq *qemu_allocate_or_irqs(qemu_irq in_irq, int n);
>> +
>> /* Extends an Array of IRQs. Old IRQs have their handlers and opaque data
>> * preserved. New IRQs are assigned the argument handler and opaque data.
>> */
>> --
>> 2.7.4
>>
>
> thanks
> -- PMM
