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

[Peter Maydell]

On Fri, 17 Jan 2020 at 18:07, Guenter Roeck <address@hidden> wrote:
>
> 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 <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.
> >
> 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.

I'd suggest following how hw/arm/armsse.c does it. This is
assuming we need one OR gate for each PL330. Roughly:

 * #include "hw/or-irq.h" in include/hw/arm/exynos4210.h
 * new field in the Exynos4210State struct "qemu_or_irq
pl330_irq_orgate[NUM_PL330S];"
 * TYPE_EXYNOS4210_SOC will need an instance_init, so add
   ".instance_init = exynos4210_init," to the exynos4210_info definition
 * all the instance_init needs to do is call object_initialize_child()
   for each OR gate, something like:
      static void exynos4210_init(Object *obj)
      {
          Exynos4210State *s = EXYNOS4210_SOC(obj);
          int i;

          for (i = 0; i < ARRAY_SIZE(s->pl330_irq_orgate); i++) {
              char *name = g_strdup_printf("pl330-irq-orgate%d", i);
              qemu_or_irq *orgate = &s->pl330_irq_orgate[i];

              object_initialize_child(obj, name, orgate, sizeof(*orgate),
                                      TYPE_OR_IRQ, &error_abort, NULL);
              g_free(name);
          }
      }
  * in exynos4210_realize() you need another loop; the loop body
    should set the number of input lines and realize the object:
    object_property_set_int(OBJECT(&s->pl330_irq_orgate[i]),
                            num_lines,
                            "num-lines", &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }
    object_property_set_bool(OBJECT(&s->pl330_irq_orgate[i]), true,
                             "realized", &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

    where num_lines is I think going to be 33, 33, and 2 (1 + the
    value of nevents for that pl330).
    It doesn't really matter exactly where in the realize function
    we do this as long as the realize of the or-gate happens
    before we wire it up. So you might want to have a single
    loop whose body does "set or-gate num_lines; realize or-gate;
    create pl330; wire up pl330".

  * finally, the "wiring up" part. a qemu_irq_orgate is just a
    qdev device with N inputs and one output. So in pl330_create()
    (assuming 'this_orgate' is a pointer to the right pl330_irq_orgate[n])
    you wire up each input with
       for (i = 0; i < nevents + 1; i++) {
             sysbus_connect_irq(busdev, i,
                 qdev_get_gpio_in(DEVICE(this_orgate), i));
       }
     (nb: unlike your patch here I've rolled the abort irq line into
     the loop with the event irqs because it's all the same code)
   And you wire up the output with
      qdev_connect_gpio_out(DEVICE(this_orgate), 0,
                            irq_the_output_goes_to);

PS: this is using what I think of as "modern style", where the
sub-objects of a device object are inline in the device struct,
and the container device's initialize initializes them and the
container device's realize realizes them. It is also possible to
use the TYPE_OR_IRQ in the more old-style way where you call
the qdev_create() function which allocates memory for the object
(as the Exynos code is currently doing for the PL330 objects,
for instance) and then qdev_init_nofail() to initialize/realize
it, but I think 'modern style' is where we should be going as we
write new code.

thanks
-- PMM