i2c: slave support framework improvements

i2c: slave support framework improvements

[Kachalov Anton]

Hello.

I'm working on multi-master i2c support in Aspeed SoC (AST2150, AST2300 and so on) and succeed with event passing to the slave software backend. Besides AST has two operation modes: byte transfer and buffer transfer. While i2c slave framework operates over bytes. In current scheme I would able to receive an interrupt on buffer fill then call N-times event function with one byte. It is a little overhead there. What we can do there?

Second one thing that I want to point is a slave support in I2C MUX/switch drivers. For instance I use PCA9548 switch and want to have N (N <= 8) slaves (one per channel). The generic switching algo tries to deselect channel each time transmission has ended. Sametime, pca954x allows to leave channel selected. It is very useful for slave operation mode (e.g. IPMB listener). There is no evidence from i2c-mux point of view if the channel is really deselected or not. If I want to code a generic slave support to re-register slaves on parent adapter (program i2c IP reg with proper slave address) each time I switch between channels, I need to leave channel "open" until I explicitly wants to deselect the channel to make slave driver operable. Like a "lock" channel function. E.g. check for lock flag before doing deselect in i2c_mux_smbus_xfer. There are four duplicate select/deselect code in each *mux*xfer routine that might be wrapped with channel lock check for the described approach. For generic i2c-mux slave switching it is needed a slave_switch function to be implemented in slave driver (mux/pca954x and bus/aspeed for instance).

In current implementation I did several hacks: dummy i2c slave driver for i2c-mux-pca954x registered on parent adapter's bus, proxy callback that passes data to the mux'es appropriate slave, manual call algo->reg_slave/unreg_slave to avoid deadlocks in select/deselect handlers.
One of the most tricky part here is calculating starting i2c bus number for array's offset (adap_off field) in slave_register/unregister driver's routines. Better to have something like a i2c_mux_channel(...) routine to access private "chan_id" field of mux. The second trick is a hotfix of algo array (came from i2c-mux as const value) to fill it with additional slaves function pointers.

Here is a patch:
https://github.com/ya-mouse/openbmc-target/blob/master/aspeed/patches-4.4/0065-i2c-slave-mux-pca954x.patch

Here is the work-in-progress IPMI I2C driver that populates i2c client & slave on MUX bus:
https://github.com/ya-mouse/openbmc-packages/blob/master/kernel/ipmi-i2c/src/ipmi_i2c.c

What should be improved in i2c-mux and i2c slave framework to rewrite such code to have robust code without a number of hacks, tricks and workarounds? Any suggestions and comments are welcome.

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

Hi,

> I'm working on multi-master i2c support in Aspeed SoC (AST2150,
> AST2300 and so on) and succeed with event passing to the slave
> software backend. Besides AST has two operation modes: byte transfer

Which upstream driver does that SoC use?

> and buffer transfer. While i2c slave framework operates over bytes. In
> current scheme I would able to receive an interrupt on buffer fill
> then call N-times event function with one byte. It is a little
> overhead there. What we can do there?

Let me quote from Documentation/i2c/slave-interface:

About buffers
-------------

During development of this API, the question of using buffers instead of just
bytes came up. Such an extension might be possible, usefulness is unclear at
this time of writing. Some points to keep in mind when using buffers:

* Buffers should be opt-in and slave drivers will always have to support
  byte-based transactions as the ultimate fallback because this is how the
  majority of HW works.

* For backends simulating hardware registers, buffers are not helpful because
  on writes an action should be immediately triggered. For reads, the data in
  the buffer might get stale.

* A master can send STOP at any time. For partially transferred buffers, this
  means additional code to handle this exception. Such code tends to be
  error-prone.

> Second one thing that I want to point is a slave support in I2C MUX/switch
> drivers. For instance I use PCA9548 switch and want to have N (N <= 8) slaves
> (one per channel)

How do the other masters know that their channel is currently muxed then? And
what if two of them want to access two of your slaves at the same time?

This setup may work for one specific case but in general sounds pretty fragile
to me. This fragility shows in all the hacks needed to work around the proper
abstractions. I don't think much can be done about that.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Kachalov Anton]

Hello,

