From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: "Cédric Le Goater" <clg@kaod.org>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Stefan Hajnoczi" <stefanha@redhat.com>
Cc: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>,
Andrew Jeffery <andrew@aj.id.au>,
Eddie James <eajames@linux.vnet.ibm.com>,
qemu-devel@nongnu.org, qemu-arm@nongnu.org,
Joel Stanley <joel@jms.id.au>
Subject: Re: [PATCH 5/5] aspeed/i2c: Add trace events
Date: Tue, 22 Oct 2019 20:10:57 +0200 [thread overview]
Message-ID: <b01c82fa-54cc-9d20-16ee-dd7acc67d32d@redhat.com> (raw)
In-Reply-To: <ed9652d8-a72b-e2ff-37d7-1e004fa2b3ce@kaod.org>
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 <clg@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é <philmd@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"
>>>
>
prev parent reply other threads:[~2019-10-22 18:11 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-16 8:50 [PATCH 0/5] aspeed/i2c: Add support for pool and DMA transfer modes Cédric Le Goater
2019-10-16 8:50 ` [PATCH 1/5] aspeed/i2c: Add support for pool buffer transfers Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
2019-10-16 19:02 ` Jae Hyun Yoo
2019-10-16 8:50 ` [PATCH 2/5] aspeed/i2c: Check SRAM enablement on A2500 Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
2019-10-16 19:03 ` Jae Hyun Yoo
2019-10-16 8:50 ` [PATCH 3/5] aspeed: Add a DRAM memory region at the SoC level Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
2019-10-16 19:03 ` Jae Hyun Yoo
2019-10-22 17:36 ` Philippe Mathieu-Daudé
2019-10-16 8:50 ` [PATCH 4/5] aspeed/i2c: Add support for DMA transfers Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
2019-10-16 19:03 ` Jae Hyun Yoo
2019-10-16 8:50 ` [PATCH 5/5] aspeed/i2c: Add trace events Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
2019-10-16 19:05 ` Jae Hyun Yoo
2019-10-17 10:22 ` Philippe Mathieu-Daudé
2019-10-17 11:52 ` Cédric Le Goater
2019-10-22 18:10 ` Philippe Mathieu-Daudé [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b01c82fa-54cc-9d20-16ee-dd7acc67d32d@redhat.com \
--to=philmd@redhat.com \
--cc=andrew@aj.id.au \
--cc=clg@kaod.org \
--cc=eajames@linux.vnet.ibm.com \
--cc=jae.hyun.yoo@linux.intel.com \
--cc=joel@jms.id.au \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).