| Message ID | 20230217094207.16882-1-qianfanguijin@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] hw: allwinner-i2c: Make the trace message more readable | expand |
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.
在 2023/2/20 6:30, Philippe Mathieu-Daudé 写道: > 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(). create desc_arrays is useful if there has more register need dump. such as trace_buffer_append_bit_descriptors(..., twi_cntr_reg_bits) or trace_buffer_append_bit_descriptors(..., twi_line_cntr_reg_bits) > >> + if (value & (1 << start)) { >> + strncat(s, desc_arrays[start], sz - 1); > > Watch out, desc_arrays[start] could be NULL. if ((value & (1 << start)) && desc_arrays[start]) is better. > >> + 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. got it. > >> + char s[256] = { 0 }; >> + >> + snprintf(s, sizeof(s), "%s %6s[0x%02x]: %02x ", > > Please prefix hexadecimal values with 0x. > OK. >> + is_write ? "write": " read", >> + allwinner_i2c_get_regname(offset), offset, >> + value); > > We prefer the safer g_autofree ... g_strdup_printf(). The next trace_buffer_append_bit_descriptors will appending to a pre-alloced buffer, so I create a buffer. Total 256 bytes seems enough for the trace strings. > >> + 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 }" I am a beginner for qemu and don't know how to use scripts to parse trace strings. Could you please give me an example for that? In my opinion the trace string is for humal, not machine, so I convert the register value to a humal readable string. I had merge the allwinner_i2c_read and allwinner_i2c_write to allwinner_i2c_rw, but it is good to split them, someone may interested the write events only. How about this? allwinner_i2c_read(const char *regname, uint8_t offset, uint8_t value, const char *desc); allwinner_i2c_write(const char *regname, uint8_t offset, uint8_t value, const char *desc); And I had a question: which type is better for offset and value, uint8_t or uint64_t? > > Regards, > > Phil.
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++) { + if (value & (1 << start)) { + strncat(s, desc_arrays[start], sz - 1); + strncat(s, " ", sz - 1); + } + } +} + +static void allwinner_i2c_trace_rw(int is_write, unsigned int offset, + unsigned int value) +{ + char s[256] = { 0 }; + + snprintf(s, sizeof(s), "%s %6s[0x%02x]: %02x ", + is_write ? "write": " read", + allwinner_i2c_get_regname(offset), offset, + value); + + 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" # aspeed_i2c.c