Re: [PATCH v2 0/8] i2c mux cleanup and locking update

Antti Palosaari
2016-01-06T17:18:07Z

On 01/05/2016 05:57 PM, Peter Rosin wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi!
>
> I have a pair of boards with this i2c topology:
>
> GPIO ---| ------ BAT1
> | v /
> I2C -----+------B---+---- MUX
> | \
> EEPROM ------ BAT2
>
> (B denotes the boundary between the boards)

Handling of I2C muxes that close channel automatically, after the first
I2C stop (P) is seen?

For example channel is selected to BAT1 => there is EEPROM write => mux
closes channel BAT1 => access to BAT1 will fail. Is it possible to lock
whole adapter, but allow only traffic to i2c mux client?

regards
Antti

>
> The problem with this is that the GPIO controller sits on the same i2c bus
> that it MUXes. For pca954x devices this is worked around by using unlocked
> transfers when updating the MUX. I have no such luck as the GPIO is a general
> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
> of the fact that it is muxing an i2c bus, and extending unlocked transfers
> into the GPIO subsystem is too ugly to even think about. But the general hw
> approach is sane in my opinion, with the number of connections between the
> two boards minimized. To put is plainly, I need support for it.
>
> So, I observe that while it is needed to have the i2c bus locked during the
> actual MUX update in order to avoid random garbage on the slave side, it
> is not strictly a must to have it locked over the whole sequence of a full
> select-transfer-deselect operation. The MUX itself needs to be locked, so
> transfers to clients behind the mux are serialized, and the MUX needs to be
> stable during all i2c traffic (otherwise individual mux slave segments
> might see garbage).
>
> This series accomplishes this by adding a dt property to i2c-mux-gpio and
> i2c-mux-pinctrl that can be used to state that the mux is updated by means
> of the muxed master bus, and that the select-transfer-deselect operations
> should be locked individually. When this holds, the i2c bus *is* locked
> during muxing, since the muxing happens as part of i2c transfers. This
> is true even if the MUX is updated with several transfers to the GPIO (at
> least as long as *all* MUX changes are using the i2s master bus). A lock
> is added to the mux so that transfers through the mux are serialized.
>
> Concerns:
> - The locking is perhaps too complex?
> - I worry about the priority inheritance aspect of the adapter lock. When
> the transfers behind the mux are divided into select-transfer-deselect all
> locked individually, low priority transfers get more chances to interfere
> with high priority transfers.
> - When doing an i2c_transfer() in_atomic() context of with irqs_disabled(),
> there is a higher possibility that the mux is not returned to its idle
> state after a failed (-EAGAIN) transfer due to trylock.
>
> To summarize the series, there's some i2c-mux infrastructure cleanup work
> first (I think that part stands by itself as desireable regardless), the
> locking changes are in the last three patches of the series, with the real
> meat in 8/8.
>
> PS. needs a bunch of testing, I do not have access to all the involved hw
>
> Changes since v1:
> - Allocate mux core and (optional) priv in a combined allocation.
> - Killed dev_err messages triggered by memory allocation failure.
> - Fix the device specific i2c muxes that I had overlooked.
> - Rebased on top of v4.4-rc8 (was based on v4.4-rc6 previously).
>
> Cheers,
> Peter
>
> Peter Rosin (8):
> i2c-mux: add common core data for every mux instance
> i2c-mux: move select and deselect ops to i2c_mux_core
> i2c-mux: move the slave side adapter management to i2c_mux_core
> i2c-mux: remove the mux dev pointer from the mux per channel data
> i2c-mux: pinctrl: get rid of the driver private struct device pointer
> i2c: allow adapter drivers to override the adapter locking
> i2c: muxes always lock the parent adapter
> i2c-mux: relax locking of the top i2c adapter during i2c controlled
> muxing
>
> .../devicetree/bindings/i2c/i2c-mux-gpio.txt | 2 +
> .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt | 4 +
> drivers/i2c/i2c-core.c | 59 ++---
> drivers/i2c/i2c-mux.c | 272 +++++++++++++++++----
> drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 46 ++--
> drivers/i2c/muxes/i2c-mux-gpio.c | 58 ++---
> drivers/i2c/muxes/i2c-mux-pca9541.c | 58 +++--
> drivers/i2c/muxes/i2c-mux-pca954x.c | 66 ++---
> drivers/i2c/muxes/i2c-mux-pinctrl.c | 89 +++----
> drivers/i2c/muxes/i2c-mux-reg.c | 63 ++---
> drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 +--
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 +-
> drivers/media/dvb-frontends/m88ds3103.c | 23 +-
> drivers/media/dvb-frontends/m88ds3103_priv.h | 2 +-
> drivers/media/dvb-frontends/rtl2830.c | 24 +-
> drivers/media/dvb-frontends/rtl2830_priv.h | 2 +-
> drivers/media/dvb-frontends/rtl2832.c | 30 ++-
> drivers/media/dvb-frontends/rtl2832_priv.h | 2 +-
> drivers/media/dvb-frontends/si2168.c | 29 ++-
> drivers/media/dvb-frontends/si2168_priv.h | 2 +-
> drivers/media/usb/cx231xx/cx231xx-core.c | 6 +-
> drivers/media/usb/cx231xx/cx231xx-i2c.c | 48 ++--
> drivers/media/usb/cx231xx/cx231xx.h | 4 +-
> drivers/of/unittest.c | 41 ++--
> include/linux/i2c-mux-gpio.h | 2 +
> include/linux/i2c-mux-pinctrl.h | 2 +
> include/linux/i2c-mux.h | 39 ++-
> include/linux/i2c.h | 28 ++-
> 28 files changed, 612 insertions(+), 424 deletions(-)
>

--