Re: [Qemu-devel] [PATCH v4 6/8] STM32F205: Connect the ADC devices

[Peter Maydell]

On 23 July 2016 at 17:42, Alistair Francis <address@hidden> wrote:
> Connect the ADC devices to the STM32F205 SoC.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V4:
>  - Connect all the interrupt lines correctly
> V2:
>  - Fix up the device/devices commit message

> @@ -132,6 +142,29 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, 
> Error **errp)
>          sysbus_mmio_map(busdev, 0, timer_addr[i]);
>          sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, timer_irq[i]));
>      }
> +
> +    /* ADC 1 to 3 */
> +    for (i = 0; i < STM_NUM_ADCS; i++) {
> +        dev = DEVICE(&(s->adc[i]));
> +        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        busdev = SYS_BUS_DEVICE(dev);
> +        sysbus_mmio_map(busdev, 0, adc_addr[i]);
> +    }
> +
> +    /* Connect each ADC device to the one GIC IRQ line */
> +    gic_adc_irq = qdev_get_gpio_in(nvic, ADC_IRQ);
> +    gic_adc_irq_arr = qemu_extend_irqs(NULL, 0,
> +                                       qemu_irq_get_handler(gic_adc_irq),
> +                                       qemu_irq_get_opaque(gic_adc_irq),
> +                                       STM_NUM_ADCS);
> +    for (i = 0; i < STM_NUM_ADCS; i++) {
> +        dev = DEVICE(&(s->adc[i]));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, gic_adc_irq_arr[i]);
> +    }

This will lower the IRQ input to the GIC as soon as any one of the
ADCs lowers its IRQ (even if another of them has it held high), and
conversely raise it again as soon as any one ADC raises its IRQ
(even if another of them has it held low). We talked about this
on the last version: http://patchwork.ozlabs.org/patch/569823/
and I still think you should implement an OR gate here.

Checking the real hardware behaviour by writing a test would
be ideal; but whatever the hardware does it isn't going to be
what this code does; and an OR gate is the obvious guess.

I was going to suggest something like:

/**
 * qemu_irq_or:
 * @irqs: array of IRQs to OR together
 * @numirqs: number of IRQs in @irqs
 *
 * Returns a new IRQ which behaves as the logical OR of all the
 * input IRQS in @irqs.
 */
qemu_irq qemu_irq_or(qemu_irq *irqs, int numirqs);

along the same lines as qemu_irq_invert(), which creates a NOT gate,
but then I realized you need some actual state in there (because
you need to remember the current state of all the input lines).
So this probably needs to be a real device object so it can be
migrated.

thanks
-- PMM