[RFC] SMBus multiplexing for the Tyan S4882

[RFC] SMBus multiplexing for the Tyan S4882

[Jean Delvare]

Hi Greg, all,

A couple weeks ago, I started working on hardware monitoring support for
the Tyan S4882 4-CPU motherboard [1]. The board features an AMD-8111
dual SMBus adapter. All hardware monitoring devices are located on the
first (SMBus 1.0) adapter, which the i2c-amd756 driver supports (both in
the lm_sensors project for Linux 2.4 and in Linux 2.6).

The particularity of the board is that the SMBus is multiplexed using 3
Philips chips: one PCA9556 (8-way I/O, [2]) for control and two PCA9516
(5-way I2C mux, [3]). I2C bus multiplexing at the i2c core level was
discussed before [4], but nothing was ever integrated. I also did write
a stand-alone driver for the Philips PCA9540 (3-way I2C mux) some times
ago but this approach is neither convenient (the user has to manually
switch the mux, only one channel is visible at a given time) nor safe
(no lock, breaks client buffers).

This time, I tried a different approach. I integrated the multiplexing
into the bus driver (i2c-amd756). I do think it is the correct way,
since it isn't too complex (and doesn't affect the i2c subsystem
core), while still safe and transparent to both the user-space and the
i2c clients. The drawback is that the added code is highly
board-specific. I yet have to see a different, working approach with
less board-specific code though.

A patch against lm_sensors CVS can be seen here:
http://khali.linux-fr.org/devel/i2c/lm_sensors-CVS-S4882.patch
This is for Linux 2.4 but I plan to port it to 2.6 soon, which is why I
am requesting for comments on the LKML.

Here is a summary of what the code does:
1* Detect the S4882 systems, using the PCI subvendor and subdevice of the
SMBus device.
2* Dynamically allocate and fill 4 i2c_adapter structures and the 4
associated i2c_algorithm structures (one adapter+algorthm per CPU). Also
alter the main adapter's algorithm structure to point to a slightly
modified smbus_xfer function.
3* Register the mux chip (I2C address 0x18) as an i2c_client so that
nobody else will request it. I do not otherwise use the client, since I
can send SMBus transfer commands directly.
4* Clean everything up on unload, of course.

The new SMBus access functions are simple wrappers which change the mux
configuration and call the main access function. I refactored the code
wherever possible. The wrappers for "virtual" busses will also only
accept mux'd addresses, while the wrapper for the main bus will accept
only non-mux'd addresses. Note that doing this required specific
knowledge about the S4882 (you need to know which addresses can be
multiplexed).

Since the monitoring devices (LM63) are located on 4 mux'd busses and
the memory module EEPROMS are located on 4 other mux'd busses at
different addresses, I chose to merge the channels on a per-CPU basis.
This has the advantage of lowering the overhead, reducing the mux
switching and being overall more user-friendly. Note that doing this
required specific knowledge about the S4882 (you need to know which
channels can/should be merged). In particular, there is no way to guess
which pair of channels corresponds to any given CPU.

The driver also remembers the last mux combination used and only send a
mux switch command when this is really needed. This significantly lowers
the SMBus use overhead.

Since this adds a significant amount of code to the driver, I made the
S4882 support selectable as a configuration option.

Expected Objections & Answers:

O: The PCA9556 support could be moved to a separate driver.
A: I don't see no benefit. There is very little code for the PCA9556
driver among the code I added, and I believe that calling a PCA9556
interface would represent no less code. There is not much code to reuse
anyway, since the way the PCA9556 driver is used is specific to each
board. It could even be used for something compeletly different than
SMBus multiplexing, since it is a simple 8 channel I/O chip.

O: The specific S4882 support could be moved to a completely different
driver.
A: This would duplicate most of the i2c-amd756 driver code. The
additional support will not affect non-S4882 users except for the size
of the driver. People concerned about the size can recompile the driver
without the S4882 support.

Before I commit my changes to the lm_sensors CVS and port them to the
Linux 2.6 driver, I welcome constructive comments about my work.

Thanks,
Jean Delvare

[1] http://www.tyan.com/products/html/thunderk8qspro.html
[2] http://www.semiconductors.philips.com/pip/PCA9556PW.html
[3] http://www.semiconductors.philips.com/pip/PCA9516.html
[4] http://archives.andrew.net.au/lm-sensors/msg05047.html
    http://archives.andrew.net.au/lm-sensors/msg23149.html
    http://archives.andrew.net.au/lm-sensors/msg27255.html

Re: [RFC] SMBus multiplexing for the Tyan S4882

[Mark M. Hoffman]

Hello Jean:

* Jean Delvare <khali@linux-fr.org> [2004-10-14 12:31:26 +0200]:

(snip)

> A patch against lm_sensors CVS can be seen here:
> http://khali.linux-fr.org/devel/i2c/lm_sensors-CVS-S4882.patch
> This is for Linux 2.4 but I plan to port it to 2.6 soon, which is why I
> am requesting for comments on the LKML.

(snip)

> Expected Objections & Answers:
> 
> O: The PCA9556 support could be moved to a separate driver.
> A: I don't see no benefit. There is very little code for the PCA9556
> driver among the code I added, and I believe that calling a PCA9556
> interface would represent no less code. There is not much code to reuse
> anyway, since the way the PCA9556 driver is used is specific to each
> board. It could even be used for something compeletly different than
> SMBus multiplexing, since it is a simple 8 channel I/O chip.
> 
> O: The specific S4882 support could be moved to a completely different
> driver.
> A: This would duplicate most of the i2c-amd756 driver code. The
> additional support will not affect non-S4882 users except for the size
> of the driver. People concerned about the size can recompile the driver
> without the S4882 support.
> 
> Before I commit my changes to the lm_sensors CVS and port them to the
> Linux 2.6 driver, I welcome constructive comments about my work.

Heh, you definitely predicted my objections. :)  But I think you can
support this board in an independent module without copying any amd756
code.  It would look very much like your patch already...

E.g. the i2c-s4882 module would act as a i2c-client for the mux chip,
but also export four virtual adapters for the segments behind the mux.
One would attach it to an existing adapter (in your case amd756) by
passing it a module parameter (i2c bus id).  Is there any reason this
wouldn't work?

The downside is that sensors-detect would need more help to recognize
this setup.  But detection is, after all, what it does.

The upside is that you don't need to modify the amd756 driver at all.
Maybe that's not a big deal for this one board, but that slope is
slippery.  How many other boards, slightly different, will need such
support?  I think it's better to leave the real bus adapters alone
and put the support for new combinations in new modules.

Somewhat related: since I wrote i2c-stub, I was thinking of creating
i2c-trace... which would attach to an existing adapter while exporting
another virtual adapter.  Anything attached to the i2c-trace adapter
would generate log messages just like i2c-stub.  That's similar to
your patch in a way; and that's why I think it can be independent
from the real bus adapter code.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [RFC] SMBus multiplexing for the Tyan S4882

[Jean Delvare]

>> Expected Objections & Answers:
>>
>> O: The PCA9556 support could be moved to a separate driver.
>> A: I don't see no benefit. There is very little code for the PCA9556
>> driver among the code I added, and I believe that calling a PCA9556
>> interface would represent no less code. There is not much code to reuse
>> anyway, since the way the PCA9556 driver is used is specific to each
>> board. It could even be used for something compeletly different than
>> SMBus multiplexing, since it is a simple 8 channel I/O chip.
>>
>> O: The specific S4882 support could be moved to a completely different
>> driver.
>> A: This would duplicate most of the i2c-amd756 driver code. The
>> additional support will not affect non-S4882 users except for the size
>> of the driver. People concerned about the size can recompile the driver
>> without the S4882 support.
>>
>> Before I commit my changes to the lm_sensors CVS and port them to the
>> Linux 2.6 driver, I welcome constructive comments about my work.
>
>Heh, you definitely predicted my objections. :)  But I think you can
>support this board in an independent module without copying any amd756
>code.  It would look very much like your patch already...
>
>E.g. the i2c-s4882 module would act as a i2c-client for the mux chip,
>but also export four virtual adapters for the segments behind the mux.
>One would attach it to an existing adapter (in your case amd756) by
>passing it a module parameter (i2c bus id).  Is there any reason this
>wouldn't work?

First of all, there would be no need to provide a bus id as a module
parameter. since the i2c-s4882 driver would know knows which bus it has
to attach to. And anyway, i2c bus ids are not unique and may change, so
it wouldn't be very convenient.

I see one major downside to and one major issue with your approach.

First, depending on the multiplexer selection, mux'd chips will show on
the main bus or not. Even worse, when the the multiplexer selection
changes, the same address on the main bus will be a physically different
chip. There is no way for a client chip to know that it should ignore
chips at given addresses on the main bus. The only way to prevent that
is to unselect all mux'd channels after each command sent to a mux'd
chip. This means that the SMBus traffic will be increased by a rough
200% in all use cases. Doesn't sound good at all. With my approach, the
overhead is hardly noticeable in the typical use (5% maybe). In the
worst case it tops to 100%.

Second, how do you handle the case where a chip driver is already loaded
before the bus drivers are? Depending on the original multiplexer
selection, loading i2c-amd756 may trigger the registration of mux'd
chips. Then loading i2c-s4882 will hide that chip from the main bus, and
the registered client will point to a non-existing chip.

>The downside is that sensors-detect would need more help to recognize
>this setup.  But detection is, after all, what it does.

True. Sensors-detect would need to detect that specific board, and make
sure it loads the additional driver (i2c-s4882) before probing the main
bus. It admittedly shouldn't be too complex (it doesn't significantly
differ from other PCI device detection).

>The upside is that you don't need to modify the amd756 driver at all.
>Maybe that's not a big deal for this one board, but that slope is
>slippery.  How many other boards, slightly different, will need such
>support?

I remember of two 2-CPU boards with LM90 chips which were using bus
multiplexing as well (one of which I wrote the pca9540 driver for). I
don't think there are that many boards like this out there (so far at
least). Other requests for virtual adapters were for home-brew designs
if I remember correctly.

Anyway, I agree that my approach won't scale well if too many boards use
SMBus multiplexing.

>I think it's better to leave the real bus adapters alone
>and put the support for new combinations in new modules.

I agree in the theory. However, I don't know how this could be done
efficiently and safely. Looks like your approach is neither, unless the
problems I saw are either inexistent (I am ill for a couple days now and
may be missing things) or can somehow be solved.

Maybe we could go with my solution for now and reconsider if the number
of boards with multiplexing reaches an alert threshold?

>Somewhat related: since I wrote i2c-stub, I was thinking of creating
>i2c-trace... which would attach to an existing adapter while exporting
>another virtual adapter.  Anything attached to the i2c-trace adapter
>would generate log messages just like i2c-stub.  That's similar to
>your patch in a way; and that's why I think it can be independent
>from the real bus adapter code.

Similar but different. The core issue in my patch is not the virtual
adapters but the multiplexing. Having a single virtual adapter on top of
a real one like you propose with i2c-trace sounds sane (and having the
user select the bus through a module parameter also, since it would be
meant for debugging.) But when you have a multiplexer entering the
arena, things go way more complex.

Thanks,
Jean Delvare