On 2016-07-26 18:51, Kachalov Anton wrote:
> 26.07.2016, 18:22, "Peter Rosin" <peda@xxxxxxxxxx>:
>> On 2016-07-25 12:00, Kachalov Anton wrote:
>>> You are welcome.
>>>
>>> In my implementation in PCA954x I did the following things that I would like to highlight:
>>>
>>> 1. handle slave register and store i2c_client data to the internal array
>>> (length equal to number of channels). First obstacle – get the right offset in
>>> the array (channel number). It's better to expose "chan_id" field from
>>> i2c_mux_priv as a public function. I did a hack during init by writing down
>>> first muxc->adapter's number ("nr" field store in private adap_off).
>>
>> The i2c-mux.c file is not sacred, nor set in stone, just add the accessor
>> if you need it. (But the function would be kind of dangerous as there is
>> no way that I know of to verify that the passed adapter is in fact a mux
>> child adapter. So, if you'd pass something else, you'd get to keep all
>> the pieces...)
>>
>> HOWEVER, chan_id is not necessarily the same as the index into the
>> muxc->adapter array. The index in the muxc->adapter array is just the
>> order in which the mux child adapters were registered, and bears no
>> relation to the chan_id. At least none that can be relied upon.
>
> I just need any way to get offset into the muxc->adapter when accessing
> mux bus in reg_slave/unreg_slave.
No, I don't think you want that at all. You are using cur_chan to index
your slaves array, and that is b0rken, since you in other places use
the index into the mux core adapter array to access the slaves array.
As stated, the channel id and the adapter index are not the same (they
might be, and probably are in your case, but that is a coincidence).
What you want is to go from the adapter to the channel id, i.e. a new
function such as this in i2c_mux.c
u32 i2c_mux_adapter_chan_id(struct i2c_adapter *adap)
{
struct i2c_mux_priv *priv = adap->algo_data;
return priv->chan_id;
}
and then always use the channel id to index your slaves array (and
totally ignore the adapter index). This will work just fine since
you know that the channel id is 0-7. The mux core does not assume
such a limited range of channel ids (a mux might use a single client
adapter with channel id e.g. 400000), and therefore uses a different
scheme to index its adapter array.
>> So, a better (but totally different) addition to the mux core might be
>> to add a function to set the reg_slave and unreg_slave pointers in
>> i2c_mux_priv->algo, instead of copying the whole algo struct.
>
> Something like a: i2c_mux_set_slave_funcs(mux, reg_slave, unreg_slave) ?
Yes, that would work.
>> Another thing is that it seems that data->last_chan represents the same
>> thing as your new variable data->cur_chan, but in a different form (bitmask
>> vs. enumeration), and they are not always kept in sync. Please use just
>> one variable to prevent nasty surprises, as two variables are bound to get
>> out of sync.
>
> Yeah, I leave separate variable right not to touch original bitmask.
> I needed to create a proof-of-concept before clean things up and rewrite
> parts in robust way.
>
>>
>>> 2. during handling of select_chan I'm trying to register new i2c dummy
>>> device with newly selected slave addr corresponding to the channel
>>> number (if it has a slave). This is the second problem – after I've registered
>>> dummy device with some slave address, I'm not able to register slave
>>> devices for other mux'ed busses with the same slave addr. In other words,
>>> I have to populate all the slave devices (with the same slave addr)
>>> right before I'm going to use the bus first time.
>>
>> That description doesn't really match the code (or maybe I'm misunderstanding
>> or the code has changed or something). What do you mean that you have to
>> populate all the slave devices beforehand? The code only creates one dummy
>> device and reuses that one for all "slave proxying".
>
> I'm able to register individual slave clients on each muxed bus like the following:
>
> echo slave-24c02 0x1011 > /sys/bus/i2c/devices/i2c-9/new_device
> echo slave-24c02 0x1011 > /sys/bus/i2c/devices/i2c-10/new_device
> echo slave-24c02 0x1013 > /sys/bus/i2c/devices/i2c-11/new_device
> echo slave-24c02 0x1011 > /sys/bus/i2c/devices/i2c-12/new_device
>
> only if I do not access i2c muxed bus somewhere between adding devices, like:
>
> echo aspeed,i2c-ipmb 0x11 > /sys/bus/i2c/devices/i2c-9/new_device
> i2cdetect -y 10
> # next command will return (-16)
> # [...] i2c i2c-10: Failed to register i2c client slave-24c02 at 0x11 (-16)
> echo slave-24c02 0x1011 > /sys/bus/i2c/devices/i2c-10/new_device
> # [...] i2c i2c-11: Failed to register i2c client slave-24c02 at 0x11 (-16)
> echo slave-24c02 0x1011 > /sys/bus/i2c/devices/i2c-11/new_device
Is this with your patch? If so, then that makes perfect sense since
your dummy device sits on the root adapter, and you are not allowed
to create address conflicts between clients on the root adapter and
clients on downstream muxes.
I think the problem is that when you call i2c_new_dummy, it calls
i2c_new_device without the I2C_CLIENT_SLAVE flag (which you try to
add too late). If you open code i2c_new_dummy and add this flag
before calling i2c_new_device, it might work better?
> I use one dummy device just not to call unreg/reg each time while proxy callback
> as far simple as just passing arguments from one object to another.
>
> BTW, sometimes I saw new device entries in /sys/bus/... even
> adding client via sysfs fails.
Further details and a reproducer would be useful...
>> Anyway, one thing that I do not like about that code is that it, on mux
>> changes, clobbers dummy->addr before the slave proxy in unregistered. It's
>> not really pretty to clobber it at all, but it would feel better if the addr
>> was only clobbered when the slave was not registered.
>
> There might be a better way to track slave addr change (on adapter bus) just not to
> overcall unreg_slave/reg_slave for the same addr.
>
>> I'm also a bit surprised that you are allowed to create a new (dummy) device
>> with an address that is already taken on a downstream mux client adapter.
>> Is that a feature in the i2c core, or is it a bug? Wolfram?
>
> This is what I mean under the "populate all the slave devices beforehand".
> From the i2c bus point of view, I just adding one device per bus. It's allowed.
> From the adapter point of view, I duplicates clients with the same addr.
> But I can't use two of them at the same time, so there is no HW conflicts.
> And I've tried to workaround such case to make this possible.
I guess what I'm really surprised about is that it seems to be allowed to
register a slave address on an adapter which creates an address conflict
with a client device (and vice versa). But the code seems pretty intentional
about it, so what do I know? I have not really considered the slave part of
the i2c code before this conversation...
Yes, the two devices can probably talk to each other since they both know
who is master at the moment, but what about other masters on the bus?
>>
>>> 3. registering and/or unregister first (if applicable) slave device on the parent
>>> bus adapter. This is the third obstacle. While the bus is already locked,
>>> calling generic i2c_slave_{register,unregister} lead to the deadlock.
>>> I did it directly via the reg_slave/unreg_slave calls of appropriate data structure.
>>
>> BTW, your open-coded unlocked versions of i2c_slave_register and
>> i2c_slave_unregister do not handle errors and skips some tests. Maybe add
>> unlocked versions to i2c-core.c instead? Also, why do you need to forbid
>> 10-bit addresses in pca954x_reg_slave? Is that a property of the mux, or
>> is it really a property of your root adapter?
>
> My bad, it is incomplete code. I keep commented i2c_slave_unregister/register
> right for the future improvements. Direct call isn't good idea, but I need unlocked
> version.
>
> I forbid 10-bit address just as a copy-paste from another code. And, yes, my HW
> doesn't support it either. This check should be eliminated.
>
>>> Notes:
>>> Slave works only with keeping channel selected while generic i2c-mux logic
>>> is to deselect channel after each i2c transfers.
>>>
>>> 25.07.2016, 12:41, "Wolfram Sang" <wsa@xxxxxxxxxxxxx>:
>>>>> There is no way to switch mux and request to BMC #2. Logically, this shouldn't
>>>>> happen too.
>>>>
>>>> I see. I think I have the big picture now, thanks for the heads up.
>>>>
>>>> However, I am not sure what recommendations I can give to you. I'd start
>>>> with adding reg_slave to the muxed adapters and then try to reconfigure
>>>> the real adapter when the mux switches (I assume you did that already).
>>>> But I don't have the bandwidth to dive into that and see what gory
>>>> details come up. Maybe Peter Rosin has something to add, though?
>>
>> The whole thing seems a bit fragile, as Wolfram already stated...
>>
>> Let me see if I got how this works; You have some process that accesses some
>> device on the different mux segments, with some amount of time between each
>> access. The mux leaves the segment selected after this access. During this
>> time slot, when the mux segment is selected but idle, there might come in a
>> slave access from that mux segment.
>
> In general it's fragile. Absolutely. For generic i2c-mux, right after master IO
> completes, i2c-mux logic tries to deselect channel, but there is a workaround
> in pca954x to keep channel selected after transfer is completed. Slave support
> in pca954x moves this "window" further to allow slave IO. In application of such
> setup MUX used only for master IO to slaves on different channels. Meanwhile,
> I need to follow IPMB proto spec and allow to receive answers as slave IO
> (from the stand of my system). So, in general such fragile situation with unwanted
> slave IO is uncommon while slaves do not acts as master until they asked for.
> Only one reason that might arise is a very slow answer from previously selected channel
> (from previous round of switch). However, such case means bogus IPMB node and
> shouldn't every happen. Switching only occurs with following of request<->response
> time specs. For IPMB/ICMB application, user software asks to switch to the next
> channel only on successful response or timeout (both should be handled by IPMB i2c driver).
> One question here: how proper lock the muxed channel bus while waiting for answer/timeout?
There is no way to lock the mux with anything but transfers, as it stands.
So I guess that is something you will have figure out. :-)
>> This all depends on the external process that round-robins the mux. I don't
>> like that. It should be some system process with a much closer tie to the
>> kernel (maybe it should be in-kernel?) that is responsible for the
>> round-robin of the mux. And, AFAIU, there is no need for any access to the
>> child side of the mux to enable these slave accesses, this process may just
>> flip the mux selection and that should be enough, no? This process can also
>> step past any mux segments that do not have any slave registered so that
>> the waiting time can be reduced, and it can be completely disabled if there
>> are no slaves registered.
>
> There might be different slaves on muxed bus.
> I didn't get you on process that may flip the mux selection. It's not clear, but
> just a note that in our application we have IPMB master/slave client that
> sends request on user-space behalf and waiting for answer/timeout (atomic operation)
> keeping muxed bus busy. Right after that we're free to switch to another channel
> if it requested by the user software or use same channel for next request.
It seems your thing is pretty application specific, in the general case,
there is no way to know when a slave access might be needed. So, I proposed
that a new mechanism/process should be added that round-robins the mux(es)
when they are otherwise idle. That mechanism/process could be extended so
that it lingers on a channel when it has been selected for other reasons,
which would fit your needs, no? Since this mechanism/process would be
closely tied to the kernel, it would be easier for it to access internal
things such as locks in a sane way.
>> A bigger issue is that the process also needs to consider other muxes in
>> case there is a large mux tree on the root adapter. So, I think it needs to
>> be a central process connected to the root adapter, and not something local
>> to the mux for it to be generic? This seems to get nastier the more I think
>> about it...
>>
>> *time passes*
>>
>> I realize that in the large mux tree case, your code doesn't work very
>> well at all. I.e. if you have
>>
>> dev1
>> /
>> mux1 dev2
>> / \ /
>> root mux2
>> \ \
>> etc dev3
>>
>> then accesses to dev1 will flip mux1 and prevent any slave accesses from
>> dev2 and dev3. I guess this just shows the fundamental fragility with the
>> approach of having multiple devices doing slave accesses to the same
>> root adapter in some ad-hoc manner. I guess you could refuse slave
>> registration in case the mux isn't sitting directly on the root adapter,
>> but I don't know how to detect and prohibit slave accesses from parallel
>> muxes on a root adapter.
>
> In general my proposal has a number of limitations that you've highlighted.
> It might not (and trull it is not) fit generic application with any possible
> tree structure. I faced only HW design where there is only one i2c switch
> per adapter bus. No nestings.
> If it is required to have more channels, then one more i2c adapter's bus is used.
And when you run out of adapters?
I'm simply reluctant to add code that may hinder future development
of something generic.
Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
