Re: [PATCH v2] hw/char: stm32f2xx_usart: replace print with trace

[Alistair Francis]

On Fri, Dec 20, 2024 at 9:18 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <nshubin@yadro.com>
>
> Drop debug printing macros and replace them with according trace
> functions.
>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> v1->v2:
> Phil:
> - fix typo s/recieve/receive/
> - initialize 'retvalue' with zero
>
> ---
>  hw/char/stm32f2xx_usart.c | 49 ++++++++++++++++++---------------------
>  hw/char/trace-events      |  6 +++++
>  2 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 17b5b1f15f..cd97f7d7e4 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -30,17 +30,7 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>
> -#ifndef STM_USART_ERR_DEBUG
> -#define STM_USART_ERR_DEBUG 0
> -#endif
> -
> -#define DB_PRINT_L(lvl, fmt, args...) do { \
> -    if (STM_USART_ERR_DEBUG >= lvl) { \
> -        qemu_log("%s: " fmt, __func__, ## args); \
> -    } \
> -} while (0)
> -
> -#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +#include "trace.h"
>
>  static int stm32f2xx_usart_can_receive(void *opaque)
>  {
> @@ -67,10 +57,11 @@ static void stm32f2xx_update_irq(STM32F2XXUsartState *s)
>  static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf, int 
> size)
>  {
>      STM32F2XXUsartState *s = opaque;
> +    DeviceState *d = DEVICE(s);
>
>      if (!(s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_RE)) {
>          /* USART not enabled - drop the chars */
> -        DB_PRINT("Dropping the chars\n");
> +        trace_stm32f2xx_usart_drop(d->id);
>          return;
>      }
>
> @@ -79,7 +70,7 @@ static void stm32f2xx_usart_receive(void *opaque, const 
> uint8_t *buf, int size)
>
>      stm32f2xx_update_irq(s);
>
> -    DB_PRINT("Receiving: %c\n", s->usart_dr);
> +    trace_stm32f2xx_usart_receive(d->id, *buf);
>  }
>
>  static void stm32f2xx_usart_reset(DeviceState *dev)
> @@ -101,49 +92,55 @@ static uint64_t stm32f2xx_usart_read(void *opaque, 
> hwaddr addr,
>                                         unsigned int size)
>  {
>      STM32F2XXUsartState *s = opaque;
> -    uint64_t retvalue;
> -
> -    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", addr);
> +    DeviceState *d = DEVICE(s);
> +    uint64_t retvalue = 0;
>
>      switch (addr) {
>      case USART_SR:
>          retvalue = s->usart_sr;
>          qemu_chr_fe_accept_input(&s->chr);
> -        return retvalue;
> +        break;
>      case USART_DR:
> -        DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) 
> s->usart_dr);
>          retvalue = s->usart_dr & 0x3FF;
>          s->usart_sr &= ~USART_SR_RXNE;
>          qemu_chr_fe_accept_input(&s->chr);
>          stm32f2xx_update_irq(s);
> -        return retvalue;
> +        break;
>      case USART_BRR:
> -        return s->usart_brr;
> +        retvalue = s->usart_brr;
> +        break;
>      case USART_CR1:
> -        return s->usart_cr1;
> +        retvalue = s->usart_cr1;
> +        break;
>      case USART_CR2:
> -        return s->usart_cr2;
> +        retvalue = s->usart_cr2;
> +        break;
>      case USART_CR3:
> -        return s->usart_cr3;
> +        retvalue = s->usart_cr3;
> +        break;
>      case USART_GTPR:
> -        return s->usart_gtpr;
> +        retvalue = s->usart_gtpr;
> +        break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>          return 0;
>      }
>
> -    return 0;
> +    trace_stm32f2xx_usart_read(d->id, size, addr, retvalue);
> +
> +    return retvalue;
>  }
>
>  static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
>                                    uint64_t val64, unsigned int size)
>  {
>      STM32F2XXUsartState *s = opaque;
> +    DeviceState *d = DEVICE(s);
>      uint32_t value = val64;
>      unsigned char ch;
>
> -    DB_PRINT("Write 0x%" PRIx32 ", 0x%"HWADDR_PRIx"\n", value, addr);
> +    trace_stm32f2xx_usart_write(d->id, size, addr, val64);
>
>      switch (addr) {
>      case USART_SR:
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index 59e1f734a7..8b847006a0 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -125,3 +125,9 @@ xen_console_unrealize(unsigned int idx) "idx %u"
>  xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
>  xen_console_device_create(unsigned int idx) "idx %u"
>  xen_console_device_destroy(unsigned int idx) "idx %u"
> +
> +# stm32f2xx_usart.c
> +stm32f2xx_usart_read(char *id, unsigned size, uint64_t offs, uint64_t val) " 
> %s size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
> +stm32f2xx_usart_write(char *id, unsigned size, uint64_t offs, uint64_t val) 
> "%s size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
> +stm32f2xx_usart_drop(char *id) " %s dropping the chars"
> +stm32f2xx_usart_receive(char *id, uint8_t chr) " %s receiving %c"
> --
> 2.45.2
>
>