Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hi Tomi,

+Cc Matti

On Thu, 28 Nov 2024 19:50:46 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> 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@ideasonboard.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@ideasonboard.com> wrote:
> >>>      
> >>>> From: Cosmin Tanislav <demonsingur@gmail.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.

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