| Message ID | 20200110203942.5745-3-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Exynos4210 DMA support | expand |
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. (Conversely, to connect one qemu_irq to multiple outputs you need a TYPE_SPLIT_IRQ, include/hw/core/split-irq.h). thanks -- PMM
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
On Fri, 17 Jan 2020 at 18:07, Guenter Roeck <linux@roeck-us.net> 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 <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. 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
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)
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(-)