Re: [PATCH v3 0/9] misc: Support TI FPC202 dual-port controller

[Romain Gantois <romain.gantois@bootlin.com>]

Hi Tomi,

On vendredi 29 novembre 2024 13:01:58 heure normale d’Europe centrale Tomi Valkeinen wrote:
> Hi,
> 
> On 25/11/2024 10:45, Romain Gantois wrote:
> > Hello everyone,
> > 
> > This is version three of my series which adds support for the TI FPC202
> > dual-port controller. This is an unusual kind of device which is used as a
> > low-speed signal aggregator for various types of SFP-like hardware ports.
> > 
> > The FPC202 exposes an I2C, or SPI (not supported in this series) control
> > interface, which can be used to access two downstream I2C busses, along
> > with a set of low-speed GPIO signals for each port. It also has I2C
> > address
> > translation (ATR) features, which allow multiple I2C devices with the same
> > address (e.g. SFP EEPROMs at address 0x50) to be accessed from the
> > upstream
> > control interface on different addresses.
> > 
> > I've chosen to add this driver to the misc subsystem, as it doesn't
> > strictly belong in either the i2c or gpio sybsystem, and as far as I know
> > it is the first device of its kind to be added to the kernel.
> > 
> > Along with the FPC202 driver itself, this series also adds support for
> > dynamic address translation to the i2c-atr module. This allows I2C address
> > translators to update their translation table on-the-fly when they receive
> > transactions to unmapped clients. This feature is needed by the FPC202
> > driver to access up to three logical I2C devices per-port, given that the
> > FPC202 address translation table only has two address slots.
> 
> While the FPD-Link devices are quite different than the TPC202, I wonder
> what's the difference wrt. the ATR... Afaics, the difference is that the
> FPC202 has 2 slots whereas UB960 has 8. So if you have 3+ remote devices
> on FPC202, you get problems, or if you have 9+ devices on UB960, you get
> problems.
> 
> Yet this series adds a I2C_ATR_FLAG_DYNAMIC_C2A flag which the driver
> needs to set, and the i2c-atr has different code paths depending on the
> flag. In other words, either the driver author (if it's a hardcoded
> flag) or the driver (if it's set dynamically) is assumed to know how
> many remote devices there are, and whether that flag is needed.
> 
> On the other hand, if I consider I2C_ATR_FLAG_DYNAMIC_C2A meaning that
> the device can support dynamically changing the ATR, then it makes more
> sense, and also UB960 should set the flag.
> 

Indeed, the need for dynamic address translation isn't solely determined by
the ATR model. It's also determined by the number of logical I2C devices
connected to the downstream ports. In that sense, hardcoding the flag in the
ATR driver doesn't seem completely appropriate.

However, you could reasonably imagine that some future ATR models won't
support hot-swapping aliases at runtime. In this case, this flag will be
necessary at the very least as a capability flag i.e. "this ATR model can do
dynamic translation but it's not necessarily activated by default".

> But then I wonder, do we even have cases with ATRs that need to be
> programmed once at init time, and cannot be changed afterwards? If not,
> then the I2C_ATR_FLAG_DYNAMIC_C2A can be the default, and the
> non-I2C_ATR_FLAG_DYNAMIC_C2A code can be dropped. Actually, even the
> current upstream i2c-atr is dynamic in a sense: the clients are attached
> via the i2c_atr_bus_notifier_call(), one by one.
> 

Indeed, if an ATR component doesn't support hot-swapping of aliases, then
it will be broken anyway if a device attaches after the ATR's been initialized.
Maybe we should just assume that all supported ATR's should be capable of
modifying their translation table after initialization then.

> If we just have .attach_addr()/.detach_addr(), and call those on demand,
> and perhaps use LRU to handle the ATR table, it would maybe work for
> both FPC202 and FPD-Link just fine.
> 
> So TLDR: do we even need any kind of special "dynamic atr"-feature for
> FPC202, or is it really just a small improvement to the current i2c-atr
> and applies to all ATR devices?
> 

In any case, it's a necessary improvement for the FPC202 case, but it could
indeed apply to all ATR devices. Maybe we should just enable dynamic
translation by default.

However, I don't quite understand what you mean by "calling attach/detach()
on demand", my current understanding is that we should call attach from
the bus notifier and from the ATR I2C transaction handler. Do you mean that
these callbacks should be called from other parts of the kernel?

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com