diff mbox

[2/6] dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK

Message ID 1468174138-32128-3-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Cave-Ayland July 10, 2016, 6:08 p.m. UTC 
By default large amounts of DBDMA debugging are produced when often it is just
1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
the developer to select the channels of interest at compile time, and then
further add the extra channel information to each debug statement where
possible.

Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
half execution.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

Comments

Mark Cave-Ayland July 15, 2016, 6:42 a.m. UTC | #1 
On 10/07/16 19:08, Mark Cave-Ayland wrote:

> By default large amounts of DBDMA debugging are produced when often it is just
> 1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
> the developer to select the channels of interest at compile time, and then
> further add the extra channel information to each debug statement where
> possible.
> 
> Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
> half execution.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index b6639f4..8e4b208 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -46,6 +46,7 @@
>  
>  /* debug DBDMA */
>  #define DEBUG_DBDMA 0
> +#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)

Someone flagged up that this doesn't work on 32-bit hosts because while
the channel is 0 to 31, the compiler shifts first and then subtracts
which gives warnings like this:

hw/misc/macio/mac_dbdma.c: In function ‘dbdma_cmdptr_load’:
hw/misc/macio/mac_dbdma.c:49:36: error: left shift count >= width of
type [-Werror=shift-count-overflow]
 #define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)

The fix here is to change the above to:

#define DEBUG_DBDMA_CHANMASK ((1ull << DBDMA_CHANNELS) - 1)

Here the unsigned long long will ensure that the shift is valid before
the subtraction to get the final 32-bit wide mask.

David, are you able to fix this up in your branch?

>  #define DBDMA_DPRINTF(fmt, ...) do { \
>      if (DEBUG_DBDMA) { \
> @@ -53,6 +54,14 @@
>      } \
>  } while (0);
>  
> +#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
> +    if (DEBUG_DBDMA) { \
> +        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
> +            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
> +        } \
> +    } \
> +} while (0);
> +

This part is still okay for a channel range 0 to 31.


ATB,

Mark.
David Gibson July 15, 2016, 7:20 a.m. UTC | #2 
On Fri, Jul 15, 2016 at 07:42:51AM +0100, Mark Cave-Ayland wrote:
> On 10/07/16 19:08, Mark Cave-Ayland wrote:
> 
> > By default large amounts of DBDMA debugging are produced when often it is just
> > 1 or 2 channels that are of interest. Introduce DEBUG_DBDMA_CHANMASK to allow
> > the developer to select the channels of interest at compile time, and then
> > further add the extra channel information to each debug statement where
> > possible.
> > 
> > Also clearly mark the start/end of DBDMA_run_bh to allow tracking the bottom
> > half execution.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  hw/misc/macio/mac_dbdma.c |   75 +++++++++++++++++++++++++--------------------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> > 
> > diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> > index b6639f4..8e4b208 100644
> > --- a/hw/misc/macio/mac_dbdma.c
> > +++ b/hw/misc/macio/mac_dbdma.c
> > @@ -46,6 +46,7 @@
> >  
> >  /* debug DBDMA */
> >  #define DEBUG_DBDMA 0
> > +#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
> 
> Someone flagged up that this doesn't work on 32-bit hosts because while
> the channel is 0 to 31, the compiler shifts first and then subtracts
> which gives warnings like this:
> 
> hw/misc/macio/mac_dbdma.c: In function ‘dbdma_cmdptr_load’:
> hw/misc/macio/mac_dbdma.c:49:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
>  #define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
> 
> The fix here is to change the above to:
> 
> #define DEBUG_DBDMA_CHANMASK ((1ull << DBDMA_CHANNELS) - 1)
> 
> Here the unsigned long long will ensure that the shift is valid before
> the subtraction to get the final 32-bit wide mask.
> 
> David, are you able to fix this up in your branch?

Done.

> 
> >  #define DBDMA_DPRINTF(fmt, ...) do { \
> >      if (DEBUG_DBDMA) { \
> > @@ -53,6 +54,14 @@
> >      } \
> >  } while (0);
> >  
> > +#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
> > +    if (DEBUG_DBDMA) { \
> > +        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
> > +            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
> > +        } \
> > +    } \
> > +} while (0);
> > +
> 
> This part is still okay for a channel range 0 to 31.
> 
> 
> ATB,
> 
> Mark.
>
diff mbox

Patch

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index b6639f4..8e4b208 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -46,6 +46,7 @@ 
 
 /* debug DBDMA */
 #define DEBUG_DBDMA 0
