Re: [PATCH v7 1/7] i2c: add I2C Address Translator (ATR) support

From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: "Andy Shevchenko" <andriy.shevchenko@intel.com>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Wolfram Sang" <wsa@kernel.org>,
	"Matti Vaittinen" <Matti.Vaittinen@fi.rohmeurope.com>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Peter Rosin" <peda@axentia.se>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Michael Tretter" <m.tretter@pengutronix.de>,
	"Shawn Tu" <shawnx.tu@intel.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Mike Pagano" <mpagano@gentoo.org>,
	"Krzysztof Hałasa" <khalasa@piap.pl>,
	"Marek Vasut" <marex@denx.de>,
	"Luca Ceresoli" <luca@lucaceresoli.net>
Subject: Re: [PATCH v7 1/7] i2c: add I2C Address Translator (ATR) support
Date: Thu, 19 Jan 2023 14:08:06 +0100	[thread overview]
Message-ID: <20230119140806.6b926cad@booty> (raw)
In-Reply-To: <ddbf82d0-155f-9189-44df-1f796c028b53@ideasonboard.com>

Hi Tomi, Andy,

On Thu, 19 Jan 2023 14:39:09 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> On 19/01/2023 10:21, Luca Ceresoli wrote:
> > Hi Andy,
> > 
> > On Wed, 18 Jan 2023 19:39:46 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >   
> >> On Wed, Jan 18, 2023 at 06:17:53PM +0100, Luca Ceresoli wrote:  
> >>> On Wed, 18 Jan 2023 16:23:53 +0200
> >>> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> >>
> >> ...
> >>  
> >>>>> +A typical example follows.
> >>>>> +
> >>>>> +Topology::
> >>>>> +
> >>>>> +                      Slave X @ 0x10
> >>>>> +              .-----.   |
> >>>>> +  .-----.     |     |---+---- B
> >>>>> +  | CPU |--A--| ATR |
> >>>>> +  `-----'     |     |---+---- C
> >>>>> +              `-----'   |
> >>>>> +                      Slave Y @ 0x10
> >>>>> +
> >>>>> +Alias table:
> >>>>> +
> >>>>> +.. table::
> >>>>> +
> >>>>> +   ======   =====
> >>>>> +   Client   Alias
> >>>>> +   ======   =====
> >>>>> +   X        0x20
> >>>>> +   Y        0x30
> >>>>> +   ======   =====
> >>>>> +
> >>>>> +Transaction:
> >>>>> +
> >>>>> + - Slave X driver sends a transaction (on adapter B), slave address 0x10
> >>>>> + - ATR driver rewrites messages with address 0x20, forwards to adapter A
> >>>>> + - Physical I2C transaction on bus A, slave address 0x20
> >>>>> + - ATR chip propagates transaction on bus B with address translated to 0x10
> >>>>> + - Slave X chip replies on bus B
> >>>>> + - ATR chip forwards reply on bus A
> >>>>> + - ATR driver rewrites messages with address 0x10
> >>>>> + - Slave X driver gets back the msgs[], with reply and address 0x10  
> >>>>
> >>>> I'm not sure I got the real / virtual status of the adapters. Are the B and C
> >>>> virtual ones, while A is the real?  
> >>>
> >>> Let me reply, as I wrote these docs back at the times and thus I feel
> >>> guilty in case that's unclear. :)
> >>>
> >>> I don't like the word "virtual" in this situation. A, B and C are all
> >>> physical busses, made of copper and run by electrons on PCBs. B and C
> >>> are the "remote" or "downstream" busses (w.r.t. the CPU), where the i2c
> >>> devices are and where transactions happen using the address that the
> >>> chip responds to. A is the "local" or "upstream" bus that is driven
> >>> directly by the CPU (*) and where address aliases are used. Using
> >>> aliases there is necessary because using address 0x10 would be
> >>> ambiguous as there are two 0x10 chips out there.
> >>>
> >>> (*) There could be more layers of course, but still A is "closer to the
> >>> CPU than B and C", for the sake of completeness.  
> >>
> >> Can the diagram and/or text be updated to elaborate this?  
> > 
> > Let's see whether the text below is better. I haven't changed the
> > image, I don't think we can do much more in ASCII, but maybe we can
> > replace it with an SVG [0]?
> > 
> > [0]
> > https://github.com/lucaceresoli/docs/blob/master/video-serdes-linux/images/i2c-ti.svg
> > 
> > A typical example follows.
> > 
> > Topology::
> > 
> >                        Slave X @ 0x10
> >                .-----.   |
> >    .-----.     |     |---+---- B
> >    | CPU |--A--| ATR |
> >    `-----'     |     |---+---- C
> >                `-----'   |
> >                        Slave Y @ 0x10  
> 
> Slightly beside the point of this discussion, but one thing (I think) I 
> tried to highlight in some older cover letter was that we don't really 
> have the above structure. We have something like this (a quick edit, sorry):
> 
>                              .------.  Slave X @ 0x10
>                  .------.    | FPDS |   |
>      .-----.     | FPDD |-F1-`------'---+---- B
>      | CPU |--A--| ATR  |
>      `-----'     |      |-F2-.------.---+---- C
>                  `------'    | FPDS |   |
>                              `------'  Slave Y @ 0x10
> 
> Where FPDD = Deserializer, FPDS = Serializer, F1/F2 = FPD-Link bus 1/2.
> 
> So the ATR functionality is in the deserializer, but the actual remote 
> i2c bus is on the serializer.

I'd rather say that the ATF functionality is in the sum of ser+des as
they really cooperate. But this is kind of philosophical. :) What
matters is that it's worth mentioning that the "ATR" box is actually an
abstract visualization of a feature that is provided by two or more
chips (in the known universe, at least).

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com