[RFC] aspeed/i2c: multi-master between SoC's

Commit Message

Peter Delevoryas July 15, 2022, 3:06 a.m. UTC

Hey Cedric, Klaus, and Corey,

So I realized something about the current state of multi-master i2c:

We can't do transfers between two Aspeed I2C controllers, e.g.  AST1030 <->
AST2600. I'm looking into this case in the new fby35 machine (which isn't even
merged yet, just in Cedric's pull request)

This is because the AspeedI2CBusSlave is only designed to receive through
i2c_send_async(). But the AspeedI2CBus master-mode transfers use i2c_send().

So, the AST2600 can't send data to the AST1030. And the AST1030 can't reply to
the AST2600.

(By the way, another small issue: AspeedI2CBusSlave expects the parent of its
parent to be its AspeedI2CBus, but that's not true if multiple SoC's are sharing
an I2CBus. But that's easy to resolve, I'll send a patch for that soon).

I'm wondering how best to resolve the multi-SoC send-async issue, while
retaining the ability to send synchronously to non-SoC slave devices.

I think there's only one way, as far as I can see:

- Force the Aspeed I2C Controller to master the I2C bus before starting a master
  transfer. Even for synchronous transfers.

This shouldn't be a big problem, we can still do synchronous transfers, we just
have to wait for the bus to be free before starting the transfer.

- If the I2C slave targets for a master2slave transfer support async_send, then
  use async_send. This requires refactoring aspeed_i2c_bus_send into a state
  machine to send data asynchronously.

In other words, don't try to do a synchronous transfer to an SoC.

But, of course, we can keep doing synchronous transfers from SoC -> sensor or
sensor -> SoC.

I see the code in aspeed_i2c_bus_send turning into something like this:


Anyways, curious to get your guys' thoughts on this. Thanks!

Peter

Comments

Klaus Jensen July 15, 2022, 7:49 a.m. UTC | #1

On Jul 14 20:06, Peter Delevoryas wrote:
> Hey Cedric, Klaus, and Corey,
> 

Hi Peter,

Regardless of the issues you are facing its awesome to see this being
put to work like this!

> So I realized something about the current state of multi-master i2c:
> 
> We can't do transfers between two Aspeed I2C controllers, e.g.  AST1030 <->
> AST2600. I'm looking into this case in the new fby35 machine (which isn't even
> merged yet, just in Cedric's pull request)
> 
> This is because the AspeedI2CBusSlave is only designed to receive through
> i2c_send_async(). But the AspeedI2CBus master-mode transfers use i2c_send().
> 
> So, the AST2600 can't send data to the AST1030. And the AST1030 can't reply to
> the AST2600.
> 
> (By the way, another small issue: AspeedI2CBusSlave expects the parent of its
> parent to be its AspeedI2CBus, but that's not true if multiple SoC's are sharing
> an I2CBus. But that's easy to resolve, I'll send a patch for that soon).
> 
> I'm wondering how best to resolve the multi-SoC send-async issue, while
> retaining the ability to send synchronously to non-SoC slave devices.
> 
> I think there's only one way, as far as I can see:
> 
> - Force the Aspeed I2C Controller to master the I2C bus before starting a master
>   transfer. Even for synchronous transfers.
> 
> This shouldn't be a big problem, we can still do synchronous transfers, we just
> have to wait for the bus to be free before starting the transfer.
> 
> - If the I2C slave targets for a master2slave transfer support async_send, then
>   use async_send. This requires refactoring aspeed_i2c_bus_send into a state
>   machine to send data asynchronously.
> 
> In other words, don't try to do a synchronous transfer to an SoC.
> 
> But, of course, we can keep doing synchronous transfers from SoC -> sensor or
> sensor -> SoC.
> 

Yeah, hmm. This is tricky because callers of bus_send expects the
transfer to be "resolved" immediately. Per design, the asynchronous send
requires the device mastering the bus to itself be asynchronous (like
the i2c-echo device I added as an example).

However, looking at aspeed_i2c_bus_handle_cmd (which is the caller of
bus_send), it should be possible to accept bus_send to "yield" as you
sketch below and not raise any interrupt. And yes, it would be required
in bus_send to call i2c_bus_master to register a BH which can then
raise the interrupt upon i2c_ack().

Peter Delevoryas July 15, 2022, 5:49 p.m. UTC | #2

On Fri, Jul 15, 2022 at 09:49:58AM +0200, Klaus Jensen wrote:
> On Jul 14 20:06, Peter Delevoryas wrote:
> > Hey Cedric, Klaus, and Corey,
> > 
> 
> Hi Peter,
> 
> Regardless of the issues you are facing its awesome to see this being
> put to work like this!

Haha yeah, well, _all_ the designs at Meta (fb) rely significantly on
multi-master i2c. I think I've been trying to get this working for months now,
but we're really close!

If I can just get the i2c layer working, then proper IPMB and MCTP testing
between BMC and BIC firmware will be much easier.

There's some part defects that have a very low frequency of occurrence, and the
patches for those defects rely on a BMC <-> BIC <-> <device> chain of IPMB
messages. With QEMU, we could test those patches much more thoroughly, because
we can inject the part-defect behavior.

