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

[Guenter Roeck <linux@roeck-us.net>]

Hi Peter,

On Fri, Jan 17, 2020 at 01:30:19PM +0000, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net>
> > ---
> >  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.
> 
Makes sense. Unfortunately, it isn't easy for the non-initiated to
figure out how to wire this up. There are several examples, all
do it differently, and I am having difficulties understanding it.
I'll try to do it, but it may take a while.

Thanks,
Guenter