Hi Cédric,

Sorry for the late reply.

On 10/17/19 1:52 PM, Cédric Le Goater wrote:
Hello Philippe,

On 17/10/2019 12:22, Philippe Mathieu-Daudé wrote:
Hi Cédric,

On 10/16/19 10:50 AM, Cédric Le Goater wrote:
Signed-off-by: Cédric Le Goater <c...@kaod.org>
---
   hw/i2c/aspeed_i2c.c | 93 ++++++++++++++++++++++++++++++++++++++-------
   hw/i2c/trace-events |  9 +++++
   2 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 030d9c56be65..2da04a4bff30 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -28,6 +28,7 @@
   #include "hw/i2c/aspeed_i2c.h"
   #include "hw/irq.h"
   #include "hw/qdev-properties.h"
+#include "trace.h"
     /* I2C Global Register */
   @@ -158,6 +159,13 @@ static inline void 
aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
   {
       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
   +    trace_aspeed_i2c_bus_raise_interrupt(bus->intr_status,
+          bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
+          bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
+          bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
+          bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
+          bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
+
       bus->intr_status &= bus->intr_ctrl;
       if (bus->intr_status) {
           bus->controller->intr_status |= 1 << bus->id;
@@ -170,41 +178,57 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr 
offset,
   {
       AspeedI2CBus *bus = opaque;
       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+    uint64_t value = -1;
         switch (offset) {
       case I2CD_FUN_CTRL_REG:
-        return bus->ctrl;
+        value = bus->ctrl;
+        break;
       case I2CD_AC_TIMING_REG1:
-        return bus->timing[0];
+        value = bus->timing[0];
+        break;
       case I2CD_AC_TIMING_REG2:
-        return bus->timing[1];
+        value = bus->timing[1];
+        break;
       case I2CD_INTR_CTRL_REG:
-        return bus->intr_ctrl;
+        value = bus->intr_ctrl;
+        break;
       case I2CD_INTR_STS_REG:
-        return bus->intr_status;
+        value = bus->intr_status;
+        break;
       case I2CD_POOL_CTRL_REG:
-        return bus->pool_ctrl;
+        value = bus->pool_ctrl;
+        break;
       case I2CD_BYTE_BUF_REG:
-        return bus->buf;
+        value = bus->buf;
+        break;
       case I2CD_CMD_REG:
-        return bus->cmd | (i2c_bus_busy(bus->bus) << 16);
+        value = bus->cmd | (i2c_bus_busy(bus->bus) << 16);
+        break;
       case I2CD_DMA_ADDR:
           if (!aic->has_dma) {
               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  
__func__);
-            return -1;
+            break;
           }
-        return bus->dma_addr;
+        value = bus->dma_addr;
+        break;
       case I2CD_DMA_LEN:
           if (!aic->has_dma) {
               qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n",  
__func__);
-            return -1;
+            break;
           }
-        return bus->dma_len;
+        value = bus->dma_len;
+        break;
+
       default:
           qemu_log_mask(LOG_GUEST_ERROR,
                         "%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, 
offset);
-        return -1;
+        value = -1;
+        break;
       }
+
+    trace_aspeed_i2c_bus_read(bus->id, offset, size, value);
+    return value;
   }
     static void aspeed_i2c_set_state(AspeedI2CBus *bus, uint8_t state)
@@ -246,6 +270,9 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t 
pool_start)
           for (i = pool_start; i < I2CD_POOL_TX_COUNT(bus->pool_ctrl); i++) {
               uint8_t *pool_base = aic->bus_pool_base(bus);
   +            trace_aspeed_i2c_bus_send("BUF", i + 1,
+                                      I2CD_POOL_TX_COUNT(bus->pool_ctrl),
+                                      pool_base[i]);
               ret = i2c_send(bus->bus, pool_base[i]);
               if (ret) {
                   break;
@@ -256,6 +283,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t 
pool_start)
           while (bus->dma_len) {
               uint8_t data;
               aspeed_i2c_dma_read(bus, &data);
+            trace_aspeed_i2c_bus_send("DMA", bus->dma_len, bus->dma_len, data);
               ret = i2c_send(bus->bus, data);
               if (ret) {
                   break;
@@ -263,6 +291,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t 
pool_start)
           }
           bus->cmd &= ~I2CD_TX_DMA_ENABLE;
       } else {
+        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1, bus->buf);
           ret = i2c_send(bus->bus, bus->buf);
       }
   @@ -281,6 +310,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
             for (i = 0; i < I2CD_POOL_RX_SIZE(bus->pool_ctrl); i++) {
               pool_base[i] = i2c_recv(bus->bus);
+            trace_aspeed_i2c_bus_recv("BUF", i + 1,
+                                      I2CD_POOL_RX_SIZE(bus->pool_ctrl),
+                                      pool_base[i]);
           }
             /* Update RX count */
@@ -294,6 +326,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
               MemTxResult result;
                 data = i2c_recv(bus->bus);
