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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 4818 bytes --]

On Jun  2 08:49, Cédric Le Goater wrote:
> 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);
>     }
> 
> 

Uhm, yeah - that's way better :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]