28.07.2016, 09:55, "Peter Rosin" <peda@xxxxxxxxxx>:
> On 2016-07-26 18:51, Kachalov Anton wrote:
>> 26.07.2016, 18:22, "Peter Rosin" <peda@xxxxxxxxxx>:
>
> 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.
Right. I'll rewrite this part to use i2c_mux_adapter_chan_id.
>> 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.
Correct. With my patch. I create one proxy i2c dummy device that sits
on the adapter's bus and proxy slave callback to the target muxed slave.
> 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?
Let me try. That means I need to copy-paste all the dummy part from i2c-core
and set flags in i2c_board_info struct?
>> BTW, sometimes I saw new device entries in /sys/bus/... even
>> adding client via sysfs fails.
>
> Further details and a reproducer would be useful...
I'll try to catch it again.
>>> 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?
It is allowed to talk more than two masters on the bus. May be I'm wrong.
http://www.i2c-bus.org/multimaster/
Aspeed SoC has a arbitration lost interrupt and some procedure to win arbitration then.
>> 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.
It is known that in very specific application (IPMB/ICMB and so) but standard there
are high dependency to the timings. For both sides.
>From the TI docu:
http://www.ti.com/lit/ds/symlink/pca9548a.pdf
we get that only upstream is a true Master while downstream channels are Slaves.
In our situation Slaves shouldn't first initiate a Master Write until they asked by
requester Master. So, it is not a generic bi-directional multi-master switch.
There is only one true Master that able to select channel and other devices are
might be only semi-Master.
>> 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?
Aspeed might have up to 16 bus :) For typical application there is no need more than 4-5
for different purpose.
It might be possible, that two nested switch might work while we will have two proxies:
first level on root adapter's bus and second one attached to the muxed adapter's bus.
> I'm simply reluctant to add code that may hinder future development
> of something generic.
Probably, we should start with something more specific and improve it in the future for
more generic (or other specific) cases.
--
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
