Re: [PATCH v1 1/2] hw: allwinner-i2c: Make the trace message more readable

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 17/2/23 10:42, qianfanguijin@163.com wrote:
> From: qianfan Zhao <qianfanguijin@163.com>
> 
> Next is an example when allwinner_i2c_rw enabled:
> 
> allwinner_i2c_rw write   CNTR[0x0c]: 50 { M_STP BUS_EN  }
> allwinner_i2c_rw write   CNTR[0x0c]: e4 { A_ACK M_STA BUS_EN INT_EN  }
> allwinner_i2c_rw  read   CNTR[0x0c]: cc { A_ACK INT_FLAG BUS_EN INT_EN }
> allwinner_i2c_rw  read   STAT[0x10]: 08 { STAT_M_STA_TX }
> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>   hw/i2c/allwinner-i2c.c | 90 +++++++++++++++++++++++++++++++++++++++++-
>   hw/i2c/trace-events    |  4 +-
>   2 files changed, 89 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i2c/allwinner-i2c.c b/hw/i2c/allwinner-i2c.c
> index a435965836..36b387520f 100644
> --- a/hw/i2c/allwinner-i2c.c
> +++ b/hw/i2c/allwinner-i2c.c
> @@ -129,6 +129,39 @@ enum {
>       STAT_IDLE = 0x1f
>   } TWI_STAT_STA;
>   
> +#define TWI_STAT_STA_DESC(sta)  [sta] = #sta
> +static const char *twi_stat_sta_descriptors[] = {
> +    TWI_STAT_STA_DESC(STAT_BUS_ERROR),
> +    TWI_STAT_STA_DESC(STAT_M_STA_TX),
> +    TWI_STAT_STA_DESC(STAT_M_RSTA_TX),
> +    TWI_STAT_STA_DESC(STAT_M_ADDR_WR_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_ADDR_WR_NACK),
> +    TWI_STAT_STA_DESC(STAT_M_DATA_TX_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_DATA_TX_NACK),
> +    TWI_STAT_STA_DESC(STAT_M_ARB_LOST),
> +    TWI_STAT_STA_DESC(STAT_M_ADDR_RD_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_ADDR_RD_NACK),
> +    TWI_STAT_STA_DESC(STAT_M_DATA_RX_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_DATA_RX_NACK),
> +    TWI_STAT_STA_DESC(STAT_S_ADDR_WR_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_AW_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_GCA_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_GCA_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_RX_SA_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_RX_SA_NACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_RX_GCA_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_RX_GCA_NACK),
> +    TWI_STAT_STA_DESC(STAT_S_STP_RSTA),
> +    TWI_STAT_STA_DESC(STAT_S_ADDR_RD_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_ARB_LOST_AR_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_TX_ACK),
> +    TWI_STAT_STA_DESC(STAT_S_DATA_TX_NACK),
> +    TWI_STAT_STA_DESC(STAT_S_LB_TX_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_2ND_ADDR_WR_ACK),
> +    TWI_STAT_STA_DESC(STAT_M_2ND_ADDR_WR_NACK),
> +    TWI_STAT_STA_DESC(STAT_IDLE),
> +};
> +
>   static const char *allwinner_i2c_get_regname(unsigned offset)
>   {
>       switch (offset) {
> @@ -155,6 +188,59 @@ static const char *allwinner_i2c_get_regname(unsigned offset)
>       }
>   }
>   
> +static const char *twi_cntr_reg_bits[] = {
> +    [2] = "A_ACK",
> +    [3] = "INT_FLAG",
> +    [4] = "M_STP",
> +    [5] = "M_STA",
> +    [6] = "BUS_EN",
> +    [7] = "INT_EN",
> +};
> +
> +static void trace_buffer_append_bit_descriptors(char *s, size_t sz,
> +                                                unsigned int value,
> +                                                unsigned int start,
> +                                                unsigned int end,
> +                                                const char **desc_arrays)
> +{
> +    for (; start <= end; start++) {

You call this once, so no need to pass a desc_arrays[] argument.
Directly iterate over twi_cntr_reg_bits[] and its ARRAY_SIZE().

> +        if (value & (1 << start)) {
> +            strncat(s, desc_arrays[start], sz - 1);

Watch out, desc_arrays[start] could be NULL.

> +            strncat(s, " ", sz - 1);
> +        }
> +    }
> +}
> +
> +static void allwinner_i2c_trace_rw(int is_write, unsigned int offset,

Please use 'bool' for 'is_write' which is a boolean.

> +                                   unsigned int value)
> +{

You can call trace_event_get_state_backends() to check if a
trace event is enabled and return early without further processing.

> +    char s[256] = { 0 };
> +
> +    snprintf(s, sizeof(s), "%s %6s[0x%02x]: %02x ",

Please prefix hexadecimal values with 0x.

> +             is_write ? "write": " read",
> +             allwinner_i2c_get_regname(offset), offset,
> +             value);

We prefer the safer g_autofree ... g_strdup_printf().

> +    switch (offset) {
> +    case TWI_CNTR_REG:
> +        strncat(s, "{ ", sizeof(s) - 1);
> +        trace_buffer_append_bit_descriptors(s, sizeof(s), value,
> +                                            2, 7, twi_cntr_reg_bits);
> +        strncat(s, " }", sizeof(s) - 1);
> +        break;
> +    case TWI_STAT_REG:
> +        if (STAT_TO_STA(value) <= STAT_IDLE) {
> +            strncat(s, "{ ", sizeof(s) - 1);
> +            strncat(s, twi_stat_sta_descriptors[STAT_TO_STA(value)],
> +                    sizeof(s) - 1);
> +            strncat(s, " }", sizeof(s) - 1);
> +        }
> +        break;
> +    }
> +
> +    trace_allwinner_i2c_rw(s);
> +}
> +
>   static inline bool allwinner_i2c_is_reset(AWI2CState *s)
>   {
>       return s->srst & TWI_SRST_MASK;
> @@ -271,7 +357,7 @@ static uint64_t allwinner_i2c_read(void *opaque, hwaddr offset,
>           break;
>       }
>   
> -    trace_allwinner_i2c_read(allwinner_i2c_get_regname(offset), offset, value);
> +    allwinner_i2c_trace_rw(0, (unsigned int)offset, (unsigned int)value);
>   
>       return (uint64_t)value;
>   }
> @@ -283,7 +369,7 @@ static void allwinner_i2c_write(void *opaque, hwaddr offset,
>   
>       value &= 0xff;
>   
> -    trace_allwinner_i2c_write(allwinner_i2c_get_regname(offset), offset, value);
> +    allwinner_i2c_trace_rw(1, (unsigned int)offset, (unsigned int)value);
>   
>       switch (offset) {
>       case TWI_ADDR_REG:
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 8e88aa24c1..fa5e8d5021 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -16,9 +16,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>   i2c_ack(void) ""
>   
>   # allwinner_i2c.c
> -
> -allwinner_i2c_read(const char* reg_name, uint64_t offset, uint64_t value) "read %s [0x%" PRIx64 "]: -> 0x%" PRIx64
> -allwinner_i2c_write(const char* reg_name, uint64_t offset, uint64_t value) "write %s [0x%" PRIx64 "]: <- 0x%" PRIx64
> +allwinner_i2c_rw(const char *s) "%s"

Please do not remove the other events. The tracing framework provides
multiple backends. Some can be processed by scripts, and providing
integer values are simpler to parse than a string.

That said, your event would be more useful for other backends as:

allwinner_i2c_rw(unsigned is_write,
                  const char *regname,
                  uing8_t regaddr,
                  uing8_t value,
                  const char *desc)
                  "wr:%u   %s[0x02x]: 0x%02x { %s }"

Regards,

Phil.