23.07.2016, 22:51, "Wolfram Sang" <wsa@the-dreams.de>:
> Hi,
>
>>  I'm working on multi-master i2c support in Aspeed SoC (AST2150,
>>  AST2300 and so on) and succeed with event passing to the slave
>>  software backend. Besides AST has two operation modes: byte transfer
>
> Which upstream driver does that SoC use?

This one:
https://github.com/openbmc/linux/blob/dev-4.4/drivers/i2c/busses/i2c-aspeed.c

Plus my work-in-progress patch with slave support:
https://github.com/ya-mouse/openbmc-target/blob/master/aspeed/patches-4.4/0068-openmouse-i2c-aspeed-slave-support.patch

>
>>  and buffer transfer. While i2c slave framework operates over bytes. In
>>  current scheme I would able to receive an interrupt on buffer fill
>>  then call N-times event function with one byte. It is a little
>>  overhead there. What we can do there?
>
> Let me quote from Documentation/i2c/slave-interface:
>
> About buffers
> -------------
>
> During development of this API, the question of using buffers instead of just
> bytes came up. Such an extension might be possible, usefulness is unclear at
> this time of writing. Some points to keep in mind when using buffers:
>
> * Buffers should be opt-in and slave drivers will always have to support
>   byte-based transactions as the ultimate fallback because this is how the
>   majority of HW works.
>
> * For backends simulating hardware registers, buffers are not helpful because
>   on writes an action should be immediately triggered. For reads, the data in
>   the buffer might get stale.
>
> * A master can send STOP at any time. For partially transferred buffers, this
>   means additional code to handle this exception. Such code tends to be
>   error-prone.
>
>>  Second one thing that I want to point is a slave support in I2C MUX/switch
>>  drivers. For instance I use PCA9548 switch and want to have N (N <= 8) slaves
>>  (one per channel)
>
> How do the other masters know that their channel is currently muxed then? And
> what if two of them want to access two of your slaves at the same time?

In current implementation this I2C switch might be only switched from one side (via i2c command). From the MUX point of view, there is only one master (which may mux) and a number of slaves behind each channel. But on proto level IPMB requires to act as multi-master to receive answers from other intelligent nodes. To receive answer requesting node have to switch to slave mode after master write request.

Please take a look at page 13: 
http://www.intel.com/content/www/us/en/servers/ipmi/ipmp-spec-v1-0.html

The quote:
"Intelligent devices on the Intelligent Platform Management Bus transmit requests and responses as bus
Masters and receive requests and response as bus Slaves. Thus, the only Master Write transactions are
seen between intelligent nodes."

>
> This setup may work for one specific case but in general sounds pretty fragile
> to me. This fragility shows in all the hacks needed to work around the proper
> abstractions. I don't think much can be done about that.
>
> Regards,
>
>    Wolfram

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]


> > How do the other masters know that their channel is currently muxed
> > then? And what if two of them want to access two of your slaves at
> > the same time?
> 
> In current implementation this I2C switch might be only switched from
> one side (via i2c command). From the MUX point of view, there is only
> one master (which may mux) and a number of slaves behind each channel.
> But on proto level IPMB requires to act as multi-master to receive
> answers from other intelligent nodes. To receive answer requesting
> node have to switch to slave mode after master write request.

And how is guaranteed that one of the intelligent nodes behind the MUX
do no send requests on their own while their channel is not muxed?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Kachalov Anton]

25.07.2016, 10:22, "Wolfram Sang" <wsa@the-dreams.de>:
>>  > How do the other masters know that their channel is currently muxed
>>  > then? And what if two of them want to access two of your slaves at
>>  > the same time?
>>
>>  In current implementation this I2C switch might be only switched from
>>  one side (via i2c command). From the MUX point of view, there is only
>>  one master (which may mux) and a number of slaves behind each channel.
>>  But on proto level IPMB requires to act as multi-master to receive
>>  answers from other intelligent nodes. To receive answer requesting
>>  node have to switch to slave mode after master write request.
>
> And how is guaranteed that one of the intelligent nodes behind the MUX
> do no send requests on their own while their channel is not muxed?

