* [RFC 1/1] i2c/aspeed: Add slave device handling in new register mode
2022-05-25 20:50 [RFC 0/1] i2c/aspeed: Add slave device handling in new register mode Peter Delevoryas
@ 2022-05-25 20:50 ` Peter Delevoryas
2022-06-01 7:10 ` [RFC 0/1] " Cédric Le Goater
1 sibling, 0 replies; 4+ messages in thread
From: Peter Delevoryas @ 2022-05-25 20:50 UTC (permalink / raw)
Cc: pdel, qemu-arm, qemu-devel, zhdaniel, troy_lee, jamin_lin,
steven_lee, k.jensen
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/aspeed_i2c.c | 118 ++++++++++++++++++++++++++++++++++--
include/hw/i2c/aspeed_i2c.h | 14 +++--
2 files changed, 124 insertions(+), 8 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 3f2dbe46df..01af647e0c 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -221,6 +221,10 @@
#define I2CM_DMA_LEN 0x1c
#define I2CS_INT_CTRL_REG 0x20
#define I2CS_INT_STS_REG 0x24
+#define I2CS_PKT_DONE BIT(16)
+#define I2CS_SLAVE_MATCH BIT(7)
+#define I2CS_STOP BIT(4)
+#define I2CS_RX_DONE BIT(2)
#define I2CS_CMD_STS_REG 0x28
#define I2CS_DMA_LEN 0x2c
#define I2CM_DMA_TX_BUF 0x30
@@ -334,16 +338,38 @@ static uint64_t aspeed_i2c_bus_read_new(void *opaque, hwaddr offset,
value = (i2c_bus_busy(bus->bus) << 16);
break;
case I2CC_M_X_POOL_BUF_CTRL_REG:
+ break;
case I2CS_INT_CTRL_REG:
+ value = bus->slave_intr_ctrl;
+ break;
case I2CS_INT_STS_REG:
+ value = bus->slave_intr_status;
+ break;
case I2CS_CMD_STS_REG:
+ value = bus->slave_cmd;
+ break;
case I2CS_DMA_LEN:
+ value = bus->slave_dma_len;
+ break;
case I2CS_DMA_TX_BUF:
+ /* FIXME: Not sure if we should return same value as RX buf */
+ value = bus->slave_dma_addr;
+ break;
case I2CS_DMA_RX_BUF:
+ value = bus->slave_dma_addr;
+ break;
case I2CS_SA_REG:
+ value = bus->dev_addr;
+ break;
case I2CS_DMA_LEN_STS_REG:
+ value = bus->slave_dma_len_tx | (bus->slave_dma_len_rx << 16);
+ break;
case I2CC_DMA_OP_ADDR_REG:
+ value = bus->slave_dma_addr;
+ break;
case I2CC_DMA_OP_LEN_REG:
+ value = bus->slave_dma_len;
+ break;
default:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Bad offset 0x%" HWADDR_PRIx "\n", __func__, offset);
@@ -870,9 +896,7 @@ static void aspeed_i2c_bus_write_new(void *opaque, hwaddr offset,
switch (offset) {
case I2CC_M_S_FUNC_CTRL_REG:
if (value & I2CD_SLAVE_EN) {
- qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
- __func__);
- break;
+ i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
}
bus->ctrl = value & 0x007FFFFF;
break;
@@ -934,16 +958,44 @@ static void aspeed_i2c_bus_write_new(void *opaque, hwaddr offset,
bus->dma_len_rx = 0;
break;
case I2CC_M_X_POOL_BUF_CTRL_REG:
+ break;
case I2CS_INT_CTRL_REG:
+ bus->slave_intr_ctrl = value;
+ break;
case I2CS_INT_STS_REG:
+ if (value & I2CM_PKT_DONE) {
+ value |= 0x280b5;
+ }
+ bus->slave_intr_status &= ~value;
+ /* FIXME: Maybe need to check master interrupt status too. */
+ if (!bus->slave_intr_status) {
+ bus->controller->intr_status &= ~(1 << bus->id);
+ qemu_irq_lower(aic->bus_get_irq(bus));
+ }
+ break;
case I2CS_CMD_STS_REG:
+ assert(!(bus->slave_cmd >> 31));
+ bus->slave_cmd = value;
+ break;
+ case I2CS_SA_REG:
+ bus->dev_addr = value;
+ break;
case I2CS_DMA_LEN:
+ assert(value);
+ bus->slave_dma_len = value;
+ break;
case I2CS_DMA_TX_BUF:
case I2CS_DMA_RX_BUF:
- case I2CS_SA_REG:
+ bus->slave_dma_addr = value;
+ break;
case I2CS_DMA_LEN_STS_REG:
+ bus->slave_dma_len_tx = 0;
+ bus->slave_dma_len_rx = 0;
+ break;
case I2CC_DMA_OP_ADDR_REG:
case I2CC_DMA_OP_LEN_REG:
+ /* Invalid to write to DMA operating status registers */
+ break;
default:
break;
}
@@ -1298,11 +1350,42 @@ static const TypeInfo aspeed_i2c_info = {
.abstract = true,
};
+static int aspeed_i2c_slave_event_new(AspeedI2CBus *bus, enum i2c_event event)
+{
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+
+ switch (event) {
+ case I2C_START_SEND:
+ bus->slave_dma_len_rx = 0;
+ assert(bus->slave_dma_len_tx == 0);
+ assert(bus->slave_dma_len);
+ assert(bus->slave_dma_addr);
+ i2c_ack(bus->bus);
+ break;
+ case I2C_FINISH:
+ bus->slave_intr_status |= I2CS_PKT_DONE;
+ bus->slave_intr_status |= I2CS_SLAVE_MATCH;
+ bus->slave_intr_status |= I2CS_RX_DONE;
+ bus->slave_intr_status |= I2CS_STOP;
+ bus->controller->intr_status |= 1 << bus->id;
+ qemu_irq_raise(aic->bus_get_irq(bus));
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
{
AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
AspeedI2CBus *bus = s->bus;
+ if (aspeed_i2c_bus_is_new_mode(bus)) {
+ return aspeed_i2c_slave_event_new(bus, event);
+ }
+
switch (event) {
case I2C_START_SEND:
bus->buf = bus->dev_addr << 1;
@@ -1330,11 +1413,29 @@ static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
return 0;
}
+static void aspeed_i2c_slave_send_async_new(AspeedI2CBus *bus, uint8_t data)
+{
+ MemTxResult result = address_space_write(&bus->controller->dram_as,
+ bus->slave_dma_addr,
+ MEMTXATTRS_UNSPECIFIED, &data, 1);
+ assert(result == MEMTX_OK);
+
+ bus->slave_dma_addr++;
+ bus->slave_dma_len--;
+ bus->slave_dma_len_rx++;
+
+ i2c_ack(bus->bus);
+}
+
static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
{
AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
AspeedI2CBus *bus = s->bus;
+ if (aspeed_i2c_bus_is_new_mode(bus)) {
+ return aspeed_i2c_slave_send_async_new(bus, data);
+ }
+
bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
bus->intr_status |= I2CD_INTR_RX_DONE;
@@ -1370,6 +1471,15 @@ static void aspeed_i2c_bus_reset(DeviceState *dev)
s->buf = 0;
s->dma_addr = 0;
s->dma_len = 0;
+ s->slave_cmd = 0;
+ s->tx_state_machine = 0;
+ s->slave_dma_addr = 0;
+ s->slave_dma_len = 0;
+ s->slave_dma_len_tx = 0;
+ s->slave_dma_len_rx = 0;
+ s->slave_intr_ctrl = 0;
+ s->slave_intr_status = 0;
+
i2c_end_transfer(s->bus);
}
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 8e0671f60b..615bcb105b 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -61,12 +61,18 @@ struct AspeedI2CBus {
uint32_t pool_ctrl;
uint32_t dma_addr;
uint32_t dma_len;
-
- uint8_t tx_state_machine;
- uint32_t intr_ctrl_slave;
- uint32_t intr_status_slave;
uint32_t dma_len_tx;
uint32_t dma_len_rx;
+
+ uint8_t tx_state_machine;
+
+ uint32_t slave_cmd;
+ uint32_t slave_dma_addr;
+ uint32_t slave_dma_len;
+ uint32_t slave_dma_len_tx;
+ uint32_t slave_dma_len_rx;
+ uint32_t slave_intr_ctrl;
+ uint32_t slave_intr_status;
};
struct AspeedI2CState {
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC 0/1] i2c/aspeed: Add slave device handling in new register mode
2022-05-25 20:50 [RFC 0/1] i2c/aspeed: Add slave device handling in new register mode Peter Delevoryas
2022-05-25 20:50 ` [RFC 1/1] " Peter Delevoryas
@ 2022-06-01 7:10 ` Cédric Le Goater
2022-06-02 1:54 ` Troy Lee
1 sibling, 1 reply; 4+ messages in thread
From: Cédric Le Goater @ 2022-06-01 7:10 UTC (permalink / raw)
To: Peter Delevoryas
Cc: qemu-arm, qemu-devel, zhdaniel, troy_lee, jamin_lin, steven_lee,
k.jensen, Joe Komlodi, Joel Stanley, Andrew Jeffery
[ Adding Joe ]
On 5/25/22 22:50, Peter Delevoryas wrote:
> The AST2600/AST1030 new register mode patches[1] and the I2C slave device
> patches[2] will be really useful, but we still need DMA slave device
> handling in the new register mode too for the use-cases I'm thinking of
> (OpenBIC Zephyr kernel using Aspeed SDK drivers[3]).
>
> My test images are on Github[4]. They can be used with the ast1030-evb, or
> the oby35-cl and oby35-bb machines in the fb qemu branch[5].
>
> I'm submitting this as an RFC cause I just want to see how other people
> expect these changes to be made based on the previously submitted "new
> register mode" and "old register mode slave device" patches.
Currently, my preferred approach would be to start with Joe's patchset
because the registerfields conversion is a huge effort and it's adding
new mode support which should cover the needs for the AST1030 SoC [1].
Troy, could you please confirm this is OK with you ? I have pushed
the patches on :
https://github.com/legoater/qemu/commits/aspeed-7.1
Then, adding slave support for old [2] and new mode (this patch)
shouldn't be too much of a problem since they are small.
we lack a test case for this controller and writing a I2C Aspeed bus
driver for qtest is not an easy task.
It might be easier to start an ast2600-evb machine with a lightweight
userspace (buildroot, I can host that somewhere on GH) and run some I2C
get/set/detect commands from a python/expect framework, like avocado.
I2C devices can be added on the command line for the purpose.
Thanks,
C.
> Thanks,
> Peter
>
> [1] https://patchwork.kernel.org/project/qemu-devel/list/?series=626028&archive=both
> [2] https://patchwork.kernel.org/project/qemu-devel/list/?series=627914&archive=both
> [3] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L1362-L1368
> [4] https://github.com/peterdelevoryas/OpenBIC/releases/tag/oby35-cl-2022.13.01
> [5] https://github.com/facebook/openbmc-qemu
>
> Peter Delevoryas (1):
> i2c/aspeed: Add slave device handling in new register mode
>
> hw/i2c/aspeed_i2c.c | 118 ++++++++++++++++++++++++++++++++++--
> include/hw/i2c/aspeed_i2c.h | 14 +++--
> 2 files changed, 124 insertions(+), 8 deletions(-)
>
^ permalink raw reply [flat|nested] 4+ messages in thread