[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/5] aspeed/i2c: Add trace events
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH 5/5] aspeed/i2c: Add trace events |
Date: |
Thu, 17 Oct 2019 13:52:33 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
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 <address@hidden>
>> ---
>> 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);
>> +}
>> +
>> /*
>> * 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 ?
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"
>>