[PATCH 0/6] Fix Exynos4210 DMA support

[PATCH 0/6] Fix Exynos4210 DMA support

[Guenter Roeck]

Commit 59520dc65e ("hw/arm/exynos4210: Add DMA support for the Exynos4210")
introduced DMA support for Exynos4210. Unfortunately, it never really
worked. DMA interrupt line and polarity was wrong, and the serial port
needs extra code to support DMA. This patch series fixes the problem.

[PATCH 1/6] dma/pl330: Convert to support tracing

[Guenter Roeck]

Replace debug logging code with tracing.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/dma/pl330.c      | 88 +++++++++++++++++++++++----------------------
 hw/dma/trace-events | 24 +++++++++++++
 2 files changed, 70 insertions(+), 42 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index f2bb2d9ac1..c64cc4e67a 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -25,19 +25,12 @@
 #include "sysemu/dma.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 #ifndef PL330_ERR_DEBUG
 #define PL330_ERR_DEBUG 0
 #endif
 
-#define DB_PRINT_L(lvl, fmt, args...) do {\
-    if (PL330_ERR_DEBUG >= lvl) {\
-        fprintf(stderr, "PL330: %s:" fmt, __func__, ## args);\
-    } \
-} while (0)
-
-#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
-
 #define PL330_PERIPH_NUM            32
 #define PL330_MAX_BURST_LEN         128
 #define PL330_INSN_MAXSIZE          6
@@ -319,6 +312,26 @@ typedef struct PL330InsnDesc {
     void (*exec)(PL330Chan *, uint8_t opcode, uint8_t *args, int len);
 } PL330InsnDesc;
 
+static void pl330_hexdump(uint8_t *buf, size_t size)
+{
+    unsigned int b, i, len;
+    char tmpbuf[80];
+
+    for (b = 0; b < size; b += 16) {
+        len = size - b;
+        if (len > 16) {
+            len = 16;
+        }
+        tmpbuf[0] = '\0';
+        for (i = 0; i < len; i++) {
+            if ((i % 4) == 0) {
+                strcat(tmpbuf, " ");
+            }
+            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
+        }
+        trace_pl330_hexdump(b, tmpbuf);
+    }
+}
 
 /* MFIFO Implementation
  *
@@ -582,7 +595,7 @@ static inline void pl330_queue_remove_tagged(PL330Queue *s, uint8_t tag)
 
 static inline void pl330_fault(PL330Chan *ch, uint32_t flags)
 {
-    DB_PRINT("ch: %p, flags: %" PRIx32 "\n", ch, flags);
+    trace_pl330_fault(ch, flags);
     ch->fault_type |= flags;
     if (ch->state == pl330_chan_fault) {
         return;
@@ -590,7 +603,7 @@ static inline void pl330_fault(PL330Chan *ch, uint32_t flags)
     ch->state = pl330_chan_fault;
     ch->parent->num_faulting++;
     if (ch->parent->num_faulting == 1) {
-        DB_PRINT("abort interrupt raised\n");
+        trace_pl330_fault_abort();
         qemu_irq_raise(ch->parent->irq_abort);
     }
 }
@@ -648,7 +661,7 @@ static void pl330_dmaend(PL330Chan *ch, uint8_t opcode,
             return;
         }
     }
-    DB_PRINT("DMA ending!\n");
+    trace_pl330_dmaend();
     pl330_fifo_tagged_remove(&s->fifo, ch->tag);
     pl330_queue_remove_tagged(&s->read_queue, ch->tag);
     pl330_queue_remove_tagged(&s->write_queue, ch->tag);
@@ -683,7 +696,7 @@ static void pl330_dmago(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
     uint32_t pc;
     PL330Chan *s;
 
-    DB_PRINT("\n");
+    trace_pl330_dmago();
 
     if (!ch->is_manager) {
         pl330_fault(ch, PL330_FAULT_UNDEF_INSTR);
@@ -740,9 +753,7 @@ static void pl330_dmald(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
     ch->stall = pl330_queue_put_insn(&ch->parent->read_queue, ch->src,
                                     size, num, inc, 0, ch->tag);
     if (!ch->stall) {
-        DB_PRINT("channel:%" PRId8 " address:%08" PRIx32 " size:%" PRIx32
-                 " num:%" PRId32 " %c\n",
-                 ch->tag, ch->src, size, num, inc ? 'Y' : 'N');
+        trace_pl330_dmald(ch->tag, ch->src, size, num, inc ? 'Y' : 'N');
         ch->src += inc ? size * num - (ch->src & (size - 1)) : 0;
     }
 }
@@ -782,7 +793,7 @@ static void pl330_dmakill(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
         ch->fault_type = 0;
         ch->parent->num_faulting--;
         if (ch->parent->num_faulting == 0) {
-            DB_PRINT("abort interrupt lowered\n");
+            trace_pl330_dmakill();
             qemu_irq_lower(ch->parent->irq_abort);
         }
     }
@@ -800,6 +811,8 @@ static void pl330_dmalpend(PL330Chan *ch, uint8_t opcode,
     uint8_t bs = opcode & 3;
     uint8_t lc = (opcode & 4) >> 2;
 
+    trace_pl330_dmalpend(nf, bs, lc, ch->lc[lc], ch->request_flag);
+
     if (bs == 2) {
         pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
         return;
@@ -813,12 +826,12 @@ static void pl330_dmalpend(PL330Chan *ch, uint8_t opcode,
         if (nf) {
             ch->lc[lc]--;
         }
-        DB_PRINT("loop reiteration\n");
+        trace_pl330_dmalpiter();
         ch->pc -= args[0];
         ch->pc -= len + 1;
         /* "ch->pc -= args[0] + len + 1" is incorrect when args[0] == 256 */
     } else {
-        DB_PRINT("loop fallthrough\n");
+        trace_pl330_dmalpfallthrough();
     }
 }
 
@@ -886,10 +899,10 @@ static void pl330_dmasev(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
     }
     if (ch->parent->inten & (1 << ev_id)) {
         ch->parent->int_status |= (1 << ev_id);
-        DB_PRINT("event interrupt raised %" PRId8 "\n", ev_id);
+        trace_pl330_dmasev_evirq(ev_id);
         qemu_irq_raise(ch->parent->irq[ev_id]);
     }
-    DB_PRINT("event raised %" PRId8 "\n", ev_id);
+    trace_pl330_dmasev_event(ev_id);
     ch->parent->ev_status |= (1 << ev_id);
 }
 
@@ -914,9 +927,7 @@ static void pl330_dmast(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
     ch->stall = pl330_queue_put_insn(&ch->parent->write_queue, ch->dst,
                                     size, num, inc, 0, ch->tag);
     if (!ch->stall) {
-        DB_PRINT("channel:%" PRId8 " address:%08" PRIx32 " size:%" PRIx32
-                 " num:%" PRId32 " %c\n",
-                 ch->tag, ch->dst, size, num, inc ? 'Y' : 'N');
+        trace_pl330_dmast(ch->tag, ch->dst, size, num, inc ? 'Y' : 'N');
         ch->dst += inc ? size * num - (ch->dst & (size - 1)) : 0;
     }
 }
@@ -992,7 +1003,7 @@ static void pl330_dmawfe(PL330Chan *ch, uint8_t opcode,
             }
         }
         ch->parent->ev_status &= ~(1 << ev_id);
-        DB_PRINT("event lowered %" PRIx8 "\n", ev_id);
+        trace_pl330_dmawfe(ev_id);
     } else {
         ch->stall = 1;
     }
@@ -1135,7 +1146,7 @@ static int pl330_chan_exec(PL330Chan *ch)
     ch->stall = 0;
     insn = pl330_fetch_insn(ch);
     if (!insn) {
-        DB_PRINT("pl330 undefined instruction\n");
+        trace_pl330_chan_exec_undef();
         pl330_fault(ch, PL330_FAULT_UNDEF_INSTR);
         return 0;
     }
@@ -1175,11 +1186,8 @@ static int pl330_exec_cycle(PL330Chan *channel)
         int len = q->len - (q->addr & (q->len - 1));
 
         dma_memory_read(&address_space_memory, q->addr, buf, len);
