Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Rosin <peda@axentia.se>
To: Luca Ceresoli <luca@lucaceresoli.net>, jacopo mondi <jacopo@jmondi.org>
Cc: "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Vladimir Zapolskiy <vz@mleia.com>
Subject: Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Date: Wed, 4 Sep 2019 08:09:01 +0000
Message-ID: <51dede3c-545b-b66a-5e89-9e889d784eb9@axentia.se> (raw)
In-Reply-To: <20bac324-c4d3-270c-5175-0a7f261fd760@lucaceresoli.net>

Hi!

[ Sorry about my absence. I've been meaning to comment on this series
  for a long time, but work and family keep interfering... ]

On 2019-09-03 09:31, Luca Ceresoli wrote:
> Hi Jacopo,
> 
> thanks for your feedback.
> 
> On 01/09/19 16:31, jacopo mondi wrote:
>> Hi Luca,
>>    thanks for keep pushing this series! I hope we can use part of this
>> for the (long time) on-going GMSL work...
>>
>> I hope you will be patient enough to provide (another :) overview
>> of this work during the BoF Wolfram has organized at LPC for the next
>> week.
> 
> Sure!
> 
>> In the meantime I would have some comments after having a read at the
>> series and trying to apply its concept to GMSL
>>
>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote:
>>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>>> slave "upstream" port and N master "downstream" ports, and forwards
>>> transactions from upstream to the appropriate downstream port. But is
>>> is different in that the forwarded transaction has a different slave
>>> address. The address used on the upstream bus is called the "alias"
>>> and is (potentially) different from the physical slave address of the
>>> downstream chip.
>>>
>>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>>> implementing ATR features in a device driver. The helper takes care or
>>> adapter creation/destruction and translates addresses at each transaction.
>>>
>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>>
>>> ---
>>>
>>> Changes RFCv1 -> RFCv2:
>>>
>>>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>>>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>>>  features are not in a MUX and vice versa, the overlapping is low. This was
>>>  almost a complete rewrite, but for the records here are the main
>>>  differences from the old implementation:
>>
>> I'm not an i2c expert, but this looks very similar to me to an
>> augmented i2c-mux with the following additional features:
>> - automatic selection of the i2c address to use for the children
>>   behind the mux
>> - automatic translation of the addresses the logical aliases to
>>   the actual physical addresses of the slaves (with the help of the
>>   deserializer address translation feature in this case).
> 
> A notable difference in the hardware is that a mux needs an explicit
> procedure to select a port. That's why the select() op is mandatory for
> muxes. The ATR, at least in the DS90UB9xx case, selects the port
> automatically based on the slave address. So I added an optional
> select() op in the atr, but I suspect it's useless for "real" ATRs.
> 
> More differences derive from how Linux implements muxes. The i2c-mux
> code has several flags for different locking schemes, and it has a

It's two locking schemes if you count them carefully, so several is
a bit of an exaggeration. But agreed, two is more than I prefer.

> fairly complex DT parsing scheme. These are mostly a historical burden.
> Adding the ATR features to i2c-mux.c was very tricky and error-prone due
> to all of this code, that's why I have moved ATR to its own file in RFCv2.

Anyway, the locking in i2c-mux may be complex, but it does solve real
problems. The way I read this series, these problems are not dealt with
and therefore the ATR code will not function in all surroundings.

Some things to think about:
- What happens when you put a mux behind an ATR?
- What happens when you put an ATR behind a mux?
- What happens when you put an ATR between two muxes?
- Does it make a difference if the mux is parent-locked or mux-locked?
- What happens if client drivers lock the adapter in order to silence the
  bus for some reason or to keep two xfers together or something, and
  then do unlocked xfers?
- Can you put an ATR behind another ATR?
etc

I'm not saying that these things must be handled, and maybe they are
handled already, and maybe some of the combinations are not valid at
all. But the possibilities and limitations should be understood. And
preferably documented.

My gut feeling (without spending much time on it) is that ATR as
implemented in this series behave approximately like mux-locked muxes,
meaning that it is not obviously safe to put a parent-locked mux behind
an ATR and making it impossible for client devices behind an ATR to force
xfers to happen back-to-back from the root adapter.

And finally, I feel that it is not wise to introduce a third locking
scheme in the I2C topology. It's bad enough with two. Why not make the
ATR locking exactly match the mux-locked scheme to reduce the number
of cases one needs to consider? But maybe that's just me?

Cheers,
Peter

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-23 20:37 [RFC,v2 0/6] TI camera serdes and I2C address translation Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 1/6] i2c: core: let adapters be notified of client attach/detach Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support Luca Ceresoli
2019-09-01 14:31   ` jacopo mondi
2019-09-03  7:31     ` Luca Ceresoli
2019-09-03  7:37       ` Wolfram Sang
2019-09-04  8:09       ` Peter Rosin [this message]
2019-09-08 19:40         ` Luca Ceresoli
2019-09-10 18:46           ` Wolfram Sang
2019-09-08 20:45     ` Vladimir Zapolskiy
2019-09-09  4:56       ` Vladimir Zapolskiy
2019-09-10 17:40         ` Luca Ceresoli
2019-09-09  7:22       ` Wolfram Sang
2019-09-09 15:10         ` Vladimir Zapolskiy
2019-09-09 17:48           ` Luca Ceresoli
2019-09-10 17:16             ` Wolfram Sang
2019-09-02 20:42   ` Wolfram Sang
2019-09-03  8:48     ` Luca Ceresoli
2019-09-03  9:06       ` Wolfram Sang
2019-07-23 20:37 ` [RFC,v2 3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer Luca Ceresoli
2019-08-13 15:44   ` Rob Herring
2019-08-19 22:41     ` Luca Ceresoli
2019-08-20 15:44       ` Rob Herring
2019-08-21 21:50         ` Luca Ceresoli
2019-09-02 20:48   ` Wolfram Sang
2019-09-03  9:09     ` Luca Ceresoli
2019-09-03  9:34       ` Wolfram Sang
2019-09-03 11:03         ` Luca Ceresoli
2019-09-03 14:16           ` Wolfram Sang
2019-09-10  9:43   ` Sakari Ailus
2019-09-10 15:02     ` Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 4/6] media: dt-bindings: add DS90UB953-Q1 video serializer Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 5/6] media: ds90ub954: new driver for TI DS90UB954-Q1 video deserializer Luca Ceresoli
2019-07-23 20:37 ` [RFC,v2 6/6] media: ds90ub953: new driver for TI DS90UB953-Q1 video serializer Luca Ceresoli

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51dede3c-545b-b66a-5e89-9e889d784eb9@axentia.se \
    --to=peda@axentia.se \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jacopo@jmondi.org \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=luca@lucaceresoli.net \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=vz@mleia.com \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git