> 
> > So I realized something about the current state of multi-master i2c:
> > 
> > We can't do transfers between two Aspeed I2C controllers, e.g.  AST1030 <->
> > AST2600. I'm looking into this case in the new fby35 machine (which isn't even
> > merged yet, just in Cedric's pull request)
> > 
> > This is because the AspeedI2CBusSlave is only designed to receive through
> > i2c_send_async(). But the AspeedI2CBus master-mode transfers use i2c_send().
> > 
> > So, the AST2600 can't send data to the AST1030. And the AST1030 can't reply to
> > the AST2600.
> > 
> > (By the way, another small issue: AspeedI2CBusSlave expects the parent of its
> > parent to be its AspeedI2CBus, but that's not true if multiple SoC's are sharing
> > an I2CBus. But that's easy to resolve, I'll send a patch for that soon).
> > 
> > I'm wondering how best to resolve the multi-SoC send-async issue, while
> > retaining the ability to send synchronously to non-SoC slave devices.
> > 
> > I think there's only one way, as far as I can see:
> > 
> > - Force the Aspeed I2C Controller to master the I2C bus before starting a master
> >   transfer. Even for synchronous transfers.
> > 
> > This shouldn't be a big problem, we can still do synchronous transfers, we just
> > have to wait for the bus to be free before starting the transfer.
> > 
> > - If the I2C slave targets for a master2slave transfer support async_send, then
> >   use async_send. This requires refactoring aspeed_i2c_bus_send into a state
> >   machine to send data asynchronously.
> > 
> > In other words, don't try to do a synchronous transfer to an SoC.
> > 
> > But, of course, we can keep doing synchronous transfers from SoC -> sensor or
> > sensor -> SoC.
> > 
> 
> Yeah, hmm. This is tricky because callers of bus_send expects the
> transfer to be "resolved" immediately. Per design, the asynchronous send
> requires the device mastering the bus to itself be asynchronous (like
> the i2c-echo device I added as an example).

Understood: I was ommitting other necessary changes. Yes, we would need to
async-ify all the way up the chain to the register read/write.

> 
> However, looking at aspeed_i2c_bus_handle_cmd (which is the caller of
> bus_send), it should be possible to accept bus_send to "yield" as you
> sketch below and not raise any interrupt. And yes, it would be required
> in bus_send to call i2c_bus_master to register a BH which can then
> raise the interrupt upon i2c_ack().

Yep, that's what I was thinking of. I think I would actually call i2c_bus_master
in aspeed_i2c_bus_handle_cmd or higher though, because I would only call
i2c_bus_master once until the STOP command is issued (or the DMA/pool transfer
is complete). But yeah, I think we're on the same page.

Patch

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 42c6d69b82..1ea530a77e 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -226,10 +226,17 @@  static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
     return 0;
 }
 
-static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
+typedef enum AsyncResult AsyncResult;
+enum AsyncResult {
+    DONE,
+    YIELD,
+    ERROR,
+};
+
+static AsyncResult aspeed_i2c_bus_send(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
-    int ret = -1;
+    AsyncResult ret = DONE;
     int i;
     uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
     uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
@@ -239,41 +246,49 @@  static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
                                                 TX_COUNT);
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
-        for (i = pool_start; i < pool_tx_count; i++) {
+        while (bus->pool_pos < pool_tx_count) {
             uint8_t *pool_base = aic->bus_pool_base(bus);
 
-            trace_aspeed_i2c_bus_send("BUF", i + 1, pool_tx_count,
+            trace_aspeed_i2c_bus_send("BUF", bus->pool_pos + 1, pool_tx_count,
                                       pool_base[i]);
-            ret = i2c_send(bus->bus, pool_base[i]);
-            if (ret) {
+            ret = i2c_send_async(bus->bus, pool_base[bus->pool_pos]);
+            if (ret == ERROR) {
                 break;
             }
+            bus->pool_pos++;
+            if (ret == YIELD) {
+                return YIELD;
+            }
         }
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_BUFF_EN, 0);
     } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
         /* In new mode, clear how many bytes we TXed */
-        if (aspeed_i2c_is_new_mode(bus->controller)) {
+        if (aspeed_i2c_is_new_mode(bus->controller) && bus->pool_pos == 0) {
             ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, TX_LEN, 0);
         }
         while (bus->regs[reg_dma_len]) {
             uint8_t data;
             aspeed_i2c_dma_read(bus, &data);
-            trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
+            trace_aspeed_i2c_bus_send("DMA", bus->regs[bus->pool_pos],
                                       bus->regs[reg_dma_len], data);
-            ret = i2c_send(bus->bus, data);
-            if (ret) {
+            ret = i2c_send_async(bus->bus, data);
+            if (ret == ERROR) {
                 break;
             }
+            bus->pool_pos++;
             /* In new mode, keep track of how many bytes we TXed */
             if (aspeed_i2c_is_new_mode(bus->controller)) {
                 ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN_STS, TX_LEN,
                                  ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN_STS,
                                                   TX_LEN) + 1);
             }
+            if (ret == YIELD) {
+                return YIELD;
+            }
         }
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_DMA_EN, 0);
     } else {
-        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
+        trace_aspeed_i2c_bus_send("BYTE", 1, 1,
                                   bus->regs[reg_byte_buf]);
         ret = i2c_send(bus->bus, bus->regs[reg_byte_buf]);
     }