-        if (PL330_ERR_DEBUG > 1) {
-            DB_PRINT("PL330 read from memory @%08" PRIx32 " (size = %08x):\n",
-                      q->addr, len);
-            qemu_hexdump((char *)buf, stderr, "", len);
-        }
+        trace_pl330_exec_cycle(q->addr, len);
+        pl330_hexdump(buf, len);
         fifo_res = pl330_fifo_push(&s->fifo, buf, len, q->tag);
         if (fifo_res == PL330_FIFO_OK) {
             if (q->inc) {
@@ -1207,11 +1215,8 @@ static int pl330_exec_cycle(PL330Chan *channel)
         }
         if (fifo_res == PL330_FIFO_OK || q->z) {
             dma_memory_write(&address_space_memory, q->addr, buf, len);
-            if (PL330_ERR_DEBUG > 1) {
-                DB_PRINT("PL330 read from memory @%08" PRIx32
-                         " (size = %08x):\n", q->addr, len);
-                qemu_hexdump((char *)buf, stderr, "", len);
-            }
+            trace_pl330_exec_cycle(q->addr, len);
+            pl330_hexdump(buf, len);
             if (q->inc) {
                 q->addr += len;
             }
@@ -1252,8 +1257,8 @@ static int pl330_exec_channel(PL330Chan *channel)
 
 static inline void pl330_exec(PL330State *s)
 {
-    DB_PRINT("\n");
     int i, insr_exec;
+    trace_pl330_exec();
     do {
         insr_exec = pl330_exec_channel(&s->manager);
 
@@ -1298,7 +1303,7 @@ static void pl330_debug_exec(PL330State *s)
     args[2] = (s->dbg[1] >>  8) & 0xff;
     args[3] = (s->dbg[1] >> 16) & 0xff;
     args[4] = (s->dbg[1] >> 24) & 0xff;
-    DB_PRINT("chan id: %" PRIx8 "\n", chan_id);
+    trace_pl330_debug_exec(chan_id);
     if (s->dbg[0] & 1) {
         ch = &s->chan[chan_id];
     } else {
@@ -1320,6 +1325,7 @@ static void pl330_debug_exec(PL330State *s)
         ch->fault_type |= PL330_FAULT_DBG_INSTR;
     }
     if (ch->stall) {
+        trace_pl330_debug_exec_stall();
         qemu_log_mask(LOG_UNIMP, "pl330: stall of debug instruction not "
                       "implemented\n");
     }
@@ -1334,7 +1340,7 @@ static void pl330_iomem_write(void *opaque, hwaddr offset,
     PL330State *s = (PL330State *) opaque;
     int i;
 
-    DB_PRINT("addr: %08x data: %08x\n", (unsigned)offset, (unsigned)value);
+    trace_pl330_iomem_write((unsigned)offset, (unsigned)value);
 
     switch (offset) {
     case PL330_REG_INTEN:
@@ -1343,7 +1349,7 @@ static void pl330_iomem_write(void *opaque, hwaddr offset,
     case PL330_REG_INTCLR:
         for (i = 0; i < s->num_events; i++) {
             if (s->int_status & s->inten & value & (1 << i)) {
-                DB_PRINT("event interrupt lowered %d\n", i);
+                trace_pl330_iomem_write_clr(i);
                 qemu_irq_lower(s->irq[i]);
             }
         }
@@ -1361,11 +1367,9 @@ static void pl330_iomem_write(void *opaque, hwaddr offset,
         }
         break;
     case PL330_REG_DBGINST0:
-        DB_PRINT("s->dbg[0] = %08x\n", (unsigned)value);
         s->dbg[0] = value;
         break;
     case PL330_REG_DBGINST1:
-        DB_PRINT("s->dbg[1] = %08x\n", (unsigned)value);
         s->dbg[1] = value;
         break;
     default:
@@ -1489,7 +1493,7 @@ static uint64_t pl330_iomem_read(void *opaque, hwaddr offset,
         unsigned size)
 {
     uint32_t ret = pl330_iomem_read_imp(opaque, offset);
-    DB_PRINT("addr: %08" HWADDR_PRIx " data: %08" PRIx32 "\n", offset, ret);
+    trace_pl330_iomem_read((uint32_t)offset, ret);
     return ret;
 }
 
diff --git a/hw/dma/trace-events b/hw/dma/trace-events
index e4498428c5..5902ac5969 100644
--- a/hw/dma/trace-events
+++ b/hw/dma/trace-events
@@ -20,3 +20,27 @@ sparc32_dma_enable_lower(void) "Lower DMA enable"
 
 # i8257.c
 i8257_unregistered_dma(int nchan, int dma_pos, int dma_len) "unregistered DMA channel used nchan=%d dma_pos=%d dma_len=%d"
+
+# pl330.c
+pl330_fault(void *ptr, uint32_t flags) "ch: %p, flags: 0x%"PRIx32
+pl330_fault_abort(void) "abort interrupt raised"
+pl330_dmaend(void) "DMA ending"
+pl330_dmago(void) "DMA run"
+pl330_dmald(uint32_t chan, uint32_t addr, uint32_t size, uint32_t num, uint32_t ch) "channel:%"PRId8" address:0x%08"PRIx32" size:0x%"PRIx32" num:%"PRId32"%c"
+pl330_dmakill(void) "abort interrupt lowered"
+pl330_dmalpend(uint8_t nf, uint8_t bs, uint8_t lc, uint8_t ch, uint8_t flag) "nf=0x%02x bs=0x%02x lc=0x%02x ch=0x%02x flag=0x%02x"
+pl330_dmalpiter(void) "loop reiteration"
+pl330_dmalpfallthrough(void) "loop fallthrough"
+pl330_dmasev_evirq(uint8_t ev_id) "event interrupt raised %"PRId8
+pl330_dmasev_event(uint8_t ev_id) "event raised %"PRId8
+pl330_dmast(uint32_t chn, uint32_t addr, uint32_t sz, uint32_t num, uint32_t c) "channel:%"PRId8" address:0x%08"PRIx32" size:0x%"PRIx32" num:%"PRId32" %c"
+pl330_dmawfe(uint8_t ev_id) "event lowered 0x%"PRIx8
+pl330_chan_exec_undef(void) "undefined instruction"
+pl330_exec_cycle(uint32_t addr, uint32_t size) "PL330 read from memory @0x%08"PRIx32" (size = 0x%08"PRIx32")"
+pl330_hexdump(uint32_t offset, char *str) " 0x%04"PRIx32":%s"
+pl330_exec(void) "pl330_exec"
+pl330_debug_exec(uint8_t ch) "chan id: 0x%"PRIx8
+pl330_debug_exec_stall(void) "stall of debug instruction not implemented"
+pl330_iomem_write(uint32_t offset, uint32_t value) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
+pl330_iomem_write_clr(int i) "event interrupt lowered %d"
+pl330_iomem_read(uint32_t addr, uint32_t data) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
-- 
2.17.1

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Replace debug logging code with tracing.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/dma/pl330.c      | 88 +++++++++++++++++++++++----------------------
>  hw/dma/trace-events | 24 +++++++++++++

> +static void pl330_hexdump(uint8_t *buf, size_t size)
> +{
> +    unsigned int b, i, len;
> +    char tmpbuf[80];
> +
> +    for (b = 0; b < size; b += 16) {
> +        len = size - b;
> +        if (len > 16) {
> +            len = 16;
> +        }
> +        tmpbuf[0] = '\0';
> +        for (i = 0; i < len; i++) {
> +            if ((i % 4) == 0) {
> +                strcat(tmpbuf, " ");
> +            }
> +            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
> +        }
> +        trace_pl330_hexdump(b, tmpbuf);
> +    }
> +}
>

> @@ -1175,11 +1186,8 @@ static int pl330_exec_cycle(PL330Chan *channel)
>          int len = q->len - (q->addr & (q->len - 1));
>
>          dma_memory_read(&address_space_memory, q->addr, buf, len);
> -        if (PL330_ERR_DEBUG > 1) {
> -            DB_PRINT("PL330 read from memory @%08" PRIx32 " (size = %08x):\n",
> -                      q->addr, len);
> -            qemu_hexdump((char *)buf, stderr, "", len);
> -        }
> +        trace_pl330_exec_cycle(q->addr, len);
> +        pl330_hexdump(buf, len);

Won't this now do all the work of constructing the hexdump strings,
even if tracing is disabled ?

thanks
-- PMM

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

[Guenter Roeck]

On Fri, Jan 17, 2020 at 01:23:46PM +0000, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Replace debug logging code with tracing.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/dma/pl330.c      | 88 +++++++++++++++++++++++----------------------
> >  hw/dma/trace-events | 24 +++++++++++++
> 
> > +static void pl330_hexdump(uint8_t *buf, size_t size)
> > +{
> > +    unsigned int b, i, len;
> > +    char tmpbuf[80];
> > +
> > +    for (b = 0; b < size; b += 16) {
> > +        len = size - b;
> > +        if (len > 16) {
> > +            len = 16;
> > +        }
> > +        tmpbuf[0] = '\0';
> > +        for (i = 0; i < len; i++) {
> > +            if ((i % 4) == 0) {
> > +                strcat(tmpbuf, " ");
> > +            }
> > +            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
> > +        }
> > +        trace_pl330_hexdump(b, tmpbuf);
> > +    }
> > +}
> >
> 
> > @@ -1175,11 +1186,8 @@ static int pl330_exec_cycle(PL330Chan *channel)
> >          int len = q->len - (q->addr & (q->len - 1));
> >
> >          dma_memory_read(&address_space_memory, q->addr, buf, len);
> > -        if (PL330_ERR_DEBUG > 1) {
> > -            DB_PRINT("PL330 read from memory @%08" PRIx32 " (size = %08x):\n",
> > -                      q->addr, len);
> > -            qemu_hexdump((char *)buf, stderr, "", len);
> > -        }
> > +        trace_pl330_exec_cycle(q->addr, len);
> > +        pl330_hexdump(buf, len);
> 
> Won't this now do all the work of constructing the hexdump strings,
> even if tracing is disabled ?
> 
That is correct. Can I check
	if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
	    qemu_loglevel_mask(LOG_TRACE)) {
directly in pl330_hexdump(), or is there some other means to handle
this kind of situation ?

Thanks,
Guenter

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

[Peter Maydell]

On Fri, 17 Jan 2020 at 16:46, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Jan 17, 2020 at 01:23:46PM +0000, Peter Maydell wrote:
> > Won't this now do all the work of constructing the hexdump strings,
> > even if tracing is disabled ?
> >
> That is correct. Can I check
>         if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
>             qemu_loglevel_mask(LOG_TRACE)) {
> directly in pl330_hexdump(), or is there some other means to handle
> this kind of situation ?

It's not something I've had to do before.
docs/devel/tracing.txt says "just use the TRACE_FOO_ENABLED
macro", but looking at what it does that doesn't seem to check
the runtime state of the trace event, so maybe those docs are out
of date. Stefan, what's the current best way to guard expensive
computations used only for trace output ?

thanks
-- PMM

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

[Guenter Roeck]

On Fri, Jan 17, 2020 at 05:05:07PM +0000, Peter Maydell wrote:
> On Fri, 17 Jan 2020 at 16:46, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Fri, Jan 17, 2020 at 01:23:46PM +0000, Peter Maydell wrote:
> > > Won't this now do all the work of constructing the hexdump strings,
> > > even if tracing is disabled ?
> > >
> > That is correct. Can I check
> >         if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
> >             qemu_loglevel_mask(LOG_TRACE)) {
> > directly in pl330_hexdump(), or is there some other means to handle
> > this kind of situation ?
> 
> It's not something I've had to do before.
> docs/devel/tracing.txt says "just use the TRACE_FOO_ENABLED
> macro", but looking at what it does that doesn't seem to check
> the runtime state of the trace event, so maybe those docs are out
> of date. Stefan, what's the current best way to guard expensive
> computations used only for trace output ?
> 
trace_event_get_state_backends(TRACE_PL330_HEXDUMP), maybe ?

Thanks,
Guenter

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

[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 <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 */
+    }
 }
 
 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

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

[Peter Maydell]

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

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

[Guenter Roeck]

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

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

[Peter Maydell]

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

[PATCH 3/6] hw/char/exynos4210_uart: Convert to support tracing

[Guenter Roeck]

Replace debug code with tracing to aid debugging.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/char/exynos4210_uart.c | 96 ++++++++++++---------------------------
 hw/char/trace-events      | 17 +++++++
 2 files changed, 47 insertions(+), 66 deletions(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index d6b6b62366..fb7a3ebd09 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -31,45 +31,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 
-#undef DEBUG_UART
-#undef DEBUG_UART_EXTEND
-#undef DEBUG_IRQ
-#undef DEBUG_Rx_DATA
-#undef DEBUG_Tx_DATA
-
-#define DEBUG_UART            0
-#define DEBUG_UART_EXTEND     0
-#define DEBUG_IRQ             0
-#define DEBUG_Rx_DATA         0
-#define DEBUG_Tx_DATA         0
-
-#if DEBUG_UART
-#define  PRINT_DEBUG(fmt, args...)  \
-        do { \
-            fprintf(stderr, "  [%s:%d]   "fmt, __func__, __LINE__, ##args); \
-        } while (0)
-
-#if DEBUG_UART_EXTEND
-#define  PRINT_DEBUG_EXTEND(fmt, args...) \
-        do { \
-            fprintf(stderr, "  [%s:%d]   "fmt, __func__, __LINE__, ##args); \
-        } while (0)
-#else
-#define  PRINT_DEBUG_EXTEND(fmt, args...) \
-        do {} while (0)
-#endif /* EXTEND */
-
-#else
-#define  PRINT_DEBUG(fmt, args...)  \
-        do {} while (0)
-#define  PRINT_DEBUG_EXTEND(fmt, args...) \
-        do {} while (0)
-#endif
-
-#define  PRINT_ERROR(fmt, args...) \
-        do { \
-            fprintf(stderr, "  [%s:%d]   "fmt, __func__, __LINE__, ##args); \
-        } while (0)
+#include "trace.h"
 
 /*
  *  Offsets for UART registers relative to SFR base address
@@ -193,8 +155,7 @@ typedef struct Exynos4210UartState {
 } Exynos4210UartState;
 
 
-#if DEBUG_UART
-/* Used only for debugging inside PRINT_DEBUG_... macros */
+/* Used only for tracing */
 static const char *exynos4210_uart_regname(hwaddr  offset)
 {
 
@@ -208,7 +169,6 @@ static const char *exynos4210_uart_regname(hwaddr  offset)
 
     return NULL;
 }
-#endif
 
 
 static void fifo_store(Exynos4210UartFIFO *q, uint8_t ch)
@@ -271,7 +231,7 @@ static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState
         break;
     default:
         level = 0;
-        PRINT_ERROR("Wrong UART channel number: %d\n", s->channel);
+        trace_exynos_uart_channel_error(s->channel);
     }
 
     return level;
@@ -297,14 +257,10 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
 
     if (s->reg[I_(UINTP)]) {
         qemu_irq_raise(s->irq);
-
-#if DEBUG_IRQ
-        fprintf(stderr, "UART%d: IRQ has been raised: %08x\n",
-                s->channel, s->reg[I_(UINTP)]);
-#endif
-
+        trace_exynos_uart_irq_raised(s->channel, s->reg[I_(UINTP)]);
     } else {
         qemu_irq_lower(s->irq);
+        trace_exynos_uart_irq_lowered(s->channel);
     }
 }
 
@@ -348,7 +304,7 @@ static void exynos4210_uart_update_parameters(Exynos4210UartState *s)
 
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
 
-    PRINT_DEBUG("UART%d: speed: %d, parity: %c, data: %d, stop: %d\n",
+    trace_exynos_uart_update_params(
                 s->channel, speed, parity, data_bits, stop_bits);
 }
 
@@ -358,8 +314,8 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
     Exynos4210UartState *s = (Exynos4210UartState *)opaque;
     uint8_t ch;
 
-    PRINT_DEBUG_EXTEND("UART%d: <0x%04x> %s <- 0x%08llx\n", s->channel,
-        offset, exynos4210_uart_regname(offset), (long long unsigned int)val);
+    trace_exynos_uart_write(s->channel, offset,
+                            exynos4210_uart_regname(offset), val);
 
     switch (offset) {
     case ULCON:
@@ -373,12 +329,12 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
         if (val & UFCON_Rx_FIFO_RESET) {
             fifo_reset(&s->rx);
             s->reg[I_(UFCON)] &= ~UFCON_Rx_FIFO_RESET;
-            PRINT_DEBUG("UART%d: Rx FIFO Reset\n", s->channel);
+            trace_exynos_uart_rx_fifo_reset(s->channel);
         }
         if (val & UFCON_Tx_FIFO_RESET) {
             fifo_reset(&s->tx);
             s->reg[I_(UFCON)] &= ~UFCON_Tx_FIFO_RESET;
-            PRINT_DEBUG("UART%d: Tx FIFO Reset\n", s->channel);
+            trace_exynos_uart_tx_fifo_reset(s->channel);
         }
         break;
 
@@ -390,9 +346,7 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
             /* XXX this blocks entire thread. Rewrite to use
              * qemu_chr_fe_write and background I/O callbacks */
             qemu_chr_fe_write_all(&s->chr, &ch, 1);
-#if DEBUG_Tx_DATA
-            fprintf(stderr, "%c", ch);
-#endif
+            trace_exynos_uart_tx(s->channel, ch);
             s->reg[I_(UTRSTAT)] |= UTRSTAT_TRANSMITTER_EMPTY |
                     UTRSTAT_Tx_BUFFER_EMPTY;
             s->reg[I_(UINTSP)]  |= UINTSP_TXD;
@@ -403,8 +357,7 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
     case UINTP:
         s->reg[I_(UINTP)] &= ~val;
         s->reg[I_(UINTSP)] &= ~val;
-        PRINT_DEBUG("UART%d: UINTP [%04x] have been cleared: %08x\n",
-                    s->channel, offset, s->reg[I_(UINTP)]);
+        trace_exynos_uart_intclr(s->channel, s->reg[I_(UINTP)]);
         exynos4210_uart_update_irq(s);
         break;
     case UTRSTAT:
@@ -412,7 +365,7 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
     case UFSTAT:
     case UMSTAT:
     case URXH:
-        PRINT_DEBUG("UART%d: Trying to write into RO register: %s [%04x]\n",
+        trace_exynos_uart_ro_write(
                     s->channel, exynos4210_uart_regname(offset), offset);
         break;
     case UINTSP:
@@ -439,6 +392,8 @@ static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
     case UERSTAT: /* Read Only */
         res = s->reg[I_(UERSTAT)];
         s->reg[I_(UERSTAT)] = 0;
+        trace_exynos_uart_read(s->channel, offset,
+                               exynos4210_uart_regname(offset), res);
         return res;
     case UFSTAT: /* Read Only */
         s->reg[I_(UFSTAT)] = fifo_elements_number(&s->rx) & 0xff;
@@ -446,20 +401,22 @@ static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
             s->reg[I_(UFSTAT)] |= UFSTAT_Rx_FIFO_FULL;
             s->reg[I_(UFSTAT)] &= ~0xff;
         }
+        trace_exynos_uart_read(s->channel, offset,
+                               exynos4210_uart_regname(offset),
+                               s->reg[I_(UFSTAT)]);
         return s->reg[I_(UFSTAT)];
     case URXH:
         if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
             if (fifo_elements_number(&s->rx)) {
                 res = fifo_retrieve(&s->rx);
-#if DEBUG_Rx_DATA
-                fprintf(stderr, "%c", res);
-#endif
+                trace_exynos_uart_rx(s->channel, res);
                 if (!fifo_elements_number(&s->rx)) {
                     s->reg[I_(UTRSTAT)] &= ~UTRSTAT_Rx_BUFFER_DATA_READY;
                 } else {
                     s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
                 }
             } else {
+                trace_exynos_uart_rx_error(s->channel);
                 s->reg[I_(UINTSP)] |= UINTSP_ERROR;
                 exynos4210_uart_update_irq(s);
                 res = 0;
@@ -468,15 +425,22 @@ static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
             s->reg[I_(UTRSTAT)] &= ~UTRSTAT_Rx_BUFFER_DATA_READY;
             res = s->reg[I_(URXH)];
         }
+        trace_exynos_uart_read(s->channel, offset,
+                               exynos4210_uart_regname(offset), res);
         return res;
     case UTXH:
-        PRINT_DEBUG("UART%d: Trying to read from WO register: %s [%04x]\n",
-                    s->channel, exynos4210_uart_regname(offset), offset);
+        trace_exynos_uart_wo_read(s->channel, exynos4210_uart_regname(offset),
+                                  offset);
         break;
     default:
+        trace_exynos_uart_read(s->channel, offset,
+                               exynos4210_uart_regname(offset),
+                               s->reg[I_(offset)]);
         return s->reg[I_(offset)];
     }
 
+    trace_exynos_uart_read(s->channel, offset, exynos4210_uart_regname(offset),
+                           0);
     return 0;
 }
 
@@ -555,7 +519,7 @@ static void exynos4210_uart_reset(DeviceState *dev)
     fifo_reset(&s->rx);
     fifo_reset(&s->tx);
 
-    PRINT_DEBUG("UART%d: Rx FIFO size: %d\n", s->channel, s->rx.size);
+    trace_exynos_uart_rxsize(s->channel, s->rx.size);
 }
 
 static const VMStateDescription vmstate_exynos4210_uart_fifo = {
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 2ce7f2f998..ba28b45b53 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -77,3 +77,20 @@ cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
 # nrf51_uart.c
 nrf51_uart_read(uint64_t addr, uint64_t r, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
 nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
+
+# exynos4210_uart.c
+exynos_uart_irq_raised(uint32_t channel, uint32_t reg) "UART%d: IRQ raised: 0x%08"PRIx32
+exynos_uart_irq_lowered(uint32_t channel) "UART%d: IRQ lowered"
+exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d"
+exynos_uart_write(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s <- 0x%" PRIx64
+exynos_uart_read(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s -> 0x%" PRIx64
+exynos_uart_rx_fifo_reset(uint32_t channel) "UART%d: Rx FIFO Reset"
+exynos_uart_tx_fifo_reset(uint32_t channel) "UART%d: Tx FIFO Reset"
+exynos_uart_tx(uint32_t channel, uint8_t ch) "UART%d: Tx 0x%02"PRIx32
+exynos_uart_intclr(uint32_t channel, uint32_t reg) "UART%d: interrupts cleared: 0x%08"PRIx32
+exynos_uart_ro_write(uint32_t channel, const char *name, uint32_t reg) "UART%d: Trying to write into RO register: %s [0x%04"PRIx32"]"
+exynos_uart_rx(uint32_t channel, uint8_t ch) "UART%d: Rx 0x%02"PRIx32
+exynos_uart_rx_error(uint32_t channel) "UART%d: Rx error"
+exynos_uart_wo_read(uint32_t channel, const char *name, uint32_t reg) "UART%d: Trying to read from WO register: %s [0x%04"PRIx32"]"
+exynos_uart_rxsize(uint32_t channel, uint32_t size) "UART%d: Rx FIFO size: %d"
+exynos_uart_channel_error(uint32_t channel) "Wrong UART channel number: %d"
-- 
2.17.1

Re: [PATCH 3/6] hw/char/exynos4210_uart: Convert to support tracing

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Replace debug code with tracing to aid debugging.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/char/exynos4210_uart.c | 96 ++++++++++++---------------------------
>  hw/char/trace-events      | 17 +++++++
>  2 files changed, 47 insertions(+), 66 deletions(-)

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

thanks
-- PMM

[PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/char/exynos4210_uart.c | 120 ++++++++++++++++++++++++++++++--------
 hw/char/trace-events      |   3 +-
 2 files changed, 97 insertions(+), 26 deletions(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index fb7a3ebd09..61134a7bdc 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -24,6 +24,7 @@
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "qemu/timer.h"
 #include "chardev/char-fe.h"
 #include "chardev/char-serial.h"
 
@@ -118,6 +119,7 @@ static const Exynos4210UartReg exynos4210_uart_regs[] = {
 #define ULCON_STOP_BIT_SHIFT  1
 
 /* UART Tx/Rx Status */
+#define UTRSTAT_Rx_TIMEOUT              0x8
 #define UTRSTAT_TRANSMITTER_EMPTY       0x4
 #define UTRSTAT_Tx_BUFFER_EMPTY         0x2
 #define UTRSTAT_Rx_BUFFER_DATA_READY    0x1
@@ -147,6 +149,9 @@ typedef struct Exynos4210UartState {
     Exynos4210UartFIFO   rx;
     Exynos4210UartFIFO   tx;
 
+    QEMUTimer *fifo_timeout_timer;
+    uint64_t wordtime;        /* word time in ns */
+
     CharBackend       chr;
     qemu_irq          irq;
 
@@ -209,15 +214,12 @@ static void fifo_reset(Exynos4210UartFIFO *q)
     q->rp = 0;
 }
 
-static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
+static uint32_t exynos4210_uart_FIFO_trigger_level(uint32_t channel,
+                                                   uint32_t reg)
 {
-    uint32_t level = 0;
-    uint32_t reg;
+    uint32_t level;
 
-    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
-            UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
-
-    switch (s->channel) {
+    switch (channel) {
     case 0:
         level = reg * 32;
         break;
@@ -231,12 +233,34 @@ static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState
         break;
     default:
         level = 0;
-        trace_exynos_uart_channel_error(s->channel);
+        trace_exynos_uart_channel_error(channel);
+        break;
     }
-
     return level;
 }
 
+static uint32_t
+exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
+{
+    uint32_t reg;
+
+    reg = (s->reg[I_(UFCON)] & UFCON_Tx_FIFO_TRIGGER_LEVEL) >>
+            UFCON_Tx_FIFO_TRIGGER_LEVEL_SHIFT;
+
+    return exynos4210_uart_FIFO_trigger_level(s->channel, reg);
+}
+
+static uint32_t
+exynos4210_uart_Rx_FIFO_trigger_level(const Exynos4210UartState *s)
+{
+    uint32_t reg;
+
+    reg = ((s->reg[I_(UFCON)] & UFCON_Rx_FIFO_TRIGGER_LEVEL) >>
+            UFCON_Rx_FIFO_TRIGGER_LEVEL_SHIFT) + 1;
+
+    return exynos4210_uart_FIFO_trigger_level(s->channel, reg);
+}
+
 static void exynos4210_uart_update_irq(Exynos4210UartState *s)
 {
     /*
@@ -244,13 +268,25 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
      * transmit FIFO is smaller than the trigger level.
      */
     if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
-
         uint32_t count = (s->reg[I_(UFSTAT)] & UFSTAT_Tx_FIFO_COUNT) >>
                 UFSTAT_Tx_FIFO_COUNT_SHIFT;
 
         if (count <= exynos4210_uart_Tx_FIFO_trigger_level(s)) {
             s->reg[I_(UINTSP)] |= UINTSP_TXD;
         }
+
+        /*
+         * Rx interrupt if trigger level is reached or if rx timeout
+         * interrupt is disabled and there is data in the receive buffer
+         */
+        count = fifo_elements_number(&s->rx);
+        if ((count && !(s->reg[I_(UCON)] & 0x80)) ||
+            count >= exynos4210_uart_Rx_FIFO_trigger_level(s)) {
+            s->reg[I_(UINTSP)] |= UINTSP_RXD;
+            timer_del(s->fifo_timeout_timer);
+        }
+    } else if (s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY) {
+        s->reg[I_(UINTSP)] |= UINTSP_RXD;
     }
 
     s->reg[I_(UINTP)] = s->reg[I_(UINTSP)] & ~s->reg[I_(UINTM)];
@@ -264,6 +300,21 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
     }
 }
 
+static void exynos4210_uart_timeout_int(void *opaque)
+{
+    Exynos4210UartState *s = opaque;
+
+    trace_exynos_uart_rx_timeout(s->channel, s->reg[I_(UTRSTAT)],
+                                 s->reg[I_(UINTSP)]);
+
+    if ((s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY) ||
+        (s->reg[I_(UCON)] & (1 << 11))) {
+        s->reg[I_(UINTSP)] |= UINTSP_RXD;
+        s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_TIMEOUT;
+        exynos4210_uart_update_irq(s);
+    }
+}
+
 static void exynos4210_uart_update_parameters(Exynos4210UartState *s)
 {
     int speed, parity, data_bits, stop_bits;
@@ -302,10 +353,24 @@ static void exynos4210_uart_update_parameters(Exynos4210UartState *s)
     ssp.data_bits = data_bits;
     ssp.stop_bits = stop_bits;
 
+    s->wordtime = NANOSECONDS_PER_SECOND * (data_bits + stop_bits + 1) / speed;
+
     qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
 
     trace_exynos_uart_update_params(
-                s->channel, speed, parity, data_bits, stop_bits);
+                s->channel, speed, parity, data_bits, stop_bits, s->wordtime);
+}
+
+static void exynos4210_uart_rx_timeout_set(Exynos4210UartState *s)
+{
+    if (s->reg[I_(UCON)] & 0x80) {
+        uint32_t timeout = ((s->reg[I_(UCON)] >> 12) & 0x0f) * s->wordtime;
+
+        timer_mod(s->fifo_timeout_timer,
+                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+    } else {
+        timer_del(s->fifo_timeout_timer);
+    }
 }
 
 static void exynos4210_uart_write(void *opaque, hwaddr offset,
@@ -361,6 +426,10 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
         exynos4210_uart_update_irq(s);
         break;
     case UTRSTAT:
+        if (val & UTRSTAT_Rx_TIMEOUT) {
+            s->reg[I_(UTRSTAT)] &= ~UTRSTAT_Rx_TIMEOUT;
+        }
+        break;
     case UERSTAT:
     case UFSTAT:
     case UMSTAT:
@@ -376,12 +445,17 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
         exynos4210_uart_update_irq(s);
         break;
     case UCON:
+        s->reg[I_(UCON)] = val;
+        exynos4210_uart_rx_timeout_set(s);
+        exynos4210_uart_update_irq(s);
+        break;
     case UMCON:
     default:
         s->reg[I_(offset)] = val;
         break;
     }
 }
+
 static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
                                   unsigned size)
 {
@@ -461,7 +535,6 @@ static int exynos4210_uart_can_receive(void *opaque)
     return fifo_empty_elements_number(&s->rx);
 }
 
-
 static void exynos4210_uart_receive(void *opaque, const uint8_t *buf, int size)
 {
     Exynos4210UartState *s = (Exynos4210UartState *)opaque;
@@ -469,24 +542,17 @@ static void exynos4210_uart_receive(void *opaque, const uint8_t *buf, int size)
 
     if (s->reg[I_(UFCON)] & UFCON_FIFO_ENABLE) {
         if (fifo_empty_elements_number(&s->rx) < size) {
-            for (i = 0; i < fifo_empty_elements_number(&s->rx); i++) {
-                fifo_store(&s->rx, buf[i]);
-            }
+            size = fifo_empty_elements_number(&s->rx);
             s->reg[I_(UINTSP)] |= UINTSP_ERROR;
-            s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
-        } else {
-            for (i = 0; i < size; i++) {
-                fifo_store(&s->rx, buf[i]);
-            }
-            s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
         }
-        /* XXX: Around here we maybe should check Rx trigger level */
-        s->reg[I_(UINTSP)] |= UINTSP_RXD;
+        for (i = 0; i < size; i++) {
+            fifo_store(&s->rx, buf[i]);
+        }
+        exynos4210_uart_rx_timeout_set(s);
     } else {
         s->reg[I_(URXH)] = buf[0];
-        s->reg[I_(UINTSP)] |= UINTSP_RXD;
-        s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
     }
+    s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_BUFFER_DATA_READY;
 
     exynos4210_uart_update_irq(s);
 }
@@ -578,6 +644,10 @@ static void exynos4210_uart_init(Object *obj)
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
     Exynos4210UartState *s = EXYNOS4210_UART(dev);
 
+    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                         exynos4210_uart_timeout_int, s);
+    s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
+
     /* memory mapping */
     memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
                           "exynos4210.uart", EXYNOS4210_UART_REGS_MEM_SIZE);
diff --git a/hw/char/trace-events b/hw/char/trace-events
index ba28b45b53..cb73fee6a9 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -81,7 +81,7 @@ nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PR
 # exynos4210_uart.c
 exynos_uart_irq_raised(uint32_t channel, uint32_t reg) "UART%d: IRQ raised: 0x%08"PRIx32
 exynos_uart_irq_lowered(uint32_t channel) "UART%d: IRQ lowered"
-exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d"
+exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop, uint64_t wordtime) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d wordtime: %"PRId64"ns"
 exynos_uart_write(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s <- 0x%" PRIx64
 exynos_uart_read(uint32_t channel, uint32_t offset, const char *name, uint64_t val) "UART%d: <0x%04x> %s -> 0x%" PRIx64
 exynos_uart_rx_fifo_reset(uint32_t channel) "UART%d: Rx FIFO Reset"
@@ -94,3 +94,4 @@ exynos_uart_rx_error(uint32_t channel) "UART%d: Rx error"
 exynos_uart_wo_read(uint32_t channel, const char *name, uint32_t reg) "UART%d: Trying to read from WO register: %s [0x%04"PRIx32"]"
 exynos_uart_rxsize(uint32_t channel, uint32_t size) "UART%d: Rx FIFO size: %d"
 exynos_uart_channel_error(uint32_t channel) "Wrong UART channel number: %d"
+exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d: Rx timeout stat=0x%x intsp=0x%x"
-- 
2.17.1

Re: [PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:

The subject just says "implement receive FIFO", but the
existing code clearly has an "Exynos4210UartFIFO   rx"
which it does some storing and retrieving data from.
Could the patch be more accurately described as something
like "Implement interrupts for rx FIFO level triggers and
timeouts" ?

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/char/exynos4210_uart.c | 120 ++++++++++++++++++++++++++++++--------
>  hw/char/trace-events      |   3 +-
>  2 files changed, 97 insertions(+), 26 deletions(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index fb7a3ebd09..61134a7bdc 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -24,6 +24,7 @@
>  #include "migration/vmstate.h"
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
> +#include "qemu/timer.h"
>  #include "chardev/char-fe.h"
>  #include "chardev/char-serial.h"
>
> @@ -118,6 +119,7 @@ static const Exynos4210UartReg exynos4210_uart_regs[] = {
>  #define ULCON_STOP_BIT_SHIFT  1
>
>  /* UART Tx/Rx Status */
> +#define UTRSTAT_Rx_TIMEOUT              0x8
>  #define UTRSTAT_TRANSMITTER_EMPTY       0x4
>  #define UTRSTAT_Tx_BUFFER_EMPTY         0x2
>  #define UTRSTAT_Rx_BUFFER_DATA_READY    0x1
> @@ -147,6 +149,9 @@ typedef struct Exynos4210UartState {
>      ";
>      Exynos4210UartFIFO   tx;
>
> +    QEMUTimer *fifo_timeout_timer;
> +    uint64_t wordtime;        /* word time in ns */

You need to do something on incoming migration to handle
the new fields. This probably looks like a .post_load function
that calculates the wordtime based on the register values
that have been set by the incoming migration and set the
QEMUTimer appropriately.

(Side note while I'm thinking about it: this device has
an "Exynos4210UartFIFO tx" but it never does anything
with it except call fifo_reset() on it. We don't migrate
it either, which is a bit of a bear trap for anybody who
adds code that uses it in future...)

thanks
-- PMM

Re: [PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

[Guenter Roeck]

Hi Peter,

On Fri, Jan 17, 2020 at 01:42:54PM +0000, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> The subject just says "implement receive FIFO", but the
> existing code clearly has an "Exynos4210UartFIFO   rx"
> which it does some storing and retrieving data from.
> Could the patch be more accurately described as something
> like "Implement interrupts for rx FIFO level triggers and
> timeouts" ?
> 
Sure, no problem.

> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/char/exynos4210_uart.c | 120 ++++++++++++++++++++++++++++++--------
> >  hw/char/trace-events      |   3 +-
> >  2 files changed, 97 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > index fb7a3ebd09..61134a7bdc 100644
> > --- a/hw/char/exynos4210_uart.c
> > +++ b/hw/char/exynos4210_uart.c
> > @@ -24,6 +24,7 @@
> >  #include "migration/vmstate.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> > +#include "qemu/timer.h"
> >  #include "chardev/char-fe.h"
> >  #include "chardev/char-serial.h"
> >
> > @@ -118,6 +119,7 @@ static const Exynos4210UartReg exynos4210_uart_regs[] = {
> >  #define ULCON_STOP_BIT_SHIFT  1
> >
> >  /* UART Tx/Rx Status */
> > +#define UTRSTAT_Rx_TIMEOUT              0x8
> >  #define UTRSTAT_TRANSMITTER_EMPTY       0x4
> >  #define UTRSTAT_Tx_BUFFER_EMPTY         0x2
> >  #define UTRSTAT_Rx_BUFFER_DATA_READY    0x1
> > @@ -147,6 +149,9 @@ typedef struct Exynos4210UartState {
> >      ";
> >      Exynos4210UartFIFO   tx;
> >
> > +    QEMUTimer *fifo_timeout_timer;
> > +    uint64_t wordtime;        /* word time in ns */
> 
> You need to do something on incoming migration to handle
> the new fields. This probably looks like a .post_load function
> that calculates the wordtime based on the register values
> that have been set by the incoming migration and set the
> QEMUTimer appropriately.
> 
Doesn't that mean that the .post_load function is missing even today,
and that it should call exynos4210_uart_update_parameters() ?

Thanks,
Guenter

Re: [PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

[Peter Maydell]

On Fri, 17 Jan 2020 at 18:21, Guenter Roeck <linux@roeck-us.net> wrote:
> Doesn't that mean that the .post_load function is missing even today,
> and that it should call exynos4210_uart_update_parameters() ?

Yes, it should, so that's an existing bug. (I think you'll only
notice an ill effect from that if you have wired the QEMU emulated
serial port through to a real host hardware serial port, though.)

thanks
-- PMM

[PATCH 5/6] hw/char/exynos4210_uart: Add receive DMA support

[Guenter Roeck]

To support receive DMA, we need to inform the DMA controller if receive data
is available. Otherwise the DMA controller keeps requesting data, causing
receive errors.

Implement this using an interrupt line. The instantiating code then needs
to connect the interrupt with the matching DMA controller GPIO pin.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/char/exynos4210_uart.c | 24 ++++++++++++++++++++++++
 hw/char/trace-events      |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 61134a7bdc..e23799f93b 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -154,6 +154,7 @@ typedef struct Exynos4210UartState {
 
     CharBackend       chr;
     qemu_irq          irq;
+    qemu_irq          dmairq;
 
     uint32_t channel;
 
@@ -261,6 +262,24 @@ exynos4210_uart_Rx_FIFO_trigger_level(const Exynos4210UartState *s)
     return exynos4210_uart_FIFO_trigger_level(s->channel, reg);
 }
 
+/*
+ * Update Rx DMA busy signal if Rx DMA is enabled. For simplicity,
+ * mark DMA as busy if DMA is enabled and the receive buffer is empty.
+ */
+static void exynos4210_uart_update_dmabusy(Exynos4210UartState *s)
+{
+    bool rx_dma_enabled = (s->reg[I_(UCON)] & 0x03) == 0x02;
+    uint32_t count = fifo_elements_number(&s->rx);
+
+    if (rx_dma_enabled && !count) {
+        qemu_irq_raise(s->dmairq);
+        trace_exynos_uart_dmabusy(s->channel);
+    } else {
+        qemu_irq_lower(s->dmairq);
+        trace_exynos_uart_dmaready(s->channel);
+    }
+}
+
 static void exynos4210_uart_update_irq(Exynos4210UartState *s)
 {
     /*
@@ -282,10 +301,12 @@ static void exynos4210_uart_update_irq(Exynos4210UartState *s)
         count = fifo_elements_number(&s->rx);
         if ((count && !(s->reg[I_(UCON)] & 0x80)) ||
             count >= exynos4210_uart_Rx_FIFO_trigger_level(s)) {
+            exynos4210_uart_update_dmabusy(s);
             s->reg[I_(UINTSP)] |= UINTSP_RXD;
             timer_del(s->fifo_timeout_timer);
         }
     } else if (s->reg[I_(UTRSTAT)] & UTRSTAT_Rx_BUFFER_DATA_READY) {
+        exynos4210_uart_update_dmabusy(s);
         s->reg[I_(UINTSP)] |= UINTSP_RXD;
     }
 
@@ -311,6 +332,7 @@ static void exynos4210_uart_timeout_int(void *opaque)
         (s->reg[I_(UCON)] & (1 << 11))) {
         s->reg[I_(UINTSP)] |= UINTSP_RXD;
         s->reg[I_(UTRSTAT)] |= UTRSTAT_Rx_TIMEOUT;
+        exynos4210_uart_update_dmabusy(s);
         exynos4210_uart_update_irq(s);
     }
 }
@@ -499,6 +521,7 @@ static uint64_t exynos4210_uart_read(void *opaque, hwaddr offset,
             s->reg[I_(UTRSTAT)] &= ~UTRSTAT_Rx_BUFFER_DATA_READY;
             res = s->reg[I_(URXH)];
         }
+        exynos4210_uart_update_dmabusy(s);
         trace_exynos_uart_read(s->channel, offset,
                                exynos4210_uart_regname(offset), res);
         return res;
@@ -654,6 +677,7 @@ static void exynos4210_uart_init(Object *obj)
     sysbus_init_mmio(dev, &s->iomem);
 
     sysbus_init_irq(dev, &s->irq);
+    sysbus_init_irq(dev, &s->dmairq);
 }
 
 static void exynos4210_uart_realize(DeviceState *dev, Error **errp)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index cb73fee6a9..6f938301d9 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -79,6 +79,8 @@ nrf51_uart_read(uint64_t addr, uint64_t r, unsigned int size) "addr 0x%" PRIx64
 nrf51_uart_write(uint64_t addr, uint64_t value, unsigned int size) "addr 0x%" PRIx64 " value 0x%" PRIx64 " size %u"
 
 # exynos4210_uart.c
+exynos_uart_dmabusy(uint32_t channel) "UART%d: DMA busy (Rx buffer empty)"
+exynos_uart_dmaready(uint32_t channel) "UART%d: DMA ready"
 exynos_uart_irq_raised(uint32_t channel, uint32_t reg) "UART%d: IRQ raised: 0x%08"PRIx32
 exynos_uart_irq_lowered(uint32_t channel) "UART%d: IRQ lowered"
 exynos_uart_update_params(uint32_t channel, int speed, uint8_t parity, int data, int stop, uint64_t wordtime) "UART%d: speed: %d, parity: %c, data bits: %d, stop bits: %d wordtime: %"PRId64"ns"
-- 
2.17.1

Re: [PATCH 5/6] hw/char/exynos4210_uart: Add receive DMA support

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
>
> To support receive DMA, we need to inform the DMA controller if receive data
> is available. Otherwise the DMA controller keeps requesting data, causing
> receive errors.
>
> Implement this using an interrupt line. The instantiating code then needs
> to connect the interrupt with the matching DMA controller GPIO pin.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

thanks
-- PMM

[PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Guenter Roeck]

The Exynos4210 serial driver uses an interrupt line to signal if receive
data is available. Connect that interrupt with the DMA controller's
'peripheral busy' gpio pin.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index c7b5c587b1..498d79ebb2 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -166,8 +166,8 @@ 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, int nevents,
-                         int width)
+static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq,
+                                 int nevents, int width)
 {
     SysBusDevice *busdev;
     DeviceState *dev;
@@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents,
     for (i = 0; i < nevents; i++) {
         sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
     }
+    return dev;
 }
 
 static void exynos4210_realize(DeviceState *socdev, Error **errp)
@@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
     MemoryRegion *system_mem = get_system_memory();
     qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
     SysBusDevice *busdev;
-    DeviceState *dev;
+    DeviceState *dev, *uart[4], *pl330[3];
     int i, n;
 
     for (n = 0; n < EXYNOS4210_NCPUS; n++) {
@@ -385,19 +386,19 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
 
 
     /*** UARTs ***/
-    exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR,
+    uart[0] = exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR,
                            EXYNOS4210_UART0_FIFO_SIZE, 0, serial_hd(0),
                   s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 0)]);
 
-    exynos4210_uart_create(EXYNOS4210_UART1_BASE_ADDR,
+    uart[1] = exynos4210_uart_create(EXYNOS4210_UART1_BASE_ADDR,
                            EXYNOS4210_UART1_FIFO_SIZE, 1, serial_hd(1),
                   s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 1)]);
 
-    exynos4210_uart_create(EXYNOS4210_UART2_BASE_ADDR,
+    uart[2] = exynos4210_uart_create(EXYNOS4210_UART2_BASE_ADDR,
                            EXYNOS4210_UART2_FIFO_SIZE, 2, serial_hd(2),
                   s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 2)]);
 
-    exynos4210_uart_create(EXYNOS4210_UART3_BASE_ADDR,
+    uart[3] = exynos4210_uart_create(EXYNOS4210_UART3_BASE_ADDR,
                            EXYNOS4210_UART3_FIFO_SIZE, 3, serial_hd(3),
                   s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 3)]);
 
@@ -445,12 +446,24 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
             s->irq_table[exynos4210_get_irq(28, 3)]);
 
     /*** DMA controllers ***/
-    pl330_create(EXYNOS4210_PL330_BASE0_ADDR,
-                 s->irq_table[exynos4210_get_irq(21, 0)], 32, 32, 32);
-    pl330_create(EXYNOS4210_PL330_BASE1_ADDR,
-                 s->irq_table[exynos4210_get_irq(21, 1)], 32, 32, 32);
-    pl330_create(EXYNOS4210_PL330_BASE2_ADDR,
-                 s->irq_table[exynos4210_get_irq(20, 1)], 1, 31, 64);
+    pl330[0] = pl330_create(EXYNOS4210_PL330_BASE0_ADDR,
+                            s->irq_table[exynos4210_get_irq(21, 0)],
+                            32, 32, 32);
+    pl330[1] = pl330_create(EXYNOS4210_PL330_BASE1_ADDR,
+                            s->irq_table[exynos4210_get_irq(21, 1)],
+                            32, 32, 32);
+    pl330[2] = pl330_create(EXYNOS4210_PL330_BASE2_ADDR,
+                            s->irq_table[exynos4210_get_irq(20, 1)],
+                            1, 31, 64);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(uart[0]), 1,
+                       qdev_get_gpio_in(pl330[0], 15));
+    sysbus_connect_irq(SYS_BUS_DEVICE(uart[1]), 1,
+                       qdev_get_gpio_in(pl330[1], 15));
+    sysbus_connect_irq(SYS_BUS_DEVICE(uart[2]), 1,
+                       qdev_get_gpio_in(pl330[0], 17));
+    sysbus_connect_irq(SYS_BUS_DEVICE(uart[3]), 1,
+                       qdev_get_gpio_in(pl330[1], 17));
 }
 
 static void exynos4210_class_init(ObjectClass *klass, void *data)
-- 
2.17.1

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Peter Maydell]

On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
>
> The Exynos4210 serial driver uses an interrupt line to signal if receive
> data is available. Connect that interrupt with the DMA controller's
> 'peripheral busy' gpio pin.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index c7b5c587b1..498d79ebb2 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -166,8 +166,8 @@ 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, int nevents,
> -                         int width)
> +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq,
> +                                 int nevents, int width)
>  {
>      SysBusDevice *busdev;
>      DeviceState *dev;
> @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents,
>      for (i = 0; i < nevents; i++) {
>          sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
>      }
> +    return dev;
>  }
>
>  static void exynos4210_realize(DeviceState *socdev, Error **errp)
> @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
>      MemoryRegion *system_mem = get_system_memory();
>      qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
>      SysBusDevice *busdev;
> -    DeviceState *dev;
> +    DeviceState *dev, *uart[4], *pl330[3];

Rather than having the uart and pl330 pointers be locals,
they should be fields in Exynos4210State. (Otherwise technically
we leak them, though this is unnoticeable in practice because there's
no way to destroy an Exynos4210State.)

Patch looks good otherwise.

thanks
-- PMM

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Guenter Roeck]

On Fri, Jan 17, 2020 at 01:48:06PM +0000, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > The Exynos4210 serial driver uses an interrupt line to signal if receive
> > data is available. Connect that interrupt with the DMA controller's
> > 'peripheral busy' gpio pin.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++-------------
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> > index c7b5c587b1..498d79ebb2 100644
> > --- a/hw/arm/exynos4210.c
> > +++ b/hw/arm/exynos4210.c
> > @@ -166,8 +166,8 @@ 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, int nevents,
> > -                         int width)
> > +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq,
> > +                                 int nevents, int width)
> >  {
> >      SysBusDevice *busdev;
> >      DeviceState *dev;
> > @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents,
> >      for (i = 0; i < nevents; i++) {
> >          sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
> >      }
> > +    return dev;
> >  }
> >
> >  static void exynos4210_realize(DeviceState *socdev, Error **errp)
> > @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
> >      MemoryRegion *system_mem = get_system_memory();
> >      qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
> >      SysBusDevice *busdev;
> > -    DeviceState *dev;
> > +    DeviceState *dev, *uart[4], *pl330[3];
> 
> Rather than having the uart and pl330 pointers be locals,
> they should be fields in Exynos4210State. (Otherwise technically
> we leak them, though this is unnoticeable in practice because there's
> no way to destroy an Exynos4210State.)
> 
Out of curiosity: Is that a new leak because they are now tied together,
or is it an existing leak ? I don't find many DeviceState entries
in xxxState structures, so find it difficult to determine if/when/why
there is such a leak.

Thanks,
Guenter

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Peter Maydell]

