Hi Tomi, +Cc Matti On Thu, 28 Nov 2024 19:50:46 +0200 Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote: > 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote: > >>> > >>>> From: Cosmin Tanislav <demonsingur@xxxxxxxxx> > >>>> > >>>> 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. Thanks for the extensive and very useful explanation. I had completely missed the GMSL serder and their different I2C handling, apologies. So, the "parent ATR" is the GMSL deser, which is not an ATR but implementing it using i2c-atr makes the implementation cleaner. That makes sense. However removing the checks in i2c_atr_map_msgs() is dangerous in the general case with "proper" ATRs (the TI ones and AFAIK the Rohm ones) and it conflicts with the FPC202 case as Romain pointed out. So I think we need those checks to be disabled only when in the the "nested ATR" scenario, which leads to Romain's suggestion of adding a flag when instantiating the ATR, so I'll switch to that branch of this discussion. > > 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. I agree this won't make sense for the GMSL case you described. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
