* [PATCH 1/5] aspeed/i2c: Add support for pool buffer transfers
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 ` 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
` (3 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-16 8:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley, Cédric Le Goater
The Aspeed I2C controller can operate in different transfer modes :
- Byte Buffer mode, using a dedicated register to transfer a
byte. This is what the model supports today.
- Pool Buffer mode, using an internal SRAM to transfer multiple
bytes in the same command sequence.
Each SoC has different SRAM characteristics. On the AST2400, 2048
bytes of SRAM are available at offset 0x800 of the controller AHB
window. The pool buffer can be configured from 1 to 256 bytes per bus.
On the AST2500, the SRAM is at offset 0x200 and the pool buffer is of
16 bytes per bus.
On the AST2600, the SRAM is at offset 0xC00 and the pool buffer is of
32 bytes per bus. It can be splitted in two for TX and RX but the
current model does not add support for it as it it unused by known
drivers.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/i2c/aspeed_i2c.h | 8 ++
hw/i2c/aspeed_i2c.c | 197 ++++++++++++++++++++++++++++++++----
2 files changed, 186 insertions(+), 19 deletions(-)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 13e01059189f..5313d07aa72f 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -32,6 +32,7 @@
OBJECT_CHECK(AspeedI2CState, (obj), TYPE_ASPEED_I2C)
#define ASPEED_I2C_NR_BUSSES 16
+#define ASPEED_I2C_MAX_POOL_SIZE 0x800
struct AspeedI2CState;
@@ -50,6 +51,7 @@ typedef struct AspeedI2CBus {
uint32_t intr_status;
uint32_t cmd;
uint32_t buf;
+ uint32_t pool_ctrl;
} AspeedI2CBus;
typedef struct AspeedI2CState {
@@ -59,6 +61,8 @@ typedef struct AspeedI2CState {
qemu_irq irq;
uint32_t intr_status;
+ MemoryRegion pool_iomem;
+ uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
} AspeedI2CState;
@@ -75,6 +79,10 @@ typedef struct AspeedI2CClass {
uint8_t reg_size;
uint8_t gap;
qemu_irq (*bus_get_irq)(AspeedI2CBus *);
+
+ uint64_t pool_size;
+ hwaddr pool_base;
+ uint8_t *(*bus_pool_base)(AspeedI2CBus *);
} AspeedI2CClass;
I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr);
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 06c119f385b8..e21f45d96868 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -35,8 +35,7 @@
/* I2C Device (Bus) Register */
#define I2CD_FUN_CTRL_REG 0x00 /* I2CD Function Control */
-#define I2CD_BUFF_SEL_MASK (0x7 << 20)
-#define I2CD_BUFF_SEL(x) (x << 20)
+#define I2CD_POOL_PAGE_SEL(x) (((x) >> 20) & 0x7) /* AST2400 */
#define I2CD_M_SDA_LOCK_EN (0x1 << 16)
#define I2CD_MULTI_MASTER_DIS (0x1 << 15)
#define I2CD_M_SCL_DRIVE_EN (0x1 << 14)
@@ -113,10 +112,12 @@
#define I2CD_SCL_O_OUT_DIR (0x1 << 12)
#define I2CD_BUS_RECOVER_CMD_EN (0x1 << 11)
#define I2CD_S_ALT_EN (0x1 << 10)
-#define I2CD_RX_DMA_ENABLE (0x1 << 9)
-#define I2CD_TX_DMA_ENABLE (0x1 << 8)
/* Command Bit */
+#define I2CD_RX_DMA_ENABLE (0x1 << 9)
+#define I2CD_TX_DMA_ENABLE (0x1 << 8)
+#define I2CD_RX_BUFF_ENABLE (0x1 << 7)
+#define I2CD_TX_BUFF_ENABLE (0x1 << 6)
#define I2CD_M_STOP_CMD (0x1 << 5)
#define I2CD_M_S_RX_CMD_LAST (0x1 << 4)
#define I2CD_M_RX_CMD (0x1 << 3)
@@ -125,7 +126,11 @@
#define I2CD_M_START_CMD (0x1)
#define I2CD_DEV_ADDR_REG 0x18 /* Slave Device Address */
-#define I2CD_BUF_CTRL_REG 0x1c /* Pool Buffer Control */
+#define I2CD_POOL_CTRL_REG 0x1c /* Pool Buffer Control */
+#define I2CD_POOL_RX_COUNT(x) (((x) >> 24) & 0xff)
+#define I2CD_POOL_RX_SIZE(x) ((((x) >> 16) & 0xff) + 1)
+#define I2CD_POOL_TX_COUNT(x) ((((x) >> 8) & 0xff) + 1)
+#define I2CD_POOL_OFFSET(x) (((x) & 0x3f) << 2) /* AST2400 */
#define I2CD_BYTE_BUF_REG 0x20 /* Transmit/Receive Byte Buffer */
#define I2CD_BYTE_BUF_TX_SHIFT 0
#define I2CD_BYTE_BUF_TX_MASK 0xff
@@ -170,6 +175,8 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
return bus->intr_ctrl;
case I2CD_INTR_STS_REG:
return bus->intr_status;
+ case I2CD_POOL_CTRL_REG:
+ return bus->pool_ctrl;
case I2CD_BYTE_BUF_REG:
return bus->buf;
case I2CD_CMD_REG:
@@ -192,14 +199,58 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
-static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
+{
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+ int ret = -1;
+ int i;
+
+ if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
+ for (i = pool_start; i < I2CD_POOL_TX_COUNT(bus->pool_ctrl); i++) {
+ uint8_t *pool_base = aic->bus_pool_base(bus);
+
+ ret = i2c_send(bus->bus, pool_base[i]);
+ if (ret) {
+ break;
+ }
+ }
+ bus->cmd &= ~I2CD_TX_BUFF_ENABLE;
+ } else {
+ ret = i2c_send(bus->bus, bus->buf);
+ }
+
+ return ret;
+}
+
+static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
{
- uint8_t ret;
+ AspeedI2CState *s = bus->controller;
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
+ uint8_t data;
+ int i;
+
+ if (bus->cmd & I2CD_RX_BUFF_ENABLE) {
+ uint8_t *pool_base = aic->bus_pool_base(bus);
+ for (i = 0; i < I2CD_POOL_RX_SIZE(bus->pool_ctrl); i++) {
+ pool_base[i] = i2c_recv(bus->bus);
+ }
+
+ /* Update RX count */
+ bus->pool_ctrl &= ~(0xff << 24);
+ bus->pool_ctrl |= (i & 0xff) << 24;
+ bus->cmd &= ~I2CD_RX_BUFF_ENABLE;
+ } else {
+ data = i2c_recv(bus->bus);
+ bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+ }
+}
+
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
aspeed_i2c_set_state(bus, I2CD_MRXD);
- ret = i2c_recv(bus->bus);
+ aspeed_i2c_bus_recv(bus);
bus->intr_status |= I2CD_INTR_RX_DONE;
- bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
i2c_nack(bus->bus);
}
@@ -207,31 +258,66 @@ static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
aspeed_i2c_set_state(bus, I2CD_MACTIVE);
}
+static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
+{
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+
+ if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
+ uint8_t *pool_base = aic->bus_pool_base(bus);
+
+ return pool_base[0];
+ } else {
+ return bus->buf;
+ }
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
*/
static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
{
+ uint8_t pool_start = 0;
+
bus->cmd &= ~0xFFFF;
bus->cmd |= value & 0xFFFF;
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
I2CD_MSTARTR : I2CD_MSTART;
+ uint8_t addr;
aspeed_i2c_set_state(bus, state);
- if (i2c_start_transfer(bus->bus, extract32(bus->buf, 1, 7),
- extract32(bus->buf, 0, 1))) {
+ addr = aspeed_i2c_get_addr(bus);
+
+ if (i2c_start_transfer(bus->bus, extract32(addr, 1, 7),
+ extract32(addr, 0, 1))) {
bus->intr_status |= I2CD_INTR_TX_NAK;
} else {
bus->intr_status |= I2CD_INTR_TX_ACK;
}
- /* START command is also a TX command, as the slave address is
- * sent on the bus */
- bus->cmd &= ~(I2CD_M_START_CMD | I2CD_M_TX_CMD);
+ bus->cmd &= ~I2CD_M_START_CMD;
+
+ /*
+ * The START command is also a TX command, as the slave
+ * address is sent on the bus. Drop the TX flag if nothing
+ * else needs to be sent in this sequence.
+ */
+ if (bus->cmd & I2CD_TX_BUFF_ENABLE) {
+ if (I2CD_POOL_TX_COUNT(bus->pool_ctrl) == 1) {
+ bus->cmd &= ~I2CD_M_TX_CMD;
+ } else {
+ /*
+ * Increase the start index in the TX pool buffer to
+ * skip the address byte.
+ */
+ pool_start++;
+ }
+ } else {
+ bus->cmd &= ~I2CD_M_TX_CMD;
+ }
/* No slave found */
if (!i2c_bus_busy(bus->bus)) {
@@ -242,7 +328,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
if (bus->cmd & I2CD_M_TX_CMD) {
aspeed_i2c_set_state(bus, I2CD_MTXD);
- if (i2c_send(bus->bus, bus->buf)) {
+ if (aspeed_i2c_bus_send(bus, pool_start)) {
bus->intr_status |= (I2CD_INTR_TX_NAK);
i2c_end_transfer(bus->bus);
} else {
@@ -313,6 +399,11 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
__func__);
break;
+ case I2CD_POOL_CTRL_REG:
+ bus->pool_ctrl &= ~0xffffff;
+ bus->pool_ctrl |= (value & 0xffffff);
+ break;
+
case I2CD_BYTE_BUF_REG:
bus->buf = (value & I2CD_BYTE_BUF_TX_MASK) << I2CD_BYTE_BUF_TX_SHIFT;
break;
@@ -378,10 +469,45 @@ static const MemoryRegionOps aspeed_i2c_ctrl_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static uint64_t aspeed_i2c_pool_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ AspeedI2CState *s = opaque;
+ uint64_t ret = 0;
+ int i;
+
+ for (i = 0; i < size; i++) {
+ ret |= (uint64_t) s->pool[offset + i] << (8 * i);
+ }
+
+ return ret;
+}
+
+static void aspeed_i2c_pool_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+ AspeedI2CState *s = opaque;
+ int i;
+
+ for (i = 0; i < size; i++) {
+ s->pool[offset + i] = (value >> (8 * i)) & 0xFF;
+ }
+}
+
+static const MemoryRegionOps aspeed_i2c_pool_ops = {
+ .read = aspeed_i2c_pool_read,
+ .write = aspeed_i2c_pool_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ },
+};
+
static const VMStateDescription aspeed_i2c_bus_vmstate = {
.name = TYPE_ASPEED_I2C,
- .version_id = 1,
- .minimum_version_id = 1,
+ .version_id = 2,
+ .minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_UINT8(id, AspeedI2CBus),
VMSTATE_UINT32(ctrl, AspeedI2CBus),
@@ -390,19 +516,21 @@ static const VMStateDescription aspeed_i2c_bus_vmstate = {
VMSTATE_UINT32(intr_status, AspeedI2CBus),
VMSTATE_UINT32(cmd, AspeedI2CBus),
VMSTATE_UINT32(buf, AspeedI2CBus),
+ VMSTATE_UINT32(pool_ctrl, AspeedI2CBus),
VMSTATE_END_OF_LIST()
}
};
static const VMStateDescription aspeed_i2c_vmstate = {
.name = TYPE_ASPEED_I2C,
- .version_id = 1,
- .minimum_version_id = 1,
+ .version_id = 2,
+ .minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_UINT32(intr_status, AspeedI2CState),
VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
AspeedI2CBus),
+ VMSTATE_UINT8_ARRAY(pool, AspeedI2CState, ASPEED_I2C_MAX_POOL_SIZE),
VMSTATE_END_OF_LIST()
}
};
@@ -472,6 +600,10 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(&s->iomem, aic->reg_size * (i + offset),
&s->busses[i].mr);
}
+
+ memory_region_init_io(&s->pool_iomem, OBJECT(s), &aspeed_i2c_pool_ops, s,
+ "aspeed.i2c-pool", aic->pool_size);
+ memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
}
static void aspeed_i2c_class_init(ObjectClass *klass, void *data)
@@ -498,6 +630,14 @@ static qemu_irq aspeed_2400_i2c_bus_get_irq(AspeedI2CBus *bus)
return bus->controller->irq;
}
+static uint8_t *aspeed_2400_i2c_bus_pool_base(AspeedI2CBus *bus)
+{
+ uint8_t *pool_page =
+ &bus->controller->pool[I2CD_POOL_PAGE_SEL(bus->ctrl) * 0x100];
+
+ return &pool_page[I2CD_POOL_OFFSET(bus->pool_ctrl)];
+}
+
static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -509,6 +649,9 @@ static void aspeed_2400_i2c_class_init(ObjectClass *klass, void *data)
aic->reg_size = 0x40;
aic->gap = 7;
aic->bus_get_irq = aspeed_2400_i2c_bus_get_irq;
+ aic->pool_size = 0x800;
+ aic->pool_base = 0x800;
+ aic->bus_pool_base = aspeed_2400_i2c_bus_pool_base;
}
static const TypeInfo aspeed_2400_i2c_info = {
@@ -522,6 +665,11 @@ static qemu_irq aspeed_2500_i2c_bus_get_irq(AspeedI2CBus *bus)
return bus->controller->irq;
}
+static uint8_t *aspeed_2500_i2c_bus_pool_base(AspeedI2CBus *bus)
+{
+ return &bus->controller->pool[bus->id * 0x10];
+}
+
static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -533,6 +681,9 @@ static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
aic->reg_size = 0x40;
aic->gap = 7;
aic->bus_get_irq = aspeed_2500_i2c_bus_get_irq;
+ aic->pool_size = 0x100;
+ aic->pool_base = 0x200;
+ aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
}
static const TypeInfo aspeed_2500_i2c_info = {
@@ -546,6 +697,11 @@ static qemu_irq aspeed_2600_i2c_bus_get_irq(AspeedI2CBus *bus)
return bus->irq;
}
+static uint8_t *aspeed_2600_i2c_bus_pool_base(AspeedI2CBus *bus)
+{
+ return &bus->controller->pool[bus->id * 0x20];
+}
+
static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -557,6 +713,9 @@ static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
aic->reg_size = 0x80;
aic->gap = -1; /* no gap */
aic->bus_get_irq = aspeed_2600_i2c_bus_get_irq;
+ aic->pool_size = 0x200;
+ aic->pool_base = 0xC00;
+ aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
}
static const TypeInfo aspeed_2600_i2c_info = {
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] aspeed/i2c: Add support for pool buffer transfers
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
1 sibling, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2019-10-16 11:24 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Maydell, Jae Hyun Yoo, Andrew Jeffery, Eddie James,
QEMU Developers, qemu-arm
On Wed, 16 Oct 2019 at 08:50, Cédric Le Goater <clg@kaod.org> wrote:
>
> The Aspeed I2C controller can operate in different transfer modes :
>
> - Byte Buffer mode, using a dedicated register to transfer a
> byte. This is what the model supports today.
>
> - Pool Buffer mode, using an internal SRAM to transfer multiple
> bytes in the same command sequence.
>
> Each SoC has different SRAM characteristics. On the AST2400, 2048
> bytes of SRAM are available at offset 0x800 of the controller AHB
> window. The pool buffer can be configured from 1 to 256 bytes per bus.
>
> On the AST2500, the SRAM is at offset 0x200 and the pool buffer is of
> 16 bytes per bus.
>
> On the AST2600, the SRAM is at offset 0xC00 and the pool buffer is of
> 32 bytes per bus. It can be splitted in two for TX and RX but the
> current model does not add support for it as it it unused by known
> drivers.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] aspeed/i2c: Add support for pool buffer transfers
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
1 sibling, 0 replies; 20+ messages in thread
From: Jae Hyun Yoo @ 2019-10-16 19:02 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Eddie James, qemu-arm, Joel Stanley, qemu-devel
On 10/16/2019 1:50 AM, Cédric Le Goater wrote:
> The Aspeed I2C controller can operate in different transfer modes :
>
> - Byte Buffer mode, using a dedicated register to transfer a
> byte. This is what the model supports today.
>
> - Pool Buffer mode, using an internal SRAM to transfer multiple
> bytes in the same command sequence.
>
> Each SoC has different SRAM characteristics. On the AST2400, 2048
> bytes of SRAM are available at offset 0x800 of the controller AHB
> window. The pool buffer can be configured from 1 to 256 bytes per bus.
>
> On the AST2500, the SRAM is at offset 0x200 and the pool buffer is of
> 16 bytes per bus.
>
> On the AST2600, the SRAM is at offset 0xC00 and the pool buffer is of
> 32 bytes per bus. It can be splitted in two for TX and RX but the
> current model does not add support for it as it it unused by known
> drivers.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] aspeed/i2c: Check SRAM enablement on A2500
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 8:50 ` 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
` (2 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-16 8:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley, Cédric Le Goater
The SRAM must be enabled before using the Buffer Pool mode or the DMA
mode. This is not required on other SoCs.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/i2c/aspeed_i2c.h | 3 +++
hw/i2c/aspeed_i2c.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 5313d07aa72f..7a555072dfbf 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -61,6 +61,7 @@ typedef struct AspeedI2CState {
qemu_irq irq;
uint32_t intr_status;
+ uint32_t ctrl_global;
MemoryRegion pool_iomem;
uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
@@ -83,6 +84,8 @@ typedef struct AspeedI2CClass {
uint64_t pool_size;
hwaddr pool_base;
uint8_t *(*bus_pool_base)(AspeedI2CBus *);
+ bool check_sram;
+
} AspeedI2CClass;
I2CBus *aspeed_i2c_get_bus(DeviceState *dev, int busnr);
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index e21f45d96868..c7929aa2850f 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -31,6 +31,8 @@
#define I2C_CTRL_STATUS 0x00 /* Device Interrupt Status */
#define I2C_CTRL_ASSIGN 0x08 /* Device Interrupt Target
Assignment */
+#define I2C_CTRL_GLOBAL 0x0C /* Global Control Register */
+#define I2C_CTRL_SRAM_EN BIT(0)
/* I2C Device (Bus) Register */
@@ -271,6 +273,29 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
}
}
+static bool aspeed_i2c_check_sram(AspeedI2CBus *bus)
+{
+ AspeedI2CState *s = bus->controller;
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(s);
+
+ if (!aic->check_sram) {
+ return true;
+ }
+
+ /*
+ * AST2500: SRAM must be enabled before using the Buffer Pool or
+ * DMA mode.
+ */
+ if (!(s->ctrl_global & I2C_CTRL_SRAM_EN) &&
+ (bus->cmd & (I2CD_RX_DMA_ENABLE | I2CD_TX_DMA_ENABLE |
+ I2CD_RX_BUFF_ENABLE | I2CD_TX_BUFF_ENABLE))) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: SRAM is not enabled\n", __func__);
+ return false;
+ }
+
+ return true;
+}
+
/*
* The state machine needs some refinement. It is only used to track
* invalid STOP commands for the moment.
@@ -282,6 +307,10 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
bus->cmd &= ~0xFFFF;
bus->cmd |= value & 0xFFFF;
+ if (!aspeed_i2c_check_sram(bus)) {
+ return;
+ }
+
if (bus->cmd & I2CD_M_START_CMD) {
uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
I2CD_MSTARTR : I2CD_MSTART;
@@ -436,6 +465,8 @@ static uint64_t aspeed_i2c_ctrl_read(void *opaque, hwaddr offset,
switch (offset) {
case I2C_CTRL_STATUS:
return s->intr_status;
+ case I2C_CTRL_GLOBAL:
+ return s->ctrl_global;
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
__func__, offset);
@@ -448,7 +479,12 @@ static uint64_t aspeed_i2c_ctrl_read(void *opaque, hwaddr offset,
static void aspeed_i2c_ctrl_write(void *opaque, hwaddr offset,
uint64_t value, unsigned size)
{
+ AspeedI2CState *s = opaque;
+
switch (offset) {
+ case I2C_CTRL_GLOBAL:
+ s->ctrl_global = value;
+ break;
case I2C_CTRL_STATUS:
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
@@ -684,6 +720,7 @@ static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
aic->pool_size = 0x100;
aic->pool_base = 0x200;
aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
+ aic->check_sram = true;
}
static const TypeInfo aspeed_2500_i2c_info = {
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] aspeed/i2c: Check SRAM enablement on A2500
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
1 sibling, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2019-10-16 11:24 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Maydell, Jae Hyun Yoo, Andrew Jeffery, Eddie James,
QEMU Developers, qemu-arm
On Wed, 16 Oct 2019 at 08:50, Cédric Le Goater <clg@kaod.org> wrote:
>
> The SRAM must be enabled before using the Buffer Pool mode or the DMA
> mode. This is not required on other SoCs.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] aspeed/i2c: Check SRAM enablement on A2500
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
1 sibling, 0 replies; 20+ messages in thread
From: Jae Hyun Yoo @ 2019-10-16 19:03 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Eddie James, qemu-arm, Joel Stanley, qemu-devel
On 10/16/2019 1:50 AM, Cédric Le Goater wrote:
> The SRAM must be enabled before using the Buffer Pool mode or the DMA
> mode. This is not required on other SoCs.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] aspeed: Add a DRAM memory region at the SoC level
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 8:50 ` [PATCH 2/5] aspeed/i2c: Check SRAM enablement on A2500 Cédric Le Goater
@ 2019-10-16 8:50 ` Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
` (2 more replies)
2019-10-16 8:50 ` [PATCH 4/5] aspeed/i2c: Add support for DMA transfers Cédric Le Goater
2019-10-16 8:50 ` [PATCH 5/5] aspeed/i2c: Add trace events Cédric Le Goater
4 siblings, 3 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-16 8:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley, Cédric Le Goater
Currently, we link the DRAM memory region to the FMC model (for DMAs)
through a property alias at the SoC level. The I2C model will need a
similar region for DMA support, add a DRAM region property at the SoC
level for both model to use.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/arm/aspeed_soc.h | 1 +
hw/arm/aspeed_ast2600.c | 7 +++++--
hw/arm/aspeed_soc.c | 9 +++++++--
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index cccb684a19bb..3375ef91607f 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -40,6 +40,7 @@ typedef struct AspeedSoCState {
ARMCPU cpu[ASPEED_CPUS_NUM];
uint32_t num_cpus;
A15MPPrivState a7mpcore;
+ MemoryRegion *dram_mr;
MemoryRegion sram;
AspeedVICState vic;
AspeedRtcState rtc;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 931887ac681f..a403c2aae067 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -158,8 +158,6 @@ static void aspeed_soc_ast2600_init(Object *obj)
typename);
object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
&error_abort);
- object_property_add_alias(obj, "dram", OBJECT(&s->fmc), "dram",
- &error_abort);
for (i = 0; i < sc->spis_num; i++) {
snprintf(typename, sizeof(typename), "aspeed.spi%d-%s", i + 1, socname);
@@ -362,6 +360,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
}
/* FMC, The number of CS is set at the board level */
+ object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
"sdram-base", &err);
if (err) {
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index f4fe243458fd..dd1ee0e3336d 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -175,8 +175,6 @@ static void aspeed_soc_init(Object *obj)
typename);
object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
&error_abort);
- object_property_add_alias(obj, "dram", OBJECT(&s->fmc), "dram",
- &error_abort);
for (i = 0; i < sc->spis_num; i++) {
snprintf(typename, sizeof(typename), "aspeed.spi%d-%s", i + 1, socname);
@@ -323,6 +321,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
aspeed_soc_get_irq(s, ASPEED_I2C));
/* FMC, The number of CS is set at the board level */
+ object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
"sdram-base", &err);
if (err) {
@@ -429,6 +432,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
+ DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
+ MemoryRegion *),
DEFINE_PROP_END_OF_LIST(),
};
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/5] aspeed: Add a DRAM memory region at the SoC level
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é
2 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2019-10-16 11:24 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Maydell, Jae Hyun Yoo, Andrew Jeffery, Eddie James,
QEMU Developers, qemu-arm
On Wed, 16 Oct 2019 at 08:50, Cédric Le Goater <clg@kaod.org> wrote:
>
> Currently, we link the DRAM memory region to the FMC model (for DMAs)
> through a property alias at the SoC level. The I2C model will need a
> similar region for DMA support, add a DRAM region property at the SoC
> level for both model to use.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] aspeed: Add a DRAM memory region at the SoC level
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é
2 siblings, 0 replies; 20+ messages in thread
From: Jae Hyun Yoo @ 2019-10-16 19:03 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Eddie James, qemu-arm, Joel Stanley, qemu-devel
On 10/16/2019 1:50 AM, Cédric Le Goater wrote:
> Currently, we link the DRAM memory region to the FMC model (for DMAs)
> through a property alias at the SoC level. The I2C model will need a
> similar region for DMA support, add a DRAM region property at the SoC
> level for both model to use.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] aspeed: Add a DRAM memory region at the SoC level
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é
2 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-22 17:36 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley
On 10/16/19 10:50 AM, Cédric Le Goater wrote:
> Currently, we link the DRAM memory region to the FMC model (for DMAs)
> through a property alias at the SoC level. The I2C model will need a
> similar region for DMA support, add a DRAM region property at the SoC
> level for both model to use.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> include/hw/arm/aspeed_soc.h | 1 +
> hw/arm/aspeed_ast2600.c | 7 +++++--
> hw/arm/aspeed_soc.c | 9 +++++++--
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index cccb684a19bb..3375ef91607f 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -40,6 +40,7 @@ typedef struct AspeedSoCState {
> ARMCPU cpu[ASPEED_CPUS_NUM];
> uint32_t num_cpus;
> A15MPPrivState a7mpcore;
> + MemoryRegion *dram_mr;
> MemoryRegion sram;
> AspeedVICState vic;
> AspeedRtcState rtc;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 931887ac681f..a403c2aae067 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -158,8 +158,6 @@ static void aspeed_soc_ast2600_init(Object *obj)
> typename);
> object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
> &error_abort);
> - object_property_add_alias(obj, "dram", OBJECT(&s->fmc), "dram",
> - &error_abort);
>
> for (i = 0; i < sc->spis_num; i++) {
> snprintf(typename, sizeof(typename), "aspeed.spi%d-%s", i + 1, socname);
> @@ -362,6 +360,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> }
>
> /* FMC, The number of CS is set at the board level */
> + object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
> "sdram-base", &err);
> if (err) {
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index f4fe243458fd..dd1ee0e3336d 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -175,8 +175,6 @@ static void aspeed_soc_init(Object *obj)
> typename);
> object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
> &error_abort);
> - object_property_add_alias(obj, "dram", OBJECT(&s->fmc), "dram",
> - &error_abort);
>
> for (i = 0; i < sc->spis_num; i++) {
> snprintf(typename, sizeof(typename), "aspeed.spi%d-%s", i + 1, socname);
> @@ -323,6 +321,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> aspeed_soc_get_irq(s, ASPEED_I2C));
>
> /* FMC, The number of CS is set at the board level */
> + object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
> "sdram-base", &err);
> if (err) {
> @@ -429,6 +432,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> }
> static Property aspeed_soc_properties[] = {
> DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
> + DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
> + MemoryRegion *),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] aspeed/i2c: Add support for DMA transfers
2019-10-16 8:50 [PATCH 0/5] aspeed/i2c: Add support for pool and DMA transfer modes Cédric Le Goater
` (2 preceding siblings ...)
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 8:50 ` 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
4 siblings, 2 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-16 8:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley, Cédric Le Goater
The I2C controller of the Aspeed AST2500 and AST2600 SoCs supports DMA
transfers to and from DRAM.
A pair of registers defines the buffer address and the length of the
DMA transfer. The address should be aligned on 4 bytes and the maximum
length should not exceed 4K. The receive or transmit DMA transfer can
then be initiated with specific bits in the Command/Status register of
the controller.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/i2c/aspeed_i2c.h | 5 ++
hw/arm/aspeed_ast2600.c | 5 ++
hw/arm/aspeed_soc.c | 5 ++
hw/i2c/aspeed_i2c.c | 126 +++++++++++++++++++++++++++++++++++-
4 files changed, 138 insertions(+), 3 deletions(-)
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 7a555072dfbf..f1b9e5bf91e2 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -52,6 +52,8 @@ typedef struct AspeedI2CBus {
uint32_t cmd;
uint32_t buf;
uint32_t pool_ctrl;
+ uint32_t dma_addr;
+ uint32_t dma_len;
} AspeedI2CBus;
typedef struct AspeedI2CState {
@@ -66,6 +68,8 @@ typedef struct AspeedI2CState {
uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
+ MemoryRegion *dram_mr;
+ AddressSpace dram_as;
} AspeedI2CState;
#define ASPEED_I2C_CLASS(klass) \
@@ -85,6 +89,7 @@ typedef struct AspeedI2CClass {
hwaddr pool_base;
uint8_t *(*bus_pool_base)(AspeedI2CBus *);
bool check_sram;
+ bool has_dma;
} AspeedI2CClass;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index a403c2aae067..0881eb25983e 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -343,6 +343,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
}
/* I2C */
+ object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
object_property_set_bool(OBJECT(&s->i2c), true, "realized", &err);
if (err) {
error_propagate(errp, err);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index dd1ee0e3336d..b01c97744196 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -311,6 +311,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
}
/* I2C */
+ object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
object_property_set_bool(OBJECT(&s->i2c), true, "realized", &err);
if (err) {
error_propagate(errp, err);
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c7929aa2850f..030d9c56be65 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -23,8 +23,11 @@
#include "migration/vmstate.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
#include "hw/i2c/aspeed_i2c.h"
#include "hw/irq.h"
+#include "hw/qdev-properties.h"
/* I2C Global Register */
@@ -138,7 +141,8 @@
#define I2CD_BYTE_BUF_TX_MASK 0xff
#define I2CD_BYTE_BUF_RX_SHIFT 8
#define I2CD_BYTE_BUF_RX_MASK 0xff
-
+#define I2CD_DMA_ADDR 0x24 /* DMA Buffer Address */
+#define I2CD_DMA_LEN 0x28 /* DMA Transfer Length < 4KB */
static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
{
@@ -165,6 +169,7 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
unsigned size)
{
AspeedI2CBus *bus = opaque;
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
switch (offset) {
case I2CD_FUN_CTRL_REG:
@@ -183,6 +188,18 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
return bus->buf;
case I2CD_CMD_REG:
return bus->cmd | (i2c_bus_busy(bus->bus) << 16);
+ case I2CD_DMA_ADDR:
+ if (!aic->has_dma) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n", __func__);
+ return -1;
+ }
+ return bus->dma_addr;
+ case I2CD_DMA_LEN:
+ if (!aic->has_dma) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n", __func__);
+ return -1;
+ }
+ return bus->dma_len;
default:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset);
@@ -201,6 +218,24 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
}
+static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
+{
+ MemTxResult result;
+ AspeedI2CState *s = bus->controller;
+
+ result = address_space_read(&s->dram_as, bus->dma_addr,
+ MEMTXATTRS_UNSPECIFIED, data, 1);
+ if (result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
+ __func__, bus->dma_addr);
+ return -1;
+ }
+
+ bus->dma_addr++;
+ bus->dma_len--;
+ return 0;
+}
+
static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
{
AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
@@ -217,6 +252,16 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
}
}
bus->cmd &= ~I2CD_TX_BUFF_ENABLE;
+ } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+ while (bus->dma_len) {
+ uint8_t data;
+ aspeed_i2c_dma_read(bus, &data);
+ ret = i2c_send(bus->bus, data);
+ if (ret) {
+ break;
+ }
+ }
+ bus->cmd &= ~I2CD_TX_DMA_ENABLE;
} else {
ret = i2c_send(bus->bus, bus->buf);
}
@@ -242,6 +287,24 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
bus->pool_ctrl &= ~(0xff << 24);
bus->pool_ctrl |= (i & 0xff) << 24;
bus->cmd &= ~I2CD_RX_BUFF_ENABLE;
+ } else if (bus->cmd & I2CD_RX_DMA_ENABLE) {
+ uint8_t data;
+
+ while (bus->dma_len) {
+ MemTxResult result;
+
+ data = i2c_recv(bus->bus);
+ result = address_space_write(&s->dram_as, bus->dma_addr,
+ MEMTXATTRS_UNSPECIFIED, &data, 1);
+ if (result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
+ __func__, bus->dma_addr);
+ return;
+ }
+ bus->dma_addr++;
+ bus->dma_len--;
+ }
+ bus->cmd &= ~I2CD_RX_DMA_ENABLE;
} else {
data = i2c_recv(bus->bus);
bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
@@ -268,6 +331,11 @@ static uint8_t aspeed_i2c_get_addr(AspeedI2CBus *bus)
uint8_t *pool_base = aic->bus_pool_base(bus);
return pool_base[0];
+ } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+ uint8_t data;
+
+ aspeed_i2c_dma_read(bus, &data);
+ return data;
} else {
return bus->buf;
}
@@ -344,6 +412,10 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
*/
pool_start++;
}
+ } else if (bus->cmd & I2CD_TX_DMA_ENABLE) {
+ if (bus->dma_len == 0) {
+ bus->cmd &= ~I2CD_M_TX_CMD;
+ }
} else {
bus->cmd &= ~I2CD_M_TX_CMD;
}
@@ -447,9 +519,35 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
break;
}
+ if (!aic->has_dma &&
+ value & (I2CD_RX_DMA_ENABLE | I2CD_TX_DMA_ENABLE)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n", __func__);
+ break;
+ }
+
aspeed_i2c_bus_handle_cmd(bus, value);
aspeed_i2c_bus_raise_interrupt(bus);
break;
+ case I2CD_DMA_ADDR:
+ if (!aic->has_dma) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n", __func__);
+ break;
+ }
+
+ bus->dma_addr = value & 0xfffffffc;
+ break;
+
+ case I2CD_DMA_LEN:
+ if (!aic->has_dma) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA support\n", __func__);
+ break;
+ }
+
+ bus->dma_len = value & 0xfff;
+ if (!bus->dma_len) {
+ qemu_log_mask(LOG_UNIMP, "%s: invalid DMA length\n", __func__);
+ }
+ break;
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
@@ -542,8 +640,8 @@ static const MemoryRegionOps aspeed_i2c_pool_ops = {
static const VMStateDescription aspeed_i2c_bus_vmstate = {
.name = TYPE_ASPEED_I2C,
- .version_id = 2,
- .minimum_version_id = 2,
+ .version_id = 3,
+ .minimum_version_id = 3,
.fields = (VMStateField[]) {
VMSTATE_UINT8(id, AspeedI2CBus),
VMSTATE_UINT32(ctrl, AspeedI2CBus),
@@ -553,6 +651,8 @@ static const VMStateDescription aspeed_i2c_bus_vmstate = {
VMSTATE_UINT32(cmd, AspeedI2CBus),
VMSTATE_UINT32(buf, AspeedI2CBus),
VMSTATE_UINT32(pool_ctrl, AspeedI2CBus),
+ VMSTATE_UINT32(dma_addr, AspeedI2CBus),
+ VMSTATE_UINT32(dma_len, AspeedI2CBus),
VMSTATE_END_OF_LIST()
}
};
@@ -584,6 +684,8 @@ static void aspeed_i2c_reset(DeviceState *dev)
s->busses[i].intr_status = 0;
s->busses[i].cmd = 0;
s->busses[i].buf = 0;
+ s->busses[i].dma_addr = 0;
+ s->busses[i].dma_len = 0;
i2c_end_transfer(s->busses[i].bus);
}
}
@@ -640,14 +742,30 @@ static void aspeed_i2c_realize(DeviceState *dev, Error **errp)
memory_region_init_io(&s->pool_iomem, OBJECT(s), &aspeed_i2c_pool_ops, s,
"aspeed.i2c-pool", aic->pool_size);
memory_region_add_subregion(&s->iomem, aic->pool_base, &s->pool_iomem);
+
+ if (aic->has_dma) {
+ if (!s->dram_mr) {
+ error_setg(errp, TYPE_ASPEED_I2C ": 'dram' link not set");
+ return;
+ }
+
+ address_space_init(&s->dram_as, s->dram_mr, "dma-dram");
+ }
}
+static Property aspeed_i2c_properties[] = {
+ DEFINE_PROP_LINK("dram", AspeedI2CState, dram_mr,
+ TYPE_MEMORY_REGION, MemoryRegion *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void aspeed_i2c_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &aspeed_i2c_vmstate;
dc->reset = aspeed_i2c_reset;
+ dc->props = aspeed_i2c_properties;
dc->realize = aspeed_i2c_realize;
dc->desc = "Aspeed I2C Controller";
}
@@ -721,6 +839,7 @@ static void aspeed_2500_i2c_class_init(ObjectClass *klass, void *data)
aic->pool_base = 0x200;
aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
aic->check_sram = true;
+ aic->has_dma = true;
}
static const TypeInfo aspeed_2500_i2c_info = {
@@ -753,6 +872,7 @@ static void aspeed_2600_i2c_class_init(ObjectClass *klass, void *data)
aic->pool_size = 0x200;
aic->pool_base = 0xC00;
aic->bus_pool_base = aspeed_2600_i2c_bus_pool_base;
+ aic->has_dma = true;
}
static const TypeInfo aspeed_2600_i2c_info = {
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] aspeed/i2c: Add support for DMA transfers
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
1 sibling, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2019-10-16 11:24 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Maydell, Jae Hyun Yoo, Andrew Jeffery, Eddie James,
QEMU Developers, qemu-arm
On Wed, 16 Oct 2019 at 08:50, Cédric Le Goater <clg@kaod.org> wrote:
>
> The I2C controller of the Aspeed AST2500 and AST2600 SoCs supports DMA
> transfers to and from DRAM.
>
> A pair of registers defines the buffer address and the length of the
> DMA transfer. The address should be aligned on 4 bytes and the maximum
> length should not exceed 4K. The receive or transmit DMA transfer can
> then be initiated with specific bits in the Command/Status register of
> the controller.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] aspeed/i2c: Add support for DMA transfers
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
1 sibling, 0 replies; 20+ messages in thread
From: Jae Hyun Yoo @ 2019-10-16 19:03 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Eddie James, qemu-arm, Joel Stanley, qemu-devel
On 10/16/2019 1:50 AM, Cédric Le Goater wrote:
> The I2C controller of the Aspeed AST2500 and AST2600 SoCs supports DMA
> transfers to and from DRAM.
>
> A pair of registers defines the buffer address and the length of the
> DMA transfer. The address should be aligned on 4 bytes and the maximum
> length should not exceed 4K. The receive or transmit DMA transfer can
> then be initiated with specific bits in the Command/Status register of
> the controller.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] aspeed/i2c: Add trace events
2019-10-16 8:50 [PATCH 0/5] aspeed/i2c: Add support for pool and DMA transfer modes Cédric Le Goater
` (3 preceding siblings ...)
2019-10-16 8:50 ` [PATCH 4/5] aspeed/i2c: Add support for DMA transfers Cédric Le Goater
@ 2019-10-16 8:50 ` Cédric Le Goater
2019-10-16 11:24 ` Joel Stanley
` (2 more replies)
4 siblings, 3 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-16 8:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley, Cédric Le Goater
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"
+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"
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] aspeed/i2c: Add trace events
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é
2 siblings, 0 replies; 20+ messages in thread
From: Joel Stanley @ 2019-10-16 11:24 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Maydell, Jae Hyun Yoo, Andrew Jeffery, Eddie James,
QEMU Developers, qemu-arm
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>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] aspeed/i2c: Add trace events
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é
2 siblings, 0 replies; 20+ messages in thread
From: Jae Hyun Yoo @ 2019-10-16 19:05 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell
Cc: Andrew Jeffery, Eddie James, qemu-arm, Joel Stanley, qemu-devel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] aspeed/i2c: Add trace events
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
2 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-17 10:22 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Stefan Hajnoczi
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley
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"
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] aspeed/i2c: Add trace events
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é
0 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-10-17 11:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Peter Maydell, Stefan Hajnoczi
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley
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"
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] aspeed/i2c: Add trace events
2019-10-17 11:52 ` Cédric Le Goater
@ 2019-10-22 18:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-22 18:10 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Stefan Hajnoczi
Cc: Jae Hyun Yoo, Andrew Jeffery, Eddie James, qemu-devel, qemu-arm,
Joel Stanley
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"
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread