[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/2] hw: allwinner-i2c: Make the trace message more readab
|
From: |
Philippe Mathieu-Daudé |
|
Subject: |
Re: [PATCH v1 1/2] hw: allwinner-i2c: Make the trace message more readable |
|
Date: |
Sun, 19 Feb 2023 23:30:11 +0100 |
|
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 |
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.