| Message ID | 20241125-fpc202-v3-0-34e86bcb5b56@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | misc: Support TI FPC202 dual-port controller | expand |
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. 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. 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? How this relates to the "Support dynamic address translation" and GMSL, I'm not sure yet. Tomi
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,
Hello Romain, Tomi, On Tue, 03 Dec 2024 09:42:07 +0100 Romain Gantois <romain.gantois@bootlin.com> wrote: > 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. I think this is a reasonable assumption, and so we should not implement support for "non-dynamic ATRs" unless (until?) there is a valid use case. Luca
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. Best Regards, Romain Signed-off-by: Romain Gantois <romain.gantois@bootlin.com> --- Changes in v3: - Described the "reg" property of downstream ports in the FPC202 bindings - Link to v2: https://lore.kernel.org/r/20241118-fpc202-v2-0-744e4f192a2d@bootlin.com Changes in v2: - Renamed port nodes to match i2c adapter bindings. - Declared atr ops struct as static const. - Free downstream ports during FPC202 removal. - Link to v1: https://lore.kernel.org/r/20241108-fpc202-v1-0-fe42c698bc92@bootlin.com --- Romain Gantois (9): dt-bindings: misc: Describe TI FPC202 dual port controller media: i2c: ds90ub960: Replace aliased clients list with bitmap media: i2c: ds90ub960: Protect alias_use_mask with a mutex i2c: use client addresses directly in ATR interface i2c: move ATR alias pool to a separate struct i2c: rename field 'alias_list' of struct i2c_atr_chan to 'alias_pairs' i2c: support per-channel ATR alias pools i2c: Support dynamic address translation misc: add FPC202 dual port controller driver .../devicetree/bindings/misc/ti,fpc202.yaml | 96 +++++ MAINTAINERS | 7 + drivers/i2c/i2c-atr.c | 480 ++++++++++++++------- drivers/media/i2c/ds90ub913.c | 9 +- drivers/media/i2c/ds90ub953.c | 9 +- drivers/media/i2c/ds90ub960.c | 65 +-- drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/ti_fpc202.c | 440 +++++++++++++++++++ include/linux/i2c-atr.h | 67 ++- 10 files changed, 989 insertions(+), 196 deletions(-) --- base-commit: adc218676eef25575469234709c2d87185ca223a change-id: 20241017-fpc202-6f0b739c2078 Best regards,