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.
> 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) ?
> 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
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.
> 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.
>
>> 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?
>
> 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.
> 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.
--
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
