Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
> I still have to examine in depth all of the problems in the i2c-mux > documented in Documentation/i2c/i2c-topology (thanks for having written > those docs!), but at first sight it looks like the ATR is not going to > introduce big problems because of how it works. Assuming we are using the previously discussed NEEDS_ATR flag for the adapter instead of the attach/detach callbacks: Can't we then simply understand an ATR as a generic 1:1 mapping device which can be setup when registering an adapter? When we add an adapter using i2c_add_adapter, we have: .-. Slave X @ 0x10 .-. | | | | CPU |--A--| ATR |---+ B `-' | | `-' When we use i2c_add_mux_adapter, we have: Slave X @ 0x10 .-. .-. | .-. | |---| ATR |---+ B | CPU |--A--| MUX | '-' `-' | | .-. | |---| ATR |---+ C `-' '-' | Slave Y @ 0x10 That way we could keep the topology handling solely to the mux-core. Am I overlooking something? signature.asc Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Vladimir, On 09/09/19 05:56, Vladimir Zapolskiy wrote: > Hi Luca, Jacopo, Wolfram, Peter, > > On 09/08/2019 11:45 PM, Vladimir Zapolskiy wrote: >> Hi Luca, Jacopo, Wolfram, Peter, >> >> On 09/01/2019 05:31 PM, jacopo mondi wrote: >>> Hi Luca, >>>thanks for keep pushing this series! I hope we can use part of this >>> for the (long time) on-going GMSL work... >>> >>> I hope you will be patient enough to provide (another :) overview >>> of this work during the BoF Wolfram has organized at LPC for the next >>> week. >>> >>> In the meantime I would have some comments after having a read at the >>> series and trying to apply its concept to GMSL >>> >> >> I won't attend the LPC, however I would appreciate if you book some >> time to review my original / alternative implementation of the TI >> DS90Ux9xx I2C bridge device driver. >> >> For your convenience the links to the driver are given below: >> * dt bindings: >> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b >> * driver code: >> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884 >> * usage example: >> https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc >> >> The reasons why my driver is better/more flexible/more functional are >> discussed earlier, please let me know, if you expect anything else >> from me to add, also I would be happy to get a summary of your offline >> discussion. > > I forgot to repeat my main objection against Luca's approach, the TI > DS90Ux9xx I2C bridge driver does not require to call i2c_add_adapter() > or register a new mux/bus and then do run select/deselect in runtime to > overcome the created handicap. Whether the ser/deser drivers should or not instantiate an adapter depends on whether the child I2C bus is a separate bus or it's "the same bus" as the parent bus. Which in turn is not obvious, and depends on how you look at it. On one hand, the child bus looks like it is the same as the parent bus because transactions on the child bus also happen on the parent bus (bus not the other way around). On the other hand the child bus also looks like a separate bus because it is electrically separated from the parent bus, it can have a different speed, and devices on one child bus cannot talk with devices on another child bus under the same ser/deser. The way you modeled it has the advantage of not requiring a runtime rewriting of slave addresses. Thas's what I implemented in i2c_atr_map_msgs(), I don't love the fact that it happens at every iteration, but its runtime cost is negligible compared to I2C speeds. On the other hand I'm not sure how your approach would work if you have an i2c bridge behind another i2c bridge (see the "ATR behind another ATR" case mentioned by Peter). By contrast, adding an adapter for each child bus has its advantages. I didn't have to write code to instantiate the devices, letting the i2c core do it. Also, as child busses show up as real busses it's possible to instantiate devices from userspace just like any standard i2c bus with: echo eeprom 0x50 > /sys/bus/i2c/devices/i2c-4/new_device and devices will be assigned an alias and be usable normally. Finally, there is no need to call select()/deselect() in runtime. Those callbacks are optional, and I'm probably removing them in a future iteration as I'm more and more convinced they are not needed at all. -- Luca
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
> Here i2c-0 is the "local" bus, i2c-4 and i2c-5 are the remote busses on > ports 0 and 1. As you can see the eeproms are accessed using a name like > "4-0050", meaning physical slave address 0x50 on bus 4. No alias is needed. > > Should you want to know the alias, perhaps for debugging (it's the > address you'll see on your logic analyzer), they are shown in the kernel > log. And to add to that: The aliases on i2c-0 will be marked busy and show up as used if you run i2cdetect. So, you'd need a force-flag if you'd want to access them from userspace. signature.asc Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Vladimir, On 09/09/19 16:10, Vladimir Zapolskiy wrote: > Hi Wolfram, > > On 09/09/2019 10:22 AM, Wolfram Sang wrote: >> Hi Vladimir, >> >>> I won't attend the LPC, however I would appreciate if you book some >> >> A pity. I would have liked to have you in the room. Let's see if we can >> get enough input from you via mail here. >> > > if it might help, I'll attend the Embedded Recipes and ELCE conferences > this year. > >>> time to review my original / alternative implementation of the TI >>> DS90Ux9xx I2C bridge device driver. >> >> We have only 45 minutes, this will not allow to review specific >> implementations. I want to give an overview of existing implementations >> with pros/cons... >> > > Sure! Any shared summary/opinions are greatly welcome. > >>> The reasons why my driver is better/more flexible/more functional are >>> discussed earlier, please let me know, if you expect anything else >>> from me to add, also I would be happy to get a summary of your offline >>> discussion. >> >> ... and I'd appreciate support here from you, like your summary of the >> back then discussion (from where I can dive deeper if needed). >> >> Also, with Luca's new series we discussed some scenarios which can >> happen WRT to I2C address conflicts. Maybe you could comment on them, >> too? > > I do remember I've commented on the Luca's suggestion of using dynamic > I2C addresses from a pool (the introduced "i2c-alias-pool" property). > > I have to scrutinize it in Luca's v2, but then it might happen that the > userspace won't know to which IC on the remote side it communicates to. I think you suspect this because the assigned alias is non-deterministic, so you might end up in writing to the wrong alias. Well, this is not possible because the user is not supposed to know the alias at all. Let's say you have two identical eeproms on remote port 0, with physical addresses 0x50 and 0x51. Then, with my RFCv2 code, you'll access them with physical numbers, not the alias: # i2cdetect -l i2c-0 i2c amba:camera-i2c@0 I2C adapter i2c-4 i2c i2c-0-atr-0I2C adapter i2c-5 i2c i2c-0-atr-1I2C adapter # hexdump /sys/bus/i2c/devices/4-0050/eeprom 000 * 100 # hexdump /sys/bus/i2c/devices/4-0051/eeprom 000 0001 0203 0405 0607 0809 0a0b 0c0d 0e0f * 100 # Here i2c-0 is the "local" bus, i2c-4 and i2c-5 are the remote busses on ports 0 and 1. As you can see the eeproms are accessed using a name like "4-0050", meaning physical slave address 0x50 on bus 4. No alias is needed. Should you want to know the alias, perhaps for debugging (it's the address you'll see on your logic analyzer), they are shown in the kernel log. Does this reply to your concern? >> As I read the old thread, you have a hardcoded aliases using >> "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g. >> add-on modules, do I get this correctly? >> > > Basically hardcoding of aliases completely resolves the highlighted > above problem. Still it is possible to have own DTSI files for the FPD > link detachable PCBs, and this is an exceptionally important scenario, > however some limitations shall be applied: > * dt overlays for "local" derializer/deserializer ICs, it's a generic > and universal solution, it is successfully used in the field, > * only "compatible" PCBs are supposed to be connected (same set of I2C > devices/addresses on every such PCB), this is imperfect for sure, > * "ti,i2c-bridge-maps" list is excessive to cover "almost compatible" > (in the sense from above) PCBs, some of the missed alias matches > just won't instantiate a driver, this is of course also imperfect. > > In general nothing critical should happen, if some I2C device on the > remote side is simply not probed in runtime, in opposite you can imagine > that writing for instance to another EEPROM of two on the remote side > could be harmful. > > Any technically better solution to the two given approaches (from Luca > and from me) is more than appreciated. For non-dynamic/fixed local and > remote PCBs the fixed mapping is better, the dynamic case is covered > by the dt overlays, why not? > > As a side note please do remember that the I2C bridging on Maxim GMSL > and TI DS90Ux9xx are different, the former one is a real mux, and the > latter one is not, I'm uncertain if it's reasonable to think of a > generalized solution which covers both IC series, so likely we > have to review the developed solutions for them separately instead > of trying to work out a combined one. > > -- > Best wishes, > Vladimir > -- Luca
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Wolfram, On 09/09/2019 10:22 AM, Wolfram Sang wrote: > Hi Vladimir, > >> I won't attend the LPC, however I would appreciate if you book some > > A pity. I would have liked to have you in the room. Let's see if we can > get enough input from you via mail here. > if it might help, I'll attend the Embedded Recipes and ELCE conferences this year. >> time to review my original / alternative implementation of the TI >> DS90Ux9xx I2C bridge device driver. > > We have only 45 minutes, this will not allow to review specific > implementations. I want to give an overview of existing implementations > with pros/cons... > Sure! Any shared summary/opinions are greatly welcome. >> The reasons why my driver is better/more flexible/more functional are >> discussed earlier, please let me know, if you expect anything else >> from me to add, also I would be happy to get a summary of your offline >> discussion. > > ... and I'd appreciate support here from you, like your summary of the > back then discussion (from where I can dive deeper if needed). > > Also, with Luca's new series we discussed some scenarios which can > happen WRT to I2C address conflicts. Maybe you could comment on them, > too? I do remember I've commented on the Luca's suggestion of using dynamic I2C addresses from a pool (the introduced "i2c-alias-pool" property). I have to scrutinize it in Luca's v2, but then it might happen that the userspace won't know to which IC on the remote side it communicates to. > As I read the old thread, you have a hardcoded aliases using > "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g. > add-on modules, do I get this correctly? > Basically hardcoding of aliases completely resolves the highlighted above problem. Still it is possible to have own DTSI files for the FPD link detachable PCBs, and this is an exceptionally important scenario, however some limitations shall be applied: * dt overlays for "local" derializer/deserializer ICs, it's a generic and universal solution, it is successfully used in the field, * only "compatible" PCBs are supposed to be connected (same set of I2C devices/addresses on every such PCB), this is imperfect for sure, * "ti,i2c-bridge-maps" list is excessive to cover "almost compatible" (in the sense from above) PCBs, some of the missed alias matches just won't instantiate a driver, this is of course also imperfect. In general nothing critical should happen, if some I2C device on the remote side is simply not probed in runtime, in opposite you can imagine that writing for instance to another EEPROM of two on the remote side could be harmful. Any technically better solution to the two given approaches (from Luca and from me) is more than appreciated. For non-dynamic/fixed local and remote PCBs the fixed mapping is better, the dynamic case is covered by the dt overlays, why not? As a side note please do remember that the I2C bridging on Maxim GMSL and TI DS90Ux9xx are different, the former one is a real mux, and the latter one is not, I'm uncertain if it's reasonable to think of a generalized solution which covers both IC series, so likely we have to review the developed solutions for them separately instead of trying to work out a combined one. -- Best wishes, Vladimir
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Vladimir, > I won't attend the LPC, however I would appreciate if you book some A pity. I would have liked to have you in the room. Let's see if we can get enough input from you via mail here. > time to review my original / alternative implementation of the TI > DS90Ux9xx I2C bridge device driver. We have only 45 minutes, this will not allow to review specific implementations. I want to give an overview of existing implementations with pros/cons... > The reasons why my driver is better/more flexible/more functional are > discussed earlier, please let me know, if you expect anything else > from me to add, also I would be happy to get a summary of your offline > discussion. ... and I'd appreciate support here from you, like your summary of the back then discussion (from where I can dive deeper if needed). Also, with Luca's new series we discussed some scenarios which can happen WRT to I2C address conflicts. Maybe you could comment on them, too? As I read the old thread, you have a hardcoded aliases using "ti,i2c-bridge-maps". This means you can't have own DTSI files for e.g. add-on modules, do I get this correctly? Regards, Wolfram signature.asc Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Luca, Jacopo, Wolfram, Peter, On 09/08/2019 11:45 PM, Vladimir Zapolskiy wrote: > Hi Luca, Jacopo, Wolfram, Peter, > > On 09/01/2019 05:31 PM, jacopo mondi wrote: >> Hi Luca, >>thanks for keep pushing this series! I hope we can use part of this >> for the (long time) on-going GMSL work... >> >> I hope you will be patient enough to provide (another :) overview >> of this work during the BoF Wolfram has organized at LPC for the next >> week. >> >> In the meantime I would have some comments after having a read at the >> series and trying to apply its concept to GMSL >> > > I won't attend the LPC, however I would appreciate if you book some > time to review my original / alternative implementation of the TI > DS90Ux9xx I2C bridge device driver. > > For your convenience the links to the driver are given below: > * dt bindings: > https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b > * driver code: > https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884 > * usage example: > https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc > > The reasons why my driver is better/more flexible/more functional are > discussed earlier, please let me know, if you expect anything else > from me to add, also I would be happy to get a summary of your offline > discussion. I forgot to repeat my main objection against Luca's approach, the TI DS90Ux9xx I2C bridge driver does not require to call i2c_add_adapter() or register a new mux/bus and then do run select/deselect in runtime to overcome the created handicap. > The undeniable fact is that the device tree bindings in my I2C bridge > implementation can be improved further, thanks to Luca for the comments. > >> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: >>> An ATR is a device that looks similar to an i2c-mux: it has an I2C >>> slave "upstream" port and N master "downstream" ports, and forwards >>> transactions from upstream to the appropriate downstream port. But is >>> is different in that the forwarded transaction has a different slave >>> address. The address used on the upstream bus is called the "alias" >>> and is (potentially) different from the physical slave address of the >>> downstream chip. >>> >>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >>> implementing ATR features in a device driver. The helper takes care or >>> adapter creation/destruction and translates addresses at each transaction. >>> >>> Signed-off-by: Luca Ceresoli >>> > -- Best wishes, Vladimir
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Luca, Jacopo, Wolfram, Peter, On 09/01/2019 05:31 PM, jacopo mondi wrote: > Hi Luca, >thanks for keep pushing this series! I hope we can use part of this > for the (long time) on-going GMSL work... > > I hope you will be patient enough to provide (another :) overview > of this work during the BoF Wolfram has organized at LPC for the next > week. > > In the meantime I would have some comments after having a read at the > series and trying to apply its concept to GMSL > I won't attend the LPC, however I would appreciate if you book some time to review my original / alternative implementation of the TI DS90Ux9xx I2C bridge device driver. For your convenience the links to the driver are given below: * dt bindings: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#mead5ea226550b * driver code: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m2fe3664c5f884 * usage example: https://lore.kernel.org/lkml/20181012060314.GU4939@dell/T/#m56c146f5decdc The reasons why my driver is better/more flexible/more functional are discussed earlier, please let me know, if you expect anything else from me to add, also I would be happy to get a summary of your offline discussion. The undeniable fact is that the device tree bindings in my I2C bridge implementation can be improved further, thanks to Luca for the comments. > On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. >> >> Signed-off-by: Luca Ceresoli >> -- Best wishes, Vladimir
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Peter, On 04/09/19 10:09, Peter Rosin wrote:> Hi! > > [ Sorry about my absence. I've been meaning to comment on this series > for a long time, but work and family keep interfering... ] No problem, thanks for your comments. See below my reply. > > On 2019-09-03 09:31, Luca Ceresoli wrote: >> Hi Jacopo, >> >> thanks for your feedback. >> >> On 01/09/19 16:31, jacopo mondi wrote: >>> Hi Luca, >>>thanks for keep pushing this series! I hope we can use part of this >>> for the (long time) on-going GMSL work... >>> >>> I hope you will be patient enough to provide (another :) overview >>> of this work during the BoF Wolfram has organized at LPC for the next >>> week. >> >> Sure! >> >>> In the meantime I would have some comments after having a read at the >>> series and trying to apply its concept to GMSL >>> >>> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: An ATR is a device that looks similar to an i2c-mux: it has an I2C slave "upstream" port and N master "downstream" ports, and forwards transactions from upstream to the appropriate downstream port. But is is different in that the forwarded transaction has a different slave address. The address used on the upstream bus is called the "alias" and is (potentially) different from the physical slave address of the downstream chip. Add a helper file (just like i2c-mux.c for a mux or switch) to allow implementing ATR features in a device driver. The helper takes care or adapter creation/destruction and translates addresses at each transaction. Signed-off-by: Luca Ceresoli --- Changes RFCv1 -> RFCv2: RFCv1 was implemented inside i2c-mux.c and added yet more complexity there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR features are not in a MUX and vice versa, the overlapping is low. This was almost a complete rewrite, but for the records here are the main differences from the old implementation: >>> >>> I'm not an i2c expert, but this looks very similar to me to an >>> augmented i2c-mux with the following additional features: >>> - automatic selection of the i2c address to use for the children >>> behind the mux >>> - automatic translation of the addresses the logical aliases to >>> the actual physical addresses of the slaves (with the help of the >>> deserializer address translation feature in this case). >> >> A notable difference in the hardware is that a mux needs an explicit >> procedure to select a port. That's why the select() op is mandatory for >> muxes. The ATR, at least in the DS90UB9xx case, selects the port >> automatically based on the slave address. So I added an optional >> select() op in the atr, but I suspect it's useless for "real" ATRs. >> >> More differences derive from how Linux implements muxes. The i2c-mux >> code has several flags for different locking schemes, and it has a > > It's two locking schemes if you count them carefully, so several is > a bit of an exaggeration. But agreed, two is more than I prefer. Right, sorry for the exaggeration. Actually the most annoying part of the mux code to me was not quite the two locking schemes, but rather the DT parsing, which had some backward-compatibility to maintain. >> fairly complex DT parsing scheme. These are mostly a historical burden. >> Adding the ATR features to i2c-mux.c was very tricky and error-prone due >> to all of this code, that's why I have moved ATR to its own file in RFCv2. > > Anyway, the locking in i2c-mux may be complex, but it does solve real > problems. The way I read this series, these problems are not dealt with > and therefore the ATR code will not function in all surroundings. > > Some things to think about: > - What happens when you put a mux behind an ATR? > - What happens when you put an ATR behind a mux? > - What happens when you put an ATR between two muxes? > - Does it make a difference if the mux is parent-locked or mux-locked? > - What happens if client drivers lock the adapter in order to silence the > bus for some reason or to keep two xfers together or something, and > then do unlocked xfers? > - Can you put an ATR behind another ATR? > etc I still have to examine in depth all of the problems in the i2c-mux documented in Documentation/i2c/i2c-topology (thanks for having written those docs!), but at first sight it looks like the ATR is not going to introduce big problems because of how it works. An I2C mux works by electrically connecting the parent bus to one of the child busses. But this implies any transactions happening on the parent bus can leak through the mux and cause the problems you documented. The Maxim GMSL serdes chips work de facto in the same way, even though there is no simple electrical connection between the busses. An ATR, at least the one in the TI serdes chips, uses a store-and-forward technique: 1. when a transaction starts on the parent bus, it waits for the
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi! [ Sorry about my absence. I've been meaning to comment on this series for a long time, but work and family keep interfering... ] On 2019-09-03 09:31, Luca Ceresoli wrote: > Hi Jacopo, > > thanks for your feedback. > > On 01/09/19 16:31, jacopo mondi wrote: >> Hi Luca, >>thanks for keep pushing this series! I hope we can use part of this >> for the (long time) on-going GMSL work... >> >> I hope you will be patient enough to provide (another :) overview >> of this work during the BoF Wolfram has organized at LPC for the next >> week. > > Sure! > >> In the meantime I would have some comments after having a read at the >> series and trying to apply its concept to GMSL >> >> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: >>> An ATR is a device that looks similar to an i2c-mux: it has an I2C >>> slave "upstream" port and N master "downstream" ports, and forwards >>> transactions from upstream to the appropriate downstream port. But is >>> is different in that the forwarded transaction has a different slave >>> address. The address used on the upstream bus is called the "alias" >>> and is (potentially) different from the physical slave address of the >>> downstream chip. >>> >>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >>> implementing ATR features in a device driver. The helper takes care or >>> adapter creation/destruction and translates addresses at each transaction. >>> >>> Signed-off-by: Luca Ceresoli >>> >>> --- >>> >>> Changes RFCv1 -> RFCv2: >>> >>> RFCv1 was implemented inside i2c-mux.c and added yet more complexity >>> there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR >>> features are not in a MUX and vice versa, the overlapping is low. This was >>> almost a complete rewrite, but for the records here are the main >>> differences from the old implementation: >> >> I'm not an i2c expert, but this looks very similar to me to an >> augmented i2c-mux with the following additional features: >> - automatic selection of the i2c address to use for the children >> behind the mux >> - automatic translation of the addresses the logical aliases to >> the actual physical addresses of the slaves (with the help of the >> deserializer address translation feature in this case). > > A notable difference in the hardware is that a mux needs an explicit > procedure to select a port. That's why the select() op is mandatory for > muxes. The ATR, at least in the DS90UB9xx case, selects the port > automatically based on the slave address. So I added an optional > select() op in the atr, but I suspect it's useless for "real" ATRs. > > More differences derive from how Linux implements muxes. The i2c-mux > code has several flags for different locking schemes, and it has a It's two locking schemes if you count them carefully, so several is a bit of an exaggeration. But agreed, two is more than I prefer. > fairly complex DT parsing scheme. These are mostly a historical burden. > Adding the ATR features to i2c-mux.c was very tricky and error-prone due > to all of this code, that's why I have moved ATR to its own file in RFCv2. Anyway, the locking in i2c-mux may be complex, but it does solve real problems. The way I read this series, these problems are not dealt with and therefore the ATR code will not function in all surroundings. Some things to think about: - What happens when you put a mux behind an ATR? - What happens when you put an ATR behind a mux? - What happens when you put an ATR between two muxes? - Does it make a difference if the mux is parent-locked or mux-locked? - What happens if client drivers lock the adapter in order to silence the bus for some reason or to keep two xfers together or something, and then do unlocked xfers? - Can you put an ATR behind another ATR? etc I'm not saying that these things must be handled, and maybe they are handled already, and maybe some of the combinations are not valid at all. But the possibilities and limitations should be understood. And preferably documented. My gut feeling (without spending much time on it) is that ATR as implemented in this series behave approximately like mux-locked muxes, meaning that it is not obviously safe to put a parent-locked mux behind an ATR and making it impossible for client devices behind an ATR to force xfers to happen back-to-back from the root adapter. And finally, I feel that it is not wise to introduce a third locking scheme in the I2C topology. It's bad enough with two. Why not make the ATR locking exactly match the mux-locked scheme to reduce the number of cases one needs to consider? But maybe that's just me? Cheers, Peter
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
> > One huge drawback for me is the attach/detach callbacks. One year ago, I
> > removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
> > deprecated attach_adapter callback") because some drivers did a lot of
> > crazy things there. It took years to remove all that.
>
> Oh dear, I was completely unaware, apologies! :-)
Oh, no need to apologize. You don't have to research the whole I2C history
before implementing something. Keeping the big picture is what I happily
provide.
> > What I could imagine here: the adapter (B and C each in the picture
> > above) gets a flag like NEEDS_ATR before registering to the core. The
> > flag means all clients on that bus will have their address translated.
> > The core will figure out a free alias when a device is registered. We
> > can then have an ATR specific callback with the original and translated
> > address as arguments, so one can setup the HW as needed.
>
> Do you mean moving the alias selection code from i2c-atr.c to the i2c
> core? And the rest of the ATR core too?
I hope for something like this in the I2C core (simplified, naming needs
to be improved etc.) in i2c_new_device:
if (client->adapter->flag & NEEDS_ATR) {
i2c_atr_get_alias_address();
/* probably a wrapper around a callback */
i2c_atr_setup_hw();
}
with all the i2c_atr_* functions in a seperate file. It would be great
if that file could be a completely independent module, but if it turns
out that we need some simple helpers in the core, I am probably OK with
that, too.
> > Do you think that would work?
>
> Yes.
Cool!
signature.asc
Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Wolfram,
On 02/09/19 22:42, Wolfram Sang wrote:
> Hi Luca,
>
>> + * Topology:
>> + *
>> + * Slave X @ 0x10
>> + * .-. |
>> + * .-. | |---+ B
>> + * | CPU |--A--| ATR |
>> + * `-' | |---+ C
>> + * `-' |
>> + * Slave Y @ 0x10
>> + *
>> + * Alias table:
>> + *
>> + * Client Alias
>> + * -
>> + * X0x20
>> + * Y0x30
>
> Great that you already provided docs for this driver!
>
> One huge drawback for me is the attach/detach callbacks. One year ago, I
> removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
> deprecated attach_adapter callback") because some drivers did a lot of
> crazy things there. It took years to remove all that.
Oh dear, I was completely unaware, apologies! :-)
> What I could imagine here: the adapter (B and C each in the picture
> above) gets a flag like NEEDS_ATR before registering to the core. The
> flag means all clients on that bus will have their address translated.
> The core will figure out a free alias when a device is registered. We
> can then have an ATR specific callback with the original and translated
> address as arguments, so one can setup the HW as needed.
Do you mean moving the alias selection code from i2c-atr.c to the i2c
core? And the rest of the ATR core too?
> Do you think that would work?
Yes.
--
Luca
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
> Adding the ATR features to i2c-mux.c was very tricky and error-prone due > to all of this code, that's why I have moved ATR to its own file in RFCv2. I forgot to say that I like this. signature.asc Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Jacopo, thanks for your feedback. On 01/09/19 16:31, jacopo mondi wrote: > Hi Luca, >thanks for keep pushing this series! I hope we can use part of this > for the (long time) on-going GMSL work... > > I hope you will be patient enough to provide (another :) overview > of this work during the BoF Wolfram has organized at LPC for the next > week. Sure! > In the meantime I would have some comments after having a read at the > series and trying to apply its concept to GMSL > > On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. >> >> Signed-off-by: Luca Ceresoli >> >> --- >> >> Changes RFCv1 -> RFCv2: >> >> RFCv1 was implemented inside i2c-mux.c and added yet more complexity >> there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR >> features are not in a MUX and vice versa, the overlapping is low. This was >> almost a complete rewrite, but for the records here are the main >> differences from the old implementation: > > I'm not an i2c expert, but this looks very similar to me to an > augmented i2c-mux with the following additional features: > - automatic selection of the i2c address to use for the children > behind the mux > - automatic translation of the addresses the logical aliases to > the actual physical addresses of the slaves (with the help of the > deserializer address translation feature in this case). A notable difference in the hardware is that a mux needs an explicit procedure to select a port. That's why the select() op is mandatory for muxes. The ATR, at least in the DS90UB9xx case, selects the port automatically based on the slave address. So I added an optional select() op in the atr, but I suspect it's useless for "real" ATRs. More differences derive from how Linux implements muxes. The i2c-mux code has several flags for different locking schemes, and it has a fairly complex DT parsing scheme. These are mostly a historical burden. Adding the ATR features to i2c-mux.c was very tricky and error-prone due to all of this code, that's why I have moved ATR to its own file in RFCv2. > In the GMSL perspective we have similar needs but limited to the > selection of the i2c addresses to assign to the children behind our > mux. The maxims's chips work by reprogramming the remote devices and > do not support address translations on the deserializer side, unlike > what the TI chips do. Indeed, the Maxim chips implement a simple mux, so it seems like the only commonality with TI is the need for address pool management. > So I wonder if we could somehow split the here proposed solution in > two, one part to perform the address selection, the other one to > support address reprogramming. > > One thing I have noticed is that it's up to the driver using the ATR > (the DS90UB954 deserializer in this case) picking into the alias pool > and assign aliases. I would have expected this to be a feature of the > ATR itself, if you aim to have the i2c-alias-pool as a standard > construct. > > I understand the pool of free aliases 'belongs' to the base board > device, but the ATR could be provided with a pointer to it and the > routines to select and assign addresses generalized. Is there a reason > why you decided otherwise that I'm not seeing? I was about to do it, but changed my mind for a non-strongly-technical reason. The entire body of ds90_atr_attach_client() and ds90_atr_detach_client() is guarded by a mutex to protect the alias pool from concurrent access. I didn't feel comfortable in hoisting the mutex into the ATR code and have all the attach and detach callbacks run with a lock that might be unneeded in specific drivers. At least not with only one driver using i2c-atr. Also, I'm not even sure a mutex is really needed to protect the pool. Is it possible that two client adding/removal operations happen concurrently? SHould they be serialized at the i2c core level, the mutexes would be unneeded. But I haven't dug in the core enough to discover it. Wolfram? However, even though the alias picking code is not that much, I'm OK in moving it into the ATR core if it looks good. Now, I see you thinking "why not moving the ATR code _and_ the alias picking code in i2c-mux and benefit in the GMSL drivers?", and I'm a little uncomfortable about this idea. Not because the idea is bad per se, but
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Luca,
> + * Topology:
> + *
> + * Slave X @ 0x10
> + * .-. |
> + * .-. | |---+ B
> + * | CPU |--A--| ATR |
> + * `-' | |---+ C
> + * `-' |
> + * Slave Y @ 0x10
> + *
> + * Alias table:
> + *
> + * Client Alias
> + * -
> + * X0x20
> + * Y0x30
Great that you already provided docs for this driver!
One huge drawback for me is the attach/detach callbacks. One year ago, I
removed a similar callback from the I2C core ("[PATCH 0/2] i2c: remove
deprecated attach_adapter callback") because some drivers did a lot of
crazy things there. It took years to remove all that.
What I could imagine here: the adapter (B and C each in the picture
above) gets a flag like NEEDS_ATR before registering to the core. The
flag means all clients on that bus will have their address translated.
The core will figure out a free alias when a device is registered. We
can then have an ATR specific callback with the original and translated
address as arguments, so one can setup the HW as needed.
Do you think that would work? Jacopo, does that meet GMSL as well?
Regards,
Wolfram
signature.asc
Description: PGP signature
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi Luca, thanks for keep pushing this series! I hope we can use part of this for the (long time) on-going GMSL work... I hope you will be patient enough to provide (another :) overview of this work during the BoF Wolfram has organized at LPC for the next week. In the meantime I would have some comments after having a read at the series and trying to apply its concept to GMSL On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. > > Signed-off-by: Luca Ceresoli > > --- > > Changes RFCv1 -> RFCv2: > > RFCv1 was implemented inside i2c-mux.c and added yet more complexity > there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR > features are not in a MUX and vice versa, the overlapping is low. This was > almost a complete rewrite, but for the records here are the main > differences from the old implementation: I'm not an i2c expert, but this looks very similar to me to an augmented i2c-mux with the following additional features: - automatic selection of the i2c address to use for the children behind the mux - automatic translation of the addresses the logical aliases to the actual physical addresses of the slaves (with the help of the deserializer address translation feature in this case). In the GMSL perspective we have similar needs but limited to the selection of the i2c addresses to assign to the children behind our mux. The maxims's chips work by reprogramming the remote devices and do not support address translations on the deserializer side, unlike what the TI chips do. So I wonder if we could somehow split the here proposed solution in two, one part to perform the address selection, the other one to support address reprogramming. One thing I have noticed is that it's up to the driver using the ATR (the DS90UB954 deserializer in this case) picking into the alias pool and assign aliases. I would have expected this to be a feature of the ATR itself, if you aim to have the i2c-alias-pool as a standard construct. I understand the pool of free aliases 'belongs' to the base board device, but the ATR could be provided with a pointer to it and the routines to select and assign addresses generalized. Is there a reason why you decided otherwise that I'm not seeing? > > - change bus description > - remove I2C_ATR_ARBITRATOR and I2C_ATR_GATE support > - select() optional > - rename i2c_atr_alloc -> i2c_atr_new, add i2c_atr_delete, move to bottom > - lock the ATR, not the adapter or the muxes on the adapter > - remove parent-locked code > - remove I2C_MUX_LOCKED flag, now unused > - remove I2C_ATR_ATR flag (always true) > - translation in i2c_atr_smbus_xfer too > - i2c_atr_map_msgs: don't ignore mapping errors > - always require the "i2c-atr" DT node, no magic > - remove ACPI support > - one algo in the atrc, not one per adapter > - remove unneeded i2c_atr_root_adapter > - ditch force_nr > - don't allocate private user memory, just provide a plain userdata pointer > - consolidate all ops in a single struct, simplify naming > - remove adapters individually, allocate in atrc->adapter[chan_id] > --- > drivers/i2c/Kconfig | 9 + > drivers/i2c/Makefile| 1 + > drivers/i2c/i2c-atr.c | 557 > include/linux/i2c-atr.h | 82 ++ > 4 files changed, 649 insertions(+) > create mode 100644 drivers/i2c/i2c-atr.c > create mode 100644 include/linux/i2c-atr.h > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig > index abedd55a1264..5df088b1d9de 100644 > --- a/drivers/i2c/Kconfig > +++ b/drivers/i2c/Kconfig > @@ -71,6 +71,15 @@ config I2C_MUX > > source "drivers/i2c/muxes/Kconfig" > > +config I2C_ATR > + tristate "I2C Address Translator (ATR) support" > + help > + Enable support for I2C Address Translator (ATR) chips. > + > + An ATR allows accessing multiple I2C busses from a single > + physical bus via address translation instead of bus selection as > + i2c-muxes do. > + > config I2C_HELPER_AUTO > bool "Autoselect pertinent helper modules" > default y > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > index bed6ba63c983..81849ea393c7 100644 > --- a/drivers/i2c/Makefile > +++ b/drivers/i2c/Makefile > @@ -13,6 +13,7 @@ i2c-core-$(CONFIG_OF) += i2c-core-of.o >