+#define DEBUG_DBDMA_CHANMASK ((1ul << DBDMA_CHANNELS) - 1)
 
 #define DBDMA_DPRINTF(fmt, ...) do { \
     if (DEBUG_DBDMA) { \
@@ -53,6 +54,14 @@ 
     } \
 } while (0);
 
+#define DBDMA_DPRINTFCH(ch, fmt, ...) do { \
+    if (DEBUG_DBDMA) { \
+        if ((1ul << (ch)->channel) & DEBUG_DBDMA_CHANMASK) { \
+            printf("DBDMA[%02x]: " fmt , (ch)->channel, ## __VA_ARGS__); \
+        } \
+    } \
+} while (0);
+
 /*
  */
 
@@ -79,26 +88,26 @@  static void dump_dbdma_cmd(dbdma_cmd *cmd)
 #endif
 static void dbdma_cmdptr_load(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("dbdma_cmdptr_load 0x%08x\n",
-                  ch->regs[DBDMA_CMDPTR_LO]);
+    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_load 0x%08x\n",
+                    ch->regs[DBDMA_CMDPTR_LO]);
     dma_memory_read(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
                     &ch->current, sizeof(dbdma_cmd));
 }
 
 static void dbdma_cmdptr_save(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("dbdma_cmdptr_save 0x%08x\n",
-                  ch->regs[DBDMA_CMDPTR_LO]);
-    DBDMA_DPRINTF("xfer_status 0x%08x res_count 0x%04x\n",
-                  le16_to_cpu(ch->current.xfer_status),
-                  le16_to_cpu(ch->current.res_count));
+    DBDMA_DPRINTFCH(ch, "dbdma_cmdptr_save 0x%08x\n",
+                    ch->regs[DBDMA_CMDPTR_LO]);
+    DBDMA_DPRINTFCH(ch, "xfer_status 0x%08x res_count 0x%04x\n",
+                    le16_to_cpu(ch->current.xfer_status),
+                    le16_to_cpu(ch->current.res_count));
     dma_memory_write(&address_space_memory, ch->regs[DBDMA_CMDPTR_LO],
                      &ch->current, sizeof(dbdma_cmd));
 }
 
 static void kill_channel(DBDMA_channel *ch)
 {
-    DBDMA_DPRINTF("kill_channel\n");
+    DBDMA_DPRINTFCH(ch, "kill_channel\n");
 
     ch->regs[DBDMA_STATUS] |= DEAD;
     ch->regs[DBDMA_STATUS] &= ~ACTIVE;
@@ -114,7 +123,7 @@  static void conditional_interrupt(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("%s\n", __func__);
+    DBDMA_DPRINTFCH(ch, "%s\n", __func__);
 
     intr = le16_to_cpu(current->command) & INTR_MASK;
 
@@ -123,7 +132,7 @@  static void conditional_interrupt(DBDMA_channel *ch)
         return;
     case INTR_ALWAYS: /* always interrupt */
         qemu_irq_raise(ch->irq);
-        DBDMA_DPRINTF("%s: raise\n", __func__);
+        DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         return;
     }
 
@@ -138,13 +147,13 @@  static void conditional_interrupt(DBDMA_channel *ch)
     case INTR_IFSET:  /* intr if condition bit is 1 */
         if (cond) {
             qemu_irq_raise(ch->irq);
-            DBDMA_DPRINTF("%s: raise\n", __func__);
+            DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
     case INTR_IFCLR:  /* intr if condition bit is 0 */
         if (!cond) {
             qemu_irq_raise(ch->irq);
-            DBDMA_DPRINTF("%s: raise\n", __func__);
+            DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
         }
         return;
     }
@@ -158,7 +167,7 @@  static int conditional_wait(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("conditional_wait\n");
+    DBDMA_DPRINTFCH(ch, "conditional_wait\n");
 
     wait = le16_to_cpu(current->command) & WAIT_MASK;
 
@@ -217,7 +226,7 @@  static void conditional_branch(DBDMA_channel *ch)
     uint32_t status;
     int cond;
 
-    DBDMA_DPRINTF("conditional_branch\n");
+    DBDMA_DPRINTFCH(ch, "conditional_branch\n");
 
     /* check if we must branch */
 
@@ -262,7 +271,7 @@  static void dbdma_end(DBDMA_io *io)
     DBDMA_channel *ch = io->channel;
     dbdma_cmd *current = &ch->current;
 
-    DBDMA_DPRINTF("%s\n", __func__);
+    DBDMA_DPRINTFCH(ch, "%s\n", __func__);
 
     if (conditional_wait(ch))
         goto wait;
@@ -288,13 +297,13 @@  wait:
 static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
                         uint16_t req_count, int is_last)
 {
-    DBDMA_DPRINTF("start_output\n");
+    DBDMA_DPRINTFCH(ch, "start_output\n");
 
     /* KEY_REGS, KEY_DEVICE and KEY_STREAM
      * are not implemented in the mac-io chip
      */
 
-    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
+    DBDMA_DPRINTFCH(ch, "addr 0x%x key 0x%x\n", addr, key);
     if (!addr || key > KEY_STREAM3) {
         kill_channel(ch);
         return;
@@ -314,13 +323,13 @@  static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
 static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
                        uint16_t req_count, int is_last)
 {
-    DBDMA_DPRINTF("start_input\n");
+    DBDMA_DPRINTFCH(ch, "start_input\n");
 
     /* KEY_REGS, KEY_DEVICE and KEY_STREAM
      * are not implemented in the mac-io chip
      */
 
-    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
+    DBDMA_DPRINTFCH(ch, "addr 0x%x key 0x%x\n", addr, key);
     if (!addr || key > KEY_STREAM3) {
         kill_channel(ch);
         return;
@@ -343,7 +352,7 @@  static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
     dbdma_cmd *current = &ch->current;
     uint32_t val;
 
-    DBDMA_DPRINTF("load_word\n");
+    DBDMA_DPRINTFCH(ch, "load_word\n");
 
     /* only implements KEY_SYSTEM */
 
@@ -382,7 +391,7 @@  static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
     dbdma_cmd *current = &ch->current;
     uint32_t val;
 
-    DBDMA_DPRINTF("store_word\n");
+    DBDMA_DPRINTFCH(ch, "store_word\n");
 
     /* only implements KEY_SYSTEM */
 
@@ -445,7 +454,7 @@  static void channel_run(DBDMA_channel *ch)
     uint16_t req_count;
     uint32_t phy_addr;
 
-    DBDMA_DPRINTF("channel_run\n");
+    DBDMA_DPRINTFCH(ch, "channel_run\n");
     dump_dbdma_cmd(current);
 
     /* clear WAKE flag at command fetch */
@@ -539,9 +548,9 @@  static void DBDMA_run_bh(void *opaque)
 {
     DBDMAState *s = opaque;
 
-    DBDMA_DPRINTF("DBDMA_run_bh\n");
-
+    DBDMA_DPRINTF("-> DBDMA_run_bh\n");
     DBDMA_run(s);
+    DBDMA_DPRINTF("<- DBDMA_run_bh\n");
 }
 
 void DBDMA_kick(DBDMAState *dbdma)
@@ -556,7 +565,7 @@  void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
     DBDMAState *s = dbdma;
     DBDMA_channel *ch = &s->channels[nchan];
 
-    DBDMA_DPRINTF("DBDMA_register_channel 0x%x\n", nchan);
+    DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan);
 
     assert(rw);
     assert(flush);
@@ -600,7 +609,7 @@  dbdma_control_write(DBDMA_channel *ch)
         status &= ~FLUSH;
     }
 
-    DBDMA_DPRINTF("    status 0x%08x\n", status);
+    DBDMA_DPRINTFCH(ch, "    status 0x%08x\n", status);
 
     ch->regs[DBDMA_STATUS] = status;
 
@@ -617,10 +626,10 @@  static void dbdma_write(void *opaque, hwaddr addr,
     DBDMA_channel *ch = &s->channels[channel];
     int reg = (addr - (channel << DBDMA_CHANNEL_SHIFT)) >> 2;
 
-    DBDMA_DPRINTF("writel 0x" TARGET_FMT_plx " <= 0x%08"PRIx64"\n",
-                  addr, value);
-    DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
-                  (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
+    DBDMA_DPRINTFCH(ch, "writel 0x" TARGET_FMT_plx " <= 0x%08"PRIx64"\n",
+                    addr, value);
+    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
+                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
     /* cmdptr cannot be modified if channel is ACTIVE */
 
@@ -671,9 +680,9 @@  static uint64_t dbdma_read(void *opaque, hwaddr addr,
 
     value = ch->regs[reg];
 
-    DBDMA_DPRINTF("readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
-    DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
-                  (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
+    DBDMA_DPRINTFCH(ch, "readl 0x" TARGET_FMT_plx " => 0x%08x\n", addr, value);
+    DBDMA_DPRINTFCH(ch, "channel 0x%x reg 0x%x\n",
+                    (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
     switch(reg) {
     case DBDMA_CONTROL: