Re: [RFC PATCH v2 1/6] hw/i2c/aspeed: rework raise interrupt trace event

[Cédric Le Goater]

On 6/1/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Build a single string instead of having several parameters on the trace
> event.
>
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/i2c/aspeed_i2c.c | 55 +++++++++++++++++++++++++++++++++++----------
>   hw/i2c/trace-events |  2 +-
>   2 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 5fce516517a5..576425898b09 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -21,6 +21,7 @@
>   #include "qemu/osdep.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
> +#include "qemu/cutils.h"
>   #include "qemu/log.h"
>   #include "qemu/module.h"
>   #include "qemu/error-report.h"
> @@ -31,6 +32,9 @@
>   #include "hw/registerfields.h"
>   #include "trace.h"
>   
> +#define ASPEED_I2C_TRACE_INTR_TEMPLATE \
> +    "pktdone|nak|ack|done|normal|abnormal|"
> +
>   static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>   {
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> @@ -38,23 +42,50 @@ static inline void 
> aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>       uint32_t intr_ctrl_reg = aspeed_i2c_bus_intr_ctrl_offset(bus);
>       bool raise_irq;
>   
> -    trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts],
> -        aspeed_i2c_bus_pkt_mode_en(bus) &&
> -        ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ?
> -                                                               "pktdone|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK) ? "nak|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK) ? "ack|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|"
> -                                                                  : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
> -                                                                "normal|" : "",
> -        SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"
> -                                                                   : "");
> +    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
> +        static const size_t BUF_SIZE = strlen(ASPEED_I2C_TRACE_INTR_TEMPLATE);
> +        g_autofree char *buf = g_malloc0(BUF_SIZE);
> +
> +        /*
> +         * Remember to update ASPEED_I2C_TRACE_INTR_TEMPLATE if you add a new
> +         * status string.
> +         */
> +
> +        if (aspeed_i2c_bus_pkt_mode_en(bus) &&
> +            ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE)) {
> +            pstrcat(buf, BUF_SIZE, "pktdone|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)) {
> +            pstrcat(buf, BUF_SIZE, "nak|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK)) {
> +            pstrcat(buf, BUF_SIZE, "ack|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE)) {
> +            pstrcat(buf, BUF_SIZE, "done|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)) {
> +            pstrcat(buf, BUF_SIZE, "normal|");
> +        }
> +
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL)) {
> +            pstrcat(buf, BUF_SIZE, "abnormal|");
> +        }
> +
> +        trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], buf);
> +    }
> +

How about :

    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_RAISE_INTERRUPT)) {
        g_autofree char *buf = g_strdup_printf("%s%s%s%s%s%s",
               aspeed_i2c_bus_pkt_mode_en(bus) &&
               ARRAY_FIELD_EX32(bus->regs, I2CM_INTR_STS, PKT_CMD_DONE) ? "pktdone|" : 
"",
               SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_NAK)? "nak|" : 
"",
               SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, TX_ACK), "ack|" : 
"",
               SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, RX_DONE) ? "done|" : 
"",
               SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP)? "normal|" : 
"",
               SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ? "abnormal"  : 
"");
        
               trace_aspeed_i2c_bus_raise_interrupt(bus->regs[reg_intr_sts], 
buf);
    }


Thanks,

C.




>       raise_irq = bus->regs[reg_intr_sts] & bus->regs[intr_ctrl_reg];
> +
>       /* In packet mode we don't mask off INTR_STS */
>       if (!aspeed_i2c_bus_pkt_mode_en(bus)) {
>           bus->regs[reg_intr_sts] &= bus->regs[intr_ctrl_reg];
>       }
> +
>       if (raise_irq) {
>           bus->controller->intr_status |= 1 << bus->id;
>           qemu_irq_raise(aic->bus_get_irq(bus));
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 85e4bddff936..209275ed2dc8 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) 
> data:0x%02x"
>   # aspeed_i2c.c
>   
>   aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
> -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, 
> const char *str3, const char *str4, const char *str5, const char *str6) "handled 
> intr=0x%x %s%s%s%s%s%s"
> +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *s) "handled 
> intr=0x%x %s"
>   aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) 
> "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) 
> "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
>   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send 
> %d/%d 0x%02x"