Re: [RFC] aspeed/i2c: multi-master between SoC's
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 <-> 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.
Re: [RFC] aspeed/i2c: multi-master between SoC's
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().
[RFC] aspeed/i2c: multi-master between SoC's
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: 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, ); -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,