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

2020-01-17 Thread Peter Maydell
On Fri, 17 Jan 2020 at 18:07, Guenter Roeck  wrote:
>
> Hi Peter,
>
> On Fri, Jan 17, 2020 at 01:30:19PM +, Peter Maydell wrote:
> > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  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 
> > > ---
> > >  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 = >pl330_irq_orgate[i];

  object_initialize_child(obj, name, orgate, sizeof(*orgate),
  TYPE_OR_IRQ, _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(>pl330_irq_orgate[i]),
num_lines,
"num-lines", );
if (err) {
error_propagate(errp, err);
return;
}
object_property_set_bool(OBJECT(>pl330_irq_orgate[i]), true,
 "realized", );
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 

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

2020-01-17 Thread Guenter Roeck
Hi Peter,

On Fri, Jan 17, 2020 at 01:30:19PM +, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  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 
> > ---
> >  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



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

2020-01-17 Thread Peter Maydell
On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  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 
> ---
>  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



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

2020-01-10 Thread Guenter Roeck
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 
---
 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 */
+}
 }
 
 static void exynos4210_realize(DeviceState *socdev, Error **errp)
@@ -432,11 +446,11 @@ static void exynos4210_realize(DeviceState *socdev, Error 
**errp)
 
 /*** DMA controllers ***/
 pl330_create(EXYNOS4210_PL330_BASE0_ADDR,
- qemu_irq_invert(s->irq_table[exynos4210_get_irq(35, 1)]), 32);
+ s->irq_table[exynos4210_get_irq(21, 0)], 32, 32, 32);
 pl330_create(EXYNOS4210_PL330_BASE1_ADDR,
- qemu_irq_invert(s->irq_table[exynos4210_get_irq(36, 1)]), 32);
+ s->irq_table[exynos4210_get_irq(21, 1)], 32, 32, 32);
 pl330_create(EXYNOS4210_PL330_BASE2_ADDR,
- qemu_irq_invert(s->irq_table[exynos4210_get_irq(34, 1)]), 1);
+ s->irq_table[exynos4210_get_irq(20, 1)], 1, 31, 64);
 }
 
 static void exynos4210_class_init(ObjectClass *klass, void *data)
-- 
2.17.1