Message-ID: <20241127131931.19af84c2@booty>
Date: Wed, 27 Nov 2024 13:19:31 +0100
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.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

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.

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists