Message-ID: <30732dbb-21e6-4075-84b1-544fc6e6abce@ideasonboard.com>
Date: Thu, 28 Nov 2024 19:50:46 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, Wolfram Sang <wsa@...nel.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Cosmin Tanislav <demonsingur@...il.com>,
 Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>,
 Romain Gantois <romain.gantois@...tlin.com>
Subject: Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested
 ATRs

Hi,

On 27/11/2024 14:19, Luca Ceresoli wrote:
> Hello Tomi,
> 
> On Tue, 26 Nov 2024 10:35:46 +0200
> Tomi Valkeinen <tomi.valkeinen@...asonboard.com> wrote:
> 
>> Hi Luca,
>>
>> On 26/11/2024 10:16, Luca Ceresoli wrote:
>>> Hello Tomi,
>>>
>>> On Fri, 22 Nov 2024 14:26:19 +0200
>>> Tomi Valkeinen <tomi.valkeinen@...asonboard.com> wrote:
>>>    
>>>> From: Cosmin Tanislav <demonsingur@...il.com>
>>>>
>>>> i2c-atr translates the i2c transactions and forwards them to its parent
>>>> i2c bus. Any transaction to an i2c address that has not been mapped on
>>>> the i2c-atr will be rejected with an error.
>>>>
>>>> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
>>>> gets a transaction to an i2c address that is not mapped in the parent
>>>> i2c-atr, and thus fails.
>>>
>>> Nested ATRs are "interesting", to say the least! :-)
>>>
>>> I must say I don't understand the problem here. If this is the picture:
>>>
>>>     adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>>>                       map:               map:              addr:
>>>                    alias addr         alias addr           0x10
>>>                    0x30  0x20         0x20  0x10
>>>
>>> Then I'd expect this:
>>>
>>>    1. the leaf device asks ATR2 for a transaction to 0x10
>>>    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
>>>    3. ATR2 asks ATR1 for a transaction to 0x20
>>>    4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
>>>    5. ATR1 asks adapter for transaction on 0x30
>>>
>>> So ATR1 is never asked for 0x10.
>>
>> Yes, that case would work. But in your example the ATR1 somehow has
>> created a mapping for ATR2's alias.
> 
> You're of course right. I had kind of assumed ATR1 is somehow
> configured to map 0x30 on 0x20, but this is not going to happen
> magically and there is no code AFAIK to do that. So of course my
> comment is bogus, thanks for taking time to explain.
> 
>> Generally speaking, ATR1 has aliases only for devices in its master bus
>> (i.e. the i2c bus where the ATR1 is the master, not slave), and
>> similarly for ATR2. Thus I think a more realistic example is:
>>
>>       adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>>                      addr: 0x50         addr: 0x30
>>                         map:               map:              addr:
>>                      alias addr         alias addr           0x10
>>                      0x40  0x30         0x20  0x10
>>
>> So, both ATRs create the alias mapping based on the i2c-aliases given to
>> them in the DT, for the slave devices in their i2c bus. Assumption is,
>> of course, that the aliases are not otherwise used, and not overlapping.
>>
>> Thus the aliases on ATR2 are not present in the alias table of ATR1.
> 
> OK, so the above is what now I'd expect to be configured in the ATR
> alias tables.
> 
> I still fail to understand how that would work. This is the actions I'd
> expect:
> 
>    1. the leaf device asks ATR2 for a transaction to 0x10
>    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
>    3. ATR2 asks ATR1 for a transaction to 0x20
>    4. 0x20 is *not* in ATR1 map, *but* this patch is applied
>        => i2c-atr lets the transaction through, unmodified
>    5. ATR1 asks adapter for transaction on 0x20
>    6. adapter sends transaction for 0x20 on wires
>    7. ATR1 chip receives transaction for 0x20
>        => 0x20 not in its tables, ignores it
> 
> Note steps 1-5 are in software (kernel). Step 7 may work if ATR1 were
> configured to let all transactions for unknown addresses go through
> unmodified, but I don't remember having seen patches to allow that in
> i2c-atr.c and I'm not even sure the hardware allows that, the DS90UB9xx
> at least.

DS90UB9xx has I2C_PASS_THROUGH_ALL. However, our particular use case is 
with Maxim GMSL desers and sers. They're not as nice as the FPD-Link 
devices in this particular area.

Cosmin, feel free to elaborate or fix my mistakes, but here's a summary:

The deserializers don't have ATRs, whereas the serializers do (so vice 
versa compared to FPD-Link). The deserializers forward everything to all 
enabled GMSL ports. At probe/setup time we can enable a single link at a 
time, so that we can direct transactions to a specific serializer (or 
devices behind it), but after the setup, we need to keep all the ports 
enabled, as otherwise the video streams would stop for all the other 
ports except the one we want to send an i2c transaction to.

The serializers have their own i2c address, but transactions to anything 
else go through the ser's ATR. The ATR does the translation, if an entry 
exists in the table, but all transactions are forwarded, whether they 
are translated or not.

Where's the nested ATR, you ask? That's a detail which is a bit 
"interesting": all the serializers have a default i2c address. So we can 
have 4 serializers all replying to the same address. But we don't have 
an ATR at the deser. However, we can change the address of the 
serializer by writing to a serializer register. This must be done at the 
deser probe time (and before the ser driver probes) where we can enable 
just a single link at a time. So at probe time we change the addresses 
of the serializers to be distinct values.

Still no ATR, right? Well, the i2c-atr accomplishes the above quite 
nicely: there's an address pool (for the new ser addresses), 
.attach_client() where we can set the new address for the serializer, 
and .detach_client() where we can (optionally) restore the original 
address. This way the serializer driver will operate using the original 
address, but when it does an i2c transaction, the i2c-atr changes it to 
the new address.

So strictly speaking it's not an ATR, but this achieves the same.

> And even in case that were possible, that would seems a bit fragile.
> What if two child ATRs attached to two different ports of the parent
> ATR use the same alias, and the parent ATR let transactions for such
> alias go through both ports unmodified? Sure, the alias pools can be
> carefully crafted to avoid such duplicated aliases, but pools have long

Yes, the pools have to be non-overlapping and no overlap with anything 
on the main i2c bus.

I feel the GMSL HW requires quite strict design rules, and preferably 
the deser would be on an i2c bus alone. I think an eeprom at 0x10 and a 
remote sensor at 0x10 would cause trouble, without any way to deal with 
it in the SW.

> been considered a non-optimal solution, and they make no sense at all
> in cases like the FPC202 that Romain is working to support.
> 
> Again, I'm pretty sure I'm missing something here. If you could
> elaborate with a complete example, including the case of two child ATRs
> attached to two ports of the same parent ATR, I'm sure that would be
> very helpful.

I hope my text above covered this.

> At my current level of understanding, it looks like the only correct
> way to manage nested ATRs is to add a "recursive alias mapping", i.e.
> to add for each alias another alias to all parent ATRs, up to the top
> one, like in my initial picture. I realize that would imply several
> complications, though.

Yes, that has complications too. Say, if we have a device that has an 
ATR but also passes everything through (like the GMSL ser), then the 
driver has to manage two different kinds of aliases: one set for the 
actual ATR, and one that are just pass-through ones.

Or what if we have i2c-atr - i2c-mux - i2c-atr chain. The system would 
need to skip the i2c-mux, and continue to the first i2c-atr.

Probably both are solvable, though, but it doesn't sound simple.

  Tomi

Powered by blists - more mailing lists