The MUX has been driven by high-level (logically) Master. RMC (Rack management controller) switches MUX to talk to slaves (BMC) of each node. There is guaranteed that BMC wouldn't act as Master (logically), but the line is muxed until timeout or answer received by RMC. BMCs are passive and only responds to requests. But regarding to the IPMB proto spec, every response have to be made as Master.

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]


Please word wrap your messages.

> but the line is muxed until timeout or answer received by RMC. BMCs
> are passive and only responds to requests.

OK, I see.

What about serialization? Can it happen that you send a request to BMC
#1 and before the answer, you need to send an urgent request to BMC #2?

Is it common in IPMI world to use muxes for this case?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Kachalov Anton]

25.07.2016, 11:28, "Wolfram Sang" <wsa@the-dreams.de>:
> Please word wrap your messages.
>
>>  but the line is muxed until timeout or answer received by RMC. BMCs
>>  are passive and only responds to requests.
>
> OK, I see.
>
> What about serialization? Can it happen that you send a request to BMC
> #1 and before the answer, you need to send an urgent request to BMC #2?

Timeout for answer is quite short. Please refer to table at page 24 in the
IPMB proto spec. I2C slave support implemented in AMI/Emerson firmware
doesn't work in such way – slave read after master write implemented right in
the kernel module as an ioctl (it is atomic operation from the software perspective).
It's like a usual read/write, but read implemented as slave read.
There is no way to switch mux and request to BMC #2. Logically, this shouldn't
happen too.

Search for I2C_SLAVE_RDWR in

   https://raw.githubusercontent.com/facebook/openbmc/master/meta-aspeed/recipes-kernel/linux/files/patch-2.6.28.9/0000-linux-aspeed-064.patch

for slave implementation details. Attn! it's a 1.4MB file.

>
> Is it common in IPMI world to use muxes for this case?

It's not a common, but not restricted while MUX master (RMC side) is logically
Master while BMC is usually passive (logically Slaves) and do not initiate IPMB
requests on their own behalf. Only requests for I2C slave devices on other busses
such as sensors. One exception is a communication with Intel ME.
Intel ME acts as a Master on I2C, but there is no muxes between in any kind of
HW design. Thus this bus is in Multi-Master mode permanently.

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Kachalov Anton]

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

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.

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.

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@the-dreams.de>:
>>  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?

Re: i2c: slave support framework improvements

[Peter Rosin]

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.

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.

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.

> 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".

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.

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?

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

> 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@the-dreams.de>:
>>>  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.

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.

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.

Cheers,
Peter

Re: i2c: slave support framework improvements

[Kachalov Anton]

26.07.2016, 18:22, "Peter Rosin" <peda@axentia.se>:
> 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@the-dreams.de>:
>>>>   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.

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-26 18:51, Kachalov Anton wrote:
> 26.07.2016, 18:22, "Peter Rosin" <peda@axentia.se>:
>> 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@the-dreams.de>:
>>>>>   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

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]


I didn't read all, just found my name in this paragraph.

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

The slave device must be attached to the muxed adapter. There, the
address should be free. The parent adapter should be reconfigured
depending on the mux setting.

At least, that's the theory, dunno if this really works. I thought you
would be doing this already?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-28 09:41, Wolfram Sang wrote:
> 
> I didn't read all, just found my name in this paragraph.
> 
>>> 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.
> 
> The slave device must be attached to the muxed adapter. There, the
> address should be free. The parent adapter should be reconfigured
> depending on the mux setting.
> 
> At least, that's the theory, dunno if this really works. I thought you
> would be doing this already?

Hi Wolfram,

Thinking about this a bit more, this is what happens.

In order to call i2c_slave_register, you need to have a slave device
on the adapter in question. So, Anton creates a proxy slave device on the
adapter the mux is connect to. This leads to an address conflict since
the proxy needs the same address that the real slave device has already
registered on the mux client adapter.

But that conflict does not happen! This is because the proxy slave device
is created using i2c_new_dummy, and the I2C_CLIENT_SLAVE flag is added
after the address conflict check in i2c_new_dummy.

The proxy slave device is created when the mux is first selected
on a channel that has a slave device on the client side of the mux, so
the whole scheme fails if there is a mux client side access before
the last real slave device has been registered. This access causes the
above address conflict to happen for subsequent additions of slave
devices on the client side, because then the proxy slave device now
has the I2C_CLIENT_FLAG, which the real slave devices also have. This
needs to be fixed somehow, but is not a bug in the i2c core. But the
i2c core might still need to be involved in fixing the problem of
course, for example by somehow not including the proxy slave device in
the normal list of client devices. Or something.

In proximity to this, I find it odd that you are allowed to register
a slave device with the same address as a client device on an adapter
(one has I2C_CLIENT_FLAG set, which sets I2C_ADDR_OFFSET_SLAVE before
the address comparisons). Why is that possible? Yes, the two devices
can probably talk each other, both being aware of who is master at
the moment, but what about other masters on the bus?

Cheers,
Peter

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-28 08:55, Peter Rosin wrote:
> On 2016-07-26 18:51, Kachalov Anton wrote:
>> 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?

No, it might not. See my reply to Wolfram...

Cheers,
Peter

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]


> In proximity to this, I find it odd that you are allowed to register
> a slave device with the same address as a client device on an adapter
> (one has I2C_CLIENT_FLAG set, which sets I2C_ADDR_OFFSET_SLAVE before
> the address comparisons). Why is that possible? Yes, the two devices
> can probably talk each other, both being aware of who is master at
> the moment, but what about other masters on the bus?

Just a quick answer to this first: The Tegra community wanted to have
loopback support. According to documentation, their hardware can indeed
read from their own slave device. So, we needed this distinction to
instantiate the slave backend driver and the regular driver.

I don't understand the multi-master question: The I2C slave device
should react to any bus master anyhow. And for competing bus masters,
there is the arbitration mechanism.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-28 10:39, Wolfram Sang wrote:
> 
>> In proximity to this, I find it odd that you are allowed to register
>> a slave device with the same address as a client device on an adapter
>> (one has I2C_CLIENT_FLAG set, which sets I2C_ADDR_OFFSET_SLAVE before
>> the address comparisons). Why is that possible? Yes, the two devices
>> can probably talk each other, both being aware of who is master at
>> the moment, but what about other masters on the bus?
> 
> Just a quick answer to this first: The Tegra community wanted to have
> loopback support. According to documentation, their hardware can indeed
> read from their own slave device. So, we needed this distinction to
> instantiate the slave backend driver and the regular driver.
> 
> I don't understand the multi-master question: The I2C slave device
> should react to any bus master anyhow. And for competing bus masters,
> there is the arbitration mechanism.

What I mean is that it is possible to have an i2c bus with some random
i2c device at e.g. address 0x48, say some eeprom, and then register to
be a slave device also at address 0x48, e.g. slave-24c02. If there is
another master on the bus, it cannot sanely use any of these two devices
at i2c address 0x48 since there is an address conflict.

Or am I misunderstanding something? In my mind i2c slave support is
the equivalent of usb gadget support, is it something else?

Cheers,
Peter

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]


> What I mean is that it is possible to have an i2c bus with some random
> i2c device at e.g. address 0x48, say some eeprom, and then register to
> be a slave device also at address 0x48, e.g. slave-24c02. If there is
> another master on the bus, it cannot sanely use any of these two devices
> at i2c address 0x48 since there is an address conflict.

That is an address conflict, yes. Even for a single-master system. You
need to make sure your slave address is unique on the bus. The kernel
can't really help you with that, because it could only detect address
conflicts if a driver would be attached to a device. If there is no
driver and all communication is done via i2c-dev, for example, then
I can't see a way to detect this.

> Or am I misunderstanding something? In my mind i2c slave support is
> the equivalent of usb gadget support, is it something else?

No, you are correct.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-28 11:15, Wolfram Sang wrote:
> 
>> What I mean is that it is possible to have an i2c bus with some random
>> i2c device at e.g. address 0x48, say some eeprom, and then register to
>> be a slave device also at address 0x48, e.g. slave-24c02. If there is
>> another master on the bus, it cannot sanely use any of these two devices
>> at i2c address 0x48 since there is an address conflict.
> 
> That is an address conflict, yes. Even for a single-master system. You
> need to make sure your slave address is unique on the bus. The kernel
> can't really help you with that, because it could only detect address
> conflicts if a driver would be attached to a device. If there is no
> driver and all communication is done via i2c-dev, for example, then
> I can't see a way to detect this.

