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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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.

The current code manages this so that the deserializer driver owns the 
ATR and programs the HW (as the ATR is part of the deserializer), but 
it's the serializer driver that adds the remote adapter to the ATR 
(using i2c_atr pointer given by the deserializer driver).

  Tomi