diff mbox series

[v2,2/7] hw/arm/exynos4210: Fix DMA initialization

Message ID 20200118164229.22539-3-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show
Series Fix Exynos4210 DMA support | expand

Commit Message

Guenter Roeck Jan. 18, 2020, 4:42 p.m. UTC
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).

Reduce the number of DMA events to 30 for both pdma and mdma. QEMU's OR
interrupt gates are currently limited to less than 32, and we would need
33 gates to support 32 event interrupts plus the abort interrupt.
Operationally this should not make a difference since they are all
routed to a single interrupt line anyway.

Fixes: 59520dc65e ("hw/arm/exynos4210: Add DMA support for the Exynos4210")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Use interrupt combiner instead of connecting all events to a
    single interrupt. Limit number of events per DMA channel
    to 31 to meet qemu interrupt combiner limitations.
    [Not sure if "assert(s->num_lines < MAX_OR_LINES);" should be
     "assert(s->num_lines <= MAX_OR_LINES);"]
    Introduce exynos4210_init() to handle interrupt combiner
    initialization.

 hw/arm/exynos4210.c         | 51 +++++++++++++++++++++++++++++++------
 include/hw/arm/exynos4210.h |  4 +++
 2 files changed, 47 insertions(+), 8 deletions(-)

Comments

Peter Maydell Jan. 20, 2020, 1:35 p.m. UTC | #1
On Sat, 18 Jan 2020 at 16:42, 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).
>
> Reduce the number of DMA events to 30 for both pdma and mdma. QEMU's OR
> interrupt gates are currently limited to less than 32, and we would need
> 33 gates to support 32 event interrupts plus the abort interrupt.
> Operationally this should not make a difference since they are all
> routed to a single interrupt line anyway.
>
> Fixes: 59520dc65e ("hw/arm/exynos4210: Add DMA support for the Exynos4210")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Use interrupt combiner instead of connecting all events to a
>     single interrupt. Limit number of events per DMA channel
>     to 31 to meet qemu interrupt combiner limitations.
>     [Not sure if "assert(s->num_lines < MAX_OR_LINES);" should be
>      "assert(s->num_lines <= MAX_OR_LINES);"]

Yes, that looks like a bug in or-irq.c -- it should be using <=,
so 32 is permissible.

As the comment in or-irq.h notes, we can safely simply bump the
#define value without breaking anything if you need more input
OR lines than 32.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Guenter Roeck Jan. 20, 2020, 2:30 p.m. UTC | #2
On 1/20/20 5:35 AM, Peter Maydell wrote:
> On Sat, 18 Jan 2020 at 16:42, Guenter Roeck <linux@roeck-us.net> wrote:
[ ... ]
>> v2: Use interrupt combiner instead of connecting all events to a
>>      single interrupt. Limit number of events per DMA channel
>>      to 31 to meet qemu interrupt combiner limitations.
>>      [Not sure if "assert(s->num_lines < MAX_OR_LINES);" should be
>>       "assert(s->num_lines <= MAX_OR_LINES);"]
> 
> Yes, that looks like a bug in or-irq.c -- it should be using <=,
> so 32 is permissible.
> 
> As the comment in or-irq.h notes, we can safely simply bump the
> #define value without breaking anything if you need more input
> OR lines than 32.
> 

Yes, I noticed the comment, and I did that initially, but then
I noticed the complexity of actually doing it in the code
increasing it from 16 to 32, and decided I better leave it alone.
I'll add another patch fixing the check and use 32.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 

Thanks,
Guenter
Peter Maydell Jan. 20, 2020, 2:46 p.m. UTC | #3
On Mon, 20 Jan 2020 at 14:30, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/20/20 5:35 AM, Peter Maydell wrote:
> > As the comment in or-irq.h notes, we can safely simply bump the
> > #define value without breaking anything if you need more input
> > OR lines than 32.
> >
>
> Yes, I noticed the comment, and I did that initially, but then
> I noticed the complexity of actually doing it in the code
> increasing it from 16 to 32, and decided I better leave it alone.

Yeah, the conversion from 16 to 32 was hairy because our
initial implementation made the migration-compatibility
awkward. When I wrote that conversion I was careful to
avoid creating a similar problem for my future self if
I needed to bump the value again :-)

> I'll add another patch fixing the check and use 32.

I just sent a patch that fixes the check.