Why do you talk about i2c-dev and other random i2c accesses?

I don't see where the conflict is prevented even if everything is
registered properly with in-kernel drivers, and it looks trivial to
prevent. Just stop accounting for the I2C_CLIENT_SLAVE flag when
comparing addresses would fix it, I think. I also don't see how the
tegra loopback has anything to do with this. Or rather, it has
everything to do with this. Because that actually makes this a
problem even without another master on the bus (but then registering
as a slave makes no sense, but that's beside the point).

Cheers,
Peter

Re: i2c: slave support framework improvements

[Kachalov Anton]

28.07.2016, 09:55, "Peter Rosin" <peda@axentia.se>:
> On 2016-07-26 18:51, Kachalov Anton wrote:
>>  26.07.2016, 18:22, "Peter Rosin" <peda@axentia.se>:
>
> 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.

Re: i2c: slave support framework improvements

[Kachalov Anton]

28.07.2016, 10:42, "Wolfram Sang" <wsa@the-dreams.de>:
> I didn't read all, just found my name in this paragraph.
>
>>  > 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.
>
> The slave device must be attached to the muxed adapter. There, the
> address should be free. The parent adapter should be reconfigured
> depending on the mux setting.
>
> At least, that's the theory, dunno if this really works. I thought you
> would be doing this already?

Correct. Here is the instantiation of the proxy client on the root bus:

https://github.com/ya-mouse/openbmc-target/blob/master/aspeed/patches-4.4/0065-i2c-slave-mux-pca954x.patch#L46

and here is reconfiguration:

https://github.com/ya-mouse/openbmc-target/blob/master/aspeed/patches-4.4/0065-i2c-slave-mux-pca954x.patch#L57
https://github.com/ya-mouse/openbmc-target/blob/master/aspeed/patches-4.4/0065-i2c-slave-mux-pca954x.patch#L60

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]


> I don't see where the conflict is prevented even if everything is
> registered properly with in-kernel drivers, and it looks trivial to
> prevent.

You can prevent it, if and only if, all devices on the bus have a kernel
driver attached. This is a kind of "corner case" to me. According to my
experience, this scenario does not happen too often. And for sure not
often enough so I could rate it as "the kernel can reliably protect you
from this address conflict". I'd rather be honest here and say: "the
kernel can't protect you from this setup, you have to design your I2C
bus carefully". Which is especially true in multi-master setups.

With that in mind, nothing is lost giving i2c slaves a seperate address
space. We gain loopback operation and (bug) reports are also easier to
understand because one can easily see from logs if somebody is operating
an i2c client driver or an i2c slave backend.

