Re: [PATCH 2/2] !fixup "hw/timer: Add limited support for AVR 16-bit timer peripheral"

[Alex Bennée]

Philippe Mathieu-Daudé <address@hidden> writes:

> Convert DB_PRINT() to trace events.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/timer/avr_timer16.c | 25 +++++++++++++++----------
>  hw/timer/trace-events  | 12 ++++++++++++
>  2 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/hw/timer/avr_timer16.c b/hw/timer/avr_timer16.c
> index 4e16afc61c..073f36aa76 100644
> --- a/hw/timer/avr_timer16.c
> +++ b/hw/timer/avr_timer16.c
> @@ -36,6 +36,7 @@
>  #include "qemu/log.h"
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
> +#include "trace.h"
>  
>  /* Register offsets */
>  #define T16_CRA     0x0
> @@ -104,7 +105,6 @@
>  /* Helper macros */
>  #define VAL16(l, h) ((h << 8) | l)
>  #define DB_PRINT(fmt, args...) /* Nothing */
> -/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## args)*/
>  
>  static inline int64_t avr_timer16_ns_to_ticks(AVRTimer16State *t16, int64_t 
> t)
>  {
> @@ -168,8 +168,8 @@ static void avr_timer16_clksrc_update(AVRTimer16State 
> *t16)
>      if (divider) {
>          t16->freq_hz = t16->cpu_freq_hz / divider;
>          t16->period_ns = NANOSECONDS_PER_SECOND / t16->freq_hz;
> -        DB_PRINT("Timer frequency %" PRIu64 " hz, period %" PRIu64 " ns (%f 
> s)",
> -                 t16->freq_hz, t16->period_ns, 1 / (double)t16->freq_hz);
> +        trace_avr_timer16_clksrc_update(t16->freq_hz, t16->period_ns,
> +                                        (uint64_t)(1e6 /
>  t16->freq_hz));

Is it worth including a computed value here?

>      }
>  }
>  
> @@ -235,8 +235,7 @@ static void avr_timer16_set_alarm(AVRTimer16State *t16)
>          t16->reset_time_ns + ((CNT(t16) + alarm_offset) * t16->period_ns);
>      timer_mod(t16->timer, alarm_ns);
>  
> -    DB_PRINT("next alarm %" PRIu64 " ns from now",
> -        alarm_offset * t16->period_ns);
> +    trace_avr_timer16_next_alarm(alarm_offset * t16->period_ns);
>  }
>  
>  static void avr_timer16_interrupt(void *opaque)
> @@ -253,11 +252,11 @@ static void avr_timer16_interrupt(void *opaque)
>          return;
>      }
>  
> -    DB_PRINT("interrupt, cnt = %d", CNT(t16));
> +    trace_avr_timer16_interrupt_count(CNT(t16));
>  
>      /* Counter overflow */
>      if (t16->next_interrupt == OVERFLOW) {
> -        DB_PRINT("0xffff overflow");
> +        trace_avr_timer16_interrupt_overflow("counter 0xffff");
>          avr_timer16_clock_reset(t16);
>          if (t16->imsk & T16_INT_TOV) {
>              t16->ifr |= T16_INT_TOV;
> @@ -266,12 +265,12 @@ static void avr_timer16_interrupt(void *opaque)
>      }
>      /* Check for ocra overflow in CTC mode */
>      if (mode == T16_MODE_CTC_OCRA && t16->next_interrupt == COMPA) {
> -        DB_PRINT("CTC OCRA overflow");
> +        trace_avr_timer16_interrupt_overflow("CTC OCRA");
>          avr_timer16_clock_reset(t16);
>      }
>      /* Check for icr overflow in CTC mode */
>      if (mode == T16_MODE_CTC_ICR && t16->next_interrupt == CAPT) {
> -        DB_PRINT("CTC ICR overflow");
> +        trace_avr_timer16_interrupt_overflow("CTC ICR");

Rather than spamming strings to the trace log I'd suggest expanding
trace_avr_timer16_interrupt_count to include the mode and next_interrupt
state as hex values.

>          avr_timer16_clock_reset(t16);
>          if (t16->imsk & T16_INT_IC) {
>              t16->ifr |= T16_INT_IC;
> @@ -367,6 +366,8 @@ static uint64_t avr_timer16_read(void *opaque, hwaddr 
> offset, unsigned size)
>      default:
>          break;
>      }
> +    trace_avr_timer16_read(offset, retval);
> +
>      return (uint64_t)retval;
>  }
>  
> @@ -378,7 +379,7 @@ static void avr_timer16_write(void *opaque, hwaddr offset,
>      uint8_t val8 = (uint8_t)val64;
>      uint8_t prev_clk_src = CLKSRC(t16);
>  
> -    DB_PRINT("write %d to offset %d", val8, (uint8_t)offset);
> +    trace_avr_timer16_write(offset, val8);
>  
>      switch (offset) {
>      case T16_CRA:
> @@ -475,6 +476,7 @@ static uint64_t avr_timer16_imsk_read(void *opaque,
>  {
>      assert(size == 1);
>      AVRTimer16State *t16 = opaque;
> +    trace_avr_timer16_read_imsk(offset ? 0 : t16->imsk);
>      if (offset != 0) {
>          return 0;
>      }
> @@ -486,6 +488,7 @@ static void avr_timer16_imsk_write(void *opaque, hwaddr 
> offset,
>  {
>      assert(size == 1);
>      AVRTimer16State *t16 = opaque;
> +    trace_avr_timer16_write_imsk(val64);
>      if (offset != 0) {
>          return;
>      }
> @@ -498,6 +501,7 @@ static uint64_t avr_timer16_ifr_read(void *opaque,
>  {
>      assert(size == 1);
>      AVRTimer16State *t16 = opaque;
> +    trace_avr_timer16_read_ifr(offset ? 0 : t16->ifr);
>      if (offset != 0) {
>          return 0;
>      }
> @@ -509,6 +513,7 @@ static void avr_timer16_ifr_write(void *opaque, hwaddr 
> offset,
>  {
>      assert(size == 1);
>      AVRTimer16State *t16 = opaque;
> +    trace_avr_timer16_write_imsk(val64);
>      if (offset != 0) {
>          return;
>      }
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index 29fda7870e..5d9f4c93fb 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -74,3 +74,15 @@ nrf51_timer_write(uint64_t addr, uint32_t value, unsigned 
> size) "write addr 0x%"
>  bcm2835_systmr_irq(bool enable) "timer irq state %u"
>  bcm2835_systmr_read(uint64_t offset, uint64_t data) "timer read: offset 0x%" 
> PRIx64 " data 0x%" PRIx64
>  bcm2835_systmr_write(uint64_t offset, uint64_t data) "timer write: offset 
> 0x%" PRIx64 " data 0x%" PRIx64
> +
> +# avr_timer16.c
> +avr_timer16_read(uint8_t addr, uint8_t value) "timer16 read addr:%u value:%u"
> +avr_timer16_read_ifr(uint8_t value) "timer16 read addr:ifr value:%u"
> +avr_timer16_read_imsk(uint8_t value) "timer16 read addr:imsk value:%u"
> +avr_timer16_write(uint8_t addr, uint8_t value) "timer16 write addr:%u 
> value:%u"
> +avr_timer16_write_ifr(uint8_t value) "timer16 write addr:ifr value:%u"
> +avr_timer16_write_imsk(uint8_t value) "timer16 write addr:imsk value:%u"
> +avr_timer16_interrupt_count(uint8_t cnt) "count: %u"
> +avr_timer16_interrupt_overflow(const char *reason) "overflow: %s"
> +avr_timer16_next_alarm(uint64_t delay_ns) "next alarm: %" PRIu64 " ns from 
> now"
> +avr_timer16_clksrc_update(uint64_t freq_hz, uint64_t period_ns, uint64_t 
> delay_s) "timer frequency: %" PRIu64 " Hz, period: %" PRIu64 " ns (%" PRId64 
> " us)"


-- 
Alex Bennée