On Fri, 17 Jan 2020 at 18:29, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Jan 17, 2020 at 01:48:06PM +0000, Peter Maydell wrote:
> > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > The Exynos4210 serial driver uses an interrupt line to signal if receive
> > > data is available. Connect that interrupt with the DMA controller's
> > > 'peripheral busy' gpio pin.

> > Rather than having the uart and pl330 pointers be locals,
> > they should be fields in Exynos4210State. (Otherwise technically
> > we leak them, though this is unnoticeable in practice because there's
> > no way to destroy an Exynos4210State.)
> >
> Out of curiosity: Is that a new leak because they are now tied together,
> or is it an existing leak ? I don't find many DeviceState entries
> in xxxState structures, so find it difficult to determine if/when/why
> there is such a leak.

Yes, it's an existing leak, though it's more of a conceptual leak
than one that valgrind would actually complain about. (The object
isn't totally unreachable because there will be references to it
in the QOM tree. Keeping track of your child objects only becomes
really important if the device is hot-pluggable, because if you
can hot-unplug the device you get a real leak if it hasn't cleaned
up its child objects.)

Aside: I think this code is written this way because although it's
a container device it has been incorrectly written against
the pattern of a board model. Originally board models did not
have any "this is the board" struct that they could keep things
in, so the only way to write board model code that created,
configured and wired up devices was to have it do it in a
way that didn't keep track of the things it created once the
board init function returned. This code is part of an SoC
"container" device, so it does have a state struct that it
could use to hold either embedded device structs or simply
pointers to device objects, but the code is written like the old
board-model code. (These days even board models have a suitable
state struct they can use.)

