On 2020-05-15 23:19, Peter Rosin wrote:
>
>
> On 2020-05-15 10:36, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>> Hi Peter,
>>
>> W dniu 15.05.2020 o 10:02, Peter Rosin pisze:
>>>
>>>
>>> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>> Hi Wolfram, Peter,
>>>>
>>>> Thank you for the quick response.
>>>>
>>>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>>>
>>>>> Adding Peter as the mux maintainer to CC.
>>>>>
>>>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>>>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>>>>
>>>>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>>>>> Can you draw a graph and give names of chips etc...?
>>>>>
>>>>> And, of course, why on earth do you need to access the same chip from
>>>>> two masters within one Linux? :) (That's how I understood it)
>>>>>
>>>
>>> *snip* useful layout.
>>>
>>>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
>>>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
>>>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
>>>> do this kind of deeper lock on the bus.
>>>
>>> Yes, that definitely makes it clearer. So, what you need is something
>>> more complex than an I2C xfer between select/deselect. Your proposal
>>> to add a flag to not do the deselect should probably have a matching
>>> flag to not do the select on the following xfer. Otherwise that second
>>> xfer is likely to do useless work. This should probably also be two
>>> independent flags, so that you can add intermediate xfers that neither
>>> select nor deselect. But you also need an explicit deselect mechanism
>>> for use if there is a problem half way through...
>>>
>>> But, I think all that exposes the too much to the users, and while it
>>> is certainly possible (most things are), I not a huge fan of it.
>>> Maintainers of other subsystems will not know the rules, and drivers
>>> are, over time, bound to start using these facilities in half-baked
>>> ways.
>>>
>>> I would rather have some generic I2C mechanism to request more than
>>> a single xfer as a unit, such that the muxes/arbs/gates can then lock
>>> the mux/arb/gate, do the select, feed the unit to the parent adapter
>>> and finally deselect and unlock the mux/arb/gate. With the locking
>>> and selecting centralized instead of spread out to all users. This
>>> helps avoid bugs where things are not cleaned up on error paths etc.
>>>
>>
>> Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not
>> want to be disturbed by anyone on local system, you do:
>>
>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>> __i2c_transfer(client->adapter, ...);
>> some_logic
>> __ice_transfer(client->adapter, ...);
>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>>
>> right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change
>> existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in):
>
> Ok, now I see what you mean, and I realize that you were talking
> about a new flag for i2c_bus_lock() all along. Brain-fart on my
> part. Sorry.
>
> Yes, by adding the flag to i2c_bus_(un)lock() you get something that
> is much closer to existing semantics.
>
>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>> __i2c_transfer(client->adapter, ...)
>> some_logic
>> __ice_transfer(client->adapter, ...)
>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>>
>>
>> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:
>
> This is not true, you would have to do it in i2c_parent_(un)lock_bus
> too. The I2C client drivers should not need to know (or care) if the
> muxes are parent-locked or mux-locked.
>
>> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>> priv->muxc->select(muxc, priv->chan_id);
>> priv->muxc->arbitrator_selected = true;
>> }
>>
>> and unlock could do the opposite:
>> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>> priv->muxc->select(muxc, priv->chan_id);
>> priv->muxc->arbitrator_selected = true;
>
> (unselect and false, but I get it)
>
>> }
>>
>> Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set,
>> it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock
>> and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could
>> make sense for other types of muxes as well..
>
> I think it may well make sense, so skip the check and name the
> name of the flag can be just 'selected'.
>
>> Would that really be confusing / un-intutive ?
>
> No, I guess not. It's pretty much on-par with the existing I2C locking
> facilities.
>
> Hmmm, on second thought, maybe the select/deselect should be part of
> lock/unlock unconditionally? The big problem with that is that
> "locking" may fail. And, I now realize that this is the big issue
> with the whole approach. You need to add a return value to
> i2c_lock_operations.lock_bus, and that feels like a *big* change.
> Maybe not technically, the there will be a signal that the function
> may fail. I.e. every client will, with that return value, get pressure
> to check and act on locking failures when it is in fact only needed in
> this new situation.
>
> I guess you could do the select/deselect thing conditionally, like
> you suggested, but make it an error to try to use it with
> i2c_lock_bus(), and only allow it from i2c_trylock_bus() which
> already has a return value. But with that, I feel that you need to
> somehow separate the cases when you want to actually wait for the lock
> and when you want to give up immediately if the lock is taken.
>
> I don't know if I'm making sense. I'm typing as I'm thinking if
> anyone hadn't noticed...
The return value could be added to i2c_lock_operations.lock_bus, but not
be exposed when called through i2c_lock_bus(). That wouldn't bee too bad
I guess. Then a separate set of functions could be added for lock+select
that do expose the return value (and adds the new flag of course). I.e.
i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus.
Or something like that.
However, I have the feeling that *most* client drivers that do their
own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would
like to also keep arbitrators locked during the operation to prevent not
only the local master from clobbering the chip status.
Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer
as it is today, but then you set a flag that the arbitrator is selected,
and delay the deselect until the unlock (instead of doing it last in the
xfer). You would of course need to check the flag before doing the select
in the xfer, so that follow-up xfers don't redo the select.
Something like this (untested, written in the mail client):
static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[], int num)
{
struct i2c_mux_priv *priv = adap->algo_data;
struct i2c_mux_core *muxc = priv->muxc;
struct i2c_adapter *parent = muxc->parent;
/* Switch to the right mux port and perform the transfer. */
if (!muxc->selected) {
int ret = muxc->select(muxc, priv->chan_id);
muxc->selected = true;
if (ret < 0)
return ret;
}
return __i2c_transfer(parent, msgs, num);
}
static void i2c_parent_unlock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
struct i2c_mux_priv *priv = adapter->algo_data;
struct i2c_mux_core *muxc = priv->muxc;
struct i2c_adapter *parent = muxc->parent;
if (muxc->selected && muxc->deselect)
muxc->deselect(muxc, priv->chan_id);
muxc->selected = false;
i2c_unlock_bus(parent, flags);
rt_mutex_unlock(&parent->mux_lock);
}
(i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and
i2c_mux_unlock_bus() need corresponding changes)
One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after
the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have
some way to opt out of whatever we do to support "I2C units" across
arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the
above is not a complete solution.
Cheers,
Peter
