Re: [Qemu-devel] [PATCH 2/6] dbdma: add per-channel debugging enabled via DEBUG_DBDMA_CHANMASK

[David Gibson]

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 <address@hidden>
> > Acked-by: Benjamin Herrenschmidt <address@hidden>
> > ---
> >  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.
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature