| Message ID | 20241122-i2c-atr-fixes-v2-2-0acd325b6916@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | i2c: atr: A few i2c-atr fixes | expand |
On Fri, Nov 22, 2024 at 02:26:19PM +0200, Tomi Valkeinen wrote:
...
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
SoB != committer.
(Yes, I know what + means in the email there, but I don't know
if it's a problem for all those checks or not)
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. However I'm very likely missing something. Can you elaborate with a practical example? Best regards, Luca
Hello Tomi, On vendredi 22 novembre 2024 13:26:19 heure normale d’Europe centrale Tomi Valkeinen 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. > > Relax the checks, and allow non-mapped transactions to fix this issue. I have a series in the review pipeline which adds optional support for dynamic I2C ATR address translation. If this option is enabled, then transactions on any unmapped address are assigned an alias on-the-fly. This feature is required to handle alias shortages on some hardware setups: https://lore.kernel.org/all/20241125-fpc202-v3-0-34e86bcb5b56@bootlin.com/ Letting all non-mapped transactions through would conflict with my series, since these two scenarios would be indistinguishable: case 1: transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x56 alias pool can only hold 2 mappings at a time, so transaction cannot go through case 2: transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x50 alias pool can hold 2 mappings at a time, so transaction can go through Could you perhaps introduce an ATR flag which would enable/disable letting all unmapped messages through? I have something similar in patch 8 of my FPC202 series: https://lore.kernel.org/all/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/ There could be a flag named "I2C_ATR_FLAG_NESTED_ATR", which would be enabled in i2c_atr_new(): ``` @@ i2c_atr_new() if (is_an_atr(parent)) atr->flags |= I2C_ATR_FLAG_NESTED_ATR; ``` Please let me know what you think. Best Regards,
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. 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. Tomi
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. 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 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. 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. Best regards, Luca
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. > 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
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
Hello Tomi, Romain, +Cc Matti On Tue, 26 Nov 2024 09:34:56 +0100 Romain Gantois <romain.gantois@bootlin.com> wrote: > Hello Tomi, > > On vendredi 22 novembre 2024 13:26:19 heure normale d’Europe centrale Tomi Valkeinen 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. > > > > Relax the checks, and allow non-mapped transactions to fix this issue. > > I have a series in the review pipeline which adds optional support for dynamic > I2C ATR address translation. If this option is enabled, then transactions on any > unmapped address are assigned an alias on-the-fly. This feature is required to > handle alias shortages on some hardware setups: > > https://lore.kernel.org/all/20241125-fpc202-v3-0-34e86bcb5b56@bootlin.com/ > > Letting all non-mapped transactions through would conflict with my series, since > these two scenarios would be indistinguishable: > > case 1: > transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x56 > alias pool can only hold 2 mappings at a time, so transaction cannot go through > > case 2: > transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x50 > alias pool can hold 2 mappings at a time, so transaction can go through > > Could you perhaps introduce an ATR flag which would enable/disable letting all unmapped > messages through? I have something similar in patch 8 of my FPC202 series: > > https://lore.kernel.org/all/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/ > > There could be a flag named "I2C_ATR_FLAG_NESTED_ATR", which would be enabled in i2c_atr_new(): > > ``` > @@ i2c_atr_new() > > if (is_an_atr(parent)) > atr->flags |= I2C_ATR_FLAG_NESTED_ATR; > ``` As I discussed on another branch of this thread, after Tomi's explanations I think the flag is the correct solution because disabling the checks in i2c_atr_map_msgs() is useful for a specific implementation of the GMSL deserializer, but armful is other cases. About the implementation, I think we should not use something like is_an_atr(parent). First, it would potentially need to recursively climb the parent tree, and handling all combinations of ats/mux/whatever would be very complex. But even more because it is not needed. Tomi explained the use case is for the GMSL deser being the "parent ATR" (even though it is not physically an ATR) and the GMSL serializer the "child ATR", and the change in this patch is only needed for the parent ATR AFAICU. So the GMSL deser driver could unconditionally set the flag, and no other driver should ever set it. Tomi, do you think this is correct? Based on the above, i2c_atr_new() or other parts of the i2c-atr code are unable to self-detect whether the flag should be set or not. That would mean we have a new user for the 'flags' field of i2c_atr_new() that Romain proposed [0]. Finally, I think the name should not mention "nested ATR" but rather tell what it does, like I2C_ATR_FLAG_PASS_THROUGH. So, that's my random thoughts, what do you think about this? [0] https://lore.kernel.org/linux-i2c/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/ Luca
On 29/11/2024 13:53, Luca Ceresoli wrote: >> 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. Right. But, honestly, I can't make my mind if I like the use of ATR here or not =). So it's not an ATR, but I'm not quite sure what it is. It's not just that we need to change the addresses of the serializers, we need to do that in particular way, enabling one port at a time to do the change. If we forget about the init time hurdles, and consider the situation after the serializers are been set up and all ports have been enabled, we have: There's the main i2c bus, on which we have the deserializer. The deserializer acts as a i2c repeater (for any transaction that's not directed to the deser), sending the messages to all serializers. The serializers catch transactions directed at the ser, and everything else goes through ATR and to the remote bus. Do we have something that represents such a "i2c repeater"? I guess we could just have an i2c bus, created by the deser, and all the sers would be on that bus. So we'd somehow do the initial address change first, then set up the i2c bus, and the serializer i2c clients would be added to that bus. Tomi
Hello Tomi, On Fri, 29 Nov 2024 15:31:45 +0200 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > On 29/11/2024 13:53, Luca Ceresoli wrote: > > >> 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. > > Right. > > But, honestly, I can't make my mind if I like the use of ATR here or not =). Hehe, indeed, hardware designers use a lot of fantasy in stretching the I2C standard to its limits, perhaps more than actually needed. > So it's not an ATR, but I'm not quite sure what it is. It's not just > that we need to change the addresses of the serializers, we need to do > that in particular way, enabling one port at a time to do the change. > > If we forget about the init time hurdles, and consider the situation > after the serializers are been set up and all ports have been enabled, > we have: > > There's the main i2c bus, on which we have the deserializer. The > deserializer acts as a i2c repeater (for any transaction that's not > directed to the deser), sending the messages to all serializers. The > serializers catch transactions directed at the ser, and everything else > goes through ATR and to the remote bus. > > Do we have something that represents such a "i2c repeater"? I guess we > could just have an i2c bus, created by the deser, and all the sers would > be on that bus. So we'd somehow do the initial address change first, > then set up the i2c bus, and the serializer i2c clients would be added > to that bus. So you think about another thing, like i2c-repeater, in addition to i2c-mux and i2c-atr? Well, I think it would make sense, as it would generalize a feature that might be used by other chips. However at the moment we do have a working driver for the GMSL deser, and so I don't see the benefit of extracting the i2c-repeater functionality to a separate file, unless there are drivers for other chips being implemented: this would motivate extracting common features to a shared file. IOW, I'd not generalize something with a single user. [Interesting side note: the i2c-atr has been implemented with a single user, violating the above principle O:-) but I think that was due to the similarity with i2c-mux or something like that. Out of luck, another ATR user appeared after some time.] Luca
On 12/3/24 11:39 AM, Luca Ceresoli wrote: > Hello Tomi, > > On Fri, 29 Nov 2024 15:31:45 +0200 > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > >> On 29/11/2024 13:53, Luca Ceresoli wrote: >> >>>> 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. >> >> Right. >> >> But, honestly, I can't make my mind if I like the use of ATR here or not =). > > Hehe, indeed, hardware designers use a lot of fantasy in stretching the > I2C standard to its limits, perhaps more than actually needed. > >> So it's not an ATR, but I'm not quite sure what it is. It's not just >> that we need to change the addresses of the serializers, we need to do >> that in particular way, enabling one port at a time to do the change. >> >> If we forget about the init time hurdles, and consider the situation >> after the serializers are been set up and all ports have been enabled, >> we have: >> >> There's the main i2c bus, on which we have the deserializer. The >> deserializer acts as a i2c repeater (for any transaction that's not >> directed to the deser), sending the messages to all serializers. The >> serializers catch transactions directed at the ser, and everything else >> goes through ATR and to the remote bus. >> >> Do we have something that represents such a "i2c repeater"? I guess we >> could just have an i2c bus, created by the deser, and all the sers would >> be on that bus. So we'd somehow do the initial address change first, >> then set up the i2c bus, and the serializer i2c clients would be added >> to that bus. > > So you think about another thing, like i2c-repeater, in addition to > i2c-mux and i2c-atr? > Since most of the functionality needed (besides allowing pass-through transfers for unmapped I2C addresses) can be achieved already using I2C ATR, I think we should make use of it. > Well, I think it would make sense, as it would generalize a feature > that might be used by other chips. However at the moment we do have a > working driver for the GMSL deser, and so I don't see the benefit of > extracting the i2c-repeater functionality to a separate file, unless > there are drivers for other chips being implemented: this would motivate > extracting common features to a shared file. IOW, I'd not generalize > something with a single user. > We have GMSL drivers for 6 new chips that make use of the I2C ATR, and we want to send these upstream. Adding pass-through support for the I2C ATR is one of the only things keeping us back, and it's the solution that makes the most sense to me. Semantically, indeed, our GMSL deserializers don't have an ATR hardware block. The serializers do, with the particularity that they pass through all traffic, even if it is for unmapped addresses. The current GMSL2 deserializer driver doesn't make use of ATR functionality because it's a single link deserializer, so it doesn't need to handle multiple serializers with the same I2C address. The GMSL deserializers we want to add have more than one link (either 2 or 4), and they need the ATR to reassign the serializer I2C addresses to ones that can be addresses without conflicts. The address changing is done in the ATR attach_client() callback. The ATR driver already exists and allows us to implement this, even if semantically there's no translation block. > [Interesting side note: the i2c-atr has been implemented with a single > user, violating the above principle O:-) but I think that was due to the > similarity with i2c-mux or something like that. Out of luck, another > ATR user appeared after some time.] > > Luca >
Hello Cosmin, Tomi, On Tue, 3 Dec 2024 12:35:29 +0200 Cosmin Tanislav <demonsingur@gmail.com> wrote: > On 12/3/24 11:39 AM, Luca Ceresoli wrote: > > Hello Tomi, > > > > On Fri, 29 Nov 2024 15:31:45 +0200 > > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > > > >> On 29/11/2024 13:53, Luca Ceresoli wrote: > >> > >>>> 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. > >> > >> Right. > >> > >> But, honestly, I can't make my mind if I like the use of ATR here or not =). > > > > Hehe, indeed, hardware designers use a lot of fantasy in stretching the > > I2C standard to its limits, perhaps more than actually needed. > > > >> So it's not an ATR, but I'm not quite sure what it is. It's not just > >> that we need to change the addresses of the serializers, we need to do > >> that in particular way, enabling one port at a time to do the change. > >> > >> If we forget about the init time hurdles, and consider the situation > >> after the serializers are been set up and all ports have been enabled, > >> we have: > >> > >> There's the main i2c bus, on which we have the deserializer. The > >> deserializer acts as a i2c repeater (for any transaction that's not > >> directed to the deser), sending the messages to all serializers. The > >> serializers catch transactions directed at the ser, and everything else > >> goes through ATR and to the remote bus. > >> > >> Do we have something that represents such a "i2c repeater"? I guess we > >> could just have an i2c bus, created by the deser, and all the sers would > >> be on that bus. So we'd somehow do the initial address change first, > >> then set up the i2c bus, and the serializer i2c clients would be added > >> to that bus. > > > > So you think about another thing, like i2c-repeater, in addition to > > i2c-mux and i2c-atr? > > > > Since most of the functionality needed (besides allowing pass-through > transfers for unmapped I2C addresses) can be achieved already using I2C > ATR, I think we should make use of it. If it allows code reuse, then it makes sense. > > Well, I think it would make sense, as it would generalize a feature > > that might be used by other chips. However at the moment we do have a > > working driver for the GMSL deser, and so I don't see the benefit of > > extracting the i2c-repeater functionality to a separate file, unless > > there are drivers for other chips being implemented: this would motivate > > extracting common features to a shared file. IOW, I'd not generalize > > something with a single user. > > > > We have GMSL drivers for 6 new chips that make use of the I2C ATR, and > we want to send these upstream. Adding pass-through support for the I2C > ATR is one of the only things keeping us back, and it's the solution > that makes the most sense to me. I see, so I understand the aim of this patch. As it was presented initially, in a "fixes" series and without any mention to new GMSL drivers, it didn't make sense to me. I think it would be ideal to send it in the same series as the GMSL driver(s) using it, otherwise there is no reason to want this change. Also it should not be presented as a fix, because it is not: it is changing the ATR functionality in order to extend it to new chips. Finally, documenting the oddities (the deser being implemented using i2c-atr even though it is not an ATR, in the first place) wouldbe very useful in the commit message and/or cover letter. Best regards, Luca
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c index 0d54d0b5e327..5adb720af84e 100644 --- a/drivers/i2c/i2c-atr.c +++ b/drivers/i2c/i2c-atr.c @@ -134,7 +134,6 @@ i2c_atr_find_mapping_by_addr(const struct list_head *list, u16 phys_addr) static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs, int num) { - struct i2c_atr *atr = chan->atr; static struct i2c_atr_alias_pair *c2a; int i; @@ -157,15 +156,8 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs, c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list, msgs[i].addr); - if (!c2a) { - dev_err(atr->dev, "client 0x%02x not mapped!\n", - msgs[i].addr); - - while (i--) - msgs[i].addr = chan->orig_addrs[i]; - - return -ENXIO; - } + if (!c2a) + continue; msgs[i].addr = c2a->alias; }