qemu-devel
[Top][All Lists]
Advanced
[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: qianfan
Subject: Re: [PATCH v1 1/2] hw: allwinner-i2c: Make the trace message more readable
Date: Mon, 20 Feb 2023 09:01:41 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 


在 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.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]