include/hw/arm/armsse.h is an example of a device state struct
with a lot of embedded device state structs in it. (Not all device
state structs have names ending "State".)

thanks
-- PMM

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Guenter Roeck]

On 1/17/20 10:44 AM, Peter Maydell wrote:
> On Fri, 17 Jan 2020 at 18:29, Guenter Roeck <linux@roeck-us.net> wrote:
[ ... ]
>>> Rather than having the uart and pl330 pointers be locals,
>>> they should be fields in Exynos4210State. (Otherwise technically
>>> we leak them, though this is unnoticeable in practice because there's
>>> no way to destroy an Exynos4210State.)
>>>
>> Out of curiosity: Is that a new leak because they are now tied together,
>> or is it an existing leak ? I don't find many DeviceState entries
>> in xxxState structures, so find it difficult to determine if/when/why
>> there is such a leak.
> 
> Yes, it's an existing leak, though it's more of a conceptual leak

Do only the pointers have to be in Exynos4210State, or the entire
data structures ? In the armsse code it looks like it is the complete
data structures.

Also, it seems to me that this means that not only pl330 and uart states
are affected, but everything created with qdev_create(). If so, the entire
file needs a serious rework, not just its pl330 / uart initialization.
Am I missing something ?

Thanks,
Guenter

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Peter Maydell]

