[Qemu-devel] [PATCH target-arm v1 0/7] PL330 Changes

[Qemu-devel] [PATCH target-arm v1 0/7] PL330 Changes

[Peter Crosthwaite]

Hi Peter,

Some minor fixups and tweaks to PL330.

Regards,
Peter


Peter Crosthwaite (7):
  dma/pl330: Delete overly verbose debug printf
  dma/pl330: Fix misleading type
  dma/pl330: printf format type sweep.
  dma/pl330: Rename parent_obj
  dma/pl330: Add event debugging printfs
  dma/pl330: Fix buffer depth
  dma/pl330: implement dmaadnh instruction

 hw/dma/pl330.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 19 deletions(-)

-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 1/7] dma/pl330: Delete overly verbose debug printf

[Peter Crosthwaite]

When using event synchronisation, this particular debug printf floods.
Just delete it.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 401399d..68adf39 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -1108,7 +1108,6 @@ static int pl330_chan_exec(PL330Chan *ch)
             ch->state != pl330_chan_waiting_periph &&
             ch->state != pl330_chan_at_barrier &&
             ch->state != pl330_chan_waiting_event) {
-        DB_PRINT("%d\n", ch->state);
         return 0;
     }
     ch->stall = 0;
-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 2/7] dma/pl330: Fix misleading type

[Peter Crosthwaite]

This type really should just be a regular int as no usages rely on it's
32 bitness (it's only meaningful as a bit position and not a bit mask).
This also fixes a printf which uses the variable with a regular %d.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 68adf39..d36f0bc 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -1310,7 +1310,7 @@ static void pl330_iomem_write(void *opaque, hwaddr offset,
                               uint64_t value, unsigned size)
 {
     PL330State *s = (PL330State *) opaque;
-    uint32_t i;
+    int i;
 
     DB_PRINT("addr: %08x data: %08x\n", (unsigned)offset, (unsigned)value);
 
-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 3/7] dma/pl330: printf format type sweep.

[Peter Crosthwaite]

Use PRI formats as appropriate rather than raw %x and %d. This fixes
debug printfery on some host platforms. Fix types of debug only
variables as appropriate.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index d36f0bc..a4cc6f9 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -577,7 +577,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: %x\n", ch, flags);
+    DB_PRINT("ch: %p, flags: %" PRIx32 "\n", ch, flags);
     ch->fault_type |= flags;
     if (ch->state == pl330_chan_fault) {
         return;
@@ -723,7 +723,8 @@ 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:%d address:%08x size:%d num:%d %c\n",
+        DB_PRINT("channel:%" PRId8 " address:%08" PRIx32 " size:%" PRIx32
+                 " num:%" PRId32 " %c\n",
                  ch->tag, ch->src, size, num, inc ? 'Y' : 'N');
         ch->src += inc ? size * num - (ch->src & (size - 1)) : 0;
     }
@@ -868,7 +869,7 @@ 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 %d\n", ev_id);
+        DB_PRINT("event interrupt raised %" PRId8 "\n", ev_id);
         qemu_irq_raise(ch->parent->irq[ev_id]);
     }
     ch->parent->ev_status |= (1 << ev_id);
@@ -895,7 +896,8 @@ 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:%d address:%08x size:%d num:%d %c\n",
+        DB_PRINT("channel:%" PRId8 " address:%08" PRIx32 " size:%" PRIx32
+                 " num:%" PRId32 " %c\n",
                  ch->tag, ch->dst, size, num, inc ? 'Y' : 'N');
         ch->dst += inc ? size * num - (ch->dst & (size - 1)) : 0;
     }