thanks
-- PMM
Guenter Roeck Jan. 20, 2020, 3:11 p.m. UTC | #4
On 1/20/20 6:46 AM, Peter Maydell wrote:
> On Mon, 20 Jan 2020 at 14:30, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 1/20/20 5:35 AM, Peter Maydell wrote:
>>> As the comment in or-irq.h notes, we can safely simply bump the
>>> #define value without breaking anything if you need more input
>>> OR lines than 32.
>>>
>>
>> Yes, I noticed the comment, and I did that initially, but then
>> I noticed the complexity of actually doing it in the code
>> increasing it from 16 to 32, and decided I better leave it alone.
> 
> Yeah, the conversion from 16 to 32 was hairy because our
> initial implementation made the migration-compatibility
> awkward. When I wrote that conversion I was careful to
> avoid creating a similar problem for my future self if
> I needed to bump the value again :-)
> 

Ok, with that in mind I'll add a patch increasing the limit.
I'll set it to 48. Then we can discuss if the new limit is
sufficient/acceptable ;-).

Thanks,
Guenter
diff mbox series

Patch

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 77fbe1baab..91586fe265 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -166,17 +166,36 @@  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_or_irq *orgate, 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);
+
+    object_property_set_int(OBJECT(orgate), nevents + 1, "num-lines",
+                            &error_abort);
+    object_property_set_bool(OBJECT(orgate), true, "realized", &error_abort);
+
+    for (i = 0; i < nevents + 1; i++) {
+        sysbus_connect_irq(busdev, i, qdev_get_gpio_in(DEVICE(orgate), i));
+    }
+    qdev_connect_gpio_out(DEVICE(orgate), 0, irq);
 }
 
 static void exynos4210_realize(DeviceState *socdev, Error **errp)
@@ -431,12 +450,27 @@  static void exynos4210_realize(DeviceState *socdev, Error **errp)
             s->irq_table[exynos4210_get_irq(28, 3)]);
 
     /*** DMA controllers ***/
-    pl330_create(EXYNOS4210_PL330_BASE0_ADDR,
-                 qemu_irq_invert(s->irq_table[exynos4210_get_irq(35, 1)]), 32);
-    pl330_create(EXYNOS4210_PL330_BASE1_ADDR,
-                 qemu_irq_invert(s->irq_table[exynos4210_get_irq(36, 1)]), 32);
-    pl330_create(EXYNOS4210_PL330_BASE2_ADDR,
-                 qemu_irq_invert(s->irq_table[exynos4210_get_irq(34, 1)]), 1);
+    pl330_create(EXYNOS4210_PL330_BASE0_ADDR, &s->pl330_irq_orgate[0],
+                 s->irq_table[exynos4210_get_irq(21, 0)], 32, 30, 32);
+    pl330_create(EXYNOS4210_PL330_BASE1_ADDR, &s->pl330_irq_orgate[1],
+                 s->irq_table[exynos4210_get_irq(21, 1)], 32, 30, 32);
+    pl330_create(EXYNOS4210_PL330_BASE2_ADDR, &s->pl330_irq_orgate[2],
+                 s->irq_table[exynos4210_get_irq(20, 1)], 1, 30, 64);
+}
+
+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);
+    }
 }
 
 static void exynos4210_class_init(ObjectClass *klass, void *data)
@@ -450,6 +484,7 @@  static const TypeInfo exynos4210_info = {
     .name = TYPE_EXYNOS4210_SOC,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(Exynos4210State),
+    .instance_init = exynos4210_init,
     .class_init = exynos4210_class_init,
 };
 
diff --git a/include/hw/arm/exynos4210.h b/include/hw/arm/exynos4210.h
index f0f23b0e9b..55260394af 100644
--- a/include/hw/arm/exynos4210.h
+++ b/include/hw/arm/exynos4210.h
@@ -24,6 +24,7 @@ 
 #ifndef EXYNOS4210_H
 #define EXYNOS4210_H
 
+#include "hw/or-irq.h"
 #include "hw/sysbus.h"
 #include "target/arm/cpu-qom.h"
 
@@ -74,6 +75,8 @@ 
 
 #define EXYNOS4210_I2C_NUMBER               9
 
+#define EXYNOS4210_NUM_DMA      3
+
 typedef struct Exynos4210Irq {
     qemu_irq int_combiner_irq[EXYNOS4210_MAX_INT_COMBINER_IN_IRQ];
     qemu_irq ext_combiner_irq[EXYNOS4210_MAX_EXT_COMBINER_IN_IRQ];
@@ -97,6 +100,7 @@  typedef struct Exynos4210State {
     MemoryRegion boot_secondary;
     MemoryRegion bootreg_mem;
     I2CBus *i2c_if[EXYNOS4210_I2C_NUMBER];
+    qemu_or_irq pl330_irq_orgate[EXYNOS4210_NUM_DMA];
 } Exynos4210State;
 
 #define TYPE_EXYNOS4210_SOC "exynos4210"