| Message ID | 20191016085035.12136-6-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed/i2c: Add support for pool and DMA transfer modes | expand |
On Wed, 16 Oct 2019 at 08:50, Cédric Le Goater <clg@kaod.org> wrote: > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Joel Stanley <joel@jms.id.au>
On 10/16/2019 1:50 AM, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater <clg@kaod.org> Tested-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> Thanks for the implementation! -Jae
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); > +} > + > /* > * 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). > +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" >
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); >> +} >> + >> /* >> * 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" >>
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" >>> >
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" +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"
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(-)