@@ -1154,7 +1156,7 @@ static int pl330_exec_cycle(PL330Chan *channel)
 
         dma_memory_read(&address_space_memory, q->addr, buf, len);
         if (PL330_ERR_DEBUG > 1) {
-            DB_PRINT("PL330 read from memory @%08x (size = %08x):\n",
+            DB_PRINT("PL330 read from memory @%08" PRIx32 " (size = %08x):\n",
                       q->addr, len);
             qemu_hexdump((char *)buf, stderr, "", len);
         }
@@ -1186,8 +1188,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 @%08x (size = %08x):\n",
-                         q->addr, len);
+                DB_PRINT("PL330 read from memory @%08" PRIx32
+                         " (size = %08x):\n", q->addr, len);
                 qemu_hexdump((char *)buf, stderr, "", len);
             }
             if (q->inc) {
@@ -1276,7 +1278,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: %d\n", chan_id);
+    DB_PRINT("chan id: %" PRIx8 "\n", chan_id);
     if (s->dbg[0] & 1) {
         ch = &s->chan[chan_id];
     } else {
@@ -1466,8 +1468,8 @@ static inline uint32_t pl330_iomem_read_imp(void *opaque,
 static uint64_t pl330_iomem_read(void *opaque, hwaddr offset,
         unsigned size)
 {
-    int ret = pl330_iomem_read_imp(opaque, offset);
-    DB_PRINT("addr: %08x data: %08x\n", (unsigned)offset, ret);
+    uint32_t ret = pl330_iomem_read_imp(opaque, offset);
+    DB_PRINT("addr: %08" HWADDR_PRIx " data: %08" PRIx32 "\n", offset, ret);
     return ret;
 }
 
@@ -1553,7 +1555,7 @@ static void pl330_realize(DeviceState *dev, Error **errp)
         s->cfg[1] |= 5;
         break;
     default:
-        error_setg(errp, "Bad value for i-cache_len property: %d\n",
+        error_setg(errp, "Bad value for i-cache_len property: %" PRIx8 "\n",
                    s->i_cache_len);
         return;
     }
@@ -1588,7 +1590,7 @@ static void pl330_realize(DeviceState *dev, Error **errp)
         s->cfg[CFG_CRD] |= 0x4;
         break;
     default:
-        error_setg(errp, "Bad value for data_width property: %d\n",
+        error_setg(errp, "Bad value for data_width property: %" PRIx8 "\n",
                    s->data_width);
         return;
     }
-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 4/7] dma/pl330: Rename parent_obj

[Peter Crosthwaite]

As per current QOM conventions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index a4cc6f9..8046a6f 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -227,7 +227,8 @@ static const VMStateDescription vmstate_pl330_queue = {
 };
 
 struct PL330State {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;
+
     MemoryRegion iomem;
     qemu_irq irq_abort;
     qemu_irq *irq;
-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 5/7] dma/pl330: Add event debugging printfs

[Peter Crosthwaite]

These are helpful to anyone trying to debug event sequencing.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 8046a6f..303f8b8 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -873,6 +873,7 @@ static void pl330_dmasev(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
         DB_PRINT("event interrupt raised %" PRId8 "\n", ev_id);
         qemu_irq_raise(ch->parent->irq[ev_id]);
     }
+    DB_PRINT("event raised %" PRId8 "\n", ev_id);
     ch->parent->ev_status |= (1 << ev_id);
 }
 
@@ -975,6 +976,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);
     } else {
         ch->stall = 1;
     }
-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 6/7] dma/pl330: Fix buffer depth

[Peter Crosthwaite]

This is the product of the data-width and the depth arguments, I.e the
depth of the FIFO is in terms of data entries and not bytes (which is
what the original implementation was suggesting). Fix.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 303f8b8..fe437cb 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -1606,7 +1606,7 @@ static void pl330_realize(DeviceState *dev, Error **errp)
 
     pl330_queue_init(&s->read_queue, s->rd_q_dep, s);
     pl330_queue_init(&s->write_queue, s->wr_q_dep, s);
-    pl330_fifo_init(&s->fifo, s->data_buffer_dep);
+    pl330_fifo_init(&s->fifo, s->data_width / 4  * s->data_buffer_dep);
 }
 
 static Property pl330_properties[] = {
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH target-arm v1 6/7] dma/pl330: Fix buffer depth

[Peter Maydell]

On 11 February 2014 06:31, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This is the product of the data-width and the depth arguments, I.e the
> depth of the FIFO is in terms of data entries and not bytes (which is
> what the original implementation was suggesting). Fix.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/dma/pl330.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index 303f8b8..fe437cb 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -1606,7 +1606,7 @@ static void pl330_realize(DeviceState *dev, Error **errp)
>
>      pl330_queue_init(&s->read_queue, s->rd_q_dep, s);
>      pl330_queue_init(&s->write_queue, s->wr_q_dep, s);
> -    pl330_fifo_init(&s->fifo, s->data_buffer_dep);
> +    pl330_fifo_init(&s->fifo, s->data_width / 4  * s->data_buffer_dep);

