Re: [PATCH v5 01/10] i3c: Add core I3C infrastructure

[Peter Rosin]

On 2018-06-23 12:17, Boris Brezillon wrote:
> Hi Peter,
> 
> On Fri, 22 Jun 2018 23:35:34 +0200
> Peter Rosin <peda@xxxxxxxxxx> wrote:
> 
>> On 2018-06-22 12:49, Boris Brezillon wrote:
>>> Add core infrastructure to support I3C in Linux and document it.
>>>
>>> This infrastructure is not complete yet and will be extended over
>>> time.
>>>
>>> There are a few design choices that are worth mentioning because they
>>> impact the way I3C device drivers can interact with their devices:
>>>
>>> - all functions used to send I3C/I2C frames must be called in
>>>   non-atomic context. Mainly done this way to ease implementation, but
>>>   this is still open to discussion. Please let me know if you think
>>>   it's worth considering an asynchronous model here
>>> - the bus element is a separate object and is not implicitly described
>>>   by the master (as done in I2C). The reason is that I want to be able
>>>   to handle multiple master connected to the same bus and visible to
>>>   Linux.
>>>   In this situation, we should only have one instance of the device and
>>>   not one per master, and sharing the bus object would be part of the
>>>   solution to gracefully handle this case.
>>>   I'm not sure we will ever need to deal with multiple masters
>>>   controlling the same bus and exposed under Linux, but separating the
>>>   bus and master concept is pretty easy, hence the decision to do it
>>>   like that.
>>>   The other benefit of separating the bus and master concepts is that
>>>   master devices appear under the bus directory in sysfs.  
>>
>> Are bus multiplexers relevant to I3C?
> 
> Not yet, but who knows.
> 
>> The locking needed for handling
>> muxes for I2C is, well, convoluted...
> 
> Do you remember what was the problem?
> 
> Anyway, I'd really like to have basic support upstreamed before we
> start considering advanced use cases that do not exist yet. Don't get
> me wrong, I'm not against having the multiplexer/locking discussion,
> but it should not block inclusion of the I3C subsystem.

I'm trying to avoid the unfortunate situation in I2C where there
are two slightly incompatible locking schemes for muxes. There's
probably nothing to worry about until the first I3C mux is added
though. But since I2C devices are supposedly compatible with I3C
that might be the case from day one?

---

If I read your code right, I3C client drivers will typically call
i3c_device_do_priv_xfer (instead of i2c_transfer/i2c_smbus_xfer)
and i3c_device_do_prov_xfer will grab the i3c_bus_normaluse_lock
during the transfer. This seems equivalent to normal use in
I2C with i2c_transfer/i2c_smbus_xfer.

When muxes are thrown into the mix, you find yourself needing to
do more than the "real" transfer with some lock held. In I2C there
is an unlocked __i2c_transfer, and locking/unlocking is exposed.
Muxes typically grab the lock, set the mux in the appropriate
position, do an unlocked __i2c_transfer, optionally sets the mux
in some default position and then lets go of the lock. See e.g.
i2c/muxes/i2c-mux-pca954x.c

However, that is the simple case. There are also muxes that are
controlled with GPIO pins, and that gets hairy if the GPIO pins
are controlled from the same I2C bus that is muxed. The GPIO
driver would have to know that some GPIO pins need to use unlocked
I2C transfers for that to work with the above locking scheme. And
no GPIO driver does not want to know about that at all. I.e. you
have two fundamentally different requirement depending on if the
GPIO pins controlling the mux are controlled using the muxed bus
or if the pins are controlled in some completely unrelated way.
The latter case is probably the normal case, with the GPIO mux
controlled directly from some SoC pins. In the latter case you
also want to prevent any transfer on the bus while the GPIO pins
for the mux are changing, i.e. the total opposite of the former
case. It gets really really hairy if you have multiple levels
of muxes...

There are a some old drivers (e.g. i2c/busses/i2c-amd756-s4882.c)
that handles this by simply bypassing the GPIO subsystem, but
that is not really an option if some pins are used to mux the
I2C bus while some pins are used for other things.

I don't know if this affects I3C before muxes are added, but I
suspect muxes will happen sooner rather than later, since the
spec mentions that the bus only support 11 devices maximum. 11
don't seem like a lot, and it seems likely that there will be
a need to have more devices somehow...

But just maybe, in order to not run into the above situation, it
needs to be handled right from the start with preparatory and
cleanup stages of each transfers, or something?

Cheers,
Peter