On Sat, 18 Jan 2020 at 15:08, Guenter Roeck <linux@roeck-us.net> wrote:
> Do only the pointers have to be in Exynos4210State, or the entire
> data structures ? In the armsse code it looks like it is the complete
> data structures.

Either works. Embedding the entire data structure is the more
"modern" approach, but we don't generally go to the effort of
converting from the older style to the newer.

> Also, it seems to me that this means that not only pl330 and uart states
> are affected, but everything created with qdev_create(). If so, the entire
> file needs a serious rework, not just its pl330 / uart initialization.
> Am I missing something ?

Yeah, all that stuff is broken, but don't feel you need to fix it.
You just brought the pl330 pointers to my attention specifically
by declaring locals in this patch, at which point it's just
as easy to put those pointers in the state struct where they
should be.

thanks
-- PMM

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Guenter Roeck]

On 1/18/20 12:02 PM, Peter Maydell wrote:
> On Sat, 18 Jan 2020 at 15:08, Guenter Roeck <linux@roeck-us.net> wrote:
>> Do only the pointers have to be in Exynos4210State, or the entire
>> data structures ? In the armsse code it looks like it is the complete
>> data structures.
> 
> Either works. Embedding the entire data structure is the more
> "modern" approach, but we don't generally go to the effort of
> converting from the older style to the newer.
> 
>> Also, it seems to me that this means that not only pl330 and uart states
>> are affected, but everything created with qdev_create(). If so, the entire
>> file needs a serious rework, not just its pl330 / uart initialization.
>> Am I missing something ?
> 
> Yeah, all that stuff is broken, but don't feel you need to fix it.
> You just brought the pl330 pointers to my attention specifically
> by declaring locals in this patch, at which point it's just
> as easy to put those pointers in the state struct where they
> should be.
> 