Stray double space after '4'.

-- PMM

[Qemu-devel] [PATCH target-arm v1 7/7] dma/pl330: implement dmaadnh instruction

[Peter Crosthwaite]

Implement the missing DMAADNH instruction. This is a minor variant
of the DMAADDH instruction, so factor out to a common implementation
for both (dmaadxh).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/dma/pl330.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index fe437cb..7acb2c4 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -601,10 +601,12 @@ static inline void pl330_fault(PL330Chan *ch, uint32_t flags)
  *   LEN - number of elements in ARGS array
  */
 
-static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
+static void pl330_dmaadxh(PL330Chan *ch, uint8_t *args, bool ra, bool neg)
 {
-    uint16_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);
-    uint8_t ra = (opcode >> 1) & 1;
+    uint32_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);
+    if (neg) {
+        im |= 0xffff << 16;
+    }
 
     if (ch->is_manager) {
         pl330_fault(ch, PL330_FAULT_UNDEF_INSTR);
@@ -617,6 +619,16 @@ static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
     }
 }
 
+static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
+{
+    pl330_dmaadxh(ch, args, extract32(opcode, 1, 1), false);
+}
+
+static void pl330_dmaadnh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
+{
+    pl330_dmaadxh(ch, args, extract32(opcode, 1, 1), true);
+}
+
 static void pl330_dmaend(PL330Chan *ch, uint8_t opcode,
                          uint8_t *args, int len)
 {
@@ -1042,6 +1054,7 @@ static void pl330_dmawmb(PL330Chan *ch, uint8_t opcode,
 /* NULL terminated array of the instruction descriptions. */
 static const PL330InsnDesc insn_desc[] = {
     { .opcode = 0x54, .opmask = 0xFD, .size = 3, .exec = pl330_dmaaddh, },
+    { .opcode = 0x5c, .opmask = 0xFD, .size = 3, .exec = pl330_dmaadnh, },
     { .opcode = 0x00, .opmask = 0xFF, .size = 1, .exec = pl330_dmaend, },
     { .opcode = 0x35, .opmask = 0xFF, .size = 2, .exec = pl330_dmaflushp, },
     { .opcode = 0xA0, .opmask = 0xFD, .size = 6, .exec = pl330_dmago, },
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH target-arm v1 7/7] dma/pl330: implement dmaadnh instruction

[Peter Maydell]

On 11 February 2014 06:32, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Implement the missing DMAADNH instruction. This is a minor variant
> of the DMAADDH instruction, so factor out to a common implementation
> for both (dmaadxh).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/dma/pl330.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index fe437cb..7acb2c4 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -601,10 +601,12 @@ static inline void pl330_fault(PL330Chan *ch, uint32_t flags)
>   *   LEN - number of elements in ARGS array
>   */
>
> -static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int len)
> +static void pl330_dmaadxh(PL330Chan *ch, uint8_t *args, bool ra, bool neg)
>  {
> -    uint16_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);
> -    uint8_t ra = (opcode >> 1) & 1;
> +    uint32_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);

These casts look kind of weird now that you've changed the type of 'im'.
In fact, you don't need them at all since the usual integer promotions
will take them up to 'int' which we know is 32 bits for us, and we're
not trying to shift into bit 31...

> +    if (neg) {
> +        im |= 0xffff << 16;
> +    }

...speaking of which, you want a 'U' suffix on the constant, otherwise
you're doing a signed shift into the sign bit, which is undefined behaviour.

I checked the docs, and this is the correct behaviour -- we're one-extending
the immediate, not sign-extending it. Maybe this could use a brief comment:
    /* One-extend the unsigned 16 bit value to 32 bits */

thanks
-- PMM

Re: [Qemu-devel] [PATCH target-arm v1 0/7] PL330 Changes

[Peter Maydell]

On 11 February 2014 06:28, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Hi Peter,
>
> Some minor fixups and tweaks to PL330.

Thanks. For patches 1-5, and also 6 if you fix the double-space:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM