From mboxrd@z Thu Jan 1 00:00:00 1970 From: Antti Palosaari Subject: Re: [PATCH v2 0/8] i2c mux cleanup and locking update Date: Wed, 6 Jan 2016 19:17:49 +0200 Message-ID: <568D4C3D.3000906@iki.fi> References: <1452009438-27347-1-git-send-email-peda@lysator.liu.se> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail.kapsi.fi ([217.30.184.167]:49233 "EHLO mail.kapsi.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751341AbcAFRSC (ORCPT ); Wed, 6 Jan 2016 12:18:02 -0500 In-Reply-To: <1452009438-27347-1-git-send-email-peda@lysator.liu.se> Sender: linux-i2c-owner@vger.kernel.org List-Id: linux-i2c@vger.kernel.org To: Peter Rosin , Wolfram Sang Cc: Peter Rosin , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Peter Korsgaard , Guenter Roeck , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Mauro Carvalho Chehab , Frank Rowand , Grant Likely , Adriana Reus , Srinivas Pandruvada , Krzysztof Kozlowski , Hans Verkuil , Nicholas Mc Guire , Olli Salonen , linux-i2c@vger.kernel.org, devicetree@vger.kerne On 01/05/2016 05:57 PM, Peter Rosin wrote: > From: Peter Rosin > > 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(-) > -- http://palosaari.fi/