Makes sense?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-07-28 11:39, Peter Rosin wrote:
> On 2016-07-28 11:15, Wolfram Sang wrote:
>>
>>> What I mean is that it is possible to have an i2c bus with some random
>>> i2c device at e.g. address 0x48, say some eeprom, and then register to
>>> be a slave device also at address 0x48, e.g. slave-24c02. If there is
>>> another master on the bus, it cannot sanely use any of these two devices
>>> at i2c address 0x48 since there is an address conflict.
>>
>> That is an address conflict, yes. Even for a single-master system. You
>> need to make sure your slave address is unique on the bus. The kernel
>> can't really help you with that, because it could only detect address
>> conflicts if a driver would be attached to a device. If there is no
>> driver and all communication is done via i2c-dev, for example, then
>> I can't see a way to detect this.
> 
> Why do you talk about i2c-dev and other random i2c accesses?
> 
> I don't see where the conflict is prevented even if everything is
> registered properly with in-kernel drivers, and it looks trivial to
> prevent. Just stop accounting for the I2C_CLIENT_SLAVE flag when
> comparing addresses would fix it, I think. I also don't see how the
> tegra loopback has anything to do with this. Or rather, it has
> everything to do with this. Because that actually makes this a
> problem even without another master on the bus (but then registering
> as a slave makes no sense, but that's beside the point).

Ok, I see what they are doing. They register a slave device AND a driver
and then they are able to communicate with themselves since the adapter
supports loopback. But that seems like the odd case which is not really
useful for anything but debugging (or maybe for cases where other code
expects an i2c device that is then emulated this way).

If you really have a separate device on the bus with the same address
that you want to add slave support for, then there really is a conflict,
and the kernel knows it. I think it would be nice if such mistakes were
prevented. Agreed? After all, the kernel prevents other obvious i2c
address conflicts and consistency is nice.

Cheers,
Peter

Re: i2c: slave support framework improvements

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Hi Peter,

> If you really have a separate device on the bus with the same address
> that you want to add slave support for, then there really is a conflict,
> and the kernel knows it.

We still have a disagreement about "the kernel knows it". The kernel
knows it only in one case, i.e. when you are able to describe all
devices on the bus.

What about this compromise: We keep the current scheme, but print a
warning when the kernel notices a slave device has the same address
which is already claimed by a client driver. This will let most users
know about the conflict but it will not hurt the debugging-via-loopback
case, since people know what they are doing and will happily ignore it.

If you can agree to that, I'll cook up a patch later this week.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: i2c: slave support framework improvements

[Peter Rosin]

On 2016-08-16 14:12, Wolfram Sang wrote:
> Hi Peter,
> 
>> If you really have a separate device on the bus with the same address
>> that you want to add slave support for, then there really is a conflict,
>> and the kernel knows it.
> 
> We still have a disagreement about "the kernel knows it". The kernel
> knows it only in one case, i.e. when you are able to describe all
> devices on the bus.
> 
> What about this compromise: We keep the current scheme, but print a
> warning when the kernel notices a slave device has the same address
> which is already claimed by a client driver. This will let most users
> know about the conflict but it will not hurt the debugging-via-loopback
> case, since people know what they are doing and will happily ignore it.
> 
> If you can agree to that, I'll cook up a patch later this week.

Hmm, maybe add a "slave loopback quirk" to adapters that allows this,
and forbid it if the quirk isn't present?

(or use whatever flag is appropriate, if quirks do not fit)

And/or require some kind of loopback flag when adding a client driver
that is supported via loopback to a slave driver.

Note, I do not feel strongly about this at all, I'm mainly trying to
understand what's going on. Now I do understand, and do not desperately
seek changes. It just looked fishy, that's all...

Cheers,
Peter

Re: i2c: slave support framework improvements

[Kachalov Anton]

Hello,

just want to remind my usage scenario where I have control board connected via i2c-mux switch to a number of Node. Control board and Nodes implements IPMB proto: each device acts as Slave (receive requests) and Master (send answers). Logically, there is one Master and many Slaves. Only Control board may switch i2c channel and talk to Nodes. CB's slave is constant (usually 0x20). To achieve this in our setup we instantiate a number of i2c-clients (dummy) and slaves with the same address. E.g. 0x11 for client write (Node) and 0x20 for slave read (ourselves, to receive answers from nodes).

Therefore, we should treat i2c-switch as one-way gate with multi-master.

Thanks.

16.08.2016, 16:33, "Peter Rosin" <peda@axentia.se>:
> On 2016-08-16 14:12, Wolfram Sang wrote:
>>  Hi Peter,
>>
>>>  If you really have a separate device on the bus with the same address
>>>  that you want to add slave support for, then there really is a conflict,
>>>  and the kernel knows it.
>>
>>  We still have a disagreement about "the kernel knows it". The kernel
>>  knows it only in one case, i.e. when you are able to describe all
>>  devices on the bus.
>>
>>  What about this compromise: We keep the current scheme, but print a
>>  warning when the kernel notices a slave device has the same address
>>  which is already claimed by a client driver. This will let most users
>>  know about the conflict but it will not hurt the debugging-via-loopback
>>  case, since people know what they are doing and will happily ignore it.
>>
>>  If you can agree to that, I'll cook up a patch later this week.
>
> Hmm, maybe add a "slave loopback quirk" to adapters that allows this,
> and forbid it if the quirk isn't present?
>
> (or use whatever flag is appropriate, if quirks do not fit)
>
> And/or require some kind of loopback flag when adding a client driver
> that is supported via loopback to a slave driver.
>
> Note, I do not feel strongly about this at all, I'm mainly trying to
> understand what's going on. Now I do understand, and do not desperately
> seek changes. It just looked fishy, that's all...
>
> Cheers,
> Peter