[RFC 0/1] i2c/aspeed: Add slave device handling in new register mode

[RFC 0/1] i2c/aspeed: Add slave device handling in new register mode

[Peter Delevoryas]

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.

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(-)

-- 
2.30.2

[RFC 1/1] i2c/aspeed: Add slave device handling in new register mode

[Peter Delevoryas]

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

Re: [RFC 0/1] i2c/aspeed: Add slave device handling in new register mode

[Cédric Le Goater]

[ 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(-)
> 

RE: [RFC 0/1] i2c/aspeed: Add slave device handling in new register mode

[Troy Lee]

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Wednesday, June 1, 2022 3:10 PM
> To: Peter Delevoryas <pdel@fb.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; zhdaniel@fb.com; Troy
> Lee <troy_lee@aspeedtech.com>; Jamin Lin <jamin_lin@aspeedtech.com>;
> Steven Lee <steven_lee@aspeedtech.com>; k.jensen@samsung.com; Joe
> Komlodi <komlodi@google.com>; Joel Stanley <joel@jms.id.au>; Andrew
> Jeffery <andrew@aj.id.au>
> Subject: Re: [RFC 0/1] i2c/aspeed: Add slave device handling in new register
> mode
> 
> [ 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
> 

Yes, I'm ok with this. We have tested Joe's patch set as well.

> Then, adding slave support for old [2] and new mode (this patch) shouldn't be
> too much of a problem since they are small.
> 

I'm looking forward to have slave device support, with that we could have more firmware test case in QEMU.

Thanks,
Troy Lee

> 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&ar
> > chive=both [2]
> > https://patchwork.kernel.org/project/qemu-devel/list/?series=627914&ar
> > chive=both [3]
> > https://github.com/AspeedTech-
> BMC/zephyr/blob/db3dbcc9c52e67a47180890a
> > c938ed380b33f91c/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(-)
> >