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

[Peter Delevoryas]

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.