+            trace_aspeed_i2c_bus_recv("DMA", bus->dma_len, bus->dma_len, data);
               result = address_space_write(&s->dram_as, bus->dma_addr,
                                            MEMTXATTRS_UNSPECIFIED, &data, 1);
               if (result != MEMTX_OK) {
@@ -307,6 +340,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
           bus->cmd &= ~I2CD_RX_DMA_ENABLE;
       } else {
           data = i2c_recv(bus->bus);
+        trace_aspeed_i2c_bus_recv("BYTE", 1, 1, bus->buf);
           bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
       }
   }
@@ -364,6 +398,33 @@ static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
       return true;
   }
   +static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
+{
+    g_autofree char *cmd_flags;
+    uint32_t count;
+
+    if (bus->cmd & (I2CD_RX_BUFF_ENABLE | I2CD_RX_BUFF_ENABLE)) {
+        count = I2CD_POOL_TX_COUNT(bus->pool_ctrl);
+    } else if (bus->cmd & (I2CD_RX_DMA_ENABLE | I2CD_RX_DMA_ENABLE)) {
+        count = bus->dma_len;
+    } else { /* BYTE mode */
+        count = 1;
+    }
+
+    cmd_flags = g_strdup_printf("%s%s%s%s%s%s%s%s%s",
+                                bus->cmd & I2CD_M_START_CMD ? "start|" : "",
+                                bus->cmd & I2CD_RX_DMA_ENABLE ? "rxdma|" : "",
+                                bus->cmd & I2CD_TX_DMA_ENABLE ? "txdma|" : "",
+                                bus->cmd & I2CD_RX_BUFF_ENABLE ? "rxbuf|" : "",
+                                bus->cmd & I2CD_TX_BUFF_ENABLE ? "txbuf|" : "",
+                                bus->cmd & I2CD_M_TX_CMD ? "tx|" : "",
+                                bus->cmd & I2CD_M_RX_CMD ? "rx|" : "",
+                                bus->cmd & I2CD_M_S_RX_CMD_LAST ? "last|" : "",
+                                bus->cmd & I2CD_M_STOP_CMD ? "stop" : "");
+
+    trace_aspeed_i2c_bus_cmd(bus->cmd, cmd_flags, count, bus->intr_status);

Hard to switch habits and review code using g_autofree :/

+}
+
   /*
    * The state machine needs some refinement. It is only used to track
    * invalid STOP commands for the moment.
@@ -379,6 +440,10 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, 
uint64_t value)
           return;
       }
   +    if (trace_event_get_state_backends(TRACE_ASPEED_I2C_BUS_CMD)) {
+        aspeed_i2c_bus_cmd_dump(bus);
+    }
+
       if (bus->cmd & I2CD_M_START_CMD) {
           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
               I2CD_MSTARTR : I2CD_MSTART;
@@ -465,6 +530,8 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr 
offset,
       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
       bool handle_rx;
   +    trace_aspeed_i2c_bus_write(bus->id, offset, size, value);
+
       switch (offset) {
       case I2CD_FUN_CTRL_REG:
           if (value & I2CD_SLAVE_EN) {
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index e1c810d5bd08..08db8fa68924 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -5,3 +5,12 @@
   i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
   i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
   i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
+
+# aspeed_i2c.c
+
+aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t 
intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
+aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, 
const char *str3, const char *str4, const char *str5) "handled intr=0x%x 
%s%s%s%s%s"

There are various trace backends, your output seems designed only for the "log" 
backend.

Using 'unsigned is_nak, unsigned is_ack, ...' "nak:%u ack:%u ..." would make 
your event compatible with the other backends (and ease their parsing).

I am not sure to understand where the incompatibility is.
Could you explain more please ?

Well, the format you used is not *incompatible*, but it is not optimal.

The aspeed_i2c_bus_raise_interrupt() trace event might be OK,
although the arguments could be better named.

The DTrace generated script is:

probe qemu.system.arm.log.aspeed_i2c_bus_raise_interrupt = qemu.system.arm.aspeed_i2c_bus_raise_interrupt ?
{
    try {
        argstr1_str = str1 ? user_string_n(str1, 512) : "<null>"
    } catch {}
    try {
        argstr2_str = str2 ? user_string_n(str2, 512) : "<null>"
    } catch {}
    try {
        argstr3_str = str3 ? user_string_n(str3, 512) : "<null>"
    } catch {}
    try {
        argstr4_str = str4 ? user_string_n(str4, 512) : "<null>"
    } catch {}
    try {
        argstr5_str = str5 ? user_string_n(str5, 512) : "<null>"
    } catch {}
printf("%d@%d aspeed_i2c_bus_raise_interrupt handled intr=0x%x %s%s%s%s%s\n", pid(), gettimeofday_ns(), intr_status, argstr1_str, argstr2_str, argstr3_str, argstr4_str, argstr5_str)
}

Acceptable.

The aspeed_i2c_bus_cmd() event is the one that bugged me, because
thinking about tracing a particular set of commands (like: "all
the commands with the rxdma bit set") I thought we'd need to parse
the 'const char *cmd_flags'. But now I realize you also pass the
bus->cmd value, so we can filter there, and ignore the description
string. Same occurs with the other event, we can parse intr_status.

Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>


Thanks,

C.

+aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: 
To 0x%" PRIx64 " of size %u: 0x%" PRIx64
+aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) 
"bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
+aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send 
%d/%d 0x%02x"
+aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv 
%d/%d 0x%02x"



Reply via email to