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

[Alistair Francis]

On Mon, Jul 25, 2016 at 10:17 AM, Peter Maydell
<address@hidden> wrote:
> 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.

So the way that the GPIO connections work it's better to return the
array of inputs, that way you can easily call sysbus_connect_irq()
with the device and GIC GPIO inputs being ORed together.

I think this function should work:

/*
 * 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);

I'll send the patches as soon as the tests finish.

Thanks,

Alistair

>
> thanks
> -- PMM