I'd rather try to fix it all if I am at it; otherwise it feels kind
of incomplete. Would you be ok with addressing this separately after
the current patch series is accepted ?

Thanks,
Guenter

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Peter Maydell]

On Sun, 19 Jan 2020 at 01:52, Guenter Roeck <linux@roeck-us.net> wrote:
> I'd rather try to fix it all if I am at it; otherwise it feels kind
> of incomplete. Would you be ok with addressing this separately after
> the current patch series is accepted ?

Absolutely, if you'd like to clean up the code please feel free.
I agree that a separate patchset is probably the best way to go.
(Do you mean by that that you'd like me to take your v2 as-is?)

thanks
-- PMM

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

[Guenter Roeck]

On 1/19/20 11:01 AM, Peter Maydell wrote:
> On Sun, 19 Jan 2020 at 01:52, Guenter Roeck <linux@roeck-us.net> wrote:
>> I'd rather try to fix it all if I am at it; otherwise it feels kind
>> of incomplete. Would you be ok with addressing this separately after
>> the current patch series is accepted ?
> 
> Absolutely, if you'd like to clean up the code please feel free.
> I agree that a separate patchset is probably the best way to go.
> (Do you mean by that that you'd like me to take your v2 as-is?)
> 

Sure, if you accept it as-is.

Thanks,
Guenter