Re: [PATCH 2/6] hw/arm/exynos4210: Fix DMA initialization

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <address@hidden> wrote:
>
> First parameter to exynos4210_get_irq() is not the SPI port number,
> but the interrupt group number. Interrupt groups are 20 for mdma
> and 21 for pdma. Interrupts are not inverted. Controllers support 32
> events (pdma) or 31 events (mdma). Events must all be routed to a single
> interrupt line. Set other parameters as documented in Exynos4210 datasheet,
> section 8 (DMA controller).
>
> Fixes: 59520dc65e ("hw/arm/exynos4210: Add DMA support for the Exynos4210")
> Signed-off-by: Guenter Roeck <address@hidden>
> ---
>  hw/arm/exynos4210.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 77fbe1baab..c7b5c587b1 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -166,17 +166,31 @@ static uint64_t exynos4210_calc_affinity(int cpu)
>      return (0x9 << ARM_AFF1_SHIFT) | cpu;
>  }
>
> -static void pl330_create(uint32_t base, qemu_irq irq, int nreq)
> +static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents,
> +                         int width)
>  {
>      SysBusDevice *busdev;
>      DeviceState *dev;
> +    int i;
>
>      dev = qdev_create(NULL, "pl330");
> +    qdev_prop_set_uint8(dev, "num_events", nevents);
> +    qdev_prop_set_uint8(dev, "num_chnls",  8);
>      qdev_prop_set_uint8(dev, "num_periph_req",  nreq);
> +
> +    qdev_prop_set_uint8(dev, "wr_cap", 4);
> +    qdev_prop_set_uint8(dev, "wr_q_dep", 8);
> +    qdev_prop_set_uint8(dev, "rd_cap", 4);
> +    qdev_prop_set_uint8(dev, "rd_q_dep", 8);
> +    qdev_prop_set_uint8(dev, "data_width", width);
> +    qdev_prop_set_uint16(dev, "data_buffer_dep", width);
>      qdev_init_nofail(dev);
>      busdev = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(busdev, 0, base);
> -    sysbus_connect_irq(busdev, 0, irq);
> +    sysbus_connect_irq(busdev, 0, irq);         /* abort irq line */
> +    for (i = 0; i < nevents; i++) {
> +        sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
> +    }

It isn't valid to connect multiple qemu_irq outputs to a single
input like this. If the hardware logically ORs the irq lines
together then you need to instantiate and wire up a TYPE_OR_IRQ
device (include/hw/or-irq.h) to do that. Unfortunately QEMU
doesn't catch accidental wiring of a qemu_irq to multiple
inputs, and it will even sort-of seem to work: the bug is that
if two inputs go high, and then one goes low, the destination
will get a "signal went low" call even though the first input
should still be holding the line high.

(Conversely, to connect one qemu_irq to multiple outputs you
need a TYPE_SPLIT_IRQ, include/hw/core/split-irq.h).

thanks
-- PMM